linux-kernel - Re: [PATCH] zram/zcomp: use GFP

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]

Message-ID: <20151124041341.GB11115@blaptop>
Date:	Tue, 24 Nov 2015 13:13:41 +0900
From:	Minchan Kim <minchan@...nel.org>
To:	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Kyeongdon Kim <kyeongdon.kim@....com>,
	Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] zram/zcomp: use GFP_NOIO to allocate streams

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!
--
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/