Re: [PATCH v2 6/9] x86/clear_huge_page: multi-page clearing

Peter Zijlstra
2023-09-08T12:38:49Z

On Wed, Aug 30, 2023 at 11:49:55AM -0700, Ankur Arora wrote:

> +#ifndef CONFIG_HIGHMEM

I'm thinking this wants to be: #ifdef CONFIG_X86_64. All the previous
stuff was 64bit specific.

> +static void clear_contig_region(struct page *page, unsigned int npages)
> +{
> + clear_pages(page_address(page), npages);
> +}

I'm not sure about the naming of this helper -- but whatever.

> +
> +/*
> + * clear_huge_page(): multi-page clearing variant of clear_huge_page().
> + *
> + * Taking inspiration from the common code variant, we split the zeroing in
> + * three parts: left of the fault, right of the fault, and up to 5 pages
> + * in the immediate neighbourhood of the target page.
> + *
> + * Cleared in that order to keep cache lines of the target region hot.
> + *
> + * For gigantic pages, there is no expectation of cache locality so we do a
> + * straight zeroing.
> + */
> +void clear_huge_page(struct page *page,
> + unsigned long addr_hint, unsigned int pages_per_huge_page)
> +{
> + unsigned long addr = addr_hint &
> + ~(((unsigned long)pages_per_huge_page << PAGE_SHIFT) - 1);
> + const long pgidx = (addr_hint - addr) / PAGE_SIZE;
> + const int first_pg = 0, last_pg = pages_per_huge_page - 1;
> + const int width = 2; /* pages cleared last on either side */
> + int sidx[3], eidx[3];
> + int i, n;
> +
> + if (pages_per_huge_page > MAX_ORDER_NR_PAGES)
> + return clear_contig_region(page, pages_per_huge_page);
> +
> + /*
> + * Neighbourhood of the fault. Cleared at the end to ensure
> + * it sticks around in the cache.
> + */
> + n = 2;
> + sidx[n] = (pgidx - width) < first_pg ? first_pg : (pgidx - width);
> + eidx[n] = (pgidx + width) > last_pg ? last_pg : (pgidx + width);
> +
> + sidx[0] = first_pg; /* Region to the left of the fault */
> + eidx[0] = sidx[n] - 1;
> +
> + sidx[1] = eidx[n] + 1; /* Region to the right of the fault */
> + eidx[1] = last_pg;
> +
> + for (i = 0; i <= 2; i++) {
> + if (eidx[i] >= sidx[i])
> + clear_contig_region(page + sidx[i],
> + eidx[i] - sidx[i] + 1);

Since the if has a multi-line statement it needs { } per coding style.

> + }
> +}
> +#endif /* CONFIG_HIGHMEM */
> #endif /* CONFIG_HUGETLB_PAGE */
>
> #ifdef CONFIG_X86_64
> --
> 2.31.1
>