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

[Ryan Roberts <ryan.roberts@arm.com>]

On 04/09/2024 21:46, Andrew Morton wrote:
> On Wed, 4 Sep 2024 11:47:29 +0100 Ryan Roberts <ryan.roberts@arm.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@arm.com>

----8<----

Thanks,
Ryan