* Re: [patch] HWBKPT: Make bp_len type to u64 generic across the arch
[not found] ` <20100130045518.GA20776@in.ibm.com>
@ 2010-01-30 18:38 ` Frederic Weisbecker
2010-01-31 8:20 ` Mahesh Jagannath Salgaonkar
2010-02-01 8:23 ` Peter Zijlstra
2010-02-04 9:51 ` [tip:perf/urgent] perf: " tip-bot for Mahesh Salgaonkar
1 sibling, 2 replies; 6+ messages in thread
From: Frederic Weisbecker @ 2010-01-30 18:38 UTC (permalink / raw)
To: Mahesh Salgaonkar, Peter Zijlstra
Cc: Linux Kernel, Ingo Molnar, Ananth N Mavinakayanahalli, K. Prasad,
Maneesh Soni, Heiko Carstens, Martin, Mahesh Salgaonkar
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 <mahesh@linux.vnet.ibm.com>
> ---
>
> 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 <fweisbec@gmail.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] HWBKPT: Make bp_len type to u64 generic across the arch
2010-01-30 18:38 ` [patch] HWBKPT: Make bp_len type to u64 generic across the arch Frederic Weisbecker
@ 2010-01-31 8:20 ` Mahesh Jagannath Salgaonkar
2010-01-31 19:32 ` Frederic Weisbecker
2010-02-01 8:23 ` Peter Zijlstra
1 sibling, 1 reply; 6+ messages in thread
From: Mahesh Jagannath Salgaonkar @ 2010-01-31 8:20 UTC (permalink / raw)
To: Frederic Weisbecker
Cc: Mahesh Salgaonkar, Peter Zijlstra, Linux Kernel, Ingo Molnar,
Ananth N Mavinakayanahalli, K. Prasad, Maneesh Soni,
Heiko Carstens, Martin
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<mahesh@linux.vnet.ibm.com>
>> ---
>>
>> 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<fweisbec@gmail.com>
>
Thanks for reviewing.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] HWBKPT: Make bp_len type to u64 generic across the arch
2010-01-31 8:20 ` Mahesh Jagannath Salgaonkar
@ 2010-01-31 19:32 ` Frederic Weisbecker
0 siblings, 0 replies; 6+ messages in thread
From: Frederic Weisbecker @ 2010-01-31 19:32 UTC (permalink / raw)
To: Mahesh Jagannath Salgaonkar
Cc: Mahesh Salgaonkar, Peter Zijlstra, Linux Kernel, Ingo Molnar,
Ananth N Mavinakayanahalli, K. Prasad, Maneesh Soni,
Heiko Carstens, Martin
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<mahesh@linux.vnet.ibm.com>
>>> ---
>>>
>>> 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!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] HWBKPT: Make bp_len type to u64 generic across the arch
2010-01-30 18:38 ` [patch] HWBKPT: Make bp_len type to u64 generic across the arch Frederic Weisbecker
2010-01-31 8:20 ` Mahesh Jagannath Salgaonkar
@ 2010-02-01 8:23 ` Peter Zijlstra
2010-02-01 17:45 ` Frederic Weisbecker
1 sibling, 1 reply; 6+ messages in thread
From: Peter Zijlstra @ 2010-02-01 8:23 UTC (permalink / raw)
To: Frederic Weisbecker
Cc: Mahesh Salgaonkar, Linux Kernel, Ingo Molnar,
Ananth N Mavinakayanahalli, K. Prasad, Maneesh Soni,
Heiko Carstens, Martin, Mahesh Salgaonkar
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] HWBKPT: Make bp_len type to u64 generic across the arch
2010-02-01 8:23 ` Peter Zijlstra
@ 2010-02-01 17:45 ` Frederic Weisbecker
0 siblings, 0 replies; 6+ messages in thread
From: Frederic Weisbecker @ 2010-02-01 17:45 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Mahesh Salgaonkar, Linux Kernel, Ingo Molnar,
Ananth N Mavinakayanahalli, K. Prasad, Maneesh Soni,
Heiko Carstens, Martin, Mahesh Salgaonkar
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip:perf/urgent] perf: Make bp_len type to u64 generic across the arch
[not found] ` <20100130045518.GA20776@in.ibm.com>
2010-01-30 18:38 ` [patch] HWBKPT: Make bp_len type to u64 generic across the arch Frederic Weisbecker
@ 2010-02-04 9:51 ` tip-bot for Mahesh Salgaonkar
1 sibling, 0 replies; 6+ messages in thread
From: tip-bot for Mahesh Salgaonkar @ 2010-02-04 9:51 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, maneesh, hpa, mingo, schwidefsky, peterz, mahesh,
ananth, fweisbec, heiko.carstens, tglx, prasad
Commit-ID: cd757645fbdc34a8343c04bb0e74e06fccc2cb10
Gitweb: http://git.kernel.org/tip/cd757645fbdc34a8343c04bb0e74e06fccc2cb10
Author: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
AuthorDate: Sat, 30 Jan 2010 10:25:18 +0530
Committer: Frederic Weisbecker <fweisbec@gmail.com>
CommitDate: Thu, 4 Feb 2010 01:07:12 +0100
perf: Make bp_len type to u64 generic across the arch
Change 'bp_len' type to __u64 to make it work across archs as
the s390 architecture watch point length can be upto 2^64.
reference:
http://lkml.org/lkml/2010/1/25/212
This is an ABI change that is not backward compatible with
the previous hardware breakpoint info layout integrated in this
development cycle, a rebuilt of perf tools is necessary for
versions based on 2.6.33-rc1 - 2.6.33-rc6 to work with a
kernel based on this patch.
Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
Acked-by: Peter Zijlstra <peterz@infradead.org>
Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Cc: "K. Prasad" <prasad@linux.vnet.ibm.com>
Cc: Maneesh Soni <maneesh@in.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Martin <schwidefsky@de.ibm.com>
LKML-Reference: <20100130045518.GA20776@in.ibm.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
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 070ba06..5977b72 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;
}
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 8fa7187..a177698 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;
};
/*
diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c
index 8a5c7d5..967e661 100644
--- a/kernel/hw_breakpoint.c
+++ b/kernel/hw_breakpoint.c
@@ -360,8 +360,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 d27746b..2b19297 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -4580,7 +4580,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))
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-02-04 9:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20100130045424.625452081@mars.in.ibm.com>
[not found] ` <20100130045518.GA20776@in.ibm.com>
2010-01-30 18:38 ` [patch] HWBKPT: Make bp_len type to u64 generic across the arch Frederic Weisbecker
2010-01-31 8:20 ` Mahesh Jagannath Salgaonkar
2010-01-31 19:32 ` Frederic Weisbecker
2010-02-01 8:23 ` Peter Zijlstra
2010-02-01 17:45 ` Frederic Weisbecker
2010-02-04 9:51 ` [tip:perf/urgent] perf: " tip-bot for Mahesh Salgaonkar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox