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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ec9e7da6-30f0-40aa-8cb7-bfa0ff814126@linux.dev>
Date: Sun, 10 Aug 2025 11:52:55 +0100
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Andrew Lunn <andrew@...n.ch>, Michael Chan <michael.chan@...adcom.com>,
 Pavan Chebbi <pavan.chebbi@...adcom.com>, Tariq Toukan <tariqt@...dia.com>,
 Gal Pressman <gal@...dia.com>, intel-wired-lan@...ts.osuosl.org,
 Donald Hunter <donald.hunter@...il.com>, Carolina Jubran
 <cjubran@...dia.com>, Paolo Abeni <pabeni@...hat.com>,
 Simon Horman <horms@...nel.org>, netdev@...r.kernel.org
Subject: Re: [RFC PATCH v4] ethtool: add FEC bins histogramm report

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?