Re: [PATCH 1/3] mm: zswap: interact directly with zsmalloc

[Yosry Ahmed <yosry.ahmed@linux.dev>]

On Mon, Sep 15, 2025 at 11:36:40AM -0400, Johannes Weiner wrote:
> On Thu, Sep 11, 2025 at 02:30:31PM +0000, Yosry Ahmed wrote:
> > On Wed, Sep 10, 2025 at 09:42:40AM -0400, Johannes Weiner wrote:
> > > @@ -314,6 +314,10 @@ static struct zswap_pool *__zswap_pool_create_fallback(void)
> > >  		}
> > >  	}
> > >  
> > > +	/* Kconfig bug? */
> > > +	if (WARN_ON(!crypto_has_acomp(zswap_compressor, 0, 0)))
> > > +		return NULL;
> > > +
> > >  	return zswap_pool_create(zswap_compressor);
> > >  }
> > 
> > Sure, looks good, although I think it's clearer (and smaller diff) to
> > preserve the old structure instead, up to you:
> > 
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index c88ad61b232cf..bbfc087792648 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -300,18 +300,21 @@ static struct zswap_pool *zswap_pool_create(char *compressor)
> > 
> >  static struct zswap_pool *__zswap_pool_create_fallback(void)
> >  {
> > -       if (!crypto_has_acomp(zswap_compressor, 0, 0) &&
> > +       bool has_comp = crypto_has_acomp(zswap_compressor, 0, 0);
> > +
> > +       if (!has_comp &&
> >             strcmp(zswap_compressor, CONFIG_ZSWAP_COMPRESSOR_DEFAULT)) {
> >                 pr_err("compressor %s not available, using default %s\n",
> >                        zswap_compressor, CONFIG_ZSWAP_COMPRESSOR_DEFAULT);
> >                 param_free_charp(&zswap_compressor);
> >                 zswap_compressor = CONFIG_ZSWAP_COMPRESSOR_DEFAULT;
> > -               if (!crypto_has_acomp(zswap_compressor, 0, 0)) {
> > -                       pr_err("default compressor %s not available\n",
> > -                              zswap_compressor);
> > -                       zswap_compressor = ZSWAP_PARAM_UNSET;
> > -                       return NULL;
> > -               }
> > +               has_comp = crypto_has_acomp(zswap_compressor, 0, 0);
> > +       }
> > +       if (!has_comp) {
> > +               pr_err("default compressor %s not available\n",
> > +                      zswap_compressor);
> > +               zswap_compressor = ZSWAP_PARAM_UNSET;
> > +               return NULL;
> >         }
> 
> No objection to moving the branch instead of adding another one. I'd
> just like to retain the warning, since it shouldn't happen. And ditch
> the bool, IMO it pointlessly splits the test from the consequences.
> 
> If you're fine with this Yosry, Andrew can you please fold it?

LGTM, with this folded:
Acked-by: Yosry Ahmed <yosry.ahmed@linux.dev>

Thanks!

> 
> ---
> 
> From b8fa4c7edd4f3c84853665b47acec8cebb4f4899 Mon Sep 17 00:00:00 2001
> From: Johannes Weiner <hannes@cmpxchg.org>
> Date: Mon, 15 Sep 2025 10:56:15 -0400
> Subject: [PATCH] mm: zswap: interact directly with zsmalloc fix
> 
> Yosry points out that the default compressor check only applies when
> something else is configured and we fall back, but not if it was
> configured out of the box but isn't available. Move the test. Kconfig
> should not permit this, so replace the pr_err() with a WARN.
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> ---
>  mm/zswap.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/zswap.c b/mm/zswap.c
> index cba7077fda40..c1af782e54ec 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -309,12 +309,12 @@ static struct zswap_pool *__zswap_pool_create_fallback(void)
>  		       zswap_compressor, CONFIG_ZSWAP_COMPRESSOR_DEFAULT);
>  		param_free_charp(&zswap_compressor);
>  		zswap_compressor = CONFIG_ZSWAP_COMPRESSOR_DEFAULT;
> -		if (!crypto_has_acomp(zswap_compressor, 0, 0)) {
> -			pr_err("default compressor %s not available\n",
> -			       zswap_compressor);
> -			zswap_compressor = ZSWAP_PARAM_UNSET;
> -			return NULL;
> -		}
> +	}
> +
> +	/* Default compressor should be available. Kconfig bug? */
> +	if (WARN_ON_ONCE(!crypto_has_acomp(zswap_compressor, 0, 0))) {
> +		zswap_compressor = ZSWAP_PARAM_UNSET;
> +		return NULL;
>  	}
>  
>  	return zswap_pool_create(zswap_compressor);
> -- 
> 2.51.0
>