Re: [PATCH] sched/isolation: Allow "isolcpus=" to skip unknown sub-parameters

[Peter Xu]

On Thu, Apr 02, 2020 at 01:29:14AM +0200, Thomas Gleixner wrote:
> Peter Xu <peterx@xxxxxxxxxx> writes:
> 
> > On Wed, Apr 01, 2020 at 10:30:08PM +0200, Thomas Gleixner wrote:
> >> Peter Xu <peterx@xxxxxxxxxx> 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

It will dump "isolcpus: Invalid flag domain1,3,5". Do you mean "%.*s"
instead?

Another issue is even if to use "%.*s" it'll only dump "domain".  How
about something like (declare "illegal" as bool):

		/*
		 * Skip unknown sub-parameter and validate that it is not
		 * containing an invalid character.
		 */
		for (par = str, len = 0; *str && *str != ','; str++, len++)
			if (!isalpha(*str))
				illegal = true;

		if (illegal) {
			pr_warn("isolcpus: Invalid flag %.*s\n", len, par);
			return 0;
		}

		pr_info("isolcpus: Skipped unknown flag %.*s\n", len, par);
		str++;

> 
> > 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.

Frankly speaking I really see it as simple as "we define a rule to
write these parameters, and people follow"...  But I won't argue more.

If you see above clip looks good, I can repost with a formal patch.

Thanks,

> 
> > 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
> 

-- 
Peter Xu