Linux-mm Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH v2] mm, zone_device: replace {get, put}_zone_device_page() with a single reference
From: Logan Gunthorpe @ 2017-05-01  3:48 UTC (permalink / raw)
  To: Jerome Glisse, Kirill A. Shutemov
  Cc: Dan Williams, Ingo Molnar, linux-kernel@vger.kernel.org, Linux MM,
	Ingo Molnar, Andrew Morton, Kirill Shutemov
In-Reply-To: <20170430231421.GA15163@redhat.com>



On 30/04/17 05:14 PM, Jerome Glisse wrote:
> HMM ZONE_DEVICE pages are use like other pages (anonymous or file back page)
> in _any_ vma. So i need to know when a page is freed ie either as result of
> unmap, exit or migration or anything that would free the memory. For zone
> device a page is free once its refcount reach 1 so i need to catch refcount
> transition from 2->1
> 
> This is the only way i can inform the device that the page is now free. See
> 
> https://cgit.freedesktop.org/~glisse/linux/commit/?h=hmm-v21&id=52da8fe1a088b87b5321319add79e43b8372ed7d
> 
> There is _no_ way around that.

I had a similar issue in a piece of my p2pmem RFC [1]. I hacked around
it by tracking the pages separately and freeing them when the vma is
closed. This is by no means a great solution, it certainly has it's own
warts. However, maybe it will spark some ideas for some alternate
choices which avoid the hot path.

Another thing I briefly looked at was hooking the vma close process
earlier so that it would callback in time that you can loop through the
pages and do your free process. Of course this all depends on the vma
not getting closed while the pages have other references. So it may not
work at all. Again, just ideas.

Logan


[1]
https://github.com/sbates130272/linux-p2pmem/commit/77c631d92cb5c451c9824b3a4cf9b6cddfde6bb7

--
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] mm, zone_device: replace {get, put}_zone_device_page() with a single reference
From: Dan Williams @ 2017-05-01  2:45 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: linux-kernel@vger.kernel.org, Linux MM, Jérôme Glisse,
	Ingo Molnar, Andrew Morton, Logan Gunthorpe, Kirill Shutemov
In-Reply-To: <20170429141838.tkyfxhldmwypyipz@gmail.com>

On Sat, Apr 29, 2017 at 7:18 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Dan Williams <dan.j.williams@intel.com> 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: Jérôme Glisse <jglisse@redhat.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
>> Suggested-by: Kirill Shutemov <kirill.shutemov@linux.intel.com>
>> Tested-by: Kirill Shutemov <kirill.shutemov@linux.intel.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> This changelog is lacking an explanation about how this solves the crashes you
> were seeing.
>

Kirill? It wasn't clear to me why the conversion to generic
get_user_pages_fast() caused the reference counts to be off.

--
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 2/3] mm/slub: wrap cpu_slab->partial in CONFIG_SLUB_CPU_PARTIAL
From: Matthew Wilcox @ 2017-05-01  2:41 UTC (permalink / raw)
  To: Wei Yang
  Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, linux-mm,
	linux-kernel
In-Reply-To: <20170430113152.6590-3-richard.weiyang@gmail.com>

On Sun, Apr 30, 2017 at 07:31:51PM +0800, Wei Yang wrote:
> @@ -2302,7 +2302,11 @@ static bool has_cpu_slab(int cpu, void *info)
>  	struct kmem_cache *s = info;
>  	struct kmem_cache_cpu *c = per_cpu_ptr(s->cpu_slab, cpu);
>  
> -	return c->page || c->partial;
> +	return c->page
> +#ifdef CONFIG_SLUB_CPU_PARTIAL
> +		|| c->partial
> +#endif
> +		;
>  }

No.  No way.  This is disgusting.

The right way to do this is to create an accessor like this:

#ifdef CONFIG_SLUB_CPU_PARTIAL
#define slub_cpu_partial(c)	((c)->partial)
#else
#define slub_cpu_partial(c)	0
#endif

And then the above becomes:

-	return c->page || c->partial;
+	return c->page || slub_cpu_partial(c);

All the other ifdefs go away, apart from these two:

> @@ -4980,6 +4990,7 @@ static ssize_t objects_partial_show(struct kmem_cache *s, char *buf)
>  }
>  SLAB_ATTR_RO(objects_partial);
>  
> +#ifdef CONFIG_SLUB_CPU_PARTIAL
>  static ssize_t slabs_cpu_partial_show(struct kmem_cache *s, char *buf)
>  {
>  	int objects = 0;
> @@ -5010,6 +5021,7 @@ static ssize_t slabs_cpu_partial_show(struct kmem_cache *s, char *buf)
>  	return len + sprintf(buf + len, "\n");
>  }
>  SLAB_ATTR_RO(slabs_cpu_partial);
> +#endif
>  
>  static ssize_t reclaim_account_show(struct kmem_cache *s, char *buf)
>  {
> @@ -5364,7 +5376,9 @@ static struct attribute *slab_attrs[] = {
>  	&destroy_by_rcu_attr.attr,
>  	&shrink_attr.attr,
>  	&reserved_attr.attr,
> +#ifdef CONFIG_SLUB_CPU_PARTIAL
>  	&slabs_cpu_partial_attr.attr,
> +#endif
>  #ifdef CONFIG_SLUB_DEBUG
>  	&total_objects_attr.attr,
>  	&slabs_attr.attr,

--
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] mm, zone_device: replace {get, put}_zone_device_page() with a single reference
From: Dan Williams @ 2017-05-01  2:40 UTC (permalink / raw)
  To: Jerome Glisse
  Cc: Kirill A. Shutemov, Ingo Molnar, linux-kernel@vger.kernel.org,
	Linux MM, Ingo Molnar, Andrew Morton, Logan Gunthorpe,
	Kirill Shutemov
In-Reply-To: <20170501015403.GA16181@redhat.com>

On Sun, Apr 30, 2017 at 6:54 PM, Jerome Glisse <jglisse@redhat.com> wrote:
> On Sun, Apr 30, 2017 at 06:42:02PM -0700, Dan Williams wrote:
>> On Sun, Apr 30, 2017 at 4:14 PM, Jerome Glisse <jglisse@redhat.com> wrote:
>> > On Sat, Apr 29, 2017 at 01:17:26PM +0300, Kirill A. Shutemov wrote:
>> >> On Fri, Apr 28, 2017 at 03:33:07PM -0400, Jerome Glisse wrote:
>> >> > On Fri, Apr 28, 2017 at 12:22:24PM -0700, Dan Williams wrote:
>> >> > > Are you sure about needing to hook the 2 -> 1 transition? Could we
>> >> > > change ZONE_DEVICE pages to not have an elevated reference count when
>> >> > > they are created so you can keep the HMM references out of the mm hot
>> >> > > path?
>> >> >
>> >> > 100% sure on that :) I need to callback into driver for 2->1 transition
>> >> > no way around that. If we change ZONE_DEVICE to not have an elevated
>> >> > reference count that you need to make a lot more change to mm so that
>> >> > ZONE_DEVICE is never use as fallback for memory allocation. Also need
>> >> > to make change to be sure that ZONE_DEVICE page never endup in one of
>> >> > the path that try to put them back on lru. There is a lot of place that
>> >> > would need to be updated and it would be highly intrusive and add a
>> >> > lot of special cases to other hot code path.
>> >>
>> >> Could you explain more on where the requirement comes from or point me to
>> >> where I can read about this.
>> >>
>> >
>> > HMM ZONE_DEVICE pages are use like other pages (anonymous or file back page)
>> > in _any_ vma. So i need to know when a page is freed ie either as result of
>> > unmap, exit or migration or anything that would free the memory. For zone
>> > device a page is free once its refcount reach 1 so i need to catch refcount
>> > transition from 2->1
>> >
>> > This is the only way i can inform the device that the page is now free. See
>> >
>> > https://cgit.freedesktop.org/~glisse/linux/commit/?h=hmm-v21&id=52da8fe1a088b87b5321319add79e43b8372ed7d
>> >
>> > There is _no_ way around that.
>>
>> Ok, but I need to point out that this not a ZONE_DEVICE requirement.
>> This is an HMM-specific need. So, this extra reference counting should
>> be clearly delineated as part of the MEMORY_DEVICE_PRIVATE use case.
>
> And it already is delimited, i think you even gave your review by on
> the patch.
>

I gave my reviewed-by to the patch that leveraged
{get,put}_zone_device_page(), but now those are gone and HMM needs a
new patch. I'm just saying these reference counts should not come back
as {get,put}_zone_device_page() because now they're HMM specific.

>> Can we hide the extra reference counting behind a static branch so
>> that the common case fast path doesn't get slower until a HMM device
>> shows up?
>
> Like i already did With likely()/unlikely() ? Or something else ?
>
> https://cgit.freedesktop.org/~glisse/linux/commit/?h=hmm-v21&id=e84778e9db0672e371eb6599dfcb812512118842

Something different:
http://lxr.free-electrons.com/source/Documentation/static-keys.txt

--
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] mm, zone_device: replace {get, put}_zone_device_page() with a single reference
From: Jerome Glisse @ 2017-05-01  1:54 UTC (permalink / raw)
  To: Dan Williams
  Cc: Kirill A. Shutemov, Ingo Molnar, linux-kernel@vger.kernel.org,
	Linux MM, Ingo Molnar, Andrew Morton, Logan Gunthorpe,
	Kirill Shutemov
In-Reply-To: <CAA9_cmekmRk3+MN0dTJap=dLgb+uLm9WEFEP+KDvg5uHbmhvCg@mail.gmail.com>

On Sun, Apr 30, 2017 at 06:42:02PM -0700, Dan Williams wrote:
> On Sun, Apr 30, 2017 at 4:14 PM, Jerome Glisse <jglisse@redhat.com> wrote:
> > On Sat, Apr 29, 2017 at 01:17:26PM +0300, Kirill A. Shutemov wrote:
> >> On Fri, Apr 28, 2017 at 03:33:07PM -0400, Jerome Glisse wrote:
> >> > On Fri, Apr 28, 2017 at 12:22:24PM -0700, Dan Williams wrote:
> >> > > Are you sure about needing to hook the 2 -> 1 transition? Could we
> >> > > change ZONE_DEVICE pages to not have an elevated reference count when
> >> > > they are created so you can keep the HMM references out of the mm hot
> >> > > path?
> >> >
> >> > 100% sure on that :) I need to callback into driver for 2->1 transition
> >> > no way around that. If we change ZONE_DEVICE to not have an elevated
> >> > reference count that you need to make a lot more change to mm so that
> >> > ZONE_DEVICE is never use as fallback for memory allocation. Also need
> >> > to make change to be sure that ZONE_DEVICE page never endup in one of
> >> > the path that try to put them back on lru. There is a lot of place that
> >> > would need to be updated and it would be highly intrusive and add a
> >> > lot of special cases to other hot code path.
> >>
> >> Could you explain more on where the requirement comes from or point me to
> >> where I can read about this.
> >>
> >
> > HMM ZONE_DEVICE pages are use like other pages (anonymous or file back page)
> > in _any_ vma. So i need to know when a page is freed ie either as result of
> > unmap, exit or migration or anything that would free the memory. For zone
> > device a page is free once its refcount reach 1 so i need to catch refcount
> > transition from 2->1
> >
> > This is the only way i can inform the device that the page is now free. See
> >
> > https://cgit.freedesktop.org/~glisse/linux/commit/?h=hmm-v21&id=52da8fe1a088b87b5321319add79e43b8372ed7d
> >
> > There is _no_ way around that.
> 
> Ok, but I need to point out that this not a ZONE_DEVICE requirement.
> This is an HMM-specific need. So, this extra reference counting should
> be clearly delineated as part of the MEMORY_DEVICE_PRIVATE use case.

And it already is delimited, i think you even gave your review by on
the patch.


> Can we hide the extra reference counting behind a static branch so
> that the common case fast path doesn't get slower until a HMM device
> shows up?

Like i already did With likely()/unlikely() ? Or something else ?

https://cgit.freedesktop.org/~glisse/linux/commit/?h=hmm-v21&id=e84778e9db0672e371eb6599dfcb812512118842

Cheers,
Jerome

--
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] mm, zone_device: replace {get, put}_zone_device_page() with a single reference
From: Dan Williams @ 2017-05-01  1:42 UTC (permalink / raw)
  To: Jerome Glisse
  Cc: Kirill A. Shutemov, Ingo Molnar, linux-kernel@vger.kernel.org,
	Linux MM, Ingo Molnar, Andrew Morton, Logan Gunthorpe,
	Kirill Shutemov
In-Reply-To: <20170430231421.GA15163@redhat.com>

On Sun, Apr 30, 2017 at 4:14 PM, Jerome Glisse <jglisse@redhat.com> wrote:
> On Sat, Apr 29, 2017 at 01:17:26PM +0300, Kirill A. Shutemov wrote:
>> On Fri, Apr 28, 2017 at 03:33:07PM -0400, Jerome Glisse wrote:
>> > On Fri, Apr 28, 2017 at 12:22:24PM -0700, Dan Williams wrote:
>> > > Are you sure about needing to hook the 2 -> 1 transition? Could we
>> > > change ZONE_DEVICE pages to not have an elevated reference count when
>> > > they are created so you can keep the HMM references out of the mm hot
>> > > path?
>> >
>> > 100% sure on that :) I need to callback into driver for 2->1 transition
>> > no way around that. If we change ZONE_DEVICE to not have an elevated
>> > reference count that you need to make a lot more change to mm so that
>> > ZONE_DEVICE is never use as fallback for memory allocation. Also need
>> > to make change to be sure that ZONE_DEVICE page never endup in one of
>> > the path that try to put them back on lru. There is a lot of place that
>> > would need to be updated and it would be highly intrusive and add a
>> > lot of special cases to other hot code path.
>>
>> Could you explain more on where the requirement comes from or point me to
>> where I can read about this.
>>
>
> HMM ZONE_DEVICE pages are use like other pages (anonymous or file back page)
> in _any_ vma. So i need to know when a page is freed ie either as result of
> unmap, exit or migration or anything that would free the memory. For zone
> device a page is free once its refcount reach 1 so i need to catch refcount
> transition from 2->1
>
> This is the only way i can inform the device that the page is now free. See
>
> https://cgit.freedesktop.org/~glisse/linux/commit/?h=hmm-v21&id=52da8fe1a088b87b5321319add79e43b8372ed7d
>
> There is _no_ way around that.

Ok, but I need to point out that this not a ZONE_DEVICE requirement.
This is an HMM-specific need. So, this extra reference counting should
be clearly delineated as part of the MEMORY_DEVICE_PRIVATE use case.

Can we hide the extra reference counting behind a static branch so
that the common case fast path doesn't get slower until a HMM device
shows up?

--
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] mm, zone_device: replace {get, put}_zone_device_page() with a single reference
From: Jerome Glisse @ 2017-04-30 23:14 UTC (permalink / raw)
  To: Kirill A. Shutemov
  Cc: Dan Williams, Ingo Molnar, linux-kernel@vger.kernel.org, Linux MM,
	Ingo Molnar, Andrew Morton, Logan Gunthorpe, Kirill Shutemov
In-Reply-To: <20170429101726.cdczojcjjupb7myy@node.shutemov.name>

On Sat, Apr 29, 2017 at 01:17:26PM +0300, Kirill A. Shutemov wrote:
> On Fri, Apr 28, 2017 at 03:33:07PM -0400, Jerome Glisse wrote:
> > On Fri, Apr 28, 2017 at 12:22:24PM -0700, Dan Williams wrote:
> > > Are you sure about needing to hook the 2 -> 1 transition? Could we
> > > change ZONE_DEVICE pages to not have an elevated reference count when
> > > they are created so you can keep the HMM references out of the mm hot
> > > path?
> > 
> > 100% sure on that :) I need to callback into driver for 2->1 transition
> > no way around that. If we change ZONE_DEVICE to not have an elevated
> > reference count that you need to make a lot more change to mm so that
> > ZONE_DEVICE is never use as fallback for memory allocation. Also need
> > to make change to be sure that ZONE_DEVICE page never endup in one of
> > the path that try to put them back on lru. There is a lot of place that
> > would need to be updated and it would be highly intrusive and add a
> > lot of special cases to other hot code path.
> 
> Could you explain more on where the requirement comes from or point me to
> where I can read about this.
> 

HMM ZONE_DEVICE pages are use like other pages (anonymous or file back page)
in _any_ vma. So i need to know when a page is freed ie either as result of
unmap, exit or migration or anything that would free the memory. For zone
device a page is free once its refcount reach 1 so i need to catch refcount
transition from 2->1

This is the only way i can inform the device that the page is now free. See

https://cgit.freedesktop.org/~glisse/linux/commit/?h=hmm-v21&id=52da8fe1a088b87b5321319add79e43b8372ed7d

There is _no_ way around that.

Cheers,
Jerome

--
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/6] mm, page_alloc: fix more premature OOM due to race with cpuset update
From: Christoph Lameter @ 2017-04-30 21:33 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: linux-mm, linux-kernel, cgroups, Li Zefan, Michal Hocko,
	Mel Gorman, David Rientjes, Hugh Dickins, Andrea Arcangeli,
	Anshuman Khandual, Kirill A. Shutemov, linux-api
In-Reply-To: <fda99ddc-94f5-456e-6560-d4991da452a6@suse.cz>

On Wed, 26 Apr 2017, Vlastimil Babka wrote:

> > Such an application typically already has such logic and executes a
> > binding after discovering its numa node configuration on startup. It would
> > have to be modified to redo that action when it gets some sort of a signal
> > from the script telling it that the node config would be changed.
> >
> > Having this logic in the application instead of the kernel avoids all the
> > kernel messes that we keep on trying to deal with and IMHO is much
> > cleaner.
>
> That would be much simpler for us indeed. But we still IMHO can't
> abruptly start denying page fault allocations for existing applications
> that don't have the necessary awareness.

We certainly can do that. The failure of the page faults are due to the
admin trying to move an application that is not aware of this and is using
mempols. That could be an error. Trying to move an application that
contains both absolute and relative node numbers is definitely something
that is potentiall so screwed up that the kernel should not muck around
with such an app.

Also user space can determine if the application is using memory policies
and can then take appropriate measures (message to the sysadmin to eval
tge situation f.e.) or mess aroud with the processes memory policies on
its own.

So this is certainly a way out of this mess.

--
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] try to save some memory for kmem_cache in some cases
From: Christoph Lameter @ 2017-04-30 21:22 UTC (permalink / raw)
  To: Wei Yang; +Cc: penberg, rientjes, iamjoonsoo.kim, akpm, linux-mm, linux-kernel
In-Reply-To: <20170430113152.6590-1-richard.weiyang@gmail.com>

On Sun, 30 Apr 2017, Wei Yang wrote:

> kmem_cache is a frequently used data in kernel. During the code reading, I
> found maybe we could save some space in some cases.
>
> 1. On 64bit arch, type int will occupy a word if it doesn't sit well.
> 2. cpu_slab->partial is just used when CONFIG_SLUB_CPU_PARTIAL is set
> 3. cpu_partial is just used when CONFIG_SLUB_CPU_PARTIAL is set, while just
> save some space on 32bit arch.

This looks fine. But do we really want to add that amount of ifdeffery?
How much memory does this save?

--
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 3/3] mm/slub: wrap kmem_cache->cpu_partial in config CONFIG_SLUB_CPU_PARTIAL
From: Wei Yang @ 2017-04-30 11:31 UTC (permalink / raw)
  To: cl, penberg, rientjes, iamjoonsoo.kim, akpm
  Cc: linux-mm, linux-kernel, Wei Yang
In-Reply-To: <20170430113152.6590-1-richard.weiyang@gmail.com>

kmem_cache->cpu_partial is just used when CONFIG_SLUB_CPU_PARTIAL is set,
so wrap it with config CONFIG_SLUB_CPU_PARTIAL will save some space
on 32bit arch.

This patch wrap kmem_cache->cpu_partial in config CONFIG_SLUB_CPU_PARTIAL
and wrap its sysfs too.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
 include/linux/slub_def.h |  2 ++
 mm/slub.c                | 72 +++++++++++++++++++++++++++++-------------------
 2 files changed, 46 insertions(+), 28 deletions(-)

diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
index 0debd8df1a7d..477ab99800ed 100644
--- a/include/linux/slub_def.h
+++ b/include/linux/slub_def.h
@@ -69,7 +69,9 @@ struct kmem_cache {
 	int size;		/* The size of an object including meta data */
 	int object_size;	/* The size of an object without meta data */
 	int offset;		/* Free pointer offset. */
+#ifdef CONFIG_SLUB_CPU_PARTIAL
 	int cpu_partial;	/* Number of per cpu partial objects to keep around */
+#endif
 	struct kmem_cache_order_objects oo;
 
 	/* Allocation and freeing of slabs */
diff --git a/mm/slub.c b/mm/slub.c
index fde499b6dad8..94978f27882a 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1829,7 +1829,10 @@ static void *get_partial_node(struct kmem_cache *s, struct kmem_cache_node *n,
 			stat(s, CPU_PARTIAL_NODE);
 		}
 		if (!kmem_cache_has_cpu_partial(s)
-			|| available > s->cpu_partial / 2)
+#ifdef CONFIG_SLUB_CPU_PARTIAL
+			|| available > s->cpu_partial / 2
+#endif
+			)
 			break;
 
 	}
@@ -3418,6 +3421,39 @@ static void set_min_partial(struct kmem_cache *s, unsigned long min)
 	s->min_partial = min;
 }
 
+static void set_cpu_partial(struct kmem_cache *s)
+{
+#ifdef CONFIG_SLUB_CPU_PARTIAL
+	/*
+	 * cpu_partial determined the maximum number of objects kept in the
+	 * per cpu partial lists of a processor.
+	 *
+	 * Per cpu partial lists mainly contain slabs that just have one
+	 * object freed. If they are used for allocation then they can be
+	 * filled up again with minimal effort. The slab will never hit the
+	 * per node partial lists and therefore no locking will be required.
+	 *
+	 * This setting also determines
+	 *
+	 * A) The number of objects from per cpu partial slabs dumped to the
+	 *    per node list when we reach the limit.
+	 * B) The number of objects in cpu partial slabs to extract from the
+	 *    per node list when we run out of per cpu objects. We only fetch
+	 *    50% to keep some capacity around for frees.
+	 */
+	if (!kmem_cache_has_cpu_partial(s))
+		s->cpu_partial = 0;
+	else if (s->size >= PAGE_SIZE)
+		s->cpu_partial = 2;
+	else if (s->size >= 1024)
+		s->cpu_partial = 6;
+	else if (s->size >= 256)
+		s->cpu_partial = 13;
+	else
+		s->cpu_partial = 30;
+#endif
+}
+
 /*
  * calculate_sizes() determines the order and the distribution of data within
  * a slab object.
@@ -3576,33 +3612,7 @@ static int kmem_cache_open(struct kmem_cache *s, unsigned long flags)
 	 */
 	set_min_partial(s, ilog2(s->size) / 2);
 
-	/*
-	 * cpu_partial determined the maximum number of objects kept in the
-	 * per cpu partial lists of a processor.
-	 *
-	 * Per cpu partial lists mainly contain slabs that just have one
-	 * object freed. If they are used for allocation then they can be
-	 * filled up again with minimal effort. The slab will never hit the
-	 * per node partial lists and therefore no locking will be required.
-	 *
-	 * This setting also determines
-	 *
-	 * A) The number of objects from per cpu partial slabs dumped to the
-	 *    per node list when we reach the limit.
-	 * B) The number of objects in cpu partial slabs to extract from the
-	 *    per node list when we run out of per cpu objects. We only fetch
-	 *    50% to keep some capacity around for frees.
-	 */
-	if (!kmem_cache_has_cpu_partial(s))
-		s->cpu_partial = 0;
-	else if (s->size >= PAGE_SIZE)
-		s->cpu_partial = 2;
-	else if (s->size >= 1024)
-		s->cpu_partial = 6;
-	else if (s->size >= 256)
-		s->cpu_partial = 13;
-	else
-		s->cpu_partial = 30;
+	set_cpu_partial(s);
 
 #ifdef CONFIG_NUMA
 	s->remote_node_defrag_ratio = 1000;
@@ -3989,7 +3999,9 @@ void __kmemcg_cache_deactivate(struct kmem_cache *s)
 	 * Disable empty slabs caching. Used to avoid pinning offline
 	 * memory cgroups by kmem pages that can be freed.
 	 */
+#ifdef CONFIG_SLUB_CPU_PARTIAL
 	s->cpu_partial = 0;
+#endif
 	s->min_partial = 0;
 
 	/*
@@ -4929,6 +4941,7 @@ static ssize_t min_partial_store(struct kmem_cache *s, const char *buf,
 }
 SLAB_ATTR(min_partial);
 
+#ifdef CONFIG_SLUB_CPU_PARTIAL
 static ssize_t cpu_partial_show(struct kmem_cache *s, char *buf)
 {
 	return sprintf(buf, "%u\n", s->cpu_partial);
@@ -4951,6 +4964,7 @@ static ssize_t cpu_partial_store(struct kmem_cache *s, const char *buf,
 	return length;
 }
 SLAB_ATTR(cpu_partial);
+#endif
 
 static ssize_t ctor_show(struct kmem_cache *s, char *buf)
 {
@@ -5363,7 +5377,9 @@ static struct attribute *slab_attrs[] = {
 	&objs_per_slab_attr.attr,
 	&order_attr.attr,
 	&min_partial_attr.attr,
+#ifdef CONFIG_SLUB_CPU_PARTIAL
 	&cpu_partial_attr.attr,
+#endif
 	&objects_attr.attr,
 	&objects_partial_attr.attr,
 	&partial_attr.attr,
-- 
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 related

* [PATCH 2/3] mm/slub: wrap cpu_slab->partial in CONFIG_SLUB_CPU_PARTIAL
From: Wei Yang @ 2017-04-30 11:31 UTC (permalink / raw)
  To: cl, penberg, rientjes, iamjoonsoo.kim, akpm
  Cc: linux-mm, linux-kernel, Wei Yang
In-Reply-To: <20170430113152.6590-1-richard.weiyang@gmail.com>

cpu_slab's field partial is used when CONFIG_SLUB_CPU_PARTIAL is set, which
means we can save a pointer's space on each cpu for every slub item.

This patch wrap cpu_slab->partial in CONFIG_SLUB_CPU_PARTIAL and wrap its
sysfs too.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
 include/linux/slub_def.h |  2 ++
 mm/slub.c                | 16 +++++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
index ec13aab32647..0debd8df1a7d 100644
--- a/include/linux/slub_def.h
+++ b/include/linux/slub_def.h
@@ -41,7 +41,9 @@ struct kmem_cache_cpu {
 	void **freelist;	/* Pointer to next available object */
 	unsigned long tid;	/* Globally unique transaction id */
 	struct page *page;	/* The slab from which we are allocating */
+#ifdef CONFIG_SLUB_CPU_PARTIAL
 	struct page *partial;	/* Partially allocated frozen slabs */
+#endif
 #ifdef CONFIG_SLUB_STATS
 	unsigned stat[NR_SLUB_STAT_ITEMS];
 #endif
diff --git a/mm/slub.c b/mm/slub.c
index 7f4bc7027ed5..fde499b6dad8 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2302,7 +2302,11 @@ static bool has_cpu_slab(int cpu, void *info)
 	struct kmem_cache *s = info;
 	struct kmem_cache_cpu *c = per_cpu_ptr(s->cpu_slab, cpu);
 
-	return c->page || c->partial;
+	return c->page
+#ifdef CONFIG_SLUB_CPU_PARTIAL
+		|| c->partial
+#endif
+		;
 }
 
 static void flush_all(struct kmem_cache *s)
@@ -2511,7 +2515,9 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
 	page = c->page;
 	if (!page)
 		goto new_slab;
+#ifdef CONFIG_SLUB_CPU_PARTIAL
 redo:
+#endif
 
 	if (unlikely(!node_match(page, node))) {
 		int searchnode = node;
@@ -2568,6 +2574,7 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
 
 new_slab:
 
+#ifdef CONFIG_SLUB_CPU_PARTIAL
 	if (c->partial) {
 		page = c->page = c->partial;
 		c->partial = page->next;
@@ -2575,6 +2582,7 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
 		c->freelist = NULL;
 		goto redo;
 	}
+#endif
 
 	freelist = new_slab_objects(s, gfpflags, node, &c);
 
@@ -4760,6 +4768,7 @@ static ssize_t show_slab_objects(struct kmem_cache *s,
 			total += x;
 			nodes[node] += x;
 
+#ifdef CONFIG_SLUB_CPU_PARTIAL
 			page = READ_ONCE(c->partial);
 			if (page) {
 				node = page_to_nid(page);
@@ -4772,6 +4781,7 @@ static ssize_t show_slab_objects(struct kmem_cache *s,
 				total += x;
 				nodes[node] += x;
 			}
+#endif
 		}
 	}
 
@@ -4980,6 +4990,7 @@ static ssize_t objects_partial_show(struct kmem_cache *s, char *buf)
 }
 SLAB_ATTR_RO(objects_partial);
 
+#ifdef CONFIG_SLUB_CPU_PARTIAL
 static ssize_t slabs_cpu_partial_show(struct kmem_cache *s, char *buf)
 {
 	int objects = 0;
@@ -5010,6 +5021,7 @@ static ssize_t slabs_cpu_partial_show(struct kmem_cache *s, char *buf)
 	return len + sprintf(buf + len, "\n");
 }
 SLAB_ATTR_RO(slabs_cpu_partial);
+#endif
 
 static ssize_t reclaim_account_show(struct kmem_cache *s, char *buf)
 {
@@ -5364,7 +5376,9 @@ static struct attribute *slab_attrs[] = {
 	&destroy_by_rcu_attr.attr,
 	&shrink_attr.attr,
 	&reserved_attr.attr,
+#ifdef CONFIG_SLUB_CPU_PARTIAL
 	&slabs_cpu_partial_attr.attr,
+#endif
 #ifdef CONFIG_SLUB_DEBUG
 	&total_objects_attr.attr,
 	&slabs_attr.attr,
-- 
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 related

* [PATCH 1/3] mm/slub: pack red_left_pad with another int to save a word
From: Wei Yang @ 2017-04-30 11:31 UTC (permalink / raw)
  To: cl, penberg, rientjes, iamjoonsoo.kim, akpm
  Cc: linux-mm, linux-kernel, Wei Yang
In-Reply-To: <20170430113152.6590-1-richard.weiyang@gmail.com>

On 64bit arch, struct is 8-bytes aligned, so int will occupy a word if it
doesn't sits well.

This patch pack red_left_pad with reserved to save 8 bytes for struct
kmem_cache on a 64bit arch.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
 include/linux/slub_def.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
index 07ef550c6627..ec13aab32647 100644
--- a/include/linux/slub_def.h
+++ b/include/linux/slub_def.h
@@ -79,9 +79,9 @@ struct kmem_cache {
 	int inuse;		/* Offset to metadata */
 	int align;		/* Alignment */
 	int reserved;		/* Reserved bytes at the end of slabs */
+	int red_left_pad;	/* Left redzone padding size */
 	const char *name;	/* Name (only for display!) */
 	struct list_head list;	/* List of slab caches */
-	int red_left_pad;	/* Left redzone padding size */
 #ifdef CONFIG_SYSFS
 	struct kobject kobj;	/* For sysfs */
 #endif
-- 
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 related

* [PATCH 0/3] try to save some memory for kmem_cache in some cases
From: Wei Yang @ 2017-04-30 11:31 UTC (permalink / raw)
  To: cl, penberg, rientjes, iamjoonsoo.kim, akpm
  Cc: linux-mm, linux-kernel, Wei Yang

kmem_cache is a frequently used data in kernel. During the code reading, I
found maybe we could save some space in some cases.

1. On 64bit arch, type int will occupy a word if it doesn't sit well.
2. cpu_slab->partial is just used when CONFIG_SLUB_CPU_PARTIAL is set
3. cpu_partial is just used when CONFIG_SLUB_CPU_PARTIAL is set, while just
save some space on 32bit arch.

Wei Yang (3):
  mm/slub: pack red_left_pad with another int to save a word
  mm/slub: wrap cpu_slab->partial in CONFIG_SLUB_CPU_PARTIAL
  mm/slub: wrap kmem_cache->cpu_partial in config
    CONFIG_SLUB_CPU_PARTIAL

 include/linux/slub_def.h |  6 +++-
 mm/slub.c                | 88 ++++++++++++++++++++++++++++++++----------------
 2 files changed, 64 insertions(+), 30 deletions(-)

-- 
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: 4.11.0-rc8+/x86_64 desktop lockup until applications closed
From: Arthur Marsh @ 2017-04-30  6:03 UTC (permalink / raw)
  To: Michal Hocko; +Cc: Linux Kernel Mailing List, linux-mm
In-Reply-To: <20170427092636.GD4706@dhcp22.suse.cz>



Michal Hocko wrote on 27/04/17 18:56:
> On Thu 27-04-17 18:36:38, Arthur Marsh wrote:
> [...]
>> [55363.482931] QXcbEventReader: page allocation stalls for 10048ms, order:0,
>> mode:0x14200ca(GFP_HIGHUSER_MOVABLE), nodemask=(null)
>
> Are there more of these stalls?

I haven't seen the same kinds of logging in dmesg, but a few minutes ago 
I did see that the desktop had locked up and after remotely logging in 
and doing a kill -HUP of iceweasel/firefox, saw this:

[92311.944443] swap_info_get: Bad swap offset entry 000ffffd
[92311.944449] swap_info_get: Bad swap offset entry 000ffffe
[92311.944451] swap_info_get: Bad swap offset entry 000fffff

I've since restarted that machine, but should it happen again I'd be 
happy to run further tests.

Arthur.

--
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: Matthew Wilcox @ 2017-04-30  5:05 UTC (permalink / raw)
  To: Laurent Dufour
  Cc: paulmck, 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:
> +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;
> +}

Hang on, this isn't vma_is_dead().  This is vma_has_changed() (possibly
from live to dead, but also possibly grown or shrunk; see your earlier
patch).

--
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: Matthew Wilcox @ 2017-04-30  4:57 UTC (permalink / raw)
  To: Laurent Dufour
  Cc: paulmck, 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:
> @@ -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 {

It doesn't look like we examine the contents of the VMA until after we've
checked that the seqlock is good, so we should be able to union virtually
any entry in the VMA with the vm_rcu_head.  vm_next, vm_prev, perhaps?
Or anon_vma_chain since a list_head is the same size as an rcu_head.

--
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 03/17] mm: Introduce pte_spinlock
From: Matthew Wilcox @ 2017-04-30  4:47 UTC (permalink / raw)
  To: Laurent Dufour
  Cc: paulmck, peterz, akpm, kirill, ak, mhocko, dave, jack,
	linux-kernel, linux-mm, haren, khandual, npiggin, bsingharora
In-Reply-To: <1493308376-23851-4-git-send-email-ldufour@linux.vnet.ibm.com>

On Thu, Apr 27, 2017 at 05:52:42PM +0200, Laurent Dufour wrote:
> +++ b/mm/memory.c
> @@ -2100,6 +2100,13 @@ static inline void wp_page_reuse(struct vm_fault *vmf)
>  	pte_unmap_unlock(vmf->pte, vmf->ptl);
>  }
>  
> +static bool pte_spinlock(struct vm_fault *vmf)
> +{
> +	vmf->ptl = pte_lockptr(vmf->vma->vm_mm, vmf->pmd);
> +	spin_lock(vmf->ptl);
> +	return true;
> +}

To me 'pte_spinlock' is a noun, but this is really pte_spin_lock() (a verb).

Actually, it's really vmf_lock_pte().  We're locking the pte
referred to by this vmf.  And so we should probably have a matching
vmf_unlock_pte(vmf) to preserve the abstraction.

--
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] mm, zone_device: replace {get, put}_zone_device_page() with a single reference
From: Ingo Molnar @ 2017-04-29 14:18 UTC (permalink / raw)
  To: Dan Williams
  Cc: linux-kernel, linux-mm, Jérôme Glisse, Ingo Molnar,
	Andrew Morton, Logan Gunthorpe, Kirill Shutemov
In-Reply-To: <149339998297.24933.1129582806028305912.stgit@dwillia2-desk3.amr.corp.intel.com>


* Dan Williams <dan.j.williams@intel.com> 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: Andrew Morton <akpm@linux-foundation.org>
> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
> Suggested-by: Kirill Shutemov <kirill.shutemov@linux.intel.com>
> Tested-by: Kirill Shutemov <kirill.shutemov@linux.intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

This changelog is lacking an explanation about how this solves the crashes you 
were seeing.

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] mm, zone_device: replace {get, put}_zone_device_page() with a single reference
From: Kirill A. Shutemov @ 2017-04-29 10:17 UTC (permalink / raw)
  To: Jerome Glisse
  Cc: Dan Williams, Ingo Molnar, linux-kernel@vger.kernel.org, Linux MM,
	Ingo Molnar, Andrew Morton, Logan Gunthorpe, Kirill Shutemov
In-Reply-To: <20170428193305.GA3912@redhat.com>

On Fri, Apr 28, 2017 at 03:33:07PM -0400, Jerome Glisse wrote:
> On Fri, Apr 28, 2017 at 12:22:24PM -0700, Dan Williams wrote:
> > Are you sure about needing to hook the 2 -> 1 transition? Could we
> > change ZONE_DEVICE pages to not have an elevated reference count when
> > they are created so you can keep the HMM references out of the mm hot
> > path?
> 
> 100% sure on that :) I need to callback into driver for 2->1 transition
> no way around that. If we change ZONE_DEVICE to not have an elevated
> reference count that you need to make a lot more change to mm so that
> ZONE_DEVICE is never use as fallback for memory allocation. Also need
> to make change to be sure that ZONE_DEVICE page never endup in one of
> the path that try to put them back on lru. There is a lot of place that
> would need to be updated and it would be highly intrusive and add a
> lot of special cases to other hot code path.

Could you explain more on where the requirement comes from or point me to
where I can read about this.

-- 
 Kirill A. Shutemov

--
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 2/2] mm/memcontrol: inc reclaim gen if restarting walk in mem_cgroup_iter()
From: Sean Christopherson @ 2017-04-28 21:55 UTC (permalink / raw)
  To: mhocko; +Cc: hannes, vdavydov.dev, cgroups, linux-mm, sean.j.christopherson
In-Reply-To: <1493416547-19212-1-git-send-email-sean.j.christopherson@intel.com>

Increment iter->generation if a reclaimer reaches the end of the tree,
even if it restarts the hierarchy walk instead of returning NULL, i.e.
this is the reclaimer's initial call to mem_cgroup_iter().  If we don't
increment the generation, other threads that are part of the current
reclaim generation will incorrectly continue to walk the tree since
iter->generation won't be updated until one of the reclaimers reaches
the end of the hierarchy a second time.

Move the put_css(&pos->css) call below the iter->generation update
to minimize the window where a thread can see a stale generation but
consume an updated position, as iter->generation and iter->position
are not updated atomically.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 mm/memcontrol.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 6a7ca3c..b858245 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -740,6 +740,7 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
 	struct cgroup_subsys_state *css = NULL;
 	struct mem_cgroup *memcg = NULL;
 	struct mem_cgroup *pos = NULL;
+	bool inc_gen = false;
 
 	if (mem_cgroup_disabled())
 		return NULL;
@@ -791,6 +792,14 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
 		css = css_next_descendant_pre(css, &root->css);
 		if (!css) {
 			/*
+			 * Increment the generation as the next call to
+			 * css_next_descendant_pre will restart at root.
+			 * Do not update iter->generation directly as we
+			 * should only do so if we update iter->position.
+			 */
+			inc_gen = true;
+
+			/*
 			 * Reclaimers share the hierarchy walk, and a
 			 * new one might jump in right at the end of
 			 * the hierarchy - make sure they see at least
@@ -838,16 +847,28 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
 				css_put(&pos->css);
 			css = NULL;
 			memcg = NULL;
+			inc_gen = false;
 			goto start;
 		}
 
-		if (pos)
-			css_put(&pos->css);
-
-		if (!memcg)
+		/*
+		 * Update iter->generation asap to minimize the window where
+		 * a different thread compares against a stale generation but
+		 * consumes an updated position.
+		 */
+		if (inc_gen)
 			iter->generation++;
-		else if (!prev)
+
+		/*
+		 * Initialize the reclaimer's generation after the potential
+		 * update to iter->generation; if we restarted the hierarchy
+		 * walk then we are part of the new generation.
+		 */
+		if (!prev)
 			reclaim->generation = iter->generation;
+
+		if (pos)
+			css_put(&pos->css);
 	}
 
 out_unlock:
-- 
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 related

* [PATCH 1/2] mm/memcontrol: check cmpxchg(iter->pos...) result in mem_cgroup_iter()
From: Sean Christopherson @ 2017-04-28 21:55 UTC (permalink / raw)
  To: mhocko; +Cc: hannes, vdavydov.dev, cgroups, linux-mm, sean.j.christopherson
In-Reply-To: <1493416547-19212-1-git-send-email-sean.j.christopherson@intel.com>

Check the return value of cmpxchg when updating iter->position in
mem_cgroup_iter().  If cmpxchg failed, i.e. a different thread won
the race to update iter->position, then restart the entire flow of
reading, processing and updating iter->position.  Simply ensuring
that there aren't multiple writes to iter->position doesn't avoid
redundant reclaims of a memcg, as competing threads will compute
the same memcg given the same iter->position.

The cmpxchg will only fail if a different thread saw the same value
of iter->position, meaning it called css_next_descendant_pre() with
the same css and therefore computed the same memcg (ignoring the
corner case where the threads see different versions of the tree).

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 mm/memcontrol.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 16c556a..6a7ca3c 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -758,6 +758,7 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
 
 	rcu_read_lock();
 
+start:
 	if (reclaim) {
 		struct mem_cgroup_per_node *mz;
 
@@ -818,11 +819,27 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
 
 	if (reclaim) {
 		/*
-		 * The position could have already been updated by a competing
-		 * thread, so check that the value hasn't changed since we read
-		 * it to avoid reclaiming from the same cgroup twice.
+		 * Competing reclaim threads may attempt to consume the same
+		 * iter->position, check that the value hasn't changed since
+		 * we read it to avoid reclaiming from the same cgroup twice.
+		 * Note that just avoiding multiple writes to iter->position
+		 * does not prevent redundant reclaims to memcg.  Given the
+		 * same input css on competing threads, the css returned by
+		 * css_next_descendant_pre will also be the same (unless the
+		 * tree itself changes).  So, if a different thread read the
+		 * same iter->position, then it also computed the same memcg.
+		 * If we lost the race, put our css references and restart
+		 * the entire process of reading and updating iter->position.
 		 */
-		(void)cmpxchg(&iter->position, pos, memcg);
+		if (cmpxchg(&iter->position, pos, memcg) != pos) {
+			if (memcg && memcg != root)
+				css_put(&memcg->css);
+			if (pos)
+				css_put(&pos->css);
+			css = NULL;
+			memcg = NULL;
+			goto start;
+		}
 
 		if (pos)
 			css_put(&pos->css);
-- 
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 related

* [PATCH 0/2] mm/memcontrol: fix reclaim bugs in mem_cgroup_iter
From: Sean Christopherson @ 2017-04-28 21:55 UTC (permalink / raw)
  To: mhocko; +Cc: hannes, vdavydov.dev, cgroups, linux-mm, sean.j.christopherson

This patch set contains two bug fixes for mem_cgroup_iter().  The bugs
were found by code inspection and were confirmed via synthetic testing
that forcefully setup the failing conditions.

Bug #1 is a race condition where mem_cgroup_iter() incorrectly returns
the same memcg to multiple threads reclaiming from the same root, zone,
priority and generation.  mem_cgroup_iter() doesn't check the result of
cmpxchg(iter->pos...) when setting the new pos, and so fails to detect
that it will return the same memcg as the thread that successfully set
iter->position.  If multiple threads read the same iter->position value,
then they will call css_next_descendant_pre() with the same css and will
compute the same memcg (unless they see different versions of the tree
due to an RCU update).

Bug #2 is also a race condition of sorts, with the same setup conditions
as bug #1.  If a reclaimer's initial call to mem_cgroup_iter() triggers
a restart of the hierarchy walk, i.e. css_next_descendant_pre() returns
NULL and prev == NULL, mem_cgroup_iter() fails to increment iter->gen...
even though it has started a new walk of the hierarchy.  This technically
isn't a bug for the thread that triggered the restart as it's reasonable
for that thread to perform a full walk of the tree, but other threads
in the current reclaim generation will incorrectly continue to walk the
tree since iter->generation won't be updated until one of the reclaimers
reaches the end of the hierarchy a second time.

The two patches can be applied independently, but I included them in a
single series as the fix for bug #1 can theoretically exacerbate bug #2,
and bug #2 is likely more serious as it results in a duplicate walk of
the entire tree as opposed to a duplicate reclaim of a single memcg.


Sean Christopherson (2):
  mm/memcontrol: check cmpxchg(iter->pos...) result in mem_cgroup_iter()
  mm/memcontrol: inc reclaim gen if restarting walk in mem_cgroup_iter()

 mm/memcontrol.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 47 insertions(+), 9 deletions(-)


--
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] mm, zone_device: replace {get, put}_zone_device_page() with a single reference
From: Jerome Glisse @ 2017-04-28 19:33 UTC (permalink / raw)
  To: Dan Williams
  Cc: Ingo Molnar, linux-kernel@vger.kernel.org, Linux MM, Ingo Molnar,
	Andrew Morton, Logan Gunthorpe, Kirill Shutemov
In-Reply-To: <CAPcyv4i+iPm=hBviOYABaroz_JJYVy8Qja8Ka=-_uAQNnGjpeg@mail.gmail.com>

On Fri, Apr 28, 2017 at 12:22:24PM -0700, Dan Williams wrote:
> On Fri, Apr 28, 2017 at 12:16 PM, Jerome Glisse <jglisse@redhat.com> wrote:
> >> On Fri, Apr 28, 2017 at 11:00 AM, Jerome Glisse <jglisse@redhat.com> wrote:
> >> >> On Fri, Apr 28, 2017 at 10:34 AM, Jerome Glisse <jglisse@redhat.com>
> >> >> 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.
> >> >> >
> >> >> > This is NAK from HMM point of view as i need those call. So if you
> >> >> > remove
> >> >> > them now i will need to add them back as part of HMM.
> >> >>
> >> >> I thought you only need them at page free time? You can still hook
> >> >> __put_page().
> >> >
> >> > No, i need a hook when page refcount reach 1, not 0. That being said
> >> > i don't care about put_dev_pagemap(page->pgmap); so that part of the
> >> > patch is fine from HMM point of view but i definitly need to hook my-
> >> > self in the general put_page() function.
> >> >
> >> > So i will have to undo part of this patch for HMM (put_page() will
> >> > need to handle ZONE_DEVICE page differently).
> >>
> >> Ok, I'd rather this go in now since it fixes the existing use case,
> >> and unblocks the get_user_pages_fast() conversion to generic code.
> >> That also gives Kirill and -mm folks a chance to review what HMM wants
> >> to do on top of the page_ref infrastructure.  The
> >> {get,put}_zone_device_page interface went in in 4.5 right before
> >> page_ref went in during 4.6, so it was just an oversight that
> >> {get,put}_zone_device_page were not removed earlier.
> >>
> >
> > I don't mind this going in, i am hopping people won't ignore HMM patchset
> > once i repost after 4.12 merge window. Note that there is absolutely no way
> > around me hooking up inside put_page(). The only other way to do it would
> > be to modify virtualy all places that call that function to handle ZONE_DEVICE
> > case.
> 
> Are you sure about needing to hook the 2 -> 1 transition? Could we
> change ZONE_DEVICE pages to not have an elevated reference count when
> they are created so you can keep the HMM references out of the mm hot
> path?

100% sure on that :) I need to callback into driver for 2->1 transition
no way around that. If we change ZONE_DEVICE to not have an elevated
reference count that you need to make a lot more change to mm so that
ZONE_DEVICE is never use as fallback for memory allocation. Also need
to make change to be sure that ZONE_DEVICE page never endup in one of
the path that try to put them back on lru. There is a lot of place that
would need to be updated and it would be highly intrusive and add a
lot of special cases to other hot code path.

Maybe i over estimate the amount of work but from top of my head it is
far from being trivial.

Jerome

--
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] mm, zone_device: replace {get, put}_zone_device_page() with a single reference
From: Dan Williams @ 2017-04-28 19:22 UTC (permalink / raw)
  To: Jerome Glisse
  Cc: Ingo Molnar, linux-kernel@vger.kernel.org, Linux MM, Ingo Molnar,
	Andrew Morton, Logan Gunthorpe, Kirill Shutemov
In-Reply-To: <1295710462.4327805.1493406971970.JavaMail.zimbra@redhat.com>

On Fri, Apr 28, 2017 at 12:16 PM, Jerome Glisse <jglisse@redhat.com> wrote:
>> On Fri, Apr 28, 2017 at 11:00 AM, Jerome Glisse <jglisse@redhat.com> wrote:
>> >> On Fri, Apr 28, 2017 at 10:34 AM, Jerome Glisse <jglisse@redhat.com>
>> >> 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.
>> >> >
>> >> > This is NAK from HMM point of view as i need those call. So if you
>> >> > remove
>> >> > them now i will need to add them back as part of HMM.
>> >>
>> >> I thought you only need them at page free time? You can still hook
>> >> __put_page().
>> >
>> > No, i need a hook when page refcount reach 1, not 0. That being said
>> > i don't care about put_dev_pagemap(page->pgmap); so that part of the
>> > patch is fine from HMM point of view but i definitly need to hook my-
>> > self in the general put_page() function.
>> >
>> > So i will have to undo part of this patch for HMM (put_page() will
>> > need to handle ZONE_DEVICE page differently).
>>
>> Ok, I'd rather this go in now since it fixes the existing use case,
>> and unblocks the get_user_pages_fast() conversion to generic code.
>> That also gives Kirill and -mm folks a chance to review what HMM wants
>> to do on top of the page_ref infrastructure.  The
>> {get,put}_zone_device_page interface went in in 4.5 right before
>> page_ref went in during 4.6, so it was just an oversight that
>> {get,put}_zone_device_page were not removed earlier.
>>
>
> I don't mind this going in, i am hopping people won't ignore HMM patchset
> once i repost after 4.12 merge window. Note that there is absolutely no way
> around me hooking up inside put_page(). The only other way to do it would
> be to modify virtualy all places that call that function to handle ZONE_DEVICE
> case.

Are you sure about needing to hook the 2 -> 1 transition? Could we
change ZONE_DEVICE pages to not have an elevated reference count when
they are created so you can keep the HMM references out of the mm hot
path?

--
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] mm, zone_device: replace {get, put}_zone_device_page() with a single reference
From: Jerome Glisse @ 2017-04-28 19:16 UTC (permalink / raw)
  To: Dan Williams
  Cc: Ingo Molnar, linux-kernel, Linux MM, Ingo Molnar, Andrew Morton,
	Logan Gunthorpe, Kirill Shutemov
In-Reply-To: <CAPcyv4hvBKG8t3e3QvUnmkaopeM8eTniz5JPVkrZ5Puu5eaViw@mail.gmail.com>

> On Fri, Apr 28, 2017 at 11:00 AM, Jerome Glisse <jglisse@redhat.com> wrote:
> >> On Fri, Apr 28, 2017 at 10:34 AM, Jerome Glisse <jglisse@redhat.com>
> >> 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.
> >> >
> >> > This is NAK from HMM point of view as i need those call. So if you
> >> > remove
> >> > them now i will need to add them back as part of HMM.
> >>
> >> I thought you only need them at page free time? You can still hook
> >> __put_page().
> >
> > No, i need a hook when page refcount reach 1, not 0. That being said
> > i don't care about put_dev_pagemap(page->pgmap); so that part of the
> > patch is fine from HMM point of view but i definitly need to hook my-
> > self in the general put_page() function.
> >
> > So i will have to undo part of this patch for HMM (put_page() will
> > need to handle ZONE_DEVICE page differently).
> 
> Ok, I'd rather this go in now since it fixes the existing use case,
> and unblocks the get_user_pages_fast() conversion to generic code.
> That also gives Kirill and -mm folks a chance to review what HMM wants
> to do on top of the page_ref infrastructure.  The
> {get,put}_zone_device_page interface went in in 4.5 right before
> page_ref went in during 4.6, so it was just an oversight that
> {get,put}_zone_device_page were not removed earlier.
> 

I don't mind this going in, i am hopping people won't ignore HMM patchset
once i repost after 4.12 merge window. Note that there is absolutely no way
around me hooking up inside put_page(). The only other way to do it would
be to modify virtualy all places that call that function to handle ZONE_DEVICE
case.

Jérôme

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