Skip to contentSkip to conversation

zram memory tracking

PATCH Superseded Minchan Kim 4 participants 11 messages Apr 9, 2018 – Apr 10, 2018

Subsystems (3)
Documentation Block layer zram

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Minchan Kim

author maintainer
#0 lore raw

v3 cover zram memory tracking

zRam as swap is useful for small memory device. However, swap means those pages on zram are mostly cold pages due to VM's LRU algorithm. Especially, once init data for application are touched for launching, they tend to be not accessed any more and finally swapped out. zRAM can store such cold pages as compressed form but it's pointless to keep in memory. As well, it's pointless to store incompressible pages to zram so better idea is app developers manages them directly like free or mlock rather than remaining them on heap.

This patch provides a debugfs /sys/kernel/debug/zram/zram0/block_state to represent each block's state so admin can investigate what memory is cold|incompressible|same page with using pagemap once the pages are swapped out.

The output is as follows,

      300    75.033841 .wh
      301    63.806904 s..
      302    63.806919 ..h

First column is zram's block index and 3rh one represents symbol (s: same page w: written page to backing store h: huge page) of the block state. Second column represents usec time unit of the block was last accessed. So above example means the 300th block is accessed at 75.033851 second and it was huge so it was written to the backing store.

* From v2:
* debugfs and Kconfig cleanup - Greg KH
* Remove unnecesarry buffer - Sergey
* Change timestamp from sec to usec

* From v1:
* Do not propagate error number for debugfs fail - Greg KH
* Add writeback and hugepage information - Sergey

Minchan Kim (4):
zram: correct flag name of ZRAM_ACCESS
zram: mark incompressible page as ZRAM_HUGE
zram: record accessed second
zram: introduce zram memory tracking

 Documentation/blockdev/zram.txt |  25 +++++
 drivers/block/zram/Kconfig      |   9 ++
 drivers/block/zram/zram_drv.c   | 172 +++++++++++++++++++++++++++++---
 drivers/block/zram/zram_drv.h   |  14 ++-
 4 files changed, 203 insertions(+), 17 deletions(-)
Signature · 1 line
2.17.0.484.g0c8726318c-goog
author maintainer
#1 lore raw

PATCH v3 1/4 zram: correct flag name of ZRAM_ACCESS

ZRAM_ACCESS is used for locking a slot of zram so correct the name. It is also not a common flag to indicate status of the block so move the declare position on top of the flag.
Lastly, let's move the function to the top of source code to be able to use it easily without forward declaration.

Signed-off-by Minchan Kim <[email protected]>
---
 drivers/block/zram/zram_drv.c | 20 ++++++++++----------
 drivers/block/zram/zram_drv.h |  6 +++---
 2 files changed, 13 insertions(+), 13 deletions(-)
drivers/block/zram/zram_drv.c +10 -10
index 0f3fadd71230..18dadeab775b 100644
@@ -52,6 +52,16 @@ static size_t huge_class_size;
 
 static void zram_free_page(struct zram *zram, size_t index);
 
+static void zram_slot_lock(struct zram *zram, u32 index)
+{
+	bit_spin_lock(ZRAM_LOCK, &zram->table[index].value);
+}
+
+static void zram_slot_unlock(struct zram *zram, u32 index)
+{
+	bit_spin_unlock(ZRAM_LOCK, &zram->table[index].value);
+}
+
 static inline bool init_done(struct zram *zram)
 {
 	return zram->disksize;

@@ -753,16 +763,6 @@ static DEVICE_ATTR_RO(io_stat);
 static DEVICE_ATTR_RO(mm_stat);
 static DEVICE_ATTR_RO(debug_stat);
 
-static void zram_slot_lock(struct zram *zram, u32 index)
-{
-	bit_spin_lock(ZRAM_ACCESS, &zram->table[index].value);
-}
-
-static void zram_slot_unlock(struct zram *zram, u32 index)
-{
-	bit_spin_unlock(ZRAM_ACCESS, &zram->table[index].value);
-}
-
 static void zram_meta_free(struct zram *zram, u64 disksize)
 {
 	size_t num_pages = disksize >> PAGE_SHIFT;
drivers/block/zram/zram_drv.h +3 -3
index 008861220723..8d8959ceabd1 100644
@@ -43,9 +43,9 @@
 
 /* Flags for zram pages (table[page_no].value) */
 enum zram_pageflags {
-	/* Page consists the same element */
-	ZRAM_SAME = ZRAM_FLAG_SHIFT,
-	ZRAM_ACCESS,	/* page is now accessed */
+	/* zram slot is locked */
+	ZRAM_LOCK = ZRAM_FLAG_SHIFT,
+	ZRAM_SAME,	/* Page consists the same element */
 	ZRAM_WB,	/* page is stored on backing_device */
 
 	__NR_ZRAM_PAGEFLAGS,
Signature · 1 line
2.17.0.484.g0c8726318c-goog
author maintainer
#2 lore raw

PATCH v3 2/4 zram: mark incompressible page as ZRAM_HUGE

In reply to Minchan Kim

Mark incompressible pages so that we could investigate who is the owner of the incompressible pages once the page is swapped out via using upcoming zram memory tracker feature.

With it, we could prevent such pages to be swapped out by using mlock. Otherwise we might remove them.

This patch exposes new stat for huge pages via mm_stat.

Signed-off-by Minchan Kim <[email protected]>
---
 Documentation/blockdev/zram.txt |  1 +
 drivers/block/zram/zram_drv.c   | 17 ++++++++++++++---
 drivers/block/zram/zram_drv.h   |  2 ++
 3 files changed, 17 insertions(+), 3 deletions(-)
Documentation/blockdev/zram.txt +1 -0
index 257e65714c6a..78db38d02bc9 100644
@@ -218,6 +218,7 @@ The stat file represents device's mm statistics. It consists of a single
  same_pages       the number of same element filled pages written to this disk.
                   No memory is allocated for such pages.
  pages_compacted  the number of pages freed during compaction
+ huge_pages	  the number of incompressible pages
 
 9) Deactivate:
 	swapoff /dev/zram0
drivers/block/zram/zram_drv.c +14 -3
index 18dadeab775b..777fb3339f59 100644
@@ -729,14 +729,15 @@ static ssize_t mm_stat_show(struct device *dev,
 	max_used = atomic_long_read(&zram->stats.max_used_pages);
 
 	ret = scnprintf(buf, PAGE_SIZE,
-			"%8llu %8llu %8llu %8lu %8ld %8llu %8lu\n",
+			"%8llu %8llu %8llu %8lu %8ld %8llu %8lu %8llu\n",
 			orig_size << PAGE_SHIFT,
 			(u64)atomic64_read(&zram->stats.compr_data_size),
 			mem_used << PAGE_SHIFT,
 			zram->limit_pages << PAGE_SHIFT,
 			max_used << PAGE_SHIFT,
 			(u64)atomic64_read(&zram->stats.same_pages),
-			pool_stats.pages_compacted);
+			pool_stats.pages_compacted,
+			(u64)atomic64_read(&zram->stats.huge_pages));
 	up_read(&zram->init_lock);
 
 	return ret;

@@ -805,6 +806,11 @@ static void zram_free_page(struct zram *zram, size_t index)
 {
 	unsigned long handle;
 
+	if (zram_test_flag(zram, index, ZRAM_HUGE)) {
+		zram_clear_flag(zram, index, ZRAM_HUGE);
+		atomic64_dec(&zram->stats.huge_pages);
+	}
+
 	if (zram_wb_enabled(zram) && zram_test_flag(zram, index, ZRAM_WB)) {
 		zram_wb_clear(zram, index);
 		atomic64_dec(&zram->stats.pages_stored);

@@ -973,6 +979,7 @@ static int __zram_bvec_write(struct zram *zram, struct bio_vec *bvec,
 	}
 
 	if (unlikely(comp_len >= huge_class_size)) {
+		comp_len = PAGE_SIZE;
 		if (zram_wb_enabled(zram) && allow_wb) {
 			zcomp_stream_put(zram->comp);
 			ret = write_to_bdev(zram, bvec, index, bio, &element);

@@ -984,7 +991,6 @@ static int __zram_bvec_write(struct zram *zram, struct bio_vec *bvec,
 			allow_wb = false;
 			goto compress_again;
 		}
-		comp_len = PAGE_SIZE;
 	}
 
 	/*

@@ -1046,6 +1052,11 @@ static int __zram_bvec_write(struct zram *zram, struct bio_vec *bvec,
 	zram_slot_lock(zram, index);
 	zram_free_page(zram, index);
 
+	if (comp_len == PAGE_SIZE) {
+		zram_set_flag(zram, index, ZRAM_HUGE);
+		atomic64_inc(&zram->stats.huge_pages);
+	}
+
 	if (flags) {
 		zram_set_flag(zram, index, flags);
 		zram_set_element(zram, index, element);
drivers/block/zram/zram_drv.h +2 -0
index 8d8959ceabd1..ff0547bdb586 100644
@@ -47,6 +47,7 @@ enum zram_pageflags {
 	ZRAM_LOCK = ZRAM_FLAG_SHIFT,
 	ZRAM_SAME,	/* Page consists the same element */
 	ZRAM_WB,	/* page is stored on backing_device */
+	ZRAM_HUGE,	/* Incompressible page */
 
 	__NR_ZRAM_PAGEFLAGS,
 };

@@ -71,6 +72,7 @@ struct zram_stats {
 	atomic64_t invalid_io;	/* non-page-aligned I/O requests */
 	atomic64_t notify_free;	/* no. of swap slot free notifications */
 	atomic64_t same_pages;		/* no. of same element filled pages */
+	atomic64_t huge_pages;		/* no. of huge pages */
 	atomic64_t pages_stored;	/* no. of pages currently stored */
 	atomic_long_t max_used_pages;	/* no. of maximum pages stored */
 	atomic64_t writestall;		/* no. of write slow paths */
Signature · 1 line
2.17.0.484.g0c8726318c-goog
author maintainer
#3 lore raw

PATCH v3 3/4 zram: record accessed second

In reply to Minchan Kim

zRam as swap is useful for small memory device. However, swap means those pages on zram are mostly cold pages due to VM's LRU algorithm. Especially, once init data for application are touched for launching, they tend to be not accessed any more and finally swapped out. zRAM can store such cold pages as compressed form but it's pointless to keep in memory. Better idea is app developers free them directly rather than remaining them on heap.

This patch records last access time of each block of zram so that With upcoming zram memory tracking, it could help userspace developers to reduce memory footprint.

Signed-off-by Minchan Kim <[email protected]>
---
 drivers/block/zram/zram_drv.c | 16 ++++++++++++++++
 drivers/block/zram/zram_drv.h |  1 +
 2 files changed, 17 insertions(+)
drivers/block/zram/zram_drv.c +16 -0
index 777fb3339f59..7fc10e2ad734 100644
@@ -107,6 +107,16 @@ static inline void zram_set_element(struct zram *zram, u32 index,
 	zram->table[index].element = element;
 }
 
+static void zram_accessed(struct zram *zram, u32 index)
+{
+	zram->table[index].ac_time = sched_clock();
+}
+
+static void zram_reset_access(struct zram *zram, u32 index)
+{
+	zram->table[index].ac_time = 0;
+}
+
 static unsigned long zram_get_element(struct zram *zram, u32 index)
 {
 	return zram->table[index].element;

@@ -806,6 +816,8 @@ static void zram_free_page(struct zram *zram, size_t index)
 {
 	unsigned long handle;
 
+	zram_reset_access(zram, index);
+
 	if (zram_test_flag(zram, index, ZRAM_HUGE)) {
 		zram_clear_flag(zram, index, ZRAM_HUGE);
 		atomic64_dec(&zram->stats.huge_pages);

@@ -1177,6 +1189,10 @@ static int zram_bvec_rw(struct zram *zram, struct bio_vec *bvec, u32 index,
 
 	generic_end_io_acct(q, rw_acct, &zram->disk->part0, start_time);
 
+	zram_slot_lock(zram, index);
+	zram_accessed(zram, index);
+	zram_slot_unlock(zram, index);
+
 	if (unlikely(ret < 0)) {
 		if (!is_write)
 			atomic64_inc(&zram->stats.failed_reads);
drivers/block/zram/zram_drv.h +1 -0
index ff0547bdb586..1075218e88b2 100644
@@ -61,6 +61,7 @@ struct zram_table_entry {
 		unsigned long element;
 	};
 	unsigned long value;
+	u64 ac_time;
 };
 
 struct zram_stats {
Signature · 1 line
2.17.0.484.g0c8726318c-goog
author maintainer
#4 lore raw

PATCH v3 4/4 zram: introduce zram memory tracking

In reply to Minchan Kim

zRam as swap is useful for small memory device. However, swap means those pages on zram are mostly cold pages due to VM's LRU algorithm. Especially, once init data for application are touched for launching, they tend to be not accessed any more and finally swapped out. zRAM can store such cold pages as compressed form but it's pointless to keep in memory. Better idea is app developers free them directly rather than remaining them on heap.

This patch tell us last access time of each block of zram via "cat /sys/kernel/debug/zram/zram0/block_state".

The output is as follows,

      300    75.033841 .wh
      301    63.806904 s..
      302    63.806919 ..h

First column is zram's block index and 3rh one represents symbol (s: same page w: written page to backing store h: huge page) of the block state. Second column represents usec time unit of the block was last accessed. So above example means the 300th block is accessed at 75.033851 second and it was huge so it was written to the backing store.

Admin can leverage this information to catch cold|incompressible pages of process with *pagemap* once part of heaps are swapped out.

Cc Greg KH <[email protected]>
Signed-off-by Minchan Kim <[email protected]>
---
 Documentation/blockdev/zram.txt |  24 ++++++
 drivers/block/zram/Kconfig      |   9 +++
 drivers/block/zram/zram_drv.c   | 139 +++++++++++++++++++++++++++++---
 drivers/block/zram/zram_drv.h   |   5 ++
 4 files changed, 166 insertions(+), 11 deletions(-)
Documentation/blockdev/zram.txt +24 -0
index 78db38d02bc9..45509c7d5716 100644
@@ -243,5 +243,29 @@ to backing storage rather than keeping it in memory.
 User should set up backing device via /sys/block/zramX/backing_dev
 before disksize setting.
 
+= memory tracking
+
+With CONFIG_ZRAM_MEMORY_TRACKING, user can know information of the
+zram block. It could be useful to catch cold or incompressible
+pages of the proess with*pagemap.
+If you enable the feature, you could see block state via
+/sys/kernel/debug/zram/zram0/block_state". The output is as follows,
+
+	  300    75.033841 .wh
+	  301    63.806904 s..
+	  302    63.806919 ..h
+
+First column is zram's block index.
+Second column is access time.
+Third column is state of the block.
+(s: same page
+w: written page to backing store
+h: huge page)
+
+First line of above example says 300th block is accessed at 75.033841sec
+and the block's state is huge so it is written back to the backing
+storage. It's a debugging feature so anyone shouldn't rely on it to work
+properly.
+
 Nitin Gupta
 [email protected]
drivers/block/zram/Kconfig +9 -0
index ac3a31d433b2..efe60c82d8ec 100644
@@ -26,3 +26,12 @@ config ZRAM_WRITEBACK
 	 /sys/block/zramX/backing_dev.
 
 	 See zram.txt for more infomration.
+
+config ZRAM_MEMORY_TRACKING
+	bool "Tracking zram block status"
+	depends on ZRAM
+	select DEBUG_FS
+	help
+	  With this feature, admin can track the state of allocated block
+	  of zRAM. Admin could see the information via
+	  /sys/kernel/debug/zram/zramX/block_state.
drivers/block/zram/zram_drv.c +128 -11

193 lines not on this page · raw message

drivers/block/zram/zram_drv.h +5 -0
index 1075218e88b2..6aeb0213afd7 100644
@@ -61,7 +61,9 @@ struct zram_table_entry {
 		unsigned long element;
 	};
 	unsigned long value;
+#ifdef	CONFIG_ZRAM_MEMORY_TRACKING
 	u64 ac_time;
+#endif
 };
 
 struct zram_stats {

@@ -110,5 +112,8 @@ struct zram {
 	unsigned long nr_pages;
 	spinlock_t bitmap_lock;
 #endif
+#ifdef CONFIG_ZRAM_MEMORY_TRACKING
+	struct dentry *debugfs_dir;
+#endif
 };
 #endif
Signature · 1 line
2.17.0.484.g0c8726318c-goog
Acked-by on v3 4/4
#5 lore raw

Re: [PATCH v3 4/4] zram: introduce zram memory tracking

Quoted 74 lines · Minchan Kim

zRam as swap is useful for small memory device. However, swap means those pages on zram are mostly cold pages due to VM's LRU algorithm. Especially, once init data for application are touched for launching, they tend to be not accessed any more and finally swapped out. zRAM can store such cold pages as compressed form but it's pointless to keep in memory. Better idea is app developers free them directly rather than remaining them on heap.

This patch tell us last access time of each block of zram via "cat /sys/kernel/debug/zram/zram0/block_state".

The output is as follows,

      300    75.033841 .wh
      301    63.806904 s..
      302    63.806919 ..h

First column is zram's block index and 3rh one represents symbol (s: same page w: written page to backing store h: huge page) of the block state. Second column represents usec time unit of the block was last accessed. So above example means the 300th block is accessed at 75.033851 second and it was huge so it was written to the backing store.

Admin can leverage this information to catch cold|incompressible pages of process with *pagemap* once part of heaps are swapped out.

Cc Greg KH <[email protected]>
Signed-off-by Minchan Kim <[email protected]>
---
 Documentation/blockdev/zram.txt |  24 ++++++
 drivers/block/zram/Kconfig      |   9 +++
 drivers/block/zram/zram_drv.c   | 139 +++++++++++++++++++++++++++++---
 drivers/block/zram/zram_drv.h   |   5 ++
 4 files changed, 166 insertions(+), 11 deletions(-)
Documentation/blockdev/zram.txt +24 -0
index 78db38d02bc9..45509c7d5716 100644
@@ -243,5 +243,29 @@ to backing storage rather than keeping it in memory.
 User should set up backing device via /sys/block/zramX/backing_dev
 before disksize setting.
 
+= memory tracking
+
+With CONFIG_ZRAM_MEMORY_TRACKING, user can know information of the
+zram block. It could be useful to catch cold or incompressible
+pages of the proess with*pagemap.
+If you enable the feature, you could see block state via
+/sys/kernel/debug/zram/zram0/block_state". The output is as follows,
+
+	  300    75.033841 .wh
+	  301    63.806904 s..
+	  302    63.806919 ..h
+
+First column is zram's block index.
+Second column is access time.
+Third column is state of the block.
+(s: same page
+w: written page to backing store
+h: huge page)
+
+First line of above example says 300th block is accessed at 75.033841sec
+and the block's state is huge so it is written back to the backing
+storage. It's a debugging feature so anyone shouldn't rely on it to work
+properly.
+
 Nitin Gupta
 [email protected]
drivers/block/zram/Kconfig +0 -0
index ac3a31d433b2..efe60c82d8ec 100644

drivers/block/zram/Kconfig config ZRAM_WRITEBACK · line 33

Quoted 8 lines
drivers/block/zram/Kconfig +5 -0
@@ -26,3 +26,12 @@ config ZRAM_WRITEBACK
 	 /sys/block/zramX/backing_dev.
 
 	 See zram.txt for more infomration.
+
+config ZRAM_MEMORY_TRACKING
+	bool "Tracking zram block status"
+	depends on ZRAM
+	select DEBUG_FS

Select? Shouldn't you depend on this instead? Selecting is a pain to try to track down what is keeping an option enabled.

drivers/block/zram/Kconfig config ZRAM_WRITEBACK · line 37

Quoted 4 lines
drivers/block/zram/Kconfig +4 -0
+	help
+	  With this feature, admin can track the state of allocated block
+	  of zRAM. Admin could see the information via
+	  /sys/kernel/debug/zram/zramX/block_state.

A short note here as to where to find the documentation for what that info is (i.e. in the file you wrote above?)

drivers/block/zram/Kconfig

Quoted 8 lines
drivers/block/zram/Kconfig +8 -0
+#else
+static void zram_debugfs_create(void) {};
+static void zram_debugfs_destroy(void) {};
+static void zram_accessed(struct zram *zram, u32 index) {};
+static void zram_reset_access(struct zram *zram, u32 index) {};
+static void zram_debugfs_register(struct zram *zram) {};
+static void zram_debugfs_unregister(struct zram *zram) {};
+#endif

Much nicer, thanks!

The above was only very minor nits, no need to change anything if you don't want to.

Acked-by Greg Kroah-Hartman <[email protected]>
#6 lore raw

Re: [PATCH v3 4/4] zram: introduce zram memory tracking

Quoted 77 lines · On Mon, 9 Apr 2018 10:03:45 +0200 Greg KH <[email protected]> wrote:
Quoted 76 lines · Minchan Kim

zRam as swap is useful for small memory device. However, swap means those pages on zram are mostly cold pages due to VM's LRU algorithm. Especially, once init data for application are touched for launching, they tend to be not accessed any more and finally swapped out. zRAM can store such cold pages as compressed form but it's pointless to keep in memory. Better idea is app developers free them directly rather than remaining them on heap.

This patch tell us last access time of each block of zram via "cat /sys/kernel/debug/zram/zram0/block_state".

The output is as follows,

      300    75.033841 .wh
      301    63.806904 s..
      302    63.806919 ..h

First column is zram's block index and 3rh one represents symbol (s: same page w: written page to backing store h: huge page) of the block state. Second column represents usec time unit of the block was last accessed. So above example means the 300th block is accessed at 75.033851 second and it was huge so it was written to the backing store.

Admin can leverage this information to catch cold|incompressible pages of process with *pagemap* once part of heaps are swapped out.

Cc Greg KH <[email protected]>
Signed-off-by Minchan Kim <[email protected]>
---
 Documentation/blockdev/zram.txt |  24 ++++++
 drivers/block/zram/Kconfig      |   9 +++
 drivers/block/zram/zram_drv.c   | 139 +++++++++++++++++++++++++++++---
 drivers/block/zram/zram_drv.h   |   5 ++
 4 files changed, 166 insertions(+), 11 deletions(-)
Documentation/blockdev/zram.txt +24 -0
index 78db38d02bc9..45509c7d5716 100644
@@ -243,5 +243,29 @@ to backing storage rather than keeping it in memory.
 User should set up backing device via /sys/block/zramX/backing_dev
 before disksize setting.
 
+= memory tracking
+
+With CONFIG_ZRAM_MEMORY_TRACKING, user can know information of the
+zram block. It could be useful to catch cold or incompressible
+pages of the proess with*pagemap.
+If you enable the feature, you could see block state via
+/sys/kernel/debug/zram/zram0/block_state". The output is as follows,
+
+	  300    75.033841 .wh
+	  301    63.806904 s..
+	  302    63.806919 ..h
+
+First column is zram's block index.
+Second column is access time.
+Third column is state of the block.
+(s: same page
+w: written page to backing store
+h: huge page)
+
+First line of above example says 300th block is accessed at 75.033841sec
+and the block's state is huge so it is written back to the backing
+storage. It's a debugging feature so anyone shouldn't rely on it to work
+properly.
+
 Nitin Gupta
 [email protected]
drivers/block/zram/Kconfig +0 -0
index ac3a31d433b2..efe60c82d8ec 100644
@@ -26,3 +26,12 @@ config ZRAM_WRITEBACK
 	 /sys/block/zramX/backing_dev.
Quoted 9 lines

drivers/block/zram/Kconfig config ZRAM_WRITEBACK · line 33

Quoted 6 lines
drivers/block/zram/Kconfig +5 -0
 	 See zram.txt for more infomration.
+
+config ZRAM_MEMORY_TRACKING
+	bool "Tracking zram block status"
+	depends on ZRAM
+	select DEBUG_FS

Select? Shouldn't you depend on this instead? Selecting is a pain to try to track down what is keeping an option enabled.

Okay, I will modify.

Quoted 8 lines

drivers/block/zram/Kconfig config ZRAM_WRITEBACK · line 37

Quoted 5 lines
drivers/block/zram/Kconfig +4 -0
+	help
+	  With this feature, admin can track the state of allocated block
+	  of zRAM. Admin could see the information via
+	  /sys/kernel/debug/zram/zramX/block_state.

A short note here as to where to find the documentation for what that info is (i.e. in the file you wrote above?)

Yub. Will change.

Quoted 12 lines

drivers/block/zram/Kconfig

Quoted 9 lines
drivers/block/zram/Kconfig +8 -0
+#else
+static void zram_debugfs_create(void) {};
+static void zram_debugfs_destroy(void) {};
+static void zram_accessed(struct zram *zram, u32 index) {};
+static void zram_reset_access(struct zram *zram, u32 index) {};
+static void zram_debugfs_register(struct zram *zram) {};
+static void zram_debugfs_unregister(struct zram *zram) {};
+#endif

Much nicer, thanks!

Quoted 4 lines

The above was only very minor nits, no need to change anything if you don't want to.

Acked-by Greg Kroah-Hartman <[email protected]>

I will wait a few days for getting review from Sergey and resend it with fix.
Thanks for the review, Greg.

#7 lore raw

Re: [PATCH v3 4/4] zram: introduce zram memory tracking

In reply to Minchan Kim

Hi Minchan,

drivers/block/zram/zram_drv.c static int read_from_bdev(struct zram *zram, struct bio_vec *bvec, · line 650

Quoted 6 lines · Minchan Kim
drivers/block/zram/zram_drv.c +6 -0
+static long long ns2usecs(u64 nsec)
+{
+	nsec += 500;
+	do_div(nsec, 1000);
+	return nsec;
+}

Sorry, I don't quite understand "nsec += 500".

Quoted 3 lines
drivers/block/zram/zram_drv.c +3 -0
+static ssize_t read_block_state(struct file *file, char __user *buf,
+				size_t count, loff_t *ppos)
+{

[..]

drivers/block/zram/zram_drv.c static int read_from_bdev(struct zram *zram, struct bio_vec *bvec, · line 701

Quoted 3 lines
drivers/block/zram/zram_drv.c +3 -0
+
+	up_read(&zram->init_lock);
+	copy_to_user(buf, kbuf, written);

drivers/block/zram/zram_drv.c: In function ‘read_block_state’:
drivers/block/zram/zram_drv.c:701:2: warning: ignoring return value of ‘copy_to_user’, declared with attribute warn_unused_result [-Wunused-result]
copy_to_user(buf, kbuf, written);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[..]

drivers/block/zram/zram_drv.h struct zram_table_entry { · line 64

Quoted 5 lines
drivers/block/zram/zram_drv.h +1 -0
@@ -61,7 +61,9 @@ struct zram_table_entry {
 		unsigned long element;
 	};
 	unsigned long value;
+#ifdef	CONFIG_ZRAM_MEMORY_TRACKING

A silly nitpick, but since you are going to resend this one, could you remove TAB between ifdef and CONFIG_ZRAM_MEMORY_TRACKING?

	-ss
#8 lore raw

Re: [PATCH v3 4/4] zram: introduce zram memory tracking

I think zram_debugfs_unregister() is called both from zram_remove() and from destroy_devices(), which explodes:

[ 1906.467981] BUG: unable to handle kernel NULL pointer dereference at 00000000000000b8
[ 1906.468007] PGD 80000003d5050067 P4D 80000003d5050067 PUD 3fa97d067 PMD 0
[ 1906.468029] Oops: 0002 [#1] PREEMPT SMP PTI
[ 1906.468035] Modules linked in: zram(-) zsmalloc mousedev hid_generic rndis_host usbhid cdc_ether usbnet hid snd_hda_codec_realtek snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hda_core coretemp snd_pcm hwmon snd_timer snd r8169 i2c_i801 soundcore mii xhci_pci xhci_hcd usbcore sd_mod usb_common
[ 1906.468069] RIP: 0010:down_write+0x29/0x46
[ 1906.468073] RSP: 0018:ffffb9dd0480fdd0 EFLAGS: 00010246
[ 1906.468077] RAX: 00000000000000b8 RBX: 00000000000000b8 RCX: 0000000000000001
[ 1906.468082] RDX: ffffffff00000001 RSI: 0000000000000045 RDI: ffffffffb1baa40a
[ 1906.468086] RBP: ffffa189fce9fbd8 R08: ffffb9dd0480fddc R09: 0000000000000007
[ 1906.468091] R10: ffffb9dd0480fe40 R11: 0000000000000000 R12: ffffa189fce9fbd8
[ 1906.468095] R13: ffffffffb23cab54 R14: 0000000000000000 R15: 0000000000000000
[ 1906.468100] FS:  00007fbc72cd2b80(0000) GS:ffffa18a1ed00000(0000) knlGS:0000000000000000
[ 1906.468105] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1906.468110] CR2: 00000000000000b8 CR3: 00000003b4570003 CR4: 00000000001606e0
[ 1906.468114] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1906.468119] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 1906.468123] Call Trace:
[ 1906.468130]  debugfs_remove_recursive+0x40/0x15e
[ 1906.468136]  zram_remove+0x7f/0xf6 [zram]
[ 1906.468141]  ? hot_remove_store+0x93/0x93 [zram]
[ 1906.468146]  zram_remove_cb+0xd/0x10 [zram]
[ 1906.468150]  idr_for_each+0x50/0xc7
[ 1906.468155]  destroy_devices+0x32/0x5c [zram]
[ 1906.468160]  SyS_delete_module+0xfd/0x19a
[ 1906.468165]  do_syscall_64+0x17e/0x195
[ 1906.468169]  entry_SYSCALL_64_after_hwframe+0x3d/0xa2
[ 1906.468174] RIP: 0033:0x7fbc723cb5d7
[ 1906.468177] RSP: 002b:00007ffc40316d48 EFLAGS: 00000206 ORIG_RAX: 00000000000000b0
[ 1906.468182] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fbc723cb5d7
[ 1906.468187] RDX: 000000000000000a RSI: 0000000000000800 RDI: 00000000013e7778
[ 1906.468191] RBP: 00000000013e7710 R08: 00007ffc40315cc1 R09: 0000000000000000
[ 1906.468196] R10: 00000000000008b2 R11: 0000000000000206 R12: 00007ffc403188b4
[ 1906.468201] R13: 0000000000000000 R14: 00000000013e7710 R15: 00000000013e7260
[ 1906.468761] Code: ff ff 0f 1f 44 00 00 53 31 d2 48 89 fb be 45 00 00 00 48 c7 c7 0a a4 ba b1 e8 ef bf b9 ff 48 89 d8 48 ba 01 00 00 00 ff ff ff ff <f0> 48 0f c1 10 85 d2 74 05 e8 9e bd ff ff 65 48 8b 04 25 80 4d
[ 1906.469296] RIP: down_write+0x29/0x46 RSP: ffffb9dd0480fdd0
[ 1906.469995] CR2: 00000000000000b8
[ 1906.475689] ---[ end trace 10d42324537dfb4c ]---
	-ss
author maintainer
#9 lore raw

Re: [PATCH v3 4/4] zram: introduce zram memory tracking

I will fix and resend tomorrow.
Huge thanks, Sergey!

Quoted 42 lines · Sergey Senozhatsky

I think zram_debugfs_unregister() is called both from zram_remove() and from destroy_devices(), which explodes:

[ 1906.467981] BUG: unable to handle kernel NULL pointer dereference at 00000000000000b8
[ 1906.468007] PGD 80000003d5050067 P4D 80000003d5050067 PUD 3fa97d067 PMD 0
[ 1906.468029] Oops: 0002 [#1] PREEMPT SMP PTI
[ 1906.468035] Modules linked in: zram(-) zsmalloc mousedev hid_generic rndis_host usbhid cdc_ether usbnet hid snd_hda_codec_realtek snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hda_core coretemp snd_pcm hwmon snd_timer snd r8169 i2c_i801 soundcore mii xhci_pci xhci_hcd usbcore sd_mod usb_common
[ 1906.468069] RIP: 0010:down_write+0x29/0x46
[ 1906.468073] RSP: 0018:ffffb9dd0480fdd0 EFLAGS: 00010246
[ 1906.468077] RAX: 00000000000000b8 RBX: 00000000000000b8 RCX: 0000000000000001
[ 1906.468082] RDX: ffffffff00000001 RSI: 0000000000000045 RDI: ffffffffb1baa40a
[ 1906.468086] RBP: ffffa189fce9fbd8 R08: ffffb9dd0480fddc R09: 0000000000000007
[ 1906.468091] R10: ffffb9dd0480fe40 R11: 0000000000000000 R12: ffffa189fce9fbd8
[ 1906.468095] R13: ffffffffb23cab54 R14: 0000000000000000 R15: 0000000000000000
[ 1906.468100] FS:  00007fbc72cd2b80(0000) GS:ffffa18a1ed00000(0000) knlGS:0000000000000000
[ 1906.468105] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1906.468110] CR2: 00000000000000b8 CR3: 00000003b4570003 CR4: 00000000001606e0
[ 1906.468114] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1906.468119] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 1906.468123] Call Trace:
[ 1906.468130]  debugfs_remove_recursive+0x40/0x15e
[ 1906.468136]  zram_remove+0x7f/0xf6 [zram]
[ 1906.468141]  ? hot_remove_store+0x93/0x93 [zram]
[ 1906.468146]  zram_remove_cb+0xd/0x10 [zram]
[ 1906.468150]  idr_for_each+0x50/0xc7
[ 1906.468155]  destroy_devices+0x32/0x5c [zram]
[ 1906.468160]  SyS_delete_module+0xfd/0x19a
[ 1906.468165]  do_syscall_64+0x17e/0x195
[ 1906.468169]  entry_SYSCALL_64_after_hwframe+0x3d/0xa2
[ 1906.468174] RIP: 0033:0x7fbc723cb5d7
[ 1906.468177] RSP: 002b:00007ffc40316d48 EFLAGS: 00000206 ORIG_RAX: 00000000000000b0
[ 1906.468182] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fbc723cb5d7
[ 1906.468187] RDX: 000000000000000a RSI: 0000000000000800 RDI: 00000000013e7778
[ 1906.468191] RBP: 00000000013e7710 R08: 00007ffc40315cc1 R09: 0000000000000000
[ 1906.468196] R10: 00000000000008b2 R11: 0000000000000206 R12: 00007ffc403188b4
[ 1906.468201] R13: 0000000000000000 R14: 00000000013e7710 R15: 00000000013e7260
[ 1906.468761] Code: ff ff 0f 1f 44 00 00 53 31 d2 48 89 fb be 45 00 00 00 48 c7 c7 0a a4 ba b1 e8 ef bf b9 ff 48 89 d8 48 ba 01 00 00 00 ff ff ff ff <f0> 48 0f c1 10 85 d2 74 05 e8 9e bd ff ff 65 48 8b 04 25 80 4d
[ 1906.469296] RIP: down_write+0x29/0x46 RSP: ffffb9dd0480fdd0
[ 1906.469995] CR2: 00000000000000b8
[ 1906.475689] ---[ end trace 10d42324537dfb4c ]---
	-ss
author maintainer
#10 lore raw

Re: [PATCH v3 4/4] zram: introduce zram memory tracking

In reply to Sergey Senozhatsky
Quoted 11 lines · Sergey Senozhatsky

Hi Minchan,

drivers/block/zram/zram_drv.c static int read_from_bdev(struct zram *zram, struct bio_vec *bvec, · line 650

Quoted 6 lines · Minchan Kim
drivers/block/zram/zram_drv.c +6 -0
+static long long ns2usecs(u64 nsec)
+{
+	nsec += 500;
+	do_div(nsec, 1000);
+	return nsec;
+}

Sorry, I don't quite understand "nsec += 500".

Round-off

Quoted 5 lines
Quoted 4 lines
drivers/block/zram/zram_drv.c +3 -0
+static ssize_t read_block_state(struct file *file, char __user *buf,
+				size_t count, loff_t *ppos)
+{

[..]

Quoted 8 lines

drivers/block/zram/zram_drv.c static int read_from_bdev(struct zram *zram, struct bio_vec *bvec, · line 701

Quoted 3 lines
drivers/block/zram/zram_drv.c +3 -0
+
+	up_read(&zram->init_lock);
+	copy_to_user(buf, kbuf, written);

drivers/block/zram/zram_drv.c: In function ‘read_block_state’:
drivers/block/zram/zram_drv.c:701:2: warning: ignoring return value of ‘copy_to_user’, declared with attribute warn_unused_result [-Wunused-result]
copy_to_user(buf, kbuf, written);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Good catch.

Quoted 10 lines

[..]

drivers/block/zram/zram_drv.h struct zram_table_entry { · line 64

Quoted 5 lines
drivers/block/zram/zram_drv.h +1 -0
@@ -61,7 +61,9 @@ struct zram_table_entry {
 		unsigned long element;
 	};
 	unsigned long value;
+#ifdef	CONFIG_ZRAM_MEMORY_TRACKING

A silly nitpick, but since you are going to resend this one, could you remove TAB between ifdef and CONFIG_ZRAM_MEMORY_TRACKING?

Sure.