lkml.org 
[lkml]   [2022]   [Jun]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 2/4] kvm: Merge "atomic" and "write" in __gfn_to_pfn_memslot()
From
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

\
 
 \ /
  Last update: 2022-06-27 21:14    [W:0.259 / U:0.240 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site