| Message ID | 20220614094119.94720-1-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: smpboot: Allocate the CPU sibling/core maps while preparing the CPU | expand |
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
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,
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
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>
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,
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;