sched_4bsd: fix vague comment

[mchoo]

Summary

The comment "was incremented in schedcpu()" doesn't give enough
background for decrementing ts_slptime by 1 (thus ignoring decay_cpu()
for 1 ts_slptime). More accurately, ts_slptime is decremented by 1
because decay_cpu() has already executed once in schedcpu() when
ts_slptime was 1.

MFC after: 2 weeks
MFC to: stable/14, stable/15
Sponsored by: FreeBSD Foundation

[olce]

Well, I don't find the new comment less vague. By contrast, the description you're giving in the current commit message is immediately intelligible.

But it is not completely true either: There is another caller of updatepri(): sched_wakeup(), which does not call decay_cpu() in advance. There is thus a probable bug with calling decay_cpu() only ts_slptime - 1 in all cases. I think updatepri() should just run the loop exactly ts_slptime always, and callers that need to adjust ts_slptime should do so before the call. But in fact no adjustment should be needed by rewriting the code in a clearer way.

For this revision, I'm OK with just updating the comment with a better stated version of what the author had in mind here. But then, what the comment means is in fact wrong, and should later be fixed along with the code.

[mchoo]

In D59406#1364025, @olce wrote:

Well, I don't find the new comment less vague. By contrast, the description you're giving in the current commit message is immediately intelligible.

But it is not completely true either: There is another caller of updatepri(): sched_wakeup(), which does not call decay_cpu() in advance. There is thus a probable bug with calling decay_cpu() only ts_slptime - 1 in all cases. I think updatepri() should just run the loop exactly ts_slptime always, and callers that need to adjust ts_slptime should do so before the call. But in fact no adjustment should be needed by rewriting the code in a clearer way.

For this revision, I'm OK with just updating the comment with a better stated version of what the author had in mind here. But then, what the comment means is in fact wrong, and should later be fixed along with the code.

In schedcpu():

if (ts->ts_slptime > 1) {
	thread_unlock(td);
	continue;
}
ts->ts_estcpu = decay_cpu(loadfac, ts->ts_estcpu);

In updatepri():

newcpu = ts->ts_estcpu;
ts->ts_slptime--;	/* was incremented in schedcpu() */
while (newcpu && --ts->ts_slptime)
	newcpu = decay_cpu(loadfac, newcpu);
ts->ts_estcpu = newcpu;

In sched_4bsd_wakeup():

if (ts->ts_slptime > 1) {
	updatepri(td);
	resetpriority(td);
}

schedcpu()'s call to decay_cpu() happens regardless of awake, that is, even sleeping threads are decayed for the first tick of ts_slptime. Then sched_4bsd_wakeup() calls updatepri() which decreases ts_slptime N-1 times.

As written in comment in schedcpu():

if (awake) {
	if (ts->ts_slptime > 1) {
		/*
		 * In an ideal world, this should not
		 * happen, because whoever woke us
		 * up from the long sleep should have
		 * unwound the slptime and reset our
		 * priority before we run at the stale
		 * priority.  Should KASSERT at some
		 * point when all the cases are fixed.
		 */
		updatepri(td);
	}
	ts->ts_slptime = 0;
} else
	ts->ts_slptime++;

updatepri() should only be called from sched_4bsd_wakeup(). I have a patch that fixes all the corner cases so schedcpu() don't need to call updatepri() and have KASSERT() instead, but I need to benchmark it before creating revision.

[olce]

In D59406#1365004, @mchoo wrote:

schedcpu()'s call to decay_cpu() happens regardless of awake, that is, even sleeping threads are decayed for the first tick of ts_slptime.

Yes, as it should. schedcpu()'s duty is to perform the CPU decay periodically, for all threads (including sleeping ones).

Then sched_4bsd_wakeup() calls updatepri() which decreases ts_slptime N-1 times.

Yes (actually, ts_slptime-2), and that's the problem: This treatment shouldn't apply to caller sched_wakeup(). Doing this compensation in updatepri() is confusing (as a proof, it causes this small bug) and generally bad design (and there's another bad thing: updatepri() does not reset ts_slptime in all cases).

If doing it in small steps, the first one would be to remove this quirk from updatepri() and adapt schedcpu() accordingly, and then, in the second one, the call from schedcpu() can be removed entirely. But you can do it in one go if you prefer, it's not that big a change.

---

Going back to this revision, the comment is still too vague, and arguably wrong (as you've noticed, schedcpu() calls decay_cpu() regardless of ts_slptime). See suggestion.

This makes it even more clear that the adjustment in updatepri(), even if we forget the bugs, is kind of silly (arbitrary choice, no real usefulness). Once schedcpu() does not call updatepri() anymore, such an adjustment will become impossible anyway.

sys/kern/sched_4bsd.c

588–589

[mchoo]

Reflect @olce's suggestion