On Mon, Dec 12, 2011 at 12:03 AM, Andriy Gapon <a...@freebsd.org> wrote:
> on 11/12/2011 23:48 m...@freebsd.org said the following:
>> On Sun, Dec 11, 2011 at 1:12 PM, Andriy Gapon <a...@freebsd.org> wrote:
>>>
>>> Does the following change do what I think that it does?
>>> Thank you!
>>>
>>> Author: Andriy Gapon <a...@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.
_______________________________________________
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
