kern_yield vs ukbd_yield

[Hans Petter Selasky]

On Thursday 15 December 2011 15:17:01 Andriy Gapon wrote:
> on 14/12/2011 23:56 Hans Petter Selasky said the following:
> > On Wednesday 14 December 2011 16:37:50 Andriy Gapon wrote:
> >> So, Hans Petter, do you recall any details of this problem?
> >> I am curious about which thread got starved by which.
> > 
> > From what I know this was 100% reproducible.
> > 
> > Remove the ukbd_yield() when at the mountroot prompt. Result: cannot type
> > any keys. No USB devices will enumerate!
> 
> This hasn't satisfied my curiosity :)
> 
> >> BTW, given your recent improvements to pause(9) what do you think about
> >> further extending it to also use DELAY(9) when kdb_active is set or when
> >> SCHEDULER_STOPPED() is true?
> > 
> > I think this is a good idea. It already checks for "cold". USB usually
> > doesn't use this function though when polling.
> > 
> >> Then, probably, pause(9) could be used for
> >> both branches in ukbd_do_poll and they could be collapsed together.
> 
> Hmm... I looked at the history of ukbd.c (which I should have done from the
> very start) and I see two relevant revisions:
> http://svnweb.freebsd.org/base/head/sys/dev/usb/input/ukbd.c?r1=199816&r2=2
> 03896&pathrev=203896
> http://svnweb.freebsd.org/base/head/sys/dev/usb/input/ukbd.c?r1=223755&r2=
> 223989&pathrev=223989
> 
> So, first use of pause(9) was introduced to work around current broken
> syscons polling mechanism.  Then pause(9) was replaced with the
> hand-rolled yield to fix a problem at shutdown, which supposedly was
> caused by times being stopped.
> 
> None of the commits seems to be directly related to thread priorities. 

Not directly, but indirect. You know, if you pause thread 1 (which I thought 
was thread 0), then other thread will get a chance to run.

It might also be that Giant is locked by syscons, and that unless you pause or 
yield, then Giant will block other parts of USB, like the callback thread, 
which is Giant locked for ukbd only to run.

Maybe that is the explanation?

> I
> suspect that even DELAY(9) could have worked in the place of the
> pause/yield. Also, I do not see any mentioning of the mountroot context in
> the commits.  Not sure why I even assumed that it was relevant.
> 
> BTW, maybe we should have a 'cold again' flag for late stages of system
> shutdown?
> 
> So, all in all, I believe in the following two things:
> 
> 1. kern_yield can be used instead of ukbd_yield with any argument
> 
> 1.1. DELAY() can be used instead of ukbd_yield too

No, it doesn't drop Giant, see above.

> 
> 2. all that special code should not be needed once I commit the patches
> that I have recently posted to arch@ (I do intend to commit them).  In
> that case the keyboard drivers would be properly put into the polling mode
> instead of the current situation where the polling mode is repeatedly
> entered and exited when kernel needs some input.

Just make sure all the use-cases work. Then you can safely commit:

1) mountroot prompt
2) dump at shutdown
3) kernel debugger (enter and exit) keyboard should work before and after.

> 
> Maybe you recall/know more than you wrote above and I have missed something
> obvious that you can point out?

--HPS