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

[Peter Xu <peterx@redhat.com>]

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