| Message ID | 20220505025407.919988-3-Henry.Wang@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Adjustment after introducing ASSERT_ALLOC_CONTEXT | expand |
On 05.05.2022 04:54, Henry Wang wrote: > 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(). > > Reported-by: Wei Chen <Wei.Chen@arm.com> > Suggested-by: Julien Grall <jgrall@amazon.com> > Signed-off-by: Henry Wang <Henry.Wang@arm.com> > Change-Id: Ia463d2241e80e8a78d7dbb5b2318694d3ca5ed67 Just two formal remarks for starters: What's this last tag? And why am I on the To: list of this patch, when ... > --- > xen/common/page_alloc.c | 7 ------- > xen/common/xmalloc_tlsf.c | 2 +- > xen/include/xen/irq.h | 7 +++++++ > 3 files changed, 8 insertions(+), 8 deletions(-) ... with this diffstat you should instead have _Cc_-ed REST maintainers? Jan
Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > > Change-Id: Ia463d2241e80e8a78d7dbb5b2318694d3ca5ed67 > > Just two formal remarks for starters: What's this last tag? And why am > I on the To: list of this patch, when ... I forgot to remove the Change-Id before sending the patch, sorry about that. This will be removed in v2. > > > --- > > xen/common/page_alloc.c | 7 ------- > > xen/common/xmalloc_tlsf.c | 2 +- > > xen/include/xen/irq.h | 7 +++++++ > > 3 files changed, 8 insertions(+), 8 deletions(-) > > ... with this diffstat you should instead have _Cc_-ed REST maintainers? I think this is because by switching to add-maintainers.pl I didn't change my git send-email command line where originally it has a --suppress-cc=all. Since you are the maintainer of common code I thought I should directly "To" you. Kind regards, Henry > > Jan
On 05.05.2022 09:26, Henry Wang wrote: >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >>> Change-Id: Ia463d2241e80e8a78d7dbb5b2318694d3ca5ed67 >> >> Just two formal remarks for starters: What's this last tag? And why am >> I on the To: list of this patch, when ... > > I forgot to remove the Change-Id before sending the patch, sorry about that. > This will be removed in v2. > >> >>> --- >>> xen/common/page_alloc.c | 7 ------- >>> xen/common/xmalloc_tlsf.c | 2 +- >>> xen/include/xen/irq.h | 7 +++++++ >>> 3 files changed, 8 insertions(+), 8 deletions(-) >> >> ... with this diffstat you should instead have _Cc_-ed REST maintainers? > > I think this is because by switching to add-maintainers.pl I didn't change > my git send-email command line where originally it has a --suppress-cc=all. > Since you are the maintainer of common code I thought I should directly > "To" you. I'm sorry, but no: For one I am not "the" maintainer, but one of several. And then patches are generally only sent _To_ the list, with _Cc_ to maintainers. Jan
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..b8f838ae74 100644 --- a/xen/common/xmalloc_tlsf.c +++ b/xen/common/xmalloc_tlsf.c @@ -594,7 +594,7 @@ void *_xmalloc(unsigned long size, unsigned long align) { void *p = NULL; - ASSERT(!in_irq()); + ASSERT_ALLOC_CONTEXT(); if ( !size ) return ZERO_BLOCK_PTR; 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(). Reported-by: Wei Chen <Wei.Chen@arm.com> Suggested-by: Julien Grall <jgrall@amazon.com> Signed-off-by: Henry Wang <Henry.Wang@arm.com> Change-Id: Ia463d2241e80e8a78d7dbb5b2318694d3ca5ed67 --- xen/common/page_alloc.c | 7 ------- xen/common/xmalloc_tlsf.c | 2 +- xen/include/xen/irq.h | 7 +++++++ 3 files changed, 8 insertions(+), 8 deletions(-)