On 13 Jan 2026, at 12:46, Ilya Maximets wrote:
> Hi, Eelco. Thanks for working on this! Sorry, I still didn't finish my > review of the whole set and there are few questions that I can't answer > for myself yet before I have a complete picture. I'm more than a half way > though and the set seems good in general, but I have a few (mostly style > and naming) comments that I can share right now. See them inline. > I expect to complete my review and post everything else I found by the end > of today. > > Note: I've been writing this email for way too long so some comments may > contradict each other. :) But I hope they make sense in general. Thanks Ilya, I’ll start working on your comments... See inlines below! //Eelco > Best regards, Ilya Maximets. > > On 1/12/26 12:20 PM, Eelco Chaudron wrote: >> This commit introduces the initial framework and APIs to support >> the hardware offload dpif provider. >> >> Acked-by: Eli Britstein <elibr.nvidia.com> > > The email address is not correct here. Same in all other patches. Noticed it, will update all the patches in v6. >> Signed-off-by: Eelco Chaudron <[email protected]> >> >> --- >> v2 changes: >> - Fixed indentation issues. >> >> v3 changes: >> - Fixed include order. >> - Use the actual variable to determine the size for xmalloc(). >> - Moved ovsthread_once_start() to dp_offload_initialize() from >> patch 4 to here. >> >> v5 changes: >> - Add comment to dpif_offload_attach_dp_offload() related to >> reference counting. >> --- >> lib/automake.mk | 4 + >> lib/dpif-offload-dummy.c | 52 +++++ >> lib/dpif-offload-provider.h | 98 +++++++++ >> lib/dpif-offload.c | 397 ++++++++++++++++++++++++++++++++++++ >> lib/dpif-offload.h | 59 ++++++ >> lib/dpif-provider.h | 7 + >> lib/dpif.c | 7 + >> ofproto/ofproto-dpif.c | 70 +++++++ >> tests/ofproto-dpif.at | 21 ++ >> 9 files changed, 715 insertions(+) >> create mode 100644 lib/dpif-offload-dummy.c >> create mode 100644 lib/dpif-offload-provider.h >> create mode 100644 lib/dpif-offload.c >> create mode 100644 lib/dpif-offload.h >> >> diff --git a/lib/automake.mk b/lib/automake.mk >> index 78d6e6516..314102ecc 100644 >> --- a/lib/automake.mk >> +++ b/lib/automake.mk >> @@ -143,6 +143,10 @@ lib_libopenvswitch_la_SOURCES = \ >> lib/dpif-netdev-private.h \ >> lib/dpif-netdev-perf.c \ >> lib/dpif-netdev-perf.h \ >> + lib/dpif-offload.c \ >> + lib/dpif-offload.h \ >> + lib/dpif-offload-dummy.c \ >> + lib/dpif-offload-provider.h \ >> lib/dpif-provider.h \ >> lib/dpif.c \ >> lib/dpif.h \ >> diff --git a/lib/dpif-offload-dummy.c b/lib/dpif-offload-dummy.c >> new file mode 100644 >> index 000000000..4ff63bdb6 >> --- /dev/null >> +++ b/lib/dpif-offload-dummy.c >> @@ -0,0 +1,52 @@ >> +/* >> + * Copyright (c) 2025 Red Hat, Inc. >> + * >> + * 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. >> + */ >> + >> +#include <config.h> >> + >> +#include "dpif.h" >> +#include "dpif-offload.h" >> +#include "dpif-offload-provider.h" >> +#include "util.h" >> + >> +static int >> +dpif_offload_dummy_open(const struct dpif_offload_class *offload_class, >> + struct dpif *dpif, struct dpif_offload >> **dpif_offload) >> +{ >> + struct dpif_offload *offload = xmalloc(sizeof *offload); >> + >> + dpif_offload_init(offload, offload_class, dpif); >> + *dpif_offload = offload; >> + return 0; >> +} >> + >> +static void >> +dpif_offload_dummy_close(struct dpif_offload *dpif_offload) >> +{ >> + free(dpif_offload); >> +} >> + >> +#define DEFINE_DPIF_DUMMY_CLASS(NAME, TYPE_STR) \ >> + struct dpif_offload_class NAME = { \ >> + .type = TYPE_STR, \ >> + .supported_dpif_types = (const char *const[]) { \ >> + "dummy", \ >> + NULL}, \ > > nit: I'd suggest either writing this in a single line or put the closing > braces on a separate line. ACK will move them to a single line. >> + .open = dpif_offload_dummy_open, \ >> + .close = dpif_offload_dummy_close, \ >> + } >> + >> +DEFINE_DPIF_DUMMY_CLASS(dpif_offload_dummy_class, "dummy"); >> +DEFINE_DPIF_DUMMY_CLASS(dpif_offload_dummy_x_class, "dummy_x"); >> diff --git a/lib/dpif-offload-provider.h b/lib/dpif-offload-provider.h >> new file mode 100644 >> index 000000000..ca8b68574 >> --- /dev/null >> +++ b/lib/dpif-offload-provider.h >> @@ -0,0 +1,98 @@ >> +/* >> + * Copyright (c) 2025 Red Hat, Inc. >> + * >> + * 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. >> + */ >> + >> +#ifndef DPIF_OFFLOAD_PROVIDER_H >> +#define DPIF_OFFLOAD_PROVIDER_H >> + >> +#include "ovs-thread.h" >> + >> +#include "openvswitch/list.h" >> + >> +/* The DPIF Offload Provider introduces an abstraction layer for hardware >> + * offload functionality implemented at the netdevice level. It sits above >> + * the netdevice layer within the DPIF (Datapath Interface) framework, >> + * providing a standardized API for offloading packet processing tasks to >> + * hardware-accelerated datapaths. >> + * >> + * By decoupling hardware-specific implementations from the core DPIF layer, >> + * this abstraction enables greater flexibility, maintainability, and >> support >> + * for multiple hardware offload mechanisms without directly modifying DPIF >> + * internals. */ >> + >> +/* DPIF Offload specific structure pointed to in struct dpif. */ >> +struct dp_offload { > > Having both dp_offload and dpif_offload creates a very confusing naming > scheme for all the functions that operate on these structures. The > 'dp_offload' name itself also tells nothing about purpose of this structure. > > As far as I can tell this is just a collection of offload providers, so > maybe we should call it that way? e.g. 'dpif_offload_provider_collection'? > Some lines may become a little longer, but it seems like a good trade off > to make. Otherwise the code is very hard to read. I'll sprincle some > name suggestions below for all other stuff that depends on this function. > Most of them don't have too be too long as they are private functions anyway. Good feedback, will change it too dpif_offload_provider_collection; >> + char *dpif_name; /* Name of the associated dpif. */ >> + >> + struct ovs_list offload_providers; /* Note that offload providers will > > This can just be called 'list', in case the structure name is > self-explainatory. Done. >> + * only be added at dpif creation >> time >> + * and removed during destruction. >> + * No intermediate additions or >> + * deletions are allowed; hence no >> + * locking of the list is required. >> */ >> + >> + struct ovs_mutex offload_mutex; /* Mutex to protect all below. */ > > And this can just be called 'mutex'. Done. >> + struct ovs_refcount ref_cnt; >> +}; >> + >> +/* This structure should be treated as opaque by dpif offload >> implementations. >> + */ >> +struct dpif_offload { >> + const struct dpif_offload_class *class; >> + struct ovs_list dpif_list_node; >> + char *name; > > Do we actually need a 'name' in this structure? IIUC, the only use is the > duplicate detection, but we can use the class type for this, no? As we can't > have two providers of the same type in the collection anyway, AFAIU. > It's also used for logging, so maybe it's fine to keep it, but I'm not sure. It’s also used for dumping data, to know the dpif->offload mapping, as there is no offload-to-dpif mapping (as there will be multiple), so I would like to keep it. >> +}; >> + >> + >> +struct dpif_offload_class { >> + /* Type of DPIF offload provider in this class, e.g., "tc", "dpdk", >> + * "dummy", etc. */ >> + const char *type; >> + >> + /* List of DPIF implementation types supported by the offload provider. >> + * This is implemented as a pointer to a null-terminated list of const >> + * type strings. For more details on these type strings, see the > > Double spaces. Overall, throughout the set, comments and docs are > inconsistent > with use of double and single spacing. I will not flag all fo them, but if > you > ca go over and fix while working on the next version, that would be great. Ack, thought I did this already, but will re-check it once more :( >> + * 'struct dpif_class' definition. */ >> + const char *const *supported_dpif_types; >> + >> + /* Called when the dpif offload provider class is registered. Note that >> + * this is the global initialization, not the per dpif one. */ >> + int (*init)(void); >> + >> + /* Attempts to open the offload provider for the specified dpif. >> + * If successful, stores a pointer to the new dpif offload in >> + * 'dpif_offload **', which must be of class 'dpif_offload_class'. >> + * On failure (indicated by a negative return value), there are no >> + * requirements for what is stored in 'dpif_offload **'. */ >> + int (*open)(const struct dpif_offload_class *, >> + struct dpif *, struct dpif_offload **); >> + >> + /* Closes 'dpif_offload' and frees associated memory and resources. >> + * This includes freeing the 'dpif_offload' structure allocated by >> + * open() above. If your implementation accesses this provider using >> + * RCU pointers, it's responsible for handling deferred deallocation. */ >> + void (*close)(struct dpif_offload *); >> +}; >> + >> + >> +extern struct dpif_offload_class dpif_offload_dummy_class; >> +extern struct dpif_offload_class dpif_offload_dummy_x_class; >> + >> + >> +/* Global function, called by the dpif layer. */ >> +void dp_offload_initialize(void); > > Should this be dpif_offload_initialize() since we're initializing the module > and not the structure? Or dpif_offload_module_init() to avoid a mix up with > the dpif_offload_init() that initializes the dpif_offload structure. I used the dp_ naming as dpif does this internally also; however, I agree that dpif_offload_module_init() is more meaningful. Will change. >> +#endif /* DPIF_OFFLOAD_PROVIDER_H */ >> diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c >> new file mode 100644 >> index 000000000..e29df6d51 >> --- /dev/null >> +++ b/lib/dpif-offload.c >> @@ -0,0 +1,397 @@ >> +/* >> + * Copyright (c) 2025 Red Hat, Inc. >> + * >> + * 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. >> + */ >> + >> +#include <config.h> >> +#include <errno.h> >> + >> +#include "dpif-offload.h" >> +#include "dpif-offload-provider.h" >> +#include "dpif-provider.h" >> +#include "unixctl.h" >> +#include "util.h" >> +#include "openvswitch/dynamic-string.h" >> +#include "openvswitch/shash.h" >> +#include "openvswitch/vlog.h" >> + >> +VLOG_DEFINE_THIS_MODULE(dpif_offload); >> + >> +static struct ovs_mutex dpif_offload_mutex = OVS_MUTEX_INITIALIZER; >> +static struct shash dpif_offload_classes \ >> + OVS_GUARDED_BY(dpif_offload_mutex) = \ >> + SHASH_INITIALIZER(&dpif_offload_classes); >> +static struct shash dpif_offload_providers \ >> + OVS_GUARDED_BY(dpif_offload_mutex) = \ >> + SHASH_INITIALIZER(&dpif_offload_providers); >> + >> +static const struct dpif_offload_class *base_dpif_offload_classes[] = { >> + &dpif_offload_dummy_class, >> + &dpif_offload_dummy_x_class, >> +}; >> + >> +static int >> +dpif_offload_register_provider__(const struct dpif_offload_class *class) >> + OVS_REQUIRES(dpif_offload_mutex) >> +{ >> + int error; >> + >> + if (shash_find(&dpif_offload_classes, class->type)) { >> + VLOG_WARN("attempted to register duplicate dpif offload class: %s", >> + class->type); >> + return EEXIST; >> + } >> + >> + if (!class->supported_dpif_types) { >> + VLOG_WARN("attempted to register a dpif offload class without any " >> + "supported dpifs: %s", class->type); > > nit: s/dpifs/dpif types/ ACK >> + return EINVAL; >> + } >> + >> + error = class->init ? class->init() : 0; >> + if (error) { >> + VLOG_WARN("failed to initialize %s dpif offload class: %s", >> + class->type, ovs_strerror(error)); >> + return error; >> + } >> + >> + shash_add(&dpif_offload_classes, class->type, class); >> + return 0; >> +} >> + >> +static int >> +dpif_offload_register_provider(const struct dpif_offload_class *class) >> +{ >> + int error; >> + >> + ovs_mutex_lock(&dpif_offload_mutex); >> + error = dpif_offload_register_provider__(class); >> + ovs_mutex_unlock(&dpif_offload_mutex); >> + >> + return error; >> +} > > Do we need above two functions separate? I don't see the internal one > being used anywhere else. can probbaly be just embedded. I did this so the mutex handling is clean, so no goto’s etc. >> + >> +static void >> +dpif_offload_show_classes(struct unixctl_conn *conn, int argc OVS_UNUSED, >> + const char *argv[] OVS_UNUSED, void *aux >> OVS_UNUSED) >> +{ >> + const struct shash_node **list; >> + struct ds ds; >> + >> + ds_init(&ds); >> + ovs_mutex_lock(&dpif_offload_mutex); >> + >> + list = shash_sort(&dpif_offload_classes); >> + for (size_t i = 0; i < shash_count(&dpif_offload_classes); i++) { >> + const struct dpif_offload_class *class = list[i]->data; >> + >> + if (i == 0) { >> + ds_put_cstr(&ds, "Offload Class Supported dpif >> class(es)\n"); >> + ds_put_cstr(&ds, "---------------- >> ------------------------\n"); >> + } >> + >> + ds_put_format(&ds, "%-16s ", list[i]->name); >> + >> + for (size_t j = 0; class->supported_dpif_types[j] != NULL; j++) { >> + ds_put_format(&ds, "%*s%s\n", j == 0 ? 0 : 18, "", >> + class->supported_dpif_types[j]); >> + } >> + } >> + >> + ovs_mutex_unlock(&dpif_offload_mutex); >> + free(list); >> + >> + unixctl_command_reply(conn, ds_cstr(&ds)); >> + ds_destroy(&ds); >> +} >> + >> +void >> +dp_offload_initialize(void) >> +{ >> + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; >> + >> + if (!ovsthread_once_start(&once)) { >> + return; >> + } >> + >> + unixctl_command_register("dpif/offload/classes", NULL, 0, 0, >> + dpif_offload_show_classes, NULL); >> + >> + for (int i = 0; i < ARRAY_SIZE(base_dpif_offload_classes); i++) { >> + ovs_assert(base_dpif_offload_classes[i]->open >> + && base_dpif_offload_classes[i]->close); >> + >> + dpif_offload_register_provider(base_dpif_offload_classes[i]); >> + } >> + >> + ovsthread_once_done(&once); >> +} >> + >> +static struct dp_offload* >> +dpif_offload_get_dp_offload(const struct dpif *dpif) > > This function works on a dpif structure, not the dpif_offload, so it > should have a plain dpif_ prefix. And with the provider collection naming, > maybe: > > dpif_get_offload_provider_collection or dpif_get_offload_providers. Used dpif_get_offload_provider_collection() >> +{ >> + return ovsrcu_get(struct dp_offload *, &dpif->dp_offload); >> +} >> + >> +static int >> +dpif_offload_attach_provider_to_dp_offload__(struct dp_offload *dp_offload, >> + struct dpif_offload *offload) >> +{ >> + struct ovs_list *providers_list = &dp_offload->offload_providers; >> + struct dpif_offload *offload_entry; >> + >> + LIST_FOR_EACH (offload_entry, dpif_list_node, providers_list) { >> + if (offload_entry == offload || !strcmp(offload->name, >> + offload_entry->name)) { >> + return EEXIST; > > This turns negative later in the set, should it be negative from the > beginning? > > Also, I don't think the caller handles the negative value > in the later patch. Ack will fix. >> + } >> + } >> + >> + ovs_list_push_back(providers_list, &offload->dpif_list_node); >> + return 0; >> +} >> + >> +static int >> +dpif_offload_attach_provider_to_dp_offload(struct dp_offload *dp_offload, >> + struct dpif_offload *offload) >> +{ >> + int error; >> + >> + ovs_assert(dp_offload); >> + >> + error = dpif_offload_attach_provider_to_dp_offload__(dp_offload, >> offload); >> + return error; >> +} > > I'm not sure why the two functions above are separate functions. Seems like > one can be fully embedded into another. They don't seem to change later in > the set either. > > For the naming, they are local static functions, so can be just: > > provider_collection_add() Ack will merge and rename... >> + >> +static void >> +dpif_offload_attach_dp_offload(struct dpif *dpif, >> + struct dp_offload *dp_offload) > > This should also have a plain dpif_ prefix, so: > > dpif_attach_offload_provider_collection() Done. > This one may be a little more verbose, because the filed would likely need to > be dpif->offload_provider_collection or dpif->offload_providers. But we're > not using it frequently, so should be fine either way. Called it offload_provider_collection. > >> + OVS_REQUIRES(dpif_offload_mutex) >> +{ >> + /* When called, 'dp_offload' should still have a refcount > 0, which is >> + * guaranteed by holding the lock from the shash lookup up to this >> point. >> + * If, for any reason, the refcount is not > 0, ovs_refcount_ref() will >> + * assert. */ >> + ovs_refcount_ref(&dp_offload->ref_cnt); >> + ovsrcu_set(&dpif->dp_offload, dp_offload); >> +} >> + >> +static int >> +dpif_offload_attach_providers_(struct dpif *dpif) > > Don't use single underscores for function names. Should be double. > If we need another layer of internal functions - maybe re-think if > it can be named differently (usually, that's the case). > > dpif_attach_offload_providers__() Done. >> + OVS_REQUIRES(dpif_offload_mutex) >> +{ >> + const char *dpif_type_str = dpif_normalize_type(dpif_type(dpif)); >> + struct dp_offload *dp_offload; >> + struct shash_node *node; >> + >> + /* Allocate and attach dp_offload to dpif. */ >> + dp_offload = xmalloc(sizeof *dp_offload); >> + dp_offload->dpif_name = xstrdup(dpif_name(dpif)); >> + ovs_mutex_init_recursive(&dp_offload->offload_mutex); >> + ovs_refcount_init(&dp_offload->ref_cnt); >> + ovs_list_init(&dp_offload->offload_providers); >> + shash_add(&dpif_offload_providers, dp_offload->dpif_name, dp_offload); >> + >> + /* Attach all the providers supporting this dpif type. */ >> + SHASH_FOR_EACH (node, &dpif_offload_classes) { >> + const struct dpif_offload_class *class = node->data; >> + >> + for (size_t i = 0; class->supported_dpif_types[i] != NULL; i++) { >> + if (!strcmp(class->supported_dpif_types[i], dpif_type_str)) { >> + struct dpif_offload *offload; >> + int error; >> + >> + error = class->open(class, dpif, &offload); >> + if (!error) { >> + error = dpif_offload_attach_provider_to_dp_offload( >> + dp_offload, offload); >> + if (error) { >> + VLOG_WARN("failed to add dpif offload provider " >> + "%s to %s: %s", >> + class->type, dpif_name(dpif), >> + ovs_strerror(error)); >> + class->close(offload); >> + } >> + } else { >> + VLOG_WARN("failed to initialize dpif offload provider " >> + "%s for %s: %s", >> + class->type, dpif_name(dpif), >> + ovs_strerror(error)); >> + } >> + break; >> + } >> + } >> + } >> + >> + /* Attach dp_offload to dpif. */ >> + ovsrcu_set(&dpif->dp_offload, dp_offload); >> + >> + return 0; >> +} >> + >> +int >> +dpif_offload_attach_providers(struct dpif *dpif) > > dpif_attach_offload_providers() Ack. >> +{ >> + struct dp_offload *dp_offload; >> + int rc = 0; >> + >> + ovs_mutex_lock(&dpif_offload_mutex); >> + >> + dp_offload = shash_find_data(&dpif_offload_providers, dpif_name(dpif)); >> + if (dp_offload) { >> + dpif_offload_attach_dp_offload(dpif, dp_offload); >> + } else { >> + rc = dpif_offload_attach_providers_(dpif); >> + } >> + >> + ovs_mutex_unlock(&dpif_offload_mutex); >> + return rc; >> +} >> + >> +static void >> +dpif_offload_free_dp_offload_rcu(struct dp_offload *dp_offload) > > provider_collection_free_rcu() > Ack. >> +{ >> + struct dpif_offload *offload_entry; >> + >> + /* We need to use the safe variant here as we removed the entry, and the >> + * close API will free() it. */ >> + LIST_FOR_EACH_SAFE (offload_entry, dpif_list_node, >> + &dp_offload->offload_providers) { >> + char *name = offload_entry->name; >> + >> + ovs_list_remove(&offload_entry->dpif_list_node); >> + offload_entry->class->close(offload_entry); >> + ovsrcu_postpone(free, name); > > Why the name is postponed? This list is not protected by RCU on its own, > only as part of the dp_offload/provider_collection structure. I think it’s a leftover from a previous implementation. I will free it directly here. >> + } >> + >> + /* Free remaining resources. */ >> + ovs_mutex_destroy(&dp_offload->offload_mutex); >> + free(dp_offload->dpif_name); >> + free(dp_offload); >> +} >> + >> +void >> +dpif_offload_detach_providers(struct dpif *dpif) > > dpif_detach_offload_providers() Ack. >> +{ >> + struct dp_offload *dp_offload = dpif_offload_get_dp_offload(dpif); >> + >> + if (dp_offload) { >> + /* Take dpif_offload_mutex so that, if dp_offload->ref_cnt falls to >> + * zero, we can't get a new reference to 'dp_offload' through the >> + * 'dpif_offload_providers' shash. */ >> + ovs_mutex_lock(&dpif_offload_mutex); >> + if (ovs_refcount_unref_relaxed(&dp_offload->ref_cnt) == 1) { >> + shash_find_and_delete(&dpif_offload_providers, >> + dp_offload->dpif_name); >> + ovsrcu_postpone(dpif_offload_free_dp_offload_rcu, dp_offload); >> + } >> + ovs_mutex_unlock(&dpif_offload_mutex); >> + ovsrcu_set(&dpif->dp_offload, NULL); >> + } >> +} >> + >> + >> +void >> +dpif_offload_init(struct dpif_offload *offload, >> + const struct dpif_offload_class *class, >> + struct dpif *dpif) >> +{ >> + ovs_assert(offload && class && dpif); >> + >> + offload->class = class; >> + offload->name = xasprintf("%s[%s]", class->type, dpif_name(dpif)); >> +} >> + >> +const char * >> +dpif_offload_name(const struct dpif_offload *offload) >> +{ >> + return offload->name; >> +} >> + >> +const char * >> +dpif_offload_class_type(const struct dpif_offload *offload) > > Do we need the 'class' in the name of this function? I guess not, let me change it. >> +{ >> + return offload->class->type; >> +} >> + >> +void >> +dpif_offload_dump_start(struct dpif_offload_dump *dump, >> + const struct dpif *dpif) >> +{ >> + memset(dump, 0, sizeof *dump); >> + dump->dpif = dpif; >> +} >> + >> +bool >> +dpif_offload_dump_next(struct dpif_offload_dump *dump, >> + struct dpif_offload **offload) >> +{ >> + struct dp_offload *dp_offload; >> + >> + if (!offload || !dump || dump->error) { >> + return false; >> + } >> + >> + dp_offload = dpif_offload_get_dp_offload(dump->dpif); >> + if (!dp_offload) { >> + return false; >> + } >> + >> + if (dump->state) { >> + /* In theory, list entries should not be removed. However, in case >> + * someone calls this during destruction and the node has >> disappeared, >> + * we will return EIDRM (Identifier removed). */ >> + struct dpif_offload *offload_entry = NULL; >> + >> + LIST_FOR_EACH (offload_entry, dpif_list_node, >> + &dp_offload->offload_providers) { >> + if (offload_entry == dump->state) { > > Can we use LIST_FOR_EACH_CONTINUE here? Done. >> + if (ovs_list_back(&dp_offload->offload_providers) >> + == &offload_entry->dpif_list_node) { >> + dump->error = EOF; >> + } else { >> + *offload = CONTAINER_OF( >> + offload_entry->dpif_list_node.next, >> + struct dpif_offload, dpif_list_node); >> + >> + dump->state = *offload; >> + } >> + break; >> + } >> + } >> + >> + if (!offload_entry) { >> + dump->error = EIDRM; >> + } >> + } else { >> + /* Get the first entry in the list. */ >> + if (!ovs_list_is_empty(&dp_offload->offload_providers)) { > > And maybe a basic LIST_FOR_EACH with a break on the first element? > > Feels like it would be easier to read this way and less prone to errors. Done, it looks nicer... >> + *offload = CONTAINER_OF( >> + ovs_list_front(&dp_offload->offload_providers), >> + struct dpif_offload, dpif_list_node); >> + >> + dump->state = *offload; >> + } else { >> + dump->error = EOF; >> + } >> + } >> + >> + return !dump->error; >> +} >> + >> +int >> +dpif_offload_dump_done(struct dpif_offload_dump *dump) >> +{ >> + return dump->error == EOF ? 0 : dump->error; >> +} >> diff --git a/lib/dpif-offload.h b/lib/dpif-offload.h >> new file mode 100644 >> index 000000000..d61d8e0bf >> --- /dev/null >> +++ b/lib/dpif-offload.h >> @@ -0,0 +1,59 @@ >> +/* >> + * Copyright (c) 2025 Red Hat, Inc. >> + * >> + * 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. >> + */ >> + >> +#ifndef DPIF_OFFLOAD_H >> +#define DPIF_OFFLOAD_H >> + >> +#include "dpif.h" >> + >> +/* Forward declarations of private structures. */ >> +struct dpif_offload_class; >> +struct dpif_offload; >> + >> +/* Structure used by the dpif_offload_dump_* functions. */ >> +struct dpif_offload_dump { >> + const struct dpif *dpif; >> + int error; >> + void *state; >> +}; >> + >> + >> +/* Per dpif specific functions. */ >> +void dpif_offload_init(struct dpif_offload *, >> + const struct dpif_offload_class *, struct dpif *); >> +int dpif_offload_attach_providers(struct dpif *); >> +void dpif_offload_detach_providers(struct dpif *); >> +const char *dpif_offload_name(const struct dpif_offload *); >> +const char *dpif_offload_class_type(const struct dpif_offload *); >> +void dpif_offload_dump_start(struct dpif_offload_dump *, const struct dpif >> *); >> +bool dpif_offload_dump_next(struct dpif_offload_dump *, >> + struct dpif_offload **); >> +int dpif_offload_dump_done(struct dpif_offload_dump *); >> + >> +/* Iterates through each DPIF_OFFLOAD in DPIF, using DUMP as state. >> + * >> + * Arguments all have pointer type. >> + * >> + * If you break out of the loop, then you need to free the dump structure by >> + * hand using dpif_offload_dump_done(). */ >> +#define DPIF_OFFLOAD_FOR_EACH(DPIF_OFFLOAD, DUMP, DPIF) \ >> + for (dpif_offload_dump_start(DUMP, DPIF); \ >> + (dpif_offload_dump_next(DUMP, &DPIF_OFFLOAD) \ >> + ? true \ >> + : (dpif_offload_dump_done(DUMP), false)); \ >> + ) >> + >> +#endif /* DPIF_OFFLOAD_H */ >> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h >> index 520e21e68..e99807782 100644 >> --- a/lib/dpif-provider.h >> +++ b/lib/dpif-provider.h >> @@ -23,6 +23,7 @@ >> * ports that they contain may be fixed or dynamic. */ >> >> #include "openflow/openflow.h" >> +#include "ovs-thread.h" >> #include "dpif.h" >> #include "util.h" >> >> @@ -30,6 +31,9 @@ >> extern "C" { >> #endif >> >> +/* Forward declarations of private structures. */ >> +struct dp_offload; >> + >> /* Open vSwitch datapath interface. >> * >> * This structure should be treated as opaque by dpif implementations. */ >> @@ -40,6 +44,9 @@ struct dpif { >> uint8_t netflow_engine_type; >> uint8_t netflow_engine_id; >> long long int current_ms; >> + >> + /* dpif offload provider specific variables. */ >> + OVSRCU_TYPE(struct dp_offload *) dp_offload; >> }; >> >> struct dpif_ipf_status; >> diff --git a/lib/dpif.c b/lib/dpif.c >> index 85ea1f552..7b7c85c72 100644 >> --- a/lib/dpif.c >> +++ b/lib/dpif.c >> @@ -27,6 +27,8 @@ >> #include "dp-packet.h" >> #include "dpctl.h" >> #include "dpif-netdev.h" >> +#include "dpif-offload.h" >> +#include "dpif-offload-provider.h" >> #include "flow.h" >> #include "netdev-offload.h" >> #include "netdev-provider.h" >> @@ -125,6 +127,7 @@ dp_initialize(void) >> tnl_port_map_init(); >> tnl_neigh_cache_init(); >> route_table_init(); >> + dp_offload_initialize(); >> >> for (i = 0; i < ARRAY_SIZE(base_dpif_classes); i++) { >> dp_register_provider(base_dpif_classes[i]); >> @@ -359,6 +362,8 @@ do_open(const char *name, const char *type, bool create, >> struct dpif **dpifp) >> >> ovs_assert(dpif->dpif_class == registered_class->dpif_class); >> >> + dpif_offload_attach_providers(dpif); >> + >> DPIF_PORT_FOR_EACH(&dpif_port, &port_dump, dpif) { >> struct netdev *netdev; >> int err; >> @@ -459,6 +464,7 @@ dpif_close(struct dpif *dpif) >> if (rc->refcount == 1) { >> dpif_remove_netdev_ports(dpif); >> } >> + dpif_offload_detach_providers(dpif); >> dpif_uninit(dpif, true); >> dp_class_unref(rc); >> } >> @@ -1711,6 +1717,7 @@ dpif_init(struct dpif *dpif, const struct dpif_class >> *dpif_class, >> dpif->full_name = xasprintf("%s@%s", dpif_class->type, name); >> dpif->netflow_engine_type = netflow_engine_type; >> dpif->netflow_engine_id = netflow_engine_id; >> + ovsrcu_set(&dpif->dp_offload, NULL); >> } >> >> /* Undoes the results of initialization. >> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >> index 4a200dd08..e502c2637 100644 >> --- a/ofproto/ofproto-dpif.c >> +++ b/ofproto/ofproto-dpif.c >> @@ -25,6 +25,7 @@ >> #include "coverage.h" >> #include "cfm.h" >> #include "ct-dpif.h" >> +#include "dpif-offload.h" >> #include "fail-open.h" >> #include "guarded-list.h" >> #include "hmapx.h" >> @@ -6708,6 +6709,72 @@ done: >> return changed; >> } >> >> +static void >> +dpif_offload_show_backer_text(const struct dpif_backer *backer, struct ds >> *ds) >> +{ >> + struct dpif_offload_dump dump; >> + struct dpif_offload *offload; >> + >> + ds_put_format(ds, "%s:\n", dpif_name(backer->dpif)); >> + >> + DPIF_OFFLOAD_FOR_EACH (offload, &dump, backer->dpif) { >> + ds_put_format(ds, " %s\n", dpif_offload_class_type(offload)); >> + } >> +} >> + >> +static struct json * >> +dpif_offload_show_backer_json(struct json *backers, >> + const struct dpif_backer *backer) >> +{ >> + struct json *json_backer = json_object_create(); >> + struct dpif_offload_dump dump; >> + struct dpif_offload *offload; >> + >> + /* Add datapath as new JSON object using its name as key. */ >> + json_object_put(backers, dpif_name(backer->dpif), json_backer); > > Feels like this should be the last operation in this function. Also, comments > don't seem particularly useful in this function, they kind of just repeat the > code. Ack, moved around and removed comments. >> + >> + /* Add provider to "providers" array using its name as key. */ >> + struct json *json_providers = json_array_create_empty(); >> + >> + /* Add offload provides as new JSON objects using its type as key. */ >> + DPIF_OFFLOAD_FOR_EACH (offload, &dump, backer->dpif) { >> + json_array_add(json_providers, >> + >> json_string_create(dpif_offload_class_type(offload))); >> + } >> + >> + json_object_put(json_backer, "providers", json_providers); >> + return json_backer; >> +} >> + >> + >> +static void >> +ofproto_unixctl_dpif_offload_show(struct unixctl_conn *conn, >> + int argc OVS_UNUSED, >> + const char *argv[] OVS_UNUSED, >> + void *aux OVS_UNUSED) >> +{ >> + if (unixctl_command_get_output_format(conn) == UNIXCTL_OUTPUT_FMT_JSON) >> { >> + struct json *backers = json_object_create(); >> + const struct shash_node *backer; >> + >> + SHASH_FOR_EACH (backer, &all_dpif_backers) { >> + dpif_offload_show_backer_json(backers, backer->data); >> + } >> + unixctl_command_reply_json(conn, backers); >> + } else { >> + const struct shash_node **backers = shash_sort(&all_dpif_backers); >> + struct ds ds = DS_EMPTY_INITIALIZER; >> + >> + for (int i = 0; i < shash_count(&all_dpif_backers); i++) { >> + dpif_offload_show_backer_text(backers[i]->data, &ds); >> + } >> + free(backers); >> + >> + unixctl_command_reply(conn, ds_cstr(&ds)); >> + ds_destroy(&ds); >> + } >> +} >> + >> static struct json * >> dpif_show_backer_json(struct json *backers, const struct dpif_backer >> *backer) >> { >> @@ -7065,6 +7132,9 @@ ofproto_unixctl_init(void) >> ofproto_unixctl_mcast_snooping_show, NULL); >> unixctl_command_register("dpif/dump-dps", "", 0, 0, >> ofproto_unixctl_dpif_dump_dps, NULL); >> + unixctl_command_register("dpif/offload/show", "", 0, 0, >> + ofproto_unixctl_dpif_offload_show, >> + NULL); > > nit: can be on the same line. Fixed. >> unixctl_command_register("dpif/show", "", 0, 0, >> ofproto_unixctl_dpif_show, >> NULL); >> unixctl_command_register("dpif/show-dp-features", "bridge", 1, 1, >> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at >> index 35e7cdeca..11f9e346a 100644 >> --- a/tests/ofproto-dpif.at >> +++ b/tests/ofproto-dpif.at >> @@ -10104,6 +10104,27 @@ AT_CHECK([ovs-appctl ofproto/trace br0 >> in_port=p0,tcp --ct-next 'trk,est' | dnl >> OVS_TRAFFIC_VSWITCHD_STOP >> AT_CLEANUP >> >> +AT_SETUP([ofproto-dpif - offload - ovs-appctl dpif/offload/]) >> +AT_KEYWORDS([dpif-offload]) >> +OVS_VSWITCHD_START([add-br br1 -- set bridge br1 datapath-type=dummy]) >> + >> +AT_CHECK([ovs-appctl dpif/offload/show], [0], [dnl >> +dummy@ovs-dummy: >> + dummy_x >> + dummy >> +]) >> + >> +AT_CHECK([ovs-appctl --format json --pretty dpif/offload/show], [0], [dnl >> +{ >> + "dummy@ovs-dummy": { >> + "providers": [[ >> + "dummy_x", >> + "dummy"]]}} >> +]) >> + >> +OVS_VSWITCHD_STOP >> +AT_CLEANUP >> + >> dnl ---------------------------------------------------------------------- >> AT_BANNER([ofproto-dpif -- megaflows]) >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
