| Message ID | 20210506232537.165788-3-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/gup: Fix pin page write cache bouncing on has_pinned | expand |
On Thu, May 6, 2021 at 4:25 PM Peter Xu <peterx@redhat.com> wrote: > > + if (flags & FOLL_PIN && !atomic_read(&mm->has_pinned)) Please add parentheses to clarify code like this and make the grouping much more visually obvious. Yes, yes, '&' has higher precedence than '&&'. This is not about the compiler, this is about the humans reading it. So please write it as if ((flags & FOLL_PIN) && !atomic_read(&mm->has_pinned)) instead (this problem remains - and the fix is the same - in the 3/3 patch). Otherwise the series looks fine to me (although admittedly I do find the commit message to be ridiculously verbose for such a trivial patch - at some point the actual _point_ if it all gets hidden in the long commit message). Linus
On 5/6/21 4:25 PM, Peter Xu wrote: > From: Andrea Arcangeli <aarcange@redhat.com> > > has_pinned cannot be written by each pin-fast or it won't scale in > SMP. This isn't "false sharing" strictly speaking (it's more like > "true non-sharing"), but it creates the same SMP scalability > bottleneck of "false sharing". > > To verify the improvement a new "pin_fast.c" program was added to > the will-it-scale benchmark. ... > > This commits increases the SMP scalability of pin_user_pages_fast() > executed by different threads of the same process by more than 4000%. > Remarkable! I mean, yes, everyone knows that atomic writes are "expensive", but this is a fun, dramatic example of just *how* expensive they can get, once you start doing contended atomic writes. Reviewed-by: John Hubbard <jhubbard@nvidia.com> Other notes, that don't have any effect on the above reviewed-by tag: On the commit log, I will add a "+1" to the idea of deleting the pin_fast.c contents from the commit log, and just providing a URL instead. No need to put C programs in the commit log, IMHO, especially when you have them elsewhere anyway. thanks, -- John Hubbard NVIDIA > Signed-off-by: Andrea Arcangeli <aarcange@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > mm/gup.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/mm/gup.c b/mm/gup.c > index 63a079e361a3d..8b513e1723b45 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -1292,7 +1292,7 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm, > BUG_ON(*locked != 1); > } > > - if (flags & FOLL_PIN) > + if (flags & FOLL_PIN && !atomic_read(&mm->has_pinned)) > atomic_set(&mm->has_pinned, 1); > > /* > @@ -2617,7 +2617,7 @@ static int internal_get_user_pages_fast(unsigned long start, > FOLL_FAST_ONLY))) > return -EINVAL; > > - if (gup_flags & FOLL_PIN) > + if (gup_flags & FOLL_PIN && !atomic_read(¤t->mm->has_pinned)) > atomic_set(¤t->mm->has_pinned, 1); > > if (!(gup_flags & FOLL_FAST_ONLY)) > thanks,
On Thu, May 06, 2021 at 11:07:32PM -0700, John Hubbard wrote: > On 5/6/21 4:25 PM, Peter Xu wrote: > > From: Andrea Arcangeli <aarcange@redhat.com> > > > > has_pinned cannot be written by each pin-fast or it won't scale in > > SMP. This isn't "false sharing" strictly speaking (it's more like > > "true non-sharing"), but it creates the same SMP scalability > > bottleneck of "false sharing". > > > > To verify the improvement a new "pin_fast.c" program was added to > > the will-it-scale benchmark. > ... > > > > This commits increases the SMP scalability of pin_user_pages_fast() > > executed by different threads of the same process by more than 4000%. > > > > Remarkable! I mean, yes, everyone knows that atomic writes are > "expensive", but this is a fun, dramatic example of just *how* > expensive they can get, once you start doing contended atomic writes. > > > Reviewed-by: John Hubbard <jhubbard@nvidia.com> > > Other notes, that don't have any effect on the above reviewed-by > tag: > > On the commit log, I will add a "+1" to the idea of deleting the > pin_fast.c contents from the commit log, and just providing a URL > instead. No need to put C programs in the commit log, IMHO, especially > when you have them elsewhere anyway. I guess it's put there only because it does not exist elsewhere. :) I didn't run it, but I think it needs to be put into tests/ of if-it-scale repo and build, something like that. https://github.com/antonblanchard/will-it-scale/tree/master/tests But yeah since we've got the 1st patch which can also reproduce this, I'll reference that instead in the commit mesasge and I'll be able to shrink it. I'll also start to use parentheses as Linus suggested. My thanks to both of you on the quick comments!
diff --git a/mm/gup.c b/mm/gup.c index 63a079e361a3d..8b513e1723b45 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -1292,7 +1292,7 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm, BUG_ON(*locked != 1); } - if (flags & FOLL_PIN) + if (flags & FOLL_PIN && !atomic_read(&mm->has_pinned)) atomic_set(&mm->has_pinned, 1); /* @@ -2617,7 +2617,7 @@ static int internal_get_user_pages_fast(unsigned long start, FOLL_FAST_ONLY))) return -EINVAL; - if (gup_flags & FOLL_PIN) + if (gup_flags & FOLL_PIN && !atomic_read(¤t->mm->has_pinned)) atomic_set(¤t->mm->has_pinned, 1); if (!(gup_flags & FOLL_FAST_ONLY))