[dpdk-dev] [PATCH] doc: announce xstats api change for 16.07

[dpdk-dev] [PATCH] doc: announce xstats api change for 16.07

[Harry van Haaren]

This patch adds a notice that the API for the xstats
functionality will be modified in the 16.07 release, with
no backwards compatibility planned as it would require
code duplication in each PMD that supports xstats.

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 doc/guides/rel_notes/deprecation.rst | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 98d5529..13c3a95 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -54,3 +54,8 @@ Deprecation Notices
   induce a modification of the rte_mempool structure, plus a
   modification of the API of rte_mempool_obj_iter(), implying a breakage
   of the ABI.
+
+* ABI change is planned for the xstats API and rte_eth_xstats struct, to
+  facilitate updating to an API that allows retrieval of values without any
+  string copies or parsing. No backwards compatibility is planned, as it would
+  require code duplication in every PMD that supports xstats.
-- 
2.5.0

Re: [dpdk-dev] [PATCH] doc: announce xstats api change for 16.07

[Thomas Monjalon]

2016-04-05 18:58, Harry van Haaren:
> +* ABI change is planned for the xstats API and rte_eth_xstats struct, to
> +  facilitate updating to an API that allows retrieval of values without any
> +  string copies or parsing. No backwards compatibility is planned, as it would
> +  require code duplication in every PMD that supports xstats.

Have you already submitted a RFC patch to let us have an opinion on the change?
We need, at least, to see the structure changes.
Thanks

Re: [dpdk-dev] [PATCH] doc: announce xstats api change for 16.07

[Van Haaren, Harry]

+ David Harton,

> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Subject: Re: [dpdk-dev] [PATCH] doc: announce xstats api change for 16.07
> 2016-04-05 18:58, Harry van Haaren:
> > +* ABI change is planned for the xstats API

> Have you already submitted a RFC patch to let us have an opinion on the change?
> We need, at least, to see the structure changes.

This API break is to allow changing the API of xstats given the conversation that was on-list, as discussed in this thread:

http://thread.gmane.org/gmane.comp.networking.dpdk.devel/31728/focus=31903

The issue we are going to fix is that currently PMDs copy strings when retrieving statistics, which causes unnecessary overhead. The implementation is not decided yet, but using an int->value mapping seems logical.

The rte_eth_xstats struct size may be modified, and the API to retrieve statistics will be modified.

Re: [dpdk-dev] [PATCH] doc: announce xstats api change for 16.07

[Thomas Monjalon]

2016-04-06 09:02, Van Haaren, Harry:
> + David Harton,
> 
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Subject: Re: [dpdk-dev] [PATCH] doc: announce xstats api change for 16.07
> > 2016-04-05 18:58, Harry van Haaren:
> > > +* ABI change is planned for the xstats API
> 
> > Have you already submitted a RFC patch to let us have an opinion on the change?
> > We need, at least, to see the structure changes.
> 
> This API break is to allow changing the API of xstats given the conversation that was on-list, as discussed in this thread:
> 
> http://thread.gmane.org/gmane.comp.networking.dpdk.devel/31728/focus=31903
> 
> The issue we are going to fix is that currently PMDs copy strings when retrieving statistics, which causes unnecessary overhead. The implementation is not decided yet, but using an int->value mapping seems logical.

I am not sure performance is so much critical when retrieving statistics.
The extended stats can be infinitely extended. So a string identifier seems
a lot more natural.
I do not agree to add a new numeric identifier in the API each time a driver
wants to report a specific statistic for debugging purpose.

> The rte_eth_xstats struct size may be modified, and the API to retrieve statistics will be modified.

Re: [dpdk-dev] [PATCH] doc: announce xstats api change for 16.07

[Van Haaren, Harry]

> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Subject: Re: [dpdk-dev] [PATCH] doc: announce xstats api change for 16.07
> > The issue we are going to fix is that currently PMDs copy strings when retrieving
> statistics, which causes unnecessary overhead. The implementation is not decided yet, but
> using an int->value mapping seems logical.

> I am not sure performance is so much critical when retrieving statistics.

In the previous discussion David was concerned about performance impact
of string copies, are those concerns still present David?

> The extended stats can be infinitely extended. So a string identifier seems
> a lot more natural.

I'm not suggesting that the string identifier is removed totally.

> I do not agree to add a new numeric identifier in the API each time a driver
> wants to report a specific statistic for debugging purpose. 

And I agree - the ints are just an index to xstats arrays, no eth-dev wide enums here.
The proposal is to make the API more flexible, see example:
http://thread.gmane.org/gmane.comp.networking.dpdk.devel/31728/focus=32795

This more flexible API would allow other types of information about
statistics be retrieved too.

For now, the sent patch announces that the API/ABI may change, and we can
discuss details of API as development starts.

Re: [dpdk-dev] [PATCH] doc: announce xstats api change for 16.07

[Thomas Monjalon]

2016-04-06 11:16, Van Haaren, Harry:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > The issue we are going to fix is that currently PMDs copy strings when retrieving
> > statistics, which causes unnecessary overhead. The implementation is not decided yet, but
> > using an int->value mapping seems logical.
> 
> > I am not sure performance is so much critical when retrieving statistics.
> 
> In the previous discussion David was concerned about performance impact
> of string copies, are those concerns still present David?
> 
> > The extended stats can be infinitely extended. So a string identifier seems
> > a lot more natural.
> 
> I'm not suggesting that the string identifier is removed totally.
> 
> > I do not agree to add a new numeric identifier in the API each time a driver
> > wants to report a specific statistic for debugging purpose. 
> 
> And I agree - the ints are just an index to xstats arrays, no eth-dev wide enums here.
> The proposal is to make the API more flexible, see example:
> http://thread.gmane.org/gmane.comp.networking.dpdk.devel/31728/focus=32795
> 
> This more flexible API would allow other types of information about
> statistics be retrieved too.

OK I think I start to understand.

> For now, the sent patch announces that the API/ABI may change, and we can
> discuss details of API as development starts.

This should not be the normal process.
It is important to understand what should be the changes to decide of
announcing or not a deprecation.
In the case of the mempool reworks, the patch have been sent and discussed
on the mailing list.
Given the previous explanations (and knowing you did good job on stats),
I give my
Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com>

Re: [dpdk-dev] [PATCH] doc: announce xstats api change for 16.07

[David Harton (dharton)]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, April 06, 2016 8:14 AM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: David Harton (dharton) <dharton@cisco.com>; dev@dpdk.org; Tahhan,
> Maryam <maryam.tahhan@intel.com>; olivier.matz@6wind.com
> Subject: Re: [dpdk-dev] [PATCH] doc: announce xstats api change for 16.07
> 
> 2016-04-06 11:16, Van Haaren, Harry:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > The issue we are going to fix is that currently PMDs copy strings
> > > > when retrieving
> > > statistics, which causes unnecessary overhead. The implementation is
> > > not decided yet, but using an int->value mapping seems logical.
> >
> > > I am not sure performance is so much critical when retrieving
> statistics.
> >
> > In the previous discussion David was concerned about performance
> > impact of string copies, are those concerns still present David?
> >
> > > The extended stats can be infinitely extended. So a string
> > > identifier seems a lot more natural.
> >
> > I'm not suggesting that the string identifier is removed totally.
> >
> > > I do not agree to add a new numeric identifier in the API each time
> > > a driver wants to report a specific statistic for debugging purpose.
> >
> > And I agree - the ints are just an index to xstats arrays, no eth-dev
> wide enums here.

Yes, I abandoned the idea of a set of stats ids.  I can see where registration will be problematic and cumbersome to driver developers.

> > The proposal is to make the API more flexible, see example:
> > http://thread.gmane.org/gmane.comp.networking.dpdk.devel/31728/focus=3
> > 2795
> >
> > This more flexible API would allow other types of information about
> > statistics be retrieved too.

I have prototyped this.  If there is interest/acceptance I can work on making an official patch to share back to the community.

Using this method still gives the flexibility the current API desires while giving the user the control to only obtain the counters.  This of course assumes that the counters per device are static but that seems a safe bet.

> 
> OK I think I start to understand.
> 
> > For now, the sent patch announces that the API/ABI may change, and we
> > can discuss details of API as development starts.
> 
> This should not be the normal process.
> It is important to understand what should be the changes to decide of
> announcing or not a deprecation.
> In the case of the mempool reworks, the patch have been sent and discussed
> on the mailing list.
> Given the previous explanations (and knowing you did good job on stats), I
> give my
> Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com>

Thanks for considering this.

Regards,
Dave

Re: [dpdk-dev] [PATCH] doc: announce xstats api change for 16.07

[Van Haaren, Harry]

> From: David Harton (dharton) [mailto:dharton@cisco.com]
> Subject: RE: [dpdk-dev] [PATCH] doc: announce xstats api change for 16.07
> > This should not be the normal process.
> > It is important to understand what should be the changes to decide of
> > announcing or not a deprecation.
> > In the case of the mempool reworks, the patch have been sent and discussed
> > on the mailing list.
> > Given the previous explanations (and knowing you did good job on stats), I
> > give my
> > Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> 
> Thanks for considering this.

Would you mind giving the patch an Ack David?

Then we'll have the 3 Acks needed and can fix this in the next release.
Cheers, -Harry

Re: [dpdk-dev] [PATCH] doc: announce xstats api change for 16.07

[Remy Horton]

On 05/04/2016 18:58, Harry van Haaren wrote:
> This patch adds a notice that the API for the xstats
> functionality will be modified in the 16.07 release, with
> no backwards compatibility planned as it would require
> code duplication in each PMD that supports xstats.
>
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>

Acked-by: Remy Horton <remy.horton@intel.com>

Re: [dpdk-dev] [PATCH] doc: announce xstats api change for 16.07

[David Harton (dharton)]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Harry van Haaren
> Sent: Tuesday, April 05, 2016 1:58 PM
> To: dev@dpdk.org
> Cc: maryam.tahhan@intel.com; Harry van Haaren <harry.van.haaren@intel.com>
> Subject: [dpdk-dev] [PATCH] doc: announce xstats api change for 16.07
> 
> This patch adds a notice that the API for the xstats functionality will be
> modified in the 16.07 release, with no backwards compatibility planned as
> it would require code duplication in each PMD that supports xstats.
> 
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> ---
>  doc/guides/rel_notes/deprecation.rst | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst
> b/doc/guides/rel_notes/deprecation.rst
> index 98d5529..13c3a95 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -54,3 +54,8 @@ Deprecation Notices
>    induce a modification of the rte_mempool structure, plus a
>    modification of the API of rte_mempool_obj_iter(), implying a breakage
>    of the ABI.
> +
> +* ABI change is planned for the xstats API and rte_eth_xstats struct,
> +to
> +  facilitate updating to an API that allows retrieval of values without
> +any
> +  string copies or parsing. No backwards compatibility is planned, as
> +it would
> +  require code duplication in every PMD that supports xstats.
> --
> 2.5.0

Acked-by: David Harton <dharton@cisco.com>

Re: [dpdk-dev] [PATCH] doc: announce xstats api change for 16.07

[Tahhan, Maryam]

> From: Van Haaren, Harry
> Sent: Tuesday, April 5, 2016 6:58 PM
> To: dev@dpdk.org
> Cc: Tahhan, Maryam <maryam.tahhan@intel.com>; Van Haaren, Harry
> <harry.van.haaren@intel.com>
> Subject: [PATCH] doc: announce xstats api change for 16.07
> 
> This patch adds a notice that the API for the xstats functionality will be
> modified in the 16.07 release, with no backwards compatibility planned
> as it would require code duplication in each PMD that supports xstats.
> 
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> ---

Acked-by: Maryam Tahhan <maryam.tahhan@intel.com>

Re: [dpdk-dev] [PATCH] doc: announce xstats api change for 16.07

[Thomas Monjalon]

> > This patch adds a notice that the API for the xstats functionality will be
> > modified in the 16.07 release, with no backwards compatibility planned
> > as it would require code duplication in each PMD that supports xstats.
> > 
> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> 
> Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> Acked-by: Remy Horton <remy.horton@intel.com>
> Acked-by: David Harton <dharton@cisco.com>
> Acked-by: Maryam Tahhan <maryam.tahhan@intel.com>

Applied, thanks