From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753085AbaEBUB4 (ORCPT ); Fri, 2 May 2014 16:01:56 -0400 Received: from mail-oa0-f52.google.com ([209.85.219.52]:56287 "EHLO mail-oa0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752136AbaEBUBz (ORCPT ); Fri, 2 May 2014 16:01:55 -0400 Date: Fri, 2 May 2014 15:01:52 -0500 From: Seth Jennings To: Weijie Yang Cc: Dan Streetman , Minchan Kim , Nitin Gupta , Andrew Morton , Bob Liu , Hugh Dickins , Mel Gorman , Rik van Riel , Weijie Yang , Johannes Weiner , Sergey Senozhatsky , Linux-MM , linux-kernel Subject: Re: [PATCH 2/4] mm: zpool: implement zsmalloc shrinking Message-ID: <20140502200152.GA18670@cerebellum.variantweb.net> References: <1397922764-1512-1-git-send-email-ddstreet@ieee.org> <1397922764-1512-3-git-send-email-ddstreet@ieee.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, Apr 26, 2014 at 04:37:31PM +0800, Weijie Yang wrote: > On Sat, Apr 19, 2014 at 11:52 PM, Dan Streetman 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