[PATCH 0/9] A few cleanup patches for z3fold

[PATCH 0/9] A few cleanup patches for z3fold

[Miaohe Lin]

Hi,
This series contains a few patches to simplify the code, remove unneeded
return value, fix obsolete comment and so on. More details can be found
in the respective changelogs. Thanks!

Miaohe Lin (9):
  mm/z3fold: declare z3fold_mount with __init
  mm/z3fold: remove obsolete comment in z3fold_alloc
  mm/z3fold: minor clean up for z3fold_free
  mm/z3fold: remove unneeded page_mapcount_reset and ClearPagePrivate
  mm/z3fold: remove confusing local variable l reassignment
  mm/z3fold: move decrement of pool->pages_nr into
    __release_z3fold_page()
  mm/z3fold: remove redundant list_del_init of zhdr->buddy in
    z3fold_free
  mm/z3fold: remove unneeded PAGE_HEADLESS check in free_handle()
  mm/z3fold: remove unneeded return value of z3fold_compact_page()

 mm/z3fold.c | 78 +++++++++++++++--------------------------------------
 1 file changed, 22 insertions(+), 56 deletions(-)

-- 
2.23.0

[PATCH 1/9] mm/z3fold: declare z3fold_mount with __init

[Miaohe Lin]

z3fold_mount is only called during init. So we should declare it
with __init.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/z3fold.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/z3fold.c b/mm/z3fold.c
index b3c0577b8095..e86aafea6599 100644
--- a/mm/z3fold.c
+++ b/mm/z3fold.c
@@ -345,7 +345,7 @@ static struct file_system_type z3fold_fs = {
 };
 
 static struct vfsmount *z3fold_mnt;
-static int z3fold_mount(void)
+static int __init z3fold_mount(void)
 {
 	int ret = 0;
 
-- 
2.23.0

Re: [PATCH 1/9] mm/z3fold: declare z3fold_mount with __init

[Vitaly Wool]

On Sat, Feb 19, 2022 at 10:26 AM Miaohe Lin <linmiaohe@huawei.com> wrote:
>
> z3fold_mount is only called during init. So we should declare it
> with __init.
>
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Reviewed-by: Vitaly Wool <vitaly.wool@konsulko.com>
> ---
>  mm/z3fold.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/z3fold.c b/mm/z3fold.c
> index b3c0577b8095..e86aafea6599 100644
> --- a/mm/z3fold.c
> +++ b/mm/z3fold.c
> @@ -345,7 +345,7 @@ static struct file_system_type z3fold_fs = {
>  };
>
>  static struct vfsmount *z3fold_mnt;
> -static int z3fold_mount(void)
> +static int __init z3fold_mount(void)
>  {
>         int ret = 0;
>
> --
> 2.23.0
>

[PATCH 2/9] mm/z3fold: remove obsolete comment in z3fold_alloc

[Miaohe Lin]

The highmem pages are supported since commit f1549cb5ab2b ("mm/z3fold.c:
allow __GFP_HIGHMEM in z3fold_alloc"). Remove the residual comment.

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

diff --git a/mm/z3fold.c b/mm/z3fold.c
index e86aafea6599..87689f50f709 100644
--- a/mm/z3fold.c
+++ b/mm/z3fold.c
@@ -1064,9 +1064,6 @@ static void z3fold_destroy_pool(struct z3fold_pool *pool)
  * performed first. If no suitable free region is found, then a new page is
  * allocated and added to the pool to satisfy the request.
  *
- * gfp should not set __GFP_HIGHMEM as highmem pages cannot be used
- * as z3fold pool pages.
- *
  * Return: 0 if success and handle is set, otherwise -EINVAL if the size or
  * gfp arguments are invalid or -ENOMEM if the pool was unable to allocate
  * a new page.
-- 
2.23.0

Re: [PATCH 2/9] mm/z3fold: remove obsolete comment in z3fold_alloc

[Vitaly Wool]

On Sat, Feb 19, 2022 at 10:26 AM Miaohe Lin <linmiaohe@huawei.com> wrote:
>
> The highmem pages are supported since commit f1549cb5ab2b ("mm/z3fold.c:
> allow __GFP_HIGHMEM in z3fold_alloc"). Remove the residual comment.
>
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Reviewed-by: Vitaly Wool <vitaly.wool@konsulko.com>

> ---
>  mm/z3fold.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/mm/z3fold.c b/mm/z3fold.c
> index e86aafea6599..87689f50f709 100644
> --- a/mm/z3fold.c
> +++ b/mm/z3fold.c
> @@ -1064,9 +1064,6 @@ static void z3fold_destroy_pool(struct z3fold_pool *pool)
>   * performed first. If no suitable free region is found, then a new page is
>   * allocated and added to the pool to satisfy the request.
>   *
> - * gfp should not set __GFP_HIGHMEM as highmem pages cannot be used
> - * as z3fold pool pages.
> - *
>   * Return: 0 if success and handle is set, otherwise -EINVAL if the size or
>   * gfp arguments are invalid or -ENOMEM if the pool was unable to allocate
>   * a new page.
> --
> 2.23.0
>

[PATCH 3/9] mm/z3fold: minor clean up for z3fold_free

[Miaohe Lin]

Use put_z3fold_header() to pair with get_z3fold_header. Also fix the wrong
comments. Minor readability improvement.

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

diff --git a/mm/z3fold.c b/mm/z3fold.c
index 87689f50f709..eb89271aea83 100644
--- a/mm/z3fold.c
+++ b/mm/z3fold.c
@@ -1187,9 +1187,9 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp,
  * @handle:	handle associated with the allocation returned by z3fold_alloc()
  *
  * In the case that the z3fold page in which the allocation resides is under
- * reclaim, as indicated by the PG_reclaim flag being set, this function
- * only sets the first|last_chunks to 0.  The page is actually freed
- * once both buddies are evicted (see z3fold_reclaim_page() below).
+ * reclaim, as indicated by the PAGE_CLAIMED flag being set, this function
+ * only sets the first|middle|last_chunks to 0.  The page is actually freed
+ * once all buddies are evicted (see z3fold_reclaim_page() below).
  */
 static void z3fold_free(struct z3fold_pool *pool, unsigned long handle)
 {
@@ -1247,7 +1247,7 @@ static void z3fold_free(struct z3fold_pool *pool, unsigned long handle)
 	}
 	if (page_claimed) {
 		/* the page has not been claimed by us */
-		z3fold_page_unlock(zhdr);
+		put_z3fold_header(zhdr);
 		return;
 	}
 	if (test_and_set_bit(NEEDS_COMPACTING, &page->private)) {
-- 
2.23.0

Re: [PATCH 3/9] mm/z3fold: minor clean up for z3fold_free

[Vitaly Wool]

On Sat, Feb 19, 2022 at 10:26 AM Miaohe Lin <linmiaohe@huawei.com> wrote:
>
> Use put_z3fold_header() to pair with get_z3fold_header. Also fix the wrong
> comments. Minor readability improvement.
>
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Reviewed-by: Vitaly Wool <vitaly.wool@konsulko.com>
> ---
>  mm/z3fold.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/mm/z3fold.c b/mm/z3fold.c
> index 87689f50f709..eb89271aea83 100644
> --- a/mm/z3fold.c
> +++ b/mm/z3fold.c
> @@ -1187,9 +1187,9 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp,
>   * @handle:    handle associated with the allocation returned by z3fold_alloc()
>   *
>   * In the case that the z3fold page in which the allocation resides is under
> - * reclaim, as indicated by the PG_reclaim flag being set, this function
> - * only sets the first|last_chunks to 0.  The page is actually freed
> - * once both buddies are evicted (see z3fold_reclaim_page() below).
> + * reclaim, as indicated by the PAGE_CLAIMED flag being set, this function
> + * only sets the first|middle|last_chunks to 0.  The page is actually freed
> + * once all buddies are evicted (see z3fold_reclaim_page() below).
>   */
>  static void z3fold_free(struct z3fold_pool *pool, unsigned long handle)
>  {
> @@ -1247,7 +1247,7 @@ static void z3fold_free(struct z3fold_pool *pool, unsigned long handle)
>         }
>         if (page_claimed) {
>                 /* the page has not been claimed by us */
> -               z3fold_page_unlock(zhdr);
> +               put_z3fold_header(zhdr);
>                 return;
>         }
>         if (test_and_set_bit(NEEDS_COMPACTING, &page->private)) {
> --
> 2.23.0
>

[PATCH 4/9] mm/z3fold: remove unneeded page_mapcount_reset and ClearPagePrivate

[Miaohe Lin]

Page->page_type and PagePrivate are not used in z3fold. We should remove
these confusing unneeded operations. The z3fold do these here is due to
referring to zsmalloc's migration code which does need these operations.

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

diff --git a/mm/z3fold.c b/mm/z3fold.c
index eb89271aea83..2f848ea45b4d 100644
--- a/mm/z3fold.c
+++ b/mm/z3fold.c
@@ -420,7 +420,6 @@ static void free_z3fold_page(struct page *page, bool headless)
 		__ClearPageMovable(page);
 		unlock_page(page);
 	}
-	ClearPagePrivate(page);
 	__free_page(page);
 }
 
@@ -1635,7 +1634,6 @@ static int z3fold_page_migrate(struct address_space *mapping, struct page *newpa
 	INIT_LIST_HEAD(&new_zhdr->buddy);
 	new_mapping = page_mapping(page);
 	__ClearPageMovable(page);
-	ClearPagePrivate(page);
 
 	get_page(newpage);
 	z3fold_page_lock(new_zhdr);
@@ -1655,7 +1653,6 @@ static int z3fold_page_migrate(struct address_space *mapping, struct page *newpa
 
 	queue_work_on(new_zhdr->cpu, pool->compact_wq, &new_zhdr->work);
 
-	page_mapcount_reset(page);
 	clear_bit(PAGE_CLAIMED, &page->private);
 	put_page(page);
 	return 0;
-- 
2.23.0

Re: [PATCH 4/9] mm/z3fold: remove unneeded page_mapcount_reset and ClearPagePrivate

[Vitaly Wool]

On Sat, Feb 19, 2022 at 10:26 AM Miaohe Lin <linmiaohe@huawei.com> wrote:
>
> Page->page_type and PagePrivate are not used in z3fold. We should remove
> these confusing unneeded operations. The z3fold do these here is due to
> referring to zsmalloc's migration code which does need these operations.

Absolutely, thanks for pointing this out.

> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Reviewed-by: Vitaly Wool <vitaly.wool@konsulko.com>

> ---
>  mm/z3fold.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/mm/z3fold.c b/mm/z3fold.c
> index eb89271aea83..2f848ea45b4d 100644
> --- a/mm/z3fold.c
> +++ b/mm/z3fold.c
> @@ -420,7 +420,6 @@ static void free_z3fold_page(struct page *page, bool headless)
>                 __ClearPageMovable(page);
>                 unlock_page(page);
>         }
> -       ClearPagePrivate(page);
>         __free_page(page);
>  }
>
> @@ -1635,7 +1634,6 @@ static int z3fold_page_migrate(struct address_space *mapping, struct page *newpa
>         INIT_LIST_HEAD(&new_zhdr->buddy);
>         new_mapping = page_mapping(page);
>         __ClearPageMovable(page);
> -       ClearPagePrivate(page);
>
>         get_page(newpage);
>         z3fold_page_lock(new_zhdr);
> @@ -1655,7 +1653,6 @@ static int z3fold_page_migrate(struct address_space *mapping, struct page *newpa
>
>         queue_work_on(new_zhdr->cpu, pool->compact_wq, &new_zhdr->work);
>
> -       page_mapcount_reset(page);
>         clear_bit(PAGE_CLAIMED, &page->private);
>         put_page(page);
>         return 0;
> --
> 2.23.0
>

[PATCH 5/9] mm/z3fold: remove confusing local variable l reassignment

[Miaohe Lin]

The local variable l holds the address of unbuddied[i] which won't change
after we take the pool lock. Remove it to avoid confusion.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/z3fold.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mm/z3fold.c b/mm/z3fold.c
index 2f848ea45b4d..adc0b3fa4906 100644
--- a/mm/z3fold.c
+++ b/mm/z3fold.c
@@ -876,7 +876,6 @@ static inline struct z3fold_header *__z3fold_alloc(struct z3fold_pool *pool,
 
 		/* Re-check under lock. */
 		spin_lock(&pool->lock);
-		l = &unbuddied[i];
 		if (unlikely(zhdr != list_first_entry(READ_ONCE(l),
 						struct z3fold_header, buddy)) ||
 		    !z3fold_page_trylock(zhdr)) {
-- 
2.23.0

Re: [PATCH 5/9] mm/z3fold: remove confusing local variable l reassignment

[Vitaly Wool]

On Sat, Feb 19, 2022 at 10:26 AM Miaohe Lin <linmiaohe@huawei.com> wrote:
>
> The local variable l holds the address of unbuddied[i] which won't change
> after we take the pool lock. Remove it to avoid confusion.
>
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Reviewed-by: Vitaly Wool <vitaly.wool@konsulko.com>

> ---
>  mm/z3fold.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/mm/z3fold.c b/mm/z3fold.c
> index 2f848ea45b4d..adc0b3fa4906 100644
> --- a/mm/z3fold.c
> +++ b/mm/z3fold.c
> @@ -876,7 +876,6 @@ static inline struct z3fold_header *__z3fold_alloc(struct z3fold_pool *pool,
>
>                 /* Re-check under lock. */
>                 spin_lock(&pool->lock);
> -               l = &unbuddied[i];
>                 if (unlikely(zhdr != list_first_entry(READ_ONCE(l),
>                                                 struct z3fold_header, buddy)) ||
>                     !z3fold_page_trylock(zhdr)) {
> --
> 2.23.0
>

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

[Miaohe Lin]

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(-)

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

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

[David Laight]

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)

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

[Miaohe Lin]

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

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

[David Laight]

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)

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

[Miaohe Lin]

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

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

[Vitaly Wool]

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)

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

[David Laight]

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

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

[Vitaly Wool]

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

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

[Miaohe Lin]

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.

> .
> 

[PATCH 7/9] mm/z3fold: remove redundant list_del_init of zhdr->buddy in z3fold_free

[Miaohe Lin]

The do_compact_page will do list_del_init(&zhdr->buddy) for us. Remove this
extra one to save some possible cpu cycles.

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

diff --git a/mm/z3fold.c b/mm/z3fold.c
index 18a697f6fe32..867c590df027 100644
--- a/mm/z3fold.c
+++ b/mm/z3fold.c
@@ -1244,9 +1244,6 @@ static void z3fold_free(struct z3fold_pool *pool, unsigned long handle)
 		return;
 	}
 	if (zhdr->cpu < 0 || !cpu_online(zhdr->cpu)) {
-		spin_lock(&pool->lock);
-		list_del_init(&zhdr->buddy);
-		spin_unlock(&pool->lock);
 		zhdr->cpu = -1;
 		kref_get(&zhdr->refcount);
 		clear_bit(PAGE_CLAIMED, &page->private);
-- 
2.23.0

Re: [PATCH 7/9] mm/z3fold: remove redundant list_del_init of zhdr->buddy in z3fold_free

[Vitaly Wool]

On Sat, Feb 19, 2022 at 10:26 AM Miaohe Lin <linmiaohe@huawei.com> wrote:
>
> The do_compact_page will do list_del_init(&zhdr->buddy) for us. Remove this
> extra one to save some possible cpu cycles.
>
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Reviewed-by: Vitaly Wool <vitaly.wool@konsulko.com>

> ---
>  mm/z3fold.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/mm/z3fold.c b/mm/z3fold.c
> index 18a697f6fe32..867c590df027 100644
> --- a/mm/z3fold.c
> +++ b/mm/z3fold.c
> @@ -1244,9 +1244,6 @@ static void z3fold_free(struct z3fold_pool *pool, unsigned long handle)
>                 return;
>         }
>         if (zhdr->cpu < 0 || !cpu_online(zhdr->cpu)) {
> -               spin_lock(&pool->lock);
> -               list_del_init(&zhdr->buddy);
> -               spin_unlock(&pool->lock);
>                 zhdr->cpu = -1;
>                 kref_get(&zhdr->refcount);
>                 clear_bit(PAGE_CLAIMED, &page->private);
> --
> 2.23.0
>

[PATCH 8/9] mm/z3fold: remove unneeded PAGE_HEADLESS check in free_handle()

[Miaohe Lin]

The only caller z3fold_free() never calls free_handle() in PAGE_HEADLESS
case. Remove this unneeded check.

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

diff --git a/mm/z3fold.c b/mm/z3fold.c
index 867c590df027..83b5a3514427 100644
--- a/mm/z3fold.c
+++ b/mm/z3fold.c
@@ -297,9 +297,6 @@ static inline void free_handle(unsigned long handle, struct z3fold_header *zhdr)
 	int i;
 	bool is_free;
 
-	if (handle & (1 << PAGE_HEADLESS))
-		return;
-
 	if (WARN_ON(*(unsigned long *)handle == 0))
 		return;
 
-- 
2.23.0

[PATCH 9/9] mm/z3fold: remove unneeded return value of z3fold_compact_page()

[Miaohe Lin]

Remove the unneeded return value of z3fold_compact_page() as it's
never checked.

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

diff --git a/mm/z3fold.c b/mm/z3fold.c
index 83b5a3514427..db41b4227ec7 100644
--- a/mm/z3fold.c
+++ b/mm/z3fold.c
@@ -746,18 +746,18 @@ static struct z3fold_header *compact_single_buddy(struct z3fold_header *zhdr)
 
 #define BIG_CHUNK_GAP	3
 /* Has to be called with lock held */
-static int z3fold_compact_page(struct z3fold_header *zhdr)
+static void z3fold_compact_page(struct z3fold_header *zhdr)
 {
 	struct page *page = virt_to_page(zhdr);
 
 	if (test_bit(MIDDLE_CHUNK_MAPPED, &page->private))
-		return 0; /* can't move middle chunk, it's used */
+		return; /* can't move middle chunk, it's used */
 
 	if (unlikely(PageIsolated(page)))
-		return 0;
+		return;
 
 	if (zhdr->middle_chunks == 0)
-		return 0; /* nothing to compact */
+		return; /* nothing to compact */
 
 	if (zhdr->first_chunks == 0 && zhdr->last_chunks == 0) {
 		/* move to the beginning */
@@ -766,7 +766,7 @@ static int z3fold_compact_page(struct z3fold_header *zhdr)
 		zhdr->middle_chunks = 0;
 		zhdr->start_middle = 0;
 		zhdr->first_num++;
-		return 1;
+		return;
 	}
 
 	/*
@@ -778,7 +778,6 @@ static int z3fold_compact_page(struct z3fold_header *zhdr)
 			BIG_CHUNK_GAP) {
 		mchunk_memmove(zhdr, zhdr->first_chunks + ZHDR_CHUNKS);
 		zhdr->start_middle = zhdr->first_chunks + ZHDR_CHUNKS;
-		return 1;
 	} else if (zhdr->last_chunks != 0 && zhdr->first_chunks == 0 &&
 		   TOTAL_CHUNKS - (zhdr->last_chunks + zhdr->start_middle
 					+ zhdr->middle_chunks) >=
@@ -787,10 +786,7 @@ static int z3fold_compact_page(struct z3fold_header *zhdr)
 			zhdr->middle_chunks;
 		mchunk_memmove(zhdr, new_start);
 		zhdr->start_middle = new_start;
-		return 1;
 	}
-
-	return 0;
 }
 
 static void do_compact_page(struct z3fold_header *zhdr, bool locked)
-- 
2.23.0

Re: [PATCH 9/9] mm/z3fold: remove unneeded return value of z3fold_compact_page()

[Souptick Joarder]

On Sat, Feb 19, 2022 at 2:56 PM Miaohe Lin <linmiaohe@huawei.com> wrote:
>
> Remove the unneeded return value of z3fold_compact_page() as it's
> never checked.
>
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Acked-by: Souptick Joarder <jrdr.linux@gmail.com>

> ---
>  mm/z3fold.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/mm/z3fold.c b/mm/z3fold.c
> index 83b5a3514427..db41b4227ec7 100644
> --- a/mm/z3fold.c
> +++ b/mm/z3fold.c
> @@ -746,18 +746,18 @@ static struct z3fold_header *compact_single_buddy(struct z3fold_header *zhdr)
>
>  #define BIG_CHUNK_GAP  3
>  /* Has to be called with lock held */
> -static int z3fold_compact_page(struct z3fold_header *zhdr)
> +static void z3fold_compact_page(struct z3fold_header *zhdr)
>  {
>         struct page *page = virt_to_page(zhdr);
>
>         if (test_bit(MIDDLE_CHUNK_MAPPED, &page->private))
> -               return 0; /* can't move middle chunk, it's used */
> +               return; /* can't move middle chunk, it's used */
>
>         if (unlikely(PageIsolated(page)))
> -               return 0;
> +               return;
>
>         if (zhdr->middle_chunks == 0)
> -               return 0; /* nothing to compact */
> +               return; /* nothing to compact */
>
>         if (zhdr->first_chunks == 0 && zhdr->last_chunks == 0) {
>                 /* move to the beginning */
> @@ -766,7 +766,7 @@ static int z3fold_compact_page(struct z3fold_header *zhdr)
>                 zhdr->middle_chunks = 0;
>                 zhdr->start_middle = 0;
>                 zhdr->first_num++;
> -               return 1;
> +               return;
>         }
>
>         /*
> @@ -778,7 +778,6 @@ static int z3fold_compact_page(struct z3fold_header *zhdr)
>                         BIG_CHUNK_GAP) {
>                 mchunk_memmove(zhdr, zhdr->first_chunks + ZHDR_CHUNKS);
>                 zhdr->start_middle = zhdr->first_chunks + ZHDR_CHUNKS;
> -               return 1;
>         } else if (zhdr->last_chunks != 0 && zhdr->first_chunks == 0 &&
>                    TOTAL_CHUNKS - (zhdr->last_chunks + zhdr->start_middle
>                                         + zhdr->middle_chunks) >=
> @@ -787,10 +786,7 @@ static int z3fold_compact_page(struct z3fold_header *zhdr)
>                         zhdr->middle_chunks;
>                 mchunk_memmove(zhdr, new_start);
>                 zhdr->start_middle = new_start;
> -               return 1;
>         }
> -
> -       return 0;
>  }
>
>  static void do_compact_page(struct z3fold_header *zhdr, bool locked)
> --
> 2.23.0
>
>

Re: [PATCH 9/9] mm/z3fold: remove unneeded return value of z3fold_compact_page()

[Vitaly Wool]

On Sat, Feb 19, 2022 at 10:26 AM Miaohe Lin <linmiaohe@huawei.com> wrote:
>
> Remove the unneeded return value of z3fold_compact_page() as it's
> never checked.

It was a sort of hook for gathering extended compaction statistics in
future. Do you really gain a lot by removing this?

~Vitaly

> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/z3fold.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/mm/z3fold.c b/mm/z3fold.c
> index 83b5a3514427..db41b4227ec7 100644
> --- a/mm/z3fold.c
> +++ b/mm/z3fold.c
> @@ -746,18 +746,18 @@ static struct z3fold_header *compact_single_buddy(struct z3fold_header *zhdr)
>
>  #define BIG_CHUNK_GAP  3
>  /* Has to be called with lock held */
> -static int z3fold_compact_page(struct z3fold_header *zhdr)
> +static void z3fold_compact_page(struct z3fold_header *zhdr)
>  {
>         struct page *page = virt_to_page(zhdr);
>
>         if (test_bit(MIDDLE_CHUNK_MAPPED, &page->private))
> -               return 0; /* can't move middle chunk, it's used */
> +               return; /* can't move middle chunk, it's used */
>
>         if (unlikely(PageIsolated(page)))
> -               return 0;
> +               return;
>
>         if (zhdr->middle_chunks == 0)
> -               return 0; /* nothing to compact */
> +               return; /* nothing to compact */
>
>         if (zhdr->first_chunks == 0 && zhdr->last_chunks == 0) {
>                 /* move to the beginning */
> @@ -766,7 +766,7 @@ static int z3fold_compact_page(struct z3fold_header *zhdr)
>                 zhdr->middle_chunks = 0;
>                 zhdr->start_middle = 0;
>                 zhdr->first_num++;
> -               return 1;
> +               return;
>         }
>
>         /*
> @@ -778,7 +778,6 @@ static int z3fold_compact_page(struct z3fold_header *zhdr)
>                         BIG_CHUNK_GAP) {
>                 mchunk_memmove(zhdr, zhdr->first_chunks + ZHDR_CHUNKS);
>                 zhdr->start_middle = zhdr->first_chunks + ZHDR_CHUNKS;
> -               return 1;
>         } else if (zhdr->last_chunks != 0 && zhdr->first_chunks == 0 &&
>                    TOTAL_CHUNKS - (zhdr->last_chunks + zhdr->start_middle
>                                         + zhdr->middle_chunks) >=
> @@ -787,10 +786,7 @@ static int z3fold_compact_page(struct z3fold_header *zhdr)
>                         zhdr->middle_chunks;
>                 mchunk_memmove(zhdr, new_start);
>                 zhdr->start_middle = new_start;
> -               return 1;
>         }
> -
> -       return 0;
>  }
>
>  static void do_compact_page(struct z3fold_header *zhdr, bool locked)
> --
> 2.23.0
>

Re: [PATCH 9/9] mm/z3fold: remove unneeded return value of z3fold_compact_page()

[Miaohe Lin]

On 2022/3/2 16:40, Vitaly Wool wrote:
> On Sat, Feb 19, 2022 at 10:26 AM Miaohe Lin <linmiaohe@huawei.com> wrote:
>>
>> Remove the unneeded return value of z3fold_compact_page() as it's
>> never checked.
> 
> It was a sort of hook for gathering extended compaction statistics in
> future. Do you really gain a lot by removing this?

I see. So I shouldn't discard the return value though it looks unused.
This is just for cleanup purpose so let's just drop this patch.

Many thanks for your time and Reviewed-by tag in this series! :)

> 
> ~Vitaly
> 
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>  mm/z3fold.c | 14 +++++---------
>>  1 file changed, 5 insertions(+), 9 deletions(-)
>>
>> diff --git a/mm/z3fold.c b/mm/z3fold.c
>> index 83b5a3514427..db41b4227ec7 100644
>> --- a/mm/z3fold.c
>> +++ b/mm/z3fold.c
>> @@ -746,18 +746,18 @@ static struct z3fold_header *compact_single_buddy(struct z3fold_header *zhdr)
>>
>>  #define BIG_CHUNK_GAP  3
>>  /* Has to be called with lock held */
>> -static int z3fold_compact_page(struct z3fold_header *zhdr)
>> +static void z3fold_compact_page(struct z3fold_header *zhdr)
>>  {
>>         struct page *page = virt_to_page(zhdr);
>>
>>         if (test_bit(MIDDLE_CHUNK_MAPPED, &page->private))
>> -               return 0; /* can't move middle chunk, it's used */
>> +               return; /* can't move middle chunk, it's used */
>>
>>         if (unlikely(PageIsolated(page)))
>> -               return 0;
>> +               return;
>>
>>         if (zhdr->middle_chunks == 0)
>> -               return 0; /* nothing to compact */
>> +               return; /* nothing to compact */
>>
>>         if (zhdr->first_chunks == 0 && zhdr->last_chunks == 0) {
>>                 /* move to the beginning */
>> @@ -766,7 +766,7 @@ static int z3fold_compact_page(struct z3fold_header *zhdr)
>>                 zhdr->middle_chunks = 0;
>>                 zhdr->start_middle = 0;
>>                 zhdr->first_num++;
>> -               return 1;
>> +               return;
>>         }
>>
>>         /*
>> @@ -778,7 +778,6 @@ static int z3fold_compact_page(struct z3fold_header *zhdr)
>>                         BIG_CHUNK_GAP) {
>>                 mchunk_memmove(zhdr, zhdr->first_chunks + ZHDR_CHUNKS);
>>                 zhdr->start_middle = zhdr->first_chunks + ZHDR_CHUNKS;
>> -               return 1;
>>         } else if (zhdr->last_chunks != 0 && zhdr->first_chunks == 0 &&
>>                    TOTAL_CHUNKS - (zhdr->last_chunks + zhdr->start_middle
>>                                         + zhdr->middle_chunks) >=
>> @@ -787,10 +786,7 @@ static int z3fold_compact_page(struct z3fold_header *zhdr)
>>                         zhdr->middle_chunks;
>>                 mchunk_memmove(zhdr, new_start);
>>                 zhdr->start_middle = new_start;
>> -               return 1;
>>         }
>> -
>> -       return 0;
>>  }
>>
>>  static void do_compact_page(struct z3fold_header *zhdr, bool locked)
>> --
>> 2.23.0
>>
> .
> 

Re: [PATCH 0/9] A few cleanup patches for z3fold

[Miaohe Lin]

On 2022/2/19 17:25, Miaohe Lin wrote:
> Hi,
> This series contains a few patches to simplify the code, remove unneeded
> return value, fix obsolete comment and so on. More details can be found
> in the respective changelogs. Thanks!
> 

Friendly ping in case this is forgotten. :)

> Miaohe Lin (9):
>   mm/z3fold: declare z3fold_mount with __init
>   mm/z3fold: remove obsolete comment in z3fold_alloc
>   mm/z3fold: minor clean up for z3fold_free
>   mm/z3fold: remove unneeded page_mapcount_reset and ClearPagePrivate
>   mm/z3fold: remove confusing local variable l reassignment
>   mm/z3fold: move decrement of pool->pages_nr into
>     __release_z3fold_page()
>   mm/z3fold: remove redundant list_del_init of zhdr->buddy in
>     z3fold_free
>   mm/z3fold: remove unneeded PAGE_HEADLESS check in free_handle()
>   mm/z3fold: remove unneeded return value of z3fold_compact_page()
> 
>  mm/z3fold.c | 78 +++++++++++++++--------------------------------------
>  1 file changed, 22 insertions(+), 56 deletions(-)
> 

Re: [PATCH 0/9] A few cleanup patches for z3fold

[Andrew Morton]

On Tue, 1 Mar 2022 21:03:37 +0800 Miaohe Lin <linmiaohe@huawei.com> wrote:

> On 2022/2/19 17:25, Miaohe Lin wrote:
> > Hi,
> > This series contains a few patches to simplify the code, remove unneeded
> > return value, fix obsolete comment and so on. More details can be found
> > in the respective changelogs. Thanks!
> > 
> 
> Friendly ping in case this is forgotten. :)

I was expecting updates resulting from the [6/9] discussion?

Re: [PATCH 0/9] A few cleanup patches for z3fold

[Miaohe Lin]

On 2022/3/2 1:36, Andrew Morton wrote:
> On Tue, 1 Mar 2022 21:03:37 +0800 Miaohe Lin <linmiaohe@huawei.com> wrote:
> 
>> On 2022/2/19 17:25, Miaohe Lin wrote:
>>> Hi,
>>> This series contains a few patches to simplify the code, remove unneeded
>>> return value, fix obsolete comment and so on. More details can be found
>>> in the respective changelogs. Thanks!
>>>
>>
>> Friendly ping in case this is forgotten. :)
> 
> I was expecting updates resulting from the [6/9] discussion?

I think that could be another separate patch to reduce the atomic operation overhead.
And I have some z3fold bugfix patches pending to sent. They might can be sent together
if such separate patch is prepared at that point.

Many thanks. :)

> .
>