Re: [PATCH] s390/mm: Add missing ctor/dtor on page table upgrade

[Heiko Carstens <hca@linux.ibm.com>]

On Fri, Jan 24, 2025 at 08:58:07AM +0100, Kevin Brodsky wrote:
> On 23/01/2025 17:03, Alexander Gordeev wrote:
> > Commit 78966b550289 ("s390: pgtable: add statistics for PUD and P4D
> > level page table") misses the call to pagetable_p4d_ctor() against
> > a newly allocated P4D table in crst_table_upgrade();
> >
> > Commit 68c601de75d8 ("mm: introduce ctor/dtor at PGD level") misses
> > the call to pagetable_pgd_ctor() against a newly allocated PGD and
> > the call to pagetable_dtor() against a newly allocated P4D that is
> > about to be freed on crst_table_upgrade() PGD upgrade fail path.
> >
> > The missed constructors and destructor break (at least) the page
> > table accounting when a process memory space is upgraded.
> >
> > Reported-by: Heiko Carstens <hca@linux.ibm.com>
> > Closes: https://lore.kernel.org/all/20250122074954.8685-A-hca@linux.ibm.com/
> > Suggested-by: Heiko Carstens <hca@linux.ibm.com>
> > Fixes: 78966b550289 ("s390: pgtable: add statistics for PUD and P4D level page table")
> > Fixes: 68c601de75d8 ("mm: introduce ctor/dtor at PGD level")
> > Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
> > ---
> > The patch is against:
> >
> >   git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git	next-20250123
> >   git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm			mm-stable
> 
> Thank you for putting together this patch! I was completely unaware of
> this "upgrade" path on s390.

This whole thing is even worse than it looks after this patch. With page
table upgrade taken into account we still have the oddity that e.g. a
previous pgd becomes a pud or p4d, which means that ctor and dtor functions
might be called for different levels for the same page table. As of now
this is ok, since they do all the same.

As a quick fix this patch is ok, and most likely it will be ok for a long
time, but it doesn't give me good feeling :)

In addition, looking at [1] where page table accounting was introduced: it
is really meant to reflect the memory consumption used by page tables. This
might work for nearly all architectures which have the same page table size
for every level; but on s390 the lowest level comes with 4kb page tables
while all other levels come with 16kb page tables.

Therefore at least on s390 you really can't tell how much memory is
consumed by page tables by only looking at nr_page_table_pages. It _might_
make sense to introduce a factor of four for page table accounting for
higher levels, so those numbers make at least some sense; but not sure
about that.

[1] https://lore.kernel.org/all/20201130212541.2781790-3-shakeelb@google.com/T/#mb6c6f2a84ded27cd9b3d54140dde1d5a75c74735