From: CGEL <cgel.zte@gmail.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: yzaikin@google.com, mingo@redhat.com, juri.lelli@redhat.com,
vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de,
bristot@redhat.com, mcgrof@kernel.org, keescook@chromium.org,
pjt@google.com, yang.yang29@zte.com.cn, joshdon@google.com,
linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
Zeal Robot <zealci@zte.com.cm>
Subject: Re: [PATCH] sched: Add a new version sysctl to control child runs first
Date: Thu, 7 Oct 2021 03:26:37 +0000 [thread overview]
Message-ID: <615e68ef.1c69fb81.bf82e.41af@mx.google.com> (raw)
In-Reply-To: <20210914040524.GA141438@cgel.zte@gmail.com>
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@gmail.com wrote:
> > > > > From: Yang Yang <yang.yang29@zte.com.cn>
> > > > >
> > > > > 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!
prev parent reply other threads:[~2021-10-07 3:26 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-12 4:12 [PATCH] sched: Add a new version sysctl to control child runs first cgel.zte
2021-09-13 8:13 ` Peter Zijlstra
2021-09-13 11:37 ` CGEL
2021-09-13 13:42 ` Peter Zijlstra
2021-09-14 4:05 ` CGEL
[not found] ` <20210914040524.GA141438@cgel.zte@gmail.com>
2021-10-07 3:26 ` CGEL [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=615e68ef.1c69fb81.bf82e.41af@mx.google.com \
--to=cgel.zte@gmail.com \
--cc=bristot@redhat.com \
--cc=bsegall@google.com \
--cc=dietmar.eggemann@arm.com \
--cc=joshdon@google.com \
--cc=juri.lelli@redhat.com \
--cc=keescook@chromium.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mcgrof@kernel.org \
--cc=mgorman@suse.de \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=pjt@google.com \
--cc=rostedt@goodmis.org \
--cc=vincent.guittot@linaro.org \
--cc=yang.yang29@zte.com.cn \
--cc=yzaikin@google.com \
--cc=zealci@zte.com.cm \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).