diff mbox series

[2/3] zram: support frontswap

Message ID 20230710221659.2473460-3-minchan@kernel.org (mailing list archive)
State New
Headers show
Series zram: use frontswap for zram swap usecase | expand

Commit Message

Minchan Kim July 10, 2023, 10:16 p.m. UTC 
With frontswap, zram can perform swapout 13% faster, which restores
its original performance with rw_page.

200M swapout using MADV_PAGEOUT:
Before: 4355ms After: 3786ms

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 drivers/block/zram/Kconfig    |  1 +
 drivers/block/zram/zram_drv.c | 98 +++++++++++++++++++++++++++++++++++
 drivers/block/zram/zram_drv.h |  1 +
 3 files changed, 100 insertions(+)

Comments

Alexey Romanov July 11, 2023, 10:08 a.m. UTC | #1 
Hello,

On Mon, Jul 10, 2023 at 03:16:58PM -0700, Minchan Kim wrote:
> With frontswap, zram can perform swapout 13% faster, which restores
> its original performance with rw_page.
> 
> 200M swapout using MADV_PAGEOUT:
> Before: 4355ms After: 3786ms
> 
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  drivers/block/zram/Kconfig    |  1 +
>  drivers/block/zram/zram_drv.c | 98 +++++++++++++++++++++++++++++++++++
>  drivers/block/zram/zram_drv.h |  1 +
>  3 files changed, 100 insertions(+)
> 
> diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig
> index 0386b7da02aa..a841d6d14c74 100644
> --- a/drivers/block/zram/Kconfig
> +++ b/drivers/block/zram/Kconfig
> @@ -4,6 +4,7 @@ config ZRAM
>  	depends on BLOCK && SYSFS && MMU
>  	depends on CRYPTO_LZO || CRYPTO_ZSTD || CRYPTO_LZ4 || CRYPTO_LZ4HC || CRYPTO_842
>  	select ZSMALLOC
> +	select FRONTSWAP
>  	help
>  	  Creates virtual block devices called /dev/zramX (X = 0, 1, ...).
>  	  Pages written to these disks are compressed and stored in memory
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 5d258a28ec43..5e973c982235 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -33,6 +33,8 @@
>  #include <linux/debugfs.h>
>  #include <linux/cpuhotplug.h>
>  #include <linux/part_stat.h>
> +#include <linux/swap.h>
> +#include <linux/frontswap.h>
>  
>  #include "zram_drv.h"
>  
> @@ -2170,6 +2172,90 @@ static struct attribute *zram_disk_attrs[] = {
>  
>  ATTRIBUTE_GROUPS(zram_disk);
>  
> +static struct zram *zram_swaps[MAX_SWAPFILES];
> +static LIST_HEAD(zram_list);
> +static DEFINE_SPINLOCK(zram_list_lock);
> +
> +static int zram_frontswap_store(unsigned int type, pgoff_t index,
> +				struct page *page)
> +{
> +	int err;
> +	struct zram *zram = zram_swaps[type];
> +
> +	err = zram_write_page(zram, page, index);
> +	if (!err) {
> +		zram_slot_lock(zram, index);
> +		zram_accessed(zram, index);
> +		zram_slot_unlock(zram, index);
> +	}
> +
> +	return err;
> +}
> +
> +static int zram_frontswap_load(unsigned int type, pgoff_t index,
> +			       struct page *page, bool *exclusive)
> +{
> +	int err;
> +	struct zram *zram = zram_swaps[type];
> +
> +	zram_slot_lock(zram, index);
> +	if (zram_test_flag(zram, index, ZRAM_WB)) {
> +		zram_slot_lock(zram, index);

Looks like zram_slot_unlock should be here instead of zram_slot_lock.

> +		return -1;
> +	}
> +
> +	err = zram_read_from_zspool(zram, page, index);
> +	if (!err)
> +		zram_accessed(zram, index);
> +	zram_slot_unlock(zram, index);
> +
> +	return err;
> +}
> +
> +static void zram_frontswap_invalidate_area(unsigned int type)
> +{
> +	struct zram *zram = zram_swaps[type];
> +
> +	if (!zram)
> +		return;
> +}

It makes sense? Maybe we just leave this function empty?

> +
> +static void zram_frontswap_init(unsigned int type, struct block_device *bdev)
> +{
> +	struct zram *zram;
> +
> +	spin_lock(&zram_list_lock);
> +	list_for_each_entry(zram, &zram_list, list) {
> +		if (&zram_devops == bdev->bd_disk->fops) {
> +			zram_swaps[type] = zram;
> +			break;
> +		}
> +	}
> +	spin_unlock(&zram_list_lock);
> +}
> +
> +static void zram_frontswap_invalidate_page(unsigned int type, pgoff_t offset)
> +{
> +	struct zram *zram = zram_swaps[type];
> +
> +	if (!zram_slot_trylock(zram, offset)) {
> +		atomic64_inc(&zram->stats.miss_free);
> +		return;
> +	}
> +
> +	zram_free_page(zram, offset);
> +	zram_slot_unlock(zram, offset);
> +}
> +
> +static const struct frontswap_ops zram_frontswap_ops = {
> +	.store = zram_frontswap_store,
> +	.load = zram_frontswap_load,
> +	.init = zram_frontswap_init,
> +	.invalidate_area = zram_frontswap_invalidate_area,
> +	.invalidate_page = zram_frontswap_invalidate_page,
> +	.init = zram_frontswap_init
> +};
> +
>  /*
>   * Allocate and initialize new zram device. the function returns
>   * '>= 0' device_id upon success, and negative value otherwise.
> @@ -2246,6 +2332,10 @@ static int zram_add(void)
>  
>  	comp_algorithm_set(zram, ZRAM_PRIMARY_COMP, default_compressor);
>  
> +	spin_lock(&zram_list_lock);
> +	list_add(&zram->list, &zram_list);
> +	spin_unlock(&zram_list_lock);
> +
>  	zram_debugfs_register(zram);
>  	pr_info("Added device: %s\n", zram->disk->disk_name);
>  	return device_id;
> @@ -2303,6 +2393,11 @@ static int zram_remove(struct zram *zram)
>  	zram_reset_device(zram);
>  
>  	put_disk(zram->disk);
> +
> +	spin_lock(&zram_list_lock);
> +	list_del(&zram->list);
> +	spin_unlock(&zram_list_lock);
> +
>  	kfree(zram);
>  	return 0;
>  }
> @@ -2428,6 +2523,9 @@ static int __init zram_init(void)
>  		num_devices--;
>  	}
>  
> +	if (frontswap_register_ops(&zram_frontswap_ops))
> +		pr_info("Frontswap is not used, which is suboptimal for zram swap.\n");
> +
>  	return 0;
>  
>  out_error:
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index ca7a15bd4845..0f52d2da8512 100644
> --- a/drivers/block/zram/zram_drv.h
> +++ b/drivers/block/zram/zram_drv.h
> @@ -139,5 +139,6 @@ struct zram {
>  #ifdef CONFIG_ZRAM_MEMORY_TRACKING
>  	struct dentry *debugfs_dir;
>  #endif
> +	struct list_head list;
>  };
>  #endif
> -- 
> 2.41.0.255.g8b1d071c50-goog
> 
>
Minchan Kim July 11, 2023, 11:58 p.m. UTC | #2 
On Tue, Jul 11, 2023 at 10:08:46AM +0000, Alexey Romanov wrote:
> Hello,
> 
> On Mon, Jul 10, 2023 at 03:16:58PM -0700, Minchan Kim wrote:
> > With frontswap, zram can perform swapout 13% faster, which restores
> > its original performance with rw_page.
> > 
> > 200M swapout using MADV_PAGEOUT:
> > Before: 4355ms After: 3786ms
> > 
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> >  drivers/block/zram/Kconfig    |  1 +
> >  drivers/block/zram/zram_drv.c | 98 +++++++++++++++++++++++++++++++++++
> >  drivers/block/zram/zram_drv.h |  1 +
> >  3 files changed, 100 insertions(+)
> > 
> > diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig
> > index 0386b7da02aa..a841d6d14c74 100644
> > --- a/drivers/block/zram/Kconfig
> > +++ b/drivers/block/zram/Kconfig
> > @@ -4,6 +4,7 @@ config ZRAM
> >  	depends on BLOCK && SYSFS && MMU
> >  	depends on CRYPTO_LZO || CRYPTO_ZSTD || CRYPTO_LZ4 || CRYPTO_LZ4HC || CRYPTO_842
> >  	select ZSMALLOC
> > +	select FRONTSWAP
> >  	help
> >  	  Creates virtual block devices called /dev/zramX (X = 0, 1, ...).
> >  	  Pages written to these disks are compressed and stored in memory
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index 5d258a28ec43..5e973c982235 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -33,6 +33,8 @@
> >  #include <linux/debugfs.h>
> >  #include <linux/cpuhotplug.h>
> >  #include <linux/part_stat.h>
> > +#include <linux/swap.h>
> > +#include <linux/frontswap.h>
> >  
> >  #include "zram_drv.h"
> >  
> > @@ -2170,6 +2172,90 @@ static struct attribute *zram_disk_attrs[] = {
> >  
> >  ATTRIBUTE_GROUPS(zram_disk);
> >  
> > +static struct zram *zram_swaps[MAX_SWAPFILES];
> > +static LIST_HEAD(zram_list);
> > +static DEFINE_SPINLOCK(zram_list_lock);
> > +
> > +static int zram_frontswap_store(unsigned int type, pgoff_t index,
> > +				struct page *page)
> > +{
> > +	int err;
> > +	struct zram *zram = zram_swaps[type];
> > +
> > +	err = zram_write_page(zram, page, index);
> > +	if (!err) {
> > +		zram_slot_lock(zram, index);
> > +		zram_accessed(zram, index);
> > +		zram_slot_unlock(zram, index);
> > +	}
> > +
> > +	return err;
> > +}
> > +
> > +static int zram_frontswap_load(unsigned int type, pgoff_t index,
> > +			       struct page *page, bool *exclusive)
> > +{
> > +	int err;
> > +	struct zram *zram = zram_swaps[type];
> > +
> > +	zram_slot_lock(zram, index);
> > +	if (zram_test_flag(zram, index, ZRAM_WB)) {
> > +		zram_slot_lock(zram, index);
> 
> Looks like zram_slot_unlock should be here instead of zram_slot_lock.

me/ slabs self.

> 
> > +		return -1;
> > +	}
> > +
> > +	err = zram_read_from_zspool(zram, page, index);
> > +	if (!err)
> > +		zram_accessed(zram, index);
> > +	zram_slot_unlock(zram, index);
> > +
> > +	return err;
> > +}
> > +
> > +static void zram_frontswap_invalidate_area(unsigned int type)
> > +{
> > +	struct zram *zram = zram_swaps[type];
> > +
> > +	if (!zram)
> > +		return;
> > +}
> 
> It makes sense? Maybe we just leave this function empty?

Sorry, it was wrong version from my git tree.
I had destroy zram in the case originally
but since I decide to drop this patchset, never mind.

Thanks for spending time for review, Alexey.
diff mbox series

Patch

diff --git a/drivers/block/zram/Kconfig b/drivers/block/zram/Kconfig
index 0386b7da02aa..a841d6d14c74 100644
--- a/drivers/block/zram/Kconfig
+++ b/drivers/block/zram/Kconfig
@@ -4,6 +4,7 @@  config ZRAM
 	depends on BLOCK && SYSFS && MMU
 	depends on CRYPTO_LZO || CRYPTO_ZSTD || CRYPTO_LZ4 || CRYPTO_LZ4HC || CRYPTO_842
 	select ZSMALLOC
+	select FRONTSWAP
 	help
 	  Creates virtual block devices called /dev/zramX (X = 0, 1, ...).
 	  Pages written to these disks are compressed and stored in memory
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 5d258a28ec43..5e973c982235 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -33,6 +33,8 @@ 
 #include <linux/debugfs.h>
 #include <linux/cpuhotplug.h>
 #include <linux/part_stat.h>
+#include <linux/swap.h>
+#include <linux/frontswap.h>
 
 #include "zram_drv.h"
 
@@ -2170,6 +2172,90 @@  static struct attribute *zram_disk_attrs[] = {
 
 ATTRIBUTE_GROUPS(zram_disk);
 
+static struct zram *zram_swaps[MAX_SWAPFILES];
+static LIST_HEAD(zram_list);
+static DEFINE_SPINLOCK(zram_list_lock);
+
+static int zram_frontswap_store(unsigned int type, pgoff_t index,
+				struct page *page)
+{
+	int err;
+	struct zram *zram = zram_swaps[type];
+
+	err = zram_write_page(zram, page, index);
+	if (!err) {
+		zram_slot_lock(zram, index);
+		zram_accessed(zram, index);
+		zram_slot_unlock(zram, index);
+	}
+
+	return err;
+}
+
+static int zram_frontswap_load(unsigned int type, pgoff_t index,
+			       struct page *page, bool *exclusive)
+{
+	int err;
+	struct zram *zram = zram_swaps[type];
+
+	zram_slot_lock(zram, index);
+	if (zram_test_flag(zram, index, ZRAM_WB)) {
+		zram_slot_lock(zram, index);
+		return -1;
+	}
+
+	err = zram_read_from_zspool(zram, page, index);
+	if (!err)
+		zram_accessed(zram, index);
+	zram_slot_unlock(zram, index);
+
+	return err;
+}
+
+static void zram_frontswap_invalidate_area(unsigned int type)
+{
+	struct zram *zram = zram_swaps[type];
+
+	if (!zram)
+		return;
+}
+
+static void zram_frontswap_init(unsigned int type, struct block_device *bdev)
+{
+	struct zram *zram;
+
+	spin_lock(&zram_list_lock);
+	list_for_each_entry(zram, &zram_list, list) {
+		if (&zram_devops == bdev->bd_disk->fops) {
+			zram_swaps[type] = zram;
+			break;
+		}
+	}
+	spin_unlock(&zram_list_lock);
+}
+
+static void zram_frontswap_invalidate_page(unsigned int type, pgoff_t offset)
+{
+	struct zram *zram = zram_swaps[type];
+
+	if (!zram_slot_trylock(zram, offset)) {
+		atomic64_inc(&zram->stats.miss_free);
+		return;
+	}
+
+	zram_free_page(zram, offset);
+	zram_slot_unlock(zram, offset);
+}
+
+static const struct frontswap_ops zram_frontswap_ops = {
+	.store = zram_frontswap_store,
+	.load = zram_frontswap_load,
+	.init = zram_frontswap_init,
+	.invalidate_area = zram_frontswap_invalidate_area,
+	.invalidate_page = zram_frontswap_invalidate_page,
+	.init = zram_frontswap_init
+};
+
 /*
  * Allocate and initialize new zram device. the function returns
  * '>= 0' device_id upon success, and negative value otherwise.
@@ -2246,6 +2332,10 @@  static int zram_add(void)
 
 	comp_algorithm_set(zram, ZRAM_PRIMARY_COMP, default_compressor);
 
+	spin_lock(&zram_list_lock);
+	list_add(&zram->list, &zram_list);
+	spin_unlock(&zram_list_lock);
+
 	zram_debugfs_register(zram);
 	pr_info("Added device: %s\n", zram->disk->disk_name);
 	return device_id;
@@ -2303,6 +2393,11 @@  static int zram_remove(struct zram *zram)
 	zram_reset_device(zram);
 
 	put_disk(zram->disk);
+
+	spin_lock(&zram_list_lock);
+	list_del(&zram->list);
+	spin_unlock(&zram_list_lock);
+
 	kfree(zram);
 	return 0;
 }
@@ -2428,6 +2523,9 @@  static int __init zram_init(void)
 		num_devices--;
 	}
 
+	if (frontswap_register_ops(&zram_frontswap_ops))
+		pr_info("Frontswap is not used, which is suboptimal for zram swap.\n");
+
 	return 0;
 
 out_error:
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index ca7a15bd4845..0f52d2da8512 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -139,5 +139,6 @@  struct zram {
 #ifdef CONFIG_ZRAM_MEMORY_TRACKING
 	struct dentry *debugfs_dir;
 #endif
+	struct list_head list;
 };
 #endif