From owner-freebsd-hackers@FreeBSD.ORG Mon Dec 12 19:05:40 2011 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 0FEF41065672; Mon, 12 Dec 2011 19:05:40 +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 C88AF8FC14; Mon, 12 Dec 2011 19:05:39 +0000 (UTC) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [96.47.65.170]) by cyrus.watson.org (Postfix) with ESMTPSA id 75B2346B23; Mon, 12 Dec 2011 14:05:39 -0500 (EST) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 04A38B93A; Mon, 12 Dec 2011 14:05:39 -0500 (EST) From: John Baldwin To: freebsd-hackers@freebsd.org Date: Mon, 12 Dec 2011 14:05:38 -0500 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p8; KDE/4.5.5; amd64; ; ) References: <4EE51CB5.1060505@FreeBSD.org> <201112121658.22864.hselasky@c2i.net> In-Reply-To: <201112121658.22864.hselasky@c2i.net> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201112121405.38322.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Mon, 12 Dec 2011 14:05:39 -0500 (EST) Cc: usb@freebsd.org, mdf@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 19:05:40 -0000 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. > 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