Re: [PATCH dpdk v3 00/17] IPv6 APIs overhaul

[David Marchand <david.marchand@redhat.com>]

Hello Robin,

On Thu, Oct 10, 2024 at 9:42 PM Robin Jarry <rjarry@redhat.com> wrote:
>
> As discussed recently [1], here is a first draft of the IPv6 APIs rework. The
> API change was announced before the 24.07 release [2]. This series is intended
> for 24.11.
>
> [1] http://inbox.dpdk.org/dev/D2SR8T1H39CJ.JRQFI6JEH0OX@redhat.com/
> [2] https://git.dpdk.org/dpdk/commit/?id=835d4c41e0ab58a115c2170c886ba6d3cc1b5764
>
> I tried to keep the patches as small as possible; unfortunately some of them
> are quite big and cannot be broken down if we want to preserve a bisectable
> tree.
>
> Let me know what you think.
>
> Thanks!
>
> Cc: Bruce Richardson <bruce.richardson@intel.com>
> Cc: Ferruh Yigit <ferruh.yigit@amd.com>
> Cc: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> Cc: Morten Brørup <mb@smartsharesystems.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Cc: Vladimir Medvedkin <vladimir.medvedkin@intel.com>
>
> Changelog:
>
> v3:
>
> - replace *memcpy(ipv6) with direct struct assignments
> - replace in6_addr with rte_ipv6_addr
> - add more ipv6 utils to deal with addresses
> - replace string initializers and RTE_IPV6_ADDR() with RTE_IPV6()
> - restore deleted macro constants and mark them as RTE_DEPRECATED()
>
> Robin Jarry (17):
>   net: split raw checksum functions in separate header
>   net: split ipv6 symbols in separate header
>   net: add structure for ipv6 addresses
>   net: add ipv6 address utilities
>   net: use struct rte_ipv6_addr for header addresses
>   fib6,rib6,lpm6: use struct rte_ipv6_addr
>   fib6,rib6,lpm6: use ipv6 utils
>   rib6,fib6,lpm6: remove duplicate constants
>   cmdline: replace in6_addr with rte_ipv6_addr
>   graph,node: use struct rte_ipv6_addr and utils
>   pipeline: use struct rte_ipv6_addr
>   ipsec,security: use struct rte_ipv6_addr and utils
>   thash: use struct rte_ipv6_addr
>   gro: use struct rte_ipv6_addr
>   rte_flow: use struct rte_ipv6_addr
>   net: add utilities for well known ipv6 address types
>   ipv6: add function to check ipv6 version
>
>  MAINTAINERS                                 |    1 +
>  app/graph/ethdev.c                          |   44 +-
>  app/graph/ethdev.h                          |    9 +-
>  app/graph/ip6_route.c                       |   51 +-
>  app/graph/meson.build                       |    2 +-
>  app/graph/neigh.c                           |   21 +-
>  app/graph/neigh_priv.h                      |    4 +-
>  app/graph/route.h                           |    8 +-
>  app/test-fib/main.c                         |   51 +-
>  app/test-flow-perf/actions_gen.c            |    4 +-
>  app/test-flow-perf/items_gen.c              |    4 +-
>  app/test-pipeline/pipeline_hash.c           |    4 +-
>  app/test-pipeline/pipeline_lpm_ipv6.c       |   11 +-
>  app/test-pmd/cmdline.c                      |    4 +-
>  app/test-pmd/cmdline_flow.c                 |   14 +-
>  app/test-pmd/testpmd.h                      |   16 +-
>  app/test-sad/main.c                         |   24 +-
>  app/test/meson.build                        |    1 +
>  app/test/packet_burst_generator.c           |    5 +-
>  app/test/test_cmdline_ipaddr.c              |   49 +-
>  app/test/test_cryptodev_security_ipsec.c    |    1 +
>  app/test/test_fib6.c                        |   92 +-
>  app/test/test_fib6_perf.c                   |    8 +-
>  app/test/test_ipfrag.c                      |    4 +-
>  app/test/test_ipsec_sad.c                   |   46 +-
>  app/test/test_lpm6.c                        |  490 +++--
>  app/test/test_lpm6_data.h                   | 2025 ++++++++++---------
>  app/test/test_lpm6_perf.c                   |   10 +-
>  app/test/test_net_ip6.c                     |  195 ++
>  app/test/test_reassembly_perf.c             |   23 +-
>  app/test/test_rib6.c                        |   55 +-
>  app/test/test_table_combined.c              |    2 +-
>  app/test/test_table_tables.c                |    8 +-
>  app/test/test_thash.c                       |   46 +-
>  doc/guides/prog_guide/ipsec_lib.rst         |    4 +-
>  doc/guides/rel_notes/deprecation.rst        |   42 -
>  doc/guides/rel_notes/release_24_11.rst      |   12 +
>  drivers/common/cnxk/cnxk_security.c         |   15 +-
>  drivers/crypto/cnxk/cn9k_cryptodev_ops.c    |    1 +
>  drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c |    1 +
>  drivers/net/bnxt/bnxt_flow.c                |   12 +-
>  drivers/net/bonding/rte_eth_bond_pmd.c      |    6 +-
>  drivers/net/cxgbe/cxgbe_flow.c              |   20 +-
>  drivers/net/dpaa2/dpaa2_flow.c              |   24 +-
>  drivers/net/hinic/hinic_pmd_flow.c          |    6 +-
>  drivers/net/hinic/hinic_pmd_tx.c            |    2 +-
>  drivers/net/hns3/hns3_flow.c                |    8 +-
>  drivers/net/i40e/i40e_flow.c                |   12 +-
>  drivers/net/iavf/iavf_fdir.c                |    8 +-
>  drivers/net/iavf/iavf_fsub.c                |    8 +-
>  drivers/net/iavf/iavf_ipsec_crypto.c        |    9 +-
>  drivers/net/ice/ice_fdir_filter.c           |   12 +-
>  drivers/net/ice/ice_switch_filter.c         |   16 +-
>  drivers/net/igc/igc_flow.c                  |    4 +-
>  drivers/net/ixgbe/ixgbe_flow.c              |   12 +-
>  drivers/net/ixgbe/ixgbe_ipsec.c             |    4 +-
>  drivers/net/mlx5/hws/mlx5dr_definer.c       |   36 +-
>  drivers/net/mlx5/mlx5_flow.c                |    8 +-
>  drivers/net/mlx5/mlx5_flow_dv.c             |   20 +-
>  drivers/net/mlx5/mlx5_flow_hw.c             |   12 +-
>  drivers/net/mlx5/mlx5_flow_verbs.c          |    8 +-
>  drivers/net/mvpp2/mrvl_flow.c               |   16 +-
>  drivers/net/nfp/flower/nfp_flower_flow.c    |   34 +-
>  drivers/net/nfp/nfp_ipsec.c                 |    4 +-
>  drivers/net/nfp/nfp_net_flow.c              |   38 +-
>  drivers/net/qede/qede_filter.c              |    4 +-
>  drivers/net/sfc/sfc_flow.c                  |   22 +-
>  drivers/net/tap/tap_flow.c                  |   14 +-
>  drivers/net/txgbe/txgbe_flow.c              |   12 +-
>  drivers/net/txgbe/txgbe_ipsec.c             |    4 +-
>  examples/cmdline/commands.c                 |   30 +-
>  examples/ip_fragmentation/main.c            |   23 +-
>  examples/ip_pipeline/cli.c                  |   61 +-
>  examples/ip_pipeline/parser.c               |    2 +-
>  examples/ip_pipeline/parser.h               |    3 +-
>  examples/ip_pipeline/pipeline.c             |   17 +-
>  examples/ip_pipeline/pipeline.h             |    6 +-
>  examples/ip_pipeline/thread.c               |   15 +-
>  examples/ip_reassembly/main.c               |   23 +-
>  examples/ipsec-secgw/flow.c                 |   45 +-
>  examples/ipsec-secgw/ipsec.c                |   33 +-
>  examples/ipsec-secgw/ipsec.h                |    5 +-
>  examples/ipsec-secgw/ipsec_lpm_neon.h       |    7 +-
>  examples/ipsec-secgw/ipsec_worker.c         |   11 +-
>  examples/ipsec-secgw/ipsec_worker.h         |    4 +-
>  examples/ipsec-secgw/parser.c               |    2 +-
>  examples/ipsec-secgw/parser.h               |    4 +-
>  examples/ipsec-secgw/rt.c                   |   24 +-
>  examples/ipsec-secgw/sa.c                   |   37 +-
>  examples/ipsec-secgw/sad.h                  |   14 +-
>  examples/ipsec-secgw/sp6.c                  |  154 +-
>  examples/l3fwd-graph/main.c                 |   33 +-
>  examples/l3fwd/l3fwd_fib.c                  |   39 +-
>  examples/l3fwd/l3fwd_lpm.c                  |    8 +-
>  examples/l3fwd/l3fwd_route.h                |    9 +-
>  examples/l3fwd/lpm_route_parse.c            |    9 +-
>  examples/l3fwd/main.c                       |   32 +-
>  lib/cmdline/cmdline_parse_ipaddr.h          |    3 +-
>  lib/ethdev/rte_flow.h                       |   27 +-
>  lib/fib/meson.build                         |    4 +-
>  lib/fib/rte_fib6.c                          |   19 +-
>  lib/fib/rte_fib6.h                          |   16 +-
>  lib/fib/trie.c                              |  117 +-
>  lib/fib/trie.h                              |   19 +-
>  lib/fib/trie_avx512.c                       |   38 +-
>  lib/fib/trie_avx512.h                       |   10 +-
>  lib/gro/gro_tcp6.c                          |    8 +-
>  lib/gro/gro_tcp6.h                          |    6 +-
>  lib/hash/rte_thash.h                        |   29 +-
>  lib/ip_frag/rte_ip_frag.h                   |    1 +
>  lib/ip_frag/rte_ipv6_reassembly.c           |    4 +-
>  lib/ipsec/iph.h                             |    1 +
>  lib/ipsec/rte_ipsec_sad.h                   |    5 +-
>  lib/lpm/meson.build                         |    1 +
>  lib/lpm/rte_lpm6.c                          |  148 +-
>  lib/lpm/rte_lpm6.h                          |   19 +-
>  lib/net/meson.build                         |    2 +
>  lib/net/rte_cksum.h                         |  180 ++
>  lib/net/rte_ip.h                            |  467 +----
>  lib/net/rte_ip6.h                           |  691 +++++++
>  lib/net/rte_net.c                           |    1 +
>  lib/net/rte_net.h                           |    1 +
>  lib/node/ip6_lookup.c                       |   21 +-
>  lib/node/rte_node_ip6_api.h                 |    3 +-
>  lib/pipeline/rte_swx_ipsec.c                |   11 +-
>  lib/pipeline/rte_swx_ipsec.h                |    5 +-
>  lib/pipeline/rte_table_action.c             |   45 +-
>  lib/pipeline/rte_table_action.h             |    7 +-
>  lib/rib/meson.build                         |    2 +-
>  lib/rib/rte_rib6.c                          |  112 +-
>  lib/rib/rte_rib6.h                          |   27 +-
>  lib/security/rte_security.h                 |    5 +-
>  lib/table/rte_table_lpm_ipv6.c              |   12 +-
>  lib/table/rte_table_lpm_ipv6.h              |    7 +-
>  lib/vhost/virtio_net.c                      |    1 +
>  135 files changed, 3399 insertions(+), 3128 deletions(-)
>  create mode 100644 app/test/test_net_ip6.c
>  create mode 100644 lib/net/rte_cksum.h
>  create mode 100644 lib/net/rte_ip6.h

- Now that many changes hit the main repo, this series needs some
rebasing (conflicts are not too difficult, but having this rebase run
through a bit in the CI would be great).


- From a compatibility pov, I am not fond of the rte_ip.h => rte_ip.h,
rte_ip6.h split.

Applications will need to be updated for something that was ambiguous
so far: rte_ip.h defined all symbols, regardless of IPv4 and IPv6.
What do you think of keeping a rte_ip.h compat header that just
includes new rte_ip4.h and rte_ip6.h headers?


- With current patch, since only rte_ip.h is parsed in doxygen, we
lost documentation for IPv6.
Regardless of keeping a compat header, doc/api/doxy-api-index.md needs
some update.


- bonus 1:
It would be worth cleaning unneeded includes in rte_ip*.h.
But changing this is risky if we want to take this series in rc1.
Please don't do this in next revision, this will wait after merging this series.

For example, running iwyu returns:

lib/net/rte_ip.h should remove these lines:
- #include <arpa/inet.h>  // lines 26-26
- #include <netinet/ip6.h>  // lines 28-28
- #include <rte_mbuf.h>  // lines 33-33
- #include <sys/socket.h>  // lines 23-23
- #include <sys/types.h>  // lines 24-24

lib/net/rte_ip6.h should remove these lines:
- #include <arpa/inet.h>  // lines 26-26
- #include <netinet/ip6.h>  // lines 27-27
- #include <rte_mbuf.h>  // lines 31-31
- #include <sys/socket.h>  // lines 23-23


- bonus 2:
Would it be possible to provide a cocci script or some shellscript for
an application to convert to the updated APIs (especially the impact
on rte_flow and other structures)?
We did something similar for the prefixing of dpdk structures with RTE_.

Idem, not necessary for rc1.


-- 
David Marchand