From: "Maciej S. Szmigiero" <mail@maciej.szmigiero.name>
To: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
Vitaly Kuznetsov <vkuznets@redhat.com>,
Wanpeng Li <wanpengli@tencent.com>,
Jim Mattson <jmattson@google.com>,
Igor Mammedov <imammedo@redhat.com>,
Marc Zyngier <maz@kernel.org>, James Morse <james.morse@arm.com>,
Julien Thierry <julien.thierry.kdev@gmail.com>,
Suzuki K Poulose <suzuki.poulose@arm.com>,
Huacai Chen <chenhuacai@kernel.org>,
Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>,
Paul Mackerras <paulus@ozlabs.org>,
Christian Borntraeger <borntraeger@de.ibm.com>,
Janosch Frank <frankja@linux.ibm.com>,
David Hildenbrand <david@redhat.com>,
Cornelia Huck <cohuck@redhat.com>,
Claudio Imbrenda <imbrenda@linux.ibm.com>,
Joerg Roedel <joro@8bytes.org>,
kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 12/13] KVM: Optimize gfn lookup in kvm_zap_gfn_range()
Date: Thu, 21 Oct 2021 23:44:50 +0200 [thread overview]
Message-ID: <284a4fcc-3618-4ba6-dfaa-ffc4039eefcc@maciej.szmigiero.name> (raw)
In-Reply-To: <YXGVwlNxaibZAnmC@google.com>
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
next prev parent reply other threads:[~2021-10-21 21:45 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-20 21:38 [PATCH v5 00/13] KVM: Scalable memslots implementation Maciej S. Szmigiero
2021-09-20 21:38 ` [PATCH v5 01/13] KVM: x86: Cache total page count to avoid traversing the memslot array Maciej S. Szmigiero
2021-10-19 22:24 ` Sean Christopherson
2021-10-19 22:31 ` Sean Christopherson
2021-10-20 18:40 ` Maciej S. Szmigiero
2021-10-20 18:41 ` Maciej S. Szmigiero
2021-10-20 19:01 ` Sean Christopherson
2021-11-01 22:29 ` Sean Christopherson
2021-11-03 11:59 ` Maciej S. Szmigiero
2021-11-03 14:47 ` Sean Christopherson
2021-11-03 15:38 ` Maciej S. Szmigiero
2021-09-20 21:38 ` [PATCH v5 02/13] KVM: x86: Don't call kvm_mmu_change_mmu_pages() if the count hasn't changed Maciej S. Szmigiero
2021-09-20 21:38 ` [PATCH v5 03/13] KVM: Add "old" memslot parameter to kvm_arch_prepare_memory_region() Maciej S. Szmigiero
2021-09-20 21:38 ` [PATCH v5 04/13] KVM: x86: Move n_memslots_pages recalc " Maciej S. Szmigiero
2021-10-19 22:38 ` Sean Christopherson
2021-10-20 18:41 ` Maciej S. Szmigiero
2021-09-20 21:38 ` [PATCH v5 05/13] KVM: Integrate gfn_to_memslot_approx() into search_memslots() Maciej S. Szmigiero
2021-10-19 23:38 ` Sean Christopherson
2021-10-20 18:41 ` Maciej S. Szmigiero
2021-09-20 21:38 ` [PATCH v5 06/13] KVM: Move WARN on invalid memslot index to update_memslots() Maciej S. Szmigiero
2021-10-19 23:42 ` Sean Christopherson
2021-09-20 21:38 ` [PATCH v5 07/13] KVM: Just resync arch fields when slots_arch_lock gets reacquired Maciej S. Szmigiero
2021-10-19 23:55 ` Sean Christopherson
2021-10-20 18:41 ` Maciej S. Szmigiero
2021-10-20 18:57 ` Sean Christopherson
2021-10-20 18:58 ` Maciej S. Szmigiero
2021-09-20 21:38 ` [PATCH v5 08/13] KVM: Resolve memslot ID via a hash table instead of via a static array Maciej S. Szmigiero
2021-10-20 0:43 ` Sean Christopherson
2021-10-20 18:42 ` Maciej S. Szmigiero
2021-10-20 22:39 ` Sean Christopherson
2021-10-21 14:15 ` Maciej S. Szmigiero
2021-09-20 21:38 ` [PATCH v5 09/13] KVM: Use interval tree to do fast hva lookup in memslots Maciej S. Szmigiero
2021-10-26 18:19 ` Sean Christopherson
2021-10-26 18:46 ` Maciej S. Szmigiero
2021-09-20 21:38 ` [PATCH v5 10/13] KVM: s390: Introduce kvm_s390_get_gfn_end() Maciej S. Szmigiero
2021-09-20 21:38 ` [PATCH v5 11/13] KVM: Keep memslots in tree-based structures instead of array-based ones Maciej S. Szmigiero
2021-10-27 0:36 ` Sean Christopherson
2021-10-27 23:54 ` Sean Christopherson
2021-10-28 22:22 ` Sean Christopherson
2021-09-20 21:39 ` [PATCH v5 12/13] KVM: Optimize gfn lookup in kvm_zap_gfn_range() Maciej S. Szmigiero
2021-10-20 23:47 ` Sean Christopherson
2021-10-21 14:16 ` Maciej S. Szmigiero
2021-10-21 16:30 ` Sean Christopherson
2021-10-21 21:44 ` Maciej S. Szmigiero [this message]
2021-09-20 21:39 ` [PATCH v5 13/13] KVM: Optimize overlapping memslots check Maciej S. Szmigiero
2021-10-26 18:59 ` Sean Christopherson
2021-10-27 13:48 ` Maciej S. Szmigiero
2021-10-28 17:53 ` Sean Christopherson
2021-10-29 16:23 ` Maciej S. Szmigiero
2021-10-30 0:32 ` Sean Christopherson
2021-10-19 22:07 ` [PATCH v5 00/13] KVM: Scalable memslots implementation Sean Christopherson
2021-10-20 18:40 ` Maciej S. Szmigiero
2021-10-20 19:58 ` Sean Christopherson
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=284a4fcc-3618-4ba6-dfaa-ffc4039eefcc@maciej.szmigiero.name \
--to=mail@maciej.szmigiero.name \
--cc=aleksandar.qemu.devel@gmail.com \
--cc=borntraeger@de.ibm.com \
--cc=chenhuacai@kernel.org \
--cc=cohuck@redhat.com \
--cc=david@redhat.com \
--cc=frankja@linux.ibm.com \
--cc=imammedo@redhat.com \
--cc=imbrenda@linux.ibm.com \
--cc=james.morse@arm.com \
--cc=jmattson@google.com \
--cc=joro@8bytes.org \
--cc=julien.thierry.kdev@gmail.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maz@kernel.org \
--cc=paulus@ozlabs.org \
--cc=pbonzini@redhat.com \
--cc=seanjc@google.com \
--cc=suzuki.poulose@arm.com \
--cc=vkuznets@redhat.com \
--cc=wanpengli@tencent.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).