On Mon, Oct 18, 2021 at 07:28:13PM +0100, Matthew Wilcox wrote:

On Mon, Oct 18, 2021 at 12:47:37PM -0400, Johannes Weiner wrote:

quoted

On Sat, Oct 16, 2021 at 04:28:23AM +0100, Matthew Wilcox wrote:

quoted

On Wed, Sep 22, 2021 at 11:08:58AM -0400, Johannes Weiner wrote:

quoted

		As per the other email, no conceptual entry point for
		tail pages into either subsystem, so no ambiguity
		around the necessity of any compound_head() calls,
		directly or indirectly. It's easy to rule out
		wholesale, so there is no justification for
		incrementally annotating every single use of the page.

The justification is that we can remove all those hidden calls to
compound_head().  Hundreds of bytes of text spread throughout this file.

I find this line of argument highly disingenuous.

No new type is necessary to remove these calls inside MM code. Migrate
them into the callsites and remove the 99.9% very obviously bogus
ones. The process is the same whether you switch to a new type or not.

(I'll send more patches like the PageSlab() ones to that effect. It's
easy. The only reason nobody has bothered removing those until now is
that nobody reported regressions when they were added.)

That kind of change is actively dangerous.  Today, you can call
PageSlab() on a tail page, and it returns true.  After your patch,
it returns false.  Sure, there's a debug check in there that's enabled
on about 0.1% of all kernel builds, but I bet most people won't notice.

We're not able to catch these kinds of mistakes at review time:
https://lore.kernel.org/linux-mm/20211001024105.3217339-1-willy@infradead.org/ (local)

which means it escaped the eagle eyes of (at least):
    Signed-off-by: Andrey Konovalov [off-list ref]
    Reviewed-by: Andrey Ryabinin [off-list ref]
    Reviewed-by: Dmitry Vyukov [off-list ref]
    Cc: Christoph Lameter [off-list ref]
    Cc: Mark Rutland [off-list ref]
    Cc: Will Deacon [off-list ref]
    Signed-off-by: Andrew Morton [off-list ref]
    Signed-off-by: Linus Torvalds [off-list ref]

I don't say that to shame these people.  We need the compiler's help
here.  If we're removing the ability to ask for whether a tail page
belongs to the slab allocator, we have to have the compiler warn us.

I have a feeling your patch also breaks tools/vm/page-types.c

As Hugh said in the meeting in response to this, "you'll look at
kernel code for any amount of time, you'll find bugs".

I already pointed out dangerous code from anon/file confusion
somewhere in this thread.

None of that is a reason not to fix it. But it should inform the
approach on how we fix it. I'm not against type safety, I'm for
incremental changes. And replacing an enormous subset of struct page
users with an unproven new type and loosely defined interaction with
other page subtypes is just not that.

quoted

But typesafety is an entirely different argument. And to reiterate the
main point of contention on these patches: there is no concensus among
MM people how (or whether) we want MM-internal typesafety for pages.

I don't think there will ever be consensus as long as you don't take
the concerns of other MM developers seriously.  On Friday's call, several
people working on using large pages for anon memory told you that using
folios for anon memory would make their lives easier, and you didn't care.

Nope, one person claimed that it would help, and I asked how. Not
because I'm against typesafety, but because I wanted to know if there
is an aspect in there that would specifically benefit from a shared
folio type. I don't remember there being one, and I'm not against type
safety for anon pages.

What several people *did* say at this meeting was whether you could
drop the anon stuff for now until we have consensus.

quoted

Anyway, the email you are responding to was an offer to split the
uncontroversial "large pages backing filesystems" part from the
controversial "MM-internal typesafety" discussion. Several people in
both the fs space and the mm space have now asked to do this to move
ahead. Since you have stated in another subthread that you "want to
get back to working on large pages in the page cache," and you never
wanted to get involved that deeply in the struct page subtyping
efforts, it's not clear to me why you are not taking this offer.

I am.  This email was written after trying to do just this.  I dropped
the patches you were opposed to and looked at the result.  It's not good.

You seem wedded to this idea that "folios are just for file backed
memory", and that's not my proposal at all.  folios are for everything.
Maybe we specialise out other types of memory later, or during, or
instead of converting something to use folios, but folios are an utterly
generic concept.

That train left the station when several people said slab should not
be in the folio. Once that happened, you could no longer say it'll
work itself out around the edges. Now it needs a real approach to
coordinating with other subtypes, including shared properties and
implementation between them.

The "simple" folio approach only works when it really is a wholesale
replacement for *everything* that page is right now - modulo PAGE_SIZE
and modulo compound tail. But it isn't that anymore, is it?

Folio can't be everything and only some subtypes simultaneously. So
when you say folio is for everything, is struct slab dead? If not, how
is the relationship between them? How do you query shared property?

There really is no coherent proposal right now. These patches start an
open-ended conversion into a nebulous direction.

All I'm saying is: start with a reasonable, delineated scope (page
cache), and if that test balloon works out we can do the next one with
lessons learned from the first. Maybe that will converge to the
"simple" folio for all compound subtypes, maybe we'll move more toward
explicit subtyping that imply the head/tail thing anyway.

What is even the counter argument to that?