Messages in this thread | | | Date | Wed, 1 Apr 2020 19:01:05 -0400 | From | Peter Xu <> | Subject | Re: [PATCH] sched/isolation: Allow "isolcpus=" to skip unknown sub-parameters |
| |
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".
> > The flags are required to be is_alpha() only. So you want something like > the untested below. Hmm?
I'm fine with it, however note that...
> > Thanks, > > tglx > > 8<--------------- > --- a/kernel/sched/isolation.c > +++ b/kernel/sched/isolation.c > @@ -149,6 +149,8 @@ static int __init housekeeping_nohz_full > 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"?
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 definitely like the pr_warn when there's unknown subparams). But just let me know your preference and I'll follow yours when repost.
Thanks,
> + return 0; > + } > + pr_info("isolcpus: Skipped unknown flag %*s\n", len, par); > + str++; > } > > /* Default behaviour for isolcpus without flags */ >
-- Peter Xu
| |