Received-SPF: pass (google.com: domain of linux-kernel+bounces-59715-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Fri, 9 Feb 2024 12:17:15 -0500
From: Daniel Jordan <daniel.m.jordan@oracle.com>
To: Jane Chu <jane.chu@oracle.com>
Cc: Muchun Song <muchun.song@linux.dev>, Gang Li <gang.li@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>, Linux-MM <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>, ligang.bdlg@bytedance.com,
        Steffen Klassert <steffen.klassert@secunet.com>
Subject: Re: Re: [PATCH v5 7/7] hugetlb: parallelize 1G hugetlb initialization
Message-ID: <j7xb7m5cy374ngbdm23rvryq6vy6jxtewtu3abjeidhho4bly7@t3aawaeybxlk>
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>
 <6A148F29-68B2-4365-872C-E6AB599C55F6@linux.dev>
 <3a8ae9f0-6250-4a53-bee4-4765024c8992@oracle.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <3a8ae9f0-6250-4a53-bee4-4765024c8992@oracle.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?Ps3+XG4C+DyXCno2dSUbv70jzAidGwd1bu5kaLrGfZqOD+BAnV0/kVdoQ4Vl?=
 =?us-ascii?Q?ely/yLYcTA6GKrmsec1t0AzUX3/3mgblj8RbftSZFr4tCAfiZsorjJ4jzULU?=
 =?us-ascii?Q?Y64BoUjI1LvouwJ1hZECMbD3dCHIPfvPT3i6UqO+GF/h+d07DGsvKYycaq3V?=
 =?us-ascii?Q?GidN8QWp3cXQObRqImkV8cxgtRxvD7rBYcMWE1eEACE8zGCyWbU/MOxyJyB1?=
 =?us-ascii?Q?nanzkIQHmCmSFzvFbUXHup2MqW66/Ia2xitJrq1/f5plsKW6mvgv6SW8wJCk?=
 =?us-ascii?Q?JNrRSdQ8Z1z9o0A6bIgMqUdoeLmEcdpTs17fMUB6UOtUpuFIw0PuhrML+MkN?=
 =?us-ascii?Q?FK89K+kU8TUhzFlvv87EmFexUApRPzKocvd3w2SVPDzA3AH8y6SbFmLVEdBq?=
 =?us-ascii?Q?cJDIW2zvgVxrvCP2r9DyStZEszIBc4giPtHuEjJ/OZo9y/ulN7x9Bq/Q/RfS?=
 =?us-ascii?Q?SZXdwSMwxaiZ/1sTXA4kI5aG/hzLxCodcY89tsu9arIzulSvHFbfqRiWNll2?=
 =?us-ascii?Q?VmM2QmjNA5MwjI1H9VLuNl5PbRo+VPECQqd460EcYCIglLNwSQQlLtdvqH3m?=
 =?us-ascii?Q?Qjt3+qMr95N0AgwZCUFQ+TafkGvTqkV65IuXnwpFerjVABSUpPnBHKdSZZi6?=
 =?us-ascii?Q?7BuY/VN+KawzF+uiEh3myL98tKX3xW/mIeLSnk5PVzGP8aKP2DFWhyNBFy2W?=
 =?us-ascii?Q?NrfYTnAgbLQC6B2u8aoLWXMAf9lr5x5BZXwI16g3MkpVrM/SjGCdcRSpgKPN?=
 =?us-ascii?Q?iNNfz6R+SQ3ChXgSUhJlSpOHA9bg/yjqCmP+5i1RnbXYNCYEensMgi90vPN/?=
 =?us-ascii?Q?fxx0ufLfD6UiDoLDdE/XoUcxziOXvwJUQzVqMJz7SNxxwWqMUGD8RRWU6vGP?=
 =?us-ascii?Q?atALHyTnMWUPJCj8oMg+oEu7S3L856900cT0CK1kPzIqxeyVY4jdVjXhvztx?=
 =?us-ascii?Q?T8vSDpZzgHiiEpPMDHQZNpwiDf7wjcUhNTNUJSxpwhdGAz6oe2O9QSXwafIQ?=
 =?us-ascii?Q?Nbzdkl70C9vSqtkehiYE1ywoBGKinp05PAIcBK0rtjWOgqXW9b/7eKOuAPyI?=
 =?us-ascii?Q?sn9FTtmPq+oRgMWAek0Putg/3Q67AaZSvI3ycWgDwpiRTagULG2hhSqfY6tp?=
 =?us-ascii?Q?YWUA/MFZtO0anSFRUafGAwwpxgXQG9PrN+7aMCkG3bkiKyNdj0X2E5Kl41r/?=
 =?us-ascii?Q?YX2upPt1c+BRbH6kfNwfL2YrR1F0+hdGRWEXwM6wJjcfIyN5d+czVupAfnQI?=
 =?us-ascii?Q?Wx5a2USAnaePlC1S/hxBYBuLGCoalPD6hLuCdiApHOb4hSQc5zo/ZZ5Xh+Bc?=
 =?us-ascii?Q?cNPobvs+GufNOkNifM9e3grYvJkdElnnOdK4rYGDW29G8MgwKqfaoYfn+FQ+?=
 =?us-ascii?Q?cgxsJ2zHFyM7ZUn/a/BqKP9nl6RzJpJtUtD/cIo/WDIcGUmbepiZpZdRXZlm?=
 =?us-ascii?Q?5DZDErkJKkBF3oBwXHuNpAKtnZS1vRgk5C28mPNYDPACOPFcCauInOSPsXP4?=
 =?us-ascii?Q?LNwSJtm6ollVcjz3OYxbByDt1T7bhAQF4tU12Vhqm9M2wybW5lhH21p5WaIO?=
 =?us-ascii?Q?Kl4AwMxPuUoKbmJBmKCyvEvNqYi4yUrPrjmPH9q0KSXieq+morbrLIDd6jce?=
 =?us-ascii?Q?Gw=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	4+DoAk1iR8FFLV2jRbuB3Icty+Y1FC8SJIeC21HaN6t8at0F3uRSKiquaBZK/DJA+5lgHQUvzE+lnO2U+Bzu9rr5MxBvDvG/Rxy2uIBpS6CZtJg/X1oFrW6eEcUvD49uMRihANI1s978eOijhj7XjcJ6YX8MmCSAX6zpO+YN9Dqc6tXAb/+veCjM3uUE9R+vYqTJl+UxemAd/AHNtbKsm8p97MWSZhW62lvA9+tkOcdVRPtE21/pyERKEvj0kHkHjvh+P2HMxc3j6Vg6pAbxl8DGuFR3PgWvM5hkhB1Uxf3w9DWyxo1en/oQ3HP+7+rLV2UZjE5ijd6sKl8UN+2whHLHQOTmQW0opUugKvUEntUBggZ2ezUxYuE3selkiq1Flex3KEs+uIUOzFPdUs3CAgMIyV/Vd6K7pIbrpus4FRTKb4rmSzYvca3lJHtntyr/NR0uZ3biUjtm7sYlTuolkXNbMSb2Jhd51vCXO8pWzhrHqlkf62UAh5ykxpWcE6GL26oMuXURMqR0OeXGmVDaR5Ryw6fs0/51HE8ogw4m/UIV81Z7yCtP5ADNHQD899qMD4MzhfyRjaX2TsAjhkP7tsF33HY2BdGBl6PL6UN7GBA=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 895573e6-41cc-45ac-cf26-08dc2992f8d0
X-MS-Exchange-CrossTenant-AuthSource: SA1PR10MB5711.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Feb 2024 17:17:19.8753
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: W5IdPoPrHpfhQrLAYQV/HNdmdpmblL2s+wk43XJpkMvqudxBZZpce3LfmFeRaW4e1/bQY9VaE6oZiMyMi7NIQZAhyQoAqECYF/NDwvLppxA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR10MB6320
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-02-09_14,2024-02-08_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 bulkscore=0
 mlxlogscore=999 phishscore=0 spamscore=0 mlxscore=0 adultscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2402090124
X-Proofpoint-GUID: VF4kpZCHgumZSQMiVzdDo68nxR0Cqjp5
X-Proofpoint-ORIG-GUID: VF4kpZCHgumZSQMiVzdDo68nxR0Cqjp5

On Tue, Feb 06, 2024 at 05:53:04PM -0800, Jane Chu wrote:
> Add Daniel Jordan.

Thanks, Jane.

I'm adding Steffen too, and please cc padata maintainers on future
patches.  MAINTAINERS has linux-crypto too under padata, but for changes
to just padata_do_multithreaded that's probably not necessary.

> On 2/5/2024 1:09 AM, Muchun Song wrote:
> > > On Feb 5, 2024, at 16:26, Gang Li <gang.li@linux.dev> wrote:
> > > On 2024/2/5 15:28, Muchun Song wrote:
> > > > On 2024/1/26 23:24, Gang Li wrote:
> > > > > -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.
> > 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

Right.  The core issue is that gather_bootmem_prealloc_node() doesn't
look at @end, but padata expects that each call of the thread function
covers the start/end range that's passed.  I understand that this
happens to work today with how padata calculates nworks, but it seems
better to honor the expectation, so I agree with Muchun's suggestion a
few messages ago to loop over the range.


I hope to look at the rest of the series and that standalone Kconfig
patch after about a week, there isn't time before that.