Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jakub Kicinski <kuba@kernel.org>
To: David Awogbemila <awogbemila@google.com>
Cc: Yangchun Fu <yangchun@google.com>,
	netdev@vger.kernel.org, Kuo Zhao <kuozhao@google.com>,
	Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Subject: Re: [PATCH net-next 05/18] gve: Add Gvnic stats AQ command and ethtool show/set-priv-flags.
Date: Tue, 25 Aug 2020 17:53:06 -0700	[thread overview]
Message-ID: <20200825175306.377be2f4@kicinski-fedora-PC1C0HJN> (raw)
In-Reply-To: <CAL9ddJfOWzO1v2FJAtb+qVAazR9Tb3CV8kH8V0_xA-GPgoAKXQ@mail.gmail.com>

On Tue, 25 Aug 2020 17:06:27 -0700 David Awogbemila wrote:
> On Tue, Aug 25, 2020 at 9:46 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > On Tue, 25 Aug 2020 08:46:12 -0700 David Awogbemila wrote:  
> > > > > +     // stats from NIC
> > > > > +     RX_QUEUE_DROP_CNT               = 65,
> > > > > +     RX_NO_BUFFERS_POSTED            = 66,
> > > > > +     RX_DROPS_PACKET_OVER_MRU        = 67,
> > > > > +     RX_DROPS_INVALID_CHECKSUM       = 68,  
> > > >
> > > > Most of these look like a perfect match for members of struct
> > > > rtnl_link_stats64. Please use the standard stats to report the errors,
> > > > wherever possible.  
> > > These stats are based on the NIC stats format which don't exactly
> > > match rtnl_link_stats64.
> > > I'll add some clarification in the description and within the comments.  
> >
> > You must report standard stats. Don't be lazy and just dump everything
> > in ethtool -S and expect the user to figure out the meaning of your
> > strings.  
> Apologies for responding a week later, I'll try to respond more quickly.

Thanks!

> I could use some help figuring out the use of rtnl_link_stats64 here.
> These 4 stats are per-queue stats written by the NIC. It looks like
> rtnl_link_stats64 is meant to sum stats for the entire device? Is the
> requirement here simply to use the member names in rtnl_link_stats64
> when reporting stats via ethtool? Thanks.

If these are per queue you can report them per queue in ethtool (name
is up to you), but you must report the sum over all queues in
rtnl_link_stats64.

FWIW the mapping I'd suggest is probably:

RX_QUEUE_DROP_CNT         -> rx_dropped
RX_NO_BUFFERS_POSTED      -> rx_missed_errors
RX_DROPS_PACKET_OVER_MRU  -> rx_length_errors
RX_DROPS_INVALID_CHECKSUM -> rx_crc_errors

The no-buffers-posted stat is unfortunately slightly disputable between
rx_missed_errors and rx_fifo_errors (even rx_over_errors). I'd go for missed.

> > > > > +static int gve_set_priv_flags(struct net_device *netdev, u32 flags)
> > > > > +{
> > > > > +     struct gve_priv *priv = netdev_priv(netdev);
> > > > > +     u64 ori_flags, new_flags;
> > > > > +     u32 i;
> > > > > +
> > > > > +     ori_flags = READ_ONCE(priv->ethtool_flags);
> > > > > +     new_flags = ori_flags;
> > > > > +
> > > > > +     for (i = 0; i < GVE_PRIV_FLAGS_STR_LEN; i++) {
> > > > > +             if (flags & BIT(i))
> > > > > +                     new_flags |= BIT(i);
> > > > > +             else
> > > > > +                     new_flags &= ~(BIT(i));
> > > > > +             priv->ethtool_flags = new_flags;
> > > > > +             /* set report-stats */
> > > > > +             if (strcmp(gve_gstrings_priv_flags[i], "report-stats") == 0) {
> > > > > +                     /* update the stats when user turns report-stats on */
> > > > > +                     if (flags & BIT(i))
> > > > > +                             gve_handle_report_stats(priv);
> > > > > +                     /* zero off gve stats when report-stats turned off */
> > > > > +                     if (!(flags & BIT(i)) && (ori_flags & BIT(i))) {
> > > > > +                             int tx_stats_num = GVE_TX_STATS_REPORT_NUM *
> > > > > +                                     priv->tx_cfg.num_queues;
> > > > > +                             int rx_stats_num = GVE_RX_STATS_REPORT_NUM *
> > > > > +                                     priv->rx_cfg.num_queues;
> > > > > +                             memset(priv->stats_report->stats, 0,
> > > > > +                                    (tx_stats_num + rx_stats_num) *
> > > > > +                                    sizeof(struct stats));  
> > > >
> > > > I don't quite get why you need the knob to disable some statistics.
> > > > Please remove or explain this in the cover letter. Looks unnecessary.  
> > > We use this to give the guest the option of disabling stats reporting
> > > through ethtool set-priv-flags. I'll update the cover letter.  
> >
> > I asked you why you reply a week later with "I want to give user the
> > option. I'll update the cover letter." :/ That's quite painful for the
> > reviewer. Please just provide the justification.  
> I apologize for the pain; it certainly wasn't intended :) .
> Just to clarify, stats will always be available to the user via ethtool.
> This is only giving users the option of disabling the reporting of
> stats from the driver to the virtual NIC should the user decide they
> do not want to share driver stats with Google as a matter of privacy.

Okay, so this is for the to-hypervisor direction. Hopefully the patch
split will make this clearer.

> > > > > @@ -880,6 +953,10 @@ static void gve_handle_status(struct gve_priv *priv, u32 status)
> > > > >               dev_info(&priv->pdev->dev, "Device requested reset.\n");
> > > > >               gve_set_do_reset(priv);
> > > > >       }
> > > > > +     if (GVE_DEVICE_STATUS_REPORT_STATS_MASK & status) {
> > > > > +             dev_info(&priv->pdev->dev, "Device report stats on.\n");  
> > > >
> > > > How often is this printed?  
> > > Stats reporting is disabled by default. But when enabled, this would
> > > only get printed whenever the virtual NIC detects
> > > an issue and triggers a report-stats request.  
> >
> > What kind of issue? Something serious? Packet drops?  
> Sorry, to correct myself, this would get printed only at the moments
> when the device switches report-stats on, not on every stats report.
> After that, it would not get printed until it is switched off and then
> on again, so rarely.
> It would get switched on if there is a networking issue such as packet
> drops and help us investigate a stuck queue for example.

Reporting of the stats is a one-shot:

+	if (gve_get_do_report_stats(priv)) {
+		gve_handle_report_stats(priv);
+		gve_clear_do_report_stats(priv);
+	}

So the hypervisor implements some rate limiting on the requests?

In general, I don't think users will want these messages to keep
popping up. A ethtool -S statistic for the number of times reporting
happened should be sufficient. Or if you really want them - have a
verbose version of the priv flag, maybe?

  reply	other threads:[~2020-08-26  1:10 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-18 19:43 [PATCH net-next 00/18] GVE Driver v1.1.0 Features David Awogbemila
2020-08-18 19:44 ` [PATCH net-next 01/18] gve: Get and set Rx copybreak via ethtool David Awogbemila
2020-08-18 20:00   ` Andrew Lunn
2020-08-18 19:44 ` [PATCH net-next 02/18] gve: Add stats for gve David Awogbemila
2020-08-18 19:44 ` [PATCH net-next 03/18] gve: Register netdev earlier David Awogbemila
2020-08-18 20:09   ` Andrew Lunn
2020-08-18 19:44 ` [PATCH net-next 04/18] gve: Add support for dma_mask register David Awogbemila
2020-08-18 20:15   ` Andrew Lunn
2020-08-18 19:44 ` [PATCH net-next 05/18] gve: Add Gvnic stats AQ command and ethtool show/set-priv-flags David Awogbemila
2020-08-19  3:13   ` Jakub Kicinski
2020-08-25 15:46     ` David Awogbemila
2020-08-25 16:46       ` Jakub Kicinski
2020-08-26  0:06         ` David Awogbemila
2020-08-26  0:53           ` Jakub Kicinski [this message]
2020-08-27 19:24             ` David Awogbemila
2020-08-18 19:44 ` [PATCH net-next 06/18] gve: Batch AQ commands for creating and destroying queues David Awogbemila
2020-08-18 20:16   ` David Miller
2020-08-18 22:25     ` David Awogbemila
2020-08-18 19:44 ` [PATCH net-next 07/18] gve: Use link status register to report link status David Awogbemila
2020-08-19  3:36   ` Jakub Kicinski
2020-08-25 15:46     ` David Awogbemila
2020-08-18 19:44 ` [PATCH net-next 08/18] gve: Enable Link Speed Reporting in the driver David Awogbemila
2020-08-18 21:30   ` Jakub Kicinski
2020-08-18 19:44 ` [PATCH net-next 09/18] gve: Add support for raw addressing device option David Awogbemila
2020-08-18 19:44 ` [PATCH net-next 10/18] gve: Add support for raw addressing to the rx path David Awogbemila
2020-08-18 20:18   ` David Miller
2020-08-18 22:25     ` David Awogbemila
2020-08-18 19:44 ` [PATCH net-next 11/18] gve: Add support for raw addressing in the tx path David Awogbemila
2020-08-18 19:44 ` [PATCH net-next 12/18] gve: Add netif_set_xps_queue call David Awogbemila
2020-08-18 19:44 ` [PATCH net-next 13/18] gve: Add rx buffer pagecnt bias David Awogbemila
2020-08-18 19:44 ` [PATCH net-next 14/18] gve: Move the irq db indexes out of the ntfy block struct David Awogbemila
2020-08-18 19:44 ` [PATCH net-next 15/18] gve: Prefetch packet pages and packet descriptors David Awogbemila
2020-08-18 19:44 ` [PATCH net-next 16/18] gve: Also WARN for skb index equals num_queues David Awogbemila
2020-08-18 19:44 ` [PATCH net-next 17/18] gve: Switch to use napi_complete_done David Awogbemila
2020-08-18 19:44 ` [PATCH net-next 18/18] gve: Bump version to 1.1.0 David Awogbemila
2020-08-19  3:40   ` Jakub Kicinski
2020-08-18 20:19 ` [PATCH net-next 00/18] GVE Driver v1.1.0 Features David Miller
2020-08-18 22:24   ` David Awogbemila

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=20200825175306.377be2f4@kicinski-fedora-PC1C0HJN \
    --to=kuba@kernel.org \
    --cc=awogbemila@google.com \
    --cc=kuozhao@google.com \
    --cc=netdev@vger.kernel.org \
    --cc=willemdebruijn.kernel@gmail.com \
    --cc=yangchun@google.com \
    --subject='Re: [PATCH net-next 05/18] gve: Add Gvnic stats AQ command and ethtool show/set-priv-flags.' \
    /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

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 NNTP newsgroup(s).