[2/2] Change p[gum]d_clear_* inlines to macros to fix p?d_ERROR

[2/2] Change p[gum]d_clear_* inlines to macros to fix p?d_ERROR

[Andi Kleen]

Change p[gum]d_clear_* inlines to macros to fix p?d_ERROR

When this code was refactored by Hugh it was moved out of the actual
functions into these inlines. The problem is that pgd_ERROR
uses __FUNCTION__ and __LINE__ to show where the error happened,
and with the inline that is pretty meaningless now because
it's the same for all callers.

Change them to be macros to avoid this problem

Cc: hugh@veritas.com

Signed-off-by: Andi Kleen <ak@suse.de>

Index: linux/include/asm-generic/pgtable.h
===================================================================
--- linux.orig/include/asm-generic/pgtable.h
+++ linux/include/asm-generic/pgtable.h
@@ -170,44 +170,46 @@ static inline void ptep_set_wrprotect(st
 /*
  * When walking page tables, we usually want to skip any p?d_none entries;
  * and any p?d_bad entries - reporting the error before resetting to none.
- * Do the tests inline, but report and clear the bad entry in mm/memory.c.
+ * Use ugly macros so that __LINE__ in *_ERROR does the right thing.
  */
-void pgd_clear_bad(pgd_t *);
-void pud_clear_bad(pud_t *);
-void pmd_clear_bad(pmd_t *);
-
-static inline int pgd_none_or_clear_bad(pgd_t *pgd)
-{
-	if (pgd_none(*pgd))
-		return 1;
-	if (unlikely(pgd_bad(*pgd))) {
-		pgd_clear_bad(pgd);
-		return 1;
-	}
-	return 0;
-}
-
-static inline int pud_none_or_clear_bad(pud_t *pud)
-{
-	if (pud_none(*pud))
-		return 1;
-	if (unlikely(pud_bad(*pud))) {
-		pud_clear_bad(pud);
-		return 1;
-	}
-	return 0;
-}
-
-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;
-}
+
+#define pgd_none_or_clear_bad(pgd) ({ \
+	int ret__ = 0;			\
+	if (pgd_none(*pgd))		\
+		ret__ = 1;		\
+	else if (unlikely(pgd_bad(*pgd))) {	\
+		pgd_ERROR(*pgd);		\
+		pgd_clear(pgd);		\
+		ret__ = 1;		\
+	}				\
+	ret__;				\
+})
+
+#define pud_none_or_clear_bad(pud) ({ \
+	int ret__ = 0;			\
+	if (pud_none(*pud))		\
+		ret__ = 1;		\
+	else if (unlikely(pud_bad(*pud))) {	\
+		pud_ERROR(*pud);		\
+		pud_clear(pud);		\
+		ret__ = 1;		\
+	}				\
+	ret__;				\
+})
+
+#define pmd_none_or_clear_bad(pmd) ({ \
+	int ret__ = 0;			\
+	if (pmd_none(*pmd))		\
+		ret__ = 1;		\
+	else if (unlikely(pmd_bad(*pmd))) {	\
+		pmd_ERROR(*pmd);		\
+		pmd_clear(pmd);		\
+		ret__ = 1;		\
+	}				\
+	ret__;				\
+})
+
+
 #endif /* !__ASSEMBLY__ */
 
 #endif /* _ASM_GENERIC_PGTABLE_H */
Index: linux/mm/memory.c
===================================================================
--- linux.orig/mm/memory.c
+++ linux/mm/memory.c
@@ -83,30 +83,6 @@ EXPORT_SYMBOL(high_memory);
 EXPORT_SYMBOL(vmalloc_earlyreserve);
 
 /*
- * If a p?d_bad entry is found while walking page tables, report
- * the error, before resetting entry to p?d_none.  Usually (but
- * very seldom) called out from the p?d_none_or_clear_bad macros.
- */
-
-void pgd_clear_bad(pgd_t *pgd)
-{
-	pgd_ERROR(*pgd);
-	pgd_clear(pgd);
-}
-
-void pud_clear_bad(pud_t *pud)
-{
-	pud_ERROR(*pud);
-	pud_clear(pud);
-}
-
-void pmd_clear_bad(pmd_t *pmd)
-{
-	pmd_ERROR(*pmd);
-	pmd_clear(pmd);
-}
-
-/*
  * Note: this doesn't free the actual pages themselves. That
  * has been handled earlier when unmapping all the memory regions.
  */

Re: [2/2] Change p[gum]d_clear_* inlines to macros to fix p?d_ERROR

[Hugh Dickins]

On Sat, 10 Sep 2005, Andi Kleen wrote:

> Change p[gum]d_clear_* inlines to macros to fix p?d_ERROR
> 
> When this code was refactored by Hugh it was moved out of the actual
> functions into these inlines. The problem is that pgd_ERROR
> uses __FUNCTION__ and __LINE__ to show where the error happened,
> and with the inline that is pretty meaningless now because
> it's the same for all callers.
> 
> Change them to be macros to avoid this problem

Please don't.  It adds much less than I misremember (only 550 bytes
to my i386 PAE config), but even so it's a waste of space.  If you
really believe information is going missing there, just put a WARN_ON(1)
into p[gum]d_clear_bad in mm/memory.c.

(And remove the __FUNCTION__ and __LINE__ from the p[gum]d_ERRORs in all
the architectures if you're very industrious - I was too lazy for that.)

But I'm afraid that 95% of the cases will just turn out to be from
unmap_page_range below exit_mmap, and even the other 5% will just tell
when the corruption was first observed, giving no hint of what caused
it sometime earlier on.

(Of course, I was emboldened to make those changes because the messages
had never been seen in living memory, beyond our own private development
screwups.  They started appearing just around the time I changed them.)

Hugh

Re: [2/2] Change p[gum]d_clear_* inlines to macros to fix p?d_ERROR

[Andi Kleen]

On Saturday 10 September 2005 16:15, Hugh Dickins wrote:
> On Sat, 10 Sep 2005, Andi Kleen wrote:
> > Change p[gum]d_clear_* inlines to macros to fix p?d_ERROR
> >
> > When this code was refactored by Hugh it was moved out of the actual
> > functions into these inlines. The problem is that pgd_ERROR
> > uses __FUNCTION__ and __LINE__ to show where the error happened,
> > and with the inline that is pretty meaningless now because
> > it's the same for all callers.
> >
> > Change them to be macros to avoid this problem
>
> Please don't.  It adds much less than I misremember (only 550 bytes
> to my i386 PAE config), but even so it's a waste of space. 

Hmm? Macros and inlines take the same amount of space. 


>
> (Of course, I was emboldened to make those changes because the messages
> had never been seen in living memory, beyond our own private development
> screwups.  They started appearing just around the time I changed them.)

I have actually seen them while debugging something. But it was useless. That 
is why I made the change.

-Andi

Re: [2/2] Change p[gum]d_clear_* inlines to macros to fix p?d_ERROR

[Hugh Dickins]

On Sat, 10 Sep 2005, Andi Kleen wrote:
> On Saturday 10 September 2005 16:15, Hugh Dickins wrote:
> > On Sat, 10 Sep 2005, Andi Kleen wrote:
> > > Change p[gum]d_clear_* inlines to macros to fix p?d_ERROR
> > >
> > > When this code was refactored by Hugh it was moved out of the actual
> > > functions into these inlines. The problem is that pgd_ERROR
> > > uses __FUNCTION__ and __LINE__ to show where the error happened,
> > > and with the inline that is pretty meaningless now because
> > > it's the same for all callers.
> > >
> > > Change them to be macros to avoid this problem
> >
> > Please don't.  It adds much less than I misremember (only 550 bytes
> > to my i386 PAE config), but even so it's a waste of space. 
> 
> Hmm? Macros and inlines take the same amount of space. 

I expect they do, but that's not the issue.  You're moving
mm/memory.c's out-of-line p?d_clear_bad code inline (or into macro).

> I have actually seen them while debugging something. But it was useless.
> That is why I made the change.

If you've found their __FUNCTION__ and __LINE__ useful,
then I think you'll find the WARN_ON(1) backtrace even more so.

Hugh