Re: [RFC PATCH v4] ethtool: add FEC bins histogramm report

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 08/08/2025 21:15, Jakub Kicinski wrote:
> On Thu, 7 Aug 2025 08:59:24 -0700 Vadim Fedorenko wrote:
>> +		/* add FEC bins information */
>> +		len += (nla_total_size(0) +  /* _A_FEC_HIST */
>> +			nla_total_size(4) +  /* _A_FEC_HIST_BIN_LOW */
>> +			nla_total_size(4) +  /* _A_FEC_HIST_BIN_HI */
>> +			/* _A_FEC_HIST_BIN_VAL + per-lane values */
>> +			nla_total_size_64bit(sizeof(u64) *
>> +					     (1 + ETHTOOL_MAX_LANES))) *
> 
> That's the calculation for basic stats because they are reported as a
> raw array. Each nla_put() should correspond to a nla_total_size().
> This patch nla_put()s things individually.
> 
>> +			ETHTOOL_FEC_HIST_MAX;
>> +	}
>>   
>>   	return len;
>>   }
>>   
>> +static int fec_put_hist(struct sk_buff *skb, const struct ethtool_fec_hist *hist)
>> +{
>> +	const struct ethtool_fec_hist_range *ranges = hist->ranges;
>> +	const struct ethtool_fec_hist_value *values = hist->values;
>> +	struct nlattr *nest;
>> +	int i, j;
>> +
>> +	if (!ranges)
>> +		return 0;
>> +
>> +	for (i = 0; i < ETHTOOL_FEC_HIST_MAX; i++) {
>> +		if (i && !ranges[i].low && !ranges[i].high)
>> +			break;
>> +
>> +		if (WARN_ON_ONCE(values[i].bin_value == ETHTOOL_STAT_NOT_SET))
>> +			break;
>> +
>> +		nest = nla_nest_start(skb, ETHTOOL_A_FEC_STAT_HIST);
>> +		if (!nest)
>> +			return -EMSGSIZE;
>> +
>> +		if (nla_put_u32(skb, ETHTOOL_A_FEC_HIST_BIN_LOW,
>> +				ranges[i].low) ||
>> +		    nla_put_u32(skb, ETHTOOL_A_FEC_HIST_BIN_HIGH,
>> +				ranges[i].high) ||
>> +		    nla_put_uint(skb, ETHTOOL_A_FEC_HIST_BIN_VAL,
>> +				 values[i].bin_value))
>> +			goto err_cancel_hist;
>> +		for (j = 0; j < ETHTOOL_MAX_LANES; j++) {
>> +			if (values[i].bin_value_per_lane[j] == ETHTOOL_STAT_NOT_SET)
>> +				break;
>> +			nla_put_uint(skb, ETHTOOL_A_FEC_HIST_BIN_VAL_PER_LANE,
>> +				     values[i].bin_value_per_lane[j]);
> 
> TBH I'm a bit unsure if this is really worth breaking out into
> individual nla_puts(). We generally recommend that, but here it's
> an array of simple ints.. maybe we're better of with a binary / C
> array of u64. Like the existing FEC stats but without also folding
> the total value into index 0.

Well, the current implementation is straight forward. Do you propose to
have drivers fill in the amount of lanes they have histogram for, or
should we always put array of ETHTOOL_MAX_LANES values and let
user-space to figure out what to show?