linux-kernel - Re: [PATCH 2/2] mm/zswap: move `zswap_has

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CALZtOND_qRY1ctLRNVGP=xu01h+FieUTnKQ3xgf7c+r+tsAxPA@mail.gmail.com>
Date:   Mon, 8 Jan 2018 14:34:32 -0500
From:   Dan Streetman <ddstreet@...e.org>
To:     Joey Pabalinas <joeypabalinas@...il.com>
Cc:     Linux-MM <linux-mm@...ck.org>, Seth Jennings <sjenning@...hat.com>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] mm/zswap: move `zswap_has_pool` to front of `if ()`

On Tue, Jan 2, 2018 at 5:03 AM, Joey Pabalinas <joeypabalinas@...il.com> 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@...il.com>
>
>  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
>