Re: [PATCH] zram/zcomp: use GFP_NOIO to allocate streams

Minchan Kim
2015-11-24T04:13:52Z

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!