On Thu, Mar 21, 2024 at 12:08:01PM -0700, Svetly Todorov wrote:
> > > > - 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:
Yes, that's true. Usually we care about this because we can optimise
out large chunks of code if a config option (eg CONFIG_KSM) is disabled.
In this case, we're talking about a couple of instructions, and it's
generally not worth optimising those out in order to add an ifdef in
the code. We've got quite a long way with Linux without it becoming
overrun with ifdefs (compare, eg, the Mach source code), and long may
that continue ;-)
> > 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)
Uh, yeah, that was a mistake. This should do the trick:
if (is_anon) {
u |= 1 << KPF_ANON;
if (mapping & PAGE_MAPPING_KSM)
u |= 1 << KPF_KSM;
}
(all KSM pages are reported as anon pages as well, both before and after
this patch; see how folio_test_anon() only checks the bottom bit)
> > 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.
At this point, it's bad naming, but it's not worth the churn of fixing
it; we have a better destination in mind, and we'll get there soon enough.
> 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...
Oh, yes, that's a bug on my part. HWPOISON is definitely per-page,
not per-folio (although the handling of it differs for hugetlb)
and I haven't looked at the PG_arch gunk yet. We are trying to
sliminate the per-page flags, because there's no space for them in the
future (we'll have special handling for hwpoison because that really is
very special)
