Re: [PATCH v3] kpageflags: respect folio head-page flag placement

[Matthew Wilcox <willy@infradead.org>]

On Wed, Mar 20, 2024 at 04:40:43PM -0700, Svetly Todorov wrote:
> 
> Hi Matthew,
> 
> > I have a somewhat different patch for this.  Let me know what you think.
> > It depends on a few other patches in my tree, so probably won't compile
> > for you.
> I don't have extensive experience with folios or anything but on the
> whole it looks good to me. I like the use of `mapping` to dodge the
> compound_head() checks. Beyond that, only a few things caught my eye.

Thanks for your careful review.

> > -	if (PageKsm(page))
> > +	if (mapping & PAGE_MAPPING_KSM)
> >   		u |= 1 << KPF_KSM;
> This might need an #ifdef?
> Say mapping is movable and anon -- then (mapping & PAGE_MAPPING_KSM) is
> true. Before, we called PageKsm, which falls through to a PG_ksm check.
> If !CONFIG_KSM then that flag is always false. But now, we're liable to
> report KPF_KSM even if !CONFIG_KSM.

I'm not sure where you see a PG_ksm check:

static __always_inline bool folio_test_ksm(const struct folio *folio)
{
        return ((unsigned long)folio->mapping & PAGE_MAPPING_FLAGS) ==
                                PAGE_MAPPING_KSM;
}

static __always_inline bool PageKsm(const struct page *page)
{
        return folio_test_ksm(page_folio(page));
}

There's no such thing as a movable anon page -- the two bits in the
bottom of the mapping pointer mean:

00	file (or NULL)
01	anon
10	movable
11	KSM

Perhaps it might be clearer to say that anon pages are inherently
movable; the movable type really means that the reset of the mapping
pointer refers to a movable_operations instead of a mapping or anon_vma.

> >   	/*
> >   	 * compound pages: export both head/tail info
> >   	 * they together define a compound page's start/end pos and order
> >   	 */
> > -	if (PageHead(page))
> > -		u |= 1 << KPF_COMPOUND_HEAD;
> > -	if (PageTail(page))
> > +	if (page == &folio->page)
> > +		u |= kpf_copy_bit(k, KPF_COMPOUND_HEAD, PG_head);
> > +	else
> >   		u |= 1 << KPF_COMPOUND_TAIL;
> This makes sense but it'd require changes to the documentation.
> I ran a python3 memhog to see if anonymous pages are currently reported
> as COMPOUND_HEAD or COMPOUND_TAIL and it seems to be a no on both.
> But with this, I think every pfn will have one of the two set.
> Unless you can have a page outside of a folio -- not sure.

I see your confusion.  We have three cases; head, tail and neither
(obviously a page is never both head & tail).  If a page is neither,
it's order-0 and it is the only page in the folio.  So we handle head
or neither in the first leg of the 'if' where we set KPF_COMPOUND_HEAD
if PG_head is set, and tail in the 'else' leg.

> Also, in
> > -	if (page_is_idle(page))
> > +#if defined(CONFIG_PAGE_IDLE_FLAG) && defined(CONFIG_64BIT)
> > +	u |= kpf_copy_bit(k, KPF_IDLE,          PG_idle);
> > +#else
> > +	if (folio_test_idle(folio))
> >   		u |= 1 << KPF_IDLE;
> > +#endif
> > 
> and
> > -	if (PageSwapCache(page))
> > +#define SWAPCACHE ((1 << PG_swapbacked) | (1 << PG_swapcache))
> > +	if ((k & SWAPCACHE) == SWAPCACHE)
> >   		u |= 1 << KPF_SWAPCACHE;
> >   	u |= kpf_copy_bit(k, KPF_SWAPBACKED,	PG_swapbacked);
> it seems to me like the #ifdef/#define could be supplanted by
> folio_test_idle and folio_test_swapcache. But I guess those would
> require extra folio_flags queries and an #include <page_idle.h>.
> So if this is more performant, I can understand the design.

It's not so much the performance as it is the atomicity.  I'm doing my
best to get an atomic snapshot of the flags and report a consistent
state, even if it might be stale by the time the user sees it.