Re: [dpdk-dev] [EXT] Re: [PATCH v4 1/4] ethdev: add tm support for shaper config in pkt mode

[Nithin Dabilpuram <ndabilpuram@marvell.com>]

On Fri, May 01, 2020 at 11:27:02AM +0100, Ferruh Yigit wrote:
> External Email
> 
> ----------------------------------------------------------------------
> On 4/29/2020 10:03 AM, Bruce Richardson wrote:
> > On Wed, Apr 29, 2020 at 09:45:44AM +0100, Dumitrescu, Cristian wrote:
> >>
> >>
> >>> -----Original Message-----
> >>> From: Thomas Monjalon <thomas@monjalon.net>
> >>> Sent: Tuesday, April 28, 2020 4:54 PM
> >>> To: Jerin Jacob <jerinjacobk@gmail.com>; Dumitrescu, Cristian
> >>> <cristian.dumitrescu@intel.com>
> >>> Cc: Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh
> >>> <ferruh.yigit@intel.com>; Luca Boccassi <bluca@debian.org>; Nithin
> >>> Dabilpuram <nithind1988@gmail.com>; Singh, Jasvinder
> >>> <jasvinder.singh@intel.com>; Andrew Rybchenko
> >>> <arybchenko@solarflare.com>; dev@dpdk.org; jerinj@marvell.com;
> >>> kkanas@marvell.com; Nithin Dabilpuram <ndabilpuram@marvell.com>;
> >>> Kinsella, Ray <ray.kinsella@intel.com>; Neil Horman
> >>> <nhorman@tuxdriver.com>; Kevin Traynor <ktraynor@redhat.com>; David
> >>> Marchand <david.marchand@redhat.com>
> >>> Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: add tm support for shaper
> >>> config in pkt mode
> >>>
> >>> 28/04/2020 17:04, Luca Boccassi:
> >>>> On Tue, 2020-04-28 at 15:45 +0100, Bruce Richardson wrote:
> >>>>> On Tue, Apr 28, 2020 at 03:06:20PM +0100, Ferruh Yigit wrote:
> >>>>>> On 4/27/2020 5:59 PM, Jerin Jacob wrote:
> >>>>>>> On Mon, Apr 27, 2020 at 10:19 PM Ferruh Yigit
> >>> <ferruh.yigit@intel.com> wrote:
> >>>>>>>> On 4/27/2020 5:29 PM, Jerin Jacob wrote:
> >>>>>>>>> On Mon, Apr 27, 2020 at 9:42 PM Ferruh Yigit
> >>> <ferruh.yigit@intel.com> wrote:
> >>>>>>>>>> On 4/27/2020 10:19 AM, Dumitrescu, Cristian wrote:
> >>>>>>>>>>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> >>>>>>>>>>>> On 4/24/2020 11:28 AM, Dumitrescu, Cristian wrote:
> >>>>>>>>>>>>> From: Nithin Dabilpuram <nithind1988@gmail.com>
> >>>>>>>>>>>>>> This patch also updates tm port/level/node capability
> >>> structures with
> >>>>>>>>>>>>>> exiting features of scheduler wfq packet mode,
> >>> scheduler wfq byte mode
> >>>>>>>>>>>>>> and private/shared shaper byte mode.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> SoftNIC PMD is also updated with new capabilities.
> >>> [...]
> >>>>>>>>>>>> Hi Nithin,
> >>>>>>>>>>>>
> >>>>>>>>>>>> It looks like patch is causing ABI break, I am getting following
> >>> warning [1],
> >>>>>>>>>>>> can you please check?
> >>>>>>>>>>>>
> >>>>>>>>>>>> [1]
> >>>>>>>>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__pastebin.com_XYNFg14u&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=FZ_tPCbgFOh18zwRPO9H0yDx8VW38vuapifdDfc8SFQ&m=ej5sP3-cEhEoCTZOia-QivXqgljtzBcMLtZGs-5c-Uc&s=B8z_5mQ2xO3C1izjmRe2zBApMrCUcW6KcAN-adglhJQ&e= 
> >>>>>>>>>>>
> >>>>>>>>>>> Hi Ferruh,
> >>>>>>>>>>>
> >>>>>>>>>>> The RTE_TM API is marked as experimental,
> >>>>>>>>>>> but it looks that this was not correctly marked
> >>>>>>>>>>> when __rte_experimental ABI checker was introduced.
> >>>>>>>>>>>
> >>>>>>>>>>> It is marked as experimental at the top of the rte_tm.h,
> >>>>>>>>>>> similarly to other APIs introduced around same time,
> >>>>>>>>>>> but it was not correctly picked up by the ABI check procedure
> >>>>>>>>>>> when later introduced, so __rte_experimental was not added
> >>> to every function.
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> :(
> >>>>>>>>>>
> >>>>>>>>>> Is it time to mature them?
> >>>>>>>>>>
> >>>>>>>>>> As you said they are not marked as experimental both in header
> >>> file (function
> >>>>>>>>>> declarations) and .map file.
> >>>>>>>>>>
> >>>>>>>>>> The problem is, they are not marked as experimental in
> >>> DPDK_20.0 ABI (v19.11),
> >>>>>>>>>> so marking them as experimental now will break the ABI. Not
> >>> sure what to do,
> >>>>>>>>>> cc'ed a few ABI related names for comment.
> >>>>>>>>>>
> >>>>>>>>>> For me, we need to proceed as the experimental tag removed
> >>> and APIs become
> >>>>>>>>>> mature starting from v19.11, since this is what happened in
> >>> practice, and remove
> >>>>>>>>>> a few existing being experimental references in the doxygen
> >>> comments.
> >>>>>>>>>
> >>>>>>>>> I think, accidentally we can not make a library as NON-
> >>> experimental.
> >>>>>>>>> TM never went through experimental to mature transition(see git
> >>> log
> >>>>>>>>> lib/librte_ethdev/rte_tm.h)
> >>>>>>>>> It was a bug to not mark as experimental in each function in the
> >>> ABI process.
> >>>>>>>>> Some of the features like packet marking are not even
> >>> implemented by any HW.
> >>>>>>>>> I think, we can make API stable only all the features are
> >>> implemented
> >>>>>>>>> by one or two HW.
> >>>
> >>> Yes this is what was decided one or two years ago I think.
> >>> But rte_tm API was introduced 3 years ago and is implemented by 6 PMDs.
> >>>
> >>>
> >>>
> >>>>>>>> Fair enough, specially if the API is not ready yet.
> >>>>>>>>
> >>>>>>>> But they were part of stable ABI, and marking them as experimental
> >>> now will
> >>>>>>>> break the old applications using these APIs.
> >>>>>>>
> >>>>>>> it is still marked as EXPERIMENTAL everywhere and API is not ready
> >>> yet.
> >>>
> >>> rte_tm is implemented in 6 PMDs.
> >>>
> >>>
> >>>>>> Existing experimental marks are text only for human parsing.
> >>>>>>
> >>>>>> The compiler attribute and build time checks are missing, and the
> >>> symbol in the
> >>>>>> binary doesn't have experimental tag. Our scripts and automated
> >>> checks won't
> >>>>>> detect it as experimental.
> >>>>>>
> >>>>>> My point is just having experimental comment in header file is not
> >>> enough to
> >>>>>> qualify the APIs as experimental.
> >>>>>>
> >>>>>>> Anyway, we need to break the ABI to make it work on various HW.
> >>>
> >>> Yes this is why I was asking in 19.11 to check our API,
> >>> in order to avoid such situation.
> >>>
> >>>
> >>>>>>> I am not sure what to do?
> >>>
> >>> Either manage ABI versioning, or wait 20.11.
> >>>
> >>>
> >>>>>>> IMO, We need to send a patch as Fixes: for the bug of not adding
> >>>>>>> __rte_experimental in each function.
> >>>
> >>> No, this is wrong.
> >>>
> >>
> >> Why exactly is this wrong? This is the gap that caused the current discussion, right?
> >>
> > It's wrong for this release, since we can't change things from stable back
> > to experimental. Any such patch will have to wait for 20.11, as agreed in
> > the discussion.
> > 
> 
> Deferring the patchet for this release.
> 
> Reminder that if the option "to mark rte_tm_* as experimental in v20.11"
> selected, requires deprecation notice before v20.11.

Thanks Ferruh for reminder. I'll send a deprecation notice patch for the same.