sched/isolation: Allow "isolcpus=" to skip unknown sub-parameters
diff mbox series

Message ID 20200204161639.267026-1-peterx@redhat.com
State New, archived
Headers show
Series
  • sched/isolation: Allow "isolcpus=" to skip unknown sub-parameters
Related show

Commit Message

Peter Xu Feb. 4, 2020, 4:16 p.m. UTC
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(-)

Comments

Ming Lei Feb. 5, 2020, 12:27 p.m. UTC | #1
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
Peter Xu Feb. 5, 2020, 1:34 p.m. UTC | #2
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 Feb. 14, 2020, 7:40 p.m. UTC | #3
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,
Thomas Gleixner Feb. 14, 2020, 8:28 p.m. UTC | #4
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>
Peter Xu March 9, 2020, 3:19 p.m. UTC | #5
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 March 31, 2020, 8:57 p.m. UTC | #6
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.
Thomas Gleixner April 1, 2020, 8:30 p.m. UTC | #7
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 */
Peter Xu April 1, 2020, 11:01 p.m. UTC | #8
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 */
>
Thomas Gleixner April 1, 2020, 11:29 p.m. UTC | #9
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
Peter Xu April 2, 2020, 12:50 a.m. UTC | #10
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
>
Thomas Gleixner April 2, 2020, 8:40 a.m. UTC | #11
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
Peter Xu April 2, 2020, 1:14 p.m. UTC | #12
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,

Patch
diff mbox series

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