| Message ID | 20220507025434.1063710-3-Henry.Wang@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Adjustment after introducing ASSERT_ALLOC_CONTEXT | expand |
Hi, On 07/05/2022 03:54, Henry Wang wrote: > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > index e866e0d864..ea59cd1a4a 100644 > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -162,13 +162,6 @@ > static char __initdata opt_badpage[100] = ""; > string_param("badpage", opt_badpage); > > -/* > - * Heap allocations may need TLB flushes which may require IRQs to be > - * enabled (except when only 1 PCPU is online). > - */ > -#define ASSERT_ALLOC_CONTEXT() \ > - ASSERT(!in_irq() && (local_irq_is_enabled() || num_online_cpus() <= 1)) > - FYI, the patch introducing ASSERT_ALLOC_CONTEXT() has been reverted. I intend to re-introduce it once your previous patch and the one fixing the ITS (not yet formally sent) have been committed. I have also checked that none of the ASSERTs() would be triggered on my x86 setup. So: Tested-by: Julien Grall <jgrall@amazon.com> Acked-by: Julien Grall <jgrall@amazon.com> On a side note (no action expected for you), I noticed that the ASSERT()s would only trigger from CPU2 and onwards at least for Arm. This is because num_online_cpus() would still be 1 when bringing-up CPU1. I went through the original discussion and I am not sure why we switched from < SYS_STATE_smp_boot to num_online_cpus() (aside that Arm doesn't set it). Aynway, this is not a major issue here as this is an ASSERT(). Cheers,
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > Subject: Re: [PATCH v3 2/2] xen/common: Use enhanced > ASSERT_ALLOC_CONTEXT in xmalloc() > > Hi, > > On 07/05/2022 03:54, Henry Wang wrote: > > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > > index e866e0d864..ea59cd1a4a 100644 > > --- a/xen/common/page_alloc.c > > +++ b/xen/common/page_alloc.c > > @@ -162,13 +162,6 @@ > > static char __initdata opt_badpage[100] = ""; > > string_param("badpage", opt_badpage); > > > > -/* > > - * Heap allocations may need TLB flushes which may require IRQs to be > > - * enabled (except when only 1 PCPU is online). > > - */ > > -#define ASSERT_ALLOC_CONTEXT() \ > > - ASSERT(!in_irq() && (local_irq_is_enabled() || num_online_cpus() <= > 1)) > > - > FYI, the patch introducing ASSERT_ALLOC_CONTEXT() has been reverted. I > intend to re-introduce it once your previous patch and the one fixing > the ITS (not yet formally sent) have been committed. Thanks for the information! IIUC the patch: "xen/arm: gic-v3-lpi: Allocate the pending table while preparing the CPU" is merged. So I guess both "page_alloc: assert IRQs are enabled in heap alloc/free" and this patch can be re-introduced if everyone is happy with the patch? > > I have also checked that none of the ASSERTs() would be triggered on my > x86 setup. So: > > Tested-by: Julien Grall <jgrall@amazon.com> > Acked-by: Julien Grall <jgrall@amazon.com> Thank you very much for both testing and acking this patch! Kind regards, Henry > > On a side note (no action expected for you), I noticed that the > ASSERT()s would only trigger from CPU2 and onwards at least for Arm. > This is because num_online_cpus() would still be 1 when bringing-up CPU1. > > I went through the original discussion and I am not sure why we switched > from < SYS_STATE_smp_boot to num_online_cpus() (aside that Arm > doesn't > set it). Aynway, this is not a major issue here as this is an ASSERT(). > > Cheers, > > -- > Julien Grall
Hi Henry, On 24/05/2022 02:53, Henry Wang wrote: >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Subject: Re: [PATCH v3 2/2] xen/common: Use enhanced >> ASSERT_ALLOC_CONTEXT in xmalloc() >> >> Hi, >> >> On 07/05/2022 03:54, Henry Wang wrote: >>> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c >>> index e866e0d864..ea59cd1a4a 100644 >>> --- a/xen/common/page_alloc.c >>> +++ b/xen/common/page_alloc.c >>> @@ -162,13 +162,6 @@ >>> static char __initdata opt_badpage[100] = ""; >>> string_param("badpage", opt_badpage); >>> >>> -/* >>> - * Heap allocations may need TLB flushes which may require IRQs to be >>> - * enabled (except when only 1 PCPU is online). >>> - */ >>> -#define ASSERT_ALLOC_CONTEXT() \ >>> - ASSERT(!in_irq() && (local_irq_is_enabled() || num_online_cpus() <= >> 1)) >>> - >> FYI, the patch introducing ASSERT_ALLOC_CONTEXT() has been reverted. I >> intend to re-introduce it once your previous patch and the one fixing >> the ITS (not yet formally sent) have been committed. > > Thanks for the information! IIUC the patch: > "xen/arm: gic-v3-lpi: Allocate the pending table while preparing the CPU" > is merged. So I guess both "page_alloc: assert IRQs are enabled in heap alloc/free" > and this patch can be re-introduced if everyone is happy with the patch? I have re-committed David's patch and committed yours. Cheers,
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index e866e0d864..ea59cd1a4a 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -162,13 +162,6 @@ static char __initdata opt_badpage[100] = ""; string_param("badpage", opt_badpage); -/* - * Heap allocations may need TLB flushes which may require IRQs to be - * enabled (except when only 1 PCPU is online). - */ -#define ASSERT_ALLOC_CONTEXT() \ - ASSERT(!in_irq() && (local_irq_is_enabled() || num_online_cpus() <= 1)) - /* * no-bootscrub -> Free pages are not zeroed during boot. */ diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c index d2ad909502..75bdf18c4e 100644 --- a/xen/common/xmalloc_tlsf.c +++ b/xen/common/xmalloc_tlsf.c @@ -378,6 +378,8 @@ void *xmem_pool_alloc(unsigned long size, struct xmem_pool *pool) int fl, sl; unsigned long tmp_size; + ASSERT_ALLOC_CONTEXT(); + if ( size < MIN_BLOCK_SIZE ) size = MIN_BLOCK_SIZE; else @@ -456,6 +458,8 @@ void xmem_pool_free(void *ptr, struct xmem_pool *pool) struct bhdr *b, *tmp_b; int fl = 0, sl = 0; + ASSERT_ALLOC_CONTEXT(); + if ( unlikely(ptr == NULL) ) return; @@ -594,7 +598,7 @@ void *_xmalloc(unsigned long size, unsigned long align) { void *p = NULL; - ASSERT(!in_irq()); + ASSERT_ALLOC_CONTEXT(); if ( !size ) return ZERO_BLOCK_PTR; @@ -697,11 +701,11 @@ void *_xrealloc(void *ptr, unsigned long size, unsigned long align) void xfree(void *p) { + ASSERT_ALLOC_CONTEXT(); + if ( p == NULL || p == ZERO_BLOCK_PTR ) return; - ASSERT(!in_irq()); - if ( !((unsigned long)p & (PAGE_SIZE - 1)) ) { unsigned long size = PFN_ORDER(virt_to_page(p)); diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h index d8beadd16b..300625e56d 100644 --- a/xen/include/xen/irq.h +++ b/xen/include/xen/irq.h @@ -10,6 +10,13 @@ #include <asm/hardirq.h> #include <public/event_channel.h> +/* + * Heap allocations may need TLB flushes which may require IRQs to be + * enabled (except when only 1 PCPU is online). + */ +#define ASSERT_ALLOC_CONTEXT() \ + ASSERT(!in_irq() && (local_irq_is_enabled() || num_online_cpus() <= 1)) + struct irqaction { void (*handler)(int, void *, struct cpu_user_regs *); const char *name;
xmalloc() will use a pool for allocation smaller than a page. The pool is extended only when there are no more space. At which point, alloc_xenheap_pages() is called to add more memory. xmalloc() must be protected by ASSERT_ALLOC_CONTEXT. It should not rely on pool expanding to trigger the ASSERT_ALLOC_CONTEXT in alloc_xenheap_pages(). Hence, this commit moves the definition of ASSERT_ALLOC_CONTEXT to header and uses the ASSERT_ALLOC_CONTEXT to replace the original assertion in xmalloc(). For consistency, the same assertion should be used in xfree(), and the position of the assertion should be at the beginning of the xfree(). Also take the opportunity to enhance the non-static functions xmem_pool_{alloc,free}() with the same assertion so that future callers of these two functions can be benefited. Reported-by: Wei Chen <Wei.Chen@arm.com> Suggested-by: Julien Grall <jgrall@amazon.com> Signed-off-by: Henry Wang <Henry.Wang@arm.com> --- v2 -> v3: - Add ASSERT_ALLOC_CONTEXT in xmem_pool_{alloc,free}() - Also change the assertion in xfree to ASSERT_ALLOC_CONTEXT and move the position of assertion to the beginning of the function. v1 -> v2: - No changes --- xen/common/page_alloc.c | 7 ------- xen/common/xmalloc_tlsf.c | 10 +++++++--- xen/include/xen/irq.h | 7 +++++++ 3 files changed, 14 insertions(+), 10 deletions(-)