Received-SPF: pass (google.com: domain of linux-kernel+bounces-209108-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::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> <CAJD7tka2e1tG6vxU2XXKrzZBUBAc1EfcvaLU+yhQbzZO2gh0=g@mail.gmail.com>
In-Reply-To: <CAJD7tka2e1tG6vxU2XXKrzZBUBAc1EfcvaLU+yhQbzZO2gh0=g@mail.gmail.com>
From: Barry Song <21cnbao@gmail.com>
Date: Tue, 11 Jun 2024 12:02:45 +1200
Message-ID: <CAGsJ_4zCuwi52BU3sW2tFj67NsxapQoS=g76QHwRo=R1i5ZBCA@mail.gmail.com>
Subject: Re: [PATCH v2] mm: zswap: handle incorrect attempts to load of large folios
To: Yosry Ahmed <yosryahmed@google.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"
Content-Transfer-Encoding: quoted-printable

On Tue, Jun 11, 2024 at 11:41=E2=80=AFAM Yosry Ahmed <yosryahmed@google.com=
> wrote:
>
> [..]
> > > > > We can't always WARN_ON for large folios, as this will fire even =
if
> > > > > zswap was never enabled. The alternative is tracking whether zswa=
p was
> > > > > ever enabled, and checking that instead of checking if any part o=
f the
> > > > > folio is in zswap.
> > > > >
> > > > > Basically replacing xa_find(..) with zswap_was_enabled(..) or som=
ething.
> > > >
> > > > 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 fo=
lios 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 (whic=
h
> > > can evidently happen).
> >
> > yes. then i feel we only need to warn_on the case mm-core fails to fall=
back.
> >
> > 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-cor=
e
> > 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.

right. This is better.

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

right. I have no objection to returning true and skipping mark uptodate.
Just searching xa is not so useful as anyway, we have to either fallback
in mm-core or bring up large folios in zswap.

>
>
> >
> > 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 *m=
emcg);
> >  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 =3D 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 =3D {
> >         .set =3D          zswap_enabled_param_set,
> >         .get =3D          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 =3D true;
> >                 zswap_has_pool =3D true;
> >         } else {
> >                 pr_err("pool creation failed\n");
> >

Thanks
Barry