[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <284a4fcc-3618-4ba6-dfaa-ffc4039eefcc@maciej.szmigiero.name>
Date: Thu, 21 Oct 2021 23:44:50 +0200
From: "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>,
Vitaly Kuznetsov <vkuznets@...hat.com>,
Wanpeng Li <wanpengli@...cent.com>,
Jim Mattson <jmattson@...gle.com>,
Igor Mammedov <imammedo@...hat.com>,
Marc Zyngier <maz@...nel.org>,
James Morse <james.morse@....com>,
Julien Thierry <julien.thierry.kdev@...il.com>,
Suzuki K Poulose <suzuki.poulose@....com>,
Huacai Chen <chenhuacai@...nel.org>,
Aleksandar Markovic <aleksandar.qemu.devel@...il.com>,
Paul Mackerras <paulus@...abs.org>,
Christian Borntraeger <borntraeger@...ibm.com>,
Janosch Frank <frankja@...ux.ibm.com>,
David Hildenbrand <david@...hat.com>,
Cornelia Huck <cohuck@...hat.com>,
Claudio Imbrenda <imbrenda@...ux.ibm.com>,
Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 12/13] KVM: Optimize gfn lookup in kvm_zap_gfn_range()
On 21.10.2021 18:30, Sean Christopherson wrote:
> On Thu, Oct 21, 2021, Maciej S. Szmigiero wrote:
>> On 21.10.2021 01:47, Sean Christopherson wrote:
>>> In this case, I would honestly just drop the helper. It's really hard to express
>>> what this function does in a name that isn't absurdly long, and there's exactly
>>> one user at the end of the series.
>>
>> The "upper bound" is a common name for a binary search operation that
>> finds the first node that has its key strictly greater than the searched key.
>
> Ah, that I did not know (obviously). But I suspect that detail will be lost on
> other readers as well, even if they are familiar with the terminology.
>
>> It can be integrated into its caller but I would leave a comment there
>> describing what kind of operation that block of code does to aid in
>> understanding the code.
>
> Yeah, completely agree a comment would be wonderful.
👍
>> Although, to be honest, I don't quite get the reason for doing this
>> considering that you want to put a single "rb_next()" call into its own
>> helper for clarity below.
>
> The goal is to make the macro itself easy to understand, even if the reader may
> not understand the underlying details. The bare rb_next() forces the reader to
> pause to think about exactly what "node" is, and perhaps even dive into the code
> for the other helpers.
>
> With something like this, a reader that doesn't know the memslots details can
> get a good idea of the basic gist of the macro without having to even know the
> type of "node". Obviously someone writing code will need to know the type, but
> for readers bouncing around code it's a detail they don't need to know.
>
> #define kvm_for_each_memslot_in_gfn_range(node, slots, start, end) \
> for (node = kvm_get_first_node(slots, start); \
> !kvm_is_valid_node(slots, node, end); \
> node = kvm_get_next_node(node))
>
> Hmm, on that point, having the caller do
>
> memslot = container_of(node, struct kvm_memory_slot, gfn_node[idx]);
>
> is more than a bit odd, and as is the case with the bare rb_next(), bleeds
> implementation details into code that really doesn't care about implementation
> details. Eww, and looking closer, the caller also needs to grab slots->node_idx.
>
> So while I would love to avoid an opaque iterator, adding one would be a net
> positive in this case. E.g.
>
> /* Iterator used for walking memslots that overlap a gfn range. */
> struct kvm_memslot_iterator iter {
> struct rb_node *node;
> struct kvm_memory_slot *memslot;
> struct kvm_memory_slots *slots;
> gfn_t start;
> gfn_t end;
> }
>
> static inline void kvm_memslot_iter_start(struct kvm_memslot_iter *iter,
> struct kvm_memslots *slots,
> gfn_t start, gfn_t end)
> {
> ...
> }
>
> static inline bool kvm_memslot_iter_is_valid(struct kvm_memslot_iter *iter)
> {
> /*
> * If this slot starts beyond or at the end of the range so does
> * every next one
> */
> return iter->node && iter->memslot->base_gfn < end;
> }
>
> static inline void kvm_memslot_iter_next(struct kvm_memslot_iter *iter)
> {
> iter->node = rb_next(iter->node);
>
> if (!iter->node)
> return;
>
> iter->memslot = container_of(iter->node, struct kvm_memory_slot,
> gfn_node[iter->slots->node_idx]);
> }
>
> /* Iterate over each memslot *possibly* intersecting [start, end) range */
> #define kvm_for_each_memslot_in_gfn_range(iter, node, slots, start, end) \
> for (kvm_memslot_iter_start(iter, node, slots, start, end); \
> kvm_memslot_iter_is_valid(iter); \
> kvm_memslot_iter_next(node)) \
>
The iterator-based for_each implementation looks pretty nice (love the
order and consistency that higher-level abstractions bring to code) -
will change the code to use iterators instead.
It also solves the kvm_is_valid_node() naming issue below.
> Ugh, this got me looking at kvm_zap_gfn_range(), and that thing is trainwreck.
> There are three calls kvm_flush_remote_tlbs_with_address(), two of which should
> be unnecessary, but become necessary because the last one is broken. *sigh*
>
> That'd also be a good excuse to extract the rmap loop to a separate helper. Then
> you don't need to constantly juggle the 80 char limit and variable collisions
> while you're modifying this mess. I'll post the attached patches separately
> since the first one (two?) should go into 5.15. They're compile tested only
> at this point, but hopefully I've had enough coffee and they're safe to base
> this series on top (note, they're based on kvm/queue, commit 73f122c4f06f ("KVM:
> cleanup allocation of rmaps and page tracking data").
All right, will make sure that a respin is based on a kvm tree with these
commits in.
>>> The kvm_for_each_in_gfn prefix is _really_ confusing. I get that these are all
>>> helpers for "kvm_for_each_memslot...", but it's hard not to think these are all
>>> iterators on their own. I would gladly sacrifice namespacing for readability in
>>> this case.
>>
>> "kvm_for_each_memslot_in_gfn_range" was your proposed name here:
>> https://lore.kernel.org/kvm/YK6GWUP107i5KAJo@google.com/
>>
>> But no problem renaming it.
>
> Oh, I was commenting on the inner helpers. The macro name itself is great. ;-)
>
>>> @@ -882,12 +875,16 @@ struct rb_node *kvm_for_each_in_gfn_first(struct kvm_memslots *slots, gfn_t star
>>> return node;
>>> }
>>>
>>> -static inline
>>> -bool kvm_for_each_in_gfn_no_more(struct kvm_memslots *slots, struct rb_node *node, gfn_t end)
>>> +static inline bool kvm_is_last_node(struct kvm_memslots *slots,
>>> + struct rb_node *node, gfn_t end)
>>
>> kvm_is_last_node() is a bit misleading since this function is supposed
>> to return true even on the last node, only returning false one node past
>> the last one (or when the tree runs out of nodes).
>
> Good point. I didn't love the name when I suggested either. What about
> kvm_is_valid_node()?
kvm_is_valid_node() sounds a bit too generic for me, but since we rewrite
the code to be iterator-based this issue goes away.
Thanks,
Maciej
Powered by blists - more mailing lists