kern_yield vs ukbd_yield

[John Baldwin]

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.

> Hi,
> 
> > 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