[PATCH 0/2] mm/zswap: add minor const/conditional optimizations

[PATCH 0/2] mm/zswap: add minor const/conditional optimizations

[Joey Pabalinas]

Make a couple minor short-circuiting order and const changes
  - Since the pointed-to objects are never modified through
    these pointers, const-qualify type and compressor
    variables.
  - Test boolean before calling `strcmp()` to take
    advantage of short-circuiting.

Joey Pabalinas (2):
  mm/zswap: make type and compressor const
  mm/zswap: move `zswap_has_pool` to front of `if ()`

 mm/zswap.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

-- 
2.15.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 1/2] mm/zswap: make type and compressor const

[Joey Pabalinas]

The characters pointed to by `zswap_compressor`, `type`, and `compressor`
aren't ever modified. Add const to the static variable and both parameters in
`zswap_pool_find_get()`, `zswap_pool_create()`, and `__zswap_param_set()`

Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>

 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index d39581a076c3aed1e9..a4f2dfaf9131694265 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -90,7 +90,7 @@ module_param_cb(enabled, &zswap_enabled_param_ops, &zswap_enabled, 0644);
 
 /* Crypto compressor to use */
 #define ZSWAP_COMPRESSOR_DEFAULT "lzo"
-static char *zswap_compressor = ZSWAP_COMPRESSOR_DEFAULT;
+static const char *zswap_compressor = ZSWAP_COMPRESSOR_DEFAULT;
 static int zswap_compressor_param_set(const char *,
 				      const struct kernel_param *);
 static struct kernel_param_ops zswap_compressor_param_ops = {
@@ -475,7 +475,8 @@ static struct zswap_pool *zswap_pool_last_get(void)
 }
 
 /* type and compressor must be null-terminated */
-static struct zswap_pool *zswap_pool_find_get(char *type, char *compressor)
+static struct zswap_pool *zswap_pool_find_get(const char *type,
+					      const char *compressor)
 {
 	struct zswap_pool *pool;
 
@@ -495,7 +496,8 @@ static struct zswap_pool *zswap_pool_find_get(char *type, char *compressor)
 	return NULL;
 }
 
-static struct zswap_pool *zswap_pool_create(char *type, char *compressor)
+static struct zswap_pool *zswap_pool_create(const char *type,
+					    const char *compressor)
 {
 	struct zswap_pool *pool;
 	char name[38]; /* 'zswap' + 32 char (max) num + \0 */
@@ -658,7 +660,7 @@ static void zswap_pool_put(struct zswap_pool *pool)
 
 /* val must be a null-terminated string */
 static int __zswap_param_set(const char *val, const struct kernel_param *kp,
-			     char *type, char *compressor)
+			     const char *type, const char *compressor)
 {
 	struct zswap_pool *pool, *put_pool = NULL;
 	char *s = strstrip((char *)val);
-- 
2.15.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Joey Pabalinas]

`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@gmail.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))
 		return 0;
 
 	/* if this is load-time (pre-init) param setting,
-- 
2.15.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] mm/zswap: make type and compressor const

[Dan Streetman]

On Tue, Jan 2, 2018 at 5:03 AM, Joey Pabalinas <joeypabalinas@gmail.com> wrote:
> The characters pointed to by `zswap_compressor`, `type`, and `compressor`
> aren't ever modified. Add const to the static variable and both parameters in
> `zswap_pool_find_get()`, `zswap_pool_create()`, and `__zswap_param_set()`
>
> Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>

Nak.

Those variables are not const; they are updated in
__zswap_param_set().  They aren't modified in pool_find_get() or
pool_create(), but they certainly aren't globally const.

>
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index d39581a076c3aed1e9..a4f2dfaf9131694265 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -90,7 +90,7 @@ module_param_cb(enabled, &zswap_enabled_param_ops, &zswap_enabled, 0644);
>
>  /* Crypto compressor to use */
>  #define ZSWAP_COMPRESSOR_DEFAULT "lzo"
> -static char *zswap_compressor = ZSWAP_COMPRESSOR_DEFAULT;
> +static const char *zswap_compressor = ZSWAP_COMPRESSOR_DEFAULT;
>  static int zswap_compressor_param_set(const char *,
>                                       const struct kernel_param *);
>  static struct kernel_param_ops zswap_compressor_param_ops = {
> @@ -475,7 +475,8 @@ static struct zswap_pool *zswap_pool_last_get(void)
>  }
>
>  /* type and compressor must be null-terminated */
> -static struct zswap_pool *zswap_pool_find_get(char *type, char *compressor)
> +static struct zswap_pool *zswap_pool_find_get(const char *type,
> +                                             const char *compressor)
>  {
>         struct zswap_pool *pool;
>
> @@ -495,7 +496,8 @@ static struct zswap_pool *zswap_pool_find_get(char *type, char *compressor)
>         return NULL;
>  }
>
> -static struct zswap_pool *zswap_pool_create(char *type, char *compressor)
> +static struct zswap_pool *zswap_pool_create(const char *type,
> +                                           const char *compressor)
>  {
>         struct zswap_pool *pool;
>         char name[38]; /* 'zswap' + 32 char (max) num + \0 */
> @@ -658,7 +660,7 @@ static void zswap_pool_put(struct zswap_pool *pool)
>
>  /* val must be a null-terminated string */
>  static int __zswap_param_set(const char *val, const struct kernel_param *kp,
> -                            char *type, char *compressor)
> +                            const char *type, const char *compressor)
>  {
>         struct zswap_pool *pool, *put_pool = NULL;
>         char *s = strstrip((char *)val);
> --
> 2.15.1
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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@gmail.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@gmail.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
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>