| Message ID | 50340f8db18db19e4434c7cd45ca8b3c601c5f51.1762950453.git.echaudro@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Architectural refactoring of hardware offload infrastructure | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
Looking at this patch, I found a potential issue with memory management and have some style questions. git sha of the commit: 50340f8db18db19e4434c7cd45ca8b3c601c5f51 Author: Eelco Chaudron <echaudro@redhat.com> One line subject of the commit: dpif-offload: Add provider for tc offload. This patch adds the tc provider framework by implementing a basic TC offload provider with open/close operations. It registers the provider for system type datapaths on Linux and adds test coverage for the offload show command. > diff --git a/lib/automake.mk b/lib/automake.mk > index 314102ecc..6e015ebfc 100644 > --- a/lib/automake.mk > +++ b/lib/automake.mk > @@ -475,6 +475,7 @@ lib_libopenvswitch_la_SOURCES += \ > lib/dpif-netlink.h \ > lib/dpif-netlink-rtnl.c \ > lib/dpif-netlink-rtnl.h \ > + lib/dpif-offload-tc.c \ > lib/if-notifier.c \ > lib/netdev-linux.c \ > lib/netdev-linux.h \ [ ... ] > diff --git a/lib/dpif-offload-tc.c b/lib/dpif-offload-tc.c > new file mode 100644 > index 000000000..f5d62566e > --- /dev/null > +++ b/lib/dpif-offload-tc.c > @@ -0,0 +1,47 @@ [ ... ] > +static int > +dpif_offload_tc_open(const struct dpif_offload_class *offload_class, > + struct dpif *dpif, struct dpif_offload **dpif_offload) > +{ > + struct dpif_offload *offload = xmalloc(sizeof(struct dpif_offload)); > + > + dpif_offload_init(offload, offload_class, dpif); > + *dpif_offload = offload; > + return 0; > +} Can dpif_offload_init() fail and potentially leave the allocated memory orphaned? Looking at the success path returning 0, if dpif_offload_init() could fail internally but not return an error, this could leak the offload structure. > +static void > +dpif_offload_tc_close(struct dpif_offload *dpif_offload) > +{ > + free(dpif_offload); > +} Should this function call any cleanup on the dpif_offload structure before freeing it? Does dpif_offload_init() allocate any resources that need to be released? [ ... ] > diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at > index f70d02926..88ee6238d 100644 > --- a/tests/system-offloads-traffic.at > +++ b/tests/system-offloads-traffic.at [ ... ] > +AT_SETUP([offloads - ovs-appctl dpif/offload/show - offloads enabled]) > +AT_KEYWORDS([dpif-offload]) > +OVS_TRAFFIC_VSWITCHD_START([], [], > + [-- set Open_vSwitch . other_config:hw-offload=true]) > + > +AT_CHECK([ovs-appctl dpif/offload/show], [0], [dnl > +system@ovs-system: > + tc > +]) Is the indentation correct here? The test shows two spaces before "tc" but should this follow the project's standard indentation pattern? > +AT_CHECK([ovs-appctl --format json --pretty dpif/offload/show], [0], [dnl > +{ > + "system@ovs-system": { > + "providers": [[ > + "tc"]]}} > +]) The JSON output shows nested arrays with "providers": [["tc"]]. Should providers be a simple array rather than an array of arrays?
diff --git a/lib/automake.mk b/lib/automake.mk index 314102ecc..6e015ebfc 100644 --- a/lib/automake.mk +++ b/lib/automake.mk @@ -475,6 +475,7 @@ lib_libopenvswitch_la_SOURCES += \ lib/dpif-netlink.h \ lib/dpif-netlink-rtnl.c \ lib/dpif-netlink-rtnl.h \ + lib/dpif-offload-tc.c \ lib/if-notifier.c \ lib/netdev-linux.c \ lib/netdev-linux.h \ diff --git a/lib/dpif-offload-provider.h b/lib/dpif-offload-provider.h index 77f89b0c3..4cd6fb88a 100644 --- a/lib/dpif-offload-provider.h +++ b/lib/dpif-offload-provider.h @@ -17,6 +17,7 @@ #ifndef DPIF_OFFLOAD_PROVIDER_H #define DPIF_OFFLOAD_PROVIDER_H +#include "dpif-provider.h" #include "ovs-thread.h" #include "openvswitch/list.h" @@ -88,7 +89,7 @@ struct dpif_offload_class { extern struct dpif_offload_class dpif_offload_dummy_class; extern struct dpif_offload_class dpif_offload_dummy_x_class; - +extern struct dpif_offload_class dpif_offload_tc_class; /* Global function, called by the dpif layer. */ void dp_offload_initialize(void); diff --git a/lib/dpif-offload-tc.c b/lib/dpif-offload-tc.c new file mode 100644 index 000000000..f5d62566e --- /dev/null +++ b/lib/dpif-offload-tc.c @@ -0,0 +1,47 @@ +/* + * 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-offload.h" +#include "dpif-offload-provider.h" +#include "util.h" + +static int +dpif_offload_tc_open(const struct dpif_offload_class *offload_class, + struct dpif *dpif, struct dpif_offload **dpif_offload) +{ + struct dpif_offload *offload = xmalloc(sizeof(struct dpif_offload)); + + dpif_offload_init(offload, offload_class, dpif); + *dpif_offload = offload; + return 0; +} + +static void +dpif_offload_tc_close(struct dpif_offload *dpif_offload) +{ + free(dpif_offload); +} + +struct dpif_offload_class dpif_offload_tc_class = { + .type = "tc", + .supported_dpif_types = (const char *const[]) { + "system", + NULL}, + .open = dpif_offload_tc_open, + .close = dpif_offload_tc_close, +}; diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c index b8ade5430..2e5be6b2f 100644 --- a/lib/dpif-offload.c +++ b/lib/dpif-offload.c @@ -37,6 +37,9 @@ static struct shash dpif_offload_providers \ SHASH_INITIALIZER(&dpif_offload_providers); static const struct dpif_offload_class *base_dpif_offload_classes[] = { +#if defined(__linux__) + &dpif_offload_tc_class, +#endif &dpif_offload_dummy_class, &dpif_offload_dummy_x_class, }; diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at index f70d02926..88ee6238d 100644 --- a/tests/system-offloads-traffic.at +++ b/tests/system-offloads-traffic.at @@ -1166,3 +1166,23 @@ OVS_WAIT_WHILE( OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP + +AT_SETUP([offloads - ovs-appctl dpif/offload/show - offloads enabled]) +AT_KEYWORDS([dpif-offload]) +OVS_TRAFFIC_VSWITCHD_START([], [], + [-- set Open_vSwitch . other_config:hw-offload=true]) + +AT_CHECK([ovs-appctl dpif/offload/show], [0], [dnl +system@ovs-system: + tc +]) + +AT_CHECK([ovs-appctl --format json --pretty dpif/offload/show], [0], [dnl +{ + "system@ovs-system": { + "providers": [[ + "tc"]]}} +]) + +OVS_TRAFFIC_VSWITCHD_STOP +AT_CLEANUP
This patch adds the tc provider framework. Signed-off-by: Eelco Chaudron <echaudro@redhat.com> --- lib/automake.mk | 1 + lib/dpif-offload-provider.h | 3 +- lib/dpif-offload-tc.c | 47 ++++++++++++++++++++++++++++++++ lib/dpif-offload.c | 3 ++ tests/system-offloads-traffic.at | 20 ++++++++++++++ 5 files changed, 73 insertions(+), 1 deletion(-) create mode 100644 lib/dpif-offload-tc.c