Linux-mm Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
From: Minchan Kim @ 2017-04-28  7:42 UTC (permalink / raw)
  To: Huang, Ying
  Cc: Andrew Morton, linux-mm, linux-kernel, Hugh Dickins, Shaohua Li,
	Rik van Riel
In-Reply-To: <87r30dz6am.fsf@yhuang-dev.intel.com>

On Fri, Apr 28, 2017 at 09:09:53AM +0800, Huang, Ying wrote:
> Minchan Kim <minchan@kernel.org> writes:
> 
> > On Wed, Apr 26, 2017 at 08:42:10PM +0800, Huang, Ying wrote:
> >> Minchan Kim <minchan@kernel.org> writes:
> >> 
> >> > On Fri, Apr 21, 2017 at 08:29:30PM +0800, Huang, Ying wrote:
> >> >> "Huang, Ying" <ying.huang@intel.com> writes:
> >> >> 
> >> >> > Minchan Kim <minchan@kernel.org> writes:
> >> >> >
> >> >> >> On Wed, Apr 19, 2017 at 04:14:43PM +0800, Huang, Ying wrote:
> >> >> >>> Minchan Kim <minchan@kernel.org> writes:
> >> >> >>> 
> >> >> >>> > Hi Huang,
> >> >> >>> >
> >> >> >>> > On Fri, Apr 07, 2017 at 02:49:01PM +0800, Huang, Ying wrote:
> >> >> >>> >> From: Huang Ying <ying.huang@intel.com>
> >> >> >>> >> 
> >> >> >>> >>  void swapcache_free_entries(swp_entry_t *entries, int n)
> >> >> >>> >>  {
> >> >> >>> >>  	struct swap_info_struct *p, *prev;
> >> >> >>> >> @@ -1075,6 +1083,10 @@ void swapcache_free_entries(swp_entry_t *entries, int n)
> >> >> >>> >>  
> >> >> >>> >>  	prev = NULL;
> >> >> >>> >>  	p = NULL;
> >> >> >>> >> +
> >> >> >>> >> +	/* Sort swap entries by swap device, so each lock is only taken once. */
> >> >> >>> >> +	if (nr_swapfiles > 1)
> >> >> >>> >> +		sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
> >> >> >>> >
> >> >> >>> > Let's think on other cases.
> >> >> >>> >
> >> >> >>> > There are two swaps and they are configured by priority so a swap's usage
> >> >> >>> > would be zero unless other swap used up. In case of that, this sorting
> >> >> >>> > is pointless.
> >> >> >>> >
> >> >> >>> > As well, nr_swapfiles is never decreased so if we enable multiple
> >> >> >>> > swaps and then disable until a swap is remained, this sorting is
> >> >> >>> > pointelss, too.
> >> >> >>> >
> >> >> >>> > How about lazy sorting approach? IOW, if we found prev != p and,
> >> >> >>> > then we can sort it.
> >> >> >>> 
> >> >> >>> Yes.  That should be better.  I just don't know whether the added
> >> >> >>> complexity is necessary, given the array is short and sort is fast.
> >> >> >>
> >> >> >> Huh?
> >> >> >>
> >> >> >> 1. swapon /dev/XXX1
> >> >> >> 2. swapon /dev/XXX2
> >> >> >> 3. swapoff /dev/XXX2
> >> >> >> 4. use only one swap
> >> >> >> 5. then, always pointless sort.
> >> >> >
> >> >> > Yes.  In this situation we will do unnecessary sorting.  What I don't
> >> >> > know is whether the unnecessary sorting will hurt performance in real
> >> >> > life.  I can do some measurement.
> >> >> 
> >> >> I tested the patch with 1 swap device and 1 process to eat memory
> >> >> (remove the "if (nr_swapfiles > 1)" for test).  I think this is the
> >> >> worse case because there is no lock contention.  The memory freeing time
> >> >> increased from 1.94s to 2.12s (increase ~9.2%).  So there is some
> >> >> overhead for some cases.  I change the algorithm to something like
> >> >> below,
> >> >> 
> >> >>  void swapcache_free_entries(swp_entry_t *entries, int n)
> >> >>  {
> >> >>  	struct swap_info_struct *p, *prev;
> >> >>  	int i;
> >> >> +	swp_entry_t entry;
> >> >> +	unsigned int prev_swp_type;
> >> >>  
> >> >>  	if (n <= 0)
> >> >>  		return;
> >> >>  
> >> >> +	prev_swp_type = swp_type(entries[0]);
> >> >> +	for (i = n - 1; i > 0; i--) {
> >> >> +		if (swp_type(entries[i]) != prev_swp_type)
> >> >> +			break;
> >> >> +	}
> >> >
> >> > That's really what I want to avoid. For many swap usecases,
> >> > it adds unnecessary overhead.
> >> >
> >> >> +
> >> >> +	/* Sort swap entries by swap device, so each lock is only taken once. */
> >> >> +	if (i)
> >> >> +		sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
> >> >>  	prev = NULL;
> >> >>  	p = NULL;
> >> >>  	for (i = 0; i < n; ++i) {
> >> >> -		p = swap_info_get_cont(entries[i], prev);
> >> >> +		entry = entries[i];
> >> >> +		p = swap_info_get_cont(entry, prev);
> >> >>  		if (p)
> >> >> -			swap_entry_free(p, entries[i]);
> >> >> +			swap_entry_free(p, entry);
> >> >>  		prev = p;
> >> >>  	}
> >> >>  	if (p)
> >> >> 
> >> >> With this patch, the memory freeing time increased from 1.94s to 1.97s.
> >> >> I think this is good enough.  Do you think so?
> >> >
> >> > What I mean is as follows(I didn't test it at all):
> >> >
> >> > With this, sort entries if we found multiple entries in current
> >> > entries. It adds some condition checks for non-multiple swap
> >> > usecase but it would be more cheaper than the sorting.
> >> > And it adds a [un]lock overhead for multiple swap usecase but
> >> > it should be a compromise for single-swap usecase which is more
> >> > popular.
> >> >
> >> 
> >> How about the following solution?  It can avoid [un]lock overhead and
> >> double lock issue for multiple swap user case and has good performance
> >> for one swap user case too.
> >
> > How worse with approach I suggested compared to as-is?
> 
> The performance difference between your version and my version is small
> for my testing.

If so, why should we add code to optimize further?

> 
> > Unless it's too bad, let's not add more complicated thing to just
> > enhance the minor usecase in such even *slow* path.
> > It adds code size/maintainance overead.
> > With your suggestion, it might enhance a bit with speicific benchmark
> > but not sure it's really worth for real practice.
> 
> I don't think the code complexity has much difference between our latest
> versions.  As for complexity, I think my original version which just

What I suggested is to avoid pointless overhead for *major* usecase
and the code you are adding now is to optimize further for *minor*
usecase. And now I dobut the code you are adding is really worth
unless it makes a meaningful output.
If it doesn't, it adds just overhead(code size, maintainance, power and
performance). You might argue it's really *small* so it would be okay
but think about that you would be not only one in the community so
kernel bloats day by day with code to handle corner cases.

> uses nr_swapfiles to avoid sort() for single swap device is simple and
> good enough for this task.  Maybe we can just improve the correctness of

But it hurts *major* usecase.

> swap device counting as Tim suggested.

I don't know what Tim suggested. Anyway, my point is that minor
usecase doesn't hurt major usecase and justify the benefit
if you want to put more. So I'm okay with either solution to
meet it.

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

^ permalink raw reply

* Re: [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks
From: Michal Hocko @ 2017-04-28  7:40 UTC (permalink / raw)
  To: Igor Stoppa; +Cc: namhyung, linux-mm, linux-kernel
In-Reply-To: <f741d053-4303-5441-21bc-ec86bca1164c@huawei.com>

On Thu 27-04-17 17:06:05, Igor Stoppa wrote:
> 
> 
> On 27/04/17 16:41, Michal Hocko wrote:
> > On Wed 26-04-17 18:29:08, Igor Stoppa wrote:
> > [...]
> >> If you prefer to have this patch only as part of the larger patchset,
> >> I'm also fine with it.
> > 
> > I agree that the situation is not ideal. If a larger set of changes
> > would benefit from this change then it would clearly add arguments...
> 
> Ok, then I'll send it out as part of the larger RFC set.
> 
> 
> >> Also, if you could reply to [1], that would be greatly appreciated.
> > 
> > I will try to get to it but from a quick glance, yet-another-zone will
> > hit a lot of opposition...
> 
> The most basic questions, that I hope can be answered with Yes/No =) are:
> 
> - should a new zone be added after DMA32?
> 
> - should I try hard to keep the mask fitting a 32bit word - at least for
> hose who do not use the new zone - or is it ok to just stretch it to 64
> bits?

Do not add a new zone, really. What you seem to be looking for is an
allocator on top of the page/memblock allocator which does write
protection on top. I understand that you would like to avoid object
management duplication but I am not really sure how much you can re-use
what slab allocators do already, anyway. I will respond to the original
thread to not mix things together.
-- 
Michal Hocko
SUSE Labs

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

^ permalink raw reply

* Re: [PATCH v2 1/2] mm: Uncharge poisoned pages
From: Michal Hocko @ 2017-04-28  7:31 UTC (permalink / raw)
  To: Laurent Dufour, Andi Kleen
  Cc: Naoya Horiguchi, linux-kernel, linux-mm, akpm, Johannes Weiner,
	Vladimir Davydov
In-Reply-To: <20170428060755.GA8143@dhcp22.suse.cz>

[CC Johannes and Vladimir - the patch is
http://lkml.kernel.org/r/1493130472-22843-2-git-send-email-ldufour@linux.vnet.ibm.com]

On Fri 28-04-17 08:07:55, Michal Hocko wrote:
> On Thu 27-04-17 13:51:23, Andi Kleen wrote:
> > Michal Hocko <mhocko@kernel.org> writes:
> > 
> > > On Tue 25-04-17 16:27:51, Laurent Dufour wrote:
> > >> When page are poisoned, they should be uncharged from the root memory
> > >> cgroup.
> > >> 
> > >> This is required to avoid a BUG raised when the page is onlined back:
> > >> BUG: Bad page state in process mem-on-off-test  pfn:7ae3b
> > >> page:f000000001eb8ec0 count:0 mapcount:0 mapping:          (null)
> > >> index:0x1
> > >> flags: 0x3ffff800200000(hwpoison)
> > >
> > > My knowledge of memory poisoning is very rudimentary but aren't those
> > > pages supposed to leak and never come back? In other words isn't the
> > > hoplug code broken because it should leave them alone?
> > 
> > Yes that would be the right interpretation. If it was really offlined
> > due to a hardware error the memory will be poisoned and any access
> > could cause a machine check.
> 
> OK, thanks for the clarification. Then I am not sure the patch is
> correct. Why do we need to uncharge that page at all?

Now, I have realized that we actually want to uncharge that page because
it will pin the memcg and we do not want to have that memcg and its
whole hierarchy pinned as well. This used to work before the charge
rework 0a31bc97c80c ("mm: memcontrol: rewrite uncharge API") I guess
because we used to uncharge on page cache removal.

I do not think the patch is correct, though. memcg_kmem_enabled() will
check whether kmem accounting is enabled and we are talking about page
cache pages here. You should be using mem_cgroup_uncharge instead.
-- 
Michal Hocko
SUSE Labs

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

^ permalink raw reply

* Re: [PATCH v2 2/2] mm: skip HWPoisoned pages when onlining pages
From: Michal Hocko @ 2017-04-28  6:51 UTC (permalink / raw)
  To: Naoya Horiguchi
  Cc: Balbir Singh, Laurent Dufour, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org, akpm@linux-foundation.org
In-Reply-To: <20170428065050.GC8143@dhcp22.suse.cz>

On Fri 28-04-17 08:50:50, Michal Hocko wrote:
> [Drop Wen Congyang because his address bounces - we will have to find
> out ourselves...]

Ble, for real this time. Sorry about spamming
 
> On Fri 28-04-17 08:30:48, Michal Hocko wrote:
> > On Wed 26-04-17 03:13:04, Naoya Horiguchi wrote:
> > > On Wed, Apr 26, 2017 at 12:10:15PM +1000, Balbir Singh wrote:
> > > > On Tue, 2017-04-25 at 16:27 +0200, Laurent Dufour wrote:
> > > > > The commit b023f46813cd ("memory-hotplug: skip HWPoisoned page when
> > > > > offlining pages") skip the HWPoisoned pages when offlining pages, but
> > > > > this should be skipped when onlining the pages too.
> > > > >
> > > > > Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> > > > > ---
> > > > >  mm/memory_hotplug.c | 4 ++++
> > > > >  1 file changed, 4 insertions(+)
> > > > >
> > > > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > > > > index 6fa7208bcd56..741ddb50e7d2 100644
> > > > > --- a/mm/memory_hotplug.c
> > > > > +++ b/mm/memory_hotplug.c
> > > > > @@ -942,6 +942,10 @@ static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages,
> > > > >  	if (PageReserved(pfn_to_page(start_pfn)))
> > > > >  		for (i = 0; i < nr_pages; i++) {
> > > > >  			page = pfn_to_page(start_pfn + i);
> > > > > +			if (PageHWPoison(page)) {
> > > > > +				ClearPageReserved(page);
> > > >
> > > > Why do we clear page reserved? Also if the page is marked PageHWPoison, it
> > > > was never offlined to begin with? Or do you expect this to be set on newly
> > > > hotplugged memory? Also don't we need to skip the entire pageblock?
> > > 
> > > If I read correctly, to "skip HWPoiosned page" in commit b023f46813cd means
> > > that we skip the page status check for hwpoisoned pages *not* to prevent
> > > memory offlining for memblocks with hwpoisoned pages. That means that
> > > hwpoisoned pages can be offlined.
> > 
> > Is this patch actually correct? I am trying to wrap my head around it
> > but it smells like it tries to avoid the problem rather than fix it
> > properly. I might be wrong here of course but to me it sounds like
> > poisoned page should simply be offlined and keep its poison state all
> > the time. If the memory is hot-removed and added again we have lost the
> > struct page along with the state which is the expected behavior. If it
> > is still broken we will re-poison it.
> > 
> > Anyway a patch to skip over poisoned pages during online makes perfect
> > sense to me. The PageReserved fiddling around much less so.
> > 
> > Or am I missing something. Let's CC Wen Congyang for the clarification
> > here.
> 
> -- 
> Michal Hocko
> SUSE Labs

-- 
Michal Hocko
SUSE Labs

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

^ permalink raw reply

* Re: [PATCH v2 2/2] mm: skip HWPoisoned pages when onlining pages
From: Michal Hocko @ 2017-04-28  6:50 UTC (permalink / raw)
  To: Naoya Horiguchi
  Cc: Balbir Singh, Laurent Dufour, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org, akpm@linux-foundation.org, Wen Congyang
In-Reply-To: <20170428063048.GA9399@dhcp22.suse.cz>

[Drop Wen Congyang because his address bounces - we will have to find
out ourselves...]

On Fri 28-04-17 08:30:48, Michal Hocko wrote:
> On Wed 26-04-17 03:13:04, Naoya Horiguchi wrote:
> > On Wed, Apr 26, 2017 at 12:10:15PM +1000, Balbir Singh wrote:
> > > On Tue, 2017-04-25 at 16:27 +0200, Laurent Dufour wrote:
> > > > The commit b023f46813cd ("memory-hotplug: skip HWPoisoned page when
> > > > offlining pages") skip the HWPoisoned pages when offlining pages, but
> > > > this should be skipped when onlining the pages too.
> > > >
> > > > Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> > > > ---
> > > >  mm/memory_hotplug.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > > > index 6fa7208bcd56..741ddb50e7d2 100644
> > > > --- a/mm/memory_hotplug.c
> > > > +++ b/mm/memory_hotplug.c
> > > > @@ -942,6 +942,10 @@ static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages,
> > > >  	if (PageReserved(pfn_to_page(start_pfn)))
> > > >  		for (i = 0; i < nr_pages; i++) {
> > > >  			page = pfn_to_page(start_pfn + i);
> > > > +			if (PageHWPoison(page)) {
> > > > +				ClearPageReserved(page);
> > >
> > > Why do we clear page reserved? Also if the page is marked PageHWPoison, it
> > > was never offlined to begin with? Or do you expect this to be set on newly
> > > hotplugged memory? Also don't we need to skip the entire pageblock?
> > 
> > If I read correctly, to "skip HWPoiosned page" in commit b023f46813cd means
> > that we skip the page status check for hwpoisoned pages *not* to prevent
> > memory offlining for memblocks with hwpoisoned pages. That means that
> > hwpoisoned pages can be offlined.
> 
> Is this patch actually correct? I am trying to wrap my head around it
> but it smells like it tries to avoid the problem rather than fix it
> properly. I might be wrong here of course but to me it sounds like
> poisoned page should simply be offlined and keep its poison state all
> the time. If the memory is hot-removed and added again we have lost the
> struct page along with the state which is the expected behavior. If it
> is still broken we will re-poison it.
> 
> Anyway a patch to skip over poisoned pages during online makes perfect
> sense to me. The PageReserved fiddling around much less so.
> 
> Or am I missing something. Let's CC Wen Congyang for the clarification
> here.

-- 
Michal Hocko
SUSE Labs

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

^ permalink raw reply

* Re: [PATCH] mm, zone_device: replace {get, put}_zone_device_page() with a single reference
From: Ingo Molnar @ 2017-04-28  6:39 UTC (permalink / raw)
  To: Kirill A. Shutemov
  Cc: Dan Williams, Ingo Molnar, akpm, linux-mm,
	Jérôme Glisse, Logan Gunthorpe, linux-kernel,
	Kirill Shutemov
In-Reply-To: <20170427083317.vzfiw7up63draslc@node.shutemov.name>


* Kirill A. Shutemov <kirill@shutemov.name> wrote:

> On Wed, Apr 26, 2017 at 05:55:31PM -0700, Dan Williams wrote:
> > Kirill points out that the calls to {get,put}_dev_pagemap() can be
> > removed from the mm fast path if we take a single get_dev_pagemap()
> > reference to signify that the page is alive and use the final put of the
> > page to drop that reference.
> > 
> > This does require some care to make sure that any waits for the
> > percpu_ref to drop to zero occur *after* devm_memremap_page_release(),
> > since it now maintains its own elevated reference.
> > 
> > Cc Ingo Molnar <mingo@redhat.com>
> > Cc: Jerome Glisse <jglisse@redhat.com>
> > Cc: Logan Gunthorpe <logang@deltatee.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Suggested-by: Kirill Shutemov <kirill.shutemov@linux.intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> > 
> > This patch might fix the regression that we found with the conversion to
> > generic get_user_pages_fast() in the x86/mm branch pending for 4.12
> > (commit 2947ba054a4d "x86/mm/gup: Switch GUP to the generic
> > get_user_page_fast() implementation"). I'll test tomorrow, but in case
> > someone can give it a try before I wake up, here's an early version.
> 
> + Ingo.
> 
> This works for me with and without GUP revert.
> 
> Tested-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> 
> >  drivers/dax/pmem.c    |    2 +-
> >  drivers/nvdimm/pmem.c |   13 +++++++++++--
> 
> There's a trivial conflict in drivers/nvdimm/pmem.c when applied to
> tip/master.

Ok, could someone please send a version either to Linus for v4.11, or a version 
against latest -tip so I can included it in x86/mm, so that x86/mm gets unbroken.

Thanks,

	Ingo

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

^ permalink raw reply

* Re: [PATCH v2 2/2] mm: skip HWPoisoned pages when onlining pages
From: Michal Hocko @ 2017-04-28  6:30 UTC (permalink / raw)
  To: Naoya Horiguchi
  Cc: Balbir Singh, Laurent Dufour, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org, akpm@linux-foundation.org, Wen Congyang
In-Reply-To: <20170426031255.GB11619@hori1.linux.bs1.fc.nec.co.jp>

On Wed 26-04-17 03:13:04, Naoya Horiguchi wrote:
> On Wed, Apr 26, 2017 at 12:10:15PM +1000, Balbir Singh wrote:
> > On Tue, 2017-04-25 at 16:27 +0200, Laurent Dufour wrote:
> > > The commit b023f46813cd ("memory-hotplug: skip HWPoisoned page when
> > > offlining pages") skip the HWPoisoned pages when offlining pages, but
> > > this should be skipped when onlining the pages too.
> > >
> > > Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> > > ---
> > >  mm/memory_hotplug.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > > index 6fa7208bcd56..741ddb50e7d2 100644
> > > --- a/mm/memory_hotplug.c
> > > +++ b/mm/memory_hotplug.c
> > > @@ -942,6 +942,10 @@ static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages,
> > >  	if (PageReserved(pfn_to_page(start_pfn)))
> > >  		for (i = 0; i < nr_pages; i++) {
> > >  			page = pfn_to_page(start_pfn + i);
> > > +			if (PageHWPoison(page)) {
> > > +				ClearPageReserved(page);
> >
> > Why do we clear page reserved? Also if the page is marked PageHWPoison, it
> > was never offlined to begin with? Or do you expect this to be set on newly
> > hotplugged memory? Also don't we need to skip the entire pageblock?
> 
> If I read correctly, to "skip HWPoiosned page" in commit b023f46813cd means
> that we skip the page status check for hwpoisoned pages *not* to prevent
> memory offlining for memblocks with hwpoisoned pages. That means that
> hwpoisoned pages can be offlined.

Is this patch actually correct? I am trying to wrap my head around it
but it smells like it tries to avoid the problem rather than fix it
properly. I might be wrong here of course but to me it sounds like
poisoned page should simply be offlined and keep its poison state all
the time. If the memory is hot-removed and added again we have lost the
struct page along with the state which is the expected behavior. If it
is still broken we will re-poison it.

Anyway a patch to skip over poisoned pages during online makes perfect
sense to me. The PageReserved fiddling around much less so.

Or am I missing something. Let's CC Wen Congyang for the clarification
here.
-- 
Michal Hocko
SUSE Labs

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

^ permalink raw reply

* Re: [PATCH] mm: Add additional consistency check
From: Michal Hocko @ 2017-04-28  6:16 UTC (permalink / raw)
  To: Kees Cook
  Cc: Christoph Lameter, Andrew Morton, Pekka Enberg, David Rientjes,
	Joonsoo Kim, Linux-MM, LKML
In-Reply-To: <CAGXu5j+vVn02Vsx5TzWPz3MS7Jow1gi+m3ojwMXrL-w6aaZhtw@mail.gmail.com>

On Thu 27-04-17 18:11:28, Kees Cook wrote:
> On Tue, Apr 11, 2017 at 7:19 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > I would do something like...
> > ---
> > diff --git a/mm/slab.c b/mm/slab.c
> > index bd63450a9b16..87c99a5e9e18 100644
> > --- a/mm/slab.c
> > +++ b/mm/slab.c
> > @@ -393,10 +393,15 @@ static inline void set_store_user_dirty(struct kmem_cache *cachep) {}
> >  static int slab_max_order = SLAB_MAX_ORDER_LO;
> >  static bool slab_max_order_set __initdata;
> >
> > +static inline struct kmem_cache *page_to_cache(struct page *page)
> > +{
> > +       return page->slab_cache;
> > +}
> > +
> >  static inline struct kmem_cache *virt_to_cache(const void *obj)
> >  {
> >         struct page *page = virt_to_head_page(obj);
> > -       return page->slab_cache;
> > +       return page_to_cache(page);
> >  }
> >
> >  static inline void *index_to_obj(struct kmem_cache *cache, struct page *page,
> > @@ -3813,14 +3818,18 @@ void kfree(const void *objp)
> >  {
> >         struct kmem_cache *c;
> >         unsigned long flags;
> > +       struct page *page;
> >
> >         trace_kfree(_RET_IP_, objp);
> >
> >         if (unlikely(ZERO_OR_NULL_PTR(objp)))
> >                 return;
> > +       page = virt_to_head_page(obj);
> > +       if (CHECK_DATA_CORRUPTION(!PageSlab(page)))
> > +               return;
> >         local_irq_save(flags);
> >         kfree_debugcheck(objp);
> > -       c = virt_to_cache(objp);
> > +       c = page_to_cache(page);
> >         debug_check_no_locks_freed(objp, c->object_size);
> >
> >         debug_check_no_obj_freed(objp, c->object_size);
> 
> Sorry for the delay, I've finally had time to look at this again.
> 
> So, this only handles the kfree() case, not the kmem_cache_free() nor
> kmem_cache_free_bulk() cases, so it misses all the non-kmalloc
> allocations (and kfree() ultimately calls down to kmem_cache_free()).
> Similarly, my proposed patch missed the kfree() path. :P

yes

> As I work on a replacement, is the goal to avoid the checks while
> under local_irq_save()? (i.e. I can't just put the check in
> virt_to_cache(), etc.)

You would have to check all callers of virt_to_cache. I would simply
replace BUG_ON(!PageSlab()) in cache_from_obj. kmem_cache_free already
handles NULL cache. kmem_cache_free_bulk and build_detached_freelist can
be made to do so.

-- 
Michal Hocko
SUSE Labs

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

^ permalink raw reply

* Re: [PATCH v2 1/2] mm: Uncharge poisoned pages
From: Michal Hocko @ 2017-04-28  6:07 UTC (permalink / raw)
  To: Laurent Dufour, Andi Kleen; +Cc: Naoya Horiguchi, linux-kernel, linux-mm, akpm
In-Reply-To: <87pofxk20k.fsf@firstfloor.org>

On Thu 27-04-17 13:51:23, Andi Kleen wrote:
> Michal Hocko <mhocko@kernel.org> writes:
> 
> > On Tue 25-04-17 16:27:51, Laurent Dufour wrote:
> >> When page are poisoned, they should be uncharged from the root memory
> >> cgroup.
> >> 
> >> This is required to avoid a BUG raised when the page is onlined back:
> >> BUG: Bad page state in process mem-on-off-test  pfn:7ae3b
> >> page:f000000001eb8ec0 count:0 mapcount:0 mapping:          (null)
> >> index:0x1
> >> flags: 0x3ffff800200000(hwpoison)
> >
> > My knowledge of memory poisoning is very rudimentary but aren't those
> > pages supposed to leak and never come back? In other words isn't the
> > hoplug code broken because it should leave them alone?
> 
> Yes that would be the right interpretation. If it was really offlined
> due to a hardware error the memory will be poisoned and any access
> could cause a machine check.

OK, thanks for the clarification. Then I am not sure the patch is
correct. Why do we need to uncharge that page at all? It is not freed.
The correct thing to do is to not online it in the first place which is
done in patch2 [1]. Even if we need to uncharge the page the reason is
not to silent the BUG, that is merely papering a issue than a real fix.
Laurent can you elaborate please.

[1] http://lkml.kernel.org/r/1493130472-22843-3-git-send-email-ldufour@linux.vnet.ibm.com
-- 
Michal Hocko
SUSE Labs

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

^ permalink raw reply

* Re: [PATCH v5 31/32] x86: Add sysfs support for Secure Memory Encryption
From: Dave Young @ 2017-04-28  5:32 UTC (permalink / raw)
  To: Dave Hansen
  Cc: Tom Lendacky, linux-arch, linux-efi, kvm, linux-doc, x86, kexec,
	linux-kernel, kasan-dev, linux-mm, iommu, Thomas Gleixner,
	Rik van Riel, Brijesh Singh, Toshimitsu Kani, Arnd Bergmann,
	Jonathan Corbet, Matt Fleming, Joerg Roedel,
	Radim Krčmář, Konrad Rzeszutek Wilk,
	Andrey Ryabinin, Ingo Molnar, Michael S. Tsirkin, Andy Lutomirski,
	H. Peter Anvin, Borislav Petkov, Paolo Bonzini,
	Alexander Potapenko, Larry Woodman, Dmitry Vyukov
In-Reply-To: <1f034974-20e6-b5e9-e6ff-434b634e1522@intel.com>

On 04/27/17 at 08:52am, Dave Hansen wrote:
> On 04/27/2017 12:25 AM, Dave Young wrote:
> > On 04/21/17 at 02:55pm, Dave Hansen wrote:
> >> On 04/18/2017 02:22 PM, Tom Lendacky wrote:
> >>> Add sysfs support for SME so that user-space utilities (kdump, etc.) can
> >>> determine if SME is active.
> >>>
> >>> A new directory will be created:
> >>>   /sys/kernel/mm/sme/
> >>>
> >>> And two entries within the new directory:
> >>>   /sys/kernel/mm/sme/active
> >>>   /sys/kernel/mm/sme/encryption_mask
> >>
> >> Why do they care, and what will they be doing with this information?
> > 
> > Since kdump will copy old memory but need this to know if the old memory
> > was encrypted or not. With this sysfs file we can know the previous SME
> > status and pass to kdump kernel as like a kernel param.
> > 
> > Tom, have you got chance to try if it works or not?
> 
> What will the kdump kernel do with it though?  We kexec() into that
> kernel so the SME keys will all be the same, right?  So, will the kdump
> kernel be just setting the encryption bit in the PTE so it can copy the
> old plaintext out?

I assume it is for active -> non active case, the new boot need to know
the old memory is encrypted. But I think I did not read all the patches
I may miss things.

> 
> Why do we need both 'active' and 'encryption_mask'?  How could it be
> that the hardware-enumerated 'encryption_mask' changes across a kexec()?

Leave this question to Tom..

Thanks
Dave

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

^ permalink raw reply

* Re: [RFC 1/4] mm: create N_COHERENT_MEMORY
From: Balbir Singh @ 2017-04-28  5:07 UTC (permalink / raw)
  To: Reza Arbab
  Cc: linux-mm, akpm, khandual, benh, aneesh.kumar, paulmck, srikar,
	haren, jglisse, mgorman, mhocko, vbabka, cl
In-Reply-To: <20170427184213.tco7hu5w2zlm4lpg@arbab-laptop.localdomain>

On Thu, 2017-04-27 at 13:42 -0500, Reza Arbab wrote:
> On Wed, Apr 19, 2017 at 05:52:39PM +1000, Balbir Singh wrote:
> > In this patch we create N_COHERENT_MEMORY, which is different
> > from N_MEMORY. A node hotplugged as coherent memory will have
> > this state set. The expectation then is that this memory gets
> > onlined like regular nodes. Memory allocation from such nodes
> > occurs only when the the node is contained explicitly in the
> > mask.
> 
> Finally got around to test drive this. From what I can see, as expected,
> both kernel and userspace seem to ignore these nodes, unless you 
> allocate specifically from them. Very convenient.

Thanks for testing them!

> 
> Is "online_coherent"/MMOP_ONLINE_COHERENT the right way to trigger this?  

Now that we mark the node state at boot/hotplug time, I think we can ignore
these changes.

> That mechanism is used to specify zone, and only for a single block of 
> memory. This concept applies to the node as a whole. I think it should 
> be independent of memory onlining.
> 
> I mean, let's say online_kernel N blocks, some of them get allocated, 
> and then you online_coherent block N+1, flipping the entire node into 
> N_COHERENT_MEMORY. That doesn't seem right.
> 

Agreed, I'll remove these bits in the next posting.

Thanks for the review!
Balbir Singh.

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

^ permalink raw reply

* Re: [PATCH v2 2/2] mm: skip HWPoisoned pages when onlining pages
From: Balbir Singh @ 2017-04-28  2:51 UTC (permalink / raw)
  To: Naoya Horiguchi
  Cc: Laurent Dufour, linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	akpm@linux-foundation.org
In-Reply-To: <20170426031255.GB11619@hori1.linux.bs1.fc.nec.co.jp>

On Wed, 2017-04-26 at 03:13 +0000, Naoya Horiguchi wrote:
> On Wed, Apr 26, 2017 at 12:10:15PM +1000, Balbir Singh wrote:
> > On Tue, 2017-04-25 at 16:27 +0200, Laurent Dufour wrote:
> > > The commit b023f46813cd ("memory-hotplug: skip HWPoisoned page when
> > > offlining pages") skip the HWPoisoned pages when offlining pages, but
> > > this should be skipped when onlining the pages too.
> > > 
> > > Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> > > ---
> > >  mm/memory_hotplug.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > > index 6fa7208bcd56..741ddb50e7d2 100644
> > > --- a/mm/memory_hotplug.c
> > > +++ b/mm/memory_hotplug.c
> > > @@ -942,6 +942,10 @@ static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages,
> > >  	if (PageReserved(pfn_to_page(start_pfn)))
> > >  		for (i = 0; i < nr_pages; i++) {
> > >  			page = pfn_to_page(start_pfn + i);
> > > +			if (PageHWPoison(page)) {
> > > +				ClearPageReserved(page);
> > 
> > Why do we clear page reserved? Also if the page is marked PageHWPoison, it
> > was never offlined to begin with? Or do you expect this to be set on newly
> > hotplugged memory? Also don't we need to skip the entire pageblock?
> 
> If I read correctly, to "skip HWPoiosned page" in commit b023f46813cd means
> that we skip the page status check for hwpoisoned pages *not* to prevent
> memory offlining for memblocks with hwpoisoned pages. That means that
> hwpoisoned pages can be offlined.
> 
> And another reason to clear PageReserved is that we could reuse the
> hwpoisoned page after onlining back with replacing the broken DIMM.
> In this usecase, we first do unpoisoning to clear PageHWPoison,
> but it doesn't work if PageReserved is set. My simple testing shows
> the BUG below in unpoisoning (without the ClearPageReserved):
>

Fair enough, thanks for the explanation

Balbir Singh. 

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

^ permalink raw reply

* Re: [PATCH] mm: Add additional consistency check
From: Kees Cook @ 2017-04-28  1:11 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Christoph Lameter, Andrew Morton, Pekka Enberg, David Rientjes,
	Joonsoo Kim, Linux-MM, LKML
In-Reply-To: <20170411141956.GP6729@dhcp22.suse.cz>

On Tue, Apr 11, 2017 at 7:19 AM, Michal Hocko <mhocko@kernel.org> wrote:
> I would do something like...
> ---
> diff --git a/mm/slab.c b/mm/slab.c
> index bd63450a9b16..87c99a5e9e18 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -393,10 +393,15 @@ static inline void set_store_user_dirty(struct kmem_cache *cachep) {}
>  static int slab_max_order = SLAB_MAX_ORDER_LO;
>  static bool slab_max_order_set __initdata;
>
> +static inline struct kmem_cache *page_to_cache(struct page *page)
> +{
> +       return page->slab_cache;
> +}
> +
>  static inline struct kmem_cache *virt_to_cache(const void *obj)
>  {
>         struct page *page = virt_to_head_page(obj);
> -       return page->slab_cache;
> +       return page_to_cache(page);
>  }
>
>  static inline void *index_to_obj(struct kmem_cache *cache, struct page *page,
> @@ -3813,14 +3818,18 @@ void kfree(const void *objp)
>  {
>         struct kmem_cache *c;
>         unsigned long flags;
> +       struct page *page;
>
>         trace_kfree(_RET_IP_, objp);
>
>         if (unlikely(ZERO_OR_NULL_PTR(objp)))
>                 return;
> +       page = virt_to_head_page(obj);
> +       if (CHECK_DATA_CORRUPTION(!PageSlab(page)))
> +               return;
>         local_irq_save(flags);
>         kfree_debugcheck(objp);
> -       c = virt_to_cache(objp);
> +       c = page_to_cache(page);
>         debug_check_no_locks_freed(objp, c->object_size);
>
>         debug_check_no_obj_freed(objp, c->object_size);

Sorry for the delay, I've finally had time to look at this again.

So, this only handles the kfree() case, not the kmem_cache_free() nor
kmem_cache_free_bulk() cases, so it misses all the non-kmalloc
allocations (and kfree() ultimately calls down to kmem_cache_free()).
Similarly, my proposed patch missed the kfree() path. :P

As I work on a replacement, is the goal to avoid the checks while
under local_irq_save()? (i.e. I can't just put the check in
virt_to_cache(), etc.)

-Kees

-- 
Kees Cook
Pixel Security

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

^ permalink raw reply

* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
From: Huang, Ying @ 2017-04-28  1:09 UTC (permalink / raw)
  To: Minchan Kim
  Cc: Huang, Ying, Andrew Morton, linux-mm, linux-kernel, Hugh Dickins,
	Shaohua Li, Rik van Riel
In-Reply-To: <20170427043545.GA1726@bbox>

Minchan Kim <minchan@kernel.org> writes:

> On Wed, Apr 26, 2017 at 08:42:10PM +0800, Huang, Ying wrote:
>> Minchan Kim <minchan@kernel.org> writes:
>> 
>> > On Fri, Apr 21, 2017 at 08:29:30PM +0800, Huang, Ying wrote:
>> >> "Huang, Ying" <ying.huang@intel.com> writes:
>> >> 
>> >> > Minchan Kim <minchan@kernel.org> writes:
>> >> >
>> >> >> On Wed, Apr 19, 2017 at 04:14:43PM +0800, Huang, Ying wrote:
>> >> >>> Minchan Kim <minchan@kernel.org> writes:
>> >> >>> 
>> >> >>> > Hi Huang,
>> >> >>> >
>> >> >>> > On Fri, Apr 07, 2017 at 02:49:01PM +0800, Huang, Ying wrote:
>> >> >>> >> From: Huang Ying <ying.huang@intel.com>
>> >> >>> >> 
>> >> >>> >>  void swapcache_free_entries(swp_entry_t *entries, int n)
>> >> >>> >>  {
>> >> >>> >>  	struct swap_info_struct *p, *prev;
>> >> >>> >> @@ -1075,6 +1083,10 @@ void swapcache_free_entries(swp_entry_t *entries, int n)
>> >> >>> >>  
>> >> >>> >>  	prev = NULL;
>> >> >>> >>  	p = NULL;
>> >> >>> >> +
>> >> >>> >> +	/* Sort swap entries by swap device, so each lock is only taken once. */
>> >> >>> >> +	if (nr_swapfiles > 1)
>> >> >>> >> +		sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
>> >> >>> >
>> >> >>> > Let's think on other cases.
>> >> >>> >
>> >> >>> > There are two swaps and they are configured by priority so a swap's usage
>> >> >>> > would be zero unless other swap used up. In case of that, this sorting
>> >> >>> > is pointless.
>> >> >>> >
>> >> >>> > As well, nr_swapfiles is never decreased so if we enable multiple
>> >> >>> > swaps and then disable until a swap is remained, this sorting is
>> >> >>> > pointelss, too.
>> >> >>> >
>> >> >>> > How about lazy sorting approach? IOW, if we found prev != p and,
>> >> >>> > then we can sort it.
>> >> >>> 
>> >> >>> Yes.  That should be better.  I just don't know whether the added
>> >> >>> complexity is necessary, given the array is short and sort is fast.
>> >> >>
>> >> >> Huh?
>> >> >>
>> >> >> 1. swapon /dev/XXX1
>> >> >> 2. swapon /dev/XXX2
>> >> >> 3. swapoff /dev/XXX2
>> >> >> 4. use only one swap
>> >> >> 5. then, always pointless sort.
>> >> >
>> >> > Yes.  In this situation we will do unnecessary sorting.  What I don't
>> >> > know is whether the unnecessary sorting will hurt performance in real
>> >> > life.  I can do some measurement.
>> >> 
>> >> I tested the patch with 1 swap device and 1 process to eat memory
>> >> (remove the "if (nr_swapfiles > 1)" for test).  I think this is the
>> >> worse case because there is no lock contention.  The memory freeing time
>> >> increased from 1.94s to 2.12s (increase ~9.2%).  So there is some
>> >> overhead for some cases.  I change the algorithm to something like
>> >> below,
>> >> 
>> >>  void swapcache_free_entries(swp_entry_t *entries, int n)
>> >>  {
>> >>  	struct swap_info_struct *p, *prev;
>> >>  	int i;
>> >> +	swp_entry_t entry;
>> >> +	unsigned int prev_swp_type;
>> >>  
>> >>  	if (n <= 0)
>> >>  		return;
>> >>  
>> >> +	prev_swp_type = swp_type(entries[0]);
>> >> +	for (i = n - 1; i > 0; i--) {
>> >> +		if (swp_type(entries[i]) != prev_swp_type)
>> >> +			break;
>> >> +	}
>> >
>> > That's really what I want to avoid. For many swap usecases,
>> > it adds unnecessary overhead.
>> >
>> >> +
>> >> +	/* Sort swap entries by swap device, so each lock is only taken once. */
>> >> +	if (i)
>> >> +		sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
>> >>  	prev = NULL;
>> >>  	p = NULL;
>> >>  	for (i = 0; i < n; ++i) {
>> >> -		p = swap_info_get_cont(entries[i], prev);
>> >> +		entry = entries[i];
>> >> +		p = swap_info_get_cont(entry, prev);
>> >>  		if (p)
>> >> -			swap_entry_free(p, entries[i]);
>> >> +			swap_entry_free(p, entry);
>> >>  		prev = p;
>> >>  	}
>> >>  	if (p)
>> >> 
>> >> With this patch, the memory freeing time increased from 1.94s to 1.97s.
>> >> I think this is good enough.  Do you think so?
>> >
>> > What I mean is as follows(I didn't test it at all):
>> >
>> > With this, sort entries if we found multiple entries in current
>> > entries. It adds some condition checks for non-multiple swap
>> > usecase but it would be more cheaper than the sorting.
>> > And it adds a [un]lock overhead for multiple swap usecase but
>> > it should be a compromise for single-swap usecase which is more
>> > popular.
>> >
>> 
>> How about the following solution?  It can avoid [un]lock overhead and
>> double lock issue for multiple swap user case and has good performance
>> for one swap user case too.
>
> How worse with approach I suggested compared to as-is?

The performance difference between your version and my version is small
for my testing.

> Unless it's too bad, let's not add more complicated thing to just
> enhance the minor usecase in such even *slow* path.
> It adds code size/maintainance overead.
> With your suggestion, it might enhance a bit with speicific benchmark
> but not sure it's really worth for real practice.

I don't think the code complexity has much difference between our latest
versions.  As for complexity, I think my original version which just
uses nr_swapfiles to avoid sort() for single swap device is simple and
good enough for this task.  Maybe we can just improve the correctness of
swap device counting as Tim suggested.

Best Regards,
Huang, Ying

>> 
>> Best Regards,
>> Huang, Ying
>> 
>> From 7bd903c42749c448ef6acbbdee8dcbc1c5b498b9 Mon Sep 17 00:00:00 2001
>> From: Huang Ying <ying.huang@intel.com>
>> Date: Thu, 23 Feb 2017 13:05:20 +0800
>> Subject: [PATCH -v5] mm, swap: Sort swap entries before free
>> 
>> To reduce the lock contention of swap_info_struct->lock when freeing
>> swap entry.  The freed swap entries will be collected in a per-CPU
>> buffer firstly, and be really freed later in batch.  During the batch
>> freeing, if the consecutive swap entries in the per-CPU buffer belongs
>> to same swap device, the swap_info_struct->lock needs to be
>> acquired/released only once, so that the lock contention could be
>> reduced greatly.  But if there are multiple swap devices, it is
>> possible that the lock may be unnecessarily released/acquired because
>> the swap entries belong to the same swap device are non-consecutive in
>> the per-CPU buffer.
>> 
>> To solve the issue, the per-CPU buffer is sorted according to the swap
>> device before freeing the swap entries.  Test shows that the time
>> spent by swapcache_free_entries() could be reduced after the patch.
>> 
>> With the patch, the memory (some swapped out) free time reduced
>> 13.6% (from 2.59s to 2.28s) in the vm-scalability swap-w-rand test
>> case with 16 processes.  The test is done on a Xeon E5 v3 system.  The
>> swap device used is a RAM simulated PMEM (persistent memory) device.
>> To test swapping, the test case creates 16 processes, which allocate
>> and write to the anonymous pages until the RAM and part of the swap
>> device is used up, finally the memory (some swapped out) is freed
>> before exit.
>> 
>> Signed-off-by: Huang Ying <ying.huang@intel.com>
>> Acked-by: Tim Chen <tim.c.chen@intel.com>
>> Cc: Hugh Dickins <hughd@google.com>
>> Cc: Shaohua Li <shli@kernel.org>
>> Cc: Minchan Kim <minchan@kernel.org>
>> Cc: Rik van Riel <riel@redhat.com>
>> 
>> v5:
>> 
>> - Use a smarter way to determine whether sort is necessary.
>> 
>> v4:
>> 
>> - Avoid unnecessary sort if all entries are from one swap device.
>> 
>> v3:
>> 
>> - Add some comments in code per Rik's suggestion.
>> 
>> v2:
>> 
>> - Avoid sort swap entries if there is only one swap device.
>> ---
>>  mm/swapfile.c | 43 ++++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 38 insertions(+), 5 deletions(-)
>> 
>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>> index 71890061f653..10e75f9e8ac1 100644
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -37,6 +37,7 @@
>>  #include <linux/swapfile.h>
>>  #include <linux/export.h>
>>  #include <linux/swap_slots.h>
>> +#include <linux/sort.h>
>>  
>>  #include <asm/pgtable.h>
>>  #include <asm/tlbflush.h>
>> @@ -1065,20 +1066,52 @@ void swapcache_free(swp_entry_t entry)
>>  	}
>>  }
>>  
>> +static int swp_entry_cmp(const void *ent1, const void *ent2)
>> +{
>> +	const swp_entry_t *e1 = ent1, *e2 = ent2;
>> +
>> +	return (int)(swp_type(*e1) - swp_type(*e2));
>> +}
>> +
>>  void swapcache_free_entries(swp_entry_t *entries, int n)
>>  {
>>  	struct swap_info_struct *p, *prev;
>> -	int i;
>> +	int i, m;
>> +	swp_entry_t entry;
>> +	unsigned int prev_swp_type;
>>  
>>  	if (n <= 0)
>>  		return;
>>  
>>  	prev = NULL;
>>  	p = NULL;
>> -	for (i = 0; i < n; ++i) {
>> -		p = swap_info_get_cont(entries[i], prev);
>> -		if (p)
>> -			swap_entry_free(p, entries[i]);
>> +	m = 0;
>> +	prev_swp_type = swp_type(entries[0]);
>> +	for (i = 0; i < n; i++) {
>> +		entry = entries[i];
>> +		if (likely(swp_type(entry) == prev_swp_type)) {
>> +			p = swap_info_get_cont(entry, prev);
>> +			if (likely(p))
>> +				swap_entry_free(p, entry);
>> +			prev = p;
>> +		} else if (!m)
>> +			m = i;
>> +	}
>> +	if (p)
>> +		spin_unlock(&p->lock);
>> +	if (likely(!m))
>> +		return;
>> +
>> +	/* Sort swap entries by swap device, so each lock is only taken once. */
>> +	sort(entries + m, n - m, sizeof(entries[0]), swp_entry_cmp, NULL);
>> +	prev = NULL;
>> +	for (i = m; i < n; i++) {
>> +		entry = entries[i];
>> +		if (swp_type(entry) == prev_swp_type)
>> +			continue;
>> +		p = swap_info_get_cont(entry, prev);
>> +		if (likely(p))
>> +			swap_entry_free(p, entry);
>>  		prev = p;
>>  	}
>>  	if (p)
>> -- 
>> 2.11.0
>> 

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

^ permalink raw reply

* Re: [PATCH v2 1/2] mm: Uncharge poisoned pages
From: Andi Kleen @ 2017-04-27 20:51 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Laurent Dufour, Naoya Horiguchi, linux-kernel, linux-mm, akpm
In-Reply-To: <20170427143721.GK4706@dhcp22.suse.cz>

Michal Hocko <mhocko@kernel.org> writes:

> On Tue 25-04-17 16:27:51, Laurent Dufour wrote:
>> When page are poisoned, they should be uncharged from the root memory
>> cgroup.
>> 
>> This is required to avoid a BUG raised when the page is onlined back:
>> BUG: Bad page state in process mem-on-off-test  pfn:7ae3b
>> page:f000000001eb8ec0 count:0 mapcount:0 mapping:          (null)
>> index:0x1
>> flags: 0x3ffff800200000(hwpoison)
>
> My knowledge of memory poisoning is very rudimentary but aren't those
> pages supposed to leak and never come back? In other words isn't the
> hoplug code broken because it should leave them alone?

Yes that would be the right interpretation. If it was really offlined
due to a hardware error the memory will be poisoned and any access
could cause a machine check.

hwpoison has an own "unpoison" option (only used for debugging), which
I think handles this.

-Andi

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

^ permalink raw reply

* Re: [RFC 1/4] mm: create N_COHERENT_MEMORY
From: Reza Arbab @ 2017-04-27 18:42 UTC (permalink / raw)
  To: Balbir Singh
  Cc: linux-mm, akpm, khandual, benh, aneesh.kumar, paulmck, srikar,
	haren, jglisse, mgorman, mhocko, vbabka, cl
In-Reply-To: <20170419075242.29929-2-bsingharora@gmail.com>

On Wed, Apr 19, 2017 at 05:52:39PM +1000, Balbir Singh wrote:
>In this patch we create N_COHERENT_MEMORY, which is different
>from N_MEMORY. A node hotplugged as coherent memory will have
>this state set. The expectation then is that this memory gets
>onlined like regular nodes. Memory allocation from such nodes
>occurs only when the the node is contained explicitly in the
>mask.

Finally got around to test drive this. From what I can see, as expected,
both kernel and userspace seem to ignore these nodes, unless you 
allocate specifically from them. Very convenient.

Is "online_coherent"/MMOP_ONLINE_COHERENT the right way to trigger this?  
That mechanism is used to specify zone, and only for a single block of 
memory. This concept applies to the node as a whole. I think it should 
be independent of memory onlining.

I mean, let's say online_kernel N blocks, some of them get allocated, 
and then you online_coherent block N+1, flipping the entire node into 
N_COHERENT_MEMORY. That doesn't seem right.

That said, this set as it stands needs an adjustment when based on top 
of Michal's onlining revamp [1]. As-is, allow_online_pfn_range() is 
returning false. The patch below fixed it for me.

[1] http://lkml.kernel.org/r/20170421120512.23960-1-mhocko@kernel.org

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 4a535f1..ccb7a84 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -869,16 +869,20 @@ bool allow_online_pfn_range(int nid, unsigned long pfn, unsigned long nr_pages,
 	 * though so let's stick with it for simplicity for now.
 	 * TODO make sure we do not overlap with ZONE_DEVICE
 	 */
-	if (online_type == MMOP_ONLINE_KERNEL) {
+	switch (online_type) {
+	case MMOP_ONLINE_KERNEL:
 		if (zone_is_empty(movable_zone))
 			return true;
 		return movable_zone->zone_start_pfn >= pfn + nr_pages;
-	} else if (online_type == MMOP_ONLINE_MOVABLE) {
+	case MMOP_ONLINE_MOVABLE:
 		return zone_end_pfn(normal_zone) <= pfn;
+	case MMOP_ONLINE_KEEP:
+	case MMOP_ONLINE_COHERENT:
+		/* These will always succeed and inherit the current zone */
+		return true;
 	}
 
-	/* MMOP_ONLINE_KEEP will always succeed and inherits the current zone */
-	return online_type == MMOP_ONLINE_KEEP;
+	return false;
 }
 
 static void __meminit resize_zone_range(struct zone *zone, unsigned long start_pfn,


-- 
Reza Arbab

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

^ permalink raw reply related

* Re: [PATCH 0/3 v3] ARM/ARM64: silence large module first time allocation
From: Florian Fainelli @ 2017-04-27 18:34 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: linux-arm-kernel, Russell King, Catalin Marinas, Will Deacon,
	Andrew Morton, Michal Hocko, zijun_hu, Kirill A. Shutemov,
	Andrey Ryabinin, Chris Wilson, open list,
	open list:MEMORY MANAGEMENT, angus
In-Reply-To: <56EE35F5-97F9-4EA4-894B-581E7AE09A78@linaro.org>

On 04/27/2017 11:24 AM, Ard Biesheuvel wrote:
> 
>> On 27 Apr 2017, at 19:18, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>> With kernels built with CONFIG_ARM{,64}_MODULES_PLTS=y, the first allocation
>> done from module space will fail, produce a general OOM allocation and also a
>> vmap warning. The second allocation from vmalloc space may or may not be
>> successful, but is actually the one we are interested about in these cases.
>>
>> This patch series passed __GFP_NOWARN to silence such allocations from the
>> ARM/ARM64 module loader's first time allocation when the MODULES_PLT option is
>> enabled, and also makes alloc_vmap_area() react to the caller setting
>> __GFP_NOWARN to silence "vmap allocation for size..." messages.
>>
>>
>> Changes in v3:
>> - check for __GFP_NOWARN not set where the check for printk_ratelimited()
>>  is done, add Michal's Acked-by
>>
>> - use C conditionals and not CPP conditionals for IS_ENABLED(), add Ard's
>>  Reviewed-by tag
>>
> 
> Ok these look fine now. But are you sure that omitting the single pr_warn() gets rid of all of that?
> Or do we need more patches on top?

Since the caller now set __GFP_NOWARN, this propagates correctly to all
functions called from alloc_vmap_area() like kmalloc_node(). With the
patches applied, I only get the stuff that I would expect:

# echo 8 7 4 1 > /proc/sys/kernel/printk
# insmod huge.ko
[   59.285547] huge: loading out-of-tree module taints kernel.
[   59.328553] big_init: I am a big module using 3932160 bytes of data!
#

> 
> 
>> Changes in v2:
>>
>> - check __GFP_NOWARN out of the printk_ratelimited() check (Michal)
>>
>> Here is an example of what we would get without these two patches, pretty
>> scary huh?
>>
>> # insmod /mnt/nfs/huge.ko 
>> [   22.114143] random: nonblocking pool is initialized
>> [   22.183575] vmap allocation for size 15736832 failed: use vmalloc=<size> to increase size.
>> [   22.191873] vmalloc: allocation failure: 15729534 bytes
>> [   22.197112] insmod: page allocation failure: order:0, mode:0xd0
>> [   22.203048] CPU: 2 PID: 1506 Comm: insmod Tainted: G           O    4.1.20-1.9pre-01082-gbbbff07bc3ce #9
>> [   22.212536] Hardware name: Broadcom STB (Flattened Device Tree)
>> [   22.218480] [<c0017eec>] (unwind_backtrace) from [<c00135c8>] (show_stack+0x10/0x14)
>> [   22.226238] [<c00135c8>] (show_stack) from [<c0638684>] (dump_stack+0x90/0xa4)
>> [   22.233473] [<c0638684>] (dump_stack) from [<c00aae1c>] (warn_alloc_failed+0x104/0x144)
>> [   22.241490] [<c00aae1c>] (warn_alloc_failed) from [<c00d72e0>] (__vmalloc_node_range+0x170/0x218)
>> [   22.250375] [<c00d72e0>] (__vmalloc_node_range) from [<c00147d0>] (module_alloc+0x50/0xac)
>> [   22.258651] [<c00147d0>] (module_alloc) from [<c008ae2c>] (module_alloc_update_bounds+0xc/0x6c)
>> [   22.267360] [<c008ae2c>] (module_alloc_update_bounds) from [<c008b778>] (load_module+0x8ec/0x2058)
>> [   22.276329] [<c008b778>] (load_module) from [<c008cfd4>] (SyS_init_module+0xf0/0x174)
>> [   22.284170] [<c008cfd4>] (SyS_init_module) from [<c0010140>] (ret_fast_syscall+0x0/0x3c)
>> [   22.292277] Mem-Info:
>> [   22.294567] active_anon:5236 inactive_anon:1773 isolated_anon:0
>> [   22.294567]  active_file:1 inactive_file:3822 isolated_file:0
>> [   22.294567]  unevictable:0 dirty:0 writeback:0 unstable:0
>> [   22.294567]  slab_reclaimable:238 slab_unreclaimable:1594
>> [   22.294567]  mapped:855 shmem:2950 pagetables:36 bounce:0
>> [   22.294567]  free:39031 free_pcp:198 free_cma:3928
>> [   22.327196] DMA free:156124kB min:1880kB low:2348kB high:2820kB active_anon:20944kB inactive_anon:7092kB active_file:4kB inactive_file:15288kB unevictable:0kB isolated(anon):0kB isolated(file):0kB present:262144kB managed:227676kB mlocked:0kB dirty:0kB writeback:0kB mapped:3420kB shmem:11800kB slab_reclaimable:952kB slab_unreclaimable:6376kB kernel_stack:560kB pagetables:144kB unstable:0kB bounce:0kB free_pcp:792kB local_pcp:68kB free_cma:15712kB writeback_tmp:0kB pages_scanned:0 all_unreclaimable? no
>> [   22.371631] lowmem_reserve[]: 0 0 0 0
>> [   22.375372] HighMem free:0kB min:128kB low:128kB high:128kB active_anon:0kB inactive_anon:0kB active_file:0kB inactive_file:0kB unevictable:0kB isolated(anon):0kB isolated(file):0kB present:2883584kB managed:0kB mlocked:0kB dirty:0kB writeback:0kB mapped:0kB shmem:0kB slab_reclaimable:0kB slab_unreclaimable:0kB kernel_stack:0kB pagetables:0kB unstable:0kB bounce:0kB free_pcp:0kB local_pcp:0kB free_cma:0kB writeback_tmp:0kB pages_scanned:0 all_unreclaimable? yes
>> [   22.416249] lowmem_reserve[]: 0 0 0 0
>> [   22.419986] DMA: 3*4kB (UEM) 4*8kB (UE) 1*16kB (M) 4*32kB (UEMC) 3*64kB (EMC) 1*128kB (E) 4*256kB (UEMC) 2*512kB (UE) 2*1024kB (MC) 4*2048kB (UEMC) 35*4096kB (MRC) = 156156kB
>> [   22.435922] HighMem: 0*4kB 0*8kB 0*16kB 0*32kB 0*64kB 0*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB 0*4096kB = 0kB
>> [   22.446130] 6789 total pagecache pages
>> [   22.449889] 0 pages in swap cache
>> [   22.453212] Swap cache stats: add 0, delete 0, find 0/0
>> [   22.458447] Free swap  = 0kB
>> [   22.461334] Total swap = 0kB
>> [   22.464222] 786432 pages RAM
>> [   22.467110] 720896 pages HighMem/MovableOnly
>> [   22.471388] 725417 pages reserved
>> [   22.474711] 4096 pages cma reserved
>> [   22.511310] big_init: I am a big module using 3932160 bytes of data!
>>
>> Florian Fainelli (3):
>>  mm: Silence vmap() allocation failures based on caller gfp_flags
>>  ARM: Silence first allocation with CONFIG_ARM_MODULE_PLTS=y
>>  arm64: Silence first allocation with CONFIG_ARM64_MODULE_PLTS=y
>>
>> arch/arm/kernel/module.c   | 11 +++++++++--
>> arch/arm64/kernel/module.c |  7 ++++++-
>> mm/vmalloc.c               |  2 +-
>> 3 files changed, 16 insertions(+), 4 deletions(-)
>>
>> -- 
>> 2.9.3
>>


-- 
Florian

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

^ permalink raw reply

* Re: [RFC v3 05/17] RCU free VMAs
From: Paul E. McKenney @ 2017-04-27 18:28 UTC (permalink / raw)
  To: Laurent Dufour
  Cc: peterz, akpm, kirill, ak, mhocko, dave, jack, linux-kernel,
	linux-mm, haren, khandual, npiggin, bsingharora
In-Reply-To: <1493308376-23851-6-git-send-email-ldufour@linux.vnet.ibm.com>

On Thu, Apr 27, 2017 at 05:52:44PM +0200, Laurent Dufour wrote:
> From: Peter Zijlstra <peterz@infradead.org>
> 
> Manage the VMAs with SRCU such that we can do a lockless VMA lookup.
> 
> We put the fput(vma->vm_file) in the SRCU callback, this keeps files
> valid during speculative faults, this is possible due to the delayed
> fput work by Al Viro -- do we need srcu_barrier() in unmount
> someplace?
> 
> We guard the mm_rb tree with a seqlock (XXX could be a seqcount but
> we'd have to disable preemption around the write side in order to make
> the retry loop in __read_seqcount_begin() work) such that we can know
> if the rb tree walk was correct. We cannot trust the restult of a
> lockless tree walk in the face of concurrent tree rotations; although
> we can trust on the termination of such walks -- tree rotations
> guarantee the end result is a tree again after all.
> 
> Furthermore, we rely on the WMB implied by the
> write_seqlock/count_begin() to separate the VMA initialization and the
> publishing stores, analogous to the RELEASE in rcu_assign_pointer().
> We also rely on the RMB from read_seqretry() to separate the vma load
> from further loads like the smp_read_barrier_depends() in regular
> RCU.
> 
> We must not touch the vmacache while doing SRCU lookups as that is not
> properly serialized against changes. We update gap information after
> publishing the VMA, but A) we don't use that and B) the seqlock
> read side would fix that anyhow.
> 
> We clear vma->vm_rb for nodes removed from the vma tree such that we
> can easily detect such 'dead' nodes, we rely on the WMB from
> write_sequnlock() to separate the tree removal and clearing the node.
> 
> Provide find_vma_srcu() which wraps the required magic.
> 
> XXX: mmap()/munmap() heavy workloads might suffer from the global lock
> in call_srcu() -- this is fixable with a 'better' SRCU implementation.

An alleged 'better' SRCU implementation is now in -tip at branch
tip/core/rcu.  This implementation maintains per-CPU callback lists,
which eliminates the previous global lock.  The goal is to get this
'better' SRCU into mainline during the upcoming merge window.

							Thanx, Paul

> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  include/linux/mm_types.h |   2 +
>  kernel/fork.c            |   1 +
>  mm/init-mm.c             |   1 +
>  mm/internal.h            |  18 +++++++++
>  mm/mmap.c                | 100 +++++++++++++++++++++++++++++++++++------------
>  5 files changed, 96 insertions(+), 26 deletions(-)
> 
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index daa5fbba9349..f276973b0f91 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -359,6 +359,7 @@ struct vm_area_struct {
>  #endif
>  	struct vm_userfaultfd_ctx vm_userfaultfd_ctx;
>  	seqcount_t vm_sequence;
> +	struct rcu_head vm_rcu_head;
>  };
> 
>  struct core_thread {
> @@ -397,6 +398,7 @@ struct kioctx_table;
>  struct mm_struct {
>  	struct vm_area_struct *mmap;		/* list of VMAs */
>  	struct rb_root mm_rb;
> +	seqlock_t mm_seq;
>  	u32 vmacache_seqnum;                   /* per-thread vmacache */
>  #ifdef CONFIG_MMU
>  	unsigned long (*get_unmapped_area) (struct file *filp,
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 11c5c8ab827c..352cf3fd6c19 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -753,6 +753,7 @@ static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p,
>  	mm->mmap = NULL;
>  	mm->mm_rb = RB_ROOT;
>  	mm->vmacache_seqnum = 0;
> +	seqlock_init(&mm->mm_seq);
>  	atomic_set(&mm->mm_users, 1);
>  	atomic_set(&mm->mm_count, 1);
>  	init_rwsem(&mm->mmap_sem);
> diff --git a/mm/init-mm.c b/mm/init-mm.c
> index 975e49f00f34..2b1fa061684f 100644
> --- a/mm/init-mm.c
> +++ b/mm/init-mm.c
> @@ -16,6 +16,7 @@
> 
>  struct mm_struct init_mm = {
>  	.mm_rb		= RB_ROOT,
> +	.mm_seq		= __SEQLOCK_UNLOCKED(init_mm.mm_seq),
>  	.pgd		= swapper_pg_dir,
>  	.mm_users	= ATOMIC_INIT(2),
>  	.mm_count	= ATOMIC_INIT(1),
> diff --git a/mm/internal.h b/mm/internal.h
> index 7aa2ea0a8623..69df80ebc93d 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -40,6 +40,24 @@ void page_writeback_init(void);
> 
>  int do_swap_page(struct vm_fault *vmf);
> 
> +extern struct srcu_struct vma_srcu;
> +
> +extern struct vm_area_struct *find_vma_srcu(struct mm_struct *mm, unsigned long addr);
> +
> +static inline bool vma_is_dead(struct vm_area_struct *vma, unsigned int sequence)
> +{
> +	int ret = RB_EMPTY_NODE(&vma->vm_rb);
> +	unsigned seq = ACCESS_ONCE(vma->vm_sequence.sequence);
> +
> +	/*
> +	 * Matches both the wmb in write_seqlock_{begin,end}() and
> +	 * the wmb in vma_rb_erase().
> +	 */
> +	smp_rmb();
> +
> +	return ret || seq != sequence;
> +}
> +
>  void free_pgtables(struct mmu_gather *tlb, struct vm_area_struct *start_vma,
>  		unsigned long floor, unsigned long ceiling);
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index cb41659bc9f9..44e19aa31315 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -159,6 +159,23 @@ void unlink_file_vma(struct vm_area_struct *vma)
>  	}
>  }
> 
> +DEFINE_SRCU(vma_srcu);
> +
> +static void __free_vma(struct rcu_head *head)
> +{
> +	struct vm_area_struct *vma =
> +		container_of(head, struct vm_area_struct, vm_rcu_head);
> +
> +	if (vma->vm_file)
> +		fput(vma->vm_file);
> +	kmem_cache_free(vm_area_cachep, vma);
> +}
> +
> +static void free_vma(struct vm_area_struct *vma)
> +{
> +	call_srcu(&vma_srcu, &vma->vm_rcu_head, __free_vma);
> +}
> +
>  /*
>   * Close a vm structure and free it, returning the next.
>   */
> @@ -169,10 +186,8 @@ static struct vm_area_struct *remove_vma(struct vm_area_struct *vma)
>  	might_sleep();
>  	if (vma->vm_ops && vma->vm_ops->close)
>  		vma->vm_ops->close(vma);
> -	if (vma->vm_file)
> -		fput(vma->vm_file);
>  	mpol_put(vma_policy(vma));
> -	kmem_cache_free(vm_area_cachep, vma);
> +	free_vma(vma);
>  	return next;
>  }
> 
> @@ -394,26 +409,37 @@ static void vma_gap_update(struct vm_area_struct *vma)
>  }
> 
>  static inline void vma_rb_insert(struct vm_area_struct *vma,
> -				 struct rb_root *root)
> +				 struct mm_struct *mm)
>  {
> +	struct rb_root *root = &mm->mm_rb;
> +
>  	/* All rb_subtree_gap values must be consistent prior to insertion */
>  	validate_mm_rb(root, NULL);
> 
>  	rb_insert_augmented(&vma->vm_rb, root, &vma_gap_callbacks);
>  }
> 
> -static void __vma_rb_erase(struct vm_area_struct *vma, struct rb_root *root)
> +static void __vma_rb_erase(struct vm_area_struct *vma, struct mm_struct *mm)
>  {
> +	struct rb_root *root = &mm->mm_rb;
>  	/*
>  	 * Note rb_erase_augmented is a fairly large inline function,
>  	 * so make sure we instantiate it only once with our desired
>  	 * augmented rbtree callbacks.
>  	 */
> +	write_seqlock(&mm->mm_seq);
>  	rb_erase_augmented(&vma->vm_rb, root, &vma_gap_callbacks);
> +	write_sequnlock(&mm->mm_seq); /* wmb */
> +
> +	/*
> +	 * Ensure the removal is complete before clearing the node.
> +	 * Matched by vma_is_dead()/handle_speculative_fault().
> +	 */
> +	RB_CLEAR_NODE(&vma->vm_rb);
>  }
> 
>  static __always_inline void vma_rb_erase_ignore(struct vm_area_struct *vma,
> -						struct rb_root *root,
> +						struct mm_struct *mm,
>  						struct vm_area_struct *ignore)
>  {
>  	/*
> @@ -421,21 +447,21 @@ static __always_inline void vma_rb_erase_ignore(struct vm_area_struct *vma,
>  	 * with the possible exception of the "next" vma being erased if
>  	 * next->vm_start was reduced.
>  	 */
> -	validate_mm_rb(root, ignore);
> +	validate_mm_rb(&mm->mm_rb, ignore);
> 
> -	__vma_rb_erase(vma, root);
> +	__vma_rb_erase(vma, mm);
>  }
> 
>  static __always_inline void vma_rb_erase(struct vm_area_struct *vma,
> -					 struct rb_root *root)
> +					 struct mm_struct *mm)
>  {
>  	/*
>  	 * All rb_subtree_gap values must be consistent prior to erase,
>  	 * with the possible exception of the vma being erased.
>  	 */
> -	validate_mm_rb(root, vma);
> +	validate_mm_rb(&mm->mm_rb, vma);
> 
> -	__vma_rb_erase(vma, root);
> +	__vma_rb_erase(vma, mm);
>  }
> 
>  /*
> @@ -552,10 +578,12 @@ void __vma_link_rb(struct mm_struct *mm, struct vm_area_struct *vma,
>  	 * immediately update the gap to the correct value. Finally we
>  	 * rebalance the rbtree after all augmented values have been set.
>  	 */
> +	write_seqlock(&mm->mm_seq);
>  	rb_link_node(&vma->vm_rb, rb_parent, rb_link);
>  	vma->rb_subtree_gap = 0;
>  	vma_gap_update(vma);
> -	vma_rb_insert(vma, &mm->mm_rb);
> +	vma_rb_insert(vma, mm);
> +	write_sequnlock(&mm->mm_seq);
>  }
> 
>  static void __vma_link_file(struct vm_area_struct *vma)
> @@ -631,7 +659,7 @@ static __always_inline void __vma_unlink_common(struct mm_struct *mm,
>  {
>  	struct vm_area_struct *next;
> 
> -	vma_rb_erase_ignore(vma, &mm->mm_rb, ignore);
> +	vma_rb_erase_ignore(vma, mm, ignore);
>  	next = vma->vm_next;
>  	if (has_prev)
>  		prev->vm_next = next;
> @@ -883,21 +911,19 @@ int __vma_adjust(struct vm_area_struct *vma, unsigned long start,
>  	}
> 
>  	if (remove_next) {
> -		if (file) {
> +		if (file)
>  			uprobe_munmap(next, next->vm_start, next->vm_end);
> -			fput(file);
> -		}
>  		if (next->anon_vma)
>  			anon_vma_merge(vma, next);
>  		mm->map_count--;
>  		mpol_put(vma_policy(next));
> -		kmem_cache_free(vm_area_cachep, next);
> -		write_seqcount_end(&next->vm_sequence);
> +		free_vma(next);
>  		/*
>  		 * In mprotect's case 6 (see comments on vma_merge),
>  		 * we must remove another next too. It would clutter
>  		 * up the code too much to do both in one go.
>  		 */
> +		write_seqcount_end(&next->vm_sequence);
>  		if (remove_next != 3) {
>  			/*
>  			 * If "next" was removed and vma->vm_end was
> @@ -2103,16 +2129,11 @@ get_unmapped_area(struct file *file, unsigned long addr, unsigned long len,
>  EXPORT_SYMBOL(get_unmapped_area);
> 
>  /* Look up the first VMA which satisfies  addr < vm_end,  NULL if none. */
> -struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
> +static struct vm_area_struct *__find_vma(struct mm_struct *mm, unsigned long addr)
>  {
>  	struct rb_node *rb_node;
>  	struct vm_area_struct *vma;
> 
> -	/* Check the cache first. */
> -	vma = vmacache_find(mm, addr);
> -	if (likely(vma))
> -		return vma;
> -
>  	rb_node = mm->mm_rb.rb_node;
> 
>  	while (rb_node) {
> @@ -2129,13 +2150,40 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
>  			rb_node = rb_node->rb_right;
>  	}
> 
> +	return vma;
> +}
> +
> +struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
> +{
> +	struct vm_area_struct *vma;
> +
> +	/* Check the cache first. */
> +	vma = vmacache_find(mm, addr);
> +	if (likely(vma))
> +		return vma;
> +
> +	vma = __find_vma(mm, addr);
>  	if (vma)
>  		vmacache_update(addr, vma);
>  	return vma;
>  }
> -
>  EXPORT_SYMBOL(find_vma);
> 
> +struct vm_area_struct *find_vma_srcu(struct mm_struct *mm, unsigned long addr)
> +{
> +	struct vm_area_struct *vma;
> +	unsigned int seq;
> +
> +	WARN_ON_ONCE(!srcu_read_lock_held(&vma_srcu));
> +
> +	do {
> +		seq = read_seqbegin(&mm->mm_seq);
> +		vma = __find_vma(mm, addr);
> +	} while (read_seqretry(&mm->mm_seq, seq));
> +
> +	return vma;
> +}
> +
>  /*
>   * Same as find_vma, but also return a pointer to the previous VMA in *pprev.
>   */
> @@ -2490,7 +2538,7 @@ detach_vmas_to_be_unmapped(struct mm_struct *mm, struct vm_area_struct *vma,
>  	insertion_point = (prev ? &prev->vm_next : &mm->mmap);
>  	vma->vm_prev = NULL;
>  	do {
> -		vma_rb_erase(vma, &mm->mm_rb);
> +		vma_rb_erase(vma, mm);
>  		mm->map_count--;
>  		tail_vma = vma;
>  		vma = vma->vm_next;
> -- 
> 2.7.4
> 

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

^ permalink raw reply

* Re: [PATCH 0/3 v3] ARM/ARM64: silence large module first time allocation
From: Ard Biesheuvel @ 2017-04-27 18:24 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: linux-arm-kernel, Russell King, Catalin Marinas, Will Deacon,
	Andrew Morton, Michal Hocko, zijun_hu, Kirill A. Shutemov,
	Andrey Ryabinin, Chris Wilson, open list,
	open list:MEMORY MANAGEMENT, angus
In-Reply-To: <20170427181902.28829-1-f.fainelli@gmail.com>


> On 27 Apr 2017, at 19:18, Florian Fainelli <f.fainelli@gmail.com> wrote:
> 
> With kernels built with CONFIG_ARM{,64}_MODULES_PLTS=y, the first allocation
> done from module space will fail, produce a general OOM allocation and also a
> vmap warning. The second allocation from vmalloc space may or may not be
> successful, but is actually the one we are interested about in these cases.
> 
> This patch series passed __GFP_NOWARN to silence such allocations from the
> ARM/ARM64 module loader's first time allocation when the MODULES_PLT option is
> enabled, and also makes alloc_vmap_area() react to the caller setting
> __GFP_NOWARN to silence "vmap allocation for size..." messages.
> 
> 
> Changes in v3:
> - check for __GFP_NOWARN not set where the check for printk_ratelimited()
>  is done, add Michal's Acked-by
> 
> - use C conditionals and not CPP conditionals for IS_ENABLED(), add Ard's
>  Reviewed-by tag
> 

Ok these look fine now. But are you sure that omitting the single pr_warn() gets rid of all of that?
Or do we need more patches on top?


> Changes in v2:
> 
> - check __GFP_NOWARN out of the printk_ratelimited() check (Michal)
> 
> Here is an example of what we would get without these two patches, pretty
> scary huh?
> 
> # insmod /mnt/nfs/huge.ko 
> [   22.114143] random: nonblocking pool is initialized
> [   22.183575] vmap allocation for size 15736832 failed: use vmalloc=<size> to increase size.
> [   22.191873] vmalloc: allocation failure: 15729534 bytes
> [   22.197112] insmod: page allocation failure: order:0, mode:0xd0
> [   22.203048] CPU: 2 PID: 1506 Comm: insmod Tainted: G           O    4.1.20-1.9pre-01082-gbbbff07bc3ce #9
> [   22.212536] Hardware name: Broadcom STB (Flattened Device Tree)
> [   22.218480] [<c0017eec>] (unwind_backtrace) from [<c00135c8>] (show_stack+0x10/0x14)
> [   22.226238] [<c00135c8>] (show_stack) from [<c0638684>] (dump_stack+0x90/0xa4)
> [   22.233473] [<c0638684>] (dump_stack) from [<c00aae1c>] (warn_alloc_failed+0x104/0x144)
> [   22.241490] [<c00aae1c>] (warn_alloc_failed) from [<c00d72e0>] (__vmalloc_node_range+0x170/0x218)
> [   22.250375] [<c00d72e0>] (__vmalloc_node_range) from [<c00147d0>] (module_alloc+0x50/0xac)
> [   22.258651] [<c00147d0>] (module_alloc) from [<c008ae2c>] (module_alloc_update_bounds+0xc/0x6c)
> [   22.267360] [<c008ae2c>] (module_alloc_update_bounds) from [<c008b778>] (load_module+0x8ec/0x2058)
> [   22.276329] [<c008b778>] (load_module) from [<c008cfd4>] (SyS_init_module+0xf0/0x174)
> [   22.284170] [<c008cfd4>] (SyS_init_module) from [<c0010140>] (ret_fast_syscall+0x0/0x3c)
> [   22.292277] Mem-Info:
> [   22.294567] active_anon:5236 inactive_anon:1773 isolated_anon:0
> [   22.294567]  active_file:1 inactive_file:3822 isolated_file:0
> [   22.294567]  unevictable:0 dirty:0 writeback:0 unstable:0
> [   22.294567]  slab_reclaimable:238 slab_unreclaimable:1594
> [   22.294567]  mapped:855 shmem:2950 pagetables:36 bounce:0
> [   22.294567]  free:39031 free_pcp:198 free_cma:3928
> [   22.327196] DMA free:156124kB min:1880kB low:2348kB high:2820kB active_anon:20944kB inactive_anon:7092kB active_file:4kB inactive_file:15288kB unevictable:0kB isolated(anon):0kB isolated(file):0kB present:262144kB managed:227676kB mlocked:0kB dirty:0kB writeback:0kB mapped:3420kB shmem:11800kB slab_reclaimable:952kB slab_unreclaimable:6376kB kernel_stack:560kB pagetables:144kB unstable:0kB bounce:0kB free_pcp:792kB local_pcp:68kB free_cma:15712kB writeback_tmp:0kB pages_scanned:0 all_unreclaimable? no
> [   22.371631] lowmem_reserve[]: 0 0 0 0
> [   22.375372] HighMem free:0kB min:128kB low:128kB high:128kB active_anon:0kB inactive_anon:0kB active_file:0kB inactive_file:0kB unevictable:0kB isolated(anon):0kB isolated(file):0kB present:2883584kB managed:0kB mlocked:0kB dirty:0kB writeback:0kB mapped:0kB shmem:0kB slab_reclaimable:0kB slab_unreclaimable:0kB kernel_stack:0kB pagetables:0kB unstable:0kB bounce:0kB free_pcp:0kB local_pcp:0kB free_cma:0kB writeback_tmp:0kB pages_scanned:0 all_unreclaimable? yes
> [   22.416249] lowmem_reserve[]: 0 0 0 0
> [   22.419986] DMA: 3*4kB (UEM) 4*8kB (UE) 1*16kB (M) 4*32kB (UEMC) 3*64kB (EMC) 1*128kB (E) 4*256kB (UEMC) 2*512kB (UE) 2*1024kB (MC) 4*2048kB (UEMC) 35*4096kB (MRC) = 156156kB
> [   22.435922] HighMem: 0*4kB 0*8kB 0*16kB 0*32kB 0*64kB 0*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB 0*4096kB = 0kB
> [   22.446130] 6789 total pagecache pages
> [   22.449889] 0 pages in swap cache
> [   22.453212] Swap cache stats: add 0, delete 0, find 0/0
> [   22.458447] Free swap  = 0kB
> [   22.461334] Total swap = 0kB
> [   22.464222] 786432 pages RAM
> [   22.467110] 720896 pages HighMem/MovableOnly
> [   22.471388] 725417 pages reserved
> [   22.474711] 4096 pages cma reserved
> [   22.511310] big_init: I am a big module using 3932160 bytes of data!
> 
> Florian Fainelli (3):
>  mm: Silence vmap() allocation failures based on caller gfp_flags
>  ARM: Silence first allocation with CONFIG_ARM_MODULE_PLTS=y
>  arm64: Silence first allocation with CONFIG_ARM64_MODULE_PLTS=y
> 
> arch/arm/kernel/module.c   | 11 +++++++++--
> arch/arm64/kernel/module.c |  7 ++++++-
> mm/vmalloc.c               |  2 +-
> 3 files changed, 16 insertions(+), 4 deletions(-)
> 
> -- 
> 2.9.3
> 

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

^ permalink raw reply

* Re: [PATCH v2 1/3] mm: Silence vmap() allocation failures based on caller gfp_flags
From: Florian Fainelli @ 2017-04-27 18:21 UTC (permalink / raw)
  To: Michal Hocko
  Cc: linux-arm-kernel, Russell King, Catalin Marinas, Will Deacon,
	Ard Biesheuvel, Andrew Morton, zijun_hu, Kirill A. Shutemov,
	Andrey Ryabinin, Chris Wilson, open list,
	open list:MEMORY MANAGEMENT, angus
In-Reply-To: <20170427182018.GC30672@dhcp22.suse.cz>

On 04/27/2017 11:20 AM, Michal Hocko wrote:
>>> would be shorter and you wouldn't need the goto and a label.
>>
>> Do you want me to resubmit with that change included?
> 
> Up to you. As I've said this is a nit at best.

I just sent a v3 based on feedback from Ard, thanks!
-- 
Florian

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

^ permalink raw reply

* Re: [PATCH v2 1/3] mm: Silence vmap() allocation failures based on caller gfp_flags
From: Michal Hocko @ 2017-04-27 18:20 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: linux-arm-kernel, Russell King, Catalin Marinas, Will Deacon,
	Ard Biesheuvel, Andrew Morton, zijun_hu, Kirill A. Shutemov,
	Andrey Ryabinin, Chris Wilson, open list,
	open list:MEMORY MANAGEMENT, angus
In-Reply-To: <416a788c-6160-1ce8-fccc-839f719b2a88@gmail.com>

On Thu 27-04-17 11:03:31, Florian Fainelli wrote:
> On 04/27/2017 10:56 AM, Michal Hocko wrote:
> > On Thu 27-04-17 10:38:58, Florian Fainelli wrote:
> >> If the caller has set __GFP_NOWARN don't print the following message:
> >> vmap allocation for size 15736832 failed: use vmalloc=<size> to increase
> >> size.
> >>
> >> This can happen with the ARM/Linux or ARM64/Linux module loader built
> >> with CONFIG_ARM{,64}_MODULE_PLTS=y which does a first attempt at loading
> >> a large module from module space, then falls back to vmalloc space.
> >>
> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > 
> > Acked-by: Michal Hocko <mhocko@suse.com>
> > 
> > just a nit
> > 
> >> ---
> >>  mm/vmalloc.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> >> index 0b057628a7ba..d8a851634674 100644
> >> --- a/mm/vmalloc.c
> >> +++ b/mm/vmalloc.c
> >> @@ -521,9 +521,13 @@ static struct vmap_area *alloc_vmap_area(unsigned long size,
> >>  		}
> >>  	}
> >>  
> >> +	if (gfp_mask & __GFP_NOWARN)
> >> +		goto out;
> >> +
> >>  	if (printk_ratelimit())
> > 
> > 	if (!(gfp_mask & __GFP_NOWARN) && printk_ratelimit())
> >>  		pr_warn("vmap allocation for size %lu failed: use vmalloc=<size> to increase size\n",
> >>  			size);
> > 
> > would be shorter and you wouldn't need the goto and a label.
> 
> Do you want me to resubmit with that change included?

Up to you. As I've said this is a nit at best.
-- 
Michal Hocko
SUSE Labs

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

^ permalink raw reply

* [PATCH v3 3/3] arm64: Silence first allocation with CONFIG_ARM64_MODULE_PLTS=y
From: Florian Fainelli @ 2017-04-27 18:19 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Florian Fainelli, Russell King, Catalin Marinas, Will Deacon,
	Ard Biesheuvel, Andrew Morton, Michal Hocko, zijun_hu,
	Kirill A. Shutemov, Andrey Ryabinin, Chris Wilson, open list,
	open list:MEMORY MANAGEMENT, angus
In-Reply-To: <20170427181902.28829-1-f.fainelli@gmail.com>

When CONFIG_ARM64_MODULE_PLTS is enabled, the first allocation using the
module space fails, because the module is too big, and then the module
allocation is attempted from vmalloc space. Silence the first allocation
failure in that case by setting __GFP_NOWARN.

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 arch/arm64/kernel/module.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index 7f316982ce00..093c13541efb 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -32,11 +32,16 @@
 
 void *module_alloc(unsigned long size)
 {
+	gfp_t gfp_mask = GFP_KERNEL;
 	void *p;
 
+	/* Silence the initial allocation */
+	if (IS_ENABLED(CONFIG_ARM64_MODULE_PLTS))
+		gfp_mask |= __GFP_NOWARN;
+
 	p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
 				module_alloc_base + MODULES_VSIZE,
-				GFP_KERNEL, PAGE_KERNEL_EXEC, 0,
+				gfp_mask, PAGE_KERNEL_EXEC, 0,
 				NUMA_NO_NODE, __builtin_return_address(0));
 
 	if (!p && IS_ENABLED(CONFIG_ARM64_MODULE_PLTS) &&
-- 
2.9.3

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

^ permalink raw reply related

* [PATCH v3 2/3] ARM: Silence first allocation with CONFIG_ARM_MODULE_PLTS=y
From: Florian Fainelli @ 2017-04-27 18:19 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Florian Fainelli, Russell King, Catalin Marinas, Will Deacon,
	Ard Biesheuvel, Andrew Morton, Michal Hocko, zijun_hu,
	Kirill A. Shutemov, Andrey Ryabinin, Chris Wilson, open list,
	open list:MEMORY MANAGEMENT, angus
In-Reply-To: <20170427181902.28829-1-f.fainelli@gmail.com>

When CONFIG_ARM_MODULE_PLTS is enabled, the first allocation using the
module space fails, because the module is too big, and then the module
allocation is attempted from vmalloc space. Silence the first allocation
failure in that case by setting __GFP_NOWARN.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 arch/arm/kernel/module.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index 80254b47dc34..3ff571c2c71c 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -40,8 +40,15 @@
 #ifdef CONFIG_MMU
 void *module_alloc(unsigned long size)
 {
-	void *p = __vmalloc_node_range(size, 1, MODULES_VADDR, MODULES_END,
-				GFP_KERNEL, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
+	gfp_t gfp_mask = GFP_KERNEL;
+	void *p;
+
+	/* Silence the initial allocation */
+	if (IS_ENABLED(CONFIG_ARM_MODULE_PLTS))
+		gfp_mask |= __GFP_NOWARN;
+
+	p = __vmalloc_node_range(size, 1, MODULES_VADDR, MODULES_END,
+				gfp_mask, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
 				__builtin_return_address(0));
 	if (!IS_ENABLED(CONFIG_ARM_MODULE_PLTS) || p)
 		return p;
-- 
2.9.3

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

^ permalink raw reply related

* [PATCH v3 1/3] mm: Silence vmap() allocation failures based on caller gfp_flags
From: Florian Fainelli @ 2017-04-27 18:19 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Florian Fainelli, Russell King, Catalin Marinas, Will Deacon,
	Ard Biesheuvel, Andrew Morton, Michal Hocko, zijun_hu,
	Kirill A. Shutemov, Andrey Ryabinin, Chris Wilson, open list,
	open list:MEMORY MANAGEMENT, angus
In-Reply-To: <20170427181902.28829-1-f.fainelli@gmail.com>

If the caller has set __GFP_NOWARN don't print the following message:
vmap allocation for size 15736832 failed: use vmalloc=<size> to increase
size.

This can happen with the ARM/Linux or ARM64/Linux module loader built
with CONFIG_ARM{,64}_MODULE_PLTS=y which does a first attempt at loading
a large module from module space, then falls back to vmalloc space.

Acked-by: Michal Hocko <mhocko@suse.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 mm/vmalloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 0b057628a7ba..b74f1d01ef76 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -521,7 +521,7 @@ static struct vmap_area *alloc_vmap_area(unsigned long size,
 		}
 	}
 
-	if (printk_ratelimit())
+	if (!(gfp_mask & __GFP_NOWARN) && printk_ratelimit())
 		pr_warn("vmap allocation for size %lu failed: use vmalloc=<size> to increase size\n",
 			size);
 	kfree(va);
-- 
2.9.3

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

^ permalink raw reply related

* [PATCH 0/3 v3] ARM/ARM64: silence large module first time allocation
From: Florian Fainelli @ 2017-04-27 18:18 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Florian Fainelli, Russell King, Catalin Marinas, Will Deacon,
	Ard Biesheuvel, Andrew Morton, Michal Hocko, zijun_hu,
	Kirill A. Shutemov, Andrey Ryabinin, Chris Wilson, open list,
	open list:MEMORY MANAGEMENT, angus

With kernels built with CONFIG_ARM{,64}_MODULES_PLTS=y, the first allocation
done from module space will fail, produce a general OOM allocation and also a
vmap warning. The second allocation from vmalloc space may or may not be
successful, but is actually the one we are interested about in these cases.

This patch series passed __GFP_NOWARN to silence such allocations from the
ARM/ARM64 module loader's first time allocation when the MODULES_PLT option is
enabled, and also makes alloc_vmap_area() react to the caller setting
__GFP_NOWARN to silence "vmap allocation for size..." messages.


Changes in v3:
- check for __GFP_NOWARN not set where the check for printk_ratelimited()
  is done, add Michal's Acked-by

- use C conditionals and not CPP conditionals for IS_ENABLED(), add Ard's
  Reviewed-by tag

Changes in v2:

- check __GFP_NOWARN out of the printk_ratelimited() check (Michal)

Here is an example of what we would get without these two patches, pretty
scary huh?

# insmod /mnt/nfs/huge.ko 
[   22.114143] random: nonblocking pool is initialized
[   22.183575] vmap allocation for size 15736832 failed: use vmalloc=<size> to increase size.
[   22.191873] vmalloc: allocation failure: 15729534 bytes
[   22.197112] insmod: page allocation failure: order:0, mode:0xd0
[   22.203048] CPU: 2 PID: 1506 Comm: insmod Tainted: G           O    4.1.20-1.9pre-01082-gbbbff07bc3ce #9
[   22.212536] Hardware name: Broadcom STB (Flattened Device Tree)
[   22.218480] [<c0017eec>] (unwind_backtrace) from [<c00135c8>] (show_stack+0x10/0x14)
[   22.226238] [<c00135c8>] (show_stack) from [<c0638684>] (dump_stack+0x90/0xa4)
[   22.233473] [<c0638684>] (dump_stack) from [<c00aae1c>] (warn_alloc_failed+0x104/0x144)
[   22.241490] [<c00aae1c>] (warn_alloc_failed) from [<c00d72e0>] (__vmalloc_node_range+0x170/0x218)
[   22.250375] [<c00d72e0>] (__vmalloc_node_range) from [<c00147d0>] (module_alloc+0x50/0xac)
[   22.258651] [<c00147d0>] (module_alloc) from [<c008ae2c>] (module_alloc_update_bounds+0xc/0x6c)
[   22.267360] [<c008ae2c>] (module_alloc_update_bounds) from [<c008b778>] (load_module+0x8ec/0x2058)
[   22.276329] [<c008b778>] (load_module) from [<c008cfd4>] (SyS_init_module+0xf0/0x174)
[   22.284170] [<c008cfd4>] (SyS_init_module) from [<c0010140>] (ret_fast_syscall+0x0/0x3c)
[   22.292277] Mem-Info:
[   22.294567] active_anon:5236 inactive_anon:1773 isolated_anon:0
[   22.294567]  active_file:1 inactive_file:3822 isolated_file:0
[   22.294567]  unevictable:0 dirty:0 writeback:0 unstable:0
[   22.294567]  slab_reclaimable:238 slab_unreclaimable:1594
[   22.294567]  mapped:855 shmem:2950 pagetables:36 bounce:0
[   22.294567]  free:39031 free_pcp:198 free_cma:3928
[   22.327196] DMA free:156124kB min:1880kB low:2348kB high:2820kB active_anon:20944kB inactive_anon:7092kB active_file:4kB inactive_file:15288kB unevictable:0kB isolated(anon):0kB isolated(file):0kB present:262144kB managed:227676kB mlocked:0kB dirty:0kB writeback:0kB mapped:3420kB shmem:11800kB slab_reclaimable:952kB slab_unreclaimable:6376kB kernel_stack:560kB pagetables:144kB unstable:0kB bounce:0kB free_pcp:792kB local_pcp:68kB free_cma:15712kB writeback_tmp:0kB pages_scanned:0 all_unreclaimable? no
[   22.371631] lowmem_reserve[]: 0 0 0 0
[   22.375372] HighMem free:0kB min:128kB low:128kB high:128kB active_anon:0kB inactive_anon:0kB active_file:0kB inactive_file:0kB unevictable:0kB isolated(anon):0kB isolated(file):0kB present:2883584kB managed:0kB mlocked:0kB dirty:0kB writeback:0kB mapped:0kB shmem:0kB slab_reclaimable:0kB slab_unreclaimable:0kB kernel_stack:0kB pagetables:0kB unstable:0kB bounce:0kB free_pcp:0kB local_pcp:0kB free_cma:0kB writeback_tmp:0kB pages_scanned:0 all_unreclaimable? yes
[   22.416249] lowmem_reserve[]: 0 0 0 0
[   22.419986] DMA: 3*4kB (UEM) 4*8kB (UE) 1*16kB (M) 4*32kB (UEMC) 3*64kB (EMC) 1*128kB (E) 4*256kB (UEMC) 2*512kB (UE) 2*1024kB (MC) 4*2048kB (UEMC) 35*4096kB (MRC) = 156156kB
[   22.435922] HighMem: 0*4kB 0*8kB 0*16kB 0*32kB 0*64kB 0*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB 0*4096kB = 0kB
[   22.446130] 6789 total pagecache pages
[   22.449889] 0 pages in swap cache
[   22.453212] Swap cache stats: add 0, delete 0, find 0/0
[   22.458447] Free swap  = 0kB
[   22.461334] Total swap = 0kB
[   22.464222] 786432 pages RAM
[   22.467110] 720896 pages HighMem/MovableOnly
[   22.471388] 725417 pages reserved
[   22.474711] 4096 pages cma reserved
[   22.511310] big_init: I am a big module using 3932160 bytes of data!

Florian Fainelli (3):
  mm: Silence vmap() allocation failures based on caller gfp_flags
  ARM: Silence first allocation with CONFIG_ARM_MODULE_PLTS=y
  arm64: Silence first allocation with CONFIG_ARM64_MODULE_PLTS=y

 arch/arm/kernel/module.c   | 11 +++++++++--
 arch/arm64/kernel/module.c |  7 ++++++-
 mm/vmalloc.c               |  2 +-
 3 files changed, 16 insertions(+), 4 deletions(-)

-- 
2.9.3

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

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox