On 11 Mar 2022, at 3:14, Chris Mi wrote:
> On 2022-03-04 6:50 PM, Eelco Chaudron wrote:
>> 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.
> OK.
>>
>> //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?
> According to one of your below comment, I moved dp_offload_class_lookup from
> dpif to dpif-netlink.
> After that, dpif-offload for dummy is not needed.
>>
>>> 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:
>>> + *
>>> + *
>>> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.apache.org%2Flicenses%2FLICENSE-2.0&data=04%7C01%7Ccmi%40nvidia.com%7Cadd31a18c66c46ea056408d9fdccd97d%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637819878551136144%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=A5fNu%2BjqL48flvxwkfz1BHOAiu6LLuBTdp%2F4HkYVEdw%3D&reserved=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?
> The above code can be deleted.
>>
>>> 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
> I think I need Ilya's feedback to change this code.
>>
>>> 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:
>>> + *
>>> + *
>>> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.apache.org%2Flicenses%2FLICENSE-2.0&data=04%7C01%7Ccmi%40nvidia.com%7Cadd31a18c66c46ea056408d9fdccd97d%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637819878551136144%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=A5fNu%2BjqL48flvxwkfz1BHOAiu6LLuBTdp%2F4HkYVEdw%3D&reserved=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?
> Removed. And since this function is only called by dummy, it is also removed.
>>
>>> +
>>> + 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?
> Done.
>>
>>> + 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.
> Now it is moved to dpif_netlink_close().
>
> Except the 1:1 mapping comment which I think need Ilya's feedback, I have
> addressed your other comments.
> Thanks for your comments. The dpif-offload for dummy is not needed and
> removed.
> If needed, I can send v21.
Thanks for taking care of the questions and fixing them in your sandbox.
I would prefer for you to not send any more revisions until we have a clear
answer from Ilya.
//Eelco
>>
>>> 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
