From: Thomas Gleixner <tglx@linutronix.de>
To: Peter Xu <peterx@redhat.com>
Cc: linux-kernel@vger.kernel.org, Ming Lei <ming.lei@redhat.com>,
Ingo Molnar <mingo@redhat.com>,
Peter Zijlstra <peterz@infradead.org>,
Juri Lelli <juri.lelli@redhat.com>,
Luiz Capitulino <lcapitulino@redhat.com>
Subject: Re: [PATCH] sched/isolation: Allow "isolcpus=" to skip unknown sub-parameters
Date: Thu, 02 Apr 2020 01:29:14 +0200 [thread overview]
Message-ID: <87wo6yokdx.fsf@nanos.tec.linutronix.de> (raw)
In-Reply-To: <20200401230105.GF648829@xz-x1>
Peter Xu <peterx@redhat.com> writes:
> On Wed, Apr 01, 2020 at 10:30:08PM +0200, Thomas Gleixner wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> > @@ -169,8 +169,12 @@ static int __init housekeeping_isolcpus_setup(char *str)
>> > continue;
>> > }
>> >
>> > - pr_warn("isolcpus: Error, unknown flag\n");
>> > - return 0;
>> > + str = strchr(str, ',');
>> > + if (str)
>> > + /* Skip unknown sub-parameter */
>> > + str++;
>> > + else
>> > + return 0;
>>
>> Just looked at it again because I wanted to apply this and contrary to
>> last time I figured out that this is broken:
>>
>> isolcpus=nohz,domain1,3,5
>>
>> is a malformatted option, but the above will make it "valid" and result
>> in:
>>
>> HK_FLAG_TICK and a cpumask of 3,5.
>
> I would think this is no worse than applying nothing - I read the
> first "isalpha()" check as something like "the subparameter's first
> character must not be a digit", so to differenciate with the cpu list.
> If we keep this, we can still have subparams like "double-word".
It _is_ worse. If the intention is to write 'nohz,domain,1,3,5' and
that missing comma morphs it silently into 'nohz,3,5' then this is
really a step backwards. The upstream version would tell you that you
screwed up.
>> static int __init housekeeping_isolcpus_setup(char *str)
>> {
>> unsigned int flags = 0;
>> + char *par;
>> + int len;
>>
>> while (isalpha(*str)) {
>> if (!strncmp(str, "nohz,", 5)) {
>> @@ -169,8 +171,17 @@ static int __init housekeeping_isolcpus_
>> continue;
>> }
>>
>> - pr_warn("isolcpus: Error, unknown flag\n");
>> - return 0;
>> + /*
>> + * Skip unknown sub-parameter and validate that it is not
>> + * containing an invalid character.
>> + */
>> + for (par = str, len = 0; isalpha(*str); str++, len++);
>> + if (*str != ',') {
>> + pr_warn("isolcpus: Invalid flag %*s\n", len, par);
>
> ... this will dump "isolcpus: Invalid flag domain1,3,5", is this what
> we wanted? Maybe only dumps "domain1"?
No, it will dump: "domain1" at least if my understanding of is_alpha()
and the '%*s' format option is halfways correct
> For me so far I would still prefer the original one, giving more
> freedom to the future params and the patch is also a bit easier (but I
Again. It does not matter whether the patch is easier or not. What
matters is correctness and usability. Silently converting a typo into
something else is horrible at best.
> definitely like the pr_warn when there's unknown subparams). But just
> let me know your preference and I'll follow yours when repost.
Enforcing a pure 'is_alpha()' subparam space is not really a substantial
restriction. Feel free to extend it by adding '|| *str == '_' if you
really think that provides a value.
Thanks,
tglx
next prev parent reply other threads:[~2020-04-01 23:29 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-04 16:16 [PATCH] sched/isolation: Allow "isolcpus=" to skip unknown sub-parameters Peter Xu
2020-02-05 12:27 ` Ming Lei
2020-02-05 13:34 ` Peter Xu
2020-02-14 19:40 ` Peter Xu
2020-02-14 20:28 ` Thomas Gleixner
2020-03-09 15:19 ` Peter Xu
2020-03-31 20:57 ` Peter Xu
2020-04-01 20:30 ` Thomas Gleixner
2020-04-01 23:01 ` Peter Xu
2020-04-01 23:29 ` Thomas Gleixner [this message]
2020-04-02 0:50 ` Peter Xu
2020-04-02 8:40 ` Thomas Gleixner
2020-04-02 13:14 ` Peter Xu
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=87wo6yokdx.fsf@nanos.tec.linutronix.de \
--to=tglx@linutronix.de \
--cc=juri.lelli@redhat.com \
--cc=lcapitulino@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=ming.lei@redhat.com \
--cc=mingo@redhat.com \
--cc=peterx@redhat.com \
--cc=peterz@infradead.org \
/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).