| Message ID | 20190705090249.1935-2-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xmalloc patches | expand |
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. > @@ -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. Jan
> -----Original Message----- > From: Jan Beulich <JBeulich@suse.com> > Sent: 05 July 2019 13:12 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: xen-devel@lists.xenproject.org; Julien Grall <julien.grall@arm.com>; Andrew Cooper > <Andrew.Cooper3@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Ian Jackson > <Ian.Jackson@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; Konrad Rzeszutek Wilk > <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>; Wei Liu <wl@xen.org> > 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
On 05.07.2019 14:18, Paul Durrant wrote: >> From: Jan Beulich <JBeulich@suse.com> >> 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@suse.com> Jan
> -----Original Message----- > From: Jan Beulich <JBeulich@suse.com> > Sent: 05 July 2019 13:54 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: JulienGrall <julien.grall@arm.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap > <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Stefano Stabellini > <sstabellini@kernel.org>; xen-devel@lists.xenproject.org; KonradRzeszutek Wilk > <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>; WeiLiu <wl@xen.org> > 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@suse.com> > >> 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@suse.com> 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
diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c index f585388dfa..e4e476a27c 100644 --- a/xen/common/xmalloc_tlsf.c +++ b/xen/common/xmalloc_tlsf.c @@ -101,7 +101,6 @@ struct xmem_pool { spinlock_t lock; - unsigned long init_size; unsigned long max_size; unsigned long grow_size; @@ -115,7 +114,6 @@ struct xmem_pool { struct list_head list; - void *init_region; char name[MAX_POOL_NAME_LEN]; }; @@ -287,14 +285,13 @@ struct xmem_pool *xmem_pool_create( const char *name, xmem_pool_get_memory get_mem, xmem_pool_put_memory put_mem, - unsigned long init_size, unsigned long max_size, unsigned long grow_size) { struct xmem_pool *pool; int pool_bytes, pool_order; - BUG_ON(max_size && (max_size < init_size)); + BUG_ON(max_size && (max_size < grow_size)); pool_bytes = ROUNDUP_SIZE(sizeof(*pool)); pool_order = get_order_from_bytes(pool_bytes); @@ -305,23 +302,18 @@ struct xmem_pool *xmem_pool_create( memset(pool, 0, pool_bytes); /* Round to next page boundary */ - init_size = ROUNDUP_PAGE(init_size); max_size = ROUNDUP_PAGE(max_size); grow_size = ROUNDUP_PAGE(grow_size); /* pool global overhead not included in used size */ pool->used_size = 0; - pool->init_size = init_size; pool->max_size = max_size; pool->grow_size = grow_size; pool->get_mem = get_mem; pool->put_mem = put_mem; strlcpy(pool->name, name, sizeof(pool->name)); - /* always obtain init_region lazily now to ensure it is get_mem'd - * in the same "context" as all other regions */ - spin_lock_init(&pool->lock); spin_lock(&pool_list_lock); @@ -340,7 +332,6 @@ unsigned long xmem_pool_get_total_size(struct xmem_pool *pool) { unsigned long total; total = ROUNDUP_SIZE(sizeof(*pool)) - + pool->init_size + (pool->num_regions - 1) * pool->grow_size; return total; } @@ -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; - } - /* Check for memory leaks in this pool */ if ( xmem_pool_get_used_size(pool) ) printk("memory leak in pool: %s (%p). " @@ -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; - } - size = (size < MIN_BLOCK_SIZE) ? MIN_BLOCK_SIZE : ROUNDUP_SIZE(size); /* Rounding up the requested size and calculating fl and sl */ @@ -401,8 +377,7 @@ void *xmem_pool_alloc(unsigned long size, struct xmem_pool *pool) /* Not found */ if ( size > (pool->grow_size - 2 * BHDR_OVERHEAD) ) goto out_locked; - if ( pool->max_size && (pool->init_size + - pool->num_regions * pool->grow_size + if ( pool->max_size && (pool->num_regions * pool->grow_size > pool->max_size) ) goto out_locked; spin_unlock(&pool->lock); @@ -551,9 +526,8 @@ static void *xmalloc_whole_pages(unsigned long size, unsigned long align) static void tlsf_init(void) { - xenpool = xmem_pool_create( - "xmalloc", xmalloc_pool_get, xmalloc_pool_put, - PAGE_SIZE, 0, PAGE_SIZE); + xenpool = xmem_pool_create("xmalloc", xmalloc_pool_get, + xmalloc_pool_put, 0, PAGE_SIZE); BUG_ON(!xenpool); } diff --git a/xen/include/xen/xmalloc.h b/xen/include/xen/xmalloc.h index b486fe4b06..f075d2da91 100644 --- a/xen/include/xen/xmalloc.h +++ b/xen/include/xen/xmalloc.h @@ -84,7 +84,6 @@ typedef void (xmem_pool_put_memory)(void *ptr); * @name: name of the pool * @get_mem: callback function used to expand pool * @put_mem: callback function used to shrink pool - * @init_size: inital pool size (in bytes) * @max_size: maximum pool size (in bytes) - set this as 0 for no limit * @grow_size: amount of memory (in bytes) added to pool whenever required * @@ -94,7 +93,6 @@ struct xmem_pool *xmem_pool_create( const char *name, xmem_pool_get_memory get_mem, xmem_pool_put_memory put_mem, - unsigned long init_size, unsigned long max_size, unsigned long grow_size);
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). Signed-off-by: Paul Durrant <paul.durrant@citrix.com> Suggested-by: Jan Beulich <jbeulich@suse.com> --- Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Julien Grall <julien.grall@arm.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Tim Deegan <tim@xen.org> Cc: Wei Liu <wl@xen.org> v2: - remove init_region instead of fixing the locking --- xen/common/xmalloc_tlsf.c | 34 ++++------------------------------ xen/include/xen/xmalloc.h | 2 -- 2 files changed, 4 insertions(+), 32 deletions(-)