Re: [PATCH net-next 0/1] mlx5: Add netdev-genl queue stats

Zhu Yanjun
2024-05-03T10:55:44Z

On 03.05.24 04:25, Joe Damato wrote:
> Hi:
>
> This is only 1 patch, so I know a cover letter isn't necessary, but it
> seems there are a few things to mention.
>
> This change adds support for the per queue netdev-genl API to mlx5,
> which seems to output stats:
>
> ./cli.py --spec ../../../Documentation/netlink/specs/netdev.yaml \
> --dump qstats-get --json '{"scope": "queue"}'
>
> ...snip
> {'ifindex': 7,
> 'queue-id': 28,
> 'queue-type': 'tx',
> 'tx-bytes': 399462,
> 'tx-packets': 3311},
> ...snip

Ethtool -S ethx can get the above information
"
...
tx-0.packets: 2094
tx-0.bytes: 294141
rx-0.packets: 2200
rx-0.bytes: 267673
...
"

>
> I've tried to use the tooling suggested to verify that the per queue
> stats match the rtnl stats by doing this:
>
> NETIF=eth0 tools/testing/selftests/drivers/net/stats.py
>
> And the tool outputs that there is a failure:
>
> # Exception| Exception: Qstats are lower, fetched later
> not ok 3 stats.pkt_byte_sum

With ethtool, does the above problem still occur?

Zhu Yanjun

>
> The other tests all pass (including stats.qstat_by_ifindex).
>
> This appears to mean that the netdev-genl queue stats have lower numbers
> than the rtnl stats even though the rtnl stats are fetched first. I
> added some debugging and found that both rx and tx bytes and packets are
> slightly lower.
>
> The only explanations I can think of for this are:
>
> 1. tx_ptp_opened and rx_ptp_opened are both true, in which case
> mlx5e_fold_sw_stats64 adds bytes and packets to the rtnl struct and
> might account for the difference. I skip this case in my
> implementation, so that could certainly explain it.
> 2. Maybe I'm just misunderstanding how stats aggregation works in mlx5,
> and that's why the numbers are slightly off?
>
> It appears that the driver uses a workqueue to queue stats updates which
> happen periodically.
>
> 0. the driver occasionally calls queue_work on the update_stats_work
> workqueue.
> 1. This eventually calls MLX5E_DECLARE_STATS_GRP_OP_UPDATE_STATS(sw),
> in drivers/net/ethernet/mellanox/mlx5/core/en_stats.c, which appears
> to begin by first memsetting the internal stats struct where stats are
> aggregated to zero. This would mean, I think, the get_base_stats
> netdev-genl API implementation that I have is correct: simply set
> everything to 0.... otherwise we'd end up double counting in the
> netdev-genl RX and TX handlers.
> 2. Next, each of the stats helpers are called to collect stats into the
> freshly 0'd internal struct (for example:
> mlx5e_stats_grp_sw_update_stats_rq_stats).
>
> That seems to be how stats are aggregated, which would suggest that if I
> simply .... do what I'm doing in this change the numbers should line up.
>
> But they don't and its either because of PTP or because I am
> misunderstanding/doing something wrong.
>
> Maybe the MLNX folks can suggest a hint?
>
> Thanks,
> Joe
>
> Joe Damato (1):
> net/mlx5e: Add per queue netdev-genl stats
>
> .../net/ethernet/mellanox/mlx5/core/en_main.c | 68 +++++++++++++++++++
> 1 file changed, 68 insertions(+)
>