sched_ule: fix invalid tdq_slice() and sched_slice_min
AcceptedPublic
Actions

Authored by mchoo on Fri, Sep 4, 5:41 PM.

Details

Reviewers

jhb
olce

Group Reviewers

scheduler

Summary

sched_slice_min should always to be greater than zero. When modifying
sched_slice through sysctl, if the new value is less than
SCHED_SLICE_MIN_DIVISOR, sched_slice_min is computed to zero. Add
imax(1, ...) to prevent this.

tdq_slice() should not return a value less than sched_slice_min since
that will cause integer underflow of ts2->ts_slice in
sched_ule_fork_thread. SCHED_SLICE_MIN_DIVISOR is currently set to 6 so
when load is 5 and sched_slice is 4, the two if conditions in
tdq_slice() will pass and the function will return zero. Thus use imax()
so tdq_slice returns sched_slice_min at minimum.

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

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 76516
Build 73399: arc lint + arc unit

Event Timeline

mchoo created this revision.Fri, Sep 4, 5:41 PM

Herald added subscribers: olce, imp. · View Herald TranscriptFri, Sep 4, 5:41 PM

mchoo requested review of this revision.Fri, Sep 4, 5:41 PM

Harbormaster completed remote builds in B76516: Diff 185844.Fri, Sep 4, 5:41 PM

mchoo added a parent revision: D59407: sched_ule: fix comment on ts_slice.Fri, Sep 4, 5:41 PM

mchoo added a child revision: D59409: sched_ule: fix typo in comment.

mchoo mentioned this in D59401: sched: fix kern.sched.{4bsd,ule}.slice.Sun, Sep 6, 9:03 PM

Slice is number of ticks a thread is allowed to run, thus having slice
value of 0 shouldn't be accepted as threads will never run. In
tdq_slice(), when sched_slice > load, the function will return 0.

You probably meant sched_slice < load, but anyway that is not true, as the first if is precisely here to avoid that.

Similarily in sysctl.kern.ule.{quantum,slice}, if sched_slice is set
less then SCHED_SLICE_MIN_DIVISOR, sched_slice_min is computated as
zero.

That's indeed a real problem. (In passing, "computated" => "computed".)

I like the change, not only because it fixes a problem, but also because it removes a comparison in tdq_slice().

This revision is now accepted and ready to land.Sun, Sep 6, 9:34 PM

In D59408#1364277, @olce wrote:

Slice is number of ticks a thread is allowed to run, thus having slice
value of 0 shouldn't be accepted as threads will never run. In
tdq_slice(), when sched_slice > load, the function will return 0.

You probably meant sched_slice < load, but anyway that is not true, as the first if is precisely here to avoid that.

I meant sched_slice < load. sysctl_kern_slice() lets you set any slice greater or equal to 1. When sched_slice is set to 4 load is 5, the first if condition passes because SCHED_SLICE_MIN_DIVISOR is set to 6 by macro definition. Second if condition also passes and tdq_slice() returns 0.

In D59408#1364378, @mchoo wrote:

sysctl_kern_slice() lets you set any slice greater or equal to 1. When sched_slice is set to 4 load is 5, the first if condition passes because SCHED_SLICE_MIN_DIVISOR is set to 6 by macro definition. Second if condition also passes and tdq_slice() returns 0.

Yes, of course, but what is written in the first paragraph gives the impression you are talking about tdq_slice()'s code only, mainly because you mention the "sched_slice > load" condition, which is not the full condition for that function to produce 0. Additionally, you're mentioning the real source bug only in the second paragraph, which is the indirect cause of tdq_slice()'s problem. So please amend the commit message to that the the reason for the bug and the conditions of occurrence are clear.