* 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.