Re: [PATCH] sched: Add a new version sysctl to control child runs first

[CGEL]

On Tue, Sep 14, 2021 at 04:05:26AM +0000, CGEL wrote:
> esOn Mon, Sep 13, 2021 at 03:42:45PM +0200, Peter Zijlstra wrote:
> > On Mon, Sep 13, 2021 at 11:37:31AM +0000, CGEL wrote:
> > > On Mon, Sep 13, 2021 at 10:13:54AM +0200, Peter Zijlstra wrote:
> > > > On Sun, Sep 12, 2021 at 04:12:23AM +0000, cgel.zte@xxxxxxxxx wrote:
> > > > > From: Yang Yang <yang.yang29@xxxxxxxxxx>
> > > > > 
> > > > > The old version sysctl has some problems. First, it allows set value
> > > > > bigger than 1, which is unnecessary. Second, it didn't follow the
> > > > > rule of capabilities. Thirdly, it didn't use static key. This new
> > > > > version fixes all the problems.
> > > > 
> > > > Does any of that actually matter?
> > > 
> > > For the first problem, I think the reason why sysctl_schedstats() only
> > > accepts 0 or 1, is suitbale for sysctl_child_runs_first(). Since
> > > task_fork_fair() only need sysctl_sched_child_runs_first to be
> > > zero or non-zero.
> > 
> > This could potentially break people that already write a larger value in
> > it -- by accident or otherwise.
> 
> Thanks for reply!
> 
> You mean it's right to set sched_child_runs_first 0 or 1, but consider about
> compatibility, just leave it?
> Should stable/longterm branches keep compatibility, but linux-next fixes it?
> 
> Let's take a look at negative influence about unnecessary values of sysctl.
> Some tune tools will automatic to set different values of sysctl to see
> performance impact. So invalid values may waste tune tools's time, specially
> when the range of values is big.
> 
> For example A-Tune, see below:
> https://docs.openeuler.org/zh/docs/20.03_LTS/docs/A-Tune/%E8%AE%A4%E8%AF%86A-Tune.html 
> Since it's wroten in Chinese, I try to explain it in short.
> A-Tune modeling sysctls first(what values sysctls accept), then automatic to iterate
> different values to find the best combination of sysctl values for the workload.
>
Hi 
Should modify this path or just abandon it?

> > 
> > > For the second problem, I remember there is a rule: try to
> > > administration system through capilities but not depends on
> > > root identity. Just like sysctl_schedstats() or other
> > > sysctl_xx().
> > 
> > It seems entirely daft to me; those files are already 644, if root opens
> > the file and passes it along, it gets to keep the pieces.
> > 
> 
> I think it's indeed a little tricky: root may drop it's own capabilites.
> Let's see another example of netdev_store(), root can't modify netdev
> attribute without CAP_NET_ADMIN, even it pass the 644 DAC check.
> 
> > > For the thirdly problem, sysctl_child_runs_first maynot changes
> > > often, but may accessed often, like static_key delayacct_key
> > > controlled by sysctl_delayacct().
> > 
> > Can you actually show it makes a performance difference in a fork
> > micro-bench? Given the amount of gunk fork() already does, I don't think
> > it'll matter one way or the other, and in that case, simpler is better.
> 
> With 5.14-rc6 and gcc6.2.0, this patch will reduce test instruct in
> task_fork_fair() as Documentation/staging/static-keys.rst said.
> Since task_fork_fair() may called often, I think it's OK to use static
> key, actually there are quit a lot static keys in kernel/xx.
> 
> When talk about simply, maybe keep in consistent with other sysctls like
> task_delayacct() is also a kind of simply in code style.
> 
> Before this patch:
> ffff810a5c60 <task_fork_fair>:
> ..
> ffffffff810a5cf3: e8 a8 b3 ff ff       callq ffffffff810a10a0 <place_entity>
> ffffffff810a5cf8: 8b 05 e2 b5 5d 01    mov 0x15db5e2(%rip),%eax # ffffffff826812e0 <sysctl_sched_child_runs_first>
> ffffffff810a5cfe: 85 c0                test %eax,%eax
> ffffffff810a5d00: 74 5b                je ffffffff810a5d5d <task_fork_fair+0xfd>
> ffffffff810a5d02: 49 8b 55 50          mov 0x50(%r13),%rdx
> ffffffff810a5d06: 49 8b 84 24 10 01 00 mov 0x110(%r12),%rax
> ffffffff810a5d0d: 00 
> ffffffff810a5d0e: 48 39 c2             cmp %rax,%rdx
> ffffffff810a5d11: 78 36                js ffffffff810a5d49 <task_fork_fair+0xe9>
> ffffffff810a5d13: 48 2b 45 28          sub    0x28(%rbp),%rax
> 
> After this patch:
> ffffffff810a5c60 <task_fork_fair>:
> ..
> ffffffff810a5cf3: e8 a8 b3 ff ff       callq  ffffffff810a10a0 <place_entity>
> ffffffff810a5cf8: 66 90                xchg   %ax,%ax
> ffffffff810a5cfa: 49 8b 84 24 10 01 00 mov    0x110(%r12),%rax
> ffffffff810a5d01: 00
> ffffffff810a5d02: 48 2b 45 28          sub    0x28(%rbp),%rax
> 
> Thanks!