diff mbox series

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

Message ID 20220219092533.12596-7-linmiaohe@huawei.com (mailing list archive)
State New
Headers show
Series A few cleanup patches for z3fold | expand

Commit Message

Miaohe Lin Feb. 19, 2022, 9:25 a.m. UTC
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

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/z3fold.c | 41 ++++++++++++-----------------------------
 1 file changed, 12 insertions(+), 29 deletions(-)

Comments

David Laight Feb. 19, 2022, 4:33 p.m. UTC | #1
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?
Although it isn't al all obvious why the 'data' size changes.

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

	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)
Miaohe Lin Feb. 21, 2022, 2:53 a.m. UTC | #2
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.

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 ?

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)
> 
> .
>
David Laight Feb. 21, 2022, 5:17 a.m. UTC | #3
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)
Miaohe Lin Feb. 21, 2022, 11:37 a.m. UTC | #4
On 2022/2/21 13:17, David Laight wrote:
> 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).
> 

So embarrassing... I should have taken a coffee first. :(

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

I think you're right. It seems __jump_table changed from 3e0 -> 360 (0x80)
which is the same value for data size (from 1376 -> 1248). __jump_table section
might gets added to the .data section.

with this patch:
[11] __jump_table      PROGBITS         0000000000000000  00003870
     0000000000000360  0000000000000000  WA       0     0     8

without this patch:
[11] __jump_table      PROGBITS         0000000000000000  00003a00
     00000000000003e0  0000000000000000  WA       0     0     8
> 
>>
>> 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.)

Do you mean something like below ?

diff --git a/mm/z3fold.c b/mm/z3fold.c
index db41b4227ec7..f30bff5e0092 100644
--- a/mm/z3fold.c
+++ b/mm/z3fold.c
@@ -161,7 +161,7 @@ struct z3fold_pool {
        struct list_head *unbuddied;
        struct list_head lru;
        struct list_head stale;
-       atomic64_t pages_nr;
+       long pages_nr;
        struct kmem_cache *c_handle;
        const struct z3fold_ops *ops;
        struct zpool *zpool;
@@ -516,9 +516,8 @@ static void __release_z3fold_page(struct z3fold_header *zhdr, bool locked)
        spin_lock(&pool->stale_lock);
        list_add(&zhdr->buddy, &pool->stale);
        queue_work(pool->release_wq, &pool->work);
+       pool->pages_nr--;
        spin_unlock(&pool->stale_lock);
-
-       atomic64_dec(&pool->pages_nr);
 }

 static void release_z3fold_page(struct kref *ref)
@@ -983,7 +982,7 @@ static struct z3fold_pool *z3fold_create_pool(const char *name, gfp_t gfp,
        }
        INIT_LIST_HEAD(&pool->lru);
        INIT_LIST_HEAD(&pool->stale);
-       atomic64_set(&pool->pages_nr, 0);
+       pool->pages_nr = 0;
        pool->name = name;
        pool->compact_wq = create_singlethread_workqueue(pool->name);
        if (!pool->compact_wq)
@@ -1119,7 +1118,9 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp,
                __free_page(page);
                return -ENOMEM;
        }
-       atomic64_inc(&pool->pages_nr);
+       spin_lock(&pool->stale_lock);
+       pool->pages_nr++;
+       spin_unlock(&pool->stale_lock);

        if (bud == HEADLESS) {
                set_bit(PAGE_HEADLESS, &page->private);
@@ -1197,7 +1198,7 @@ static void z3fold_free(struct z3fold_pool *pool, unsigned long handle)
                        spin_unlock(&pool->lock);
                        put_z3fold_header(zhdr);
                        free_z3fold_page(page, true);
-                       atomic64_dec(&pool->pages_nr);
+                       pool->pages_nr--;
                }
                return;
        }
@@ -1410,7 +1411,7 @@ static int z3fold_reclaim_page(struct z3fold_pool *pool, unsigned int retries)
                if (test_bit(PAGE_HEADLESS, &page->private)) {
                        if (ret == 0) {
                                free_z3fold_page(page, true);
-                               atomic64_dec(&pool->pages_nr);
+                               pool->pages_nr--;
                                return 0;
                        }
                        spin_lock(&pool->lock);
@@ -1526,7 +1527,7 @@ static void z3fold_unmap(struct z3fold_pool *pool, unsigned long handle)
  */
 static u64 z3fold_get_pool_size(struct z3fold_pool *pool)
 {
-       return atomic64_read(&pool->pages_nr);
+       return pool->pages_nr;
 }

 static bool z3fold_page_isolate(struct page *page, isolate_mode_t mode)


Are we expected to hold page->stale lock when we fetch pages_nr in the z3fold_get_pool_size?
Anyway, I think this could be another separate patch. What do you think ?

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

'long' should be enough. I think there can't be that many z3fold pages.

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

> 
> 
>>
>> 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)
>
Vitaly Wool March 2, 2022, 8:31 a.m. UTC | #5
On Mon, Feb 21, 2022 at 6:17 AM David Laight <David.Laight@aculab.com> wrote:
>
> 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.)

It's actually *stale_lock* and it's very misleading to use it for this.
I would actually like to keep atomics but I have no problem with
making it 32-bit for 32-bit systems. Would that work for you guys?

~Vitaly

> 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)
Vitaly Wool March 2, 2022, 10:19 a.m. UTC | #6
On Wed, Mar 2, 2022 at 10:12 AM David Laight <David.Laight@aculab.com> wrote:
>
> > > 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.)
> >
> > It's actually *stale_lock* and it's very misleading to use it for this.
> > I would actually like to keep atomics but I have no problem with
> > making it 32-bit for 32-bit systems. Would that work for you guys?
>
> It would be better to rename the lock.

No it would not because that lock is protecting the list of entries
that could not be immediately freed.

~Vitaly
Miaohe Lin March 3, 2022, 2:27 a.m. UTC | #7
On 2022/3/2 18:19, Vitaly Wool wrote:
> On Wed, Mar 2, 2022 at 10:12 AM David Laight <David.Laight@aculab.com> wrote:
>>
>>>> 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.)
>>>
>>> It's actually *stale_lock* and it's very misleading to use it for this.
>>> I would actually like to keep atomics but I have no problem with
>>> making it 32-bit for 32-bit systems. Would that work for you guys?
>>
>> It would be better to rename the lock.
> 
> No it would not because that lock is protecting the list of entries
> that could not be immediately freed.
> 

Or could we use pool->lock to do this ?

> ~Vitaly

Vitaly, is the patch itself worth a Reviewed-by tag and go to the mm-tree ? Could this
enhance discussed here be sent as another separate patch or am I supposed to make this
change into the current patch?

Many thanks for comment.

> .
>
diff mbox series

Patch

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);
 }
 
 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);