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 <[email protected]>
---
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);
}
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
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 <[email protected]>
> ---
> 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)
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 <[email protected]>
>> ---
>> 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)
>
> .
>
From: Miaohe Lin <[email protected]>
> 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 <[email protected]>
> >> ---
> >> 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)
On 2022/2/21 13:17, David Laight wrote:
> From: Miaohe Lin <[email protected]>
>> 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 <[email protected]>
>>>> ---
>>>> 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)
>
On Wed, Mar 2, 2022 at 10:12 AM David Laight <[email protected]> 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
On Mon, Feb 21, 2022 at 6:17 AM David Laight <[email protected]> wrote:
>
> From: Miaohe Lin <[email protected]>
> > 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 <[email protected]>
> > >> ---
> > >> 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)
> > 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.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On 2022/3/2 18:19, Vitaly Wool wrote:
> On Wed, Mar 2, 2022 at 10:12 AM David Laight <[email protected]> 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.
> .
>