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