On 9/10/26 14:10, Eelco Chaudron wrote:
This patch adds support for offload providers to hook into PMD thread
lifecycle events, specifically thread initialization, reinitialization,
and termination. The new pmd_thread_lifecycle() callback in
dpif_offload_class allows a provider to maintain per-PMD-thread context
across reloads and to release resources when a thread exits.
Thanks Eelco. Code operation looks good, a few minor comments on it.
It will need an Ack from Gaetan to confirm that it meets his requirements.
Signed-off-by: Eelco Chaudron <[email protected]>
---
lib/dpif-netdev.c | 25 ++++++++
lib/dpif-offload-dummy.c | 22 +++++++
lib/dpif-offload-provider.h | 14 +++++
lib/dpif-offload.c | 108 ++++++++++++++++++++++++++++++++++
lib/dpif-offload.h | 8 +++
tests/pmd.at | 25 ++++++++
utilities/checkpatch_dict.txt | 3 +
7 files changed, 205 insertions(+)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 4151ea056..b15552934 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -331,6 +331,9 @@ struct dp_netdev {
uint64_t last_reconfigure_seq;
struct ovsthread_once once_set_config;
+ /* When a reconfigure is requested, forcefully reload all PMDs. */
+ bool force_pmd_reload;
+
/* Cpu mask for pin of pmd threads. */
char *pmd_cmask;
@@ -341,6 +344,7 @@ struct dp_netdev {
struct conntrack *conntrack;
struct pmd_auto_lb pmd_alb;
+ bool offload_enabled;
/* Bonds. */
struct ovs_mutex bond_mutex; /* Protects updates of 'tx_bonds'. */
@@ -4061,6 +4065,14 @@ dpif_netdev_set_config(struct dpif *dpif, const struct
smap *other_config)
log_all_pmd_sleeps(dp);
}
+ if (!dp->offload_enabled) {
+ dp->offload_enabled = dpif_offload_enabled();
+ if (dp->offload_enabled) {
+ dp->force_pmd_reload = true;
+ dp_netdev_request_reconfigure(dp);
+ }
+ }
+
You could simplify a little,
if (!dp->offload_enabled && dpif_offload_enabled()) {
dp->offload_enabled = true;
dp->force_pmd_reload = true;
dp_netdev_request_reconfigure(dp);
}
return 0;
}
@@ -5769,6 +5781,14 @@ reconfigure_datapath(struct dp_netdev *dp)
ovs_mutex_unlock(&pmd->port_mutex);
}
+ /* Do we need to forcefully reload all threads? */
+ if (dp->force_pmd_reload) {
+ CMAP_FOR_EACH (pmd, node, &dp->poll_threads) {
+ pmd->need_reload = true;
+ }
+ dp->force_pmd_reload = false;
+ }
+
/* Reload affected pmd threads. */
reload_affected_pmds(dp);
@@ -6054,6 +6074,7 @@ pmd_thread_main(void *f_)
{
struct dp_netdev_pmd_thread *pmd = f_;
struct pmd_perf_stats *s = &pmd->perf_stats;
+ struct dpif_offload_pmd_ctx *offload_ctx = NULL;
unsigned int lc = 0;
struct polled_queue *poll_list;
bool wait_for_reload = false;
@@ -6087,6 +6108,9 @@ reload:
dpdk_attached = dpdk_attach_thread(pmd->core_id);
}
+ dpif_offload_pmd_thread_reload(pmd->dp->full_name, pmd->core_id,
+ pmd->numa_id, &offload_ctx);
+
/* List port/core affinity */
for (i = 0; i < poll_cnt; i++) {
VLOG_DBG("Core %d processing port \'%s\' with queue-id %d\n",
@@ -6246,6 +6270,7 @@ reload:
goto reload;
}
+ dpif_offload_pmd_thread_exit(offload_ctx);
pmd_free_static_tx_qid(pmd);
dfc_cache_uninit(&pmd->flow_cache);
free(poll_list);
diff --git a/lib/dpif-offload-dummy.c b/lib/dpif-offload-dummy.c
index 878276a94..99f2b3733 100644
--- a/lib/dpif-offload-dummy.c
+++ b/lib/dpif-offload-dummy.c
@@ -1146,6 +1146,27 @@ dummy_netdev_hw_offload_run(struct netdev *netdev)
}
}
+static void
+dummy_pmd_thread_lifecycle(const struct dpif_offload *dpif_offload, bool exit,
+ unsigned core_id, int numa_id, void **ctx)
+{
+ /* Only do this for the 'dummy' class, not for 'dummy_x'. */
+ if (strcmp(dpif_offload_type(dpif_offload), "dummy")) {
+ *ctx = NULL;
I don't think it will happen, but if it's somehow the wrong offload, I
don't think we should do anything with ctx.
+ return;
+ }
+
+ VLOG_DBG(
+ "pmd_thread_lifecycle; exit=%s, core=%u, numa=%d, ctx=%p",
+ exit ? "true" : "false", core_id, numa_id, *ctx);
+
+ if (exit) {
+ free(*ctx);
+ } else {
+ *ctx = *ctx ? *ctx : xstrdup("DUMMY_OFFLOAD_WORK");
+ }
+}
+
#define DEFINE_DPIF_DUMMY_CLASS(NAME, TYPE_STR) \
struct dpif_offload_class NAME = { \
.type = TYPE_STR, \
@@ -1166,6 +1187,7 @@ dummy_netdev_hw_offload_run(struct netdev *netdev)
.netdev_flow_del = dummy_flow_del, \
.netdev_flow_stats = dummy_flow_stats, \
.register_flow_unreference_cb = dummy_register_flow_unreference_cb, \
+ .pmd_thread_lifecycle = dummy_pmd_thread_lifecycle \
}
DEFINE_DPIF_DUMMY_CLASS(dpif_offload_dummy_class, "dummy");
diff --git a/lib/dpif-offload-provider.h b/lib/dpif-offload-provider.h
index 444b13138..0b098f6c1 100644
--- a/lib/dpif-offload-provider.h
+++ b/lib/dpif-offload-provider.h
@@ -318,6 +318,20 @@ struct dpif_offload_class {
* to netdev_flow_put() is no longer held by the offload provider. */
void (*register_flow_unreference_cb)(const struct dpif_offload *,
dpif_offload_flow_unreference_cb *);
+
+ /* The API below is specific to PMD (userspace) thread lifecycle handling.
+ *
+ * The lifecycle hook may be invoked multiple times for the same PMD
+ * thread. For example, when the thread is reinitialized, this function
+ * will be called again and the previous 'ctx' value will be passed back
+ * in. It is the provider's responsibility to decide whether it should
+ * be reused, replaced, or cleaned up before storing the new 'ctx' value.
+ *
+ * When the PMD thread is terminating, this API is called with
+ * 'exit == true'. At that point, the provider must release any resources
+ * associated with the previously returned 'ctx'. */
+ void (*pmd_thread_lifecycle)(const struct dpif_offload *, bool exit,
+ unsigned core_id, int numa_id, void **ctx);
Not sure about the naming. "lifecycle" is not a verb and I don't feel it
describes much. Then again, I can't think of a good suggestion.
Do we need the offload as a param, when the function will be called per
offload ?
};
extern struct dpif_offload_class dpif_offload_dummy_class;
diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c
index 04dabc42c..9b0083200 100644
--- a/lib/dpif-offload.c
+++ b/lib/dpif-offload.c
@@ -54,6 +54,7 @@ static const struct dpif_offload_class
*base_dpif_offload_classes[] = {
&dpif_offload_dummy_x_class,
};
+#define TOTAL_PROVIDERS ARRAY_SIZE(base_dpif_offload_classes)
#define DEFAULT_PROVIDER_PRIORITY_LIST "tc,dpdk,dummy,dummy_x"
static char *dpif_offload_provider_priority_list = NULL;
@@ -1693,3 +1694,110 @@ dpif_offload_port_mgr_port_count(const struct
dpif_offload *offload)
return cmap_count(&offload->ports->odp_port_to_port);
}
+
+struct dpif_offload_pmd_ctx_node {
+ const struct dpif_offload *offload;
+ void *provider_ctx;
+};
+
+struct dpif_offload_pmd_ctx {
+ unsigned core_id;
+ int numa_id;
+ size_t n_nodes;
+ struct dpif_offload_pmd_ctx_node nodes[TOTAL_PROVIDERS];
+};
+
+void
+dpif_offload_pmd_thread_reload(const char *dpif_name, unsigned core_id,
+ int numa_id, struct dpif_offload_pmd_ctx **ctx_)
+{
+ struct dpif_offload_pmd_ctx_node old_nodes[TOTAL_PROVIDERS];
+ struct dpif_offload_provider_collection *collection;
+ struct dpif_offload_pmd_ctx *ctx;
+ struct dpif_offload *offload;
+ size_t old_n_nodes = 0;
+
+ if (!dpif_offload_enabled()) {
+ ovs_assert(!*ctx_);
+ return;
+ }
+
+ ovs_mutex_lock(&dpif_offload_mutex);
+ collection = shash_find_data(&dpif_offload_providers, dpif_name);
+ ovs_mutex_unlock(&dpif_offload_mutex);
+
+ if (OVS_UNLIKELY(!collection)) {
+ ovs_assert(!*ctx_);
+ return;
+ }
+
+ if (!*ctx_) {
+ /* Would be nice if we have a numa specific xzalloc(). */
If you want to keep a note, then probably a FIXME or TODO (I didn't
check Coding stds)
+ ctx = xzalloc(sizeof *ctx);
+ ctx->core_id = core_id;
+ ctx->numa_id = numa_id;
+ *ctx_ = ctx;
+ } else {
+ ctx = *ctx_;
+ old_n_nodes = ctx->n_nodes;
+
+ if (old_n_nodes) {
The guard is not really needed but you may want to keep for readability
+ memcpy(old_nodes, ctx->nodes, old_n_nodes * sizeof old_nodes[0]);
+ }
+
+ /* Reset active nodes array. */
+ memset(ctx->nodes, 0, sizeof ctx->nodes);
+ ctx->n_nodes = 0;
+ }
+
+ LIST_FOR_EACH (offload, dpif_list_node, &collection->list) {
+
+ ovs_assert(ctx->n_nodes < TOTAL_PROVIDERS);
+
+ if (!offload->class->pmd_thread_lifecycle) {
+ continue;
+ }
+
+ if (old_n_nodes) {
Similar to above, guard not strictly needed but you may want to keep anyway
+ /* If this is a reload, try to find previous context. */
+ for (size_t i = 0; i < old_n_nodes; i++) {
+ struct dpif_offload_pmd_ctx_node *node = &old_nodes[i];
+
+ if (offload == node->offload) {
+ ctx->nodes[ctx->n_nodes].provider_ctx = node->provider_ctx;
+ break;
+ }
+ }
+ }
+
+ offload->class->pmd_thread_lifecycle(
+ offload, false, core_id, numa_id,
+ &ctx->nodes[ctx->n_nodes].provider_ctx);
+
+ if (ctx->nodes[ctx->n_nodes].provider_ctx) {
+ ctx->nodes[ctx->n_nodes].offload = offload;
+ ctx->n_nodes++;
+ } else {
+ memset(&ctx->nodes[ctx->n_nodes], 0,
+ sizeof ctx->nodes[ctx->n_nodes]);
This branch doesn't seem to be needed as provider_ctx is just checked
and nothing will have written to offload field
+ }
+ }
+}
+
+void
+dpif_offload_pmd_thread_exit(struct dpif_offload_pmd_ctx *ctx)
+{
+ if (!ctx) {
+ return;
+ }
+
+ for (size_t i = 0; i < ctx->n_nodes; i++) {
+ struct dpif_offload_pmd_ctx_node *node = &ctx->nodes[i];
+
+ node->offload->class->pmd_thread_lifecycle(node->offload, true,
+ ctx->core_id, ctx->numa_id,
+ &node->provider_ctx);
+ }
+
+ free(ctx);
+}
diff --git a/lib/dpif-offload.h b/lib/dpif-offload.h
index bf7643320..b6d3c1f37 100644
--- a/lib/dpif-offload.h
+++ b/lib/dpif-offload.h
@@ -189,4 +189,12 @@ dpif_offload_datapath_flow_op_continue(struct
dpif_offload_flow_cb_data *cb,
}
}
+/* PMD Thread helper functions. */
+struct dpif_offload_pmd_ctx;
+
+void dpif_offload_pmd_thread_reload(const char *dpif_name,
+ unsigned core_id, int numa_id,
+ struct dpif_offload_pmd_ctx **);
Is NUMA needed for something ? if in doubt better to keep
+void dpif_offload_pmd_thread_exit(struct dpif_offload_pmd_ctx *);
+
#endif /* DPIF_OFFLOAD_H */
diff --git a/tests/pmd.at b/tests/pmd.at
index 66fccc1d5..e315bc3a7 100644
--- a/tests/pmd.at
+++ b/tests/pmd.at
@@ -1599,3 +1599,28 @@
recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst=10.1.2.
OVS_VSWITCHD_STOP
AT_CLEANUP
+
+AT_SETUP([PMD - offload thread lifecycle])
+OVS_VSWITCHD_START([], [], [], [DUMMY_NUMA],
+ [-- set Open_vSwitch . other_config:hw-offload=true])
+
+AT_CHECK([ovs-appctl vlog/set dpif_offload_dummy:dbg])
+AT_CHECK([ovs-vsctl add-port br0 p0 -- set Interface p0 type=dummy-pmd])
+
+CHECK_CPU_DISCOVERED()
+CHECK_PMD_THREADS_CREATED()
+
+OVS_VSWITCHD_STOP
+
+LOG="$(sed -n 's/.*\(pmd_thread_lifecycle.*\)/\1/p' ovs-vswitchd.log)"
+CTX=$(echo "$LOG" | sed -n '2p' | sed -n 's/.*ctx=\(.*\)$/\1/p')
+
+AT_CHECK([echo "$LOG" | sed -n '1p' | sed 's/(nil)/0x0/g'], [0], [dnl
+pmd_thread_lifecycle; exit=false, core=0, numa=0, ctx=0x0
+])
+AT_CHECK([echo "$LOG" | sed -n '2p' \
+ | grep -q "exit=false, core=0, numa=0, ctx=$CTX"])
+AT_CHECK([echo "$LOG" | sed -n '$p' \
+ | grep -q "exit=true, core=0, numa=0, ctx=$CTX"])
+
+AT_CLEANUP
diff --git a/utilities/checkpatch_dict.txt b/utilities/checkpatch_dict.txt
index cb25a86ea..acdaf2c74 100644
--- a/utilities/checkpatch_dict.txt
+++ b/utilities/checkpatch_dict.txt
@@ -35,6 +35,7 @@ cpu
cpus
cstime
csum
+ctx
cutime
cvlan
datapath
@@ -45,6 +46,7 @@ decap
decapsulation
defrag
defragment
+deinitialization
deref
dereference
dest
@@ -233,6 +235,7 @@ rebased
recirc
recirculation
recirculations
+reinitialization
revalidate
revalidation
revalidator
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
