by Jakub Kicinski

[permalink] [raw]

Subject: Re: [RFC net-next v4 2/2] net/mlx5e: Add per queue netdev-genl stats

On Fri, 7 Jun 2024 10:53:55 +0300 Tariq Toukan wrote:
> I don't want to create more work for you, but IMO in the longterm I
> should follow it up with a patch that adds PTP-RX to real_num_rx_queues.

Please don't. real_num_*x_queues is basically uAPI. Let's not modify
behavior at the uAPI level to work around deficiencies of the device.

2024-06-12 20:14:21

by Joe Damato

[permalink] [raw]

Subject: Re: [RFC net-next v4 2/2] net/mlx5e: Add per queue netdev-genl stats

On Fri, Jun 07, 2024 at 01:50:37PM -0700, Joe Damato wrote:
> On Thu, Jun 06, 2024 at 11:11:57PM +0300, Tariq Toukan wrote:
> >
> >
> > On 04/06/2024 3:46, Joe Damato wrote:
> > > ./cli.py --spec netlink/specs/netdev.yaml \
> > > --dump qstats-get --json '{"scope": "queue"}'
> > >
> > > ...snip
> > >
> > > {'ifindex': 7,
> > > 'queue-id': 62,
> > > 'queue-type': 'rx',
> > > 'rx-alloc-fail': 0,
> > > 'rx-bytes': 105965251,
> > > 'rx-packets': 179790},
> > > {'ifindex': 7,
> > > 'queue-id': 0,
> > > 'queue-type': 'tx',
> > > 'tx-bytes': 9402665,
> > > 'tx-packets': 17551},
> > >
> > > ...snip
> > >
> > > Also tested with the script tools/testing/selftests/drivers/net/stats.py
> > > in several scenarios to ensure stats tallying was correct:
> > >
> > > - on boot (default queue counts)
> > > - adjusting queue count up or down (ethtool -L eth0 combined ...)
> > >
> > > The tools/testing/selftests/drivers/net/stats.py brings the device up,
> > > so to test with the device down, I did the following:
> > >
> > > $ ip link show eth4
> > > 7: eth4: <BROADCAST,MULTICAST> mtu 9000 qdisc mq state DOWN [..snip..]
> > > [..snip..]
> > >
> > > $ cat /proc/net/dev | grep eth4
> > > eth4: 235710489 434811 [..snip rx..] 2878744 21227 [..snip tx..]
> > >
> > > $ ./cli.py --spec ../../../Documentation/netlink/specs/netdev.yaml \
> > > --dump qstats-get --json '{"ifindex": 7}'
> > > [{'ifindex': 7,
> > > 'rx-alloc-fail': 0,
> > > 'rx-bytes': 235710489,
> > > 'rx-packets': 434811,
> > > 'tx-bytes': 2878744,
> > > 'tx-packets': 21227}]
> > >
> > > Compare the values in /proc/net/dev match the output of cli for the same
> > > device, even while the device is down.
> > >
> > > Note that while the device is down, per queue stats output nothing
> > > (because the device is down there are no queues):
> >
> > This part is not true anymore.
> >
> > >
> > > $ ./cli.py --spec ../../../Documentation/netlink/specs/netdev.yaml \
> > > --dump qstats-get --json '{"scope": "queue", "ifindex": 7}'
> > > []
> > >
> > > Signed-off-by: Joe Damato <[email protected]>
> > > ---
> > > .../net/ethernet/mellanox/mlx5/core/en_main.c | 138 ++++++++++++++++++
> > > 1 file changed, 138 insertions(+)
> > >
> > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > > index d03fd1c98eb6..76d64bbcf250 100644
> > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > > @@ -39,6 +39,7 @@
> > > #include <linux/debugfs.h>
> > > #include <linux/if_bridge.h>
> > > #include <linux/filter.h>
> > > +#include <net/netdev_queues.h>
> > > #include <net/page_pool/types.h>
> > > #include <net/pkt_sched.h>
> > > #include <net/xdp_sock_drv.h>
> > > @@ -5279,6 +5280,142 @@ static bool mlx5e_tunnel_any_tx_proto_supported(struct mlx5_core_dev *mdev)
> > > return (mlx5_vxlan_allowed(mdev->vxlan) || mlx5_geneve_tx_allowed(mdev));
> > > }
> > > +static void mlx5e_get_queue_stats_rx(struct net_device *dev, int i,
> > > + struct netdev_queue_stats_rx *stats)
> > > +{
> > > + struct mlx5e_priv *priv = netdev_priv(dev);
> > > + struct mlx5e_channel_stats *channel_stats;
> > > + struct mlx5e_rq_stats *xskrq_stats;
> > > + struct mlx5e_rq_stats *rq_stats;
> > > +
> > > + ASSERT_RTNL();
> > > + if (mlx5e_is_uplink_rep(priv))
> > > + return;
> > > +
> > > + /* ptp was ever opened, is currently open, and channel index matches i
> > > + * then export stats
> > > + */
> > > + if (priv->rx_ptp_opened && priv->channels.ptp) {
> > > + if (test_bit(MLX5E_PTP_STATE_RX, priv->channels.ptp->state) &&
> > > + priv->channels.ptp->rq.ix == i) {
> >
> > PTP RQ index is naively assigned to zero:
> > rq->ix = MLX5E_PTP_CHANNEL_IX;
> >
> > but this isn't to be used as the stats index.
> > Today, the PTP-RQ has no matcing rxq in the kernel level.
> > i.e. turning PTP-RQ on won't add a kernel-level RXQ to the
> > real_num_rx_queues.
> > Maybe we better do.
> > If not, and the current state is kept, the best we can do is let the PTP-RQ
> > naively contribute its queue-stat to channel 0.
> >
> > > + rq_stats = &priv->ptp_stats.rq;
> > > + stats->packets = rq_stats->packets;
> > > + stats->bytes = rq_stats->bytes;
> > > + stats->alloc_fail = rq_stats->buff_alloc_err;
> > > + return;
> > > + }
> > > + }
> > > +
> > > + channel_stats = priv->channel_stats[i];
> > > + xskrq_stats = &channel_stats->xskrq;
> > > + rq_stats = &channel_stats->rq;
> > > +
> > > + stats->packets = rq_stats->packets + xskrq_stats->packets;
> > > + stats->bytes = rq_stats->bytes + xskrq_stats->bytes;
> > > + stats->alloc_fail = rq_stats->buff_alloc_err +
> > > + xskrq_stats->buff_alloc_err;
> > > +}
> > > +
> > > +static void mlx5e_get_queue_stats_tx(struct net_device *dev, int i,
> > > + struct netdev_queue_stats_tx *stats)
> > > +{
> > > + struct mlx5e_priv *priv = netdev_priv(dev);
> > > + struct mlx5e_sq_stats *sq_stats;
> > > +
> > > + ASSERT_RTNL();
> > > + /* no special case needed for ptp htb etc since txq2sq_stats is kept up
> > > + * to date for active sq_stats, otherwise get_base_stats takes care of
> > > + * inactive sqs.
> > > + */
> > > + sq_stats = priv->txq2sq_stats[i];
> > > + stats->packets = sq_stats->packets;
> > > + stats->bytes = sq_stats->bytes;
> > > +}
> > > +
> > > +static void mlx5e_get_base_stats(struct net_device *dev,
> > > + struct netdev_queue_stats_rx *rx,
> > > + struct netdev_queue_stats_tx *tx)
> > > +{
> > > + struct mlx5e_priv *priv = netdev_priv(dev);
> > > + int i, tc;
> > > +
> > > + ASSERT_RTNL();
> > > + if (!mlx5e_is_uplink_rep(priv)) {
> > > + rx->packets = 0;
> > > + rx->bytes = 0;
> > > + rx->alloc_fail = 0;
> > > +
> > > + for (i = priv->channels.params.num_channels; i < priv->stats_nch; i++) {
> > > + struct netdev_queue_stats_rx rx_i = {0};
> > > +
> > > + mlx5e_get_queue_stats_rx(dev, i, &rx_i);
> > > +
> > > + rx->packets += rx_i.packets;
> > > + rx->bytes += rx_i.bytes;
> > > + rx->alloc_fail += rx_i.alloc_fail;
> > > + }
> > > +
> > > + if (priv->rx_ptp_opened) {
> > > + /* if PTP was opened, but is not currently open, then
> > > + * report the stats here. otherwise,
> > > + * mlx5e_get_queue_stats_rx will get it
> > > + */
> >
> > We shouldn't care if the RQ is currently open. The stats are always there.
> > This applies to all RQs and SQs.
> >
> > > + if (priv->channels.ptp &&
> > > + !test_bit(MLX5E_PTP_STATE_RX, priv->channels.ptp->state)) {
> > > + struct mlx5e_rq_stats *rq_stats = &priv->ptp_stats.rq;
> > > +
> > > + rx->packets += rq_stats->packets;
> > > + rx->bytes += rq_stats->bytes;
> > > + }
> > > + }
> > > + }
> > > +
> > > + tx->packets = 0;
> > > + tx->bytes = 0;
> > > +
> > > + for (i = 0; i < priv->stats_nch; i++) {
> > > + struct mlx5e_channel_stats *channel_stats = priv->channel_stats[i];
> > > +
> > > + /* while iterating through all channels [0, stats_nch], there
> > > + * are two cases to handle:
> > > + *
> > > + * 1. the channel is available, so sum only the unavailable TCs
> > > + * [mlx5e_get_dcb_num_tc, max_opened_tc).
> > > + *
> > > + * 2. the channel is unavailable, so sum all TCs [0, max_opened_tc).
> > > + */
> >
> > Even if the channel is not available, mlx5e_get_queue_stats_tx() accesses
> > and returns its stats.
>
> Thanks for your review; I do sincerely appreciate it.
>
> I'm sorry, but either I'm misunderstanding something or I simply
> disagree with you.
>
> Imagine a simple case with 64 queues at boot, no QoS/HTB, no PTP.
>
> At boot, [0..63] are the txq_ixs that will be passed in as i to
> mlx5e_get_queue_stats_tx.
>
> The user then reduces the queues to 4 via ethtool.
>
> Now [0..3] are the txq_ixs that will be passed in as i to
> mlx5e_get_queue_stats_tx.
>
> In both cases: [0..real_num_tx_queues) are valid "i" that
> mlx5e_get_queue_stats_tx will have stats for.
>
> Now, consider the code for mlx5e_get_dcb_num_tc (from
> drivers/net/ethernet/mellanox/mlx5/core/en.h):
>
> return params->mqprio.mode == TC_MQPRIO_MODE_DCB ?
> params->mqprio.num_tc : 1;
>
> In our simple case calling mlx5e_get_dcb_num_tc returns 1.
> In mlx5e_priv_init, the code sets priv->max_opened_tc = 1.
>
> If we simply do a loop like this:
>
> [0...stats->n_ch)
> [ mlx5e_get_dcb_num_tc ... max_opened_tc )
> [...collect_tx_stats...]
>
> We won't collect any stats for channels 4..63 in our example above
> because the inner loop because [1..1) which does nothing.
>
> To confirm this, I tested the above loop anyway and the test script
> showed that stats were missing:
>
> NETIF=eth4 tools/testing/selftests/drivers/net/stats.py
>
> # Exception| Exception: Qstats are lower, fetched later
> not ok 3 stats.pkt_byte_sum
>
> I think the original code for TX stats in base for deactivated
> queues may be correct.
>
> Another way to explain the problem: any queue from [4..63] will be
> gone, so we need to accumulate the stats from all TCs from 0 to
> max_opened_tc (which in our example is 1).
>
> Can you let me know if you agree? I would like to submit a real v5
> which will include:
> - output PTP RX in base always if it was ever opened
> - output PTP TX in base only if it was ever opened and ptp is NULL
> or the bit is unset
> - leave the existing TX queue code in base from this RFC v4 (other
> than the changes to PTP TX)
>
> Because in my tests using:
>
> NETIF=eth4 tools/testing/selftests/drivers/net/stats.py
>
> shows that stats match:
>
> KTAP version 1
> 1..4
> ok 1 stats.check_pause
> ok 2 stats.check_fec
> ok 3 stats.pkt_byte_sum
> ok 4 stats.qstat_by_ifindex
> # Totals: pass:4 fail:0 xfail:0 xpass:0 skip:0 error:0

I didn't hear back on the above, so I've sent a v5 that leaves the
TX logic mostly the same (except for PTP).

As explained above, tests suggest that this is correct unless I am
missing something -- which I totally could be.

I've added a comment to the code to explain what it is doing, which
will hopefully make my thought process more clear.

Let's pick this back up with any comments on the v5 thread:

https://lore.kernel.org/netdev/[email protected]/

Thanks,
Joe