| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <94c84a3c-8ed9-4bb5-8e64-69bcb8306aba@arm.com>
Date: Sat, 24 Jan 2026 16:24:24 +0530
From: Dev Jain <dev.jain@....com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
"Garg, Shivank" <shivankg@....com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
David Hildenbrand <david@...nel.org>, Zi Yan <ziy@...dia.com>,
Baolin Wang <baolin.wang@...ux.alibaba.com>,
"Liam R . Howlett" <Liam.Howlett@...cle.com>, Nico Pache
<npache@...hat.com>, Ryan Roberts <ryan.roberts@....com>,
Barry Song <baohua@...nel.org>, Lance Yang <lance.yang@...ux.dev>,
linux-mm@...ck.org, linux-kernel@...r.kernel.org,
Wei Yang <richard.weiyang@...il.com>,
Anshuman Khandual <anshuman.khandual@....com>
Subject: Re: [PATCH V3 5/5] mm/khugepaged: make khugepaged_collapse_control
static
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@...il.com>
>>>>> Reviewed-by: Zi Yan <ziy@...dia.com>
>>>>> Acked-by: David Hildenbrand (Red Hat) <david@...nel.org>
>>>>> Reviewed-by: Anshuman Khandual <anshuman.khandual@....com>
>>>>> Signed-off-by: Shivank Garg <shivankg@....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@....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@....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@....com>
>> Signed-off-by: Shivank Garg <shivankg@....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
Powered by blists - more mailing lists