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

[Dumitrescu, Cristian]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas at monjalon.net]
> Sent: Sunday, July 9, 2017 9:02 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu at intel.com>
> Cc: dev at dpdk.org; jerin.jacob at caviumnetworks.com;
> hemant.agrawal at nxp.com; Singh, Jasvinder <jasvinder.singh at intel.com>;
> Lu, Wenzhuo <wenzhuo.lu at 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