From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48224) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UarK7-0001Cn-Sc for qemu-devel@nongnu.org; Fri, 10 May 2013 13:41:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UarK5-0002ye-Nw for qemu-devel@nongnu.org; Fri, 10 May 2013 13:41:39 -0400 Received: from e23smtp04.au.ibm.com ([202.81.31.146]:39546) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UarK5-0002yJ-6t for qemu-devel@nongnu.org; Fri, 10 May 2013 13:41:37 -0400 Received: from /spool/local by e23smtp04.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Sat, 11 May 2013 03:28:47 +1000 Received: from d23relay03.au.ibm.com (d23relay03.au.ibm.com [9.190.235.21]) by d23dlp02.au.ibm.com (Postfix) with ESMTP id F04CD2BB004F for ; Sat, 11 May 2013 03:41:16 +1000 (EST) Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r4AHf8Ao21692522 for ; Sat, 11 May 2013 03:41:09 +1000 Received: from d23av03.au.ibm.com (loopback [127.0.0.1]) by d23av03.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r4AHfFRm019703 for ; Sat, 11 May 2013 03:41:15 +1000 From: Anthony Liguori In-Reply-To: <518D0B68.505@redhat.com> References: <1368188203-3407-1-git-send-email-pbonzini@redhat.com> <874nebdm4r.fsf@codemonkey.ws> <518CF130.2030402@redhat.com> <518CF4C4.6020901@suse.de> <518CF669.30803@redhat.com> <871u9ezypj.fsf@codemonkey.ws> <518D0B68.505@redhat.com> Date: Fri, 10 May 2013 12:41:07 -0500 Message-ID: <87ip2q91ho.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: mst@redhat.com, Andreas =?utf-8?Q?F=C3=A4rber?= , Aurelien Jarno , qemu-devel@nongnu.org Paolo Bonzini 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. 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. I strongly suspect that if there is a problem, that optimizing leaf/concrete casts will take care of it. Otherwise, a small lookup cache ought to do the trick too. We can even use pointer comparisons for the lookup cache which should make it extremely cheap. Disabling casts doesn't give us a long term fix. One of the solutions above does and patches also exist. So first, let's establish if there really is a performance issue here and if so, let's find a long term solution. Let's independently evaluate your proposal for 1.6. You may in fact be right that it's the right thing to do long term, but I'm quite confident that it's not the right solution to the potential issue here. > I have hardly seen any of these failures _during development_, much less > on a release. It's the most common failure that I catch in my own testing. Most often around hotplug which tends to break the most. Of course, these changes never make it into the tree which is an indication that the mechanism works very well :-) > I appreciate the advantage of type-safe casts, but in > QEMU they are a solution in search of a problem. They are cheap to > implement (though not that cheap to execute ;)) so it's perfectly fine > to have them, but they are not _needed_; disabling them is anyway a good > build-time option to have. Note that the cast macros are a big improvement in code readability. The only real replacement would be static casts which would downgrade safety. If you want to debate the merits of the safety, that's fine. > Type-safe casts make sense in GTK+/GObject where: 1) type-safe casts > return NULL and log a "critical" error, they do not abort; 2) all > functions fail with another "critical" error if they are passed NULL. > We do neither, so we're just trading a crash now for a crash very soon > after (our call stacks tend to be relatively shallow, with some > exceptions such as the block layer). The big assumption here is that dereference a NULL results in predictable failure. This is not always the case and can lead to security vulnerabilities. > Also, in GTK+/GObject the code paths are unpredictable because they > depend on user interaction, and a crash can lead to data loss. By > contrast, in QEMU most of the code is hardly ever run, and the possible > paths are very few because driver writers tend to use always the same > path. The day someone is bringing up a new guest OS and encounters such > a crash, we'll tell them to either build from git, or to use > --enable-qom-cast-debug. > > I'm sure it will be a long time before that happens... If you are concerned about the performance, provide a concrete example of poor performance and I will fix it. If we find one that is unfixable, then we should consider your proposal. Regards, Anthony Liguori > > Paolo > >> Regards, >> >> Anthony Liguori >> >>>> Either way, it would be nice to see the call sites of those >>>> most-impacting dynamic casts! So far I held back my APIC RFC since I'm >>>> not sure how to reproducibly profile things. >>> >>> It's interrupts (both sending and returning from them). >>> >>> Paolo >> >> >>