| 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: <fa408e3e-36d6-4993-813f-3de4264c6b03@arm.com>
Date: Sat, 24 Jan 2026 17:26:11 +0530
From: Dev Jain <dev.jain@....com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: "Garg, Shivank" <shivankg@....com>,
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 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@...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."
> 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
Powered by blists - more mailing lists