[PATCH] fio: fix alignement to prevent bus error on ARM

[PATCH] fio: fix alignement to prevent bus error on ARM

[Gwendal Grignou]

Add a filler field to be ensure 64bit alignment.
Otherwise, we would trigger SIGBUS error in sum_stat()

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
 stat.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/stat.h b/stat.h
index 32ea226..9595e59 100644
--- a/stat.h
+++ b/stat.h
@@ -172,6 +172,7 @@ struct thread_stat {
 	 * IO Error related stats
 	 */
 	uint16_t continue_on_error;
+	uint16_t filler[3];
 	uint64_t total_err_count;
 	uint32_t first_error;
 
-- 
2.1.0.rc2.206.gedb03e5

Re: [PATCH] fio: fix alignement to prevent bus error on ARM

[Jens Axboe]

On 10/24/2014 02:42 PM, Gwendal Grignou wrote:
> Add a filler field to be ensure 64bit alignment.
> Otherwise, we would trigger SIGBUS error in sum_stat()

Thanks, applied.


-- 
Jens Axboe

Re: [PATCH] fio: fix alignement to prevent bus error on ARM

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 840 bytes --]

the code seems to be fine with gcc-isms, so i might suggest anon unions
instead to make it harder to break:
union {
  uint64_t __aligner;
  uint16_t continue_on_error;
};
-mike

On Fri, Oct 24, 2014 at 4:42 PM, Gwendal Grignou <gwendal@chromium.org>
wrote:

> Add a filler field to be ensure 64bit alignment.
> Otherwise, we would trigger SIGBUS error in sum_stat()
>
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> ---
>  stat.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/stat.h b/stat.h
> index 32ea226..9595e59 100644
> --- a/stat.h
> +++ b/stat.h
> @@ -172,6 +172,7 @@ struct thread_stat {
>          * IO Error related stats
>          */
>         uint16_t continue_on_error;
> +       uint16_t filler[3];
>         uint64_t total_err_count;
>         uint32_t first_error;
>
> --
> 2.1.0.rc2.206.gedb03e5
>
>

[-- Attachment #2: Type: text/html, Size: 1391 bytes --]

Re: [PATCH] fio: fix alignement to prevent bus error on ARM

[Jens Axboe]

On 10/24/2014 02:47 PM, Mike Frysinger wrote:
> the code seems to be fine with gcc-isms, so i might suggest anon unions
> instead to make it harder to break:
> union {
>   uint64_t __aligner;
>   uint16_t continue_on_error;
> };

It's not a bad idea... Your commit broke alignment further down, btw,
but I fixed it up.

I'll be happy to take anon union patches for the alignment parts :-)

-- 
Jens Axboe

[PATCH v2] fio: Fix padding properly

[Gwendal Grignou]

Completely fix padding:
- use anonymous union for padding
- remove remaining padding in thread_stat.

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
 stat.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/stat.h b/stat.h
index 16b3d1a..1257835 100644
--- a/stat.h
+++ b/stat.h
@@ -171,8 +171,10 @@ struct thread_stat {
 	/*
 	 * IO Error related stats
 	 */
-	uint16_t continue_on_error;
-	uint16_t filler[3];
+	union {
+		uint16_t continue_on_error;
+		uint64_t pad;
+	}
 	uint64_t total_err_count;
 	uint32_t first_error;
 
@@ -181,7 +183,6 @@ struct thread_stat {
 
 	uint32_t latency_depth;
 	uint64_t latency_target;
-	uint32_t pad;
 	fio_fp64_t latency_percentile;
 	uint64_t latency_window;
 } __attribute__((packed));
-- 
2.1.0.rc2.206.gedb03e5

Re: [PATCH v2] fio: Fix padding properly

[Gwendal Grignou]

Gwendal Grignou <gwendal@...> writes:
> 
> Completely fix padding:

Ignore this patch, I did not properly test it.

Gwendal.

[PATCH v3] fio: Fix padding properly

[Gwendal Grignou]

Completely fix padding:
- use anonymous union for padding
- remove remaining padding in thread_stat.

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
 stat.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/stat.h b/stat.h
index 16b3d1a..c2c30b2 100644
--- a/stat.h
+++ b/stat.h
@@ -171,8 +171,10 @@ struct thread_stat {
 	/*
 	 * IO Error related stats
 	 */
-	uint16_t continue_on_error;
-	uint16_t filler[3];
+	union {
+		uint16_t continue_on_error;
+		uint64_t pad;
+	};
 	uint64_t total_err_count;
 	uint32_t first_error;
 
@@ -181,7 +183,6 @@ struct thread_stat {
 
 	uint32_t latency_depth;
 	uint64_t latency_target;
-	uint32_t pad;
 	fio_fp64_t latency_percentile;
 	uint64_t latency_window;
 } __attribute__((packed));
-- 
2.1.0.rc2.206.gedb03e5

[PATCH v4] fio: Fix padding properly

[Gwendal Grignou]

Completely fix padding:
- use anonymous union for padding.
- move existing padding in thread_stat.
- add alignment checks.

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
 libfio.c | 2 ++
 stat.h   | 9 ++++++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/libfio.c b/libfio.c
index b823bd4..1abf39a 100644
--- a/libfio.c
+++ b/libfio.c
@@ -305,6 +305,8 @@ int initialize_fio(char *envp[])
 	 * access (ARM).
 	 */
 	compiletime_assert((offsetof(struct thread_stat, percentile_list) % 8) == 0, "stat percentile_list");
+	compiletime_assert((offsetof(struct thread_stat, total_run_time) % 8) == 0, "total_run_time");
+	compiletime_assert((offsetof(struct thread_stat, total_err_count) % 8) == 0, "total_err_count");
 	compiletime_assert((offsetof(struct thread_stat, latency_percentile) % 8) == 0, "stat latency_percentile");
 	compiletime_assert((offsetof(struct thread_options_pack, zipf_theta) % 8) == 0, "zipf_theta");
 	compiletime_assert((offsetof(struct thread_options_pack, pareto_h) % 8) == 0, "pareto_h");
diff --git a/stat.h b/stat.h
index 16b3d1a..db83f65 100644
--- a/stat.h
+++ b/stat.h
@@ -158,6 +158,8 @@ struct thread_stat {
 	uint32_t io_u_lat_u[FIO_IO_U_LAT_U_NR];
 	uint32_t io_u_lat_m[FIO_IO_U_LAT_M_NR];
 	uint32_t io_u_plat[DDIR_RWDIR_CNT][FIO_IO_U_PLAT_NR];
+	uint32_t pad;
+
 	uint64_t total_io_u[3];
 	uint64_t short_io_u[3];
 	uint64_t drop_io_u[3];
@@ -171,8 +173,10 @@ struct thread_stat {
 	/*
 	 * IO Error related stats
 	 */
-	uint16_t continue_on_error;
-	uint16_t filler[3];
+	union {
+		uint16_t continue_on_error;
+		uint64_t pad;
+	};
 	uint64_t total_err_count;
 	uint32_t first_error;
 
@@ -181,7 +185,6 @@ struct thread_stat {
 
 	uint32_t latency_depth;
 	uint64_t latency_target;
-	uint32_t pad;
 	fio_fp64_t latency_percentile;
 	uint64_t latency_window;
 } __attribute__((packed));
-- 
2.1.0.rc2.206.gedb03e5

Re: [PATCH v4] fio: Fix padding properly

[Jens Axboe]

On 10/25/2014 05:04 PM, Gwendal Grignou wrote:
> diff --git a/stat.h b/stat.h
> index 16b3d1a..db83f65 100644
> --- a/stat.h
> +++ b/stat.h
> @@ -158,6 +158,8 @@ struct thread_stat {
>  	uint32_t io_u_lat_u[FIO_IO_U_LAT_U_NR];
>  	uint32_t io_u_lat_m[FIO_IO_U_LAT_M_NR];
>  	uint32_t io_u_plat[DDIR_RWDIR_CNT][FIO_IO_U_PLAT_NR];
> +	uint32_t pad;
> +
>  	uint64_t total_io_u[3];
>  	uint64_t short_io_u[3];
>  	uint64_t drop_io_u[3];
> @@ -171,8 +173,10 @@ struct thread_stat {
>  	/*
>  	 * IO Error related stats
>  	 */
> -	uint16_t continue_on_error;
> -	uint16_t filler[3];
> +	union {
> +		uint16_t continue_on_error;
> +		uint64_t pad;
> +	};
>  	uint64_t total_err_count;
>  	uint32_t first_error;
>  

These two hunks are both in struct thread_stat, the compile wont be
happy about the duplicate naming...

-- 
Jens Axboe

[PATCH v5] fio: Fix padding properly

[Gwendal Grignou]

Completely fix padding:
- use anonymous union for padding
- remove remaining padding in thread_stat.

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
 libfio.c | 2 ++
 stat.h   | 9 ++++++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/libfio.c b/libfio.c
index b823bd4..1abf39a 100644
--- a/libfio.c
+++ b/libfio.c
@@ -305,6 +305,8 @@ int initialize_fio(char *envp[])
 	 * access (ARM).
 	 */
 	compiletime_assert((offsetof(struct thread_stat, percentile_list) % 8) == 0, "stat percentile_list");
+	compiletime_assert((offsetof(struct thread_stat, total_run_time) % 8) == 0, "total_run_time");
+	compiletime_assert((offsetof(struct thread_stat, total_err_count) % 8) == 0, "total_err_count");
 	compiletime_assert((offsetof(struct thread_stat, latency_percentile) % 8) == 0, "stat latency_percentile");
 	compiletime_assert((offsetof(struct thread_options_pack, zipf_theta) % 8) == 0, "zipf_theta");
 	compiletime_assert((offsetof(struct thread_options_pack, pareto_h) % 8) == 0, "pareto_h");
diff --git a/stat.h b/stat.h
index 16b3d1a..7a0dd55 100644
--- a/stat.h
+++ b/stat.h
@@ -158,6 +158,8 @@ struct thread_stat {
 	uint32_t io_u_lat_u[FIO_IO_U_LAT_U_NR];
 	uint32_t io_u_lat_m[FIO_IO_U_LAT_M_NR];
 	uint32_t io_u_plat[DDIR_RWDIR_CNT][FIO_IO_U_PLAT_NR];
+	uint32_t pad1;
+
 	uint64_t total_io_u[3];
 	uint64_t short_io_u[3];
 	uint64_t drop_io_u[3];
@@ -171,8 +173,10 @@ struct thread_stat {
 	/*
 	 * IO Error related stats
 	 */
-	uint16_t continue_on_error;
-	uint16_t filler[3];
+	union {
+		uint16_t continue_on_error;
+		uint64_t pad2;
+	};
 	uint64_t total_err_count;
 	uint32_t first_error;
 
@@ -181,7 +185,6 @@ struct thread_stat {
 
 	uint32_t latency_depth;
 	uint64_t latency_target;
-	uint32_t pad;
 	fio_fp64_t latency_percentile;
 	uint64_t latency_window;
 } __attribute__((packed));
-- 
2.1.0.rc2.206.gedb03e5