On Tue, Jun 11, 2024 at 11:41 AM Yosry Ahmed <yosryahmed@xxxxxxxxxx> 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 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.
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 *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
