[dpdk-dev,v3,4/7] ethdev: remove HW specific stats in stats structs

Message ID 1A27633A6DA49C4A92FCD5D4312DBF536A45182E@IRSMSX109.ger.corp.intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Tahhan, Maryam June 26, 2015, 2:30 p.m. UTC
  On Fri, Jun 26, 2015 at 8:59 AM, Maryam Tahhan <maryam.tahhan@intel.com<mailto:maryam.tahhan@intel.com>> wrote:
Remove non generic stats in rte_stats_strings and mark the relevant
fields in struct rte_eth_stats as deprecated.

Signed-off-by: Maryam Tahhan <maryam.tahhan@intel.com<mailto:maryam.tahhan@intel.com>>

---
 doc/guides/rel_notes/abi.rst  | 11 +++++++++++
 lib/librte_ether/rte_ethdev.c |  9 ---------
 lib/librte_ether/rte_ethdev.h | 30 ++++++++++++++++++++----------
 3 files changed, 31 insertions(+), 19 deletions(-)


Are CRC errors (ibadcrc) truly hardware specific? Which NIC (aside from purely virtual ones) does not have a MAC which does frame checksumming? Likewise, which NIC doesn't drop because the PCI bus/cpu/etc is too busy to pull packets off of it (imissed)?
Debugging interactions with NICs is hard enough with only CRC errors and missed packets to go on. Without those it is close to impossible. CRC errors are almost guaranteed any time a bare-metal application is deployed: dirty fiber, bad SFPs, etc. How will users of the application be able to determine why their packets are dropping if they can only see "in errors"?
I understand that we want to avoid placing too much useless information into these statistics structures. However, without a hardware-independent way of accessing fairly standard networking-equipment diagnostics, I feel like any real-world application using DPDK will be terribly cumbersome to build: every single one will need to develop an abstraction layer which detects the attached NICs, and loads an appropriate driver to integrate with the xstats api.

Is there any plan for such an API? If not, is it really a good idea to deprecate these stats?
Thanks,
Kyle

Hi Kyle

If it’s just for debug/diagnostic purposes that this information is being used then I would recommend using the proc_info app which is already integrated with xstats will give you detailed error statistics. It runs as a DPDK secondary process.

I’m not sure about crcerrors and imissed, I had taken feedback to my previous version of the patches onboard and made the changes based on that.

Regards
Maryam
  

Comments

Kyle Larose June 26, 2015, 2:37 p.m. UTC | #1
Hi Maryam,

I was not aware of the proc_info app. Is there any documentation on dpdk.org
about it, or should I browse the code?

Thanks,

Kyle

On Fri, Jun 26, 2015 at 10:30 AM, Tahhan, Maryam <maryam.tahhan@intel.com>
wrote:

>
>
> On Fri, Jun 26, 2015 at 8:59 AM, Maryam Tahhan <maryam.tahhan@intel.com>
> wrote:
>
> Remove non generic stats in rte_stats_strings and mark the relevant
> fields in struct rte_eth_stats as deprecated.
>
> Signed-off-by: Maryam Tahhan <maryam.tahhan@intel.com>
> ---
>  doc/guides/rel_notes/abi.rst  | 11 +++++++++++
>  lib/librte_ether/rte_ethdev.c |  9 ---------
>  lib/librte_ether/rte_ethdev.h | 30 ++++++++++++++++++++----------
>  3 files changed, 31 insertions(+), 19 deletions(-)
>
> diff --git a/doc/guides/rel_notes/abi.rst b/doc/guides/rel_notes/abi.rst
> index f00a6ee..957b13f 100644
> --- a/doc/guides/rel_notes/abi.rst
> +++ b/doc/guides/rel_notes/abi.rst
> @@ -38,3 +38,14 @@ Examples of Deprecation Notices
>
>  Deprecation Notices
>  -------------------
> +* The following fields have been deprecated in rte_eth_stats:
> +  * uint64_t imissed
> +  * uint64_t ibadcrc
> +  * uint64_t ibadlen
> +  * uint64_t imcasts
> +  * uint64_t fdirmatch
> +  * uint64_t fdirmiss
> +  * uint64_t tx_pause_xon
> +  * uint64_t rx_pause_xon
> +  * uint64_t tx_pause_xoff
> +  * uint64_t rx_pause_xoff
>
>
>
> Are CRC errors (ibadcrc) truly hardware specific? Which NIC (aside from
> purely virtual ones) does not have a MAC which does frame checksumming?
> Likewise, which NIC doesn't drop because the PCI bus/cpu/etc is too busy to
> pull packets off of it (imissed)?
>
> Debugging interactions with NICs is hard enough with only CRC errors and
> missed packets to go on. Without those it is close to impossible. CRC
> errors are almost guaranteed any time a bare-metal application is deployed:
> dirty fiber, bad SFPs, etc. How will users of the application be able to
> determine why their packets are dropping if they can only see "in errors"?
>
> I understand that we want to avoid placing too much useless information
> into these statistics structures. However, without a hardware-independent
> way of accessing fairly standard networking-equipment diagnostics, I feel
> like any real-world application using DPDK will be terribly cumbersome to
> build: every single one will need to develop an abstraction layer which
> detects the attached NICs, and loads an appropriate driver to integrate
> with the xstats api.
>
>
>
> Is there any plan for such an API? If not, is it really a good idea to
> deprecate these stats?
>
> Thanks,
>
> Kyle
>
>
>
> Hi Kyle
>
>
>
> If it’s just for debug/diagnostic purposes that this information is being
> used then I would recommend using the proc_info app which is already
> integrated with xstats will give you detailed error statistics. It runs as
> a DPDK secondary process.
>
>
>
> I’m not sure about crcerrors and imissed, I had taken feedback to my
> previous version of the patches onboard and made the changes based on that.
>
>
>
> Regards
>
> Maryam
>
  
Tahhan, Maryam June 26, 2015, 2:47 p.m. UTC | #2
> Hi Maryam,

> I was not aware of the proc_info app. Is there any documentation on dpdk.org about it, or should I browse the code?

> Thanks,

> Kyle


Hi Kyle,
It's the last patch in the patchset I posted. I haven't written up the documentation yet, but will do so, for now feel free to peruse the code. If you have any questions let me know.

Regards
Maryam
  

Patch

diff --git a/doc/guides/rel_notes/abi.rst b/doc/guides/rel_notes/abi.rst
index f00a6ee..957b13f 100644
--- a/doc/guides/rel_notes/abi.rst
+++ b/doc/guides/rel_notes/abi.rst
@@ -38,3 +38,14 @@  Examples of Deprecation Notices

 Deprecation Notices
 -------------------
+* The following fields have been deprecated in rte_eth_stats:
+  * uint64_t imissed
+  * uint64_t ibadcrc
+  * uint64_t ibadlen
+  * uint64_t imcasts
+  * uint64_t fdirmatch
+  * uint64_t fdirmiss
+  * uint64_t tx_pause_xon
+  * uint64_t rx_pause_xon
+  * uint64_t tx_pause_xoff
+  * uint64_t rx_pause_xoff