Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Anthony Liguori <aliguori@us.ibm.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 10/05/2013 19:41, Anthony Liguori ha scritto:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> Il 10/05/2013 16:39, Anthony Liguori ha scritto:
>>>> I just oppose the notion of disabling casts and *especially* only
>>>> disabling casts for official builds.
>>>
>>> This actually happens all the time.  Exactly this kind of type-safe cast
>>> is disabled in releases of GCC, but enabled when building from svn trunk.
>> 
>> Let's assume for a moment that you are right and this behavior is what
>> we should have.  Let's also assume there is a real regression here
>> which has yet to have been established.
>
> Aurelien timed the effect of my patch two hours before you sent this
> message.  If it's not a regression in 1.5 (which is quite obvious from
> the profile), it is a regression from the introduction of CPU classes
> (1.3 or 1.4), when this code didn't exist at all.
>
> And in 1.5 we introduced virtio-net casts as well (or did mst sneak in
> his change anyway? ;)). If 10% is the effect of a few hundred
> interrupts/sec, perhaps the same effect is visible on a few thousand
> packets/sec.  I wouldn't bet against that one week from release.

The thing is, none of these casts should be for more than 1 level and
the patch I provided makes those casts almost free.

I believe the reason it didn't fix the problem for Aurelien is because
the string addresses were different because of different compilation
units.  I guess binutils doesn't collapse strings when linking.

>> As soon as we open up 1.6 for development, we face an immediate
>> regression in performance.  So we need to fix the real problem here
>> anyway.
>
> No, we don't.  We will simply have to live with a debugging vs.
> production tradeoff.

I appreciate all of the arguments below.  I'm very concerned about
reducing safety checks but am sympathetic to performance concerns.

Here's what I would like to do.  I'll apply 1-6 of your series which
gives us the infrastructure to disable qom casts.  That at least let's
the code get tested in case we need it.

I will hold off applying patch 7 hoping that the patch I provided to
Aurelien solves the problem.  If it doesn't and we're unable to find a
better solution, I'll apply patch 7 for the release.

Either way, the infrastructure is there for a distro to make a decision
although I think disabling qom casts is an extremely bad decision to
make.

Given the v2 version of my patch, I'm quite confident that casting in
the vast majority of circumstances would avoid a g_hash_table lookup so
that should eliminate any performance concerns.

Regards,

Anthony Liguori