From: Thomas Monjalon <thomas@monjalon.net>
To: "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>
Cc: dev@dpdk.org,
"jerin.jacob@caviumnetworks.com" <jerin.jacob@caviumnetworks.com>,
"hemant.agrawal@nxp.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
Date: Mon, 10 Jul 2017 14:57:24 +0200 [thread overview]
Message-ID: <6030891.m1QB3o9leh@xps> (raw)
In-Reply-To: <3EB4FA525960D640B5BDFFD6A3D891267BA7D62D@IRSMSX108.ger.corp.intel.com>
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.
next prev parent reply other threads:[~2017-07-10 12:57 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-04 15:38 Cristian Dumitrescu
2017-07-04 15:47 ` Thomas Monjalon
2017-07-04 16:52 ` Dumitrescu, Cristian
2017-07-04 20:21 ` Thomas Monjalon
2017-07-05 10:36 ` Dumitrescu, Cristian
2017-07-09 20:01 ` Thomas Monjalon
2017-07-10 7:43 ` Adrien Mazarguil
2017-07-10 7:51 ` Thomas Monjalon
2017-07-10 10:55 ` Dumitrescu, Cristian
2017-07-10 12:57 ` Thomas Monjalon [this message]
2017-07-10 13:21 ` Dumitrescu, Cristian
2017-07-10 13:49 ` Thomas Monjalon
2017-07-10 15:46 ` Dumitrescu, Cristian
2017-07-10 15:54 ` Thomas Monjalon
2017-07-10 16:47 ` Dumitrescu, Cristian
2017-07-10 16:58 ` Thomas Monjalon
2017-07-11 18:20 ` Dumitrescu, Cristian
2017-07-12 17:33 ` Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6030891.m1QB3o9leh@xps \
--to=thomas@monjalon.net \
--cc=adrien.mazarguil@6wind.com \
--cc=cristian.dumitrescu@intel.com \
--cc=dev@dpdk.org \
--cc=hemant.agrawal@nxp.com \
--cc=jasvinder.singh@intel.com \
--cc=jerin.jacob@caviumnetworks.com \
--cc=michael.j.glynn@intel.com \
--cc=tim.odriscoll@intel.com \
--cc=wenzhuo.lu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).