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

[Minchan Kim]

Hi Greg,

On Tue, Mar 27, 2018 at 09:12:38AM +0200, Greg KH wrote:
> On Tue, Mar 27, 2018 at 03:50:33PM +0900, Minchan Kim wrote:
> > 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,
> > 
> >    25064    .wh       100
> >    25065    s..       9
> >    25067    ..h       15
> > 
> > First column is zram's block index and second one represents symbol
> > (s: same page w: written page to backing store h: huge page) of the
> > block state. Last one means number of seconds elapsed since the block
> > was last accessed. So above example means the 25065th block is accessed
> > 100 second ago 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.
> 
> For debugging, yes, but remember that no system functionality should
> ever depend on debugfs :)

Sure, any functionality of zram doesn't rely on the this feature.

> 
> > +config ZRAM_MEMORY_TRACKING
> > +       bool "Tracking zram block status"
> > +       depends on ZRAM
> > +       select DEBUG_FS
> 
> depends on?
> 
> > +       default n
> 
> That's always the default, no need to list it.

Thanks. I will clean it out.

> 
> > +       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.
> 
> Odd indentation, please use tabs everywhere here.

Oops.

> 
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -31,10 +31,15 @@
> >  #include <linux/err.h>
> >  #include <linux/idr.h>
> >  #include <linux/sysfs.h>
> > +#include <linux/debugfs.h>
> >  #include <linux/cpuhotplug.h>
> >  
> >  #include "zram_drv.h"
> >  
> > +#ifdef CONFIG_ZRAM_MEMORY_TRACKING
> > +static struct dentry *zram_debugfs_root;
> > +#endif
> > +
> >  static DEFINE_IDR(zram_index_idr);
> >  /* idr index must be protected */
> >  static DEFINE_MUTEX(zram_index_mutex);
> > @@ -67,6 +72,13 @@ static inline bool init_done(struct zram *zram)
> >  	return zram->disksize;
> >  }
> >  
> > +static inline bool zram_allocated(struct zram *zram, u32 index)
> > +{
> > +
> > +	return (zram->table[index].value >> (ZRAM_FLAG_SHIFT + 1)) ||
> > +					zram->table[index].handle;
> > +}
> > +
> >  static inline struct zram *dev_to_zram(struct device *dev)
> >  {
> >  	return (struct zram *)dev_to_disk(dev)->private_data;
> > @@ -83,7 +95,7 @@ static void zram_set_handle(struct zram *zram, u32 index, unsigned long handle)
> >  }
> >  
> >  /* flag operations require table entry bit_spin_lock() being held */
> > -static int zram_test_flag(struct zram *zram, u32 index,
> > +static bool zram_test_flag(struct zram *zram, u32 index,
> >  			enum zram_pageflags flag)
> >  {
> >  	return zram->table[index].value & BIT(flag);
> > @@ -107,16 +119,6 @@ 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 = get_seconds();
> > -}
> > -
> > -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;
> > @@ -620,6 +622,98 @@ static int read_from_bdev(struct zram *zram, struct bio_vec *bvec,
> >  static void zram_wb_clear(struct zram *zram, u32 index) {}
> >  #endif
> >  
> > +#ifdef CONFIG_ZRAM_MEMORY_TRACKING
> > +static void zram_accessed(struct zram *zram, u32 index)
> > +{
> > +	zram->table[index].ac_time = get_seconds();
> > +}
> > +
> > +static void zram_reset_access(struct zram *zram, u32 index)
> > +{
> > +	zram->table[index].ac_time = 0;
> > +}
> > +
> > +static ssize_t read_block_state(struct file *file, char __user *buf,
> > +				size_t count, loff_t *ppos)
> > +{
> > +	char *kbuf;
> > +	ssize_t index, written = 0;
> > +	struct zram *zram = file->private_data;
> > +	unsigned long last_access, nr_pages = zram->disksize >> PAGE_SHIFT;
> > +	char flags[__NR_ZRAM_PAGEFLAGS - ZRAM_FLAG_SHIFT];
> > +
> > +	kbuf = kvmalloc(count, GFP_KERNEL);
> > +	if (!kbuf)
> > +		return -ENOMEM;
> > +
> > +	down_read(&zram->init_lock);
> > +	if (!init_done(zram)) {
> > +		up_read(&zram->init_lock);
> > +		kvfree(kbuf);
> > +		return -EINVAL;
> > +	}
> > +
> > +	for (index = *ppos; index < nr_pages; index++) {
> > +		int copied;
> > +
> > +		zram_slot_lock(zram, index);
> > +		if (!zram_allocated(zram, index))
> > +			goto next;
> > +
> > +		snprintf(flags, sizeof(flags), "%c%c%c",
> > +			zram_test_flag(zram, index, ZRAM_SAME) ? 's' : '.',
> > +			zram_test_flag(zram, index, ZRAM_WB) ? 'w' : '.',
> > +			zram_test_flag(zram, index, ZRAM_HUGE) ? 'h' : '.');
> > +
> > +		last_access = get_seconds() - zram->table[index].ac_time;
> > +		copied = snprintf(kbuf + written, count, "%12lu %6s %12lu\n",
> > +						index, flags, last_access);
> > +		if (count < copied) {
> > +			zram_slot_unlock(zram, index);
> > +			break;
> > +		}
> > +		written += copied;
> > +		count -= copied;
> > +next:
> > +		zram_slot_unlock(zram, index);
> > +		*ppos += 1;
> > +	}
> > +
> > +	up_read(&zram->init_lock);
> > +	copy_to_user(buf, kbuf, written);
> > +	kvfree(kbuf);
> > +
> > +	return written;
> > +}
> > +
> > +static const struct file_operations proc_zram_block_state_op = {
> > +	.open = simple_open,
> > +	.read = read_block_state,
> > +	.llseek = default_llseek,
> > +};
> > +
> > +static void zram_debugfs_register(struct zram *zram)
> > +{
> > +	if (!zram_debugfs_root)
> > +		return;
> > +
> > +	zram->debugfs_dir = debugfs_create_dir(zram->disk->disk_name,
> > +						zram_debugfs_root);
> > +	debugfs_create_file("block_state", 0400, zram->debugfs_dir,
> > +				zram, &proc_zram_block_state_op);
> > +}
> 
> Much nicer code, thanks for the changes.
> 
> >  static void destroy_devices(void)
> >  {
> >  	class_unregister(&zram_control_class);
> > +#ifdef CONFIG_ZRAM_MEMORY_TRACKING
> > +	debugfs_remove_recursive(zram_debugfs_root);
> > +#endif
> 
> No need for #ifdefs in .c code if your .h files are correct.
> 
> >  	idr_for_each(&zram_index_idr, &zram_remove_cb, NULL);
> >  	idr_destroy(&zram_index_idr);
> >  	unregister_blkdev(zram_major, "zram");
> > @@ -1760,6 +1859,9 @@ static int __init zram_init(void)
> >  		return ret;
> >  	}
> >  
> > +#ifdef CONFIG_ZRAM_MEMORY_TRACKING
> > +	zram_debugfs_root = debugfs_create_dir("zram", NULL);
> > +#endif
> 
> Same here. Put this in a wrapper function somewhere.

Okay.
Thanks for the super fast review, Greg!

I bet there are several Greg in the world and each of them live
different time zone conutries.