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.
It seems unfair to ask me to do better than what is there right now.
* Matthew Wilcox <[email protected]> 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.
It means that we don't want to copy-and-change a crappy comment that slipped
through 5 years ago, we want to copy-and-improve. I even suggested the comment
improvement (which needs to be checked though).
> It seems unfair to ask me to do better than what is there right now.
It's absolutely fair for maintainers to require the improvement of existing code
you want to modify, especially when you are complicating existing code ...
Thanks,
Ingo
On Wed, Mar 09, 2016 at 01:08:08PM +0100, Ingo Molnar wrote:
> * Matthew Wilcox <[email protected]> wrote:
> > I have no idea what it means. This is copy-and-change of the pmd version,
> > which was originally commit db3eb96f4e6281b84dd33c8980dacc27f2efe177 by
> > Andrea.
>
> It means that we don't want to copy-and-change a crappy comment that slipped
> through 5 years ago, we want to copy-and-improve. I even suggested the comment
> improvement (which needs to be checked though).
The "it" in my sentence referred to the comment. I have no idea what
the comment is supposed to mean. I am the worst person to figure out
what the comment is supposed to mean as I have the least experience with
the code here.
The PUD and PMD code should be as similar as possible, down to the
comments and the spacing. If you want the original fixed, that's fine,
and I'm willing to include it as part of this patch set. But it's not
my responsibility to fix up the comments that you don't like.
> > It seems unfair to ask me to do better than what is there right now.
>
> It's absolutely fair for maintainers to require the improvement of existing code
> you want to modify, especially when you are complicating existing code ...
I'm not complicating it. I'm duplicating it.
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
On Wed, Mar 09, 2016 at 06:40:09PM +0100, Andrea Arcangeli wrote:
> On Fri, Mar 04, 2016 at 03:30:18PM -0500, Matthew Wilcox wrote:
> > 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:
Hah ;-)
> 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.
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 ...
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(-)
diff --git a/arch/x86/include/asm/pgtable-2level.h b/arch/x86/include/asm/pgtable-2level.h
index fd74a11..520318f 100644
--- a/arch/x86/include/asm/pgtable-2level.h
+++ b/arch/x86/include/asm/pgtable-2level.h
@@ -42,17 +42,11 @@ static inline pte_t native_ptep_get_and_clear(pte_t *xp)
{
return __pte(xchg(&xp->pte_low, 0));
}
-#else
-#define native_ptep_get_and_clear(xp) native_local_ptep_get_and_clear(xp)
-#endif
-#ifdef CONFIG_SMP
static inline pmd_t native_pmdp_get_and_clear(pmd_t *xp)
{
return __pmd(xchg((pmdval_t *)xp, 0));
}
-#else
-#define native_pmdp_get_and_clear(xp) native_local_pmdp_get_and_clear(xp)
#endif
/* Bit manipulation helper on pte/pgoff entry */
diff --git a/arch/x86/include/asm/pgtable-3level.h b/arch/x86/include/asm/pgtable-3level.h
index cdaa58c..b1b6412 100644
--- a/arch/x86/include/asm/pgtable-3level.h
+++ b/arch/x86/include/asm/pgtable-3level.h
@@ -149,11 +149,7 @@ static inline pte_t native_ptep_get_and_clear(pte_t *ptep)
return res;
}
-#else
-#define native_ptep_get_and_clear(xp) native_local_ptep_get_and_clear(xp)
-#endif
-#ifdef CONFIG_SMP
union split_pmd {
struct {
u32 pmd_low;
@@ -161,6 +157,7 @@ union split_pmd {
};
pmd_t pmd;
};
+
static inline pmd_t native_pmdp_get_and_clear(pmd_t *pmdp)
{
union split_pmd res, *orig = (union split_pmd *)pmdp;
@@ -172,8 +169,6 @@ static inline pmd_t native_pmdp_get_and_clear(pmd_t *pmdp)
return res.pmd;
}
-#else
-#define native_pmdp_get_and_clear(xp) native_local_pmdp_get_and_clear(xp)
#endif
/* Encode and de-code a swap entry */
diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index 1ff49ec..35306ca 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -740,6 +740,11 @@ static inline pmd_t native_local_pmdp_get_and_clear(pmd_t *pmdp)
return res;
}
+#ifndef CONFIG_SMP
+#define native_ptep_get_and_clear(p) native_local_ptep_get_and_clear(p)
+#define native_pmdp_get_and_clear(p) native_local_pmdp_get_and_clear(p)
+#endif
+
static inline void native_set_pte_at(struct mm_struct *mm, unsigned long addr,
pte_t *ptep , pte_t pte)
{
diff --git a/arch/x86/include/asm/pgtable_64.h b/arch/x86/include/asm/pgtable_64.h
index 2ee7811..a0c0219 100644
--- a/arch/x86/include/asm/pgtable_64.h
+++ b/arch/x86/include/asm/pgtable_64.h
@@ -70,31 +70,17 @@ static inline void native_pmd_clear(pmd_t *pmd)
native_set_pmd(pmd, native_make_pmd(0));
}
+#ifdef CONFIG_SMP
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
}
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
}
+#endif
static inline void native_set_pud(pud_t *pudp, pud_t pud)
{
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 <[email protected]>
* Matthew Wilcox <[email protected]> wrote:
> On Wed, Mar 09, 2016 at 01:08:08PM +0100, Ingo Molnar wrote:
> > * Matthew Wilcox <[email protected]> wrote:
> > > I have no idea what it means. This is copy-and-change of the pmd version,
> > > which was originally commit db3eb96f4e6281b84dd33c8980dacc27f2efe177 by
> > > Andrea.
> >
> > It means that we don't want to copy-and-change a crappy comment that slipped
> > through 5 years ago, we want to copy-and-improve. I even suggested the comment
> > improvement (which needs to be checked though).
>
> The "it" in my sentence referred to the comment. I have no idea what
> the comment is supposed to mean. I am the worst person to figure out
> what the comment is supposed to mean as I have the least experience with
> the code here.
>
> The PUD and PMD code should be as similar as possible, down to the
> comments and the spacing. If you want the original fixed, that's fine,
> and I'm willing to include it as part of this patch set. But it's not
> my responsibility to fix up the comments that you don't like.
>
> > > It seems unfair to ask me to do better than what is there right now.
> >
> > It's absolutely fair for maintainers to require the improvement of existing code
> > you want to modify, especially when you are complicating existing code ...
>
> I'm not complicating it. I'm duplicating it.
I don't think your language lawyering is particularly constructive: you are adding
new functionality to existing x86 code, and as such you need to address review
feedback from x86 maintainers - even if it involves old code.
( There's an obvious maintainability threshold concern behind such requests from
maintainers: existing bad practices in old code accumulate, and the code can
bear only so much complexity, so there's a level over which we require cleanups
to existing code before we accept new changes. )
This is nothing new, this happens all the time, it's a routine review practice
when new patches are applied.
Anyway, until my concerns are addressed the x86 bits are NAK-ed:
NAKed-by: Ingo Molnar <[email protected]>
Thanks,
Ingo
On Thu, Mar 10, 2016 at 10:37:50AM +0100, Ingo Molnar wrote:
> > > > It seems unfair to ask me to do better than what is there right now.
> > >
> > > It's absolutely fair for maintainers to require the improvement of existing code
> > > you want to modify, especially when you are complicating existing code ...
> >
> > I'm not complicating it. I'm duplicating it.
>
> I don't think your language lawyering is particularly constructive: you are adding
> new functionality to existing x86 code, and as such you need to address review
> feedback from x86 maintainers - even if it involves old code.
Absolutely. But if I've just copied-and-pasted code from elsewhere in
the same file, then I have no obligation to fix style problems. Indeed,
with the variety of styles throughout the kernel, following local style
is clearly the right thing to do for someone who is at best an occasional
contributor to a particular file.
> Anyway, until my concerns are addressed the x86 bits are NAK-ed:
>
> NAKed-by: Ingo Molnar <[email protected]>
You're adorable. You'd reject this feature over the format of a comment
that you don't like. And you'd rather argue about whether I should fix
the comment than review the patch elsewhere in this thread where I remove
the need for the comment.
It's a good thing Andrea is constructive.