Re: [PATCH v2] mm: zswap: handle incorrect attempts to load of large folios

[Barry Song]

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