On Sat, Jan 30, 2010 at 10:25:18AM +0530, Mahesh Salgaonkar wrote:
>
> Change 'bp_len' type to __u64 to make it work across the arch.
> The s390 architecture watch point length can be upto 2^64.
>
> reference:
> http://lkml.org/lkml/2010/1/25/212
>
> Based on commit 6aa41f8b01301199af6c9febb24f3c1f5a0bc9d5
>
> Signed-off-by: Mahesh Salgaonkar <[email protected]>
> ---
>
> include/linux/hw_breakpoint.h | 2 +-
> include/linux/perf_event.h | 6 ++----
> kernel/hw_breakpoint.c | 2 +-
> kernel/perf_event.c | 2 +-
> 4 files changed, 5 insertions(+), 7 deletions(-)
>
>
> diff --git a/include/linux/hw_breakpoint.h b/include/linux/hw_breakpoint.h
> index 41235c9..76e7427 100644
> --- a/include/linux/hw_breakpoint.h
> +++ b/include/linux/hw_breakpoint.h
> @@ -44,7 +44,7 @@ static inline int hw_breakpoint_type(struct perf_event *bp)
> return bp->attr.bp_type;
> }
>
> -static inline int hw_breakpoint_len(struct perf_event *bp)
> +static inline unsigned long hw_breakpoint_len(struct perf_event *bp)
> {
> return bp->attr.bp_len;
> }
This should return a u64, or gcc will warn us about loosing
informations in 32 bits arch?
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 1438463..30c78bd 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -211,11 +211,9 @@ struct perf_event_attr {
> __u32 wakeup_watermark; /* bytes before wakeup */
> };
>
> - __u32 __reserved_2;
> -
> - __u64 bp_addr;
> __u32 bp_type;
> - __u32 bp_len;
> + __u64 bp_addr;
> + __u64 bp_len;
> };
Peter, what do you think about this new layout?
Putting the bp_type right after the wakeup_* fields
is going to remove the padding difference between
64 and 32 archs. That looks better than the __reserved_2
we had.
If this patch can make it for .33, it would be nice.
>
> /*
> diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c
> index 50dbd59..a1f511f 100644
> --- a/kernel/hw_breakpoint.c
> +++ b/kernel/hw_breakpoint.c
> @@ -324,8 +324,8 @@ EXPORT_SYMBOL_GPL(register_user_hw_breakpoint);
> int modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_attr *attr)
> {
> u64 old_addr = bp->attr.bp_addr;
> + u64 old_len = bp->attr.bp_len;
> int old_type = bp->attr.bp_type;
> - int old_len = bp->attr.bp_len;
> int err = 0;
>
> perf_event_disable(bp);
> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> index 8f8f7aa..1473cc9 100644
> --- a/kernel/perf_event.c
> +++ b/kernel/perf_event.c
> @@ -4720,7 +4720,7 @@ static int perf_copy_attr(struct perf_event_attr __user *uattr,
> if (attr->type >= PERF_TYPE_MAX)
> return -EINVAL;
>
> - if (attr->__reserved_1 || attr->__reserved_2)
> + if (attr->__reserved_1)
> return -EINVAL;
>
> if (attr->sample_type & ~(PERF_SAMPLE_MAX-1))
>
Acked-by: Frederic Weisbecker <[email protected]>
On 01/31/2010 12:08 AM, Frederic Weisbecker wrote:
> On Sat, Jan 30, 2010 at 10:25:18AM +0530, Mahesh Salgaonkar wrote:
>>
>> Change 'bp_len' type to __u64 to make it work across the arch.
>> The s390 architecture watch point length can be upto 2^64.
>>
>> reference:
>> http://lkml.org/lkml/2010/1/25/212
>>
>> Based on commit 6aa41f8b01301199af6c9febb24f3c1f5a0bc9d5
>>
>> Signed-off-by: Mahesh Salgaonkar<[email protected]>
>> ---
>>
>> include/linux/hw_breakpoint.h | 2 +-
>> include/linux/perf_event.h | 6 ++----
>> kernel/hw_breakpoint.c | 2 +-
>> kernel/perf_event.c | 2 +-
>> 4 files changed, 5 insertions(+), 7 deletions(-)
>>
>>
>> diff --git a/include/linux/hw_breakpoint.h b/include/linux/hw_breakpoint.h
>> index 41235c9..76e7427 100644
>> --- a/include/linux/hw_breakpoint.h
>> +++ b/include/linux/hw_breakpoint.h
>> @@ -44,7 +44,7 @@ static inline int hw_breakpoint_type(struct perf_event *bp)
>> return bp->attr.bp_type;
>> }
>>
>> -static inline int hw_breakpoint_len(struct perf_event *bp)
>> +static inline unsigned long hw_breakpoint_len(struct perf_event *bp)
>> {
>> return bp->attr.bp_len;
>> }
>
>
>
> This should return a u64, or gcc will warn us about loosing
> informations in 32 bits arch?
>
Yup, I even thought so, but then I figured out the function
'hw_breakpoint_addr()' also has same return type for returning
'attr.bp_addr' and kept the same type to be consistent. We may have to
fix that also. What do you think?
>
>
>>
>
>
> Acked-by: Frederic Weisbecker<[email protected]>
>
Thanks for reviewing.
On Sun, Jan 31, 2010 at 01:50:02PM +0530, Mahesh Jagannath Salgaonkar wrote:
> On 01/31/2010 12:08 AM, Frederic Weisbecker wrote:
>> On Sat, Jan 30, 2010 at 10:25:18AM +0530, Mahesh Salgaonkar wrote:
>>>
>>> Change 'bp_len' type to __u64 to make it work across the arch.
>>> The s390 architecture watch point length can be upto 2^64.
>>>
>>> reference:
>>> http://lkml.org/lkml/2010/1/25/212
>>>
>>> Based on commit 6aa41f8b01301199af6c9febb24f3c1f5a0bc9d5
>>>
>>> Signed-off-by: Mahesh Salgaonkar<[email protected]>
>>> ---
>>>
>>> include/linux/hw_breakpoint.h | 2 +-
>>> include/linux/perf_event.h | 6 ++----
>>> kernel/hw_breakpoint.c | 2 +-
>>> kernel/perf_event.c | 2 +-
>>> 4 files changed, 5 insertions(+), 7 deletions(-)
>>>
>>>
>>> diff --git a/include/linux/hw_breakpoint.h b/include/linux/hw_breakpoint.h
>>> index 41235c9..76e7427 100644
>>> --- a/include/linux/hw_breakpoint.h
>>> +++ b/include/linux/hw_breakpoint.h
>>> @@ -44,7 +44,7 @@ static inline int hw_breakpoint_type(struct perf_event *bp)
>>> return bp->attr.bp_type;
>>> }
>>>
>>> -static inline int hw_breakpoint_len(struct perf_event *bp)
>>> +static inline unsigned long hw_breakpoint_len(struct perf_event *bp)
>>> {
>>> return bp->attr.bp_len;
>>> }
>>
>>
>>
>> This should return a u64, or gcc will warn us about loosing
>> informations in 32 bits arch?
>>
> Yup, I even thought so, but then I figured out the function
> 'hw_breakpoint_addr()' also has same return type for returning
> 'attr.bp_addr' and kept the same type to be consistent. We may have to
> fix that also. What do you think?
True. I'll fix both then, thanks!
On Sat, 2010-01-30 at 19:38 +0100, Frederic Weisbecker wrote:
> > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> > index 1438463..30c78bd 100644
> > --- a/include/linux/perf_event.h
> > +++ b/include/linux/perf_event.h
> > @@ -211,11 +211,9 @@ struct perf_event_attr {
> > __u32 wakeup_watermark; /* bytes before wakeup */
> > };
> >
> > - __u32 __reserved_2;
> > -
> > - __u64 bp_addr;
> > __u32 bp_type;
> > - __u32 bp_len;
> > + __u64 bp_addr;
> > + __u64 bp_len;
> > };
>
>
>
> Peter, what do you think about this new layout?
> Putting the bp_type right after the wakeup_* fields
> is going to remove the padding difference between
> 64 and 32 archs. That looks better than the __reserved_2
> we had.
Right, I think this works nicely in that all elements will be naturally
aligned and not result in different layouts between 32/64 bit builds.
> If this patch can make it for .33, it would be nice.
It has to make .33, if it doesn't you're hosed because then the old
layout is fixed in stone.
On Mon, Feb 01, 2010 at 09:23:04AM +0100, Peter Zijlstra wrote:
> On Sat, 2010-01-30 at 19:38 +0100, Frederic Weisbecker wrote:
> > > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> > > index 1438463..30c78bd 100644
> > > --- a/include/linux/perf_event.h
> > > +++ b/include/linux/perf_event.h
> > > @@ -211,11 +211,9 @@ struct perf_event_attr {
> > > __u32 wakeup_watermark; /* bytes before wakeup */
> > > };
> > >
> > > - __u32 __reserved_2;
> > > -
> > > - __u64 bp_addr;
> > > __u32 bp_type;
> > > - __u32 bp_len;
> > > + __u64 bp_addr;
> > > + __u64 bp_len;
> > > };
> >
> >
> >
> > Peter, what do you think about this new layout?
> > Putting the bp_type right after the wakeup_* fields
> > is going to remove the padding difference between
> > 64 and 32 archs. That looks better than the __reserved_2
> > we had.
>
> Right, I think this works nicely in that all elements will be naturally
> aligned and not result in different layouts between 32/64 bit builds.
>
> > If this patch can make it for .33, it would be nice.
>
> It has to make .33, if it doesn't you're hosed because then the old
> layout is fixed in stone.
>
Truly.
I'll send a pull request to Ingo quickly then.
Thanks.