From: Andrea Arcangeli <aarcange@redhat.com> To: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> Cc: Mel Gorman <mgorman@techsingularity.net>, Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] mprotect: drop overprotective lock_pte_protection() Date: Tue, 7 Feb 2017 22:29:35 +0100 [thread overview] Message-ID: <20170207212935.GL25530@redhat.com> (raw) In-Reply-To: <20170207143347.123871-1-kirill.shutemov@linux.intel.com> On Tue, Feb 07, 2017 at 05:33:47PM +0300, Kirill A. Shutemov wrote: > lock_pte_protection() uses pmd_lock() to make sure that we have stable > PTE page table before walking pte range. > > That's not necessary. We only need to make sure that PTE page table is > established. It cannot vanish under us as long as we hold mmap_sem at > least for read. > > And we already have helper for that -- pmd_trans_unstable(). > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > --- > mm/mprotect.c | 43 ++++++++++++------------------------------- > 1 file changed, 12 insertions(+), 31 deletions(-) > > diff --git a/mm/mprotect.c b/mm/mprotect.c > index f9c07f54dd62..e919e4613eab 100644 > --- a/mm/mprotect.c > +++ b/mm/mprotect.c > @@ -33,34 +33,6 @@ > > #include "internal.h" > > -/* > - * For a prot_numa update we only hold mmap_sem for read so there is a > - * potential race with faulting where a pmd was temporarily none. This > - * function checks for a transhuge pmd under the appropriate lock. It > - * returns a pte if it was successfully locked or NULL if it raced with > - * a transhuge insertion. > - */ > -static pte_t *lock_pte_protection(struct vm_area_struct *vma, pmd_t *pmd, > - unsigned long addr, int prot_numa, spinlock_t **ptl) > -{ > - pte_t *pte; > - spinlock_t *pmdl; > - > - /* !prot_numa is protected by mmap_sem held for write */ > - if (!prot_numa) > - return pte_offset_map_lock(vma->vm_mm, pmd, addr, ptl); > - > - pmdl = pmd_lock(vma->vm_mm, pmd); > - if (unlikely(pmd_trans_huge(*pmd) || pmd_none(*pmd))) { > - spin_unlock(pmdl); > - return NULL; > - } > - > - pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, ptl); > - spin_unlock(pmdl); > - return pte; > -} > - > static unsigned long change_pte_range(struct vm_area_struct *vma, pmd_t *pmd, > unsigned long addr, unsigned long end, pgprot_t newprot, > int dirty_accountable, int prot_numa) > @@ -71,7 +43,7 @@ static unsigned long change_pte_range(struct vm_area_struct *vma, pmd_t *pmd, > unsigned long pages = 0; > int target_node = NUMA_NO_NODE; > > - pte = lock_pte_protection(vma, pmd, addr, prot_numa, &ptl); > + pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl); > if (!pte) I cleaned it up too but I moved the pmd_trans_unstable in the caller above instead of the callee, otherwise it's the same. > > @@ -177,8 +149,6 @@ static inline unsigned long change_pmd_range(struct vm_area_struct *vma, > if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd)) { > if (next - addr != HPAGE_PMD_SIZE) { > __split_huge_pmd(vma, pmd, addr, false, NULL); > - if (pmd_trans_unstable(pmd)) > - continue; Agree it can be removed too, but I only removed lock_pte_protection in my version. If you prefer this version to be merged so we don't have to cleanup the above superfluous check in a incremental patch that's fine of course, otherwise at runtime they're equivalent as far as I can tell. The version in -mm is here. https://git.kernel.org/cgit/linux/kernel/git/mhocko/mm.git/commit/?h=auto-latest&id=d84ff4e4985f397ca4ecfe7ec029c45c6f2b9906 Thanks, Andrea
WARNING: multiple messages have this Message-ID (diff)
From: Andrea Arcangeli <aarcange@redhat.com> To: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> Cc: Mel Gorman <mgorman@techsingularity.net>, Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] mprotect: drop overprotective lock_pte_protection() Date: Tue, 7 Feb 2017 22:29:35 +0100 [thread overview] Message-ID: <20170207212935.GL25530@redhat.com> (raw) In-Reply-To: <20170207143347.123871-1-kirill.shutemov@linux.intel.com> On Tue, Feb 07, 2017 at 05:33:47PM +0300, Kirill A. Shutemov wrote: > lock_pte_protection() uses pmd_lock() to make sure that we have stable > PTE page table before walking pte range. > > That's not necessary. We only need to make sure that PTE page table is > established. It cannot vanish under us as long as we hold mmap_sem at > least for read. > > And we already have helper for that -- pmd_trans_unstable(). > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > --- > mm/mprotect.c | 43 ++++++++++++------------------------------- > 1 file changed, 12 insertions(+), 31 deletions(-) > > diff --git a/mm/mprotect.c b/mm/mprotect.c > index f9c07f54dd62..e919e4613eab 100644 > --- a/mm/mprotect.c > +++ b/mm/mprotect.c > @@ -33,34 +33,6 @@ > > #include "internal.h" > > -/* > - * For a prot_numa update we only hold mmap_sem for read so there is a > - * potential race with faulting where a pmd was temporarily none. This > - * function checks for a transhuge pmd under the appropriate lock. It > - * returns a pte if it was successfully locked or NULL if it raced with > - * a transhuge insertion. > - */ > -static pte_t *lock_pte_protection(struct vm_area_struct *vma, pmd_t *pmd, > - unsigned long addr, int prot_numa, spinlock_t **ptl) > -{ > - pte_t *pte; > - spinlock_t *pmdl; > - > - /* !prot_numa is protected by mmap_sem held for write */ > - if (!prot_numa) > - return pte_offset_map_lock(vma->vm_mm, pmd, addr, ptl); > - > - pmdl = pmd_lock(vma->vm_mm, pmd); > - if (unlikely(pmd_trans_huge(*pmd) || pmd_none(*pmd))) { > - spin_unlock(pmdl); > - return NULL; > - } > - > - pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, ptl); > - spin_unlock(pmdl); > - return pte; > -} > - > static unsigned long change_pte_range(struct vm_area_struct *vma, pmd_t *pmd, > unsigned long addr, unsigned long end, pgprot_t newprot, > int dirty_accountable, int prot_numa) > @@ -71,7 +43,7 @@ static unsigned long change_pte_range(struct vm_area_struct *vma, pmd_t *pmd, > unsigned long pages = 0; > int target_node = NUMA_NO_NODE; > > - pte = lock_pte_protection(vma, pmd, addr, prot_numa, &ptl); > + pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl); > if (!pte) I cleaned it up too but I moved the pmd_trans_unstable in the caller above instead of the callee, otherwise it's the same. > > @@ -177,8 +149,6 @@ static inline unsigned long change_pmd_range(struct vm_area_struct *vma, > if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd)) { > if (next - addr != HPAGE_PMD_SIZE) { > __split_huge_pmd(vma, pmd, addr, false, NULL); > - if (pmd_trans_unstable(pmd)) > - continue; Agree it can be removed too, but I only removed lock_pte_protection in my version. If you prefer this version to be merged so we don't have to cleanup the above superfluous check in a incremental patch that's fine of course, otherwise at runtime they're equivalent as far as I can tell. The version in -mm is here. https://git.kernel.org/cgit/linux/kernel/git/mhocko/mm.git/commit/?h=auto-latest&id=d84ff4e4985f397ca4ecfe7ec029c45c6f2b9906 Thanks, Andrea -- 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>
next prev parent reply other threads:[~2017-02-07 21:29 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-02-07 14:33 [PATCH] mprotect: drop overprotective lock_pte_protection() Kirill A. Shutemov 2017-02-07 14:33 ` Kirill A. Shutemov 2017-02-07 21:29 ` Andrea Arcangeli [this message] 2017-02-07 21:29 ` Andrea Arcangeli 2017-02-07 21:44 ` Andrew Morton 2017-02-07 21:44 ` Andrew Morton 2017-02-08 12:04 ` Kirill A. Shutemov 2017-02-08 12:04 ` Kirill A. Shutemov 2017-02-08 13:45 ` Andrea Arcangeli 2017-02-08 13:45 ` Andrea Arcangeli
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=20170207212935.GL25530@redhat.com \ --to=aarcange@redhat.com \ --cc=akpm@linux-foundation.org \ --cc=kirill.shutemov@linux.intel.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mgorman@techsingularity.net \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.