Message-ID: <35C27A066ED4844F952811E08E4D95090D398A32@IRSMSX103.ger.corp.intel.com>
Date:   Mon, 28 Oct 2019 13:51:07 +0000
From:   "Grubba, Arkadiusz" <arkadiusz.grubba@...el.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "nhorman@...hat.com" <nhorman@...hat.com>,
        "sassmann@...hat.com" <sassmann@...hat.com>,
        "Bowers, AndrewX" <andrewx.bowers@...el.com>,
        "Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
        "Michael, Alice" <alice.michael@...el.com>
Subject: RE: [net-next 02/11] i40e: Add ability to display VF stats along
 with PF core stats

Hi,

The main info about _what_ and _why_ , as you wrote, is explained in the first (i.e. title) line.
Namely, this change was introduced to "Add ability to display VF stats along with PF core stats" (for i40e equipment as prefix "i40e:" stands for it in the title).

(And if it was about general issues, i.e. why we introduce such changes, then, of course, they usually result mostly from the needs reported, e.g. by users using a given solution, although this does not change the nature/significance of the change from a technical point of view.)

As for further comments, that's right, you rightly notice here that the basic VF statistics are displayed and there may actually be an alternative possibility to check them (or other, newer solutions may appear that may enable it). The solution introduced here is simply one of the options (and maybe also the basis for further development of it).
But I don't know exactly for what specific purpose you mention it here?
What is the question? ...
(but for sure, if I guess right what you would like to ask, it's good to keep in mind that no tool is perfectly well in itself to the full extent of all use cases or... preferences - that's why we have alternatives and generally good to have them.)

[But also, such considerations already fall, for example, into the area of user preferences. And of course, the role of this patch is not to want to influence someone's preferences but only to provide some opportunity (as opposed to limiting the possibility of using various solutions, which should probably not be our goal...)
because among others here, this particular change is to be made available in connection with the exact and targeted needs raised by the users of the equipment affected by this code.]

As for the last point, this is indeed some oversight - yes, the last sentence is now unnecessary after rearranging this patch to meet the final requirements / agreements for the upstream (in-tree) version of it (as I also mentioned in my previous email - see attachment).
I think that instead of this last sentence in the commit message discussed here, and if you think it is important here, we may add (copy) from the original commit message this part of the text regarding description of displayed statistics:

+Testing hints:
+
+Use ethtool -S with this PF interface and check the output.
+Extra lines with the prefix "vf" should be displayed, e.g.:
+"
+     vf012.rx_bytes: 69264721849
+     vf012.rx_unicast: 45629259
+     vf012.rx_multicast: 9
+     vf012.rx_broadcast: 1
+     vf012.rx_discards: 2958
+     vf012.rx_unknown_protocol: 0
+     vf012.tx_bytes: 93048734
+     vf012.tx_unicast: 1409700
+     vf012.tx_multicast: 11
+     vf012.tx_broadcast: 0
+     vf012.tx_discards: 0
+     vf012.tx_errors: 0
+"
+(it's an example of a whole stats block for one VF).
+
+(For more specific tests:
+Create some VF interfaces, link them and give them IP addresses.
+Generate same network traffic and then follow the instructions above.)


(but for me it's really not certain whether in this particular case a larger description means better, especially since it is not so important from the point of view of the functioning of the kernel / driver or the system or interaction with them.)

Best Regards
A.G.


-----Original Message-----
From: Jakub Kicinski [mailto:jakub.kicinski@...ronome.com] 
Sent: Thursday, October 24, 2019 5:42 AM
To: Kirsher, Jeffrey T <jeffrey.t.kirsher@...el.com>
Cc: davem@...emloft.net; Grubba, Arkadiusz <arkadiusz.grubba@...el.com>; netdev@...r.kernel.org; nhorman@...hat.com; sassmann@...hat.com; Bowers, AndrewX <andrewx.bowers@...el.com>
Subject: Re: [net-next 02/11] i40e: Add ability to display VF stats along with PF core stats

On Wed, 23 Oct 2019 11:24:17 -0700, Jeff Kirsher wrote:
> From: Arkadiusz Grubba <arkadiusz.grubba@...el.com>
> 
> This change introduces the ability to display extended (enhanced) 
> statistics for PF interfaces.
> 
> The patch introduces new arrays defined for these extra stats (in 
> i40e_ethtool.c file) and enhances/extends ethtool ops functions 
> intended for dealing with PF stats (i.e.: i40e_get_stats_count(), 
> i40e_get_ethtool_stats(), i40e_get_stat_strings() ).

This commit message doesn't explain _what_ stats your adding, and _why_.

>From glancing at the code you're dumping 128 * 12 stats, which are basic netdev stats per-VF. 

These are trivially exposed on representors in modern designs.

> There have also been introduced the new build flag named 
> "I40E_PF_EXTRA_STATS_OFF" to exclude from the driver code all code 
> snippets associated with these extra stats.

And this doesn't even exist in the patch.

> Signed-off-by: Arkadiusz Grubba <arkadiusz.grubba@...el.com>
> Tested-by: Andrew Bowers <andrewx.bowers@...el.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@...el.com>

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

View attachment "email.txt" of type "text/plain" (9983 bytes)

Powered by blists - more mailing lists