| Message ID | 1419581126-12927-1-git-send-email-tiejun.chen@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); }
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(-)