* 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: Igor Stoppa @ 2017-04-28 7:43 UTC (permalink / raw)
To: Michal Hocko; +Cc: namhyung, linux-mm, linux-kernel
In-Reply-To: <20170428074028.GF8143@dhcp22.suse.cz>
On 28/04/17 10:40, Michal Hocko wrote:
> 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.
I'm writing an alternative different proposal, let's call it last attempt.
Should be ready in a few minutes.
thanks, igor
--
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: post-init-read-only protection for data allocated dynamically
From: Michal Hocko @ 2017-04-28 7:45 UTC (permalink / raw)
To: Igor Stoppa; +Cc: linux-mm
In-Reply-To: <3eba3df7-6694-5c47-48f4-30088845035b@huawei.com>
On Fri 21-04-17 11:30:04, Igor Stoppa wrote:
> Hello,
>
> I am looking for a mechanism to protect the kernel data which is allocated
> dynamically during system initialization and is later-on accessed only for
> reads.
>
> The functionality would be, in spirit, like the __read_only modifier, which
> can be used to mark static data as read-only, in the post-init phase. Only,
> it would apply to dynamically allocated data.
>
> I couldn't find any such feature (did I miss it?), so I started looking at
> what could be the best way to introduce it.
>
> The static post-init write protection is achieved by placing all the data
> into a page-aligned segment and then protecting the page from writes, using
> the MMU, once the data is in its final state.
>
> In my case, as example, I want to protect the SE Linux policy database,
> after the set of policy has been loaded from file.
> SE Linux uses fairly complex data structures, which are allocated
> dynamically, depending on what rules/policy are loaded into it.
>
> If I knew upfront, roughly, which sizes will be requested and how many
> requests will happen, for each size, I could use multiple pools of objects.
> However, I cannot assume upfront to know these parameters, because it's very
> likely that the set of policies & rules will evolve.
>
> I would also like to extend the write protection to other data structures,
> which means I would probably end up writing another memory allocator, if I
> started to generate on-demand object pools.
What is the expected life time of those objects? Are they ever freed? If
yes are they freed at once or some might outlive others?
> The alternative I'm considering is that, if I were to add a new memory zone
> (let's call it LOCKABLE), I could piggy back on the existing infrastructure
> for memory allocation.
No, please no new memory zones! This doesn't look like a good fit
anyway. I believe you need an allocator on top of the page allocator
which manages kernel page tables on top of pools of pages. You really do
not care about where the page is placed physically. I am not sure how
much you can reuse from the SL.B object management because that highly
depends on the life time of objects.
--
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 -v3] mm, swap: Sort swap entries before free
From: Huang, Ying @ 2017-04-28 8:05 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: <20170428074257.GA19510@bbox>
Minchan Kim <minchan@kernel.org> writes:
> 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.
Tim suggested to add a mechanism to correctly track how many swap
devices are in use in swapon/swapoff. So we only sort if the number of
the swap device > 1. This will not cover multiple swap devices with
different priorities, but will cover the major usecases. The code
should be simpler.
Best Regards,
Huang, Ying
--
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
* Generic approach to customizable zones - was: Re: [PATCH v7 0/7] Introduce ZONE_CMA
From: Igor Stoppa @ 2017-04-28 8:04 UTC (permalink / raw)
To: Michal Hocko, Joonsoo Kim
Cc: Andrew Morton, Rik van Riel, Johannes Weiner, mgorman,
Laura Abbott, Minchan Kim, Marek Szyprowski, Michal Nazarewicz,
Aneesh Kumar K . V, Vlastimil Babka, Russell King, Will Deacon,
linux-mm, linux-kernel, kernel-team
In-Reply-To: <20170427150636.GM4706@dhcp22.suse.cz>
On 27/04/17 18:06, Michal Hocko wrote:
> On Tue 25-04-17 12:42:57, Joonsoo Kim wrote:
[...]
>> Yes, it requires one more bit for a new zone and it's handled by the patch.
>
> I am pretty sure that you are aware that consuming new page flag bits
> is usually a no-go and something we try to avoid as much as possible
> because we are in a great shortage there. So there really have to be a
> _strong_ reason if we go that way. My current understanding that the
> whole zone concept is more about a more convenient implementation rather
> than a fundamental change which will solve unsolvable problems with the
> current approach. More on that below.
Since I am in a similar situation, I think it's better if I join this
conversation instead of going through the same in a separate thread.
In this regard, I have a few observations (are they correct?):
* not everyone seems to be interested in having all the current
zones active simultaneously
* some zones are even not so meaningful on certain architectures or
platforms
* some architectures/platforms that are 64 bits would have no penalty
in dealing with a larger data type.
So I wonder, would anybody be against this:
* within the 32bits constraint, define some optional zones
* decouple the specific position of a bit from the zone it represents;
iow: if the zone is enabled, ensure that it gets a bit in the mask,
but do not make promises about which one it is, provided that the
corresponding macros work properly
* ensure that if one selects more optional zones than there are bits
available (in the case of a 32bits mask), an error is produced at
compile time
* if one is happy to have a 64bits type, allow for as many zones as
it's possible to fit, or anyway more than what is possible with
the 32 bit mask.
I think I can re-factor the code so that there is no runtime performance
degradation, if there is no immediate objection to what I described. Or
maybe I failed to notice some obvious pitfall?
>From what I see, there seems to be a lot of interest in using functions
like Kmalloc / vmalloc, with the ability of specifying pseudo-custom
areas from where they should tap into.
Why not, as long as those who do not need it are not negatively impacted?
I understand that if the association between bits and zones is fixed,
then suddenly bits become very precious stuff, but if they could be used
in a more efficient way, then maybe they could be used more liberally.
The alternative is to keep getting requests about new zones and turning
them away because they do not pass the bar of being extremely critical,
even if indeed they would simplify people's life.
The change shouldn't be too ugly, if I do something along these lines of
the pseudo code below.
Note: the #ifdefs would be mainly concentrated in the declaration part.
enum gfp_zone_shift {
#if IS_ENABLED(CONFIG_ZONE_DMA)
/*I haven't checked if this is the correct name, but it gives the idea*/
ZONE_DMA_SHIFT = 0,
#endif
#if IS_ENABLED(CONFIG_ZONE_HIGHMEM)
ZONE_HIGHMEM_SHIFT,
#endif
#if IS_ENABLED(CONFIG_ZONE_DMA32)
ZONE_DMA32_SHIFT,
#endif
#if IS_ENABLED(CONFIG_ZONE_xxx)
ZONE_xxx,
#endif
NON_OPTIONAL_ZONE_SHIFT,
...
USED_ZONES_NUMBER,
ZONE_MOVABLE_SHIFT = USED_ZONES_NUMBER,
...
};
#if USED_ZONES_NUMBER < MAX_ZONES_32BITS
typedef gfp_zones_t uint32_t
#elif IS_ENABLED(CONFIG_ZONES_64BITS
typedef gfp_zones_t uint64_t
#else
#error
#endif
The type should be adjusted in other places where it is used, but I
didn't find too many occurrences.
#define __ZONE_DMA \
(((gfp_zones_t)IS_ENABLED(CONFIG_ZONE_DMA)) << \
(ZONE_DMA_SHIFT - 0))
[rinse and repeat]
Code referring to these optional zones can be sandboxed in
#if IS_ENABLED(CONFIG_ZONE_DMA)
inline function do_something_dma() {
....
}
#else
#define do_something_dma()
#endif
Or equivalent, effectively removing many #ifdefs from the main code of
functions like those called by kmalloc.
So, would this approach stand a chance?
thanks, igor
--
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] mm, zone_device: Replace {get, put}_zone_device_page() with a single reference
From: Kirill A. Shutemov @ 2017-04-28 8:14 UTC (permalink / raw)
To: Ingo Molnar, akpm
Cc: Dan Williams, Jérôme Glisse, Logan Gunthorpe, linux-mm,
linux-kernel, Kirill A . Shutemov
In-Reply-To: <20170428063913.iz6xjcxblecofjlq@gmail.com>
From: Dan Williams <dan.j.williams@intel.com>
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: JA(C)rA'me 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>
Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
[kirill.shutemov@linux.intel.com: rebased to -tip]
Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
drivers/dax/pmem.c | 2 +-
drivers/nvdimm/pmem.c | 13 +++++++++++--
include/linux/mm.h | 14 --------------
kernel/memremap.c | 22 +++++++++-------------
mm/swap.c | 10 ++++++++++
5 files changed, 31 insertions(+), 30 deletions(-)
diff --git a/drivers/dax/pmem.c b/drivers/dax/pmem.c
index 033f49b31fdc..cb0d742fa23f 100644
--- a/drivers/dax/pmem.c
+++ b/drivers/dax/pmem.c
@@ -43,6 +43,7 @@ static void dax_pmem_percpu_exit(void *data)
struct dax_pmem *dax_pmem = to_dax_pmem(ref);
dev_dbg(dax_pmem->dev, "%s\n", __func__);
+ wait_for_completion(&dax_pmem->cmp);
percpu_ref_exit(ref);
}
@@ -53,7 +54,6 @@ static void dax_pmem_percpu_kill(void *data)
dev_dbg(dax_pmem->dev, "%s\n", __func__);
percpu_ref_kill(ref);
- wait_for_completion(&dax_pmem->cmp);
}
static int dax_pmem_probe(struct device *dev)
diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index 5b536be5a12e..fb7bbc79ac26 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -25,6 +25,7 @@
#include <linux/badblocks.h>
#include <linux/memremap.h>
#include <linux/vmalloc.h>
+#include <linux/blk-mq.h>
#include <linux/pfn_t.h>
#include <linux/slab.h>
#include <linux/pmem.h>
@@ -231,6 +232,11 @@ static void pmem_release_queue(void *q)
blk_cleanup_queue(q);
}
+static void pmem_freeze_queue(void *q)
+{
+ blk_mq_freeze_queue_start(q);
+}
+
static void pmem_release_disk(void *disk)
{
del_gendisk(disk);
@@ -284,6 +290,9 @@ static int pmem_attach_disk(struct device *dev,
if (!q)
return -ENOMEM;
+ if (devm_add_action_or_reset(dev, pmem_release_queue, q))
+ return -ENOMEM;
+
pmem->pfn_flags = PFN_DEV;
if (is_nd_pfn(dev)) {
addr = devm_memremap_pages(dev, &pfn_res, &q->q_usage_counter,
@@ -303,10 +312,10 @@ static int pmem_attach_disk(struct device *dev,
pmem->size, ARCH_MEMREMAP_PMEM);
/*
- * At release time the queue must be dead before
+ * At release time the queue must be frozen before
* devm_memremap_pages is unwound
*/
- if (devm_add_action_or_reset(dev, pmem_release_queue, q))
+ if (devm_add_action_or_reset(dev, pmem_freeze_queue, q))
return -ENOMEM;
if (IS_ERR(addr))
diff --git a/include/linux/mm.h b/include/linux/mm.h
index a835edd2db34..695da2a19b4c 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -762,19 +762,11 @@ static inline enum zone_type page_zonenum(const struct page *page)
}
#ifdef CONFIG_ZONE_DEVICE
-void get_zone_device_page(struct page *page);
-void put_zone_device_page(struct page *page);
static inline bool is_zone_device_page(const struct page *page)
{
return page_zonenum(page) == ZONE_DEVICE;
}
#else
-static inline void get_zone_device_page(struct page *page)
-{
-}
-static inline void put_zone_device_page(struct page *page)
-{
-}
static inline bool is_zone_device_page(const struct page *page)
{
return false;
@@ -790,9 +782,6 @@ static inline void get_page(struct page *page)
*/
VM_BUG_ON_PAGE(page_ref_count(page) <= 0, page);
page_ref_inc(page);
-
- if (unlikely(is_zone_device_page(page)))
- get_zone_device_page(page);
}
static inline void put_page(struct page *page)
@@ -801,9 +790,6 @@ static inline void put_page(struct page *page)
if (put_page_testzero(page))
__put_page(page);
-
- if (unlikely(is_zone_device_page(page)))
- put_zone_device_page(page);
}
#if defined(CONFIG_SPARSEMEM) && !defined(CONFIG_SPARSEMEM_VMEMMAP)
diff --git a/kernel/memremap.c b/kernel/memremap.c
index 07e85e5229da..5316efdde083 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -182,18 +182,6 @@ struct page_map {
struct vmem_altmap altmap;
};
-void get_zone_device_page(struct page *page)
-{
- percpu_ref_get(page->pgmap->ref);
-}
-EXPORT_SYMBOL(get_zone_device_page);
-
-void put_zone_device_page(struct page *page)
-{
- put_dev_pagemap(page->pgmap);
-}
-EXPORT_SYMBOL(put_zone_device_page);
-
static void pgmap_radix_release(struct resource *res)
{
resource_size_t key, align_start, align_size, align_end;
@@ -237,6 +225,10 @@ static void devm_memremap_pages_release(struct device *dev, void *data)
struct resource *res = &page_map->res;
resource_size_t align_start, align_size;
struct dev_pagemap *pgmap = &page_map->pgmap;
+ unsigned long pfn;
+
+ for_each_device_pfn(pfn, page_map)
+ put_page(pfn_to_page(pfn));
if (percpu_ref_tryget_live(pgmap->ref)) {
dev_WARN(dev, "%s: page mapping is still live!\n", __func__);
@@ -277,7 +269,10 @@ struct dev_pagemap *find_dev_pagemap(resource_size_t phys)
*
* Notes:
* 1/ @ref must be 'live' on entry and 'dead' before devm_memunmap_pages() time
- * (or devm release event).
+ * (or devm release event). The expected order of events is that @ref has
+ * been through percpu_ref_kill() before devm_memremap_pages_release(). The
+ * wait for the completion of kill and percpu_ref_exit() must occur after
+ * devm_memremap_pages_release().
*
* 2/ @res is expected to be a host memory range that could feasibly be
* treated as a "System RAM" range, i.e. not a device mmio range, but
@@ -379,6 +374,7 @@ void *devm_memremap_pages(struct device *dev, struct resource *res,
*/
list_del(&page->lru);
page->pgmap = pgmap;
+ percpu_ref_get(ref);
}
devres_add(dev, page_map);
return __va(res->start);
diff --git a/mm/swap.c b/mm/swap.c
index 5dabf444d724..01267dda6668 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -97,6 +97,16 @@ static void __put_compound_page(struct page *page)
void __put_page(struct page *page)
{
+ if (is_zone_device_page(page)) {
+ put_dev_pagemap(page->pgmap);
+
+ /*
+ * The page belong to device, do not return it to
+ * page allocator.
+ */
+ return;
+ }
+
if (unlikely(PageCompound(page)))
__put_compound_page(page);
else
--
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
* Re: [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks
From: Igor Stoppa @ 2017-04-28 8:13 UTC (permalink / raw)
To: Michal Hocko; +Cc: namhyung, linux-mm, linux-kernel
In-Reply-To: <4b077316-b381-08d7-7797-1eaf65d01a02@huawei.com>
On 28/04/17 10:43, Igor Stoppa wrote:
[...]
> I'm writing an alternative different proposal, let's call it last attempt.
>
> Should be ready in a few minutes.
Here: http://marc.info/?l=linux-mm&m=149336675129967&w=2
--
thanks, igor
--
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: Generic approach to customizable zones - was: Re: [PATCH v7 0/7] Introduce ZONE_CMA
From: Michal Hocko @ 2017-04-28 8:36 UTC (permalink / raw)
To: Igor Stoppa
Cc: Joonsoo Kim, Andrew Morton, Rik van Riel, Johannes Weiner,
mgorman, Laura Abbott, Minchan Kim, Marek Szyprowski,
Michal Nazarewicz, Aneesh Kumar K . V, Vlastimil Babka,
Russell King, Will Deacon, linux-mm, linux-kernel, kernel-team
In-Reply-To: <d3c0d01c-ef3f-56f8-2701-a32f8be2d13b@huawei.com>
I didn't read this thoughly yet because I will be travelling shortly but
this point alone just made ask, because it seems there is some
misunderstanding
On Fri 28-04-17 11:04:27, Igor Stoppa wrote:
[...]
> * if one is happy to have a 64bits type, allow for as many zones as
> it's possible to fit, or anyway more than what is possible with
> the 32 bit mask.
zones are currently placed in struct page::flags. And that already is
64b size on 64b arches. And we do not really have any room spare there.
We encode page flags, zone id, numa_nid/sparse section_nr there. How can
you add more without enlarging the struct page itself or using external
means to store the same information (page_ext comes to mind)? Even if
the later would be possible then note thatpage_zone() is used in many
performance sensitive paths and making it perform well with special
casing would be far from trivial.
--
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 -v10 1/3] mm, THP, swap: Delay splitting THP during swap out
From: Minchan Kim @ 2017-04-28 8:40 UTC (permalink / raw)
To: Huang, Ying
Cc: Andrew Morton, linux-mm, linux-kernel, Andrea Arcangeli,
Ebru Akagunduz, Johannes Weiner, Michal Hocko, Tejun Heo,
Hugh Dickins, Shaohua Li, Rik van Riel, cgroups
In-Reply-To: <87mvb21fz1.fsf@yhuang-dev.intel.com>
On Thu, Apr 27, 2017 at 03:12:34PM +0800, Huang, Ying wrote:
> Minchan Kim <minchan@kernel.org> writes:
>
> > On Tue, Apr 25, 2017 at 08:56:56PM +0800, Huang, Ying wrote:
> >> From: Huang Ying <ying.huang@intel.com>
> >>
> >> In this patch, splitting huge page is delayed from almost the first
> >> step of swapping out to after allocating the swap space for the
> >> THP (Transparent Huge Page) and adding the THP into the swap cache.
> >> This will batch the corresponding operation, thus improve THP swap out
> >> throughput.
> >>
> >> This is the first step for the THP swap optimization. The plan is to
> >> delay splitting the THP step by step and avoid splitting the THP
> >> finally.
> >>
> >> The advantages of the THP swap support include:
> >>
> >> - Batch the swap operations for the THP and reduce lock
> >> acquiring/releasing, including allocating/freeing the swap space,
> >> adding/deleting to/from the swap cache, and writing/reading the swap
> >> space, etc. This will help to improve the THP swap performance.
> >>
> >> - The THP swap space read/write will be 2M sequential IO. It is
> >> particularly helpful for the swap read, which usually are 4k random
> >> IO. This will help to improve the THP swap performance.
> >>
> >> - It will help the memory fragmentation, especially when the THP is
> >> heavily used by the applications. The 2M continuous pages will be
> >> free up after the THP swapping out.
> >>
> >> - It will improve the THP utilization on the system with the swap
> >> turned on. Because the speed for khugepaged to collapse the normal
> >> pages into the THP is quite slow. After the THP is split during the
> >> swapping out, it will take quite long time for the normal pages to
> >> collapse back into the THP after being swapped in. The high THP
> >> utilization helps the efficiency of the page based memory management
> >> too.
> >>
> >> There are some concerns regarding THP swap in, mainly because possible
> >> enlarged read/write IO size (for swap in/out) may put more overhead on
> >> the storage device. To deal with that, the THP swap in should be
> >> turned on only when necessary. For example, it can be selected via
> >> "always/never/madvise" logic, to be turned on globally, turned off
> >> globally, or turned on only for VMA with MADV_HUGEPAGE, etc.
> >>
> >> In this patch, one swap cluster is used to hold the contents of each
> >> THP swapped out. So, the size of the swap cluster is changed to that
> >> of the THP (Transparent Huge Page) on x86_64 architecture (512). For
> >> other architectures which want such THP swap optimization,
> >> ARCH_USES_THP_SWAP_CLUSTER needs to be selected in the Kconfig file
> >> for the architecture. In effect, this will enlarge swap cluster size
> >> by 2 times on x86_64. Which may make it harder to find a free cluster
> >> when the swap space becomes fragmented. So that, this may reduce the
> >> continuous swap space allocation and sequential write in theory. The
> >> performance test in 0day shows no regressions caused by this.
> >
> > What about other architecures?
> >
> > I mean THP page size on every architectures would be various.
> > If THP page size is much bigger than 2M, the architecture should
> > have big swap cluster size for supporting THP swap-out feature.
> > It means fast empty-swap cluster consumption so that it can suffer
> > from fragmentation easily which causes THP swap void and swap slot
> > allocations slow due to not being able to use per-cpu.
> >
> > What I suggested was contiguous multiple swap cluster allocations
> > to meet THP page size. If some of architecure's THP size is 64M
> > and SWAP_CLUSTER_SIZE is 2M, it should allocate 32 contiguos
> > swap clusters. For that, swap layer need to manage clusters sort
> > in order which would be more overhead in CONFIG_THP_SWAP case
> > but I think it's tradeoff. With that, every architectures can
> > support THP swap easily without arch-specific something.
>
> That may be a good solution for other architectures. But I am afraid I
> am not the right person to work on that. Because I don't know the
> requirement of other architectures, and I have no other architectures
> machines to work on and measure the performance.
IMO, THP swapout is good thing for every architecture so I dobut
you need to know other architecture's requirement.
>
> And the swap clusters aren't sorted in order now intentionally to avoid
> cache line false sharing between the spinlock of struct
> swap_cluster_info. If we want to sort clusters in order, we need a
> solution for that.
Does it really matter for this work? IOW, if we couldn't solve it,
cannot we support THP swapout? I don't think so. That's the least
of your worries.
Also, if we have sorted cluster data structure, we need to change
current single linked list of swap cluster to other one so we would
need to revisit to see whether it's really problem.
>
> > If (PAGE_SIZE * 512) swap cluster size were okay for most of
> > architecture, just increase it. It's orthogonal work regardless of
> > THP swapout. Then, we don't need to manage swap clusters sort
> > in order in x86_64 which SWAP_CLUSTER_SIZE is equal to
> > THP_PAGE_SIZE. It's just a bonus by side-effect.
>
> Andrew suggested to make swap cluster size = huge page size (or turn on
> THP swap optimization) only if we enabled CONFIG_THP_SWAP. So that, THP
> swap optimization will not be turned on unintentionally.
>
> We may adjust default swap cluster size, but I don't think it need to be
> in this patchset.
That's it. This feature shouldn't be aware of swap cluster size. IOW,
it would be better to work with every swap cluster size if the align
between THP and swap cluster size is matched at least.
> >> --- a/mm/shmem.c
> >> +++ b/mm/shmem.c
> >> @@ -1290,7 +1290,7 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
> >> SetPageUptodate(page);
> >> }
> >>
> >> - swap = get_swap_page();
> >> + swap = get_swap_page(page);
> >> if (!swap.val)
> >> goto redirty;
> >>
> >
> > If swap is non-ssd, swap.val could be zero. Right?
> > If so, could we retry like anonymous page swapout?
>
> This is for shmem, where the THP will be split before goes here. That
> is, "page" here is always normal page.
Thanks. I missed it.
However, get_swap_page is ugly now. The caller should take care of
failure and should retry after split. I hope get_swap_page includes
split and retry logic in itself without reling on the caller.
diff --git a/include/linux/swap.h b/include/linux/swap.h
index b60fea3748f8..96d41fade8d9 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -392,7 +392,7 @@ static inline long get_nr_swap_pages(void)
}
extern void si_swapinfo(struct sysinfo *);
-extern swp_entry_t get_swap_page(struct page *page);
+extern swp_entry_t get_swap_page(struct page *page, struct list_head *list);
extern swp_entry_t get_swap_page_of_type(int);
extern int get_swap_pages(int n, bool cluster, swp_entry_t swp_entries[]);
extern int add_swap_count_continuation(swp_entry_t, gfp_t);
@@ -400,7 +400,7 @@ extern void swap_shmem_alloc(swp_entry_t);
extern int swap_duplicate(swp_entry_t);
extern int swapcache_prepare(swp_entry_t);
extern void swap_free(swp_entry_t);
-extern void swapcache_free(swp_entry_t);
+extern void swapcache_free(struct page *page, swp_entry_t);
extern void swapcache_free_entries(swp_entry_t *entries, int n);
extern int free_swap_and_cache(swp_entry_t);
extern int swap_type_of(dev_t, sector_t, struct block_device **);
@@ -459,7 +459,7 @@ static inline void swap_free(swp_entry_t swp)
{
}
-static inline void swapcache_free(swp_entry_t swp)
+static inline void swapcache_free(struct page *page, swp_entry_t swp)
{
}
@@ -521,7 +521,8 @@ static inline int try_to_free_swap(struct page *page)
return 0;
}
-static inline swp_entry_t get_swap_page(struct page *page)
+static inline swp_entry_t get_swap_page(struct page *page,
+ struct list_head *list)
{
swp_entry_t entry;
entry.val = 0;
diff --git a/mm/shmem.c b/mm/shmem.c
index 29948d7da172..59afa7fc4313 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1290,7 +1290,7 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
SetPageUptodate(page);
}
- swap = get_swap_page(page);
+ swap = get_swap_page(page, NULL);
if (!swap.val)
goto redirty;
@@ -1326,7 +1326,7 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
mutex_unlock(&shmem_swaplist_mutex);
free_swap:
- swapcache_free(swap);
+ swapcache_free(page, swap);
redirty:
set_page_dirty(page);
if (wbc->for_reclaim)
diff --git a/mm/swap_slots.c b/mm/swap_slots.c
index eb7524f8296d..ed5170f0bb7e 100644
--- a/mm/swap_slots.c
+++ b/mm/swap_slots.c
@@ -302,7 +302,7 @@ int free_swap_slot(swp_entry_t entry)
return 0;
}
-swp_entry_t get_swap_page(struct page *page)
+swp_entry_t get_swap_page(struct page *page, struct list_head *list)
{
swp_entry_t entry, *pentry;
struct swap_slots_cache *cache;
@@ -312,7 +312,15 @@ swp_entry_t get_swap_page(struct page *page)
if (PageTransHuge(page)) {
if (hpage_nr_pages(page) == SWAPFILE_CLUSTER)
get_swap_pages(1, true, &entry);
- return entry;
+ if (entry.val != 0)
+ return entry;
+ /*
+ * If swap device is not a SSD or cannot find
+ * a empty cluster, split the page and fall back
+ * to swap slot allocation.
+ */
+ if (split_huge_page_to_list(page, list))
+ return entry;
}
/*
diff --git a/mm/swap_state.c b/mm/swap_state.c
index 16ff89d058f4..d218c8513ff1 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -192,12 +192,12 @@ int add_to_swap(struct page *page, struct list_head *list)
VM_BUG_ON_PAGE(!PageLocked(page), page);
VM_BUG_ON_PAGE(!PageUptodate(page), page);
-retry:
- entry = get_swap_page(page);
+ entry = get_swap_page(page, list);
if (!entry.val)
- goto fail;
+ return 0;
+
if (mem_cgroup_try_charge_swap(page, entry))
- goto fail_free;
+ goto fail;
/*
* Radix-tree node allocations from PF_MEMALLOC contexts could
@@ -218,7 +218,7 @@ int add_to_swap(struct page *page, struct list_head *list)
* add_to_swap_cache() doesn't return -EEXIST, so we can safely
* clear SWAP_HAS_CACHE flag.
*/
- goto fail_free;
+ goto fail;
if (PageTransHuge(page)) {
err = split_huge_page_to_list(page, list);
@@ -230,14 +230,8 @@ int add_to_swap(struct page *page, struct list_head *list)
return 1;
-fail_free:
- if (PageTransHuge(page))
- swapcache_free_cluster(entry);
- else
- swapcache_free(entry);
fail:
- if (PageTransHuge(page) && !split_huge_page_to_list(page, list))
- goto retry;
+ swapcache_free(page, entry);
return 0;
}
@@ -259,11 +253,7 @@ void delete_from_swap_cache(struct page *page)
__delete_from_swap_cache(page);
spin_unlock_irq(&address_space->tree_lock);
- if (PageTransHuge(page))
- swapcache_free_cluster(entry);
- else
- swapcache_free(entry);
-
+ swapcache_free(page, entry);
page_ref_sub(page, hpage_nr_pages(page));
}
@@ -415,7 +405,7 @@ struct page *__read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
* add_to_swap_cache() doesn't return -EEXIST, so we can safely
* clear SWAP_HAS_CACHE flag.
*/
- swapcache_free(entry);
+ swapcache_free(new_page, entry);
} while (err != -ENOMEM);
if (new_page)
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 596306272059..9496cc3e955a 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1144,7 +1144,7 @@ void swap_free(swp_entry_t entry)
/*
* Called after dropping swapcache to decrease refcnt to swap entries.
*/
-void swapcache_free(swp_entry_t entry)
+void __swapcache_free(swp_entry_t entry)
{
struct swap_info_struct *p;
@@ -1156,7 +1156,7 @@ void swapcache_free(swp_entry_t entry)
}
#ifdef CONFIG_THP_SWAP
-void swapcache_free_cluster(swp_entry_t entry)
+void __swapcache_free_cluster(swp_entry_t entry)
{
unsigned long offset = swp_offset(entry);
unsigned long idx = offset / SWAPFILE_CLUSTER;
@@ -1182,6 +1182,14 @@ void swapcache_free_cluster(swp_entry_t entry)
}
#endif /* CONFIG_THP_SWAP */
+void swapcache_free(struct page *page, swp_entry_t entry)
+{
+ if (!PageTransHuge(page))
+ __swapcache_free(entry);
+ else
+ __swapcache_free_cluster(entry);
+}
+
static int swp_entry_cmp(const void *ent1, const void *ent2)
{
const swp_entry_t *e1 = ent1, *e2 = ent2;
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 5ebf468c5429..0f8ca3d1761d 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -708,7 +708,7 @@ static int __remove_mapping(struct address_space *mapping, struct page *page,
mem_cgroup_swapout(page, swap);
__delete_from_swap_cache(page);
spin_unlock_irqrestore(&mapping->tree_lock, flags);
- swapcache_free(swap);
+ swapcache_free(page, swap);
} else {
void (*freepage)(struct page *);
void *shadow = NULL;
>
> >>
> >> -swp_entry_t get_swap_page(void)
> >> +swp_entry_t get_swap_page(struct page *page)
> >> {
> >> swp_entry_t entry, *pentry;
> >> struct swap_slots_cache *cache;
> >>
> >> + entry.val = 0;
> >> +
> >> + if (PageTransHuge(page)) {
> >> + if (hpage_nr_pages(page) == SWAPFILE_CLUSTER)
> >> + get_swap_pages(1, true, &entry);
> >> + return entry;
> >> + }
> >> +
> >
> >
> > < snip >
> >
> >> /**
> >> @@ -178,20 +192,12 @@ int add_to_swap(struct page *page, struct list_head *list)
> >> VM_BUG_ON_PAGE(!PageLocked(page), page);
> >> VM_BUG_ON_PAGE(!PageUptodate(page), page);
> >>
> >> - entry = get_swap_page();
> >> +retry:
> >> + entry = get_swap_page(page);
> >> if (!entry.val)
> >> - return 0;
> >> -
> >> - if (mem_cgroup_try_charge_swap(page, entry)) {
> >> - swapcache_free(entry);
> >> - return 0;
> >> - }
> >> -
> >> - if (unlikely(PageTransHuge(page)))
> >> - if (unlikely(split_huge_page_to_list(page, list))) {
> >> - swapcache_free(entry);
> >> - return 0;
> >> - }
> >> + goto fail;
> >
> > So, with non-SSD swap, THP page *always* get the fail to get swp_entry_t
> > and retry after split the page. However, it makes unncessary get_swap_pages
> > call which is not trivial. If there is no SSD swap, thp-swap out should
> > be void without adding any performance overhead.
> > Hmm, but I have no good idea to do it simple. :(
>
> For HDD swap, the device raw throughput is so low (< 100M Bps
> typically), that the added overhead here will not be a big issue. Do
> you agree?
I agree. Actually, I wanted to remove the pointless overhead
if we have a enough *simple* solution. However, as I said,
I have no idea so just raised an issue wit hope that someone might
have an idea.
Frankly speaking, I think we should support THP swap with hdd
as well as ssd but it's limited to just *implementation* direction
which is really unfortunate. :(
--
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: Generic approach to customizable zones - was: Re: [PATCH v7 0/7] Introduce ZONE_CMA
From: Igor Stoppa @ 2017-04-28 9:04 UTC (permalink / raw)
To: Michal Hocko
Cc: Joonsoo Kim, Andrew Morton, Rik van Riel, Johannes Weiner,
mgorman, Laura Abbott, Minchan Kim, Marek Szyprowski,
Michal Nazarewicz, Aneesh Kumar K . V, Vlastimil Babka,
Russell King, Will Deacon, linux-mm, linux-kernel, kernel-team
In-Reply-To: <20170428083625.GG8143@dhcp22.suse.cz>
On 28/04/17 11:36, Michal Hocko wrote:
> I didn't read this thoughly yet because I will be travelling shortly
ok, thanks for bearing with me =)
> but
> this point alone just made ask, because it seems there is some
> misunderstanding
It is possible, so far I did some changes, but I have not completed the
whole conversion.
> On Fri 28-04-17 11:04:27, Igor Stoppa wrote:
> [...]
>> * if one is happy to have a 64bits type, allow for as many zones as
>> it's possible to fit, or anyway more than what is possible with
>> the 32 bit mask.
>
> zones are currently placed in struct page::flags. And that already is
> 64b size on 64b arches.
Ok, the issues I had so fare were related to the enum for zones being
treated as 32b.
> And we do not really have any room spare there.
> We encode page flags, zone id, numa_nid/sparse section_nr there. How can
> you add more without enlarging the struct page itself or using external
> means to store the same information (page_ext comes to mind)?
Then I'll be conservative and assume I can't, unless I can prove otherwise.
There is still the possibility I mentioned of loosely coupling DMA,
DMA32 and HIGHMEM with the bits currently reserved for them, right?
If my system doesn't use those zones as such, because it doesn't
have/need them, those bits are wasted for me. Otoh someone else is
probably not interested in what I'm after but needs one or more of those
zones.
Making the meaning of the bits configurable should still be a viable
option. It's not altering their amount, just their purpose on a specific
build.
> Even if
> the later would be possible then note thatpage_zone() is used in many
> performance sensitive paths and making it perform well with special
> casing would be far from trivial.
If the solution I propose is acceptable, I'm willing to bite the bullet
and go for implementing the conversion.
In my case I really would like to be able to use kmalloc, because it
would provide an easy path to convert also other portions of the kernel,
besides SE Linux.
I suspect I would encounter overall far less resistance if the type of
change I propose is limited to:
s/GFP_KERNEL/GFP_LOCKABLE/
And if I can guarrantee that GFP_LOCKABLE falls back to GFP_KERNEL when
the "lockable" feature is not enabled.
--
thanks, igor
--
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: Minchan Kim @ 2017-04-28 9:00 UTC (permalink / raw)
To: Huang, Ying
Cc: Andrew Morton, linux-mm, linux-kernel, Hugh Dickins, Shaohua Li,
Rik van Riel
In-Reply-To: <871ssdvtx5.fsf@yhuang-dev.intel.com>
On Fri, Apr 28, 2017 at 04:05:26PM +0800, Huang, Ying wrote:
> Minchan Kim <minchan@kernel.org> writes:
>
> > 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.
>
> Tim suggested to add a mechanism to correctly track how many swap
> devices are in use in swapon/swapoff. So we only sort if the number of
> the swap device > 1. This will not cover multiple swap devices with
> different priorities, but will cover the major usecases. The code
> should be simpler.
As you know, it doesn't solve multiple swaps by priority.
Even, there are cases full with entries same swap device
although multiple swap devices are used.
So, I think runtime sorting by judging need to be sored is still
better.
--
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: Laurent Dufour @ 2017-04-28 9:17 UTC (permalink / raw)
To: Michal Hocko, Andi Kleen
Cc: Naoya Horiguchi, linux-kernel, linux-mm, akpm, Johannes Weiner,
Vladimir Davydov
In-Reply-To: <20170428073136.GE8143@dhcp22.suse.cz>
On 28/04/2017 09:31, Michal Hocko wrote:
> [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.
Thanks for the review Michal.
I was not comfortable either with this patch.
I did some tests calling mem_cgroup_uncharge() when isolate_lru_page()
succeeds only, so not calling it if isolate_lru_page() failed.
This seems to work as well, so if everyone agree on that, I'll send a
new version soon.
Cheers,
Laurent.
--
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: Laurent Dufour @ 2017-04-28 9:32 UTC (permalink / raw)
To: Balbir Singh, Naoya Horiguchi
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
akpm@linux-foundation.org
In-Reply-To: <1493197141.16329.1.camel@gmail.com>
On 26/04/2017 10:59, Balbir Singh wrote:
> On Wed, 2017-04-26 at 04:46 +0000, Naoya Horiguchi wrote:
>> On Wed, Apr 26, 2017 at 01:45:00PM +1000, Balbir Singh wrote:
>>>>>> static int delete_from_lru_cache(struct page *p)
>>>>>> {
>>>>>> + if (memcg_kmem_enabled())
>>>>>> + memcg_kmem_uncharge(p, 0);
>>>>>> +
>>>>>
>>>>> The changelog is not quite clear, so we are uncharging a page using
>>>>> memcg_kmem_uncharge for a page in swap cache/page cache?
>>>>
>>>> Hi Balbir,
>>>>
>>>> Yes, in the normal page lifecycle, uncharge is done in page free time.
>>>> But in memory error handling case, in-use pages (i.e. swap cache and page
>>>> cache) are removed from normal path and they don't pass page freeing code.
>>>> So I think that this change is to keep the consistent charging for such a case.
>>>
>>> I agree we should uncharge, but looking at the API name, it seems to
>>> be for kmem pages, why are we not using mem_cgroup_uncharge()? Am I missing
>>> something?
>>
>> Thank you for pointing out.
>> Actually I had the same question and this surely looks strange.
>> But simply calling mem_cgroup_uncharge() here doesn't work because it
>> assumes that page_refcount(p) == 0, which is not true in hwpoison context.
>> We need some other clearer way or at least some justifying comment about
>> why this is ok.
>>
>
> We should call mem_cgroup_uncharge() after isolate_lru_page()/put_page().
Thanks for the review Naoya and Balbir,
I changed the patch to call mem_cgroup_uncharge() once
isolate_lru_page() succeeded, but before calling put_page().
It seems to work fine.
> We could check if page_count() is 0 or force if required (!MF_RECOVERED &&
> !MF_DELAYED). We could even skip the VM_BUG_ON if the page is poisoned.
This doesn't seem to be needed. Am I still missing something here ?
Cheers,
Laurent.
--
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 man-pages 1/2] userfaultfd.2: start documenting non-cooperative events
From: Mike Rapoprt @ 2017-04-28 9:45 UTC (permalink / raw)
To: Michael Kerrisk (man-pages)
Cc: Andrea Arcangeli, linux-kernel, linux-mm, linux-man
In-Reply-To: <a95f9ae6-f7db-1ed9-6e25-99ced1fd37a3@gmail.com>
On April 27, 2017 8:26:16 PM GMT+03:00, "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> wrote:
>Hi Mike,
>
>I've applied this, but have some questions/points I think
>further clarification.
>
>On 04/27/2017 04:14 PM, Mike Rapoport wrote:
>> Signed-off-by: Mike Rapoport <rppt@linux.vnet.ibm.com>
>> ---
>> man2/userfaultfd.2 | 135
>++++++++++++++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 128 insertions(+), 7 deletions(-)
>>
>> diff --git a/man2/userfaultfd.2 b/man2/userfaultfd.2
>> index cfea5cb..44af3e4 100644
>> --- a/man2/userfaultfd.2
>> +++ b/man2/userfaultfd.2
>> @@ -75,7 +75,7 @@ flag in
>> .PP
>> When the last file descriptor referring to a userfaultfd object is
>closed,
>> all memory ranges that were registered with the object are
>unregistered
>> -and unread page-fault events are flushed.
>> +and unread events are flushed.
>> .\"
>> .SS Usage
>> The userfaultfd mechanism is designed to allow a thread in a
>multithreaded
>> @@ -99,6 +99,20 @@ In such non-cooperative mode,
>> the process that monitors userfaultfd and handles page faults
>> needs to be aware of the changes in the virtual memory layout
>> of the faulting process to avoid memory corruption.
>> +
>> +Starting from Linux 4.11,
>> +userfaultfd may notify the fault-handling threads about changes
>> +in the virtual memory layout of the faulting process.
>> +In addition, if the faulting process invokes
>> +.BR fork (2)
>> +system call,
>> +the userfaultfd objects associated with the parent may be duplicated
>> +into the child process and the userfaultfd monitor will be notified
>> +about the file descriptor associated with the userfault objects
>
>What does "notified about the file descriptor" mean?
Well, seems that I've made this one really awkward :)
When the monitored process forks, all the userfault objects associated with it are duplicated into the child process. For each duplicated object, userfault generates event of type UFFD_EVENT_FORK and the uffdio_msg for this event contains the file descriptor that should be used to manipulate the duplicated userfault object.
Hope this clarifies.
>> +created for the child process,
>> +which allows userfaultfd monitor to perform user-space paging
>> +for the child process.
>> +
>> .\" FIXME elaborate about non-cooperating mode, describe its
>limitations
>> .\" for kernels before 4.11, features added in 4.11
>> .\" and limitations remaining in 4.11
>> @@ -144,6 +158,10 @@ Details of the various
>> operations can be found in
>> .BR ioctl_userfaultfd (2).
>>
>> +Since Linux 4.11, events other than page-fault may enabled during
>> +.B UFFDIO_API
>> +operation.
>> +
>> Up to Linux 4.11,
>> userfaultfd can be used only with anonymous private memory mappings.
>>
>> @@ -156,7 +174,8 @@ Each
>> .BR read (2)
>> from the userfaultfd file descriptor returns one or more
>> .I uffd_msg
>> -structures, each of which describes a page-fault event:
>> +structures, each of which describes a page-fault event
>> +or an event required for the non-cooperative userfaultfd usage:
>>
>> .nf
>> .in +4n
>> @@ -168,6 +187,23 @@ struct uffd_msg {
>> __u64 flags; /* Flags describing fault */
>> __u64 address; /* Faulting address */
>> } pagefault;
>> + struct {
>> + __u32 ufd; /* userfault file descriptor
>> + of the child process */
>> + } fork; /* since Linux 4.11 */
>> + struct {
>> + __u64 from; /* old address of the
>> + remapped area */
>> + __u64 to; /* new address of the
>> + remapped area */
>> + __u64 len; /* original mapping length */
>> + } remap; /* since Linux 4.11 */
>> + struct {
>> + __u64 start; /* start address of the
>> + removed area */
>> + __u64 end; /* end address of the
>> + removed area */
>> + } remove; /* since Linux 4.11 */
>> ...
>> } arg;
>>
>> @@ -194,14 +230,73 @@ structure are as follows:
>> .TP
>> .I event
>> The type of event.
>> -Currently, only one value can appear in this field:
>> -.BR UFFD_EVENT_PAGEFAULT ,
>> -which indicates a page-fault event.
>> +Depending of the event type,
>> +different fields of the
>> +.I arg
>> +union represent details required for the event processing.
>> +The non-page-fault events are generated only when appropriate
>feature
>> +is enabled during API handshake with
>> +.B UFFDIO_API
>> +.BR ioctl (2).
>> +
>> +The following values can appear in the
>> +.I event
>> +field:
>> +.RS
>> +.TP
>> +.B UFFD_EVENT_PAGEFAULT
>> +A page-fault event.
>> +The page-fault details are available in the
>> +.I pagefault
>> +field.
>> .TP
>> -.I address
>> +.B UFFD_EVENT_FORK
>> +Generated when the faulting process invokes
>> +.BR fork (2)
>> +system call.
>> +The event details are available in the
>> +.I fork
>> +field.
>> +.\" FIXME descirbe duplication of userfault file descriptor during
>fork
>> +.TP
>> +.B UFFD_EVENT_REMAP
>> +Generated when the faulting process invokes
>> +.BR mremap (2)
>> +system call.
>> +The event details are available in the
>> +.I remap
>> +field.
>> +.TP
>> +.B UFFD_EVENT_REMOVE
>> +Generated when the faulting process invokes
>> +.BR madvise (2)
>> +system call with
>> +.BR MADV_DONTNEED
>> +or
>> +.BR MADV_REMOVE
>> +advice.
>> +The event details are available in the
>> +.I remove
>> +field.
>> +.TP
>> +.B UFFD_EVENT_UNMAP
>> +Generated when the faulting process unmaps a memory range,
>> +either explicitly using
>> +.BR munmap (2)
>> +system call or implicitly during
>> +.BR mmap (2)
>> +or
>> +.BR mremap (2)
>> +system calls.
>> +The event details are available in the
>> +.I remove
>> +field.
>> +.RE
>> +.TP
>> +.I pagefault.address
>> The address that triggered the page fault.
>> .TP
>> -.I flags
>> +.I pagefault.flags
>> A bit mask of flags that describe the event.
>> For
>> .BR UFFD_EVENT_PAGEFAULT ,
>> @@ -218,6 +313,32 @@ otherwise it is a read fault.
>> .\"
>> .\" UFFD_PAGEFAULT_FLAG_WP is not yet supported.
>> .RE
>> +.TP
>> +.I fork.ufd
>> +The file descriptor associated with the userfault object
>> +created for the child process
>> +.TP
>> +.I remap.from
>> +The original address of the memory range that was remapped using
>> +.BR mremap (2).
>> +.TP
>> +.I remap.to
>> +The new address of the memory range that was remapped using
>> +.BR mremap (2).
>> +.TP
>> +.I remap.len
>> +The original length of the the memory range that was remapped using
>> +.BR mremap (2).
>> +.TP
>> +.I remove.start
>> +The start address of the memory range that was freed using
>> +.BR madvise (2)
>> +or unmapped
>> +.TP
>> +.I remove.end
>> +The end address of the memory range that was freed using
>> +.BR madvise (2)
>> +or unmapped
>> .PP
>> A
>> .BR read (2)
>
>Cheers,
>
>Michael
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
--
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 11:48 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: <20170428090049.GA26460@bbox>
Minchan Kim <minchan@kernel.org> writes:
> On Fri, Apr 28, 2017 at 04:05:26PM +0800, Huang, Ying wrote:
>> Minchan Kim <minchan@kernel.org> writes:
>>
>> > 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.
>>
>> Tim suggested to add a mechanism to correctly track how many swap
>> devices are in use in swapon/swapoff. So we only sort if the number of
>> the swap device > 1. This will not cover multiple swap devices with
>> different priorities, but will cover the major usecases. The code
>> should be simpler.
>
> As you know, it doesn't solve multiple swaps by priority.
I don't think this is *major* usecase.
> Even, there are cases full with entries same swap device
> although multiple swap devices are used.
Why, if you have multiple swap device, every time you will allocate from
different swap device. Although there are swap alloc slots cache, the
possibility of full alignment is low.
Even if there are cases all entries come from one swap device, the
sorting is fast in fact because the array is short and the elements are
sorted (same swap type) already. So it is not necessary to worry about
that too much.
Best Regards,
Huang, Ying
> So, I think runtime sorting by judging need to be sored is still
> better.
--
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 -v10 1/3] mm, THP, swap: Delay splitting THP during swap out
From: Huang, Ying @ 2017-04-28 12:21 UTC (permalink / raw)
To: Minchan Kim, Johannes Weiner
Cc: Huang, Ying, Andrew Morton, linux-mm, linux-kernel,
Andrea Arcangeli, Ebru Akagunduz, Michal Hocko, Tejun Heo,
Hugh Dickins, Shaohua Li, Rik van Riel, cgroups
In-Reply-To: <20170428084044.GB19510@bbox>
Minchan Kim <minchan@kernel.org> writes:
> On Thu, Apr 27, 2017 at 03:12:34PM +0800, Huang, Ying wrote:
>> Minchan Kim <minchan@kernel.org> writes:
>>
>> > On Tue, Apr 25, 2017 at 08:56:56PM +0800, Huang, Ying wrote:
>> >> From: Huang Ying <ying.huang@intel.com>
>> >>
>> >> In this patch, splitting huge page is delayed from almost the first
>> >> step of swapping out to after allocating the swap space for the
>> >> THP (Transparent Huge Page) and adding the THP into the swap cache.
>> >> This will batch the corresponding operation, thus improve THP swap out
>> >> throughput.
>> >>
>> >> This is the first step for the THP swap optimization. The plan is to
>> >> delay splitting the THP step by step and avoid splitting the THP
>> >> finally.
>> >>
>> >> The advantages of the THP swap support include:
>> >>
>> >> - Batch the swap operations for the THP and reduce lock
>> >> acquiring/releasing, including allocating/freeing the swap space,
>> >> adding/deleting to/from the swap cache, and writing/reading the swap
>> >> space, etc. This will help to improve the THP swap performance.
>> >>
>> >> - The THP swap space read/write will be 2M sequential IO. It is
>> >> particularly helpful for the swap read, which usually are 4k random
>> >> IO. This will help to improve the THP swap performance.
>> >>
>> >> - It will help the memory fragmentation, especially when the THP is
>> >> heavily used by the applications. The 2M continuous pages will be
>> >> free up after the THP swapping out.
>> >>
>> >> - It will improve the THP utilization on the system with the swap
>> >> turned on. Because the speed for khugepaged to collapse the normal
>> >> pages into the THP is quite slow. After the THP is split during the
>> >> swapping out, it will take quite long time for the normal pages to
>> >> collapse back into the THP after being swapped in. The high THP
>> >> utilization helps the efficiency of the page based memory management
>> >> too.
>> >>
>> >> There are some concerns regarding THP swap in, mainly because possible
>> >> enlarged read/write IO size (for swap in/out) may put more overhead on
>> >> the storage device. To deal with that, the THP swap in should be
>> >> turned on only when necessary. For example, it can be selected via
>> >> "always/never/madvise" logic, to be turned on globally, turned off
>> >> globally, or turned on only for VMA with MADV_HUGEPAGE, etc.
>> >>
>> >> In this patch, one swap cluster is used to hold the contents of each
>> >> THP swapped out. So, the size of the swap cluster is changed to that
>> >> of the THP (Transparent Huge Page) on x86_64 architecture (512). For
>> >> other architectures which want such THP swap optimization,
>> >> ARCH_USES_THP_SWAP_CLUSTER needs to be selected in the Kconfig file
>> >> for the architecture. In effect, this will enlarge swap cluster size
>> >> by 2 times on x86_64. Which may make it harder to find a free cluster
>> >> when the swap space becomes fragmented. So that, this may reduce the
>> >> continuous swap space allocation and sequential write in theory. The
>> >> performance test in 0day shows no regressions caused by this.
>> >
>> > What about other architecures?
>> >
>> > I mean THP page size on every architectures would be various.
>> > If THP page size is much bigger than 2M, the architecture should
>> > have big swap cluster size for supporting THP swap-out feature.
>> > It means fast empty-swap cluster consumption so that it can suffer
>> > from fragmentation easily which causes THP swap void and swap slot
>> > allocations slow due to not being able to use per-cpu.
>> >
>> > What I suggested was contiguous multiple swap cluster allocations
>> > to meet THP page size. If some of architecure's THP size is 64M
>> > and SWAP_CLUSTER_SIZE is 2M, it should allocate 32 contiguos
>> > swap clusters. For that, swap layer need to manage clusters sort
>> > in order which would be more overhead in CONFIG_THP_SWAP case
>> > but I think it's tradeoff. With that, every architectures can
>> > support THP swap easily without arch-specific something.
>>
>> That may be a good solution for other architectures. But I am afraid I
>> am not the right person to work on that. Because I don't know the
>> requirement of other architectures, and I have no other architectures
>> machines to work on and measure the performance.
>
> IMO, THP swapout is good thing for every architecture so I dobut
> you need to know other architecture's requirement.
>
>>
>> And the swap clusters aren't sorted in order now intentionally to avoid
>> cache line false sharing between the spinlock of struct
>> swap_cluster_info. If we want to sort clusters in order, we need a
>> solution for that.
>
> Does it really matter for this work? IOW, if we couldn't solve it,
> cannot we support THP swapout? I don't think so. That's the least
> of your worries.
> Also, if we have sorted cluster data structure, we need to change
> current single linked list of swap cluster to other one so we would
> need to revisit to see whether it's really problem.
>
>>
>> > If (PAGE_SIZE * 512) swap cluster size were okay for most of
>> > architecture, just increase it. It's orthogonal work regardless of
>> > THP swapout. Then, we don't need to manage swap clusters sort
>> > in order in x86_64 which SWAP_CLUSTER_SIZE is equal to
>> > THP_PAGE_SIZE. It's just a bonus by side-effect.
>>
>> Andrew suggested to make swap cluster size = huge page size (or turn on
>> THP swap optimization) only if we enabled CONFIG_THP_SWAP. So that, THP
>> swap optimization will not be turned on unintentionally.
>>
>> We may adjust default swap cluster size, but I don't think it need to be
>> in this patchset.
>
> That's it. This feature shouldn't be aware of swap cluster size. IOW,
> it would be better to work with every swap cluster size if the align
> between THP and swap cluster size is matched at least.
Using one swap cluster for each THP is simpler, so why not start from
the simple design? Complex design may be necessary in the future, but
we can work on that at that time.
>> >> --- a/mm/shmem.c
>> >> +++ b/mm/shmem.c
>> >> @@ -1290,7 +1290,7 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
>> >> SetPageUptodate(page);
>> >> }
>> >>
>> >> - swap = get_swap_page();
>> >> + swap = get_swap_page(page);
>> >> if (!swap.val)
>> >> goto redirty;
>> >>
>> >
>> > If swap is non-ssd, swap.val could be zero. Right?
>> > If so, could we retry like anonymous page swapout?
>>
>> This is for shmem, where the THP will be split before goes here. That
>> is, "page" here is always normal page.
>
> Thanks. I missed it.
>
> However, get_swap_page is ugly now. The caller should take care of
> failure and should retry after split. I hope get_swap_page includes
> split and retry logic in itself without reling on the caller.
The current interface of get_swap_page() and swapcache_free() is
proposed by Johannes.
Hi, Johannes, what do you think about Minchan's interface proposal?
Best Regards,
Huang, Ying
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index b60fea3748f8..96d41fade8d9 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -392,7 +392,7 @@ static inline long get_nr_swap_pages(void)
> }
>
> extern void si_swapinfo(struct sysinfo *);
> -extern swp_entry_t get_swap_page(struct page *page);
> +extern swp_entry_t get_swap_page(struct page *page, struct list_head *list);
> extern swp_entry_t get_swap_page_of_type(int);
> extern int get_swap_pages(int n, bool cluster, swp_entry_t swp_entries[]);
> extern int add_swap_count_continuation(swp_entry_t, gfp_t);
> @@ -400,7 +400,7 @@ extern void swap_shmem_alloc(swp_entry_t);
> extern int swap_duplicate(swp_entry_t);
> extern int swapcache_prepare(swp_entry_t);
> extern void swap_free(swp_entry_t);
> -extern void swapcache_free(swp_entry_t);
> +extern void swapcache_free(struct page *page, swp_entry_t);
> extern void swapcache_free_entries(swp_entry_t *entries, int n);
> extern int free_swap_and_cache(swp_entry_t);
> extern int swap_type_of(dev_t, sector_t, struct block_device **);
> @@ -459,7 +459,7 @@ static inline void swap_free(swp_entry_t swp)
> {
> }
>
> -static inline void swapcache_free(swp_entry_t swp)
> +static inline void swapcache_free(struct page *page, swp_entry_t swp)
> {
> }
>
> @@ -521,7 +521,8 @@ static inline int try_to_free_swap(struct page *page)
> return 0;
> }
>
> -static inline swp_entry_t get_swap_page(struct page *page)
> +static inline swp_entry_t get_swap_page(struct page *page,
> + struct list_head *list)
> {
> swp_entry_t entry;
> entry.val = 0;
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 29948d7da172..59afa7fc4313 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1290,7 +1290,7 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
> SetPageUptodate(page);
> }
>
> - swap = get_swap_page(page);
> + swap = get_swap_page(page, NULL);
> if (!swap.val)
> goto redirty;
>
> @@ -1326,7 +1326,7 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
>
> mutex_unlock(&shmem_swaplist_mutex);
> free_swap:
> - swapcache_free(swap);
> + swapcache_free(page, swap);
> redirty:
> set_page_dirty(page);
> if (wbc->for_reclaim)
> diff --git a/mm/swap_slots.c b/mm/swap_slots.c
> index eb7524f8296d..ed5170f0bb7e 100644
> --- a/mm/swap_slots.c
> +++ b/mm/swap_slots.c
> @@ -302,7 +302,7 @@ int free_swap_slot(swp_entry_t entry)
> return 0;
> }
>
> -swp_entry_t get_swap_page(struct page *page)
> +swp_entry_t get_swap_page(struct page *page, struct list_head *list)
> {
> swp_entry_t entry, *pentry;
> struct swap_slots_cache *cache;
> @@ -312,7 +312,15 @@ swp_entry_t get_swap_page(struct page *page)
> if (PageTransHuge(page)) {
> if (hpage_nr_pages(page) == SWAPFILE_CLUSTER)
> get_swap_pages(1, true, &entry);
> - return entry;
> + if (entry.val != 0)
> + return entry;
> + /*
> + * If swap device is not a SSD or cannot find
> + * a empty cluster, split the page and fall back
> + * to swap slot allocation.
> + */
> + if (split_huge_page_to_list(page, list))
> + return entry;
> }
>
> /*
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 16ff89d058f4..d218c8513ff1 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -192,12 +192,12 @@ int add_to_swap(struct page *page, struct list_head *list)
> VM_BUG_ON_PAGE(!PageLocked(page), page);
> VM_BUG_ON_PAGE(!PageUptodate(page), page);
>
> -retry:
> - entry = get_swap_page(page);
> + entry = get_swap_page(page, list);
> if (!entry.val)
> - goto fail;
> + return 0;
> +
> if (mem_cgroup_try_charge_swap(page, entry))
> - goto fail_free;
> + goto fail;
>
> /*
> * Radix-tree node allocations from PF_MEMALLOC contexts could
> @@ -218,7 +218,7 @@ int add_to_swap(struct page *page, struct list_head *list)
> * add_to_swap_cache() doesn't return -EEXIST, so we can safely
> * clear SWAP_HAS_CACHE flag.
> */
> - goto fail_free;
> + goto fail;
>
> if (PageTransHuge(page)) {
> err = split_huge_page_to_list(page, list);
> @@ -230,14 +230,8 @@ int add_to_swap(struct page *page, struct list_head *list)
>
> return 1;
>
> -fail_free:
> - if (PageTransHuge(page))
> - swapcache_free_cluster(entry);
> - else
> - swapcache_free(entry);
> fail:
> - if (PageTransHuge(page) && !split_huge_page_to_list(page, list))
> - goto retry;
> + swapcache_free(page, entry);
> return 0;
> }
>
> @@ -259,11 +253,7 @@ void delete_from_swap_cache(struct page *page)
> __delete_from_swap_cache(page);
> spin_unlock_irq(&address_space->tree_lock);
>
> - if (PageTransHuge(page))
> - swapcache_free_cluster(entry);
> - else
> - swapcache_free(entry);
> -
> + swapcache_free(page, entry);
> page_ref_sub(page, hpage_nr_pages(page));
> }
>
> @@ -415,7 +405,7 @@ struct page *__read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
> * add_to_swap_cache() doesn't return -EEXIST, so we can safely
> * clear SWAP_HAS_CACHE flag.
> */
> - swapcache_free(entry);
> + swapcache_free(new_page, entry);
> } while (err != -ENOMEM);
>
> if (new_page)
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 596306272059..9496cc3e955a 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1144,7 +1144,7 @@ void swap_free(swp_entry_t entry)
> /*
> * Called after dropping swapcache to decrease refcnt to swap entries.
> */
> -void swapcache_free(swp_entry_t entry)
> +void __swapcache_free(swp_entry_t entry)
> {
> struct swap_info_struct *p;
>
> @@ -1156,7 +1156,7 @@ void swapcache_free(swp_entry_t entry)
> }
>
> #ifdef CONFIG_THP_SWAP
> -void swapcache_free_cluster(swp_entry_t entry)
> +void __swapcache_free_cluster(swp_entry_t entry)
> {
> unsigned long offset = swp_offset(entry);
> unsigned long idx = offset / SWAPFILE_CLUSTER;
> @@ -1182,6 +1182,14 @@ void swapcache_free_cluster(swp_entry_t entry)
> }
> #endif /* CONFIG_THP_SWAP */
>
> +void swapcache_free(struct page *page, swp_entry_t entry)
> +{
> + if (!PageTransHuge(page))
> + __swapcache_free(entry);
> + else
> + __swapcache_free_cluster(entry);
> +}
> +
> static int swp_entry_cmp(const void *ent1, const void *ent2)
> {
> const swp_entry_t *e1 = ent1, *e2 = ent2;
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 5ebf468c5429..0f8ca3d1761d 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -708,7 +708,7 @@ static int __remove_mapping(struct address_space *mapping, struct page *page,
> mem_cgroup_swapout(page, swap);
> __delete_from_swap_cache(page);
> spin_unlock_irqrestore(&mapping->tree_lock, flags);
> - swapcache_free(swap);
> + swapcache_free(page, swap);
> } else {
> void (*freepage)(struct page *);
> void *shadow = NULL;
>>
>> >>
>> >> -swp_entry_t get_swap_page(void)
>> >> +swp_entry_t get_swap_page(struct page *page)
>> >> {
>> >> swp_entry_t entry, *pentry;
>> >> struct swap_slots_cache *cache;
>> >>
>> >> + entry.val = 0;
>> >> +
>> >> + if (PageTransHuge(page)) {
>> >> + if (hpage_nr_pages(page) == SWAPFILE_CLUSTER)
>> >> + get_swap_pages(1, true, &entry);
>> >> + return entry;
>> >> + }
>> >> +
>> >
>> >
>> > < snip >
>> >
>> >> /**
>> >> @@ -178,20 +192,12 @@ int add_to_swap(struct page *page, struct list_head *list)
>> >> VM_BUG_ON_PAGE(!PageLocked(page), page);
>> >> VM_BUG_ON_PAGE(!PageUptodate(page), page);
>> >>
>> >> - entry = get_swap_page();
>> >> +retry:
>> >> + entry = get_swap_page(page);
>> >> if (!entry.val)
>> >> - return 0;
>> >> -
>> >> - if (mem_cgroup_try_charge_swap(page, entry)) {
>> >> - swapcache_free(entry);
>> >> - return 0;
>> >> - }
>> >> -
>> >> - if (unlikely(PageTransHuge(page)))
>> >> - if (unlikely(split_huge_page_to_list(page, list))) {
>> >> - swapcache_free(entry);
>> >> - return 0;
>> >> - }
>> >> + goto fail;
>> >
>> > So, with non-SSD swap, THP page *always* get the fail to get swp_entry_t
>> > and retry after split the page. However, it makes unncessary get_swap_pages
>> > call which is not trivial. If there is no SSD swap, thp-swap out should
>> > be void without adding any performance overhead.
>> > Hmm, but I have no good idea to do it simple. :(
>>
>> For HDD swap, the device raw throughput is so low (< 100M Bps
>> typically), that the added overhead here will not be a big issue. Do
>> you agree?
>
> I agree. Actually, I wanted to remove the pointless overhead
> if we have a enough *simple* solution. However, as I said,
> I have no idea so just raised an issue wit hope that someone might
> have an idea.
>
> Frankly speaking, I think we should support THP swap with hdd
> as well as ssd but it's limited to just *implementation* direction
> which is really unfortunate. :(
--
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 -v10 3/3] mm, THP, swap: Enable THP swap optimization only if has compound map
From: Kirill A. Shutemov @ 2017-04-28 13:16 UTC (permalink / raw)
To: Johannes Weiner; +Cc: Huang, Ying, Andrew Morton, linux-mm, linux-kernel
In-Reply-To: <20170425214618.GB6841@cmpxchg.org>
On Tue, Apr 25, 2017 at 05:46:18PM -0400, Johannes Weiner wrote:
> On Tue, Apr 25, 2017 at 08:56:58PM +0800, Huang, Ying wrote:
> > From: Huang Ying <ying.huang@intel.com>
> >
> > If there is no compound map for a THP (Transparent Huge Page), it is
> > possible that the map count of some sub-pages of the THP is 0. So it
> > is better to split the THP before swapping out. In this way, the
> > sub-pages not mapped will be freed, and we can avoid the unnecessary
> > swap out operations for these sub-pages.
> >
> > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
>
> CC Kirill to double check the reasoning here
Looks good to me:
Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
--
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
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
From: Huang, Ying @ 2017-04-28 13:35 UTC (permalink / raw)
To: Huang, Ying
Cc: Minchan Kim, Andrew Morton, linux-mm, linux-kernel, Hugh Dickins,
Shaohua Li, Rik van Riel
In-Reply-To: <87h918vjlr.fsf@yhuang-dev.intel.com>
"Huang, Ying" <ying.huang@intel.com> writes:
> Minchan Kim <minchan@kernel.org> writes:
>
>> On Fri, Apr 28, 2017 at 04:05:26PM +0800, Huang, Ying wrote:
>>> Minchan Kim <minchan@kernel.org> writes:
>>>
>>> > 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.
>>>
>>> Tim suggested to add a mechanism to correctly track how many swap
>>> devices are in use in swapon/swapoff. So we only sort if the number of
>>> the swap device > 1. This will not cover multiple swap devices with
>>> different priorities, but will cover the major usecases. The code
>>> should be simpler.
>>
>> As you know, it doesn't solve multiple swaps by priority.
>
> I don't think this is *major* usecase.
>
>> Even, there are cases full with entries same swap device
>> although multiple swap devices are used.
>
> Why, if you have multiple swap device, every time you will allocate from
> different swap device. Although there are swap alloc slots cache, the
> possibility of full alignment is low.
>
> Even if there are cases all entries come from one swap device, the
> sorting is fast in fact because the array is short and the elements are
> sorted (same swap type) already. So it is not necessary to worry about
> that too much.
In fact, during the test, I found the overhead of sort() is comparable
with the performance difference of adding likely()/unlikely() to the
"if" in the function.
Best Regards,
Huang, Ying
> Best Regards,
> Huang, Ying
>
>> So, I think runtime sorting by judging need to be sored is still
>> better.
--
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 13:48 UTC (permalink / raw)
To: Laurent Dufour
Cc: Andi Kleen, Naoya Horiguchi, linux-kernel, linux-mm, akpm,
Johannes Weiner, Vladimir Davydov
In-Reply-To: <3eb86373-dafc-6db9-82cd-84eb9e8b0d37@linux.vnet.ibm.com>
On Fri 28-04-17 11:17:34, Laurent Dufour wrote:
> On 28/04/2017 09:31, Michal Hocko wrote:
> > [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.
>
> Thanks for the review Michal.
>
> I was not comfortable either with this patch.
>
> I did some tests calling mem_cgroup_uncharge() when isolate_lru_page()
> succeeds only, so not calling it if isolate_lru_page() failed.
Wait a moment. This cannot possibly work. isolate_lru_page asumes page
count > 0 and increments the counter so the resulting page count is > 1
I have only now realized that we have VM_BUG_ON_PAGE(page_count(page), page)
in uncharge_list().
This is getting quite hairy. What is the expected page count of the
hwpoison page? I guess we would need to update the VM_BUG_ON in the
memcg uncharge code to ignore the page count of hwpoison pages if it can
be arbitrary.
Before we go any further, is there any documentation about the expected
behavior and the state of the hwpoison pages? I have a very bad feeling
that the current behavior is quite arbitrary and "testing driven"
holes plugging will make it only more messy. So let's start with the
clear description of what should happen with the hwpoison pages.
--
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 v2] mm, zone_device: replace {get, put}_zone_device_page() with a single reference
From: Dan Williams @ 2017-04-28 17:23 UTC (permalink / raw)
To: mingo
Cc: linux-kernel, linux-mm, Jérôme Glisse, Ingo Molnar,
Andrew Morton, Logan Gunthorpe, Kirill Shutemov
In-Reply-To: <20170428063913.iz6xjcxblecofjlq@gmail.com>
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: JA(C)rA'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>
---
Changes in v2:
* Rebased to tip/master
* Clarified comment in __put_page
* Clarified devm_memremap_pages() kernel doc about ordering of
devm_memremap_pages_release() vs percpu_ref_kill() vs wait for
percpu_ref to drop to zero.
Ingo, I retested this with a revert of commit 6dd29b3df975 "Revert
'x86/mm/gup: Switch GUP to the generic get_user_page_fast()
implementation'". It should be good to go through x86/mm.
drivers/dax/pmem.c | 2 +-
drivers/nvdimm/pmem.c | 13 +++++++++++--
include/linux/mm.h | 14 --------------
kernel/memremap.c | 22 +++++++++-------------
mm/swap.c | 10 ++++++++++
5 files changed, 31 insertions(+), 30 deletions(-)
diff --git a/drivers/dax/pmem.c b/drivers/dax/pmem.c
index 033f49b31fdc..cb0d742fa23f 100644
--- a/drivers/dax/pmem.c
+++ b/drivers/dax/pmem.c
@@ -43,6 +43,7 @@ static void dax_pmem_percpu_exit(void *data)
struct dax_pmem *dax_pmem = to_dax_pmem(ref);
dev_dbg(dax_pmem->dev, "%s\n", __func__);
+ wait_for_completion(&dax_pmem->cmp);
percpu_ref_exit(ref);
}
@@ -53,7 +54,6 @@ static void dax_pmem_percpu_kill(void *data)
dev_dbg(dax_pmem->dev, "%s\n", __func__);
percpu_ref_kill(ref);
- wait_for_completion(&dax_pmem->cmp);
}
static int dax_pmem_probe(struct device *dev)
diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index 5b536be5a12e..fb7bbc79ac26 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -25,6 +25,7 @@
#include <linux/badblocks.h>
#include <linux/memremap.h>
#include <linux/vmalloc.h>
+#include <linux/blk-mq.h>
#include <linux/pfn_t.h>
#include <linux/slab.h>
#include <linux/pmem.h>
@@ -231,6 +232,11 @@ static void pmem_release_queue(void *q)
blk_cleanup_queue(q);
}
+static void pmem_freeze_queue(void *q)
+{
+ blk_mq_freeze_queue_start(q);
+}
+
static void pmem_release_disk(void *disk)
{
del_gendisk(disk);
@@ -284,6 +290,9 @@ static int pmem_attach_disk(struct device *dev,
if (!q)
return -ENOMEM;
+ if (devm_add_action_or_reset(dev, pmem_release_queue, q))
+ return -ENOMEM;
+
pmem->pfn_flags = PFN_DEV;
if (is_nd_pfn(dev)) {
addr = devm_memremap_pages(dev, &pfn_res, &q->q_usage_counter,
@@ -303,10 +312,10 @@ static int pmem_attach_disk(struct device *dev,
pmem->size, ARCH_MEMREMAP_PMEM);
/*
- * At release time the queue must be dead before
+ * At release time the queue must be frozen before
* devm_memremap_pages is unwound
*/
- if (devm_add_action_or_reset(dev, pmem_release_queue, q))
+ if (devm_add_action_or_reset(dev, pmem_freeze_queue, q))
return -ENOMEM;
if (IS_ERR(addr))
diff --git a/include/linux/mm.h b/include/linux/mm.h
index a835edd2db34..695da2a19b4c 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -762,19 +762,11 @@ static inline enum zone_type page_zonenum(const struct page *page)
}
#ifdef CONFIG_ZONE_DEVICE
-void get_zone_device_page(struct page *page);
-void put_zone_device_page(struct page *page);
static inline bool is_zone_device_page(const struct page *page)
{
return page_zonenum(page) == ZONE_DEVICE;
}
#else
-static inline void get_zone_device_page(struct page *page)
-{
-}
-static inline void put_zone_device_page(struct page *page)
-{
-}
static inline bool is_zone_device_page(const struct page *page)
{
return false;
@@ -790,9 +782,6 @@ static inline void get_page(struct page *page)
*/
VM_BUG_ON_PAGE(page_ref_count(page) <= 0, page);
page_ref_inc(page);
-
- if (unlikely(is_zone_device_page(page)))
- get_zone_device_page(page);
}
static inline void put_page(struct page *page)
@@ -801,9 +790,6 @@ static inline void put_page(struct page *page)
if (put_page_testzero(page))
__put_page(page);
-
- if (unlikely(is_zone_device_page(page)))
- put_zone_device_page(page);
}
#if defined(CONFIG_SPARSEMEM) && !defined(CONFIG_SPARSEMEM_VMEMMAP)
diff --git a/kernel/memremap.c b/kernel/memremap.c
index 07e85e5229da..23a6483c3666 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -182,18 +182,6 @@ struct page_map {
struct vmem_altmap altmap;
};
-void get_zone_device_page(struct page *page)
-{
- percpu_ref_get(page->pgmap->ref);
-}
-EXPORT_SYMBOL(get_zone_device_page);
-
-void put_zone_device_page(struct page *page)
-{
- put_dev_pagemap(page->pgmap);
-}
-EXPORT_SYMBOL(put_zone_device_page);
-
static void pgmap_radix_release(struct resource *res)
{
resource_size_t key, align_start, align_size, align_end;
@@ -237,6 +225,10 @@ static void devm_memremap_pages_release(struct device *dev, void *data)
struct resource *res = &page_map->res;
resource_size_t align_start, align_size;
struct dev_pagemap *pgmap = &page_map->pgmap;
+ unsigned long pfn;
+
+ for_each_device_pfn(pfn, page_map)
+ put_page(pfn_to_page(pfn));
if (percpu_ref_tryget_live(pgmap->ref)) {
dev_WARN(dev, "%s: page mapping is still live!\n", __func__);
@@ -277,7 +269,10 @@ struct dev_pagemap *find_dev_pagemap(resource_size_t phys)
*
* Notes:
* 1/ @ref must be 'live' on entry and 'dead' before devm_memunmap_pages() time
- * (or devm release event).
+ * (or devm release event). The expected order of events is that @ref has
+ * been through percpu_ref_kill() before devm_memremap_pages_release(). The
+ * wait for the completion of all references being dropped and
+ * percpu_ref_exit() must occur after devm_memremap_pages_release().
*
* 2/ @res is expected to be a host memory range that could feasibly be
* treated as a "System RAM" range, i.e. not a device mmio range, but
@@ -379,6 +374,7 @@ void *devm_memremap_pages(struct device *dev, struct resource *res,
*/
list_del(&page->lru);
page->pgmap = pgmap;
+ percpu_ref_get(ref);
}
devres_add(dev, page_map);
return __va(res->start);
diff --git a/mm/swap.c b/mm/swap.c
index 5dabf444d724..d8d9ee9e311a 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -97,6 +97,16 @@ static void __put_compound_page(struct page *page)
void __put_page(struct page *page)
{
+ if (is_zone_device_page(page)) {
+ put_dev_pagemap(page->pgmap);
+
+ /*
+ * The page belongs to the device that created pgmap. Do
+ * not return it to page allocator.
+ */
+ return;
+ }
+
if (unlikely(PageCompound(page)))
__put_compound_page(page);
else
--
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 v2] mm, zone_device: replace {get, put}_zone_device_page() with a single reference
From: Jerome Glisse @ 2017-04-28 17:34 UTC (permalink / raw)
To: Dan Williams
Cc: mingo, linux-kernel, linux-mm, Ingo Molnar, Andrew Morton,
Logan Gunthorpe, Kirill Shutemov
In-Reply-To: <149339998297.24933.1129582806028305912.stgit@dwillia2-desk3.amr.corp.intel.com>
> 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.
Cheers,
Jérôme
>
> 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>
> ---
> Changes in v2:
> * Rebased to tip/master
> * Clarified comment in __put_page
> * Clarified devm_memremap_pages() kernel doc about ordering of
> devm_memremap_pages_release() vs percpu_ref_kill() vs wait for
> percpu_ref to drop to zero.
>
> Ingo, I retested this with a revert of commit 6dd29b3df975 "Revert
> 'x86/mm/gup: Switch GUP to the generic get_user_page_fast()
> implementation'". It should be good to go through x86/mm.
>
> drivers/dax/pmem.c | 2 +-
> drivers/nvdimm/pmem.c | 13 +++++++++++--
> include/linux/mm.h | 14 --------------
> kernel/memremap.c | 22 +++++++++-------------
> mm/swap.c | 10 ++++++++++
> 5 files changed, 31 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/dax/pmem.c b/drivers/dax/pmem.c
> index 033f49b31fdc..cb0d742fa23f 100644
> --- a/drivers/dax/pmem.c
> +++ b/drivers/dax/pmem.c
> @@ -43,6 +43,7 @@ static void dax_pmem_percpu_exit(void *data)
> struct dax_pmem *dax_pmem = to_dax_pmem(ref);
>
> dev_dbg(dax_pmem->dev, "%s\n", __func__);
> + wait_for_completion(&dax_pmem->cmp);
> percpu_ref_exit(ref);
> }
>
> @@ -53,7 +54,6 @@ static void dax_pmem_percpu_kill(void *data)
>
> dev_dbg(dax_pmem->dev, "%s\n", __func__);
> percpu_ref_kill(ref);
> - wait_for_completion(&dax_pmem->cmp);
> }
>
> static int dax_pmem_probe(struct device *dev)
> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index 5b536be5a12e..fb7bbc79ac26 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
> @@ -25,6 +25,7 @@
> #include <linux/badblocks.h>
> #include <linux/memremap.h>
> #include <linux/vmalloc.h>
> +#include <linux/blk-mq.h>
> #include <linux/pfn_t.h>
> #include <linux/slab.h>
> #include <linux/pmem.h>
> @@ -231,6 +232,11 @@ static void pmem_release_queue(void *q)
> blk_cleanup_queue(q);
> }
>
> +static void pmem_freeze_queue(void *q)
> +{
> + blk_mq_freeze_queue_start(q);
> +}
> +
> static void pmem_release_disk(void *disk)
> {
> del_gendisk(disk);
> @@ -284,6 +290,9 @@ static int pmem_attach_disk(struct device *dev,
> if (!q)
> return -ENOMEM;
>
> + if (devm_add_action_or_reset(dev, pmem_release_queue, q))
> + return -ENOMEM;
> +
> pmem->pfn_flags = PFN_DEV;
> if (is_nd_pfn(dev)) {
> addr = devm_memremap_pages(dev, &pfn_res, &q->q_usage_counter,
> @@ -303,10 +312,10 @@ static int pmem_attach_disk(struct device *dev,
> pmem->size, ARCH_MEMREMAP_PMEM);
>
> /*
> - * At release time the queue must be dead before
> + * At release time the queue must be frozen before
> * devm_memremap_pages is unwound
> */
> - if (devm_add_action_or_reset(dev, pmem_release_queue, q))
> + if (devm_add_action_or_reset(dev, pmem_freeze_queue, q))
> return -ENOMEM;
>
> if (IS_ERR(addr))
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index a835edd2db34..695da2a19b4c 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -762,19 +762,11 @@ static inline enum zone_type page_zonenum(const struct
> page *page)
> }
>
> #ifdef CONFIG_ZONE_DEVICE
> -void get_zone_device_page(struct page *page);
> -void put_zone_device_page(struct page *page);
> static inline bool is_zone_device_page(const struct page *page)
> {
> return page_zonenum(page) == ZONE_DEVICE;
> }
> #else
> -static inline void get_zone_device_page(struct page *page)
> -{
> -}
> -static inline void put_zone_device_page(struct page *page)
> -{
> -}
> static inline bool is_zone_device_page(const struct page *page)
> {
> return false;
> @@ -790,9 +782,6 @@ static inline void get_page(struct page *page)
> */
> VM_BUG_ON_PAGE(page_ref_count(page) <= 0, page);
> page_ref_inc(page);
> -
> - if (unlikely(is_zone_device_page(page)))
> - get_zone_device_page(page);
> }
>
> static inline void put_page(struct page *page)
> @@ -801,9 +790,6 @@ static inline void put_page(struct page *page)
>
> if (put_page_testzero(page))
> __put_page(page);
> -
> - if (unlikely(is_zone_device_page(page)))
> - put_zone_device_page(page);
> }
>
> #if defined(CONFIG_SPARSEMEM) && !defined(CONFIG_SPARSEMEM_VMEMMAP)
> diff --git a/kernel/memremap.c b/kernel/memremap.c
> index 07e85e5229da..23a6483c3666 100644
> --- a/kernel/memremap.c
> +++ b/kernel/memremap.c
> @@ -182,18 +182,6 @@ struct page_map {
> struct vmem_altmap altmap;
> };
>
> -void get_zone_device_page(struct page *page)
> -{
> - percpu_ref_get(page->pgmap->ref);
> -}
> -EXPORT_SYMBOL(get_zone_device_page);
> -
> -void put_zone_device_page(struct page *page)
> -{
> - put_dev_pagemap(page->pgmap);
> -}
> -EXPORT_SYMBOL(put_zone_device_page);
> -
> static void pgmap_radix_release(struct resource *res)
> {
> resource_size_t key, align_start, align_size, align_end;
> @@ -237,6 +225,10 @@ static void devm_memremap_pages_release(struct device
> *dev, void *data)
> struct resource *res = &page_map->res;
> resource_size_t align_start, align_size;
> struct dev_pagemap *pgmap = &page_map->pgmap;
> + unsigned long pfn;
> +
> + for_each_device_pfn(pfn, page_map)
> + put_page(pfn_to_page(pfn));
>
> if (percpu_ref_tryget_live(pgmap->ref)) {
> dev_WARN(dev, "%s: page mapping is still live!\n", __func__);
> @@ -277,7 +269,10 @@ struct dev_pagemap *find_dev_pagemap(resource_size_t
> phys)
> *
> * Notes:
> * 1/ @ref must be 'live' on entry and 'dead' before devm_memunmap_pages()
> time
> - * (or devm release event).
> + * (or devm release event). The expected order of events is that @ref has
> + * been through percpu_ref_kill() before devm_memremap_pages_release().
> The
> + * wait for the completion of all references being dropped and
> + * percpu_ref_exit() must occur after devm_memremap_pages_release().
> *
> * 2/ @res is expected to be a host memory range that could feasibly be
> * treated as a "System RAM" range, i.e. not a device mmio range, but
> @@ -379,6 +374,7 @@ void *devm_memremap_pages(struct device *dev, struct
> resource *res,
> */
> list_del(&page->lru);
> page->pgmap = pgmap;
> + percpu_ref_get(ref);
> }
> devres_add(dev, page_map);
> return __va(res->start);
> diff --git a/mm/swap.c b/mm/swap.c
> index 5dabf444d724..d8d9ee9e311a 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -97,6 +97,16 @@ static void __put_compound_page(struct page *page)
>
> void __put_page(struct page *page)
> {
> + if (is_zone_device_page(page)) {
> + put_dev_pagemap(page->pgmap);
> +
> + /*
> + * The page belongs to the device that created pgmap. Do
> + * not return it to page allocator.
> + */
> + return;
> + }
> +
> if (unlikely(PageCompound(page)))
> __put_compound_page(page);
> else
>
>
--
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 17:41 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: <1743017574.4309811.1493400875692.JavaMail.zimbra@redhat.com>
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().
--
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 18:00 UTC (permalink / raw)
To: Dan Williams
Cc: Ingo Molnar, linux-kernel, Linux MM, Ingo Molnar, Andrew Morton,
Logan Gunthorpe, Kirill Shutemov
In-Reply-To: <CAPcyv4jCfMwthPwbE-iuvef1KkMYUtA=qAydgfJzH0_otXoAOg@mail.gmail.com>
> 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).
Cheers,
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
* Re: [PATCH v2] mm, zone_device: replace {get, put}_zone_device_page() with a single reference
From: Dan Williams @ 2017-04-28 19:02 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: <1579714997.4315035.1493402406629.JavaMail.zimbra@redhat.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.
--
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
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox