[PATCH v2 0/2] Dummy HPAGE_* constants for !CONFIG_HUGETLB_PAGE

[PATCH v2 0/2] Dummy HPAGE_* constants for !CONFIG_HUGETLB_PAGE

[David Daney]

From: David Daney <david.daney@cavium.com>

After a, somewhat heated, discussion with David Rientjes, I think the
following approach will work.

The first patch adds HPAGE_SHIFT, needed by MIPS.

The second cleans up the exiting HPAGE_MASK and HPAGE_SIZE

David Daney (2):
  hugetlb: Provide a default HPAGE_SHIFT if !CONFIG_HUGETLB_PAGE
  hugetlb: Provide safer dummy values for HPAGE_MASK and HPAGE_SIZE

 include/linux/hugetlb.h |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

Cc: David Rientjes <rientjes@google.com>
-- 
1.7.2.3

[PATCH v2 1/2] hugetlb: Provide a default HPAGE_SHIFT if !CONFIG_HUGETLB_PAGE

[David Daney]

From: David Daney <david.daney@cavium.com>

When linux-next 10111117 is build for MIPS with
cavium-octeon_defconfig, we get:

  arch/mips/mm/tlb-r4k.c: In function 'local_flush_tlb_range':
  arch/mips/mm/tlb-r4k.c:129:28: error: 'HPAGE_SHIFT' undeclared (first use in this function)

The fix is to supply a dummy HPAGE_SHIFT in the !CONFIG_HUGETLB_PAGE
case.  Instead of supplying the non-huge value, as was done for
HPAGE_MASK and HPAGE_SIZE, we do BUG() instead.  This satisfies the
compiler, but give us runtime safety if someone were to use HPAGE_SIZE
incorrectly.

Cc: David Rientjes <rientjes@google.com>
Signed-off-by: David Daney <david.daney@cavium.com>
---
 include/linux/hugetlb.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index 19644e0..380451c 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -113,6 +113,7 @@ static inline void copy_huge_page(struct page *dst, struct page *src)
 #ifndef HPAGE_MASK
 #define HPAGE_MASK	PAGE_MASK		/* Keep the compiler happy */
 #define HPAGE_SIZE	PAGE_SIZE
+#define HPAGE_SHIFT	({BUG(); 0; })
 #endif
 
 #endif /* !CONFIG_HUGETLB_PAGE */
-- 
1.7.2.3

[PATCH v2 2/2] hugetlb: Provide safer dummy values for HPAGE_MASK and HPAGE_SIZE

[David Daney]

From: David Daney <david.daney@cavium.com>

It was pointed out by David Rientjes that the dummy values for
HPAGE_MASK and HPAGE_SIZE are quite unsafe.  It they are inadvertently
used with !CONFIG_HUGETLB_PAGE, compilation would succeed, but the
resulting code would surly not do anything sensible.

Place BUG() in the these dummy definitions, as we do in similar
circumstances in other places, so any abuse can be easily detected.

Since the only sane place to use these symbols when
!CONFIG_HUGETLB_PAGE is on dead code paths, the BUG() cause any actual
code to be emitted by the compiler.

Cc: David Rientjes <rientjes@google.com>
Signed-off-by: David Daney <david.daney@cavium.com>
---
 include/linux/hugetlb.h |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index 380451c..3ed6dbd 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -111,8 +111,9 @@ static inline void copy_huge_page(struct page *dst, struct page *src)
 #define hugetlb_change_protection(vma, address, end, newprot)
 
 #ifndef HPAGE_MASK
-#define HPAGE_MASK	PAGE_MASK		/* Keep the compiler happy */
-#define HPAGE_SIZE	PAGE_SIZE
+/* Keep the compiler happy with some dummy (but BUGgy) values */
+#define HPAGE_MASK	({BUG(); 0; })
+#define HPAGE_SIZE	({BUG(); 0; })
 #define HPAGE_SHIFT	({BUG(); 0; })
 #endif
 
-- 
1.7.2.3

Re: [PATCH v2 2/2] hugetlb: Provide safer dummy values for HPAGE_MASK and HPAGE_SIZE

[Andrew Morton]

On Thu, 17 Nov 2011 13:57:30 -0800
David Daney <ddaney.cavm@gmail.com> wrote:

> From: David Daney <david.daney@cavium.com>
> 
> It was pointed out by David Rientjes that the dummy values for
> HPAGE_MASK and HPAGE_SIZE are quite unsafe.  It they are inadvertently
> used with !CONFIG_HUGETLB_PAGE, compilation would succeed, but the
> resulting code would surly not do anything sensible.
> 
> Place BUG() in the these dummy definitions, as we do in similar
> circumstances in other places, so any abuse can be easily detected.
> 
> Since the only sane place to use these symbols when
> !CONFIG_HUGETLB_PAGE is on dead code paths, the BUG() cause any actual
> code to be emitted by the compiler.

I assume you meant "omitted" here.

But I don't think it's true.  Any such code would occur after testing
is_vm_hugetlb_page() or similar, and would have been omitted anyway.

> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -111,8 +111,9 @@ static inline void copy_huge_page(struct page *dst, struct page *src)
>  #define hugetlb_change_protection(vma, address, end, newprot)
>  
>  #ifndef HPAGE_MASK
> -#define HPAGE_MASK	PAGE_MASK		/* Keep the compiler happy */
> -#define HPAGE_SIZE	PAGE_SIZE
> +/* Keep the compiler happy with some dummy (but BUGgy) values */

That's a quite poor comment.  This?

--- a/include/linux/hugetlb.h~hugetlb-provide-safer-dummy-values-for-hpage_mask-and-hpage_size-fix
+++ a/include/linux/hugetlb.h
@@ -111,7 +111,11 @@ static inline void copy_huge_page(struct
 #define hugetlb_change_protection(vma, address, end, newprot)
 
 #ifndef HPAGE_MASK
-/* Keep the compiler happy with some dummy (but BUGgy) values */
+/*
+ * HPAGE_MASK and friends are defined if !CONFIG_HUGETLB_PAGE as an
+ * ifdef-avoiding convenience.  However they should never be evaluated at
+ * runtime if !CONFIG_HUGETLB_PAGE.
+ */
 #define HPAGE_MASK	({BUG(); 0; })
 #define HPAGE_SIZE	({BUG(); 0; })
 #define HPAGE_SHIFT	({BUG(); 0; })
_

> +#define HPAGE_MASK	({BUG(); 0; })
> +#define HPAGE_SIZE	({BUG(); 0; })
>  #define HPAGE_SHIFT	({BUG(); 0; })

This change means that HPAGE_* cannot be evaluated at compile time.  So

int foo = HPAGE_SIZE;

outside functions will explode.  I guess that's OK - actually desirable
- as such code shouldn't have been compiled anyway.

Re: [PATCH v2 2/2] hugetlb: Provide safer dummy values for HPAGE_MASK and HPAGE_SIZE

[David Daney]

On 11/17/2011 03:28 PM, Andrew Morton wrote:
> On Thu, 17 Nov 2011 13:57:30 -0800
> David Daney<ddaney.cavm@gmail.com>  wrote:
>
>> From: David Daney<david.daney@cavium.com>
>>
>> It was pointed out by David Rientjes that the dummy values for
>> HPAGE_MASK and HPAGE_SIZE are quite unsafe.  It they are inadvertently
>> used with !CONFIG_HUGETLB_PAGE, compilation would succeed, but the
>> resulting code would surly not do anything sensible.
>>
>> Place BUG() in the these dummy definitions, as we do in similar
>> circumstances in other places, so any abuse can be easily detected.
>>
>> Since the only sane place to use these symbols when
>> !CONFIG_HUGETLB_PAGE is on dead code paths, the BUG() cause any actual
>> code to be emitted by the compiler.
>
> I assume you meant "omitted" here.

I jumbled it up.  It should read:

... the BUG() will not cause any actual code to be emitted by the 
compiler.  In fact I have verified this on both MIPS64 and x86_64 kernels.

I could re-spin the patch with a corrected changelog if desired.

>
> But I don't think it's true.  Any such code would occur after testing
> is_vm_hugetlb_page() or similar, and would have been omitted anyway.
>

The point being that we are doing:

if (is_vm_hugetlb_page(vma)) {
	/* Do something with HPAGE_MASK*/
} else {
	/* Do something with PAGE_MASK */
}

In the !CONFIG_HUGETLB_PAGE case we have:
static inline int is_vm_hugetlb_page(struct vm_area_struct *vma)
{
	return 0;
}

The compiler sees that the usage of the dummy definitions is in a dead 
code path and nothing is emitted.

>> --- a/include/linux/hugetlb.h
>> +++ b/include/linux/hugetlb.h
>> @@ -111,8 +111,9 @@ static inline void copy_huge_page(struct page *dst, struct page *src)
>>   #define hugetlb_change_protection(vma, address, end, newprot)
>>
>>   #ifndef HPAGE_MASK
>> -#define HPAGE_MASK	PAGE_MASK		/* Keep the compiler happy */
>> -#define HPAGE_SIZE	PAGE_SIZE
>> +/* Keep the compiler happy with some dummy (but BUGgy) values */
>
> That's a quite poor comment.  This?

I was trying to communicate the presence of the BUG() in the definition. 
  Perhaps it is more confusing than it was before.

>
> --- a/include/linux/hugetlb.h~hugetlb-provide-safer-dummy-values-for-hpage_mask-and-hpage_size-fix
> +++ a/include/linux/hugetlb.h
> @@ -111,7 +111,11 @@ static inline void copy_huge_page(struct
>   #define hugetlb_change_protection(vma, address, end, newprot)
>
>   #ifndef HPAGE_MASK
> -/* Keep the compiler happy with some dummy (but BUGgy) values */
> +/*
> + * HPAGE_MASK and friends are defined if !CONFIG_HUGETLB_PAGE as an
> + * ifdef-avoiding convenience.  However they should never be evaluated at
> + * runtime if !CONFIG_HUGETLB_PAGE.
> + */
>   #define HPAGE_MASK	({BUG(); 0; })
>   #define HPAGE_SIZE	({BUG(); 0; })
>   #define HPAGE_SHIFT	({BUG(); 0; })
> _
>
>> +#define HPAGE_MASK	({BUG(); 0; })
>> +#define HPAGE_SIZE	({BUG(); 0; })
>>   #define HPAGE_SHIFT	({BUG(); 0; })
>
> This change means that HPAGE_* cannot be evaluated at compile time.  So
>
> int foo = HPAGE_SIZE;
>
> outside functions will explode.  I guess that's OK - actually desirable
> - as such code shouldn't have been compiled anyway.
>

The exact point of the patch.

Thanks,
David Daney

Re: [PATCH v2 2/2] hugetlb: Provide safer dummy values for HPAGE_MASK and HPAGE_SIZE

[Sergei Shtylyov]

Hello.

On 18-11-2011 1:57, David Daney wrote:

> From: David Daney<david.daney@cavium.com>

> It was pointed out by David Rientjes that the dummy values for
> HPAGE_MASK and HPAGE_SIZE are quite unsafe.  It they are inadvertently
> used with !CONFIG_HUGETLB_PAGE, compilation would succeed, but the
> resulting code would surly not do anything sensible.
>
> Place BUG() in the these dummy definitions, as we do in similar
> circumstances in other places, so any abuse can be easily detected.
>
> Since the only sane place to use these symbols when
> !CONFIG_HUGETLB_PAGE is on dead code paths, the BUG() cause any actual
> code to be emitted by the compiler.

    You mean "doesn't cause"?

> Cc: David Rientjes<rientjes@google.com>
> Signed-off-by: David Daney<david.daney@cavium.com>

WBR, Sergei

Re: [PATCH v2 2/2] hugetlb: Provide safer dummy values for HPAGE_MASK and HPAGE_SIZE

[David Daney]

On 11/18/2011 12:56 AM, Sergei Shtylyov wrote:
> Hello.
>
> On 18-11-2011 1:57, David Daney wrote:
>
>> From: David Daney<david.daney@cavium.com>
>
>> It was pointed out by David Rientjes that the dummy values for
>> HPAGE_MASK and HPAGE_SIZE are quite unsafe. It they are inadvertently
>> used with !CONFIG_HUGETLB_PAGE, compilation would succeed, but the
>> resulting code would surly not do anything sensible.
>>
>> Place BUG() in the these dummy definitions, as we do in similar
>> circumstances in other places, so any abuse can be easily detected.
>>
>> Since the only sane place to use these symbols when
>> !CONFIG_HUGETLB_PAGE is on dead code paths, the BUG() cause any actual
>> code to be emitted by the compiler.
>
> You mean "doesn't cause"?

Yes.

I mentioned this in a different message to akpm, but I am not sure if I 
should resend the patch with a corrected change log.

David Daney

>
>> Cc: David Rientjes<rientjes@google.com>
>> Signed-off-by: David Daney<david.daney@cavium.com>
>
> WBR, Sergei
>
>

Re: [PATCH v2 0/2] Dummy HPAGE_* constants for !CONFIG_HUGETLB_PAGE

[David Rientjes]

On Thu, 17 Nov 2011, David Daney wrote:

> From: David Daney <david.daney@cavium.com>
> 
> After a, somewhat heated, discussion with David Rientjes, I think the
> following approach will work.
> 
> The first patch adds HPAGE_SHIFT, needed by MIPS.
> 
> The second cleans up the exiting HPAGE_MASK and HPAGE_SIZE
> 
> David Daney (2):
>   hugetlb: Provide a default HPAGE_SHIFT if !CONFIG_HUGETLB_PAGE
>   hugetlb: Provide safer dummy values for HPAGE_MASK and HPAGE_SIZE
> 
>  include/linux/hugetlb.h |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> Cc: David Rientjes <rientjes@google.com>

Nack on both, we already discussed this in the other thread (and I 
wouldn't call it "somewhat heated").

We don't need these dummy definitions at all for any current architectures 
that use them and they should be removed so that any future code using 
them will have the proper dependency on CONFIG_HUGETLB_PAGE.

[patch] hugetlb: remove dummy definitions of HPAGE_MASK and HPAGE_SIZE

[David Rientjes]

Dummy, non-zero definitions for HPAGE_MASK and HPAGE_SIZE were added in
51c6f666fceb ("mm: ZAP_BLOCK causes redundant work") to avoid a divide
by zero in generic kernel code.

That code has since been removed, but probably should never have been
added in the first place: we don't want HPAGE_SIZE to act like PAGE_SIZE
for code that is working with hugepages, for example, when the dependency
on CONFIG_HUGETLB_PAGE has not been fulfilled.

Because hugepage size can differ from architecture to architecture, each
is required to have their own definitions for both HPAGE_MASK and
HPAGE_SIZE.  This is always done in arch/*/include/asm/page.h.

So, just remove the dummy and dangerous definitions since they are no
longer needed and reveals the correct dependencies.  Tested on
architectures using the definitions with allyesconfig: x86 (even with
thp), hppa, mips, powerpc, s390, sh3, sh4, sparc, and sparc64, and
with defconfig on ia64.

Cc: Robin Holt <holt@sgi.com>
Cc: David Daney <david.daney@cavium.com>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 include/linux/hugetlb.h |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -110,11 +110,6 @@ static inline void copy_huge_page(struct page *dst, struct page *src)
 
 #define hugetlb_change_protection(vma, address, end, newprot)
 
-#ifndef HPAGE_MASK
-#define HPAGE_MASK	PAGE_MASK		/* Keep the compiler happy */
-#define HPAGE_SIZE	PAGE_SIZE
-#endif
-
 #endif /* !CONFIG_HUGETLB_PAGE */
 
 #define HUGETLB_ANON_FILE "anon_hugepage"

Re: [patch] hugetlb: remove dummy definitions of HPAGE_MASK and HPAGE_SIZE

[Andrew Morton]

On Thu, 17 Nov 2011 15:22:53 -0800 (PST)
David Rientjes <rientjes@google.com> wrote:

> So, just remove the dummy and dangerous definitions since they are no
> longer needed and reveals the correct dependencies.  Tested on
> architectures using the definitions with allyesconfig: x86 (even with
> thp), hppa, mips, powerpc, s390, sh3, sh4, sparc, and sparc64, and
> with defconfig on ia64.

How could arch/mips/mm/tlb-r4k.c:local_flush_tlb_range() compile OK
with this change?

What that function is doing looks reasonable to me.  Why fill the poor
thing with an ifdef mess?

otoh, catching mistakes is good too.  Doing it at runtime as David
proposes is OK.

Re: [patch] hugetlb: remove dummy definitions of HPAGE_MASK and HPAGE_SIZE

[David Rientjes]

On Thu, 17 Nov 2011, Andrew Morton wrote:

> > So, just remove the dummy and dangerous definitions since they are no
> > longer needed and reveals the correct dependencies.  Tested on
> > architectures using the definitions with allyesconfig: x86 (even with
> > thp), hppa, mips, powerpc, s390, sh3, sh4, sparc, and sparc64, and
> > with defconfig on ia64.
> 
> How could arch/mips/mm/tlb-r4k.c:local_flush_tlb_range() compile OK
> with this change?
> 

This was tested on Linus' tree, not on Ralf's linux-next tree.  All uses 
of HPAGE_* are protected by CONFIG_HUGETLB_PAGE as it appropriately should 
be in Linus' tree in that file.

> What that function is doing looks reasonable to me.  Why fill the poor
> thing with an ifdef mess?
> 
> otoh, catching mistakes is good too.  Doing it at runtime as David
> proposes is OK.
> 

Nobody else needs it other than Ralf's pending change, and you're 
suggesting we need them in a generic header file when any sane arch that 
uses hugepages (all of them, in the current tree) declares these 
themselves in arch/*/include/asm/page.h where it's supposed to be done?

Why on earth do we have CONFIG_HUGETLB_PAGE for at all, then?  To catch 
code that's operating on hugepages when our kernel doesn't support it.  
I'd much rather break the build than get a runtime BUG() because we want 
to avoid an #ifdef or actually write well-written code like every other 
arch has!  Panicking the code to find errors like this is just insanity.

Re: [patch] hugetlb: remove dummy definitions of HPAGE_MASK and HPAGE_SIZE

[David Daney]

On 11/17/2011 03:44 PM, David Rientjes wrote:
> On Thu, 17 Nov 2011, Andrew Morton wrote:
>
>>> So, just remove the dummy and dangerous definitions since they are no
>>> longer needed and reveals the correct dependencies.  Tested on
>>> architectures using the definitions with allyesconfig: x86 (even with
>>> thp), hppa, mips, powerpc, s390, sh3, sh4, sparc, and sparc64, and
>>> with defconfig on ia64.
>>
>> How could arch/mips/mm/tlb-r4k.c:local_flush_tlb_range() compile OK
>> with this change?
>>
>
> This was tested on Linus' tree, not on Ralf's linux-next tree.  All uses
> of HPAGE_* are protected by CONFIG_HUGETLB_PAGE as it appropriately should
> be in Linus' tree in that file.
>
>> What that function is doing looks reasonable to me.  Why fill the poor
>> thing with an ifdef mess?
>>
>> otoh, catching mistakes is good too.  Doing it at runtime as David
>> proposes is OK.
>>
>
> Nobody else needs it other than Ralf's pending change, and you're
> suggesting we need them in a generic header file when any sane arch that
> uses hugepages (all of them, in the current tree) declares these
> themselves in arch/*/include/asm/page.h where it's supposed to be done?
>
> Why on earth do we have CONFIG_HUGETLB_PAGE for at all, then?  To catch
> code that's operating on hugepages when our kernel doesn't support it.
> I'd much rather break the build than get a runtime BUG() because we want
> to avoid an #ifdef or actually write well-written code like every other
> arch has!  Panicking the code to find errors like this is just insanity.
>

A counter argument would be:

There are hundreds of places in the kernel where dummy definitions are 
selected by !CONFIG_* so that we can do:

    if (test_something()) {
       do_one_thing();
    } else {
       do_the_other_thing();
    }


Rather than:

#ifdef CONFIG_SOMETHING
    if (test_something()) {
       do_one_thing();
    } else
#else
    {
       do_the_other_thing();
    }



We even do this all over the place with dummy definitions selected by 
CONFIG_HUGETLB_PAGE, What exactly makes HPAGE_MASK special and not the 
hundreds of other similar situations?

David Daney

Re: [patch] hugetlb: remove dummy definitions of HPAGE_MASK and HPAGE_SIZE

[Andrew Morton]

On Thu, 17 Nov 2011 15:52:28 -0800
David Daney <ddaney.cavm@gmail.com> wrote:

> A counter argument would be:
> 
> There are hundreds of places in the kernel where dummy definitions are 
> selected by !CONFIG_* so that we can do:
> 
>     if (test_something()) {
>        do_one_thing();
>     } else {
>        do_the_other_thing();
>     }
> 
> 
> Rather than:
> 
> #ifdef CONFIG_SOMETHING
>     if (test_something()) {
>        do_one_thing();
>     } else
> #else
>     {
>        do_the_other_thing();
>     }
> 
> 
> 
> We even do this all over the place with dummy definitions selected by 
> CONFIG_HUGETLB_PAGE, What exactly makes HPAGE_MASK special and not the 
> hundreds of other similar situations?

yup.  Look at free_pgtables():

		if (is_vm_hugetlb_page(vma)) {
			hugetlb_free_pgd_range(tlb, addr, vma->vm_end,
				floor, next? next->vm_start: ceiling);
		} else {

and

#define hugetlb_free_pgd_range(tlb, addr, end, floor, ceiling) ({BUG(); 0; })

This is the same thing.

Re: [patch] hugetlb: remove dummy definitions of HPAGE_MASK and HPAGE_SIZE

[David Rientjes]

On Thu, 17 Nov 2011, David Daney wrote:

> A counter argument would be:
> 
> There are hundreds of places in the kernel where dummy definitions are
> selected by !CONFIG_* so that we can do:
> 
>    if (test_something()) {
>       do_one_thing();
>    } else {
>       do_the_other_thing();
>    }
> 
> 
> Rather than:
> 
> #ifdef CONFIG_SOMETHING
>    if (test_something()) {
>       do_one_thing();
>    } else
> #else
>    {
>       do_the_other_thing();
>    }
> 
> 
> 
> We even do this all over the place with dummy definitions selected by
> CONFIG_HUGETLB_PAGE, What exactly makes HPAGE_MASK special and not the
> hundreds of other similar situations?
> 

Dummy functions that return 0 when a feature isn't enabled isn't the 
problem, that's very convenient.  Definitions of constants are a 
completely separate story because they can be easily used outside the 
required context and cause brekage.  What happens if you reference a 
variable in a function that is declarated in a different function?  Does 
the compiler put a BUG() in there and let you explode at runtime?  This is 
just absurd, every other arch has done the necessary declarations in their 
own header files and everything works fine because there's a clear config 
dependency on using HPAGE_*.  Adding dummy definitions to panic at runtime 
is irresponsibly to an extreme.

Re: [patch] hugetlb: remove dummy definitions of HPAGE_MASK and HPAGE_SIZE

[David Daney]

Linus,

It may not have been evident when you committed this (commit 
a5c86e986f0b2fe779f13cf53ce6e9f467b03950), but there was considerable 
discussion around this patch.

Andrew had taken into his tree a couple of patches to make the 
definitions that David Rientjes is removing safer:

http://marc.info/?l=linux-kernel&m=132156712623915&w=2
http://marc.info/?l=linux-kernel&m=132156712523914&w=2

David objected, but Andrew wasn't convinced, see all the replies to this 
patch but especially:

http://marc.info/?l=linux-kernel&m=132157428626522&w=2

On 11/17/2011 03:22 PM, David Rientjes wrote:
> Dummy, non-zero definitions for HPAGE_MASK and HPAGE_SIZE were added in
> 51c6f666fceb ("mm: ZAP_BLOCK causes redundant work") to avoid a divide
> by zero in generic kernel code.
>
> That code has since been removed, but probably should never have been
> added in the first place: we don't want HPAGE_SIZE to act like PAGE_SIZE
> for code that is working with hugepages, for example, when the dependency
> on CONFIG_HUGETLB_PAGE has not been fulfilled.
>
> Because hugepage size can differ from architecture to architecture, each
> is required to have their own definitions for both HPAGE_MASK and
> HPAGE_SIZE.  This is always done in arch/*/include/asm/page.h.
>
> So, just remove the dummy and dangerous definitions since they are no
> longer needed and reveals the correct dependencies.  Tested on
> architectures using the definitions with allyesconfig: x86 (even with
> thp), hppa, mips, powerpc, s390, sh3, sh4, sparc, and sparc64, and
> with defconfig on ia64.
>

This whole comment strikes me as somewhat dishonest, as at the time 
David Rientjes wrote it, he knew that there were dependencies on these 
symbols in the linux-next tree.

Now we can add these:
+#define HPAGE_SHIFT	({ BUG(); 0; })
+#define HPAGE_SIZE	({ BUG(); 0; })
+#define HPAGE_MASK	({ BUG(); 0; })

To the different architecture header files instead of having them in the 
common include/linux/hugetlb.h

If this is the way Linus wants it, I can live with that.  But it was a 
little surprising to see that this was merged when there were strong 
arguments against it.

David Daney

> Cc: Robin Holt<holt@sgi.com>
> Cc: David Daney<david.daney@cavium.com>
> Signed-off-by: David Rientjes<rientjes@google.com>
> ---
>   include/linux/hugetlb.h |    5 -----
>   1 files changed, 0 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -110,11 +110,6 @@ static inline void copy_huge_page(struct page *dst, struct page *src)
>
>   #define hugetlb_change_protection(vma, address, end, newprot)
>
> -#ifndef HPAGE_MASK
> -#define HPAGE_MASK	PAGE_MASK		/* Keep the compiler happy */
> -#define HPAGE_SIZE	PAGE_SIZE
> -#endif
> -
>   #endif /* !CONFIG_HUGETLB_PAGE */
>
>   #define HUGETLB_ANON_FILE "anon_hugepage"
>

Re: [patch] hugetlb: remove dummy definitions of HPAGE_MASK and HPAGE_SIZE

[Linus Torvalds]

On Mon, Nov 21, 2011 at 2:23 PM, David Daney <ddaney.cavm@gmail.com> wrote:
>
> This whole comment strikes me as somewhat dishonest, as at the time David
> Rientjes wrote it, he knew that there were dependencies on these symbols in
> the linux-next tree.
>
> Now we can add these:
> +#define HPAGE_SHIFT    ({ BUG(); 0; })
> +#define HPAGE_SIZE     ({ BUG(); 0; })
> +#define HPAGE_MASK     ({ BUG(); 0; })

Hell no.

We don't do run-time BUG() things. No way, no how.

If that #define cannot be used, then it damn well shouldn't be defined at all.

David's patch is clearly the right thing to do. Don't try to send me
the above kind of insane crap.

                     Linus

Re: [patch] hugetlb: remove dummy definitions of HPAGE_MASK and HPAGE_SIZE

[David Daney]

On 11/21/2011 02:43 PM, Linus Torvalds wrote:
> On Mon, Nov 21, 2011 at 2:23 PM, David Daney<ddaney.cavm@gmail.com>  wrote:
>>
>> This whole comment strikes me as somewhat dishonest, as at the time David
>> Rientjes wrote it, he knew that there were dependencies on these symbols in
>> the linux-next tree.
>>
>> Now we can add these:
>> +#define HPAGE_SHIFT    ({ BUG(); 0; })
>> +#define HPAGE_SIZE     ({ BUG(); 0; })
>> +#define HPAGE_MASK     ({ BUG(); 0; })
>
> Hell no.
>
> We don't do run-time BUG() things. No way, no how.
>
> If that #define cannot be used, then it damn well shouldn't be defined at all.
>
> David's patch is clearly the right thing to do. Don't try to send me
> the above kind of insane crap.
>

Ok Linus, for you I would recommend against running this git command on 
your tree:

git grep -E '#define.+BUG\(\);'

It's not like there isn't precedence.

David Daney

Re: [patch] hugetlb: remove dummy definitions of HPAGE_MASK and HPAGE_SIZE

[Linus Torvalds]

On Mon, Nov 21, 2011 at 3:23 PM, David Daney <ddaney@caviumnetworks.com> wrote:
>
> Ok Linus, for you I would recommend against running this git command on your
> tree:
>
> git grep -E '#define.+BUG\(\);'
>
> It's not like there isn't precedence.

So two wrongs make a right?

I do note that almost all the BUG() ones are in the same broken area:
hugepages. There's something wrong with the development there.

I wish people whose code had stuff like that would take a deep look at it.

                        Linus

Re: [patch] hugetlb: remove dummy definitions of HPAGE_MASK and HPAGE_SIZE

[Andrew Morton]

On Mon, 21 Nov 2011 15:43:26 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Mon, Nov 21, 2011 at 3:23 PM, David Daney <ddaney@caviumnetworks.com> wrote:
> >
> > Ok Linus, for you I would recommend against running this git command on your
> > tree:
> >
> > git grep -E '#define.+BUG\(\);'
> >
> > It's not like there isn't precedence.
> 
> So two wrongs make a right?
> 
> I do note that almost all the BUG() ones are in the same broken area:
> hugepages. There's something wrong with the development there.
> 
> I wish people whose code had stuff like that would take a deep look at it.
> 

The original decision way back when was that huge pages shouldn't mess
up the core VM too much.  One way in which we addressed that was to
make all its functions compilable with CONFIG_HUGETLB_PAGE=n, but they
should never be executed.  So the basic implementation pattern is:

#ifndef CONFIG_HUGETLB_PAGE
#define is_vm_hugetlb_page(p) 0
#define hugetlb_foo(x) BUG()

and...

	if (is_vm_hugetlb_page(...))
		hugetlb_foo(...);

The is_vm_hugetlb_page() evaluates to literal zero and so no code is
emitted for the hugetlb_foo() call.  The BUG() should never appear in
vmlinux but it's in there as a we-screwed-up thing.

Re: [patch] hugetlb: remove dummy definitions of HPAGE_MASK and HPAGE_SIZE

[Andi Kleen]

Andrew Morton <akpm@linux-foundation.org> writes:
>> 
>> I wish people whose code had stuff like that would take a deep look at it.
>> 
>
> The original decision way back when was that huge pages shouldn't mess
> up the core VM too much.  One way in which we addressed that was to

IMHO this decision should really be revisited now.  Originally hugetlb
was pretty simple, but these days it has most of the functionality of a
full VM now. In fact with THP we have 3 different VM systems now, all
subtle different with different issues. And with THP hugetlb will be
even more widely used than it is today.

On the other hand there are strange gaps now, like shared memory
doesn't work with THP, but only with hugetlbfs.

It would be far better to think about unifying these three VMs. Then with
less ifdefs it would also not need hacks like this anymore.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

Re: [patch] hugetlb: remove dummy definitions of HPAGE_MASK and HPAGE_SIZE

[David Daney]

Just to expand on this lovely topic...

On 11/21/2011 02:43 PM, Linus Torvalds wrote:
> On Mon, Nov 21, 2011 at 2:23 PM, David Daney<ddaney.cavm@gmail.com>  wrote:
>>
>> This whole comment strikes me as somewhat dishonest, as at the time David
>> Rientjes wrote it, he knew that there were dependencies on these symbols in
>> the linux-next tree.
>>
>> Now we can add these:
>> +#define HPAGE_SHIFT    ({ BUG(); 0; })
>> +#define HPAGE_SIZE     ({ BUG(); 0; })
>> +#define HPAGE_MASK     ({ BUG(); 0; })
>
> Hell no.
>
> We don't do run-time BUG() things. No way, no how.
>

These symbols are on dead code paths, so they are eliminated by the 
compiler's Dead Code Elimination (DCE) optimizations, and the BUG() code 
never gets emitted to the final executable.

I agree that it is not the best thing to do, but given the current state 
of the art in build bug macros, it is the best we could have done.

What I think we need instead, and for which I will send a patch soon, is 
something like this:

extern void you_are_screwed() __attribute__ ((error("BUILD_BUG_ON_USED 
failed")));
#define BUILD_BUG_ON_USED() you_are_screwed()
#define HPAGE_SHIFT    ({ BUILD_BUG_ON_USED(); 0; })

This allows us to use the symbols in straight line C code without a ton 
of ugly #ifdefery, but give us build time error checking.

Thanks,
David Daney


> If that #define cannot be used, then it damn well shouldn't be defined at all.
>
> David's patch is clearly the right thing to do. Don't try to send me
> the above kind of insane crap.
>
>                       Linus
>

Re: [patch] hugetlb: remove dummy definitions of HPAGE_MASK and HPAGE_SIZE

[Andrew Morton]

On Mon, 21 Nov 2011 15:47:32 -0800
David Daney <ddaney.cavm@gmail.com> wrote:

> Just to expand on this lovely topic...
> 
> On 11/21/2011 02:43 PM, Linus Torvalds wrote:
> > On Mon, Nov 21, 2011 at 2:23 PM, David Daney<ddaney.cavm@gmail.com>  wrote:
> >>
> >> This whole comment strikes me as somewhat dishonest, as at the time David
> >> Rientjes wrote it, he knew that there were dependencies on these symbols in
> >> the linux-next tree.
> >>
> >> Now we can add these:
> >> +#define HPAGE_SHIFT    ({ BUG(); 0; })
> >> +#define HPAGE_SIZE     ({ BUG(); 0; })
> >> +#define HPAGE_MASK     ({ BUG(); 0; })
> >
> > Hell no.
> >
> > We don't do run-time BUG() things. No way, no how.
> >
> 
> These symbols are on dead code paths, so they are eliminated by the 
> compiler's Dead Code Elimination (DCE) optimizations, and the BUG() code 
> never gets emitted to the final executable.
> 
> I agree that it is not the best thing to do, but given the current state 
> of the art in build bug macros, it is the best we could have done.
> 
> What I think we need instead, and for which I will send a patch soon, is 
> something like this:
> 
> extern void you_are_screwed() __attribute__ ((error("BUILD_BUG_ON_USED 
> failed")));
> #define BUILD_BUG_ON_USED() you_are_screwed()
> #define HPAGE_SHIFT    ({ BUILD_BUG_ON_USED(); 0; })
> 
> This allows us to use the symbols in straight line C code without a ton 
> of ugly #ifdefery, but give us build time error checking.

The way we usually handle that is to emit a call to a
this_function_does_not_exist(), so it fails at link time.

I guess we should do that to all those follow_hugetlb_page() and
friends.

gcc has had issues at times where it incorrectly emits references to
this_function_does_not_exist(), but that hasn't happened in a couple of
years as far as I know.

Re: [patch] hugetlb: remove dummy definitions of HPAGE_MASK and HPAGE_SIZE

[Linus Torvalds]

On Mon, Nov 21, 2011 at 3:47 PM, David Daney <ddaney.cavm@gmail.com> wrote:
>
> These symbols are on dead code paths, so they are eliminated by the
> compiler's Dead Code Elimination (DCE) optimizations, and the BUG() code
> never gets emitted to the final executable.

If you are so damn sure of that, then DON'T MAKE IT A BUG_ON! If you
are 100% syre, then you might as well leave out the BUG_ON() entirely.

Seriously. What's so hard to understand?

Either you are 100% sure, or you are not. If you are 100% sure, then
the BUG_ON() is pointless. And if you are not, then the BUG_ON() is
*wrong*.

Notice? The BUG_ON() is never *ever* valid. You cannot have it both
ways. So stop pushing crap, already!

So what are non-crap solutions?

 - the current one: error out at compile time (early) if somebody uses
them in invalid contexts.

   This seems to be a good case, especially since apparently no actual
current code wants to use them outside of the existing #ifdef's. And
there is no reason to think that some random MIPS-only future code is
a good enough reason to re-introduce these things

 - if you really want to use them, but expect the compiler to always
compile them away as dead code, use a non-existing function linkage,
so that you at least get a static failure at link-time for incorrect
code, rather than some random BUG_ON() at run-time that may be
impossible to find.

See? There are real solutions. BUG_ON() is not one of them.

                  Linus

Re: [patch] hugetlb: remove dummy definitions of HPAGE_MASK and HPAGE_SIZE

[David Daney]

On 11/21/2011 04:37 PM, Linus Torvalds wrote:
> On Mon, Nov 21, 2011 at 3:47 PM, David Daney<ddaney.cavm@gmail.com>  wrote:
>>
>> These symbols are on dead code paths, so they are eliminated by the
>> compiler's Dead Code Elimination (DCE) optimizations, and the BUG() code
>> never gets emitted to the final executable.
>
> If you are so damn sure of that, then DON'T MAKE IT A BUG_ON! If you
> are 100% syre, then you might as well leave out the BUG_ON() entirely.
>
> Seriously. What's so hard to understand?

Really Linus, did you read the other half of the message you quoted?

The part you quote above explains the reason things are the way they 
currently are.

The second part, that I think you may have missed, is the part I had 
hoped you would read...

>
> Either you are 100% sure, or you are not. If you are 100% sure, then
> the BUG_ON() is pointless. And if you are not, then the BUG_ON() is
> *wrong*.
>
> Notice? The BUG_ON() is never *ever* valid. You cannot have it both
> ways. So stop pushing crap, already!
>
> So what are non-crap solutions?
>
>   - the current one: error out at compile time (early) if somebody uses
> them in invalid contexts.
>
>     This seems to be a good case, especially since apparently no actual
> current code wants to use them outside of the existing #ifdef's. And
> there is no reason to think that some random MIPS-only future code is
> a good enough reason to re-introduce these things
>
>   - if you really want to use them, but expect the compiler to always
> compile them away as dead code, use a non-existing function linkage,
> so that you at least get a static failure at link-time for incorrect
> code, rather than some random BUG_ON() at run-time that may be
> impossible to find.
>
> See? There are real solutions. BUG_ON() is not one of them.

... because it said exactly what you say above.

I will send you a patch within the next two hours that shows what may be 
an acceptable solution.

Thanks,
David Daney

Re: [patch] hugetlb: remove dummy definitions of HPAGE_MASK and HPAGE_SIZE

[Linus Torvalds]

On Mon, Nov 21, 2011 at 4:48 PM, David Daney <ddaney@caviumnetworks.com> wrote:
>
> Really Linus, did you read the other half of the message you quoted?

Sorry no. It was just an automatic reaction along the lines of "oh
christ, is this argument still going on"?

I'm ok with magic gcc function attributes, although I still don't see
why you need that symbol so badly.

Quite frankly, some MIPS-only patch is *not* important-enough to worry
about this. I'd rather have the simpler "that symbol doesn't exist,
and MIPS just needs an #ifdef".

I realize that we *used* to have code that did that

   if (something_that_evaluates_to_zero) {
       .. use HPAGE_MASK ..

but considering that we don't have that any more and people are happy,
I'm not all that convinced that we need to try to re-introduce it.

                     Linus

Re: [patch] hugetlb: remove dummy definitions of HPAGE_MASK and HPAGE_SIZE

[David Rientjes]

On Mon, 21 Nov 2011, David Daney wrote:

> > So, just remove the dummy and dangerous definitions since they are no
> > longer needed and reveals the correct dependencies.  Tested on
> > architectures using the definitions with allyesconfig: x86 (even with
> > thp), hppa, mips, powerpc, s390, sh3, sh4, sparc, and sparc64, and
> > with defconfig on ia64.
> > 
> 
> This whole comment strikes me as somewhat dishonest, as at the time David
> Rientjes wrote it, he knew that there were dependencies on these symbols in
> the linux-next tree.
> 

I was referring to Linus' tree at the time the patch was merged, not 
linux-next.  However, yes, I was aware of the build breakage it caused in 
Ralf's mips tree and I sent a patch to fix that breakage to the mips 
folks:

	http://marc.info/?l=linux-mips&m=132175788803677

Thanks.