lkml.org 
[lkml]   [2009]   [Dec]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/9] perf: Remove pointless union that wraps the hw breakpoint fields
On Sun, Dec 06, 2009 at 11:38:55AM +0100, Peter Zijlstra wrote:
> On Sun, 2009-12-06 at 08:34 +0100, Frederic Weisbecker wrote:
> > It stands to anonymize a structure, but structures can already
> > anonymize by themselves.
> >
> > Reported-by: Peter Zijlstra <peterz@infradead.org>
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > Cc: Paul Mackerras <paulus@samba.org>
> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> > Cc: "K. Prasad" <prasad@linux.vnet.ibm.com>
> > ---
> > include/linux/perf_event.h | 14 ++++++--------
> > 1 files changed, 6 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> > index a61e4de..53230e9 100644
> > --- a/include/linux/perf_event.h
> > +++ b/include/linux/perf_event.h
> > @@ -215,14 +215,12 @@ struct perf_event_attr {
> > __u32 wakeup_watermark; /* bytes before wakeup */
> > };
> >
> > - union {
> > - struct { /* Hardware breakpoint info */
> > - __u64 bp_addr;
> > - __u32 bp_type;
> > - __u32 bp_len;
> > - __u64 __bp_reserved_1;
> > - __u64 __bp_reserved_2;
> > - };
> > + struct { /* Hardware breakpoint info */
> > + __u64 bp_addr;
> > + __u32 bp_type;
> > + __u32 bp_len;
> > + __u64 __bp_reserved_1;
> > + __u64 __bp_reserved_2;
> > };
> >
> > __u32 __reserved_2;
>
> So I'm a bit puzzled by the need for
> - that structure to begin with



It has no practical use. It's just a logical separation
that makes it easier to review.

I won't mind much if you prefer to remove it.


> - specialized __bp reserves


Because we'll probably have further new needs in the future
in the breakpoint fields. But well, these can map to the
current reserved fields already.



> Furthermore, you still got the packing wrong, leading to different
> structure layout on 32 and 64 bit platforms,..
>
> How about?
>
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 89098e3..5595154 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -215,17 +215,12 @@ struct perf_event_attr {
> __u32 wakeup_watermark; /* bytes before wakeup */
> };
>
> - struct { /* Hardware breakpoint info */
> - __u64 bp_addr;
> - __u32 bp_type;
> - __u32 bp_len;
> - __u64 __bp_reserved_1;
> - __u64 __bp_reserved_2;
> - };
> -
> __u32 __reserved_2;
>
> - __u64 __reserved_3;
> + /* Hardware breakpoint info */
> + __u64 bp_addr;
> + __u32 bp_type;
> + __u32 bp_len;
> };



Right this fixes the packing layout, but what if
we need other fields for the breakpoints in the future?

Thanks.



\
 
 \ /
  Last update: 2009-12-07 02:49    [W:0.194 / U:0.800 seconds]
©2003-2014 Jasper Spaans. hosted at Digital Ocean