Messages in this thread | | | Date | Mon, 7 Dec 2009 02:46:47 +0100 | From | Frederic Weisbecker <> | Subject | Re: [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.
| |