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

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 11/24/20 3:56 PM, Ferruh Yigit wrote:
> 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.

I hope it is still possible to add hdr fields without ABI/ABI breakage
in 20.02.