Re: [PATCH net-next 2/4] eth: mpnic: report per-queue stats

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] eth: mpnic: missing drops and errors in queue stats reporting
--

commit cda12cc259f9eb77d108f5cb4427efce257c6e0b
Author: Daniel Zahka <daniel.zahka@gmail.com>

eth: mpnic: report per-queue stats

This commit exposes the per-ring packet and byte counters through the
queue stats API. It also reports counters for deactivated rings through
the base stats callback.

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
> index 5164475a0f943..c2f4a326a8f0f 100644
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
[ ... ]
> +static void mpnic_get_queue_stats_rx(struct net_device *dev, int idx,
> +				     struct netdev_queue_stats_rx *rx)
> +{
> +	struct mpnic_net *mpn = netdev_priv(dev);
> +	struct mpnic_ring *rxr = mpn->rx[idx];
> +	struct mpnic_queue_stats *stats;
> +	unsigned int start;
> +	u64 bytes, packets;
> +
> +	if (!rxr)
> +		return;
> +
> +	stats = &rxr->stats;
> +	do {
> +		start = u64_stats_fetch_begin(&stats->syncp);
> +		bytes = u64_stats_read(&stats->rcq.bytes);
> +		packets = u64_stats_read(&stats->rcq.packets);
> +	} while (u64_stats_fetch_retry(&stats->syncp, start));
> +
> +	rx->bytes = bytes;
> +	rx->packets = packets;

[Severity: Medium]
Should this also read stats->rcq.dropped and stats->rcq.errors from the
queue stats and assign them to rx->hw_drops or similar fields? The
driver collects these internal stats, but they appear to be missing from
the queue stats API reporting here.

> +}
> +
> +static void mpnic_get_queue_stats_tx(struct net_device *dev, int idx,
> +				     struct netdev_queue_stats_tx *tx)
> +{
> +	struct mpnic_net *mpn = netdev_priv(dev);
> +	struct mpnic_ring *txr = mpn->tx[idx];
> +	struct mpnic_queue_stats *stats;
> +	struct mpnic_q_triad *qt;
> +	unsigned int start;
> +	u64 bytes, packets;
> +
> +	if (!txr)
> +		return;
> +
> +	qt = container_of(txr, struct mpnic_q_triad, sub0);
> +
> +	stats = &qt->cmpl.stats;
> +	do {
> +		start = u64_stats_fetch_begin(&stats->syncp);
> +		bytes = u64_stats_read(&stats->tcq.bytes);
> +		packets = u64_stats_read(&stats->tcq.packets);
> +	} while (u64_stats_fetch_retry(&stats->syncp, start));
> +
> +	tx->bytes = bytes;
> +	tx->packets = packets;

[Severity: Medium]
Does this miss assigning stats->twq.dropped from the transmission queue
stats to tx->hw_drops or tx->hw_drop_errors?

> +}
> +
> +static void mpnic_get_base_stats(struct net_device *dev,
> +				 struct netdev_queue_stats_rx *rx,
> +				 struct netdev_queue_stats_tx *tx)
> +{
> +	struct mpnic_net *mpn = netdev_priv(dev);
> +	struct mpnic_base_stats *base;
> +
> +	base = netdev_lock_dereference(mpn->stats.base, dev);
> +
> +	tx->bytes = base->tx.bytes;
> +	tx->packets = base->tx.packets;
> +
> +	rx->bytes = base->rx.bytes;
> +	rx->packets = base->rx.packets;

[Severity: Medium]
Since the deactivated rings collect drop and error counts internally,
should this also map base->tx.dropped, base->rx.dropped, and
base->rx.errors to the corresponding rx and tx struct fields?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-mpnic-counters3-v1-0-7dc7644cc500@gmail.com?part=2