linux-kernel - Re: [PATCH 2/4] mm: zpool: implement zsmalloc shrinking

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 2 May 2014 15:01:52 -0500
From:	Seth Jennings <sjennings@...iantweb.net>
To:	Weijie Yang <weijie.yang.kh@...il.com>
Cc:	Dan Streetman <ddstreet@...e.org>,
	Minchan Kim <minchan@...nel.org>,
	Nitin Gupta <ngupta@...are.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Bob Liu <bob.liu@...cle.com>, Hugh Dickins <hughd@...gle.com>,
	Mel Gorman <mgorman@...e.de>, Rik van Riel <riel@...hat.com>,
	Weijie Yang <weijie.yang@...sung.com>,
	Johannes Weiner <hannes@...xchg.org>,
	Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
	Linux-MM <linux-mm@...ck.org>,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/4] mm: zpool: implement zsmalloc shrinking

On Sat, Apr 26, 2014 at 04:37:31PM +0800, Weijie Yang wrote:
> On Sat, Apr 19, 2014 at 11:52 PM, Dan Streetman <ddstreet@...e.org> wrote:
> > Add zs_shrink() and helper functions to zsmalloc.  Update zsmalloc
> > zs_create_pool() creation function to include ops param that provides
> > an evict() function for use during shrinking.  Update helper function
> > fix_fullness_group() to always reinsert changed zspages even if the
> > fullness group did not change, so they are updated in the fullness
> > group lru.  Also update zram to use the new zsmalloc pool creation
> > function but pass NULL as the ops param, since zram does not use
> > pool shrinking.
> >
> 
> I only review the code without test, however, I think this patch is
> not acceptable.
> 
> The biggest problem is it will call zswap_writeback_entry() under lock,
> zswap_writeback_entry() may sleep, so it is a bug. see below
> 
> The 3/4 patch has a lot of #ifdef, I don't think it's a good kind of
> abstract way.
> 
> What about just disable zswap reclaim when using zsmalloc?

I agree here.  Making a generic allocator layer and zsmalloc reclaim
support should be two different efforts, since zsmalloc reclaim is
fraught with peril.

The generic layer can be done though, as long as you provide a way for
the backend to indicate that it doesn't support reclaim, which just
results in lru-inverse overflow to the swap device at the zswap layer.
Hopefully, if the user overrides the default to use zsmalloc, they
understand the implications and have sized their workload properly.

Also, the fallback logic shouldn't be in this generic layer.  It should
not be transparent to the user.  The user (in this case zswap) should
implement the fallback if they care to have it.  The generic allocator
layer makes it trivial for the user to implement.

Thanks,
Seth
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/