* [PATCH 1/3] frontswap: support backing_dev
2023-07-10 22:16 [PATCH 0/3] zram: use frontswap for zram swap usecase Minchan Kim
@ 2023-07-10 22:16 ` Minchan Kim
2023-07-10 22:16 ` [PATCH 2/3] zram: support frontswap Minchan Kim
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Minchan Kim @ 2023-07-10 22:16 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-mm, Jens Axboe, Konrad Rzeszutek Wilk, Seth Jennings,
Sergey Senozhatsky, Minchan Kim
Modify frontswap to be used as a block device driver for swap
partitions.
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
include/linux/frontswap.h | 7 +++++--
mm/frontswap.c | 4 ++--
mm/swapfile.c | 3 ++-
mm/zswap.c | 2 +-
4 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/include/linux/frontswap.h b/include/linux/frontswap.h
index eaa0ac5f9003..466d617dc3c7 100644
--- a/include/linux/frontswap.h
+++ b/include/linux/frontswap.h
@@ -7,8 +7,10 @@
#include <linux/bitops.h>
#include <linux/jump_label.h>
+struct block_device;
+
struct frontswap_ops {
- void (*init)(unsigned); /* this swap type was just swapon'ed */
+ void (*init)(unsigned, struct block_device *); /* this swap type was just swapon'ed */
int (*store)(unsigned, pgoff_t, struct page *); /* store a page */
int (*load)(unsigned, pgoff_t, struct page *, bool *); /* load a page */
void (*invalidate_page)(unsigned, pgoff_t); /* page no longer needed */
@@ -17,7 +19,8 @@ struct frontswap_ops {
int frontswap_register_ops(const struct frontswap_ops *ops);
-extern void frontswap_init(unsigned type, unsigned long *map);
+extern void frontswap_init(unsigned type, unsigned long *map,
+ struct block_device *bdev);
extern int __frontswap_store(struct page *page);
extern int __frontswap_load(struct page *page);
extern void __frontswap_invalidate_page(unsigned, pgoff_t);
diff --git a/mm/frontswap.c b/mm/frontswap.c
index 2fb5df3384b8..967ce610a1de 100644
--- a/mm/frontswap.c
+++ b/mm/frontswap.c
@@ -107,7 +107,7 @@ int frontswap_register_ops(const struct frontswap_ops *ops)
/*
* Called when a swap device is swapon'd.
*/
-void frontswap_init(unsigned type, unsigned long *map)
+void frontswap_init(unsigned type, unsigned long *map, struct block_device *bdev)
{
struct swap_info_struct *sis = swap_info[type];
@@ -128,7 +128,7 @@ void frontswap_init(unsigned type, unsigned long *map)
if (!frontswap_enabled())
return;
- frontswap_ops->init(type);
+ frontswap_ops->init(type, bdev);
}
static bool __frontswap_test(struct swap_info_struct *sis,
diff --git a/mm/swapfile.c b/mm/swapfile.c
index a6945c2e0d03..a9424fd226bc 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -2339,7 +2339,8 @@ static void enable_swap_info(struct swap_info_struct *p, int prio,
unsigned long *frontswap_map)
{
if (IS_ENABLED(CONFIG_FRONTSWAP))
- frontswap_init(p->type, frontswap_map);
+ frontswap_init(p->type, frontswap_map, p->bdev);
+
spin_lock(&swap_lock);
spin_lock(&p->lock);
setup_swap_info(p, prio, swap_map, cluster_info);
diff --git a/mm/zswap.c b/mm/zswap.c
index c122f042a49d..9247e3adc21f 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -1510,7 +1510,7 @@ static void zswap_frontswap_invalidate_area(unsigned type)
zswap_trees[type] = NULL;
}
-static void zswap_frontswap_init(unsigned type)
+static void zswap_frontswap_init(unsigned type, struct block_device *bdev)
{
struct zswap_tree *tree;
--
2.41.0.255.g8b1d071c50-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 2/3] zram: support frontswap
2023-07-10 22:16 [PATCH 0/3] zram: use frontswap for zram swap usecase Minchan Kim
2023-07-10 22:16 ` [PATCH 1/3] frontswap: support backing_dev Minchan Kim
@ 2023-07-10 22:16 ` Minchan Kim
2023-07-11 10:08 ` Alexey Romanov
2023-07-10 22:16 ` [PATCH 3/3] zram: remove swap_slot_free_notify Minchan Kim
2023-07-11 5:17 ` [PATCH 0/3] zram: use frontswap for zram swap usecase Christoph Hellwig
3 siblings, 1 reply; 10+ messages in thread
From: Minchan Kim @ 2023-07-10 22:16 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-mm, Jens Axboe, Konrad Rzeszutek Wilk, Seth Jennings,
Sergey Senozhatsky, 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 <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);
+ 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
--
2.41.0.255.g8b1d071c50-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 2/3] zram: support frontswap
2023-07-10 22:16 ` [PATCH 2/3] zram: support frontswap Minchan Kim
@ 2023-07-11 10:08 ` Alexey Romanov
2023-07-11 23:58 ` Minchan Kim
0 siblings, 1 reply; 10+ messages in thread
From: Alexey Romanov @ 2023-07-11 10:08 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, linux-mm, Jens Axboe, Konrad Rzeszutek Wilk,
Seth Jennings, Sergey Senozhatsky
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2/3] zram: support frontswap
2023-07-11 10:08 ` Alexey Romanov
@ 2023-07-11 23:58 ` Minchan Kim
0 siblings, 0 replies; 10+ messages in thread
From: Minchan Kim @ 2023-07-11 23:58 UTC (permalink / raw)
To: Alexey Romanov
Cc: Andrew Morton, linux-mm, Jens Axboe, Konrad Rzeszutek Wilk,
Seth Jennings, Sergey Senozhatsky
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] zram: remove swap_slot_free_notify
2023-07-10 22:16 [PATCH 0/3] zram: use frontswap for zram swap usecase Minchan Kim
2023-07-10 22:16 ` [PATCH 1/3] frontswap: support backing_dev Minchan Kim
2023-07-10 22:16 ` [PATCH 2/3] zram: support frontswap Minchan Kim
@ 2023-07-10 22:16 ` Minchan Kim
2023-07-11 10:09 ` Alexey Romanov
2023-07-11 5:17 ` [PATCH 0/3] zram: use frontswap for zram swap usecase Christoph Hellwig
3 siblings, 1 reply; 10+ messages in thread
From: Minchan Kim @ 2023-07-10 22:16 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-mm, Jens Axboe, Konrad Rzeszutek Wilk, Seth Jennings,
Sergey Senozhatsky, Minchan Kim
Frontswap replaces the need for swap_slot_free_notify in block_device.
Therefore, we can remove it.
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
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(-)
diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
index aa1a233b0fa8..c94ef7d9fc6e 100644
--- a/Documentation/filesystems/locking.rst
+++ b/Documentation/filesystems/locking.rst
@@ -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
===============
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 5e973c982235..ec040ab3ab91 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -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
};
@@ -2206,7 +2188,7 @@ static int zram_frontswap_load(unsigned int type, pgoff_t index,
err = zram_read_from_zspool(zram, page, index);
if (!err)
- zram_accessed(zram, index);
+ zram_free_page(zram, index);
zram_slot_unlock(zram, index);
return err;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index c0ffe203a602..49e1843e44ea 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -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);
diff --git a/mm/swapfile.c b/mm/swapfile.c
index a9424fd226bc..adc16d8883c8 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -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);
--
2.41.0.255.g8b1d071c50-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 3/3] zram: remove swap_slot_free_notify
2023-07-10 22:16 ` [PATCH 3/3] zram: remove swap_slot_free_notify Minchan Kim
@ 2023-07-11 10:09 ` Alexey Romanov
0 siblings, 0 replies; 10+ messages in thread
From: Alexey Romanov @ 2023-07-11 10:09 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, linux-mm, Jens Axboe, Konrad Rzeszutek Wilk,
Seth Jennings, Sergey Senozhatsky
Hello,
On Mon, Jul 10, 2023 at 03:16:59PM -0700, Minchan Kim wrote:
> Frontswap replaces the need for swap_slot_free_notify in block_device.
> Therefore, we can remove it.
>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
> 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(-)
>
> diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
> index aa1a233b0fa8..c94ef7d9fc6e 100644
> --- a/Documentation/filesystems/locking.rst
> +++ b/Documentation/filesystems/locking.rst
> @@ -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
> ===============
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 5e973c982235..ec040ab3ab91 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -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
> };
>
> @@ -2206,7 +2188,7 @@ static int zram_frontswap_load(unsigned int type, pgoff_t index,
>
> err = zram_read_from_zspool(zram, page, index);
> if (!err)
> - zram_accessed(zram, index);
> + zram_free_page(zram, index);
Probably it should be in the previous commit.
> zram_slot_unlock(zram, index);
>
> return err;
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index c0ffe203a602..49e1843e44ea 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -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);
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index a9424fd226bc..adc16d8883c8 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -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);
> --
> 2.41.0.255.g8b1d071c50-goog
>
>
--
Thank you,
Alexey
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/3] zram: use frontswap for zram swap usecase
2023-07-10 22:16 [PATCH 0/3] zram: use frontswap for zram swap usecase Minchan Kim
` (2 preceding siblings ...)
2023-07-10 22:16 ` [PATCH 3/3] zram: remove swap_slot_free_notify Minchan Kim
@ 2023-07-11 5:17 ` Christoph Hellwig
2023-07-11 17:52 ` Nhat Pham
3 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2023-07-11 5:17 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, linux-mm, Jens Axboe, Konrad Rzeszutek Wilk,
Seth Jennings, Sergey Senozhatsky
On Mon, Jul 10, 2023 at 03:16:56PM -0700, Minchan Kim wrote:
> 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.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 0/3] zram: use frontswap for zram swap usecase
2023-07-11 5:17 ` [PATCH 0/3] zram: use frontswap for zram swap usecase Christoph Hellwig
@ 2023-07-11 17:52 ` Nhat Pham
2023-07-11 23:56 ` Minchan Kim
0 siblings, 1 reply; 10+ messages in thread
From: Nhat Pham @ 2023-07-11 17:52 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Minchan Kim, Andrew Morton, linux-mm, Jens Axboe,
Konrad Rzeszutek Wilk, Seth Jennings, Sergey Senozhatsky
On Mon, Jul 10, 2023 at 10:18 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Jul 10, 2023 at 03:16:56PM -0700, Minchan Kim wrote:
> > 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.
>
+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/20230530235447.GB102494@cmpxchg.org/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/3] zram: use frontswap for zram swap usecase
2023-07-11 17:52 ` Nhat Pham
@ 2023-07-11 23:56 ` Minchan Kim
0 siblings, 0 replies; 10+ messages in thread
From: Minchan Kim @ 2023-07-11 23:56 UTC (permalink / raw)
To: Nhat Pham
Cc: Christoph Hellwig, Andrew Morton, linux-mm, Jens Axboe,
Konrad Rzeszutek Wilk, Seth Jennings, Sergey Senozhatsky
On Tue, Jul 11, 2023 at 10:52:02AM -0700, Nhat Pham wrote:
> On Mon, Jul 10, 2023 at 10:18 PM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Mon, Jul 10, 2023 at 03:16:56PM -0700, Minchan Kim wrote:
> > > 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.
> >
>
> +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/20230530235447.GB102494@cmpxchg.org/
Thanks for feedback.
Understood. I also agree frontswap is hack so let me this patchset.
I hope swap abstraction goes to the way Chris Li suggested in the end.
^ permalink raw reply [flat|nested] 10+ messages in thread