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

[Sasha Khapyorsky]

On 08:53 Thu 15 Jan     , Eli Dorfman (Voltaire) wrote:
> Sasha Khapyorsky wrote:
> > 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.
> 
> This is a good idea, but it requires to pass the mask to the mad_dump_perfcounters 
> which is a generic function.
> I preferred to leave it like this than making a special case for perfcounters dump.

So I'm removing this if ()...?

Sasha

> 
> > 
> >>  		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 */
> > 
> 
> i agree. 
> 
> 
> > 
> > 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
> >>
>