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.
