zram: correct flag name of ZRAM_ACCESS
Subsystems (2)
Minchan Kim
maintainerPATCH 1/2 zram: correct flag name of ZRAM_ACCESS
ZRAM_ACCESS is meant to lock a slot of zram so correct the name.
--- drivers/block/zram/zram_drv.c | 4 ++-- drivers/block/zram/zram_drv.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
drivers/block/zram/zram_drv.c +2 -2
@@ -755,12 +755,12 @@ 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); +bit_spin_lock (ZRAM_LOCK , &zram->table[index].value); }static void zram_slot_unlock (struct zram *zram,u32 index) { -bit_spin_unlock (ZRAM_ACCESS , &zram->table[index].value); +bit_spin_unlock (ZRAM_LOCK , &zram->table[index].value); }static void zram_meta_free (struct zram *zram,u64 disksize)
drivers/block/zram/zram_drv.h +1 -1
@@ -46,7 +46,7 @@enum zram_pageflags {/* Page consists the same element */ ZRAM_SAME =ZRAM_FLAG_SHIFT , -ZRAM_ACCESS ,/* page is now accessed */ +ZRAM_LOCK ,/* zram slot is locked */ ZRAM_WB ,/* page is stored on backing_device */ __NR_ZRAM_PAGEFLAGS,
Signature · 1 line
2.17.0.rc0.231.g781580f067-goog
Minchan Kim
maintainerPATCH 2/2 zram: idle 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. Better idea is app developers free them directly rather than remaining them on heap.
This patch tell us last accesss time of each block of zram via "cat /sys/kernel/debug/zram/zram0/access_time".
The output is as follows,
276 1250
277 1800
.. ..
.. ..
First column is zram's block index and second one represents number of seconds elapsed since the block was last accessed.
So above example means the 276th block is accessed 1250 second ago and 277th block is accessed 1800 second ago.
Admin can leverage this information to catch cold pages of process with *pagemap* once part of heaps are swapped out.
--- drivers/block/zram/Kconfig | 11 +++ drivers/block/zram/zram_drv.c | 159 +++++++++++++++++++++++++++++++--- drivers/block/zram/zram_drv.h | 6 ++ 3 files changed, 166 insertions(+), 10 deletions(-)
drivers/block/zram/Kconfig +11 -0
@@ -26,3 +26,14 @@ config ZRAM_WRITEBACK /sys/block/zramX/backing_dev. See zram.txt for more infomration. + +config ZRAM_IDLE_MEMORY_TRACKING + bool "tracking access time of zram block" + depends on ZRAM + select DEBUG_FS + default n + help + Record zram block's access time and expose the information via + /sys/kernel/debug/zram/zramX/access_time. If blocks of zram are + mapped of process's address space, you can see which processes + have cold pages via using pagemap and access_time.
drivers/block/zram/zram_drv.c +149 -10
220 lines not on this page · raw message
drivers/block/zram/zram_drv.h +6 -0
@@ -61,6 +61,9 @@ struct zram_table_entry {unsigned long element; };unsigned long value; +#ifdef CONFIG_ZRAM_IDLE_MEMORY_TRACKING +unsigned long ac_time; +#endif };struct zram_stats {
@@ -108,5 +111,8 @@ struct zram {unsigned long nr_pages;spinlock_t bitmap_lock;#endif +#ifdef CONFIG_DEBUG_FS +struct dentry *debugfs_dir; +#endif };#endif
Signature · 1 line
2.17.0.rc0.231.g781580f067-goog
Re: [PATCH 2/2] zram: idle memory tracking
drivers/block/zram/zram_drv.c static ssize_t mem_used_max_store(struct device *dev, · line 349
Quoted 6 lines · Minchan Kim
drivers/block/zram/zram_drv.c +6
-0+static int zram_debugfs_register (struct zram *zram) +{ +struct dentry *ret; + +if (!zram_debugfs_root) +return -ENOENT ;
No need to care, you should not error out if debugfs is not enabled or not working, your code path should be identical either way. debugfs is not required for any functionality, so don't treat it like it matters :)
drivers/block/zram/zram_drv.c static ssize_t mem_used_max_store(struct device *dev, · line 354
Quoted 4 lines
drivers/block/zram/zram_drv.c +4
-0+ zram->debugfs_dir =debugfs_create_dir (zram->disk->disk_name, + zram_debugfs_root); +if (!zram->debugfs_dir) +return -ENOMEM ;
No need to check the return value of any debugfs call, you can always either use it for future debugfs calls, or ignore it.
drivers/block/zram/zram_drv.c static ssize_t mem_used_max_store(struct device *dev, · line 359
Quoted 4 lines
drivers/block/zram/zram_drv.c +4
-0+ ret =debugfs_create_file ("access_time" ,0400 , zram->debugfs_dir, + zram, &proc_zram_access_operations); +if (!ret) +return -ENOMEM ;
Again, you shouldn't care :)
Review
Quoted 2 lines
(quoted diff) +2
-0+ + return 0;
just return void, no need to test any of this.
thanks,
greg k-h
Minchan Kim
maintainerRe: [PATCH 2/2] zram: idle memory tracking
Quoted 11 lines · Greg KH
drivers/block/zram/zram_drv.c static ssize_t mem_used_max_store(struct device *dev, · line 349
Quoted 6 lines · Minchan Kim
drivers/block/zram/zram_drv.c +6
-0+static int zram_debugfs_register (struct zram *zram) +{ +struct dentry *ret; + +if (!zram_debugfs_root) +return -ENOENT ;No need to care, you should not error out if debugfs is not enabled or not working, your code path should be identical either way. debugfs is not required for any functionality, so don't treat it like it matters :)
Glad to hear. It makes code much clean.
Quoted 16 lines
drivers/block/zram/zram_drv.c static ssize_t mem_used_max_store(struct device *dev, · line 354
Quoted 5 lines
drivers/block/zram/zram_drv.c +4
-0+ zram->debugfs_dir =debugfs_create_dir (zram->disk->disk_name, + zram_debugfs_root); +if (!zram->debugfs_dir) +return -ENOMEM ;No need to check the return value of any debugfs call, you can always either use it for future debugfs calls, or ignore it.
drivers/block/zram/zram_drv.c static ssize_t mem_used_max_store(struct device *dev, · line 359
Quoted 4 lines
drivers/block/zram/zram_drv.c +4
-0+ ret =debugfs_create_file ("access_time" ,0400 , zram->debugfs_dir, + zram, &proc_zram_access_operations); +if (!ret) +return -ENOMEM ;Again, you shouldn't care :)
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ + return 0;just return void, no need to test any of this.
Thanks for the quick review, Greg.
Re: [PATCH 2/2] zram: idle memory tracking
In reply to Minchan KimQuoted 11 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 accesss time of each block of zram via "cat
/sys/kernel/debug/zram/zram0/access_time".
Quoted 6 lines
The output is as follows,
276 1250
277 1800
.. ..
.. ..
So can we just use CONFIG_IDLE_PAGE_TRACKING + CONFIG_PAGE_OWNER?
-ss
Minchan Kim
maintainerRe: [PATCH 2/2] zram: idle memory tracking
Quoted 12 lines · Sergey Senozhatsky
Quoted 11 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 accesss time of each block of zram via "cat
/sys/kernel/debug/zram/zram0/access_time".
Quoted 8 lines
Quoted 6 lines
The output is as follows,
276 1250
277 1800
.. ..
.. ..So can we just use
CONFIG_IDLE_PAGE_TRACKING+CONFIG_PAGE_OWNER?
The goal is not mapped memory tracking but one swapped out. Can we do it by above two combination?
Furthermore, I am planning to provide incompressible pages as well as access time so I will change interface at v2.
Thanks.
Re: [PATCH 2/2] zram: idle memory tracking
Quoted 20 lines · Minchan Kim
Quoted 19 lines · Sergey Senozhatsky
Quoted 17 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 accesss time of each block of zram via "cat
/sys/kernel/debug/zram/zram0/access_time".The output is as follows,
276 1250
277 1800
.. ..
.. ..
Quoted 4 lines
Quoted 1 line
So can we just use
CONFIG_IDLE_PAGE_TRACKING+CONFIG_PAGE_OWNER?The goal is not mapped memory tracking but one swapped out. Can we do it by above two combination?
Ah, you are right. page_idle tracks only pages which are on LRU lists.
Quoted 2 lines
Furthermore, I am planning to provide incompressible pages as well as access time so I will change interface at v2.
OK.
-ss
Re: [PATCH 2/2] zram: idle memory tracking
Quoted 21 lines · Sergey Senozhatsky
Quoted 20 lines · Minchan Kim
Quoted 19 lines · Sergey Senozhatsky
Quoted 17 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 accesss time of each block of zram via "cat
/sys/kernel/debug/zram/zram0/access_time".The output is as follows,
276 1250
277 1800
.. ..
.. ..
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
So can we just use
CONFIG_IDLE_PAGE_TRACKING+CONFIG_PAGE_OWNER?The goal is not mapped memory tracking but one swapped out. Can we do it by above two combination?
Ah, you are right.
page_idletracks only pages which are on LRU lists.
Can a universal (not specific to zram) idle swapped out page tracking be of interest to MM people? With the exactly same motivation. Some swaps use nfs, for instance, so idle swap pages tracking can be helpful not only in embedded domain.
-ss
Minchan Kim
maintainerRe: [PATCH 2/2] zram: idle memory tracking
Quoted 27 lines · Sergey Senozhatsky
Quoted 26 lines · Sergey Senozhatsky
Quoted 24 lines · Minchan Kim
Quoted 12 lines · Sergey Senozhatsky
Quoted 11 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 accesss time of each block of zram via "cat
/sys/kernel/debug/zram/zram0/access_time".Quoted 8 lines
Quoted 6 lines
The output is as follows,
276 1250
277 1800
.. ..
.. ..So can we just use
CONFIG_IDLE_PAGE_TRACKING+CONFIG_PAGE_OWNER?The goal is not mapped memory tracking but one swapped out. Can we do it by above two combination?
Quoted 6 lines
Quoted 1 line
Ah, you are right.
page_idletracks only pages which are on LRU lists.Can a universal (not specific to zram) idle swapped out page tracking be of interest to MM people? With the exactly same motivation. Some swaps use nfs, for instance, so idle swap pages tracking can be helpful not only in embedded domain.
If somebody outside of embedded has an interest in, sometime, he could. However, I doubt because swap is really hard part to get review because many people on current MM seem to not care. I guess they don't use swap heavily compared to old. That's why swap code is really horrible these days. :/
Anyway, I don't want to be stuck because zram has more own attributes generic cannot cover.
Re: [PATCH 2/2] zram: idle memory tracking
In reply to Minchan KimOn (03/26/18 15:49), Minchan Kim wrote:
[..]
Quoted 3 lines
drivers/block/zram/zram_drv.c +3
-0+static ssize_t read_access_time (struct file *file,char __user *buf, +size_t count,loff_t *ppos) +{
[..]
drivers/block/zram/zram_drv.c static ssize_t mem_used_max_store(struct device *dev, · line 312
Quoted 5 lines
drivers/block/zram/zram_drv.c +5
-0+ +for (index = *ppos; index < nr_pages; index++) { +zram_slot_lock (zram, index); +if (zram_test_flag (zram, index,ZRAM_WB )) + goto next;
I think it'll be better to show ZRAM_WB pages as well. Those pages are swapped out pages after all, consuming swap space - a physical page. Freeing those should also be beneficial. So tracking "bad" incompressible pages is quite handful.
drivers/block/zram/zram_drv.c static ssize_t mem_used_max_store(struct device *dev, · line 319
Quoted 7 lines
drivers/block/zram/zram_drv.c +7
-0+if (zram_test_flag (zram, index,ZRAM_SAME )) + goto next; +if (!zram_get_handle (zram, index)) + goto next; + + ret =snprintf (kbuf + written, remained,"%lu %lu\n" , index, +get_seconds () - zram->table[index].ac_time);
So here you can add a flag after index and age which will tell if the page is in zsmalloc pool or on backing swap device.
Quoted 7 lines
drivers/block/zram/zram_drv.c +7
-0+if (remained < ret) { +zram_slot_unlock (zram, index); +break ; + } + written += ret; + remained -= ret; +next:
drivers/block/zram/zram_drv.c static ssize_t mem_used_max_store(struct device *dev, · line 336
Quoted 10 lines
drivers/block/zram/zram_drv.c +10
-0+zram_slot_unlock (zram, index); + *ppos +=1 ; + } + +up_read (&zram->init_lock); +copy_to_user (buf, kbuf, written); +kvfree (kbuf); + +return written; +}
-ss
Minchan Kim
maintainerRe: [PATCH 2/2] zram: idle memory tracking
Quoted 17 lines · Sergey Senozhatsky
On (03/26/18 15:49), Minchan Kim wrote:
[..]Quoted 3 lines
drivers/block/zram/zram_drv.c +3
-0+static ssize_t read_access_time (struct file *file,char __user *buf, +size_t count,loff_t *ppos) +{[..]
drivers/block/zram/zram_drv.c static ssize_t mem_used_max_store(struct device *dev, · line 312
Quoted 5 lines
drivers/block/zram/zram_drv.c +5
-0+ +for (index = *ppos; index < nr_pages; index++) { +zram_slot_lock (zram, index); +if (zram_test_flag (zram, index,ZRAM_WB )) + goto next;I think it'll be better to show
ZRAM_WBpages as well. Those pages are swapped out pages after all, consuming swap space - a physical page. Freeing those should also be beneficial. So tracking "bad" incompressible pages is quite handful.
Quoted 10 lines
drivers/block/zram/zram_drv.c static ssize_t mem_used_max_store(struct device *dev, · line 319
Quoted 7 lines
drivers/block/zram/zram_drv.c +7
-0+if (zram_test_flag (zram, index,ZRAM_SAME )) + goto next; +if (!zram_get_handle (zram, index)) + goto next; + + ret =snprintf (kbuf + written, remained,"%lu %lu\n" , index, +get_seconds () - zram->table[index].ac_time);So here you can add a flag after index and age which will tell if the page is in zsmalloc pool or on backing swap device.
Yeb, that's the way I'm approach during our discussion!
Thanks for the review, Sergey!