Messages in this thread | | | Date | Mon, 27 Jun 2022 12:12:40 -0700 | Subject | Re: [PATCH 2/4] kvm: Merge "atomic" and "write" in __gfn_to_pfn_memslot() | From | 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
| |