Re: [PATCH 00/62] Separate struct slab from struct page

[Johannes Weiner <hannes@cmpxchg.org>]

On Mon, Oct 04, 2021 at 02:45:48PM +0100, Matthew Wilcox (Oracle) wrote:
> This is an offshoot of the folio work, although it does not depend on any
> of the outstanding pieces of folio work.  One of the more complex parts
> of the struct page definition is the parts used by the slab allocators.
> It would be good for the MM in general if struct slab were its own data
> type, and it also helps to prevent tail pages from slipping in anywhere.
> 
> The slub conversion is done "properly", ie in individually reviewable,
> bisectable chunks.  The slab & slob conversions are slapdash.  The
> patches to switch bootmem & zsmalloc away from slab elements are also
> expedient instead of well thought through.  The KASAN and memcg parts
> would also benefit from a more thoughtful approach.

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.

> 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.