On Tue, Oct 05, 2021 at 04:13:12PM -0400, Kent Overstreet wrote:
> On Tue, Oct 05, 2021 at 06:51:32PM +0100, Matthew Wilcox wrote:
> > We're trying to tidy up the mess in struct page, and as part of removing
> > slab from struct page, zsmalloc came on my radar because it's using some
> > of slab's fields. The eventual endgame is to get struct page down to a
> > single word which points to the "memory descriptor" (ie the current
> > zspage).
> >
> > zsmalloc, like vmalloc, allocates order-0 pages. Unlike vmalloc,
> > zsmalloc allows compaction. Currently (from the file):
> >
> > * Usage of struct page fields:
> > * page->private: points to zspage
> > * page->freelist(index): links together all component pages of a zspage
> > * For the huge page, this is always 0, so we use this field
> > * to store handle.
> > * page->units: first object offset in a subpage of zspage
> > *
> > * Usage of struct page flags:
> > * PG_private: identifies the first component page
> > * PG_owner_priv_1: identifies the huge component page
> >
> > This isn't quite everything. For compaction, zsmalloc also uses
> > page->mapping (set in __SetPageMovable()), PG_lock (to sync with
> > compaction) and page->_refcount (compaction gets a refcount on the page).
> >
> > Since zsmalloc is so well-contained, I propose we completely stop
> > using struct page in it, as we intend to do for the rest of the users
> > of struct page. That is, the _only_ element of struct page we use is
> > compound_head and it points to struct zspage.
> >
> > That means every single page allocated by zsmalloc is PageTail(). Also it
> > means that when isolate_movable_page() calls trylock_page(), it redirects
> > to the zspage. That means struct zspage must now have page flags as its
> > first element. Also, zspage->_refcount, and zspage->mapping must match
> > their locations in struct page. That's something that we'll get cleaned
> > up eventually, but for now, we're relying on offsetof() assertions.
> >
> > The good news is that trylock_zspage() no longer needs to walk the
> > list of pages, calling trylock_page() on each of them.
> >
> > Anyway, is there a good test suite for zsmalloc()? Particularly something
> > that would exercise its interactions with compaction / migration?
> > I don't have any code written yet.
>
> This is some deep hackery.
... thank you? ;-) Actually, it's indicative that we need to work
through what we're doing in a bit more detail.
> So to restate - and making sure I understand correctly - the reason for doing it
> this way is that the compaction code calls lock_page(); by using compound_head
> (instead of page->private) for the pointer to the zspage is so that
> compound_head() will return the pointer to zspage, and lock_page() uses
> compound_page(), so the compaction code, when it calls lock_page(), will
> actually be taking the lock in struct zspage.
Yes. Just like when we call lock_page() on a THP or hugetlbfs tail
page, we actually take the lock in the head page.
> So on the one hand, getting struct page down to two words means that we're going
> to be moving the page lock bit into those external structs (maybe we _should_ be
> doing some kind of inheritence thing, for the allocatee interface?) - so it is
> cool that that all lines up.
>
> But long term, we're going to need two words in struct page, not just one: We
> need to store the order of the allocation, and a type tagged pointer, and I
> don't think we can realistically cram compound_order into the same word as the
> type tagged pointer. Plus, transparently using slab for larger allocations -
> that today use the buddy allocator interface - means slab can't be using the
> allocatee pointer field.
I'm still not convinced of the need for allocator + allocatee words.
But I don't think we need to resolve that point of disagreement in
order to make progress towards the things we do agree on.
> And in my mind, compound_head is allocator state, not allocatee state, and it's
> always been using for getting to the head page, which... this is not, so using
> it this way, as slick as it is... eh, not sure that's quite what we want to do.
In my mind, in the future where all memory descriptors are dynamically
allocated, when we allocate an order-3 page, we initialise the
'allocatee state' of each of the 8 consecutive pages to point to the
memory descriptor that we just allocated. We probably also encode the
type of the memory descriptor in the allocatee state (I think we've
probably got about 5 bits for that).
The lock state has to be in the memory descriptor. It can't be in the
individual page. So I think all memory descriptors needs to start with
a flags word. Memory compaction could refrain from locking pages if
the memory descriptor is of the wrong type, of course.
> Q: should lock_page() be calling compound_head() at all? If the goal of the type
> system stuff that we're doing is to move the compound_head() calls up to where
> we're dealing with tail pages and never more after that, then that call in
> lock_page() should go - and we'll need to figure out something different for the
> migration code to take the lock in the allocatee-private structure.
Eventually, I think lock_page() disappears in favour of folio_lock().
That doesn't quite work for compaction, but maybe we could do something
like this ...
typedef struct { unsigned long f; } pgflags_t;
void pgflags_lock(pgflags_t *);
struct folio {
pgflags_t flags;
...
};
static inline void folio_lock(struct folio *folio)
{
pgflags_lock(&folio->flags);
}
That way, compaction doesn't need to know what kind of memory descriptor
this page belongs to.
Something similar could happen for user-mappable memory. eg:
struct mmapable {
atomic_t _refcount;
atomic_t _mapcount;
struct address_space *mapping;
};
struct folio {
pgflags_t flags;
struct mmapable m;
struct list_head lru;
pgoff_t index;
unsigned long private;
...
};
None of this can happen before we move struct slab out of struct page,
because we can't mess with the alignment of freelist+counters and
struct mmapable is 3 words on 32-bit and 2 on 64-bit. So I'm trying
to break off pieces that can be done to get us a little bit closer.
> I've been kind of feeling that since folios are furthest along, separately
> allocating them and figuring out how to make that all work might be the logical
> next step.
I'd rather not put anything else on top of the folio work until some of
it -- any of it -- is merged. Fortunately, our greatest bottleneck is
reviewer bandwidth, and it's actually parallelisable; the people who need
to approve of slab are independent of anon, file and zsmalloc reviewers.
