Sean Christopherson <[email protected]> writes:
> On Mon, Mar 02, 2026, Vlastimil Babka wrote:
>> On 2/25/26 08:20, Ackerley Tng wrote:
>> > __filemap_get_folio_mpol() is parametrized by a bunch of GFP flags, which
>>
>> FGP?
>>
>> > adds complexity for the reader. Since guest_memfd doesn't meaningfully use
>> > any of the other FGP flags, undo that complexity by directly calling
>> > filemap_alloc_folio().
>> >
>> > Directly calling filemap_alloc_folio() also allows the order of 0 to be
>> > explicitly specified, which is the only order guest_memfd supports. This is
>> > easier to understand,
>
> That's debatable. IMO, one isn't clearly better than the other, especially
> since
> filemap_lock_folio() is itself a wrapper for __filemap_get_folio_mpol(). And
> there
> is a cost to open-coding, as it means we risk missing something if there's a
> change
> in __filemap_get_folio_mpol() that's beneficial to guest_memfd.
>
> As Vlastimil said, if this greatly simplifies accounting, then I'm ok with it.
> But the changelog needs to focus on that aspect, because I don't see this as a
> clear win versus using __filemap_get_folio_mpol().
>
FGF_GET_ORDER() indeed caps the order at 0. I was overly focused on the
earlier line where it did mapping_min_folio_order(), where I thought
other code could possibly influence the eventual order.
I'll revert to __filemap_get_folio_mpol() in the next version and see
how that goes. Thanks!
> And if we go through with this, we should probably revert 16a542e22339
> ("mm/filemap:
> Extend __filemap_get_folio() to support NUMA memory policies"), because
> guest_memfd
> is/was the only user.
>
>> > +static struct folio *__kvm_gmem_get_folio(struct inode *inode, pgoff_t
>> > index)
>> > +{
>> > + /* TODO: Support huge pages. */
>> > + struct mempolicy *policy;
>> > + struct folio *folio;
>> > + gfp_t gfp;
>> > + int ret;
>> > +
>> > + /*
>> > + * Fast-path: See if folio is already present in mapping to avoid
>> > + * policy_lookup.
>> > + */
>> > + folio = filemap_lock_folio(inode->i_mapping, index);
>> > + if (!IS_ERR(folio))
>> > + return folio;
>> > +
>> > + gfp = mapping_gfp_mask(inode->i_mapping);
>> > +
>> > + policy = mpol_shared_policy_lookup(&GMEM_I(inode)->policy, index);
>
> This is a potential performance regression. Previously, KVM would do a policy
> lookup once per retry loop. Now KVM will do the lookup
>
> I doubt it will matter in practice, because on EEXIST filemap_lock_folio()
> should
> be all but guaranteed to find the existing folio. But it's also something
> that
> should be easy enough to avoid, and it's also another argument for using
> __filemap_get_folio_mpol() instead of open coding our own version.
