From owner-freebsd-hackers@FreeBSD.ORG Mon Dec 12 21:18:09 2011 Return-Path: Delivered-To: hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 951531065677; Mon, 12 Dec 2011 21:18:09 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 59AC48FC1C; Mon, 12 Dec 2011 21:18:09 +0000 (UTC) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [96.47.65.170]) by cyrus.watson.org (Postfix) with ESMTPSA id 04E9A46B0D; Mon, 12 Dec 2011 16:18:09 -0500 (EST) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 795A7B948; Mon, 12 Dec 2011 16:18:08 -0500 (EST) From: John Baldwin To: mdf@freebsd.org Date: Mon, 12 Dec 2011 16:18:07 -0500 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p8; KDE/4.5.5; amd64; ; ) References: <4EE51CB5.1060505@FreeBSD.org> <201112121405.38322.jhb@freebsd.org> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201112121618.07833.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Mon, 12 Dec 2011 16:18:08 -0500 (EST) Cc: usb@freebsd.org, freebsd-hackers@freebsd.org, hackers@freebsd.org, Andriy Gapon , Hans Petter Selasky Subject: Re: kern_yield vs ukbd_yield X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 12 Dec 2011 21:18:09 -0000 On Monday, December 12, 2011 2:13:49 pm mdf@freebsd.org wrote: > On Mon, Dec 12, 2011 at 11:05 AM, John Baldwin 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 wrote: > >> > > on 11/12/2011 23:48 mdf@FreeBSD.org said the following: > >> > >> On Sun, Dec 11, 2011 at 1:12 PM, Andriy Gapon wrote: > >> > >>> Does the following change do what I think that it does? > >> > >>> Thank you! > >> > >>> > >> > >>> Author: Andriy Gapon > >> > >>> 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