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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <JBeulich@xxxxxxxx>
> Sent: 05 July 2019 13:54
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: JulienGrall <julien.grall@xxxxxxx>; Andrew Cooper 
> <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Stefano 
> Stabellini
> <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx; KonradRzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>; WeiLiu <wl@xxxxxxx>
> Subject: Re: [PATCH v2 1/2] xmalloc: remove struct xmem_pool init_region
> 
> 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.

Oh yes, I was completely blind to that '!' as it only made sense to me the 
other way round.

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

Yes, I think it was indeed totally bogus.

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

The code is over a decade old according to the boilerplate at the top so 
probably not surprising.

> 
> 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?

Yes, hence me getting fooled into thinking this was because because init_region 
was not freed.

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

Oh... another casualty.

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

Thanks. If you want me to re-work the comment then let me know, otherwise I'm 
totally happy for you to do it on commit.

  Paul

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