Re: [PATCH v1 2/6] mm: allow reuse of the lower 16 bit of the page type with an actual type

[David Hildenbrand]

Am 27.05.24 um 17:26 schrieb Matthew Wilcox:
> On Mon, May 27, 2024 at 04:14:50PM +0200, David Hildenbrand wrote:
> > As long as the owner sets a page type first, we can allow reuse of the
> > lower 18 bit: sufficient to store an offset into a 64 KiB page, which
>
> You say 18 here and 16 below.

Thanks, missed to fixup one instance after going back and forth.

> > is the maximum base page size in *common* configurations (ignoring the
> > 256 KiB variant). Restrict it to the head page.
> >
> > We'll use that for zsmalloc next, to set a proper type while still
> > reusing that field to store information (offset into a base page) that
> > cannot go elsewhere for now.
> >
> > Fear of running out of bits for storing the actual type? Actually, we
> > don't need one bit per type, we could store a single value instead.
> > Further, we could likely limit PAGE_TYPE_BASE to a single (highest) bit.
>
> We could, but it's more instructions to check.

Maybe, and maybe not sufficient more that we care.

I was thinking of something like the following (probably broken but you should 
get the idea):

/*
 * If the _mapcount is negative, we might store a page type. The
 * page_type field corresponds to the most significant byte of the
 * _mapcount field. As the mapcount is initialized to -1, we have no
 * type as defaults. We have plenty of room to underflow the mapcount
 * before we would end up indicating a valid page_type.
 */
#define PAGE_TYPE_BASE	0x80
enum page_type {
	PT_buddy = PAGE_TYPE_BASE,
	PT_offline,
	PT_table,
	PT_guard,
	PT_hugetlb,
	PT_slab,
	/* we must forbid page_type == -1 */
	PT_unusable = 0xff
};

In struct page:

union {
	atomic_t _mapcount;

#if __BYTE_ORDER == __BIG_ENDIAN
	struct {
		uint16_t page_type_data;
		uint8_t page_type_reserved;
		uint8_t page_type;
	};
#else
	struct {
		uint8_t page_type;
		uint8_t page_type_reserved;
		uint16_t page_type_data;
	};
#end
};

#define PageType(page, type) (page->page_type == type)

Once could maybe also change page_has_type to simply work on the
fact that the highest bit must be set and any other bit of the type must be clear:

static inline int page_has_type(const struct page *page)
{
	return (page->page_type & PAGE_TYPE_BASE) &&
		page->page_type != 0xffff;
}

But just some thought.

> > +++ b/include/linux/page-flags.h
> > @@ -945,14 +945,18 @@ PAGEFLAG_FALSE(HasHWPoisoned, has_hwpoisoned)
> >    */
> >   
> >   #define PAGE_TYPE_BASE	0xf0000000
> > -/* Reserve		0x0000007f to catch underflows of _mapcount */
> > -#define PAGE_MAPCOUNT_RESERVE	-128
> > -#define PG_buddy	0x00000080
> > -#define PG_offline	0x00000100
> > -#define PG_table	0x00000200
> > -#define PG_guard	0x00000400
> > -#define PG_hugetlb	0x00000800
> > -#define PG_slab		0x00001000
> > +/*
> > + * Reserve		0x0000ffff to catch underflows of _mapcount and
> > + * allow owners that set a type to reuse the lower 16 bit for their own
> > + * purposes.
> > + */
> > +#define PAGE_MAPCOUNT_RESERVE	-65536
>
> I think my original comment was misleading.  This should be:
>
>   * Reserve 0xffff0000 - 0xfffffffe to catch _mapcount underflow.

Makes sense.

> How about we start at the top end and let people extend down?  ie:
>
> #define PAGE_TYPE_BASE	0x80000000
> #define PG_buddy	0x40000000
> #define PG_offline	0x20000000
> #define PG_table	0x10000000
> #define PG_guard	0x08000000
> #define PG_hugetlb	0x04000000
> #define PG_slab		0x02000000
> #define PAGE_MAPCOUNT_RESERVE	(~0x0000ffff)
>
> Now we can see that we have 9 flags remaining, which should last until
> we can have proper memdesc typing.

Also works for me.

Thanks!

--
Thanks,

David / dhildenb