Re: Lockdep complain for zram

[Minchan Kim <minchan@kernel.org>]

On Thu, Nov 22, 2012 at 02:08:43AM -0800, Nitin Gupta wrote:
> On 11/22/2012 12:31 AM, Minchan Kim wrote:
> >Hi Nitin,
> >
> >On Wed, Nov 21, 2012 at 10:06:33AM -0800, Nitin Gupta wrote:
> >>On 11/21/2012 12:37 AM, Minchan Kim wrote:
> >>>Hi alls,
> >>>
> >>>Today, I saw below complain of lockdep.
> >>>As a matter of fact, I knew it long time ago but forgot that.
> >>>The reason lockdep complains is that now zram uses GFP_KERNEL
> >>>in reclaim path(ex, __zram_make_request) :(
> >>>I can fix it via replacing GFP_KERNEL with GFP_NOIO.
> >>>But more big problem is vzalloc in zram_init_device which calls GFP_KERNEL.
> >>>Of course, I can change it with __vmalloc which can receive gfp_t.
> >>>But still we have a problem. Althoug __vmalloc can handle gfp_t, it calls
> >>>allocation of GFP_KERNEL. That's why I sent the patch.
> >>>https://lkml.org/lkml/2012/4/23/77
> >>>Since then, I forgot it, saw the bug today and poped the question again.
> >>>
> >>>Yes. Fundamental problem is utter crap API vmalloc.
> >>>If we can fix it, everyone would be happy. But life isn't simple like seeing
> >>>my thread of the patch.
> >>>
> >>>So next option is to move zram_init_device into setting disksize time.
> >>>But it makes unnecessary metadata waste until zram is used really(That's why
> >>>Nitin move zram_init_device from disksize setting time to make_request) and
> >>>it makes user should set the disksize before using, which are behavior change.
> >>>
> >>>I would like to clean up this issue before promoting because it might change
> >>>usage behavior.
> >>>
> >>>Do you have any idea?
> >>>
> >>
> >>Maybe we can alloc_vm_area() right on device creation in
> >>create_device() assuming the default disksize. If user explicitly
> >>sets the disksize, this vm area is deallocated and a new one is
> >>allocated based on the new disksize.  When the device is reset, we
> >>should only free physical pages allocated for the table and the
> >>virtual area should be set back as if disksize is set to the
> >>default.
> >>
> >>At the device init time, all the pages can be allocated with
> >>GFP_NOIO | __GPF_HIGHMEM and since the VM area is preallocated,
> >>map_vm_area() will not hit any of those page-table allocations with
> >>GFP_KERNEL.
> >>
> >>Other allocations made directly from zram, for instance in the
> >>partial I/O case, should also be changed to GFP_NOIO |
> >>__GFP_HIGHMEM.
> >>
> >
> >Yes. It's a good idea and actually I thought about it.
> >My concern about that approach is following as.
> >
> >1) User of zram normally do mkfs.xxx or mkswap before using
> >    the zram block device(ex, normally, do it at booting time)
> >    It ends up allocating such metadata of zram before real usage so
> >    benefit of lazy initialzation would be mitigated.
> >
> >2) Some user want to use zram when memory pressure is high.(ie, load zram
> >    dynamically, NOT booting time). It does make sense because people don't
> >    want to waste memory until memory pressure is high(ie, where zram is really
> >    helpful time). In this case, lazy initialzation could be failed easily
> >    because we will use GFP_NOIO instead of GFP_KERNEL due to swap use-case.
> >    So the benefit of lazy initialzation would be mitigated, too.
> >
> >3) Current zram's documenation is wrong.
> >    Set Disksize isn't optional when we use zram firstly.
> >    Once user set disksize, it could be optional, but NOT optional
> >    at first usage time. It's very odd behavior. So I think user set to disksizes
> >    before using is more safe and clear.
> >
> >So my suggestion is following as.
> >
> >  * Let's change disksize setting to MUST before using for consistent behavior.
> >  * When user set to disksize, let's allocate metadata all at once.
> >    4K : 12 byte(64bit) -> 64G : 192M so 0.3% isn't big overhead.
> >    If insane user use such big zram device up to 20, it could consume 6% of ram
> >    but efficieny of zram will cover the waste.
> >  * If someone has a concern about this, let's guide for him set to disksize
> >    right before zram using.
> >
> >What do you think about it?
> >
> 
> I agree. This lazy initialization has been a problem since the
> beginning. So, lets just force user to first set the disksize and
> document this behavior as such.

Sure.

> 
> I plan to reduce struct table to just an array of handles (so
> getting rid of size, flag, count fields), but for now we could just
> use existing struct table and do this change later.

Good to hear.

Thanks for the quick feedback.

-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>