From owner-freebsd-hackers@FreeBSD.ORG Wed Mar 6 15:38:08 2013 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 1F4E6438; Wed, 6 Mar 2013 15:38:08 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigknife-pt.tunnel.tserv9.chi1.ipv6.he.net [IPv6:2001:470:1f10:75::2]) by mx1.freebsd.org (Postfix) with ESMTP id C26A31C4; Wed, 6 Mar 2013 15:38:07 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 2A5D1B917; Wed, 6 Mar 2013 10:38:07 -0500 (EST) From: John Baldwin To: Ian Lepore Subject: Re: rtprio_thread trouble Date: Wed, 6 Mar 2013 09:17:13 -0500 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p25; KDE/4.5.5; amd64; ; ) References: <1361559960.1185.81.camel@revolution.hippie.lan> <201302261529.24862.jhb@freebsd.org> <1362081556.1195.62.camel@revolution.hippie.lan> In-Reply-To: <1362081556.1195.62.camel@revolution.hippie.lan> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201303060917.13642.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Wed, 06 Mar 2013 10:38:07 -0500 (EST) Cc: freebsd-hackers@freebsd.org X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 Mar 2013 15:38:08 -0000 On Thursday, February 28, 2013 2:59:16 pm Ian Lepore wrote: > On Tue, 2013-02-26 at 15:29 -0500, John Baldwin wrote: > > On Friday, February 22, 2013 2:06:00 pm Ian Lepore wrote: > > > I ran into some trouble with rtprio_thread() today. I have a worker > > > thread that I want to run at idle priority most of the time, but if it > > > falls too far behind I'd like to bump it back up to regular timeshare > > > priority until it catches up. In a worst case, the system is > > > continuously busy and something scheduled at idle priority is never > > > going to run (for some definition of 'never'). > > > > > > What I found is that in this worst case, even after my main thread has > > > used rtprio_thread() to change the worker thread back to > > > RTP_PRIO_NORMAL, the worker thread never gets scheduled. This is with > > > the 4BSD scheduler but it appears that the same would be the case with > > > ULE, based on code inspection. I find that this fixes it for 4BSD, and > > > I think the same would be true for ULE... > > > > > > --- a/sys/kern/sched_4bsd.c Wed Feb 13 12:54:36 2013 -0700 > > > +++ b/sys/kern/sched_4bsd.c Fri Feb 22 11:55:35 2013 -0700 > > > @@ -881,6 +881,9 @@ sched_user_prio(struct thread *td, u_cha > > > return; > > > oldprio = td->td_user_pri; > > > td->td_user_pri = prio; > > > + if (td->td_flags & TDF_BORROWING && td->td_priority <= prio) > > > + return; > > > + sched_priority(td, prio); > > > } > > > > > > void > > > > > > But I'm not sure if this would have any negative side effects, > > > especially since in the ULE case there's a comment on this function that > > > specifically notes that it changes the the user priority without > > > changing the current priority (but it doesn't say why that matters). > > > > > > Is this a reasonable way to fix this problem, or is there a better way? > > > > This will lose the "priority boost" afforded to interactive threads when they > > sleep in the kernel in the 4BSD scheduler. You aren't supposed to drop the > > user priority to loose this boost until userret(). You could perhaps try > > only altering the priority if the new user pri is lower than your current > > priority (and then you don't have to check TDF_BORROWING I believe): > > > > if (prio < td->td_priority) > > sched_priority(td, prio); > > > > That's just the sort of insight I was looking for, thanks. That made me > look at the code more and think harder about the problem I'm trying to > solve, and I concluded that doing it within the scheduler is all wrong. > > That led me to look elsewhere, and I discovered the change you made in > r228207, which does almost what I want, but your change does it only for > realtime priorities, and I need a similar effect for idle priorities. > What I came up with is a bit different than yours (attached below) and > I'd like your thoughts on it. > > I start with the same test as yours: if sched_user_prio() didn't > actually change the user priority (due to borrowing), do nothing. Then > mine differs: call sched_prio() to effect the change only if either the > old or the new priority class is not timeshare. > > My reasoning for the second half of the test is that if it's a change in > timeshare priority then the scheduler is going to adjust that priority > in a way that completely wipes out the requested change anyway, so > what's the point? (If that's not true, then allowing a thread to change > its own timeshare priority would subvert the scheduler's adjustments and > let a cpu-bound thread monopolize the cpu; if allowed at all, that > should require priveleges.) > > On the other hand, if either the old or new priority class is not > timeshare, then the scheduler doesn't make automatic adjustments, so we > should honor the request and make the priority change right away. The > reason the old class gets caught up in this is the very reason I'm > wanting to make a change: when thread A changes the priority of its > child thread B from idle back to timeshare, thread B never actually gets > moved to a timeshare-range run queue unless there are some idle cycles > available to allow it to first get scheduled again as an idle thread. > > Finally, my change doesn't consider the td == curthread situation at > all, because I don't see how that's germane. This is the thing I'm > least sure of -- I don't at all understand why the old code (even before > your changes) had that test. The old code had that flagged as "XXX > dubious" (a comment a bit too cryptic to be useful). I think your change is correct. One style nit: please sort the order of variables (oldclass comes before oldpri). -- John Baldwin