Received-SPF: pass (google.com: domain of linux-kernel+bounces-52205-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <277e0eed-918f-414f-b19d-219bd155ac14@linux.dev>
Date: Mon, 5 Feb 2024 16:26:23 +0800
Precedence: bulk
MIME-Version: 1.0
Subject: Re: [PATCH v5 7/7] hugetlb: parallelize 1G hugetlb initialization
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: <20240126152411.1238072-1-gang.li@linux.dev>
 <20240126152411.1238072-8-gang.li@linux.dev>
 <f8c89a25-e7f2-4f3c-a99a-a1945e18e026@linux.dev>
From: Gang Li <gang.li@linux.dev>
In-Reply-To: <f8c89a25-e7f2-4f3c-a99a-a1945e18e026@linux.dev>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


On 2024/2/5 15:28, Muchun Song wrote:
> On 2024/1/26 23:24, Gang Li wrote:
>> @@ -3390,8 +3390,6 @@ static void __init 
>> prep_and_add_bootmem_folios(struct hstate *h,
>>       /* Send list for bulk vmemmap optimization processing */
>>       hugetlb_vmemmap_optimize_folios(h, folio_list);
>> -    /* Add all new pool pages to free lists in one lock cycle */
>> -    spin_lock_irqsave(&hugetlb_lock, flags);
>>       list_for_each_entry_safe(folio, tmp_f, folio_list, lru) {
>>           if (!folio_test_hugetlb_vmemmap_optimized(folio)) {
>>               /*
>> @@ -3404,23 +3402,27 @@ static void __init 
>> prep_and_add_bootmem_folios(struct hstate *h,
>>                       HUGETLB_VMEMMAP_RESERVE_PAGES,
>>                       pages_per_huge_page(h));
>>           }
>> +        /* Subdivide locks to achieve better parallel performance */
>> +        spin_lock_irqsave(&hugetlb_lock, flags);
>>           __prep_account_new_huge_page(h, folio_nid(folio));
>>           enqueue_hugetlb_folio(h, folio);
>> +        spin_unlock_irqrestore(&hugetlb_lock, flags);
>>       }
>> -    spin_unlock_irqrestore(&hugetlb_lock, flags);
>>   }
>>   /*
>>    * Put bootmem huge pages into the standard lists after mem_map is up.
>>    * Note: This only applies to gigantic (order > MAX_PAGE_ORDER) pages.
>>    */
>> -static void __init gather_bootmem_prealloc(void)
>> +static void __init gather_bootmem_prealloc_node(unsigned long start, 
>> unsigned long end, void *arg)
>> +
>>   {
>> +    int nid = start;
> 
> Sorry for so late to notice an issue here. I have seen a comment from
> PADATA, whcih says:
> 
>      @max_threads: Max threads to use for the job, actual number may be 
> less
>                    depending on task size and minimum chunk size.
> 
> PADATA will not guarantee gather_bootmem_prealloc_node() will be called
> ->max_threads times (You have initialized it to the number of NUMA nodes in
> gather_bootmem_prealloc). Therefore, we should add a loop here to 
> initialize
> multiple nodes, namely (@end - @start) here. Otherwise, we will miss
> initializing some nodes.
> 
> Thanks.
>
In padata_do_multithreaded:

```
/* Ensure at least one thread when size < min_chunk. */
nworks = max(job->size / max(job->min_chunk, job->align), 1ul);
nworks = min(nworks, job->max_threads);

ps.nworks      = padata_work_alloc_mt(nworks, &ps, &works);
```

So we have works <= max_threads, but >= size/min_chunk.

Thanks!