[Qemu-devel] [PATCH] kvm_irqchip_assign_irqfd: just set irqfd in case of kvm_irqfds_enabled()

[Qemu-devel] [PATCH] kvm_irqchip_assign_irqfd: just set irqfd in case of kvm_irqfds_enabled()

[Tiejun Chen]

We should avoid to set irqfd{} unconditionally.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 kvm-all.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 18cc6b4..5b9786b 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -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);
 }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH] kvm_irqchip_assign_irqfd: just set irqfd in case of kvm_irqfds_enabled()

[Peter Maydell]

On 26 December 2014 at 08:05, Tiejun Chen <tiejun.chen@intel.com> wrote:
> We should avoid to set irqfd{} unconditionally.
>
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH] kvm_irqchip_assign_irqfd: just set irqfd in case of kvm_irqfds_enabled()

[Denis V. Lunev]

On 26/12/14 13:00, Peter Maydell wrote:
> On 26 December 2014 at 08:05, Tiejun Chen <tiejun.chen@intel.com> wrote:
>> We should avoid to set irqfd{} unconditionally.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>
> 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

Re: [Qemu-devel] [PATCH] kvm_irqchip_assign_irqfd: just set irqfd in case of kvm_irqfds_enabled()

[Peter Maydell]

On 26 December 2014 at 10:16, Denis V. Lunev <den-lists@parallels.com> wrote:
> 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

Re: [Qemu-devel] [PATCH] kvm_irqchip_assign_irqfd: just set irqfd in case of kvm_irqfds_enabled()

[Paolo Bonzini]

On 26/12/2014 18:59, Peter Maydell wrote:
> 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

Re: [Qemu-devel] [PATCH] kvm_irqchip_assign_irqfd: just set irqfd in case of kvm_irqfds_enabled()

[Chen, Tiejun]

On 2014/12/27 22:52, Paolo Bonzini wrote:
>
>
> On 26/12/2014 18:59, Peter Maydell wrote:
>> 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...
>

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