Re: [PATCH v3 2/3] mm: zswap: add zswap_never_enabled()

[Barry Song <21cnbao@gmail.com>]

On Wed, Jun 12, 2024 at 9:55 AM Yosry Ahmed <yosryahmed@google.com> wrote:
>
> On Tue, Jun 11, 2024 at 2:53 PM Barry Song <21cnbao@gmail.com> wrote:
> >
> > On Tue, Jun 11, 2024 at 2:45 PM Yosry Ahmed <yosryahmed@google.com> wrote:
> > >
> > > Add zswap_never_enabled() to skip the xarray lookup in zswap_load() if
> > > zswap was never enabled on the system. It is implemented using static
> > > branches for efficiency, as enabling zswap should be a rare event. This
> > > could shave some cycles off zswap_load() when CONFIG_ZSWAP is used but
> > > zswap is never enabled.
> > >
> > > However, the real motivation behind this patch is two-fold:
> > > - Incoming large folio swapin work will need to fallback to order-0
> > >   folios if zswap was ever enabled, because any part of the folio could
> > >   be in zswap, until proper handling of large folios with zswap is
> > >   added.
> > >
> > > - A warning and recovery attempt will be added in a following change in
> > >   case the above was not done incorrectly. Zswap will fail the read if
> > >   the folio is large and it was ever enabled.
> > >
> > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
> > > ---
> > >  mm/zswap.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/mm/zswap.c b/mm/zswap.c
> > > index a8c8dd8cfe6f5..7fcd751e847d6 100644
> > > --- a/mm/zswap.c
> > > +++ b/mm/zswap.c
> > > @@ -83,6 +83,7 @@ static bool zswap_pool_reached_full;
> > >  static int zswap_setup(void);
> > >
> > >  /* Enable/disable zswap */
> > > +static DEFINE_STATIC_KEY_MAYBE(CONFIG_ZSWAP_DEFAULT_ON, zswap_ever_enabled);
> > >  static bool zswap_enabled = IS_ENABLED(CONFIG_ZSWAP_DEFAULT_ON);
> > >  static int zswap_enabled_param_set(const char *,
> > >                                    const struct kernel_param *);
> > > @@ -136,6 +137,11 @@ bool zswap_is_enabled(void)
> > >         return zswap_enabled;
> > >  }
> > >
> > > +static bool zswap_never_enabled(void)
> > > +{
> > > +       return !static_branch_maybe(CONFIG_ZSWAP_DEFAULT_ON, &zswap_ever_enabled);
> > > +}
> >
> > Will we "extern" this one so that mm-core can use it to fallback
> > to small folios?
> > or you prefer this to be done within the coming swapin series?
>
> My intention was to keep it static for now, and expose it in the
> header when needed (in the swapin series). If others think it's better
> to do this now to avoid the churn I am happy to do it as well.

Personally, I'd vote for exposing it now to avoid one more patch which might
come shortly. And this patchset serves the clear purpose of drawing attention
from mm-core to fallback to small folios.

Thanks
Barry