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: <20151124012927.GC705@swordfish>
Date:	Tue, 24 Nov 2015 10:29:27 +0900
From:	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To:	Andrew Morton <akpm@...ux-foundation.org>,
	Minchan Kim <minchan@...nel.org>,
	Kyeongdon Kim <kyeongdon.kim@....com>
Cc:	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
	Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] zram/zcomp: use GFP_NOIO to allocate streams

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?

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