[ofa-general] [PATCH v3 2/2] infiniband-diags support PortXmitWait get and set

[Sasha Khapyorsky]

Hi Eli,

On 16:56 Tue 13 Jan     , Eli Dorfman (Voltaire) wrote:
> support PortXmitWait get and set
> fix syntax error
> 
> Signed-off-by: Eli Dorfman <elid at voltaire.com>
> ---
>  infiniband-diags/src/perfquery.c |   14 +++++++++++++-
>  libibmad/src/gs.c                |    2 ++
>  2 files changed, 15 insertions(+), 1 deletions(-)
> 
> diff --git a/infiniband-diags/src/perfquery.c b/infiniband-diags/src/perfquery.c
> index 41a8b74..5e5b3ed 100644
> --- a/infiniband-diags/src/perfquery.c
> +++ b/infiniband-diags/src/perfquery.c
> @@ -67,6 +67,7 @@ struct perf_count {
>  	uint32_t rcvdata;
>  	uint32_t xmtpkts;
>  	uint32_t rcvpkts;
> +	uint32_t xmtwait;
>  };
>  
>  struct perf_count_ext {
> @@ -209,6 +210,8 @@ static void aggregate_perfcounters(void)
>  	aggregate_32bit(&perf_count.xmtpkts, val);
>  	mad_decode_field(pc, IB_PC_RCV_PKTS_F, &val);
>  	aggregate_32bit(&perf_count.rcvpkts, val);
> +        mad_decode_field(pc, IB_PC_XMT_WAIT_F, &val);

Please use tab character for indentation.

> +	aggregate_32bit(&perf_count.xmtwait, val);
>  }
>  
>  static void output_aggregate_perfcounters(ib_portid_t *portid)
> @@ -235,6 +238,7 @@ static void output_aggregate_perfcounters(ib_portid_t *portid)
>  	mad_encode_field(pc, IB_PC_RCV_BYTES_F, &perf_count.rcvdata);
>  	mad_encode_field(pc, IB_PC_XMT_PKTS_F, &perf_count.xmtpkts);
>  	mad_encode_field(pc, IB_PC_RCV_PKTS_F, &perf_count.rcvpkts);
> +	mad_encode_field(pc, IB_PC_XMT_WAIT_F, &perf_count.xmtwait);
>  
>  	mad_dump_perfcounters(buf, sizeof buf, pc, sizeof pc);
>  
> @@ -298,9 +302,14 @@ static void dump_perfcounters(int extended, int timeout, uint16_t cap_mask, ib_p
>  	if (extended != 1) {
>  		if (!port_performance_query(pc, portid, port, timeout))
>  			IBERROR("perfquery");
> +		if (!(cap_mask & 0x1000)) {
> +			/* if PortCounters:PortXmitWait not suppported clear this counter */
> +			perf_count.xmtwait = 0;
> +			mad_encode_field(pc, IB_PC_XMT_WAIT_F, &perf_count.xmtwait);
> +		}

Is this a good thing to hide the reported value? We could to not show
XmitWait at all in case when it is not supported, or to show it as was
reported by port and not "to lie" about zero value.

>  		if (aggregate)
>  			aggregate_perfcounters();
> -		else
> +		else 
>  			mad_dump_perfcounters(buf, sizeof buf, pc, sizeof pc);
>  	} else {
>  		if (!(cap_mask & 0x200)) /* 1.2 errata: bit 9 is extended counter support */
> @@ -500,6 +509,9 @@ main(int argc, char **argv)
>  
>  do_reset:
>  
> +	if (!extended && (cap_mask & 0x1000))
> +		mask |= (1<<16); /* reset portxmitwait */

The counter mask can be specified by user in command line to reset only
certain counters. The code above will add XmitWait unconditionally
regardless to user wishes. So shouldn't it be something like:

	if (argc <= 2 && !extended && (cap_mask & 0x1000))
		mask |= (1<<16); /* reset portxmitwait */

?

Sasha

> +		
>  	if (all_ports_loop || (loop_ports && (all_ports || port == ALL_PORTS))) {
>  		for (i = start_port; i <= num_ports; i++)
>  			reset_counters(extended, timeout, mask, &portid, i);
> diff --git a/libibmad/src/gs.c b/libibmad/src/gs.c
> index d350c0d..30f00fb 100644
> --- a/libibmad/src/gs.c
> +++ b/libibmad/src/gs.c
> @@ -142,6 +142,8 @@ performance_reset_via(void *rcvbuf, ib_portid_t *dest, int port, unsigned mask,
>  	/* Same for attribute IDs */
>  	mad_set_field(rcvbuf, 0, IB_PC_PORT_SELECT_F, port);
>  	mad_set_field(rcvbuf, 0, IB_PC_COUNTER_SELECT_F, mask);
> +	mask = mask >> 16;
> +	mad_set_field(rcvbuf, 0, IB_PC_COUNTER_SELECT2_F, mask);
>  	rpc.attr.mod = 0;
>  	rpc.timeout = timeout;
>  	rpc.datasz = IB_PC_DATA_SZ;
> -- 
> 1.5.5
>