Re: [PATCH net-next 1/2] eth: fbnic: add basic rtnl stats

[Joe Damato <jdamato@fastly.com>]

On Tue, Aug 06, 2024 at 07:26:30PM -0700, Jakub Kicinski wrote:
> Count packets, bytes and drop on the datapath, and report
> to the user. Since queues are completely freed when the
> device is down - accumulate the stats in the main netdev struct.
> This means that per-queue stats will only report values since
> last reset (per qstat recommendation).
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  .../net/ethernet/meta/fbnic/fbnic_netdev.c    | 69 +++++++++++++++++++
>  .../net/ethernet/meta/fbnic/fbnic_netdev.h    |  3 +
>  drivers/net/ethernet/meta/fbnic/fbnic_txrx.c  | 56 ++++++++++++++-
>  drivers/net/ethernet/meta/fbnic/fbnic_txrx.h  | 10 +++
>  4 files changed, 137 insertions(+), 1 deletion(-)
>
[...]
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> index 0ed4c9fff5d8..88aaa08b4fe9 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
[...]
> +static void fbnic_aggregate_ring_rx_counters(struct fbnic_net *fbn,
> +					     struct fbnic_ring *rxr)
> +{
> +	struct fbnic_queue_stats *stats = &rxr->stats;
> +
> +	if (!(rxr->flags & FBNIC_RING_F_STATS))
> +		return;
> +

Nit: I noticed this check is in both aggregate functions and just
before where the functions are called below. I'm sure you have
better folks internally to review this than me, but: maybe the extra
flags check isn't necessary?

Could be good if you are trying to be defensive, though.

[...]

> +static void fbnic_aggregate_ring_tx_counters(struct fbnic_net *fbn,
> +					     struct fbnic_ring *txr)
> +{
> +	struct fbnic_queue_stats *stats = &txr->stats;
> +
> +	if (!(txr->flags & FBNIC_RING_F_STATS))
> +		return;
> +

[...]

>  static void fbnic_remove_tx_ring(struct fbnic_net *fbn,
>  				 struct fbnic_ring *txr)
>  {
>  	if (!(txr->flags & FBNIC_RING_F_STATS))
>  		return;
>  
> +	fbnic_aggregate_ring_tx_counters(fbn, txr);
> +
>  	/* Remove pointer to the Tx ring */
>  	WARN_ON(fbn->tx[txr->q_idx] && fbn->tx[txr->q_idx] != txr);
>  	fbn->tx[txr->q_idx] = NULL;
> @@ -882,6 +934,8 @@ static void fbnic_remove_rx_ring(struct fbnic_net *fbn,
>  	if (!(rxr->flags & FBNIC_RING_F_STATS))
>  		return;
>  
> +	fbnic_aggregate_ring_rx_counters(fbn, rxr);
> +

Note the flags checks above the
fbnic_aggregate_ring_{rt}x_counters() call sites.

Probably fine to do the check twice, though, and otherwise the code
seems fine to me.

Reviewed-by: Joe Damato <jdamato@fastly.com>