mm: negative left shift count when PAGE_SHIFT > 20

PATCH In review Jerry 3 participants 7 messages Jul 18, 2013 – Jul 20, 2013

Subsystems (2)

Memory management Page allocator

lore

Share & tools

lore

Jerry

author

2013-07-18 16:56 UTC #0 lore raw

PATCH 1/1 mm: negative left shift count when PAGE_SHIFT > 20

When PAGE_SHIFT > 20, the result of "20 - PAGE_SHIFT" is negative. The calculating here will generate an unexpected result. In addition, if PAGE_SHIFT > 20, The memory size represented by numentries was already integral multiple of 1MB.

Signed-off-by Jerry <[email protected]>

---
 mm/page_alloc.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

mm/page_alloc.c +5 -3

index b100255..cd41797 100644
@@ -5745,9 +5745,11 @@ void *__init alloc_large_system_hash(const char *tablename,
 	if (!numentries) {
 		/* round applicable memory size up to nearest megabyte */
 		numentries = nr_kernel_pages;
-		numentries += (1UL << (20 - PAGE_SHIFT)) - 1;
-		numentries >>= 20 - PAGE_SHIFT;
-		numentries <<= 20 - PAGE_SHIFT;
+		if (20 > PAGE_SHIFT) {
+			numentries += (1UL << (20 - PAGE_SHIFT)) - 1;
+			numentries >>= 20 - PAGE_SHIFT;
+			numentries <<= 20 - PAGE_SHIFT;
+		}
 
 		/* limit to 1 bucket per 2^scale bytes of low memory */
 		if (scale > PAGE_SHIFT)

Signature · 7 lines

1.8.1.5

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

John Stoffel

2013-07-18 17:13 UTC #1 lore raw

Jerry> When PAGE_SHIFT > 20, the result of "20 - PAGE_SHIFT" is negative. The Jerry> calculating here will generate an unexpected result. In addition, if Jerry> PAGE_SHIFT > 20, The memory size represented by numentries was already Jerry> integral multiple of 1MB.

Why this magic number of 20? Please explain it better and replace it was a #define that means something here.

Jerry> Signed-off-by: Jerry <[email protected]> Jerry> ---
Jerry> mm/page_alloc.c | 8 +++++---
Jerry> 1 file changed, 5 insertions(+), 3 deletions(-)

Jerry> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
Jerry> index b100255..cd41797 100644
Jerry> --- a/mm/page_alloc.c
Jerry> +++ b/mm/page_alloc.c
Jerry> @@ -5745,9 +5745,11 @@ void *__init alloc_large_system_hash(const char *tablename,

Jerry>  	if (!numentries) {
Jerry>  		/* round applicable memory size up to nearest megabyte */
Jerry>  		numentries = nr_kernel_pages;

Jerry> - numentries += (1UL << (20 - PAGE_SHIFT)) - 1;
Jerry> - numentries >>= 20 - PAGE_SHIFT;
Jerry> - numentries <<= 20 - PAGE_SHIFT;
Jerry> + if (20 > PAGE_SHIFT) {
Jerry> + numentries += (1UL << (20 - PAGE_SHIFT)) - 1;
Jerry> + numentries >>= 20 - PAGE_SHIFT;
Jerry> + numentries <<= 20 - PAGE_SHIFT;
Jerry> + }

Jerry> /* limit to 1 bucket per 2^scale bytes of low memory */
Jerry> if (scale > PAGE_SHIFT)
Jerry> --
Jerry> 1.8.1.5

Jerry> --
Jerry> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in Jerry> the body of a message to [email protected]
Jerry> More majordomo info at http://vger.kernel.org/majordomo-info.html
Jerry> Please read the FAQ at http://www.tux.org/lkml/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Jerry

author

2013-07-18 17:21 UTC #2 lore raw

Quoted 7 lines

Jerry> When PAGE_SHIFT > 20, the result of "20 - PAGE_SHIFT" is negative. The Jerry> calculating here will generate an unexpected result. In addition, if Jerry> PAGE_SHIFT > 20, The memory size represented by numentries was already Jerry> integral multiple of 1MB.
Why this magic number of 20? Please explain it better and replace it was a #define that means something here.

Because 2^20 = 1MB.

The intention of previous code is "/* round applicable memory size up to nearest megabyte */".

Quoted 28 lines

Jerry> Signed-off-by: Jerry <[email protected]> Jerry> ---
Jerry> mm/page_alloc.c | 8 +++++---
Jerry> 1 file changed, 5 insertions(+), 3 deletions(-)
Jerry> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
Jerry> index b100255..cd41797 100644
Jerry> --- a/mm/page_alloc.c
Jerry> +++ b/mm/page_alloc.c
Jerry> @@ -5745,9 +5745,11 @@ void *__init alloc_large_system_hash(const char *tablename,
Jerry> if (!numentries) {
Jerry>                  /* round applicable memory size up to nearest megabyte */
Jerry>                  numentries = nr_kernel_pages;
Jerry> -                numentries += (1UL << (20 - PAGE_SHIFT)) - 1;
Jerry> -                numentries >>= 20 - PAGE_SHIFT;
Jerry> -                numentries <<= 20 - PAGE_SHIFT;
Jerry> +                if (20 > PAGE_SHIFT) {
Jerry> +                        numentries += (1UL << (20 - PAGE_SHIFT)) - 1;
Jerry> +                        numentries >>= 20 - PAGE_SHIFT;
Jerry> +                        numentries <<= 20 - PAGE_SHIFT;
Jerry> + }
Jerry> /* limit to 1 bucket per 2^scale bytes of low memory */
Jerry> if (scale > PAGE_SHIFT)
Jerry> --
Jerry> 1.8.1.5

Quoted 5 lines

Jerry> --
Jerry> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in Jerry> the body of a message to [email protected]
Jerry> More majordomo info at http://vger.kernel.org/majordomo-info.html
Jerry> Please read the FAQ at http://www.tux.org/lkml/

--
I love linux!!!

Andrew Morton

maintainer

2013-07-18 21:39 UTC #3 lore raw

In reply to Jerry

Quoted 5 lines · On Fri, 19 Jul 2013 00:56:12 +0800 Jerry <[email protected]> wrote:

When PAGE_SHIFT > 20, the result of "20 - PAGE_SHIFT" is negative. The calculating here will generate an unexpected result. In addition, if PAGE_SHIFT > 20, The memory size represented by numentries was already integral multiple of 1MB.

If you tell me that you have a machine which has PAGE_SIZE=2MB and this was the only problem which prevented Linux from running on that machine then I'll apply the patch ;)

Jerry

author

2013-07-18 23:47 UTC #4 lore raw

2013/7/19 Andrew Morton <[email protected]>:

Quoted 8 lines · On Fri, 19 Jul 2013 00:56:12 +0800 Jerry <[email protected]> wrote:

Quoted 6 lines
When PAGE_SHIFT > 20, the result of "20 - PAGE_SHIFT" is negative. The calculating here will generate an unexpected result. In addition, if PAGE_SHIFT > 20, The memory size represented by numentries was already integral multiple of 1MB.

Quoted 4 lines

If you tell me that you have a machine which has PAGE_SIZE=2MB and this was the only problem which prevented Linux from running on that machine then I'll apply the patch ;)

Hi Morton:
I just "grep -rn "#define\s\+PAGE_SHIFT" arch/", and find the PAGE_SHIFT in some architecture is very big.
such as the following in "arch/hexagon/include/asm/page.h"
....
#ifdef CONFIG_PAGE_SIZE_256KB
#define PAGE_SHIFT 18
#define HEXAGON_L1_PTE_SIZE __HVM_PDE_S_256KB #endif

#ifdef CONFIG_PAGE_SIZE_1MB
#define PAGE_SHIFT 20
#define HEXAGON_L1_PTE_SIZE __HVM_PDE_S_1MB
#endif
.....

Maybe the day of "A 2MB page" is not far. :-) I know it is just a latent issue. Even if it won't generate a error when PAGE_SIZE == 20, the calculating here is not necessary. In my mind, compiler would optimize the calculating at that situation. But it is a little tricky.

In my patch, I think compiler would optimize "if (20 > PAGE_SIZE)", it won't generate any machine instruction. Just a guarantee.

--
I love linux!!!

Andrew Morton

maintainer

2013-07-19 21:57 UTC #5 lore raw

Quoted 24 lines · On Fri, 19 Jul 2013 07:47:02 +0800 Jerry <[email protected]> wrote:

2013/7/19 Andrew Morton <[email protected]>:
Quoted 8 lines · On Fri, 19 Jul 2013 00:56:12 +0800 Jerry <[email protected]> wrote:
Quoted 6 lines
When PAGE_SHIFT > 20, the result of "20 - PAGE_SHIFT" is negative. The calculating here will generate an unexpected result. In addition, if PAGE_SHIFT > 20, The memory size represented by numentries was already integral multiple of 1MB.
Quoted 4 lines
If you tell me that you have a machine which has PAGE_SIZE=2MB and this was the only problem which prevented Linux from running on that machine then I'll apply the patch ;)
Hi Morton:
I just "grep -rn "#define\s\+PAGE_SHIFT" arch/", and find the PAGE_SHIFT in some architecture is very big.
such as the following in "arch/hexagon/include/asm/page.h"
....
#ifdef CONFIG_PAGE_SIZE_256KB
#define PAGE_SHIFT 18
#define HEXAGON_L1_PTE_SIZE __HVM_PDE_S_256KB #endif

Quoted 5 lines

#ifdef CONFIG_PAGE_SIZE_1MB
#define PAGE_SHIFT 20
#define HEXAGON_L1_PTE_SIZE __HVM_PDE_S_1MB
#endif
.....

Good heavens.

Quoted 2 lines

In my patch, I think compiler would optimize "if (20 > PAGE_SIZE)", it won't generate any machine instruction. Just a guarantee.

Well the existing code is a bit silly looking. Why can't we just do

	/* round applicable memory size up to nearest megabyte */
	if (PAGE_SHIFT < 20)
		numentries = round_up(nr_kernel_pages, (1 << 20)/PAGE_SIZE);

or similar?

Jerry

author

2013-07-20 17:17 UTC #6 lore raw

2013/7/20 Andrew Morton <[email protected]>:

Quoted 37 lines · On Fri, 19 Jul 2013 07:47:02 +0800 Jerry <[email protected]> wrote:

Quoted 25 lines
2013/7/19 Andrew Morton <[email protected]>:
Quoted 8 lines · On Fri, 19 Jul 2013 00:56:12 +0800 Jerry <[email protected]> wrote:
Quoted 6 lines
When PAGE_SHIFT > 20, the result of "20 - PAGE_SHIFT" is negative. The calculating here will generate an unexpected result. In addition, if PAGE_SHIFT > 20, The memory size represented by numentries was already integral multiple of 1MB.
Quoted 4 lines
If you tell me that you have a machine which has PAGE_SIZE=2MB and this was the only problem which prevented Linux from running on that machine then I'll apply the patch ;)
Hi Morton:
I just "grep -rn "#define\s\+PAGE_SHIFT" arch/", and find the PAGE_SHIFT in some architecture is very big.
such as the following in "arch/hexagon/include/asm/page.h"
....
#ifdef CONFIG_PAGE_SIZE_256KB
#define PAGE_SHIFT 18
#define HEXAGON_L1_PTE_SIZE __HVM_PDE_S_256KB #endif
Quoted 5 lines
#ifdef CONFIG_PAGE_SIZE_1MB
#define PAGE_SHIFT 20
#define HEXAGON_L1_PTE_SIZE __HVM_PDE_S_1MB
#endif
.....
Good heavens.
Quoted 2 lines
In my patch, I think compiler would optimize "if (20 > PAGE_SIZE)", it won't generate any machine instruction. Just a guarantee.

Quoted 7 lines

Well the existing code is a bit silly looking. Why can't we just do

        /* round applicable memory size up to nearest megabyte */
        if (PAGE_SHIFT < 20)
                numentries = round_up(nr_kernel_pages, (1 << 20)/PAGE_SIZE);

or similar?

Great. I have adjusted these code lines, and sent the latest one.

--
I love linux!!!