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 <tiejun.chen@intel.com>
---
 kvm-all.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

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

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

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

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

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