* [PATCH] mm/gup: dereference page table entry using helper
@ 2020-04-14 15:10 Alexander Gordeev
2020-04-14 16:32 ` Jason Gunthorpe
0 siblings, 1 reply; 6+ messages in thread
From: Alexander Gordeev @ 2020-04-14 15:10 UTC (permalink / raw)
To: linux-kernel; +Cc: Alexander Gordeev, Kirill A. Shutemov, linux-mm
Commit 0005d20 ("mm/gup: Move page table entry dereference
into helper function") wrapped access to page table entries
larger than sizeof(long) into a race-aware accessor. One of
the two dereferences in gup_fast path was however overlooked.
CC: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
CC: linux-mm@kvack.org
Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
---
mm/gup.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/gup.c b/mm/gup.c
index d53f7dd..eceb98b 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2208,7 +2208,7 @@ static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end,
if (!head)
goto pte_unmap;
- if (unlikely(pte_val(pte) != pte_val(*ptep))) {
+ if (unlikely(pte_val(pte) != pte_val(gup_get_pte(ptep)))) {
put_compound_head(head, 1, flags);
goto pte_unmap;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] mm/gup: dereference page table entry using helper 2020-04-14 15:10 [PATCH] mm/gup: dereference page table entry using helper Alexander Gordeev @ 2020-04-14 16:32 ` Jason Gunthorpe 2020-04-14 18:58 ` Ira Weiny 0 siblings, 1 reply; 6+ messages in thread From: Jason Gunthorpe @ 2020-04-14 16:32 UTC (permalink / raw) To: Alexander Gordeev; +Cc: linux-kernel, Kirill A. Shutemov, linux-mm On Tue, Apr 14, 2020 at 05:10:01PM +0200, Alexander Gordeev wrote: > Commit 0005d20 ("mm/gup: Move page table entry dereference > into helper function") wrapped access to page table entries > larger than sizeof(long) into a race-aware accessor. One of > the two dereferences in gup_fast path was however overlooked. > > CC: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > CC: linux-mm@kvack.org > Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com> > mm/gup.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/gup.c b/mm/gup.c > index d53f7dd..eceb98b 100644 > +++ b/mm/gup.c > @@ -2208,7 +2208,7 @@ static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end, > if (!head) > goto pte_unmap; > > - if (unlikely(pte_val(pte) != pte_val(*ptep))) { > + if (unlikely(pte_val(pte) != pte_val(gup_get_pte(ptep)))) { It doesn't seem like this needs the special helper as it is just checking that the pte hasn't changed, it doesn't need to be read exactly. But it probably should technically still be a READ_ONCE. Although I think the atomic inside try_grab_compound_head prevents any real problems. Jason ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/gup: dereference page table entry using helper 2020-04-14 16:32 ` Jason Gunthorpe @ 2020-04-14 18:58 ` Ira Weiny 2020-04-14 19:06 ` Jason Gunthorpe 0 siblings, 1 reply; 6+ messages in thread From: Ira Weiny @ 2020-04-14 18:58 UTC (permalink / raw) To: Jason Gunthorpe Cc: Alexander Gordeev, linux-kernel, Kirill A. Shutemov, linux-mm On Tue, Apr 14, 2020 at 01:32:34PM -0300, Jason Gunthorpe wrote: > On Tue, Apr 14, 2020 at 05:10:01PM +0200, Alexander Gordeev wrote: > > Commit 0005d20 ("mm/gup: Move page table entry dereference > > into helper function") wrapped access to page table entries > > larger than sizeof(long) into a race-aware accessor. One of > > the two dereferences in gup_fast path was however overlooked. > > > > CC: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > > CC: linux-mm@kvack.org > > Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com> > > mm/gup.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/mm/gup.c b/mm/gup.c > > index d53f7dd..eceb98b 100644 > > +++ b/mm/gup.c > > @@ -2208,7 +2208,7 @@ static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end, > > if (!head) > > goto pte_unmap; > > > > - if (unlikely(pte_val(pte) != pte_val(*ptep))) { > > + if (unlikely(pte_val(pte) != pte_val(gup_get_pte(ptep)))) { > > It doesn't seem like this needs the special helper as it is just > checking that the pte hasn't changed, it doesn't need to be read > exactly. > > But it probably should technically still be a READ_ONCE. Although I > think the atomic inside try_grab_compound_head prevents any real > problems. I think we should go for consistency here and use the helper function. Ira > > Jason > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/gup: dereference page table entry using helper 2020-04-14 18:58 ` Ira Weiny @ 2020-04-14 19:06 ` Jason Gunthorpe 2020-04-14 19:39 ` Ira Weiny 0 siblings, 1 reply; 6+ messages in thread From: Jason Gunthorpe @ 2020-04-14 19:06 UTC (permalink / raw) To: Ira Weiny; +Cc: Alexander Gordeev, linux-kernel, Kirill A. Shutemov, linux-mm On Tue, Apr 14, 2020 at 11:58:29AM -0700, Ira Weiny wrote: > On Tue, Apr 14, 2020 at 01:32:34PM -0300, Jason Gunthorpe wrote: > > On Tue, Apr 14, 2020 at 05:10:01PM +0200, Alexander Gordeev wrote: > > > Commit 0005d20 ("mm/gup: Move page table entry dereference > > > into helper function") wrapped access to page table entries > > > larger than sizeof(long) into a race-aware accessor. One of > > > the two dereferences in gup_fast path was however overlooked. > > > > > > CC: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > > > CC: linux-mm@kvack.org > > > Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com> > > > mm/gup.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/mm/gup.c b/mm/gup.c > > > index d53f7dd..eceb98b 100644 > > > +++ b/mm/gup.c > > > @@ -2208,7 +2208,7 @@ static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end, > > > if (!head) > > > goto pte_unmap; > > > > > > - if (unlikely(pte_val(pte) != pte_val(*ptep))) { > > > + if (unlikely(pte_val(pte) != pte_val(gup_get_pte(ptep)))) { > > > > It doesn't seem like this needs the special helper as it is just > > checking that the pte hasn't changed, it doesn't need to be read > > exactly. > > > > But it probably should technically still be a READ_ONCE. Although I > > think the atomic inside try_grab_compound_head prevents any real > > problems. > > I think we should go for consistency here and use the helper function. It seems quite expensive to do two more unncessary barriers.. Jason ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/gup: dereference page table entry using helper 2020-04-14 19:06 ` Jason Gunthorpe @ 2020-04-14 19:39 ` Ira Weiny 2020-04-14 19:45 ` Jason Gunthorpe 0 siblings, 1 reply; 6+ messages in thread From: Ira Weiny @ 2020-04-14 19:39 UTC (permalink / raw) To: Jason Gunthorpe Cc: Alexander Gordeev, linux-kernel, Kirill A. Shutemov, linux-mm On Tue, Apr 14, 2020 at 04:06:20PM -0300, Jason Gunthorpe wrote: > On Tue, Apr 14, 2020 at 11:58:29AM -0700, Ira Weiny wrote: > > On Tue, Apr 14, 2020 at 01:32:34PM -0300, Jason Gunthorpe wrote: > > > On Tue, Apr 14, 2020 at 05:10:01PM +0200, Alexander Gordeev wrote: > > > > Commit 0005d20 ("mm/gup: Move page table entry dereference > > > > into helper function") wrapped access to page table entries > > > > larger than sizeof(long) into a race-aware accessor. One of > > > > the two dereferences in gup_fast path was however overlooked. > > > > > > > > CC: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > > > > CC: linux-mm@kvack.org > > > > Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com> > > > > mm/gup.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/mm/gup.c b/mm/gup.c > > > > index d53f7dd..eceb98b 100644 > > > > +++ b/mm/gup.c > > > > @@ -2208,7 +2208,7 @@ static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end, > > > > if (!head) > > > > goto pte_unmap; > > > > > > > > - if (unlikely(pte_val(pte) != pte_val(*ptep))) { > > > > + if (unlikely(pte_val(pte) != pte_val(gup_get_pte(ptep)))) { > > > > > > It doesn't seem like this needs the special helper as it is just > > > checking that the pte hasn't changed, it doesn't need to be read > > > exactly. > > > > > > But it probably should technically still be a READ_ONCE. Although I > > > think the atomic inside try_grab_compound_head prevents any real > > > problems. > > > > I think we should go for consistency here and use the helper function. > > It seems quite expensive to do two more unncessary barriers.. But won't a failure to read the 'real' pte result in falling back to GUP slow? Not sure which is worse? And most arch's don't suffer from this... Ira ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/gup: dereference page table entry using helper 2020-04-14 19:39 ` Ira Weiny @ 2020-04-14 19:45 ` Jason Gunthorpe 0 siblings, 0 replies; 6+ messages in thread From: Jason Gunthorpe @ 2020-04-14 19:45 UTC (permalink / raw) To: Ira Weiny; +Cc: Alexander Gordeev, linux-kernel, Kirill A. Shutemov, linux-mm On Tue, Apr 14, 2020 at 12:39:53PM -0700, Ira Weiny wrote: > On Tue, Apr 14, 2020 at 04:06:20PM -0300, Jason Gunthorpe wrote: > > On Tue, Apr 14, 2020 at 11:58:29AM -0700, Ira Weiny wrote: > > > On Tue, Apr 14, 2020 at 01:32:34PM -0300, Jason Gunthorpe wrote: > > > > On Tue, Apr 14, 2020 at 05:10:01PM +0200, Alexander Gordeev wrote: > > > > > Commit 0005d20 ("mm/gup: Move page table entry dereference > > > > > into helper function") wrapped access to page table entries > > > > > larger than sizeof(long) into a race-aware accessor. One of > > > > > the two dereferences in gup_fast path was however overlooked. > > > > > > > > > > CC: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > > > > > CC: linux-mm@kvack.org > > > > > Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com> > > > > > mm/gup.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/mm/gup.c b/mm/gup.c > > > > > index d53f7dd..eceb98b 100644 > > > > > +++ b/mm/gup.c > > > > > @@ -2208,7 +2208,7 @@ static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end, > > > > > if (!head) > > > > > goto pte_unmap; > > > > > > > > > > - if (unlikely(pte_val(pte) != pte_val(*ptep))) { > > > > > + if (unlikely(pte_val(pte) != pte_val(gup_get_pte(ptep)))) { > > > > > > > > It doesn't seem like this needs the special helper as it is just > > > > checking that the pte hasn't changed, it doesn't need to be read > > > > exactly. > > > > > > > > But it probably should technically still be a READ_ONCE. Although I > > > > think the atomic inside try_grab_compound_head prevents any real > > > > problems. > > > > > > I think we should go for consistency here and use the helper function. > > > > It seems quite expensive to do two more unncessary barriers.. > > But won't a failure to read the 'real' pte result in falling back to GUP slow? If there is no concurrent writer then the direct read will give the same result. If there is a concurrent writer then it is a random race if fallback to gup slow is required. Jason ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-04-14 19:46 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-14 15:10 [PATCH] mm/gup: dereference page table entry using helper Alexander Gordeev 2020-04-14 16:32 ` Jason Gunthorpe 2020-04-14 18:58 ` Ira Weiny 2020-04-14 19:06 ` Jason Gunthorpe 2020-04-14 19:39 ` Ira Weiny 2020-04-14 19:45 ` Jason Gunthorpe
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).