Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/3] pseries: Add DPRINTF macros to spapr pci code

[Blue Swirl]

On Wed, Apr 11, 2012 at 02:02, David Gibson <address@hidden> wrote:
> On Wed, Apr 04, 2012 at 08:34:52PM +0000, Blue Swirl wrote:
>> On Wed, Apr 4, 2012 at 20:11, Peter Maydell <address@hidden> wrote:
>> > On 4 April 2012 20:18, Blue Swirl <address@hidden> wrote:
>> >> On Mon, Apr 2, 2012 at 04:17, David Gibson <address@hidden> wrote:
>> >>> From: Alexey Kardashevskiy <address@hidden>
>> >>> This adds DPRINTF() macros with the usual conventions to the spapr_pci
>> >>> code.
>> >>
>> >> Please use tracepoints instead of printf statements. Tracing is more
>> >> flexible, more efficient and does not suffer from bitrot.
>> >
>> > I'd much rather enable a #define to turn on debugging than faff about
>> > with tracing. It's simple and straightforward, you can do it with a
>> > single obvious change and recompile, and nobody has to look up
>> > documentation to figure out how it works.
>>
>> Laziness. Even the built in help should be sufficient to start using
>> tracepoints.
>>
>> > If tracepoints were always-compiled-in and enabled at runtime I'd
>> > agree with you: then you could have linux-kernel-style "enable debug
>> > tracing", "enable warnings about odd guest behaviour", "be silent",
>> > etc. But they're not, so they don't gain anything over a simple
>> > DPRINTF for programmer debugging.
>>
>> False. It's easy to compile in tracepoints
>> (--enable-trace-backend=simple) and the overhead is zero or marginal.
>> It's not possible to enable or disable DPRINTFs at run time easily
>> which is trivial with tracepoints. When the tracepoints are enabled,
>> due to binary format they are faster than text output.  There's no
>> need to relink the files when triggering on and off tracepoints. Trace
>> files are much more compact than text. Processing is offline. Delta
>> timestamps are provided. Tracepoints do not suffer from bitrot. From
>> programming standpoint, DPRINTF is changed to a function call and the
>> format text is put to a different file, that's it.
>>
>> Have you ever waded through hundreds of megabytes of DPRINTF stuff or
>> other logs, 99.99999% (literally, only a few bytes were interesting!)
>> of it not what you want, just because the interesting stuff happens to
>> be near to end of it? Even 'vi' is slow when handling these kind of
>> files.
>>
>> Just for fun, try also enabling all DPRINTFs (small sed script
>> exercise left to reader) and see how many breakages there are due to
>> bit rotted DPRINTFs.
>
> Madness.  Tracepoints may well be useful, but dprintfs are simple,
> used in projects all over the place and instantly understandable by
> developers.

Not madness, but its opposite. Tracepoints are simple too. The goal of
QEMU is efficiency and technical excellence over simplicity of code.
Why perform tedious code generation, maintain a dedicated kernel
module or coalesce MMIO accesses when a simple switch() evaluating
each instruction would be much simpler? Easy life for developers or
catering for random developers from the street are not a priority in
this project. Though we're also not talking about any obfuscation
contest with tracepoints either and the tracepoints are a powerful
tool for developers.

> Yes, I've frequently waded through hundred megabyte debug logs to find
> the few useful bugs.  I'd do it again without hesitation.  The logs
> may be big, but they're transient and grep works very well, so I
> really don't care.  Not to mention that the fact you can switch on the
> dprintfs just for the code you're looking at means they're frequently
> not that big in the first place.

Tracepoints would work also in this case and the ability to turn on
any tracepoints at run time makes the logs and dumps to be examined
even smaller.

> Bitrot is also not a big problem.  If you're enabling dprintfs, almost
> by definition, you're already hacking the code they appear in, so
> fixing some bitrotted format specifiers is rarely more than a moment's
> work.

Really? Please run

grep -r -l '--exclude-dir=.git*' '--exclude-dir=roms' '^//#define ' .
| xargs -n 1 sed -i 's/\/\/#define DEBUG/#define DEBUG/g'

and fix the problems, should be a moment's work.

The situation could be worse, for example near
7fd3f494409775b491ca08f9026c45da13852a6d there are commits fixing the
debug statements. Those commits would not be necessary if tracepoints
were used already before breakages.

>
> --
> David Gibson                    | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
>                                | _way_ _around_!
> http://www.ozlabs.org/~dgibson