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

["Garg, Shivank" <shivankg@amd.com>]

On 1/24/2026 5:10 PM, Lorenzo Stoakes wrote:
> On Sat, Jan 24, 2026 at 04:24:24PM +0530, Dev Jain wrote:
>>
>> 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."
> 
> Yeah I would suggest more precision in your language in future :) 'as a local
> variable' and set . not -> is_khugepaged true... I can see why Shivank
> interpreted it at as a stack variable.
> 
>>
>> madvise_collapse does kmalloc() to allocate this large struct. I was suggesting to do the
>> same for khugepaged, to enforce consistency.
> 
> As I said in reply to Andrew, NAK to the kmalloc idea too.
> 
> This consistency argument is nonsense, madvise_collapse() does that because
> _there can be multiple instances_ of the cc around for different processes, you
> literally _have_ to kmalloc there.
> 
> For khugepaged this isn't the case. We're good as we are.
> 

Thank you Lorenzo for explanation on frame-size/pahole and single-instance vs multi-instance pattern.

This was a valuable discussion and I learned a lot from it :)

Best Regards,
Shivank