mm: memory-hotplug: enable memory hotplug to handle hugepage

Hello Naoya Horiguchi,

The patch c8721bbbdd36: "mm: memory-hotplug: enable memory hotplug to handle hugepage" from Sep 11, 2013, leads to the following static checker warning:

	mm/hugetlb.c:1203 dissolve_free_huge_pages()
	warn: potential right shift more than type allows '9,18,64'

mm/hugetlb.c
1189 void dissolve_free_huge_pages(unsigned long start_pfn, unsigned long end_pfn) 1190 {

  1191          unsigned int order = 8 * sizeof(void *);
                                     ^^^^^^^^^^^^^^^^^^

Let's say order is 64.

Quoted 9 lines · Dan Carpenter

Hello Naoya Horiguchi,

The patch c8721bbbdd36: "mm: memory-hotplug: enable memory hotplug to handle hugepage" from Sep 11, 2013, leads to the following static checker warning:

	mm/hugetlb.c:1203 dissolve_free_huge_pages()
	warn: potential right shift more than type allows '9,18,64'

Quoted 6 lines

mm/hugetlb.c
1189 void dissolve_free_huge_pages(unsigned long start_pfn, unsigned long end_pfn) 1190 {

  1191          unsigned int order = 8 * sizeof(void *);
                                     ^^^^^^^^^^^^^^^^^^

Let's say order is 64.

Actually, the 64 here is just chosen to be an impossibly high number isn't it?

It's a bit complicated to understand that at first glance.

Hello Naoya Horiguchi,

The patch c8721bbbdd36: "mm: memory-hotplug: enable memory hotplug to handle hugepage" from Sep 11, 2013, leads to the following static checker warning:

	mm/hugetlb.c:1203 dissolve_free_huge_pages()
	warn: potential right shift more than type allows '9,18,64'

mm/hugetlb.c
1189 void dissolve_free_huge_pages(unsigned long start_pfn, unsigned long end_pfn) 1190 {

  1191          unsigned int order = 8 * sizeof(void *);
                                     ^^^^^^^^^^^^^^^^^^

Let's say order is 64.

1192 unsigned long pfn;
1193 struct hstate *h;
1194
1195 if (!hugepages_supported())
1196 return;
1197
1198 /* Set scan step to minimum hugepage size */
1199 for_each_hstate(h)
1200 if (order > huge_page_order(h))
1201 order = huge_page_order(h);
1202 VM_BUG_ON(!IS_ALIGNED(start_pfn, 1 << order));
1203 for (pfn = start_pfn; pfn < end_pfn; pfn += 1 << order)

                                                            ^^^^^^^^^^

1 << 64 is undefined but let's say it's zero because that's normal for GCC. This is an endless loop.

Review static void dissolve_free_huge_page(struct page *page) · line 1090

Quoted 7 lines · Naoya Horiguchi

(quoted diff) +2 -1

@@ -1086,7 +1086,8 @@ static void dissolve_free_huge_page(struct page *page)
  */
 void dissolve_free_huge_pages(unsigned long start_pfn, unsigned long end_pfn)
 {
-	unsigned int order = 8 * sizeof(void *);
+	/* Initialized to "high enough" value which is capped later */
+	unsigned int order = 8 * sizeof(void *) - 1;

Why not use UINT_MAX? It's more clear that it's not valid that way.

Otherwise doing a complicated calculation it makes it seem like we will use the variable.

Quoted 10 lines · Dan Carpenter

mm/hugetlb.c static void dissolve_free_huge_page(struct page *page) · line 1090

Quoted 7 lines · Naoya Horiguchi

mm/hugetlb.c +2 -1

@@ -1086,7 +1086,8 @@ static void dissolve_free_huge_page(struct page *page)
  */
 void dissolve_free_huge_pages(unsigned long start_pfn, unsigned long end_pfn)
 {
-	unsigned int order = 8 * sizeof(void *);
+	/* Initialized to "high enough" value which is capped later */
+	unsigned int order = 8 * sizeof(void *) - 1;

Why not use UINT_MAX? It's more clear that it's not valid that way.

It's OK if code checker doesn't show "too much right shift" warning.

Quoted 12 lines · Naoya Horiguchi

Quoted 10 lines · Dan Carpenter

mm/hugetlb.c static void dissolve_free_huge_page(struct page *page) · line 1090

Quoted 7 lines · Naoya Horiguchi

mm/hugetlb.c +2 -1

@@ -1086,7 +1086,8 @@ static void dissolve_free_huge_page(struct page *page)
  */
 void dissolve_free_huge_pages(unsigned long start_pfn, unsigned long end_pfn)
 {
-	unsigned int order = 8 * sizeof(void *);
+	/* Initialized to "high enough" value which is capped later */
+	unsigned int order = 8 * sizeof(void *) - 1;

Why not use UINT_MAX? It's more clear that it's not valid that way.

Quoted 1 line

It's OK if code checker doesn't show "too much right shift" warning.

It's a comlicated question to answer but with the new VM_BUG_ON() then it won't warn.

Currently the initial value of order in dissolve_free_huge_page is 64 or 32, which leads to the following warning in static checker:

mm/hugetlb.c:1203 dissolve_free_huge_pages()
warn: potential right shift more than type allows '9,18,64'

This is a potential risk of infinite loop, because 1 << order (== 0) is used in for-loop like this:

for (pfn = start_pfn; pfn < end_pfn; pfn += 1 << order)

      ...

So this patch simply avoids the risk by initializing with UINT_MAX.

..

mm/hugetlb.c +1 -1

@@ -1188,7 +1188,7 @@ static void dissolve_free_huge_page(struct page *page)
  */
 void dissolve_free_huge_pages(unsigned long start_pfn, unsigned long end_pfn)
 {
-	unsigned int order = 8 * sizeof(void *);
+	unsigned int order = UINT_MAX;
 	unsigned long pfn;
 	struct hstate *h;
 

Quoted 27 lines · On Tue, 12 May 2015 09:20:35 +0000 Naoya Horiguchi <[email protected]>…

Currently the initial value of order in dissolve_free_huge_page is 64 or 32, which leads to the following warning in static checker:

mm/hugetlb.c:1203 dissolve_free_huge_pages()
warn: potential right shift more than type allows '9,18,64'

This is a potential risk of infinite loop, because 1 << order (== 0) is used in for-loop like this:

for (pfn = start_pfn; pfn < end_pfn; pfn += 1 << order)

      ...

So this patch simply avoids the risk by initializing with UINT_MAX.

..

mm/hugetlb.c +1 -1

@@ -1188,7 +1188,7 @@ static void dissolve_free_huge_page(struct page *page)
  */
 void dissolve_free_huge_pages(unsigned long start_pfn, unsigned long end_pfn)
 {
-	unsigned int order = 8 * sizeof(void *);
+	unsigned int order = UINT_MAX;
 	unsigned long pfn;
 	struct hstate *h;
 

mm/hugetlb.c void dissolve_free_huge_pages(unsigned long start_pfn, unsigned long end_pfn) · line 1205

Quoted 7 lines

mm/hugetlb.c +1 -0

@@ -1200,6 +1200,7 @@ void dissolve_free_huge_pages(unsigned long start_pfn, unsigned long end_pfn)
 		if (order > huge_page_order(h))
 			order = huge_page_order(h);
 	VM_BUG_ON(!IS_ALIGNED(start_pfn, 1 << order));
+	VM_BUG_ON(order == UINT_MAX);
 	for (pfn = start_pfn; pfn < end_pfn; pfn += 1 << order)
 		dissolve_free_huge_page(pfn_to_page(pfn));

Do we need to calculate this each time? Can it be done in hugetlb_init_hstates(), save the result in a global?

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +1 -0

 			order = huge_page_order(h);
 	VM_BUG_ON(!IS_ALIGNED(start_pfn, 1 << order));
+	VM_BUG_ON(order == UINT_MAX);
 	for (pfn = start_pfn; pfn < end_pfn; pfn += 1 << order)
 		dissolve_free_huge_page(pfn_to_page(pfn));

Do we need to calculate this each time? Can it be done in hugetlb_init_hstates(), save the result in a global?

Yes, it should work. How about the following?
This adds 4bytes to .data due to a new global variable, but reduces 47 bytes .text size of code reduces, so it's a win in total.

text data bss dec hex filename

  28313     469   84236  113018   1b97a mm/hugetlb.o (above patch)
  28266     473   84236  112975   1b94f mm/hugetlb.o (below patch)

Quoted 13 lines · On Wed, 13 May 2015 01:44:22 +0000 Naoya Horiguchi <[email protected]>…

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +1 -0

 			order = huge_page_order(h);
 	VM_BUG_ON(!IS_ALIGNED(start_pfn, 1 << order));
+	VM_BUG_ON(order == UINT_MAX);
 	for (pfn = start_pfn; pfn < end_pfn; pfn += 1 << order)
 		dissolve_free_huge_page(pfn_to_page(pfn));

Do we need to calculate this each time? Can it be done in hugetlb_init_hstates(), save the result in a global?

Yes, it should work. How about the following?
This adds 4bytes to .data due to a new global variable, but reduces 47 bytes .text size of code reduces, so it's a win in total.

Quoted 3 lines

text data bss dec hex filename

  28313     469   84236  113018   1b97a mm/hugetlb.o (above patch)
  28266     473   84236  112975   1b94f mm/hugetlb.o (below patch)

Looks good. Please turn it into a real patch and send it over when convenient?

mm/hugetlb.c int hugepages_treat_as_movable; · line 43

Quoted 7 lines

mm/hugetlb.c +1 -0

@@ -40,6 +40,7 @@ int hugepages_treat_as_movable;
 int hugetlb_max_hstate __read_mostly;
 unsigned int default_hstate_idx;
 struct hstate hstates[HUGE_MAX_HSTATE];
+unsigned int minimum_order __read_mostly;

static.

And a comment would be nice ;)

mm/hugetlb.c static void __init hugetlb_init_hstates(void) · line 1624

Quoted 9 lines

...

mm/hugetlb.c +1 -0

@@ -1626,11 +1621,16 @@ static void __init hugetlb_init_hstates(void)
 {
 	struct hstate *h;
 
+	minimum_order = UINT_MAX;

Do this at compile time.

mm/hugetlb.c static void __init hugetlb_init_hstates(void) · line 1633

Quoted 9 lines

mm/hugetlb.c +4 -0

 	for_each_hstate(h) {
+		if (minimum_order > huge_page_order(h))
+			minimum_order = huge_page_order(h);
+
 		/* oversize hugepages were init'ed in early boot */
 		if (!hstate_is_gigantic(h))
 			hugetlb_hstate_alloc_pages(h);
 	}
+	VM_BUG_ON(minimum_order == UINT_MAX);

Is the system hopelessly screwed up when this happens, or will it still be able to boot up and do useful things?

If the system is hopelessly broken then BUG_ON or, better, panic should be used here. But if there's still potential to do useful things then I guess VM_BUG_ON is appropriate.