Re: [PATCH 2/2] mm/zswap: move `zswap_has_pool` to front of `if ()`

[Dan Streetman]

On Tue, Jan 2, 2018 at 5:03 AM, Joey Pabalinas <joeypabalinas@xxxxxxxxx> wrote:
> `zwap_has_pool` is a simple boolean, so it should be tested first
> to avoid unnecessarily calling `strcmp()`. Test `zswap_has_pool`
> first to take advantage of the short-circuiting behavior of && in
> `__zswap_param_set()`.
>
> Signed-off-by: Joey Pabalinas <joeypabalinas@xxxxxxxxx>
>
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index a4f2dfaf9131694265..dbf35139471f692798 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -672,7 +672,7 @@ static int __zswap_param_set(const char *val, const struct kernel_param *kp,
>         }
>
>         /* no change required */
> -       if (!strcmp(s, *(char **)kp->arg) && zswap_has_pool)
> +       if (zswap_has_pool && !strcmp(s, *(char **)kp->arg))

Nak.

This function is only called when actually changing one of the zswap
module params, which is extremely rare (typically once per boot, per
parameter, if at all).  Changing the ordering will have virtually no
noticeable impact on anything.

Additionally, !zswap_has_pool is strictly an initialization-failed
temporary situation (until the compressor/zpool params are be set to
working implementation values), and in all "normal" conditions it will
be true, meaning this reordering will actually
*add* time - the normal path is for this check to *not* be true, so
keeping the strcmp first bypasses bothering with checking
zswap_has_pool.

>                 return 0;
>
>         /* if this is load-time (pre-init) param setting,
> --
> 2.15.1
>