kern_yield vs ukbd_yield

[mdf at FreeBSD.org]

On Mon, Dec 12, 2011 at 12:03 AM, Andriy Gapon <avg at freebsd.org> wrote:
> on 11/12/2011 23:48 mdf at FreeBSD.org said the following:
>> On Sun, Dec 11, 2011 at 1:12 PM, Andriy Gapon <avg at freebsd.org> wrote:
>>>
>>> Does the following change do what I think that it does?
>>> Thank you!
>>>
>>> Author: Andriy Gapon <avg at icyb.net.ua>
>>> Date:   Thu Sep 1 16:50:13 2011 +0300
>>>
>>>    ukbd: drop local duplicate of kern_yield and use that instead
>>>
>>> diff --git a/sys/dev/usb/input/ukbd.c b/sys/dev/usb/input/ukbd.c
>>> index 086c178..8078cbb 100644
>>> --- a/sys/dev/usb/input/ukbd.c
>>> +++ b/sys/dev/usb/input/ukbd.c
>>> @@ -399,33 +399,6 @@ ukbd_put_key(struct ukbd_softc *sc, uint32_t key)
>>>  }
>>>
>>>  static void
>>> -ukbd_yield(void)
>>> -{
>>> -       struct thread *td = curthread;
>>> -       uint32_t old_prio;
>>> -
>>> -       DROP_GIANT();
>>> -
>>> -       thread_lock(td);
>>> -
>>> -       /* get current priority */
>>> -       old_prio = td->td_base_pri;
>>> -
>>> -       /* set new priority */
>>> -       sched_prio(td, td->td_user_pri);
>>> -
>>> -       /* cause a task switch */
>>> -       mi_switch(SW_INVOL | SWT_RELINQUISH, NULL);
>>> -
>>> -       /* restore priority */
>>> -       sched_prio(td, old_prio);
>>> -
>>> -       thread_unlock(td);
>>> -
>>> -       PICKUP_GIANT();
>>> -}
>>> -
>>> -static void
>>>  ukbd_do_poll(struct ukbd_softc *sc, uint8_t wait)
>>>  {
>>>
>>> @@ -439,7 +412,7 @@ ukbd_do_poll(struct ukbd_softc *sc, uint8_t wait)
>>>                while (sc->sc_inputs == 0) {
>>>
>>>                        /* give USB threads a chance to run */
>>> -                       ukbd_yield();
>>> +                       kern_yield(-1);
>>
>> Not quite.
>>
>> 1) -1 should be spelled PRI_UNCHANGED, except ukbd_yield() uses
>> td_user_pri, but then puts it back again, so I think UNCHANGED is what
>> is meant.
>> 2) kern_yield() calls it a SW_VOL rather than SW_INVOL, which seems
>> the desired behaviour here anyways, since this is an explicit (i.e.
>> voluntary) yield.
>
> Thank you for the explanation.  So would you say that the patch is OK?

As far as I know, yes.  There may be a difference in behaviour,
though, while yielding, if the priority of the thread remains high (as
this change would make it) -- I'm not completely sure how the
scheduler chooses threads, because I'm pretty sure I've seen it take
threads with lower (higher numbered) priorities even when there's
runnable threads with a higher (lower numbered) priority available.

It has always seemed weird to me that the priorities in the kernel are
strictly higher than user-space -- but only after a prio change like
that done implicitly by many of the calls to sleep(9).  So it may be
that the better patch is to use PRI_USER, not PRI_UNCHANGED, which
would revert any potentially changed thread prio (e.g. due to a
sleep(9)) back to its user-space level, so that it contended as
expected with other threads.

hselasky@ or someone else familiar with the various usb threads would
have to answer that.