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>
> 
> On 06.05.2022 14:03, Henry Wang wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@xxxxxxxx>
> >> Sent: Friday, May 6, 2022 6:33 PM
> >>
> >> 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,
> 
> xmem_pool_{alloc,free}() are what my comment was about. And "coverage"
> was meant as "if the assertions were there, more [potential] call sites
> would be covered". xmalloc_whole_pages(), as you ...
> 
> > 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?
> 
> ... validly note, is of no interest in this regard, as it's (1) a static
> helper and (2) would hit the checks in page_alloc.c.
> xmem_pool_{alloc,free}() otoh are non-static functions, so we will want
> to care about not only existing callers, but also potential future ones.

Great thanks for your explanation. This makes great sense, so I will add
ASSERT_ALLOC_CONTEXT in the beginning of xmem_pool_{alloc,free}().

Kind regards,
Henry

> 
> Jan