Re: [PATCH v3 04/21] mm/hugetlb: Introduce nr_free_vmemmap_pages in the struct hstate

[Oscar Salvador <osalvador@suse.de>]

On Sun, Nov 08, 2020 at 10:10:56PM +0800, Muchun Song wrote:
> +#ifdef CONFIG_HUGETLB_PAGE_FREE_VMEMMAP
> +/*
> + * There are 512 struct page structs(8 pages) associated with each 2MB
> + * hugetlb page. For tail pages, the value of compound_dtor is the same.
I gess you meant "For tail pages, the value of compound_head ...", right?

> + * So we can reuse first page of tail page structs. We map the virtual
> + * addresses of the remaining 6 pages of tail page structs to the first
> + * tail page struct, and then free these 6 pages. Therefore, we need to
> + * reserve at least 2 pages as vmemmap areas.
> + */
> +#define RESERVE_VMEMMAP_NR	2U
> +
> +static void __init hugetlb_vmemmap_init(struct hstate *h)
> +{
> +	unsigned int order = huge_page_order(h);
> +	unsigned int vmemmap_pages;
> +
> +	vmemmap_pages = ((1 << order) * sizeof(struct page)) >> PAGE_SHIFT;
> +	/*
> +	 * The head page and the first tail page not free to buddy system,

"The head page and the first tail page are not to be freed to..." better?


> +	 * the others page will map to the first tail page. So there are
> +	 * (@vmemmap_pages - RESERVE_VMEMMAP_NR) pages can be freed.
						      ^^^
                                                      that

> +	else
> +		h->nr_free_vmemmap_pages = 0;

I would specify that this is not expected to happen.
(At least I could not come up with a real scenario unless the system is
corrupted)
So, I would drop a brief comment pointing out that it is only a safety
net.


Unrelated to this patch but related in general, I am not sure about Mike but
would it be cleaner to move all the vmemmap functions to hugetlb_vmemmap.c?
hugetlb code is quite tricky, so I am not sure about stuffing more code
in there.

-- 
Oscar Salvador
SUSE L3