On 2/1/22 06:13, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@xxxxxxx>
>
> Store the location of the ss_data in the payload itself, rather than
> assuming that it is always located at a fixed location, directly after
> the cmd_ts_pdu data.
>
> This is done as a cleanup patch in order to be able to handle
> clat_prio_stats, which just like ss_data, may or may not be part of the
> payload.
>
> Server version is intentionally not incremented, as it will be incremented
> in a later patch in the series. No need to bump it twice for the same patch
> series.
This comment should be added to the previous patch commit message too
(as it added the ioprio field to the stats).
>
> Signed-off-by: Niklas Cassel <niklas.cassel@xxxxxxx>
> ---
> client.c | 10 ++++++----
> server.c | 59 ++++++++++++++++++++++++++++++++++++++++++--------------
> stat.h | 14 ++++++++++++--
> 3 files changed, 62 insertions(+), 21 deletions(-)
>
> diff --git a/client.c b/client.c
> index 381af054..e51138ee 100644
> --- a/client.c
> +++ b/client.c
> @@ -1761,7 +1761,6 @@ int fio_handle_client(struct fio_client *client)
> {
> struct client_ops *ops = client->ops;
> struct fio_net_cmd *cmd;
> - int size;
>
> dprint(FD_NET, "client: handle %s\n", client->hostname);
>
> @@ -1795,14 +1794,17 @@ int fio_handle_client(struct fio_client *client)
> }
> case FIO_NET_CMD_TS: {
> struct cmd_ts_pdu *p = (struct cmd_ts_pdu *) cmd->payload;
> + uint64_t offset;
>
> dprint(FD_NET, "client: ts->ss_state = %u\n", (unsigned int) le32_to_cpu(p->ts.ss_state));
> if (le32_to_cpu(p->ts.ss_state) & FIO_SS_DATA) {
> dprint(FD_NET, "client: received steadystate ring buffers\n");
>
> - size = le64_to_cpu(p->ts.ss_dur);
> - p->ts.ss_iops_data = (uint64_t *) ((struct cmd_ts_pdu *)cmd->payload + 1);
> - p->ts.ss_bw_data = p->ts.ss_iops_data + size;
> + offset = le64_to_cpu(p->ts.ss_iops_data_offset);
> + p->ts.ss_iops_data = (uint64_t *)((char *)p + offset);
> +
> + offset = le64_to_cpu(p->ts.ss_bw_data_offset);
> + p->ts.ss_bw_data = (uint64_t *)((char *)p + offset);
> }
>
> convert_ts(&p->ts, &p->ts);
> diff --git a/server.c b/server.c
> index af94cd78..4ddf682b 100644
> --- a/server.c
> +++ b/server.c
> @@ -1465,8 +1465,10 @@ void fio_server_send_ts(struct thread_stat *ts, struct group_run_stats *rs)
> {
> struct cmd_ts_pdu p;
> int i, j, k;
> - void *ss_buf;
> - uint64_t *ss_iops, *ss_bw;
> + size_t ss_extra_size = 0;
> + size_t extended_buf_size = 0;
> + void *extended_buf;
> + void *extended_buf_wp;
>
> dprint(FD_NET, "server sending end stats\n");
>
> @@ -1590,26 +1592,53 @@ void fio_server_send_ts(struct thread_stat *ts, struct group_run_stats *rs)
> convert_gs(&p.rs, rs);
>
> dprint(FD_NET, "ts->ss_state = %d\n", ts->ss_state);
> - if (ts->ss_state & FIO_SS_DATA) {
> - dprint(FD_NET, "server sending steadystate ring buffers\n");
> + if (ts->ss_state & FIO_SS_DATA)
> + ss_extra_size = 2 * ts->ss_dur * sizeof(uint64_t);
>
> - ss_buf = malloc(sizeof(p) + 2*ts->ss_dur*sizeof(uint64_t));
> + extended_buf_size += ss_extra_size;
> + if (!extended_buf_size) {
> + fio_net_queue_cmd(FIO_NET_CMD_TS, &p, sizeof(p), NULL, SK_F_COPY);
> + return;
> + }
>
> - memcpy(ss_buf, &p, sizeof(p));
> + extended_buf_size += sizeof(p);
> + extended_buf = calloc(1, extended_buf_size);
> + if (!extended_buf) {
> + log_err("fio: failed to alloc FIO_NET_CMD_TS buffer\n");
nit: s/alloc/allocate
Let's write proper English :)
> + return;
> + }
> +
> + memcpy(extended_buf, &p, sizeof(p));
> + extended_buf_wp = (struct cmd_ts_pdu *)extended_buf + 1;
>
> - ss_iops = (uint64_t *) ((struct cmd_ts_pdu *)ss_buf + 1);
> - ss_bw = ss_iops + (int) ts->ss_dur;
> - for (i = 0; i < ts->ss_dur; i++) {
> + if (ss_extra_size) {
> + uint64_t *ss_iops, *ss_bw;
> + uint64_t offset;
> + struct cmd_ts_pdu *ptr = extended_buf;
> +
> + dprint(FD_NET, "server sending steadystate ring buffers\n");
> +
> + /* ss iops */
> + ss_iops = (uint64_t *) extended_buf_wp;
> + for (i = 0; i < ts->ss_dur; i++)
> ss_iops[i] = cpu_to_le64(ts->ss_iops_data[i]);
> - ss_bw[i] = cpu_to_le64(ts->ss_bw_data[i]);
> - }
>
> - fio_net_queue_cmd(FIO_NET_CMD_TS, ss_buf, sizeof(p) + 2*ts->ss_dur*sizeof(uint64_t), NULL, SK_F_COPY);
> + offset = (char *)extended_buf_wp - (char *)extended_buf;
> + ptr->ts.ss_iops_data_offset = cpu_to_le64(offset);
> + extended_buf_wp = ss_iops + (int) ts->ss_dur;
> +
> + /* ss bw */
> + ss_bw = extended_buf_wp;
> + for (i = 0; i < ts->ss_dur; i++)
> + ss_bw[i] = cpu_to_le64(ts->ss_bw_data[i]);
>
> - free(ss_buf);
> + offset = (char *)extended_buf_wp - (char *)extended_buf;
> + ptr->ts.ss_bw_data_offset = cpu_to_le64(offset);
> + extended_buf_wp = ss_bw + (int) ts->ss_dur;
> }
> - else
> - fio_net_queue_cmd(FIO_NET_CMD_TS, &p, sizeof(p), NULL, SK_F_COPY);
> +
> + fio_net_queue_cmd(FIO_NET_CMD_TS, extended_buf, extended_buf_size, NULL, SK_F_COPY);
> + free(extended_buf);
> }
>
> void fio_server_send_gs(struct group_run_stats *rs)
> diff --git a/stat.h b/stat.h
> index 3ce821a7..711df24a 100644
> --- a/stat.h
> +++ b/stat.h
> @@ -262,12 +262,22 @@ struct thread_stat {
>
> union {
> uint64_t *ss_iops_data;
> - uint64_t pad4;
> + /*
> + * For FIO_NET_CMD_TS, the pointed to data will temporarily
> + * be stored at this offset from the start of the payload.
> + * (The union is also needed to pad this field on 32-bit.)
> + */
> + uint64_t ss_iops_data_offset;
Nit: May be keep the pad4 field last ?
> };
>
> union {
> uint64_t *ss_bw_data;
> - uint64_t pad5;
> + /*
> + * For FIO_NET_CMD_TS, the pointed to data will temporarily
> + * be stored at this offset from the start of the payload.
> + * (The union is also needed to pad this field on 32-bit.)
> + */
> + uint64_t ss_bw_data_offset;
Same.
> };
>
> uint64_t cachehit;
Apart from the above nits, looks good.
Reviewed-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
--
Damien Le Moal
Western Digital Research
