by David Rientjes

[permalink] [raw]

Subject: Re: [PATCH 2/7] mm: introduce kvmalloc and kvmalloc_node

On Wed, 15 Jul 2015, Dave Chinner wrote:

> > Sure, but it's not accomplishing the same thing: things like
> > ext4_kvmalloc() only want to fallback to vmalloc() when high-order
> > allocations fail: the function is used for different sizes. This cannot
> > be converted to kvmalloc_node() since it fallsback immediately when
> > reclaim fails. Same issue with single_file_open() for the seq_file code.
> > We could go through every kmalloc() -> vmalloc() fallback for more
> > examples in the code, but those two instances were the first I looked at
> > and couldn't be converted to kvmalloc_node() without work.
> >
> > > It is always easier to shoehorn utility functions locally within a
> > > subsystem (be it ext4, dm, etc) but once enough do something in a
> > > similar but different way it really should get elevated.
> > >
> >
> > I would argue that
> >
> > void *ext4_kvmalloc(size_t size, gfp_t flags)
> > {
> > void *ret;
> >
> > ret = kmalloc(size, flags | __GFP_NOWARN);
> > if (!ret)
> > ret = __vmalloc(size, flags, PAGE_KERNEL);
> > return ret;
> > }
> >
> > is simple enough that we don't need to convert it to anything.
>
> Except that it will have problems with GFP_NOFS context when the pte
> code inside vmalloc does a GFP_KERNEL allocation. Hence we have
> stuff in other subsystems (such as XFS) where we've noticed lockdep
> whining about this:
>

Does anyone have an example of ext4_kvmalloc() having a lockdep violation?
Presumably the GFP_NOFS calls to ext4_kvmalloc() will never have
size > (1 << (PAGE_SHIFT + PAGE_ALLOC_COSTLY_ORDER)) so that kmalloc()
above actually never returns NULL and __vmalloc() only gets used for the
ext4_kvmalloc(..., GFP_KERNEL) call.

It should be fixed, though, probably in the same way as
kmem_zalloc_large() today, but it seems the real fix would be to attack
the whole vmalloc() GFP_KERNEL issue that has been talked about several
times in the past. Then the existing ext4_kvmalloc() implementation
should be fine.

Once that's done, we can revisit the idea of a generalized kvmalloc() or
kvmalloc_node(), but since the implementation such as above is different
from the proposed kvmalloc_node() implementation with respect to
high-order allocations, I doubt a generalized form will be helpful.

2015-07-15 00:25:56

by Dave Chinner

[permalink] [raw]

Subject: Re: [PATCH 2/7] mm: introduce kvmalloc and kvmalloc_node

On Tue, Jul 14, 2015 at 03:45:40PM -0700, David Rientjes wrote:
> On Wed, 15 Jul 2015, Dave Chinner wrote:
>
> > > Sure, but it's not accomplishing the same thing: things like
> > > ext4_kvmalloc() only want to fallback to vmalloc() when high-order
> > > allocations fail: the function is used for different sizes. This cannot
> > > be converted to kvmalloc_node() since it fallsback immediately when
> > > reclaim fails. Same issue with single_file_open() for the seq_file code.
> > > We could go through every kmalloc() -> vmalloc() fallback for more
> > > examples in the code, but those two instances were the first I looked at
> > > and couldn't be converted to kvmalloc_node() without work.
> > >
> > > > It is always easier to shoehorn utility functions locally within a
> > > > subsystem (be it ext4, dm, etc) but once enough do something in a
> > > > similar but different way it really should get elevated.
> > > >
> > >
> > > I would argue that
> > >
> > > void *ext4_kvmalloc(size_t size, gfp_t flags)
> > > {
> > > void *ret;
> > >
> > > ret = kmalloc(size, flags | __GFP_NOWARN);
> > > if (!ret)
> > > ret = __vmalloc(size, flags, PAGE_KERNEL);
> > > return ret;
> > > }
> > >
> > > is simple enough that we don't need to convert it to anything.
> >
> > Except that it will have problems with GFP_NOFS context when the pte
> > code inside vmalloc does a GFP_KERNEL allocation. Hence we have
> > stuff in other subsystems (such as XFS) where we've noticed lockdep
> > whining about this:
> >
>
> Does anyone have an example of ext4_kvmalloc() having a lockdep violation?
> Presumably the GFP_NOFS calls to ext4_kvmalloc() will never have
> size > (1 << (PAGE_SHIFT + PAGE_ALLOC_COSTLY_ORDER)) so that kmalloc()
> above actually never returns NULL and __vmalloc() only gets used for the
> ext4_kvmalloc(..., GFP_KERNEL) call.

Code inspection is all that is necessary. For example,
fs/ext4/resize.c::add_new_gdb() does:

818 n_group_desc = ext4_kvmalloc((gdb_num + 1) *
819 sizeof(struct buffer_head *),
820 GFP_NOFS);

I have to assume this was done because resize was failing kmalloc()
calls on large filesystems in GFP_NOFS context as the commit message
is less than helpful:

commit f18a5f21c25707b4fe64b326e2b4d150565e7300
Author: Theodore Ts'o <[email protected]>
Date: Mon Aug 1 08:45:38 2011 -0400

ext4: use ext4_kvzalloc()/ext4_kvmalloc() for s_group_desc and s_group_info

Signed-off-by: "Theodore Ts'o" <[email protected]>

> It should be fixed, though, probably in the same way as
> kmem_zalloc_large() today, but it seems the real fix would be to attack
> the whole vmalloc() GFP_KERNEL issue that has been talked about several
> times in the past. Then the existing ext4_kvmalloc() implementation
> should be fine.

Agreed, we really need to ensure that the generic kernel allocation
functions follow the context guidelines they are provided by
callers. I'm not going to hold my breathe waiting for this to
happen, though....

Cheers,

Dave.
--
Dave Chinner
[email protected]