[pull-request] next-tm 17.08 pre-rc1

[pull-request] next-tm 17.08 pre-rc1

[Cristian Dumitrescu]

The following changes since commit a566400e8b73ec646e0cc6dd0bc44def8535fb98:

  net: implement CRC for ARM64 NEON (2017-07-04 15:58:45 +0200)

are available in the git repository at:

  http://dpdk.org/git/next/dpdk-next-tm 

for you to fetch changes up to dcb4c3f881e29e1062a3e298e60474912e8a4ece:

  net/ixgbe: support committing TM hierarchy (2017-07-04 16:00:01 +0100)

----------------------------------------------------------------
Dumitrescu, Cristian (2):
      ethdev: add traffic management ops get API
      ethdev: add traffic management API

Wenzhuo Lu (20):
      net/i40e: support getting TM ops
      net/i40e: support getting TM capability
      net/i40e: support adding TM shaper profile
      net/i40e: support deleting TM shaper profile
      net/i40e: support adding TM node
      net/i40e: support deleting TM node
      net/i40e: support getting TM node type
      net/i40e: support getting TM level capability
      net/i40e: support getting TM node capability
      net/i40e: support committing TM hierarchy
      net/ixgbe: support getting TM ops
      net/ixgbe: support getting TM capability
      net/ixgbe: support adding TM shaper profile
      net/ixgbe: support deleting TM shaper profile
      net/ixgbe: support adding TM node
      net/ixgbe: support deleting TM node
      net/ixgbe: support getting TM node type
      net/ixgbe: support getting TM level capability
      net/ixgbe: support getting TM node capability
      net/ixgbe: support committing TM hierarchy

 MAINTAINERS                            |    5 +
 doc/api/doxy-api-index.md              |    2 +
 drivers/net/i40e/Makefile              |    1 +
 drivers/net/i40e/i40e_ethdev.c         |   15 +
 drivers/net/i40e/i40e_ethdev.h         |   80 ++
 drivers/net/i40e/i40e_tm.c             |  974 ++++++++++++++++
 drivers/net/i40e/rte_pmd_i40e.c        |    9 -
 drivers/net/ixgbe/Makefile             |    1 +
 drivers/net/ixgbe/ixgbe_ethdev.c       |   27 +-
 drivers/net/ixgbe/ixgbe_ethdev.h       |   72 ++
 drivers/net/ixgbe/ixgbe_tm.c           | 1041 +++++++++++++++++
 lib/librte_ether/Makefile              |    5 +-
 lib/librte_ether/rte_ethdev.c          |   12 +
 lib/librte_ether/rte_ethdev.h          |   20 +
 lib/librte_ether/rte_ether_version.map |   31 +
 lib/librte_ether/rte_tm.c              |  438 ++++++++
 lib/librte_ether/rte_tm.h              | 1904 ++++++++++++++++++++++++++++++++
 lib/librte_ether/rte_tm_driver.h       |  366 ++++++
 18 files changed, 4988 insertions(+), 15 deletions(-)
 create mode 100644 drivers/net/i40e/i40e_tm.c
 create mode 100644 drivers/net/ixgbe/ixgbe_tm.c
 create mode 100644 lib/librte_ether/rte_tm.c
 create mode 100644 lib/librte_ether/rte_tm.h
 create mode 100644 lib/librte_ether/rte_tm_driver.h

Re: [pull-request] next-tm 17.08 pre-rc1

[Thomas Monjalon]

Hi Cristian,

> Dumitrescu, Cristian (2):
>       ethdev: add traffic management ops get API
>       ethdev: add traffic management API

The original request was to split this huge patch.
It is too messy to bring a whole new API area in one patch.
We have nothing to refer in case of bug, and it is hard to dive in.

Please, could you try to split it, bringing features one by one?

Re: [pull-request] next-tm 17.08 pre-rc1

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Tuesday, July 4, 2017 4:47 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; jerin.jacob@caviumnetworks.com;
> hemant.agrawal@nxp.com; Singh, Jasvinder <jasvinder.singh@intel.com>;
> Lu, Wenzhuo <wenzhuo.lu@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>
> Subject: Re: [dpdk-dev] [pull-request] next-tm 17.08 pre-rc1
> 
> Hi Cristian,
> 
> > Dumitrescu, Cristian (2):
> >       ethdev: add traffic management ops get API
> >       ethdev: add traffic management API
> 
> The original request was to split this huge patch.
> It is too messy to bring a whole new API area in one patch.
> We have nothing to refer in case of bug, and it is hard to dive in.
> 
> Please, could you try to split it, bringing features one by one?

Hi Thomas,

Technically, it can be done, but IMO it should not be done this way for the following reasons:

1. None of the new APIs recently introduced in DPDK follow this approach. The rte_flow [1] and the eventdev [2] API are of the same order of magnitude with the TM API, and both were introduced as a single patch header file. Why do things differently for TM API?
	
2. Breaking an API header file into multiple patches usually does not make sense because the sub-components are inter-connected and cross-referenced. When evaluating an API, it needs to be evaluated as a whole for consistency reasons rather than piece by piece. On TM API for example, the capability API is inter-connected with congestion management, shaping, scheduling and marking features; cman and shaping are connected to the nodes that make up the scheduling tree, etc. IMO the end result is adding more confusion than clarity.

This request also comes very late in our preparation to hit RC1. I know you made this mention previously, but I regarded it as a comment/suggestion rather than a hard requirement (sorry for not explaining it my rationale better at the time). You also had several other comments and requests that we fulfilled, as described in the revision history.

So, what do you want me to do? If you still want to go ahead with this request, I will do my best to do it and still meet RC1.

Regards,
Cristian

[1] eventdev API: http://www.dpdk.org/ml/archives/dev/2016-November/050356.html
[2] rte_flow API: http://www.dpdk.org/ml/archives/dev/2016-December/052951.html

Re: [pull-request] next-tm 17.08 pre-rc1

[Thomas Monjalon]

04/07/2017 18:52, Dumitrescu, Cristian:
> > Hi Cristian,
> > 
> > > Dumitrescu, Cristian (2):
> > >       ethdev: add traffic management ops get API
> > >       ethdev: add traffic management API
> > 
> > The original request was to split this huge patch.
> > It is too messy to bring a whole new API area in one patch.
> > We have nothing to refer in case of bug, and it is hard to dive in.
> > 
> > Please, could you try to split it, bringing features one by one?
> 
> Hi Thomas,
> 
> Technically, it can be done, but IMO it should not be done this way for the following reasons:
> 
> 1. None of the new APIs recently introduced in DPDK follow this approach. The rte_flow [1] and the eventdev [2] API are of the same order of magnitude with the TM API, and both were introduced as a single patch header file. Why do things differently for TM API?

Yes you're right, same magnitude (but 2 times bigger).
I would have preffered eventdev and rte_flow be better introduced.

> 2. Breaking an API header file into multiple patches usually does not make sense because the sub-components are inter-connected and cross-referenced. When evaluating an API, it needs to be evaluated as a whole for consistency reasons rather than piece by piece. On TM API for example, the capability API is inter-connected with congestion management, shaping, scheduling and marking features; cman and shaping are connected to the nodes that make up the scheduling tree, etc. IMO the end result is adding more confusion than clarity.

For me it's simpler to start with basic stuff and add more features.
But it may be just a taste.

> This request also comes very late in our preparation to hit RC1. I know you made this mention previously, but I regarded it as a comment/suggestion rather than a hard requirement (sorry for not explaining it my rationale better at the time). You also had several other comments and requests that we fulfilled, as described in the revision history.

I had not seen any news about this patchset and the tree was empty
during a long time so I thought you were working on it.

> So, what do you want me to do?
> If you still want to go ahead with this request, I will do my best to do it and still meet RC1.

No, I do not want to insist.
I understand you have a different taste than mine :)

I will check for pulling your tree in following days.
Please try to be available on IRC, in case I catch a last minute detail to fix.
Thanks

Re: [pull-request] next-tm 17.08 pre-rc1

[Dumitrescu, Cristian]

> I will check for pulling your tree in following days.
> Please try to be available on IRC, in case I catch a last minute detail to fix.
> Thanks

Thank you, Thomas. No issues, I will present on IRC and reply promptly on any emails.

Regards,
Cristian

Re: [pull-request] next-tm 17.08 pre-rc1

[Thomas Monjalon]

Hi,

04/07/2017 17:38, Cristian Dumitrescu:
>   http://dpdk.org/git/next/dpdk-next-tm 

I'm sorry to not have considered this tree as a high priority.
I think it may be integrated in RC2 because it is a totally new area
and should not break any existing code.

I prefer to wait because I have seen some things to fix:

1/ There is a compilation error with clang (notified in related thread).

2/ Some functions are exposed in the API to query the ops.
It seems dangerous and useless:
	- rte_eth_dev_tm_ops_get
	- rte_tm_ops_get

3/ The PMD interface file is referenced in the doxygen index:
+  [rte_tm_driver]      (@ref rte_tm_driver.h),
I see that rte_flow_driver.h is also referenced but it seems a mistake.

4/ As it is a totally new API, it should be declared as EXPERIMENTAL
in the MAINTAINERS file and in the doxygen.

5/ There is no doc in the programmer's guide.

6/ There is no application to test it.

I know that the points 5/ and 6/ are long to complete.
However I would like to know what is the plan?
And should we integrate TM in 17.08 without neither doc nor app?

Re: [pull-request] next-tm 17.08 pre-rc1

[Adrien Mazarguil]

Hi Thomas,

On Sun, Jul 09, 2017 at 10:01:31PM +0200, Thomas Monjalon wrote:
> Hi,
> 
> 04/07/2017 17:38, Cristian Dumitrescu:
> >   http://dpdk.org/git/next/dpdk-next-tm 
> 
> I'm sorry to not have considered this tree as a high priority.
> I think it may be integrated in RC2 because it is a totally new area
> and should not break any existing code.
> 
> I prefer to wait because I have seen some things to fix:
> 
> 1/ There is a compilation error with clang (notified in related thread).
> 
> 2/ Some functions are exposed in the API to query the ops.
> It seems dangerous and useless:
> 	- rte_eth_dev_tm_ops_get
> 	- rte_tm_ops_get
> 
> 3/ The PMD interface file is referenced in the doxygen index:
> +  [rte_tm_driver]      (@ref rte_tm_driver.h),
> I see that rte_flow_driver.h is also referenced but it seems a mistake.

Why? It was done on purpose, I think exposing through Doxygen internal APIs
implemented by drivers must be a requirement, even if the exposed symbols
are not necessarily versioned.

-- 
Adrien Mazarguil
6WIND

Re: [pull-request] next-tm 17.08 pre-rc1

[Thomas Monjalon]

10/07/2017 09:43, Adrien Mazarguil:
> Hi Thomas,
> 
> On Sun, Jul 09, 2017 at 10:01:31PM +0200, Thomas Monjalon wrote:
> > 3/ The PMD interface file is referenced in the doxygen index:
> > +  [rte_tm_driver]      (@ref rte_tm_driver.h),
> > I see that rte_flow_driver.h is also referenced but it seems a mistake.
> 
> Why? It was done on purpose, I think exposing through Doxygen internal APIs
> implemented by drivers must be a requirement, even if the exposed symbols
> are not necessarily versioned.

No, this doxygen documentation is about API only.
Reminder: API means Application Programming Interface ;)

Re: [pull-request] next-tm 17.08 pre-rc1

[Dumitrescu, Cristian]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Sunday, July 9, 2017 9:02 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; jerin.jacob@caviumnetworks.com;
> hemant.agrawal@nxp.com; Singh, Jasvinder <jasvinder.singh@intel.com>;
> Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-dev] [pull-request] next-tm 17.08 pre-rc1
> 
> Hi,
> 
> 04/07/2017 17:38, Cristian Dumitrescu:
> >   http://dpdk.org/git/next/dpdk-next-tm
> 
> I'm sorry to not have considered this tree as a high priority.
> I think it may be integrated in RC2 because it is a totally new area
> and should not break any existing code.
> 
> I prefer to wait because I have seen some things to fix:
> 
> 1/ There is a compilation error with clang (notified in related thread).

Thanks for sending the error log, looks simple to fix, we will fix this ASAP.

> 
> 2/ Some functions are exposed in the API to query the ops.
> It seems dangerous and useless:
> 	- rte_eth_dev_tm_ops_get
> 	- rte_tm_ops_get
> 

Thomas, hopefully this is a misunderstanding on your side :(((.

This is a critical point that we debated ad nauseam on this email list (RFC, V1 -V6) and privately as well. You were included in the conversation, you also provided feed-back that we incorporated in the code, as documented in the patchset history log.

This is simply the mechanism that we (including you) agreed to use for modularizing the DPDK ethdev by adding new functionality in a modular plug-in way using separate namespace. This is the exact clone of the same mechanism that rte_flow is using and was merged in DPDK release 17.02. Why this change on the fundamentals now?

Hopefully, it is just misunderstanding.

> 3/ The PMD interface file is referenced in the doxygen index:
> +  [rte_tm_driver]      (@ref rte_tm_driver.h),
> I see that rte_flow_driver.h is also referenced but it seems a mistake.

We simply followed the rte_flow precedent. We will remove this line.

> 
> 4/ As it is a totally new API, it should be declared as EXPERIMENTAL
> in the MAINTAINERS file and in the doxygen.

We can add the EXPERIMENTAL tag for this API in the MANTAINERS file and at the top of the API file for DPDK release 17.08. Is this OK with you?

But, as Jerin also asked at the time when eventdev API was introduced, what is the process to remove the EXPERIMENTAL label? Do you agree that we can remove the experimental label in the next release 17.11?

IMO it makes sense to mark new APIs as experimental for some time, it should be very clear when this label can be removed. We had examples of customers being confused by this label and questioning us whether they should use such APIs or not, therefore the process or applying & removing this label should be clearly documented, which right now it is not at all.

To this moment, this was not followed consistently in DPDK either. Recent examples:
1. rte_flow API, introduced in DPDK release 17.02 was never marked as EXPERIMENTAL in either MANTAINERS or API file
2. eventdev API, introduced in DPDK release 17.05, was marked as EXPERIMENTAL in the MAINTAINERS file, but not in the API file


> 
> 5/ There is no doc in the programmer's guide.

We are definitely going to add comprehensive documentation for this new API before the 17.08 documentation deadline. Is this OK with you?

As a precedent, eventdev API was introduced in 17.05 with test application just added now (one release later).

> 
> 6/ There is no application to test it.
> 

Yes, we will either extend test-pmd or add a new example application to exercise this API for next DPDK release 17.11. CC-ing Tim and Mike to confirm.

> I know that the points 5/ and 6/ are long to complete.
> However I would like to know what is the plan?
> And should we integrate TM in 17.08 without neither doc nor app?

As stated above, we are going to add documentation for this new API on this release and test application in the next release.

Regards,
Cristian

Re: [pull-request] next-tm 17.08 pre-rc1

[Thomas Monjalon]

Hi,

10/07/2017 12:55, Dumitrescu, Cristian:
> Hi Thomas,
> 
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 
> > Hi,
> > 
> > 04/07/2017 17:38, Cristian Dumitrescu:
> > >   http://dpdk.org/git/next/dpdk-next-tm
> > 
> > I'm sorry to not have considered this tree as a high priority.
> > I think it may be integrated in RC2 because it is a totally new area
> > and should not break any existing code.
> > 
> > I prefer to wait because I have seen some things to fix:
> > 
> > 1/ There is a compilation error with clang (notified in related thread).
> 
> Thanks for sending the error log, looks simple to fix, we will fix this ASAP.
> 
> > 2/ Some functions are exposed in the API to query the ops.
> > It seems dangerous and useless:
> > 	- rte_eth_dev_tm_ops_get
> > 	- rte_tm_ops_get
> 
> Thomas, hopefully this is a misunderstanding on your side :(((.

Don't worry :)

> This is a critical point that we debated ad nauseam on this email list (RFC, V1 -V6) and privately as well. You were included in the conversation, you also provided feed-back that we incorporated in the code, as documented in the patchset history log.
> 
> This is simply the mechanism that we (including you) agreed to use for modularizing the DPDK ethdev by adding new functionality in a modular plug-in way using separate namespace. This is the exact clone of the same mechanism that rte_flow is using and was merged in DPDK release 17.02. Why this change on the fundamentals now?
> 
> Hopefully, it is just misunderstanding.

I mean that only the drivers need to get the ops.
The applications are using some dedicated functions rte_tm_* , right?
So the applications does not need direct ops access with rte_eth_dev_tm_ops_get()?
Sorry if it is my misunderstanding.

About rte_tm_ops_get, I don't remember why I talked about it.
It seems exposed only to drivers. My mistake. No issue there.

> > 3/ The PMD interface file is referenced in the doxygen index:
> > +  [rte_tm_driver]      (@ref rte_tm_driver.h),
> > I see that rte_flow_driver.h is also referenced but it seems a mistake.
> 
> We simply followed the rte_flow precedent. We will remove this line.
> 
> > 4/ As it is a totally new API, it should be declared as EXPERIMENTAL
> > in the MAINTAINERS file and in the doxygen.
> 
> We can add the EXPERIMENTAL tag for this API in the MANTAINERS file and at the top of the API file for DPDK release 17.08. Is this OK with you?

Yes, perfect.

> But, as Jerin also asked at the time when eventdev API was introduced, what is the process to remove the EXPERIMENTAL label? Do you agree that we can remove the experimental label in the next release 17.11?

Yes I think it is reasonnable.

> IMO it makes sense to mark new APIs as experimental for some time, it should be very clear when this label can be removed. We had examples of customers being confused by this label and questioning us whether they should use such APIs or not, therefore the process or applying & removing this label should be clearly documented, which right now it is not at all.

The idea is to highlight that it is a new API and may be not stable enough.
The question is when do we know it is mature enough?
I don't know and I guess it is up to the maintainer of the API.
Please remind that after removing the EXPERIMENTAL status, you must follow
the API deprecation status.

> To this moment, this was not followed consistently in DPDK either. Recent examples:
> 1. rte_flow API, introduced in DPDK release 17.02 was never marked as EXPERIMENTAL in either MANTAINERS or API file
> 2. eventdev API, introduced in DPDK release 17.05, was marked as EXPERIMENTAL in the MAINTAINERS file, but not in the API file

Yes, not perfect ;)

> > 5/ There is no doc in the programmer's guide.
> 
> We are definitely going to add comprehensive documentation for this new API before the 17.08 documentation deadline. Is this OK with you?

Yes, good to know.

> As a precedent, eventdev API was introduced in 17.05 with test application just added now (one release later).
> 
> > 6/ There is no application to test it.
> 
> Yes, we will either extend test-pmd or add a new example application to exercise this API for next DPDK release 17.11. CC-ing Tim and Mike to confirm.

Yes I think it would be important to have it in 17.11.

> > I know that the points 5/ and 6/ are long to complete.
> > However I would like to know what is the plan?
> > And should we integrate TM in 17.08 without neither doc nor app?
> 
> As stated above, we are going to add documentation for this new API on this release and test application in the next release.

OK looks a good plan.
Please be aware that the documentation must be submitted in two weeks
to let us few days for reviewing.

Re: [pull-request] next-tm 17.08 pre-rc1

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Monday, July 10, 2017 1:57 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; jerin.jacob@caviumnetworks.com;
> hemant.agrawal@nxp.com; Singh, Jasvinder <jasvinder.singh@intel.com>;
> Lu, Wenzhuo <wenzhuo.lu@intel.com>; O'Driscoll, Tim
> <tim.odriscoll@intel.com>; Glynn, Michael J <michael.j.glynn@intel.com>;
> Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Subject: Re: [dpdk-dev] [pull-request] next-tm 17.08 pre-rc1
> 
> Hi,
> 
> 10/07/2017 12:55, Dumitrescu, Cristian:
> > Hi Thomas,
> >
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > >
> > > Hi,
> > >
> > > 04/07/2017 17:38, Cristian Dumitrescu:
> > > >   http://dpdk.org/git/next/dpdk-next-tm
> > >
> > > I'm sorry to not have considered this tree as a high priority.
> > > I think it may be integrated in RC2 because it is a totally new area
> > > and should not break any existing code.
> > >
> > > I prefer to wait because I have seen some things to fix:
> > >
> > > 1/ There is a compilation error with clang (notified in related thread).
> >
> > Thanks for sending the error log, looks simple to fix, we will fix this ASAP.
> >
> > > 2/ Some functions are exposed in the API to query the ops.
> > > It seems dangerous and useless:
> > > 	- rte_eth_dev_tm_ops_get
> > > 	- rte_tm_ops_get
> >
> > Thomas, hopefully this is a misunderstanding on your side :(((.
> 
> Don't worry :)
> 
> > This is a critical point that we debated ad nauseam on this email list (RFC, V1
> -V6) and privately as well. You were included in the conversation, you also
> provided feed-back that we incorporated in the code, as documented in the
> patchset history log.
> >
> > This is simply the mechanism that we (including you) agreed to use for
> modularizing the DPDK ethdev by adding new functionality in a modular plug-
> in way using separate namespace. This is the exact clone of the same
> mechanism that rte_flow is using and was merged in DPDK release 17.02.
> Why this change on the fundamentals now?
> >
> > Hopefully, it is just misunderstanding.
> 
> I mean that only the drivers need to get the ops.
> The applications are using some dedicated functions rte_tm_* , right?
> So the applications does not need direct ops access with
> rte_eth_dev_tm_ops_get()?
> Sorry if it is my misunderstanding.
> 
> About rte_tm_ops_get, I don't remember why I talked about it.
> It seems exposed only to drivers. My mistake. No issue there.
> 

OK, so we're good then?

> > > 3/ The PMD interface file is referenced in the doxygen index:
> > > +  [rte_tm_driver]      (@ref rte_tm_driver.h),
> > > I see that rte_flow_driver.h is also referenced but it seems a mistake.
> >
> > We simply followed the rte_flow precedent. We will remove this line.
> >
> > > 4/ As it is a totally new API, it should be declared as EXPERIMENTAL
> > > in the MAINTAINERS file and in the doxygen.
> >
> > We can add the EXPERIMENTAL tag for this API in the MANTAINERS file and
> at the top of the API file for DPDK release 17.08. Is this OK with you?
> 
> Yes, perfect.
> 
> > But, as Jerin also asked at the time when eventdev API was introduced,
> what is the process to remove the EXPERIMENTAL label? Do you agree that
> we can remove the experimental label in the next release 17.11?
> 
> Yes I think it is reasonnable.
> 
> > IMO it makes sense to mark new APIs as experimental for some time, it
> should be very clear when this label can be removed. We had examples of
> customers being confused by this label and questioning us whether they
> should use such APIs or not, therefore the process or applying & removing
> this label should be clearly documented, which right now it is not at all.
> 
> The idea is to highlight that it is a new API and may be not stable enough.
> The question is when do we know it is mature enough?
> I don't know and I guess it is up to the maintainer of the API.
> Please remind that after removing the EXPERIMENTAL status, you must
> follow
> the API deprecation status.
> 
> > To this moment, this was not followed consistently in DPDK either. Recent
> examples:
> > 1. rte_flow API, introduced in DPDK release 17.02 was never marked as
> EXPERIMENTAL in either MANTAINERS or API file
> > 2. eventdev API, introduced in DPDK release 17.05, was marked as
> EXPERIMENTAL in the MAINTAINERS file, but not in the API file
> 
> Yes, not perfect ;)
> 
> > > 5/ There is no doc in the programmer's guide.
> >
> > We are definitely going to add comprehensive documentation for this new
> API before the 17.08 documentation deadline. Is this OK with you?
> 
> Yes, good to know.
> 
> > As a precedent, eventdev API was introduced in 17.05 with test application
> just added now (one release later).
> >
> > > 6/ There is no application to test it.
> >
> > Yes, we will either extend test-pmd or add a new example application to
> exercise this API for next DPDK release 17.11. CC-ing Tim and Mike to
> confirm.
> 
> Yes I think it would be important to have it in 17.11.
> 
> > > I know that the points 5/ and 6/ are long to complete.
> > > However I would like to know what is the plan?
> > > And should we integrate TM in 17.08 without neither doc nor app?
> >
> > As stated above, we are going to add documentation for this new API on
> this release and test application in the next release.
> 
> OK looks a good plan.
> Please be aware that the documentation must be submitted in two weeks
> to let us few days for reviewing.

Re: [pull-request] next-tm 17.08 pre-rc1

[Thomas Monjalon]

10/07/2017 15:21, Dumitrescu, Cristian:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 10/07/2017 12:55, Dumitrescu, Cristian:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > 2/ Some functions are exposed in the API to query the ops.
> > > > It seems dangerous and useless:
> > > > 	- rte_eth_dev_tm_ops_get
> > > > 	- rte_tm_ops_get
> > >
> > > Thomas, hopefully this is a misunderstanding on your side :(((.
> > 
> > Don't worry :)
> > 
> > > This is a critical point that we debated ad nauseam on this email list (RFC, V1
> > -V6) and privately as well. You were included in the conversation, you also
> > provided feed-back that we incorporated in the code, as documented in the
> > patchset history log.
> > >
> > > This is simply the mechanism that we (including you) agreed to use for
> > modularizing the DPDK ethdev by adding new functionality in a modular plug-
> > in way using separate namespace. This is the exact clone of the same
> > mechanism that rte_flow is using and was merged in DPDK release 17.02.
> > Why this change on the fundamentals now?
> > >
> > > Hopefully, it is just misunderstanding.
> > 
> > I mean that only the drivers need to get the ops.
> > The applications are using some dedicated functions rte_tm_* , right?
> > So the applications does not need direct ops access with
> > rte_eth_dev_tm_ops_get()?
> > Sorry if it is my misunderstanding.
> > 
> > About rte_tm_ops_get, I don't remember why I talked about it.
> > It seems exposed only to drivers. My mistake. No issue there.
> 
> OK, so we're good then?

Not exactly. In my understanding, rte_eth_dev_tm_ops_get() is useless.
Should it be removed then?

Re: [pull-request] next-tm 17.08 pre-rc1

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Monday, July 10, 2017 2:50 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; jerin.jacob@caviumnetworks.com;
> hemant.agrawal@nxp.com; Singh, Jasvinder <jasvinder.singh@intel.com>;
> Lu, Wenzhuo <wenzhuo.lu@intel.com>; O'Driscoll, Tim
> <tim.odriscoll@intel.com>; Glynn, Michael J <michael.j.glynn@intel.com>;
> Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Subject: Re: [dpdk-dev] [pull-request] next-tm 17.08 pre-rc1
> 
> 10/07/2017 15:21, Dumitrescu, Cristian:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > 10/07/2017 12:55, Dumitrescu, Cristian:
> > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > 2/ Some functions are exposed in the API to query the ops.
> > > > > It seems dangerous and useless:
> > > > > 	- rte_eth_dev_tm_ops_get
> > > > > 	- rte_tm_ops_get
> > > >
> > > > Thomas, hopefully this is a misunderstanding on your side :(((.
> > >
> > > Don't worry :)
> > >
> > > > This is a critical point that we debated ad nauseam on this email list
> (RFC, V1
> > > -V6) and privately as well. You were included in the conversation, you
> also
> > > provided feed-back that we incorporated in the code, as documented in
> the
> > > patchset history log.
> > > >
> > > > This is simply the mechanism that we (including you) agreed to use for
> > > modularizing the DPDK ethdev by adding new functionality in a modular
> plug-
> > > in way using separate namespace. This is the exact clone of the same
> > > mechanism that rte_flow is using and was merged in DPDK release 17.02.
> > > Why this change on the fundamentals now?
> > > >
> > > > Hopefully, it is just misunderstanding.
> > >
> > > I mean that only the drivers need to get the ops.
> > > The applications are using some dedicated functions rte_tm_* , right?
> > > So the applications does not need direct ops access with
> > > rte_eth_dev_tm_ops_get()?
> > > Sorry if it is my misunderstanding.
> > >
> > > About rte_tm_ops_get, I don't remember why I talked about it.
> > > It seems exposed only to drivers. My mistake. No issue there.
> >
> > OK, so we're good then?
> 
> Not exactly. In my understanding, rte_eth_dev_tm_ops_get() is useless.
> Should it be removed then?

Why do you think it is useless? How would the driver get the function specific (i.e. rte_flow, rte_tm, ...) operations structure?

I am afraid of reopening a lengthy discussion that we had when the time was right ...

Re: [pull-request] next-tm 17.08 pre-rc1

[Thomas Monjalon]

10/07/2017 17:46, Dumitrescu, Cristian:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 10/07/2017 15:21, Dumitrescu, Cristian:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > 10/07/2017 12:55, Dumitrescu, Cristian:
> > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > 2/ Some functions are exposed in the API to query the ops.
> > > > > > It seems dangerous and useless:
> > > > > > 	- rte_eth_dev_tm_ops_get
> > > > > > 	- rte_tm_ops_get
> > > > >
> > > > > Thomas, hopefully this is a misunderstanding on your side :(((.
> > > >
> > > > Don't worry :)
> > > >
> > > > > This is a critical point that we debated ad nauseam on this email list
> > (RFC, V1
> > > > -V6) and privately as well. You were included in the conversation, you
> > also
> > > > provided feed-back that we incorporated in the code, as documented in
> > the
> > > > patchset history log.
> > > > >
> > > > > This is simply the mechanism that we (including you) agreed to use for
> > > > modularizing the DPDK ethdev by adding new functionality in a modular
> > plug-
> > > > in way using separate namespace. This is the exact clone of the same
> > > > mechanism that rte_flow is using and was merged in DPDK release 17.02.
> > > > Why this change on the fundamentals now?
> > > > >
> > > > > Hopefully, it is just misunderstanding.
> > > >
> > > > I mean that only the drivers need to get the ops.
> > > > The applications are using some dedicated functions rte_tm_* , right?
> > > > So the applications does not need direct ops access with
> > > > rte_eth_dev_tm_ops_get()?
> > > > Sorry if it is my misunderstanding.
> > > >
> > > > About rte_tm_ops_get, I don't remember why I talked about it.
> > > > It seems exposed only to drivers. My mistake. No issue there.
> > >
> > > OK, so we're good then?
> > 
> > Not exactly. In my understanding, rte_eth_dev_tm_ops_get() is useless.
> > Should it be removed then?
> 
> Why do you think it is useless? How would the driver get the function specific (i.e. rte_flow, rte_tm, ...) operations structure?

The drivers get the structure via rte_tm_ops_get() function which is
in the well named file rte_tm_driver.h
My question is about rte_eth_dev_tm_ops_get() function which is
in the file rte_ethdev.h.
Please explain the difference between both functions and why
rte_eth_dev_tm_ops_get() is needed.

Sorry for opening the discussion, I don't see the explanation in doxygen.

Re: [pull-request] next-tm 17.08 pre-rc1

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Monday, July 10, 2017 4:54 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; jerin.jacob@caviumnetworks.com;
> hemant.agrawal@nxp.com; Singh, Jasvinder <jasvinder.singh@intel.com>;
> Lu, Wenzhuo <wenzhuo.lu@intel.com>; O'Driscoll, Tim
> <tim.odriscoll@intel.com>; Glynn, Michael J <michael.j.glynn@intel.com>;
> Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Subject: Re: [dpdk-dev] [pull-request] next-tm 17.08 pre-rc1
> 
> 10/07/2017 17:46, Dumitrescu, Cristian:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > 10/07/2017 15:21, Dumitrescu, Cristian:
> > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > 10/07/2017 12:55, Dumitrescu, Cristian:
> > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > > 2/ Some functions are exposed in the API to query the ops.
> > > > > > > It seems dangerous and useless:
> > > > > > > 	- rte_eth_dev_tm_ops_get
> > > > > > > 	- rte_tm_ops_get
> > > > > >
> > > > > > Thomas, hopefully this is a misunderstanding on your side :(((.
> > > > >
> > > > > Don't worry :)
> > > > >
> > > > > > This is a critical point that we debated ad nauseam on this email list
> > > (RFC, V1
> > > > > -V6) and privately as well. You were included in the conversation, you
> > > also
> > > > > provided feed-back that we incorporated in the code, as documented
> in
> > > the
> > > > > patchset history log.
> > > > > >
> > > > > > This is simply the mechanism that we (including you) agreed to use
> for
> > > > > modularizing the DPDK ethdev by adding new functionality in a
> modular
> > > plug-
> > > > > in way using separate namespace. This is the exact clone of the same
> > > > > mechanism that rte_flow is using and was merged in DPDK release
> 17.02.
> > > > > Why this change on the fundamentals now?
> > > > > >
> > > > > > Hopefully, it is just misunderstanding.
> > > > >
> > > > > I mean that only the drivers need to get the ops.
> > > > > The applications are using some dedicated functions rte_tm_* , right?
> > > > > So the applications does not need direct ops access with
> > > > > rte_eth_dev_tm_ops_get()?
> > > > > Sorry if it is my misunderstanding.
> > > > >
> > > > > About rte_tm_ops_get, I don't remember why I talked about it.
> > > > > It seems exposed only to drivers. My mistake. No issue there.
> > > >
> > > > OK, so we're good then?
> > >
> > > Not exactly. In my understanding, rte_eth_dev_tm_ops_get() is useless.
> > > Should it be removed then?
> >
> > Why do you think it is useless? How would the driver get the function
> specific (i.e. rte_flow, rte_tm, ...) operations structure?
> 
> The drivers get the structure via rte_tm_ops_get() function which is
> in the well named file rte_tm_driver.h
> My question is about rte_eth_dev_tm_ops_get() function which is
> in the file rte_ethdev.h.
> Please explain the difference between both functions and why
> rte_eth_dev_tm_ops_get() is needed.
> 
> Sorry for opening the discussion, I don't see the explanation in doxygen.

Hi Thomas,

Yes, you're right: drivers get the TM ops structure through the rte_tm_ops_get(), which directly accesses the dev_ops. You are fine with this, right?

Your concern is on the rte_eth_dev_tm_ops_get(), right? This function can be used by the app to see if TM feature is supported (the ops output argument is non-NULL) or not (the ops output argument is NULL). Here we followed the rte_flow pattern. Are you suggesting that we should remove it?

Regards,
Cristian

Re: [pull-request] next-tm 17.08 pre-rc1

[Thomas Monjalon]

10/07/2017 18:47, Dumitrescu, Cristian:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 10/07/2017 17:46, Dumitrescu, Cristian:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > 10/07/2017 15:21, Dumitrescu, Cristian:
> > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > 10/07/2017 12:55, Dumitrescu, Cristian:
> > > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > > > 2/ Some functions are exposed in the API to query the ops.
> > > > > > > > It seems dangerous and useless:
> > > > > > > > 	- rte_eth_dev_tm_ops_get
> > > > > > > > 	- rte_tm_ops_get
> > > > > > >
> > > > > > > Thomas, hopefully this is a misunderstanding on your side :(((.
> > > > > >
> > > > > > Don't worry :)
> > > > > >
> > > > > > > This is a critical point that we debated ad nauseam on this email list
> > > > (RFC, V1
> > > > > > -V6) and privately as well. You were included in the conversation, you
> > > > also
> > > > > > provided feed-back that we incorporated in the code, as documented
> > in
> > > > the
> > > > > > patchset history log.
> > > > > > >
> > > > > > > This is simply the mechanism that we (including you) agreed to use
> > for
> > > > > > modularizing the DPDK ethdev by adding new functionality in a
> > modular
> > > > plug-
> > > > > > in way using separate namespace. This is the exact clone of the same
> > > > > > mechanism that rte_flow is using and was merged in DPDK release
> > 17.02.
> > > > > > Why this change on the fundamentals now?
> > > > > > >
> > > > > > > Hopefully, it is just misunderstanding.
> > > > > >
> > > > > > I mean that only the drivers need to get the ops.
> > > > > > The applications are using some dedicated functions rte_tm_* , right?
> > > > > > So the applications does not need direct ops access with
> > > > > > rte_eth_dev_tm_ops_get()?
> > > > > > Sorry if it is my misunderstanding.
> > > > > >
> > > > > > About rte_tm_ops_get, I don't remember why I talked about it.
> > > > > > It seems exposed only to drivers. My mistake. No issue there.
> > > > >
> > > > > OK, so we're good then?
> > > >
> > > > Not exactly. In my understanding, rte_eth_dev_tm_ops_get() is useless.
> > > > Should it be removed then?
> > >
> > > Why do you think it is useless? How would the driver get the function
> > specific (i.e. rte_flow, rte_tm, ...) operations structure?
> > 
> > The drivers get the structure via rte_tm_ops_get() function which is
> > in the well named file rte_tm_driver.h
> > My question is about rte_eth_dev_tm_ops_get() function which is
> > in the file rte_ethdev.h.
> > Please explain the difference between both functions and why
> > rte_eth_dev_tm_ops_get() is needed.
> > 
> > Sorry for opening the discussion, I don't see the explanation in doxygen.
> 
> Hi Thomas,
> 
> Yes, you're right: drivers get the TM ops structure through the rte_tm_ops_get(), which directly accesses the dev_ops. You are fine with this, right?

Yes

> Your concern is on the rte_eth_dev_tm_ops_get(), right?

Yes, I feel you start understanding what I'm talking about ;)

> This function can be used by the app to see if TM feature is supported (the ops output argument is non-NULL) or not (the ops output argument is NULL). Here we followed the rte_flow pattern. Are you suggesting that we should remove it?

Yes
As far as I know, the rte_flow API does not expose the ops to the application.
Can we have the drivers capabilities in a different way?
In general, capabilities are richer than just checking there
is a function. I think it is better to have flags.
Anyway, capabilities API can be discussed after 17.08 merge.

Re: [pull-request] next-tm 17.08 pre-rc1

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Monday, July 10, 2017 5:58 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; jerin.jacob@caviumnetworks.com;
> hemant.agrawal@nxp.com; Singh, Jasvinder <jasvinder.singh@intel.com>;
> Lu, Wenzhuo <wenzhuo.lu@intel.com>; O'Driscoll, Tim
> <tim.odriscoll@intel.com>; Glynn, Michael J <michael.j.glynn@intel.com>;
> Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Subject: Re: [dpdk-dev] [pull-request] next-tm 17.08 pre-rc1
> 
> 10/07/2017 18:47, Dumitrescu, Cristian:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > 10/07/2017 17:46, Dumitrescu, Cristian:
> > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > 10/07/2017 15:21, Dumitrescu, Cristian:
> > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > > 10/07/2017 12:55, Dumitrescu, Cristian:
> > > > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > > > > > > 2/ Some functions are exposed in the API to query the ops.
> > > > > > > > > It seems dangerous and useless:
> > > > > > > > > 	- rte_eth_dev_tm_ops_get
> > > > > > > > > 	- rte_tm_ops_get
> > > > > > > >
> > > > > > > > Thomas, hopefully this is a misunderstanding on your side :(((.
> > > > > > >
> > > > > > > Don't worry :)
> > > > > > >
> > > > > > > > This is a critical point that we debated ad nauseam on this email
> list
> > > > > (RFC, V1
> > > > > > > -V6) and privately as well. You were included in the conversation,
> you
> > > > > also
> > > > > > > provided feed-back that we incorporated in the code, as
> documented
> > > in
> > > > > the
> > > > > > > patchset history log.
> > > > > > > >
> > > > > > > > This is simply the mechanism that we (including you) agreed to
> use
> > > for
> > > > > > > modularizing the DPDK ethdev by adding new functionality in a
> > > modular
> > > > > plug-
> > > > > > > in way using separate namespace. This is the exact clone of the
> same
> > > > > > > mechanism that rte_flow is using and was merged in DPDK release
> > > 17.02.
> > > > > > > Why this change on the fundamentals now?
> > > > > > > >
> > > > > > > > Hopefully, it is just misunderstanding.
> > > > > > >
> > > > > > > I mean that only the drivers need to get the ops.
> > > > > > > The applications are using some dedicated functions rte_tm_* ,
> right?
> > > > > > > So the applications does not need direct ops access with
> > > > > > > rte_eth_dev_tm_ops_get()?
> > > > > > > Sorry if it is my misunderstanding.
> > > > > > >
> > > > > > > About rte_tm_ops_get, I don't remember why I talked about it.
> > > > > > > It seems exposed only to drivers. My mistake. No issue there.
> > > > > >
> > > > > > OK, so we're good then?
> > > > >
> > > > > Not exactly. In my understanding, rte_eth_dev_tm_ops_get() is
> useless.
> > > > > Should it be removed then?
> > > >
> > > > Why do you think it is useless? How would the driver get the function
> > > specific (i.e. rte_flow, rte_tm, ...) operations structure?
> > >
> > > The drivers get the structure via rte_tm_ops_get() function which is
> > > in the well named file rte_tm_driver.h
> > > My question is about rte_eth_dev_tm_ops_get() function which is
> > > in the file rte_ethdev.h.
> > > Please explain the difference between both functions and why
> > > rte_eth_dev_tm_ops_get() is needed.
> > >
> > > Sorry for opening the discussion, I don't see the explanation in doxygen.
> >
> > Hi Thomas,
> >
> > Yes, you're right: drivers get the TM ops structure through the
> rte_tm_ops_get(), which directly accesses the dev_ops. You are fine with
> this, right?
> 
> Yes
> 
> > Your concern is on the rte_eth_dev_tm_ops_get(), right?
> 
> Yes, I feel you start understanding what I'm talking about ;)
> 
> > This function can be used by the app to see if TM feature is supported (the
> ops output argument is non-NULL) or not (the ops output argument is NULL).
> Here we followed the rte_flow pattern. Are you suggesting that we should
> remove it?
> 
> Yes
> As far as I know, the rte_flow API does not expose the ops to the application.
> Can we have the drivers capabilities in a different way?
> In general, capabilities are richer than just checking there
> is a function. I think it is better to have flags.
> Anyway, capabilities API can be discussed after 17.08 merge.

Hi Thomas,

Fixed everything you asked on the next-tm repository, please resume the pull.

I am working to send documentation as separate patch most likely next week. 

Changes:
1. rte_ethdev.[hc]: removed unused function rte_eth_dev_tm_ops_get()
2. doc/api/doxy-api-index.md: removed reference to rte_tm_driver.h
3. rte_tm.h: added EXPERIMENTAL warning at the top of the file
4. MANTAINERS: added EXPERIMENTAL tag for the Traffic Management API
5. Fixed clang warnings due to unused static function

Thanks!

Regards,
Cristian

Re: [pull-request] next-tm 17.08 pre-rc1

[Thomas Monjalon]

11/07/2017 20:20, Dumitrescu, Cristian:
> Hi Thomas,
> 
> Fixed everything you asked on the next-tm repository, please resume the pull.
> 
> I am working to send documentation as separate patch most likely next week. 
> 
> Changes:
> 1. rte_ethdev.[hc]: removed unused function rte_eth_dev_tm_ops_get()
> 2. doc/api/doxy-api-index.md: removed reference to rte_tm_driver.h
> 3. rte_tm.h: added EXPERIMENTAL warning at the top of the file
> 4. MANTAINERS: added EXPERIMENTAL tag for the Traffic Management API
> 5. Fixed clang warnings due to unused static function
> 
> Thanks!

Pulled, thanks