From: Valentin Schneider <valentin.schneider@arm.com> To: Marc Zyngier <maz@kernel.org> Cc: linux-kernel@vger.kernel.org, linux-rt-users@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Will Deacon <will@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Thomas Gleixner <tglx@linutronix.de>, Sebastian Andrzej Siewior <bigeasy@linutronix.de>, Ard Biesheuvel <ardb@kernel.org> Subject: Re: [PATCH 2/3] irqchip/gic-v3-its: Postpone LPI pending table freeing and memreserve Date: Sun, 24 Oct 2021 16:51:53 +0100 [thread overview] Message-ID: <87a6iyju92.mognet@arm.com> (raw) In-Reply-To: <87pmrwt6l5.wl-maz@kernel.org> On 23/10/21 10:48, Marc Zyngier wrote: > On Fri, 22 Oct 2021 11:33:06 +0100, > Valentin Schneider <valentin.schneider@arm.com> wrote: >> @@ -5202,6 +5205,39 @@ int its_cpu_init(void) >> return 0; >> } >> >> +#ifdef CONFIG_EFI > > Why do we need this? I can't see anything that'd be problematic even > if EFI was disabled. > You're right, that's not required. >> +static int its_cpu_memreserve_lpi(unsigned int cpu) >> +{ >> + struct page *pend_page = gic_data_rdist()->pend_page; >> + phys_addr_t paddr; >> + >> + /* >> + * If the pending table was pre-programmed, free the memory we >> + * preemptively allocated. >> + */ >> + if (pend_page && >> + (gic_data_rdist()->flags & RDIST_FLAGS_PENDTABLE_PREALLOCATED)) { >> + its_free_pending_table(gic_data_rdist()->pend_page); >> + gic_data_rdist()->pend_page = NULL; >> + } > > So you set it to NULL and carry on, ending up reserving a 64kB block > at address 0 if the RESERVED flag isn't set. Can this happen at all? > If, as I suspect, it cannot happen because the two flags are always > set at the same time, why do we need two flags? > PREALLOCATED implies RESERVED, but the reverse isn't true. > My gut feeling is that if pend_page is non-NULL and that the RESERVED > flag is set, you should free the memory and leave the building. > Otherwise, reserve the memory and set the flag. PREALLOCATED doesn't > seem to make much sense on a per-CPU basis here. > One thing I was concerned about is that this cpuhp callback can be invoked more than once on the same CPU, even with the removal in patch 3. Consider a system with maxcpus=X on the cmdline; not all secondaries will be brought up in smp_init(). You then get to userspace which can issue all sorts of hotplug sequences. Let me try to paint a picture: maxcpus=2 CPU0 CPU1 CPU2 its_init() <nothing ever happens here> [...] its_cpu_memreserve_lpi() flags |= RESERVED [...] smp_init() its_cpu_memreserve_lpi() flags |= RESERVED [.....] cpu_down(CPU1, CPUHP_OFFLINE) cpu_up(CPU1, CPUHP_ONLINE) its_cpu_memreserve_lpi() // pend_page != NULL && (flags & RESERVED) != 0 // we musn't free the memory Now, my approach clearly isn't great (I also went through the "wait those two flags are the same thing" phase, which in hindsight wasn't a good sign). What we could do instead is only have a PREALLOCATED flag (or RESERVED; in any case just one rather than two) set in its_cpu_init_lpis(), and ensure each CPU only ever executes the body of the callback exactly once. if (already_booted()) return 0; if (PREALLOCATED) its_free_pending_table(); else gic_reserve_range(); out: // callback removal faff here return 0; Unfortunately, the boot CPU will already be present in cpus_booted_once_mask when this is first invoked for the BP, so AFAICT we'd need some new tracking utility (either a new RDIST_LOCAL flag or a separate cpumask). WDYT?
WARNING: multiple messages have this Message-ID (diff)
From: Valentin Schneider <valentin.schneider@arm.com> To: Marc Zyngier <maz@kernel.org> Cc: linux-kernel@vger.kernel.org, linux-rt-users@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Will Deacon <will@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Thomas Gleixner <tglx@linutronix.de>, Sebastian Andrzej Siewior <bigeasy@linutronix.de>, Ard Biesheuvel <ardb@kernel.org> Subject: Re: [PATCH 2/3] irqchip/gic-v3-its: Postpone LPI pending table freeing and memreserve Date: Sun, 24 Oct 2021 16:51:53 +0100 [thread overview] Message-ID: <87a6iyju92.mognet@arm.com> (raw) In-Reply-To: <87pmrwt6l5.wl-maz@kernel.org> On 23/10/21 10:48, Marc Zyngier wrote: > On Fri, 22 Oct 2021 11:33:06 +0100, > Valentin Schneider <valentin.schneider@arm.com> wrote: >> @@ -5202,6 +5205,39 @@ int its_cpu_init(void) >> return 0; >> } >> >> +#ifdef CONFIG_EFI > > Why do we need this? I can't see anything that'd be problematic even > if EFI was disabled. > You're right, that's not required. >> +static int its_cpu_memreserve_lpi(unsigned int cpu) >> +{ >> + struct page *pend_page = gic_data_rdist()->pend_page; >> + phys_addr_t paddr; >> + >> + /* >> + * If the pending table was pre-programmed, free the memory we >> + * preemptively allocated. >> + */ >> + if (pend_page && >> + (gic_data_rdist()->flags & RDIST_FLAGS_PENDTABLE_PREALLOCATED)) { >> + its_free_pending_table(gic_data_rdist()->pend_page); >> + gic_data_rdist()->pend_page = NULL; >> + } > > So you set it to NULL and carry on, ending up reserving a 64kB block > at address 0 if the RESERVED flag isn't set. Can this happen at all? > If, as I suspect, it cannot happen because the two flags are always > set at the same time, why do we need two flags? > PREALLOCATED implies RESERVED, but the reverse isn't true. > My gut feeling is that if pend_page is non-NULL and that the RESERVED > flag is set, you should free the memory and leave the building. > Otherwise, reserve the memory and set the flag. PREALLOCATED doesn't > seem to make much sense on a per-CPU basis here. > One thing I was concerned about is that this cpuhp callback can be invoked more than once on the same CPU, even with the removal in patch 3. Consider a system with maxcpus=X on the cmdline; not all secondaries will be brought up in smp_init(). You then get to userspace which can issue all sorts of hotplug sequences. Let me try to paint a picture: maxcpus=2 CPU0 CPU1 CPU2 its_init() <nothing ever happens here> [...] its_cpu_memreserve_lpi() flags |= RESERVED [...] smp_init() its_cpu_memreserve_lpi() flags |= RESERVED [.....] cpu_down(CPU1, CPUHP_OFFLINE) cpu_up(CPU1, CPUHP_ONLINE) its_cpu_memreserve_lpi() // pend_page != NULL && (flags & RESERVED) != 0 // we musn't free the memory Now, my approach clearly isn't great (I also went through the "wait those two flags are the same thing" phase, which in hindsight wasn't a good sign). What we could do instead is only have a PREALLOCATED flag (or RESERVED; in any case just one rather than two) set in its_cpu_init_lpis(), and ensure each CPU only ever executes the body of the callback exactly once. if (already_booted()) return 0; if (PREALLOCATED) its_free_pending_table(); else gic_reserve_range(); out: // callback removal faff here return 0; Unfortunately, the boot CPU will already be present in cpus_booted_once_mask when this is first invoked for the BP, so AFAICT we'd need some new tracking utility (either a new RDIST_LOCAL flag or a separate cpumask). WDYT? _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-10-24 15:52 UTC|newest] Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-10-22 10:33 [PATCH 0/3] irqchip/gic-v3-its: Fix LPI pending table handling vs PREEMPT_RT Valentin Schneider 2021-10-22 10:33 ` Valentin Schneider 2021-10-22 10:33 ` [PATCH 1/3] irqchip/gic-v3-its: Give the percpu rdist struct its own flags field Valentin Schneider 2021-10-22 10:33 ` Valentin Schneider 2021-10-23 9:10 ` Marc Zyngier 2021-10-23 9:10 ` Marc Zyngier 2021-10-24 15:50 ` Valentin Schneider 2021-10-24 15:50 ` Valentin Schneider 2021-10-22 10:33 ` [PATCH 2/3] irqchip/gic-v3-its: Postpone LPI pending table freeing and memreserve Valentin Schneider 2021-10-22 10:33 ` Valentin Schneider 2021-10-23 9:48 ` Marc Zyngier 2021-10-23 9:48 ` Marc Zyngier 2021-10-24 15:51 ` Valentin Schneider [this message] 2021-10-24 15:51 ` Valentin Schneider 2021-10-25 11:57 ` Marc Zyngier 2021-10-25 11:57 ` Marc Zyngier 2021-10-22 10:33 ` [PATCH 3/3] irqchip/gic-v3-its: Limit memreserve cpuhp state lifetime Valentin Schneider 2021-10-22 10:33 ` Valentin Schneider 2021-10-23 10:37 ` Marc Zyngier 2021-10-23 10:37 ` Marc Zyngier 2021-10-24 15:52 ` Valentin Schneider 2021-10-24 15:52 ` Valentin Schneider
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87a6iyju92.mognet@arm.com \ --to=valentin.schneider@arm.com \ --cc=ardb@kernel.org \ --cc=bigeasy@linutronix.de \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-rt-users@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=maz@kernel.org \ --cc=tglx@linutronix.de \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.