| Message ID | 20230602161921.208564-8-amoorthy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve scalability of KVM + userfaultfd live migration via annotated memory faults. | expand |
On Fri, Jun 02, 2023, Anish Moorthy wrote: > KVM_HVA_ERR_RO_BAD satisfies kvm_is_error_hva(), so there's no need to > duplicate the "if (writable)" block. Fix this by bringing all > kvm_is_error_hva() cases under one conditional. > > Signed-off-by: Anish Moorthy <amoorthy@google.com> > --- > virt/kvm/kvm_main.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index b9d2606f9251..05d6e7e3994d 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -2711,16 +2711,14 @@ kvm_pfn_t __gfn_to_pfn_memslot(const struct kvm_memory_slot *slot, gfn_t gfn, > if (hva) > *hva = addr; > > - if (addr == KVM_HVA_ERR_RO_BAD) { > - if (writable) > - *writable = false; > - return KVM_PFN_ERR_RO_FAULT; > - } > - > if (kvm_is_error_hva(addr)) { > if (writable) > *writable = false; > - return KVM_PFN_NOSLOT; > + > + if (addr == KVM_HVA_ERR_RO_BAD) > + return KVM_PFN_ERR_RO_FAULT; > + else > + return KVM_PFN_NOSLOT; Similar to an earlier patch, preferred style for terminal if-statements is if (addr == KVM_HVA_ERR_RO_BAD) return KVM_PFN_ERR_RO_FAULT; return KVM_PFN_NOSLOT; Again, there are reasons for the style/rule. In this case, it will yield a smaller diff (obviously not a huge deal, but helpful), and it makes it more obvious that the taken path of "if (kvm_is_error_hva(addr))" is itself terminal. Alternatively, a ternary operator is often used for these types of things, though in this case I much prefer the above, as I find the below hard to read. return addr == KVM_HVA_ERR_RO_BAD ? KVM_PFN_ERR_RO_FAULT : KVM_PFN_NOSLOT;
Done (somebody please let me know if these short "ack"/"done" messages are frowned upon btw. Nobody's complained about it so far, but I'm not sure if people consider it spam)
On Fri, Jul 07, 2023, Anish Moorthy wrote: > Done > > (somebody please let me know if these short "ack"/"done" messages are > frowned upon btw. Nobody's complained about it so far, but I'm not > sure if people consider it spam) I personally think that ack/done messages that don't add anything else to the conversation are useless. The bar for "anything else" can be very low, e.g. a simple "gotcha" can be valuable if it wraps up a conversation, but "accepting" every piece of feedback is a waste of everyone's time IMO as the expectation is that all review feedback will be addressed, either by a follow-up conversation or by modifying the patch in the next version, i.e. by *not* pushing back you are implicitly accepting feedback. And an "ack/done" isn't binding, i.e. doesn't magically morph into code and guarantee that the next version of the patch will actually contain the requested changes.
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index b9d2606f9251..05d6e7e3994d 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -2711,16 +2711,14 @@ kvm_pfn_t __gfn_to_pfn_memslot(const struct kvm_memory_slot *slot, gfn_t gfn, if (hva) *hva = addr; - if (addr == KVM_HVA_ERR_RO_BAD) { - if (writable) - *writable = false; - return KVM_PFN_ERR_RO_FAULT; - } - if (kvm_is_error_hva(addr)) { if (writable) *writable = false; - return KVM_PFN_NOSLOT; + + if (addr == KVM_HVA_ERR_RO_BAD) + return KVM_PFN_ERR_RO_FAULT; + else + return KVM_PFN_NOSLOT; } /* Do not map writable pfn in the readonly memslot. */
KVM_HVA_ERR_RO_BAD satisfies kvm_is_error_hva(), so there's no need to duplicate the "if (writable)" block. Fix this by bringing all kvm_is_error_hva() cases under one conditional. Signed-off-by: Anish Moorthy <amoorthy@google.com> --- virt/kvm/kvm_main.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)