Looking at the patch, I've identified several potential issues in the implementation:
git sha of the commit: 0e456aa6e371be2efa2296015c5ebb66d284d826 Author: Eelco Chaudron <[email protected]> dpif-offload-provider: Add dpif-offload-provider implementation. This commit introduces the initial framework and APIs to support the hardware offload dpif provider. > diff --git a/lib/automake.mk b/lib/automake.mk > index 78d6e6516..314102ecc 100644 > --- a/lib/automake.mk > +++ b/lib/automake.mk > [ ... ] > diff --git a/lib/dpif-offload-dummy.c b/lib/dpif-offload-dummy.c > new file mode 100644 > index 000000000..04f4b5227 > --- /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> There's extra whitespace at the start of this line. Should this leading space be removed for proper alignment? > diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c > new file mode 100644 > index 000000000..b8ade5430 > --- /dev/null > +++ b/lib/dpif-offload.c > @@ -0,0 +1,387 @@ > [ ... ] > +static void > +dpif_offload_free_dp_offload_rcu(struct dp_offload *dp_offload) > +{ > + 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); > + } Does this code avoid use-after-free issues? The name pointer is extracted from offload_entry, then offload_entry->class->close() is called which might free the entire structure, followed by ovsrcu_postpone(free, name). If the close function frees the structure containing the name field, would this create a dangling pointer? > +void > +dpif_offload_detach_providers(struct dpif *dpif) > +{ > + 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); There's extra whitespace before this line. Should the leading space be removed for proper alignment? > + ovsrcu_set(&dpif->dp_offload, NULL); Same alignment issue here - extra space before the line? > diff --git a/lib/dpif-offload.h b/lib/dpif-offload.h > new file mode 100644 > index 000000000..66de104e8 > --- /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 Extra space before this #ifndef - should it be removed for consistency? > + #define DPIF_OFFLOAD_H Same alignment issue here? > + > + #include "dpif.h" And here as well? > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 4a200dd08..9674e6de6 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > [ ... ] > +static void > +ofproto_unixctl_dpif_offload_show(struct unixctl_conn *conn, > + int argc OVS_UNUSED, > + const char *argv[] OVS_UNUSED, > + void *aux OVS_UNUSED) { The opening brace placement doesn't follow OVS coding style. Should the brace be on a separate line after the function signature? diff --git a/lib/dpif.c b/lib/dpif.c > index 070fc0131..b77cfa0e5 100644 > --- a/lib/dpif.c > +++ b/lib/dpif.c > [ ... ] > @@ -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); > } Would this call path dpif_close()->dpif_offload_detach_providers() create race conditions? The offload detach happens after the dpif_remove_netdev_ports() call but before dpif_uninit(). During this window, might there be references to the offload providers that are being removed? _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
