From: David Stevens <stevensd@chromium.org>
To: Sean Christopherson <seanjc@google.com>
Cc: kvmarm@lists.linux.dev, linux-kernel@vger.kernel.org,
kvm@vger.kernel.org
Subject: Re: [PATCH v9 0/6] KVM: allow mapping non-refcounted pages
Date: Wed, 21 Feb 2024 15:05:28 +0900 [thread overview]
Message-ID: <CAD=HUj5fO9QCaMJhiJdzQsMPnVSRvM6T9RLYqE03_dEfzeQmtw@mail.gmail.com> (raw)
In-Reply-To: <ZcrkhTn1Da5-vND2@google.com>
On Tue, Feb 13, 2024 at 12:39 PM Sean Christopherson <seanjc@google.com> 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@google.com> 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
prev parent reply other threads:[~2024-02-21 6:05 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-11 2:16 [PATCH v9 0/6] KVM: allow mapping non-refcounted pages David Stevens
2023-09-11 2:16 ` [PATCH v9 1/6] KVM: Assert that a page's refcount is elevated when marking accessed/dirty David Stevens
2023-09-11 2:16 ` [PATCH v9 2/6] KVM: mmu: Introduce __kvm_follow_pfn function David Stevens
2023-10-03 16:54 ` Maxim Levitsky
2024-02-06 1:25 ` Sean Christopherson
2024-02-06 3:16 ` Sean Christopherson
2024-02-13 3:27 ` Sean Christopherson
2024-02-13 3:44 ` Sean Christopherson
2023-09-11 2:16 ` [PATCH v9 3/6] KVM: mmu: Improve handling of non-refcounted pfns David Stevens
2023-10-03 16:54 ` Maxim Levitsky
2024-02-06 2:54 ` Sean Christopherson
2024-02-13 3:44 ` Sean Christopherson
2023-09-11 2:16 ` [PATCH v9 4/6] KVM: Migrate kvm_vcpu_map to __kvm_follow_pfn David Stevens
2023-10-03 16:54 ` Maxim Levitsky
2023-09-11 2:16 ` [PATCH v9 5/6] KVM: x86: Migrate " David Stevens
2023-10-03 16:54 ` Maxim Levitsky
2023-10-03 20:58 ` Sean Christopherson
2023-09-11 2:16 ` [PATCH v9 6/6] KVM: x86/mmu: Handle non-refcounted pages David Stevens
2023-09-18 9:53 ` Dmitry Osipenko
2023-09-19 2:25 ` David Stevens
2023-09-30 13:34 ` Dmitry Osipenko
2023-09-18 9:58 ` Dmitry Osipenko
2023-09-18 11:19 ` Dmitry Osipenko
2023-09-19 2:59 ` David Stevens
2023-09-21 20:06 ` Dmitry Osipenko
2023-09-30 13:34 ` Dmitry Osipenko
2023-09-19 2:31 ` David Stevens
2023-09-21 20:04 ` Dmitry Osipenko
2024-02-06 3:02 ` Sean Christopherson
2023-10-03 16:54 ` Maxim Levitsky
2024-02-06 3:23 ` Sean Christopherson
2023-09-29 5:19 ` [PATCH v9 0/6] KVM: allow mapping " Christoph Hellwig
2023-09-29 16:06 ` Sean Christopherson
2023-10-02 6:25 ` Christoph Hellwig
2024-02-06 3:29 ` Sean Christopherson
2023-10-31 4:30 ` David Stevens
2023-10-31 14:30 ` Sean Christopherson
2023-12-12 1:59 ` David Stevens
2023-12-20 1:37 ` Sean Christopherson
2024-02-06 3:30 ` Sean Christopherson
2024-02-13 3:39 ` Sean Christopherson
2024-02-21 6:05 ` David Stevens [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAD=HUj5fO9QCaMJhiJdzQsMPnVSRvM6T9RLYqE03_dEfzeQmtw@mail.gmail.com' \
--to=stevensd@chromium.org \
--cc=kvm@vger.kernel.org \
--cc=kvmarm@lists.linux.dev \
--cc=linux-kernel@vger.kernel.org \
--cc=seanjc@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).