Messages in this thread | | | Date | Wed, 9 Mar 2016 18:40:09 +0100 | From | Andrea Arcangeli <> | Subject | Re: + x86-add-support-for-pud-sized-transparent-hugepages-checkpatch-fixes.patch added to -mm tree |
| |
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
| |