Record ticks in td_swvoltick after sched_switch() returns

[vangyzen]

Summary

should_yield() compares thread::td_swvoltick to 'ticks' to determine
whether a thread is hogging and should yield. Prior to this commit,
td_swvoltick recorded 'ticks' only /before/ the actual context switch.
Therefore, the calculation in should_yield() includeed any time that
the thread was switched out. It seems that should_yield() wants to know
how long the thread has actually been running.

Record 'ticks' in td_swvoltick after sched_switch() returns. Keep the
existing assignment, before sched_switch(), so that db_show_thread will
still show useful data for sleeping threads. Do the same for
td_swinvolticks. This is only for consistency, since it's only used by
db_show_thread at present.

[mjg]

I think the condition is fishy. Why would it matter whether the thread gave up the CPU voluntarily when deciding whether to yield? I suspect it makes much more sense to check when it got on cpu in general. In particular a non-voluntary long sleep followed by checking only invol ticks will easily give a false-positive: the thread was not doing anything for quite some time and is now about to yield to to stop being a hog?

Someone(tm) should investigate what other kernels do.

[vangyzen]

In D13892#291170, @mjg wrote:

I think the condition is fishy. Why would it matter whether the thread gave up the CPU voluntarily when deciding whether to yield? I suspect it makes much more sense to check when it got on cpu in general. In particular a non-voluntary long sleep followed by checking only invol ticks will easily give a false-positive: the thread was not doing anything for quite some time and is now about to yield to to stop being a hog?

I don't fully understand the condition, but I'll take a guess. An involuntary context switch is usually a preemption. A preempted thread is given the highest priority of any thread currently in that CPU's run queue, so that when the preempting thread is finished, the preempted thread has a good chance of running. Therefore, an involuntary context switch does not help fairness, so should_yield() ignores them. The false-positive you describe certainly seems possible, though, so maybe there's room for improvement in the algorithm.

[rgrimes]

sys/kern/kern_synch.c

444

Isn't this already done at line 407 of this file?

[vangyzen]

sys/kern/kern_synch.c

444

Yes. I tried to explain the rationale in the review summary. If some part of that is not clear, let me know which.

[rgrimes]

sys/kern/kern_synch.c

380

This comment is rather useless, for a 50 line function we should have a much more detailed block comment.

413

We account for ticks here, as either vol or invol.

444

How about some comments added at line 407, and added here before we repeat this operation, explaining
why each is needed and why they are different. Is tick changed in value? Or is td? Or flags?
Do we end up double charging the tick in charngingsome case?

I think I see what it is now, if you find places for your summary as comments in this code it would probably
make it clear why we have to do this assign of ticks to the right place 2 times. If you do not comment this
code someone in the future may remove this code as seeming redundant.

The function mi_switch is missing a reasonable top block comment that explains how it does what it does
and why that is. See inline comment at 380.

[jhb]

I think should_yield is pretty broken regardless of this change. I think the idea is to demote to PRI_USER if the thread has been running for more than hogticks without yielding the CPU. Rather than using 'ticks' which counts walltime and not CPU time, I wonder if it would be better to have a 'td_swvoltime' (for lack of a better name) which is set to 'td_runtime' in the SW_VOL case and left alone in the SW_INVOL case. To check for a 'hog' you would do something like:

return (cputick2usec(td->td_runtime + (cpu_ticks() - PCPU_GET(switchtime) - td->td_ swvoltime) >= hogtime);

Where 'hogtime' would be in 'usec' instead of ticks. This would effectively make the calculation be one in CLOCK_THREAD_CPUTIME_ID rather than CLOCK_REALTIME.