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

[Svetly Todorov]

> Thanks for your careful review.
No problem!! It's a valuable learning experience for me.

> > > -	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));
> }
My bad. What I meant was, if CONFIG_KSM is undefined, then

> #ifdef CONFIG_KSM
> ...
> static __always_inline bool PageKsm(struct page *page)
> {
> 	return folio_test_ksm(page_folio(page));
> }

will fall through to

> # else
> TESTPAGEFLAG_FALSE(Ksm, ksm)
> #endif

And you're right -- there is no PG_ksm comparison --
but the autogenerated PageKsm will always return false:

> #define TESTPAGEFLAG_FALSE(uname, lname) \
> ...
> static inline int Page##uname(const struct page *page)
> {
> 	return 0;
> }

But given your comments below, I'm realizing this isn't as important
as I thought it was.

> 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.
I see. I misunderstood how the flags are applied.
I thought that 11 == (01 | 10) -- i.e. that KSM was an intersection of
MOVABLE and ANON. But they're more like mutually-exclusive states. And
I doubt that a page will end up in the KSM "state" if CONFIG_KSM is
disabled. So we don't need to rely on PageKsm() for the CONFIG_KSM
check.

That said, won't

	if (mapping & PAGE_MAPPING_KSM)

return true even if a mapping is ANON (01) or MOVABLE (10)
but not KSM (11)? Shouldn't this at least be

	if (mapping & PAGE_MAPPING_KSM == PAGE_MAPPING_KSM)

?

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

Dumb mistake on my part. For some reason, I thought that every
folio->page had its PG_head set.

> 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.
I see. That makes sense.

Cool! Thanks for bearing with me. Beyond the KSM stuff, my only
hangup is that this patch doesn't account for the handful of
remaining per-page flags (KPF_HWPOISON, KPF_ARCH_*). Should I
take this diff, tack those on in a second commit, and then put
up a v4? Forgive me, I'm very green to the kernel dev process...