Page migration's __unmap_and_move(), and rmap's try_to_unmap(),
were created for use on pages almost certainly mapped into userspace.
But nowadays compaction often applies them to unmapped page cache pages:
which may exacerbate contention on i_mmap_rwsem quite unnecessarily,
since try_to_unmap_file() makes no preliminary page_mapped() check.
Now check page_mapped() in __unmap_and_move(); and avoid repeating the
same overhead in rmap_walk_file() - don't remove_migration_ptes() when
we never inserted any.
(The PageAnon(page) comment blocks now look even sillier than before,
but clean that up on some other occasion. And note in passing that
try_to_unmap_one() does not use a migration entry when PageSwapCache,
so remove_migration_ptes() will then not update that swap entry to
newpage pte: not a big deal, but something else to clean up later.)
Davidlohr remarked in "mm,fs: introduce helpers around the i_mmap_mutex"
conversion to i_mmap_rwsem, that "The biggest winner of these changes
is migration": a part of the reason might be all of that unnecessary
taking of i_mmap_mutex in page migration; and it's rather a shame that
I didn't get around to sending this patch in before his - this one is
much less useful after Davidlohr's conversion to rwsem, but still good.
Signed-off-by: Hugh Dickins <[email protected]>
---
mm/migrate.c | 28 ++++++++++++++++++----------
1 file changed, 18 insertions(+), 10 deletions(-)
--- 3.18-rc7/mm/migrate.c 2014-10-19 22:12:56.809625067 -0700
+++ linux/mm/migrate.c 2014-11-30 20:17:51.205187663 -0800
@@ -746,7 +746,7 @@ static int fallback_migrate_page(struct
* MIGRATEPAGE_SUCCESS - success
*/
static int move_to_new_page(struct page *newpage, struct page *page,
- int remap_swapcache, enum migrate_mode mode)
+ int page_was_mapped, enum migrate_mode mode)
{
struct address_space *mapping;
int rc;
@@ -784,7 +784,7 @@ static int move_to_new_page(struct page
newpage->mapping = NULL;
} else {
mem_cgroup_migrate(page, newpage, false);
- if (remap_swapcache)
+ if (page_was_mapped)
remove_migration_ptes(page, newpage);
page->mapping = NULL;
}
@@ -798,7 +798,7 @@ static int __unmap_and_move(struct page
int force, enum migrate_mode mode)
{
int rc = -EAGAIN;
- int remap_swapcache = 1;
+ int page_was_mapped = 0;
struct anon_vma *anon_vma = NULL;
if (!trylock_page(page)) {
@@ -870,7 +870,6 @@ static int __unmap_and_move(struct page
* migrated but are not remapped when migration
* completes
*/
- remap_swapcache = 0;
} else {
goto out_unlock;
}
@@ -910,13 +909,17 @@ static int __unmap_and_move(struct page
}
/* Establish migration ptes or remove ptes */
- try_to_unmap(page, TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS);
+ if (page_mapped(page)) {
+ try_to_unmap(page,
+ TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS);
+ page_was_mapped = 1;
+ }
skip_unmap:
if (!page_mapped(page))
- rc = move_to_new_page(newpage, page, remap_swapcache, mode);
+ rc = move_to_new_page(newpage, page, page_was_mapped, mode);
- if (rc && remap_swapcache)
+ if (rc && page_was_mapped)
remove_migration_ptes(page, page);
/* Drop an anon_vma reference if we took one */
@@ -1017,6 +1020,7 @@ static int unmap_and_move_huge_page(new_
{
int rc = 0;
int *result = NULL;
+ int page_was_mapped = 0;
struct page *new_hpage;
struct anon_vma *anon_vma = NULL;
@@ -1047,12 +1051,16 @@ static int unmap_and_move_huge_page(new_
if (PageAnon(hpage))
anon_vma = page_get_anon_vma(hpage);
- try_to_unmap(hpage, TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS);
+ if (page_mapped(hpage)) {
+ try_to_unmap(hpage,
+ TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS);
+ page_was_mapped = 1;
+ }
if (!page_mapped(hpage))
- rc = move_to_new_page(new_hpage, hpage, 1, mode);
+ rc = move_to_new_page(new_hpage, hpage, page_was_mapped, mode);
- if (rc != MIGRATEPAGE_SUCCESS)
+ if (rc != MIGRATEPAGE_SUCCESS && page_was_mapped)
remove_migration_ptes(hpage, hpage);
if (anon_vma)
(2014/12/01 13:52), Hugh Dickins wrote:
> Page migration's __unmap_and_move(), and rmap's try_to_unmap(),
> were created for use on pages almost certainly mapped into userspace.
> But nowadays compaction often applies them to unmapped page cache pages:
> which may exacerbate contention on i_mmap_rwsem quite unnecessarily,
> since try_to_unmap_file() makes no preliminary page_mapped() check.
>
> Now check page_mapped() in __unmap_and_move(); and avoid repeating the
> same overhead in rmap_walk_file() - don't remove_migration_ptes() when
> we never inserted any.
>
> (The PageAnon(page) comment blocks now look even sillier than before,
> but clean that up on some other occasion. And note in passing that
> try_to_unmap_one() does not use a migration entry when PageSwapCache,
> so remove_migration_ptes() will then not update that swap entry to
> newpage pte: not a big deal, but something else to clean up later.)
>
> Davidlohr remarked in "mm,fs: introduce helpers around the i_mmap_mutex"
> conversion to i_mmap_rwsem, that "The biggest winner of these changes
> is migration": a part of the reason might be all of that unnecessary
> taking of i_mmap_mutex in page migration; and it's rather a shame that
> I didn't get around to sending this patch in before his - this one is
> much less useful after Davidlohr's conversion to rwsem, but still good.
>
> Signed-off-by: Hugh Dickins <[email protected]>
> ---
>
> mm/migrate.c | 28 ++++++++++++++++++----------
> 1 file changed, 18 insertions(+), 10 deletions(-)
>
> --- 3.18-rc7/mm/migrate.c 2014-10-19 22:12:56.809625067 -0700
> +++ linux/mm/migrate.c 2014-11-30 20:17:51.205187663 -0800
> @@ -746,7 +746,7 @@ static int fallback_migrate_page(struct
> * MIGRATEPAGE_SUCCESS - success
> */
> static int move_to_new_page(struct page *newpage, struct page *page,
> - int remap_swapcache, enum migrate_mode mode)
> + int page_was_mapped, enum migrate_mode mode)
> {
> struct address_space *mapping;
> int rc;
> @@ -784,7 +784,7 @@ static int move_to_new_page(struct page
> newpage->mapping = NULL;
> } else {
> mem_cgroup_migrate(page, newpage, false);
> - if (remap_swapcache)
> + if (page_was_mapped)
> remove_migration_ptes(page, newpage);
> page->mapping = NULL;
> }
> @@ -798,7 +798,7 @@ static int __unmap_and_move(struct page
> int force, enum migrate_mode mode)
> {
> int rc = -EAGAIN;
> - int remap_swapcache = 1;
> + int page_was_mapped = 0;
> struct anon_vma *anon_vma = NULL;
>
> if (!trylock_page(page)) {
> @@ -870,7 +870,6 @@ static int __unmap_and_move(struct page
> * migrated but are not remapped when migration
> * completes
> */
> - remap_swapcache = 0;
> } else {
> goto out_unlock;
> }
> @@ -910,13 +909,17 @@ static int __unmap_and_move(struct page
> }
>
> /* Establish migration ptes or remove ptes */
> - try_to_unmap(page, TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS);
> + if (page_mapped(page)) {
> + try_to_unmap(page,
> + TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS);
> + page_was_mapped = 1;
> + }
Is there no possibility that page is swap cache? If page is swap cache,
this code changes behavior of move_to_new_page(). Is it O.K.?
Thanks,
Yasuaki Ishimatsu
>
> skip_unmap:
> if (!page_mapped(page))
> - rc = move_to_new_page(newpage, page, remap_swapcache, mode);
> + rc = move_to_new_page(newpage, page, page_was_mapped, mode);
>
> - if (rc && remap_swapcache)
> + if (rc && page_was_mapped)
> remove_migration_ptes(page, page);
>
> /* Drop an anon_vma reference if we took one */
> @@ -1017,6 +1020,7 @@ static int unmap_and_move_huge_page(new_
> {
> int rc = 0;
> int *result = NULL;
> + int page_was_mapped = 0;
> struct page *new_hpage;
> struct anon_vma *anon_vma = NULL;
>
> @@ -1047,12 +1051,16 @@ static int unmap_and_move_huge_page(new_
> if (PageAnon(hpage))
> anon_vma = page_get_anon_vma(hpage);
>
> - try_to_unmap(hpage, TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS);
> + if (page_mapped(hpage)) {
> + try_to_unmap(hpage,
> + TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS);
> + page_was_mapped = 1;
> + }
>
> if (!page_mapped(hpage))
> - rc = move_to_new_page(new_hpage, hpage, 1, mode);
> + rc = move_to_new_page(new_hpage, hpage, page_was_mapped, mode);
>
> - if (rc != MIGRATEPAGE_SUCCESS)
> + if (rc != MIGRATEPAGE_SUCCESS && page_was_mapped)
> remove_migration_ptes(hpage, hpage);
>
> if (anon_vma)
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to [email protected]. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
>
On Mon, 1 Dec 2014, Yasuaki Ishimatsu wrote:
> (2014/12/01 13:52), Hugh Dickins wrote:
> > @@ -798,7 +798,7 @@ static int __unmap_and_move(struct page
> > int force, enum migrate_mode mode)
> > {
> > int rc = -EAGAIN;
> > - int remap_swapcache = 1;
> > + int page_was_mapped = 0;
> > struct anon_vma *anon_vma = NULL;
> >
> > if (!trylock_page(page)) {
> > @@ -870,7 +870,6 @@ static int __unmap_and_move(struct page
> > * migrated but are not remapped when migration
> > * completes
> > */
> > - remap_swapcache = 0;
> > } else {
> > goto out_unlock;
> > }
> > @@ -910,13 +909,17 @@ static int __unmap_and_move(struct page
> > }
> >
> > /* Establish migration ptes or remove ptes */
>
> > - try_to_unmap(page, TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS);
> > + if (page_mapped(page)) {
> > + try_to_unmap(page,
> > + TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS);
> > + page_was_mapped = 1;
> > + }
>
> Is there no possibility that page is swap cache? If page is swap cache,
> this code changes behavior of move_to_new_page(). Is it O.K.?
Certainly the page may be swap cache, but I don't see how the behavior
of move_to_new_page() is changed.
Do you mean how I removed that "remap_swapcache = 0;" line above, so that
it now looks as if move_to_new_page() may be called with page_was_mapped
1, where before it was called with remap_swapcache 0?
No: although it cannot be seen from the patch context, that reset
of remap_swapcache was in a block where we have a PageAnon page, but
page_get_anon_vma() failed to "get" the anon_vma for it: that means
that the page was not mapped, so page_was_mapped will be 0 too.
(I was going to add that the page might be faulted back in again by
the time we reach the page_mapped() test above try_to_unmap(), and
that yes I'd would be making a change in that case, but it does not
matter at all to diverge in racy cases. But actually even that cannot
happen, since faulting back swap needs page lock which we hold here.)
There is an argument that move_to_new_page() behavior should be
changed in the case of swap cache: since try_to_unmap() then uses
the ordinary swap instead of a migration entry, there's not much
point in going to remove swap entries afterwards; though it would
be good to make those pages present again. But I didn't try to
change that in this patch: this was just a lock contention thing.
Hugh
(2014/12/01 16:28), Hugh Dickins wrote:
> On Mon, 1 Dec 2014, Yasuaki Ishimatsu wrote:
>> (2014/12/01 13:52), Hugh Dickins wrote:
>>> @@ -798,7 +798,7 @@ static int __unmap_and_move(struct page
>>> int force, enum migrate_mode mode)
>>> {
>>> int rc = -EAGAIN;
>>> - int remap_swapcache = 1;
>>> + int page_was_mapped = 0;
>>> struct anon_vma *anon_vma = NULL;
>>>
>>> if (!trylock_page(page)) {
>>> @@ -870,7 +870,6 @@ static int __unmap_and_move(struct page
>>> * migrated but are not remapped when migration
>>> * completes
>>> */
>>> - remap_swapcache = 0;
>>> } else {
>>> goto out_unlock;
>>> }
>>> @@ -910,13 +909,17 @@ static int __unmap_and_move(struct page
>>> }
>>>
>>> /* Establish migration ptes or remove ptes */
>>
>>> - try_to_unmap(page, TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS);
>>> + if (page_mapped(page)) {
>>> + try_to_unmap(page,
>>> + TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS);
>>> + page_was_mapped = 1;
>>> + }
>>
>> Is there no possibility that page is swap cache? If page is swap cache,
>> this code changes behavior of move_to_new_page(). Is it O.K.?
>
> Certainly the page may be swap cache, but I don't see how the behavior
> of move_to_new_page() is changed.
>
> Do you mean how I removed that "remap_swapcache = 0;" line above, so that
> it now looks as if move_to_new_page() may be called with page_was_mapped
> 1, where before it was called with remap_swapcache 0?
Yes. I pointed it.
>
> No: although it cannot be seen from the patch context, that reset
> of remap_swapcache was in a block where we have a PageAnon page, but
> page_get_anon_vma() failed to "get" the anon_vma for it: that means
> that the page was not mapped, so page_was_mapped will be 0 too.
>
> (I was going to add that the page might be faulted back in again by
> the time we reach the page_mapped() test above try_to_unmap(), and
> that yes I'd would be making a change in that case, but it does not
> matter at all to diverge in racy cases. But actually even that cannot
> happen, since faulting back swap needs page lock which we hold here.)
>
> There is an argument that move_to_new_page() behavior should be
> changed in the case of swap cache: since try_to_unmap() then uses
> the ordinary swap instead of a migration entry, there's not much
> point in going to remove swap entries afterwards; though it would
> be good to make those pages present again. But I didn't try to
> change that in this patch: this was just a lock contention thing.
Thank you for the explanation.
I understood it.
Thanks,
Yasuaki Ishimatsu
>
> Hugh
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>