Re: [dpdk-dev] [RFC 00/14] prefix network structures

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

On 12/27/2018 9:35 AM, Olivier Matz wrote:
> Hi,
> 
> On Fri, Dec 21, 2018 at 03:14:29PM +0000, Ferruh Yigit wrote:
>> On 12/21/2018 2:38 PM, Wiles, Keith wrote:
>>>
>>>
>>>> On Dec 20, 2018, at 5:48 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
>>>>
>>>> On Thu, 20 Dec 2018 21:59:37 +0000
>>>> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>>>
>>>>> On 10/24/2018 9:18 AM, olivier.matz at 6wind.com (Olivier Matz) wrote:
>>>>>> This RFC targets 19.02.
>>>>>>
>>>>>> The rte_net headers conflict with the libc headers, because
>>>>>> some definitions are duplicated, sometimes with few differences.
>>>>>> This was discussed in [1], and more recently at the techboard.
>>>>>>
>>>>>> Before sending the deprecation notice (target for this is 18.11),
>>>>>> here is a draft that can be discussed.
>>>>>>
>>>>>> This RFC adds the rte_ (or RTE_) prefix to all structures, functions
>>>>>> and defines in rte_net library. This is a big changeset, that will
>>>>>> break the API of many functions, but not the ABI.
>>>>>>
>>>>>> One question I'm asking is how can we manage the transition.
>>>>>> Initially, I hoped it was possible to have a compat layer during
>>>>>> one release (supporting both prefixed and unprefixed names), but
>>>>>> now that the patch is done, it seems the impact is too big, and
>>>>>> impacts too many libraries.
>>>>>>
>>>>>> Few examples:
>>>>>>  - rte_eth_macaddr_get/add/remove() use a (struct rte_ether_addr *)
>>>>>>  - many rte_flow structures use the rte_ prefixed net structures
>>>>>>  - the mac field of virtio_net structure is rte_ether_addr
>>>>>>  - the first arg of rte_thash_load_v6_addrs is (struct rte_ipv6_hdr *)
>>>>>>  ...
>>>>>>
>>>>>> Therefore, it is clear that doing this would break the compilation
>>>>>> of many external applications.
>>>>>>
>>>>>> Another drawback we need to take in account: it will make the
>>>>>> backport of patches more difficult, although this is something
>>>>>> that could be tempered by a script.
>>>>>>
>>>>>> While it is obviously better to have a good namespace convention, 
>>>>>> we need to identify the issues we have today before deciding it's
>>>>>> worth doing the change.
>>>>>>
>>>>>> Comments?  
>>>>>
>>>>> Is there an consensus about the patchset? There was a decision on techboard to
>>>>> go with this change (adding rte_ prefix) [1].
>>>>>
>>>>>
>>>>> This is something that will affect DPDK consumers. Since many APIs are changing
>>>>> most probably will break API compatibility for many libraries.
>>>>>
>>>>> Meanwhile the conflict with the libc headers mentioned a few times in the past,
>>>>> this is something we need to fix.
>>>>>
>>>>> There are a few comments reluctant to this big modification, but what I
>>>>> understand from Olivier's response both using BSD defines or having
>>>>> compatibility headers in DPDK won't solve the problem completely.
>>>>>
>>>>> And assuming we will continue with this solution, another question is do we
>>>>> still want to push in 19.02 scope? (And from process point of view I think a
>>>>> deprecation notice is not merged for this change in 18.11 scope.)
>>>>>
>>>>> With the prediction of 19.05 will be big and already break API/ABI for some
>>>>> libraries, can we push this into 19.05 as an early merge into repo?
>>>>>
>>>>> And I think this patch will affect LTS releases and will break auto backporting
>>>>> for many fixes because it touches many places, so pushing this change even to
>>>>> next LTS (19.11) can be an option.
>>>>>
>>>>>
>>>>> Olivier, Thomas,
>>>>>
>>>>> What do you think about postponing this to 19.05 or even 19.11 ?
>>>>>
>>>>>
>>>>>
>>>>> [1]
>>>>> https://mails.dpdk.org/archives/dev/2018-October/116695.html
>>>>>
>>>>>>
>>>>>>
>>>>>> Things that are missing in RFC:
>>>>>> - test with FreeBSD
>>>>>> - manually fix some indentation issues
>>>>>>
>>>>>>
>>>>>> Olivier Matz (14):
>>>>>>  net: add rte prefix to arp structures
>>>>>>  net: add rte prefix to arp defines
>>>>>>  net: add rte prefix to ether structures
>>>>>>  net: add rte prefix to ether functions
>>>>>>  net: add rte prefix to ether defines
>>>>>>  net: add rte prefix to esp structure
>>>>>>  net: add rte prefix to gre structure
>>>>>>  net: add rte prefix to icmp structure
>>>>>>  net: add rte prefix to icmp defines
>>>>>>  net: add rte prefix to ip structure
>>>>>>  net: add rte prefix to ip defines
>>>>>>  net: add rte prefix to sctp structure
>>>>>>  net: add rte prefix to tcp structure
>>>>>>  net: add rte prefix to udp structure  
>>>>>
>>>>>
>>>>
>>>> Sigh. Another case where DPDK has to reinvent something because
>>>> we can't figure out how to do dependent libraries correctly.
>>>> I would have rather just using the existing Linux, BSD definitions
>>>> and fixing the DPDK code.
> 
> 
> It is not that simple. As I said in [1], there are still some
> differences between gnu libc and freebsd libc. Unfortunatly, the struct
> ether_addr is one of the most important in dpdk, because it is widely
> used in APIs (drivers).
> 
> We can find others differences, for instance in constant definitions in
> if_arp.h. I also see that some structures are packed in freebsd but not
> in glibc (ex: icmp6), this could have performance impact.
> 
> Many protocols that are currently defined in dpdk are missing in glibc:
> esp, sctp, gre, mpls, ... so we will at least need rte_ structures for
> these protocols.
> 
> Supporting other OSes or libc in the future could also increase the gaps.
> 
> For these reasons think it is reasonable to have a consistent set of
> network structures in dpdk.
> 
> 
> [1] https://mails.dpdk.org/archives/dev/2018-October/117258.html
> 
> 
>>>> It is probably the only viable compromise, but it adds to long
>>>> term maintenance, and breaks DPDK applications. Neither of which
>>>> is a good thing.
>>>>
>>>> Should this be done by marking the old structure deprecated
>>>> first? Ideally, spread over three releases: first, tell the users
>>>> in the documentation it is coming; second, mark the old structures
>>>> as deprecated causing compiler warnings; third, remove the old
>>>> definitions.  Doing at once is introducing user pain for no gain.
>>>
>>> +1
> 
> Annoucing the change before doing it is obvious. Marking the old
> structures as deprecated before removing them is maybe doable (to be
> checked that it does not cause conflicts with new structures), but it
> means the conflict with libc headers that we are trying to solve will
> remain for one more version, for a limited gain.
> 
>> With the current timeline, readiness of the patch and comments, at least it
>> won't able to make this release, I will update the patchset status as 'Deferred'
>>
>> Should we discuss this again in techboard?
> 
> We should surely weigh the pros and cons. Especially the additional
> backport troubles it can bring.
> 
> Are many people bothered by the current conflict with the libc headers?

This is still open.

If we will get these patchset, I suggest it getting early in the 19.05, patch is
mechanical but it is huge and will affect almost all other patches under
development. So I am not really for pushing this close to RC.

Is there any way to decide on this week, at worst next week?

Thanks,
ferruh