| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20180327072310.GA189539@rodete-desktop-imager.corp.google.com>
Date: Tue, 27 Mar 2018 16:23:10 +0900
From: Minchan Kim <minchan@...nel.org>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 4/4] zram: introduce zram memory tracking
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.
Powered by blists - more mailing lists