From mboxrd@z Thu Jan 1 00:00:00 1970 From: Leonardo Bras Date: Mon, 30 Sep 2019 15:14:31 +0000 Subject: Re: [PATCH v4 01/11] powerpc/mm: Adds counting method to monitor lockless pgtable walks Message-Id: <2533a13f226a6e1fab387669b6cced2aa8d2e129.camel@linux.ibm.com> MIME-Version: 1 Content-Type: multipart/mixed; boundary="=-z3BiED0HRCK0SvQzWnl4" List-Id: References: <20190927234008.11513-1-leonardo@linux.ibm.com> <20190927234008.11513-2-leonardo@linux.ibm.com> <4ff1e8e8-929b-9cfc-9bf8-ee88e34de888@nvidia.com> In-Reply-To: <4ff1e8e8-929b-9cfc-9bf8-ee88e34de888@nvidia.com> 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: Keith Busch , Thomas Gleixner , Arnd Bergmann , Greg Kroah-Hartman , YueHaibing , Nicholas Piggin , Mike Rapoport , Mahesh Salgaonkar , Jason Gunthorpe , Paul Mackerras , "Aneesh Kumar K.V" , Ganesh Goudar , Andrew Morton , Ira Weiny , Dan Williams , Allison Randal --=-z3BiED0HRCK0SvQzWnl4 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Sun, 2019-09-29 at 15:40 -0700, John Hubbard wrote: > Hi, Leonardo, Hello John, thanks for the feedback. > Can we please do it as shown below, instead (compile-tested only)? >=20 > This addresses all of the comments that I was going to make about structu= re > of this patch, which are: >=20 > * The lockless synch is tricky, so it should be encapsulated in function > calls if possible. As I told before, there are cases where this function is called from 'real mode' in powerpc, which doesn't disable irqs and may have a tricky behavior if we do. So, encapsulate the irq disable in this function can be a bad choice. Of course, if we really need that, we can add a bool parameter to the function to choose about disabling/enabling irqs. >=20 > * This is really a core mm function, so don't hide it away in arch layers= . > (If you're changing mm/ files, that's a big hint.) My idea here is to let the arch decide on how this 'register' is going to work, as archs may have different needs (in powerpc for example, we can't always disable irqs, since we may be in realmode). Maybe we can create a generic function instead of a dummy, and let it be replaced in case the arch needs to do so. > * Other things need parts of this: gup.c needs the memory barriers; IMHO = you'll > be fixing a pre-existing, theoretical (we've never seen bug reports) p= roblem. Humm, you are right. Here I would suggest adding the barrier to the generic function. > * The documentation needs to accurately explain what's going on here. Yes, my documentation was probably not good enough due to my lack of experience with memory barriers (I learnt about using them last week, and tried to come with the best solution.) > (Not shown: one or more of the PPC Kconfig files should select > LOCKLESS_PAGE_TABLE_WALK_TRACKING.) The way it works today is defining it on platform pgtable.h. I agree that using Kconfig may be a better solution that can make this config more visible to disable/enable.=20 Thanks for the feedback, Leonardo Bras --=-z3BiED0HRCK0SvQzWnl4 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEMdeUgIzgjf6YmUyOlQYWtz9SttQFAl2SG9cACgkQlQYWtz9S ttQkeBAAx0Ae0+jxZ1EEdiZHaIu+kj0HWBBAg+YW5DdbPJSTzQYna9TEMmAmBeT0 CQ7BI8sDaz54d/59tm5cNRPc142RU5w/QTRv7OE+0IQJBL1BxLb1XoaCTRqtnnts gf3fSukTddDt2zm3/8Gx9Q160AZdCLK1I4NtW1rtqFbUH3EFhCaPM4ydwhy9WOzO jpMacVjvfqJey8NKeo12RCkKLObhAdYwdJlfVM+j7srREfe/e9Hdz/hF+m5XYFt8 PlMTsWAuOpjEJG09E8yttF/Zog2ZRLnaFeY05tRcd9n3jqKXClMcP5En5nyAMl49 vSUr6+RE72lMt6xzK8mkX5V3WfTS6NvR4N95hLjLKwdW3gPAeZrXb+4SJRq/vWbE 24iALWVQZodVwQ44tEBFiVgSfPTSIjzBT6FoQkwqFYebo5gAYf3ObVM+OmFSHhmd GZ93Mnoj/3k2Em/NOwjGFSYbDZi5H3NHHTL04jJ3gGEqceGvd55OXtHfYIuDs6it 9hFQrU8n265wmsNJmcQGjj8NK62QIqsGcYLfNp1mVpseKVhakpDbLeBiG/PG/w7I FoSj52zaJVheB7tgSEEMkUxeBLgY7OmywnTedYFVhOFlwD/81wMkZAxz/fgvAmXK IxXI+ZP410dboTHEija3m4Z+m2BMb6wamF4Q1J5Ys8wWeQ3QhGc= =+FFG -----END PGP SIGNATURE----- --=-z3BiED0HRCK0SvQzWnl4--