From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752965AbbKXENw (ORCPT ); Mon, 23 Nov 2015 23:13:52 -0500 Received: from mail-pa0-f49.google.com ([209.85.220.49]:35247 "EHLO mail-pa0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751203AbbKXENv (ORCPT ); Mon, 23 Nov 2015 23:13:51 -0500 Date: Tue, 24 Nov 2015 13:13:41 +0900 From: Minchan Kim To: Sergey Senozhatsky Cc: Andrew Morton , Kyeongdon Kim , Sergey Senozhatsky , linux-kernel@vger.kernel.org Subject: Re: [PATCH] zram/zcomp: use GFP_NOIO to allocate streams Message-ID: <20151124041341.GB11115@blaptop> References: <1448285279-4013-1-git-send-email-sergey.senozhatsky@gmail.com> <20151123151812.a335f9a52abd74d7017ecd85@linux-foundation.org> <20151124003027.GA705@swordfish> <20151123164740.495e5067d9d45a7f98b5ace3@linux-foundation.org> <20151124012927.GC705@swordfish> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151124012927.GC705@swordfish> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Sergey, On Tue, Nov 24, 2015 at 10:29:27AM +0900, Sergey Senozhatsky wrote: > Cc Kyeongdon > > On (11/23/15 16:47), Andrew Morton wrote: > [..] > > > > Doesn't make a lot of sense to me. We use a weakened gfp for the > > kmalloc and if that fails, fall into vmalloc() using the stronger gfp > > anyway. > > Sir, you are right. that was "fixed" in my patch (but I definitely should > have been more attentive when I reviewed Kyeongdon's patch and that was > a mistake to address this in my patch) > > I didn't spot it until I replaced vzalloc() with __vmalloc() working on > my GFP_NOIO patch: > > + return __vmalloc(LZ4_MEM_COMPRESS, > + GFP_NOIO | __GFP_NOWARN | __GFP_HIGHMEM | __GFP_ZERO, > + PAGE_KERNEL); > > > but I agree that we have created a mess already. > > > Perhaps it makes sense for higher-order allocations: we don't want to > > thrash around trying to create an order-2 page - we'd prefer to give up > > and fall into vmalloc to do a bunch of order-0 allocations. > > > > But this argument holds for 1000 other kmalloc->vmalloc allocation > > attempts - what's special about this one? > > > > And whatever is the reason for this peculiar setup, > > > > a) where's the proof that the change is actually beneficial? > > b) let's get a good code comment in place so that future readers are not > > similarly puzzled. > > or > > c) start anew (hopefully Minchan and Kyeongdon are with me) > > > Per Kyeongdon's comment > > :When we're using LZ4 multi compression streams for zram swap, > :we found out page allocation failure message in system running test. > :That was not only once, but a few(2 - 5 times per test). > :Also, some failure cases were continually occurring to try allocation > :order 3. > : > :In order to make parallel compression private data, we should call > :kzalloc() with order 2/3 in runtime(lzo/lz4). But if there is no order > : 2/3 size memory to allocate in that time, page allocation fails. > :This patch makes to use vmalloc() as fallback of kmalloc(), this > :prevents page alloc failure warning. > > > so (what I missed in the first place) is that the patch does not really > prevent page alloc failures warnings, because vmalloc() is still free to > warn us on every failed allocation. second, vmalloc() can fail and, thus, > we still will go down the 'do not attempt to allocate any memory anymore, > just wait for available stream to become idle'. > > > so my proposal > > patch 1: > a) switch to GFP_NOIO in critical parts (OR remove entirely remove the > 'dynamic' stream creation functionality. IOW, do not allocate compression > streams from IO path, wait for and use available streams). > > patch 2: > b) do not fallback to vmalloc (we are prepared to handle kmalloc failures) > c) add NOWARN to kmalloc (just to reduce the warning pressure) > > > well, (b) and (c), technically, can be merged with (a) but I have no > objections to have it as a separate patch. > > > > what do you guys think? First of all, Thanks for the summary and proposal. I think GFP_NOIO critical part(ie, your lockdep fix patch) should go to -stable so it should stand alone. About vmalloc, I like that. Just problem was gfp and we can pass it from upper layer so I believe it makes code looks clean and solve differnt gfp problem. Please look at my patchset I just sent. Thanks a lot!