linux-kernel - Re: [PATCH] mm, hwpoison, hugetlb: Free hwpoison huge page to list tail and dissolve hwpoison huge page first

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 2 Aug 2022 09:58:55 -0700
From:   Mike Kravetz <mike.kravetz@...cle.com>
To:     luofei <luofei@...cloud.com>
Cc:     Naoya Horiguchi <naoya.horiguchi@...ux.dev>,
        Miaohe Lin <linmiaohe@...wei.com>, songmuchun@...edance.com,
        akpm@...ux-foundation.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm, hwpoison, hugetlb: Free hwpoison huge page to list
 tail and dissolve hwpoison huge page first

On 08/02/22 06:07, luofei wrote:
> If free hwpoison huge page to the end of hugepage_freelists, the
> loop can exit directly when the hwpoison huge page is traversed,
> which can effectively reduce the repeated traversal of the hwpoison
> huge page. Meanwhile, when free the free huge pages to lower level
> allocators, if hwpoison ones are released first, this can improve
> the effecvive utilization rate of huge page.

In general, I think this is a good idea.  Although, it seems that with
recent changes to hugetlb poisioning code we are even less likely to
have a poisioned page on hugetlb free lists.

Adding Naoya and Miaohe as they have been looking at page poison of hugetlb
pages recently.

> Signed-off-by: luofei <luofei@...cloud.com>
> ---
>  mm/hugetlb.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 28516881a1b2..ca72220eedd9 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1116,7 +1116,10 @@ static void enqueue_huge_page(struct hstate *h, struct page *page)
>  	lockdep_assert_held(&hugetlb_lock);
>  	VM_BUG_ON_PAGE(page_count(page), page);
>  
> -	list_move(&page->lru, &h->hugepage_freelists[nid]);
> +	if (unlikely(PageHWPoison(page)))
> +		list_move_tail(&page->lru, &h->hugepage_freelists[nid]);
> +	else
> +		list_move(&page->lru, &h->hugepage_freelists[nid]);
>  	h->free_huge_pages++;
>  	h->free_huge_pages_node[nid]++;
>  	SetHPageFreed(page);
> @@ -1133,7 +1136,7 @@ static struct page *dequeue_huge_page_node_exact(struct hstate *h, int nid)
>  			continue;
>  
>  		if (PageHWPoison(page))
> -			continue;
> +			break;

IIRC, it is 'possible' to unpoison a page via the debug/testing interfaces.
If so, then we could end up with free unpoisioned page(s) at the end of
the list that would never be used because we quit when encountering a
poisioned page.

Naoya and Miaohe would know for sure.

Same possible issue in demote_pool_huge_page().
-- 
Mike Kravetz

>  
>  		list_move(&page->lru, &h->hugepage_activelist);
>  		set_page_refcounted(page);
> @@ -2045,7 +2048,7 @@ static struct page *remove_pool_huge_page(struct hstate *h,
>  		 */
>  		if ((!acct_surplus || h->surplus_huge_pages_node[node]) &&
>  		    !list_empty(&h->hugepage_freelists[node])) {
> -			page = list_entry(h->hugepage_freelists[node].next,
> +			page = list_entry(h->hugepage_freelists[node].prev,
>  					  struct page, lru);
>  			remove_hugetlb_page(h, page, acct_surplus);
>  			break;
> @@ -3210,7 +3213,7 @@ static void try_to_free_low(struct hstate *h, unsigned long count,
>  	for_each_node_mask(i, *nodes_allowed) {
>  		struct page *page, *next;
>  		struct list_head *freel = &h->hugepage_freelists[i];
> -		list_for_each_entry_safe(page, next, freel, lru) {
> +		list_for_each_entry_safe_reverse(page, next, freel, lru) {
>  			if (count >= h->nr_huge_pages)
>  				goto out;
>  			if (PageHighMem(page))
> @@ -3494,7 +3497,7 @@ static int demote_pool_huge_page(struct hstate *h, nodemask_t *nodes_allowed)
>  	for_each_node_mask_to_free(h, nr_nodes, node, nodes_allowed) {
>  		list_for_each_entry(page, &h->hugepage_freelists[node], lru) {
>  			if (PageHWPoison(page))
> -				continue;
> +				break;
>  
>  			return demote_free_huge_page(h, page);
>  		}
> -- 
> 2.27.0
>