On 1 Apr 2026, at 11:13, Eli Britstein wrote: > As a pre-step towards introducing netdev-doca, that has common parts > with netdev-dpdk, refactor declarations to be non-static, declared in a > new file netdev-dpdk-private. > > Signed-off-by: Eli Britstein <[email protected]>
Thank you, Eli, for starting this big rework to avoid all the code duplication. I have some comments below, but skimming through the hundreds of changes, most of it is fine. I've included Kevin and David, who worked quite a lot on the DPDK code, to get some extra pairs of eyes on these changes (and the subsequent ones). Cheers, Eelco > --- > lib/automake.mk | 1 + > lib/netdev-dpdk-private.h | 173 +++++ > lib/netdev-dpdk.c | 1519 +++++++++++++++++-------------------- > 3 files changed, 880 insertions(+), 813 deletions(-) > create mode 100644 lib/netdev-dpdk-private.h > > diff --git a/lib/automake.mk b/lib/automake.mk > index cb6458b0d..bab03c3e7 100644 > --- a/lib/automake.mk > +++ b/lib/automake.mk > @@ -209,6 +209,7 @@ lib_libopenvswitch_la_SOURCES = \ > lib/multipath.h \ > lib/namemap.c \ > lib/netdev-dpdk.h \ > + lib/netdev-dpdk-private.h \ > lib/netdev-dummy.c \ > lib/netdev-provider.h \ > lib/netdev-vport.c \ > diff --git a/lib/netdev-dpdk-private.h b/lib/netdev-dpdk-private.h > new file mode 100644 > index 000000000..9b82db750 > --- /dev/null > +++ b/lib/netdev-dpdk-private.h > @@ -0,0 +1,173 @@ > +/* > + * SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & > AFFILIATES. > + * All rights reserved. > + * SPDX-License-Identifier: Apache-2.0 > + * > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * Unless required by applicable law or agreed to in writing, software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ As this is mainly a move of code from netdev-dpdk.c, should this file retain the original copyright header? The current header shows 2026 NVIDIA, but much of this code dates back to 2014 (Nicira). Preserving the original copyright maintains the historical record of when the code as first written. > + > +#ifndef NETDEV_DPDK_PRIVATE_H > +#define NETDEV_DPDK_PRIVATE_H > + > +#include <config.h> > + > +#include <rte_config.h> > +#include <rte_ethdev.h> > +#include <rte_spinlock.h> > + > +#include "netdev-provider.h" > +#include "util.h" > + > +#include "openvswitch/thread.h" > + > +extern const struct rte_eth_conf port_conf; > + > +/* Defines. */ > + > +#define SOCKET0 0 > + > +/* > + * need to reserve tons of extra space in the mbufs so we can align the > + * DMA addresses to 4KB. > + * The minimum mbuf size is limited to avoid scatter behaviour and drop in > + * performance for standard Ethernet MTU. > + */ > +#define ETHER_HDR_MAX_LEN (RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN \ > + + (2 * VLAN_HEADER_LEN)) > +#define MTU_TO_FRAME_LEN(mtu) ((mtu) + RTE_ETHER_HDR_LEN + \ > + RTE_ETHER_CRC_LEN) > +#define MTU_TO_MAX_FRAME_LEN(mtu) ((mtu) + ETHER_HDR_MAX_LEN) > +#define FRAME_LEN_TO_MTU(frame_len) ((frame_len) \ > + - RTE_ETHER_HDR_LEN - RTE_ETHER_CRC_LEN) > +#define NETDEV_DPDK_MBUF_ALIGN 1024 > + > +#define MP_CACHE_SZ RTE_MEMPOOL_CACHE_MAX_SIZE > + > +/* Default size of Physical NIC RXQ */ > +#define NIC_PORT_DEFAULT_RXQ_SIZE 2048 > +/* Default size of Physical NIC TXQ */ > +#define NIC_PORT_DEFAULT_TXQ_SIZE 2048 > + > +#define DPDK_ETH_PORT_ID_INVALID RTE_MAX_ETHPORTS > + > +/* DPDK library uses uint16_t for port_id. */ > +typedef uint16_t dpdk_port_t; > +#define DPDK_PORT_ID_FMT "%"PRIu16 > + > +/* Enums. */ > + > +enum dpdk_hw_ol_features { > + NETDEV_RX_CHECKSUM_OFFLOAD = 1 << 0, > + NETDEV_RX_HW_CRC_STRIP = 1 << 1, > + NETDEV_RX_HW_SCATTER = 1 << 2, > + NETDEV_TX_IPV4_CKSUM_OFFLOAD = 1 << 3, > + NETDEV_TX_TCP_CKSUM_OFFLOAD = 1 << 4, > + NETDEV_TX_UDP_CKSUM_OFFLOAD = 1 << 5, > + NETDEV_TX_SCTP_CKSUM_OFFLOAD = 1 << 6, > + NETDEV_TX_TSO_OFFLOAD = 1 << 7, > + NETDEV_TX_VXLAN_TNL_TSO_OFFLOAD = 1 << 8, > + NETDEV_TX_GENEVE_TNL_TSO_OFFLOAD = 1 << 9, > + NETDEV_TX_OUTER_IP_CKSUM_OFFLOAD = 1 << 10, > + NETDEV_TX_OUTER_UDP_CKSUM_OFFLOAD = 1 << 11, > + NETDEV_TX_GRE_TNL_TSO_OFFLOAD = 1 << 12, > +}; > + > +/* Structs. */ > + > +#ifndef NETDEV_DPDK_TX_Q_TYPE > +#error "NETDEV_DPDK_TX_Q_TYPE must be defined before" \ > + "including netdev-dpdk-private.h" > +#endif > + > +#ifndef NETDEV_DPDK_SW_STATS_TYPE > +#error "NETDEV_DPDK_SW_STATS_TYPE must be defined before" \ > + "including netdev-dpdk-private.h" > +#endif > + > +#ifndef NETDEV_DPDK_GLOBAL_MUTEX > +#error "NETDEV_DPDK_GLOBAL_MUTEX must be defined before" \ > + "including netdev-dpdk-private.h" > +#endif Should the names have some kind of suffix so we can easily see what they are? Something like: #define NETDEV_DPDK_TX_QUEUE_STRUCT #define NETDEV_DPDK_SW_STATS_STRUCT #define NETDEV_DPDK_GLOBAL_MUTEX_NAME Open for any other suggestions. > +struct netdev_rxq_dpdk { > + struct netdev_rxq up; > + dpdk_port_t port_id; > +}; > + > +struct netdev_dpdk_common { > + PADDED_MEMBERS_CACHELINE_MARKER(CACHE_LINE_SIZE, cacheline0, > + uint16_t port_id; > + bool attached; > + bool is_representor; > + bool started; > + struct eth_addr hwaddr; > + int mtu; > + int socket_id; > + int max_packet_len; > + enum netdev_flags flags; > + int link_reset_cnt; > + char *devargs; > + NETDEV_DPDK_TX_Q_TYPE *tx_q; > + struct rte_eth_link link; > + ); > + > + PADDED_MEMBERS_CACHELINE_MARKER(CACHE_LINE_SIZE, cacheline1, > + struct ovs_mutex mutex OVS_ACQ_AFTER(NETDEV_DPDK_GLOBAL_MUTEX); > + struct dpdk_mp *dpdk_mp; > + ); > + > + PADDED_MEMBERS(CACHE_LINE_SIZE, > + struct netdev up; > + struct ovs_list list_node OVS_GUARDED_BY(NETDEV_DPDK_GLOBAL_MUTEX); > + bool rx_metadata_delivery_configured; > + ); > + > + PADDED_MEMBERS(CACHE_LINE_SIZE, > + struct netdev_stats stats; > + NETDEV_DPDK_SW_STATS_TYPE *sw_stats; > + rte_spinlock_t stats_lock; > + ); > + > + PADDED_MEMBERS(CACHE_LINE_SIZE, > + /* Configuration fields */ > + int requested_mtu; > + int requested_n_txq; > + int user_n_rxq; > + int requested_n_rxq; > + int requested_rxq_size; > + int requested_txq_size; > + int rxq_size; > + int txq_size; > + int requested_socket_id; > + struct rte_eth_fc_conf fc_conf; > + uint32_t hw_ol_features; > + bool requested_lsc_interrupt_mode; > + bool lsc_interrupt_mode; > + struct eth_addr requested_hwaddr; > + ); > + > + PADDED_MEMBERS(CACHE_LINE_SIZE, > + struct rte_eth_xstat_name *rte_xstats_names; > + int rte_xstats_names_size; > + int rte_xstats_ids_size; > + uint64_t *rte_xstats_ids; > + ); > +}; > + > +static inline struct netdev_dpdk_common * > +netdev_dpdk_common_cast(const struct netdev *netdev) > +{ > + return CONTAINER_OF(netdev, struct netdev_dpdk_common, up); > +} > + > +#endif /* NETDEV_DPDK_PRIVATE_H */ > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 54959ff0d..e34e96dd3 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -17,6 +17,14 @@ > #include <config.h> > #include "netdev-dpdk.h" > > +#include "openvswitch/thread.h" > + > +#define NETDEV_DPDK_TX_Q_TYPE struct dpdk_tx_queue > +#define NETDEV_DPDK_SW_STATS_TYPE struct netdev_dpdk_sw_stats > +static struct ovs_mutex dpdk_mutex; > +#define NETDEV_DPDK_GLOBAL_MUTEX dpdk_mutex > +#include "netdev-dpdk-private.h" > Move this block at the end of the include definition, making it a bit cleaner. @@ -84,6 +78,11 @@ static struct ovs_mutex dpdk_mutex; #include "util.h" #include "uuid.h" +#define NETDEV_DPDK_TX_Q_TYPE struct dpdk_tx_queue +#define NETDEV_DPDK_SW_STATS_TYPE struct netdev_dpdk_sw_stats +#define NETDEV_DPDK_GLOBAL_MUTEX dpdk_mutex +#include "netdev-dpdk-private.h" + > #include <errno.h> > #include <signal.h> > #include <stdint.h> > @@ -94,20 +102,6 @@ static bool per_port_memory = false; /* Status of per > port memory support */ > #define OVS_CACHE_LINE_SIZE CACHE_LINE_SIZE > #define OVS_VPORT_DPDK "ovs_dpdk" > > -/* > - * need to reserve tons of extra space in the mbufs so we can align the > - * DMA addresses to 4KB. > - * The minimum mbuf size is limited to avoid scatter behaviour and drop in > - * performance for standard Ethernet MTU. > - */ > -#define ETHER_HDR_MAX_LEN (RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN \ > - + (2 * VLAN_HEADER_LEN)) > -#define MTU_TO_FRAME_LEN(mtu) ((mtu) + RTE_ETHER_HDR_LEN + \ > - RTE_ETHER_CRC_LEN) > -#define MTU_TO_MAX_FRAME_LEN(mtu) ((mtu) + ETHER_HDR_MAX_LEN) > -#define FRAME_LEN_TO_MTU(frame_len) ((frame_len) \ > - - RTE_ETHER_HDR_LEN - RTE_ETHER_CRC_LEN) > -#define NETDEV_DPDK_MBUF_ALIGN 1024 > #define NETDEV_DPDK_MAX_PKT_LEN 9728 > > /* Max and min number of packets in the mempool. OVS tries to allocate a > @@ -117,7 +111,6 @@ static bool per_port_memory = false; /* Status of per > port memory support */ > > #define MAX_NB_MBUF (4096 * 64) > #define MIN_NB_MBUF (4096 * 4) > -#define MP_CACHE_SZ RTE_MEMPOOL_CACHE_MAX_SIZE > > /* MAX_NB_MBUF can be divided by 2 many times, until MIN_NB_MBUF */ > BUILD_ASSERT_DECL(MAX_NB_MBUF % ROUND_DOWN_POW2(MAX_NB_MBUF / MIN_NB_MBUF) > @@ -128,24 +121,11 @@ BUILD_ASSERT_DECL(MAX_NB_MBUF % > ROUND_DOWN_POW2(MAX_NB_MBUF / MIN_NB_MBUF) > BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF / MIN_NB_MBUF)) > % MP_CACHE_SZ == 0); [...] _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
