Date: Mon, 12 Dec 2011 11:13:49 -0800 From: mdf@FreeBSD.org To: John Baldwin <jhb@freebsd.org> Cc: usb@freebsd.org, freebsd-hackers@freebsd.org, hackers@freebsd.org, Andriy Gapon <avg@freebsd.org>, Hans Petter Selasky <hselasky@c2i.net> Subject: Re: kern_yield vs ukbd_yield Message-ID: <CAMBSHm-AsLFXYWtdDavsEruZUp_R2sVSzm3_Bo3CowWZLJwGWA@mail.gmail.com> In-Reply-To: <201112121405.38322.jhb@freebsd.org> References: <4EE51CB5.1060505@FreeBSD.org> <CAMBSHm_ChT5X1bhE3oLNiRJPf4LZ5OmmxthjTXxbE2kJNxOyTw@mail.gmail.com> <201112121658.22864.hselasky@c2i.net> <201112121405.38322.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Dec 12, 2011 at 11:05 AM, John Baldwin <jhb@freebsd.org> wrote:
> On Monday, December 12, 2011 10:58:22 am Hans Petter Selasky wrote:
>> On Monday 12 December 2011 16:55:38 mdf@freebsd.org wrote:
>> > 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> wro=
te:
>> > >>> 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 inste=
ad
>> > >>>
>> > >>> 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 wa=
it)
>> > >>> =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 chance 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 w=
hat
>> > >> 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. =A0There 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.
>
> The scheduler generally should not do this with the following exceptions:
>
> =A01) for timesharing threads, priorities are in bands, so effectively th=
e
> =A0 =A0pri / 4 is what is used for comparison and if two threads have the=
same
> =A0 =A0pri / 4 the scheduler will round-robin among htem.
>
> =A02) ULE might be a bit different because of the way it assigns threads =
to
> =A0 =A0CPUs, so if a CPU has two high priority threads and another CPU on=
ly
> =A0 =A0has a low priority thread, the second CPU will not run the second =
high
> =A0 =A0priority thread. =A04BSD handles this case more correctly.
>
>> > 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). =A0So 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.
>
> Realtime priorities (for rtprio user threads) are higher than the kernel
> "sleep" priorities. =A0Also, keep in mind that a thread does not get an
> automatic priority boost when it enters the kernel. =A0It only gets a boo=
st
> either temporarily from priority propagation, or a slightly longer (but
> still temporary) boost from a sleep(9) call.
I still don't understand why threads are boosting their priority while
sleeping; if it's so they are woken preferentially, that makes some
sense, but then they shouldn't be keeping the boosted priority after
the thread is woken. If a thread really needs higher priority to do
its work, that should be an explicit call to sched_prio(9), rather
than implicitly only when/if it first sleeps.
Thanks,
matthew
>> > hselasky@ or someone else familiar with the various usb threads would
>> > have to answer that.
>>
>> The problem is only during init() where the init thread has highest prio=
rity
>> and that doesn't allow other threads to run even if the scheduler is
> running!
>
> Hmm, that should be fixed by lowering the relevant thread's priority.
> Do you mean thread0 (the one doing all the SYSINIT's or thread we create =
for
> init (pid 1) before it executes init?
>
> --
> John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAMBSHm-AsLFXYWtdDavsEruZUp_R2sVSzm3_Bo3CowWZLJwGWA>