On 14 Sep 2026, at 19:04, Kevin Traynor wrote:
> 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.
Thanks, Kevin, for the review. I definitely need the ACK from Gaetan,
in the meantime, I'll send out a v2 with your comments addressed (see
also below).
//Eelco
>> 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);
> }
Thanks, this is way better ;)
>> 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.
This is not the wrong offload class, the same function handles both
'dummy' and 'dummy_x'. In the 'dummy_x' case we simply set ctx to
NULL to indicate there is no context needed.
>> + 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.
I agree the name is not ideal, but it reflects what is being managed
rather than a single action, as the callback covers the full thread
lifetime: initialization, reinitialization, and exit. I'll keep it
as is for now.
> Do we need the offload as a param, when the function will be called per
> offload ?
Yes, it is needed. The same callback function is registered for all
offload providers that implement it. The 'dpif_offload' pointer allows
the provider to identify which offload instance is calling it. This
follows the same pattern used in 'dpif-provider.h', where 'const struct
dpif *' is the first argument to every provider callback.
>> };
>> 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)
We can use FIXME: or XXX, will use XXX as it's what is used most in
this file.
>> + 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
Agreed, the guard is not needed as memcpy() with zero will be a nop.
Will keep it for readability as it makes the intent explicit.
>> + 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
Will remove the guard here as the for loop makes it explicit and it
saves an indentation level.
>> + /* 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
Your correct, will remove it.
>> + }
>> + }
>> +}
>> +
>> +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
The numa_id was added assuming DOCA might need it, so will keep it for
now unless Gaetan confirms it is not needed.
>> +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
