* [PATCH] xen/arm: smpboot: Allocate the CPU sibling/core maps while preparing the CPU
@ 2022-06-14 9:41 Julien Grall
2022-06-14 11:02 ` Michal Orzel
0 siblings, 1 reply; 6+ messages in thread
From: Julien Grall @ 2022-06-14 9:41 UTC (permalink / raw)
To: xen-devel
Cc: andrew.cooper3, Julien Grall, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
From: Julien Grall <jgrall@amazon.com>
Commit 5047cd1d5dea "xen/common: Use enhanced ASSERT_ALLOC_CONTEXT in
xmalloc()" extended the checks in _xmalloc() to catch any use of the
helpers from context with interrupts disabled.
Unfortunately, the rule is not followed when allocating the CPU
sibling/core maps.
(XEN) Xen call trace:
(XEN) [<00238a5c>] _xmalloc+0xfc/0x314 (PC)
(XEN) [<00000000>] 00000000 (LR)
(XEN) [<00238c8c>] _xzalloc+0x18/0x4c
(XEN) [<00288cb4>] smpboot.c#setup_cpu_sibling_map+0x38/0x138
(XEN) [<00289024>] start_secondary+0x1b4/0x270
(XEN) [<40010170>] 40010170
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 2:
(XEN) Assertion '!in_irq() && (local_irq_is_enabled() || num_online_cpus() <= 1)' failed at common/xmalloc_tlsf.c:601
(XEN) ****************************************
This is happening because zalloc_cpumask_var() may allocate memory
if NR_CPUS is > 2 * sizeof(unsigned long).
Avoid the problem by allocate the per-CPU IRQs while preparing the
CPU.
This also has the benefit to remove a panic() in the secondary CPU
code.
Signed-off-by: Julien Grall <jgrall@amazon.com>
---
xen/arch/arm/smpboot.c | 25 ++++++++++++++++++++-----
1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 4888bcd78a5a..2b0c92cd369b 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -79,15 +79,17 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_core_mask);
static bool __read_mostly opt_hmp_unsafe = false;
boolean_param("hmp-unsafe", opt_hmp_unsafe);
-static void setup_cpu_sibling_map(int cpu)
+static int setup_cpu_sibling_map(int cpu)
{
if ( !zalloc_cpumask_var(&per_cpu(cpu_sibling_mask, cpu)) ||
!zalloc_cpumask_var(&per_cpu(cpu_core_mask, cpu)) )
- panic("No memory for CPU sibling/core maps\n");
+ return -ENOMEM;
/* A CPU is a sibling with itself and is always on its own core. */
cpumask_set_cpu(cpu, per_cpu(cpu_sibling_mask, cpu));
cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu));
+
+ return 0;
}
static void remove_cpu_sibling_map(int cpu)
@@ -292,9 +294,14 @@ smp_get_max_cpus (void)
void __init
smp_prepare_cpus(void)
{
+ int rc;
+
cpumask_copy(&cpu_present_map, &cpu_possible_map);
- setup_cpu_sibling_map(0);
+ rc = setup_cpu_sibling_map(0);
+ if ( rc )
+ panic("Unable to allocate CPU sibling/core maps\n");
+
}
/* Boot the current CPU */
@@ -361,8 +368,6 @@ void start_secondary(void)
set_current(idle_vcpu[cpuid]);
- setup_cpu_sibling_map(cpuid);
-
/* Run local notifiers */
notify_cpu_starting(cpuid);
/*
@@ -530,9 +535,19 @@ static int cpu_smpboot_callback(struct notifier_block *nfb,
void *hcpu)
{
unsigned int cpu = (unsigned long)hcpu;
+ unsigned int rc = 0;
switch ( action )
{
+ case CPU_UP_PREPARE:
+ rc = setup_cpu_sibling_map(cpu);
+ if ( rc )
+ printk(XENLOG_ERR
+ "Unable to allocate CPU sibling/core map for CPU%u\n",
+ cpu);
+
+ break;
+
case CPU_DEAD:
remove_cpu_sibling_map(cpu);
break;
--
2.32.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/arm: smpboot: Allocate the CPU sibling/core maps while preparing the CPU
2022-06-14 9:41 [PATCH] xen/arm: smpboot: Allocate the CPU sibling/core maps while preparing the CPU Julien Grall
@ 2022-06-14 11:02 ` Michal Orzel
2022-06-14 11:08 ` Julien Grall
0 siblings, 1 reply; 6+ messages in thread
From: Michal Orzel @ 2022-06-14 11:02 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: andrew.cooper3, Julien Grall, Stefano Stabellini,
Bertrand Marquis, Volodymyr Babchuk
Hi Julien,
On 14.06.2022 11:41, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
>
> Commit 5047cd1d5dea "xen/common: Use enhanced ASSERT_ALLOC_CONTEXT in
> xmalloc()" extended the checks in _xmalloc() to catch any use of the
> helpers from context with interrupts disabled.
>
> Unfortunately, the rule is not followed when allocating the CPU
> sibling/core maps.
>
> (XEN) Xen call trace:
> (XEN) [<00238a5c>] _xmalloc+0xfc/0x314 (PC)
> (XEN) [<00000000>] 00000000 (LR)
> (XEN) [<00238c8c>] _xzalloc+0x18/0x4c
> (XEN) [<00288cb4>] smpboot.c#setup_cpu_sibling_map+0x38/0x138
> (XEN) [<00289024>] start_secondary+0x1b4/0x270
> (XEN) [<40010170>] 40010170
> (XEN)
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 2:
> (XEN) Assertion '!in_irq() && (local_irq_is_enabled() || num_online_cpus() <= 1)' failed at common/xmalloc_tlsf.c:601
> (XEN) ****************************************
>
> This is happening because zalloc_cpumask_var() may allocate memory
> if NR_CPUS is > 2 * sizeof(unsigned long).
>
> Avoid the problem by allocate the per-CPU IRQs while preparing the
> CPU.
Shouldn't this be "by allocating the CPU sibling/core maps while ..."
to reflect the commit title and to distinguish between this change and the IRQ one?
>
> This also has the benefit to remove a panic() in the secondary CPU
> code.
>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
> xen/arch/arm/smpboot.c | 25 ++++++++++++++++++++-----
> 1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 4888bcd78a5a..2b0c92cd369b 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -79,15 +79,17 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_core_mask);
> static bool __read_mostly opt_hmp_unsafe = false;
> boolean_param("hmp-unsafe", opt_hmp_unsafe);
>
> -static void setup_cpu_sibling_map(int cpu)
> +static int setup_cpu_sibling_map(int cpu)
> {
> if ( !zalloc_cpumask_var(&per_cpu(cpu_sibling_mask, cpu)) ||
> !zalloc_cpumask_var(&per_cpu(cpu_core_mask, cpu)) )
> - panic("No memory for CPU sibling/core maps\n");
> + return -ENOMEM;
>
> /* A CPU is a sibling with itself and is always on its own core. */
> cpumask_set_cpu(cpu, per_cpu(cpu_sibling_mask, cpu));
> cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu));
> +
> + return 0;
> }
>
> static void remove_cpu_sibling_map(int cpu)
> @@ -292,9 +294,14 @@ smp_get_max_cpus (void)
> void __init
> smp_prepare_cpus(void)
> {
> + int rc;
Here you are leaving rc uninitialized (which is ok) but ...
> +
> cpumask_copy(&cpu_present_map, &cpu_possible_map);
>
> - setup_cpu_sibling_map(0);
> + rc = setup_cpu_sibling_map(0);
> + if ( rc )
> + panic("Unable to allocate CPU sibling/core maps\n");
> +
> }
>
> /* Boot the current CPU */
> @@ -361,8 +368,6 @@ void start_secondary(void)
>
> set_current(idle_vcpu[cpuid]);
>
> - setup_cpu_sibling_map(cpuid);
> -
> /* Run local notifiers */
> notify_cpu_starting(cpuid);
> /*
> @@ -530,9 +535,19 @@ static int cpu_smpboot_callback(struct notifier_block *nfb,
> void *hcpu)
> {
> unsigned int cpu = (unsigned long)hcpu;
> + unsigned int rc = 0;
... here you are setting rc to 0 even though it will be reassigned.
Furthermore, if rc is used only in case of CPU_UP_PREPARE, why not moving the definition there?
>
> switch ( action )
> {
> + case CPU_UP_PREPARE:
> + rc = setup_cpu_sibling_map(cpu);
> + if ( rc )
> + printk(XENLOG_ERR
> + "Unable to allocate CPU sibling/core map for CPU%u\n",
Too many spaces between 'map' and 'for'.
> + cpu);
> +
> + break;
> +
> case CPU_DEAD:
> remove_cpu_sibling_map(cpu);
> break;
Cheers,
Michal
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/arm: smpboot: Allocate the CPU sibling/core maps while preparing the CPU
2022-06-14 11:02 ` Michal Orzel
@ 2022-06-14 11:08 ` Julien Grall
2022-06-14 11:13 ` Michal Orzel
0 siblings, 1 reply; 6+ messages in thread
From: Julien Grall @ 2022-06-14 11:08 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: andrew.cooper3, Julien Grall, Stefano Stabellini,
Bertrand Marquis, Volodymyr Babchuk
On 14/06/2022 12:02, Michal Orzel wrote:
> Hi Julien,
Hi Michal,
> On 14.06.2022 11:41, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Commit 5047cd1d5dea "xen/common: Use enhanced ASSERT_ALLOC_CONTEXT in
>> xmalloc()" extended the checks in _xmalloc() to catch any use of the
>> helpers from context with interrupts disabled.
>>
>> Unfortunately, the rule is not followed when allocating the CPU
>> sibling/core maps.
>>
>> (XEN) Xen call trace:
>> (XEN) [<00238a5c>] _xmalloc+0xfc/0x314 (PC)
>> (XEN) [<00000000>] 00000000 (LR)
>> (XEN) [<00238c8c>] _xzalloc+0x18/0x4c
>> (XEN) [<00288cb4>] smpboot.c#setup_cpu_sibling_map+0x38/0x138
>> (XEN) [<00289024>] start_secondary+0x1b4/0x270
>> (XEN) [<40010170>] 40010170
>> (XEN)
>> (XEN)
>> (XEN) ****************************************
>> (XEN) Panic on CPU 2:
>> (XEN) Assertion '!in_irq() && (local_irq_is_enabled() || num_online_cpus() <= 1)' failed at common/xmalloc_tlsf.c:601
>> (XEN) ****************************************
>>
>> This is happening because zalloc_cpumask_var() may allocate memory
>> if NR_CPUS is > 2 * sizeof(unsigned long).
>>
>> Avoid the problem by allocate the per-CPU IRQs while preparing the
>> CPU.
> Shouldn't this be "by allocating the CPU sibling/core maps while ..."
> to reflect the commit title and to distinguish between this change and the IRQ one?
Yes. I will update it.
[...]
>> static void remove_cpu_sibling_map(int cpu)
>> @@ -292,9 +294,14 @@ smp_get_max_cpus (void)
>> void __init
>> smp_prepare_cpus(void)
>> {
>> + int rc;
> Here you are leaving rc uninitialized (which is ok) but ...
>
>> +
>> cpumask_copy(&cpu_present_map, &cpu_possible_map);
>>
>> - setup_cpu_sibling_map(0);
>> + rc = setup_cpu_sibling_map(0);
>> + if ( rc )
>> + panic("Unable to allocate CPU sibling/core maps\n");
>> +
>> }
>>
>> /* Boot the current CPU */
>> @@ -361,8 +368,6 @@ void start_secondary(void)
>>
>> set_current(idle_vcpu[cpuid]);
>>
>> - setup_cpu_sibling_map(cpuid);
>> -
>> /* Run local notifiers */
>> notify_cpu_starting(cpuid);
>> /*
>> @@ -530,9 +535,19 @@ static int cpu_smpboot_callback(struct notifier_block *nfb,
>> void *hcpu)
>> {
>> unsigned int cpu = (unsigned long)hcpu;
>> + unsigned int rc = 0;
> ... here you are setting rc to 0 even though it will be reassigned.
> Furthermore, if rc is used only in case of CPU_UP_PREPARE, why not moving the definition there?
Because I forgot to replace "return NOTIFY_DONE;" with :
return !rc ? NOTIFY_DONE : notifier_from_errno(rc);
In this case, we would need to initialize rc to 0 so it doesn't get used
initialized.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/arm: smpboot: Allocate the CPU sibling/core maps while preparing the CPU
2022-06-14 11:08 ` Julien Grall
@ 2022-06-14 11:13 ` Michal Orzel
2022-06-15 0:23 ` Stefano Stabellini
0 siblings, 1 reply; 6+ messages in thread
From: Michal Orzel @ 2022-06-14 11:13 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: andrew.cooper3, Julien Grall, Stefano Stabellini,
Bertrand Marquis, Volodymyr Babchuk
On 14.06.2022 13:08, Julien Grall wrote:
>>> + unsigned int rc = 0;
>> ... here you are setting rc to 0 even though it will be reassigned.
>> Furthermore, if rc is used only in case of CPU_UP_PREPARE, why not moving the definition there?
>
> Because I forgot to replace "return NOTIFY_DONE;" with :
>
> return !rc ? NOTIFY_DONE : notifier_from_errno(rc);
That is what I thought.
With these fixes you can add my Rb.
>
> In this case, we would need to initialize rc to 0 so it doesn't get used initialized.
>
> Cheers,
>
Cheers,
Michal
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/arm: smpboot: Allocate the CPU sibling/core maps while preparing the CPU
2022-06-14 11:13 ` Michal Orzel
@ 2022-06-15 0:23 ` Stefano Stabellini
2022-06-22 17:57 ` Julien Grall
0 siblings, 1 reply; 6+ messages in thread
From: Stefano Stabellini @ 2022-06-15 0:23 UTC (permalink / raw)
To: Michal Orzel
Cc: Julien Grall, xen-devel, andrew.cooper3, Julien Grall,
Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
[-- Attachment #1: Type: text/plain, Size: 551 bytes --]
On Tue, 14 Jun 2022, Michal Orzel wrote:
> On 14.06.2022 13:08, Julien Grall wrote:
> >>> + unsigned int rc = 0;
> >> ... here you are setting rc to 0 even though it will be reassigned.
> >> Furthermore, if rc is used only in case of CPU_UP_PREPARE, why not moving the definition there?
> >
> > Because I forgot to replace "return NOTIFY_DONE;" with :
> >
> > return !rc ? NOTIFY_DONE : notifier_from_errno(rc);
> That is what I thought.
> With these fixes you can add my Rb.
And also my
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/arm: smpboot: Allocate the CPU sibling/core maps while preparing the CPU
2022-06-15 0:23 ` Stefano Stabellini
@ 2022-06-22 17:57 ` Julien Grall
0 siblings, 0 replies; 6+ messages in thread
From: Julien Grall @ 2022-06-22 17:57 UTC (permalink / raw)
To: Stefano Stabellini, Michal Orzel
Cc: xen-devel, andrew.cooper3, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
On 15/06/2022 01:23, Stefano Stabellini wrote:
> On Tue, 14 Jun 2022, Michal Orzel wrote:
>> On 14.06.2022 13:08, Julien Grall wrote:
>>>>> + unsigned int rc = 0;
>>>> ... here you are setting rc to 0 even though it will be reassigned.
>>>> Furthermore, if rc is used only in case of CPU_UP_PREPARE, why not moving the definition there?
>>>
>>> Because I forgot to replace "return NOTIFY_DONE;" with :
>>>
>>> return !rc ? NOTIFY_DONE : notifier_from_errno(rc);
>> That is what I thought.
>> With these fixes you can add my Rb.
>
> And also my
>
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Thanks. I have committed this patch.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-06-22 17:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-14 9:41 [PATCH] xen/arm: smpboot: Allocate the CPU sibling/core maps while preparing the CPU Julien Grall
2022-06-14 11:02 ` Michal Orzel
2022-06-14 11:08 ` Julien Grall
2022-06-14 11:13 ` Michal Orzel
2022-06-15 0:23 ` Stefano Stabellini
2022-06-22 17:57 ` Julien Grall
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.