From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id ECC09C433E1 for ; Tue, 25 Aug 2020 16:46:42 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id BB80B207BC for ; Tue, 25 Aug 2020 16:46:42 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1598374002; bh=xPf4qnr4U10YGaUJZ1G5IsRUUwhYWrqDszxcH16X62Y=; h=Date:From:To:Cc:Subject:In-Reply-To:References:List-ID:From; b=iWPslfgWtitlGTfcDp/dnmzpmkydSN6yjPlZz2OIjY3lvIWuDVmERkAdmsm1cmVYB xkhJIYCWhZqtu9r1tKsM89EPqUgzrh2f5Yj5qjeJphEiCsT/MjxZhRaFBzoG0YT2hz UY1m08JDI6SP0yCDt//B4qwX6tn3Tk/M51uLMqgg= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726713AbgHYQql (ORCPT ); Tue, 25 Aug 2020 12:46:41 -0400 Received: from mail.kernel.org ([198.145.29.99]:39036 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726356AbgHYQqi (ORCPT ); Tue, 25 Aug 2020 12:46:38 -0400 Received: from kicinski-fedora-PC1C0HJN (unknown [163.114.132.6]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 91F3F20782; Tue, 25 Aug 2020 16:46:37 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1598373997; bh=xPf4qnr4U10YGaUJZ1G5IsRUUwhYWrqDszxcH16X62Y=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=nDfrk6jLD7xqv6LU0haWgF1GbNXkCrNlAlI3fge5N9f63UH4+I3J7rWCh6qqRHwYZ n/sL7ByRLaoAT/WXr42x9MaHCRzHDQP6SN5sBngmmxJo5B5MZVdE5LFFkVgMTDjADz fQX9ZN7+wFC4Ist2/3adTKSeVduTbSyIHabUF4fc= Date: Tue, 25 Aug 2020 09:46:35 -0700 From: Jakub Kicinski To: David Awogbemila , Yangchun Fu Cc: netdev@vger.kernel.org, Kuo Zhao , Willem de Bruijn Subject: Re: [PATCH net-next 05/18] gve: Add Gvnic stats AQ command and ethtool show/set-priv-flags. Message-ID: <20200825094635.715db5c0@kicinski-fedora-PC1C0HJN> In-Reply-To: References: <20200818194417.2003932-1-awogbemila@google.com> <20200818194417.2003932-6-awogbemila@google.com> <20200818201350.58024c28@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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. > > > +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. > > > @@ -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?