On Tue, Feb 13, 2024 at 12:39 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> On Mon, Feb 05, 2024, Sean Christopherson wrote:
> > On Tue, Dec 19, 2023, Sean Christopherson wrote:
> > > On Tue, Dec 12, 2023, David Stevens wrote:
> > > > On Tue, Oct 31, 2023 at 11:30 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Tue, Oct 31, 2023, David Stevens wrote:
> > > > > > Sean, have you been waiting for a new patch series with responses to
> > > > > > Maxim's comments? I'm not really familiar with kernel contribution
> > > > > > etiquette, but I was hoping to get your feedback before spending the
> > > > > > time to put together another patch series.
> > > > >
> > > > > No, I'm working my way back toward it. The guest_memfd series took precedence
> > > > > over everything that I wasn't confident would land in 6.7, i.e. larger series
> > > > > effectively got put on the back burner. Sorry :-(
> > > >
> > > > Is this series something that may be able to make it into 6.8 or 6.9?
> > >
> > > 6.8 isn't realistic. Between LPC, vacation, and non-upstream stuff, I've done
> > > frustratingly little code review since early November. Sorry :-(
> > >
> > > I haven't paged this series back into memory, so take this with a grain of salt,
> > > but IIRC there was nothing that would block this from landing in 6.9. Timing will
> > > likely be tight though, especially for getting testing on all architectures.
> >
> > I did a quick-ish pass today. If you can hold off on v10 until later this week,
> > I'll try to take a more in-depth look by EOD Thursday.
>
> So I took a "deeper" look, but honestly it wasn't really any more in-depth than
> the previous look. I think I was somewhat surprised at the relatively small amount
> of churn this ended up requiring.
>
> Anywho, no major complaints. This might be fodder for 6.9? Maybe. It'll be
> tight. At the very least though, I expect to shove v10 in a branch and start
> beating on it in anticipation of landing it no later than 6.10.
>
> One question though: what happened to the !FOLL_GET logic in kvm_follow_refcounted_pfn()?
>
> In a previous version, I suggested:
>
> static kvm_pfn_t kvm_follow_refcounted_pfn(struct kvm_follow_pfn *foll,
> struct page *page)
> {
> kvm_pfn_t pfn = page_to_pfn(page);
>
> foll->is_refcounted_page = true;
>
> /*
> * FIXME: Ideally, KVM wouldn't pass FOLL_GET to gup() when the caller
> * doesn't want to grab a reference, but gup() doesn't support getting
> * just the pfn, i.e. FOLL_GET is effectively mandatory. If that ever
> * changes, drop this and simply don't pass FOLL_GET to gup().
> */
> if (!(foll->flags & FOLL_GET))
> put_page(page);
>
> return pfn;
> }
>
> but in v9 it's simply:
>
> static kvm_pfn_t kvm_follow_refcounted_pfn(struct kvm_follow_pfn *foll,
> struct page *page)
> {
> kvm_pfn_t pfn = page_to_pfn(page);
>
> foll->is_refcounted_page = true;
> return pfn;
> }
>
> And instead the x86 page fault handlers manually drop the reference. Why is that?
I don't think FOLL_GET adds much to the API if is_refcounted_page is
present. At least right now, all of the callers need to pay attention
to is_refcounted_page so that they can update the access/dirty flags
properly. If they already have to do that anyway, then it's not any
harder for them to call put_page(). Not taking a reference also adds
one more footgun to the API, since the caller needs to make sure it
only accesses the page under an appropriate lock (v7 of this series
had that bug).
That said, I don't have particularly strong feelings one way or the
other, so I've added it back to v10 of the series.
-David
