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

[Ray Kinsella <mdr@ashroe.eu>]

On 01/05/2020 14:18, Jerin Jacob wrote:
> On Tue, Apr 28, 2020 at 9:24 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>>
>> 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://pastebin.com/XYNFg14u
>>>>>>>>>>
>>>>>>>>>> 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.
> 
> None of the 6 PMDS covers all the features.
> 
>>
>>>>> 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.
> 
> ABI change are in structures. So the function versioning does not
> help. So we will wait for 20.11 then :-(
> 

yes - I spent some time looking at this also, and came to same conclusion.

Ray K