* Re: [PATCH] mm: zswap: fix zswap_never_enabled() for CONFIG_ZSWAP==N
2024-06-29 23:22 [PATCH] mm: zswap: fix zswap_never_enabled() for CONFIG_ZSWAP==N Barry Song
@ 2024-07-01 2:54 ` Chengming Zhou
2024-07-01 13:32 ` Yosry Ahmed
` (4 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Chengming Zhou @ 2024-07-01 2:54 UTC (permalink / raw)
To: Barry Song, akpm, linux-mm
Cc: linux-kernel, Barry Song, Yosry Ahmed, Nhat Pham, Chris Li,
David Hildenbrand, Johannes Weiner, Matthew Wilcox
On 2024/6/30 07:22, Barry Song wrote:
> From: Barry Song <v-songbaohua@oppo.com>
>
> If CONFIG_ZSWAP is set to N, it means zswap cannot be enabled.
> zswap_never_enabled() should return true.
>
> Fixes: 0300e17d67c3 ("mm: zswap: add zswap_never_enabled()")
> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> Cc: Yosry Ahmed <yosryahmed@google.com>
> Cc: Nhat Pham <nphamcs@gmail.com>
> Cc: Chengming Zhou <chengming.zhou@linux.dev>
> Cc: Chris Li <chrisl@kernel.org>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
Reviewed-by: Chengming Zhou <chengming.zhou@linux.dev>
> include/linux/zswap.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/zswap.h b/include/linux/zswap.h
> index bf83ae5e285d..6cecb4a4f68b 100644
> --- a/include/linux/zswap.h
> +++ b/include/linux/zswap.h
> @@ -68,7 +68,7 @@ static inline bool zswap_is_enabled(void)
>
> static inline bool zswap_never_enabled(void)
> {
> - return false;
> + return true;
> }
>
> #endif
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] mm: zswap: fix zswap_never_enabled() for CONFIG_ZSWAP==N
2024-06-29 23:22 [PATCH] mm: zswap: fix zswap_never_enabled() for CONFIG_ZSWAP==N Barry Song
2024-07-01 2:54 ` Chengming Zhou
@ 2024-07-01 13:32 ` Yosry Ahmed
2024-07-01 17:58 ` Chris Li
` (3 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Yosry Ahmed @ 2024-07-01 13:32 UTC (permalink / raw)
To: Barry Song
Cc: akpm, linux-mm, linux-kernel, Barry Song, Nhat Pham,
Chengming Zhou, Chris Li, David Hildenbrand, Johannes Weiner,
Matthew Wilcox
On Sat, Jun 29, 2024 at 4:22 PM Barry Song <21cnbao@gmail.com> wrote:
>
> From: Barry Song <v-songbaohua@oppo.com>
>
> If CONFIG_ZSWAP is set to N, it means zswap cannot be enabled.
> zswap_never_enabled() should return true.
>
> Fixes: 0300e17d67c3 ("mm: zswap: add zswap_never_enabled()")
> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
Acked-by: Yosry Ahmed <yosryahmed@google.com>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] mm: zswap: fix zswap_never_enabled() for CONFIG_ZSWAP==N
2024-06-29 23:22 [PATCH] mm: zswap: fix zswap_never_enabled() for CONFIG_ZSWAP==N Barry Song
2024-07-01 2:54 ` Chengming Zhou
2024-07-01 13:32 ` Yosry Ahmed
@ 2024-07-01 17:58 ` Chris Li
2024-07-02 6:52 ` Andrew Morton
` (2 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Chris Li @ 2024-07-01 17:58 UTC (permalink / raw)
To: Barry Song
Cc: akpm, linux-mm, linux-kernel, Barry Song, Yosry Ahmed, Nhat Pham,
Chengming Zhou, Chris Li, David Hildenbrand, Johannes Weiner,
Matthew Wilcox
Acked-by: Chris Li <chrisl@kernel.org>
Chris
On Sat, Jun 29, 2024 at 4:22 PM Barry Song <21cnbao@gmail.com> wrote:
>
> From: Barry Song <v-songbaohua@oppo.com>
>
> If CONFIG_ZSWAP is set to N, it means zswap cannot be enabled.
> zswap_never_enabled() should return true.
>
> Fixes: 0300e17d67c3 ("mm: zswap: add zswap_never_enabled()")
> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> Cc: Yosry Ahmed <yosryahmed@google.com>
> Cc: Nhat Pham <nphamcs@gmail.com>
> Cc: Chengming Zhou <chengming.zhou@linux.dev>
> Cc: Chris Li <chrisl@kernel.org>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
> include/linux/zswap.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/zswap.h b/include/linux/zswap.h
> index bf83ae5e285d..6cecb4a4f68b 100644
> --- a/include/linux/zswap.h
> +++ b/include/linux/zswap.h
> @@ -68,7 +68,7 @@ static inline bool zswap_is_enabled(void)
>
> static inline bool zswap_never_enabled(void)
> {
> - return false;
> + return true;
> }
>
> #endif
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] mm: zswap: fix zswap_never_enabled() for CONFIG_ZSWAP==N
2024-06-29 23:22 [PATCH] mm: zswap: fix zswap_never_enabled() for CONFIG_ZSWAP==N Barry Song
` (2 preceding siblings ...)
2024-07-01 17:58 ` Chris Li
@ 2024-07-02 6:52 ` Andrew Morton
2024-07-02 7:00 ` Barry Song
2024-07-02 11:58 ` Yosry Ahmed
2024-07-02 6:58 ` David Hildenbrand
2024-07-03 23:22 ` Nhat Pham
5 siblings, 2 replies; 10+ messages in thread
From: Andrew Morton @ 2024-07-02 6:52 UTC (permalink / raw)
To: Barry Song
Cc: linux-mm, linux-kernel, Barry Song, Yosry Ahmed, Nhat Pham,
Chengming Zhou, Chris Li, David Hildenbrand, Johannes Weiner,
Matthew Wilcox
On Sun, 30 Jun 2024 11:22:31 +1200 Barry Song <21cnbao@gmail.com> wrote:
> From: Barry Song <v-songbaohua@oppo.com>
>
> If CONFIG_ZSWAP is set to N, it means zswap cannot be enabled.
> zswap_never_enabled() should return true.
>
> ...
>
> --- a/include/linux/zswap.h
> +++ b/include/linux/zswap.h
> @@ -68,7 +68,7 @@ static inline bool zswap_is_enabled(void)
>
> static inline bool zswap_never_enabled(void)
> {
> - return false;
> + return true;
> }
Well, that code was as wrong as it's possible to get.
But what effect does this have? Seems "not much"? Perhaps we'll
attempt a zswap_load() which later fails for other reasons?
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] mm: zswap: fix zswap_never_enabled() for CONFIG_ZSWAP==N
2024-07-02 6:52 ` Andrew Morton
@ 2024-07-02 7:00 ` Barry Song
2024-07-02 11:58 ` Yosry Ahmed
1 sibling, 0 replies; 10+ messages in thread
From: Barry Song @ 2024-07-02 7:00 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-mm, linux-kernel, Barry Song, Yosry Ahmed, Nhat Pham,
Chengming Zhou, Chris Li, David Hildenbrand, Johannes Weiner,
Matthew Wilcox
On Tue, Jul 2, 2024 at 6:52 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Sun, 30 Jun 2024 11:22:31 +1200 Barry Song <21cnbao@gmail.com> wrote:
>
> > From: Barry Song <v-songbaohua@oppo.com>
> >
> > If CONFIG_ZSWAP is set to N, it means zswap cannot be enabled.
> > zswap_never_enabled() should return true.
> >
> > ...
> >
> > --- a/include/linux/zswap.h
> > +++ b/include/linux/zswap.h
> > @@ -68,7 +68,7 @@ static inline bool zswap_is_enabled(void)
> >
> > static inline bool zswap_never_enabled(void)
> > {
> > - return false;
> > + return true;
> > }
>
> Well, that code was as wrong as it's possible to get.
>
> But what effect does this have? Seems "not much"? Perhaps we'll
> attempt a zswap_load() which later fails for other reasons?
Yes, but the API was created to inform the mm core that zswap has never been
enabled, allowing the mm core to perform mTHP swap-in. This is a transitional
solution until zswap supports mTHP. If zswap has been enabled, performing
mTHP swap-in will result in corrupted data. You may find the answer in the
mTHP swap-in series:
https://lore.kernel.org/linux-mm/CAJD7tkZ4FQr6HZpduOdvmqgg_-whuZYE-Bz5O2t6yzw6Yg+v1A@mail.gmail.com/
Thanks
Barry
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] mm: zswap: fix zswap_never_enabled() for CONFIG_ZSWAP==N
2024-07-02 6:52 ` Andrew Morton
2024-07-02 7:00 ` Barry Song
@ 2024-07-02 11:58 ` Yosry Ahmed
2024-07-03 2:02 ` Andrew Morton
1 sibling, 1 reply; 10+ messages in thread
From: Yosry Ahmed @ 2024-07-02 11:58 UTC (permalink / raw)
To: Andrew Morton
Cc: Barry Song, linux-mm, linux-kernel, Barry Song, Nhat Pham,
Chengming Zhou, Chris Li, David Hildenbrand, Johannes Weiner,
Matthew Wilcox
On Mon, Jul 1, 2024 at 11:52 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Sun, 30 Jun 2024 11:22:31 +1200 Barry Song <21cnbao@gmail.com> wrote:
>
> > From: Barry Song <v-songbaohua@oppo.com>
> >
> > If CONFIG_ZSWAP is set to N, it means zswap cannot be enabled.
> > zswap_never_enabled() should return true.
> >
> > ...
> >
> > --- a/include/linux/zswap.h
> > +++ b/include/linux/zswap.h
> > @@ -68,7 +68,7 @@ static inline bool zswap_is_enabled(void)
> >
> > static inline bool zswap_never_enabled(void)
> > {
> > - return false;
> > + return true;
> > }
>
> Well, that code was as wrong as it's possible to get.
>
> But what effect does this have? Seems "not much"? Perhaps we'll
> attempt a zswap_load() which later fails for other reasons?
Actually zswap_load() is a noop with !CONFIG_ZSWAP, so it doesn't have
an effect there. The only effect is that with Barry's latest large
folio swapin patches for zram, we will always fallback to order-0
swapin, even mistakenly when !CONFIG_ZSWAP.
Basically the bug just makes Barry's in progress patches not work at all.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] mm: zswap: fix zswap_never_enabled() for CONFIG_ZSWAP==N
2024-07-02 11:58 ` Yosry Ahmed
@ 2024-07-03 2:02 ` Andrew Morton
0 siblings, 0 replies; 10+ messages in thread
From: Andrew Morton @ 2024-07-03 2:02 UTC (permalink / raw)
To: Yosry Ahmed
Cc: Barry Song, linux-mm, linux-kernel, Barry Song, Nhat Pham,
Chengming Zhou, Chris Li, David Hildenbrand, Johannes Weiner,
Matthew Wilcox
On Tue, 2 Jul 2024 04:58:05 -0700 Yosry Ahmed <yosryahmed@google.com> wrote:
> > > --- a/include/linux/zswap.h
> > > +++ b/include/linux/zswap.h
> > > @@ -68,7 +68,7 @@ static inline bool zswap_is_enabled(void)
> > >
> > > static inline bool zswap_never_enabled(void)
> > > {
> > > - return false;
> > > + return true;
> > > }
> >
> > Well, that code was as wrong as it's possible to get.
> >
> > But what effect does this have? Seems "not much"? Perhaps we'll
> > attempt a zswap_load() which later fails for other reasons?
>
> Actually zswap_load() is a noop with !CONFIG_ZSWAP, so it doesn't have
> an effect there. The only effect is that with Barry's latest large
> folio swapin patches for zram, we will always fallback to order-0
> swapin, even mistakenly when !CONFIG_ZSWAP.
>
> Basically the bug just makes Barry's in progress patches not work at all.
OK, thanks, I added this to the changelog:
The only effect of this issue is that with Barry's latest large folio
swapin patches for zram ("mm: support mTHP swap-in for zRAM-like
swapfile"), we will always fallback to order-0 swapin, even mistakenly
when !CONFIG_ZSWAP.
Basically this bug makes Barry's in progress patches not work at all.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mm: zswap: fix zswap_never_enabled() for CONFIG_ZSWAP==N
2024-06-29 23:22 [PATCH] mm: zswap: fix zswap_never_enabled() for CONFIG_ZSWAP==N Barry Song
` (3 preceding siblings ...)
2024-07-02 6:52 ` Andrew Morton
@ 2024-07-02 6:58 ` David Hildenbrand
2024-07-03 23:22 ` Nhat Pham
5 siblings, 0 replies; 10+ messages in thread
From: David Hildenbrand @ 2024-07-02 6:58 UTC (permalink / raw)
To: Barry Song, akpm, linux-mm
Cc: linux-kernel, Barry Song, Yosry Ahmed, Nhat Pham, Chengming Zhou,
Chris Li, Johannes Weiner, Matthew Wilcox
On 30.06.24 01:22, Barry Song wrote:
> From: Barry Song <v-songbaohua@oppo.com>
>
> If CONFIG_ZSWAP is set to N, it means zswap cannot be enabled.
> zswap_never_enabled() should return true.
>
> Fixes: 0300e17d67c3 ("mm: zswap: add zswap_never_enabled()")
> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> Cc: Yosry Ahmed <yosryahmed@google.com>
> Cc: Nhat Pham <nphamcs@gmail.com>
> Cc: Chengming Zhou <chengming.zhou@linux.dev>
> Cc: Chris Li <chrisl@kernel.org>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
> include/linux/zswap.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/zswap.h b/include/linux/zswap.h
> index bf83ae5e285d..6cecb4a4f68b 100644
> --- a/include/linux/zswap.h
> +++ b/include/linux/zswap.h
> @@ -68,7 +68,7 @@ static inline bool zswap_is_enabled(void)
>
> static inline bool zswap_never_enabled(void)
> {
> - return false;
> + return true;
> }
>
> #endif
Acked-by: David Hildenbrand <david@redhat.com>
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] mm: zswap: fix zswap_never_enabled() for CONFIG_ZSWAP==N
2024-06-29 23:22 [PATCH] mm: zswap: fix zswap_never_enabled() for CONFIG_ZSWAP==N Barry Song
` (4 preceding siblings ...)
2024-07-02 6:58 ` David Hildenbrand
@ 2024-07-03 23:22 ` Nhat Pham
5 siblings, 0 replies; 10+ messages in thread
From: Nhat Pham @ 2024-07-03 23:22 UTC (permalink / raw)
To: Barry Song
Cc: akpm, linux-mm, linux-kernel, Barry Song, Yosry Ahmed,
Chengming Zhou, Chris Li, David Hildenbrand, Johannes Weiner,
Matthew Wilcox
On Sat, Jun 29, 2024 at 4:22 PM Barry Song <21cnbao@gmail.com> wrote:
>
> From: Barry Song <v-songbaohua@oppo.com>
>
> If CONFIG_ZSWAP is set to N, it means zswap cannot be enabled.
> zswap_never_enabled() should return true.
>
> Fixes: 0300e17d67c3 ("mm: zswap: add zswap_never_enabled()")
> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> Cc: Yosry Ahmed <yosryahmed@google.com>
> Cc: Nhat Pham <nphamcs@gmail.com>
> Cc: Chengming Zhou <chengming.zhou@linux.dev>
> Cc: Chris Li <chrisl@kernel.org>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
> include/linux/zswap.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/zswap.h b/include/linux/zswap.h
> index bf83ae5e285d..6cecb4a4f68b 100644
> --- a/include/linux/zswap.h
> +++ b/include/linux/zswap.h
> @@ -68,7 +68,7 @@ static inline bool zswap_is_enabled(void)
>
> static inline bool zswap_never_enabled(void)
> {
> - return false;
> + return true;
> }
>
> #endif
> --
> 2.34.1
>
That's some pretty yikesy bug :) Thankfully this seems unused thus far
(which is probably why it is not caught until now).
Thanks for fixing this, Barry!
Reviewed-by: Nhat Pham <nphamcs@gmail.com>
^ permalink raw reply [flat|nested] 10+ messages in thread