kern_yield vs ukbd_yield

[Hans Petter Selasky]

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.
> 
> 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.
> 

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!

--HPS