From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id E80CC27A122 for ; Sat, 24 Jan 2026 11:56:19 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1769255783; cv=none; b=GLKYjLM7NuS3AAKxYiUOqsjMIgm4gdfYj+GFHOFTPhpPEBr7baEWK3tf+kU02mXcTLe8OTdmJHoh1FyGdNgCFbEix+wvdfvA3kia0LU6VxwmTb7TzORas+D2d/UFN619KqW8/E2wF0WcFPdykNEHpbycbUXmFoJmsRp7VV50c3k= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1769255783; c=relaxed/simple; bh=n2TaLyLFmluai4lKbeULHzNXPR8oJE+axOIyfqpXMeQ=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=jT2sRgakeWMuASAbEVDoEUek5roPfuD6KoRGhPgljvctNHqcHfe9taYCftFVpMON88JrVt1WuCKDsAQIAOtW/vUSHq62lnJd8tZ9RLrptwuDDwAUW+dK/LEwpU8JiHR2nPB8pO8pH6MHkqKgKXh20C4ufYX6p2XHAImpHNbRb8g= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 8117C150C; Sat, 24 Jan 2026 03:56:12 -0800 (PST) Received: from [10.163.130.81] (unknown [10.163.130.81]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 5512F3F632; Sat, 24 Jan 2026 03:56:14 -0800 (PST) Message-ID: Date: Sat, 24 Jan 2026 17:26:11 +0530 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH V3 5/5] mm/khugepaged: make khugepaged_collapse_control static To: Lorenzo Stoakes Cc: "Garg, Shivank" , Andrew Morton , David Hildenbrand , Zi Yan , Baolin Wang , "Liam R . Howlett" , Nico Pache , Ryan Roberts , Barry Song , Lance Yang , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Wei Yang , Anshuman Khandual References: <20260118192253.9263-4-shivankg@amd.com> <20260118192253.9263-14-shivankg@amd.com> <6486c6dd-2702-4a4d-9662-09639532ce6f@arm.com> <50da84da-1cd6-4b8b-babd-b6dea405713b@lucifer.local> <94c84a3c-8ed9-4bb5-8e64-69bcb8306aba@arm.com> <7ac06a41-73c8-4089-873e-7bb4cc1b3e02@lucifer.local> Content-Language: en-US From: Dev Jain In-Reply-To: <7ac06a41-73c8-4089-873e-7bb4cc1b3e02@lucifer.local> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On 24/01/26 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 >>>>>>> Reviewed-by: Zi Yan >>>>>>> Acked-by: David Hildenbrand (Red Hat) >>>>>>> Reviewed-by: Anshuman Khandual >>>>>>> Signed-off-by: Shivank Garg >>>>>>> --- >>>>>>> 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 >>>>> >>>> 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 >>>> 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 >>>> Signed-off-by: Shivank Garg >>>> --- >>>> 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. Ah yes nice observation : ) Thanks. > > For khugepaged this isn't the case. We're good as we are. > > Thanks, Lorenzo