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 DA2EE271A94 for ; Sat, 24 Jan 2026 10:54:38 +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=1769252081; cv=none; b=ODAdaY55WG9RN8NE1Ioc+jIjZjyVxua9bwbzYQKeGOgdhHJKCAJbL59sdNtt1qK007lylEyjQN5hiMulrGXcK9i0RuwiHAOq0GxeuvQqu76A5Q98GTb43Ipl3po4VT9jQYP9uTwo8nbggrckfavejAAOMne4O+Ixwl5LIPTHTBg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1769252081; c=relaxed/simple; bh=RxvO5vONpnxQ+ZV22i/na1jrxAmtLk2xnvBEY5Wlu3w=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=so2snZ7a9qIWIBYKa2o3vXlzPISGLGQ8hNgoelsvMy9jStSIgtSXt2ciAjd3v1XGlacPwXSGuK1CH7zMQ6R1RSz2Xb3A5WVNCTrWa2rT6WF5OoUgg39QqN7L+ByToiF5yTRgzwWes6pVs/8+OGFGyixuqlGbSjvmnh9XxcTUIaM= 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 8BC13150C; Sat, 24 Jan 2026 02:54:25 -0800 (PST) Received: from [10.164.10.250] (unknown [10.164.10.250]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id B52993F73F; Sat, 24 Jan 2026 02:54:27 -0800 (PST) Message-ID: <94c84a3c-8ed9-4bb5-8e64-69bcb8306aba@arm.com> Date: Sat, 24 Jan 2026 16:24:24 +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 , "Garg, Shivank" Cc: 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> Content-Language: en-US From: Dev Jain In-Reply-To: <50da84da-1cd6-4b8b-babd-b6dea405713b@lucifer.local> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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." madvise_collapse does kmalloc() to allocate this large struct. I was suggesting to do the same for khugepaged, to enforce consistency. > > Thanks, Lorenzo