* [PATCH v2] sched/isolation: Allow "isolcpus=" to skip unknown sub-parameters
@ 2020-04-02 14:59 Peter Xu
2020-04-03 1:15 ` Peter Xu
0 siblings, 1 reply; 3+ messages in thread
From: Peter Xu @ 2020-04-02 14:59 UTC (permalink / raw)
To: linux-kernel
Cc: peterx, Ming Lei, Ingo Molnar, Peter Zijlstra, Juri Lelli,
Luiz Capitulino, Thomas Gleixner
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>
---
v2:
- only allow isalpha() for sub-parameters [tglx]
---
kernel/sched/isolation.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
index 008d6ac2342b..c2e8b4a778d6 100644
--- a/kernel/sched/isolation.c
+++ b/kernel/sched/isolation.c
@@ -149,6 +149,9 @@ __setup("nohz_full=", housekeeping_nohz_full_setup);
static int __init housekeeping_isolcpus_setup(char *str)
{
unsigned int flags = 0;
+ char *par;
+ int len;
+ bool illegal = false;
while (isalpha(*str)) {
if (!strncmp(str, "nohz,", 5)) {
@@ -169,8 +172,21 @@ static int __init housekeeping_isolcpus_setup(char *str)
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; *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++;
}
/* Default behaviour for isolcpus without flags */
--
2.24.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] sched/isolation: Allow "isolcpus=" to skip unknown sub-parameters
2020-04-02 14:59 [PATCH v2] sched/isolation: Allow "isolcpus=" to skip unknown sub-parameters Peter Xu
@ 2020-04-03 1:15 ` Peter Xu
2020-04-03 20:27 ` Thomas Gleixner
0 siblings, 1 reply; 3+ messages in thread
From: Peter Xu @ 2020-04-03 1:15 UTC (permalink / raw)
To: linux-kernel
Cc: Ming Lei, Ingo Molnar, Peter Zijlstra, Juri Lelli,
Luiz Capitulino, Thomas Gleixner
On Thu, Apr 02, 2020 at 10:59:29AM -0400, 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>
> ---
> v2:
> - only allow isalpha() for sub-parameters [tglx]
> ---
> kernel/sched/isolation.c | 20 ++++++++++++++++++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
> index 008d6ac2342b..c2e8b4a778d6 100644
> --- a/kernel/sched/isolation.c
> +++ b/kernel/sched/isolation.c
> @@ -149,6 +149,9 @@ __setup("nohz_full=", housekeeping_nohz_full_setup);
> static int __init housekeeping_isolcpus_setup(char *str)
> {
> unsigned int flags = 0;
> + char *par;
> + int len;
> + bool illegal = false;
>
> while (isalpha(*str)) {
> if (!strncmp(str, "nohz,", 5)) {
> @@ -169,8 +172,21 @@ static int __init housekeeping_isolcpus_setup(char *str)
> 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; *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++;
> }
I just noticed this is still problematic if we want to mark this as
stable, because "managed_irq" violate the "isalpha()" rule already...
It means even if we apply this patch to the stable trees it'll still
think managed_irq as illegal and ignore the whole isolcpus=.
Thomas, do you want me to repost a v3 as v1 plus some pr_warn()s?
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] sched/isolation: Allow "isolcpus=" to skip unknown sub-parameters
2020-04-03 1:15 ` Peter Xu
@ 2020-04-03 20:27 ` Thomas Gleixner
0 siblings, 0 replies; 3+ messages in thread
From: Thomas Gleixner @ 2020-04-03 20:27 UTC (permalink / raw)
To: Peter Xu, linux-kernel
Cc: Ming Lei, Ingo Molnar, Peter Zijlstra, Juri Lelli, Luiz Capitulino
Peter Xu <peterx@redhat.com> writes:
> On Thu, Apr 02, 2020 at 10:59:29AM -0400, Peter Xu wrote:
>> - 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; *str && *str != ','; str++, len++)
lacks {
>> + if (!isalpha(*str))
>> + illegal = true;
lacks }
>> +
>> + if (illegal) {
>> + pr_warn("isolcpus: Invalid flag %.*s\n", len, par);
>> + return 0;
>> + }
>> +
>> + pr_info("isolcpus: Skipped unknown flag %.*s\n", len, par);
>> + str++;
>> }
>
> I just noticed this is still problematic if we want to mark this as
> stable, because "managed_irq" violate the "isalpha()" rule already...
> It means even if we apply this patch to the stable trees it'll still
> think managed_irq as illegal and ignore the whole isolcpus=.
if (!isalpha(*str) && *str != '_')
which is what I told you a couple of days ago already.
Thanks,
tglx
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-04-03 20:27 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-02 14:59 [PATCH v2] sched/isolation: Allow "isolcpus=" to skip unknown sub-parameters Peter Xu
2020-04-03 1:15 ` Peter Xu
2020-04-03 20:27 ` Thomas Gleixner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).