Re: [PATCH v1 5/5] zram: add fullness knob to control swap full
On Mon, Sep 22, 2014 at 02:17:33PM -0700, Andrew Morton wrote:
> On Mon, 22 Sep 2014 09:03:11 +0900 Minchan Kim wrote:
>
> > Some zram usecase could want lower fullness than default 80 to
> > avoid unnecessary swapout-and-fail-recover overhead.
> >
> > A typical example is that mutliple swap with high piroirty
> > zram-swap and low priority HDD-swap so it could still enough
> > free swap space although one of swap devices is full(ie, zram).
> > It would be better to fail over to HDD-swap rather than failing
> > swap write to zram in this case.
> >
> > This patch exports fullness to user so user can control it
> > via the knob.
>
> Adding new userspace interfaces requires a pretty strong justification
> and it's unclear to me that this is being met. In fact the whole
> patchset reads like "we have some problem, don't know how to fix it so
> let's add a userspace knob to make it someone else's problem".
I explained rationale in 4/5's reply but if it's not enough or wrong,
please tell me.
>
> > index b13dc993291f..817738d14061 100644
> > --- a/Documentation/ABI/testing/sysfs-block-zram
> > +++ b/Documentation/ABI/testing/sysfs-block-zram
> > @@ -138,3 +138,13 @@ Description:
> > amount of memory ZRAM can use to store the compressed data. The
> > limit could be changed in run time and "0" means disable the
> > limit. No limit is the initial state. Unit: bytes
> > +
> > +What: /sys/block/zram/fullness
> > +Date: August 2014
> > +Contact: Minchan Kim
> > +Description:
> > + The fullness file is read/write and specifies how easily
> > + zram become full state so if you set it to lower value,
> > + zram can reach full state easily compared to higher value.
> > + Curretnly, initial value is 80% but it could be changed.
> > + Unit: Percentage
>
> And I don't think that there is sufficient information here for a user
> to be able to work out what to do with this tunable.
I will put more words.
>
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -136,6 +136,37 @@ static ssize_t max_comp_streams_show(struct device
> > *dev,
> > return scnprintf(buf, PAGE_SIZE, "%d\n", val);
> > }
> >
> > +static ssize_t fullness_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + int val;
> > + struct zram *zram = dev_to_zram(dev);
> > +
> > + down_read(>init_lock);
> > + val = zram->fullness;
> > + up_read(>init_lock);
>
> Did we really need to take a lock to display a value which became
> out-of-date as soon as we released that lock?
>
> > + return scnprintf(buf, PAGE_SIZE, "%d\n", val);
> > +}
> > +
> > +static ssize_t fullness_store(struct device *dev,
> > + struct device_attribute *attr, const char *buf, size_t len)
> > +{
> > + int err;
> > + unsigned long val;
> > + struct zram *zram = dev_to_zram(dev);
> > +
> > + err = kstrtoul(buf, 10, );
> > + if (err || val > 100)
> > + return -EINVAL;
>
> This overwrites the kstrtoul() return value.
Will fix.
Thanks for the reivew, Andrew.
--
Kind regards,
Minchan Kim
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 5/5] zram: add fullness knob to control swap full
On Mon, 22 Sep 2014 09:03:11 +0900 Minchan Kim wrote:
> Some zram usecase could want lower fullness than default 80 to
> avoid unnecessary swapout-and-fail-recover overhead.
>
> A typical example is that mutliple swap with high piroirty
> zram-swap and low priority HDD-swap so it could still enough
> free swap space although one of swap devices is full(ie, zram).
> It would be better to fail over to HDD-swap rather than failing
> swap write to zram in this case.
>
> This patch exports fullness to user so user can control it
> via the knob.
Adding new userspace interfaces requires a pretty strong justification
and it's unclear to me that this is being met. In fact the whole
patchset reads like "we have some problem, don't know how to fix it so
let's add a userspace knob to make it someone else's problem".
> index b13dc993291f..817738d14061 100644
> --- a/Documentation/ABI/testing/sysfs-block-zram
> +++ b/Documentation/ABI/testing/sysfs-block-zram
> @@ -138,3 +138,13 @@ Description:
> amount of memory ZRAM can use to store the compressed data. The
> limit could be changed in run time and "0" means disable the
> limit. No limit is the initial state. Unit: bytes
> +
> +What:/sys/block/zram/fullness
> +Date:August 2014
> +Contact: Minchan Kim
> +Description:
> + The fullness file is read/write and specifies how easily
> + zram become full state so if you set it to lower value,
> + zram can reach full state easily compared to higher value.
> + Curretnly, initial value is 80% but it could be changed.
> + Unit: Percentage
And I don't think that there is sufficient information here for a user
to be able to work out what to do with this tunable.
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -136,6 +136,37 @@ static ssize_t max_comp_streams_show(struct device *dev,
> return scnprintf(buf, PAGE_SIZE, "%d\n", val);
> }
>
> +static ssize_t fullness_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int val;
> + struct zram *zram = dev_to_zram(dev);
> +
> + down_read(>init_lock);
> + val = zram->fullness;
> + up_read(>init_lock);
Did we really need to take a lock to display a value which became
out-of-date as soon as we released that lock?
> + return scnprintf(buf, PAGE_SIZE, "%d\n", val);
> +}
> +
> +static ssize_t fullness_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t len)
> +{
> + int err;
> + unsigned long val;
> + struct zram *zram = dev_to_zram(dev);
> +
> + err = kstrtoul(buf, 10, );
> + if (err || val > 100)
> + return -EINVAL;
This overwrites the kstrtoul() return value.
> +
> + down_write(>init_lock);
> + zram->fullness = val;
> + up_write(>init_lock);
> +
> + return len;
> +}
> +
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 5/5] zram: add fullness knob to control swap full
On Mon, 22 Sep 2014 09:03:11 +0900 Minchan Kim minc...@kernel.org wrote:
Some zram usecase could want lower fullness than default 80 to
avoid unnecessary swapout-and-fail-recover overhead.
A typical example is that mutliple swap with high piroirty
zram-swap and low priority HDD-swap so it could still enough
free swap space although one of swap devices is full(ie, zram).
It would be better to fail over to HDD-swap rather than failing
swap write to zram in this case.
This patch exports fullness to user so user can control it
via the knob.
Adding new userspace interfaces requires a pretty strong justification
and it's unclear to me that this is being met. In fact the whole
patchset reads like we have some problem, don't know how to fix it so
let's add a userspace knob to make it someone else's problem.
index b13dc993291f..817738d14061 100644
--- a/Documentation/ABI/testing/sysfs-block-zram
+++ b/Documentation/ABI/testing/sysfs-block-zram
@@ -138,3 +138,13 @@ Description:
amount of memory ZRAM can use to store the compressed data. The
limit could be changed in run time and 0 means disable the
limit. No limit is the initial state. Unit: bytes
+
+What:/sys/block/zramid/fullness
+Date:August 2014
+Contact: Minchan Kim minc...@kernel.org
+Description:
+ The fullness file is read/write and specifies how easily
+ zram become full state so if you set it to lower value,
+ zram can reach full state easily compared to higher value.
+ Curretnly, initial value is 80% but it could be changed.
+ Unit: Percentage
And I don't think that there is sufficient information here for a user
to be able to work out what to do with this tunable.
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -136,6 +136,37 @@ static ssize_t max_comp_streams_show(struct device *dev,
return scnprintf(buf, PAGE_SIZE, %d\n, val);
}
+static ssize_t fullness_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int val;
+ struct zram *zram = dev_to_zram(dev);
+
+ down_read(zram-init_lock);
+ val = zram-fullness;
+ up_read(zram-init_lock);
Did we really need to take a lock to display a value which became
out-of-date as soon as we released that lock?
+ return scnprintf(buf, PAGE_SIZE, %d\n, val);
+}
+
+static ssize_t fullness_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t len)
+{
+ int err;
+ unsigned long val;
+ struct zram *zram = dev_to_zram(dev);
+
+ err = kstrtoul(buf, 10, val);
+ if (err || val 100)
+ return -EINVAL;
This overwrites the kstrtoul() return value.
+
+ down_write(zram-init_lock);
+ zram-fullness = val;
+ up_write(zram-init_lock);
+
+ return len;
+}
+
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v1 5/5] zram: add fullness knob to control swap full
On Mon, Sep 22, 2014 at 02:17:33PM -0700, Andrew Morton wrote:
On Mon, 22 Sep 2014 09:03:11 +0900 Minchan Kim minc...@kernel.org wrote:
Some zram usecase could want lower fullness than default 80 to
avoid unnecessary swapout-and-fail-recover overhead.
A typical example is that mutliple swap with high piroirty
zram-swap and low priority HDD-swap so it could still enough
free swap space although one of swap devices is full(ie, zram).
It would be better to fail over to HDD-swap rather than failing
swap write to zram in this case.
This patch exports fullness to user so user can control it
via the knob.
Adding new userspace interfaces requires a pretty strong justification
and it's unclear to me that this is being met. In fact the whole
patchset reads like we have some problem, don't know how to fix it so
let's add a userspace knob to make it someone else's problem.
I explained rationale in 4/5's reply but if it's not enough or wrong,
please tell me.
index b13dc993291f..817738d14061 100644
--- a/Documentation/ABI/testing/sysfs-block-zram
+++ b/Documentation/ABI/testing/sysfs-block-zram
@@ -138,3 +138,13 @@ Description:
amount of memory ZRAM can use to store the compressed data. The
limit could be changed in run time and 0 means disable the
limit. No limit is the initial state. Unit: bytes
+
+What: /sys/block/zramid/fullness
+Date: August 2014
+Contact: Minchan Kim minc...@kernel.org
+Description:
+ The fullness file is read/write and specifies how easily
+ zram become full state so if you set it to lower value,
+ zram can reach full state easily compared to higher value.
+ Curretnly, initial value is 80% but it could be changed.
+ Unit: Percentage
And I don't think that there is sufficient information here for a user
to be able to work out what to do with this tunable.
I will put more words.
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -136,6 +136,37 @@ static ssize_t max_comp_streams_show(struct device
*dev,
return scnprintf(buf, PAGE_SIZE, %d\n, val);
}
+static ssize_t fullness_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int val;
+ struct zram *zram = dev_to_zram(dev);
+
+ down_read(zram-init_lock);
+ val = zram-fullness;
+ up_read(zram-init_lock);
Did we really need to take a lock to display a value which became
out-of-date as soon as we released that lock?
+ return scnprintf(buf, PAGE_SIZE, %d\n, val);
+}
+
+static ssize_t fullness_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t len)
+{
+ int err;
+ unsigned long val;
+ struct zram *zram = dev_to_zram(dev);
+
+ err = kstrtoul(buf, 10, val);
+ if (err || val 100)
+ return -EINVAL;
This overwrites the kstrtoul() return value.
Will fix.
Thanks for the reivew, Andrew.
--
Kind regards,
Minchan Kim
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v1 5/5] zram: add fullness knob to control swap full
Some zram usecase could want lower fullness than default 80 to
avoid unnecessary swapout-and-fail-recover overhead.
A typical example is that mutliple swap with high piroirty
zram-swap and low priority HDD-swap so it could still enough
free swap space although one of swap devices is full(ie, zram).
It would be better to fail over to HDD-swap rather than failing
swap write to zram in this case.
This patch exports fullness to user so user can control it
via the knob.
Signed-off-by: Minchan Kim
---
Documentation/ABI/testing/sysfs-block-zram | 10
drivers/block/zram/zram_drv.c | 38 +-
drivers/block/zram/zram_drv.h | 1 +
3 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/sysfs-block-zram
b/Documentation/ABI/testing/sysfs-block-zram
index b13dc993291f..817738d14061 100644
--- a/Documentation/ABI/testing/sysfs-block-zram
+++ b/Documentation/ABI/testing/sysfs-block-zram
@@ -138,3 +138,13 @@ Description:
amount of memory ZRAM can use to store the compressed data. The
limit could be changed in run time and "0" means disable the
limit. No limit is the initial state. Unit: bytes
+
+What: /sys/block/zram/fullness
+Date: August 2014
+Contact: Minchan Kim
+Description:
+ The fullness file is read/write and specifies how easily
+ zram become full state so if you set it to lower value,
+ zram can reach full state easily compared to higher value.
+ Curretnly, initial value is 80% but it could be changed.
+ Unit: Percentage
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 649cad9d0b1c..ec3656f6891d 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -136,6 +136,37 @@ static ssize_t max_comp_streams_show(struct device *dev,
return scnprintf(buf, PAGE_SIZE, "%d\n", val);
}
+static ssize_t fullness_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int val;
+ struct zram *zram = dev_to_zram(dev);
+
+ down_read(>init_lock);
+ val = zram->fullness;
+ up_read(>init_lock);
+
+ return scnprintf(buf, PAGE_SIZE, "%d\n", val);
+}
+
+static ssize_t fullness_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t len)
+{
+ int err;
+ unsigned long val;
+ struct zram *zram = dev_to_zram(dev);
+
+ err = kstrtoul(buf, 10, );
+ if (err || val > 100)
+ return -EINVAL;
+
+ down_write(>init_lock);
+ zram->fullness = val;
+ up_write(>init_lock);
+
+ return len;
+}
+
static ssize_t mem_limit_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
@@ -733,6 +764,7 @@ static void zram_reset_device(struct zram *zram, bool
reset_capacity)
zram->limit_pages = 0;
atomic_set(>alloc_fail, 0);
+ zram->fullness = ZRAM_FULLNESS_PERCENT;
if (!init_done(zram)) {
up_write(>init_lock);
@@ -984,7 +1016,7 @@ static int zram_full(struct block_device *bdev, void *arg)
compr_pages = atomic64_read(>stats.compr_data_size)
>> PAGE_SHIFT;
if ((100 * compr_pages / total_pages)
- >= ZRAM_FULLNESS_PERCENT)
+ >= zram->fullness)
return 1;
}
@@ -1020,6 +1052,8 @@ static DEVICE_ATTR(orig_data_size, S_IRUGO,
orig_data_size_show, NULL);
static DEVICE_ATTR(mem_used_total, S_IRUGO, mem_used_total_show, NULL);
static DEVICE_ATTR(mem_limit, S_IRUGO | S_IWUSR, mem_limit_show,
mem_limit_store);
+static DEVICE_ATTR(fullness, S_IRUGO | S_IWUSR, fullness_show,
+ fullness_store);
static DEVICE_ATTR(mem_used_max, S_IRUGO | S_IWUSR, mem_used_max_show,
mem_used_max_store);
static DEVICE_ATTR(max_comp_streams, S_IRUGO | S_IWUSR,
@@ -1051,6 +1085,7 @@ static struct attribute *zram_disk_attrs[] = {
_attr_compr_data_size.attr,
_attr_mem_used_total.attr,
_attr_mem_limit.attr,
+ _attr_fullness.attr,
_attr_mem_used_max.attr,
_attr_max_comp_streams.attr,
_attr_comp_algorithm.attr,
@@ -1132,6 +1167,7 @@ static int create_device(struct zram *zram, int device_id)
strlcpy(zram->compressor, default_compressor, sizeof(zram->compressor));
zram->meta = NULL;
zram->max_comp_streams = 1;
+ zram->fullness = ZRAM_FULLNESS_PERCENT;
return 0;
out_free_disk:
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index fcf3176a9f15..6a9f383d0d78 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -119,6 +119,7 @@ struct zram {
*/
unsigned long limit_pages;
+ int
[PATCH v1 5/5] zram: add fullness knob to control swap full
Some zram usecase could want lower fullness than default 80 to
avoid unnecessary swapout-and-fail-recover overhead.
A typical example is that mutliple swap with high piroirty
zram-swap and low priority HDD-swap so it could still enough
free swap space although one of swap devices is full(ie, zram).
It would be better to fail over to HDD-swap rather than failing
swap write to zram in this case.
This patch exports fullness to user so user can control it
via the knob.
Signed-off-by: Minchan Kim minc...@kernel.org
---
Documentation/ABI/testing/sysfs-block-zram | 10
drivers/block/zram/zram_drv.c | 38 +-
drivers/block/zram/zram_drv.h | 1 +
3 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/sysfs-block-zram
b/Documentation/ABI/testing/sysfs-block-zram
index b13dc993291f..817738d14061 100644
--- a/Documentation/ABI/testing/sysfs-block-zram
+++ b/Documentation/ABI/testing/sysfs-block-zram
@@ -138,3 +138,13 @@ Description:
amount of memory ZRAM can use to store the compressed data. The
limit could be changed in run time and 0 means disable the
limit. No limit is the initial state. Unit: bytes
+
+What: /sys/block/zramid/fullness
+Date: August 2014
+Contact: Minchan Kim minc...@kernel.org
+Description:
+ The fullness file is read/write and specifies how easily
+ zram become full state so if you set it to lower value,
+ zram can reach full state easily compared to higher value.
+ Curretnly, initial value is 80% but it could be changed.
+ Unit: Percentage
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 649cad9d0b1c..ec3656f6891d 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -136,6 +136,37 @@ static ssize_t max_comp_streams_show(struct device *dev,
return scnprintf(buf, PAGE_SIZE, %d\n, val);
}
+static ssize_t fullness_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int val;
+ struct zram *zram = dev_to_zram(dev);
+
+ down_read(zram-init_lock);
+ val = zram-fullness;
+ up_read(zram-init_lock);
+
+ return scnprintf(buf, PAGE_SIZE, %d\n, val);
+}
+
+static ssize_t fullness_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t len)
+{
+ int err;
+ unsigned long val;
+ struct zram *zram = dev_to_zram(dev);
+
+ err = kstrtoul(buf, 10, val);
+ if (err || val 100)
+ return -EINVAL;
+
+ down_write(zram-init_lock);
+ zram-fullness = val;
+ up_write(zram-init_lock);
+
+ return len;
+}
+
static ssize_t mem_limit_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
@@ -733,6 +764,7 @@ static void zram_reset_device(struct zram *zram, bool
reset_capacity)
zram-limit_pages = 0;
atomic_set(zram-alloc_fail, 0);
+ zram-fullness = ZRAM_FULLNESS_PERCENT;
if (!init_done(zram)) {
up_write(zram-init_lock);
@@ -984,7 +1016,7 @@ static int zram_full(struct block_device *bdev, void *arg)
compr_pages = atomic64_read(zram-stats.compr_data_size)
PAGE_SHIFT;
if ((100 * compr_pages / total_pages)
- = ZRAM_FULLNESS_PERCENT)
+ = zram-fullness)
return 1;
}
@@ -1020,6 +1052,8 @@ static DEVICE_ATTR(orig_data_size, S_IRUGO,
orig_data_size_show, NULL);
static DEVICE_ATTR(mem_used_total, S_IRUGO, mem_used_total_show, NULL);
static DEVICE_ATTR(mem_limit, S_IRUGO | S_IWUSR, mem_limit_show,
mem_limit_store);
+static DEVICE_ATTR(fullness, S_IRUGO | S_IWUSR, fullness_show,
+ fullness_store);
static DEVICE_ATTR(mem_used_max, S_IRUGO | S_IWUSR, mem_used_max_show,
mem_used_max_store);
static DEVICE_ATTR(max_comp_streams, S_IRUGO | S_IWUSR,
@@ -1051,6 +1085,7 @@ static struct attribute *zram_disk_attrs[] = {
dev_attr_compr_data_size.attr,
dev_attr_mem_used_total.attr,
dev_attr_mem_limit.attr,
+ dev_attr_fullness.attr,
dev_attr_mem_used_max.attr,
dev_attr_max_comp_streams.attr,
dev_attr_comp_algorithm.attr,
@@ -1132,6 +1167,7 @@ static int create_device(struct zram *zram, int device_id)
strlcpy(zram-compressor, default_compressor, sizeof(zram-compressor));
zram-meta = NULL;
zram-max_comp_streams = 1;
+ zram-fullness = ZRAM_FULLNESS_PERCENT;
return 0;
out_free_disk:
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index fcf3176a9f15..6a9f383d0d78 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -119,6 +119,7 @@ struct zram
