Re: [kvm-devel] [PATCH 1/5]Add some trace markers and exposeinterfaces in kernel for tracing

[Hollis B. <ho...@us...>]

On Tuesday 15 April 2008 22:13:28 Liu, Eric E wrote:
> Hollis Blanchard wrote:
> > On Wednesday 09 April 2008 05:01:36 Liu, Eric E wrote:
> >> +/* This structure represents a single trace buffer record. */
> >> +struct kvm_trace_rec { +       __u32 event:28;
> >> +       __u32 extra_u32:3;
> >> +       __u32 cycle_in:1;
> >> +       __u32 pid;
> >> +       __u32 vcpu_id;
> >> +       union {
> >> +               struct {
> >> +                       __u32 cycle_lo, cycle_hi;
> >> +                       __u32 extra_u32[KVM_TRC_EXTRA_MAX];
> >> +               } cycle; +               struct {
> >> +                       __u32 extra_u32[KVM_TRC_EXTRA_MAX];
> >> +               } nocycle; +       } u;
> >> +};
> > 
> > Do we really need bitfields here? They are notoriously non-portable.
> > 
> > Practically speaking, this will prevent me from copying a trace file
> > from my big-endian target to my little-endian workstation for
> > analysis, at least without some ugly hacking in the userland tool.
> Here the main consideration using bitfields is to save storage space for 
each record, but as you said it is non-portable for your mentioned case, so 
should we need to adjust the struct like this? 
> 	__u32 event;
> 	__16 extra_u32;
> 	__16 cycle_in;

If space really is a worry, you could still combine the fields, and just use 
masks to extract the data later. No matter what, byteswapping is required in 
the userland tool. I suspect this isn't there already, but it will be easier 
to add without the bitfields.

Hmm, while we're on the subject, I'm not sure what the best way to 
automatically byteswap will be. It probably isn't worth it to convert all 
trace data to a standard ordering (which would add overhead to tracing), but 
I suppose there is no metadata in the trace log? A command line switch might 
be inconvenient but inevitable.

-- 
Hollis Blanchard
IBM Linux Technology Center