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!
