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:12
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Julien Grall <julien.grall@xxxxxxx>; 
> Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>; Ian 
> Jackson
> <Ian.Jackson@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Konrad 
> Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>; Wei Liu <wl@xxxxxxx>
> Subject: Re: [PATCH v2 1/2] xmalloc: remove struct xmem_pool init_region
> 
> On 05.07.2019 11:02, Paul Durrant wrote:
> > This patch dispenses with the init_region. It's simply not necessary
> > (pools will still happily grow and shrink on demand in its absence) and the
> > code can be shortended by removing it. It also avoids the sole evaluation
> > of ADD_REGION without holding the pool lock (which is unsafe).
> 
> Oh, so you've figured there can be even more code removed than
> we first thought. Nice.
> 
> > @@ -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.

> 
> > @@ -380,14 +364,6 @@ void *xmem_pool_alloc(unsigned long size, struct 
> > xmem_pool *pool)
> >       int fl, sl;
> >       unsigned long tmp_size;
> >
> > -    if ( pool->init_region == NULL )
> > -    {
> > -        if ( (region = pool->get_mem(pool->init_size)) == NULL )
> > -            goto out;
> > -        ADD_REGION(region, pool->init_size, pool);
> > -        pool->init_region = region;
> > -    }
> 
> I.e. the code further down in this function turned out to not
> depend on there being at least one region in the pool, other
> than I was afraid it would. Good - even more pretty a change.

Nope. All the lists can start empty :-)

  Paul

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