Received-SPF: pass (google.com: domain of linux-kernel+bounces-52266-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Content-Type: text/plain;
	charset=us-ascii
Precedence: bulk
Mime-Version: 1.0
Subject: Re: [PATCH v5 7/7] hugetlb: parallelize 1G hugetlb initialization
From: Muchun Song <muchun.song@linux.dev>
In-Reply-To: <277e0eed-918f-414f-b19d-219bd155ac14@linux.dev>
Date: Mon, 5 Feb 2024 17:09:15 +0800
Cc: 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>,
 Linux-MM <linux-mm@kvack.org>,
 LKML <linux-kernel@vger.kernel.org>,
 ligang.bdlg@bytedance.com
Content-Transfer-Encoding: quoted-printable
Message-Id: <6A148F29-68B2-4365-872C-E6AB599C55F6@linux.dev>
References: <20240126152411.1238072-1-gang.li@linux.dev>
 <20240126152411.1238072-8-gang.li@linux.dev>
 <f8c89a25-e7f2-4f3c-a99a-a1945e18e026@linux.dev>
 <277e0eed-918f-414f-b19d-219bd155ac14@linux.dev>
To: Gang Li <gang.li@linux.dev>


> On Feb 5, 2024, at 16:26, Gang Li <gang.li@linux.dev> wrote:
>=20
>=20
>=20
> 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 =3D 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.
>>=20
> In padata_do_multithreaded:
>=20
> ```
> /* Ensure at least one thread when size < min_chunk. */
> nworks =3D max(job->size / max(job->min_chunk, job->align), 1ul);
> nworks =3D min(nworks, job->max_threads);
>=20
> ps.nworks      =3D padata_work_alloc_mt(nworks, &ps, &works);
> ```
>=20
> So we have works <=3D max_threads, but >=3D size/min_chunk.

Given a 4-node system, the current implementation will schedule
4 threads to call gather_bootmem_prealloc() respectively, and
there is no problems here. But what if PADATA schedules 2
threads and each thread aims to handle 2 nodes? I think
it is possible for PADATA in the future, because it does not
break any semantics exposed to users. The comment about @min_chunk:

	The minimum chunk size in job-specific units. This
	allows the client to communicate the minimum amount
	of work that's appropriate for one worker thread to
	do at once.

It only defines the minimum chunk size but not maximum size,
so it is possible to let each ->thread_fn handle multiple
minimum chunk size. Right? Therefore, I am not concerned
about the current implementation of PADATA but that of future.

Maybe a separate patch is acceptable since it is an improving
patch instead of a fix one (at least there is no bug currently).

Thanks.