On 4/20/2020 5:10 PM, Thomas Monjalon wrote:
> 20/04/2020 16:06, Ferruh Yigit:
>> On 4/18/2020 10:44 AM, Thomas Monjalon wrote:
>>> 18/04/2020 07:04, Bill Zhou:
>>>> From: Ferruh Yigit <ferruh.yi...@intel.com>
>>>>> On 4/14/2020 9:32 AM, Dong Zhou wrote:
>>>>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>>>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>>>>> @@ -3015,6 +3015,7 @@ enum rte_eth_event_type {
>>>>>> RTE_ETH_EVENT_NEW, /**< port is probed */
>>>>>> RTE_ETH_EVENT_DESTROY, /**< port is released */
>>>>>> RTE_ETH_EVENT_IPSEC, /**< IPsec offload related event */
>>>>>> + RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected
>>>>> */
>>>>>> RTE_ETH_EVENT_MAX /**< max value of this enum */
>>>>>> };
>>>>>
>>>>>
>>>>> Just recognized that this is failing in ABI check [1], as far as last
>>>>> time for a
>>>>> similar enum warning a QAT patch has been dropped, should this need to
>>>>> wait for
>>>>> 20.11 too?
>>>>
>>>> This patch is commonly used for flow aging, there are 2 other patches have
>>>> implement flow aging in mlx5 driver reply to this patch.
> [...]
>>> These MAX values in enums are a pain.
>>> We can try to think what can be done, waiting 20.11.
>>> Not sure there is a solution, except hijacking an existing value
>>> not used in the PMD, waiting the definitive value in 20.11...
>>
>> Dropping from the tree as of now, to not cause more merge conflicts, we can
>> add
>> it later when issue is resolved.
>
> Thanks for dropping, that's the right thing to do
> when a patch is breaking ABI check.
>
> After some thoughts, I think it is acceptable to make a v3
> which ignore this specific enum change. I explain my thought below:
>
> An enum can accept a new value at 2 conditions:
> - added as last value (not changing old values)
> - new value not used by existing API
>
> The value RTE_ETH_EVENT_FLOW_AGED meet the above 2 conditions:
> - only RTE_ETH_EVENT_MAX is changed, which is consistent
> - new value sent to the app only if the app registered for it
>
Same here, as far as I can see it is safe to get this change.
If any DPDK API returns this enum, either as return of the API or as output
parameter, this still can be problem, because application may use that returned
value, this was the concern in the QAT sample.
But here application registers an event and DPDK library process callback for
it, so application callbacks won't be called for anything that application
doesn't already know about, in that respect this should be safe for old
applications.
Not sure if we can generalize above two conditions for all enum changes, but we
can investigate them case by case as we get the warnings.
> So, except if I miss something, I suggest we add this exception:
> Allow new value in rte_eth_event_type if added just before RTE_ETH_EVENT_MAX.
> In other words, allow changing the value of RTE_ETH_EVENT_MAX.
> The file to add such exception is devtools/libabigail.abignore.
>
OK to exception.
