Received-SPF: pass (google.com: domain of linux-kernel+bounces-209066-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240608023654.3513385-1-yosryahmed@google.com>
 <CAGsJ_4yVmDKtcKKAdjPOkOMWB+=ZT5TrrWz188xJaTV4EpM1Bw@mail.gmail.com>
 <CAJD7tkaHLVcjUgiUFfBK+ztCqxBTRfyVbSVH9vytK=5JYOw+Sw@mail.gmail.com>
 <CAGsJ_4w-magFysq4uLBm46AzHLD+r=v6pJphwmQn+OFvECHjrA@mail.gmail.com>
 <CAJD7tkYdq533Z7nubjFT5jQYuS4oq2u15RAz2oGHGxYSk5Oicg@mail.gmail.com>
 <CAGsJ_4zNxC5u088RRnKeM18skEJvwTd22mB_FWSA67K3S-CKPw@mail.gmail.com>
 <CAJD7tkb0Rv4mSPS3DXqF888iVwd++nd99N3WrZYuJhLPDN+dhA@mail.gmail.com> <CAGsJ_4ztBavP+ic15V1F0-KUhoE1zh08xuOZ3jMMfuHu=JHNEw@mail.gmail.com>
In-Reply-To: <CAGsJ_4ztBavP+ic15V1F0-KUhoE1zh08xuOZ3jMMfuHu=JHNEw@mail.gmail.com>
From: Yosry Ahmed <yosryahmed@google.com>
Date: Mon, 10 Jun 2024 16:41:14 -0700
Message-ID: <CAJD7tka2e1tG6vxU2XXKrzZBUBAc1EfcvaLU+yhQbzZO2gh0=g@mail.gmail.com>
Subject: Re: [PATCH v2] mm: zswap: handle incorrect attempts to load of large folios
To: Barry Song <21cnbao@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>, Johannes Weiner <hannes@cmpxchg.org>, 
	Nhat Pham <nphamcs@gmail.com>, Chengming Zhou <chengming.zhou@linux.dev>, 
	Baolin Wang <baolin.wang@linux.alibaba.com>, Chris Li <chrisl@kernel.org>, 
	Ryan Roberts <ryan.roberts@arm.com>, David Hildenbrand <david@redhat.com>, 
	Matthew Wilcox <willy@infradead.org>, linux-mm@kvack.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

[..]
> > > > We can't always WARN_ON for large folios, as this will fire even if
> > > > zswap was never enabled. The alternative is tracking whether zswap was
> > > > ever enabled, and checking that instead of checking if any part of the
> > > > folio is in zswap.
> > > >
> > > > Basically replacing xa_find(..) with zswap_was_enabled(..) or something.
> > >
> > > My point is that mm core should always fallback
> > >
> > > if (zswap_was_or_is_enabled())
> > >      goto fallback;
> > >
> > > till zswap fixes the issue. This is the only way to enable large folios swap-in
> > > development before we fix zswap.
> >
> > I agree with this, I just want an extra fallback in zswap itself in
> > case something was missed during large folio swapin development (which
> > can evidently happen).
>
> yes. then i feel we only need to warn_on the case mm-core fails to fallback.
>
> I mean, only WARN_ON  is_zswap_ever_enabled&&large folio. there is no
> need to do more. Before zswap brings up the large folio support, mm-core
> will need is_zswap_ever_enabled() to do fallback.

I don't have a problem with doing it this way instead of checking if
any part of the folio is in zswap. Such a check may be needed for core
MM to fallback to order-0 anyway, as we discussed. But I'd rather have
this as a static key since it will never be changed.

Also, I still prefer we do not mark the folio as uptodate in this
case. It is one extra line of code to propagate the kernel warning to
userspace as well and make it much more noticeable.


>
> diff --git a/include/linux/zswap.h b/include/linux/zswap.h
> index 2a85b941db97..035e51ed89c4 100644
> --- a/include/linux/zswap.h
> +++ b/include/linux/zswap.h
> @@ -36,6 +36,7 @@ void zswap_memcg_offline_cleanup(struct mem_cgroup *memcg);
>  void zswap_lruvec_state_init(struct lruvec *lruvec);
>  void zswap_folio_swapin(struct folio *folio);
>  bool is_zswap_enabled(void);
> +bool is_zswap_ever_enabled(void);
>  #else
>
>  struct zswap_lruvec_state {};
> @@ -65,6 +66,10 @@ static inline bool is_zswap_enabled(void)
>         return false;
>  }
>
> +static inline bool is_zswap_ever_enabled(void)
> +{
> +       return false;
> +}
>  #endif
>
>  #endif /* _LINUX_ZSWAP_H */
> diff --git a/mm/zswap.c b/mm/zswap.c
> index b9b35ef86d9b..bf2da5d37e47 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -86,6 +86,9 @@ static int zswap_setup(void);
>  static bool zswap_enabled = IS_ENABLED(CONFIG_ZSWAP_DEFAULT_ON);
>  static int zswap_enabled_param_set(const char *,
>                                    const struct kernel_param *);
> +
> +static bool zswap_ever_enable;
> +
>  static const struct kernel_param_ops zswap_enabled_param_ops = {
>         .set =          zswap_enabled_param_set,
>         .get =          param_get_bool,
> @@ -136,6 +139,11 @@ bool is_zswap_enabled(void)
>         return zswap_enabled;
>  }
>
> +bool is_zswap_ever_enabled(void)
> +{
> +       return zswap_enabled || zswap_ever_enabled;
> +}
> +
>  /*********************************
>  * data structures
>  **********************************/
> @@ -1734,6 +1742,7 @@ static int zswap_setup(void)
>                 pr_info("loaded using pool %s/%s\n", pool->tfm_name,
>                         zpool_get_type(pool->zpools[0]));
>                 list_add(&pool->list, &zswap_pools);
> +               zswap_ever_enabled = true;
>                 zswap_has_pool = true;
>         } else {
>                 pr_err("pool creation failed\n");
>
> Thanks
> Barry