[PATCH 1/1] mm: unify pmd_free() implementation

[PATCH 1/1] mm: unify pmd_free() implementation

[Andrea Righi]

Move multiple definitions of pmd_free() from different include/asm-* into
mm/util.c.

This also fixes the following warning on x86 when CONFIG_X86_PAE is not set:

    arch/x86/mm/pgtable.c: In function ‘pgd_mop_up_pmds’:
    arch/x86/mm/pgtable.c:194: warning: unused variable ‘pmd’

Signed-off-by: Andrea Righi <righi.andrea@gmail.com>
---
 include/asm-arm/pgalloc.h           |    1 -
 include/asm-frv/pgalloc.h           |    1 -
 include/asm-generic/pgtable-nopmd.h |    3 ++-
 include/asm-m32r/pgalloc.h          |    1 -
 include/asm-m68k/sun3_pgalloc.h     |    1 -
 include/asm-mips/pgalloc.h          |    1 -
 include/asm-parisc/pgalloc.h        |    1 -
 include/asm-powerpc/pgalloc-32.h    |    1 -
 include/asm-s390/pgalloc.h          |    1 -
 include/asm-sh/pgalloc.h            |    1 -
 mm/util.c                           |    6 ++++++
 11 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/include/asm-arm/pgalloc.h b/include/asm-arm/pgalloc.h
index 163b030..c1da401 100644
--- a/include/asm-arm/pgalloc.h
+++ b/include/asm-arm/pgalloc.h
@@ -27,7 +27,6 @@
  * Since we have only two-level page tables, these are trivial
  */
 #define pmd_alloc_one(mm,addr)		({ BUG(); ((pmd_t *)2); })
-#define pmd_free(mm, pmd)		do { } while (0)
 #define pgd_populate(mm,pmd,pte)	BUG()
 
 extern pgd_t *get_pgd_slow(struct mm_struct *mm);
diff --git a/include/asm-frv/pgalloc.h b/include/asm-frv/pgalloc.h
index 971e6ad..8a9df71 100644
--- a/include/asm-frv/pgalloc.h
+++ b/include/asm-frv/pgalloc.h
@@ -61,7 +61,6 @@ do {							\
  * (In the PAE case we free the pmds as part of the pgd.)
  */
 #define pmd_alloc_one(mm, addr)		({ BUG(); ((pmd_t *) 2); })
-#define pmd_free(mm, x)			do { } while (0)
 #define __pmd_free_tlb(tlb,x)		do { } while (0)
 
 #endif /* CONFIG_MMU */
diff --git a/include/asm-generic/pgtable-nopmd.h b/include/asm-generic/pgtable-nopmd.h
index 087325e..2ace3ac 100644
--- a/include/asm-generic/pgtable-nopmd.h
+++ b/include/asm-generic/pgtable-nopmd.h
@@ -54,7 +54,8 @@ static inline pmd_t * pmd_offset(pud_t * pud, unsigned long address)
  * inside the pud, so has no extra memory associated with it.
  */
 #define pmd_alloc_one(mm, address)		NULL
-#define pmd_free(mm, x)				do { } while (0)
+struct mm_struct;
+extern void __weak pmd_free(struct mm_struct *mm, pmd_t *pmd);
 #define __pmd_free_tlb(tlb, x)			do { } while (0)
 
 #undef  pmd_addr_end
diff --git a/include/asm-m32r/pgalloc.h b/include/asm-m32r/pgalloc.h
index f11a2b9..a5aa119 100644
--- a/include/asm-m32r/pgalloc.h
+++ b/include/asm-m32r/pgalloc.h
@@ -67,7 +67,6 @@ static inline void pte_free(struct mm_struct *mm, pgtable_t pte)
  */
 
 #define pmd_alloc_one(mm, addr)		({ BUG(); ((pmd_t *)2); })
-#define pmd_free(mm, x)			do { } while (0)
 #define __pmd_free_tlb(tlb, x)		do { } while (0)
 #define pgd_populate(mm, pmd, pte)	BUG()
 
diff --git a/include/asm-m68k/sun3_pgalloc.h b/include/asm-m68k/sun3_pgalloc.h
index d4c83f1..e52eaec 100644
--- a/include/asm-m68k/sun3_pgalloc.h
+++ b/include/asm-m68k/sun3_pgalloc.h
@@ -79,7 +79,6 @@ static inline void pmd_populate(struct mm_struct *mm, pmd_t *pmd, pgtable_t page
  * allocating and freeing a pmd is trivial: the 1-entry pmd is
  * inside the pgd, so has no extra memory associated with it.
  */
-#define pmd_free(mm, x)			do { } while (0)
 #define __pmd_free_tlb(tlb, x)		do { } while (0)
 
 static inline void pgd_free(struct mm_struct *mm, pgd_t *pgd)
diff --git a/include/asm-mips/pgalloc.h b/include/asm-mips/pgalloc.h
index 1275831..3ccc7e7 100644
--- a/include/asm-mips/pgalloc.h
+++ b/include/asm-mips/pgalloc.h
@@ -110,7 +110,6 @@ do {							\
  * allocating and freeing a pmd is trivial: the 1-entry pmd is
  * inside the pgd, so has no extra memory associated with it.
  */
-#define pmd_free(mm, x)			do { } while (0)
 #define __pmd_free_tlb(tlb, x)		do { } while (0)
 
 #endif
diff --git a/include/asm-parisc/pgalloc.h b/include/asm-parisc/pgalloc.h
index fc987a1..a1654ed 100644
--- a/include/asm-parisc/pgalloc.h
+++ b/include/asm-parisc/pgalloc.h
@@ -91,7 +91,6 @@ static inline void pmd_free(struct mm_struct *mm, pmd_t *pmd)
  */
 
 #define pmd_alloc_one(mm, addr)		({ BUG(); ((pmd_t *)2); })
-#define pmd_free(mm, x)			do { } while (0)
 #define pgd_populate(mm, pmd, pte)	BUG()
 
 #endif
diff --git a/include/asm-powerpc/pgalloc-32.h b/include/asm-powerpc/pgalloc-32.h
index 58c0714..95fca55 100644
--- a/include/asm-powerpc/pgalloc-32.h
+++ b/include/asm-powerpc/pgalloc-32.h
@@ -13,7 +13,6 @@ extern void pgd_free(struct mm_struct *mm, pgd_t *pgd);
  * the pgd will always be present..
  */
 /* #define pmd_alloc_one(mm,address)       ({ BUG(); ((pmd_t *)2); }) */
-#define pmd_free(mm, x) 		do { } while (0)
 #define __pmd_free_tlb(tlb,x)		do { } while (0)
 /* #define pgd_populate(mm, pmd, pte)      BUG() */
 
diff --git a/include/asm-s390/pgalloc.h b/include/asm-s390/pgalloc.h
index f5b2bf3..67b4758 100644
--- a/include/asm-s390/pgalloc.h
+++ b/include/asm-s390/pgalloc.h
@@ -61,7 +61,6 @@ static inline unsigned long pgd_entry_type(struct mm_struct *mm)
 #define pud_free(mm, x)				do { } while (0)
 
 #define pmd_alloc_one(mm,address)		({ BUG(); ((pmd_t *)2); })
-#define pmd_free(mm, x)				do { } while (0)
 
 #define pgd_populate(mm, pgd, pud)		BUG()
 #define pgd_populate_kernel(mm, pgd, pud)	BUG()
diff --git a/include/asm-sh/pgalloc.h b/include/asm-sh/pgalloc.h
index 84dd2db..cfdf487 100644
--- a/include/asm-sh/pgalloc.h
+++ b/include/asm-sh/pgalloc.h
@@ -84,7 +84,6 @@ do {							\
  * inside the pgd, so has no extra memory associated with it.
  */
 
-#define pmd_free(mm, x)			do { } while (0)
 #define __pmd_free_tlb(tlb,x)		do { } while (0)
 
 static inline void check_pgt_cache(void)
diff --git a/mm/util.c b/mm/util.c
index 9341ca7..e5c614b 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -163,6 +163,12 @@ char *strndup_user(const char __user *s, long n)
 }
 EXPORT_SYMBOL(strndup_user);
 
+#ifndef pmd_free
+void __weak pmd_free(struct mm_struct *mm, pmd_t *pmd)
+{
+}
+#endif
+
 #ifndef HAVE_ARCH_PICK_MMAP_LAYOUT
 void arch_pick_mmap_layout(struct mm_struct *mm)
 {
-- 
1.5.4.3

Re: [PATCH 1/1] mm: unify pmd_free() implementation

[Linus Torvalds]

On Mon, 28 Jul 2008, Andrea Righi wrote:
>
> Move multiple definitions of pmd_free() from different include/asm-* into
> mm/util.c.

But this is horrible, because it forces a totally unnecessary function 
call for that empty function.

Yeah, the function will be cheap, but the call itself will not be (it's a 
C language barrier and basically disables optimizations around it, causing 
thigns like register spill/reload for no good reason).

		Linus

Re: [PATCH 1/1] mm: unify pmd_free() implementation

[Andrea Righi]

Linus Torvalds wrote:
> 
> On Mon, 28 Jul 2008, Andrea Righi wrote:
>> Move multiple definitions of pmd_free() from different include/asm-* into
>> mm/util.c.
> 
> But this is horrible, because it forces a totally unnecessary function 
> call for that empty function.
> 
> Yeah, the function will be cheap, but the call itself will not be (it's a 
> C language barrier and basically disables optimizations around it, causing 
> thigns like register spill/reload for no good reason).
> 
> 		Linus

yep! clear.

Ok, in this case wouldn't be better at least to define pud_free() as:

static inline pud_free(struct mm_struct *mm, pmd_t *pmd)
{
}

in include/asm-generic/pgtable-nopmd.h, just to avoid the warning
on x86 without PAE?

Thanks for the explanation,
-Andrea

Re: [PATCH 1/1] mm: unify pmd_free() implementation

[James Bottomley]

On Mon, 2008-07-28 at 08:53 -0700, Linus Torvalds wrote:
> But this is horrible, because it forces a totally unnecessary function 
> call for that empty function.
> 
> Yeah, the function will be cheap, but the call itself will not be (it's a 
> C language barrier and basically disables optimizations around it, causing 
> thigns like register spill/reload for no good reason).

Are you sure about this (the barrier)?  We've been struggling to find a
paradigm for our trace points but the consensus seemed to be that
compiler barriers were pretty tiny perturbations in the optimiser stream
(they affect calculation ordering, but not usually enough to be
noticed).  The register spills to get known locations for the tracepoint
variables seemed to be the much more expensive thing.

If this basic assumption is wrong, we need to know now ...

James

Re: [PATCH 1/1] mm: unify pmd_free() implementation

[KOSAKI Motohiro]

> yep! clear.
> 
> Ok, in this case wouldn't be better at least to define pud_free() as:
> 
> static inline pud_free(struct mm_struct *mm, pmd_t *pmd)
> {
> }

I also like this :)

Re: [PATCH 1/1] mm: unify pmd_free() implementation

[Linus Torvalds]

On Mon, 28 Jul 2008, James Bottomley wrote:
> 
> Are you sure about this (the barrier)?

I'm sure. Try it. It perturbs the code quite a bit to have a function call 
in the thing, because it

 - clobbers all callee-clobbered registers.

   This means that all functions that _used_ to be leaf functions and 
   needed no stack frame at all (because they were simple enough to use 
   only the callee-clobbered registers) are suddenly now going to be 
   significantly more costly.

   Ergo: you get more stack movement with save/restore crud.

 - it is a barrier wrt any variables that may be visible externally 
   (perhaps because they had their address taken), so it forces a flush to 
   memory for those.

 - if it has arguments and return values, it also ends up forcing a 
   totally unnecessary argument setup (and all the fixed register crap 
   that involves, which means that you lost almost all your register 
   allocation freedom - not that you likely care, since most of your 
   registers are dead _anyway_ around the function call)

So empty functions calls are _deadly_ especially if the code was a leaf 
function before, and suddenly isn't any more.

On the other hand, there are also many cases where function calls won't 
matter much at all. If you had other function calls around that same area, 
all the above issues essentially go away, since your registers are dead 
anyway, and the function obviously wasn't a leaf function before the new 
call.

So it does depend quite a bit on the pattern of use. And yes, function 
argument setup can be a big part of it too.

				Linus

Re: [PATCH 1/1] mm: unify pmd_free() implementation

[James Bottomley]

On Mon, 2008-07-28 at 09:45 -0700, Linus Torvalds wrote:
> 
> On Mon, 28 Jul 2008, James Bottomley wrote:
> > 
> > Are you sure about this (the barrier)?
> 
> I'm sure. Try it. It perturbs the code quite a bit to have a function call 
> in the thing, because it
> 
>  - clobbers all callee-clobbered registers.
> 
>    This means that all functions that _used_ to be leaf functions and 
>    needed no stack frame at all (because they were simple enough to use 
>    only the callee-clobbered registers) are suddenly now going to be 
>    significantly more costly.
> 
>    Ergo: you get more stack movement with save/restore crud.
> 
>  - it is a barrier wrt any variables that may be visible externally 
>    (perhaps because they had their address taken), so it forces a flush to 
>    memory for those.
> 
>  - if it has arguments and return values, it also ends up forcing a 
>    totally unnecessary argument setup (and all the fixed register crap 
>    that involves, which means that you lost almost all your register 
>    allocation freedom - not that you likely care, since most of your 
>    registers are dead _anyway_ around the function call)
> 
> So empty functions calls are _deadly_ especially if the code was a leaf 
> function before, and suddenly isn't any more.
> 
> On the other hand, there are also many cases where function calls won't 
> matter much at all. If you had other function calls around that same area, 
> all the above issues essentially go away, since your registers are dead 
> anyway, and the function obviously wasn't a leaf function before the new 
> call.
> 
> So it does depend quite a bit on the pattern of use. And yes, function 
> argument setup can be a big part of it too.

Sorry ... should have been clearer.  My main concern is the cost of
barrier() which is just a memory clobber ... we have to use barriers to
place the probe points correctly in the code.

We already get that spilling function clobbered registers to stack (or
elsewhere) and yanking values out of the optimisation stream for the
arguments is pretty costly ... although the current LTT tracepoint code
argues that this cost can be borne.

James

Re: [PATCH 1/1] mm: unify pmd_free() implementation

[Linus Torvalds]

On Mon, 28 Jul 2008, James Bottomley wrote:
> 
> Sorry ... should have been clearer.  My main concern is the cost of
> barrier() which is just a memory clobber ... we have to use barriers to
> place the probe points correctly in the code.

Oh, "barrier()" itself has _much_ less cost.

It still has all the "needs to flush any global/address-taken-of variables 
to memory" property and can thus cause reloads, but that's kind of the 
point of it, after all. So in that sense "barrier()" is free: the only 
cost of a barrier is the cost of what you actually need to get done. It's 
not really "free", but it's also not any more costly than what your 
objective was.

In contrast, the "objective" in an empty function call is seldom the 
serialization, so in that case the serialization is all just unnecessary 
overhead.

Also, barrier() avoids the big hit of turning a leaf function into a 
non-leaf one. It also avoids all the fixed registers and the register 
clobbers (although for tracing purposes you may end up setting up fixed 
regs, of course).

The leaf -> non-leaf thing is actually often the major thing. Yes, the 
compiler will often inline functions that are simple enough to be leaf 
functions with no stack frame, so we don't have _that_ many of them, but 
when it hits, it's often the most noticeable part of an unnecessary 
function call. And "barrier()" should never trigger that problem.

			Linus

Re: [PATCH 1/1] mm: unify pmd_free() implementation

[Andrea Righi]

KOSAKI Motohiro wrote:
>> yep! clear.
>>
>> Ok, in this case wouldn't be better at least to define pud_free() as:
>>
>> static inline pud_free(struct mm_struct *mm, pmd_t *pmd)
>> {
>> }
> 
> I also like this :)

ok, a simpler patch using the inline function will follow.

-Andrea

Re: [PATCH 1/1] mm: unify pmd_free() implementation -> instrumentation

[Mathieu Desnoyers]

* Linus Torvalds (torvalds@linux-foundation.org) wrote:
> 
> 
> On Mon, 28 Jul 2008, James Bottomley wrote:
> > 
> > Sorry ... should have been clearer.  My main concern is the cost of
> > barrier() which is just a memory clobber ... we have to use barriers to
> > place the probe points correctly in the code.
> 
> Oh, "barrier()" itself has _much_ less cost.
> 
> It still has all the "needs to flush any global/address-taken-of variables 
> to memory" property and can thus cause reloads, but that's kind of the 
> point of it, after all. So in that sense "barrier()" is free: the only 
> cost of a barrier is the cost of what you actually need to get done. It's 
> not really "free", but it's also not any more costly than what your 
> objective was.
> 

The tracing instrumentation objective, the way I see it, is to have the
instrumented registers live at the instrumentation site when tracing is
active. Also, given that tracing probes _could_ modify global variables
(this is kernel code after all, so one could want to use it to provide
specific tuning of the scheduler activity), we would like this
instrumentation point to behave like a normal function call. Doing
otherwise could produce hard to debug compiler related issues when there
is expected interaction between specialized probes and the actual
instrumented function.

Just to make one point clear : tracers such as LTTng simply dumps the
data to a buffer, so such interaction does not exist. However, I've seen
that other "specialized" tracers (ftrace, blktrace, ..) can be much more
tied to the subsystem they trace, and that will problably end up being
used as a feedback-loop tuning hook.

So besides having a behavior consistent with what can be expected by the
C ABI (probes are written in C after all), the major point is to have
minimal side-effect on the instrumented function. The idea is to have
something that 1 - works without weird hard to debug corner cases, so
stable and production-ready _and_ 2 - is very efficient.

The tracepoints proposed, like the Markers, actually add a function call
in an unlikely() branch in the instrumented functions. It therefore uses
the compiler to generate the correct register dependencies at the
instrumentation site, but leaves the stack spilling in a dynamically
disabled region on cache-cold instructions. FWIW, it's actually better
than the Dtrace approach of nopping out a function call, because they
leave the stack setup in cache-hot instructions.

About turning leaf functions into non-leaf functions, we indeed have to
make sure we add them in locations less likely to change the resulting
code. It implies trying to instrument functions that are non-leaf. As a
quick example, let's have a look at the actual location of the proposed
scheduler tracepoints in sched.c :

wait_task_inactive()
  At least calls task_rq_lock, non-leaf.
try_to_wake_up()
  At least calls task_rq_lock, activate_task, ... : non-leaf.
wake_up_new_task()
  At least calls activate_task : non-leaf.
context_switch()
  Static inline, embedded in schedule(), which is itself non leaf : it
  calls, at least, thread_return : non-leaf.
sched_migrate_task()
  static function, inlined in sched_exec, which also calls
  sched_balance_self : non-leaf.

Result : none of these were leaf functions.

Actually, most of the function I have seen that were worth instrumenting
fell in either in those three categories :

- Complex function which includes function calls.
- Simple static inline function, embedded in another more complex
  function, itself including function calls.
- Simple "wrapper" function, taking arguments and passing them to a
  function pointer (VFS system calls provide many examples). These
  include, indeed, a function call.

So my conclusion is that adding a tracepoint should be done with care.
Probably it's worth documenting the side-effect of adding a tracepoint
to a leaf function, thus encouraging developers to add those in
non-leaf functions only, and to place them close to actual use of the
passed variables to minimize the register pressure overhead incurred by
trying to keep the variables live for a longer sequence of instruction.


Mathieu


> In contrast, the "objective" in an empty function call is seldom the 
> serialization, so in that case the serialization is all just unnecessary 
> overhead.
> 
> Also, barrier() avoids the big hit of turning a leaf function into a 
> non-leaf one. It also avoids all the fixed registers and the register 
> clobbers (although for tracing purposes you may end up setting up fixed 
> regs, of course).
> 
> The leaf -> non-leaf thing is actually often the major thing. Yes, the 
> compiler will often inline functions that are simple enough to be leaf 
> functions with no stack frame, so we don't have _that_ many of them, but 
> when it hits, it's often the most noticeable part of an unnecessary 
> function call. And "barrier()" should never trigger that problem.
> 
> 			Linus
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH 1/1] mm: unify pmd_free() implementation

[Andrew Morton]

On Mon, 28 Jul 2008 19:19:44 +0200
Andrea Righi <righi.andrea@gmail.com> wrote:

> KOSAKI Motohiro wrote:
> >> yep! clear.
> >>
> >> Ok, in this case wouldn't be better at least to define pud_free() as:
> >>
> >> static inline pud_free(struct mm_struct *mm, pmd_t *pmd)
> >> {
> >> }
> > 
> > I also like this :)
> 
> ok, a simpler patch using the inline function will follow.
> 

I can second that.  See
http://userweb.kernel.org/~akpm/mmotm/broken-out/include-asm-generic-pgtable-nopmdh-macros-are-noxious-reason-435.patch

Ingo cruelly ignored it.  Probably he's used to ignoring the comit
storm which I send in his direction - I'll need to resend it sometime.

I'd consider that patch to be partial - we should demacroize the
surrounding similar functions too.  But that will require a bit more
testing.

Re: [PATCH 1/1] mm: unify pmd_free() implementation

[Jeremy Fitzhardinge]

Andrew Morton wrote:
> I can second that.  See
> http://userweb.kernel.org/~akpm/mmotm/broken-out/include-asm-generic-pgtable-nopmdh-macros-are-noxious-reason-435.patch
>
> Ingo cruelly ignored it.  Probably he's used to ignoring the comit
> storm which I send in his direction - I'll need to resend it sometime.
>
> I'd consider that patch to be partial - we should demacroize the
> surrounding similar functions too.  But that will require a bit more
> testing.

Its immediate neighbours should be easy enough (pmd_alloc_one, 
__pmd_free_tlb), but any of the ones involving pmd_t risk #include hell 
(though the earlier references to pud_t in inline functions suggest it 
will work).  And pmd_addr_end is just ugly.

    J

Re: [PATCH 1/1] mm: unify pmd_free() and __pmd_free_tlb() implementation

[Andrea Righi]

Jeremy Fitzhardinge wrote:
> Andrew Morton wrote:
>> I can second that.  See
>> http://userweb.kernel.org/~akpm/mmotm/broken-out/include-asm-generic-pgtable-nopmdh-macros-are-noxious-reason-435.patch
>>
>> Ingo cruelly ignored it.  Probably he's used to ignoring the comit
>> storm which I send in his direction - I'll need to resend it sometime.
>>
>> I'd consider that patch to be partial - we should demacroize the
>> surrounding similar functions too.  But that will require a bit more
>> testing.
> 
> Its immediate neighbours should be easy enough (pmd_alloc_one, 
> __pmd_free_tlb), but any of the ones involving pmd_t risk #include hell 
> (though the earlier references to pud_t in inline functions suggest it 
> will work).  And pmd_addr_end is just ugly.
> 
>     J
> 

ok, let's start with the easiest: pmd_free() and __pmd_free_tlb().

Following another attempt to unify the implementations using inline
functions. It seems to build fine on x86 (pae / non-pae) and on x86_64.
This is an RFC patch right now, not for inclusion (just asking if it
could be a reasonable approach or not). And in any case this would need
more testing.

Signed-off-by: Andrea Righi <righi.andrea@gmail.com>
---
 arch/sparc/include/asm/pgalloc_64.h |    1 +
 include/asm-alpha/pgalloc.h         |    1 +
 include/asm-arm/pgalloc.h           |    1 -
 include/asm-frv/pgalloc.h           |    2 --
 include/asm-generic/pgtable-nopmd.h |   19 +++++++++++++++++--
 include/asm-ia64/pgalloc.h          |    1 +
 include/asm-m32r/pgalloc.h          |    2 --
 include/asm-m68k/motorola_pgalloc.h |    3 ++-
 include/asm-m68k/sun3_pgalloc.h     |    7 -------
 include/asm-mips/pgalloc.h          |   12 +-----------
 include/asm-parisc/pgalloc.h        |    2 +-
 include/asm-powerpc/pgalloc-32.h    |    2 --
 include/asm-powerpc/pgalloc-64.h    |    1 +
 include/asm-s390/pgalloc.h          |    1 -
 include/asm-sh/pgalloc.h            |    8 --------
 include/asm-um/pgalloc.h            |    1 +
 include/asm-x86/pgalloc.h           |    2 ++
 17 files changed, 28 insertions(+), 38 deletions(-)

diff --git a/arch/sparc/include/asm/pgalloc_64.h b/arch/sparc/include/asm/pgalloc_64.h
index 5bdfa2c..17cf9f5 100644
--- a/arch/sparc/include/asm/pgalloc_64.h
+++ b/arch/sparc/include/asm/pgalloc_64.h
@@ -35,6 +35,7 @@ static inline void pmd_free(struct mm_struct *mm, pmd_t *pmd)
 {
 	quicklist_free(0, NULL, pmd);
 }
+#define pmd_free pmd_free
 
 static inline pte_t *pte_alloc_one_kernel(struct mm_struct *mm,
 					  unsigned long address)
diff --git a/include/asm-alpha/pgalloc.h b/include/asm-alpha/pgalloc.h
index fd09015..ba68ca2 100644
--- a/include/asm-alpha/pgalloc.h
+++ b/include/asm-alpha/pgalloc.h
@@ -49,6 +49,7 @@ pmd_free(struct mm_struct *mm, pmd_t *pmd)
 {
 	free_page((unsigned long)pmd);
 }
+#define pmd_free pmd_free
 
 extern pte_t *pte_alloc_one_kernel(struct mm_struct *mm, unsigned long addr);
 
diff --git a/include/asm-arm/pgalloc.h b/include/asm-arm/pgalloc.h
index 163b030..c1da401 100644
--- a/include/asm-arm/pgalloc.h
+++ b/include/asm-arm/pgalloc.h
@@ -27,7 +27,6 @@
  * Since we have only two-level page tables, these are trivial
  */
 #define pmd_alloc_one(mm,addr)		({ BUG(); ((pmd_t *)2); })
-#define pmd_free(mm, pmd)		do { } while (0)
 #define pgd_populate(mm,pmd,pte)	BUG()
 
 extern pgd_t *get_pgd_slow(struct mm_struct *mm);
diff --git a/include/asm-frv/pgalloc.h b/include/asm-frv/pgalloc.h
index 971e6ad..790b9ff 100644
--- a/include/asm-frv/pgalloc.h
+++ b/include/asm-frv/pgalloc.h
@@ -61,8 +61,6 @@ do {							\
  * (In the PAE case we free the pmds as part of the pgd.)
  */
 #define pmd_alloc_one(mm, addr)		({ BUG(); ((pmd_t *) 2); })
-#define pmd_free(mm, x)			do { } while (0)
-#define __pmd_free_tlb(tlb,x)		do { } while (0)
 
 #endif /* CONFIG_MMU */
 
diff --git a/include/asm-generic/pgtable-nopmd.h b/include/asm-generic/pgtable-nopmd.h
index 087325e..7bf330e 100644
--- a/include/asm-generic/pgtable-nopmd.h
+++ b/include/asm-generic/pgtable-nopmd.h
@@ -5,6 +5,9 @@
 
 #include <asm-generic/pgtable-nopud.h>
 
+struct mm_struct;
+struct mmu_gather;
+
 #define __PAGETABLE_PMD_FOLDED
 
 /*
@@ -54,8 +57,20 @@ static inline pmd_t * pmd_offset(pud_t * pud, unsigned long address)
  * inside the pud, so has no extra memory associated with it.
  */
 #define pmd_alloc_one(mm, address)		NULL
-#define pmd_free(mm, x)				do { } while (0)
-#define __pmd_free_tlb(tlb, x)			do { } while (0)
+
+#ifndef pmd_free
+static inline void pmd_free(struct mm_struct *mm, pmd_t *pmd)
+{
+}
+#define pmd_free pmd_free
+#endif
+
+#ifndef __pmd_free_tlb
+static inline void __pmd_free_tlb(struct mmu_gather *tlb, pmd_t *pmd)
+{
+}
+#define __pmd_free_tlb __pmd_free_tlb
+#endif
 
 #undef  pmd_addr_end
 #define pmd_addr_end(addr, end)			(end)
diff --git a/include/asm-ia64/pgalloc.h b/include/asm-ia64/pgalloc.h
index b9ac1a6..6c9575c 100644
--- a/include/asm-ia64/pgalloc.h
+++ b/include/asm-ia64/pgalloc.h
@@ -66,6 +66,7 @@ static inline void pmd_free(struct mm_struct *mm, pmd_t *pmd)
 {
 	quicklist_free(0, NULL, pmd);
 }
+#define pmd_free pmd_free
 
 #define __pmd_free_tlb(tlb, pmd)	pmd_free((tlb)->mm, pmd)
 
diff --git a/include/asm-m32r/pgalloc.h b/include/asm-m32r/pgalloc.h
index f11a2b9..f7ecc72 100644
--- a/include/asm-m32r/pgalloc.h
+++ b/include/asm-m32r/pgalloc.h
@@ -67,8 +67,6 @@ static inline void pte_free(struct mm_struct *mm, pgtable_t pte)
  */
 
 #define pmd_alloc_one(mm, addr)		({ BUG(); ((pmd_t *)2); })
-#define pmd_free(mm, x)			do { } while (0)
-#define __pmd_free_tlb(tlb, x)		do { } while (0)
 #define pgd_populate(mm, pmd, pte)	BUG()
 
 #define check_pgt_cache()	do { } while (0)
diff --git a/include/asm-m68k/motorola_pgalloc.h b/include/asm-m68k/motorola_pgalloc.h
index d08bf62..1dc96c7 100644
--- a/include/asm-m68k/motorola_pgalloc.h
+++ b/include/asm-m68k/motorola_pgalloc.h
@@ -72,12 +72,13 @@ static inline int pmd_free(struct mm_struct *mm, pmd_t *pmd)
 {
 	return free_pointer_table(pmd);
 }
+#define pmd_free pmd_free
 
 static inline int __pmd_free_tlb(struct mmu_gather *tlb, pmd_t *pmd)
 {
 	return free_pointer_table(pmd);
 }
-
+#define __pmd_free_tlb __pmd_free_tlb
 
 static inline void pgd_free(struct mm_struct *mm, pgd_t *pgd)
 {
diff --git a/include/asm-m68k/sun3_pgalloc.h b/include/asm-m68k/sun3_pgalloc.h
index d4c83f1..795e3c0 100644
--- a/include/asm-m68k/sun3_pgalloc.h
+++ b/include/asm-m68k/sun3_pgalloc.h
@@ -75,13 +75,6 @@ static inline void pmd_populate(struct mm_struct *mm, pmd_t *pmd, pgtable_t page
 }
 #define pmd_pgtable(pmd) pmd_page(pmd)
 
-/*
- * allocating and freeing a pmd is trivial: the 1-entry pmd is
- * inside the pgd, so has no extra memory associated with it.
- */
-#define pmd_free(mm, x)			do { } while (0)
-#define __pmd_free_tlb(tlb, x)		do { } while (0)
-
 static inline void pgd_free(struct mm_struct *mm, pgd_t *pgd)
 {
         free_page((unsigned long) pgd);
diff --git a/include/asm-mips/pgalloc.h b/include/asm-mips/pgalloc.h
index 1275831..79a4182 100644
--- a/include/asm-mips/pgalloc.h
+++ b/include/asm-mips/pgalloc.h
@@ -104,17 +104,6 @@ do {							\
 	tlb_remove_page((tlb), pte);			\
 } while (0)
 
-#ifdef CONFIG_32BIT
-
-/*
- * allocating and freeing a pmd is trivial: the 1-entry pmd is
- * inside the pgd, so has no extra memory associated with it.
- */
-#define pmd_free(mm, x)			do { } while (0)
-#define __pmd_free_tlb(tlb, x)		do { } while (0)
-
-#endif
-
 #ifdef CONFIG_64BIT
 
 static inline pmd_t *pmd_alloc_one(struct mm_struct *mm, unsigned long address)
@@ -131,6 +120,7 @@ static inline void pmd_free(struct mm_struct *mm, pmd_t *pmd)
 {
 	free_pages((unsigned long)pmd, PMD_ORDER);
 }
+#define pmd_free pmd_free
 
 #define __pmd_free_tlb(tlb, x)	pmd_free((tlb)->mm, x)
 
diff --git a/include/asm-parisc/pgalloc.h b/include/asm-parisc/pgalloc.h
index fc987a1..632b2c5 100644
--- a/include/asm-parisc/pgalloc.h
+++ b/include/asm-parisc/pgalloc.h
@@ -80,6 +80,7 @@ static inline void pmd_free(struct mm_struct *mm, pmd_t *pmd)
 #endif
 	free_pages((unsigned long)pmd, PMD_ORDER);
 }
+#define pmd_free pmd_free
 
 #else
 
@@ -91,7 +92,6 @@ static inline void pmd_free(struct mm_struct *mm, pmd_t *pmd)
  */
 
 #define pmd_alloc_one(mm, addr)		({ BUG(); ((pmd_t *)2); })
-#define pmd_free(mm, x)			do { } while (0)
 #define pgd_populate(mm, pmd, pte)	BUG()
 
 #endif
diff --git a/include/asm-powerpc/pgalloc-32.h b/include/asm-powerpc/pgalloc-32.h
index 58c0714..dc6dca5 100644
--- a/include/asm-powerpc/pgalloc-32.h
+++ b/include/asm-powerpc/pgalloc-32.h
@@ -13,8 +13,6 @@ extern void pgd_free(struct mm_struct *mm, pgd_t *pgd);
  * the pgd will always be present..
  */
 /* #define pmd_alloc_one(mm,address)       ({ BUG(); ((pmd_t *)2); }) */
-#define pmd_free(mm, x) 		do { } while (0)
-#define __pmd_free_tlb(tlb,x)		do { } while (0)
 /* #define pgd_populate(mm, pmd, pte)      BUG() */
 
 #ifndef CONFIG_BOOKE
diff --git a/include/asm-powerpc/pgalloc-64.h b/include/asm-powerpc/pgalloc-64.h
index 812a1d8..0b63bc4 100644
--- a/include/asm-powerpc/pgalloc-64.h
+++ b/include/asm-powerpc/pgalloc-64.h
@@ -87,6 +87,7 @@ static inline void pmd_free(struct mm_struct *mm, pmd_t *pmd)
 {
 	kmem_cache_free(pgtable_cache[PMD_CACHE_NUM], pmd);
 }
+#define pmd_free pmd_free
 
 static inline pte_t *pte_alloc_one_kernel(struct mm_struct *mm,
 					  unsigned long address)
diff --git a/include/asm-s390/pgalloc.h b/include/asm-s390/pgalloc.h
index f5b2bf3..67b4758 100644
--- a/include/asm-s390/pgalloc.h
+++ b/include/asm-s390/pgalloc.h
@@ -61,7 +61,6 @@ static inline unsigned long pgd_entry_type(struct mm_struct *mm)
 #define pud_free(mm, x)				do { } while (0)
 
 #define pmd_alloc_one(mm,address)		({ BUG(); ((pmd_t *)2); })
-#define pmd_free(mm, x)				do { } while (0)
 
 #define pgd_populate(mm, pgd, pud)		BUG()
 #define pgd_populate_kernel(mm, pgd, pud)	BUG()
diff --git a/include/asm-sh/pgalloc.h b/include/asm-sh/pgalloc.h
index 84dd2db..f9d9ccb 100644
--- a/include/asm-sh/pgalloc.h
+++ b/include/asm-sh/pgalloc.h
@@ -79,14 +79,6 @@ do {							\
 	tlb_remove_page((tlb), (pte));			\
 } while (0)
 
-/*
- * allocating and freeing a pmd is trivial: the 1-entry pmd is
- * inside the pgd, so has no extra memory associated with it.
- */
-
-#define pmd_free(mm, x)			do { } while (0)
-#define __pmd_free_tlb(tlb,x)		do { } while (0)
-
 static inline void check_pgt_cache(void)
 {
 	quicklist_trim(QUICK_PGD, NULL, 25, 16);
diff --git a/include/asm-um/pgalloc.h b/include/asm-um/pgalloc.h
index 9062a6e..264120b 100644
--- a/include/asm-um/pgalloc.h
+++ b/include/asm-um/pgalloc.h
@@ -52,6 +52,7 @@ static inline void pmd_free(struct mm_struct *mm, pmd_t *pmd)
 {
 	free_page((unsigned long)pmd);
 }
+#define pmd_free pmd_free
 
 #define __pmd_free_tlb(tlb,x)   tlb_remove_page((tlb),virt_to_page(x))
 #endif
diff --git a/include/asm-x86/pgalloc.h b/include/asm-x86/pgalloc.h
index d63ea43..3c46c59 100644
--- a/include/asm-x86/pgalloc.h
+++ b/include/asm-x86/pgalloc.h
@@ -76,8 +76,10 @@ static inline void pmd_free(struct mm_struct *mm, pmd_t *pmd)
 	BUG_ON((unsigned long)pmd & (PAGE_SIZE-1));
 	free_page((unsigned long)pmd);
 }
+#define pmd_free pmd_free
 
 extern void __pmd_free_tlb(struct mmu_gather *tlb, pmd_t *pmd);
+#define __pmd_free_tlb __pmd_free_tlb
 
 #ifdef CONFIG_X86_PAE
 extern void pud_populate(struct mm_struct *mm, pud_t *pudp, pmd_t *pmd);

Re: [PATCH 1/1] mm: unify pmd_free() and __pmd_free_tlb() implementation

[Ingo Molnar]

* Andrea Righi <righi.andrea@gmail.com> wrote:

> Jeremy Fitzhardinge wrote:
> > Andrew Morton wrote:
> >> I can second that.  See
> >> http://userweb.kernel.org/~akpm/mmotm/broken-out/include-asm-generic-pgtable-nopmdh-macros-are-noxious-reason-435.patch
> >>
> >> Ingo cruelly ignored it.  Probably he's used to ignoring the comit
> >> storm which I send in his direction - I'll need to resend it sometime.
> >>
> >> I'd consider that patch to be partial - we should demacroize the
> >> surrounding similar functions too.  But that will require a bit more
> >> testing.
> > 
> > Its immediate neighbours should be easy enough (pmd_alloc_one, 
> > __pmd_free_tlb), but any of the ones involving pmd_t risk #include hell 
> > (though the earlier references to pud_t in inline functions suggest it 
> > will work).  And pmd_addr_end is just ugly.
> > 
> >     J
> > 
> 
> ok, let's start with the easiest: pmd_free() and __pmd_free_tlb().
> 
> Following another attempt to unify the implementations using inline 
> functions. It seems to build fine on x86 (pae / non-pae) and on 
> x86_64. This is an RFC patch right now, not for inclusion (just asking 
> if it could be a reasonable approach or not). And in any case this 
> would need more testing.
> 
> Signed-off-by: Andrea Righi <righi.andrea@gmail.com>
> ---
>  arch/sparc/include/asm/pgalloc_64.h |    1 +
>  include/asm-alpha/pgalloc.h         |    1 +
>  include/asm-arm/pgalloc.h           |    1 -
>  include/asm-frv/pgalloc.h           |    2 --
>  include/asm-generic/pgtable-nopmd.h |   19 +++++++++++++++++--
>  include/asm-ia64/pgalloc.h          |    1 +
>  include/asm-m32r/pgalloc.h          |    2 --
>  include/asm-m68k/motorola_pgalloc.h |    3 ++-
>  include/asm-m68k/sun3_pgalloc.h     |    7 -------
>  include/asm-mips/pgalloc.h          |   12 +-----------
>  include/asm-parisc/pgalloc.h        |    2 +-
>  include/asm-powerpc/pgalloc-32.h    |    2 --
>  include/asm-powerpc/pgalloc-64.h    |    1 +
>  include/asm-s390/pgalloc.h          |    1 -
>  include/asm-sh/pgalloc.h            |    8 --------
>  include/asm-um/pgalloc.h            |    1 +
>  include/asm-x86/pgalloc.h           |    2 ++
>  17 files changed, 28 insertions(+), 38 deletions(-)

the x86 bits look good to me in principle but touches a ton of 
architectures and deals with VM issues - the perfect candidate for -mm?

	Ingo

Re: [PATCH 1/1] mm: unify pmd_free() and __pmd_free_tlb() implementation

[Andrea Righi]

Ingo Molnar wrote:
> * Andrea Righi <righi.andrea@gmail.com> wrote:
> 
>> Jeremy Fitzhardinge wrote:
>>> Andrew Morton wrote:
>>>> I can second that.  See
>>>> http://userweb.kernel.org/~akpm/mmotm/broken-out/include-asm-generic-pgtable-nopmdh-macros-are-noxious-reason-435.patch
>>>>
>>>> Ingo cruelly ignored it.  Probably he's used to ignoring the comit
>>>> storm which I send in his direction - I'll need to resend it sometime.
>>>>
>>>> I'd consider that patch to be partial - we should demacroize the
>>>> surrounding similar functions too.  But that will require a bit more
>>>> testing.
>>> Its immediate neighbours should be easy enough (pmd_alloc_one, 
>>> __pmd_free_tlb), but any of the ones involving pmd_t risk #include hell 
>>> (though the earlier references to pud_t in inline functions suggest it 
>>> will work).  And pmd_addr_end is just ugly.
>>>
>>>     J
>>>
>> ok, let's start with the easiest: pmd_free() and __pmd_free_tlb().
>>
>> Following another attempt to unify the implementations using inline 
>> functions. It seems to build fine on x86 (pae / non-pae) and on 
>> x86_64. This is an RFC patch right now, not for inclusion (just asking 
>> if it could be a reasonable approach or not). And in any case this 
>> would need more testing.
>>
>> Signed-off-by: Andrea Righi <righi.andrea@gmail.com>
>> ---
>>  arch/sparc/include/asm/pgalloc_64.h |    1 +
>>  include/asm-alpha/pgalloc.h         |    1 +
>>  include/asm-arm/pgalloc.h           |    1 -
>>  include/asm-frv/pgalloc.h           |    2 --
>>  include/asm-generic/pgtable-nopmd.h |   19 +++++++++++++++++--
>>  include/asm-ia64/pgalloc.h          |    1 +
>>  include/asm-m32r/pgalloc.h          |    2 --
>>  include/asm-m68k/motorola_pgalloc.h |    3 ++-
>>  include/asm-m68k/sun3_pgalloc.h     |    7 -------
>>  include/asm-mips/pgalloc.h          |   12 +-----------
>>  include/asm-parisc/pgalloc.h        |    2 +-
>>  include/asm-powerpc/pgalloc-32.h    |    2 --
>>  include/asm-powerpc/pgalloc-64.h    |    1 +
>>  include/asm-s390/pgalloc.h          |    1 -
>>  include/asm-sh/pgalloc.h            |    8 --------
>>  include/asm-um/pgalloc.h            |    1 +
>>  include/asm-x86/pgalloc.h           |    2 ++
>>  17 files changed, 28 insertions(+), 38 deletions(-)
> 
> the x86 bits look good to me in principle but touches a ton of 
> architectures and deals with VM issues - the perfect candidate for -mm?
> 
> 	Ingo

Yes, sounds reasonable. I'll rebase to -mm and post a new patch.

Thanks,
-Andrea