On Mon, Oct 11, 2021 at 04:07:14PM -0400, Johannes Weiner wrote:

This looks great to me. It's a huge step in disentangling struct page,
and it's already showing very cool downstream effects in somewhat
unexpected places like the memory cgroup controller.

Thanks!

quoted

I'm not entirely happy with slab_test_cache() for a predicate name.
I actually liked SlabAllocation() better, but then I remembered that we're
trying to get away from InterCapping, and somehow slab_test_allocation()
didn't feel right either.

I touched on this in the reply to the memcg patch, but I wonder if it
would be better to not have this test against struct slab at all.

It's basically a type test that means slab_is_slab(), and its
existence implies that if you see 'struct slab' somewhere, you don't
know for sure - without having more context - whether it's been vetted
or not. This makes 'struct slab' and option/maybe type that needs a
method of self-identification. Right now it can use the PG_slab bit in
the flags field shared with the page, but if it were separately
allocated some day, that would occupy dedicated memory.

And describing memory, that's struct page's role: to identify what's
sitting behind a random pfn or an address. "struct page should be a
pointer and a type tag", or something like that ;-)

If instead of this:

	slab = virt_to_slab(p);
	if (!slab_test_cache(slab)) {
		page = (struct page *)slab;
		do_page_things(page);
	} else {
		...
	}

you wrote it like this:

	page = virt_to_page(p);
	if (PageSlab(page)) {			/* page->type */
		slab = page_slab(page);		/* page->pointer */
		do_slab_things(slab);
	} else {
		do_bypass_things(page);
	}

it would keep the ambiguity and type resolution in the domain of the
page, and it would make struct slab a strong type that doesn't need to
self-identify.

... yeah.  I'm absolutely on board with the goal of moving to each
struct page being very small and having essentially a single
discriminated pointer to its descriptor.  eg all 2^N pages allocated
to a slab would have a bit pattern of 0110 in the bottom 4 bits.
So PageSlab would still make sense, and calling page_slab() on something
that wasn't tagged with 0110 would cause some kind of error.

But until we get to that goal, it's hard to write code that's going to
work in either scenario.  We do need to keep the PG_slab bit to
distinguish slab pages from other kinds of memory.  And we don't want
to be unnecessarily calling compound_head() (although I'm willing to
do that for the sake of better looking code).

What if we replicated the PG_slab bit across all the tail pages
today?  So we could call PageSlab() on any page and instead of it
doing a compound_head() lookup, it just looked at the tail->flags.
Then page_slab() does the actual compound_head() lookup.  I think it
should be fairly cheap to set the extra bits at allocation because we
just set tail->compound_head, so the lines should still be in cache.
(PageSlab is marked as PF_NO_TAIL, so it calls compound_head() on
every access).