* 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