Date: Wed, 9 Mar 2016 18:40:09 +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: <20160309174009.GR14882@redhat.com>
References: <56b13381.v0wS03ZQEKxwivVW%akpm@linux-foundation.org>
 <20160203074835.GB32652@gmail.com>
 <20160304203018.GC5530@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20160304203018.GC5530@linux.intel.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3473
Lines: 104

Hello everyone,

On Fri, Mar 04, 2016 at 03:30:18PM -0500, Matthew Wilcox wrote:
> On Wed, Feb 03, 2016 at 08:48:35AM +0100, Ingo Molnar wrote:
> > > @@ -111,8 +111,10 @@ static inline pud_t native_pudp_get_and_
> > >  #ifdef CONFIG_SMP
> > >  	return native_make_pud(xchg(&pudp->pud, 0));
> > >  #else
> > > -	/* native_local_pudp_get_and_clear,
> > > -	   but duplicated because of cyclic dependency */
> > > +	/*
> > > +	 * native_local_pudp_get_and_clear, but duplicated because of cyclic
> > > +	 * dependency
> > > +	 */
> > >  	pud_t ret = *pudp;
> > >  	native_pud_clear(pudp);
> > >  	return ret;
> > 
> > When referring to functions in comments (or changelogs) please use () to make it 
> > clear on sight what is being referred to.
> > 
> > Also, please try to construct proper English sentences with verbs and such!
> > 
> > I.e. something like this would work for me:
> > 
> > > +	/*
> > > +	 * This is a duplicate of native_local_pudp_get_and_clear(),
> > > +	 * because we cannot use the original due to a cyclic header
> > > +	 * file dependency:
> > > +	 */
> > 
> > (Assuming I managed to decode the shorthand form properly.)
> 
> I have no idea what it means.  This is copy-and-change of the pmd version,
> which was originally commit db3eb96f4e6281b84dd33c8980dacc27f2efe177 by
> Andrea.

which I also copied from native_ptep_get_and_clear:

tatic 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
}

static inline pmd_t native_pmdp_get_and_clear(pmd_t *xp)
{
#ifdef CONFIG_SMP
       return native_make_pmd(xchg(&xp->pmd, 0));
#else
	/* native_local_pmdp_get_and_clear,
	   but duplicated because of cyclic dependency */
	   pmd_t ret = *xp;
	   native_pmd_clear(xp);
	   return ret;
#endif
}

So if you intend to expand the comment in native_pmdp_get_and_clear
that I added with my commit (from v2.6.38), I would suggest to also
improve the comment in native_ptep_get_and_clear.

I did only s/ptep/pmdp, the comment originated in commit
4891645e764d2e181b834509a689fcd12e890c10 (from v2.6.25).

The comment means native_local_pmdp_get_and_clear() couldn't be
called, or the build would break because of preprocessor include order
dependencies. I CC'ed Jeremy just in case, but I've no doubts about
the comment myself.

See also what native_local_pmdp_get_and_clear does..

static inline pmd_t native_local_pmdp_get_and_clear(pmd_t *pmdp)
{
	pmd_t res = *pmdp;

	native_pmd_clear(pmdp);
	return res;
}

It'd be sure fine to improve the comment, but a comment, even a short
one, was in order. If a solution is found for the include ordering,
one could call native_local_pmdp_get_and_clear there, so it was good
to keep that in mind. Nothing special about the pmd-THP part, this
build issue originated in the pte.

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.

Thanks,
Andrea