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. 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. > 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. > + .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. > + 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. > + * 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'. > + 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. > +}; > + > + > +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. > + * '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. > + > + > +#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/ > + 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. > + > +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. > +{ > + 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. > + } > + } > + > + 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() > + > +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() 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. > + 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__() > + 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() > +{ > + 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() > +{ > + 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. > + } > + > + /* 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() > +{ > + 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? > +{ > + 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? > + 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. > + *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. > + > + /* 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. > 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
