[PATCH 0/3] eldie generated code for folded p4d/pud

[PATCH 0/3] eldie generated code for folded p4d/pud

[Vineet Gupta]

Hi,

This series elides extraneous generate code for folded p4d/pud.
This came up when trying to remove __ARCH_USE_5LEVEL_HACK from ARC port.
The code saving are not a while lot, but still worthwhile IMHO.

bloat-o-meter2 vmlinux-A-baseline vmlinux-E-elide-p?d_clear_bad
add/remove: 0/2 grow/shrink: 0/1 up/down: 0/-146 (-146)
function                                     old     new   delta
p4d_clear_bad                                  2       -      -2
pud_clear_bad                                 20       -     -20
free_pgd_range                               546     422    -124
Total: Before=4137148, After=4137002, chg -1.000000%

Thx,
-Vineet

Vineet Gupta (3):
  asm-generic/tlb: stub out pud_free_tlb() if __PAGETABLE_PUD_FOLDED ...
  asm-generic/tlb: stub out p4d_free_tlb() if __PAGETABLE_P4D_FOLDED ...
  asm-generic/mm: stub out p{4,d}d_clear_bad() if
    __PAGETABLE_P{4,u}D_FOLDED

 include/asm-generic/4level-fixup.h |  2 --
 include/asm-generic/5level-fixup.h |  2 --
 include/asm-generic/pgtable.h      | 11 +++++++++++
 include/asm-generic/tlb.h          |  8 ++++++--
 mm/pgtable-generic.c               |  4 ++++
 5 files changed, 21 insertions(+), 6 deletions(-)

-- 
2.20.1

[PATCH 1/3] asm-generic/tlb: stub out pud_free_tlb() if __PAGETABLE_PUD_FOLDED ...

[Vineet Gupta]

... independent of __ARCH_HAS_4LEVEL_HACK

This came up when removing __ARCH_HAS_5LEVEL_HACK for ARC as code bloat
from pud_free_tlb() despite pud being folded (with 2 levels on ARC)

| bloat-o-meter2 vmlinux-B-elide-ARCH_USE_5LEVEL_HACK vmlinux-C-elide-pud_free_tlb
| add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-104 (-104)
| function                                     old     new   delta
| free_pgd_range                               656     552    -104
| Total: Before=4137276, After=4137172, chg -1.000000%

Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 include/asm-generic/4level-fixup.h | 2 --
 include/asm-generic/tlb.h          | 4 +++-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/asm-generic/4level-fixup.h b/include/asm-generic/4level-fixup.h
index e3667c9a33a5..d7c5ba1968d3 100644
--- a/include/asm-generic/4level-fixup.h
+++ b/include/asm-generic/4level-fixup.h
@@ -27,8 +27,6 @@
 #define pud_page(pud)			pgd_page(pud)
 #define pud_page_vaddr(pud)		pgd_page_vaddr(pud)
 
-#undef pud_free_tlb
-#define pud_free_tlb(tlb, x, addr)	do { } while (0)
 #define pud_free(mm, x)			do { } while (0)
 #define __pud_free_tlb(tlb, x, addr)	do { } while (0)
 
diff --git a/include/asm-generic/tlb.h b/include/asm-generic/tlb.h
index 04c0644006fd..1f83188cb331 100644
--- a/include/asm-generic/tlb.h
+++ b/include/asm-generic/tlb.h
@@ -584,7 +584,7 @@ static inline void tlb_end_vma(struct mmu_gather *tlb, struct vm_area_struct *vm
 	} while (0)
 #endif
 
-#ifndef __ARCH_HAS_4LEVEL_HACK
+#ifndef __PAGETABLE_PUD_FOLDED
 #ifndef pud_free_tlb
 #define pud_free_tlb(tlb, pudp, address)			\
 	do {							\
@@ -594,6 +594,8 @@ static inline void tlb_end_vma(struct mmu_gather *tlb, struct vm_area_struct *vm
 		__pud_free_tlb(tlb, pudp, address);		\
 	} while (0)
 #endif
+#else
+#define pud_free_tlb(tlb, pudp, address)	do { } while (0)
 #endif
 
 #ifndef __ARCH_HAS_5LEVEL_HACK
-- 
2.20.1

[PATCH 2/3] asm-generic/tlb: stub out p4d_free_tlb() if __PAGETABLE_P4D_FOLDED ...

[Vineet Gupta]

... independent of __ARCH_HAS_5LEVEL_HACK

This came up when removing __ARCH_HAS_5LEVEL_HACK for ARC as code bloat
from p4d_free_tlb() despite pud being folded (with 2 levels on ARC)

| bloat-o-meter2 vmlinux-C-elide-pud_free_tlb vmlinux-D-elide-p4d_free_tlb
| add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-104 (-104)
| function                                     old     new   delta
| free_pgd_range                               552     422    -130
| Total: Before=4137172, After=4137042, chg -1.000000%

Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 include/asm-generic/5level-fixup.h | 2 --
 include/asm-generic/tlb.h          | 4 +++-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/asm-generic/5level-fixup.h b/include/asm-generic/5level-fixup.h
index f6947da70d71..c855b5cf4425 100644
--- a/include/asm-generic/5level-fixup.h
+++ b/include/asm-generic/5level-fixup.h
@@ -48,8 +48,6 @@ static inline int p4d_present(p4d_t p4d)
 #define __p4d(x)			__pgd(x)
 #define set_p4d(p4dp, p4d)		set_pgd(p4dp, p4d)
 
-#undef p4d_free_tlb
-#define p4d_free_tlb(tlb, x, addr)	do { } while (0)
 #define p4d_free(mm, x)			do { } while (0)
 #define __p4d_free_tlb(tlb, x, addr)	do { } while (0)
 
diff --git a/include/asm-generic/tlb.h b/include/asm-generic/tlb.h
index 1f83188cb331..f3dad87f4ecc 100644
--- a/include/asm-generic/tlb.h
+++ b/include/asm-generic/tlb.h
@@ -598,7 +598,7 @@ static inline void tlb_end_vma(struct mmu_gather *tlb, struct vm_area_struct *vm
 #define pud_free_tlb(tlb, pudp, address)	do { } while (0)
 #endif
 
-#ifndef __ARCH_HAS_5LEVEL_HACK
+#ifndef __PAGETABLE_P4D_FOLDED
 #ifndef p4d_free_tlb
 #define p4d_free_tlb(tlb, pudp, address)			\
 	do {							\
@@ -607,6 +607,8 @@ static inline void tlb_end_vma(struct mmu_gather *tlb, struct vm_area_struct *vm
 		__p4d_free_tlb(tlb, pudp, address);		\
 	} while (0)
 #endif
+#else
+#define p4d_free_tlb(tlb, pudp, address)	do { } while (0)
 #endif
 
 #endif /* CONFIG_MMU */
-- 
2.20.1

[PATCH 3/3] asm-generic/mm: stub out p{4,d}d_clear_bad() if __PAGETABLE_P{4,u}D_FOLDED

[Vineet Gupta]

This removes the code for 2 level paging as seen on ARC

| bloat-o-meter2 vmlinux-D-elide-p4d_free_tlb vmlinux-E-elide-p?d_clear_bad
| add/remove: 0/2 grow/shrink: 0/0 up/down: 0/-22 (-22)
| function                                     old     new   delta
| pud_clear_bad                                 20       -     -20
| p4d_clear_bad                                 20       -     -20
| Total: Before=4137104, After=4137082, chg -1.000000%

Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 include/asm-generic/pgtable.h | 11 +++++++++++
 mm/pgtable-generic.c          |  4 ++++
 2 files changed, 15 insertions(+)

diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
index 818691846c90..9cdcbc7c0b7b 100644
--- a/include/asm-generic/pgtable.h
+++ b/include/asm-generic/pgtable.h
@@ -558,8 +558,19 @@ static inline pgprot_t pgprot_modify(pgprot_t oldprot, pgprot_t newprot)
  * Do the tests inline, but report and clear the bad entry in mm/memory.c.
  */
 void pgd_clear_bad(pgd_t *);
+
+#ifndef __PAGETABLE_P4D_FOLDED
 void p4d_clear_bad(p4d_t *);
+#else
+#define p4d_clear_bad(p4d)        do { } while (0)
+#endif
+
+#ifndef __PAGETABLE_PUD_FOLDED
 void pud_clear_bad(pud_t *);
+#else
+#define pud_clear_bad(p4d)        do { } while (0)
+#endif
+
 void pmd_clear_bad(pmd_t *);
 
 static inline int pgd_none_or_clear_bad(pgd_t *pgd)
diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c
index 532c29276fce..856dc3bb77e6 100644
--- a/mm/pgtable-generic.c
+++ b/mm/pgtable-generic.c
@@ -24,17 +24,21 @@ void pgd_clear_bad(pgd_t *pgd)
 	pgd_clear(pgd);
 }
 
+#ifndef __PAGETABLE_P4D_FOLDED
 void p4d_clear_bad(p4d_t *p4d)
 {
 	p4d_ERROR(*p4d);
 	p4d_clear(p4d);
 }
+#endif
 
+#ifndef __PAGETABLE_PUD_FOLDED
 void pud_clear_bad(pud_t *pud)
 {
 	pud_ERROR(*pud);
 	pud_clear(pud);
 }
+#endif
 
 void pmd_clear_bad(pmd_t *pmd)
 {
-- 
2.20.1

Re: [PATCH 0/3] eldie generated code for folded p4d/pud

[Peter Zijlstra]

On Wed, Oct 09, 2019 at 03:26:55PM -0700, Vineet Gupta wrote:
> Hi,
> 
> This series elides extraneous generate code for folded p4d/pud.
> This came up when trying to remove __ARCH_USE_5LEVEL_HACK from ARC port.
> The code saving are not a while lot, but still worthwhile IMHO.
> 
> bloat-o-meter2 vmlinux-A-baseline vmlinux-E-elide-p?d_clear_bad
> add/remove: 0/2 grow/shrink: 0/1 up/down: 0/-146 (-146)
> function                                     old     new   delta
> p4d_clear_bad                                  2       -      -2
> pud_clear_bad                                 20       -     -20
> free_pgd_range                               546     422    -124
> Total: Before=4137148, After=4137002, chg -1.000000%
> 

Works for me, thanks!

Re: [PATCH 0/3] eldie generated code for folded p4d/pud

[Kirill A. Shutemov]

On Wed, Oct 09, 2019 at 10:26:55PM +0000, Vineet Gupta wrote:
> Hi,
> 
> This series elides extraneous generate code for folded p4d/pud.
> This came up when trying to remove __ARCH_USE_5LEVEL_HACK from ARC port.
> The code saving are not a while lot, but still worthwhile IMHO.

Agreed.

Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

-- 
 Kirill A. Shutemov

Re: [PATCH 0/3] eldie generated code for folded p4d/pud

[Vineet Gupta]

Hi Kirill,

On 10/10/19 1:56 AM, Kirill A. Shutemov wrote:
> On Wed, Oct 09, 2019 at 10:26:55PM +0000, Vineet Gupta wrote:
>>
>> This series elides extraneous generate code for folded p4d/pud.
>> This came up when trying to remove __ARCH_USE_5LEVEL_HACK from ARC port.
>> The code saving are not a while lot, but still worthwhile IMHO.
> 
> Agreed.

Thx.

So given we are folding pmd too, it seemed we could do the following as well.

+#ifndef __PAGETABLE_PMD_FOLDED
 void pmd_clear_bad(pmd_t *);
+#else
+#define pmd_clear_bad(pmd)        do { } while (0)
+#endif

+#ifndef __PAGETABLE_PMD_FOLDED
 void pmd_clear_bad(pmd_t *pmd)
 {
        pmd_ERROR(*pmd);
        pmd_clear(pmd);
 }
+#endif

I stared at generated code and it seems a bit wrong.
free_pgd_range() -> pgd_none_or_clear_bad() is no longer checking for unmapped pgd
entries as pgd_none/pgd_bad are all stubs returning 0.

This whole pmd folding is a bit confusing considering I only revisit it every few
years :-) Abstraction wise, __PAGETABLE_PMD_FOLDED only has pgd, pte but even in
this regime bunch of pmd macros are still valid

    pmd_set(pmdp, ptep) {
        *pmdp.pud.p4d.pgd = (unsigned long)ptep
    }

Is there a better way to make a mental model of this code folding.

In an ideal world pmd folded would have meant pmd_* routines just vanish - poof.
So in that sense I like your implementation under #[45]LEVEL_HACK where the level
simply vanishes by code like #define p4d_t pgd_t. Perhaps there is lot of historic
baggage, proliferated into arch code so hard to untangle.

Thx,
-Vineet

Re: [PATCH 0/3] eldie generated code for folded p4d/pud

[Kirill A. Shutemov]

On Thu, Oct 10, 2019 at 01:05:56PM -0700, Vineet Gupta wrote:
> 
> Hi Kirill,
> 
> On 10/10/19 1:56 AM, Kirill A. Shutemov wrote:
> > On Wed, Oct 09, 2019 at 10:26:55PM +0000, Vineet Gupta wrote:
> >>
> >> This series elides extraneous generate code for folded p4d/pud.
> >> This came up when trying to remove __ARCH_USE_5LEVEL_HACK from ARC port.
> >> The code saving are not a while lot, but still worthwhile IMHO.
> > 
> > Agreed.
> 
> Thx.
> 
> So given we are folding pmd too, it seemed we could do the following as well.
> 
> +#ifndef __PAGETABLE_PMD_FOLDED
>  void pmd_clear_bad(pmd_t *);
> +#else
> +#define pmd_clear_bad(pmd)        do { } while (0)
> +#endif
> 
> +#ifndef __PAGETABLE_PMD_FOLDED
>  void pmd_clear_bad(pmd_t *pmd)
>  {
>         pmd_ERROR(*pmd);
>         pmd_clear(pmd);
>  }
> +#endif
> 
> I stared at generated code and it seems a bit wrong.
> free_pgd_range() -> pgd_none_or_clear_bad() is no longer checking for unmapped pgd
> entries as pgd_none/pgd_bad are all stubs returning 0.
> 
> This whole pmd folding is a bit confusing considering I only revisit it every few
> years :-) Abstraction wise, __PAGETABLE_PMD_FOLDED only has pgd, pte but even in
> this regime bunch of pmd macros are still valid
> 
>     pmd_set(pmdp, ptep) {
>         *pmdp.pud.p4d.pgd = (unsigned long)ptep
>     }
> 
> Is there a better way to make a mental model of this code folding.

I don't have any. PMD folding predates me and have never looked at it
closely. Quick look brings more confusion than clarity. :P

> In an ideal world pmd folded would have meant pmd_* routines just vanish - poof.
> So in that sense I like your implementation under #[45]LEVEL_HACK where the level
> simply vanishes by code like #define p4d_t pgd_t. Perhaps there is lot of historic
> baggage, proliferated into arch code so hard to untangle.

In ideal world all these pgd/p4d/pud/pmd/pte should die and we have
something more flexible to begin with.

I played with this before:

https://lore.kernel.org/lkml/20180424154355.mfjgkf47kdp2by4e@black.fi.intel.com/

-- 
 Kirill A. Shutemov

[RFC] asm-generic/tlb: stub out pmd_free_tlb() if __PAGETABLE_PMD_FOLDED

[Vineet Gupta]

This is inine with similar patches for nopud [1] and nop4d [2] cases.

  However I'm not really sure I understand clearly how the nopmd code is
  supposed to work (for a 2 tier paging system) - hence the RFC.
  Consider free_pmd_range() simplified/annotated below

  free_pmd_range
  ...
	pmd = pmd_offset(pud, addr);
	do {
		next = pmd_addr_end(addr, end);
		if (pmd_none_or_clear_bad(pmd)) => *pmd_bad()/pmd_clear_bad() [a]*
			continue;
		free_pte_range(tlb, pmd, addr);
	} while (pmd++, addr = next, addr != end);
   ...
	*pmd_free_tlb(tlb, pmd, start); => [b]*

   For ARC/nopmd case [a] is actually checking pgd and consequently
   pmd_clear_bad() can't be stubbed out for PMD_FOLDED case. However it seems
   case [b] can be stubbed out (hence this patch) along same lines as [1] and [2]

| bloat-o-meter2 vmlinux-E-elide-p?d_clear_bad vmlinux-F-elide-pmd_free_tlb
| add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-112 (-112)
| function                                     old     new   delta
| free_pgd_range                               422     310    -112
| Total: Before=4137002, After=4136890, chg -1.000000%

[1] http://lists.infradead.org/pipermail/linux-snps-arc/2019-October/006266.html
[2] http://lists.infradead.org/pipermail/linux-snps-arc/2019-October/006265.html

Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 include/asm-generic/tlb.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/asm-generic/tlb.h b/include/asm-generic/tlb.h
index f3dad87f4ecc..a1edad7d4170 100644
--- a/include/asm-generic/tlb.h
+++ b/include/asm-generic/tlb.h
@@ -574,6 +574,7 @@ static inline void tlb_end_vma(struct mmu_gather *tlb, struct vm_area_struct *vm
 	} while (0)
 #endif
 
+#ifndef __PAGETABLE_PMD_FOLDED
 #ifndef pmd_free_tlb
 #define pmd_free_tlb(tlb, pmdp, address)			\
 	do {							\
@@ -583,6 +584,9 @@ static inline void tlb_end_vma(struct mmu_gather *tlb, struct vm_area_struct *vm
 		__pmd_free_tlb(tlb, pmdp, address);		\
 	} while (0)
 #endif
+#else
+#define pmd_free_tlb(tlb, pmdp, address)        do { } while (0)
+#endif
 
 #ifndef __PAGETABLE_PUD_FOLDED
 #ifndef pud_free_tlb
-- 
2.20.1

Re: [RFC] asm-generic/tlb: stub out pmd_free_tlb() if __PAGETABLE_PMD_FOLDED

[Linus Torvalds]

On Fri, Oct 11, 2019 at 3:38 PM Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote:
>
> This is inine with similar patches for nopud [1] and nop4d [2] cases.

I don't think your patch is wrong, but wouldn't it be easier and
cleaner to just do this instead

    --- a/include/asm-generic/pgtable-nopmd.h
    +++ b/include/asm-generic/pgtable-nopmd.h
    @@ -60,7 +60,7 @@ static inline pmd_t * pmd_offset(pud_t * pud,
unsigned long address)
     static inline void pmd_free(struct mm_struct *mm, pmd_t *pmd)
     {
     }
    -#define __pmd_free_tlb(tlb, x, a)          do { } while (0)
    +#define pmd_free_tlb(tlb, x, a)            do { } while (0)

     #undef  pmd_addr_end
     #define pmd_addr_end(addr, end)                    (end)

and just rely on the regular "#ifndef pmd_free_tlb" in
include/asm-generic/tlb.h?

Completely untested.

              Linus

Re: [RFC] asm-generic/tlb: stub out pmd_free_tlb() if __PAGETABLE_PMD_FOLDED

[Vineet Gupta]

On 10/14/19 10:41 AM, Linus Torvalds wrote:
> On Fri, Oct 11, 2019 at 3:38 PM Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote:
>>
>> This is inine with similar patches for nopud [1] and nop4d [2] cases.
> 
> I don't think your patch is wrong, but wouldn't it be easier and
> cleaner to just do this instead
> 
>     --- a/include/asm-generic/pgtable-nopmd.h
>     +++ b/include/asm-generic/pgtable-nopmd.h
>     @@ -60,7 +60,7 @@ static inline pmd_t * pmd_offset(pud_t * pud,
> unsigned long address)
>      static inline void pmd_free(struct mm_struct *mm, pmd_t *pmd)
>      {
>      }
>     -#define __pmd_free_tlb(tlb, x, a)          do { } while (0)
>     +#define pmd_free_tlb(tlb, x, a)            do { } while (0)
> 
>      #undef  pmd_addr_end
>      #define pmd_addr_end(addr, end)                    (end)

I suppose we could but

(a) It would be asymmetric with the __p{u,4}d_free_tlb() changes in [1] and [2].
Do you  prefer [1] and [2] be repun along the same lines as you propose above ?

(b) IMHO p?d_free_tlb() under corresponding #ifndef *P?D_FOLDED is much clearer to
read as being stubbed out. But this is minor point.

Also would you care to shed light on my other question about not being able to
fold away pmd_clear_bad() despite PMD_FOLDED given the pmd macros actually
checking for pgd. Of all the people you are likely to have most insight on how the
pmd folding actually evolved and works :-)

Thx,
-Vineet

[1] http://lists.infradead.org/pipermail/linux-snps-arc/2019-October/006266.html
[2] http://lists.infradead.org/pipermail/linux-snps-arc/2019-October/006265.html

Re: [RFC] asm-generic/tlb: stub out pmd_free_tlb() if __PAGETABLE_PMD_FOLDED

[Linus Torvalds]

On Mon, Oct 14, 2019 at 11:02 AM Vineet Gupta <vineetg76@gmail.com> wrote:
>
> I suppose we could but
>
> (a) It would be asymmetric with the __p{u,4}d_free_tlb() changes in [1] and [2].

Your patch is already assymmetric wrt those anyway - you had to add that

  +#else
  +#define pmd_free_tlb(tlb, pmdp, address)        do { } while (0)
  +#endif

that the other cases don't currently have, so then you point to
another patch that makes the code uglier instead.

> Do you  prefer [1] and [2] be repun along the same lines as you propose above ?

In general, I absolutely detest how we have random

   #ifndef ARCH_HAS_ONE_DEFINE
   #define another_define_entirely()
   ...

which makes no sense and is ugly, and also wreaks havoc on simple
things like "git grep another_define_entirely"

I've long tried to convince people to just do

  #ifndef special_define
  #define special_define(xyz) ..
  #endif

instead, which doesn't mix up two completely unrelated names, and when
you grep for that function name, you _see_ all the context.

> Also would you care to shed light on my other question about not being able to
> fold away pmd_clear_bad() despite PMD_FOLDED given the pmd macros actually
> checking for pgd. Of all the people you are likely to have most insight on how the
> pmd folding actually evolved and works :-)

I think some of it is just ugly and historical, and confused.

In general, it should always be the "higher" level that folds away. So
I think the best example of this is

  include/asm-generic/pgtable-nop4d.h

where basically all the "pgd" functions become no-ops, and can never
not exist or be bad, because they are always just containers for the
lower level and don't have any data in them themselves:

  static inline int pgd_none(pgd_t pgd)           { return 0; }
  static inline int pgd_bad(pgd_t pgd)            { return 0; }
  static inline int pgd_present(pgd_t pgd)        { return 1; }
  static inline void pgd_clear(pgd_t *pgd)        { }

and walking from pgd to p4d is that nice folded op:

  static inline p4d_t *p4d_offset(pgd_t *pgd, unsigned long address)
  { return (p4d_t *)pgd; }

and this is how it should always work.See "nopud" and "nopmd"(which
are 3rd/2nd level respectively) doing the same thing exactly.

And yes, pmd_clear_bad() should just go away. We have

  static inline int pmd_none_or_clear_bad(pmd_t *pmd)
  {
        if (pmd_none(*pmd))
                return 1;
        if (unlikely(pmd_bad(*pmd))) {
                pmd_clear_bad(pmd);
                return 1;
        }
        return 0;
  }

and if the pmd doesn't exist, then both pmd_none() and pmd_bad()
should just be zero (see above), and the pmd_none_or_clear_bad()
should just become "return 0";

Exactly what part isn't working for you?

I suspect part of the problem is exactly that we than have that stupid
confusion with some code checking "#ifdef __PAGETABLE_PMD_FOLDED" and
then making their own random decisions based on things like that
instead.

When you do that, the code ends up relying on other magic than just
the natural folding.

            Linus

Re: [RFC] asm-generic/tlb: stub out pmd_free_tlb() if __PAGETABLE_PMD_FOLDED

[Vineet Gupta]

On 10/14/19 11:25 AM, Linus Torvalds wrote:
> On Mon, Oct 14, 2019 at 11:02 AM Vineet Gupta <vineetg76@gmail.com> wrote:
>>
>> I suppose we could but
>>
>> (a) It would be asymmetric with the __p{u,4}d_free_tlb() changes in [1] and [2].
> 
> Your patch is already assymmetric wrt those anyway - you had to add that
> 
>   +#else
>   +#define pmd_free_tlb(tlb, pmdp, address)        do { } while (0)
>   +#endif
> 
> that the other cases don't currently have, so then you point to
> another patch that makes the code uglier instead.
> 
>> Do you  prefer [1] and [2] be repun along the same lines as you propose above ?
> 
> In general, I absolutely detest how we have random
> 
>    #ifndef ARCH_HAS_ONE_DEFINE
>    #define another_define_entirely()
>    ...
> 
> which makes no sense and is ugly, and also wreaks havoc on simple
> things like "git grep another_define_entirely"
> 
> I've long tried to convince people to just do
> 
>   #ifndef special_define
>   #define special_define(xyz) ..
>   #endif
> 
> instead, which doesn't mix up two completely unrelated names, and when
> you grep for that function name, you _see_ all the context.

Ok fair enough, I'd just add extra comments to non stubbed p?d_free_tlb that they
are stubbed out for corresponding case.

> 
>> Also would you care to shed light on my other question about not being able to
>> fold away pmd_clear_bad() despite PMD_FOLDED given the pmd macros actually
>> checking for pgd. Of all the people you are likely to have most insight on how the
>> pmd folding actually evolved and works :-)
> 
> I think some of it is just ugly and historical, and confused.
> 
> In general, it should always be the "higher" level that folds away. So
> I think the best example of this is
> 
>   include/asm-generic/pgtable-nop4d.h
> 
> where basically all the "pgd" functions become no-ops, and can never
> not exist or be bad, because they are always just containers for the
> lower level and don't have any data in them themselves:
> 
>   static inline int pgd_none(pgd_t pgd)           { return 0; }
>   static inline int pgd_bad(pgd_t pgd)            { return 0; }
>   static inline int pgd_present(pgd_t pgd)        { return 1; }
>   static inline void pgd_clear(pgd_t *pgd)        { }
> 
> and walking from pgd to p4d is that nice folded op:
> 
>   static inline p4d_t *p4d_offset(pgd_t *pgd, unsigned long address)
>   { return (p4d_t *)pgd; }
> 
> and this is how it should always work.See "nopud" and "nopmd"(which
> are 3rd/2nd level respectively) doing the same thing exactly.

Right, my naive mental model was assuming nopmd would somehow stub out pmd_*
macros (or call into upper level function somehow etc), wheres here
(1) we stub out the prior level and
(2) the function of stubbed level operate on the data type of higher level.


> And yes, pmd_clear_bad() should just go away. We have
> 
>   static inline int pmd_none_or_clear_bad(pmd_t *pmd)
>   {
>         if (pmd_none(*pmd))
>                 return 1;
>         if (unlikely(pmd_bad(*pmd))) {
>                 pmd_clear_bad(pmd);
>                 return 1;
>         }
>         return 0;
>   }
> 
> and if the pmd doesn't exist, then both pmd_none() and pmd_bad()
> should just be zero (see above), and the pmd_none_or_clear_bad()
> should just become "return 0";
> 
> Exactly what part isn't working for you?

I haven't tested that patch but I suspect even if it was broken, it would not
necessarily show right away with a trivial test.

Anyhow my worry/confusions starts at free_pgd_range() where
pgd_none_or_clear_bad(pgd) is no-op given pgd_none()/pgd_bad() are stubs for nopmd
case.

  free_pgd_range
	pgd = pgd_offset(tlb->mm, addr);
	do {
		next = pgd_addr_end(addr, end);
		if (pgd_none_or_clear_bad(pgd))
			continue;
		free_p4e_range(tlb, pgd, addr);
	} while (pgd++, addr = next, addr != end);
   ...

And the validation of pgd entry actually happens in pmd_none_or_clear_bad(pmd)
since there pmd actually ends up referencing pgd entry. Hence the ensuing
pmd_clear_bad() doesn't seem like if it could be stubbed out.

  free_pmd_range
	pmd = pmd_offset(pud, addr);
	do {
		next = pmd_addr_end(addr, end);
		if (pmd_none_or_clear_bad(pmd)) <--- pmd_bad()/pmd_clear_bad()
			continue;
		free_pte_range(tlb, pmd, addr);
	} while (pmd++, addr = next, addr != end);

I'm sure I'm missing something, but don't understand what !

Re: [RFC] asm-generic/tlb: stub out pmd_free_tlb() if __PAGETABLE_PMD_FOLDED

[Linus Torvalds]

On Mon, Oct 14, 2019 at 12:08 PM Vineet Gupta <vineetg76@gmail.com> wrote:
>
> > And yes, pmd_clear_bad() should just go away. We have
> >
> >   static inline int pmd_none_or_clear_bad(pmd_t *pmd)
> >   {
> >         if (pmd_none(*pmd))
> >                 return 1;
> >         if (unlikely(pmd_bad(*pmd))) {
> >                 pmd_clear_bad(pmd);
> >                 return 1;
> >         }
> >         return 0;
> >   }

That was a particularly bad example.

The pmd always exists, even in a 2-level setup.

It's the pgd/p4d/pud that end up containing a lower level, but
pmd_none() is never one of the fixed "doesn't exist" cases.

> > Exactly what part isn't working for you?
>
> I haven't tested that patch but I suspect even if it was broken, it would not
> necessarily show right away with a trivial test.
>
> Anyhow my worry/confusions starts at free_pgd_range() where
> pgd_none_or_clear_bad(pgd) is no-op given pgd_none()/pgd_bad() are stubs for nopmd
> case.

Right. If you have a two-level setup, then p[g4u]d_none_or_clear_bad()
should end up being no-ops.

Buit then:

> And the validation of pgd entry actually happens in pmd_none_or_clear_bad(pmd)
> since there pmd actually ends up referencing pgd entry. Hence the ensuing
> pmd_clear_bad() doesn't seem like if it could be stubbed out.

Yes, you're correct, I was just "off by one" in my levels.

Yeah, the folding is damn confusing. And it doesn't help that I think
some of the code talks about the lower level being folded into the
higher level for historical reasons, so we have those PMD_FOLDED
macros etc, which are really about pud() just going away because pmd
is folded inside the pud.

So when the pud level is compiled away, we talk about the pmd level
being folded into it, and then we get confusion (like mine above)
where you end up being off by one level, because depending on how it's
being talked about, you talk about one or the other.

And it shows in the header files too. We have "pgtable-nopmd.h", which
then defines the page table accessors not for the pmd level, but for
the pud level.

Which is why I then spout nonsense like the above about pmd_none() -
because I was thinking of the nopmd case, but that makes the
p*u*d_none() be always 0, not p*m*d_none().

So we have this whole "off-by-one" error in our naming and thus our
thinking, and it's really easy to just get really confused about it.

We should probably get rid of the whole "PMD_FOLDED" logic, and
instead talk about "no PUD level".

It actually shows in our types too. We do this:

   typedef struct { pud_t pud; } pmd_t;
   #define PTRS_PER_PMD    1

because some of the code thinks of the pmd as containing the pud.

But it would probably be better to do it the other way around, and
just consistently think of it as "pud level doesn't exist, the pud
level just contains a pmd" instead.

So we have these really odd "somethimes we think of pmd as part of a
pud entry" vs "sometimes we think of pud as just containing a single
pmd".

And I think that latter model is the better mental model, but then we
should have

   typedef struct { pmd_t pud; } pud_t;
   #define PTRS_PER_PUD    1

instead, and we'd get

   static inline pmd_t * pmd_offset(pud_t * pud, unsigned long address)
   { return &pud->pmd; }

and that would make more sense, wouldn't it?

But trying to fix our odd "we seem to think about it wrong" model
would likely be too painful to be realistic., It would involve
renaming

  nop4d.h -> nopgd.h
  nopud.h -> nop4d.h
  nopmd.h -> nopud.h

and turning those types around (so we'd have those

   typedef struct { p4d_t p4d; } pgd_t;
   typedef struct { pud_t pud; | p4d_t;
   typedef struct { pmd_t pmd; } pud_t;

for no-pgd/no-p4d/no-pud respectively.

So then a 2-level machine would only define the pmd and pte levels,
and be done with it, because the upper levels would be defined in
terms of those.

But that's not what we do, and we mix up levels in odd and confusing ways.

And now I've said pgd/pud/p4d/pmd so many times that I've confused
myself and think I'm wrong again, and I think that historically -
originally - we always had a pgd, and then the pmd didn't exist
because it was folded into it. That makes sense from a x86 naming
standpoint. Then x86 _did_ get a pmd, and then we added more levels in
between, and other architectures did things differently.

So I think the confusion is historical, and is because we've switched
between thinking that the the lower level that doesn't exist, but is
embedded in the upper level, and slowly converted to "it's the upper
level that doesn't exist, and just contains the lower level"

The point stands: it's confusing, and we should probably pick one
model, and the model we pick should likely be "this level doesn't
exist, and just wraps the lower level", so it *should* be "no pgd"/"no
p4d"/"no pud".

            Linus

Re: [RFC] asm-generic/tlb: stub out pmd_free_tlb() if __PAGETABLE_PMD_FOLDED

[Matthew Wilcox]

On Mon, Oct 14, 2019 at 01:38:34PM -0700, Linus Torvalds wrote:
> And now I've said pgd/pud/p4d/pmd so many times that I've confused
> myself and think I'm wrong again, and I think that historically -
> originally - we always had a pgd, and then the pmd didn't exist
> because it was folded into it. That makes sense from a x86 naming
> standpoint. Then x86 _did_ get a pmd, and then we added more levels in
> between, and other architectures did things differently.

Oh my goodness.  Thank you for writing all this out and finally getting
to this point.  I was reading the whole thing thinking "This is different
from what I remember" and then you got here.  This explains so much about
how our MM does/doesn't work, and it's not just me that's confused ;-)