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

[Eli Dorfman (Voltaire)]

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.

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