On 15 Oct 2025, at 16:59, Eli Britstein wrote:
> Currently netdev-dpdk class is not registered as other netdev classes, > as it requires global other_config configuration. Also, status update of > dpdk's initialized and version are updated via dedicated dpdk calls > directly from the bridge module. > Use the introduced infrastructure to change the registration to a > generic one. Also avoid dedicated dpdk calls from the bridge module. Hi Eli, I didn’t do a full review, just a quick browse through. And maybe I found the cause of your occasional problem? //Eelco > Signed-off-by: Eli Britstein <[email protected]> > --- > lib/dpdk.c | 3 - > lib/netdev-dpdk.c | 178 +++++++++++++++++++++++++----------------- > lib/netdev-dpdk.h | 1 - > lib/netdev-provider.h | 5 ++ > lib/netdev.c | 7 ++ > vswitchd/bridge.c | 4 +- > 6 files changed, 119 insertions(+), 79 deletions(-) > > diff --git a/lib/dpdk.c b/lib/dpdk.c > index 077bdfc09..19f147418 100644 > --- a/lib/dpdk.c > +++ b/lib/dpdk.c > @@ -506,9 +506,6 @@ dpdk_init__(const struct smap *ovs_other_config) > /* We are called from the main thread here */ > RTE_PER_LCORE(_lcore_id) = NON_PMD_CORE_ID; > > - /* Finally, register the dpdk classes */ > - netdev_dpdk_register(ovs_other_config); > - netdev_register_flow_api_provider(&netdev_offload_dpdk); > return true; > } > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 45b9f4896..bd4e4b471 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -2120,8 +2120,13 @@ static atomic_bool > netdev_dpdk_pending_reset[RTE_MAX_ETHPORTS]; > static void > netdev_dpdk_wait(const struct netdev_class *netdev_class OVS_UNUSED) > { > - uint64_t last_reset_seq = seq_read(netdev_dpdk_reset_seq); > + uint64_t last_reset_seq; > > + if (!dpdk_available()) { > + return; > + } > + > + last_reset_seq = seq_read(netdev_dpdk_reset_seq); > if (netdev_dpdk_last_reset_seq == last_reset_seq) { > seq_wait(netdev_dpdk_reset_seq, netdev_dpdk_last_reset_seq); > } else { > @@ -2132,8 +2137,13 @@ netdev_dpdk_wait(const struct netdev_class > *netdev_class OVS_UNUSED) > static void > netdev_dpdk_run(const struct netdev_class *netdev_class OVS_UNUSED) > { > - uint64_t reset_seq = seq_read(netdev_dpdk_reset_seq); > + uint64_t reset_seq; > + > + if (!dpdk_available()) { > + return; > + } > > + reset_seq = seq_read(netdev_dpdk_reset_seq); > if (reset_seq != netdev_dpdk_last_reset_seq) { > dpdk_port_t port_id; > > @@ -5209,58 +5219,6 @@ netdev_dpdk_get_vid(const struct netdev_dpdk *dev) > return ovsrcu_index_get(&dev->vid); > } > > -static int > -netdev_dpdk_class_init(void) > -{ > - static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; > - > - /* This function can be called for different classes. The initialization > - * needs to be done only once */ > - if (ovsthread_once_start(&once)) { > - int ret; > - > - ovs_thread_create("dpdk_watchdog", dpdk_watchdog, NULL); > - unixctl_command_register("netdev-dpdk/set-admin-state", > - "[netdev] up|down", 1, 2, > - netdev_dpdk_set_admin_state, NULL); > - > - unixctl_command_register("netdev-dpdk/detach", > - "pci address of device", 1, 1, > - netdev_dpdk_detach, NULL); > - > - unixctl_command_register("netdev-dpdk/get-mempool-info", > - "[netdev]", 0, 1, > - netdev_dpdk_get_mempool_info, NULL); > - Do we maybe want to keep the init() api to just do the command registration? > - netdev_dpdk_reset_seq = seq_create(); > - netdev_dpdk_last_reset_seq = seq_read(netdev_dpdk_reset_seq); > - ret = rte_eth_dev_callback_register(RTE_ETH_ALL, > - RTE_ETH_EVENT_INTR_RESET, > - dpdk_eth_event_callback, NULL); > - if (ret != 0) { > - VLOG_ERR("Ethernet device callback register error: %s", > - rte_strerror(-ret)); > - } > - > - ovsthread_once_done(&once); > - } > - > - return 0; > -} > - > -static int > -netdev_dpdk_vhost_class_init(void) > -{ > - static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; > - > - if (ovsthread_once_start(&once)) { > - ovs_thread_create("ovs_vhost", netdev_dpdk_vhost_events_main, NULL); > - ovsthread_once_done(&once); > - } We could just keep this in the class init function, as it requires no configuration. > - return 0; > -} > - > /* QoS Functions */ > > struct ingress_policer * > @@ -6827,6 +6785,93 @@ parse_vhost_config(const struct smap *ovs_other_config) > vhost_postcopy_enabled ? "enabled" : "disabled"); > } > > +static int > +netdev_dpdk_set_other_config(const struct smap *other_config) > +{ > + static struct ovsthread_once register_once = OVSTHREAD_ONCE_INITIALIZER; > + > + /* This function can be called for different classes. The initialization > + * needs to be done only once */ > + if (ovsthread_once_start(®ister_once)) { > + ovs_thread_create("dpdk_watchdog", dpdk_watchdog, NULL); > + unixctl_command_register("netdev-dpdk/set-admin-state", > + "[netdev] up|down", 1, 2, > + netdev_dpdk_set_admin_state, NULL); > + > + unixctl_command_register("netdev-dpdk/detach", > + "pci address of device", 1, 1, > + netdev_dpdk_detach, NULL); > + > + unixctl_command_register("netdev-dpdk/get-mempool-info", > + "[netdev]", 0, 1, > + netdev_dpdk_get_mempool_info, NULL); > + > + ovsthread_once_done(®ister_once); > + } > + > + dpdk_init(other_config); > + > + if (dpdk_available()) { > + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; > + int ret; > + > + if (!ovsthread_once_start(&once)) { > + return 0; > + } > + > + parse_mempool_config(other_config); > + parse_user_mempools_list(other_config); > + parse_vhost_config(other_config); > + > + ovsthread_once_done(&once); > + > + netdev_dpdk_reset_seq = seq_create(); > + netdev_dpdk_last_reset_seq = seq_read(netdev_dpdk_reset_seq); > + ret = rte_eth_dev_callback_register(RTE_ETH_ALL, > + RTE_ETH_EVENT_INTR_RESET, > + dpdk_eth_event_callback, NULL); > + if (ret != 0) { > + VLOG_ERR("Ethernet device callback register error: %s", > + rte_strerror(-ret)); > + } > + } Maybe we could move this part to dpdk_init()? > + > + return 0; > +} > + > +static int > +netdev_vhost_set_other_config(void) See earlier comment; I don’t think this is needed; however, for consistency, I would do the same here. netdev_vhost_set_other_config(const struct smap *other_config OVS_UNUSED) > +{ > + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; > + > + if (ovsthread_once_start(&once)) { > + ovs_thread_create("ovs_vhost", netdev_dpdk_vhost_events_main, NULL); > + ovsthread_once_done(&once); > + } > + > + return 0; > +} > + > +static int > +netdev_dpdk_common_set_other_config(const struct netdev_class *class, > + const struct smap *other_config) > +{ > + if (is_dpdk_class(class)) { Would both vhost and dpdk be matching this? 612 static bool 613 is_dpdk_class(const struct netdev_class *class) 614 { 615 return class->destruct == netdev_dpdk_destruct 616 || class->destruct == netdev_dpdk_vhost_destruct; 617 } 618 > + return netdev_dpdk_set_other_config(other_config); > + } else { > + return netdev_vhost_set_other_config(); > + } > + > + OVS_NOT_REACHED(); > + return 0; > +} > + > +static void > +netdev_dpdk_class_status(const struct ovsrec_open_vswitch *cfg) > +{ > + dpdk_status(cfg); > +} > + > #define NETDEV_DPDK_CLASS_COMMON \ > .is_pmd = true, \ > .alloc = netdev_dpdk_alloc, \ > @@ -6854,13 +6899,14 @@ parse_vhost_config(const struct smap > *ovs_other_config) > .rxq_alloc = netdev_dpdk_rxq_alloc, \ > .rxq_construct = netdev_dpdk_rxq_construct, \ > .rxq_destruct = netdev_dpdk_rxq_destruct, \ > - .rxq_dealloc = netdev_dpdk_rxq_dealloc > + .rxq_dealloc = netdev_dpdk_rxq_dealloc, \ > + .set_other_config = netdev_dpdk_common_set_other_config > > #define NETDEV_DPDK_CLASS_BASE \ > NETDEV_DPDK_CLASS_COMMON, \ > - .init = netdev_dpdk_class_init, \ > .run = netdev_dpdk_run, \ > .wait = netdev_dpdk_wait, \ > + .class_status = netdev_dpdk_class_status, \ > .destruct = netdev_dpdk_destruct, \ > .set_tx_multiq = netdev_dpdk_set_tx_multiq, \ > .get_carrier = netdev_dpdk_get_carrier, \ > @@ -6872,7 +6918,7 @@ parse_vhost_config(const struct smap *ovs_other_config) > .reconfigure = netdev_dpdk_reconfigure, \ > .rxq_recv = netdev_dpdk_rxq_recv > > -static const struct netdev_class dpdk_class = { > +const struct netdev_class dpdk_class = { > .type = "dpdk", > NETDEV_DPDK_CLASS_BASE, > .construct = netdev_dpdk_construct, > @@ -6881,10 +6927,9 @@ static const struct netdev_class dpdk_class = { > .send = netdev_dpdk_eth_send, > }; > > -static const struct netdev_class dpdk_vhost_class = { > +const struct netdev_class dpdk_vhost_class = { > .type = "dpdkvhostuser", > NETDEV_DPDK_CLASS_COMMON, > - .init = netdev_dpdk_vhost_class_init, > .construct = netdev_dpdk_vhost_construct, > .destruct = netdev_dpdk_vhost_destruct, > .send = netdev_dpdk_vhost_send, > @@ -6897,10 +6942,9 @@ static const struct netdev_class dpdk_vhost_class = { > .rxq_enabled = netdev_dpdk_vhost_rxq_enabled, > }; > > -static const struct netdev_class dpdk_vhost_client_class = { > +const struct netdev_class dpdk_vhost_client_class = { > .type = "dpdkvhostuserclient", > NETDEV_DPDK_CLASS_COMMON, > - .init = netdev_dpdk_vhost_class_init, > .construct = netdev_dpdk_vhost_client_construct, > .destruct = netdev_dpdk_vhost_destruct, > .get_config = netdev_dpdk_vhost_client_get_config, > @@ -6914,15 +6958,3 @@ static const struct netdev_class > dpdk_vhost_client_class = { > .rxq_recv = netdev_dpdk_vhost_rxq_recv, > .rxq_enabled = netdev_dpdk_vhost_rxq_enabled, > }; > - > -void > -netdev_dpdk_register(const struct smap *ovs_other_config) > -{ > - parse_mempool_config(ovs_other_config); > - parse_user_mempools_list(ovs_other_config); > - parse_vhost_config(ovs_other_config); > - > - netdev_register_provider(&dpdk_class); > - netdev_register_provider(&dpdk_vhost_class); > - netdev_register_provider(&dpdk_vhost_client_class); > -} > diff --git a/lib/netdev-dpdk.h b/lib/netdev-dpdk.h > index 86df7a1e8..1cbee1d80 100644 > --- a/lib/netdev-dpdk.h > +++ b/lib/netdev-dpdk.h > @@ -29,7 +29,6 @@ struct netdev; > > #include <rte_flow.h> > > -void netdev_dpdk_register(const struct smap *); > void free_dpdk_buf(struct dp_packet *); > > bool netdev_dpdk_flow_api_supported(struct netdev *); > diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h > index afbb1912d..f4f82ec92 100644 > --- a/lib/netdev-provider.h > +++ b/lib/netdev-provider.h > @@ -882,6 +882,11 @@ extern const struct netdev_class netdev_tap_class; > extern const struct netdev_class netdev_afxdp_class; > extern const struct netdev_class netdev_afxdp_nonpmd_class; > #endif > +#ifdef DPDK_NETDEV > +extern const struct netdev_class dpdk_class; > +extern const struct netdev_class dpdk_vhost_class; > +extern const struct netdev_class dpdk_vhost_client_class; > +#endif > #ifdef __cplusplus > } > #endif > diff --git a/lib/netdev.c b/lib/netdev.c > index bc8406966..b8e4efd78 100644 > --- a/lib/netdev.c > +++ b/lib/netdev.c > @@ -169,6 +169,13 @@ netdev_initialize(void) > netdev_register_provider(&netdev_windows_class); > netdev_register_provider(&netdev_internal_class); > netdev_vport_tunnel_register(); > +#endif > +#ifdef DPDK_NETDEV > + netdev_register_provider(&dpdk_class); > + netdev_register_provider(&dpdk_vhost_class); > + netdev_register_provider(&dpdk_vhost_client_class); > + > + netdev_register_flow_api_provider(&netdev_offload_dpdk); > #endif > ovsthread_once_done(&once); > } > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > index 456b784c0..ac36f31ea 100644 > --- a/vswitchd/bridge.c > +++ b/vswitchd/bridge.c > @@ -3261,7 +3261,7 @@ run_status_update(void) > > connectivity_seqno = seq; > status_txn = ovsdb_idl_txn_create(idl); > - dpdk_status(cfg); > + netdev_class_status(cfg); > HMAP_FOR_EACH (br, node, &all_bridges) { > struct port *port; > > @@ -3398,7 +3398,7 @@ bridge_run(void) > > if (cfg) { > netdev_set_flow_api_enabled(&cfg->other_config); > - dpdk_init(&cfg->other_config); > + netdev_set_other_config(&cfg->other_config); > userspace_tso_init(&cfg->other_config); > } > > -- > 2.34.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
