Re: [PATCH v1] mm: shmem: Rename mTHP shmem counters

[Lance Yang <ioworker0@gmail.com>]

On Tue, Jul 9, 2024 at 4:50 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 08.07.24 14:29, Ryan Roberts wrote:
> > On 08/07/2024 12:36, Barry Song wrote:
> >> On Mon, Jul 8, 2024 at 11:24 PM Ryan Roberts <ryan.roberts@arm.com> wrote:
> >>>
> >>> The legacy PMD-sized THP counters at /proc/vmstat include
> >>> thp_file_alloc, thp_file_fallback and thp_file_fallback_charge, which
> >>> rather confusingly refer to shmem THP and do not include any other types
> >>> of file pages. This is inconsistent since in most other places in the
> >>> kernel, THP counters are explicitly separated for anon, shmem and file
> >>> flavours. However, we are stuck with it since it constitutes a user ABI.
> >>>
> >>> Recently, commit 66f44583f9b6 ("mm: shmem: add mTHP counters for
> >>> anonymous shmem") added equivalent mTHP stats for shmem, keeping the
> >>> same "file_" prefix in the names. But in future, we may want to add
> >>> extra stats to cover actual file pages, at which point, it would all
> >>> become very confusing.
> >>>
> >>> So let's take the opportunity to rename these new counters "shmem_"
> >>> before the change makes it upstream and the ABI becomes immutable.
> >>
> >> Personally, I think this approach is much clearer. However, I recall
> >> we discussed this
> >> before [1], and it seems that inconsistency is a concern?
> >
> > Embarrassingly, I don't recall that converstation at all :-| but at least what I
> > said then is consistent with what I've done in this patch.
> >
> > I think David's conclusion from that thread was to call them FILE_, and add both
> > shmem and pagecache counts to those counters, meaning we can keep the same name
> > as legacy THP counters. But those legacy THP counters only count shmem, and I
> > don't think we would get away with adding pagecache counts to those at this
> > point? (argument: they have been around for long time and there is a risk that
> > user space relies on them and if they were to dramatically increase due to
> > pagecache addition now that could break things). In that case, there is still
> > inconsistency, but its worse; the names are consistent but the semantics are
> > inconsistent.
> >
> > So my vote is to change to SHMEM_ as per this patch :)
>
> I also forgot most of the discussion, but these 3 legacy counters are
> really only (currently) incremented for shmem. I think my idea was to
> keep everything as FILE_ for now, maybe at some point make the pagecache
> also use them, and then maybe have separate FILE_ + SHMEM_.
>
> But yeah, likely it's best to only have "shmem" here for now, because
> who knows what we can actually change about the legacy counters. But
> it's always though messing with legacy stuff that is clearly suboptimal ...

Couldn't agree more! It's never an easy task to handle such matters :)
Perhaps, the time has come for us to separate FILE_ and SHMEM_.

Thanks,
Lance

>
> --
> Cheers,
>
> David / dhildenb
>