TodoWrite: I need to analyze this patch for regressions and create a comprehensive report.
git sha: ec9c42df3c3369f1d7f213efa5f67b6e80dc32f7 Author: Eelco Chaudron <[email protected]> dpif-offload: Add offload provider set_config API. This patch introduces an API that allows offload providers to manage global configurations. It also moves the 'n-offload-threads' and 'tc-policy' setting to the appropriate providers using this new API. > diff --git a/lib/dpif-offload-dpdk.c b/lib/dpif-offload-dpdk.c > index 03c8f6492..6ac4af8d8 100644 > --- a/lib/dpif-offload-dpdk.c > +++ b/lib/dpif-offload-dpdk.c > @@ -20,21 +20,84 @@ > #include "dpif-offload-provider.h" > #include "util.h" > > +#include "openvswitch/vlog.h" > + > +VLOG_DEFINE_THIS_MODULE(dpif_offload_dpdk); > + > +#define DEFAULT_OFFLOAD_THREAD_NB 1 > +#define MAX_OFFLOAD_THREAD_NB 10 > + > +static unsigned int offload_thread_nb = DEFAULT_OFFLOAD_THREAD_NB; > + > +/* dpif offload interface for the dpdk rte_flow implementation. */ > +struct dpif_offload_dpdk { > + struct dpif_offload offload; > + > + /* Configuration specific variables. */ > + struct ovsthread_once once_enable; /* Track first-time enablement. */ > +}; > + > +static struct dpif_offload_dpdk * > +dpif_offload_dpdk_cast(const struct dpif_offload *offload) > +{ > + dpif_offload_assert_class(offload, &dpif_offload_dpdk_class); > + return CONTAINER_OF(offload, struct dpif_offload_dpdk, offload); > +} > + > static int > dpif_offload_dpdk_open(const struct dpif_offload_class *offload_class, > - struct dpif *dpif, struct dpif_offload **dpif_offload) > + struct dpif *dpif, struct dpif_offload **offload_) > { > - struct dpif_offload *offload = xmalloc(sizeof(struct dpif_offload)); > + struct dpif_offload_dpdk *offload; > + > + offload = xmalloc(sizeof(struct dpif_offload_dpdk)); Does this allocation handle failure cases properly when dpif_offload_init() fails below? > + > + dpif_offload_init(&offload->offload, offload_class, dpif); > + offload->once_enable = (struct ovsthread_once) > OVSTHREAD_ONCE_INITIALIZER; Would the once_enable initialization be cleaner using direct initialization rather than a cast? > + > + *offload_ = &offload->offload; > + return 0; > +} > [ ... ] > +static void > +dpif_offload_dpdk_set_config(struct dpif_offload *offload_, > + const struct smap *other_cfg) > +{ > + struct dpif_offload_dpdk *offload = dpif_offload_dpdk_cast(offload_); > + > + /* We maintain the existing behavior where global configurations > + * are only accepted when hardware offload is initially enabled. > + * Once enabled, they cannot be updated or reconfigured. */ > + if (smap_get_bool(other_cfg, "hw-offload", false)) { > + if (ovsthread_once_start(&offload->once_enable)) { > + > + offload_thread_nb = smap_get_ullong(other_cfg, > + "n-offload-threads", > + DEFAULT_OFFLOAD_THREAD_NB); Is the global variable offload_thread_nb thread-safe when accessed from multiple threads during configuration updates? > + if (offload_thread_nb == 0 || > + offload_thread_nb > MAX_OFFLOAD_THREAD_NB) { > + VLOG_WARN("netdev: Invalid number of threads requested: %u", > + offload_thread_nb); > + offload_thread_nb = DEFAULT_OFFLOAD_THREAD_NB; > + } > [ ... ] > +/* XXX: Temporary functions below, which will be removed once fully > + * refactored. */ > +unsigned int dpif_offload_dpdk_get_thread_nb(void); > +unsigned int dpif_offload_dpdk_get_thread_nb(void) > +{ > + return offload_thread_nb; > +} Can this temporary function cause race conditions when dpif_offload_dpdk_get_thread_nb() is called while dpif_offload_dpdk_set_config() is modifying offload_thread_nb? > diff --git a/lib/dpif-offload-tc.c b/lib/dpif-offload-tc.c > index f5d62566e..c09530daa 100644 > --- a/lib/dpif-offload-tc.c > +++ b/lib/dpif-offload-tc.c > @@ -19,22 +19,65 @@ > #include "dpif-offload.h" > #include "dpif-offload-provider.h" > #include "util.h" > +#include "tc.h" > + > +/* dpif offload interface for the tc implementation. */ > +struct dpif_offload_tc { > + struct dpif_offload offload; > + > + /* Configuration specific variables. */ > + struct ovsthread_once once_enable; /* Track first-time enablement. */ > +}; > [ ... ] > + offload_tc->once_enable = (struct ovsthread_once) \ > + OVSTHREAD_ONCE_INITIALIZER; Does this line continuation backslash introduce unnecessary complexity compared to keeping the assignment on one line? > [ ... ] > +static void > +dpif_offload_tc_set_config(struct dpif_offload *offload, > + const struct smap *other_cfg) > +{ > + struct dpif_offload_tc *offload_tc = dpif_offload_tc_cast(offload); > + > + /* We maintain the existing behavior where global configurations > + * are only accepted when hardware offload is initially enabled. > + * Once enabled, they cannot be updated or reconfigured. */ > + if (smap_get_bool(other_cfg, "hw-offload", false)) { > + if (ovsthread_once_start(&offload_tc->once_enable)) { > + > + tc_set_policy(smap_get_def(other_cfg, "tc-policy", > + TC_POLICY_DEFAULT)); Can tc_set_policy() fail and if so, should error handling be added here? > diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c > index 01fdadbc3..0fad1b983 100644 > --- a/lib/netdev-offload.c > +++ b/lib/netdev-offload.c > @@ -61,10 +61,16 @@ VLOG_DEFINE_THIS_MODULE(netdev_offload); > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > > -#define DEFAULT_OFFLOAD_THREAD_NB 1 > +/* XXX: Temporarily duplicates definition in dpif-offload-dpdk.c. */ > #define MAX_OFFLOAD_THREAD_NB 10 > - > +#define DEFAULT_OFFLOAD_THREAD_NB 1 > static unsigned int offload_thread_nb = DEFAULT_OFFLOAD_THREAD_NB; Does this duplication of constants between netdev-offload.c and dpif-offload-dpdk.c create maintenance issues if values need to change? > + > +unsigned int dpif_offload_dpdk_get_thread_nb(void); /* XXX: Temporarily > + * external declaration > + * until fully > refactored. > + */ Should this external declaration be in a header file instead of embedded in the source file? > @@ -853,37 +859,18 @@ netdev_ports_flow_init(void) > } > > void > -netdev_set_flow_api_enabled(const struct smap *ovs_other_config) > +netdev_set_flow_api_enabled(const struct smap *ovs_other_config OVS_UNUSED) Is the OVS_UNUSED annotation correct here when ovs_other_config is still used in the conditional compilation block below? > [ ... ] > +#ifdef DPDK_NETDEV > + offload_thread_nb = dpif_offload_dpdk_get_thread_nb(); > +#endif Can this conditional compilation cause offload_thread_nb to remain uninitialized when DPDK_NETDEV is not defined? _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
