From mboxrd@z Thu Jan 1 00:00:00 1970 From: Leonardo Bras Date: Tue, 01 Oct 2019 19:40:59 +0000 Subject: Re: [PATCH v4 03/11] mm/gup: Applies counting method to monitor gup_pgd_range Message-Id: <1f5f2f689385ceeb1240b4cc66ef3f4b66638ab0.camel@linux.ibm.com> MIME-Version: 1 Content-Type: multipart/mixed; boundary="=-4Fh/TZDFlMRM8Gq0A1+I" List-Id: References: <20190927234008.11513-1-leonardo@linux.ibm.com> <20190927234008.11513-4-leonardo@linux.ibm.com> <2cebe169221ae9270963d4bc4fd8e43066745f98.camel@linux.ibm.com> In-Reply-To: To: John Hubbard , linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, kvm-ppc@vger.kernel.org, linux-arch@vger.kernel.org, linux-mm@kvack.org Cc: Arnd Bergmann , Greg Kroah-Hartman , YueHaibing , Nicholas Piggin , Mike Rapoport , Keith Busch , Jason Gunthorpe , Paul Mackerras , "Aneesh Kumar K.V" , Allison Randal , Mahesh Salgaonkar , Ganesh Goudar , Thomas Gleixner , Ira Weiny , Andrew Morton , Dan Williams --=-4Fh/TZDFlMRM8Gq0A1+I Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 2019-10-01 at 12:04 -0700, John Hubbard wrote: > On 10/1/19 10:56 AM, Leonardo Bras wrote: > > On Mon, 2019-09-30 at 14:51 -0700, John Hubbard wrote: > > > On 9/27/19 4:40 PM, Leonardo Bras wrote: > ... > > > > diff --git a/mm/gup.c b/mm/gup.c > > > > index 98f13ab37bac..7105c829cf44 100644 > > > > --- a/mm/gup.c > > > > +++ b/mm/gup.c > > > > @@ -2325,6 +2325,7 @@ static bool gup_fast_permitted(unsigned long = start, unsigned long end) > > > > int __get_user_pages_fast(unsigned long start, int nr_pages, int w= rite, > > > > struct page **pages) > > > > { > > > > + struct mm_struct *mm; > > >=20 > > > I don't think that this local variable adds any value, so let's not u= se it. > > > Similar point in a few other patches too. > >=20 > > It avoids 1 deference of current->mm, it's a little performance gain. > >=20 >=20 > No, it isn't. :)=20 >=20 > Longer answer: at this level (by which I mean, "wrote the C code, haven't= looked > at the generated asm yet, and haven't done a direct perf test yet"), none= of us > C programmers are entitled to imagine that we can second guess both the c= ompiler=20 > and the CPU well enough to claim that declaring a local pointer variable= on the > stack will even *affect* performance, much less know which way it will go= ! >=20 I did this based on how costly can be 'current', and I could notice reduction in assembly size most of the time. (powerpc) But I get what you mean, maybe the (possible) performance gain don't worth the extra work. > The compiler at -O2 will *absolutely* optimize away any local variables t= hat > it doesn't need. >=20 > And that leads to how kernel programmers routinely decide about that kind= of=20 > variable: "does the variable's added clarity compensate for the extra vis= ual=20 > noise and for the need to manage the variable?" That's a good way to decide it. :) >=20 > Here, and in most (all?) other points in the patchset where you've added = an > mm local variable, the answer is no. >=20 Well, IMHO it's cleaner that way. But I get that other people may disagree.=20 >=20 > ... start_lockless_pgtbl_walk(mm); > > > Minor: I'd like to rename this register_lockless_pgtable_walker(). > > >=20 > > > > local_irq_disable(); > > > > gup_pgd_range(addr, end, gup_flags, pages, &nr); > > > > local_irq_enable(); > > > > + end_lockless_pgtbl_walk(mm); > > >=20 > > > ...and deregister_lockless_pgtable_walker(). > > >=20 > >=20 > > I have no problem changing the name, but I don't register/deregister > > are good terms for this.=20 > >=20 > > I would rather use start/finish, begin/end, and so on. Register sounds > > like something more complicated than what we are trying to achieve > > here.=20 > >=20 >=20 > OK, well, I don't want to bikeshed on naming more than I usually do, and= =20 > what you have is reasonable, so I'll leave that alone. :) >=20 > thanks, Thank for the feedback, --=-4Fh/TZDFlMRM8Gq0A1+I Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEMdeUgIzgjf6YmUyOlQYWtz9SttQFAl2Tq8sACgkQlQYWtz9S ttT8qxAAne6RW5q0WKv5peF0RKabbsIJL1OCaP6zH9hop0gMkhg22WNHTZvFgjXl OEtQ351gFTykOpobpF/1R7xkVELLy7utygCckNBGlSNa16DtHdpp9HWXMb94oNYx NrMrKA1+keF3gli3I2b2pVDgpRMp2qt5BNOm6D3kvmH64WWG43BnSq+okS6Cg8KS vkQzJKJ0kmoqPWy/uvV0SWGIGj4xWL5TII1ZiS0cnTSYFRyb0YJKmlL6BnT5kmLC qcFaZTfck+d6xSqAuoXeXLTKXsAH0enh/E0ignLxqOcaHULOeQ+jN7jg0vWFpMl6 Udb8pD4mH9Ff9nXadz7+w9aGxwJSSzMXMCIyemP1ny9Msl+5r/9yObv+cQ65fOiw q6+PQOx6DgHakjDNXa/RQiBOu0YyIKpP5RWRu/i1BBPkFipzjMnnvqvrNiZMhABT Isfod8XDatUBtXqXqzHsgTjnjuRwixriQ0EQbqaLOlIwoBq6aZcZ4QPGU6Vta1wj kFJZdzq9puwo14lZeMMsfGP+s+FT+9+zrrN05Ij1rYPDIdmmVFe3w7F7lDnP5alg RuXvFvBmhE1Xl2UqzB1VVcBWiphAjoE5vVlD21smjVYr5Y6+h9usTzp2BTQHeLVA VG39+Wsr6CYPVLw1Yab3YSJIFmCFxihM9Oui1yOh4vcBKrxa4ew= =37MB -----END PGP SIGNATURE----- --=-4Fh/TZDFlMRM8Gq0A1+I--