Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
Daniel P Berrange writes: > On Tue, Sep 13, 2016 at 08:36:25PM +0200, Lluís Vilanova wrote: >> Daniel P Berrange writes: >> >> > On Thu, Sep 08, 2016 at 03:23:26PM +0200, Lluís Vilanova wrote: >> >> Daniel P Berrange writes: >> >> >> >> > I previously split the global trace-events file up into one file >> >> > per-subdirectory to avoid merge conflict hell. >> >> [...] >> >> >> >> Sorry, I could not find the message where the infrastructure is modified >> >> to >> >> provide this. But I think there's a more efficient way to provide modular >> >> auto-generated tracing code without the hierarchical indexing you >> >> proposed. >> >> > NB, the simpletrace backend requires a globally unique 32-bit integer ID >> > to be assigned to each trace event, so even with the approach you suggest >> > below we still need to be able to assign a global ID for each event. >> >> > So while your suggest below avoids having to pass around the dstate >> > arrays, which is nice, we still have to assign event id offsets to >> > each trace-event file in some manner TBD. >> >> Corect me if I'm wrong, but if we only require these consecutive IDs for >> simpletrace, they don't need to be visible to the tracing headers (these IDs >> are >> only used in "trace/generated-tracers.c"). Therefore, we can get them from >> the >> trace-events-all file, and minimize the complexity of the changes to >> tracetool. > The IDs need to be present in the generated tracers files which are split > up. I'm not sure if I'm just missing what you mean. Changing any of the trace-events files will trigger a recompilation of the .c files specific to the simpletrace backend, but won't trigger a recompile of all QEMU files including any of the tracing headers (just the files including the "trace.h" corresponding to the modified "trace-events"). Cheers, Lluis
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
Daniel P Berrange writes:
> On Fri, Sep 09, 2016 at 03:16:50PM +0200, Lluís Vilanova wrote:
>> > The various _DSTATE variables are still arbitrarily scattered in
>> > memory, as opposed to in a contiguous cache friendly array, which
>> > is one of the goals of Paolo's original change.
>>
>> > That said, I'm unclear on how much of the performance win from
>> > Paolo's change came from eliminating the struct field de-reference,
>> > vs having the contiguous array. I'm guessing most of the win is
>> > from the former, the latter only being important if we hit multiple
>> > related tracepoints on close succession.
>>
>> The latter can also be achieved by packing them all together in a single
>> section, but I don't know if that's acceptable within portable QEMU code. For
>> example:
>>
>> bool ___TRACE_EVENTNAME_DSTATE __attribute__((section("dstate_array")))
> Acutally, it should be sufficient if we just generate all the dstate
> variables in one place in the .c file - the compiler isn't going to
> scatter a set of variables declared one after each other.
That should work.
Lluis
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
On Tue, Sep 13, 2016 at 08:36:25PM +0200, Lluís Vilanova wrote: > Daniel P Berrange writes: > > > On Thu, Sep 08, 2016 at 03:23:26PM +0200, Lluís Vilanova wrote: > >> Daniel P Berrange writes: > >> > >> > I previously split the global trace-events file up into one file > >> > per-subdirectory to avoid merge conflict hell. > >> [...] > >> > >> Sorry, I could not find the message where the infrastructure is modified to > >> provide this. But I think there's a more efficient way to provide modular > >> auto-generated tracing code without the hierarchical indexing you proposed. > > > NB, the simpletrace backend requires a globally unique 32-bit integer ID > > to be assigned to each trace event, so even with the approach you suggest > > below we still need to be able to assign a global ID for each event. > > > So while your suggest below avoids having to pass around the dstate > > arrays, which is nice, we still have to assign event id offsets to > > each trace-event file in some manner TBD. > > Corect me if I'm wrong, but if we only require these consecutive IDs for > simpletrace, they don't need to be visible to the tracing headers (these IDs > are > only used in "trace/generated-tracers.c"). Therefore, we can get them from the > trace-events-all file, and minimize the complexity of the changes to > tracetool. The IDs need to be present in the generated tracers files which are split up. 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 :|
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
Daniel P Berrange writes: > On Thu, Sep 08, 2016 at 03:23:26PM +0200, Lluís Vilanova wrote: >> Daniel P Berrange writes: >> >> > I previously split the global trace-events file up into one file >> > per-subdirectory to avoid merge conflict hell. >> [...] >> >> Sorry, I could not find the message where the infrastructure is modified to >> provide this. But I think there's a more efficient way to provide modular >> auto-generated tracing code without the hierarchical indexing you proposed. > NB, the simpletrace backend requires a globally unique 32-bit integer ID > to be assigned to each trace event, so even with the approach you suggest > below we still need to be able to assign a global ID for each event. > So while your suggest below avoids having to pass around the dstate > arrays, which is nice, we still have to assign event id offsets to > each trace-event file in some manner TBD. Corect me if I'm wrong, but if we only require these consecutive IDs for simpletrace, they don't need to be visible to the tracing headers (these IDs are only used in "trace/generated-tracers.c"). Therefore, we can get them from the trace-events-all file, and minimize the complexity of the changes to tracetool. Cheers, Lluis
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
On Fri, Sep 09, 2016 at 03:16:50PM +0200, Lluís Vilanova wrote:
> > The various _DSTATE variables are still arbitrarily scattered in
> > memory, as opposed to in a contiguous cache friendly array, which
> > is one of the goals of Paolo's original change.
>
> > That said, I'm unclear on how much of the performance win from
> > Paolo's change came from eliminating the struct field de-reference,
> > vs having the contiguous array. I'm guessing most of the win is
> > from the former, the latter only being important if we hit multiple
> > related tracepoints on close succession.
>
> The latter can also be achieved by packing them all together in a single
> section, but I don't know if that's acceptable within portable QEMU code. For
> example:
>
> bool ___TRACE_EVENTNAME_DSTATE __attribute__((section("dstate_array")))
Acutally, it should be sufficient if we just generate all the dstate
variables in one place in the .c file - the compiler isn't going to
scatter a set of variables declared one after each other.
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 :|
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
On Thu, Sep 08, 2016 at 03:23:26PM +0200, Lluís Vilanova wrote:
> Daniel P Berrange writes:
>
> > I previously split the global trace-events file up into one file
> > per-subdirectory to avoid merge conflict hell.
> [...]
>
> Sorry, I could not find the message where the infrastructure is modified to
> provide this. But I think there's a more efficient way to provide modular
> auto-generated tracing code without the hierarchical indexing you proposed.
NB, the simpletrace backend requires a globally unique 32-bit integer ID
to be assigned to each trace event, so even with the approach you suggest
below we still need to be able to assign a global ID for each event.
So while your suggest below avoids having to pass around the dstate
arrays, which is nice, we still have to assign event id offsets to
each trace-event file in some manner TBD.
>
> What about using global variables? Instead of the dstate array, each event
> could
> have this on the "public" header:
>
> /* define for static state */
> #define TRACE_EVENTNAME_ENABLED 1
> /* pointer to event descriptor */
> extern TraceEvent *TRACE_EVENTNAME;
> /* variable with dynamic state */
> extern bool ___TRACE_EVENTNAME_DSTATE;
>
> void trace_eventname(...) {
> if (trace_event_get_stateTRACE_EVENTNAME_ENABLED &&
> ___trace_eventname_dstate) {
> /* ... */
> }
> }
>
> The use of event IDs on generic code can be adapted like this:
>
> #define trace_event_get_state_dynamic_by_id(id) \
> (unlikely(trace_events_enabled_count) && \
>(___ ## id ## _DSTATE))
>
> And then we can concatenate all "trace-events" files to generate the .c files:
>
> struct TraceEvent {
> /* ... */
> bool *dstate;
> };
>
> bool ___TRACE_EVENTNAME_DSTATE;
>
> struct TraceEvent ___trace_events[] = {
> {
> .name = "eventname",
> .sstate = 1,
> .dstate = ___trace_eventname_dstate;
> }
> }
>
> TraceEvent *TRACE_EVENTNAME = &___trace_events[...];
>
> So updating a single "trace-events" file does not force a recompile of the
> whole
> QEMU, but we retain the performance of the flat dstate array (now a per-event
> pointer) and the simpler flat structure for iteration based on event names.
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 :|
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
On Tue, Sep 13, 2016 at 06:05:07PM +0200, Lluís Vilanova wrote: > Daniel P Berrange writes: > > > On Tue, Sep 13, 2016 at 03:54:01PM +0100, Stefan Hajnoczi wrote: > >> Dan: Ping? > >> > >> A few people have reviewed this series. A v2 would be appreciated. I'd > >> like to merge it quickly and as early into the development cycle as > >> possible. > > > Sorry, been focused on other series, but will try and post an update this > > week considering the feedback. > > I can help in implementing the global variable approach I described, if you > want. Thanks for the offer, but the series is hairy enough to rebase that trying to split the work will just create even more rebase pain than there already is. 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 :|
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
Daniel P Berrange writes: > On Tue, Sep 13, 2016 at 03:54:01PM +0100, Stefan Hajnoczi wrote: >> Dan: Ping? >> >> A few people have reviewed this series. A v2 would be appreciated. I'd >> like to merge it quickly and as early into the development cycle as >> possible. > Sorry, been focused on other series, but will try and post an update this > week considering the feedback. I can help in implementing the global variable approach I described, if you want. Cheers, Lluis
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
On Tue, Sep 13, 2016 at 03:54:01PM +0100, Stefan Hajnoczi wrote: > Dan: Ping? > > A few people have reviewed this series. A v2 would be appreciated. I'd > like to merge it quickly and as early into the development cycle as > possible. Sorry, been focused on other series, but will try and post an update this week considering the feedback. 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 :|
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
Dan: Ping? A few people have reviewed this series. A v2 would be appreciated. I'd like to merge it quickly and as early into the development cycle as possible. Stefan signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
Daniel P Berrange writes:
> On Fri, Sep 09, 2016 at 01:03:51PM +0200, Lluís Vilanova wrote:
>> Daniel P Berrange writes:
>>
>> > On Thu, Sep 08, 2016 at 03:23:26PM +0200, Lluís Vilanova wrote:
>> >> Daniel P Berrange writes:
>> >>
>> >> > I previously split the global trace-events file up into one file
>> >> > per-subdirectory to avoid merge conflict hell.
>> >> [...]
>> >>
>> >> Sorry, I could not find the message where the infrastructure is modified
>> >> to
>> >> provide this. But I think there's a more efficient way to provide modular
>> >> auto-generated tracing code without the hierarchical indexing you
>> >> proposed.
>>
>> > [snip]
>>
>> >> struct TraceEvent ___trace_events[] = {
>> >> {
>> >> .name = "eventname",
>> >> .sstate = 1,
>> >> .dstate = ___trace_eventname_dstate;
>> >> }
>> >> }
>> >>
>> >> TraceEvent *TRACE_EVENTNAME = &___trace_events[...];
>>
>> > Life would be simpler if we had the 'bool dstate' as part of the
>> > TraceEvent struct, but doing so would essentially be reverting this
>> > previous change:
>>
>> > commit 585ec7273e6fdab902b2128bc6c2a8136aafef04
>> > Author: Paolo Bonzini
>> > Date: Wed Oct 28 07:06:27 2015 +0100
>>
>> > trace: track enabled events in a separate array
>>
>> > This is more cache friendly on the fast path, where we already have
>> > the event id available.
>>
>> > I asked Paolo about this previously and he indicated it was a notable
>> > performance improvement, so we can't put dstate back into the TraceEvent
>> > struct :-(
>>
>> Sorry, there was a typo in my example code that led to this misunderstanding.
>>
>> I meant this for the global auto-generated .c:
>>
>> struct TraceEvent {
>> /* ... */
>> bool *dstate;
>> };
>>
>> bool ___TRACE_EVENTNAME_DSTATE;
>>
>> struct TraceEvent ___trace_events[] = {
>> {
>> .name = "eventname",
>> .sstate = 1,
>> .dstate = &___TRACE_EVENTNAME_DSTATE;
>> }
>> }
>>
>> TraceEvent *TRACE_EVENTNAME = &___trace_events[...];
>>
>>
>> If you look at the modified macro I pasted for "trace/control-internal.h":
>>
>>
>> #define trace_event_get_state_dynamic_by_id(id) \
>> (unlikely(trace_events_enabled_count) && \
>> (___ ## id ## _DSTATE))
>>
>> We're retaining the fast-path performance of the seggregated boolean dstate
>> array, while the event descriptor contains a pointer to the per-event dstate
>> global boolean variable in case you want to get/set the dstate based on an
>> event
>> pointer.
> The various _DSTATE variables are still arbitrarily scattered in
> memory, as opposed to in a contiguous cache friendly array, which
> is one of the goals of Paolo's original change.
> That said, I'm unclear on how much of the performance win from
> Paolo's change came from eliminating the struct field de-reference,
> vs having the contiguous array. I'm guessing most of the win is
> from the former, the latter only being important if we hit multiple
> related tracepoints on close succession.
The latter can also be achieved by packing them all together in a single
section, but I don't know if that's acceptable within portable QEMU code. For
example:
bool ___TRACE_EVENTNAME_DSTATE __attribute__((section("dstate_array")))
Cheers,
Lluis
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
On Fri, Sep 09, 2016 at 01:03:51PM +0200, Lluís Vilanova wrote:
> Daniel P Berrange writes:
>
> > On Thu, Sep 08, 2016 at 03:23:26PM +0200, Lluís Vilanova wrote:
> >> Daniel P Berrange writes:
> >>
> >> > I previously split the global trace-events file up into one file
> >> > per-subdirectory to avoid merge conflict hell.
> >> [...]
> >>
> >> Sorry, I could not find the message where the infrastructure is modified to
> >> provide this. But I think there's a more efficient way to provide modular
> >> auto-generated tracing code without the hierarchical indexing you proposed.
>
> > [snip]
>
> >> struct TraceEvent ___trace_events[] = {
> >> {
> >> .name = "eventname",
> >> .sstate = 1,
> >> .dstate = ___trace_eventname_dstate;
> >> }
> >> }
> >>
> >> TraceEvent *TRACE_EVENTNAME = &___trace_events[...];
>
> > Life would be simpler if we had the 'bool dstate' as part of the
> > TraceEvent struct, but doing so would essentially be reverting this
> > previous change:
>
> > commit 585ec7273e6fdab902b2128bc6c2a8136aafef04
> > Author: Paolo Bonzini
> > Date: Wed Oct 28 07:06:27 2015 +0100
>
> > trace: track enabled events in a separate array
>
> > This is more cache friendly on the fast path, where we already have
> > the event id available.
>
> > I asked Paolo about this previously and he indicated it was a notable
> > performance improvement, so we can't put dstate back into the TraceEvent
> > struct :-(
>
> Sorry, there was a typo in my example code that led to this misunderstanding.
>
> I meant this for the global auto-generated .c:
>
> struct TraceEvent {
> /* ... */
> bool *dstate;
> };
>
> bool ___TRACE_EVENTNAME_DSTATE;
>
> struct TraceEvent ___trace_events[] = {
> {
> .name = "eventname",
> .sstate = 1,
> .dstate = &___TRACE_EVENTNAME_DSTATE;
> }
> }
>
> TraceEvent *TRACE_EVENTNAME = &___trace_events[...];
>
>
> If you look at the modified macro I pasted for "trace/control-internal.h":
>
>
> #define trace_event_get_state_dynamic_by_id(id) \
> (unlikely(trace_events_enabled_count) && \
>(___ ## id ## _DSTATE))
>
> We're retaining the fast-path performance of the seggregated boolean dstate
> array, while the event descriptor contains a pointer to the per-event dstate
> global boolean variable in case you want to get/set the dstate based on an
> event
> pointer.
The various _DSTATE variables are still arbitrarily scattered in
memory, as opposed to in a contiguous cache friendly array, which
is one of the goals of Paolo's original change.
That said, I'm unclear on how much of the performance win from
Paolo's change came from eliminating the struct field de-reference,
vs having the contiguous array. I'm guessing most of the win is
from the former, the latter only being important if we hit multiple
related tracepoints on close succession.
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 :|
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
Daniel P Berrange writes:
> On Thu, Sep 08, 2016 at 03:23:26PM +0200, Lluís Vilanova wrote:
>> Daniel P Berrange writes:
>>
>> > I previously split the global trace-events file up into one file
>> > per-subdirectory to avoid merge conflict hell.
>> [...]
>>
>> Sorry, I could not find the message where the infrastructure is modified to
>> provide this. But I think there's a more efficient way to provide modular
>> auto-generated tracing code without the hierarchical indexing you proposed.
> [snip]
>> struct TraceEvent ___trace_events[] = {
>> {
>> .name = "eventname",
>> .sstate = 1,
>> .dstate = ___trace_eventname_dstate;
>> }
>> }
>>
>> TraceEvent *TRACE_EVENTNAME = &___trace_events[...];
> Life would be simpler if we had the 'bool dstate' as part of the
> TraceEvent struct, but doing so would essentially be reverting this
> previous change:
> commit 585ec7273e6fdab902b2128bc6c2a8136aafef04
> Author: Paolo Bonzini
> Date: Wed Oct 28 07:06:27 2015 +0100
> trace: track enabled events in a separate array
> This is more cache friendly on the fast path, where we already have
> the event id available.
> I asked Paolo about this previously and he indicated it was a notable
> performance improvement, so we can't put dstate back into the TraceEvent
> struct :-(
Sorry, there was a typo in my example code that led to this misunderstanding.
I meant this for the global auto-generated .c:
struct TraceEvent {
/* ... */
bool *dstate;
};
bool ___TRACE_EVENTNAME_DSTATE;
struct TraceEvent ___trace_events[] = {
{
.name = "eventname",
.sstate = 1,
.dstate = &___TRACE_EVENTNAME_DSTATE;
}
}
TraceEvent *TRACE_EVENTNAME = &___trace_events[...];
If you look at the modified macro I pasted for "trace/control-internal.h":
#define trace_event_get_state_dynamic_by_id(id) \
(unlikely(trace_events_enabled_count) && \
(___ ## id ## _DSTATE))
We're retaining the fast-path performance of the seggregated boolean dstate
array, while the event descriptor contains a pointer to the per-event dstate
global boolean variable in case you want to get/set the dstate based on an event
pointer.
Cheers,
Lluis
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
Daniel P Berrange writes:
> I previously split the global trace-events file up into one file
> per-subdirectory to avoid merge conflict hell.
[...]
Sorry, I could not find the message where the infrastructure is modified to
provide this. But I think there's a more efficient way to provide modular
auto-generated tracing code without the hierarchical indexing you proposed.
What about using global variables? Instead of the dstate array, each event could
have this on the "public" header:
/* define for static state */
#define TRACE_EVENTNAME_ENABLED 1
/* pointer to event descriptor */
extern TraceEvent *TRACE_EVENTNAME;
/* variable with dynamic state */
extern bool ___TRACE_EVENTNAME_DSTATE;
void trace_eventname(...) {
if (trace_event_get_stateTRACE_EVENTNAME_ENABLED &&
___trace_eventname_dstate) {
/* ... */
}
}
The use of event IDs on generic code can be adapted like this:
#define trace_event_get_state_dynamic_by_id(id) \
(unlikely(trace_events_enabled_count) && \
(___ ## id ## _DSTATE))
And then we can concatenate all "trace-events" files to generate the .c files:
struct TraceEvent {
/* ... */
bool *dstate;
};
bool ___TRACE_EVENTNAME_DSTATE;
struct TraceEvent ___trace_events[] = {
{
.name = "eventname",
.sstate = 1,
.dstate = ___trace_eventname_dstate;
}
}
TraceEvent *TRACE_EVENTNAME = &___trace_events[...];
So updating a single "trace-events" file does not force a recompile of the whole
QEMU, but we retain the performance of the flat dstate array (now a per-event
pointer) and the simpler flat structure for iteration based on event names.
Cheers,
Lluis
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
On Thu, Sep 08, 2016 at 03:23:26PM +0200, Lluís Vilanova wrote:
> Daniel P Berrange writes:
>
> > I previously split the global trace-events file up into one file
> > per-subdirectory to avoid merge conflict hell.
> [...]
>
> Sorry, I could not find the message where the infrastructure is modified to
> provide this. But I think there's a more efficient way to provide modular
> auto-generated tracing code without the hierarchical indexing you proposed.
[snip]
> struct TraceEvent ___trace_events[] = {
> {
> .name = "eventname",
> .sstate = 1,
> .dstate = ___trace_eventname_dstate;
> }
> }
>
> TraceEvent *TRACE_EVENTNAME = &___trace_events[...];
Life would be simpler if we had the 'bool dstate' as part of the
TraceEvent struct, but doing so would essentially be reverting this
previous change:
commit 585ec7273e6fdab902b2128bc6c2a8136aafef04
Author: Paolo Bonzini
Date: Wed Oct 28 07:06:27 2015 +0100
trace: track enabled events in a separate array
This is more cache friendly on the fast path, where we already have
the event id available.
I asked Paolo about this previously and he indicated it was a notable
performance improvement, so we can't put dstate back into the TraceEvent
struct :-(
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 :|
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
On 10/08/2016 10:39, Fam Zheng wrote: > Then the script needs to be updated to work with different package management > systems (rpm and deb). For now they only call standard Linux commands. That's > least of the problem, though. > > The more tricky question is how the script can tell which packages are > relevant > (or, used). I assume the number of packages in a docker image is quite limited > (fedora has 396, ubuntu 574), but dumping the whole list is still noisy > nevertheless. Also, depending on the configure option, a submodule can be used > instead, or a local built library. Not that we have a lot of these now, but > it's a factor of complication. > > On the other hand, configure knows when to use pkg-config --modversion, or git > describe, or something else, depending on how and where it discovers the > compiler and -devel libs etc.. Can you include a description of how to reproduce with "make docker-test-FOO@BAR DEBUG=1"? Thanks, Paolo
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
On Wed, Aug 10, 2016 at 04:05:06PM +0200, Lluís Vilanova wrote: > Daniel P Berrange writes: > > > I previously split the global trace-events file up into one file > > per-subdirectory to avoid merge conflict hell. > > > This series builds on that to now actually generate the individual > > trace files per-subdirectory too. The key benefit of doing this is > > that a change in a trace-events file will no longer cause a rebuild > > of the entire world. Instead only that one affected sub-directory > > will have files rebuilt. > > Is re-compilation time worth the effort? Absolutely yes, especially if you have build for multiple targets enabled. Rebuilding all files when you have all 30+ targets enabled takes an incredibly long time. 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 :|
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
Daniel P Berrange writes: > I previously split the global trace-events file up into one file > per-subdirectory to avoid merge conflict hell. > This series builds on that to now actually generate the individual > trace files per-subdirectory too. The key benefit of doing this is > that a change in a trace-events file will no longer cause a rebuild > of the entire world. Instead only that one affected sub-directory > will have files rebuilt. Is re-compilation time worth the effort? Cheers, Lluis
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
On Wed, Aug 10, 2016 at 04:39:21PM +0800, Fam Zheng wrote: > On Wed, 08/10 09:24, Daniel P. Berrange wrote: > > On Wed, Aug 10, 2016 at 04:13:10PM +0800, Fam Zheng wrote: > > > On Wed, 08/10 09:06, Daniel P. Berrange wrote: > > > > On Wed, Aug 10, 2016 at 09:41:40AM +0800, Fam Zheng wrote: > > > > > On Tue, 08/09 18:11, Daniel P. Berrange wrote: > > > > > > > > > > > > Can we get this report to include details of the > > > > > > > > > > > > a) the software versions of gcc, binutils, and any -devel packages > > > > > > we're building against > > > > > > > > > > > > b) the exact arguments + env variables used to invoke configure, > > > > > > not merely its output > > > > > > > > > > > > so we don't have to go digging into the docker test systemm to > > > > > > try and reverse engineer this info > > > > > > > > > > The whole point of docker test system is offer a relatively easy > > > > > reproducer to > > > > > developers, so I'm reluctant to engineer patchew or the test script > > > > > it runs for > > > > > that. > > > > > > > > That's just pointlessly creating extra work for the developers reading > > > > these build reports. If you outputted the info I suggest, it can help > > > > developers diagnose the problems more quickly. Even if running the > > > > docker env locally is possible & even easy, it doesn't make it faster > > > > than reading the relevant info from email report in front of you. > > > > > > I agree with your point, I just don't know how to do a) neatly, except > > > for a > > > vast change to configure. > > > > Why do you need todo it in configure ? I was thinking your docker receipe > > that invokes configure could just do it - not least because the commands to > > run to get the data will be different based on which distro you're running > > in the docker image in question. > > Then the script needs to be updated to work with different package management > systems (rpm and deb). For now they only call standard Linux commands. That's > least of the problem, though. > > The more tricky question is how the script can tell which packages are > relevant > (or, used). I assume the number of packages in a docker image is quite limited > (fedora has 396, ubuntu 574), but dumping the whole list is still noisy > nevertheless. Also, depending on the configure option, a submodule can be used > instead, or a local built library. Not that we have a lot of these now, but > it's a factor of complication. > > On the other hand, configure knows when to use pkg-config --modversion, or git > describe, or something else, depending on how and where it discovers the > compiler and -devel libs etc.. In the tests/docker/dockerfiles/*.docker files you install a bunch of RPMs we build against. I was just thinking to report versions of those particular RPMs. IIt would be sufficient if you just captured the output of the 'dnf install' command (or equivalent) to, say /tmp/installed.txt. Then have build_qemu() in common.rc simply cat /tmp/installed.txt before running configure. That way all the distro-specific bits stay in the *.docker files. 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 :|
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
On Wed, 08/10 09:24, Daniel P. Berrange wrote: > On Wed, Aug 10, 2016 at 04:13:10PM +0800, Fam Zheng wrote: > > On Wed, 08/10 09:06, Daniel P. Berrange wrote: > > > On Wed, Aug 10, 2016 at 09:41:40AM +0800, Fam Zheng wrote: > > > > On Tue, 08/09 18:11, Daniel P. Berrange wrote: > > > > > > > > > > Can we get this report to include details of the > > > > > > > > > > a) the software versions of gcc, binutils, and any -devel packages > > > > > we're building against > > > > > > > > > > b) the exact arguments + env variables used to invoke configure, > > > > > not merely its output > > > > > > > > > > so we don't have to go digging into the docker test systemm to > > > > > try and reverse engineer this info > > > > > > > > The whole point of docker test system is offer a relatively easy > > > > reproducer to > > > > developers, so I'm reluctant to engineer patchew or the test script it > > > > runs for > > > > that. > > > > > > That's just pointlessly creating extra work for the developers reading > > > these build reports. If you outputted the info I suggest, it can help > > > developers diagnose the problems more quickly. Even if running the > > > docker env locally is possible & even easy, it doesn't make it faster > > > than reading the relevant info from email report in front of you. > > > > I agree with your point, I just don't know how to do a) neatly, except for a > > vast change to configure. > > Why do you need todo it in configure ? I was thinking your docker receipe > that invokes configure could just do it - not least because the commands to > run to get the data will be different based on which distro you're running > in the docker image in question. Then the script needs to be updated to work with different package management systems (rpm and deb). For now they only call standard Linux commands. That's least of the problem, though. The more tricky question is how the script can tell which packages are relevant (or, used). I assume the number of packages in a docker image is quite limited (fedora has 396, ubuntu 574), but dumping the whole list is still noisy nevertheless. Also, depending on the configure option, a submodule can be used instead, or a local built library. Not that we have a lot of these now, but it's a factor of complication. On the other hand, configure knows when to use pkg-config --modversion, or git describe, or something else, depending on how and where it discovers the compiler and -devel libs etc.. Fam
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
On Wed, Aug 10, 2016 at 04:13:10PM +0800, Fam Zheng wrote: > On Wed, 08/10 09:06, Daniel P. Berrange wrote: > > On Wed, Aug 10, 2016 at 09:41:40AM +0800, Fam Zheng wrote: > > > On Tue, 08/09 18:11, Daniel P. Berrange wrote: > > > > > > > > Can we get this report to include details of the > > > > > > > > a) the software versions of gcc, binutils, and any -devel packages > > > > we're building against > > > > > > > > b) the exact arguments + env variables used to invoke configure, > > > > not merely its output > > > > > > > > so we don't have to go digging into the docker test systemm to > > > > try and reverse engineer this info > > > > > > The whole point of docker test system is offer a relatively easy > > > reproducer to > > > developers, so I'm reluctant to engineer patchew or the test script it > > > runs for > > > that. > > > > That's just pointlessly creating extra work for the developers reading > > these build reports. If you outputted the info I suggest, it can help > > developers diagnose the problems more quickly. Even if running the > > docker env locally is possible & even easy, it doesn't make it faster > > than reading the relevant info from email report in front of you. > > I agree with your point, I just don't know how to do a) neatly, except for a > vast change to configure. Why do you need todo it in configure ? I was thinking your docker receipe that invokes configure could just do it - not least because the commands to run to get the data will be different based on which distro you're running in the docker image in question. 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 :|
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
On Wed, 08/10 09:06, Daniel P. Berrange wrote: > On Wed, Aug 10, 2016 at 09:41:40AM +0800, Fam Zheng wrote: > > On Tue, 08/09 18:11, Daniel P. Berrange wrote: > > > > > > Can we get this report to include details of the > > > > > > a) the software versions of gcc, binutils, and any -devel packages > > > we're building against > > > > > > b) the exact arguments + env variables used to invoke configure, > > > not merely its output > > > > > > so we don't have to go digging into the docker test systemm to > > > try and reverse engineer this info > > > > The whole point of docker test system is offer a relatively easy reproducer > > to > > developers, so I'm reluctant to engineer patchew or the test script it runs > > for > > that. > > That's just pointlessly creating extra work for the developers reading > these build reports. If you outputted the info I suggest, it can help > developers diagnose the problems more quickly. Even if running the > docker env locally is possible & even easy, it doesn't make it faster > than reading the relevant info from email report in front of you. I agree with your point, I just don't know how to do a) neatly, except for a vast change to configure. Fam
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
On Wed, Aug 10, 2016 at 09:41:40AM +0800, Fam Zheng wrote: > On Tue, 08/09 18:11, Daniel P. Berrange wrote: > > > > Can we get this report to include details of the > > > > a) the software versions of gcc, binutils, and any -devel packages > > we're building against > > > > b) the exact arguments + env variables used to invoke configure, > > not merely its output > > > > so we don't have to go digging into the docker test systemm to > > try and reverse engineer this info > > The whole point of docker test system is offer a relatively easy reproducer to > developers, so I'm reluctant to engineer patchew or the test script it runs > for > that. That's just pointlessly creating extra work for the developers reading these build reports. If you outputted the info I suggest, it can help developers diagnose the problems more quickly. Even if running the docker env locally is possible & even easy, it doesn't make it faster than reading the relevant info from email report in front of you. 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 :|
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
On Tue, 08/09 18:11, Daniel P. Berrange wrote: > so we don't have to go digging into the docker test systemm to > try and reverse engineer this info Actually, digging this info is pretty easy if you already use docker test locally (if you don't, may I ask why?). Just append DEBUG=1 and you can mess with things in the building env. Something like this $ make docker-test-quick@centos6 DEBUG=1 Then you'll be dropped in the shell that is all set up to run the build command. Fam
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
On Tue, 08/09 18:11, Daniel P. Berrange wrote: > > Can we get this report to include details of the > > a) the software versions of gcc, binutils, and any -devel packages > we're building against > > b) the exact arguments + env variables used to invoke configure, > not merely its output > > so we don't have to go digging into the docker test systemm to > try and reverse engineer this info The whole point of docker test system is offer a relatively easy reproducer to developers, so I'm reluctant to engineer patchew or the test script it runs for that. On the other hand, it's reasonable to allow the docker testing code to be more verbose than currently. Or maybe we can patch ./configure to output these information? Fam
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
On Tue, Aug 09, 2016 at 10:03:49AM -0700, no-re...@ec2-52-6-146-230.compute-1.amazonaws.com wrote: > Hi, > > Your series failed automatic build test. Please find the testing commands and > their output below. If you have docker installed, you can probably reproduce > it > locally. > > Message-id: 1470756748-18933-1-git-send-email-berra...@redhat.com > Type: series > Subject: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event > files > > === TEST SCRIPT BEGIN === > #!/bin/bash > set -e > git submodule update --init dtc > make J=8 docker-test-quick@centos6 > > # we need CURL DPRINTF patch > # http://patchew.org/QEMU/1470027888-24381-1-git-send-email-famz%40redhat.com/ > #make J=8 docker-test-mingw@fedora [snip] > === OUTPUT BEGIN === > Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' > Cloning into 'dtc'... > Submodule path 'dtc': checked out '65cc4d2748a2c2e6f27f1cf39e07a5dbabd80ebf' > BUILD centos6 > ARCHIVE qemu.tgz > ARCHIVE dtc.tgz > COPY RUNNER > RUN test-quick in centos6 Can we get this report to include details of the a) the software versions of gcc, binutils, and any -devel packages we're building against b) the exact arguments + env variables used to invoke configure, not merely its output so we don't have to go digging into the docker test systemm to try and reverse engineer this info > No C++ compiler available; disabling C++ specific optional code > Install prefix/tmp/qemu-test/src/tests/docker/install > BIOS directory/tmp/qemu-test/src/tests/docker/install/share/qemu > binary directory /tmp/qemu-test/src/tests/docker/install/bin > library directory /tmp/qemu-test/src/tests/docker/install/lib > module directory /tmp/qemu-test/src/tests/docker/install/lib/qemu > libexec directory /tmp/qemu-test/src/tests/docker/install/libexec > include directory /tmp/qemu-test/src/tests/docker/install/include > config directory /tmp/qemu-test/src/tests/docker/install/etc > local state directory /tmp/qemu-test/src/tests/docker/install/var > Manual directory /tmp/qemu-test/src/tests/docker/install/share/man > ELF interp prefix /usr/gnemul/qemu-%M > Source path /tmp/qemu-test/src > C compilercc > Host C compiler cc > C++ compiler > Objective-C compiler cc > ARFLAGS rv > CFLAGS-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -pthread > -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -g > QEMU_CFLAGS -I/usr/include/pixman-1-fPIE -DPIE -m64 -D_GNU_SOURCE > -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes > -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes > -fno-strict-aliasing -fno-common -Wendif-labels -Wmissing-include-dirs > -Wempty-body -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self > -Wignored-qualifiers -Wold-style-declaration -Wold-style-definition > -Wtype-limits -fstack-protector-all > LDFLAGS -Wl,--warn-common -Wl,-z,relro -Wl,-z,now -pie -m64 -g > make make > install install > pythonpython -B > smbd /usr/sbin/smbd > module supportno > host CPU x86_64 > host big endian no > target list x86_64-softmmu aarch64-softmmu > tcg debug enabled no > gprof enabled no > sparse enabledno > strip binariesyes > profiler no > static build no > pixmansystem > SDL support yes (1.2.14) > GTK support no > GTK GL supportno > VTE support no > TLS priority NORMAL > GNUTLS supportno > GNUTLS rndno > libgcrypt no > libgcrypt kdf no > nettleno > nettle kdfno > libtasn1 no > curses supportno > virgl support no > curl support no > mingw32 support no > Audio drivers oss > Block whitelist (rw) > Block whitelist (ro) > VirtFS supportno > VNC support yes > VNC SASL support no > VNC JPEG support no > VNC PNG support no > xen support no > brlapi supportno > bluez supportno > Documentation no > PIE yes > vde support no > netmap supportno > Linux AIO support no > ATTR/XATTR support yes > Install blobs yes > KVM support yes > RDMA support no > TCG interpreter no > fdt support yes > preadv supportyes > fdatasync yes > madvise yes > posix_madvise yes > uuid support no > libcap-ng support no > vhost-net support yes > vhost-scsi support yes > Trace backendslog > spice support no > rbd support no > xfsctl supportno > smartcard support no > libusbno > usb net redir no > OpenGL supportno > OpenGL dmabufsno > libiscsi support no > libnfs supportno > build guest agent yes > QGA VSS support no > QGA w32 disk info no > QGA MSI support no > seccomp support no > coroutine backend ucontext > coroutine poolyes > GlusterFS support no > Archipelago support no > gcov
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
Hi, Your series failed automatic build test. Please find the testing commands and their output below. If you have docker installed, you can probably reproduce it locally. Message-id: 1470756748-18933-1-git-send-email-berra...@redhat.com Type: series Subject: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files === TEST SCRIPT BEGIN === #!/bin/bash set -e git submodule update --init dtc make J=8 docker-test-quick@centos6 # we need CURL DPRINTF patch # http://patchew.org/QEMU/1470027888-24381-1-git-send-email-famz%40redhat.com/ #make J=8 docker-test-mingw@fedora === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/1470756748-18933-1-git-send-email-berra...@redhat.com -> patchew/1470756748-18933-1-git-send-email-berra...@redhat.com Switched to a new branch 'test' f782d9f trace: update docs to reflect new code generation approach 8b58708 trace: remove the global include/trace.h file 19403bf trace: switch linux-user/ directory to modular trace.h file 0a68cb2 trace: switch qom/ directory to modular trace.h file da8cde7 trace: switch target-ppc/ directory to modular trace.h file 172fdf5 trace: switch target-s390x/ directory to modular trace.h file 001334e trace: switch target-sparc/ directory to modular trace.h file fbf7e8d trace: switch target-i386/ directory to modular trace.h file 5b3a404 trace: switch net/ directory to modular trace.h file e4008a1 trace: switch audio/ directory to modular trace.h file 0d0c118 trace: switch ui/ directory to modular trace.h file a424af5 trace: switch hw/alpha/ directory to modular trace.h file 6899dbc trace: switch hw/arm/ directory to modular trace.h file 698e90e trace: switch hw/acpi/ directory to modular trace.h file 6798bcb trace: switch hw/vfio/ directory to modular trace.h file 777af60 trace: switch hw/s390x/ directory to modular trace.h file 0d37f07 trace: switch hw/pci/ directory to modular trace.h file 8a745ba trace: switch hw/ppc/ directory to modular trace.h file d315fff trace: switch hw/9pfs/ directory to modular trace.h file 200d752 trace: switch hw/i386/ directory to modular trace.h file 5963cc6 trace: switch hw/mem/ directory to modular trace.h file 3cd532b trace: switch hw/isa/ directory to modular trace.h file 6e0943c trace: switch hw/sd/ directory to modular trace.h file d48f418 trace: switch hw/sparc/ directory to modular trace.h file f30fb3c trace: switch hw/dma/ directory to modular trace.h file aeff5ce trace: switch hw/timer/ directory to modular trace.h file 0a357f4 trace: switch hw/input/ directory to modular trace.h file 788e458 trace: switch hw/display/ directory to modular trace.h file 9e0ec18 trace: switch hw/nvram/ directory to modular trace.h file f66d633 trace: switch hw/scsi/ directory to modular trace.h file 2c27a5c trace: switch hw/usb/ directory to modular trace.h file c2c6a4a trace: switch hw/misc/ directory to modular trace.h file 5e51950 trace: switch hw/audio/ directory to modular trace.h file 24e7612 trace: switch hw/virtio/ directory to modular trace.h file 3f96b32 trace: switch hw/net/ directory to modular trace.h file 6cb5b07 trace: switch hw/intc/ directory to modular trace.h file 7138800 trace: switch hw/char/ directory to modular trace.h file 8e60ca5 trace: switch hw/block/ directory to modular trace.h file ce95a93 trace: switch block/ directory to modular trace.h file c1f9359 trace: switch migration/ directory to modular trace.h file a9399ee trace: switch crypto/ directory to modular trace.h file cf9d0b1 trace: switch util/ directory to modular trace.h file b22e603 trace: switch io/ directory to modular trace.h file b7dfca1 trace: introduce some Makefile rules for module code gen eb94362 trace: introduce ID range offsets per trace-events file 5b37803 trace: introduce a formal group name for trace events 85db364 trace: get rid of generated-events.h/generated-events.c b2a27bd trace: remove generated-events.h from many includes 08d783c3 trace: use -1 instead of TRACE_VCPU_EVENT_COUNT as magic value 3c2bc9c trace: remove use of event ID enums from APIs 35a51ca trace: remove fixed global event state arrays 584727d trace: remove use of TRACE_VCPU_EVENT_COUNT in cpu.h cc33976 trace: provide mechanism for registering trace events 832db43 trace: remove some now unused functions 59adbd8 trace: convert code to use event iterators 58f9e9d trace: add trace event iterator APIs 969db6a trace: move hw/virtio/virtio-balloon.c trace points into correct file 2a9286e trace: move hw/mem/pc-dimm.c trace points into correct file b8fe348 trace: move util/qemu-coroutine*.c trace points into correct file b4b74ca trace: move util/buffer.c trace points into correct file === OUTPUT BEGIN === Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' Cloning into 'dtc'... Submodule path 'dtc': checked out '65cc4d2748a2c2e6f27f1cf39e07a5dbabd80ebf' BUILD centos6 ARCHIVE qemu.tgz ARCHIVE dtc.tgz COPY RUNNER RUN
