| Message ID | 20240627075959.611783-1-chengming.zhou@linux.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] mm/zsmalloc: fix class per-fullness zspage counts | expand |
On Thu, 27 Jun 2024 15:59:58 +0800 Chengming Zhou <chengming.zhou@linux.dev> wrote: > 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. > > ... > > Signed-off-by: Chengming Zhou <chengming.zhou@linux.dev> > +++ b/mm/zsmalloc.c > @@ -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.
On (24/06/27 13:33), Andrew Morton wrote: > On Thu, 27 Jun 2024 15:59:58 +0800 Chengming Zhou <chengming.zhou@linux.dev> wrote: > > 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. > > > > ... > > > > Signed-off-by: Chengming Zhou <chengming.zhou@linux.dev> > > +++ b/mm/zsmalloc.c > > @@ -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? 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.
On (24/06/27 15:59), Chengming Zhou wrote: > 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).
On (24/06/28 09:55), Sergey Senozhatsky wrote: > 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".
On 2024/6/28 09:08, Sergey Senozhatsky wrote: > On (24/06/28 09:55), Sergey Senozhatsky wrote: >> 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.
On (24/06/28 11:19), Chengming Zhou wrote: > 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?
On 2024/7/1 09:37, Sergey Senozhatsky wrote: > On (24/06/28 11:19), Chengming Zhou wrote: >> 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.
On 2024/7/1 10:20, Chengming Zhou wrote: > On 2024/7/1 09:37, Sergey Senozhatsky wrote: >> On (24/06/28 11:19), Chengming Zhou wrote: >>> 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 <chengming.zhou@linux.dev> 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/20240627075959.611783-1-chengming.zhou@linux.dev Signed-off-by: Chengming Zhou <chengming.zhou@linux.dev> Cc: Minchan Kim <minchan@kernel.org> Cc: Sergey Senozhatsky <senozhatsky@chromium.org> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
On (24/07/01 10:49), Chengming Zhou wrote: > On 2024/7/1 10:20, Chengming Zhou wrote: > > On 2024/7/1 09:37, Sergey Senozhatsky wrote: > > > On (24/06/28 11:19), Chengming Zhou wrote: > > > > 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! Oh, I see, thanks for checking it. Thank you Andrew!
diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c index fec1a39e5bbe..7fc25fa4e6b3 100644 --- a/mm/zsmalloc.c +++ b/mm/zsmalloc.c @@ -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); }
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. Signed-off-by: Chengming Zhou <chengming.zhou@linux.dev> --- mm/zsmalloc.c | 1 + 1 file changed, 1 insertion(+)