Date:	Wed, 15 Jul 2015 10:25:40 +1000
From:	Dave Chinner <david@...morbit.com>
To:	David Rientjes <rientjes@...gle.com>
Cc:	Mike Snitzer <snitzer@...hat.com>,
	Mikulas Patocka <mpatocka@...hat.com>,
	Edward Thornber <thornber@...hat.com>,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	dm-devel@...hat.com, Vivek Goyal <vgoyal@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	"Alasdair G. Kergon" <agk@...hat.com>
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 <tytso@....edu>
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" <tytso@....edu>


> 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
david@...morbit.com
--
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/

Powered by blists - more mailing lists