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

Joe Damato
2024-08-07T15:43:50Z

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>