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

[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.

Jan