From: Joe Damato <jdamato@fastly.com>
To: Jakub Kicinski <kuba@kernel.org>
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: Thu, 25 Apr 2024 09:31:14 -0700 [thread overview]
Message-ID: <ZiqFUs-z5We2--5n@LQ3V64L9R2> (raw)
In-Reply-To: <Zik1zCI9W9EUi13T@LQ3V64L9R2>
On Wed, Apr 24, 2024 at 09:39:43AM -0700, Joe Damato wrote:
> On Wed, Apr 24, 2024 at 07:28:18AM -0700, Jakub Kicinski wrote:
> > 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.
>
> OK, thanks for your patience and the explanation. Will do.
>
> > > > 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.
>
> Yes, not just zero the struct. Since I am reporting bytes packets for both
> RX and TX and alloc_fail for RX I'll be setting those fields to 0
> explicitly.
>
> And there's also a warning about unused qtype (oops) in patch 2/3.
>
> So, the revised v2 list (pending anything Mellanox wants) is:
>
> v1 -> v2:
> - Patch 1/3 now initializes dropped to 0.
> - Patch 2/3 fix use of unitialized qtype warning.
> - 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 set all stat fields to 0 individually (no locking etc needed).
>
> I'll hold off on sending this v2 until we hear back from Mellanox to be
> sure there isn't anything else I'm missing.
It's been a few days and I haven't heard back from the mlx4 folks, so I
think I'll probably send my v2 later today which, hopefully, will fix most
of the above issues.
next prev parent reply other threads:[~2024-04-25 16:31 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
2024-04-24 16:39 ` Joe Damato
2024-04-25 3:46 ` Jakub Kicinski
2024-04-25 16:31 ` Joe Damato [this message]
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=ZiqFUs-z5We2--5n@LQ3V64L9R2 \
--to=jdamato@fastly.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=gal@nvidia.com \
--cc=kuba@kernel.org \
--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).