zram: use frontswap for zram swap usecase

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 <[email protected]>

---
 drivers/block/zram/Kconfig    |  1 +
 drivers/block/zram/zram_drv.c | 98 +++++++++++++++++++++++++++++++++++
 drivers/block/zram/zram_drv.h |  1 +
 3 files changed, 100 insertions(+)

drivers/block/zram/Kconfig +1 -0

index 0386b7da02aa..a841d6d14c74 100644
@@ -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

drivers/block/zram/zram_drv.c +21 -0

index 5d258a28ec43..5e973c982235 100644
@@ -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;
+}

drivers/block/zram/zram_drv.c +8 -0

+		return -1;
+	}
+
+	err = zram_read_from_zspool(zram, page, index);
+	if (!err)
+		zram_accessed(zram, index);
+	zram_slot_unlock(zram, index);
+

drivers/block/zram/zram_drv.c +49 -0

+
+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:

drivers/block/zram/zram_drv.h +1 -0

index ca7a15bd4845..0f52d2da8512 100644
@@ -139,5 +139,6 @@ struct zram {
 #ifdef CONFIG_ZRAM_MEMORY_TRACKING
 	struct dentry *debugfs_dir;
 #endif
+	struct list_head list;
 };
 #endif

Signature · 1 line

2.41.0.255.g8b1d071c50-goog

Hello,

Quoted 65 lines · Minchan Kim

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 <[email protected]>

---
 drivers/block/zram/Kconfig    |  1 +
 drivers/block/zram/zram_drv.c | 98 +++++++++++++++++++++++++++++++++++
 drivers/block/zram/zram_drv.h |  1 +
 3 files changed, 100 insertions(+)

drivers/block/zram/Kconfig +1 -0

index 0386b7da02aa..a841d6d14c74 100644
@@ -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

drivers/block/zram/zram_drv.c +25 -0

index 5d258a28ec43..5e973c982235 100644
@@ -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)
+{

drivers/block/zram/zram_drv.c static struct attribute *zram_disk_attrs[] = { · line 2203

Quoted 6 lines

drivers/block/zram/zram_drv.c +6 -0

+	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.

Quoted 13 lines

drivers/block/zram/zram_drv.c +12 -0

+		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)

drivers/block/zram/zram_drv.c static struct attribute *zram_disk_attrs[] = { · line 2221

Quoted 6 lines

drivers/block/zram/zram_drv.c +6 -0

+{
+	struct zram *zram = zram_swaps[type];
+
+	if (!zram)
+		return;
+}

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

Frontswap replaces the need for swap_slot_free_notify in block_device. Therefore, we can remove it.

Signed-off-by Minchan Kim <[email protected]>

---
 Documentation/filesystems/locking.rst |  5 -----
 drivers/block/zram/zram_drv.c         | 20 +-------------------
 include/linux/blkdev.h                |  2 --
 mm/swapfile.c                         |  8 --------
 4 files changed, 1 insertion(+), 34 deletions(-)

Documentation/filesystems/locking.rst +0 -5

index aa1a233b0fa8..c94ef7d9fc6e 100644
@@ -477,7 +477,6 @@ block_device_operations
 				unsigned long *);
 	void (*unlock_native_capacity) (struct gendisk *);
 	int (*getgeo)(struct block_device *, struct hd_geometry *);
-	void (*swap_slot_free_notify) (struct block_device *, unsigned long);
 
 locking rules:
 

@@ -491,12 +490,8 @@ compat_ioctl:		no
 direct_access:		no
 unlock_native_capacity:	no
 getgeo:			no
-swap_slot_free_notify:	no	(see below)
 ======================= ===================
 
-swap_slot_free_notify is called with swap_lock and sometimes the page lock
-held.
-
 
 file_operations
 ===============

drivers/block/zram/zram_drv.c +0 -18

index 5e973c982235..ec040ab3ab91 100644
@@ -1947,23 +1947,6 @@ static void zram_submit_bio(struct bio *bio)
 	}
 }
 
-static void zram_slot_free_notify(struct block_device *bdev,
-				unsigned long index)
-{
-	struct zram *zram;
-
-	zram = bdev->bd_disk->private_data;
-
-	atomic64_inc(&zram->stats.notify_free);
-	if (!zram_slot_trylock(zram, index)) {
-		atomic64_inc(&zram->stats.miss_free);
-		return;
-	}
-
-	zram_free_page(zram, index);
-	zram_slot_unlock(zram, index);
-}
-
 static void zram_destroy_comps(struct zram *zram)
 {
 	u32 prio;

@@ -2117,7 +2100,6 @@ static int zram_open(struct block_device *bdev, fmode_t mode)
 static const struct block_device_operations zram_devops = {
 	.open = zram_open,
 	.submit_bio = zram_submit_bio,
-	.swap_slot_free_notify = zram_slot_free_notify,
 	.owner = THIS_MODULE
 };
 

drivers/block/zram/zram_drv.c +0 -0

 	zram_slot_unlock(zram, index);
 
 	return err;

include/linux/blkdev.h +0 -2

index c0ffe203a602..49e1843e44ea 100644
@@ -1390,8 +1390,6 @@ struct block_device_operations {
 	int (*getgeo)(struct block_device *, struct hd_geometry *);
 	int (*set_read_only)(struct block_device *bdev, bool ro);
 	void (*free_disk)(struct gendisk *disk);
-	/* this callback is with swap_lock and sometimes page table lock held */
-	void (*swap_slot_free_notify) (struct block_device *, unsigned long);
 	int (*report_zones)(struct gendisk *, sector_t sector,
 			unsigned int nr_zones, report_zones_cb cb, void *data);
 	char *(*devnode)(struct gendisk *disk, umode_t *mode);

mm/swapfile.c +0 -8

index a9424fd226bc..adc16d8883c8 100644
@@ -726,7 +726,6 @@ static void swap_range_free(struct swap_info_struct *si, unsigned long offset,
 {
 	unsigned long begin = offset;
 	unsigned long end = offset + nr_entries - 1;
-	void (*swap_slot_free_notify)(struct block_device *, unsigned long);
 
 	if (offset < si->lowest_bit)
 		si->lowest_bit = offset;

@@ -739,16 +738,9 @@ static void swap_range_free(struct swap_info_struct *si, unsigned long offset,
 	}
 	atomic_long_add(nr_entries, &nr_swap_pages);
 	WRITE_ONCE(si->inuse_pages, si->inuse_pages - nr_entries);
-	if (si->flags & SWP_BLKDEV)
-		swap_slot_free_notify =
-			si->bdev->bd_disk->fops->swap_slot_free_notify;
-	else
-		swap_slot_free_notify = NULL;
 	while (offset <= end) {
 		arch_swap_invalidate_page(si->type, offset);
 		frontswap_invalidate_page(si->type, offset);
-		if (swap_slot_free_notify)
-			swap_slot_free_notify(si->bdev, offset);
 		offset++;
 	}
 	clear_shadow_from_swap_cache(si->type, begin, end);

Signature · 1 line

2.41.0.255.g8b1d071c50-goog

This patchset uses frontswap for zram swap usecase and remove swap_slot_free_notify swap specific operation in block device. It shows 13% swapout improvement for MADV_PAGEOUT.

Quoted 3 lines · Minchan Kim

This patchset uses frontswap for zram swap usecase and remove swap_slot_free_notify swap specific operation in block device. It shows 13% swapout improvement for MADV_PAGEOUT.

Err, no. frontswap needs to go away, and not be tried to a block driver. If you want compressed swap without a block driver please use zswap and help improvig it and the swap abstraction to not require a backing allocation for it.

We need to fix swap and not pile hacks on top of hacks.

Quoted 11 lines · Christoph Hellwig

Quoted 3 lines · Minchan Kim

This patchset uses frontswap for zram swap usecase and remove swap_slot_free_notify swap specific operation in block device. It shows 13% swapout improvement for MADV_PAGEOUT.

Err, no. frontswap needs to go away, and not be tried to a block driver. If you want compressed swap without a block driver please use zswap and help improvig it and the swap abstraction to not require a backing allocation for it.

Quoted 2 lines

We need to fix swap and not pile hacks on top of hacks.

+1. Based on earlier discussions, it seems like the agreed upon way forward is to remove the frontswap interface:

https://lore.kernel.org/linux-mm/[email protected]/