Received-SPF: pass (google.com: domain of linux-kernel+bounces-32608-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <b71d1750-87b9-4f2d-af4a-3555eeffaafd@linux.dev>
Date: Mon, 22 Jan 2024 17:14:33 +0800
Precedence: bulk
MIME-Version: 1.0
Subject: Re: [PATCH v4 4/7] hugetlb: pass *next_nid_to_alloc directly to
 for_each_node_mask_to_alloc
Content-Language: en-US
To: Muchun Song <muchun.song@linux.dev>, David Hildenbrand
 <david@redhat.com>, David Rientjes <rientjes@google.com>,
 Mike Kravetz <mike.kravetz@oracle.com>,
 Andrew Morton <akpm@linux-foundation.org>,
 Tim Chen <tim.c.chen@linux.intel.com>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
 ligang.bdlg@bytedance.com
References: <20240118123911.88833-1-gang.li@linux.dev>
 <20240118123911.88833-5-gang.li@linux.dev>
 <f7e072c0-4b41-40d3-bf2e-19d4fdf7bc0d@linux.dev>
From: Gang Li <gang.li@linux.dev>
In-Reply-To: <f7e072c0-4b41-40d3-bf2e-19d4fdf7bc0d@linux.dev>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

On 2024/1/22 14:16, Muchun Song wrote:
> On 2024/1/18 20:39, Gang Li wrote:
>>   static struct folio *alloc_pool_huge_folio(struct hstate *h,
>>                       nodemask_t *nodes_allowed,
>> -                    nodemask_t *node_alloc_noretry)
>> +                    nodemask_t *node_alloc_noretry,
>> +                    int *next_node)
>>   {
>>       gfp_t gfp_mask = htlb_alloc_mask(h) | __GFP_THISNODE;
>>       int nr_nodes, node;
>> -    for_each_node_mask_to_alloc(h, nr_nodes, node, nodes_allowed) {
>> +    for_each_node_mask_to_alloc(next_node, nr_nodes, node, 
>> nodes_allowed) {
> 
> 
> A small question here, why not pass h->next_nid_to_alloc to
> for_each_node_mask_to_alloc()? What's the purpose of the third
> parameter of alloc_pool_huge_folio()?
> 
> Thanks.
> 

In hugetlb_alloc_node->alloc_pool_huge_folio, hugetlb is initialized in
parallel at boot time, then it needs each thread to have its own
next_nid, and can't use the global h->next_nid_to_alloc. so an extra 
parameter is added.

And h->next_nid_to_alloc in set_max_huge_pages->alloc_pool_huge_folio
can not be removed. Because if the user calls set_max_huge_pages
frequently and only adds 1 page at a time, that would result in each
request being made on the same node if local variables are used.