Re: [PATCH 2/4] kvm: Merge "atomic" and "write" in __gfn_to_pfn_memslot()

[John Hubbard]

On 6/23/22 14:52, Sean Christopherson wrote:
> 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.

It's pretty unusual to see that claim, in kernel mm code. :) Flags are often
used, because they take less space than booleans, and C bitfields have other
problems.

> 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.

Just to add a light counter-argument: the readability is similar enough that
I think the compactness in memory makes flags a little better. imho anyway.


thanks,
--
John Hubbard
NVIDIA