Re: [PATCH V3 5/5] mm/khugepaged: make khugepaged_collapse_control static

[Dev Jain <dev.jain@arm.com>]

On 24/01/26 2:31 pm, Lorenzo Stoakes wrote:
> NAK to this change....
>
> On Fri, Jan 23, 2026 at 03:03:58PM +0530, Garg, Shivank wrote:
>>
>> On 1/23/2026 1:18 PM, Dev Jain wrote:
>>> On 22/01/26 2:58 pm, Dev Jain wrote:
>>>> On 19/01/26 12:53 am, Shivank Garg wrote:
>>>>> The global variable 'khugepaged_collapse_control' is not used outside of
>>>>> mm/khugepaged.c. Make it static to limit its scope.
>>>>>
>>>>> Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
>>>>> Reviewed-by: Zi Yan <ziy@nvidia.com>
>>>>> Acked-by: David Hildenbrand (Red Hat) <david@kernel.org>
>>>>> Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>>>> Signed-off-by: Shivank Garg <shivankg@amd.com>
>>>>> ---
>>>>>  mm/khugepaged.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>>>>> index 1667abae6d8d..fba6aea5bea6 100644
>>>>> --- a/mm/khugepaged.c
>>>>> +++ b/mm/khugepaged.c
>>>>> @@ -827,7 +827,7 @@ static void khugepaged_alloc_sleep(void)
>>>>>  	remove_wait_queue(&khugepaged_wait, &wait);
>>>>>  }
>>>>>
>>>>> -struct collapse_control khugepaged_collapse_control = {
>>>>> +static struct collapse_control khugepaged_collapse_control = {
>>>>>  	.is_khugepaged = true,
>>>>>  };
>>>>>
>>>> Will it not be better to just remove this variable? In madvise_collapse,
>>>> we defined cc as a local variable and set .is_khugepaged = false. The
>>>> same can be done in int khugepaged() - define a local variable and set
>>>> .is_khugepaged = true.
>>> Since this patch has been stabilized already by 4 R-bs, it may be a headache
>>> to now remove this, we can do my suggestion later.
>>>
>>> Reviewed-by: Dev Jain <dev.jain@arm.com>
>>>
>>>>
>> Thank you Dev for the feedback and review.
>>
>> I've attached the patch implementing your suggestion and sending this as a separate
>> follow-up to avoid disrupting the current series.
>>
>> I’m happy to queue it for next cycle or if it’s acceptable now, please take it.
>>
>> Thanks for the suggestion!
>>
>> Regards,
>> Shivank
>>
>> ---
>> From: Shivank Garg <shivankg@amd.com>
>> Date: Thu, 22 Jan 2026 12:36:28 +0000
>> Subject: [PATCH] mm/khugepaged: convert khugepaged_collapse_control to local
>>  variable in khugepaged()
>>
>> Make khugepaged_collapse_control a local variable in khugepaged() instead
>> of static global, consistent with how madvise_collapse() handles its
>> collapse_control. Static storage is unnecessary here as node_load and
>> alloc_nmask are reset per-VMA during scanning.
>>
>> No functional change.
>>
>> Suggested-by: Dev Jain <dev.jain@arm.com>
>> Signed-off-by: Shivank Garg <shivankg@amd.com>
>> ---
>>  mm/khugepaged.c | 9 ++++-----
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index 9f790ec34400..c18d2ce639b1 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -829,10 +829,6 @@ static void khugepaged_alloc_sleep(void)
>>  	remove_wait_queue(&khugepaged_wait, &wait);
>>  }
>>
>> -static struct collapse_control khugepaged_collapse_control = {
>> -	.is_khugepaged = true,
>> -};
>> -
>>  static bool hpage_collapse_scan_abort(int nid, struct collapse_control *cc)
>>  {
>>  	int i;
>> @@ -2629,13 +2625,16 @@ static void khugepaged_wait_work(void)
>>
>>  static int khugepaged(void *none)
>>  {
>> +	struct collapse_control cc = {
>> +		.is_khugepaged = true,
>> +	};
>>  	struct mm_slot *slot;
>>
>>  	set_freezable();
>>  	set_user_nice(current, MAX_NICE);
>>
>>  	while (!kthread_should_stop()) {
>> -		khugepaged_do_scan(&khugepaged_collapse_control);
>> +		khugepaged_do_scan(&cc);
>>  		khugepaged_wait_work();
>>  	}
>>
>> --
>> 2.43.0
>>
>>
>>
>>
> Andrew's already commented but this is terribly mistaken.
>
> The argument against it (why did nobody check...) is that this struct is HUGE
> and there's really no benefit to doing this.
>
> Nico's series makes this struct even bigger (...!)
>
> Dev - PLEASE use pahole or sizeof(...) or something before suggesting moving
> things like this on to the stack, in future e.g.:
>
> $ pahole collapse_control
> struct collapse_control {
> 	bool                       is_khugepaged;        /*     0     1 */
>
> 	/* XXX 3 bytes hole, try to pack */
>
> 	u32                        node_load[1024];      /*     4  4096 */
>
> 	/* XXX 4 bytes hole, try to pack */
>
> 	/* --- cacheline 64 boundary (4096 bytes) was 8 bytes ago --- */
> 	nodemask_t                 alloc_nmask;          /*  4104   128 */
>
> 	/* size: 4232, cachelines: 67, members: 3 */
> 	/* sum members: 4225, holes: 2, sum holes: 7 */
> 	/* last cacheline: 8 bytes */
> };
>
> Making this static was fine. Leave it as-is.

I wasn't suggesting that! When I said

"In madvise_collapse, we defined cc as a local variable and set .is_khugepaged = false. The
same can be done in int khugepaged() - define a local variable and set .is_khugepaged = true."

madvise_collapse does kmalloc() to allocate this large struct. I was suggesting to do the
same for khugepaged, to enforce consistency.

>
> Thanks, Lorenzo