lkml.org 
[lkml]   [2013]   [Jan]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCHv2 8/9] zswap: add to mm/
    On 01/07/2013 03:24 PM, Seth Jennings wrote:
    > zswap is a thin compression backend for frontswap. It receives
    > pages from frontswap and attempts to store them in a compressed
    > memory pool, resulting in an effective partial memory reclaim and
    > dramatically reduced swap device I/O.
    >
    > Additional, in most cases, pages can be retrieved from this
    > compressed store much more quickly than reading from tradition
    > swap devices resulting in faster performance for many workloads.
    >
    > This patch adds the zswap driver to mm/
    >
    > Signed-off-by: Seth Jennings <sjenning@linux.vnet.ibm.com>

    I like the approach of flushing pages into actual disk based
    swap when compressed swap is full. I would like it if that
    was advertised more prominently in the changelog :)

    The code looks mostly good, complaints are at the nitpick level.

    One worry is that the pool can grow to whatever maximum was
    decided, and there is no way to shrink it when memory is
    required for something else.

    Would it be an idea to add a shrinker for the zcache pool,
    that can also shrink the zcache pool when required?

    Of course, that does lead to the question of how to balance
    the pressure from that shrinker, with the new memory entering
    zcache from the swap side. I have no clear answers here, just
    something to think about...


    > +static void zswap_flush_entries(unsigned type, int nr)
    > +{
    > + struct zswap_tree *tree = zswap_trees[type];
    > + struct zswap_entry *entry;
    > + int i, ret;
    > +
    > +/*
    > + * This limits is arbitrary for now until a better
    > + * policy can be implemented. This is so we don't
    > + * eat all of RAM decompressing pages for writeback.
    > + */
    > +#define ZSWAP_MAX_OUTSTANDING_FLUSHES 64
    > + if (atomic_read(&zswap_outstanding_flushes) >
    > + ZSWAP_MAX_OUTSTANDING_FLUSHES)
    > + return;

    Having this #define right in the middle of the function is
    rather ugly. Might be worth moving it to the top.

    > +static int __init zswap_debugfs_init(void)
    > +{
    > + if (!debugfs_initialized())
    > + return -ENODEV;
    > +
    > + zswap_debugfs_root = debugfs_create_dir("zswap", NULL);
    > + if (!zswap_debugfs_root)
    > + return -ENOMEM;
    > +
    > + debugfs_create_u64("saved_by_flush", S_IRUGO,
    > + zswap_debugfs_root, &zswap_saved_by_flush);
    > + debugfs_create_u64("pool_limit_hit", S_IRUGO,
    > + zswap_debugfs_root, &zswap_pool_limit_hit);
    > + debugfs_create_u64("reject_flush_attempted", S_IRUGO,
    > + zswap_debugfs_root, &zswap_flush_attempted);
    > + debugfs_create_u64("reject_tmppage_fail", S_IRUGO,
    > + zswap_debugfs_root, &zswap_reject_tmppage_fail);
    > + debugfs_create_u64("reject_flush_fail", S_IRUGO,
    > + zswap_debugfs_root, &zswap_reject_flush_fail);
    > + debugfs_create_u64("reject_zsmalloc_fail", S_IRUGO,
    > + zswap_debugfs_root, &zswap_reject_zsmalloc_fail);
    > + debugfs_create_u64("reject_kmemcache_fail", S_IRUGO,
    > + zswap_debugfs_root, &zswap_reject_kmemcache_fail);
    > + debugfs_create_u64("reject_compress_poor", S_IRUGO,
    > + zswap_debugfs_root, &zswap_reject_compress_poor);
    > + debugfs_create_u64("flushed_pages", S_IRUGO,
    > + zswap_debugfs_root, &zswap_flushed_pages);
    > + debugfs_create_u64("duplicate_entry", S_IRUGO,
    > + zswap_debugfs_root, &zswap_duplicate_entry);
    > + debugfs_create_atomic_t("pool_pages", S_IRUGO,
    > + zswap_debugfs_root, &zswap_pool_pages);
    > + debugfs_create_atomic_t("stored_pages", S_IRUGO,
    > + zswap_debugfs_root, &zswap_stored_pages);
    > + debugfs_create_atomic_t("outstanding_flushes", S_IRUGO,
    > + zswap_debugfs_root, &zswap_outstanding_flushes);
    > +

    Some of these statistics would be very useful to system
    administrators, who will not be mounting debugfs on
    production systems.

    Would it make sense to export some of these statistics
    through sysfs?

    --
    All rights reversed


    \
     
     \ /
      Last update: 2013-01-26 06:42    [W:2.816 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site