RE: [PATCH v2 2/2] xen/common: Use enhanced ASSERT_ALLOC_CONTEXT in xmalloc()

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Friday, May 6, 2022 6:33 PM
> To: Henry Wang <Henry.Wang@xxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Julien Grall 
> <julien@xxxxxxx>;
> Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxx>;
> Wei Liu <wl@xxxxxxx>; Wei Chen <Wei.Chen@xxxxxxx>; Julien Grall
> <jgrall@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 2/2] xen/common: Use enhanced
> ASSERT_ALLOC_CONTEXT in xmalloc()
> 
> On 06.05.2022 07:52, Henry Wang wrote:
> > --- 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();
> 
> For one - what about xfree()?

Oh you are definitely correct, thanks for pointing this out. I will
definitely change the assertion in xfree() as well in v3.

> 
> And then did you consider taking the opportunity and moving both to
> the respective pool alloc functions, thus giving even better coverage?

Yeah I would love to. But sorry about the question (just for learning):
I assume you are talking about code coverage, could you please kindly
add a little bit more detail to help me understand why adding the same
ASSERT_ALLOC_CONTEXT would help to a better coverage? Since...

> Granted there's one downside to moving it to xmem_pool_alloc(): Then
> the early zero-size and error returns won't be covered, so maybe we
> actually want checks in both places. 

...after reading these I have a feeling that we need to add the same
ASSERT_ALLOC_CONTEXT in the beginning of the xmem_pool_alloc,
xmalloc_whole_pages, and xmem_pool_free, while keeping
ASSERT_ALLOC_CONTEXT in _xmalloc. I think xmem_pool_alloc and
xmalloc_whole_pages are only called in _xmalloc and xmem_pool_free
is only called in xfree. Adding the same assertion in these three functions
is duplication of code?

Thank you for your explanation and patience.

> In xfree() I think the check
> would then also want to move ahead of the early return.

Sure, will do as you suggested.

Kind regards,
Henry

> 
> Jan