kern_yield vs ukbd_yield

[John Baldwin]

On Monday, December 12, 2011 2:13:49 pm mdf at freebsd.org wrote:
> 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.

This is how the BSD scheduler traditionally boosted the priority of
"interactive" threads to favor them over CPU-bound threads.  That is, BSD
assumed that interactive threads are going to be blocking in the kernel
(on a socket, on a tty, etc.), then running for a little bit after waking
before blocking again.  The boost when you sleep is to let threads with
that work pattern always have a higher priority than CPU-bound threads
(which will always have priorities in the timeshare range).  You have to
keep the boost long enough to ensure you get some user time however, so
the threads keep the raised priority out to the userland boundary where
it is dropped in userret(), but it is intentionally done in a careful
manner where the thread drops its priority but does not do a reschedule
so it will still run out in userland with the "effective" priority from
its boost until the next schedule event happens (timeslice expires, thread
blocks, etc.).  Preemption from ithreads makes scheduling events happen
far more often though (prior to SMPng, interrupts didn't trigger a
reschedule), so interactive threads perhaps don't get favored quite as
much since 5.

-- 
John Baldwin