Re: [PATCH v3 5/5] hugetlb: add hugetlb demote page support
From: Oscar Salvador <osalvador@suse.de>
Date: 2021-10-06 08:41:20
Also in:
lkml
On Fri, Oct 01, 2021 at 10:52:10AM -0700, Mike Kravetz wrote:
Demote page functionality will split a huge page into a number of huge pages of a smaller size. For example, on x86 a 1GB huge page can be demoted into 512 2M huge pages. Demotion is done 'in place' by simply splitting the huge page. Added '*_for_demote' wrappers for remove_hugetlb_page, destroy_compound_gigantic_page and prep_compound_gigantic_page for use by demote code. Signed-off-by: Mike Kravetz <redacted> --- mm/hugetlb.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 74 insertions(+), 8 deletions(-)
...
+static int demote_free_huge_page(struct hstate *h, struct page *page)
+{
+ int i, nid = page_to_nid(page);
+ struct hstate *target_hstate;
+ int rc = 0;
+
+ target_hstate = size_to_hstate(PAGE_SIZE << h->demote_order);
+
+ remove_hugetlb_page_for_demote(h, page, false);
+ spin_unlock_irq(&hugetlb_lock);
+
+ rc = alloc_huge_page_vmemmap(h, page);
+ if (rc) {
+ /* Allocation of vmemmmap failed, we can not demote page */
+ spin_lock_irq(&hugetlb_lock);
+ set_page_refcounted(page);
+ add_hugetlb_page(h, page, false);
+ return rc;
+ }Question: You keep the original error code returned from alloc_huge_page_vmemmap() here, but then you lose it on demote_pool_huge_page() when doing the !demote_free_huge_page. Would not make more sense to keep it all the way down to demote_store() in case you want to return the actual error code?
+ + /* + * Use destroy_compound_gigantic_page_for_demote for all huge page + * sizes as it will not ref count pages. + */ + destroy_compound_gigantic_page_for_demote(page, huge_page_order(h));
It seems that for now we only allow gigantic pages to be demoted, but destroy_compound_gigantic_page_for_demote feels kind of wrong, even if it is only a wrapper that ends up calling _*gigantic_ functions. We want a routine that destroy a hugetlb to be demoted into smaller hugetlb pages, so the name gigantic makes little sense to appear in my opinion.
quoted hunk ↗ jump to hunk
static int demote_pool_huge_page(struct hstate *h, nodemask_t *nodes_allowed) __must_hold(&hugetlb_lock) { + int nr_nodes, node; + struct page *page; int rc = 0; lockdep_assert_held(&hugetlb_lock);@@ -3313,9 +3377,15 @@ static int demote_pool_huge_page(struct hstate *h, nodemask_t *nodes_allowed) if (!h->demote_order) return rc; - /* - * TODO - demote fucntionality will be added in subsequent patch - */ + for_each_node_mask_to_free(h, nr_nodes, node, nodes_allowed) { + if (!list_empty(&h->hugepage_freelists[node])) { + page = list_entry(h->hugepage_freelists[node].next, + struct page, lru); + rc = !demote_free_huge_page(h, page);
I kinda dislike this as I pointed out. -- Oscar Salvador SUSE Labs