[ovs-dev] [PATCH v18 5/8] dpif-offload-netlink: Implement dpif-offload-provider API
Eelco Chaudron
echaudro at redhat.com
Thu Nov 25 11:18:10 UTC 2021
On 15 Nov 2021, at 3:53, Chris Mi wrote:
> Implement dpif-offload API for netlink datapath. And implement a
> dummy dpif-offload API for netdev datapath to make tests pass.
>
> Signed-off-by: Chris Mi <cmi at nvidia.com>
> Reviewed-by: Eli Britstein <elibr at nvidia.com>
> ---
> lib/automake.mk | 2 +
> lib/dpif-netdev.c | 8 +-
> lib/dpif-netlink.c | 4 +-
> lib/dpif-offload-netdev.c | 43 +++++++
> lib/dpif-offload-netlink.c | 222 ++++++++++++++++++++++++++++++++++++
> lib/dpif-offload-provider.h | 25 +++-
> lib/dpif-offload.c | 163 ++++++++++++++++++++++++++
> lib/dpif-provider.h | 6 +-
> lib/dpif.c | 20 +++-
> 9 files changed, 486 insertions(+), 7 deletions(-)
> create mode 100644 lib/dpif-offload-netdev.c
> create mode 100644 lib/dpif-offload-netlink.c
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index 9259f57de..ce9f1c98a 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -128,6 +128,7 @@ lib_libopenvswitch_la_SOURCES = \
> lib/dpif-netdev-perf.c \
> lib/dpif-netdev-perf.h \
> lib/dpif-offload.c \
> + lib/dpif-offload-netdev.c \
> lib/dpif-offload-provider.h \
> lib/dpif-provider.h \
> lib/dpif.c \
> @@ -446,6 +447,7 @@ lib_libopenvswitch_la_SOURCES += \
> lib/dpif-netlink.h \
> lib/dpif-netlink-rtnl.c \
> lib/dpif-netlink-rtnl.h \
> + lib/dpif-offload-netlink.c \
> lib/if-notifier.c \
> lib/netdev-linux.c \
> lib/netdev-linux.h \
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 98453a206..b40a1071c 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1593,7 +1593,8 @@ create_dpif_netdev(struct dp_netdev *dp)
> ovs_refcount_ref(&dp->ref_cnt);
>
> dpif = xmalloc(sizeof *dpif);
> - dpif_init(&dpif->dpif, dp->class, dp->name, netflow_id >> 8, netflow_id);
> + dpif_init(&dpif->dpif, dp->class, NULL, dp->name, netflow_id >> 8,
> + netflow_id);
> dpif->dp = dp;
> dpif->last_port_seq = seq_read(dp->port_seq);
>
> @@ -8893,6 +8894,10 @@ dpif_dummy_override(const char *type)
> if (error == 0 || error == EAFNOSUPPORT) {
> dpif_dummy_register__(type);
> }
> + error = dp_offload_unregister_provider(type);
> + if (error == 0 || error == EAFNOSUPPORT) {
> + dpif_offload_dummy_register(type);
> + }
> }
>
> void
> @@ -8913,6 +8918,7 @@ dpif_dummy_register(enum dummy_level level)
> }
>
> dpif_dummy_register__("dummy");
> + dpif_offload_dummy_register("dummy");
>
> unixctl_command_register("dpif-dummy/change-port-number",
> "dp port new-number",
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 18cdfe6e5..b253dc668 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -461,8 +461,8 @@ open_dpif(const struct dpif_netlink_dp *dp, struct dpif **dpifp)
> dpif->port_notifier = NULL;
> fat_rwlock_init(&dpif->upcall_lock);
>
> - dpif_init(&dpif->dpif, &dpif_netlink_class, dp->name,
> - dp->dp_ifindex, dp->dp_ifindex);
> + dpif_init(&dpif->dpif, &dpif_netlink_class, &dpif_offload_netlink_class,
> + dp->name, dp->dp_ifindex, dp->dp_ifindex);
>
> dpif->dp_ifindex = dp->dp_ifindex;
> dpif->user_features = dp->user_features;
> diff --git a/lib/dpif-offload-netdev.c b/lib/dpif-offload-netdev.c
> new file mode 100644
> index 000000000..35dac2cc3
> --- /dev/null
> +++ b/lib/dpif-offload-netdev.c
> @@ -0,0 +1,43 @@
> +/*
> + * Copyright (c) 2021, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
> + *
> + * 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-provider.h"
> +#include "openvswitch/vlog.h"
> +
> +VLOG_DEFINE_THIS_MODULE(dpif_offload_netdev);
> +
> +/* Currently, it is used only for dummy netdev to make tests pass. */
> +const struct dpif_offload_class dpif_offload_netdev_class = {
> + .type = "netdev",
> + .init = NULL,
> + .destroy = NULL,
> + .sflow_recv_wait = NULL,
> + .sflow_recv = NULL,
> +};
> +
> +void
> +dpif_offload_dummy_register(const char *type)
> +{
> + struct dpif_offload_class *class;
> +
> + class = xmalloc(sizeof *class);
> + *class = dpif_offload_netdev_class;
> + class->type = xstrdup(type);
> + dp_offload_register_provider(class);
> +}
So here I assume you can not use the one defined in dpif-offload-netlink.c as it will fail initialization.
If this is not true, please use the one in dpif-offload-netlink.c like dpif-netlink.c does.
However assuming this is the case, why is this in dpif-offload-netdev.c? I would just add it to the dpif-offload.c as a new dummy class.
Also because we should not have netlink/netdev classes, but real offload classes like: TC, rte_flow, etc., etc.
Somehting like:
+/* Currently, it is used only for dummy netdev to make tests pass. */
+const struct dpif_offload_class dpif_offload_dummy_class = {
+ .type = “d”ummy,
+ .init = NULL,
+ .destroy = NULL,
+ .sflow_recv_wait = NULL,
+ .sflow_recv = NULL,
+};
+
+void
+dpif_offload_dummy_register()
+{
+ dp_offload_register_provider(dpif_offload_dummy_class);
+}
I’m also wondering, why you need this only for the dummy netdev to make the tests pass, but not for real live netdev devices?
> diff --git a/lib/dpif-offload-netlink.c b/lib/dpif-offload-netlink.c
As mentioned before this should be offload classes, so for this example I would think it should be called dpif-offload-tc.c
> new file mode 100644
> index 000000000..320363bbc
> --- /dev/null
> +++ b/lib/dpif-offload-netlink.c
> @@ -0,0 +1,222 @@
> +/*
> + * Copyright (c) 2021, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
> + *
> + * 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 <linux/psample.h>
> +#include <sys/poll.h>
> +
> +#include "dpif-offload-provider.h"
> +#include "netdev-offload.h"
> +#include "netlink-protocol.h"
> +#include "netlink-socket.h"
> +#include "openvswitch/vlog.h"
> +
> +VLOG_DEFINE_THIS_MODULE(dpif_offload_netlink);
> +
> +static struct nl_sock *psample_sock;
> +static int psample_family;
> +
> +/* Receive psample netlink message and save the attributes. */
> +struct offload_psample {
> + struct nlattr *packet; /* Packet data. */
> + int dp_group_id; /* Mapping id for sFlow offload. */
> + int iifindex; /* Input ifindex. */
> +};
> +
> +/* In order to keep compatibility with kernels without psample module,
> + * return success even if psample is not initialized successfully. */
> +static void
> +psample_init(void)
> +{
> + unsigned int psample_mcgroup;
> + int err;
> +
> + if (!netdev_is_flow_api_enabled()) {
> + VLOG_DBG("Flow API is not enabled.");
> + return;
> + }
> +
> + if (psample_sock) {
> + VLOG_DBG("Psample socket is already initialized.");
> + return;
> + }
> +
> + err = nl_lookup_genl_family(PSAMPLE_GENL_NAME,
> + &psample_family);
> + if (err) {
> + VLOG_WARN("Generic Netlink family '%s' does not exist: %s\n"
> + "Please make sure the kernel module psample is loaded.",
> + PSAMPLE_GENL_NAME, ovs_strerror(err));
> + return;
> + }
> +
> + err = nl_lookup_genl_mcgroup(PSAMPLE_GENL_NAME,
> + PSAMPLE_NL_MCGRP_SAMPLE_NAME,
> + &psample_mcgroup);
> + if (err) {
> + VLOG_WARN("Failed to join Netlink multicast group '%s': %s",
> + PSAMPLE_NL_MCGRP_SAMPLE_NAME, ovs_strerror(err));
> + return;
> + }
> +
> + err = nl_sock_create(NETLINK_GENERIC, &psample_sock);
> + if (err) {
> + VLOG_WARN("Failed to create psample socket: %s", ovs_strerror(err));
> + return;
> + }
> +
> + err = nl_sock_join_mcgroup(psample_sock, psample_mcgroup);
> + if (err) {
> + VLOG_WARN("Failed to join psample mcgroup: %s", ovs_strerror(err));
> + nl_sock_destroy(psample_sock);
> + psample_sock = NULL;
> + return;
> + }
> +}
> +
> +static int
> +dpif_offload_netlink_init(void)
> +{
> + psample_init();
> +
> + return 0;
> +}
> +
> +static void
> +psample_destroy(void)
> +{
> + if (!psample_sock) {
> + return;
> + }
> +
> + nl_sock_destroy(psample_sock);
> + psample_sock = NULL;
> +}
> +
> +static void
> +dpif_offload_netlink_destroy(void)
> +{
> + psample_destroy();
> +}
> +
> +static void
> +dpif_offload_netlink_sflow_recv_wait(void)
> +{
> + if (psample_sock) {
> + nl_sock_wait(psample_sock, POLLIN);
> + }
> +}
> +
> +static int
> +psample_from_ofpbuf(struct offload_psample *psample,
> + const struct ofpbuf *buf)
> +{
> + static const struct nl_policy ovs_psample_policy[] = {
> + [PSAMPLE_ATTR_IIFINDEX] = { .type = NL_A_U16 },
> + [PSAMPLE_ATTR_SAMPLE_GROUP] = { .type = NL_A_U32 },
> + [PSAMPLE_ATTR_GROUP_SEQ] = { .type = NL_A_U32 },
> + [PSAMPLE_ATTR_DATA] = { .type = NL_A_UNSPEC },
> + };
> + struct nlattr *a[ARRAY_SIZE(ovs_psample_policy)];
> + struct genlmsghdr *genl;
> + struct nlmsghdr *nlmsg;
> + struct ofpbuf b;
> +
> + b = ofpbuf_const_initializer(buf->data, buf->size);
> + nlmsg = ofpbuf_try_pull(&b, sizeof *nlmsg);
> + genl = ofpbuf_try_pull(&b, sizeof *genl);
> + if (!nlmsg || !genl || nlmsg->nlmsg_type != psample_family
> + || !nl_policy_parse(&b, 0, ovs_psample_policy, a,
> + ARRAY_SIZE(ovs_psample_policy))) {
> + return EINVAL;
> + }
> +
> + psample->iifindex = nl_attr_get_u16(a[PSAMPLE_ATTR_IIFINDEX]);
> + psample->dp_group_id = nl_attr_get_u32(a[PSAMPLE_ATTR_SAMPLE_GROUP]);
> + psample->packet = a[PSAMPLE_ATTR_DATA];
> +
> + return 0;
> +}
> +
> +static int
> +psample_parse_packet(struct offload_psample *psample,
> + struct dpif_offload_sflow *sflow)
> +{
> + dp_packet_use_stub(&sflow->packet,
> + CONST_CAST(struct nlattr *,
> + nl_attr_get(psample->packet)) - 1,
> + nl_attr_get_size(psample->packet) +
> + sizeof(struct nlattr));
> + dp_packet_set_data(&sflow->packet,
> + (char *) dp_packet_data(&sflow->packet) +
> + sizeof(struct nlattr));
> + dp_packet_set_size(&sflow->packet, nl_attr_get_size(psample->packet));
> +
> + sflow->attr = dpif_offload_sflow_attr_find(psample->dp_group_id);
> + if (!sflow->attr) {
> + return ENOENT;
> + }
> + sflow->iifindex = psample->iifindex;
> +
> + return 0;
> +}
> +
> +static int
> +dpif_offload_netlink_sflow_recv(struct dpif_offload_sflow *sflow)
> +{
> + if (!psample_sock) {
> + return ENOENT;
> + }
> +
> + for (;;) {
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> + struct offload_psample psample;
> + uint64_t buf_stub[4096 / 8];
> + struct ofpbuf buf;
> + int error;
> +
> + ofpbuf_use_stub(&buf, buf_stub, sizeof buf_stub);
> + error = nl_sock_recv(psample_sock, &buf, NULL, false);
> +
> + if (!error) {
> + error = psample_from_ofpbuf(&psample, &buf);
> + if (!error) {
> + ofpbuf_uninit(&buf);
> + error = psample_parse_packet(&psample, sflow);
> + return error;
> + }
> + } else if (error != EAGAIN) {
> + VLOG_WARN_RL(&rl, "Error reading or parsing netlink (%s).",
> + ovs_strerror(error));
> + nl_sock_drain(psample_sock);
> + error = ENOBUFS;
> + }
> +
> + ofpbuf_uninit(&buf);
> + if (error) {
> + return error;
> + }
> + }
> +}
> +
> +const struct dpif_offload_class dpif_offload_netlink_class = {
> + .type = "system",
> + .init = dpif_offload_netlink_init,
> + .destroy = dpif_offload_netlink_destroy,
> + .sflow_recv_wait = dpif_offload_netlink_sflow_recv_wait,
> + .sflow_recv = dpif_offload_netlink_sflow_recv,
> +};
> diff --git a/lib/dpif-offload-provider.h b/lib/dpif-offload-provider.h
> index af49eedb9..4ef50266d 100644
> --- a/lib/dpif-offload-provider.h
> +++ b/lib/dpif-offload-provider.h
> @@ -17,12 +17,18 @@
> #ifndef DPIF_OFFLOAD_PROVIDER_H
> #define DPIF_OFFLOAD_PROVIDER_H
>
> +#include "dp-packet.h"
> #include "netlink-protocol.h"
> #include "openvswitch/packets.h"
> #include "openvswitch/types.h"
>
> struct dpif;
> -struct dpif_offload_sflow;
> +struct registered_dpif_offload_class;
> +
> +#ifdef __linux__
> +extern const struct dpif_offload_class dpif_offload_netlink_class;
> +#endif
> +extern const struct dpif_offload_class dpif_offload_netdev_class;
>
> /* When offloading sample action, userspace creates a unique ID to map
> * sFlow action and tunnel info and passes this ID to datapath instead
> @@ -37,6 +43,13 @@ struct dpif_sflow_attr {
> ovs_u128 ufid; /* Flow ufid. */
> };
>
> +/* Parse the specific dpif message to sFlow. So OVS can process it. */
> +struct dpif_offload_sflow {
> + struct dp_packet packet; /* Packet data. */
> + uint32_t iifindex; /* Input ifindex. */
> + const struct dpif_sflow_attr *attr; /* SFlow attribute. */
> +};
> +
> /* Datapath interface offload structure, to be defined by each implementation
> * of a datapath interface.
> */
> @@ -62,6 +75,16 @@ struct dpif_offload_class {
> int (*sflow_recv)(struct dpif_offload_sflow *sflow);
> };
>
> +void dp_offload_initialize(void);
> +void dpif_offload_close(struct dpif *);
> +void dpif_offload_delete(struct dpif *);
> +
> +int dp_offload_register_provider(const struct dpif_offload_class *);
> +int dp_offload_unregister_provider(const char *type);
> +void dpif_offload_dummy_register(const char *type);
> +void dp_offload_class_unref(struct registered_dpif_offload_class *rc);
> +struct registered_dpif_offload_class *dp_offload_class_lookup(const char *);
> +
> void dpif_offload_sflow_recv_wait(const struct dpif *dpif);
> int dpif_offload_sflow_recv(const struct dpif *dpif,
> struct dpif_offload_sflow *sflow);
> diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c
> index f2bf3e634..10d0bff53 100644
> --- a/lib/dpif-offload.c
> +++ b/lib/dpif-offload.c
> @@ -18,6 +18,169 @@
> #include <errno.h>
>
> #include "dpif-provider.h"
> +#include "openvswitch/shash.h"
> +#include "openvswitch/vlog.h"
> +
> +VLOG_DEFINE_THIS_MODULE(dpif_offload);
> +
> +static const struct dpif_offload_class *base_dpif_offload_classes[] = {
> +#if defined(__linux__)
> + &dpif_offload_netlink_class,
> +#endif
> + &dpif_offload_netdev_class,
> +};
> +
> +struct registered_dpif_offload_class {
> + const struct dpif_offload_class *offload_class;
> + int refcount;
> +};
> +static struct shash dpif_offload_classes =
> + SHASH_INITIALIZER(&dpif_offload_classes);
> +
> +/* Protects 'dpif_offload_classes', including the refcount. */
> +static struct ovs_mutex dpif_offload_mutex = OVS_MUTEX_INITIALIZER;
Can you make sure we use the OVS_ACQUIRES, OVS_RELEASES, OVS_REQUIRES, OVS_EXCLUDED macro’s for this mutex, see compiler.h.
> +
> +void
> +dp_offload_initialize(void)
> +{
> + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> +
> + if (ovsthread_once_start(&once)) {
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(base_dpif_offload_classes); i++) {
This can be “for (int I = 0;...”
> + dp_offload_register_provider(base_dpif_offload_classes[i]);
> + }
> + ovsthread_once_done(&once);
> + }
> +}
> +
> +static int
> +dp_offload_register_provider__(const struct dpif_offload_class *new_class)
> +{
> + struct registered_dpif_offload_class *registered_class;
> + int error;
> +
> + if (shash_find(&dpif_offload_classes, new_class->type)) {
> + VLOG_WARN("attempted to register duplicate datapath offload "
Capital A for Attempted
> + "provider: %s", new_class->type);
> + return EEXIST;
> + }
> +
> + error = new_class->init ? new_class->init() : 0;
> + if (error) {
> + VLOG_WARN("failed to initialize %s datapath offload class: %s",
Capital F for Failed
> + new_class->type, ovs_strerror(error));
> + return error;
> + }
> +
> + registered_class = xmalloc(sizeof *registered_class);
> + registered_class->offload_class = new_class;
> + registered_class->refcount = 0;
> +
> + shash_add(&dpif_offload_classes, new_class->type, registered_class);
> +
> + return 0;
> +}
> +
> +void dpif_offload_close(struct dpif *dpif)
> +{
> + if (dpif->offload_class) {
> + struct registered_dpif_offload_class *rc;
> +
> + rc = shash_find_data(&dpif_offload_classes, dpif->offload_class->type);
> + dp_offload_class_unref(rc);
> + }
> +}
> +
> +void dpif_offload_delete(struct dpif *dpif)
> +{
> + const struct dpif_offload_class *offload_class = dpif->offload_class;
> +
> + if (offload_class && offload_class->destroy) {
> + offload_class->destroy();
> + }
> +}
> +
> +int
> +dp_offload_register_provider(const struct dpif_offload_class *new_class)
> +{
> + int error;
> +
> + ovs_mutex_lock(&dpif_offload_mutex);
> + error = dp_offload_register_provider__(new_class);
> + ovs_mutex_unlock(&dpif_offload_mutex);
> +
> + return error;
> +}
> +
> +/* Unregisters an offload datapath provider. 'type' must have been previously
> + * registered and not currently be in use by any dpifs. After unregistration
> + * new offload datapaths of that type cannot be opened using dpif_open(). */
> +static int
> +dp_offload_unregister_provider__(const char *type)
> +{
> + struct shash_node *node;
> + struct registered_dpif_offload_class *registered_class;
> +
> + node = shash_find(&dpif_offload_classes, type);
> + if (!node) {
> + return EAFNOSUPPORT;
> + }
> +
> + registered_class = node->data;
> + if (registered_class->refcount) {
> + VLOG_WARN("attempted to unregister in use offload datapath provider: "
> + "%s", type);
Capital A for Attempted
> + return EBUSY;
> + }
> +
> + shash_delete(&dpif_offload_classes, node);
> + free(registered_class);
> +
> + return 0;
> +}
> +
> +/* Unregisters an offload datapath provider. 'type' must have been previously
> + * registered and not currently be in use by any dpifs. After unregistration
> + * new offload datapaths of that type cannot be opened using dpif_open(). */
> +int
> +dp_offload_unregister_provider(const char *type)
> +{
> + int error;
> +
> + dp_offload_initialize();
Why do we call this?
> +
> + ovs_mutex_lock(&dpif_offload_mutex);
> + error = dp_offload_unregister_provider__(type);
> + ovs_mutex_unlock(&dpif_offload_mutex);
> +
> + return error;
> +}
> +
> +void
> +dp_offload_class_unref(struct registered_dpif_offload_class *rc)
> +{
> + ovs_mutex_lock(&dpif_offload_mutex);
> + ovs_assert(rc->refcount);
> + rc->refcount--;
> + ovs_mutex_unlock(&dpif_offload_mutex);
> +}
> +
> +struct registered_dpif_offload_class *
> +dp_offload_class_lookup(const char *type)
> +{
> + struct registered_dpif_offload_class *rc;
> +
> + ovs_mutex_lock(&dpif_offload_mutex);
> + rc = shash_find_data(&dpif_offload_classes, type);
> + if (rc) {
> + rc->refcount++;
> + }
> + ovs_mutex_unlock(&dpif_offload_mutex);
> +
> + return rc;
> +}
>
> void
> dpif_offload_sflow_recv_wait(const struct dpif *dpif)
> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index 37b3d3618..3b9607afc 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -47,8 +47,10 @@ struct dpif {
> struct dpif_ipf_status;
> struct ipf_dump_ctx;
>
> -void dpif_init(struct dpif *, const struct dpif_class *, const char *name,
> - uint8_t netflow_engine_type, uint8_t netflow_engine_id);
> +void dpif_init(struct dpif *, const struct dpif_class *,
> + const struct dpif_offload_class *offload_class,
> + const char *name, uint8_t netflow_engine_type,
> + uint8_t netflow_engine_id);
> void dpif_uninit(struct dpif *dpif, bool close);
>
> static inline void dpif_assert_class(const struct dpif *dpif,
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 38bcb47cb..a550bf5dd 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -332,6 +332,7 @@ do_open(const char *name, const char *type, bool create, struct dpif **dpifp)
> struct dpif *dpif = NULL;
> int error;
> struct registered_dpif_class *registered_class;
> + struct registered_dpif_offload_class *registered_offload_class;
>
> dp_initialize();
>
> @@ -344,6 +345,17 @@ do_open(const char *name, const char *type, bool create, struct dpif **dpifp)
> goto exit;
> }
>
> + db_offload_initialize();
> +
> + registered_offload_class = dp_offload_class_lookup(type);
Don’t think we need to do this here, do we? The offload class is initialized as part of dpif we query below.
So if the dpif is found you could do dp_offload_class_lookup(dpif->offload_class->type)?
However is this function right place to do this? You assign an offload_class as part of dpif_init() so who ever is calling dpif_init() should that not be responsible for calling dp_offload_class_lookup() before using it?
> + if (!registered_offload_class) {
> + VLOG_WARN("could not create offload datapath %s of unknown type %s",
Capital C.
> + name, type);
> + error = EAFNOSUPPORT;
> + dp_class_unref(registered_class);
> + goto exit;
> + }
> +
> error = registered_class->dpif_class->open(registered_class->dpif_class,
> name, create, &dpif);
> if (!error) {
> @@ -372,6 +384,7 @@ do_open(const char *name, const char *type, bool create, struct dpif **dpifp)
> }
> }
> } else {
> + dp_offload_class_unref(registered_offload_class);
> dp_class_unref(registered_class);
> }
>
> @@ -447,6 +460,7 @@ dpif_close(struct dpif *dpif)
> if (dpif) {
> struct registered_dpif_class *rc;
>
> + dpif_offload_close(dpif);
> rc = shash_find_data(&dpif_classes, dpif->dpif_class->type);
>
> if (rc->refcount == 1) {
> @@ -527,6 +541,9 @@ dpif_delete(struct dpif *dpif)
>
> COVERAGE_INC(dpif_destroy);
>
> + dpif_offload_delete(dpif);
Not sure I understand, but why are we destroying the offload dpif class here, it can be used by another dpif type.
I guess this is all because your design has a 1:1 mapping? Guess it should be two dpif_types that could share the same offload class type.
> + log_operation(dpif, "offload delete", 0);
> +
> error = dpif->dpif_class->destroy(dpif);
> log_operation(dpif, "delete", error);
> return error;
> @@ -1680,10 +1697,11 @@ dpif_queue_to_priority(const struct dpif *dpif, uint32_t queue_id,
>
> void
> dpif_init(struct dpif *dpif, const struct dpif_class *dpif_class,
> - const char *name,
> + const struct dpif_offload_class *offload_class, const char *name,
> uint8_t netflow_engine_type, uint8_t netflow_engine_id)
> {
> dpif->dpif_class = dpif_class;
> + dpif->offload_class = offload_class;
> dpif->base_name = xstrdup(name);
> dpif->full_name = xasprintf("%s@%s", dpif_class->type, name);
> dpif->netflow_engine_type = netflow_engine_type;
> --
> 2.30.2
More information about the dev
mailing list