linux-kernel - Re: [PATCH v3 2/2] mm: Tidy up shmem mTHP controls and stats

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <78ddd0df-e8e1-414b-92fc-19a4860f183d@arm.com>
Date: Thu, 5 Sep 2024 08:29:43 +0100
From: Ryan Roberts <ryan.roberts@....com>
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: Baolin Wang <baolin.wang@...ux.alibaba.com>,
 Hugh Dickins <hughd@...gle.com>,
 "Matthew Wilcox (Oracle)" <willy@...radead.org>,
 David Hildenbrand <david@...hat.com>, Barry Song <baohua@...nel.org>,
 Lance Yang <ioworker0@...il.com>, Gavin Shan <gshan@...hat.com>,
 linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH v3 2/2] mm: Tidy up shmem mTHP controls and stats

On 04/09/2024 21:46, Andrew Morton wrote:
> On Wed, 4 Sep 2024 11:47:29 +0100 Ryan Roberts <ryan.roberts@....com> wrote:
> 
>>>> +static struct attribute *any_ctrl_attrs[] = {
>>>> +    NULL,
>>>> +};
>>>> +
>>>> +static const struct attribute_group any_ctrl_attr_grp = {
>>>> +    .attrs = any_ctrl_attrs,
>>>>   };
>>>
>>> I wonder why adding a NULL group?
> 
> Was this review comment addressed?

Yes, we had discussion at [1] and concluded to leave it as is; Baolin had a
preference to remove it (but no strong opinion) and Barry preferred it as is to
explicitly indicate that there are no controls shared by file/shmem/anon currently.

[1]
https://lore.kernel.org/linux-mm/CAGsJ_4z+yERPLwzm-8Mkx8MsNZAz0zZWycZfuGRjOc4kxS=HwQ@mail.gmail.com/


> 
>>
>> ...
>>
>>>> +    &shmem_alloc_attr.attr,
>>>> +    &shmem_fallback_attr.attr,
>>>> +    &shmem_fallback_charge_attr.attr,
>>>> +#endif
>>>> +    NULL,
>>>> +};
>>>> +
>>>> +static struct attribute_group file_stats_attr_grp = {
>>>> +    .name = "stats",
>>>> +    .attrs = file_stats_attrs,
>>>> +};
>>>> +
>>>> +static struct attribute *any_stats_attrs[] = {
>>>> +#ifdef CONFIG_SHMEM
>>>> +    &swpout_attr.attr,
>>>> +    &swpout_fallback_attr.attr,
>>>> +#endif
>>>
>>> Sorry I did not point it out in early version. I think file pages and shmem can
>>> also be split, while 'split_deferred' is only for anonymous page. So I think the
>>> any_stats_attrs should be:
>>> static struct attribute *any_stats_attrs[] = {
>>> #ifdef CONFIG_SHMEM
>>>     &swpout_attr.attr,
>>>     &swpout_fallback_attr.attr,
>>> #endif
>>>     &split_attr.attr,
>>>     &split_failed_attr.attr,
>>>     NULL,
>>> };
>>
>> Could you please squash the following into this patch, which is already in
>> mm-unstable? I'm hoping this sufficient and I don't need to send a whole new
>> revision since there are changes on top of this in mm-unstable, which makes
>> things tricky.
> 
> I did that.  Please send along a Signoff and a changelog?

Ahh sorry, so even though you are squashing into an existing patch which has the
change log and sign-off, the process still requires a separate one here? My bad.

----8<----

Fixup commit ("mm: Tidy up shmem mTHP controls and stats") by moving split_attr
and split_failed_attr stats to the any_stats_attrs group. All memory types can
have their folios split, so it was incorrect to only show this stat for anon memory.


Signed-off-by: Ryan Roberts <ryan.roberts@....com>

----8<----

Thanks,
Ryan