Re: [PATCH net-next 3/3] net/mlx4: support per-queue statistics via netlink

[Joe Damato <jdamato@fastly.com>]

On Tue, Apr 23, 2024 at 05:57:18PM -0700, Jakub Kicinski wrote:
> On Tue, 23 Apr 2024 12:42:13 -1000 Joe Damato wrote:
> > I realized in this case, I'll need to set the fields initialized to 0xff
> > above to 0 before doing the increments below.
> 
> I don't know mlx4 very well, but glancing at the code - are you sure we
> need to loop over the queues is the "base" callbacks?
> 
> The base callbacks are for getting "historical" data, i.e. info which
> was associated with queues which are no longer present. You seem to
> sweep all queues, so I'd have expected "base" to just set the values 
> to 0. And the real values to come from the per-queue callbacks.

Hmm. Sorry I must have totally misunderstood what the purpose of "base"
was. I've just now more closely looked at bnxt which (maybe?) is the only
driver that implements base and I think maybe I kind of get it now.

For some reason, I thought it meant "the total stats of all queues"; I didn't
know it was intended to provide "historical" data as you say.

Making it set everything to 0 makes sense to me. I suppose I could also simply
omit it? What do you think?

> The init to 0xff looks quite sus.

Yes the init to 0xff is wrong, too. I noticed that, as well.

Here's what I have listed so far in my changelog for the v2 (which I haven't
sent yet), but perhaps the maintainers of mlx4 can weigh in?

v1 -> v2:
 - Patch 1/3 now initializes dropped to 0.
 - Patch 3/3 includes several changes:
   - mlx4_get_queue_stats_rx and mlx4_get_queue_stats_tx check if i is
     valid before proceeding.
   - All initialization to 0xff for stats fields has been omit. The
     network stack does this before calling into the driver functions, so
     I've adjusted the driver functions to only set values if there is
     data to set, leaving the network stack's 0xff in place if not.
   - mlx4_get_base_stats sets all stats to 0 (no locking etc needed).

Let me know if that sounds vaguely correct?

> Also what does this:
> 
> >	if (!priv->port_up || mlx4_is_master(priv->mdev->dev))
> 
> do? 🤔️ what's a "master" in this context?

I have a guess, but I'd rather let the Mellanox folks provide the official
answer :)

> > Sorry about that; just realized that now and will fix that in the v2 (along
> > with any other feedback I get), probably something:
> > 
> >   if (priv->rx_ring_num) {
> >           rx->packets = 0;
> >           rx->bytes = 0;
> >           rx->alloc_fail = 0;
> >   }
> > 
> > Here for the RX side and see below for the TX side.
> 
> FWIW I added a simple test for making sure queue stats match interface
> stats, it's tools/testing/selftests/drivers/net/stats.py
> 
> You have to export NETIF=$name to make it run on a real interface.
> 
> To copy the tests to a remote machine I do:
> 
> make -C tools/testing/selftests/ TARGETS="net drivers/net drivers/net/hw" install INSTALL_PATH=/tmp/ksft-net-drv
> rsync -ra --delete /tmp/ksft-net-drv root@${machine}:/root/
> 
> HTH

Thanks, this is a great help actually.

I have a similar changeset for mlx5 (which is hardware I do have access to)
that adds the per-queue stats stuff so I'll definitely give your test a try.

Seeing as I made a lot of errors in this series, I'll hold off on sending the
mlx5 series until this mlx4 series is fixed and accepted, that way I can
produce a much better v1 for mlx5.

Thanks,
Joe