RE: [PATCH 6/9] mm/z3fold: move decrement of pool->pages_nr into __release_z3fold_page()

[David Laight <David.Laight@ACULAB.COM>]

From: Miaohe Lin <linmiaohe@huawei.com>
> Sent: 21 February 2022 02:53
> 
> On 2022/2/20 0:33, David Laight wrote:
> > From: Miaohe Lin
> >> Sent: 19 February 2022 09:26
> >>
> >> The z3fold will always do atomic64_dec(&pool->pages_nr) when the
> >> __release_z3fold_page() is called. Thus we can move decrement of
> >> pool->pages_nr into __release_z3fold_page() to simplify the code.
> >> Also we can reduce the size of z3fold.o ~1k.
> >> Without this patch:
> >>    text	   data	    bss	    dec	    hex	filename
> >>   15444	   1376	      8	  16828	   41bc	mm/z3fold.o
> >> With this patch:
> >>    text	   data	    bss	    dec	    hex	filename
> >>   15044	   1248	      8	  16300	   3fac	mm/z3fold.o
> >
> > I can't see anything obvious in this patch that would reduce the size much.
> > OTOH there are some large functions that are pointlessly marked 'inline'.
> > Maybe the compiler made a better choice?
> 
> I think so too.
> 
> > Although it isn't al all obvious why the 'data' size changes.
> 
> I checked the header of z3fold.o. The size of .data is unchanged while
> align is changed from 00003818 to 00003688. Maybe this is the reason
> .data size changes.

You are misreading the double line header.
If is Offset that is changing, Align in 8 (as expected).

It will be another section that gets added to the 'data' size
reported by 'size'.

> 
> Section Headers:
>   [Nr] Name              Type             Address           Offset
>        Size              EntSize          Flags  Link  Info  Align
> 
> with this patch:
> [ 3] .data             PROGBITS         0000000000000000  00003688
>        00000000000000c0  0000000000000000  WA       0     0     8
> 
> without this patch:
> [ 3] .data             PROGBITS         0000000000000000  00003818
>        00000000000000c0  0000000000000000  WA       0     0     8
> 
> >
> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> >> ---
> >>  mm/z3fold.c | 41 ++++++++++++-----------------------------
> >>  1 file changed, 12 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/mm/z3fold.c b/mm/z3fold.c
> >> index adc0b3fa4906..18a697f6fe32 100644
> >> --- a/mm/z3fold.c
> >> +++ b/mm/z3fold.c
> >> @@ -520,6 +520,8 @@ static void __release_z3fold_page(struct z3fold_header *zhdr, bool locked)
> >>  	list_add(&zhdr->buddy, &pool->stale);
> >>  	queue_work(pool->release_wq, &pool->work);
> >>  	spin_unlock(&pool->stale_lock);
> >> +
> >> +	atomic64_dec(&pool->pages_nr);
> >
> > Looks like you can move the decrement inside the lock.
> > If you can do the same for the increment you can avoid the
> > expensive locked bus cycle.
> >
> 
> atomic64_inc(&pool->pages_nr); is only done when init a new or reused z3fold_page.
> There is no lock around. If we hold pool->lock there, this potential gain might be
> nullified. Or am I miss something ?

Atomic operations aren't magic.
Atomic operations are (at best) one slow locked bus cycle.
Acquiring a lock is the same.
Releasing a lock might be cheaper, but is probably a locked bus cycle.

So if you use state_lock to protect pages_nr then you lose an atomic
operation for the decrement and gain one (for the unlock) in the increment.
That is even or maybe a slight gain.
OTOH a 64bit atomic is a PITA on some 32bit systems.
(In fact any atomic is a PITA on sparc32.)

Actually does this even need to be 64bit, should it just be 'long'.
That will mean that any 'read' just needs a simple single memory read.

I've just looked at the code.
Some of the one line wrapper functions don't make the code any
easier to read.
There is no point having inline wrappers to acquire locks if you
only use them some of the time.

	David


> 
> Many thanks for your review and reply.
> 
> > 	David
> >
> >>  }
> >>
> >>  static void release_z3fold_page(struct kref *ref)
> >> @@ -737,13 +739,9 @@ static struct z3fold_header *compact_single_buddy(struct z3fold_header *zhdr)
> >>  	return new_zhdr;
> >>
> >>  out_fail:
> >> -	if (new_zhdr) {
> >> -		if (kref_put(&new_zhdr->refcount, release_z3fold_page_locked))
> >> -			atomic64_dec(&pool->pages_nr);
> >> -		else {
> >> -			add_to_unbuddied(pool, new_zhdr);
> >> -			z3fold_page_unlock(new_zhdr);
> >> -		}
> >> +	if (new_zhdr && !kref_put(&new_zhdr->refcount, release_z3fold_page_locked)) {
> >> +		add_to_unbuddied(pool, new_zhdr);
> >> +		z3fold_page_unlock(new_zhdr);
> >>  	}
> >>  	return NULL;
> >>
> >> @@ -816,10 +814,8 @@ static void do_compact_page(struct z3fold_header *zhdr, bool locked)
> >>  	list_del_init(&zhdr->buddy);
> >>  	spin_unlock(&pool->lock);
> >>
> >> -	if (kref_put(&zhdr->refcount, release_z3fold_page_locked)) {
> >> -		atomic64_dec(&pool->pages_nr);
> >> +	if (kref_put(&zhdr->refcount, release_z3fold_page_locked))
> >>  		return;
> >> -	}
> >>
> >>  	if (test_bit(PAGE_STALE, &page->private) ||
> >>  	    test_and_set_bit(PAGE_CLAIMED, &page->private)) {
> >> @@ -829,9 +825,7 @@ static void do_compact_page(struct z3fold_header *zhdr, bool locked)
> >>
> >>  	if (!zhdr->foreign_handles && buddy_single(zhdr) &&
> >>  	    zhdr->mapped_count == 0 && compact_single_buddy(zhdr)) {
> >> -		if (kref_put(&zhdr->refcount, release_z3fold_page_locked))
> >> -			atomic64_dec(&pool->pages_nr);
> >> -		else {
> >> +		if (!kref_put(&zhdr->refcount, release_z3fold_page_locked)) {
> >>  			clear_bit(PAGE_CLAIMED, &page->private);
> >>  			z3fold_page_unlock(zhdr);
> >>  		}
> >> @@ -1089,10 +1083,8 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp,
> >>  		if (zhdr) {
> >>  			bud = get_free_buddy(zhdr, chunks);
> >>  			if (bud == HEADLESS) {
> >> -				if (kref_put(&zhdr->refcount,
> >> +				if (!kref_put(&zhdr->refcount,
> >>  					     release_z3fold_page_locked))
> >> -					atomic64_dec(&pool->pages_nr);
> >> -				else
> >>  					z3fold_page_unlock(zhdr);
> >>  				pr_err("No free chunks in unbuddied\n");
> >>  				WARN_ON(1);
> >> @@ -1239,10 +1231,8 @@ static void z3fold_free(struct z3fold_pool *pool, unsigned long handle)
> >>
> >>  	if (!page_claimed)
> >>  		free_handle(handle, zhdr);
> >> -	if (kref_put(&zhdr->refcount, release_z3fold_page_locked_list)) {
> >> -		atomic64_dec(&pool->pages_nr);
> >> +	if (kref_put(&zhdr->refcount, release_z3fold_page_locked_list))
> >>  		return;
> >> -	}
> >>  	if (page_claimed) {
> >>  		/* the page has not been claimed by us */
> >>  		put_z3fold_header(zhdr);
> >> @@ -1353,9 +1343,7 @@ static int z3fold_reclaim_page(struct z3fold_pool *pool, unsigned int
> retries)
> >>  				break;
> >>  			}
> >>  			if (!z3fold_page_trylock(zhdr)) {
> >> -				if (kref_put(&zhdr->refcount,
> >> -						release_z3fold_page))
> >> -					atomic64_dec(&pool->pages_nr);
> >> +				kref_put(&zhdr->refcount, release_z3fold_page);
> >>  				zhdr = NULL;
> >>  				continue; /* can't evict at this point */
> >>  			}
> >> @@ -1366,10 +1354,8 @@ static int z3fold_reclaim_page(struct z3fold_pool *pool, unsigned int
> retries)
> >>  			 */
> >>  			if (zhdr->foreign_handles ||
> >>  			    test_and_set_bit(PAGE_CLAIMED, &page->private)) {
> >> -				if (kref_put(&zhdr->refcount,
> >> +				if (!kref_put(&zhdr->refcount,
> >>  						release_z3fold_page_locked))
> >> -					atomic64_dec(&pool->pages_nr);
> >> -				else
> >>  					z3fold_page_unlock(zhdr);
> >>  				zhdr = NULL;
> >>  				continue; /* can't evict such page */
> >> @@ -1447,7 +1433,6 @@ static int z3fold_reclaim_page(struct z3fold_pool *pool, unsigned int
> retries)
> >>  			if (kref_put(&zhdr->refcount,
> >>  					release_z3fold_page_locked)) {
> >>  				kmem_cache_free(pool->c_handle, slots);
> >> -				atomic64_dec(&pool->pages_nr);
> >>  				return 0;
> >>  			}
> >>  			/*
> >> @@ -1669,10 +1654,8 @@ static void z3fold_page_putback(struct page *page)
> >>  	if (!list_empty(&zhdr->buddy))
> >>  		list_del_init(&zhdr->buddy);
> >>  	INIT_LIST_HEAD(&page->lru);
> >> -	if (kref_put(&zhdr->refcount, release_z3fold_page_locked)) {
> >> -		atomic64_dec(&pool->pages_nr);
> >> +	if (kref_put(&zhdr->refcount, release_z3fold_page_locked))
> >>  		return;
> >> -	}
> >>  	spin_lock(&pool->lock);
> >>  	list_add(&page->lru, &pool->lru);
> >>  	spin_unlock(&pool->lock);
> >> --
> >> 2.23.0
> >
> > -
> > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> > Registration No: 1397386 (Wales)
> >
> > .
> >

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)