* Re: [PATCH] blkiomon: fix unaligned accesses on ia64
2009-05-06 21:49 [PATCH] blkiomon: fix unaligned accesses on ia64 Eric Sandeen
@ 2009-05-08 16:09 ` Martin Peschke
2009-05-08 16:22 ` Eric Sandeen
2009-05-11 6:41 ` Jens Axboe
2 siblings, 0 replies; 4+ messages in thread
From: Martin Peschke @ 2009-05-08 16:09 UTC (permalink / raw)
To: linux-btrace
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>
---
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,
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] blkiomon: fix unaligned accesses on ia64
2009-05-06 21:49 [PATCH] blkiomon: fix unaligned accesses on ia64 Eric Sandeen
2009-05-08 16:09 ` Martin Peschke
@ 2009-05-08 16:22 ` Eric Sandeen
2009-05-11 6:41 ` Jens Axboe
2 siblings, 0 replies; 4+ messages in thread
From: Eric Sandeen @ 2009-05-08 16:22 UTC (permalink / raw)
To: linux-btrace
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?
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,
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] blkiomon: fix unaligned accesses on ia64
2009-05-06 21:49 [PATCH] blkiomon: fix unaligned accesses on ia64 Eric Sandeen
2009-05-08 16:09 ` Martin Peschke
2009-05-08 16:22 ` Eric Sandeen
@ 2009-05-11 6:41 ` Jens Axboe
2 siblings, 0 replies; 4+ messages in thread
From: Jens Axboe @ 2009-05-11 6:41 UTC (permalink / raw)
To: linux-btrace
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
^ permalink raw reply [flat|nested] 4+ messages in thread