Re: [dpdk-dev] [PATCH] doc: announce flow API matching pattern struct changes

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 11/24/2020 11:43 AM, Ori Kam wrote:
> Hi
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Monday, November 23, 2020 5:51 PM
>> Subject: Re: [PATCH] doc: announce flow API matching pattern struct changes
>>
>> On 11/23/2020 2:25 PM, Andrew Rybchenko wrote:
>>> On 11/23/20 5:17 PM, Ferruh Yigit wrote:
>>>> On 11/23/2020 1:50 PM, Andrew Rybchenko wrote:
>>>>> On 11/23/20 4:40 PM, Ferruh Yigit wrote:
>>>>>> Proposing to replace protocol header fields in the ``rte_flow_item_*``
>>>>>> structures with the protocol structs, like:
>>>>>>
>>>>>> Current ``struct rte_flow_item_eth``,
>>>>>>
>>>>>> struct rte_flow_item_eth {
>>>>>>       struct rte_ether_addr dst;
>>>>>>       struct rte_ether_addr src;
>>>>>>       rte_be16_t type;
>>>>>>       uint32_t has_vlan:1;
>>>>>>       uint32_t reserved:31;
>>>>>> }
>>>>>>
>>>>>> will become
>>>>>>
>>>>>> struct rte_flow_item_eth {
>>>>>>       struct rte_ether_hdr hdr;
>>>>>>       uint32_t has_vlan:1;
>>>>>>       uint32_t reserved:31;
>>>>>> }
>>>>>>
>>>>>> This is both for documenting the intention and to be sure
>>>>>> ``rte_flow_item_*`` always starts with complete protocol header.
>>>>>>
>>>>>> Already many ``rte_flow_item_*`` structs implemented to have protocol
>>>>>> struct, target is convert all to this usage.
>>>>>>
>>>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>>
>>>>> Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>
>>>>> a minor note below
>>>>>
>>>>>> ---
>>>>>> Cc: Thomas Monjalon <thomas@monjalon.net>
>>>>>> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>> Cc: Ori Kam <orika@nvidia.com>
>>>>>> ---
>>>>>>     doc/guides/rel_notes/deprecation.rst | 7 +++++++
>>>>>>     1 file changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/doc/guides/rel_notes/deprecation.rst
>>>>>> b/doc/guides/rel_notes/deprecation.rst
>>>>>> index 96986fabd598..a2fa0c196472 100644
>>>>>> --- a/doc/guides/rel_notes/deprecation.rst
>>>>>> +++ b/doc/guides/rel_notes/deprecation.rst
>>>>>> @@ -88,6 +88,13 @@ Deprecation Notices
>>>>>>       will be limited to maximum 256 queues.
>>>>>>       Also compile time flag ``RTE_ETHDEV_QUEUE_STAT_CNTRS`` will be
>>>>>> removed.
>>>>>>     +* ethdev: The flow API matching pattern structures, ``struct
>>>>>> rte_flow_item_*``,
>>>>>> +  should start with relevant protocol header.
>>>>>> +  Some matching pattern structures implements this by duplicating
>>>>>> protocol header
>>>>>> +  fields in the struct. To clarify the intention and to be sure
>>>>>> protocol header
>>>>>> +  is intact, will replace those fields with relevant protocol
>>>>>> header struct.
>>>>>> +  Target is v21.02 release and this should not change the ABI.
>>>>>> +
>>>>>>     * sched: To allow more traffic classes, flexible mapping of pipe
>>>>>> queues to
>>>>>>       traffic classes, and subport level configuration of pipes and
>>>>>> queues
>>>>>>       changes will be made to macros, data structures and API
>>>>>> functions defined
>>>>>>
>>>>>
>>>>> Just want to highlight that even API could be kept using
>>>>> unnamed union for hdr and unnamed structure for existing
>>>>> protocol header fields.
>>>>>
>>>>
>>>> Then we may never clean the protocol header fields out of it,
>>>> yes this will impact the user but I believe the impact is small and
>>>> trivial,
>>>> I prefer replacing fields with protocol struct.
>>>
>>> The problem that API breakages are bad and, for example, OvS uses these
>>> fields.
>>>
>>> May be API breakage should be postponed to 21.11?
>>>
>>
>> Agree but it is not as bad as ABI break, if user is already compiling their
>> code, it is not too bad to adjust the struct for changes, and the changes are
>> straightforward.
>>
> I'm not sure which is worse ABI or API, API is more straight forward but all apps must be modified,
> while ABI is hidden and happens only in rare cases.
> In a addition it may result in large number of changes (simple but large number)
> 
>> But if, somehow, application needs to support multiple version of the DPDK it
>> can be headache.
>>
> 
> Agree,
> 
>> We may go with your suggestion until 21.11, and do the cleanup on 21.11, will
>> it
>> work?
> +1 also when considering my next line,
> 
> One more point to consider what happens to struct that are not according to spec,
> for example mpls, geneve where the struct is different than the item.
> 

At least for mpls & geneve, the ABI still looks same so change is still 
possible, but a few fields seems merged which means the change will require more 
updates in the user application and the drivers. Anyway, agree to postpone 
change to the 21.11.

I will send a v2.