linux-kernel - Re: [PATCH v1] mm: shmem: Rename mTHP shmem counters

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK1f24mjcukbjBnrrO3TLZb1KdVhSxqL4_jsob5E_FAjGD4mdQ@mail.gmail.com>
Date: Tue, 9 Jul 2024 09:21:09 +0800
From: Lance Yang <ioworker0@...il.com>
To: David Hildenbrand <david@...hat.com>
Cc: Ryan Roberts <ryan.roberts@....com>, Barry Song <baohua@...nel.org>, 
	Andrew Morton <akpm@...ux-foundation.org>, Hugh Dickins <hughd@...gle.com>, 
	Jonathan Corbet <corbet@....net>, Baolin Wang <baolin.wang@...ux.alibaba.com>, 
	Matthew Wilcox <willy@...radead.org>, Zi Yan <ziy@...dia.com>, 
	Daniel Gomez <da.gomez@...sung.com>, linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH v1] mm: shmem: Rename mTHP shmem counters

On Tue, Jul 9, 2024 at 4:50 AM David Hildenbrand <david@...hat.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@....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
>