Re: [PATCH 2/3] zram: support frontswap

[Alexey Romanov <AVRomanov@sberdevices.ru>]

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

-- 
Thank you,
Alexey