rtprio_thread trouble

[John Baldwin]

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