From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Zijlstra Subject: Re: [PATCH v5 01/11] asm-generic/pgtable: Adds generic functions to monitor lockless pgtable walks Date: Fri, 4 Oct 2019 13:28:44 +0200 Message-ID: <20191004112844.GC19463@hirez.programming.kicks-ass.net> References: <20191003013325.2614-1-leonardo@linux.ibm.com> <20191003013325.2614-2-leonardo@linux.ibm.com> <20191003071145.GM4536@hirez.programming.kicks-ass.net> <20191003115141.GJ4581@hirez.programming.kicks-ass.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Leonardo Bras Cc: Song Liu , Michal Hocko , "Dmitry V. Levin" , Keith Busch , linux-mm@kvack.org, Paul Mackerras , Christoph Lameter , Ira Weiny , Dan Williams , Elena Reshetova , linux-arch@vger.kernel.org, Santosh Sivaraj , Davidlohr Bueso , "Aneesh Kumar K.V" , Bartlomiej Zolnierkiewicz , Mike Rapoport , Jason Gunthorpe , Vlastimil Babka , Mahesh Salgaonkar , Andrey Ryabinin , Alexey List-Id: linux-arch.vger.kernel.org On Thu, Oct 03, 2019 at 06:24:07PM -0300, Leonardo Bras wrote: > Hello Peter, thanks for the feedback! >=20 > On Thu, 2019-10-03 at 13:51 +0200, Peter Zijlstra wrote: > > On Thu, Oct 03, 2019 at 09:11:45AM +0200, Peter Zijlstra wrote: > > > On Wed, Oct 02, 2019 at 10:33:15PM -0300, Leonardo Bras wrote: > > > > diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pg= table.h > > > > index 818691846c90..3043ea9812d5 100644 > > > > --- a/include/asm-generic/pgtable.h > > > > +++ b/include/asm-generic/pgtable.h > > > > @@ -1171,6 +1171,64 @@ static inline bool arch_has_pfn_modify_check= (void) > > > > #endif > > > > #endif > > > > =20 > > > > +#ifndef __HAVE_ARCH_LOCKLESS_PGTBL_WALK_CONTROL > > > > +static inline unsigned long begin_lockless_pgtbl_walk(struct mm_st= ruct *mm) > > > > +{ > > > > + unsigned long irq_mask; > > > > + > > > > + if (IS_ENABLED(CONFIG_LOCKLESS_PAGE_TABLE_WALK_TRACKING)) > > > > + atomic_inc(&mm->lockless_pgtbl_walkers); > > >=20 > > > This will not work for file backed THP. Also, this is a fairly serious > > > contention point all on its own. > >=20 > > Kiryl says we have tmpfs-thp, this would be broken vs that, as would > > your (PowerPC) use of mm_cpumask() for that IPI. >=20 > Could you please explain it? > I mean, why this breaks tmpfs-thp? > Also, why mm_cpumask() is also broken? Because shared pages are not bound by a mm; or does it not share the thp state between mappings? > > And I still think all that wrong, you really shouldn't need to wait on > > munmap(). >=20 > That is something I need to better understand. I mean, before coming > with this patch, I thought exactly this: not serialize when on munmap.=20 >=20 > But on the way I was convinced it would not work on munmap. I need to > recall why, and if it was false to assume this, re-think the whole > solution. And once you (re)figure it out, please write it down. It is a crucial bit of the puzzle and needs to be part of the Changelogs. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from merlin.infradead.org ([205.233.59.134]:36116 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727254AbfJDLaA (ORCPT ); Fri, 4 Oct 2019 07:30:00 -0400 Date: Fri, 4 Oct 2019 13:28:44 +0200 From: Peter Zijlstra Subject: Re: [PATCH v5 01/11] asm-generic/pgtable: Adds generic functions to monitor lockless pgtable walks Message-ID: <20191004112844.GC19463@hirez.programming.kicks-ass.net> References: <20191003013325.2614-1-leonardo@linux.ibm.com> <20191003013325.2614-2-leonardo@linux.ibm.com> <20191003071145.GM4536@hirez.programming.kicks-ass.net> <20191003115141.GJ4581@hirez.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable In-Reply-To: Sender: linux-arch-owner@vger.kernel.org List-ID: To: Leonardo Bras Cc: Song Liu , Michal Hocko , "Dmitry V. Levin" , Keith Busch , linux-mm@kvack.org, Paul Mackerras , Christoph Lameter , Ira Weiny , Dan Williams , Elena Reshetova , linux-arch@vger.kernel.org, Santosh Sivaraj , Davidlohr Bueso , "Aneesh Kumar K.V" , Bartlomiej Zolnierkiewicz , Mike Rapoport , Jason Gunthorpe , Vlastimil Babka , Mahesh Salgaonkar , Andrey Ryabinin , Alexey Dobriyan , Ingo Molnar , Andrea Arcangeli , Ralph Campbell , Arnd Bergmann , Jann Horn , John Hubbard , Jesper Dangaard Brouer , Nicholas Piggin , =?iso-8859-1?B?Suly9G1l?= Glisse , Mathieu Desnoyers , kvm-ppc@vger.kernel.org, Thomas Gleixner , Reza Arbab , Allison Randal , Christian Brauner , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, Logan Gunthorpe , Souptick Joarder , Andrew Morton , linuxppc-dev@lists.ozlabs.org, Roman Gushchin , "Kirill A. Shutemov" , Al Viro Message-ID: <20191004112844.mfHN-6xPy1Re1TgZCIKgKXaCOPG9U-ovhcW-i7KYBXQ@z> On Thu, Oct 03, 2019 at 06:24:07PM -0300, Leonardo Bras wrote: > Hello Peter, thanks for the feedback! >=20 > On Thu, 2019-10-03 at 13:51 +0200, Peter Zijlstra wrote: > > On Thu, Oct 03, 2019 at 09:11:45AM +0200, Peter Zijlstra wrote: > > > On Wed, Oct 02, 2019 at 10:33:15PM -0300, Leonardo Bras wrote: > > > > diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pg= table.h > > > > index 818691846c90..3043ea9812d5 100644 > > > > --- a/include/asm-generic/pgtable.h > > > > +++ b/include/asm-generic/pgtable.h > > > > @@ -1171,6 +1171,64 @@ static inline bool arch_has_pfn_modify_check= (void) > > > > #endif > > > > #endif > > > > =20 > > > > +#ifndef __HAVE_ARCH_LOCKLESS_PGTBL_WALK_CONTROL > > > > +static inline unsigned long begin_lockless_pgtbl_walk(struct mm_st= ruct *mm) > > > > +{ > > > > + unsigned long irq_mask; > > > > + > > > > + if (IS_ENABLED(CONFIG_LOCKLESS_PAGE_TABLE_WALK_TRACKING)) > > > > + atomic_inc(&mm->lockless_pgtbl_walkers); > > >=20 > > > This will not work for file backed THP. Also, this is a fairly serious > > > contention point all on its own. > >=20 > > Kiryl says we have tmpfs-thp, this would be broken vs that, as would > > your (PowerPC) use of mm_cpumask() for that IPI. >=20 > Could you please explain it? > I mean, why this breaks tmpfs-thp? > Also, why mm_cpumask() is also broken? Because shared pages are not bound by a mm; or does it not share the thp state between mappings? > > And I still think all that wrong, you really shouldn't need to wait on > > munmap(). >=20 > That is something I need to better understand. I mean, before coming > with this patch, I thought exactly this: not serialize when on munmap.=20 >=20 > But on the way I was convinced it would not work on munmap. I need to > recall why, and if it was false to assume this, re-think the whole > solution. And once you (re)figure it out, please write it down. It is a crucial bit of the puzzle and needs to be part of the Changelogs.