cvs commit: src/sys/dev/syscons/apm apm_saver.c src/sys/i386/bios apm.c apm.h

[John Baldwin]

On Wednesday 31 May 2006 10:58, Scott Long wrote:
> John Baldwin wrote:
> > On Thursday 25 May 2006 23:01, Scott Long wrote:
> > 
> >>Warner Losh wrote:
> >>
> >>
> >>>imp         2006-05-25 23:06:38 UTC
> >>>
> >>>  FreeBSD src repository
> >>>
> >>>  Modified files:
> >>>    sys/dev/syscons/apm  apm_saver.c 
> >>>    sys/i386/bios        apm.c apm.h 
> >>>  Log:
> >>>  APM was calling the suspend process from a timeout.  This meant that
> >>>  other timeouts could not happen while suspending, including timeouts
> >>>  for things like msleep.  This caused the system to hang on suspend
> >>>  when the cbb was enabled, since its suspend path powered down the
> >>>  socket which used a timeout to wait for it to be done.
> >>>  
> >>>  APM now creates a thread when it is enabled, and deletes the thread
> >>>  when it is disabled.  This thread takes the place of the timeout by
> >>>  doing its polling every ~.9s.  When the thread is disabled, it will
> >>>  wakeup early, otherwise it times out and polls the varius things the
> >>>  old timeout polled (APM events, suspend delays, etc).
> >>>  
> >>>  This makes my Sony VAIO 505TS suspend/resume correctly when APM is
> >>>  enabled (ACPI is black listed on my 505TS).
> >>>  
> >>>  This will likely fix other problems with the suspend path where
> >>>  drivers would sleep with msleep and/or do other timeouts.  Maybe
> >>>  there's some special case code that would use DELAY while suspending
> >>>  and msleep otherwise that can be revisited and removed.
> >>>  
> >>>  This was also tested by glebius@, who pointed out that in the patch I
> >>>  sent him, I'd forgotten apm_saver.c
> >>>  
> >>>  MFC After: 3 weeks
> >>
> >>In the past, I've been against mandating that callouts/timeouts/generic 
> >>taskqueues should not be allowed to sleep.  However, after looking over
> >>the history of this problem as well as others, it seems that it's just
> >>too easy for driver authors to make bad assumptions and wind up with a
> >>priority inversion/deadlock like this.  It would be relatively trivial
> >>to mark these contexts as being non-sleepable and have the msleep code
> >>enforce it, like is done with ithreads.  What do you think?  Anyways,
> >>thanks for looking at this and fixing it.
> > 
> > 
> > We already do for timeouts if INVARIANTS is on:
> > 
> > softclock()
> > {
> > 	...
> > 				THREAD_NO_SLEEPING();
> > 				c_func(c_arg);
> > 				THREAD_SLEEPING_OK();
> > 	...
> > }
> > 
> > That has been in place since 6.0 IIRC.
> > 
> 
> I thought that it was only enabled for DIAGNOSTIC.
> 
> Scott

Nah, that was phk's other timekeeping code to see which timeouts take a long 
time to execute.  The THREAD_NO_SLEEPING() stuff was added just before 6.0 
was released and replaced a couple of "special" mutexes that were held just 
to provoke WITNESS warnings.

-- 
John Baldwin