* Re: [Qemu-devel] Our use of #include is undisciplined, and what to do about it
2016-03-15 9:29 [Qemu-devel] Our use of #include is undisciplined, and what to do about it Markus Armbruster
@ 2016-03-15 9:40 ` Paolo Bonzini
2016-03-15 12:19 ` Markus Armbruster
2016-03-15 10:03 ` Peter Maydell
` (3 subsequent siblings)
4 siblings, 1 reply; 24+ messages in thread
From: Paolo Bonzini @ 2016-03-15 9:40 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: Peter Maydell, Stefan Hajnoczi
On 15/03/2016 10:29, Markus Armbruster wrote:
> NEED_CPU_H further adds
>
> include/disas/bfd.h
> include/exec/cpu-all.h
> include/exec/cpu-common.h
> include/exec/cpu-defs.h
> include/exec/exec-all.h
> include/exec/hwaddr.h
> include/exec/memattrs.h
> include/exec/memory.h
> include/hw/hotplug.h
> include/hw/i386/apic.h
> include/hw/irq.h
> include/hw/qdev-core.h
> include/qemu/bitmap.h
> include/qemu/bitops.h
> include/qemu/int128.h
> include/qemu/log.h
> include/qemu/notify.h
> include/qemu/rcu.h
> include/qemu/thread-posix.h
> include/qemu/thread.h
> include/qom/cpu.h
> include/qom/object.h
> include/standard-headers/asm-x86/hyperv.h
> include/standard-headers/linux/types.h
> target-i386/cpu-qom.h
> target-i386/cpu.h
> target-i386/svm.h
> tcg/i386/tcg-target.h
> x86_64-softmmu/config-target.h
FWIW, I'm going to do something about this in 2.7. I have patches to
remove cpu.h inclusion from qemu-common.h.
Generally though qemu-common.h needs to go. :)
Paolo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] Our use of #include is undisciplined, and what to do about it
2016-03-15 9:40 ` Paolo Bonzini
@ 2016-03-15 12:19 ` Markus Armbruster
2016-03-15 14:51 ` Paolo Bonzini
0 siblings, 1 reply; 24+ messages in thread
From: Markus Armbruster @ 2016-03-15 12:19 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Peter Maydell, qemu-devel, Stefan Hajnoczi
Paolo Bonzini <pbonzini@redhat.com> writes:
> On 15/03/2016 10:29, Markus Armbruster wrote:
>> NEED_CPU_H further adds
>>
>> include/disas/bfd.h
>> include/exec/cpu-all.h
>> include/exec/cpu-common.h
>> include/exec/cpu-defs.h
>> include/exec/exec-all.h
>> include/exec/hwaddr.h
>> include/exec/memattrs.h
>> include/exec/memory.h
>> include/hw/hotplug.h
>> include/hw/i386/apic.h
>> include/hw/irq.h
>> include/hw/qdev-core.h
>> include/qemu/bitmap.h
>> include/qemu/bitops.h
>> include/qemu/int128.h
>> include/qemu/log.h
>> include/qemu/notify.h
>> include/qemu/rcu.h
>> include/qemu/thread-posix.h
>> include/qemu/thread.h
>> include/qom/cpu.h
>> include/qom/object.h
>> include/standard-headers/asm-x86/hyperv.h
>> include/standard-headers/linux/types.h
>> target-i386/cpu-qom.h
>> target-i386/cpu.h
>> target-i386/svm.h
>> tcg/i386/tcg-target.h
>> x86_64-softmmu/config-target.h
>
> FWIW, I'm going to do something about this in 2.7. I have patches to
> remove cpu.h inclusion from qemu-common.h.
Appreciated!
> Generally though qemu-common.h needs to go. :)
Yes. The series I hope to post today is a baby step in that direction.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] Our use of #include is undisciplined, and what to do about it
2016-03-15 12:19 ` Markus Armbruster
@ 2016-03-15 14:51 ` Paolo Bonzini
0 siblings, 0 replies; 24+ messages in thread
From: Paolo Bonzini @ 2016-03-15 14:51 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Peter Maydell, qemu-devel, Stefan Hajnoczi
On 15/03/2016 13:19, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> On 15/03/2016 10:29, Markus Armbruster wrote:
>>> NEED_CPU_H further adds
>>>
>>> include/disas/bfd.h
>>> include/exec/cpu-all.h
>>> include/exec/cpu-common.h
>>> include/exec/cpu-defs.h
>>> include/exec/exec-all.h
>>> include/exec/hwaddr.h
>>> include/exec/memattrs.h
>>> include/exec/memory.h
>>> include/hw/hotplug.h
>>> include/hw/i386/apic.h
>>> include/hw/irq.h
>>> include/hw/qdev-core.h
>>> include/qemu/bitmap.h
>>> include/qemu/bitops.h
>>> include/qemu/int128.h
>>> include/qemu/log.h
>>> include/qemu/notify.h
>>> include/qemu/rcu.h
>>> include/qemu/thread-posix.h
>>> include/qemu/thread.h
>>> include/qom/cpu.h
>>> include/qom/object.h
>>> include/standard-headers/asm-x86/hyperv.h
>>> include/standard-headers/linux/types.h
>>> target-i386/cpu-qom.h
>>> target-i386/cpu.h
>>> target-i386/svm.h
>>> tcg/i386/tcg-target.h
>>> x86_64-softmmu/config-target.h
>>
>> FWIW, I'm going to do something about this in 2.7. I have patches to
>> remove cpu.h inclusion from qemu-common.h.
>
> Appreciated!
Now pushed to github.com/bonzini/qemu.git branch need-cpu-h.
Shortlog follows:
Paolo Bonzini (48):
include: move some definitions out of qemu-common.h
log: do not use CONFIG_USER_ONLY
hw: explicitly include qemu-common.h and cpu.h
cpu: make cpu-qom.h only include-able from cpu.h
target-alpha: make cpu-qom.h not target specific
target-arm: make cpu-qom.h not target specific
target-cris: make cpu-qom.h not target specific
target-i386: make cpu-qom.h not target specific
target-lm32: make cpu-qom.h not target specific
target-m68k: make cpu-qom.h not target specific
target-microblaze: make cpu-qom.h not target specific
target-mips: make cpu-qom.h not target specific
target-ppc: do not use target_ulong in cpu-qom.h
target-ppc: make cpu-qom.h not target specific
target-s390x: make cpu-qom.h not target specific
target-sh4: make cpu-qom.h not target specific
target-sparc: make cpu-qom.h not target specific
target-tricore: make cpu-qom.h not target specific
target-unicore32: make cpu-qom.h not target specific
target-xtensa: make cpu-qom.h not target specific
arm: include cpu-qom.h in files that require ARMCPU
m68k: include cpu-qom.h in files that require M68KCPU
sh4: include cpu-qom.h in files that require M68KCPU
alpha: include cpu-qom.h in files that require AlphaCPU
mips: use MIPSCPU instead of CPUMIPSState
ppc: use PowerPCCPU instead of CPUPPCState
arm: remove useless cpu.h inclusion
explicitly include qom/cpu.h
explicitly include hw/qdev-core.h
explicitly include linux/kvm.h
apic: move target-dependent definitions to cpu.h
include: poison symbols in osdep.h
hw: do not use VMSTATE_*TL
hw: move CPU state serialization to migration/cpu.h
hw: cannot include hw/hw.h from user emulation
cpu: move endian-dependent load/store functions to cpu-all.h
qemu-common: stop including qemu/bswap.h from qemu-common.h
qemu-common: stop including qemu/host-utils.h from qemu-common.h
gdbstub: remove includes from gdbstub-xml.c
dma: do not depend on kvm_enabled()
do not include qemu-common.h in hw/hw.h
s390x: move stuff out of cpu.h
qemu-common: push cpu.h inclusion out of qemu-common.h
arm: move arm_log_exception into .c file
mips: move CP0 functions out of cpu.h
hw: explicitly include qemu/log.h
exec: extract exec/tb-context.h
cpu: move exec-all.h inclusion out of cpu.h
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] Our use of #include is undisciplined, and what to do about it
2016-03-15 9:29 [Qemu-devel] Our use of #include is undisciplined, and what to do about it Markus Armbruster
2016-03-15 9:40 ` Paolo Bonzini
@ 2016-03-15 10:03 ` Peter Maydell
2016-03-15 12:28 ` Markus Armbruster
2016-03-15 13:39 ` Stefan Hajnoczi
` (2 subsequent siblings)
4 siblings, 1 reply; 24+ messages in thread
From: Peter Maydell @ 2016-03-15 10:03 UTC (permalink / raw)
To: Markus Armbruster; +Cc: QEMU Developers, Stefan Hajnoczi
On 15 March 2016 at 09:29, Markus Armbruster <armbru@redhat.com> wrote:
> = What to do about it =
>
> The immediately obvious thing to do is reduce "recompile the world"
> headers that change frequently. I've started to do that.
This is obviously worthwhile.
> Another one is attacking widely included bulky files (see "Top
> scorers"). Some can simply be included less. Others need to be split,
> in particular the generated tracers.
But how important is this? Yes, the headers may make up a large
%-of-total-source-lines, but how much extra actual compile time
do they add? My off-the-top-of-my-head guess is that most of the
time will be spent on optimization passes of actual code, and that
things like structure definitions, typedefs, etc that you find in
headers aren't going to make a major contribution.
> Yet another one is reviewing the way we include system and GLib headers.
We must include glib.h from osdep.h because it has compatibility
fixups (in glib-compat.h) and we need to avoid the risk of a .c
file including the system header directly without the fixups.
> But our root problem is our undisciplined use of #include. Can we agree
> on a sane set of rules? Here's my proposal:
>
> 1. Have a carefully curated header that's included everywhere first. We
> got that already thanks to Peter: osdep.h.
>
> 2. Headers should normally include everything they need beyond osdep.h.
> If exceptions are needed for some reason, they must be documented in
> the header. If all that's needed from a header is typedefs, put
> those into qemu/typedefs.h instead of including the header.
>
> 3. Cyclic inclusion is forbidden.
I would be happy with these rules; we're a fair way away from them
though. (Part of my aim with the osdep cleanup was to get to a point
where it was easier to make header files self-contained without that
implying every header needing a lot of system #includes.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] Our use of #include is undisciplined, and what to do about it
2016-03-15 10:03 ` Peter Maydell
@ 2016-03-15 12:28 ` Markus Armbruster
2016-03-15 12:56 ` Peter Maydell
0 siblings, 1 reply; 24+ messages in thread
From: Markus Armbruster @ 2016-03-15 12:28 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers, Stefan Hajnoczi
Peter Maydell <peter.maydell@linaro.org> writes:
> On 15 March 2016 at 09:29, Markus Armbruster <armbru@redhat.com> wrote:
>> = What to do about it =
>>
>> The immediately obvious thing to do is reduce "recompile the world"
>> headers that change frequently. I've started to do that.
>
> This is obviously worthwhile.
>
>> Another one is attacking widely included bulky files (see "Top
>> scorers"). Some can simply be included less. Others need to be split,
>> in particular the generated tracers.
>
> But how important is this? Yes, the headers may make up a large
> %-of-total-source-lines, but how much extra actual compile time
> do they add? My off-the-top-of-my-head guess is that most of the
> time will be spent on optimization passes of actual code, and that
> things like structure definitions, typedefs, etc that you find in
> headers aren't going to make a major contribution.
I readily concede that we shouldn't speculate about compile time without
measurements.
We need to do something about the generated tracers, though. Every time
you fiddle with a tracepoint, you get to recompile ~900 out of ~4000
files. I suspect it's only 900 mostly because people avoid tracepoints
for that exact reason. I certainly do. If only the fiddled-with file
got recompiled, I'd happily use them for debugging, and when done post
any new tracepoints I found useful for others to enjoy.
>> Yet another one is reviewing the way we include system and GLib headers.
>
> We must include glib.h from osdep.h because it has compatibility
> fixups (in glib-compat.h) and we need to avoid the risk of a .c
> file including the system header directly without the fixups.
I understand the problem. Perhaps we can find a less heavy-handed
solution. Not a priority for me right now.
>> But our root problem is our undisciplined use of #include. Can we agree
>> on a sane set of rules? Here's my proposal:
>>
>> 1. Have a carefully curated header that's included everywhere first. We
>> got that already thanks to Peter: osdep.h.
>>
>> 2. Headers should normally include everything they need beyond osdep.h.
>> If exceptions are needed for some reason, they must be documented in
>> the header. If all that's needed from a header is typedefs, put
>> those into qemu/typedefs.h instead of including the header.
>>
>> 3. Cyclic inclusion is forbidden.
>
> I would be happy with these rules; we're a fair way away from them
> though. (Part of my aim with the osdep cleanup was to get to a point
> where it was easier to make header files self-contained without that
> implying every header needing a lot of system #includes.)
Okay, I can try to take us a few more steps in this direction.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] Our use of #include is undisciplined, and what to do about it
2016-03-15 12:28 ` Markus Armbruster
@ 2016-03-15 12:56 ` Peter Maydell
0 siblings, 0 replies; 24+ messages in thread
From: Peter Maydell @ 2016-03-15 12:56 UTC (permalink / raw)
To: Markus Armbruster; +Cc: QEMU Developers, Stefan Hajnoczi
On 15 March 2016 at 12:28, Markus Armbruster <armbru@redhat.com> wrote:
> We need to do something about the generated tracers, though. Every time
> you fiddle with a tracepoint, you get to recompile ~900 out of ~4000
> files. I suspect it's only 900 mostly because people avoid tracepoints
> for that exact reason. I certainly do. If only the fiddled-with file
> got recompiled, I'd happily use them for debugging, and when done post
> any new tracepoints I found useful for others to enjoy.
Yes, definitely -- having a single file listing every tracepoint
in the system is a recipe for merge conflicts too, so splitting
it would be sensible. (Either lots of separate files, or maybe we
could put tracepoint definitions in the .c files and have the
scripts pull them out...)
thanks
-- PMM
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] Our use of #include is undisciplined, and what to do about it
2016-03-15 9:29 [Qemu-devel] Our use of #include is undisciplined, and what to do about it Markus Armbruster
2016-03-15 9:40 ` Paolo Bonzini
2016-03-15 10:03 ` Peter Maydell
@ 2016-03-15 13:39 ` Stefan Hajnoczi
2016-03-15 13:51 ` Daniel P. Berrange
2016-03-15 13:56 ` Dr. David Alan Gilbert
2016-03-17 19:40 ` Richard Henderson
2019-05-27 13:12 ` Markus Armbruster
4 siblings, 2 replies; 24+ messages in thread
From: Stefan Hajnoczi @ 2016-03-15 13:39 UTC (permalink / raw)
To: Markus Armbruster
Cc: Lluís Vilanova, Peter Maydell, qemu-devel, Dr. David Alan Gilbert
[-- Attachment #1: Type: text/plain, Size: 1633 bytes --]
On Tue, Mar 15, 2016 at 10:29:06AM +0100, Markus Armbruster wrote:
> Stefan cc'ed because tracing is part of the problem. Search for
> "tracers".
Thanks for bringing up the generated tracing headers. David Gilbert and
I discussed splitting them up a few months ago. I'll summarize the
problem and we can think about solutions:
trace.h includes generated-tracers.h and generated-events.h. These
files are generated from scripts/tracetool.py. The contents of the
files depends on the configured --enable-trace-backends= list (log,
simple, ftrace, dtrace, ust).
Idealy ./trace-events would have "sections" so that multiple files are
generated:
# section virtio-blk
foo() ...
# section memory
bar() ...
This would put trace_foo() in generated-tracers-virtio-blk.h and
trace_bar() in generated-tracers-memory.h. Source files using tracing
would need to include headers for relevant sections.
This way we can narrow the scope of tracing headers and prevent global
recompilation.
The fly in the ointment is that trace/control.h defines enum
TraceEventID, a global numbering of all trace events. The enum is used
in trace/contro.h APIs and also in the simpletrace file format.
If ./trace-event is modified the numbering of trace events could change.
This would require global recompilation :(.
So in order to avoid global recompilation we need to eliminate enum
TraceEventID. Perhaps it's possible to use TraceEvent* instead of
TraceEventID. For the simpletrace backend we would continue to use a
global ordering but that only affects generated-tracers.c and not header
files (thankfully!).
Any comments?
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] Our use of #include is undisciplined, and what to do about it
2016-03-15 13:39 ` Stefan Hajnoczi
@ 2016-03-15 13:51 ` Daniel P. Berrange
2016-03-15 13:56 ` Dr. David Alan Gilbert
1 sibling, 0 replies; 24+ messages in thread
From: Daniel P. Berrange @ 2016-03-15 13:51 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: qemu-devel, Peter Maydell, Markus Armbruster,
Dr. David Alan Gilbert, Lluís Vilanova
On Tue, Mar 15, 2016 at 01:39:17PM +0000, Stefan Hajnoczi wrote:
> On Tue, Mar 15, 2016 at 10:29:06AM +0100, Markus Armbruster wrote:
> > Stefan cc'ed because tracing is part of the problem. Search for
> > "tracers".
>
> Thanks for bringing up the generated tracing headers. David Gilbert and
> I discussed splitting them up a few months ago. I'll summarize the
> problem and we can think about solutions:
>
> trace.h includes generated-tracers.h and generated-events.h. These
> files are generated from scripts/tracetool.py. The contents of the
> files depends on the configured --enable-trace-backends= list (log,
> simple, ftrace, dtrace, ust).
>
> Idealy ./trace-events would have "sections" so that multiple files are
> generated:
>
> # section virtio-blk
> foo() ...
>
> # section memory
> bar() ...
>
> This would put trace_foo() in generated-tracers-virtio-blk.h and
> trace_bar() in generated-tracers-memory.h. Source files using tracing
> would need to include headers for relevant sections.
I'd actually like to see the trace events associated with the subsystem
eg any trace events for block devices should live in block/trace-events,
likewise for things like crypto/trace-events, io/trace-events, etc, etc.
This would naturally lead to generating a block/trace-events.h file that
only contained the block trace point definitions.
As well as simplifying compilation deps, it means the trace-events file
changes get associated with correct people for get_maintainer.pl usage,
and we reduce liklihood of merge conflicts for pull requests by getting
rid of the single big file
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] Our use of #include is undisciplined, and what to do about it
2016-03-15 13:39 ` Stefan Hajnoczi
2016-03-15 13:51 ` Daniel P. Berrange
@ 2016-03-15 13:56 ` Dr. David Alan Gilbert
2016-03-16 18:23 ` Stefan Hajnoczi
1 sibling, 1 reply; 24+ messages in thread
From: Dr. David Alan Gilbert @ 2016-03-15 13:56 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Lluís Vilanova, Peter Maydell, Markus Armbruster, qemu-devel
* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> On Tue, Mar 15, 2016 at 10:29:06AM +0100, Markus Armbruster wrote:
> > Stefan cc'ed because tracing is part of the problem. Search for
> > "tracers".
>
> Thanks for bringing up the generated tracing headers. David Gilbert and
> I discussed splitting them up a few months ago. I'll summarize the
> problem and we can think about solutions:
>
> trace.h includes generated-tracers.h and generated-events.h. These
> files are generated from scripts/tracetool.py. The contents of the
> files depends on the configured --enable-trace-backends= list (log,
> simple, ftrace, dtrace, ust).
>
> Idealy ./trace-events would have "sections" so that multiple files are
> generated:
>
> # section virtio-blk
> foo() ...
>
> # section memory
> bar() ...
I was going to split that the other way, and have
trace-events
migration/trace-events
block/trace-events
and then they each generate their own header.
> This would put trace_foo() in generated-tracers-virtio-blk.h and
> trace_bar() in generated-tracers-memory.h. Source files using tracing
> would need to include headers for relevant sections.
>
> This way we can narrow the scope of tracing headers and prevent global
> recompilation.
>
> The fly in the ointment is that trace/control.h defines enum
> TraceEventID, a global numbering of all trace events. The enum is used
> in trace/contro.h APIs and also in the simpletrace file format.
>
> If ./trace-event is modified the numbering of trace events could change.
> This would require global recompilation :(.
>
> So in order to avoid global recompilation we need to eliminate enum
> TraceEventID. Perhaps it's possible to use TraceEvent* instead of
> TraceEventID. For the simpletrace backend we would continue to use a
> global ordering but that only affects generated-tracers.c and not header
> files (thankfully!).
I think the hardest part is going to be understanding all of the different
tracers and the things they need to know; like that thing about simpletrace
needing the single ordering; I don't know what the other backends need
but I bet they've each got their own surprise.
(And yes, I'd love to split them up - I tend to use traceing quite a bit
in migration now and trying to do a bisect over a big migration patch
series takes ages mostly because of the trace.h changes)
Dave
>
> Any comments?
>
> Stefan
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] Our use of #include is undisciplined, and what to do about it
2016-03-15 13:56 ` Dr. David Alan Gilbert
@ 2016-03-16 18:23 ` Stefan Hajnoczi
2016-03-16 18:27 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 24+ messages in thread
From: Stefan Hajnoczi @ 2016-03-16 18:23 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Lluís Vilanova, Peter Maydell, Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2110 bytes --]
On Tue, Mar 15, 2016 at 01:56:47PM +0000, Dr. David Alan Gilbert wrote:
> > This would put trace_foo() in generated-tracers-virtio-blk.h and
> > trace_bar() in generated-tracers-memory.h. Source files using tracing
> > would need to include headers for relevant sections.
> >
> > This way we can narrow the scope of tracing headers and prevent global
> > recompilation.
> >
> > The fly in the ointment is that trace/control.h defines enum
> > TraceEventID, a global numbering of all trace events. The enum is used
> > in trace/contro.h APIs and also in the simpletrace file format.
> >
> > If ./trace-event is modified the numbering of trace events could change.
> > This would require global recompilation :(.
> >
> > So in order to avoid global recompilation we need to eliminate enum
> > TraceEventID. Perhaps it's possible to use TraceEvent* instead of
> > TraceEventID. For the simpletrace backend we would continue to use a
> > global ordering but that only affects generated-tracers.c and not header
> > files (thankfully!).
>
> I think the hardest part is going to be understanding all of the different
> tracers and the things they need to know; like that thing about simpletrace
> needing the single ordering; I don't know what the other backends need
> but I bet they've each got their own surprise.
>
> (And yes, I'd love to split them up - I tend to use traceing quite a bit
> in migration now and trying to do a bisect over a big migration patch
> series takes ages mostly because of the trace.h changes)
Yes, this is the hard part. The simpletrace.py pretty-printing script
takes a trace-events file as input. I suppose it could alphabetically
sort the trace-events filenames from the command-line to produce a
global ordering of trace event IDs. Of course that doesn't work
properly if the script is invoked with relative paths from a
sub-directory...
Perhaps we need a new script during the QEMU build process that produces
a trace-event-ids.csv file. Then simpletrace.py could take that instead
of processing ./trace-events.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] Our use of #include is undisciplined, and what to do about it
2016-03-16 18:23 ` Stefan Hajnoczi
@ 2016-03-16 18:27 ` Dr. David Alan Gilbert
2016-03-17 11:25 ` Stefan Hajnoczi
0 siblings, 1 reply; 24+ messages in thread
From: Dr. David Alan Gilbert @ 2016-03-16 18:27 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Lluís Vilanova, Peter Maydell, Markus Armbruster, qemu-devel
* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> On Tue, Mar 15, 2016 at 01:56:47PM +0000, Dr. David Alan Gilbert wrote:
> > > This would put trace_foo() in generated-tracers-virtio-blk.h and
> > > trace_bar() in generated-tracers-memory.h. Source files using tracing
> > > would need to include headers for relevant sections.
> > >
> > > This way we can narrow the scope of tracing headers and prevent global
> > > recompilation.
> > >
> > > The fly in the ointment is that trace/control.h defines enum
> > > TraceEventID, a global numbering of all trace events. The enum is used
> > > in trace/contro.h APIs and also in the simpletrace file format.
> > >
> > > If ./trace-event is modified the numbering of trace events could change.
> > > This would require global recompilation :(.
> > >
> > > So in order to avoid global recompilation we need to eliminate enum
> > > TraceEventID. Perhaps it's possible to use TraceEvent* instead of
> > > TraceEventID. For the simpletrace backend we would continue to use a
> > > global ordering but that only affects generated-tracers.c and not header
> > > files (thankfully!).
> >
> > I think the hardest part is going to be understanding all of the different
> > tracers and the things they need to know; like that thing about simpletrace
> > needing the single ordering; I don't know what the other backends need
> > but I bet they've each got their own surprise.
> >
> > (And yes, I'd love to split them up - I tend to use traceing quite a bit
> > in migration now and trying to do a bisect over a big migration patch
> > series takes ages mostly because of the trace.h changes)
>
> Yes, this is the hard part. The simpletrace.py pretty-printing script
> takes a trace-events file as input. I suppose it could alphabetically
> sort the trace-events filenames from the command-line to produce a
> global ordering of trace event IDs. Of course that doesn't work
> properly if the script is invoked with relative paths from a
> sub-directory...
>
> Perhaps we need a new script during the QEMU build process that produces
> a trace-event-ids.csv file. Then simpletrace.py could take that instead
> of processing ./trace-events.
Well that sounds easy - but is it easy to avoid using those fixed IDs
in any of the trace.h functions that are included everywhere (without
making them more expensive).
Dave
> Stefan
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] Our use of #include is undisciplined, and what to do about it
2016-03-16 18:27 ` Dr. David Alan Gilbert
@ 2016-03-17 11:25 ` Stefan Hajnoczi
2016-03-17 16:29 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 24+ messages in thread
From: Stefan Hajnoczi @ 2016-03-17 11:25 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Lluís Vilanova, Peter Maydell, Markus Armbruster, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2850 bytes --]
On Wed, Mar 16, 2016 at 06:27:48PM +0000, Dr. David Alan Gilbert wrote:
> * Stefan Hajnoczi (stefanha@redhat.com) wrote:
> > On Tue, Mar 15, 2016 at 01:56:47PM +0000, Dr. David Alan Gilbert wrote:
> > > > This would put trace_foo() in generated-tracers-virtio-blk.h and
> > > > trace_bar() in generated-tracers-memory.h. Source files using tracing
> > > > would need to include headers for relevant sections.
> > > >
> > > > This way we can narrow the scope of tracing headers and prevent global
> > > > recompilation.
> > > >
> > > > The fly in the ointment is that trace/control.h defines enum
> > > > TraceEventID, a global numbering of all trace events. The enum is used
> > > > in trace/contro.h APIs and also in the simpletrace file format.
> > > >
> > > > If ./trace-event is modified the numbering of trace events could change.
> > > > This would require global recompilation :(.
> > > >
> > > > So in order to avoid global recompilation we need to eliminate enum
> > > > TraceEventID. Perhaps it's possible to use TraceEvent* instead of
> > > > TraceEventID. For the simpletrace backend we would continue to use a
> > > > global ordering but that only affects generated-tracers.c and not header
> > > > files (thankfully!).
> > >
> > > I think the hardest part is going to be understanding all of the different
> > > tracers and the things they need to know; like that thing about simpletrace
> > > needing the single ordering; I don't know what the other backends need
> > > but I bet they've each got their own surprise.
> > >
> > > (And yes, I'd love to split them up - I tend to use traceing quite a bit
> > > in migration now and trying to do a bisect over a big migration patch
> > > series takes ages mostly because of the trace.h changes)
> >
> > Yes, this is the hard part. The simpletrace.py pretty-printing script
> > takes a trace-events file as input. I suppose it could alphabetically
> > sort the trace-events filenames from the command-line to produce a
> > global ordering of trace event IDs. Of course that doesn't work
> > properly if the script is invoked with relative paths from a
> > sub-directory...
> >
> > Perhaps we need a new script during the QEMU build process that produces
> > a trace-event-ids.csv file. Then simpletrace.py could take that instead
> > of processing ./trace-events.
>
> Well that sounds easy - but is it easy to avoid using those fixed IDs
> in any of the trace.h functions that are included everywhere (without
> making them more expensive).
How about this:
Instead of using TraceEventID in tracing APIs, pass the TraceEvent
pointer directly. The tracetool.py code generator should produce
"extern TraceEvent trace_event_foo;" definitions in headers so that the
users have direct access to the TraceEvents.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] Our use of #include is undisciplined, and what to do about it
2016-03-17 11:25 ` Stefan Hajnoczi
@ 2016-03-17 16:29 ` Dr. David Alan Gilbert
2016-03-17 19:21 ` Paolo Bonzini
0 siblings, 1 reply; 24+ messages in thread
From: Dr. David Alan Gilbert @ 2016-03-17 16:29 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Lluís Vilanova, Peter Maydell, Markus Armbruster, qemu-devel
* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> On Wed, Mar 16, 2016 at 06:27:48PM +0000, Dr. David Alan Gilbert wrote:
> > * Stefan Hajnoczi (stefanha@redhat.com) wrote:
> > > On Tue, Mar 15, 2016 at 01:56:47PM +0000, Dr. David Alan Gilbert wrote:
> > > > > This would put trace_foo() in generated-tracers-virtio-blk.h and
> > > > > trace_bar() in generated-tracers-memory.h. Source files using tracing
> > > > > would need to include headers for relevant sections.
> > > > >
> > > > > This way we can narrow the scope of tracing headers and prevent global
> > > > > recompilation.
> > > > >
> > > > > The fly in the ointment is that trace/control.h defines enum
> > > > > TraceEventID, a global numbering of all trace events. The enum is used
> > > > > in trace/contro.h APIs and also in the simpletrace file format.
> > > > >
> > > > > If ./trace-event is modified the numbering of trace events could change.
> > > > > This would require global recompilation :(.
> > > > >
> > > > > So in order to avoid global recompilation we need to eliminate enum
> > > > > TraceEventID. Perhaps it's possible to use TraceEvent* instead of
> > > > > TraceEventID. For the simpletrace backend we would continue to use a
> > > > > global ordering but that only affects generated-tracers.c and not header
> > > > > files (thankfully!).
> > > >
> > > > I think the hardest part is going to be understanding all of the different
> > > > tracers and the things they need to know; like that thing about simpletrace
> > > > needing the single ordering; I don't know what the other backends need
> > > > but I bet they've each got their own surprise.
> > > >
> > > > (And yes, I'd love to split them up - I tend to use traceing quite a bit
> > > > in migration now and trying to do a bisect over a big migration patch
> > > > series takes ages mostly because of the trace.h changes)
> > >
> > > Yes, this is the hard part. The simpletrace.py pretty-printing script
> > > takes a trace-events file as input. I suppose it could alphabetically
> > > sort the trace-events filenames from the command-line to produce a
> > > global ordering of trace event IDs. Of course that doesn't work
> > > properly if the script is invoked with relative paths from a
> > > sub-directory...
> > >
> > > Perhaps we need a new script during the QEMU build process that produces
> > > a trace-event-ids.csv file. Then simpletrace.py could take that instead
> > > of processing ./trace-events.
> >
> > Well that sounds easy - but is it easy to avoid using those fixed IDs
> > in any of the trace.h functions that are included everywhere (without
> > making them more expensive).
>
> How about this:
>
> Instead of using TraceEventID in tracing APIs, pass the TraceEvent
> pointer directly. The tracetool.py code generator should produce
> "extern TraceEvent trace_event_foo;" definitions in headers so that the
> users have direct access to the TraceEvents.
OK, so I see TraceEvent has a TraceEventID field; so yes that works easily;
it turns out to be a little more expensive though since what was a:
trace_events_dstate[id]
is now
trace_events_dstate[te->id]
But hang on, what's the 'sstate' in TraceEvent; do we actually need two
state fields if we're passing a TraceEvent pointer around?
Dave
> Stefan
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] Our use of #include is undisciplined, and what to do about it
2016-03-17 16:29 ` Dr. David Alan Gilbert
@ 2016-03-17 19:21 ` Paolo Bonzini
2016-03-17 19:43 ` Dr. David Alan Gilbert
` (2 more replies)
0 siblings, 3 replies; 24+ messages in thread
From: Paolo Bonzini @ 2016-03-17 19:21 UTC (permalink / raw)
To: Dr. David Alan Gilbert, Stefan Hajnoczi
Cc: qemu-devel, Peter Maydell, Richard Henderson,
Lluís Vilanova, Markus Armbruster
On 17/03/2016 17:29, Dr. David Alan Gilbert wrote:
> OK, so I see TraceEvent has a TraceEventID field; so yes that works easily;
> it turns out to be a little more expensive though since what was a:
>
> trace_events_dstate[id]
>
> is now
> trace_events_dstate[te->id]
That however makes you waste a lot of cache on trace_events_dstate
(commit 585ec72, "trace: track enabled events in a separate array",
2016-02-03).
Perhaps we get the linker to do compute the id, for example by using a
separate data section and then use te-&te_first to compute the id...
Richard, do you have ideas on how to do this in a reasonably portable
manner?
> But hang on, what's the 'sstate' in TraceEvent; do we actually need two
> state fields if we're passing a TraceEvent pointer around?
sstate means the event is unavailable, it's basically just a way to
provide better error messages.
Paolo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] Our use of #include is undisciplined, and what to do about it
2016-03-17 19:21 ` Paolo Bonzini
@ 2016-03-17 19:43 ` Dr. David Alan Gilbert
2016-03-17 19:58 ` Paolo Bonzini
2016-03-17 20:14 ` Richard Henderson
2 siblings, 0 replies; 24+ messages in thread
From: Dr. David Alan Gilbert @ 2016-03-17 19:43 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Peter Maydell, qemu-devel, Markus Armbruster, Stefan Hajnoczi,
Lluís Vilanova, Richard Henderson
* Paolo Bonzini (pbonzini@redhat.com) wrote:
> On 17/03/2016 17:29, Dr. David Alan Gilbert wrote:
> > OK, so I see TraceEvent has a TraceEventID field; so yes that works easily;
> > it turns out to be a little more expensive though since what was a:
> >
> > trace_events_dstate[id]
> >
> > is now
> > trace_events_dstate[te->id]
>
> That however makes you waste a lot of cache on trace_events_dstate
> (commit 585ec72, "trace: track enabled events in a separate array",
> 2016-02-03).
>
> Perhaps we get the linker to do compute the id, for example by using a
> separate data section and then use te-&te_first to compute the id...
> Richard, do you have ideas on how to do this in a reasonably portable
> manner?
It's possible we don't need the unique-id on the fast-path; for example
if we had an id that was only unique within each trace-events file,
and it's own trace_events_dstate[] that used that ID, then that would still
be a nice statically known id. You'd still need to be able to get the
global ID for the simple-event and anything else that needed it.
> > But hang on, what's the 'sstate' in TraceEvent; do we actually need two
> > state fields if we're passing a TraceEvent pointer around?
>
> sstate means the event is unavailable, it's basically just a way to
> provide better error messages.
Dave
>
> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] Our use of #include is undisciplined, and what to do about it
2016-03-17 19:21 ` Paolo Bonzini
2016-03-17 19:43 ` Dr. David Alan Gilbert
@ 2016-03-17 19:58 ` Paolo Bonzini
2016-03-17 20:14 ` Richard Henderson
2 siblings, 0 replies; 24+ messages in thread
From: Paolo Bonzini @ 2016-03-17 19:58 UTC (permalink / raw)
To: Dr. David Alan Gilbert, Stefan Hajnoczi
Cc: Lluís Vilanova, Peter Maydell, qemu-devel,
Markus Armbruster, Richard Henderson
On 17/03/2016 20:21, Paolo Bonzini wrote:
> On 17/03/2016 17:29, Dr. David Alan Gilbert wrote:
>> OK, so I see TraceEvent has a TraceEventID field; so yes that works easily;
>> it turns out to be a little more expensive though since what was a:
>>
>> trace_events_dstate[id]
>>
>> is now
>> trace_events_dstate[te->id]
>
> That however makes you waste a lot of cache on trace_events_dstate
> (commit 585ec72, "trace: track enabled events in a separate array",
> 2016-02-03).
>
> Perhaps we get the linker to do compute the id, for example by using a
> separate data section and then use te-&te_first to compute the id...
> Richard, do you have ideas on how to do this in a reasonably portable
> manner?
Dave and I actually got to a much better design on IRC.
1) since the .h files will be per-directory, make the dstate array per
directory (e.g. trace_events_dstate_migration[])
2) changes must have no effect on unrelated .h files, but we can keep a
global .c file. Instead of using trace_events_dstate[te->id], we can
store a pointer directly in te, e.g. ".p_dstate =
&trace_events_dstate_migration[23]" and then *(te->p_dstate). The
global .c file can also store mappings from local id to global id, so
that the global id can be retrieved with "trace_events_ids_migration[23]".
Processing trace-events would remain a global process, but unchanged .h
files would not be changed on disk and would not trigger the world.
Splitting the input to tracetool becomes a separate task from splitting
its output, and could be achieved simply by introducing include directives.
Paolo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] Our use of #include is undisciplined, and what to do about it
2016-03-17 19:21 ` Paolo Bonzini
2016-03-17 19:43 ` Dr. David Alan Gilbert
2016-03-17 19:58 ` Paolo Bonzini
@ 2016-03-17 20:14 ` Richard Henderson
2016-03-17 20:27 ` Peter Maydell
2016-03-17 20:59 ` Paolo Bonzini
2 siblings, 2 replies; 24+ messages in thread
From: Richard Henderson @ 2016-03-17 20:14 UTC (permalink / raw)
To: Paolo Bonzini, Dr. David Alan Gilbert, Stefan Hajnoczi
Cc: Peter Maydell, qemu-devel, Markus Armbruster, Lluís Vilanova
On 03/17/2016 12:21 PM, Paolo Bonzini wrote:
> That however makes you waste a lot of cache on trace_events_dstate
> (commit 585ec72, "trace: track enabled events in a separate array",
> 2016-02-03).
I must say I'm not really convinced by that patch, since I don't see that
there's much locality between the ID's that would be polled.
> Perhaps we get the linker to do compute the id, for example by using a
> separate data section and then use te-&te_first to compute the id...
> Richard, do you have ideas on how to do this in a reasonably portable
> manner?
That works for all of the hosts we care about (ELF and MachO).
All you need is "crt0"/"crtn" files to force to be first and last, placing
symbols in known sections. Then emit all of the "id"s as symbols in those
sections.
E.g. for an event FOO, generate
bool trace_event_FOO_dstate
__attribute__((section(".bss.trace_event_dstate")));
TraceEvent trace_event_FOO
__attribute__((section(".bss.trace_event")));
For portability, you must put the section attribute on the extern declaration
as well, lest the compiler make assumptions about small variables living in
".sbss" or the like, and thus being addressable with gp-relative offsets.
See gcc's crtstuff.c for examples of how to write the crt files.
Or, if you value your sanity, don't. ;-)
r~
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] Our use of #include is undisciplined, and what to do about it
2016-03-17 20:14 ` Richard Henderson
@ 2016-03-17 20:27 ` Peter Maydell
2016-03-17 20:29 ` Richard Henderson
2016-03-17 20:59 ` Paolo Bonzini
1 sibling, 1 reply; 24+ messages in thread
From: Peter Maydell @ 2016-03-17 20:27 UTC (permalink / raw)
To: Richard Henderson
Cc: QEMU Developers, Markus Armbruster, Dr. David Alan Gilbert,
Stefan Hajnoczi, Paolo Bonzini, Lluís Vilanova
On 17 March 2016 at 20:14, Richard Henderson <rth@twiddle.net> wrote:
> That works for all of the hosts we care about (ELF and MachO).
...Windows is something other than both of those, right?
Or does it use ELF these days?
thanks
-- PMM
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] Our use of #include is undisciplined, and what to do about it
2016-03-17 20:27 ` Peter Maydell
@ 2016-03-17 20:29 ` Richard Henderson
2016-03-17 21:02 ` Paolo Bonzini
0 siblings, 1 reply; 24+ messages in thread
From: Richard Henderson @ 2016-03-17 20:29 UTC (permalink / raw)
To: Peter Maydell
Cc: QEMU Developers, Markus Armbruster, Dr. David Alan Gilbert,
Stefan Hajnoczi, Paolo Bonzini, Lluís Vilanova
On 03/17/2016 01:27 PM, Peter Maydell wrote:
> On 17 March 2016 at 20:14, Richard Henderson <rth@twiddle.net> wrote:
>> That works for all of the hosts we care about (ELF and MachO).
>
> ...Windows is something other than both of those, right?
> Or does it use ELF these days?
PECOFF. But it still supports named sections, so not a deal-breaker.
We just don't support old a.out systems (e.g. really old OpenBSD).
r~
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] Our use of #include is undisciplined, and what to do about it
2016-03-17 20:29 ` Richard Henderson
@ 2016-03-17 21:02 ` Paolo Bonzini
0 siblings, 0 replies; 24+ messages in thread
From: Paolo Bonzini @ 2016-03-17 21:02 UTC (permalink / raw)
To: Richard Henderson, Peter Maydell
Cc: Lluís Vilanova, Stefan Hajnoczi, QEMU Developers,
Dr. David Alan Gilbert, Markus Armbruster
On 17/03/2016 21:29, Richard Henderson wrote:
>> > ...Windows is something other than both of those, right?
>> > Or does it use ELF these days?
> PECOFF. But it still supports named sections, so not a deal-breaker.
> We just don't support old a.out systems (e.g. really old OpenBSD).
FWIW Windows also supports sorted sections. You could put the "first"
symbol in data.trace_events$AAA, the last symbol in
data.trace_events$ZZZ and the intermediate symbols in
data.trace_events$FOO; the linker would merge them into a single
data.trace_events section and you wouldn't have to ensure the correct
order of the object files on the command line.
It's actually a pretty cool feature. It makes such linker tricks much
easier to do. But it's not portable.
Paolo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] Our use of #include is undisciplined, and what to do about it
2016-03-17 20:14 ` Richard Henderson
2016-03-17 20:27 ` Peter Maydell
@ 2016-03-17 20:59 ` Paolo Bonzini
1 sibling, 0 replies; 24+ messages in thread
From: Paolo Bonzini @ 2016-03-17 20:59 UTC (permalink / raw)
To: Richard Henderson, Dr. David Alan Gilbert, Stefan Hajnoczi
Cc: Lluís Vilanova, Peter Maydell, qemu-devel, Markus Armbruster
On 17/03/2016 21:14, Richard Henderson wrote:
> On 03/17/2016 12:21 PM, Paolo Bonzini wrote:
>> That however makes you waste a lot of cache on trace_events_dstate
>> (commit 585ec72, "trace: track enabled events in a separate array",
>> 2016-02-03).
>
> I must say I'm not really convinced by that patch, since I don't see that
> there's much locality between the ID's that would be polled.
There are usually just three-four files in hot paths; they could be
kvm-all.c, memory.c, hw/virtio/virtio.c and hw/block/virtio-blk.c for a
disk benchmark for example. All tracepoints for a file are adjacent,
hence the trace_events_dstate portion that represents one file (assuming
that file has <=64 events) costs 1-2 cache lines. Without the patch the
footprint of trace_events is 1 cache line for every 2-3 events since
sizeof(TraceEvent) == 24.
It's true that the patch before 585ec72 also helps removing overhead in
the case where all events are disabled (and that's the really common
case). That obviously avoids consuming _any_ amount of cache on
disabled trace events. However I believe that separate dstate arrays
are anyway helpful for David's plan to split the generated tracing
headers and avoid world rebuilds. That's because the headers only need
the dstate arrays and not the big global arrays.
Paolo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] Our use of #include is undisciplined, and what to do about it
2016-03-15 9:29 [Qemu-devel] Our use of #include is undisciplined, and what to do about it Markus Armbruster
` (2 preceding siblings ...)
2016-03-15 13:39 ` Stefan Hajnoczi
@ 2016-03-17 19:40 ` Richard Henderson
2019-05-27 13:12 ` Markus Armbruster
4 siblings, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2016-03-17 19:40 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: Peter Maydell, Stefan Hajnoczi
On 03/15/2016 02:29 AM, Markus Armbruster wrote:
> Headers can
> be read multiple times, which can only hurt compilation time. You need
> to make an effort to avoid cyclic dependencies and excessive inclusion.
I just want to point out that the preprocessor understands the
#ifndef FOO
#define FOO
...
#endif
idiom, where nothing but comments precedes the #ifndef in the file, and
likewise nothing succeeds the matching #endif. If FOO is still defined at the
next #include of the file, the preprocessor will skip the #include entirely.
So don't let this be a consideration in the argument.
r~
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] Our use of #include is undisciplined, and what to do about it
2016-03-15 9:29 [Qemu-devel] Our use of #include is undisciplined, and what to do about it Markus Armbruster
` (3 preceding siblings ...)
2016-03-17 19:40 ` Richard Henderson
@ 2019-05-27 13:12 ` Markus Armbruster
4 siblings, 0 replies; 24+ messages in thread
From: Markus Armbruster @ 2019-05-27 13:12 UTC (permalink / raw)
To: qemu-devel
It's been three years, let's examine how things have evolved.
I'm using commit db3d11ee3f0, which is a bit behind current master, just
so I can apply my "[PATCH 0/4] Cleanups around qemu-common.h" cleanly.
Markus Armbruster <armbru@redhat.com> writes:
[...]
> = The status quo and why I hate it =
>
> I've seen several schools of thought on use of #include.
>
> There's the "no #include in headers" school: every .c file includes
> exactly the headers it needs, and the prerequisites they need. Cyclic
> inclusion becomes impossible. You can't sweep cyclic dependencies under
> the rug. Headers are read just once per compilation unit. The amount
> of crap you include is clearly visible. However, maintaining the
> #include directives is a drag, not least because their order matters.
> Especially when headers neglect to spell out their dependencies. Or
> they do, but it's wrong.
>
> There's the "headers must be self-contained" school: every header
> includes everything it needs. Headers can be included in any order.
> Sorted #include directives are tidy and easy to navigate. Headers can
> be read multiple times, which can only hurt compilation time.
Our compilers avoid this for headers with proper header guards.
> You need
> to make an effort to avoid cyclic dependencies and excessive inclusion.
>
> And then there's the school of non-thought: when it doesn't compile,
> sprinkle #include on the mess semi-randomly until it does.
>
> We do a bit of all three, but the result looks awfully close to what the
> school of non-thought produces.
>
> Every .c file includes qemu/osdep.h first. For me, a .c file that
> includes nothing but that comes out well over half a Megabyte in >23k
> lines preprocessed. Where does all this crap come from?
>
> #lines KiBytes #files source
> 5233 102 5 QEMU
> 8035 159 70 system
> 7915 224 73 GLib
> 2458 89 1 # lines
> 23641 576 149 total
>
> "# lines" are lines added by the preprocessor so the rest of the
> compiler can keep track of source locations.
#lines KiBytes #files source
375 8 5 QEMU
9722 230 113 system
8212 254 74 GLib
1517 65 N/A # lines
19826 557 192 total
The weight QEMU lost, system + GLib put on.
> Having the compiler wade through almost half a Megabyte of system+GLib
> crap before it begins to consider the code we care about feels wasteful.
> Perhaps we should rethink our approach to including library headers.
No change.
> Of the 102K that are actually our own, just 7K come from include/. 95K
> come from qapi-types.h.
Fixed.
> Judging from the .d files in my build tree, 95% of the .c files include
> qemu-common.h. That makes things a good deal worse.
Down to 90%. My "[PATCH 0/4] Cleanups around qemu-common.h" shrinks it
to less than 10%. Small enough for me not to repeat the measurements
below.
> Without
> NEED_CPU_H, this adds a modest 44K of our own headers, but almost 100K
> of system headers:
>
> #lines KiBytes #files source
> 6938 146 16 QEMU
> 11426 254 74 system
> 7915 224 73 GLib
> 2658 100 1 # lines
> 28937 726 164 total
>
> NEED_CPU_H adds another 120K of our own headers:
>
> #lines KiBytes #files source
> 11534 263 43 QEMU
> 11548 256 78 system
> 7915 225 72 GLib
> 3370 138 1 # lines
> 34367 883 194 total
>
> The average size of a .c file is just over 15KiB. To get to the actual
> C code there, the compiler has to wade through at least 550-880KiB of
> headers. In other words, roughly 2% of the source comes from .c in the
> best case.
>
> But that's not even the worst part. The worst part by far are our
> "touch this and recompile the world" headers.
>
> I find just short 4000 .d files in my build tree.
Some 6400 now, ignoring the .d that don't contain ".o:".
> Guess how many of our
> headers are listed as prerequisites in more than 90% of them (thus
> touching them will recompile the .c file)? *Twenty-two*.
Down to 12 before my "[PATCH 0/4] Cleanups around qemu-common.h", and to
10 afterwards.
> Almost fifty
> recompile more half of the world.
No significant change.
> Naturally, touching osdep.h or anything it includes recompiles the
> world. These are:
>
> config-host.h
> include/glib-compat.h
> include/qapi/error.h
> include/qemu/compiler.h
> include/qemu/osdep.h
> include/qemu/typedefs.h
> include/sysemu/os-posix.h
> qapi-types.h
>
> NEED_CPU_H adds
>
> config-target.h
>
> Fine, except for qapi/error.h and qapi-types.h. The latter is an itch I
> need to scratch urgently. My first patch series will take a swing at
> it.
Both are gone.
New: include/exec/poison.h
[...]
>
> A fun exercise is to count occurences of each header in .d files and
> multiply their number by their size. That's the number of bytes read
> from them when compiling from scratch. Top scorers:
>
> size * count size count
> 525760413 698221 753 trace/generated-tracers.h
> 298039140 93723 3180 qapi-types.h
> 197442619 55759 3541 include/qom/object.h
> 185845916 53884 3449 include/exec/memory.h
> 143750444 36878 3898 /usr/include/glib-2.0/glib/gunicode.h
> 117362690 30643 3830 include/fpu/softfloat.h
> 109783272 28164 3898 /usr/include/glib-2.0/glib/gregex.h
> 105830700 27150 3898 /usr/include/glib-2.0/glib/gvariant.h
> 92972157 123469 753 trace/generated-events.h
> 88706786 22757 3898 /usr/include/glib-2.0/glib/gtestutils.h
After my "[PATCH 0/4] Cleanups around qemu-common.h":
size * count size count
428019130 82789 5170 include/exec/memory.h
336965209 60857 5537 include/qom/object.h
248105382 39121 6342 /usr/include/glib-2.0/glib/gunicode.h
187247550 29525 6342 /usr/include/glib-2.0/glib/gvariant.h
182359220 172037 1060 trace-root.h
178178490 28095 6342 /usr/include/glib-2.0/glib/gregex.h
167477688 71848 2331 qapi/qapi-types-block-core.h
166926546 36947 4518 include/qom/cpu.h
161105826 25403 6342 /usr/include/glib-2.0/glib/gmessages.h
153508110 24205 6342 /usr/include/glib-2.0/glib/gtestutils.h
[...]
>
> = What to do about it =
>
> The immediately obvious thing to do is reduce "recompile the world"
> headers that change frequently. I've started to do that.
>
> Another one is attacking widely included bulky files (see "Top
> scorers"). Some can simply be included less. Others need to be split,
> in particular the generated tracers.
Some progress, notably around trace and QAPI.
> Yet another one is reviewing the way we include system and GLib headers.
Not attempted.
> But our root problem is our undisciplined use of #include. Can we agree
> on a sane set of rules? Here's my proposal:
>
> 1. Have a carefully curated header that's included everywhere first. We
> got that already thanks to Peter: osdep.h.
>
> 2. Headers should normally include everything they need beyond osdep.h.
> If exceptions are needed for some reason, they must be documented in
> the header. If all that's needed from a header is typedefs, put
> those into qemu/typedefs.h instead of including the header.
>
> 3. Cyclic inclusion is forbidden.
>
> Nice to have: "make check" checks 2. and 3.
See my "[RFC v4 0/7] Baby steps towards saner headers".
[...]
Headers that are prerequisites of more than 2000 .o:
6312 config-host.h
2827 include/block/aio.h
4537 include/disas/dis-asm.h
3634 include/exec/cpu-all.h
5177 include/exec/cpu-common.h
3634 include/exec/cpu-defs.h
5401 include/exec/hwaddr.h
5395 include/exec/memattrs.h
5170 include/exec/memory.h
3514 include/exec/memory_ldst.inc.h
3514 include/exec/memory_ldst_cached.inc.h
3514 include/exec/memory_ldst_phys.inc.h
3517 include/exec/ramlist.h
5699 include/fpu/softfloat-types.h
6312 include/glib-compat.h
5429 include/hw/hotplug.h
2423 include/hw/hw.h
5437 include/hw/irq.h
5428 include/hw/qdev-core.h
2520 include/hw/qdev-properties.h
2297 include/hw/qdev.h
2425 include/migration/qemu-file-types.h
2564 include/migration/vmstate.h
2502 include/qapi/error.h
6063 include/qapi/util.h
5939 include/qemu/atomic.h
5494 include/qemu/bitmap.h
5673 include/qemu/bitops.h
5697 include/qemu/bswap.h
6312 include/qemu/compiler.h
3701 include/qemu/coroutine.h
2838 include/qemu/event_notifier.h
5683 include/qemu/host-utils.h
4680 include/qemu/int128.h
3701 include/qemu/lockable.h
3187 include/qemu/log-for-trace.h
2230 include/qemu/log.h
2483 include/qemu/main-loop.h
5568 include/qemu/module.h
5137 include/qemu/notify.h
6312 include/qemu/osdep.h
5561 include/qemu/processor.h
5560 include/qemu/qsp.h
5863 include/qemu/queue.h
5178 include/qemu/rcu.h
5398 include/qemu/rcu_queue.h
5178 include/qemu/sys_membarrier.h
5560 include/qemu/thread-posix.h
5560 include/qemu/thread.h
4356 include/qemu/timer.h
6312 include/qemu/typedefs.h
2008 include/qemu/uuid.h
4518 include/qom/cpu.h
5537 include/qom/object.h
6312 include/sysemu/os-posix.h
2456 include/sysemu/reset.h
2000 include/sysemu/sysemu.h
6062 qapi/qapi-builtin-types.h
2821 qapi/qapi-types-block-core.h
2669 qapi/qapi-types-block.h
3806 qapi/qapi-types-common.h
2860 qapi/qapi-types-crypto.h
2825 qapi/qapi-types-job.h
3038 qapi/qapi-types-misc.h
5249 qapi/qapi-types-run-state.h
3148 qapi/qapi-types-sockets.h
3634 tcg/i386/tcg-target.h
3634 tcg/tcg-mo.h
2369 trace/control-internal.h
2369 trace/control.h
2369 trace/event-internal.h
2361 trace/ftrace.h
5224 x86_64-softmmu/config-target.h
Bulky headers:
size * count size count
17744958 16461 1078 accel/tcg/tcg-runtime.h
32519424 10304 6312 config-host.h
21298950 525900 81 hw/display/trace.h
25070220 1432584 35 hw/net/trace.h
28700355 604218 95 hw/vfio/trace.h
57913922 20486 2827 include/block/aio.h
54939528 30744 1787 include/block/block.h
15698796 53763 292 include/block/block_int.h
12076546 6758 1787 include/block/dirty-bitmap.h
90694630 19990 4537 include/disas/dis-asm.h
22187844 68481 324 include/elf.h
41198658 11337 3634 include/exec/cpu-all.h
19118661 3693 5177 include/exec/cpu-common.h
26052146 7169 3634 include/exec/cpu-defs.h
10698472 11098 964 include/exec/cpu_ldst.h
32181064 19144 1681 include/exec/exec-all.h
14027000 2600 5395 include/exec/memattrs.h
428019130 82789 5170 include/exec/memory.h
12590662 3583 3514 include/exec/memory_ldst.inc.h
13254808 3772 3514 include/exec/memory_ldst_cached.inc.h
18258744 5196 3514 include/exec/memory_ldst_phys.inc.h
37693186 6614 5699 include/fpu/softfloat-types.h
40152640 41480 968 include/fpu/softfloat.h
24408504 3867 6312 include/glib-compat.h
16216423 2987 5429 include/hw/hotplug.h
10178064 1872 5437 include/hw/irq.h
27001315 27137 995 include/hw/pci/pci.h
10509377 10541 997 include/hw/pci/pci_ids.h
84058008 15486 5428 include/hw/qdev-core.h
38099880 15119 2520 include/hw/qdev-properties.h
10165600 4192 2425 include/migration/qemu-file-types.h
140199520 54680 2564 include/migration/vmstate.h
28062432 11216 2502 include/qapi/error.h
14093499 21649 651 include/qapi/visitor.h
122361217 20603 5939 include/qemu/atomic.h
53242354 9691 5494 include/qemu/bitmap.h
91409049 16113 5673 include/qemu/bitops.h
82589409 14497 5697 include/qemu/bswap.h
48009072 7606 6312 include/qemu/compiler.h
32872282 8882 3701 include/qemu/coroutine.h
20290224 11348 1788 include/qemu/hbitmap.h
64189485 11295 5683 include/qemu/host-utils.h
24059880 5141 4680 include/qemu/int128.h
16473456 8474 1944 include/qemu/iov.h
33191317 18553 1789 include/qemu/job.h
11054887 2987 3701 include/qemu/lockable.h
29294434 11798 2483 include/qemu/main-loop.h
14081472 2529 5568 include/qemu/module.h
120111048 19029 6312 include/qemu/osdep.h
14389467 7383 1949 include/qemu/qht.h
142840269 24363 5863 include/qemu/queue.h
24735306 4777 5178 include/qemu/rcu.h
68419650 12675 5398 include/qemu/rcu_queue.h
59775560 10751 5560 include/qemu/thread.h
122708520 28170 4356 include/qemu/timer.h
26573520 4210 6312 include/qemu/typedefs.h
166926546 36947 4518 include/qom/cpu.h
336965209 60857 5537 include/qom/object.h
55562856 55786 996 include/standard-headers/linux/pci_regs.h
12834339 18361 699 include/sysemu/kvm.h
19453584 3082 6312 include/sysemu/os-posix.h
12720000 6360 2000 include/sysemu/sysemu.h
16650270 26429 630 linux-user/qemu.h
53241630 84110 633 linux-user/syscall_defs.h
29144784 1214366 48 migration/trace.h
16888732 5572 6062 qapi/qapi-builtin-types.h
202683208 215544 2821 qapi/qapi-types-block-core.h
11236490 12630 2669 qapi/qapi-types-block.h
15791094 12447 3806 qapi/qapi-types-common.h
21564400 22620 2860 qapi/qapi-types-crypto.h
83229048 82188 3038 qapi/qapi-types-misc.h
11551995 28665 1209 qapi/qapi-types-net.h
25888068 14796 5249 qapi/qapi-types-run-state.h
14465060 13785 3148 qapi/qapi-types-sockets.h
18174224 62526 872 qapi/qapi-types-ui.h
60562920 124872 485 target/arm/cpu.h
18587552 63656 292 target/i386/cpu.h
18575446 36139 514 target/mips/cpu.h
44133336 109512 403 target/ppc/cpu.h
28893934 7951 3634 tcg/i386/tcg-target.h
24384936 51663 472 tcg/tcg-op.h
18304270 11285 1622 tcg/tcg-opc.h
79108184 48772 1622 tcg/tcg.h
64600980 95352 1355 trace-dtrace-root.h
233110135 344074 1355 trace-root.h
22750450 33580 1355 trace-ust-root.h
16220543 6847 2369 trace/control.h
^ permalink raw reply [flat|nested] 24+ messages in thread