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

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

[Peter Xu]

The "isolcpus=" parameter allows sub-parameters to exist before the
cpulist is specified, and if it sees unknown sub-parameters the whole
parameter will be ignored.  This design is incompatible with itself
when we add more sub-parameters to "isolcpus=", because the old
kernels will not recognize the new "isolcpus=" sub-parameters, then it
will invalidate the whole parameter so the CPU isolation will not
really take effect if we start to use the new sub-parameters while
later we reboot into an old kernel. Instead we will see this when
booting the old kernel:

    isolcpus: Error, unknown flag

The better and compatible way is to allow "isolcpus=" to skip unknown
sub-parameters, so that even if we add new sub-parameters to it the
old kernel will still be able to behave as usual even if with the new
sub-parameter is specified.

Ideally this patch should be there when we introduce the first
sub-parameter for "isolcpus=", so it's already a bit late.  However
late is better than nothing.

CC: Ming Lei <ming.lei@redhat.com>
CC: Ingo Molnar <mingo@redhat.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: Juri Lelli <juri.lelli@redhat.com>
CC: Luiz Capitulino <lcapitulino@redhat.com>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 kernel/sched/isolation.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
index 008d6ac2342b..d5defb667bbc 100644
--- a/kernel/sched/isolation.c
+++ b/kernel/sched/isolation.c
@@ -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;
 	}
 
 	/* Default behaviour for isolcpus without flags */
-- 
2.24.1

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

[Ming Lei]

On Tue, Feb 04, 2020 at 11:16:39AM -0500, Peter Xu wrote:
> The "isolcpus=" parameter allows sub-parameters to exist before the
> cpulist is specified, and if it sees unknown sub-parameters the whole
> parameter will be ignored.  This design is incompatible with itself
> when we add more sub-parameters to "isolcpus=", because the old
> kernels will not recognize the new "isolcpus=" sub-parameters, then it
> will invalidate the whole parameter so the CPU isolation will not
> really take effect if we start to use the new sub-parameters while
> later we reboot into an old kernel. Instead we will see this when
> booting the old kernel:
> 
>     isolcpus: Error, unknown flag
> 
> The better and compatible way is to allow "isolcpus=" to skip unknown
> sub-parameters, so that even if we add new sub-parameters to it the
> old kernel will still be able to behave as usual even if with the new
> sub-parameter is specified.
> 
> Ideally this patch should be there when we introduce the first
> sub-parameter for "isolcpus=", so it's already a bit late.  However
> late is better than nothing.
> 
> CC: Ming Lei <ming.lei@redhat.com>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Peter Zijlstra <peterz@infradead.org>
> CC: Juri Lelli <juri.lelli@redhat.com>
> CC: Luiz Capitulino <lcapitulino@redhat.com>
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  kernel/sched/isolation.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
> index 008d6ac2342b..d5defb667bbc 100644
> --- a/kernel/sched/isolation.c
> +++ b/kernel/sched/isolation.c
> @@ -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;

Looks fine, even though the 'old' kernel has to apply this patch. 

Reviewed-by: Ming Lei <ming.lei@redhat.com>


thanks,
Ming

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

[Peter Xu]

On Wed, Feb 05, 2020 at 08:27:54PM +0800, Ming Lei wrote:
> On Tue, Feb 04, 2020 at 11:16:39AM -0500, Peter Xu wrote:
> > The "isolcpus=" parameter allows sub-parameters to exist before the
> > cpulist is specified, and if it sees unknown sub-parameters the whole
> > parameter will be ignored.  This design is incompatible with itself
> > when we add more sub-parameters to "isolcpus=", because the old
> > kernels will not recognize the new "isolcpus=" sub-parameters, then it
> > will invalidate the whole parameter so the CPU isolation will not
> > really take effect if we start to use the new sub-parameters while
> > later we reboot into an old kernel. Instead we will see this when
> > booting the old kernel:
> > 
> >     isolcpus: Error, unknown flag
> > 
> > The better and compatible way is to allow "isolcpus=" to skip unknown
> > sub-parameters, so that even if we add new sub-parameters to it the
> > old kernel will still be able to behave as usual even if with the new
> > sub-parameter is specified.
> > 
> > Ideally this patch should be there when we introduce the first
> > sub-parameter for "isolcpus=", so it's already a bit late.  However
> > late is better than nothing.
> > 
> > CC: Ming Lei <ming.lei@redhat.com>
> > CC: Ingo Molnar <mingo@redhat.com>
> > CC: Peter Zijlstra <peterz@infradead.org>
> > CC: Juri Lelli <juri.lelli@redhat.com>
> > CC: Luiz Capitulino <lcapitulino@redhat.com>
> > Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  kernel/sched/isolation.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
> > index 008d6ac2342b..d5defb667bbc 100644
> > --- a/kernel/sched/isolation.c
> > +++ b/kernel/sched/isolation.c
> > @@ -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;
> 
> Looks fine, even though the 'old' kernel has to apply this patch. 

Right, I think this suites for stable.  However I guess there's no way
we can really fix this on all old kernel binaries, which is really a
lesson that we should always design the parameters to be compatible
with the old binaries.

> 
> Reviewed-by: Ming Lei <ming.lei@redhat.com>

Thanks!

-- 
Peter Xu

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

[Peter Xu]

On Tue, Feb 04, 2020 at 11:16:39AM -0500, Peter Xu wrote:
> The "isolcpus=" parameter allows sub-parameters to exist before the
> cpulist is specified, and if it sees unknown sub-parameters the whole
> parameter will be ignored.  This design is incompatible with itself
> when we add more sub-parameters to "isolcpus=", because the old
> kernels will not recognize the new "isolcpus=" sub-parameters, then it
> will invalidate the whole parameter so the CPU isolation will not
> really take effect if we start to use the new sub-parameters while
> later we reboot into an old kernel. Instead we will see this when
> booting the old kernel:
> 
>     isolcpus: Error, unknown flag
> 
> The better and compatible way is to allow "isolcpus=" to skip unknown
> sub-parameters, so that even if we add new sub-parameters to it the
> old kernel will still be able to behave as usual even if with the new
> sub-parameter is specified.
> 
> Ideally this patch should be there when we introduce the first
> sub-parameter for "isolcpus=", so it's already a bit late.  However
> late is better than nothing.

Ping - Hi, Thomas, do you have any further comment on this patch?

Thanks,

-- 
Peter Xu

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

[Thomas Gleixner]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Feb 04, 2020 at 11:16:39AM -0500, Peter Xu wrote:
>> The "isolcpus=" parameter allows sub-parameters to exist before the
>> cpulist is specified, and if it sees unknown sub-parameters the whole
>> parameter will be ignored.  This design is incompatible with itself
>> when we add more sub-parameters to "isolcpus=", because the old
>> kernels will not recognize the new "isolcpus=" sub-parameters, then it
>> will invalidate the whole parameter so the CPU isolation will not
>> really take effect if we start to use the new sub-parameters while
>> later we reboot into an old kernel. Instead we will see this when
>> booting the old kernel:
>> 
>>     isolcpus: Error, unknown flag
>> 
>> The better and compatible way is to allow "isolcpus=" to skip unknown
>> sub-parameters, so that even if we add new sub-parameters to it the
>> old kernel will still be able to behave as usual even if with the new
>> sub-parameter is specified.
>> 
>> Ideally this patch should be there when we introduce the first
>> sub-parameter for "isolcpus=", so it's already a bit late.  However
>> late is better than nothing.
>
> Ping - Hi, Thomas, do you have any further comment on this patch?

Fine with me.

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

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

[Peter Xu]

On Fri, Feb 14, 2020 at 09:28:25PM +0100, Thomas Gleixner wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Tue, Feb 04, 2020 at 11:16:39AM -0500, Peter Xu wrote:
> >> The "isolcpus=" parameter allows sub-parameters to exist before the
> >> cpulist is specified, and if it sees unknown sub-parameters the whole
> >> parameter will be ignored.  This design is incompatible with itself
> >> when we add more sub-parameters to "isolcpus=", because the old
> >> kernels will not recognize the new "isolcpus=" sub-parameters, then it
> >> will invalidate the whole parameter so the CPU isolation will not
> >> really take effect if we start to use the new sub-parameters while
> >> later we reboot into an old kernel. Instead we will see this when
> >> booting the old kernel:
> >> 
> >>     isolcpus: Error, unknown flag
> >> 
> >> The better and compatible way is to allow "isolcpus=" to skip unknown
> >> sub-parameters, so that even if we add new sub-parameters to it the
> >> old kernel will still be able to behave as usual even if with the new
> >> sub-parameter is specified.
> >> 
> >> Ideally this patch should be there when we introduce the first
> >> sub-parameter for "isolcpus=", so it's already a bit late.  However
> >> late is better than nothing.
> >
> > Ping - Hi, Thomas, do you have any further comment on this patch?
> 
> Fine with me.
> 
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Thanks Thomas!

Does anyone like to pick this up, or does this patch needs more
review?

Thanks,

-- 
Peter Xu

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

[Peter Xu]

On Mon, Mar 09, 2020 at 11:19:17AM -0400, Peter Xu wrote:
> On Fri, Feb 14, 2020 at 09:28:25PM +0100, Thomas Gleixner wrote:
> > Peter Xu <peterx@redhat.com> writes:
> > 
> > > On Tue, Feb 04, 2020 at 11:16:39AM -0500, Peter Xu wrote:
> > >> The "isolcpus=" parameter allows sub-parameters to exist before the
> > >> cpulist is specified, and if it sees unknown sub-parameters the whole
> > >> parameter will be ignored.  This design is incompatible with itself
> > >> when we add more sub-parameters to "isolcpus=", because the old
> > >> kernels will not recognize the new "isolcpus=" sub-parameters, then it
> > >> will invalidate the whole parameter so the CPU isolation will not
> > >> really take effect if we start to use the new sub-parameters while
> > >> later we reboot into an old kernel. Instead we will see this when
> > >> booting the old kernel:
> > >> 
> > >>     isolcpus: Error, unknown flag
> > >> 
> > >> The better and compatible way is to allow "isolcpus=" to skip unknown
> > >> sub-parameters, so that even if we add new sub-parameters to it the
> > >> old kernel will still be able to behave as usual even if with the new
> > >> sub-parameter is specified.
> > >> 
> > >> Ideally this patch should be there when we introduce the first
> > >> sub-parameter for "isolcpus=", so it's already a bit late.  However
> > >> late is better than nothing.
> > >
> > > Ping - Hi, Thomas, do you have any further comment on this patch?
> > 
> > Fine with me.
> > 
> > Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
> 
> Thanks Thomas!
> 
> Does anyone like to pick this up, or does this patch needs more
> review?

Another gentle ping with the hope that this patch can be picked up.

It's a very simple patch, but I really hope it can be in asap because
the latter means the more kernel versions will be affected by this
isolcpus incompatibility defect (and imo should consider stable too).

Thanks.

-- 
Peter Xu

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

[Thomas Gleixner]

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.

The flags are required to be is_alpha() only. So you want something like
the untested below. Hmm?

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);
+			return 0;
+		}
+		pr_info("isolcpus: Skipped unknown flag %*s\n", len, par);
+		str++;
 	}
 
 	/* Default behaviour for isolcpus without flags */

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

[Peter Xu]

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

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

[Thomas Gleixner]

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

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

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

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

[Thomas Gleixner]

Peter Xu <peterx@redhat.com> writes:
> On Thu, Apr 02, 2020 at 01:29:14AM +0200, Thomas Gleixner wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> >> +		/*
>> >> +		 * 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?

Obviously.

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

You can achieve the same thing without the illegal indirection with

	pr_warn("....", len + 1, par);

Thanks,

        tglx

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

[Peter Xu]

On Thu, Apr 02, 2020 at 10:40:49AM +0200, Thomas Gleixner wrote:
> Peter Xu <peterx@redhat.com> writes:
> > On Thu, Apr 02, 2020 at 01:29:14AM +0200, Thomas Gleixner wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >> >> +		/*
> >> >> +		 * 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?
> 
> Obviously.
> 
> > 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);
> 
> You can achieve the same thing without the illegal indirection with
> 
> 	pr_warn("....", len + 1, par);

I think it will stop working with "isolcpus=nohz,domain11,12,13".

I'll repost soon.  Thanks,

-- 
Peter Xu