Date: Thu, 10 Mar 2016 00:08:58 +0100
From: Andrea Arcangeli <aarcange@redhat.com>
To: Matthew Wilcox <willy@linux.intel.com>
Cc: Ingo Molnar <mingo@kernel.org>, akpm@linux-foundation.org, hpa@zytor.com,
        mingo@elte.hu, tglx@linutronix.de, linux-kernel@vger.kernel.org,
        Jeremy Fitzhardinge <jeremy@goop.org>
Subject: Re: +
 x86-add-support-for-pud-sized-transparent-hugepages-checkpatch-fixes.patch
 added to -mm tree
Message-ID: <20160309230858.GS14882@redhat.com>
References: <56b13381.v0wS03ZQEKxwivVW%akpm@linux-foundation.org>
 <20160203074835.GB32652@gmail.com>
 <20160304203018.GC5530@linux.intel.com>
 <20160309174009.GR14882@redhat.com>
 <20160309184540.GC2464@linux.intel.com>
 <20160309200345.GA26114@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20160309200345.GA26114@linux.intel.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2568
Lines: 72

On Wed, Mar 09, 2016 at 03:03:45PM -0500, Matthew Wilcox wrote:
> On Wed, Mar 09, 2016 at 01:45:40PM -0500, Matthew Wilcox wrote:
> > > In fact even before starting to fix the comment, I would recommend to
> > > try again to call native_local_pmdp_get_and_clear and
> > > native_local_ptep_get_and_clear to verify if it still breaks, just in
> > > case the include ordering got fixed by accident in the meanwhile (that
> > > was a comment in 2.6.25 when arch/x86/include/asm didn't even exist
> > > yet, it was still in include/asm-x86). If it would manage to build
> > > without the manual expansion, the comment could go and the duplication
> > > as well.
> > 
> > The ordering problem is still there.  native_local_ptep_get_and_clear()
> > is declared at line 726 of asm/pgtable.h and asm/pgtable_64.h is included
> > at line 466 of asm/pgtable.h.
> > 
> > I'll have a little play; see if I can resolve this ...
> 
> asm/pgtable.h:466
> # include <asm/pgtable_64.h>
> 
> asm/pgtable.h:726
> static inline pte_t native_local_ptep_get_and_clear(pte_t *ptep)
> {
>         pte_t res = *ptep;
> 
>         /* Pure native function needs no input for mm, addr */
>         native_pte_clear(NULL, 0, ptep);
>         return res;
> }
> 
> asm/pgtable_64.h:47
> static inline void native_pte_clear(struct mm_struct *mm, unsigned long addr,
>                                     pte_t *ptep)
> {
>         *ptep = native_make_pte(0);
> }
> 
> asm/pgtable_64.h:73
> static inline pte_t native_ptep_get_and_clear(pte_t *xp)
> {
> #ifdef CONFIG_SMP
>         return native_make_pte(xchg(&xp->pte, 0));
> #else
>         /* native_local_ptep_get_and_clear,
>            but duplicated because of cyclic dependency */
>         pte_t ret = *xp;
>         native_pte_clear(NULL, 0, xp);
>         return ret;
> #endif
> }
> 
> Why don't we just convert native_ptep_get_and_clear to work the same way that
> pgtable-2level and pgtable-3level work?  ie:
> 
> #ifdef CONFIG_SMP
> static inline pte_t native_ptep_get_and_clear(pte_t *xp)
> {
>         return native_make_pte(xchg(&xp->pte, 0));
> }
> #else
> #define native_ptep_get_and_clear(xp) native_local_ptep_get_and_clear(xp)
> #endif
> 
> Or perhaps better, centralise the non-SMP definitions:
> 
>  arch/x86/include/asm/pgtable-2level.h |  6 ------
>  arch/x86/include/asm/pgtable-3level.h |  7 +------
>  arch/x86/include/asm/pgtable.h        |  5 +++++
>  arch/x86/include/asm/pgtable_64.h     | 18 ++----------------
>  4 files changed, 8 insertions(+), 28 deletions(-)

Reviewed-by: Andrea Arcangeli <aarcange@redhat.com>