blkiomon: fix unaligned accesses on ia64

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 <[email protected]>

---

blkiomon.h +2 -1

index 2e430a6..ae48b4c 100644
@@ -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,

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 <[email protected]>

---
 blkiomon.c |    2 +-
 blkiomon.h |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

blkiomon.h +2 -2

@@ -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,

blkiomon.c +1 -1

@@ -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;

Quoted 38 lines · Eric Sandeen

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 <[email protected]>

---

blkiomon.h +2 -1

index 2e430a6..ae48b4c 100644
@@ -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,

Quoted 14 lines · Martin Peschke

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.

Quoted 6 lines

Thanks,
Martin

Signed-off-by Martin Peschke <[email protected]>

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

Quoted 76 lines

---
 blkiomon.c |    2 +-
 blkiomon.h |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

blkiomon.h +2 -2

@@ -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,

blkiomon.c +1 -1

@@ -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;

Quoted 35 lines · Eric Sandeen

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 <[email protected]>

---

blkiomon.h +2 -1

index 2e430a6..ae48b4c 100644
@@ -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));
+};
 

Quoted 4 lines

blkiomon.h struct blkiomon_stat { · line 49

Quoted 3 lines

blkiomon.h +0 -0

 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html