I’m re-adding some of my v18 comments, which I know are depending on Ilya’s
feedback, although I still think we should have provider classes, i.e., not
based on the datapath class.
Some other questions could still be answered regardless of Ilya’s feedback
(will mark them with **). Please answer them inline, before sending another
revision so we can have some discussion around them if needed.
//Eelco
On 15 Feb 2022, at 10:17, Chris Mi wrote:
> Implement dpif-offload API for netlink datapath. And implement a
> dummy dpif-offload API for netdev datapath to make tests pass.
** Why do we need a dummy? If there is no hardware offload class we should just
pass, shouldn’t we?
>
> Issue: 2181036
> Change-Id: I950c3cc3c7092c3d87602da8928ad4aa2df4f38a
> Signed-off-by: Chris Mi <c...@nvidia.com>
> Reviewed-by: Eli Britstein <el...@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 | 221 ++++++++++++++++++++++++++++++++++++
> lib/dpif-offload-provider.h | 25 +++-
> lib/dpif-offload.c | 157 +++++++++++++++++++++++++
> lib/dpif-provider.h | 6 +-
> lib/dpif.c | 17 ++-
> 9 files changed, 476 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 781fba47a..9ed61029a 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -130,6 +130,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 \
> @@ -453,6 +454,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 e28e0b554..5ebd127b9 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1683,7 +1683,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);
>
> @@ -9639,6 +9640,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
> @@ -9659,6 +9664,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 71e35ccdd..75f85c254 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 = “dummy”,
+ .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..02aea7e2d
> --- /dev/null
> +++ b/lib/dpif-offload-netlink.c
> @@ -0,0 +1,221 @@
> +/*
> + * 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;
> + struct ofpbuf buf;
> + int error;
> +
> + ofpbuf_use_stub(&buf, sflow->buf_stub, sizeof sflow->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..ac13601b5 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,14 @@ 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. */
> + uint64_t buf_stub[4096 / 8]; /* Buffer stub for 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 +76,15 @@ struct dpif_offload_class {
> int (*sflow_recv)(struct dpif_offload_sflow *sflow);
> };
>
> +void dp_offload_initialize(void);
> +void dpif_offload_close(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..f3ac539ab 100644
> --- a/lib/dpif-offload.c
> +++ b/lib/dpif-offload.c
> @@ -18,6 +18,163 @@
> #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;
> +
> +void
> +dp_offload_initialize(void)
> +{
> + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> +
> + if (ovsthread_once_start(&once)) {
> + for (int i = 0; i < ARRAY_SIZE(base_dpif_offload_classes); i++) {
> + 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)
> + OVS_REQUIRES(dpif_offload_mutex)
> +{
> + 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 "
> + "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",
> + 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);
> + }
> +}
> +
> +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)
> + OVS_REQUIRES(dpif_offload_mutex)
> +{
> + 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);
> + return EBUSY;
> + }
> +
> + if (registered_class->offload_class->destroy) {
> + registered_class->offload_class->destroy();
> + }
> + 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 99009722a..c8ed385a1 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 40f5fe446..00c1a7e65 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -333,6 +333,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();
>
> @@ -345,6 +346,17 @@ do_open(const char *name, const char *type, bool create,
> struct dpif **dpifp)
> goto exit;
> }
>
> + dp_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 the right place to do this? You assign an
offload_class as part of dpif_init() so whoever 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",
> + 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) {
> @@ -374,6 +386,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);
> }
>
> @@ -449,6 +462,7 @@ dpif_close(struct dpif *dpif)
> if (dpif) {
> struct registered_dpif_class *rc;
>
> + dpif_offload_close(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.
> rc = shash_find_data(&dpif_classes, dpif->dpif_class->type);
>
> if (rc->refcount == 1) {
> @@ -1699,10 +1713,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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
