Skip to contentSkip to conversation

kvm_irqchip_assign_irqfd: just set irqfd in case of kvm_irqfds_enabled()

PATCH In review Tiejun Chen 4 participants 6 messages Dec 26, 2014 – Dec 29, 2014 KVM

lore
Share & tools
lore

Tiejun Chen

author
#0 lore raw

PATCH 1/1 kvm_irqchip_assign_irqfd: just set irqfd in case of kvm_irqfds_enabled()

We should avoid to set irqfd{} unconditionally.

Signed-off-by Tiejun Chen <[email protected]>
---
 kvm-all.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)
kvm-all.c +9 -9
index 18cc6b4..5b9786b 100644
@@ -1257,21 +1257,21 @@ int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg)
 static int kvm_irqchip_assign_irqfd(KVMState *s, int fd, int rfd, int virq,
                                     bool assign)
 {
-    struct kvm_irqfd irqfd = {
-        .fd = fd,
-        .gsi = virq,
-        .flags = assign ? 0 : KVM_IRQFD_FLAG_DEASSIGN,
-    };
+    struct kvm_irqfd irqfd = {};
+
+    if (!kvm_irqfds_enabled()) {
+        return -ENOSYS;
+    }
+
+    irqfd.fd = fd;
+    irqfd.gsi = virq;
+    irqfd.flags = assign ? 0 : KVM_IRQFD_FLAG_DEASSIGN;
 
     if (rfd != -1) {
         irqfd.flags |= KVM_IRQFD_FLAG_RESAMPLE;
         irqfd.resamplefd = rfd;
     }
 
-    if (!kvm_irqfds_enabled()) {
-        return -ENOSYS;
-    }
-
     return kvm_vm_ioctl(s, KVM_IRQFD, &irqfd);
 }
Signature · 1 line
1.9.1
#1 lore raw
Quoted 3 lines · Tiejun Chen

We should avoid to set irqfd{} unconditionally.

Signed-off-by Tiejun Chen <[email protected]>

Is there a hot path that we use this on such that the difference in code order matters at all?

thanks
-- PMM

#2 lore raw
Quoted 11 lines · Peter Maydell
Quoted 3 lines · Tiejun Chen

We should avoid to set irqfd{} unconditionally.

Signed-off-by Tiejun Chen <[email protected]>

Is there a hot path that we use this on such that the difference in code order matters at all?

thanks
-- PMM

IMHO the patch does not change anything even on hot-hot path. the declaration 'struct kvm_irqfd irqfd = {};' will result in memset inside.

Thus in order to achieve declared goal author should declare
struct kvm_irqfd irqfd;
and perform
memset(&irqfd, 0, sizeof(irqfd));
later after the check.

Regards,
Den

#3 lore raw
Quoted 10 lines · Denis V. Lunev

IMHO the patch does not change anything even on hot-hot path. the declaration 'struct kvm_irqfd irqfd = {};' will result in memset inside.

Thus in order to achieve declared goal author should declare
struct kvm_irqfd irqfd;
and perform
memset(&irqfd, 0, sizeof(irqfd));
later after the check.

Mm, but once you're into such microoptimisations as this you really need to have a good justification for the effort, in the form of profiling measurements that indicate that this is a hot path. In this case that seems pretty unlikely, because I'd expect all the systems where we care about performance will support irqfds, so we won't be taking the early-exit code path anyhow. (And not supporting irqfds is leaving much more performance on the table than we could possibly be talking about in this function.)

thanks
-- PMM

maintainer
#4 lore raw
Quoted 8 lines · Peter Maydell

Mm, but once you're into such microoptimisations as this you really need to have a good justification for the effort, in the form of profiling measurements that indicate that this is a hot path. In this case that seems pretty unlikely, because I'd expect all the systems where we care about performance will support irqfds, so we won't be taking the early-exit code path anyhow. (And not supporting irqfds is leaving much more performance on the table than we could possibly be talking about in this function.)

Also, it's even possible for a compiler to figure this out. All in all, I don't see any advantage to this patch...

Paolo

author
#5 lore raw
Quoted 12 lines · Paolo Bonzini
Quoted 8 lines · Peter Maydell

Mm, but once you're into such microoptimisations as this you really need to have a good justification for the effort, in the form of profiling measurements that indicate that this is a hot path. In this case that seems pretty unlikely, because I'd expect all the systems where we care about performance will support irqfds, so we won't be taking the early-exit code path anyhow. (And not supporting irqfds is leaving much more performance on the table than we could possibly be talking about in this function.)

Quoted 3 lines

Also, it's even possible for a compiler to figure this out. All in all, I don't see any advantage to this patch...

Indeed, its just a cleanup to make codes readable and comprehensible since oftentimes we don't initially write such a subsequent code just because we have this possibility to figure them out by the compiler, or others. And this is why I'm CCing Qemu trivial.

Tiejun