| Message ID | 20191023182426.13233-3-jeffrey.t.kirsher@intel.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | 40GbE Intel Wired LAN Driver Updates 2019-10-23 | expand |
On Wed, 23 Oct 2019 11:24:17 -0700, Jeff Kirsher wrote: > From: Arkadiusz Grubba <arkadiusz.grubba@intel.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@intel.com> > Tested-by: Andrew Bowers <andrewx.bowers@intel.com> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
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@netronome.com] Sent: Thursday, October 24, 2019 5:42 AM To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com> Cc: davem@davemloft.net; Grubba, Arkadiusz <arkadiusz.grubba@intel.com>; netdev@vger.kernel.org; nhorman@redhat.com; sassmann@redhat.com; Bowers, AndrewX <andrewx.bowers@intel.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@intel.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@intel.com> > Tested-by: Andrew Bowers <andrewx.bowers@intel.com> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.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. From: Grubba, Arkadiusz Sent: Tuesday, September 10, 2019 11:58 PM To: Michael, Alice <alice.michael@intel.com>; e1000-patches@eclists.intel.com Subject: RE: [e1000-patches] [next PATCH S10 02/11] i40e: Add ability to display VF stats along with PF core stats Hi Alice, The last sentence in the commit message should be deleted because it is unnecessary/unrelated to this particular case. (I mean the sentence about the build flag.) And originally there was also a comment (five lines) to the code in the function i40e_get_stats_count() ... Apart from this, ACK. Thanks Arek -----Original Message----- From: e1000-patches-request@eclists.intel.com [mailto:e1000-patches-request@eclists.intel.com] On Behalf Of Michael, Alice Sent: Tuesday, September 10, 2019 10:00 PM To: Michael, Alice <alice.michael@intel.com>; e1000-patches@eclists.intel.com Cc: Grubba, Arkadiusz <arkadiusz.grubba@intel.com> Subject: [e1000-patches] [next PATCH S10 02/11] i40e: Add ability to display VF stats along with PF core stats From: Arkadiusz Grubba <arkadiusz.grubba@intel.com> This change introduces the ability to display extended (enhanced) statistics for PF interfaces (in accordance to the new build flags also introduced here). The patch introduces new arrays and preprocessor symbols 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() ). 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. Signed-off-by: Arkadiusz Grubba <arkadiusz.grubba@intel.com> --- .../net/ethernet/intel/i40e/i40e_ethtool.c | 149 ++++++++++++++++++ 1 file changed, 149 insertions(+) diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c index 41e1240acaea..c814c756b4bb 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c @@ -389,6 +389,7 @@ static const struct i40e_stats i40e_gstrings_pfc_stats[] = { #define I40E_GLOBAL_STATS_LEN ARRAY_SIZE(i40e_gstrings_stats) +/* Length (number) of PF core stats only (i.e. without queues / extra +stats): */ #define I40E_PF_STATS_LEN (I40E_GLOBAL_STATS_LEN + \ I40E_PFC_STATS_LEN + \ I40E_VEB_STATS_LEN + \ @@ -397,6 +398,44 @@ static const struct i40e_stats i40e_gstrings_pfc_stats[] = { /* Length of stats for a single queue */ #define I40E_QUEUE_STATS_LEN ARRAY_SIZE(i40e_gstrings_queue_stats) +#define I40E_STATS_NAME_VFID_EXTRA "vf___." +#define I40E_STATS_NAME_VFID_EXTRA_LEN +(sizeof(I40E_STATS_NAME_VFID_EXTRA) - 1) + +static struct i40e_stats i40e_gstrings_eth_stats_extra[] = { + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA + "rx_bytes", eth_stats.rx_bytes), + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA + "rx_unicast", eth_stats.rx_unicast), + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA + "rx_multicast", eth_stats.rx_multicast), + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA + "rx_broadcast", eth_stats.rx_broadcast), + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA + "rx_discards", eth_stats.rx_discards), + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA + "rx_unknown_protocol", eth_stats.rx_unknown_protocol), + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA + "tx_bytes", eth_stats.tx_bytes), + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA + "tx_unicast", eth_stats.tx_unicast), + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA + "tx_multicast", eth_stats.tx_multicast), + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA + "tx_broadcast", eth_stats.tx_broadcast), + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA + "tx_discards", eth_stats.tx_discards), + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA + "tx_errors", eth_stats.tx_errors), }; + +#define I40E_STATS_EXTRA_COUNT 128 /* as for now only I40E_MAX_VF_COUNT */ +/* Following length value does not include the length values for queues stats */ +#define I40E_STATS_EXTRA_LEN ARRAY_SIZE(i40e_gstrings_eth_stats_extra) +/* Length (number) of PF extra stats only (i.e. without core stats / +queues): */ #define I40E_PF_STATS_EXTRA_LEN (I40E_STATS_EXTRA_COUNT * +I40E_STATS_EXTRA_LEN) +/* Length (number) of enhanced/all PF stats (i.e. core with extra +stats): */ #define I40E_PF_STATS_ENHANCE_LEN (I40E_PF_STATS_LEN + +I40E_PF_STATS_EXTRA_LEN) + enum i40e_ethtool_test_id { I40E_ETH_TEST_REG = 0, I40E_ETH_TEST_EEPROM, @@ -2190,6 +2229,9 @@ static int i40e_get_stats_count(struct net_device *netdev) */ stats_len += I40E_QUEUE_STATS_LEN * 2 * netdev->num_tx_queues; + if (vsi == pf->vsi[pf->lan_vsi] && pf->hw.partition_id == 1) + stats_len += I40E_PF_STATS_EXTRA_LEN; + return stats_len; } @@ -2258,6 +2300,10 @@ static void i40e_get_ethtool_stats(struct net_device *netdev, struct i40e_vsi *vsi = np->vsi; struct i40e_pf *pf = vsi->back; struct i40e_veb *veb = NULL; + unsigned int vsi_idx; + unsigned int vf_idx; + unsigned int vf_id; + bool is_vf_valid; unsigned int i; bool veb_stats; u64 *p = data; @@ -2307,11 +2353,109 @@ static void i40e_get_ethtool_stats(struct net_device *netdev, i40e_add_ethtool_stats(&data, &pfc, i40e_gstrings_pfc_stats); } + /* As for now, we only process the SRIOV type VSIs (as extra stats to + * PF core stats) which are correlated with VF LAN VSI (hence below, + * in this for-loop instruction block, only VF's LAN VSIs are currently + * processed). + */ + for (vf_id = 0; vf_id < pf->num_alloc_vfs; vf_id++) { + is_vf_valid = true; + for (vf_idx = 0; vf_idx < pf->num_alloc_vfs; vf_idx++) + if (pf->vf[vf_idx].vf_id == vf_id) + break; + if (vf_idx >= pf->num_alloc_vfs) { + dev_info(&pf->pdev->dev, + "In the PF's array, there is no VF instance with VF_ID identifier %d or it is not set/initialized correctly yet\n", + vf_id); + is_vf_valid = false; + goto check_vf; + } + vsi_idx = pf->vf[vf_idx].lan_vsi_idx; + + vsi = pf->vsi[vsi_idx]; + if (!vsi) { + /* It means empty field in the PF VSI array... */ + dev_info(&pf->pdev->dev, + "No LAN VSI instance referenced by VF %d or it is not set/initialized correctly yet\n", + vf_id); + is_vf_valid = false; + goto check_vf; + } + if (vsi->vf_id != vf_id) { + dev_info(&pf->pdev->dev, + "In the PF's array, there is incorrectly set/initialized LAN VSI or reference to it from VF %d is not set/initialized correctly yet\n", + vf_id); + is_vf_valid = false; + goto check_vf; + } + if (vsi->vf_id != pf->vf[vf_idx].vf_id || + !i40e_find_vsi_from_id(pf, pf->vf[vsi->vf_id].lan_vsi_id)) { + /* Disjointed identifiers or broken references VF-VSI */ + dev_warn(&pf->pdev->dev, + "SRIOV LAN VSI (index %d in PF VSI array) with invalid VF Identifier %d (referenced by VF %d, ordered as %d in VF array)\n", + vsi_idx, pf->vsi[vsi_idx]->vf_id, + pf->vf[vf_idx].vf_id, vf_idx); + is_vf_valid = false; + } +check_vf: + if (!is_vf_valid) { + i40e_add_ethtool_stats(&data, NULL, + i40e_gstrings_eth_stats_extra); + } else { + i40e_update_eth_stats(vsi); + i40e_add_ethtool_stats(&data, vsi, + i40e_gstrings_eth_stats_extra); + } + } + for (; vf_id < I40E_STATS_EXTRA_COUNT; vf_id++) + i40e_add_ethtool_stats(&data, NULL, + i40e_gstrings_eth_stats_extra); + check_data_pointer: WARN_ONCE(data - p != i40e_get_stats_count(netdev), "ethtool stats count mismatch!"); } +/** + * __i40e_update_vfid_in_stats_strings - print VF num to stats names + * @stats_extra: array of stats structs with stats name strings + * @strings_num: number of stats name strings in array above (length) + * @vf_id: VF number to update stats name strings with + * + * Helper function to i40e_get_stat_strings() in case of extra stats. + **/ +static inline void +__i40e_update_vfid_in_stats_strings(struct i40e_stats stats_extra[], + int strings_num, int vf_id) +{ + int i; + + for (i = 0; i < strings_num; i++) { + snprintf(stats_extra[i].stat_string, + I40E_STATS_NAME_VFID_EXTRA_LEN, "vf%03d", vf_id); + stats_extra[i].stat_string[I40E_STATS_NAME_VFID_EXTRA_LEN - + 1] = '.'; + } +} + +/** + * i40e_update_vfid_in_stats - print VF num to stat names + * @stats_extra: array of stats structs with stats name strings + * @vf_id: VF number to update stats name strings with + * + * Helper macro to i40e_get_stat_strings() to ease use of + * __i40e_update_vfid_in_stats_strings() function due to extra stats. + * + * Macro to ease the use of __i40e_update_vfid_in_stats_strings by +taking + * a static constant stats array and passing the ARRAY_SIZE(). This +avoids typos + * by ensuring that we pass the size associated with the given stats array. + * + * The parameter @stats_extra is evaluated twice, so parameters with +side + * effects should be avoided. + **/ +#define i40e_update_vfid_in_stats(stats_extra, vf_id) \ +__i40e_update_vfid_in_stats_strings(stats_extra, +ARRAY_SIZE(stats_extra), vf_id) + /** * i40e_get_stat_strings - copy stat strings into supplied buffer * @netdev: the netdev to collect strings for @@ -2354,6 +2498,11 @@ static void i40e_get_stat_strings(struct net_device *netdev, u8 *data) for (i = 0; i < I40E_MAX_USER_PRIORITY; i++) i40e_add_stat_strings(&data, i40e_gstrings_pfc_stats, i); + for (i = 0; i < I40E_STATS_EXTRA_COUNT; i++) { + i40e_update_vfid_in_stats(i40e_gstrings_eth_stats_extra, i); + i40e_add_stat_strings(&data, i40e_gstrings_eth_stats_extra); + } + check_data_pointer: WARN_ONCE(data - p != i40e_get_stats_count(netdev) * ETH_GSTRING_LEN, "stat strings count mismatch!"); -- 2.21.0
On Mon, 28 Oct 2019 13:51:07 +0000, Grubba, Arkadiusz wrote: > Hi, Please don't top post. > 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.] It's not a matter of preference. I object to abuse of free-form APIs for things which have proper, modern interfaces. You're adding 12 * 128 = 1536 statistics to ethtool -S. That's going to make the output absolutely unreadable for a human being. > 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@netronome.com] > Sent: Thursday, October 24, 2019 5:42 AM > To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com> > Cc: davem@davemloft.net; Grubba, Arkadiusz <arkadiusz.grubba@intel.com>; netdev@vger.kernel.org; nhorman@redhat.com; sassmann@redhat.com; Bowers, AndrewX <andrewx.bowers@intel.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@intel.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@intel.com> > > Tested-by: Andrew Bowers <andrewx.bowers@intel.com> > > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.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.
From: Jakub Kicinski <jakub.kicinski@netronome.com> Date: Mon, 28 Oct 2019 10:30:47 -0700 > It's not a matter of preference. I object to abuse of free-form APIs > for things which have proper, modern interfaces. +1
On Thu, Oct 24, 2019 at 2:08 AM Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote: > > From: Arkadiusz Grubba <arkadiusz.grubba@intel.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() ). > > 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. > > Signed-off-by: Arkadiusz Grubba <arkadiusz.grubba@intel.com> > Tested-by: Andrew Bowers <andrewx.bowers@intel.com> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> So this patch is bad. It is overwriting the statistics strings for each VF separately. In addition the code isn't really easy to follow for the stats update as it seems like it is doing a bunch of extra work and generating far more noise then it needs to. > .../net/ethernet/intel/i40e/i40e_ethtool.c | 149 ++++++++++++++++++ > 1 file changed, 149 insertions(+) > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > index 41e1240acaea..c814c756b4bb 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > @@ -389,6 +389,7 @@ static const struct i40e_stats i40e_gstrings_pfc_stats[] = { > > #define I40E_GLOBAL_STATS_LEN ARRAY_SIZE(i40e_gstrings_stats) > > +/* Length (number) of PF core stats only (i.e. without queues / extra stats): */ > #define I40E_PF_STATS_LEN (I40E_GLOBAL_STATS_LEN + \ > I40E_PFC_STATS_LEN + \ > I40E_VEB_STATS_LEN + \ > @@ -397,6 +398,44 @@ static const struct i40e_stats i40e_gstrings_pfc_stats[] = { > /* Length of stats for a single queue */ > #define I40E_QUEUE_STATS_LEN ARRAY_SIZE(i40e_gstrings_queue_stats) > > +#define I40E_STATS_NAME_VFID_EXTRA "vf___." > +#define I40E_STATS_NAME_VFID_EXTRA_LEN (sizeof(I40E_STATS_NAME_VFID_EXTRA) - 1) > + Why bother with this? If you are just going to skip over it in __i40e_update_vfid_in_stats_strings() anyway why waste the memory on 5 characters per stat? It would simplify your code to just skip it here since you are inserting it later anyway. > +static struct i40e_stats i40e_gstrings_eth_stats_extra[] = { This should be const. > + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA > + "rx_bytes", eth_stats.rx_bytes), > + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA > + "rx_unicast", eth_stats.rx_unicast), > + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA > + "rx_multicast", eth_stats.rx_multicast), > + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA > + "rx_broadcast", eth_stats.rx_broadcast), > + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA > + "rx_discards", eth_stats.rx_discards), > + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA > + "rx_unknown_protocol", eth_stats.rx_unknown_protocol), > + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA > + "tx_bytes", eth_stats.tx_bytes), > + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA > + "tx_unicast", eth_stats.tx_unicast), > + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA > + "tx_multicast", eth_stats.tx_multicast), > + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA > + "tx_broadcast", eth_stats.tx_broadcast), > + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA > + "tx_discards", eth_stats.tx_discards), > + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA > + "tx_errors", eth_stats.tx_errors), > +}; > + > +#define I40E_STATS_EXTRA_COUNT 128 /* as for now only I40E_MAX_VF_COUNT */ > +/* Following length value does not include the length values for queues stats */ > +#define I40E_STATS_EXTRA_LEN ARRAY_SIZE(i40e_gstrings_eth_stats_extra) > +/* Length (number) of PF extra stats only (i.e. without core stats / queues): */ > +#define I40E_PF_STATS_EXTRA_LEN (I40E_STATS_EXTRA_COUNT * I40E_STATS_EXTRA_LEN) > +/* Length (number) of enhanced/all PF stats (i.e. core with extra stats): */ > +#define I40E_PF_STATS_ENHANCE_LEN (I40E_PF_STATS_LEN + I40E_PF_STATS_EXTRA_LEN) > + > enum i40e_ethtool_test_id { > I40E_ETH_TEST_REG = 0, > I40E_ETH_TEST_EEPROM, > @@ -2190,6 +2229,9 @@ static int i40e_get_stats_count(struct net_device *netdev) > */ > stats_len += I40E_QUEUE_STATS_LEN * 2 * netdev->num_tx_queues; > > + if (vsi == pf->vsi[pf->lan_vsi] && pf->hw.partition_id == 1) > + stats_len += I40E_PF_STATS_EXTRA_LEN; > + > return stats_len; > } > This bit is just wasteful. You already have this check up above in this function. Why not just add this to I40E_PF_STATS_LEN and be done with it? > @@ -2258,6 +2300,10 @@ static void i40e_get_ethtool_stats(struct net_device *netdev, > struct i40e_vsi *vsi = np->vsi; > struct i40e_pf *pf = vsi->back; > struct i40e_veb *veb = NULL; > + unsigned int vsi_idx; > + unsigned int vf_idx; > + unsigned int vf_id; > + bool is_vf_valid; > unsigned int i; > bool veb_stats; > u64 *p = data; > @@ -2307,11 +2353,109 @@ static void i40e_get_ethtool_stats(struct net_device *netdev, > i40e_add_ethtool_stats(&data, &pfc, i40e_gstrings_pfc_stats); > } > > + /* As for now, we only process the SRIOV type VSIs (as extra stats to > + * PF core stats) which are correlated with VF LAN VSI (hence below, > + * in this for-loop instruction block, only VF's LAN VSIs are currently > + * processed). > + */ > + for (vf_id = 0; vf_id < pf->num_alloc_vfs; vf_id++) { > + is_vf_valid = true; > + for (vf_idx = 0; vf_idx < pf->num_alloc_vfs; vf_idx++) > + if (pf->vf[vf_idx].vf_id == vf_id) > + break; > + if (vf_idx >= pf->num_alloc_vfs) { > + dev_info(&pf->pdev->dev, > + "In the PF's array, there is no VF instance with VF_ID identifier %d or it is not set/initialized correctly yet\n", > + vf_id); > + is_vf_valid = false; > + goto check_vf; > + } > + vsi_idx = pf->vf[vf_idx].lan_vsi_idx; > + Okay so this whole block here is just ugly.Why bother with trying to output this all in-order? We have the stats you need to output as a giant array, and you should know the base index of that array. So instead of making this way more complicated and expensive then it needs to be why not just determine the offset that you need to output the stats to based off of the vf_id? It would be much more readable then the approach you have taken. > + vsi = pf->vsi[vsi_idx]; > + if (!vsi) { > + /* It means empty field in the PF VSI array... */ > + dev_info(&pf->pdev->dev, > + "No LAN VSI instance referenced by VF %d or it is not set/initialized correctly yet\n", > + vf_id); > + is_vf_valid = false; > + goto check_vf; > + } This is getting noisy really quick. Do you really need to dump information if you cannot collect stats on a given VF? There are way too many messages in here in my opinion. > + if (vsi->vf_id != vf_id) { > + dev_info(&pf->pdev->dev, > + "In the PF's array, there is incorrectly set/initialized LAN VSI or reference to it from VF %d is not set/initialized correctly yet\n", > + vf_id); > + is_vf_valid = false; > + goto check_vf; > + } This is more noise. It concerns me that you need all these checks. Is this something you can actually encounter. If so then maybe these should be wrapped in some sort of reader/writer lock like what we have for the netdev queue statistics. > + if (vsi->vf_id != pf->vf[vf_idx].vf_id || > + !i40e_find_vsi_from_id(pf, pf->vf[vsi->vf_id].lan_vsi_id)) { > + /* Disjointed identifiers or broken references VF-VSI */ > + dev_warn(&pf->pdev->dev, > + "SRIOV LAN VSI (index %d in PF VSI array) with invalid VF Identifier %d (referenced by VF %d, ordered as %d in VF array)\n", > + vsi_idx, pf->vsi[vsi_idx]->vf_id, > + pf->vf[vf_idx].vf_id, vf_idx); > + is_vf_valid = false; > + } Here we finally get to any productive work. So as I mentioned before there is a much simpler way to deal with all of this. What you can do is zero out all of the stats, and then when you hit this part you just have to access "data + (vf_id * I40E_STATS_EXTRA_LEN)". > +check_vf: > + if (!is_vf_valid) { > + i40e_add_ethtool_stats(&data, NULL, > + i40e_gstrings_eth_stats_extra); > + } else { > + i40e_update_eth_stats(vsi); > + i40e_add_ethtool_stats(&data, vsi, > + i40e_gstrings_eth_stats_extra); > + } > + } > + for (; vf_id < I40E_STATS_EXTRA_COUNT; vf_id++) > + i40e_add_ethtool_stats(&data, NULL, > + i40e_gstrings_eth_stats_extra); > + > check_data_pointer: > WARN_ONCE(data - p != i40e_get_stats_count(netdev), > "ethtool stats count mismatch!"); > } > > +/** > + * __i40e_update_vfid_in_stats_strings - print VF num to stats names > + * @stats_extra: array of stats structs with stats name strings > + * @strings_num: number of stats name strings in array above (length) > + * @vf_id: VF number to update stats name strings with > + * > + * Helper function to i40e_get_stat_strings() in case of extra stats. > + **/ > +static inline void > +__i40e_update_vfid_in_stats_strings(struct i40e_stats stats_extra[], > + int strings_num, int vf_id) > +{ > + int i; > + > + for (i = 0; i < strings_num; i++) { > + snprintf(stats_extra[i].stat_string, > + I40E_STATS_NAME_VFID_EXTRA_LEN, "vf%03d", vf_id); > + stats_extra[i].stat_string[I40E_STATS_NAME_VFID_EXTRA_LEN - > + 1] = '.'; > + } > +} > + So this is just ugly on so many levels. Actually now that I have looked into it a bit more why couldn't you simply re-purpose the __i40e-add_stat_strings() code? You could pre-format your VF strings and then just have them all get inserted with the correct names and indexes later. > +/** > + * i40e_update_vfid_in_stats - print VF num to stat names > + * @stats_extra: array of stats structs with stats name strings > + * @vf_id: VF number to update stats name strings with > + * > + * Helper macro to i40e_get_stat_strings() to ease use of > + * __i40e_update_vfid_in_stats_strings() function due to extra stats. > + * > + * Macro to ease the use of __i40e_update_vfid_in_stats_strings by taking > + * a static constant stats array and passing the ARRAY_SIZE(). This avoids typos > + * by ensuring that we pass the size associated with the given stats array. > + * > + * The parameter @stats_extra is evaluated twice, so parameters with side > + * effects should be avoided. > + **/ > +#define i40e_update_vfid_in_stats(stats_extra, vf_id) \ > +__i40e_update_vfid_in_stats_strings(stats_extra, ARRAY_SIZE(stats_extra), vf_id) > + > /** > * i40e_get_stat_strings - copy stat strings into supplied buffer > * @netdev: the netdev to collect strings for > @@ -2354,6 +2498,11 @@ static void i40e_get_stat_strings(struct net_device *netdev, u8 *data) > for (i = 0; i < I40E_MAX_USER_PRIORITY; i++) > i40e_add_stat_strings(&data, i40e_gstrings_pfc_stats, i); > > + for (i = 0; i < I40E_STATS_EXTRA_COUNT; i++) { > + i40e_update_vfid_in_stats(i40e_gstrings_eth_stats_extra, i); > + i40e_add_stat_strings(&data, i40e_gstrings_eth_stats_extra); > + } > + Okay, now this is officially a hard NAK. I hadn't noticed this until now but you are overwriting the i40e_gstrings_eth_stats_extra? That should be a const value. My advice is that this should work like the Tx/Rx rings and PFC stats do. You cannot be rewriting the strings for every VF. It makes much more sense to simply use them as an input string and out put the formatted string into the destination.
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c index 41e1240acaea..c814c756b4bb 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c @@ -389,6 +389,7 @@ static const struct i40e_stats i40e_gstrings_pfc_stats[] = { #define I40E_GLOBAL_STATS_LEN ARRAY_SIZE(i40e_gstrings_stats) +/* Length (number) of PF core stats only (i.e. without queues / extra stats): */ #define I40E_PF_STATS_LEN (I40E_GLOBAL_STATS_LEN + \ I40E_PFC_STATS_LEN + \ I40E_VEB_STATS_LEN + \ @@ -397,6 +398,44 @@ static const struct i40e_stats i40e_gstrings_pfc_stats[] = { /* Length of stats for a single queue */ #define I40E_QUEUE_STATS_LEN ARRAY_SIZE(i40e_gstrings_queue_stats) +#define I40E_STATS_NAME_VFID_EXTRA "vf___." +#define I40E_STATS_NAME_VFID_EXTRA_LEN (sizeof(I40E_STATS_NAME_VFID_EXTRA) - 1) + +static struct i40e_stats i40e_gstrings_eth_stats_extra[] = { + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA + "rx_bytes", eth_stats.rx_bytes), + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA + "rx_unicast", eth_stats.rx_unicast), + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA + "rx_multicast", eth_stats.rx_multicast), + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA + "rx_broadcast", eth_stats.rx_broadcast), + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA + "rx_discards", eth_stats.rx_discards), + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA + "rx_unknown_protocol", eth_stats.rx_unknown_protocol), + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA + "tx_bytes", eth_stats.tx_bytes), + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA + "tx_unicast", eth_stats.tx_unicast), + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA + "tx_multicast", eth_stats.tx_multicast), + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA + "tx_broadcast", eth_stats.tx_broadcast), + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA + "tx_discards", eth_stats.tx_discards), + I40E_VSI_STAT(I40E_STATS_NAME_VFID_EXTRA + "tx_errors", eth_stats.tx_errors), +}; + +#define I40E_STATS_EXTRA_COUNT 128 /* as for now only I40E_MAX_VF_COUNT */ +/* Following length value does not include the length values for queues stats */ +#define I40E_STATS_EXTRA_LEN ARRAY_SIZE(i40e_gstrings_eth_stats_extra) +/* Length (number) of PF extra stats only (i.e. without core stats / queues): */ +#define I40E_PF_STATS_EXTRA_LEN (I40E_STATS_EXTRA_COUNT * I40E_STATS_EXTRA_LEN) +/* Length (number) of enhanced/all PF stats (i.e. core with extra stats): */ +#define I40E_PF_STATS_ENHANCE_LEN (I40E_PF_STATS_LEN + I40E_PF_STATS_EXTRA_LEN) + enum i40e_ethtool_test_id { I40E_ETH_TEST_REG = 0, I40E_ETH_TEST_EEPROM, @@ -2190,6 +2229,9 @@ static int i40e_get_stats_count(struct net_device *netdev) */ stats_len += I40E_QUEUE_STATS_LEN * 2 * netdev->num_tx_queues; + if (vsi == pf->vsi[pf->lan_vsi] && pf->hw.partition_id == 1) + stats_len += I40E_PF_STATS_EXTRA_LEN; + return stats_len; } @@ -2258,6 +2300,10 @@ static void i40e_get_ethtool_stats(struct net_device *netdev, struct i40e_vsi *vsi = np->vsi; struct i40e_pf *pf = vsi->back; struct i40e_veb *veb = NULL; + unsigned int vsi_idx; + unsigned int vf_idx; + unsigned int vf_id; + bool is_vf_valid; unsigned int i; bool veb_stats; u64 *p = data; @@ -2307,11 +2353,109 @@ static void i40e_get_ethtool_stats(struct net_device *netdev, i40e_add_ethtool_stats(&data, &pfc, i40e_gstrings_pfc_stats); } + /* As for now, we only process the SRIOV type VSIs (as extra stats to + * PF core stats) which are correlated with VF LAN VSI (hence below, + * in this for-loop instruction block, only VF's LAN VSIs are currently + * processed). + */ + for (vf_id = 0; vf_id < pf->num_alloc_vfs; vf_id++) { + is_vf_valid = true; + for (vf_idx = 0; vf_idx < pf->num_alloc_vfs; vf_idx++) + if (pf->vf[vf_idx].vf_id == vf_id) + break; + if (vf_idx >= pf->num_alloc_vfs) { + dev_info(&pf->pdev->dev, + "In the PF's array, there is no VF instance with VF_ID identifier %d or it is not set/initialized correctly yet\n", + vf_id); + is_vf_valid = false; + goto check_vf; + } + vsi_idx = pf->vf[vf_idx].lan_vsi_idx; + + vsi = pf->vsi[vsi_idx]; + if (!vsi) { + /* It means empty field in the PF VSI array... */ + dev_info(&pf->pdev->dev, + "No LAN VSI instance referenced by VF %d or it is not set/initialized correctly yet\n", + vf_id); + is_vf_valid = false; + goto check_vf; + } + if (vsi->vf_id != vf_id) { + dev_info(&pf->pdev->dev, + "In the PF's array, there is incorrectly set/initialized LAN VSI or reference to it from VF %d is not set/initialized correctly yet\n", + vf_id); + is_vf_valid = false; + goto check_vf; + } + if (vsi->vf_id != pf->vf[vf_idx].vf_id || + !i40e_find_vsi_from_id(pf, pf->vf[vsi->vf_id].lan_vsi_id)) { + /* Disjointed identifiers or broken references VF-VSI */ + dev_warn(&pf->pdev->dev, + "SRIOV LAN VSI (index %d in PF VSI array) with invalid VF Identifier %d (referenced by VF %d, ordered as %d in VF array)\n", + vsi_idx, pf->vsi[vsi_idx]->vf_id, + pf->vf[vf_idx].vf_id, vf_idx); + is_vf_valid = false; + } +check_vf: + if (!is_vf_valid) { + i40e_add_ethtool_stats(&data, NULL, + i40e_gstrings_eth_stats_extra); + } else { + i40e_update_eth_stats(vsi); + i40e_add_ethtool_stats(&data, vsi, + i40e_gstrings_eth_stats_extra); + } + } + for (; vf_id < I40E_STATS_EXTRA_COUNT; vf_id++) + i40e_add_ethtool_stats(&data, NULL, + i40e_gstrings_eth_stats_extra); + check_data_pointer: WARN_ONCE(data - p != i40e_get_stats_count(netdev), "ethtool stats count mismatch!"); } +/** + * __i40e_update_vfid_in_stats_strings - print VF num to stats names + * @stats_extra: array of stats structs with stats name strings + * @strings_num: number of stats name strings in array above (length) + * @vf_id: VF number to update stats name strings with + * + * Helper function to i40e_get_stat_strings() in case of extra stats. + **/ +static inline void +__i40e_update_vfid_in_stats_strings(struct i40e_stats stats_extra[], + int strings_num, int vf_id) +{ + int i; + + for (i = 0; i < strings_num; i++) { + snprintf(stats_extra[i].stat_string, + I40E_STATS_NAME_VFID_EXTRA_LEN, "vf%03d", vf_id); + stats_extra[i].stat_string[I40E_STATS_NAME_VFID_EXTRA_LEN - + 1] = '.'; + } +} + +/** + * i40e_update_vfid_in_stats - print VF num to stat names + * @stats_extra: array of stats structs with stats name strings + * @vf_id: VF number to update stats name strings with + * + * Helper macro to i40e_get_stat_strings() to ease use of + * __i40e_update_vfid_in_stats_strings() function due to extra stats. + * + * Macro to ease the use of __i40e_update_vfid_in_stats_strings by taking + * a static constant stats array and passing the ARRAY_SIZE(). This avoids typos + * by ensuring that we pass the size associated with the given stats array. + * + * The parameter @stats_extra is evaluated twice, so parameters with side + * effects should be avoided. + **/ +#define i40e_update_vfid_in_stats(stats_extra, vf_id) \ +__i40e_update_vfid_in_stats_strings(stats_extra, ARRAY_SIZE(stats_extra), vf_id) + /** * i40e_get_stat_strings - copy stat strings into supplied buffer * @netdev: the netdev to collect strings for @@ -2354,6 +2498,11 @@ static void i40e_get_stat_strings(struct net_device *netdev, u8 *data) for (i = 0; i < I40E_MAX_USER_PRIORITY; i++) i40e_add_stat_strings(&data, i40e_gstrings_pfc_stats, i); + for (i = 0; i < I40E_STATS_EXTRA_COUNT; i++) { + i40e_update_vfid_in_stats(i40e_gstrings_eth_stats_extra, i); + i40e_add_stat_strings(&data, i40e_gstrings_eth_stats_extra); + } + check_data_pointer: WARN_ONCE(data - p != i40e_get_stats_count(netdev) * ETH_GSTRING_LEN, "stat strings count mismatch!");