Re: [Xen-devel] [PATCH v2 1/2] xmalloc: remove struct xmem_pool init_region

[Jan Beulich <JBeulich@xxxxxxxx>]

On 05.07.2019 14:18, Paul Durrant wrote:
>> From: Jan Beulich <JBeulich@xxxxxxxx>
>> Sent: 05 July 2019 13:12
>>
>> On 05.07.2019 11:02, Paul Durrant wrote:
>>> @@ -352,13 +343,6 @@ void xmem_pool_destroy(struct xmem_pool *pool)
>>>        if ( pool == NULL )
>>>            return;
>>>
>>> -    /* User is destroying without ever allocating from this pool */
>>> -    if ( xmem_pool_get_used_size(pool) == BHDR_OVERHEAD )
>>> -    {
>>> -        ASSERT(!pool->init_region);
>>> -        pool->used_size -= BHDR_OVERHEAD;
>>> -    }
>>
>> I can see that the ASSERT() can (and needs to) go away. But I
>> don't think you've changed anything elsewhere that would also
>> allow the entire if() to go away.
> 
> I think so. AFAICT the size check against BHDR_OVERHEAD is entirely
> to avoid reporting presence of the init_region as a leak. I.e. in
> the presence of an init_region, the used_size would never drop
> below BHDR_OVERHEAD. Without an init_region, used_size should drop
> all the way (back) to 0 when the last xfree() is done.

But the old code asserted that there was _no_ init region, and then
reduced pool->used_size. And the state of the pool when there is no
init region doesn't change with your patch. If anything this if()
was bogus altogether, which I think was the case now that I've
looked more closely at how / when ->used_size gets updated. I would
have wanted to check the original code (which ours was cloned from)
to see whether they've changed this piece at some time, but the site
doesn't seem to be properly working (anymore).

Do you agree that ->used_size could equal BHDR_OVERHEAD only when
there's exactly one region, and when that one region has no
outstanding allocations? Seeing the region removal in
xmem_pool_free() I also can't seem to see how the init region
would have been excluded from getting freed here, at which point
asserting whether there is (ever was) one looks even more fishy.

Actually there's a perhaps telling comment in xmem_pool_create():

     /* always obtain init_region lazily now to ensure it is get_mem'd
      * in the same "context" as all other regions */

This suggests to me that originally the init region was set up right
here, at which point that assertion would have made sense. And there
we go - I'm not at all surprised that this stems from 6009f4ddb2
('Transcendent memory ("tmem") for Xen'), with no mention at all in
the commit message as to why the allocator needed to be changed.

IOW - as long as you agree with the analysis, and as long as a
reference to the above commit rendering stale that entire if() gets
added to the description (which may still be reasonable to do while
committing):

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel