Re: [PATCH 2/2] mm/zswap: move `zswap_has_pool` to front of `if ()`
On Tue, Jan 2, 2018 at 5:03 AM, Joey Pabalinaswrote: > `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 > > 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 >
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 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 > > 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 >
[PATCH 2/2] mm/zswap: move `zswap_has_pool` to front of `if ()`
`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 Pabalinas1 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)) return 0; /* if this is load-time (pre-init) param setting, -- 2.15.1
[PATCH 2/2] mm/zswap: move `zswap_has_pool` to front of `if ()`
`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 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)) return 0; /* if this is load-time (pre-init) param setting, -- 2.15.1