On Thu, Jun 13, 2024 at 11:25:12PM +0300, Tariq Toukan wrote:
>
>
> On 12/06/2024 23:08, 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):
>
> Yeah, the query doesn't reach the device driver...
Yes. Are you suggesting that I update the commit message? I can do
so, if you think that is needed?
> >
> > $ ./cli.py --spec ../../../Documentation/netlink/specs/netdev.yaml \
> > --dump qstats-get --json '{"scope": "queue", "ifindex": 7}'
> > []
> >
> > Signed-off-by: Joe Damato <jdamato@xxxxxxxxxx>
> > ---
> > .../net/ethernet/mellanox/mlx5/core/en_main.c | 132 ++++++++++++++++++
> > 1 file changed, 132 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > index c548e2fdc58f..d3f38b4b18eb 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>
> > @@ -5299,6 +5300,136 @@ 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;
> > +
> > + 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);
> > + struct mlx5e_ptp *ptp_channel;
> > + 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++) {
>
> IIUC, per the current kernel implementation, the lower parts won't be
> completed in a loop over [0..real_num_rx_queues-1], as that loop is
> conditional, happening only if the queues are active.
Sorry, but I'm probably missing something -- you said:
> as that loop is conditional, happening only if the queues are active.
I don't think so? Please continue reading for an example with code.
Let me clarify one thing, please? When you say "the lower parts"
here you mean [0...priv->channels.params.num_channels], is that
right?
If yes, I don't understand why the code in this v5 is wrong. It looks correct
to me, if my understanding of "lower parts" is right.
Here's an example:
1. Machine boots with 32 queues by default.
2. User runs ethtool -L eth0 combined 4
