From: Sean Christopherson <seanjc@google.com>
To: Peter Xu <peterx@redhat.com>
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
Paolo Bonzini <pbonzini@redhat.com>,
Andrew Morton <akpm@linux-foundation.org>,
David Hildenbrand <david@redhat.com>,
"Dr . David Alan Gilbert" <dgilbert@redhat.com>,
Andrea Arcangeli <aarcange@redhat.com>,
Linux MM Mailing List <linux-mm@kvack.org>
Subject: Re: [PATCH 2/4] kvm: Merge "atomic" and "write" in __gfn_to_pfn_memslot()
Date: Thu, 23 Jun 2022 21:52:38 +0000 [thread overview]
Message-ID: <YrTgpjLrnRpqFnIa@google.com> (raw)
In-Reply-To: <YrTbKaRe497n8M0o@xz-m1.local>
On Thu, Jun 23, 2022, Peter Xu wrote:
> On Thu, Jun 23, 2022 at 08:29:13PM +0000, Sean Christopherson wrote:
> > This is what I came up with for splitting @async into a pure input (no_wait) and
> > a return value (KVM_PFN_ERR_NEEDS_IO).
>
> The attached patch looks good to me. It's just that..
>
> [...]
>
> > kvm_pfn_t __gfn_to_pfn_memslot(const struct kvm_memory_slot *slot, gfn_t gfn,
> > - bool atomic, bool *async, bool write_fault,
> > + bool atomic, bool no_wait, bool write_fault,
> > bool *writable, hva_t *hva)
>
> .. with this patch on top we'll have 3 booleans already. With the new one
> to add separated as suggested then it'll hit 4.
>
> Let's say one day we'll have that struct, but.. are you sure you think
> keeping four booleans around is nicer than having a flag, no matter whether
> we'd like to have a struct or not?
No.
> kvm_pfn_t __gfn_to_pfn_memslot(const struct kvm_memory_slot *slot, gfn_t gfn,
> bool atomic, bool no_wait, bool write_fault,
> bool interruptible, bool *writable, hva_t *hva);
>
> What if the booleans goes to 5, 6, or more?
>
> /me starts to wonder what'll be the magic number that we'll start to think
> a bitmask flag will be more lovely here. :)
For the number to really matter, it'd have to be comically large, e.g. 100+. This
is all on-stack memory, so it's as close to free as can we can get. Overhead in
terms of (un)marshalling is likely a wash for flags versus bools. Bools pack in
nicely, so until there are a _lot_ of bools, memory is a non-issue.
That leaves readability, which isn't dependent on the number so much as it is on
the usage, and will be highly subjective based on the final code.
In other words, I'm not dead set against flags, but I would like to see a complete
cleanup before making a decision. My gut reaction is to use bools, as it makes
consumption cleaner in most cases, e.g.
if (!(xxx->write_fault || writable))
return false;
versus
if (!((xxx->flags & KVM_GTP_WRITE) || writable))
return false;
but again I'm not going to say never until I actually see the end result.
next prev parent reply other threads:[~2022-06-23 21:52 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-22 21:36 [PATCH 0/4] kvm/mm: Allow GUP to respond to non fatal signals Peter Xu
2022-06-22 21:36 ` [PATCH 1/4] mm/gup: Add FOLL_INTERRUPTIBLE Peter Xu
2022-06-25 0:35 ` Jason Gunthorpe
2022-06-25 1:23 ` Peter Xu
2022-06-25 23:59 ` Jason Gunthorpe
2022-06-27 15:29 ` Peter Xu
2022-06-28 2:07 ` John Hubbard
2022-06-28 19:31 ` Peter Xu
2022-06-28 21:40 ` John Hubbard
2022-06-28 22:33 ` Peter Xu
2022-06-29 0:31 ` John Hubbard
2022-06-29 15:47 ` Peter Xu
2022-06-30 1:53 ` John Hubbard
2022-06-30 13:49 ` Peter Xu
2022-06-30 19:01 ` John Hubbard
2022-06-30 21:27 ` Peter Xu
2022-07-04 22:48 ` Matthew Wilcox
2022-07-07 15:06 ` Peter Xu
2022-06-22 21:36 ` [PATCH 2/4] kvm: Merge "atomic" and "write" in __gfn_to_pfn_memslot() Peter Xu
2022-06-23 14:49 ` Sean Christopherson
2022-06-23 19:46 ` Peter Xu
2022-06-23 20:29 ` Sean Christopherson
2022-06-23 21:29 ` Peter Xu
2022-06-23 21:52 ` Sean Christopherson [this message]
2022-06-27 19:12 ` John Hubbard
2022-06-28 2:17 ` John Hubbard
2022-06-28 19:46 ` Peter Xu
2022-06-28 21:52 ` John Hubbard
2022-06-28 22:50 ` Peter Xu
2022-06-28 22:55 ` John Hubbard
2022-06-28 23:02 ` Peter Xu
2022-06-22 21:36 ` [PATCH 3/4] kvm: Add new pfn error KVM_PFN_ERR_INTR Peter Xu
2022-06-23 14:31 ` Sean Christopherson
2022-06-23 19:32 ` Peter Xu
2022-06-22 21:36 ` [PATCH 4/4] kvm/x86: Allow to respond to generic signals during slow page faults Peter Xu
2022-06-23 14:46 ` Sean Christopherson
2022-06-23 19:31 ` Peter Xu
2022-06-23 20:07 ` Sean Christopherson
2022-06-23 20:18 ` Peter Xu
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=YrTgpjLrnRpqFnIa@google.com \
--to=seanjc@google.com \
--cc=aarcange@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=david@redhat.com \
--cc=dgilbert@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=pbonzini@redhat.com \
--cc=peterx@redhat.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 NNTP newsgroup(s).