2020-08-31 02:25:37

by Wei Yang

Subject: [Patch v3 7/7] mm/hugetlb: take the free hpage during the iteration directly

Function dequeue_huge_page_node_exact() iterates the free list and
return the first valid free hpage.

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]>
---
mm/hugetlb.c | 20 ++++++++------------
1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 7b3357c1dcec..709be7ab65af 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1040,21 +1040,17 @@ static struct page *dequeue_huge_page_node_exact(struct hstate *h, int nid)
if (nocma && is_migrate_cma_page(page))
continue;

- if (!PageHWPoison(page))
+ if (PageHWPoison(page))
break;
+
+ list_move(&page->lru, &h->hugepage_activelist);
+ set_page_refcounted(page);
+ h->free_huge_pages--;
+ h->free_huge_pages_node[nid]--;
+ return page;
}

- /*
- * 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;
+ return NULL;
}

static struct page *dequeue_huge_page_nodemask(struct hstate *h, gfp_t gfp_mask, int nid,
--
2.20.1 (Apple Git-117)

2020-08-31 23:08:07

by Mike Kravetz

Subject: Re: [Patch v3 7/7] mm/hugetlb: take the free hpage during the iteration directly

On 8/30/20 7:23 PM, Wei Yang wrote:
> Function dequeue_huge_page_node_exact() iterates the free list and
> return the first valid free hpage.
>
> 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]>
> ---
> mm/hugetlb.c | 20 ++++++++------------
> 1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 7b3357c1dcec..709be7ab65af 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1040,21 +1040,17 @@ static struct page *dequeue_huge_page_node_exact(struct hstate *h, int nid)
> if (nocma && is_migrate_cma_page(page))
> continue;
>
> - if (!PageHWPoison(page))
> + if (PageHWPoison(page))
> break;

Previously, when encountering a PageHWPoison(page) the loop would continue
and check the next page in the list. It now breaks the loop and returns
NULL. Is not this a change in behavior? Perhaps you want to change that
break to a continue. Or, restructure in some other way.
--
Mike Kravetz

> +
> + list_move(&page->lru, &h->hugepage_activelist);
> + set_page_refcounted(page);
> + h->free_huge_pages--;
> + h->free_huge_pages_node[nid]--;
> + return page;
> }
>
> - /*
> - * 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;
> + return NULL;
> }
>
> static struct page *dequeue_huge_page_nodemask(struct hstate *h, gfp_t gfp_mask, int nid,
>

2020-09-01 01:44:50

by Wei Yang

Subject: Re: [Patch v3 7/7] mm/hugetlb: take the free hpage during the iteration directly

On Mon, Aug 31, 2020 at 04:06:54PM -0700, Mike Kravetz wrote:
>On 8/30/20 7:23 PM, Wei Yang wrote:
>> Function dequeue_huge_page_node_exact() iterates the free list and
>> return the first valid free hpage.
>>
>> 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]>
>> ---
>> mm/hugetlb.c | 20 ++++++++------------
>> 1 file changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index 7b3357c1dcec..709be7ab65af 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -1040,21 +1040,17 @@ static struct page *dequeue_huge_page_node_exact(struct hstate *h, int nid)
>> if (nocma && is_migrate_cma_page(page))
>> continue;
>>
>> - if (!PageHWPoison(page))
>> + if (PageHWPoison(page))
>> break;
>
>Previously, when encountering a PageHWPoison(page) the loop would continue
>and check the next page in the list. It now breaks the loop and returns
>NULL. Is not this a change in behavior? Perhaps you want to change that
>break to a continue. Or, restructure in some other way.

Shame on me. You are right. I should change it to continue.

Andrew,

Sorry for the inconvenience.

>--
>Mike Kravetz
>
>> +
>> + list_move(&page->lru, &h->hugepage_activelist);
>> + set_page_refcounted(page);
>> + h->free_huge_pages--;
>> + h->free_huge_pages_node[nid]--;
>> + return page;
>> }
>>
>> - /*
>> - * 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;
>> + return NULL;
>> }
>>
>> static struct page *dequeue_huge_page_nodemask(struct hstate *h, gfp_t gfp_mask, int nid,
>>

--
Wei Yang
Help you, Help me