mm/hugetlb: Small cleanup and improvement

The local variable is for global reservation of region.

Signed-off-by Baoquan He <[email protected]>

---
 mm/hugetlb.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

mm/hugetlb.c +0 -0

index f24acb3af741..191a585bb315 100644

Quoted 7 lines · Baoquan He

The local variable is for global reservation of region.

Signed-off-by Baoquan He <[email protected]>

---
 mm/hugetlb.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

mm/hugetlb.c static void hugetlb_vm_op_close(struct vm_area_struct *vma) · line 3652

Quoted 10 lines

mm/hugetlb.c +1 -1

index f24acb3af741..191a585bb315 100644
@@ -3649,7 +3649,7 @@ static void hugetlb_vm_op_close(struct vm_area_struct *vma)
 	struct resv_map *resv = vma_resv_map(vma);
 	struct hugepage_subpool *spool = subpool_vma(vma);
 	unsigned long reserve, start, end;
-	long gbl_reserve;
+	long glb_reserve;

I see both 'gbl' and 'glb' being used for global in variable names. grep will actually return more hits for gbl than glb. Unless there is consensus that 'glb' should be used for global, I would prefer not to make this change.

Just like his neighbour is_hugetlb_entry_migration() has done.

Signed-off-by Baoquan He <[email protected]>

---
 mm/hugetlb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

mm/hugetlb.c +4 -4

index 191a585bb315..a58f976a9dd9 100644
@@ -3754,17 +3754,17 @@ bool is_hugetlb_entry_migration(pte_t pte)
 		return false;
 }
 
-static int is_hugetlb_entry_hwpoisoned(pte_t pte)
+static bool is_hugetlb_entry_hwpoisoned(pte_t pte)
 {
 	swp_entry_t swp;
 
 	if (huge_pte_none(pte) || pte_present(pte))
-		return 0;
+		return false;
 	swp = pte_to_swp_entry(pte);
 	if (non_swap_entry(swp) && is_hwpoison_entry(swp))
-		return 1;
+		return true;
 	else
-		return 0;
+		return false;
 }
 
 int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,

Just like his neighbour is_hugetlb_entry_migration() has done.

Signed-off-by Baoquan He <[email protected]>

---
 mm/hugetlb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

mm/hugetlb.c +2 -2

index 191a585bb315..a58f976a9dd9 100644
@@ -3754,17 +3754,17 @@ bool is_hugetlb_entry_migration(pte_t pte)
 		return false;
 }
 
-static int is_hugetlb_entry_hwpoisoned(pte_t pte)
+static bool is_hugetlb_entry_hwpoisoned(pte_t pte)
 {
 	swp_entry_t swp;
 
 	if (huge_pte_none(pte) || pte_present(pte))
-		return 0;
+		return false;

The checking is_migration_entry() and is_hwpoison_entry() are stricter than non_swap_entry(), means they have covered the conditional check which non_swap_entry() is doing.

Hence remove the unnecessary non_swap_entry() in is_hugetlb_entry_migration() and is_hugetlb_entry_hwpoisoned() to simplify code.

Signed-off-by Baoquan He <[email protected]>

---
 mm/hugetlb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

mm/hugetlb.c +2 -2

index a58f976a9dd9..467894d8332a 100644
@@ -3748,7 +3748,7 @@ bool is_hugetlb_entry_migration(pte_t pte)
 	if (huge_pte_none(pte) || pte_present(pte))
 		return false;
 	swp = pte_to_swp_entry(pte);
-	if (non_swap_entry(swp) && is_migration_entry(swp))
+	if (is_migration_entry(swp))
 		return true;
 	else
 		return false;

@@ -3761,7 +3761,7 @@ static bool is_hugetlb_entry_hwpoisoned(pte_t pte)
 	if (huge_pte_none(pte) || pte_present(pte))
 		return false;
 	swp = pte_to_swp_entry(pte);
-	if (non_swap_entry(swp) && is_hwpoison_entry(swp))
+	if (is_hwpoison_entry(swp))
 		return true;
 	else
 		return false;

The checking is_migration_entry() and is_hwpoison_entry() are stricter than non_swap_entry(), means they have covered the conditional check which non_swap_entry() is doing.

Hence remove the unnecessary non_swap_entry() in is_hugetlb_entry_migration() and is_hugetlb_entry_hwpoisoned() to simplify code.

Signed-off-by Baoquan He <[email protected]>

---
 mm/hugetlb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

mm/hugetlb.c +1 -1

index a58f976a9dd9..467894d8332a 100644
@@ -3748,7 +3748,7 @@ bool is_hugetlb_entry_migration(pte_t pte)
 	if (huge_pte_none(pte) || pte_present(pte))
 		return false;
 	swp = pte_to_swp_entry(pte);
-	if (non_swap_entry(swp) && is_migration_entry(swp))
+	if (is_migration_entry(swp))
 		return true;
 	else
 		return false;

Change 'pecify' to 'Specify'.

Signed-off-by Baoquan He <[email protected]>

---
 Documentation/admin-guide/mm/hugetlbpage.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Documentation/admin-guide/mm/hugetlbpage.rst +0 -0

index 015a5f7d7854..f7b1c7462991 100644

Change 'pecify' to 'Specify'.

Signed-off-by Baoquan He <[email protected]>

---
 Documentation/admin-guide/mm/hugetlbpage.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Documentation/admin-guide/mm/hugetlbpage.rst +0 -0

index 015a5f7d7854..f7b1c7462991 100644

A customer complained that no any message is printed out when failed to allocate explicitly specified number of persistent huge pages. That specifying can be done by writing into /proc/sys/vm/nr_hugepages to increase the persisten huge pages.

In the current code, it takes the best effort way to allocate the expected number of huge pages. If only succeeding to get part of them, no any information is printed out.

Here try to send out warning message if the expected number of huge pages adjustment is not achieved, including increasing and decreasing the count of persistent huge pages.

Signed-off-by Baoquan He <[email protected]>

---
 mm/hugetlb.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

mm/hugetlb.c +3 -1

index 467894d8332a..1dfb5d9e4e06 100644
@@ -2661,7 +2661,7 @@ static int adjust_pool_surplus(struct hstate *h, nodemask_t *nodes_allowed,
 static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid,
 			      nodemask_t *nodes_allowed)
 {
-	unsigned long min_count, ret;
+	unsigned long min_count, ret, old_max;
 	NODEMASK_ALLOC(nodemask_t, node_alloc_noretry, GFP_KERNEL);
 
 	/*

@@ -2723,6 +2723,7 @@ static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid,
 	 * pool might be one hugepage larger than it needs to be, but
 	 * within all the constraints specified by the sysctls.
 	 */
+	old_max = persistent_huge_pages(h);
 	while (h->surplus_huge_pages && count > persistent_huge_pages(h)) {
 		if (!adjust_pool_surplus(h, nodes_allowed, -1))
 			break;

@@ -2779,6 +2780,16 @@ static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid,
 	}
 out:
 	h->max_huge_pages = persistent_huge_pages(h);
+	if (count != h->max_huge_pages) {

mm/hugetlb.c +0 -0

 	NODEMASK_FREE(node_alloc_noretry);
 

Quoted 5 lines · Baoquan He

A customer complained that no any message is printed out when failed to allocate explicitly specified number of persistent huge pages. That specifying can be done by writing into /proc/sys/vm/nr_hugepages to increase the persisten huge pages.

Quoted 7 lines

In the current code, it takes the best effort way to allocate the expected number of huge pages. If only succeeding to get part of them, no any information is printed out.

Here try to send out warning message if the expected number of huge pages adjustment is not achieved, including increasing and decreasing the count of persistent huge pages.

Perhaps change the wording a bit,

A customer complained that no message is logged when the number of persistent huge pages is not changed to the exact value written to the sysfs or proc nr_hugepages file.

In the current code, a best effort is made to satisfy requests made via the nr_hugepages file. However, requests may be only partially satisfied.

Log a message if the code was unsuccessful in fully satisfying a request. This includes both increasing and decreasing the number of persistent huge pages.

Quoted 6 lines

Signed-off-by Baoquan He <[email protected]>

---
 mm/hugetlb.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

I am not opposed to this patch. However, I believe the best way for a user to determine if their request was successful is to compare the value of nr_hugepages to the value which was written.

Quoted 33 lines

mm/hugetlb.c +8 -1

index 467894d8332a..1dfb5d9e4e06 100644
@@ -2661,7 +2661,7 @@ static int adjust_pool_surplus(struct hstate *h, nodemask_t *nodes_allowed,
 static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid,
 			      nodemask_t *nodes_allowed)
 {
-	unsigned long min_count, ret;
+	unsigned long min_count, ret, old_max;
 	NODEMASK_ALLOC(nodemask_t, node_alloc_noretry, GFP_KERNEL);
 
 	/*

@@ -2723,6 +2723,7 @@ static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid,
 	 * pool might be one hugepage larger than it needs to be, but
 	 * within all the constraints specified by the sysctls.
 	 */
+	old_max = persistent_huge_pages(h);
 	while (h->surplus_huge_pages && count > persistent_huge_pages(h)) {
 		if (!adjust_pool_surplus(h, nodes_allowed, -1))
 			break;

@@ -2779,6 +2780,16 @@ static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid,
 	}
 out:
 	h->max_huge_pages = persistent_huge_pages(h);
+	if (count != h->max_huge_pages) {
+		char buf[32];
+
+		string_get_size(huge_page_size(h), 1, STRING_UNITS_2, buf, 32);
+		pr_warn("HugeTLB: %s %lu of page size %s failed. Only %s %lu hugepages.\n",
+			count > old_max ? "increasing" : "decreasing",

mm/hugetlb.c static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid, · line 2793

Quoted 5 lines

mm/hugetlb.c +4 -0

+			abs(count - old_max), buf,
+			count > old_max ? "increased" : "decreased",
+			abs(old_max - h->max_huge_pages));
+	}
 	spin_unlock(&hugetlb_lock);

I would prefer if we drop the lock before logging the message. That would involve grabbing the value of h->max_huge_pages before dropping the lock.

Quoted 31 lines

mm/hugetlb.c +8 -1

index 467894d8332a..1dfb5d9e4e06 100644
@@ -2661,7 +2661,7 @@ static int adjust_pool_surplus(struct hstate *h, nodemask_t *nodes_allowed,
 static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid,
 			      nodemask_t *nodes_allowed)
 {
-	unsigned long min_count, ret;
+	unsigned long min_count, ret, old_max;
 	NODEMASK_ALLOC(nodemask_t, node_alloc_noretry, GFP_KERNEL);
 
 	/*

@@ -2723,6 +2723,7 @@ static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid,
 	 * pool might be one hugepage larger than it needs to be, but
 	 * within all the constraints specified by the sysctls.
 	 */
+	old_max = persistent_huge_pages(h);
 	while (h->surplus_huge_pages && count > persistent_huge_pages(h)) {
 		if (!adjust_pool_surplus(h, nodes_allowed, -1))
 			break;

@@ -2779,6 +2780,16 @@ static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid,
 	}
 out:
 	h->max_huge_pages = persistent_huge_pages(h);
+	if (count != h->max_huge_pages) {
+		char buf[32];
+
+		string_get_size(huge_page_size(h), 1, STRING_UNITS_2, buf, 32);
+		pr_warn("HugeTLB: %s %lu of page size %s failed. Only %s %lu hugepages.\n",
+			count > old_max ? "increasing" : "decreasing",

mm/hugetlb.c static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid, · line 2793

Quoted 5 lines

mm/hugetlb.c +4 -0

+			abs(count - old_max), buf,
+			count > old_max ? "increased" : "decreased",
+			abs(old_max - h->max_huge_pages));
+	}
 	spin_unlock(&hugetlb_lock);

I would prefer if we drop the lock before logging the message. That would involve grabbing the value of h->max_huge_pages before dropping the lock.

Quoted 6 lines · Mike Kravetz

Quoted 6 lines

(quoted diff) +6 -0

+	if (count != h->max_huge_pages) {
+		char buf[32];
+
+		string_get_size(huge_page_size(h), 1, STRING_UNITS_2, buf, 32);
+		pr_warn("HugeTLB: %s %lu of page size %s failed. Only %s %lu hugepages.\n",
+			count > old_max ? "increasing" : "decreasing",

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +4 -0

+			abs(count - old_max), buf,
+			count > old_max ? "increased" : "decreased",
+			abs(old_max - h->max_huge_pages));
+	}
 	spin_unlock(&hugetlb_lock);

I would prefer if we drop the lock before logging the message. That would involve grabbing the value of h->max_huge_pages before dropping the lock.

Do you think the below change is OK to you to move the message logging after lock dropping? If yes, I will repost with updated patches.

mm/hugetlb.c +5 -2

index 6a9b7556ce5b..b5aa32a13569 100644
@@ -2661,7 +2661,7 @@ static int adjust_pool_surplus(struct hstate *h, nodemask_t *nodes_allowed,
 static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid,
 			      nodemask_t *nodes_allowed)
 {
-	unsigned long min_count, ret, old_max;
+	unsigned long min_count, ret, old_max, new_max;
 	NODEMASK_ALLOC(nodemask_t, node_alloc_noretry, GFP_KERNEL);
 
 	/*

@@ -2780,7 +2780,10 @@ static int set_max_huge_pages(struct hstate *h, unsigned long count, int nid,
 	}
 out:
 	h->max_huge_pages = persistent_huge_pages(h);
-	if (count != h->max_huge_pages) {
+	new_max = h->max_huge_pages;
+	spin_unlock(&hugetlb_lock);
+
+	if (count != new_max) {
 		char buf[32];
 
 		string_get_size(huge_page_size(h), 1, STRING_UNITS_2, buf, 32);