Following are some cleanup for hugetlb.
Simple test with tools/testing/selftests/vm/map_hugetlb pass.
v2:
* drop 5/6/10 since similar patches are merged or under review.
* adjust 2 based on comment from Mike Kravetz
Wei Yang (7):
mm/hugetlb: not necessary to coalesce regions recursively
mm/hugetlb: remove VM_BUG_ON(!nrg) in
get_file_region_entry_from_cache()
mm/hugetlb: use list_splice to merge two list at once
mm/hugetlb: count file_region to be added when regions_needed != NULL
mm/hugetlb: a page from buddy is not on any list
mm/hugetlb: return non-isolated page in the loop instead of break and
check
mm/hugetlb: narrow the hugetlb_lock protection area during preparing
huge page
mm/hugetlb.c | 77 +++++++++++++++++++++++-----------------------------
1 file changed, 34 insertions(+), 43 deletions(-)
--
2.20.1 (Apple Git-117)
Per my understanding, we keep the regions ordered and would always
coalesce regions properly. So the task to keep this property is just
to coalesce its neighbour.
Let's simplify this.
Signed-off-by: Wei Yang <[email protected]>
Reviewed-by: Baoquan He <[email protected]>
Reviewed-by: Mike Kravetz <[email protected]>
---
mm/hugetlb.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 590111ea6975..62ec74f6d03f 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -307,8 +307,7 @@ static void coalesce_file_region(struct resv_map *resv, struct file_region *rg)
list_del(&rg->link);
kfree(rg);
- coalesce_file_region(resv, prg);
- return;
+ rg = prg;
}
nrg = list_next_entry(rg, link);
@@ -318,9 +317,6 @@ static void coalesce_file_region(struct resv_map *resv, struct file_region *rg)
list_del(&rg->link);
kfree(rg);
-
- coalesce_file_region(resv, nrg);
- return;
}
}
--
2.20.1 (Apple Git-117)
The page allocated from buddy is not on any list, so just use list_add()
is enough.
Signed-off-by: Wei Yang <[email protected]>
Reviewed-by: Baoquan He <[email protected]>
Reviewed-by: Mike Kravetz <[email protected]>
---
mm/hugetlb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index bbccbfeb8601..5a71cb7acf6b 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -2428,7 +2428,7 @@ struct page *alloc_huge_page(struct vm_area_struct *vma,
h->resv_huge_pages--;
}
spin_lock(&hugetlb_lock);
- list_move(&page->lru, &h->hugepage_activelist);
+ list_add(&page->lru, &h->hugepage_activelist);
/* Fall through */
}
hugetlb_cgroup_commit_charge(idx, pages_per_huge_page(h), h_cg, page);
--
2.20.1 (Apple Git-117)
set_hugetlb_cgroup_[rsvd] just manipulate page local data, which is not
necessary to be protected by hugetlb_lock.
Let's take this out.
Signed-off-by: Wei Yang <[email protected]>
Reviewed-by: Baoquan He <[email protected]>
Reviewed-by: Mike Kravetz <[email protected]>
---
mm/hugetlb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 6ad365dd1e96..ae840dc09197 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1492,9 +1492,9 @@ static void prep_new_huge_page(struct hstate *h, struct page *page, int nid)
{
INIT_LIST_HEAD(&page->lru);
set_compound_page_dtor(page, HUGETLB_PAGE_DTOR);
- spin_lock(&hugetlb_lock);
set_hugetlb_cgroup(page, NULL);
set_hugetlb_cgroup_rsvd(page, NULL);
+ spin_lock(&hugetlb_lock);
h->nr_huge_pages++;
h->nr_huge_pages_node[nid]++;
spin_unlock(&hugetlb_lock);
--
2.20.1 (Apple Git-117)
Function dequeue_huge_page_node_exact() iterates the free list and
return the first non-isolated one.
Instead of break and check the loop variant, we could return in the loop
directly. This could reduce some redundant check.
Signed-off-by: Wei Yang <[email protected]>
Reviewed-by: Mike Kravetz <[email protected]>
---
mm/hugetlb.c | 26 ++++++++++++--------------
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 5a71cb7acf6b..6ad365dd1e96 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1033,20 +1033,18 @@ static struct page *dequeue_huge_page_node_exact(struct hstate *h, int nid)
{
struct page *page;
- list_for_each_entry(page, &h->hugepage_freelists[nid], lru)
- if (!PageHWPoison(page))
- break;
- /*
- * if 'non-isolated free hugepage' not found on the list,
- * the allocation fails.
- */
- if (&h->hugepage_freelists[nid] == &page->lru)
- return NULL;
- list_move(&page->lru, &h->hugepage_activelist);
- set_page_refcounted(page);
- h->free_huge_pages--;
- h->free_huge_pages_node[nid]--;
- return page;
+ list_for_each_entry(page, &h->hugepage_freelists[nid], lru) {
+ if (PageHWPoison(page))
+ continue;
+
+ list_move(&page->lru, &h->hugepage_activelist);
+ set_page_refcounted(page);
+ h->free_huge_pages--;
+ h->free_huge_pages_node[nid]--;
+ return page;
+ }
+
+ return NULL;
}
static struct page *dequeue_huge_page_nodemask(struct hstate *h, gfp_t gfp_mask, int nid,
--
2.20.1 (Apple Git-117)
We are sure to get a valid file_region, otherwise the
VM_BUG_ON(resv->region_cache_count <= 0) at the very beginning would be
triggered.
Let's remove the redundant one.
Signed-off-by: Wei Yang <[email protected]>
---
mm/hugetlb.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 62ec74f6d03f..f325839be617 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -238,7 +238,6 @@ get_file_region_entry_from_cache(struct resv_map *resv, long from, long to)
resv->region_cache_count--;
nrg = list_first_entry(&resv->region_cache, struct file_region, link);
- VM_BUG_ON(!nrg);
list_del(&nrg->link);
nrg->from = from;
--
2.20.1 (Apple Git-117)
Instead of add allocated file_region one by one to region_cache, we
could use list_splice to merge two list at once.
Also we know the number of entries in the list, increase the number
directly.
Signed-off-by: Wei Yang <[email protected]>
Reviewed-by: Baoquan He <[email protected]>
Reviewed-by: Mike Kravetz <[email protected]>
---
mm/hugetlb.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index f325839be617..cbe67428bf99 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -441,11 +441,8 @@ static int allocate_file_region_entries(struct resv_map *resv,
spin_lock(&resv->lock);
- list_for_each_entry_safe(rg, trg, &allocated_regions, link) {
- list_del(&rg->link);
- list_add(&rg->link, &resv->region_cache);
- resv->region_cache_count++;
- }
+ list_splice(&allocated_regions, &resv->region_cache);
+ resv->region_cache_count += to_allocate;
}
return 0;
--
2.20.1 (Apple Git-117)
There are only two cases of function add_reservation_in_range()
* count file_region and return the number in regions_needed
* do the real list operation without counting
This means it is not necessary to have two parameters to classify these
two cases.
Just use regions_needed to separate them.
Signed-off-by: Wei Yang <[email protected]>
Reviewed-by: Baoquan He <[email protected]>
Reviewed-by: Mike Kravetz <[email protected]>
---
mm/hugetlb.c | 33 +++++++++++++++++----------------
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index cbe67428bf99..bbccbfeb8601 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -319,16 +319,17 @@ static void coalesce_file_region(struct resv_map *resv, struct file_region *rg)
}
}
-/* Must be called with resv->lock held. Calling this with count_only == true
- * will count the number of pages to be added but will not modify the linked
- * list. If regions_needed != NULL and count_only == true, then regions_needed
- * will indicate the number of file_regions needed in the cache to carry out to
- * add the regions for this range.
+/*
+ * Must be called with resv->lock held.
+ *
+ * Calling this with regions_needed != NULL will count the number of pages
+ * to be added but will not modify the linked list. And regions_needed will
+ * indicate the number of file_regions needed in the cache to carry out to add
+ * the regions for this range.
*/
static long add_reservation_in_range(struct resv_map *resv, long f, long t,
struct hugetlb_cgroup *h_cg,
- struct hstate *h, long *regions_needed,
- bool count_only)
+ struct hstate *h, long *regions_needed)
{
long add = 0;
struct list_head *head = &resv->regions;
@@ -364,14 +365,14 @@ static long add_reservation_in_range(struct resv_map *resv, long f, long t,
*/
if (rg->from > last_accounted_offset) {
add += rg->from - last_accounted_offset;
- if (!count_only) {
+ if (!regions_needed) {
nrg = get_file_region_entry_from_cache(
resv, last_accounted_offset, rg->from);
record_hugetlb_cgroup_uncharge_info(h_cg, h,
resv, nrg);
list_add(&nrg->link, rg->link.prev);
coalesce_file_region(resv, nrg);
- } else if (regions_needed)
+ } else
*regions_needed += 1;
}
@@ -383,13 +384,13 @@ static long add_reservation_in_range(struct resv_map *resv, long f, long t,
*/
if (last_accounted_offset < t) {
add += t - last_accounted_offset;
- if (!count_only) {
+ if (!regions_needed) {
nrg = get_file_region_entry_from_cache(
resv, last_accounted_offset, t);
record_hugetlb_cgroup_uncharge_info(h_cg, h, resv, nrg);
list_add(&nrg->link, rg->link.prev);
coalesce_file_region(resv, nrg);
- } else if (regions_needed)
+ } else
*regions_needed += 1;
}
@@ -482,8 +483,8 @@ static long region_add(struct resv_map *resv, long f, long t,
retry:
/* Count how many regions are actually needed to execute this add. */
- add_reservation_in_range(resv, f, t, NULL, NULL, &actual_regions_needed,
- true);
+ add_reservation_in_range(resv, f, t, NULL, NULL,
+ &actual_regions_needed);
/*
* Check for sufficient descriptors in the cache to accommodate
@@ -511,7 +512,7 @@ static long region_add(struct resv_map *resv, long f, long t,
goto retry;
}
- add = add_reservation_in_range(resv, f, t, h_cg, h, NULL, false);
+ add = add_reservation_in_range(resv, f, t, h_cg, h, NULL);
resv->adds_in_progress -= in_regions_needed;
@@ -547,9 +548,9 @@ static long region_chg(struct resv_map *resv, long f, long t,
spin_lock(&resv->lock);
- /* Count how many hugepages in this range are NOT respresented. */
+ /* Count how many hugepages in this range are NOT represented. */
chg = add_reservation_in_range(resv, f, t, NULL, NULL,
- out_regions_needed, true);
+ out_regions_needed);
if (*out_regions_needed == 0)
*out_regions_needed = 1;
--
2.20.1 (Apple Git-117)
On 8/27/20 8:32 PM, Wei Yang wrote:
> We are sure to get a valid file_region, otherwise the
> VM_BUG_ON(resv->region_cache_count <= 0) at the very beginning would be
> triggered.
>
> Let's remove the redundant one.
>
> Signed-off-by: Wei Yang <[email protected]>
Thank you.
Reviewed-by: Mike Kravetz <[email protected]>
--
Mike Kravetz
On 8/27/20 8:32 PM, Wei Yang wrote:
> Function dequeue_huge_page_node_exact() iterates the free list and
> return the first non-isolated one.
>
> Instead of break and check the loop variant, we could return in the loop
> directly. This could reduce some redundant check.
>
> Signed-off-by: Wei Yang <[email protected]>
> Reviewed-by: Mike Kravetz <[email protected]>
Commit bbe88753bd42 (mm/hugetlb: make hugetlb migration callback CMA aware)
in v5.9-rc2 modified dequeue_huge_page_node_exact. This patch will need
to be updated to take those changes into account.
--
Mike Kravetz