kern_yield vs ukbd_yield

[mdf at FreeBSD.org]

On Mon, Dec 12, 2011 at 11:05 AM, John Baldwin <jhb at 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 at freebsd.org wrote:
>> > 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.
>
> The scheduler generally should not do this with the following exceptions:
>
>  1) for timesharing threads, priorities are in bands, so effectively the
>    pri / 4 is what is used for comparison and if two threads have the same
>    pri / 4 the scheduler will round-robin among htem.
>
>  2) ULE might be a bit different because of the way it assigns threads to
>    CPUs, so if a CPU has two high priority threads and another CPU only
>    has a low priority thread, the second CPU will not run the second high
>    priority thread.  4BSD 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).  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.
>
> Realtime priorities (for rtprio user threads) are higher than the kernel
> "sleep" priorities.  Also, keep in mind that a thread does not get an
> automatic priority boost when it enters the kernel.  It only gets a boost
> 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 priority
>> 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