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.026 / U:0.320 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site