Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1200793 > unrolled thread

[PATCH 0/3] make zswap params changeable at runtime

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] make zswap params changeable at runtime Dan Streetman <ddstreet@ieee.org> - 2015-08-05 16:00 +0200
    [PATCH 3/3] zswap: change zpool/compressor at runtime Dan Streetman <ddstreet@ieee.org> - 2015-08-05 16:00 +0200
      Re: [PATCH 3/3] zswap: change zpool/compressor at runtime Andrew Morton <akpm@linux-foundation.org> - 2015-08-05 22:20 +0200
        Re: [PATCH 3/3] zswap: change zpool/compressor at runtime Dan Streetman <ddstreet@ieee.org> - 2015-08-06 12:10 +0200
          [PATCH] zswap: comment clarifying maxlen Dan Streetman <ddstreet@ieee.org> - 2015-08-06 20:00 +0200
      Re: [PATCH 3/3] zswap: change zpool/compressor at runtime Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2015-08-06 02:10 +0200
        Re: [PATCH 3/3] zswap: change zpool/compressor at runtime Dan Streetman <ddstreet@ieee.org> - 2015-08-06 12:30 +0200
          Re: [PATCH 3/3] zswap: change zpool/compressor at runtime Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2015-08-06 13:00 +0200
            Re: [PATCH 3/3] zswap: change zpool/compressor at runtime Dan Streetman <ddstreet@ieee.org> - 2015-08-06 13:10 +0200

#1200793 — [PATCH 0/3] make zswap params changeable at runtime

This is a resend of the patch series.  It makes creation of the zpool and
compressor dynamic, so that they can be changed at runtime.  This makes
using/configuring zswap easier, as before this zswap had to be configured
at boot time, using boot params.

This uses a single list to track both the zpool and compressor together,
although Seth had mentioned an alternative which is to track the zpools
and compressors using separate lists.  In the most common case, only a
single zpool and single compressor, using one list is slightly simpler
than using two lists, and for the uncommon case of multiple zpools and/or
compressors, using one list is slightly less simple (and uses slightly
more memory, probably) than using two lists.

Dan Streetman (3):
  zpool: add zpool_has_pool()
  zswap: dynamic pool creation
  zswap: change zpool/compressor at runtime

 include/linux/zpool.h |   2 +
 mm/zpool.c            |  25 ++
 mm/zswap.c            | 683 ++++++++++++++++++++++++++++++++++++++------------
 3 files changed, 555 insertions(+), 155 deletions(-)

-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1200801 — [PATCH 3/3] zswap: change zpool/compressor at runtime

Update the zpool and compressor parameters to be changeable at runtime.
When changed, a new pool is created with the requested zpool/compressor,
and added as the current pool at the front of the pool list.  Previous
pools remain in the list only to remove existing compressed pages from.
The old pool(s) are removed once they become empty.

Signed-off-by: Dan Streetman <ddstreet@ieee.org>
---
 mm/zswap.c | 135 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 122 insertions(+), 13 deletions(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index f8fcd7e..3eaff21 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -80,23 +80,39 @@ static u64 zswap_duplicate_entry;
 static bool zswap_enabled;
 module_param_named(enabled, zswap_enabled, bool, 0644);
 
-/* Compressor to be used by zswap (fixed at boot for now) */
+/* Crypto compressor to use */
 #define ZSWAP_COMPRESSOR_DEFAULT "lzo"
-static char *zswap_compressor = ZSWAP_COMPRESSOR_DEFAULT;
-module_param_named(compressor, zswap_compressor, charp, 0444);
-
-/* The maximum percentage of memory that the compressed pool can occupy */
-static unsigned int zswap_max_pool_percent = 20;
-module_param_named(max_pool_percent,
-			zswap_max_pool_percent, uint, 0644);
+static char zswap_compressor[CRYPTO_MAX_ALG_NAME] = ZSWAP_COMPRESSOR_DEFAULT;
+static struct kparam_string zswap_compressor_kparam = {
+	.string =	zswap_compressor,
+	.maxlen =	sizeof(zswap_compressor),
+};
+static int zswap_compressor_param_set(const char *,
+				      const struct kernel_param *);
+static struct kernel_param_ops zswap_compressor_param_ops = {
+	.set =		zswap_compressor_param_set,
+	.get =		param_get_string,
+};
+module_param_cb(compressor, &zswap_compressor_param_ops,
+		&zswap_compressor_kparam, 0644);
 
-/* Compressed storage to use */
+/* Compressed storage zpool to use */
 #define ZSWAP_ZPOOL_DEFAULT "zbud"
-static char *zswap_zpool_type = ZSWAP_ZPOOL_DEFAULT;
-module_param_named(zpool, zswap_zpool_type, charp, 0444);
+static char zswap_zpool_type[32 /* arbitrary */] = ZSWAP_ZPOOL_DEFAULT;
+static struct kparam_string zswap_zpool_kparam = {
+	.string =	zswap_zpool_type,
+	.maxlen =	sizeof(zswap_zpool_type),
+};
+static int zswap_zpool_param_set(const char *, const struct kernel_param *);
+static struct kernel_param_ops zswap_zpool_param_ops = {
+	.set =	zswap_zpool_param_set,
+	.get =	param_get_string,
+};
+module_param_cb(zpool, &zswap_zpool_param_ops, &zswap_zpool_kparam, 0644);
 
-/* zpool is shared by all of zswap backend  */
-static struct zpool *zswap_pool;
+/* The maximum percentage of memory that the compressed pool can occupy */
+static unsigned int zswap_max_pool_percent = 20;
+module_param_named(max_pool_percent, zswap_max_pool_percent, uint, 0644);
 
 /*********************************
 * data structures
@@ -161,6 +177,9 @@ static LIST_HEAD(zswap_pools);
 /* protects zswap_pools list modification */
 static DEFINE_SPINLOCK(zswap_pools_lock);
 
+/* used by param callback function */
+static bool zswap_init_started;
+
 /*********************************
 * helpers and fwd declarations
 **********************************/
@@ -661,6 +680,94 @@ static void zswap_pool_put(struct zswap_pool *pool)
 	kref_put(&pool->kref, __zswap_pool_empty);
 }
 
+/*********************************
+* param callbacks
+**********************************/
+
+static int __zswap_param_set(const char *val, const struct kernel_param *kp,
+			     char *type, char *compressor)
+{
+	struct zswap_pool *pool, *put_pool = NULL;
+	char str[kp->str->maxlen], *s;
+	int ret;
+
+	strlcpy(str, val, kp->str->maxlen);
+	s = strim(str);
+
+	/* if this is load-time (pre-init) param setting,
+	 * don't create a pool; that's done during init.
+	 */
+	if (!zswap_init_started)
+		return param_set_copystring(s, kp);
+
+	/* no change required */
+	if (!strncmp(kp->str->string, s, kp->str->maxlen))
+		return 0;
+
+	if (!type) {
+		type = s;
+		if (!zpool_has_pool(type)) {
+			pr_err("zpool %s not available\n", type);
+			return -ENOENT;
+		}
+	} else if (!compressor) {
+		compressor = s;
+		if (!crypto_has_comp(compressor, 0, 0)) {
+			pr_err("compressor %s not available\n", compressor);
+			return -ENOENT;
+		}
+	}
+
+	spin_lock(&zswap_pools_lock);
+
+	pool = zswap_pool_find_get(type, compressor);
+	if (pool) {
+		zswap_pool_debug("using existing", pool);
+		list_del_rcu(&pool->list);
+	} else {
+		spin_unlock(&zswap_pools_lock);
+		pool = zswap_pool_create(type, compressor);
+		spin_lock(&zswap_pools_lock);
+	}
+
+	if (pool)
+		ret = param_set_copystring(s, kp);
+	else
+		ret = -EINVAL;
+
+	if (!ret) {
+		put_pool = zswap_pool_current();
+		list_add_rcu(&pool->list, &zswap_pools);
+	} else if (pool) {
+		/* add the possibly pre-existing pool to the end of the pools
+		 * list; if it's new (and empty) then it'll be removed and
+		 * destroyed by the put after we drop the lock
+		 */
+		list_add_tail_rcu(&pool->list, &zswap_pools);
+		put_pool = pool;
+	}
+
+	spin_unlock(&zswap_pools_lock);
+
+	/* drop the ref from either the old current pool,
+	 * or the new pool we failed to add
+	 */
+	if (put_pool)
+		zswap_pool_put(put_pool);
+
+	return ret;
+}
+
+static int zswap_compressor_param_set(const char *val,
+				      const struct kernel_param *kp)
+{
+	return __zswap_param_set(val, kp, zswap_zpool_type, NULL);
+}
+
+static int zswap_zpool_param_set(const char *val,
+				 const struct kernel_param *kp)
+{
+	return __zswap_param_set(val, kp, NULL, zswap_compressor);
 }
 
 /*********************************
@@ -1116,6 +1223,8 @@ static int __init init_zswap(void)
 {
 	struct zswap_pool *pool;
 
+	zswap_init_started = true;
+
 	if (zswap_entry_cache_create()) {
 		pr_err("entry cache creation failed\n");
 		goto cache_fail;
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1201118 — Re: [PATCH 3/3] zswap: change zpool/compressor at runtime

On Wed,  5 Aug 2015 09:46:43 -0400 Dan Streetman <ddstreet@ieee.org> wrote:

> Update the zpool and compressor parameters to be changeable at runtime.
> When changed, a new pool is created with the requested zpool/compressor,
> and added as the current pool at the front of the pool list.  Previous
> pools remain in the list only to remove existing compressed pages from.
> The old pool(s) are removed once they become empty.
> 
> +/*********************************
> +* param callbacks
> +**********************************/
> +
> +static int __zswap_param_set(const char *val, const struct kernel_param *kp,
> +			     char *type, char *compressor)
> +{
> +	struct zswap_pool *pool, *put_pool = NULL;
> +	char str[kp->str->maxlen], *s;

What's the upper bound on the size of this variable-sized array?

> +	int ret;
> +
> +	strlcpy(str, val, kp->str->maxlen);
> +	s = strim(str);
> +
> +	/* if this is load-time (pre-init) param setting,

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1201618 — Re: [PATCH 3/3] zswap: change zpool/compressor at runtime

On Wed, Aug 5, 2015 at 4:14 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed,  5 Aug 2015 09:46:43 -0400 Dan Streetman <ddstreet@ieee.org> wrote:
>
>> Update the zpool and compressor parameters to be changeable at runtime.
>> When changed, a new pool is created with the requested zpool/compressor,
>> and added as the current pool at the front of the pool list.  Previous
>> pools remain in the list only to remove existing compressed pages from.
>> The old pool(s) are removed once they become empty.
>>
>> +/*********************************
>> +* param callbacks
>> +**********************************/
>> +
>> +static int __zswap_param_set(const char *val, const struct kernel_param *kp,
>> +                          char *type, char *compressor)
>> +{
>> +     struct zswap_pool *pool, *put_pool = NULL;
>> +     char str[kp->str->maxlen], *s;
>
> What's the upper bound on the size of this variable-sized array?

the kernel_param in this function will always be either
zswap_compressor_kparam or zswap_zpool_kparam, which are defined at
the top, and their maxlen fields are set to sizeof(their string),
which is either CRYPTO_MAX_ALG_NAME (currently 64) or 32 (arbitrary
max for zpool name).

I can also add a comment here to clarify that.

>
>> +     int ret;
>> +
>> +     strlcpy(str, val, kp->str->maxlen);
>> +     s = strim(str);
>> +
>> +     /* if this is load-time (pre-init) param setting,
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1201949 — [PATCH] zswap: comment clarifying maxlen

Add a comment clarifying the variable-size array created on the stack will
always be either CRYPTO_MAX_ALG_NAME (64) or 32 bytes long.

Signed-off-by: Dan Streetman <ddstreet@ieee.org>
---
 mm/zswap.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/mm/zswap.c b/mm/zswap.c
index 7bbecd9..b198081 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -691,6 +691,11 @@ static int __zswap_param_set(const char *val, const struct kernel_param *kp,
 	char str[kp->str->maxlen], *s;
 	int ret;
 
+	/*
+	 * kp is either zswap_zpool_kparam or zswap_compressor_kparam, defined
+	 * at the top of this file, so maxlen is CRYPTO_MAX_ALG_NAME (64) or
+	 * 32 (arbitrary).
+	 */
 	strlcpy(str, val, kp->str->maxlen);
 	s = strim(str);
 
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1201355 — Re: [PATCH 3/3] zswap: change zpool/compressor at runtime

Hi,

On (08/05/15 09:46), Dan Streetman wrote:
> Update the zpool and compressor parameters to be changeable at runtime.
> When changed, a new pool is created with the requested zpool/compressor,
> and added as the current pool at the front of the pool list.  Previous
> pools remain in the list only to remove existing compressed pages from.
> The old pool(s) are removed once they become empty.
> 

Sorry, just curious, is this functionality/complication really
necessary? How often do you expect people to do that? The way I
see it -- a static configuration works just fine: boot, test,
re-configure, boot test; compare the results and done.

	-ss

> Signed-off-by: Dan Streetman <ddstreet@ieee.org>
> ---
>  mm/zswap.c | 135 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 122 insertions(+), 13 deletions(-)
> 
> diff --git a/mm/zswap.c b/mm/zswap.c
> index f8fcd7e..3eaff21 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -80,23 +80,39 @@ static u64 zswap_duplicate_entry;
>  static bool zswap_enabled;
>  module_param_named(enabled, zswap_enabled, bool, 0644);
>  
> -/* Compressor to be used by zswap (fixed at boot for now) */
> +/* Crypto compressor to use */
>  #define ZSWAP_COMPRESSOR_DEFAULT "lzo"
> -static char *zswap_compressor = ZSWAP_COMPRESSOR_DEFAULT;
> -module_param_named(compressor, zswap_compressor, charp, 0444);
> -
> -/* The maximum percentage of memory that the compressed pool can occupy */
> -static unsigned int zswap_max_pool_percent = 20;
> -module_param_named(max_pool_percent,
> -			zswap_max_pool_percent, uint, 0644);
> +static char zswap_compressor[CRYPTO_MAX_ALG_NAME] = ZSWAP_COMPRESSOR_DEFAULT;
> +static struct kparam_string zswap_compressor_kparam = {
> +	.string =	zswap_compressor,
> +	.maxlen =	sizeof(zswap_compressor),
> +};
> +static int zswap_compressor_param_set(const char *,
> +				      const struct kernel_param *);
> +static struct kernel_param_ops zswap_compressor_param_ops = {
> +	.set =		zswap_compressor_param_set,
> +	.get =		param_get_string,
> +};
> +module_param_cb(compressor, &zswap_compressor_param_ops,
> +		&zswap_compressor_kparam, 0644);
>  
> -/* Compressed storage to use */
> +/* Compressed storage zpool to use */
>  #define ZSWAP_ZPOOL_DEFAULT "zbud"
> -static char *zswap_zpool_type = ZSWAP_ZPOOL_DEFAULT;
> -module_param_named(zpool, zswap_zpool_type, charp, 0444);
> +static char zswap_zpool_type[32 /* arbitrary */] = ZSWAP_ZPOOL_DEFAULT;
> +static struct kparam_string zswap_zpool_kparam = {
> +	.string =	zswap_zpool_type,
> +	.maxlen =	sizeof(zswap_zpool_type),
> +};
> +static int zswap_zpool_param_set(const char *, const struct kernel_param *);
> +static struct kernel_param_ops zswap_zpool_param_ops = {
> +	.set =	zswap_zpool_param_set,
> +	.get =	param_get_string,
> +};
> +module_param_cb(zpool, &zswap_zpool_param_ops, &zswap_zpool_kparam, 0644);
>  
> -/* zpool is shared by all of zswap backend  */
> -static struct zpool *zswap_pool;
> +/* The maximum percentage of memory that the compressed pool can occupy */
> +static unsigned int zswap_max_pool_percent = 20;
> +module_param_named(max_pool_percent, zswap_max_pool_percent, uint, 0644);
>  
>  /*********************************
>  * data structures
> @@ -161,6 +177,9 @@ static LIST_HEAD(zswap_pools);
>  /* protects zswap_pools list modification */
>  static DEFINE_SPINLOCK(zswap_pools_lock);
>  
> +/* used by param callback function */
> +static bool zswap_init_started;
> +
>  /*********************************
>  * helpers and fwd declarations
>  **********************************/
> @@ -661,6 +680,94 @@ static void zswap_pool_put(struct zswap_pool *pool)
>  	kref_put(&pool->kref, __zswap_pool_empty);
>  }
>  
> +/*********************************
> +* param callbacks
> +**********************************/
> +
> +static int __zswap_param_set(const char *val, const struct kernel_param *kp,
> +			     char *type, char *compressor)
> +{
> +	struct zswap_pool *pool, *put_pool = NULL;
> +	char str[kp->str->maxlen], *s;
> +	int ret;
> +
> +	strlcpy(str, val, kp->str->maxlen);
> +	s = strim(str);
> +
> +	/* if this is load-time (pre-init) param setting,
> +	 * don't create a pool; that's done during init.
> +	 */
> +	if (!zswap_init_started)
> +		return param_set_copystring(s, kp);
> +
> +	/* no change required */
> +	if (!strncmp(kp->str->string, s, kp->str->maxlen))
> +		return 0;
> +
> +	if (!type) {
> +		type = s;
> +		if (!zpool_has_pool(type)) {
> +			pr_err("zpool %s not available\n", type);
> +			return -ENOENT;
> +		}
> +	} else if (!compressor) {
> +		compressor = s;
> +		if (!crypto_has_comp(compressor, 0, 0)) {
> +			pr_err("compressor %s not available\n", compressor);
> +			return -ENOENT;
> +		}
> +	}
> +
> +	spin_lock(&zswap_pools_lock);
> +
> +	pool = zswap_pool_find_get(type, compressor);
> +	if (pool) {
> +		zswap_pool_debug("using existing", pool);
> +		list_del_rcu(&pool->list);
> +	} else {
> +		spin_unlock(&zswap_pools_lock);
> +		pool = zswap_pool_create(type, compressor);
> +		spin_lock(&zswap_pools_lock);
> +	}
> +
> +	if (pool)
> +		ret = param_set_copystring(s, kp);
> +	else
> +		ret = -EINVAL;
> +
> +	if (!ret) {
> +		put_pool = zswap_pool_current();
> +		list_add_rcu(&pool->list, &zswap_pools);
> +	} else if (pool) {
> +		/* add the possibly pre-existing pool to the end of the pools
> +		 * list; if it's new (and empty) then it'll be removed and
> +		 * destroyed by the put after we drop the lock
> +		 */
> +		list_add_tail_rcu(&pool->list, &zswap_pools);
> +		put_pool = pool;
> +	}
> +
> +	spin_unlock(&zswap_pools_lock);
> +
> +	/* drop the ref from either the old current pool,
> +	 * or the new pool we failed to add
> +	 */
> +	if (put_pool)
> +		zswap_pool_put(put_pool);
> +
> +	return ret;
> +}
> +
> +static int zswap_compressor_param_set(const char *val,
> +				      const struct kernel_param *kp)
> +{
> +	return __zswap_param_set(val, kp, zswap_zpool_type, NULL);
> +}
> +
> +static int zswap_zpool_param_set(const char *val,
> +				 const struct kernel_param *kp)
> +{
> +	return __zswap_param_set(val, kp, NULL, zswap_compressor);
>  }
>  
>  /*********************************
> @@ -1116,6 +1223,8 @@ static int __init init_zswap(void)
>  {
>  	struct zswap_pool *pool;
>  
> +	zswap_init_started = true;
> +
>  	if (zswap_entry_cache_create()) {
>  		pr_err("entry cache creation failed\n");
>  		goto cache_fail;
> -- 
> 2.1.0
> 
> --
> 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>
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1201629 — Re: [PATCH 3/3] zswap: change zpool/compressor at runtime

On Wed, Aug 5, 2015 at 8:08 PM, Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
> Hi,
>
> On (08/05/15 09:46), Dan Streetman wrote:
>> Update the zpool and compressor parameters to be changeable at runtime.
>> When changed, a new pool is created with the requested zpool/compressor,
>> and added as the current pool at the front of the pool list.  Previous
>> pools remain in the list only to remove existing compressed pages from.
>> The old pool(s) are removed once they become empty.
>>
>
> Sorry, just curious, is this functionality/complication really
> necessary?

Well you could ask the same question about many other module params;
can't people just configure everything using boot params?  ;-)

> How often do you expect people to do that? The way I
> see it -- a static configuration works just fine: boot, test,
> re-configure, boot test; compare the results and done.

Sure a static configuration will work (it has since Seth wrote zswap),
but that doesn't guarantee everyone will want to do it that way.
Certainly for testing/development/benchmarking avoiding a reboot is
helpful.  And for long-running and/or critical systems that need to
change their zpool or compressor, for whatever reason, forcing a
reboot isn't desirable.

Why would someone want to change their compressor or zpool?  A simple
exampe comes to mind - maybe they have 1000's of systems and a bug was
found in the current level of compressor or zpool - they would then
have to either reboot all the systems to change to a different
zpool/compressor, or leave it using the known-buggy one.

In addition, a static boot-time configuration requires adding params
to the bootloader configuration, *and* rebuilding the initramfs to
include both the required zpool and compressor.  So even for static
configurations, it's simpler to be able to set the zpool and
compressor immediately after boot, instead of at boot time.

>
>         -ss
>
>> Signed-off-by: Dan Streetman <ddstreet@ieee.org>
>> ---
>>  mm/zswap.c | 135 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
>>  1 file changed, 122 insertions(+), 13 deletions(-)
>>
>> diff --git a/mm/zswap.c b/mm/zswap.c
>> index f8fcd7e..3eaff21 100644
>> --- a/mm/zswap.c
>> +++ b/mm/zswap.c
>> @@ -80,23 +80,39 @@ static u64 zswap_duplicate_entry;
>>  static bool zswap_enabled;
>>  module_param_named(enabled, zswap_enabled, bool, 0644);
>>
>> -/* Compressor to be used by zswap (fixed at boot for now) */
>> +/* Crypto compressor to use */
>>  #define ZSWAP_COMPRESSOR_DEFAULT "lzo"
>> -static char *zswap_compressor = ZSWAP_COMPRESSOR_DEFAULT;
>> -module_param_named(compressor, zswap_compressor, charp, 0444);
>> -
>> -/* The maximum percentage of memory that the compressed pool can occupy */
>> -static unsigned int zswap_max_pool_percent = 20;
>> -module_param_named(max_pool_percent,
>> -                     zswap_max_pool_percent, uint, 0644);
>> +static char zswap_compressor[CRYPTO_MAX_ALG_NAME] = ZSWAP_COMPRESSOR_DEFAULT;
>> +static struct kparam_string zswap_compressor_kparam = {
>> +     .string =       zswap_compressor,
>> +     .maxlen =       sizeof(zswap_compressor),
>> +};
>> +static int zswap_compressor_param_set(const char *,
>> +                                   const struct kernel_param *);
>> +static struct kernel_param_ops zswap_compressor_param_ops = {
>> +     .set =          zswap_compressor_param_set,
>> +     .get =          param_get_string,
>> +};
>> +module_param_cb(compressor, &zswap_compressor_param_ops,
>> +             &zswap_compressor_kparam, 0644);
>>
>> -/* Compressed storage to use */
>> +/* Compressed storage zpool to use */
>>  #define ZSWAP_ZPOOL_DEFAULT "zbud"
>> -static char *zswap_zpool_type = ZSWAP_ZPOOL_DEFAULT;
>> -module_param_named(zpool, zswap_zpool_type, charp, 0444);
>> +static char zswap_zpool_type[32 /* arbitrary */] = ZSWAP_ZPOOL_DEFAULT;
>> +static struct kparam_string zswap_zpool_kparam = {
>> +     .string =       zswap_zpool_type,
>> +     .maxlen =       sizeof(zswap_zpool_type),
>> +};
>> +static int zswap_zpool_param_set(const char *, const struct kernel_param *);
>> +static struct kernel_param_ops zswap_zpool_param_ops = {
>> +     .set =  zswap_zpool_param_set,
>> +     .get =  param_get_string,
>> +};
>> +module_param_cb(zpool, &zswap_zpool_param_ops, &zswap_zpool_kparam, 0644);
>>
>> -/* zpool is shared by all of zswap backend  */
>> -static struct zpool *zswap_pool;
>> +/* The maximum percentage of memory that the compressed pool can occupy */
>> +static unsigned int zswap_max_pool_percent = 20;
>> +module_param_named(max_pool_percent, zswap_max_pool_percent, uint, 0644);
>>
>>  /*********************************
>>  * data structures
>> @@ -161,6 +177,9 @@ static LIST_HEAD(zswap_pools);
>>  /* protects zswap_pools list modification */
>>  static DEFINE_SPINLOCK(zswap_pools_lock);
>>
>> +/* used by param callback function */
>> +static bool zswap_init_started;
>> +
>>  /*********************************
>>  * helpers and fwd declarations
>>  **********************************/
>> @@ -661,6 +680,94 @@ static void zswap_pool_put(struct zswap_pool *pool)
>>       kref_put(&pool->kref, __zswap_pool_empty);
>>  }
>>
>> +/*********************************
>> +* param callbacks
>> +**********************************/
>> +
>> +static int __zswap_param_set(const char *val, const struct kernel_param *kp,
>> +                          char *type, char *compressor)
>> +{
>> +     struct zswap_pool *pool, *put_pool = NULL;
>> +     char str[kp->str->maxlen], *s;
>> +     int ret;
>> +
>> +     strlcpy(str, val, kp->str->maxlen);
>> +     s = strim(str);
>> +
>> +     /* if this is load-time (pre-init) param setting,
>> +      * don't create a pool; that's done during init.
>> +      */
>> +     if (!zswap_init_started)
>> +             return param_set_copystring(s, kp);
>> +
>> +     /* no change required */
>> +     if (!strncmp(kp->str->string, s, kp->str->maxlen))
>> +             return 0;
>> +
>> +     if (!type) {
>> +             type = s;
>> +             if (!zpool_has_pool(type)) {
>> +                     pr_err("zpool %s not available\n", type);
>> +                     return -ENOENT;
>> +             }
>> +     } else if (!compressor) {
>> +             compressor = s;
>> +             if (!crypto_has_comp(compressor, 0, 0)) {
>> +                     pr_err("compressor %s not available\n", compressor);
>> +                     return -ENOENT;
>> +             }
>> +     }
>> +
>> +     spin_lock(&zswap_pools_lock);
>> +
>> +     pool = zswap_pool_find_get(type, compressor);
>> +     if (pool) {
>> +             zswap_pool_debug("using existing", pool);
>> +             list_del_rcu(&pool->list);
>> +     } else {
>> +             spin_unlock(&zswap_pools_lock);
>> +             pool = zswap_pool_create(type, compressor);
>> +             spin_lock(&zswap_pools_lock);
>> +     }
>> +
>> +     if (pool)
>> +             ret = param_set_copystring(s, kp);
>> +     else
>> +             ret = -EINVAL;
>> +
>> +     if (!ret) {
>> +             put_pool = zswap_pool_current();
>> +             list_add_rcu(&pool->list, &zswap_pools);
>> +     } else if (pool) {
>> +             /* add the possibly pre-existing pool to the end of the pools
>> +              * list; if it's new (and empty) then it'll be removed and
>> +              * destroyed by the put after we drop the lock
>> +              */
>> +             list_add_tail_rcu(&pool->list, &zswap_pools);
>> +             put_pool = pool;
>> +     }
>> +
>> +     spin_unlock(&zswap_pools_lock);
>> +
>> +     /* drop the ref from either the old current pool,
>> +      * or the new pool we failed to add
>> +      */
>> +     if (put_pool)
>> +             zswap_pool_put(put_pool);
>> +
>> +     return ret;
>> +}
>> +
>> +static int zswap_compressor_param_set(const char *val,
>> +                                   const struct kernel_param *kp)
>> +{
>> +     return __zswap_param_set(val, kp, zswap_zpool_type, NULL);
>> +}
>> +
>> +static int zswap_zpool_param_set(const char *val,
>> +                              const struct kernel_param *kp)
>> +{
>> +     return __zswap_param_set(val, kp, NULL, zswap_compressor);
>>  }
>>
>>  /*********************************
>> @@ -1116,6 +1223,8 @@ static int __init init_zswap(void)
>>  {
>>       struct zswap_pool *pool;
>>
>> +     zswap_init_started = true;
>> +
>>       if (zswap_entry_cache_create()) {
>>               pr_err("entry cache creation failed\n");
>>               goto cache_fail;
>> --
>> 2.1.0
>>
>> --
>> 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>
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1201641 — Re: [PATCH 3/3] zswap: change zpool/compressor at runtime

On (08/06/15 06:20), Dan Streetman wrote:
> > On (08/05/15 09:46), Dan Streetman wrote:
> >> Update the zpool and compressor parameters to be changeable at runtime.
> >> When changed, a new pool is created with the requested zpool/compressor,
> >> and added as the current pool at the front of the pool list.  Previous
> >> pools remain in the list only to remove existing compressed pages from.
> >> The old pool(s) are removed once they become empty.
> >>
> >
> > Sorry, just curious, is this functionality/complication really
> > necessary?
> 
> Well you could ask the same question about many other module params;
> can't people just configure everything using boot params?  ;-)
> 
> > How often do you expect people to do that? The way I
> > see it -- a static configuration works just fine: boot, test,
> > re-configure, boot test; compare the results and done.
> 
> Sure a static configuration will work (it has since Seth wrote zswap),
> but that doesn't guarantee everyone will want to do it that way.
> Certainly for testing/development/benchmarking avoiding a reboot is
> helpful.  And for long-running and/or critical systems that need to
> change their zpool or compressor, for whatever reason, forcing a
> reboot isn't desirable.

Sorry, I didn't have time to read the patches carefully/attentively
(will do); so my email may be a complete nonsense.

> Why would someone want to change their compressor or zpool?  A simple
> exampe comes to mind - maybe they have 1000's of systems and a bug was
> found in the current level of compressor or zpool - they would then
> have to either reboot all the systems to change to a different
> zpool/compressor, or leave it using the known-buggy one.

Well, if that buggy compressor is being used by other modules
then rebooting is sort of inevitable. But you still preserve pages
compressed with the old compressor and let user access them, right?
Thus read operation possibly will hit the bug regardless of current
'front' pool.

> In addition, a static boot-time configuration requires adding params
> to the bootloader configuration, *and* rebuilding the initramfs to
> include both the required zpool and compressor.  So even for static
> configurations, it's simpler to be able to set the zpool and
> compressor immediately after boot, instead of at boot time.

I mean, it just feels that this is a way too big change for no particular
use case (no offense). It doesn't take much time to figure out (a simple
google request does the trick here) which one of the available compressors
gives best ratio in general or which one has better read/write
(compress/decompress) speeds.

A buggy compressor is a good use case, I agree (with the exception that
reboot is still very much possible). But if someone changes compressing
backend because he or she estimates a better compression ratio or
performance then there will be no immediate benefit -- pages compressed
with the old compressor are still there and it will take some unpredictable
amount of time to drain old pools and to remove them.

	-ss
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1201646 — Re: [PATCH 3/3] zswap: change zpool/compressor at runtime

On Thu, Aug 6, 2015 at 6:59 AM, Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
> On (08/06/15 06:20), Dan Streetman wrote:
>> > On (08/05/15 09:46), Dan Streetman wrote:
>> >> Update the zpool and compressor parameters to be changeable at runtime.
>> >> When changed, a new pool is created with the requested zpool/compressor,
>> >> and added as the current pool at the front of the pool list.  Previous
>> >> pools remain in the list only to remove existing compressed pages from.
>> >> The old pool(s) are removed once they become empty.
>> >>
>> >
>> > Sorry, just curious, is this functionality/complication really
>> > necessary?
>>
>> Well you could ask the same question about many other module params;
>> can't people just configure everything using boot params?  ;-)
>>
>> > How often do you expect people to do that? The way I
>> > see it -- a static configuration works just fine: boot, test,
>> > re-configure, boot test; compare the results and done.
>>
>> Sure a static configuration will work (it has since Seth wrote zswap),
>> but that doesn't guarantee everyone will want to do it that way.
>> Certainly for testing/development/benchmarking avoiding a reboot is
>> helpful.  And for long-running and/or critical systems that need to
>> change their zpool or compressor, for whatever reason, forcing a
>> reboot isn't desirable.
>
> Sorry, I didn't have time to read the patches carefully/attentively
> (will do); so my email may be a complete nonsense.
>
>> Why would someone want to change their compressor or zpool?  A simple
>> exampe comes to mind - maybe they have 1000's of systems and a bug was
>> found in the current level of compressor or zpool - they would then
>> have to either reboot all the systems to change to a different
>> zpool/compressor, or leave it using the known-buggy one.
>
> Well, if that buggy compressor is being used by other modules
> then rebooting is sort of inevitable. But you still preserve pages
> compressed with the old compressor and let user access them, right?
> Thus read operation possibly will hit the bug regardless of current
> 'front' pool.

Yes, currently-compressed pages will be uncompressed using the same
compressor.  It's only freed once all the pages using it have been
removed.

>
>> In addition, a static boot-time configuration requires adding params
>> to the bootloader configuration, *and* rebuilding the initramfs to
>> include both the required zpool and compressor.  So even for static
>> configurations, it's simpler to be able to set the zpool and
>> compressor immediately after boot, instead of at boot time.
>
> I mean, it just feels that this is a way too big change for no particular
> use case (no offense). It doesn't take much time to figure out (a simple
> google request does the trick here) which one of the available compressors
> gives best ratio in general or which one has better read/write
> (compress/decompress) speeds.

There are hardware compressors now, you know (see PowerPC 842 hw
compressor).  While a sw compressor won't fail during use (excepting a
buggy driver), a hw compressor might fail for
who-knows-what-hardware-issue.  I suspect there will be more hw
compressors in the future.

>
> A buggy compressor is a good use case, I agree (with the exception that
> reboot is still very much possible). But if someone changes compressing
> backend because he or she estimates a better compression ratio or
> performance then there will be no immediate benefit -- pages compressed
> with the old compressor are still there and it will take some unpredictable
> amount of time to drain old pools and to remove them.

To me, avoiding the need to set boot parameters through the bootloader
AND the need to rebuild the initramfs is use case enough to justify
this.  I can't think of any other driver configuration that *requires*
updating the bootloader config and rebuilding the initramfs.

>
>         -ss
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web