Re: [PATCH 07/15] ftrace: fix event alignment: kvm:kvm_hv_hypercall

Re: [PATCH 07/15] ftrace: fix event alignment: kvm:kvm_hv_hypercall

[Avi Kivity]

On 12/04/2010 02:13 AM, David Sharp wrote:
> Signed-off-by: David Sharp<dhsharp@google.com>
> ---
>   arch/x86/kvm/trace.h |    8 ++++----
>   1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> index a6544b8..ab41fb0 100644
> --- a/arch/x86/kvm/trace.h
> +++ b/arch/x86/kvm/trace.h
> @@ -62,21 +62,21 @@ TRACE_EVENT(kvm_hv_hypercall,
>   	TP_ARGS(code, fast, rep_cnt, rep_idx, ingpa, outgpa),
>
>   	TP_STRUCT__entry(
> -		__field(	__u16, 		code		)
> -		__field(	bool,		fast		)
>   		__field(	__u16,		rep_cnt		)
>   		__field(	__u16,		rep_idx		)
>   		__field(	__u64,		ingpa		)
>   		__field(	__u64,		outgpa		)
> +		__field(	__u16, 		code		)
> +		__field(	bool,		fast		)
>   	),
>

Looks like a pessimisation.

Before: 24 bytes
After: 32 bytes

(on a 64-bit machine, assuming no packing)

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH 07/15] ftrace: fix event alignment: kvm:kvm_hv_hypercall

[David Sharp]

On Sat, Dec 4, 2010 at 12:11 AM, Avi Kivity <avi@redhat.com> wrote:
> On 12/04/2010 02:13 AM, David Sharp wrote:
>>
>> Signed-off-by: David Sharp<dhsharp@google.com>
>> ---
>>  arch/x86/kvm/trace.h |    8 ++++----
>>  1 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
>> index a6544b8..ab41fb0 100644
>> --- a/arch/x86/kvm/trace.h
>> +++ b/arch/x86/kvm/trace.h
>> @@ -62,21 +62,21 @@ TRACE_EVENT(kvm_hv_hypercall,
>>        TP_ARGS(code, fast, rep_cnt, rep_idx, ingpa, outgpa),
>>
>>        TP_STRUCT__entry(
>> -               __field(        __u16,          code            )
>> -               __field(        bool,           fast            )
>>                __field(        __u16,          rep_cnt         )
>>                __field(        __u16,          rep_idx         )
>>                __field(        __u64,          ingpa           )
>>                __field(        __u64,          outgpa          )
>> +               __field(        __u16,          code            )
>> +               __field(        bool,           fast            )
>>        ),
>>
>
> Looks like a pessimisation.
>
> Before: 24 bytes
> After: 32 bytes
>
> (on a 64-bit machine, assuming no packing)

This patch is predicated on packing the event structures. And since
the ring buffer is 32-bit addressable, I don't attempt to improve
alignment beyond 32-bit boundaries.

>
> --
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.
>
>

Re: [PATCH 07/15] ftrace: fix event alignment: kvm:kvm_hv_hypercall

[Avi Kivity]

On 12/06/2010 10:38 PM, David Sharp wrote:
> On Sat, Dec 4, 2010 at 12:11 AM, Avi Kivity<avi@redhat.com>  wrote:
> >  On 12/04/2010 02:13 AM, David Sharp wrote:
> >>
> >>  Signed-off-by: David Sharp<dhsharp@google.com>
> >>  ---
> >>    arch/x86/kvm/trace.h |    8 ++++----
> >>    1 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >>  diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> >>  index a6544b8..ab41fb0 100644
> >>  --- a/arch/x86/kvm/trace.h
> >>  +++ b/arch/x86/kvm/trace.h
> >>  @@ -62,21 +62,21 @@ TRACE_EVENT(kvm_hv_hypercall,
> >>          TP_ARGS(code, fast, rep_cnt, rep_idx, ingpa, outgpa),
> >>
> >>          TP_STRUCT__entry(
> >>  -               __field(        __u16,          code            )
> >>  -               __field(        bool,           fast            )
> >>                  __field(        __u16,          rep_cnt         )
> >>                  __field(        __u16,          rep_idx         )
> >>                  __field(        __u64,          ingpa           )
> >>                  __field(        __u64,          outgpa          )
> >>  +               __field(        __u16,          code            )
> >>  +               __field(        bool,           fast            )
> >>          ),
> >>
> >
> >  Looks like a pessimisation.
> >
> >  Before: 24 bytes
> >  After: 32 bytes
> >
> >  (on a 64-bit machine, assuming no packing)
>
> This patch is predicated on packing the event structures. And since
> the ring buffer is 32-bit addressable, I don't attempt to improve
> alignment beyond 32-bit boundaries.

I don't understand this.  Can you elaborate?  What does "32-bit 
addressable" mean?  And "predicated on packing the event structures"?  
Is the structure __attribute__((packed)), or is it not?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 07/15] ftrace: fix event alignment: kvm:kvm_hv_hypercall

[David Sharp]

On Tue, Dec 7, 2010 at 1:22 AM, Avi Kivity <avi@redhat.com> wrote:
> On 12/06/2010 10:38 PM, David Sharp wrote:
>> On Sat, Dec 4, 2010 at 12:11 AM, Avi Kivity<avi@redhat.com>  wrote:
>> >  On 12/04/2010 02:13 AM, David Sharp wrote:
>> >>
>> >>  Signed-off-by: David Sharp<dhsharp@google.com>
>> >>  ---
>> >>    arch/x86/kvm/trace.h |    8 ++++----
>> >>    1 files changed, 4 insertions(+), 4 deletions(-)
>> >>
>> >>  diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
>> >>  index a6544b8..ab41fb0 100644
>> >>  --- a/arch/x86/kvm/trace.h
>> >>  +++ b/arch/x86/kvm/trace.h
>> >>  @@ -62,21 +62,21 @@ TRACE_EVENT(kvm_hv_hypercall,
>> >>          TP_ARGS(code, fast, rep_cnt, rep_idx, ingpa, outgpa),
>> >>
>> >>          TP_STRUCT__entry(
>> >>  -               __field(        __u16,          code            )
>> >>  -               __field(        bool,           fast            )
>> >>                  __field(        __u16,          rep_cnt         )
>> >>                  __field(        __u16,          rep_idx         )
>> >>                  __field(        __u64,          ingpa           )
>> >>                  __field(        __u64,          outgpa          )
>> >>  +               __field(        __u16,          code            )
>> >>  +               __field(        bool,           fast            )
>> >>          ),
>> >>
>> >
>> >  Looks like a pessimisation.
>> >
>> >  Before: 24 bytes
>> >  After: 32 bytes
>> >
>> >  (on a 64-bit machine, assuming no packing)
>>
>> This patch is predicated on packing the event structures. And since
>> the ring buffer is 32-bit addressable, I don't attempt to improve
>> alignment beyond 32-bit boundaries.
>
> I don't understand this.  Can you elaborate?  What does "32-bit addressable"
> mean?

The ring buffer gives you space that is a multiple of 4 bytes in
length, and 32-bit aligned. Therefore it is useless to attempt to
align the structure beyond 32-bit boundaries, eg, a 64-bit boundary,
because it is unpredictable if the memory the structure will be
written to is at a 64-bit boundary (addr % 8 could be 0 or 4).

> And "predicated on packing the event structures"?  Is the structure
> __attribute__((packed)), or is it not?

It is not packed in Linus' tree, but one of the patches before this
patch in this patch series adds __attribute__((packed)). This patch
assumes that the event packing patch has been applied. This patch
should not be applied if the packing patch is not (hence,
"predicated").

>
> --
> error compiling committee.c: too many arguments to function
>
>

Re: [PATCH 07/15] ftrace: fix event alignment: kvm:kvm_hv_hypercall

[Avi Kivity]

On 12/07/2010 11:16 PM, David Sharp wrote:
> >
> >  I don't understand this.  Can you elaborate?  What does "32-bit addressable"
> >  mean?
>
> The ring buffer gives you space that is a multiple of 4 bytes in
> length, and 32-bit aligned. Therefore it is useless to attempt to
> align the structure beyond 32-bit boundaries, eg, a 64-bit boundary,
> because it is unpredictable if the memory the structure will be
> written to is at a 64-bit boundary (addr % 8 could be 0 or 4).
>
> >  And "predicated on packing the event structures"?  Is the structure
> >  __attribute__((packed)), or is it not?
>
> It is not packed in Linus' tree, but one of the patches before this
> patch in this patch series adds __attribute__((packed)). This patch
> assumes that the event packing patch has been applied. This patch
> should not be applied if the packing patch is not (hence,
> "predicated").

Thanks for the explanations, it makes sense now.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 07/15] ftrace: fix event alignment: kvm:kvm_hv_hypercall

[Steven Rostedt]

Hi Avi,

This patch set got lost in the shuffle, and I'm not looking to include
it.

On Wed, 2010-12-08 at 11:18 +0200, Avi Kivity wrote:
> On 12/07/2010 11:16 PM, David Sharp wrote:
> > >
> > >  I don't understand this.  Can you elaborate?  What does "32-bit addressable"
> > >  mean?
> >
> > The ring buffer gives you space that is a multiple of 4 bytes in
> > length, and 32-bit aligned. Therefore it is useless to attempt to
> > align the structure beyond 32-bit boundaries, eg, a 64-bit boundary,
> > because it is unpredictable if the memory the structure will be
> > written to is at a 64-bit boundary (addr % 8 could be 0 or 4).
> >
> > >  And "predicated on packing the event structures"?  Is the structure
> > >  __attribute__((packed)), or is it not?
> >
> > It is not packed in Linus' tree, but one of the patches before this
> > patch in this patch series adds __attribute__((packed)). This patch
> > assumes that the event packing patch has been applied. This patch
> > should not be applied if the packing patch is not (hence,
> > "predicated").
> 
> Thanks for the explanations, it makes sense now.
> 

Does this mean I can add your "Acked-by"?

-- Steve

Re: [PATCH 07/15] ftrace: fix event alignment: kvm:kvm_hv_hypercall

[Avi Kivity]

On 03/09/2011 01:55 AM, Steven Rostedt wrote:
> Hi Avi,
>
> This patch set got lost in the shuffle, and I'm not looking to include
> it.

Anything I need to do?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 07/15] ftrace: fix event alignment: kvm:kvm_hv_hypercall

[Steven Rostedt]

On Wed, 2011-03-09 at 10:50 +0200, Avi Kivity wrote:
> On 03/09/2011 01:55 AM, Steven Rostedt wrote:
> > Hi Avi,
> >
> > This patch set got lost in the shuffle, and I'm not looking to include

Nasty typo:  s/not/now/

> > it.
> 
> Anything I need to do?
> 

Yeah, give me your Acked-by ;)

-- Steve

Re: [PATCH 07/15] ftrace: fix event alignment: kvm:kvm_hv_hypercall

[Avi Kivity]

On 03/09/2011 02:54 PM, Steven Rostedt wrote:
> On Wed, 2011-03-09 at 10:50 +0200, Avi Kivity wrote:
> >  On 03/09/2011 01:55 AM, Steven Rostedt wrote:
> >  >  Hi Avi,
> >  >
> >  >  This patch set got lost in the shuffle, and I'm not looking to include
>
> Nasty typo:  s/not/now/

Makes sense now.

> >  >  it.
> >
> >  Anything I need to do?
> >
>
> Yeah, give me your Acked-by ;)

Acked-by: $me.

Sorry, your previous mail got lost.

-- 
error compiling committee.c: too many arguments to function