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

[Olivier Matz <olivier.matz@6wind.com>]

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?

Olivier