From owner-freebsd-hackers@FreeBSD.ORG Thu Feb 28 21:11:09 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 92137A86; Thu, 28 Feb 2013 21:11:09 +0000 (UTC) (envelope-from ian@FreeBSD.org) Received: from mho-01-ewr.mailhop.org (mho-03-ewr.mailhop.org [204.13.248.66]) by mx1.freebsd.org (Postfix) with ESMTP id 5DDCB23A; Thu, 28 Feb 2013 21:11:08 +0000 (UTC) Received: from c-24-8-232-202.hsd1.co.comcast.net ([24.8.232.202] helo=damnhippie.dyndns.org) by mho-01-ewr.mailhop.org with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.72) (envelope-from ) id 1UB9dO-0007n7-OQ; Thu, 28 Feb 2013 19:59:18 +0000 Received: from [172.22.42.240] (revolution.hippie.lan [172.22.42.240]) by damnhippie.dyndns.org (8.14.3/8.14.3) with ESMTP id r1SJxGjU085690; Thu, 28 Feb 2013 12:59:16 -0700 (MST) (envelope-from ian@FreeBSD.org) X-Mail-Handler: Dyn Standard SMTP by Dyn X-Originating-IP: 24.8.232.202 X-Report-Abuse-To: abuse@dyndns.com (see http://www.dyndns.com/services/sendlabs/outbound_abuse.html for abuse reporting information) X-MHO-User: U2FsdGVkX18T80ULGcz2T7nsdkwef8bU Subject: Re: rtprio_thread trouble From: Ian Lepore To: John Baldwin In-Reply-To: <201302261529.24862.jhb@freebsd.org> References: <1361559960.1185.81.camel@revolution.hippie.lan> <201302261529.24862.jhb@freebsd.org> Content-Type: multipart/mixed; boundary="=-fruE5NV5DEzFjKJ3O5EF" Date: Thu, 28 Feb 2013 12:59:16 -0700 Message-ID: <1362081556.1195.62.camel@revolution.hippie.lan> Mime-Version: 1.0 X-Mailer: Evolution 2.32.1 FreeBSD GNOME Team Port 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: Thu, 28 Feb 2013 21:11:09 -0000 --=-fruE5NV5DEzFjKJ3O5EF Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 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). -- Ian --=-fruE5NV5DEzFjKJ3O5EF Content-Disposition: inline; filename="rtprio_thread.diff" Content-Type: text/x-patch; name="rtprio_thread.diff"; charset="us-ascii" Content-Transfer-Encoding: 7bit Index: sys/kern/kern_resource.c =================================================================== --- sys/kern/kern_resource.c (revision 247421) +++ sys/kern/kern_resource.c (working copy) @@ -469,8 +469,7 @@ sys_rtprio(td, uap) int rtp_to_pri(struct rtprio *rtp, struct thread *td) { - u_char newpri; - u_char oldpri; + u_char newpri, oldpri, oldclass; switch (RTP_PRIO_BASE(rtp->type)) { case RTP_PRIO_REALTIME: @@ -493,11 +492,12 @@ rtp_to_pri(struct rtprio *rtp, struct thread *td) } thread_lock(td); + oldclass = td->td_pri_class; sched_class(td, rtp->type); /* XXX fix */ oldpri = td->td_user_pri; sched_user_prio(td, newpri); - if (td->td_user_pri != oldpri && (td == curthread || - td->td_priority == oldpri || td->td_user_pri <= PRI_MAX_REALTIME)) + if (td->td_user_pri != oldpri && (oldclass != RTP_PRIO_NORMAL || + td->td_pri_class != RTP_PRIO_NORMAL)) sched_prio(td, td->td_user_pri); if (TD_ON_UPILOCK(td) && oldpri != newpri) { critical_enter(); --=-fruE5NV5DEzFjKJ3O5EF--