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

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

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.

Thanks, Lorenzo