From owner-freebsd-hackers@FreeBSD.ORG Mon Dec 12 15:55:39 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 04603106568B; Mon, 12 Dec 2011 15:55:39 +0000 (UTC) (envelope-from mdf356@gmail.com) Received: from mail-pw0-f54.google.com (mail-pw0-f54.google.com [209.85.160.54]) by mx1.freebsd.org (Postfix) with ESMTP id C366A8FC16; Mon, 12 Dec 2011 15:55:38 +0000 (UTC) Received: by pbcc3 with SMTP id c3so1421804pbc.13 for ; Mon, 12 Dec 2011 07:55:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=LVNI0dsH6t38MaDE8Bum3cfnK4RkvPdQP/TfyWG2fck=; b=cqiW3/lsb1ZNfh9tSbIDFSEiWFl3F3kCvKYEiZVs+doPAbpYqJWTSqLuks1t5T8Vzc LVznDMw/KcbZD388McItVbuqE6T3YkfH184te+8F9lOL4AC2uzjzlZ48pZZC6cTJFia9 Xo1TCLPaK2Ed3/iiuVR9Ugn98MsK3cG3uTL9I= MIME-Version: 1.0 Received: by 10.68.72.230 with SMTP id g6mr23147601pbv.119.1323705338115; Mon, 12 Dec 2011 07:55:38 -0800 (PST) Sender: mdf356@gmail.com Received: by 10.68.197.198 with HTTP; Mon, 12 Dec 2011 07:55:38 -0800 (PST) In-Reply-To: <4EE5B56A.4000106@FreeBSD.org> References: <4EE51CB5.1060505@FreeBSD.org> <4EE5B56A.4000106@FreeBSD.org> Date: Mon, 12 Dec 2011 07:55:38 -0800 X-Google-Sender-Auth: JFKoPj31iT2LTiDvbOUmiyPW2Rg Message-ID: From: mdf@FreeBSD.org To: Andriy Gapon Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: usb@freebsd.org, hackers@freebsd.org 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 15:55:39 -0000 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: =A0 Thu Sep 1 16:50:13 2011 +0300 >>> >>> =A0 =A0ukbd: 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) >>> =A0} >>> >>> =A0static void >>> -ukbd_yield(void) >>> -{ >>> - =A0 =A0 =A0 struct thread *td =3D curthread; >>> - =A0 =A0 =A0 uint32_t old_prio; >>> - >>> - =A0 =A0 =A0 DROP_GIANT(); >>> - >>> - =A0 =A0 =A0 thread_lock(td); >>> - >>> - =A0 =A0 =A0 /* get current priority */ >>> - =A0 =A0 =A0 old_prio =3D td->td_base_pri; >>> - >>> - =A0 =A0 =A0 /* set new priority */ >>> - =A0 =A0 =A0 sched_prio(td, td->td_user_pri); >>> - >>> - =A0 =A0 =A0 /* cause a task switch */ >>> - =A0 =A0 =A0 mi_switch(SW_INVOL | SWT_RELINQUISH, NULL); >>> - >>> - =A0 =A0 =A0 /* restore priority */ >>> - =A0 =A0 =A0 sched_prio(td, old_prio); >>> - >>> - =A0 =A0 =A0 thread_unlock(td); >>> - >>> - =A0 =A0 =A0 PICKUP_GIANT(); >>> -} >>> - >>> -static void >>> =A0ukbd_do_poll(struct ukbd_softc *sc, uint8_t wait) >>> =A0{ >>> >>> @@ -439,7 +412,7 @@ ukbd_do_poll(struct ukbd_softc *sc, uint8_t wait) >>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0while (sc->sc_inputs =3D=3D 0) { >>> >>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* give USB threads a ch= ance to run */ >>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ukbd_yield(); >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 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. =A0So 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. hselasky@ or someone else familiar with the various usb threads would have to answer that.