Date: Mon, 12 Dec 2011 07:55:38 -0800 From: mdf@FreeBSD.org To: Andriy Gapon <avg@freebsd.org> Cc: usb@freebsd.org, hackers@freebsd.org Subject: Re: kern_yield vs ukbd_yield Message-ID: <CAMBSHm_ChT5X1bhE3oLNiRJPf4LZ5OmmxthjTXxbE2kJNxOyTw@mail.gmail.com> In-Reply-To: <4EE5B56A.4000106@FreeBSD.org> References: <4EE51CB5.1060505@FreeBSD.org> <CAMBSHm-Gmyfm83wDnhKTWAM%2BM90SEdub9uNOexG7QvuGWvN3iQ@mail.gmail.com> <4EE5B56A.4000106@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Dec 12, 2011 at 12:03 AM, Andriy Gapon <avg@freebsd.org> wrote:
> on 11/12/2011 23:48 mdf@FreeBSD.org said the following:
>> On Sun, Dec 11, 2011 at 1:12 PM, Andriy Gapon <avg@freebsd.org> wrote:
>>>
>>> Does the following change do what I think that it does?
>>> Thank you!
>>>
>>> Author: Andriy Gapon <avg@icyb.net.ua>
>>> Date: =A0 Thu Sep 1 16:50:13 2011 +0300
>>>
>>> =A0 =A0ukbd: 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)
>>> =A0}
>>>
>>> =A0static void
>>> -ukbd_yield(void)
>>> -{
>>> - =A0 =A0 =A0 struct thread *td =3D curthread;
>>> - =A0 =A0 =A0 uint32_t old_prio;
>>> -
>>> - =A0 =A0 =A0 DROP_GIANT();
>>> -
>>> - =A0 =A0 =A0 thread_lock(td);
>>> -
>>> - =A0 =A0 =A0 /* get current priority */
>>> - =A0 =A0 =A0 old_prio =3D td->td_base_pri;
>>> -
>>> - =A0 =A0 =A0 /* set new priority */
>>> - =A0 =A0 =A0 sched_prio(td, td->td_user_pri);
>>> -
>>> - =A0 =A0 =A0 /* cause a task switch */
>>> - =A0 =A0 =A0 mi_switch(SW_INVOL | SWT_RELINQUISH, NULL);
>>> -
>>> - =A0 =A0 =A0 /* restore priority */
>>> - =A0 =A0 =A0 sched_prio(td, old_prio);
>>> -
>>> - =A0 =A0 =A0 thread_unlock(td);
>>> -
>>> - =A0 =A0 =A0 PICKUP_GIANT();
>>> -}
>>> -
>>> -static void
>>> =A0ukbd_do_poll(struct ukbd_softc *sc, uint8_t wait)
>>> =A0{
>>>
>>> @@ -439,7 +412,7 @@ ukbd_do_poll(struct ukbd_softc *sc, uint8_t wait)
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0while (sc->sc_inputs =3D=3D 0) {
>>>
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* give USB threads a ch=
ance to run */
>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ukbd_yield();
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 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. =A0So 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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAMBSHm_ChT5X1bhE3oLNiRJPf4LZ5OmmxthjTXxbE2kJNxOyTw>