linux-kernel - Re: [PATCH 2/7] mm: introduce kvmalloc and kvmalloc

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: <alpine.DEB.2.10.1507141420350.16182@chino.kir.corp.google.com>
Date:	Tue, 14 Jul 2015 14:24:24 -0700 (PDT)
From:	David Rientjes <rientjes@...gle.com>
To:	Mike Snitzer <snitzer@...hat.com>
cc:	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, 14 Jul 2015, Mike Snitzer wrote:

> > > > > Index: linux-4.2-rc1/mm/util.c
> > > > > ===================================================================
> > > > > --- linux-4.2-rc1.orig/mm/util.c	2015-07-07 15:58:11.000000000 +0200
> > > > > +++ linux-4.2-rc1/mm/util.c	2015-07-08 19:22:26.000000000 +0200
> > > > > @@ -316,6 +316,61 @@ unsigned long vm_mmap(struct file *file,
> > > > >  }
> > > > >  EXPORT_SYMBOL(vm_mmap);
> > > > >  
> > > > > +void *kvmalloc_node(size_t size, gfp_t gfp, int node)
> > > > > +{
> > > > > +	void *p;
> > > > > +	unsigned uninitialized_var(noio_flag);
> > > > > +
> > > > > +	/* vmalloc doesn't support no-wait allocations */
> > > > > +	WARN_ON_ONCE(!(gfp & __GFP_WAIT));
> > > > > +
> > > > > +	if (likely(size <= KMALLOC_MAX_SIZE)) {
> > > > > +		/*
> > > > > +		 * Use __GFP_NORETRY so that we don't loop waiting for the
> > > > > +		 *	allocation - we don't have to loop here, if the memory
> > > > > +		 *	is too fragmented, we fallback to vmalloc.
> > > > 
> > > > I'm not sure about this decision.  The direct reclaim retry code is the
> > > > normal default behaviour and becomes more important with larger allocation
> > > > attempts.  So why turn it off, and make it more likely that we return
> > > > vmalloc memory?
> > > 
> > > It can avoid triggering the OOM killer in case of fragmented memory.
> > > 
> > > This is general question - if the code can handle allocation failure 
> > > gracefully, what gfp flags should it use? Maybe add some flag 
> > > __GFP_MAYFAIL instead of __GFP_NORETRY that changes the behavior in 
> > > desired way?
> > > 
> > 
> > There's a misunderstanding in regards to the comment: __GFP_NORETRY 
> > doesn't turn direct reclaim or compaction off, it is still attempted and 
> > with the same priority as any other allocation.  This only stops the page 
> > allocator from calling the oom killer, which will free memory or panic the 
> > system, and looping when memory is available.
> > 
> > In regards to the proposal in general, I think it's unnecessary because we 
> > are still left behind with other users who open code their call to 
> > vmalloc.  I was interested in commit 058504edd026 ("fs/seq_file: fallback 
> > to vmalloc allocation") since it solved an issue with high memory 
> > fragmentation.  Note how it falls back to vmalloc(): _without_ this 
> > __GFP_NORETRY.  That's because we only want to fallback when high-order 
> > allocations fail and the page allocator doesn't implicitly loop due to the 
> > order.  ext4_kvmalloc(), ext4_kzmalloc() does the same.
> > 
> > The differences in implementations between those that do kmalloc() and 
> > fallback to vmalloc() are different enough that I don't think we need this 
> > addition.
> 
> Wouldn't mm benefit from acknowledging the pattern people are
> open-coding and switching existing code over to official methods for
> accomplishing the same?
> 

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.

If all such fallback was done in the same way as the implementation as 
kvmalloc_node(), and perhaps only very few exceptions were needed, this 
would be helpful.  Unfortunately, that isn't the case.
--
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/