Skip to contentSkip to conversation

mm/zsmalloc: fix class per-fullness zspage counts

PATCH Applied Chengming Zhou 3 participants 12 messages Jun 27, 2024 – Jul 1, 2024

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/2 mm/zsmalloc: fix class per-fullness zspage counts

We always use insert_zspage() and remove_zspage() to update zspage's fullness location, which will account correctly.

But this special async free path use "splice" instead of remove_zspage(), so the per-fullness zspage count for ZS_INUSE_RATIO_0 won't decrease.

Fix it by decreasing when iterate over the zspage free list.

---
 mm/zsmalloc.c | 1 +
 1 file changed, 1 insertion(+)
mm/zsmalloc.c +1 -0
index fec1a39e5bbe..7fc25fa4e6b3 100644
@@ -1883,6 +1883,7 @@ static void async_free_zspage(struct work_struct *work)
 
 		class = zspage_class(pool, zspage);
 		spin_lock(&class->lock);
+		class_stat_dec(class, ZS_INUSE_RATIO_0, 1);
 		__free_zspage(pool, class, zspage);
 		spin_unlock(&class->lock);
 	}
Signature · 1 line
2.20.1
#1 lore raw

PATCH 2/2 mm/zsmalloc: move record_obj() into obj_malloc()

We always record_obj() to make handle points to object after obj_malloc(), so simplify the code by moving record_obj() into obj_malloc(). There should be no functional change.

---
 mm/zsmalloc.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)
mm/zsmalloc.c +6 -9
index 7fc25fa4e6b3..c2f4e62ffb46 100644
@@ -1306,7 +1306,6 @@ static unsigned long obj_malloc(struct zs_pool *pool,
 	void *vaddr;
 
 	class = pool->size_class[zspage->class];
-	handle |= OBJ_ALLOCATED_TAG;
 	obj = get_freeobj(zspage);
 
 	offset = obj * class->size;
@@ -1322,15 +1321,16 @@ static unsigned long obj_malloc(struct zs_pool *pool,
 	set_freeobj(zspage, link->next >> OBJ_TAG_BITS);
 	if (likely(!ZsHugePage(zspage)))
 		/* record handle in the header of allocated chunk */
-		link->handle = handle;
+		link->handle = handle | OBJ_ALLOCATED_TAG;
 	else
 		/* record handle to page->index */
-		zspage->first_page->index = handle;
+		zspage->first_page->index = handle | OBJ_ALLOCATED_TAG;
 
 	kunmap_atomic(vaddr);
 	mod_zspage_inuse(zspage, 1);
 
 	obj = location_to_obj(m_page, obj);
+	record_obj(handle, obj);
 
 	return obj;
 }
@@ -1348,7 +1348,7 @@ static unsigned long obj_malloc(struct zs_pool *pool,
  */
 unsigned long zs_malloc(struct zs_pool *pool, size_t size, gfp_t gfp)
 {
-	unsigned long handle, obj;
+	unsigned long handle;
 	struct size_class *class;
 	int newfg;
 	struct zspage *zspage;
@@ -1371,10 +1371,9 @@ unsigned long zs_malloc(struct zs_pool *pool, size_t size, gfp_t gfp)
 	spin_lock(&class->lock);
 	zspage = find_get_zspage(class);
 	if (likely(zspage)) {
-		obj = obj_malloc(pool, zspage, handle);
+		obj_malloc(pool, zspage, handle);
 		/* Now move the zspage to another fullness group, if required */
 		fix_fullness_group(class, zspage);
-		record_obj(handle, obj);
 		class_stat_inc(class, ZS_OBJS_INUSE, 1);
 
 		goto out;
@@ -1389,10 +1388,9 @@ unsigned long zs_malloc(struct zs_pool *pool, size_t size, gfp_t gfp)
 	}
 
 	spin_lock(&class->lock);
-	obj = obj_malloc(pool, zspage, handle);
+	obj_malloc(pool, zspage, handle);
 	newfg = get_fullness_group(class, zspage);
 	insert_zspage(class, zspage, newfg);
-	record_obj(handle, obj);
 	atomic_long_add(class->pages_per_zspage, &pool->pages_allocated);
 	class_stat_inc(class, ZS_OBJS_ALLOCATED, class->objs_per_zspage);
 	class_stat_inc(class, ZS_OBJS_INUSE, 1);
@@ -1591,7 +1589,6 @@ static void migrate_zspage(struct zs_pool *pool, struct zspage *src_zspage,
 		free_obj = obj_malloc(pool, dst_zspage, handle);
 		zs_object_copy(class, free_obj, used_obj);
 		obj_idx++;
-		record_obj(handle, free_obj);
 		obj_free(class->size, used_obj);
 
 		/* Stop if there is no more space */
Signature · 1 line
2.20.1
maintainer Reviewed-by on v1 2/2
#2 lore raw

Re: [PATCH 2/2] mm/zsmalloc: move record_obj() into obj_malloc()

Quoted 5 lines · Chengming Zhou

We always record_obj() to make handle points to object after obj_malloc(), so simplify the code by moving record_obj() into obj_malloc(). There should be no functional change.

I guess I don't have a strong opinion on that.

FWIW

[..]

mm/zsmalloc.c static void migrate_zspage(struct zs_pool *pool, struct zspage *src_zspage, · line 1592

Quoted 6 lines
mm/zsmalloc.c +0 -1
@@ -1591,7 +1589,6 @@ static void migrate_zspage(struct zs_pool *pool, struct zspage *src_zspage,
 		free_obj = obj_malloc(pool, dst_zspage, handle);
 		zs_object_copy(class, free_obj, used_obj);
 		obj_idx++;
-		record_obj(handle, free_obj);
 		obj_free(class->size, used_obj);

I sort of like how here we would copy the object first and then record it, some sort of "commit" stage. But I don't see any issues with the new code.

maintainer
#3 lore raw
In reply to Chengming Zhou
Quoted 13 lines · On Thu, 27 Jun 2024 15:59:58 +0800 Chengming Zhou <[email protected]>…

We always use insert_zspage() and remove_zspage() to update zspage's fullness location, which will account correctly.

But this special async free path use "splice" instead of remove_zspage(), so the per-fullness zspage count for ZS_INUSE_RATIO_0 won't decrease.

Fix it by decreasing when iterate over the zspage free list.

...

+++ b/mm/zsmalloc.c

mm/zsmalloc.c static void async_free_zspage(struct work_struct *work) · line 1889

Quoted 7 lines
mm/zsmalloc.c +1 -0
@@ -1883,6 +1883,7 @@ static void async_free_zspage(struct work_struct *work)
 
 		class = zspage_class(pool, zspage);
 		spin_lock(&class->lock);
+		class_stat_dec(class, ZS_INUSE_RATIO_0, 1);
 		__free_zspage(pool, class, zspage);
 		spin_unlock(&class->lock);
 	}

What are the runtime effects of this bug? Should we backport the fix into earlier kernels? And are we able to identify the appropriate
Fixes: target?

Thanks.

#4 lore raw
Quoted 15 lines · Andrew Morton
Quoted 14 lines · On Thu, 27 Jun 2024 15:59:58 +0800 Chengming Zhou <[email protected]>…

We always use insert_zspage() and remove_zspage() to update zspage's fullness location, which will account correctly.

But this special async free path use "splice" instead of remove_zspage(), so the per-fullness zspage count for ZS_INUSE_RATIO_0 won't decrease.

Fix it by decreasing when iterate over the zspage free list.

...

+++ b/mm/zsmalloc.c

(quoted diff) +0 -0
@@ -1883,6 +1883,7 @@ static void async_free_zspage(struct work_struct *work)
 
Quoted 10 lines

mm/zsmalloc.c static void async_free_zspage(struct work_struct *work) · line 1889

Quoted 6 lines
mm/zsmalloc.c +1 -0
 		class = zspage_class(pool, zspage);
 		spin_lock(&class->lock);
+		class_stat_dec(class, ZS_INUSE_RATIO_0, 1);
 		__free_zspage(pool, class, zspage);
 		spin_unlock(&class->lock);
 	}

What are the runtime effects of this bug? Should we backport the fix into earlier kernels? And are we able to identify the appropriate
Fixes: target?

I don't think this has any run-time visible effects. Class stats (ZS_OBJS_ALLOCATED and ZS_OBJS_INUSE) play their role during compaction (defragmentation), but ZS_INUSE_RATIO_0 is for zspage fullness type, moreover for empty zspage, which we don't look at during compaction.

With CONFIG_ZSMALLOC_STAT enabled we show pool/class stats to the users via zs_stats_size_show() but ZS_INUSE_RATIO_0 is ignored. So no one (external) should know what value is there and ZS_INUSE_RATIO_0 should never be of any importance to zsmalloc (internally).

Code in question (async_free_zspage()) was introduced by 48b4800a1c6af in 2016-07-26, so it's been a long time.

#5 lore raw
In reply to Chengming Zhou
Quoted 7 lines · Chengming Zhou

We always use insert_zspage() and remove_zspage() to update zspage's fullness location, which will account correctly.

But this special async free path use "splice" instead of remove_zspage(), so the per-fullness zspage count for ZS_INUSE_RATIO_0 won't decrease.

Fix it by decreasing when iterate over the zspage free list.

So this doesn't fix anything. ZS_INUSE_RATIO_0 is just a "placeholder" which is never used anywhere.

We can land this patch, if you insist/prefer, but it's some sort of a NOOP, essentially (in a sense that it has no visible effects).

#6 lore raw
Quoted 2 lines · Sergey Senozhatsky

We can land this patch, if you insist/prefer, but it's some sort of a NOOP, essentially (in a sense that it has no visible effects).

Forgot to mention, sorry. If we land this then I'll ask for a different commit message. "Fix" sounds alarming. It would be better to say something like "keep that stat counter accurate, but RATIO_O is never used anywhere".

#7 lore raw
Quoted 7 lines · Sergey Senozhatsky
Quoted 2 lines · Sergey Senozhatsky

We can land this patch, if you insist/prefer, but it's some sort of a NOOP, essentially (in a sense that it has no visible effects).

Forgot to mention, sorry. If we land this then I'll ask for a different commit message. "Fix" sounds alarming. It would be better to say something like "keep that stat counter accurate, but RATIO_O is never used anywhere".

You're right, "fix" sounds alarming although it's a fix, but it has no
any virtual effect. I just found it when I last time changed to the per-class locking but forgot to send it.

Andrew, could you please help to change the subject as Sergey asked? Sorry, I should have noted these details in the changelog when I wrote this subject.

Thanks.

#8 lore raw
Quoted 3 lines · Chengming Zhou

Andrew, could you please help to change the subject as Sergey asked? Sorry, I should have noted these details in the changelog when I wrote this subject.

Chengming, can I ask you to resend these patches with a proper commit message?

#9 lore raw
Quoted 7 lines · Sergey Senozhatsky
Quoted 3 lines · Chengming Zhou

Andrew, could you please help to change the subject as Sergey asked? Sorry, I should have noted these details in the changelog when I wrote this subject.

Chengming, can I ask you to resend these patches with a proper commit message?

Of course, will update and send later.

#10 lore raw
Quoted 10 lines · Chengming Zhou
Quoted 7 lines · Sergey Senozhatsky
Quoted 3 lines · Chengming Zhou

Andrew, could you please help to change the subject as Sergey asked? Sorry, I should have noted these details in the changelog when I wrote this subject.

Chengming, can I ask you to resend these patches with a proper commit message?

Of course, will update and send later.

I just pulled mm/mm-unstable and ready to update, but find Andrew has already helped to change the subject and commit message as below, which is great enough! Thanks!

commit 84d0abc5905bbdf29dc7ff8083d21145d78a3ffe

Author: Chengming Zhou <[email protected]>
Date:   Thu Jun 27 15:59:58 2024 +0800

mm/zsmalloc: clarify class per-fullness zspage counts

We always use insert_zspage() and remove_zspage() to update zspage's fullness location, which will account correctly.

But this special async free path use "splice" instead of
remove_zspage(),
so the per-fullness zspage count for ZS_INUSE_RATIO_0 won't decrease.

Clean things up by decreasing when iterate over the zspage free list.

This doesn't actually fix anything. ZS_INUSE_RATIO_0 is just a "placeholder" which is never used anywhere.

Link:
https://lkml.kernel.org/r/[email protected]

#11 lore raw
Quoted 7 lines · Chengming Zhou
Quoted 6 lines · Chengming Zhou
Quoted 5 lines · Sergey Senozhatsky
Quoted 3 lines · Chengming Zhou

Andrew, could you please help to change the subject as Sergey asked? Sorry, I should have noted these details in the changelog when I wrote this subject.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

Chengming, can I ask you to resend these patches with a proper commit message?

Of course, will update and send later.

I just pulled mm/mm-unstable and ready to update, but find Andrew has already helped to change the subject and commit message as below, which is great enough! Thanks!

Oh, I see, thanks for checking it. Thank you Andrew!