| Message ID | 20211022103307.1711619-4-valentin.schneider@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | irqchip/gic-v3-its: Fix LPI pending table handling vs PREEMPT_RT | expand |
On Fri, 22 Oct 2021 11:33:07 +0100, Valentin Schneider <valentin.schneider@arm.com> wrote: > > The new memreserve cpuhp callback only needs to survive up until a point > where every CPU in the system has booted once. Beyond that, it becomes a > no-op and can be put in the bin. > > Signed-off-by: Valentin Schneider <valentin.schneider@arm.com> > --- > drivers/irqchip/irq-gic-v3-its.c | 23 ++++++++++++++++++++--- > include/linux/irqchip/arm-gic-v3.h | 1 + > 2 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c > index a6a4af59205e..4ae9ae6b90fe 100644 > --- a/drivers/irqchip/irq-gic-v3-its.c > +++ b/drivers/irqchip/irq-gic-v3-its.c > @@ -5206,6 +5206,15 @@ int its_cpu_init(void) > } > > #ifdef CONFIG_EFI > +static void rdist_memreserve_cpuhp_cleanup_workfn(struct work_struct *work) > +{ > + cpuhp_remove_state_nocalls(gic_rdists->cpuhp_memreserve_state); > + gic_rdists->cpuhp_memreserve_state = CPUHP_INVALID; > +} > + > +static DECLARE_WORK(rdist_memreserve_cpuhp_cleanup_work, > + rdist_memreserve_cpuhp_cleanup_workfn); > + > static int its_cpu_memreserve_lpi(unsigned int cpu) > { > struct page *pend_page = gic_data_rdist()->pend_page; > @@ -5226,7 +5235,7 @@ static int its_cpu_memreserve_lpi(unsigned int cpu) > * invocation of this callback, or in a previous life before kexec. > */ > if (gic_data_rdist()->flags & RDIST_FLAGS_PENDTABLE_RESERVED) > - return 0; > + goto out; > > gic_data_rdist()->flags |= RDIST_FLAGS_PENDTABLE_RESERVED; > > @@ -5234,6 +5243,11 @@ static int its_cpu_memreserve_lpi(unsigned int cpu) > paddr = page_to_phys(pend_page); > WARN_ON(gic_reserve_range(paddr, LPI_PENDBASE_SZ)); > > +out: > + /* This only needs to run once per CPU */ > + if (cpumask_equal(&cpus_booted_once_mask, cpu_possible_mask)) > + schedule_work(&rdist_memreserve_cpuhp_cleanup_work); Which makes me wonder. Do we actually need any flag at all if all we need to check is whether the CPU has been through the callback at least once? I have the strong feeling that we are tracking the same state multiple times here. Also, could the cpuhp callbacks ever run concurrently? If they could, two CPUs could schedule the cleanup work in parallel, with interesting results. You'd need a cmpxchg on the cpuhp state in the workfn. > + > return 0; > } > #endif > @@ -5421,13 +5435,14 @@ static void __init its_acpi_probe(void) > static void __init its_acpi_probe(void) { } > #endif > > -static int __init its_lpi_memreserve_init(void) > +static int __init its_lpi_memreserve_init(struct rdists *rdists) > { > int state; > > if (!efi_enabled(EFI_CONFIG_TABLES)) > return 0; > > + rdists->cpuhp_memreserve_state = CPUHP_INVALID; > state = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, > "irqchip/arm/gicv3/memreserve:online", > its_cpu_memreserve_lpi, > @@ -5435,6 +5450,8 @@ static int __init its_lpi_memreserve_init(void) > if (state < 0) > return state; > > + rdists->cpuhp_memreserve_state = state; > + > return 0; > } > > @@ -5465,7 +5482,7 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists, > if (err) > return err; > > - err = its_lpi_memreserve_init(); > + err = its_lpi_memreserve_init(rdists); > if (err) > return err; > > diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h > index 0dc34d7d735a..95479b315918 100644 > --- a/include/linux/irqchip/arm-gic-v3.h > +++ b/include/linux/irqchip/arm-gic-v3.h > @@ -624,6 +624,7 @@ struct rdists { > u64 flags; > u32 gicd_typer; > u32 gicd_typer2; > + int cpuhp_memreserve_state; > bool has_vlpis; > bool has_rvpeid; > bool has_direct_lpi; Thanks, M.
On 23/10/21 11:37, Marc Zyngier wrote: > On Fri, 22 Oct 2021 11:33:07 +0100, > Valentin Schneider <valentin.schneider@arm.com> wrote: >> @@ -5234,6 +5243,11 @@ static int its_cpu_memreserve_lpi(unsigned int cpu) >> paddr = page_to_phys(pend_page); >> WARN_ON(gic_reserve_range(paddr, LPI_PENDBASE_SZ)); >> >> +out: >> + /* This only needs to run once per CPU */ >> + if (cpumask_equal(&cpus_booted_once_mask, cpu_possible_mask)) >> + schedule_work(&rdist_memreserve_cpuhp_cleanup_work); > > Which makes me wonder. Do we actually need any flag at all if all we > need to check is whether the CPU has been through the callback at > least once? I have the strong feeling that we are tracking the same > state multiple times here. > Agreed, cf. my reply on 2/3. > Also, could the cpuhp callbacks ever run concurrently? If they could, > two CPUs could schedule the cleanup work in parallel, with interesting > results. You'd need a cmpxchg on the cpuhp state in the workfn. > So I think the cpuhp callbacks may run concurrently, but at a quick glance it seems like we can't get two instances of the same work executing concurrently: schedule_work()->queue_work() doesn't re-queue a work if it's already pending, and __queue_work() checks a work's previous pool in case it might still be running there. Regardless, that's one less thing to worry about if we make the cpuhp callback body run at most once on each CPU (only a single CPU will be able to queue the removal work).
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c index a6a4af59205e..4ae9ae6b90fe 100644 --- a/drivers/irqchip/irq-gic-v3-its.c +++ b/drivers/irqchip/irq-gic-v3-its.c @@ -5206,6 +5206,15 @@ int its_cpu_init(void) } #ifdef CONFIG_EFI +static void rdist_memreserve_cpuhp_cleanup_workfn(struct work_struct *work) +{ + cpuhp_remove_state_nocalls(gic_rdists->cpuhp_memreserve_state); + gic_rdists->cpuhp_memreserve_state = CPUHP_INVALID; +} + +static DECLARE_WORK(rdist_memreserve_cpuhp_cleanup_work, + rdist_memreserve_cpuhp_cleanup_workfn); + static int its_cpu_memreserve_lpi(unsigned int cpu) { struct page *pend_page = gic_data_rdist()->pend_page; @@ -5226,7 +5235,7 @@ static int its_cpu_memreserve_lpi(unsigned int cpu) * invocation of this callback, or in a previous life before kexec. */ if (gic_data_rdist()->flags & RDIST_FLAGS_PENDTABLE_RESERVED) - return 0; + goto out; gic_data_rdist()->flags |= RDIST_FLAGS_PENDTABLE_RESERVED; @@ -5234,6 +5243,11 @@ static int its_cpu_memreserve_lpi(unsigned int cpu) paddr = page_to_phys(pend_page); WARN_ON(gic_reserve_range(paddr, LPI_PENDBASE_SZ)); +out: + /* This only needs to run once per CPU */ + if (cpumask_equal(&cpus_booted_once_mask, cpu_possible_mask)) + schedule_work(&rdist_memreserve_cpuhp_cleanup_work); + return 0; } #endif @@ -5421,13 +5435,14 @@ static void __init its_acpi_probe(void) static void __init its_acpi_probe(void) { } #endif -static int __init its_lpi_memreserve_init(void) +static int __init its_lpi_memreserve_init(struct rdists *rdists) { int state; if (!efi_enabled(EFI_CONFIG_TABLES)) return 0; + rdists->cpuhp_memreserve_state = CPUHP_INVALID; state = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "irqchip/arm/gicv3/memreserve:online", its_cpu_memreserve_lpi, @@ -5435,6 +5450,8 @@ static int __init its_lpi_memreserve_init(void) if (state < 0) return state; + rdists->cpuhp_memreserve_state = state; + return 0; } @@ -5465,7 +5482,7 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists, if (err) return err; - err = its_lpi_memreserve_init(); + err = its_lpi_memreserve_init(rdists); if (err) return err; diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h index 0dc34d7d735a..95479b315918 100644 --- a/include/linux/irqchip/arm-gic-v3.h +++ b/include/linux/irqchip/arm-gic-v3.h @@ -624,6 +624,7 @@ struct rdists { u64 flags; u32 gicd_typer; u32 gicd_typer2; + int cpuhp_memreserve_state; bool has_vlpis; bool has_rvpeid; bool has_direct_lpi;
The new memreserve cpuhp callback only needs to survive up until a point where every CPU in the system has booted once. Beyond that, it becomes a no-op and can be put in the bin. Signed-off-by: Valentin Schneider <valentin.schneider@arm.com> --- drivers/irqchip/irq-gic-v3-its.c | 23 ++++++++++++++++++++--- include/linux/irqchip/arm-gic-v3.h | 1 + 2 files changed, 21 insertions(+), 3 deletions(-)