From: Nadav Amit <nadav.amit@gmail.com>
To: Jerome Glisse <jglisse@redhat.com>
Cc: "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
"open list:MEMORY MANAGEMENT" <linux-mm@kvack.org>,
"Dan Williams" <dan.j.williams@intel.com>,
"Ross Zwisler" <ross.zwisler@linux.intel.com>,
"Linus Torvalds" <torvalds@linux-foundation.org>,
"Bernhard Held" <berny156@gmx.de>,
"Adam Borowski" <kilobyte@angband.pl>,
"Andrea Arcangeli" <aarcange@redhat.com>,
"Radim Krčmář" <rkrcmar@redhat.com>,
"Wanpeng Li" <kernellwp@gmail.com>,
"Paolo Bonzini" <pbonzini@redhat.com>,
"Takashi Iwai" <tiwai@suse.de>, "Mike Galbraith" <efault@gmx.de>,
"Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,
axie <axie@amd.com>, "Andrew Morton" <akpm@linux-foundation.org>
Subject: Re: [PATCH 02/13] mm/rmap: update to new mmu_notifier semantic
Date: Tue, 29 Aug 2017 20:16:44 -0700 [thread overview]
Message-ID: <D65EED38-F1F2-4378-9457-21371EFD66F0@gmail.com> (raw)
In-Reply-To: <20170830025910.GB2386@redhat.com>
Jerome Glisse <jglisse@redhat.com> wrote:
> On Tue, Aug 29, 2017 at 07:46:07PM -0700, Nadav Amit wrote:
>> Jérôme Glisse <jglisse@redhat.com> wrote:
>>
>>> Replacing all mmu_notifier_invalidate_page() by mmu_notifier_invalidat_range()
>>> and making sure it is bracketed by call to mmu_notifier_invalidate_range_start/
>>> end.
>>>
>>> Note that because we can not presume the pmd value or pte value we have to
>>> assume the worse and unconditionaly report an invalidation as happening.
>>>
>>> Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
>>> Cc: Dan Williams <dan.j.williams@intel.com>
>>> Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
>>> Cc: Linus Torvalds <torvalds@linux-foundation.org>
>>> Cc: Bernhard Held <berny156@gmx.de>
>>> Cc: Adam Borowski <kilobyte@angband.pl>
>>> Cc: Andrea Arcangeli <aarcange@redhat.com>
>>> Cc: Radim Krčmář <rkrcmar@redhat.com>
>>> Cc: Wanpeng Li <kernellwp@gmail.com>
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Cc: Takashi Iwai <tiwai@suse.de>
>>> Cc: Nadav Amit <nadav.amit@gmail.com>
>>> Cc: Mike Galbraith <efault@gmx.de>
>>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>> Cc: axie <axie@amd.com>
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> ---
>>> mm/rmap.c | 44 +++++++++++++++++++++++++++++++++++++++++---
>>> 1 file changed, 41 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/mm/rmap.c b/mm/rmap.c
>>> index c8993c63eb25..da97ed525088 100644
>>> --- a/mm/rmap.c
>>> +++ b/mm/rmap.c
>>> @@ -887,11 +887,21 @@ static bool page_mkclean_one(struct page *page, struct vm_area_struct *vma,
>>> .address = address,
>>> .flags = PVMW_SYNC,
>>> };
>>> + unsigned long start = address, end;
>>> int *cleaned = arg;
>>>
>>> + /*
>>> + * We have to assume the worse case ie pmd for invalidation. Note that
>>> + * the page can not be free from this function.
>>> + */
>>> + end = min(vma->vm_end, (start & PMD_MASK) + PMD_SIZE);
>>> + mmu_notifier_invalidate_range_start(vma->vm_mm, start, end);
>>> +
>>> while (page_vma_mapped_walk(&pvmw)) {
>>> + unsigned long cstart, cend;
>>> int ret = 0;
>>> - address = pvmw.address;
>>> +
>>> + cstart = address = pvmw.address;
>>> if (pvmw.pte) {
>>> pte_t entry;
>>> pte_t *pte = pvmw.pte;
>>> @@ -904,6 +914,7 @@ static bool page_mkclean_one(struct page *page, struct vm_area_struct *vma,
>>> entry = pte_wrprotect(entry);
>>> entry = pte_mkclean(entry);
>>> set_pte_at(vma->vm_mm, address, pte, entry);
>>> + cend = cstart + PAGE_SIZE;
>>> ret = 1;
>>> } else {
>>> #ifdef CONFIG_TRANSPARENT_HUGE_PAGECACHE
>>> @@ -918,6 +929,8 @@ static bool page_mkclean_one(struct page *page, struct vm_area_struct *vma,
>>> entry = pmd_wrprotect(entry);
>>> entry = pmd_mkclean(entry);
>>> set_pmd_at(vma->vm_mm, address, pmd, entry);
>>> + cstart &= PMD_MASK;
>>> + cend = cstart + PMD_SIZE;
>>> ret = 1;
>>> #else
>>> /* unexpected pmd-mapped page? */
>>> @@ -926,11 +939,13 @@ static bool page_mkclean_one(struct page *page, struct vm_area_struct *vma,
>>> }
>>>
>>> if (ret) {
>>> - mmu_notifier_invalidate_page(vma->vm_mm, address);
>>> + mmu_notifier_invalidate_range(vma->vm_mm, cstart, cend);
>>> (*cleaned)++;
>>> }
>>> }
>>>
>>> + mmu_notifier_invalidate_range_end(vma->vm_mm, start, end);
>>> +
>>> return true;
>>> }
>>>
>>> @@ -1324,6 +1339,7 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>>> pte_t pteval;
>>> struct page *subpage;
>>> bool ret = true;
>>> + unsigned long start = address, end;
>>> enum ttu_flags flags = (enum ttu_flags)arg;
>>>
>>> /* munlock has nothing to gain from examining un-locked vmas */
>>> @@ -1335,6 +1351,14 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>>> flags & TTU_MIGRATION, page);
>>> }
>>>
>>> + /*
>>> + * We have to assume the worse case ie pmd for invalidation. Note that
>>> + * the page can not be free in this function as call of try_to_unmap()
>>> + * must hold a reference on the page.
>>> + */
>>> + end = min(vma->vm_end, (start & PMD_MASK) + PMD_SIZE);
>>> + mmu_notifier_invalidate_range_start(vma->vm_mm, start, end);
>>> +
>>> while (page_vma_mapped_walk(&pvmw)) {
>>> /*
>>> * If the page is mlock()d, we cannot swap it out.
>>> @@ -1408,6 +1432,8 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>>> set_huge_swap_pte_at(mm, address,
>>> pvmw.pte, pteval,
>>> vma_mmu_pagesize(vma));
>>> + mmu_notifier_invalidate_range(mm, address,
>>> + address + vma_mmu_pagesize(vma));
>>
>> I don’t think that the notifier should be called after the PTE is set, but
>> after the PTE is cleared, PTE permissions are demoted (e.g., RW->RO) or
>> access/dirty bits are cleared. [There is an exception: if the PFN in the PTE
>> is changed without clearing the PTE before, but it does not apply here, and
>> there is a different notifier for this case.]
>>
>> Therefore, IIUC, try_to_umap_one() should only call
>> mmu_notifier_invalidate_range() after ptep_get_and_clear() and
>> ptep_clear_flush() are called. All the other calls to
>> mmu_notifier_invalidate_range() in this function can be removed.
>
> Yes it would simplify the patch, i was trying to optimize for the case
> where we restore the pte to its original value after ptep_clear_flush()
> or ptep_get_and_clear() as in this case there is no need to invalidate
> any secondary page table but that's an overkill optimization that just
> add too much complexity.
Interesting. Actually, prior to your changes, it seems that the break
statements would skip mmu_notifier_invalidate_page() when the PTE is not
changed. So why not just change mmu_notifier_invalidate_page() into
mmu_notifier_invalidate_range() ?
Regards,
Nadav
next prev parent reply other threads:[~2017-08-30 3:16 UTC|newest]
Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-29 23:54 [PATCH 00/13] mmu_notifier kill invalidate_page callback Jérôme Glisse
2017-08-29 23:54 ` [PATCH 01/13] dax: update to new mmu_notifier semantic Jérôme Glisse
2017-08-29 23:54 ` [PATCH 02/13] mm/rmap: " Jérôme Glisse
2017-08-30 2:46 ` Nadav Amit
2017-08-30 2:59 ` Jerome Glisse
2017-08-30 3:16 ` Nadav Amit [this message]
2017-08-30 3:18 ` Nadav Amit
2017-08-30 17:27 ` Andrea Arcangeli
2017-08-30 18:00 ` Nadav Amit
2017-08-30 21:25 ` Andrea Arcangeli
2017-08-30 23:25 ` Nadav Amit
2017-08-31 0:47 ` Jerome Glisse
2017-08-31 17:12 ` Andrea Arcangeli
2017-08-31 19:15 ` Nadav Amit
2017-08-30 18:20 ` Jerome Glisse
2017-08-30 18:40 ` Nadav Amit
2017-08-30 20:45 ` Jerome Glisse
2017-08-30 22:17 ` Andrea Arcangeli
2017-08-30 20:55 ` Andrea Arcangeli
2017-08-30 16:52 ` Andrea Arcangeli
2017-08-30 17:48 ` Jerome Glisse
2017-08-30 21:53 ` Linus Torvalds
2017-08-30 23:01 ` Andrea Arcangeli
2017-08-31 18:25 ` Jerome Glisse
2017-08-31 19:40 ` Linus Torvalds
2017-08-29 23:54 ` [PATCH 03/13] powerpc/powernv: " Jérôme Glisse
2017-08-29 23:54 ` [PATCH 04/13] drm/amdgpu: " Jérôme Glisse
2017-08-30 6:18 ` Christian König
2017-08-29 23:54 ` [PATCH 05/13] IB/umem: " Jérôme Glisse
2017-08-30 6:13 ` Leon Romanovsky
2017-08-29 23:54 ` [PATCH 06/13] IB/hfi1: " Jérôme Glisse
2017-09-06 14:08 ` Arumugam, Kamenee
2017-08-29 23:54 ` [PATCH 07/13] iommu/amd: " Jérôme Glisse
2017-08-29 23:54 ` [PATCH 08/13] iommu/intel: " Jérôme Glisse
2017-08-29 23:54 ` [PATCH 09/13] misc/mic/scif: " Jérôme Glisse
2017-08-29 23:54 ` [PATCH 10/13] sgi-gru: " Jérôme Glisse
2017-08-29 23:54 ` [PATCH 11/13] xen/gntdev: " Jérôme Glisse
2017-08-29 23:54 ` [PATCH 12/13] KVM: " Jérôme Glisse
2017-08-29 23:54 ` [PATCH 13/13] mm/mmu_notifier: kill invalidate_page Jérôme Glisse
2017-08-30 0:11 ` [PATCH 00/13] mmu_notifier kill invalidate_page callback Linus Torvalds
2017-08-30 0:56 ` Jerome Glisse
2017-08-30 8:40 ` Mike Galbraith
2017-08-30 14:57 ` Adam Borowski
2017-09-01 14:47 ` Jeff Cook
2017-11-30 9:33 ` BSOD with " Fabian Grünbichler
2017-11-30 11:20 ` Paolo Bonzini
2017-11-30 16:19 ` Radim Krčmář
2017-11-30 18:05 ` [PATCH 1/2] KVM: x86: fix APIC page invalidation Radim Krčmář
2017-11-30 18:05 ` [PATCH 2/2] TESTING! KVM: x86: add invalidate_range mmu notifier Radim Krčmář
2017-12-01 15:15 ` Paolo Bonzini
2017-12-03 17:24 ` Andrea Arcangeli
2017-12-01 12:21 ` [PATCH 1/2] KVM: x86: fix APIC page invalidation Fabian Grünbichler
2017-12-01 15:27 ` Paolo Bonzini
2017-12-03 17:28 ` Andrea Arcangeli
2017-12-06 2:32 ` Wanpeng Li
2017-12-06 9:50 ` 王金浦
2017-12-06 10:00 ` Paolo Bonzini
2017-12-06 8:15 ` Fabian Grünbichler
2017-12-13 12:54 ` Richard Purdie
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=D65EED38-F1F2-4378-9457-21371EFD66F0@gmail.com \
--to=nadav.amit@gmail.com \
--cc=aarcange@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=axie@amd.com \
--cc=berny156@gmx.de \
--cc=dan.j.williams@intel.com \
--cc=efault@gmx.de \
--cc=jglisse@redhat.com \
--cc=kernellwp@gmail.com \
--cc=kilobyte@angband.pl \
--cc=kirill.shutemov@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=pbonzini@redhat.com \
--cc=rkrcmar@redhat.com \
--cc=ross.zwisler@linux.intel.com \
--cc=tiwai@suse.de \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).