From: Jakub Kicinski <kuba@kernel.org>
To: Joe Damato <jdamato@fastly.com>
Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
tariqt@nvidia.com, saeedm@nvidia.com, mkarsten@uwaterloo.ca,
gal@nvidia.com, nalramli@fastly.com,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Paolo Abeni <pabeni@redhat.com>,
"open list:MELLANOX MLX4 core VPI driver"
<linux-rdma@vger.kernel.org>
Subject: Re: [PATCH net-next 3/3] net/mlx4: support per-queue statistics via netlink
Date: Wed, 24 Apr 2024 07:28:18 -0700 [thread overview]
Message-ID: <20240424072818.2c68a1ab@kernel.org> (raw)
In-Reply-To: <ZiieqiuqNiy_W0mr@LQ3V64L9R2>
On Tue, 23 Apr 2024 22:54:50 -0700 Joe Damato wrote:
> 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 base is used to figure out which stats are reported when we dump
a summary for the whole device. So you gotta set them to 0.
> > 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).
All the ones you report right? Not just zero the struct.
Any day now (tm) someone will add a lot more stats to the struct
so the init should be selective only to the stats that are actually
supported.
> 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 :)
My guess is that on multi-port only one of the netdevs is "in charge"
of the PCIe function. But these are queue stat, PCIe ownership may
matter for refresh but not for having the stats. So my guess must be
wrong..
> > > 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.
mlx5 would be awesome! But no pressure on sending ASAP. I was writing a
test for page pool allocation failures, which depends on qstat to check
if driver actually saw the errors (I'll send it later today), and I
couldn't confirm it working on mlx5 :(
next prev parent reply other threads:[~2024-04-24 14:28 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-23 19:49 [PATCH net-next 0/3] mlx4: Add support for netdev-genl API Joe Damato
2024-04-23 19:49 ` [PATCH net-next 1/3] net/mlx4: Track RX allocation failures in a stat Joe Damato
2024-04-23 22:38 ` Joe Damato
2024-04-24 6:27 ` kernel test robot
2024-04-23 19:49 ` [PATCH net-next 2/3] net/mlx4: link NAPI instances to queues and IRQs Joe Damato
2024-04-24 7:41 ` kernel test robot
2024-04-23 19:49 ` [PATCH net-next 3/3] net/mlx4: support per-queue statistics via netlink Joe Damato
2024-04-23 22:42 ` Joe Damato
2024-04-24 0:57 ` Jakub Kicinski
2024-04-24 5:54 ` Joe Damato
2024-04-24 14:28 ` Jakub Kicinski [this message]
2024-04-24 16:39 ` Joe Damato
2024-04-25 3:46 ` Jakub Kicinski
2024-04-25 16:31 ` Joe Damato
2024-04-30 12:33 ` Leon Romanovsky
2024-04-30 16:18 ` Joe Damato
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240424072818.2c68a1ab@kernel.org \
--to=kuba@kernel.org \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=gal@nvidia.com \
--cc=jdamato@fastly.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rdma@vger.kernel.org \
--cc=mkarsten@uwaterloo.ca \
--cc=nalramli@fastly.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=saeedm@nvidia.com \
--cc=tariqt@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).