Re: [PATCH] blkiomon: fix unaligned accesses on ia64

[Jens Axboe <jens.axboe@oracle.com>]

On Fri, May 08 2009, Eric Sandeen wrote:
> Martin Peschke wrote:
> > Eric,
> > I think you have got a valid point. But I would like to make two small
> > changes to you patch:
> > 
> > A new blkiomon version number is needed then. Not a perfect solution for
> > reflecting this change, but good enough at least for our tool that
> > consumes blkiomon data through a message queue.
> > 
> > Moving the __u32 device member instead of a new padding field should be
> > fine.
> > 
> > I have tested the patch below. If it's fine with you, please just push
> > it upstream through Jens.
> > 
> > Thanks,
> > Martin
> > 
> >
> > 
> > Signed-off-by: Martin Peschke <mpeschke@linux.vnet.ibm.com>
> 
> Thanks Martin -
> 
> Sure, that way looks fine to me too.  I suppose adding the explicit pad
> doesn't actually change anything on most architectures, so maybe that's
> a little safer without the version bump, but probably best to be
> explicit about it as you have done.
> 
> Jens, want to pick up this patch instead, then?

Yep, added. Thanks guys!

> 
> Thanks,
> -Eric
> 
> 
> > ---
> >  blkiomon.c |    2 +-
> >  blkiomon.h |    4 ++--
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > --- a/blkiomon.h
> > +++ b/blkiomon.h
> > @@ -34,6 +34,7 @@ struct blkiomon_stat {
> >  	__u64 time;
> >  	__u32 size_hist[BLKIOMON_SIZE_BUCKETS];
> >  	__u32 d2c_hist[BLKIOMON_D2C_BUCKETS];
> > +	__u32 device;
> >  	struct minmax size_r;
> >  	struct minmax size_w;
> >  	struct minmax d2c_r;
> > @@ -41,8 +42,7 @@ struct blkiomon_stat {
> >  	struct minmax thrput_r;
> >  	struct minmax thrput_w;
> >  	__u64 bidir;
> > -	__u32 device;
> > -} __attribute__ ((packed));
> > +};
> >  
> >  static struct histlog2 size_hist = {
> >  	.first = 0,
> > --- a/blkiomon.c
> > +++ b/blkiomon.c
> > @@ -71,7 +71,7 @@ struct output {
> >  	int pipe;
> >  };
> >  
> > -static char blkiomon_version[] = "0.2";
> > +static char blkiomon_version[] = "0.3";
> >  
> >  static FILE *ifp;
> >  static int interval = -1;
> > 
> > 
> > 
> > 
> > On Wed, 2009-05-06 at 16:49 -0500, Eric Sandeen wrote: 
> >> commit 7aa3ebcec011bfe9cc60d6476252c03376a37551 packed
> >> the blkiomon_stat structure so that traces from one 
> >> arch could be analyzed on another (in truth only x86
> >> is different, at least from x86_64/ia64/ppc/ppc64/s390/s390x)
> >>
> >> Rather than packing it, which generates unaligned access
> >> warnings on ia64, just pad the structure out so that it's
> >> naturally aligned on all arches.
> >>
> >> Martin, care to test this to be sure it still works for
> >> you?  (I'm not sure if we might also need a 4 byte pad on 
> >> the end of the structure to align the containing structure...)
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
> >>
> >> diff --git a/blkiomon.h b/blkiomon.h
> >> index 2e430a6..ae48b4c 100644
> >> --- a/blkiomon.h
> >> +++ b/blkiomon.h
> >> @@ -34,6 +34,7 @@ struct blkiomon_stat {
> >>  	__u64 time;
> >>  	__u32 size_hist[BLKIOMON_SIZE_BUCKETS];
> >>  	__u32 d2c_hist[BLKIOMON_D2C_BUCKETS];
> >> +	__u32 pad; /* Align the structure */
> >>  	struct minmax size_r;
> >>  	struct minmax size_w;
> >>  	struct minmax d2c_r;
> >> @@ -42,7 +43,7 @@ struct blkiomon_stat {
> >>  	struct minmax thrput_w;
> >>  	__u64 bidir;
> >>  	__u32 device;
> >> -} __attribute__ ((packed));
> >> +};
> >>
> >>  static struct histlog2 size_hist = {
> >>  	.first = 0,
> >>
> > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrace" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Jens Axboe