[ovs-dev] [PATCH v14 5/7] dpif-offload-netlink: Implement dpif-offload-provider API
Eelco Chaudron
echaudro at redhat.com
Mon Sep 6 13:20:33 UTC 2021
On 15 Jul 2021, at 8:01, Chris Mi wrote:
> Implement dpif-offload API for netlink datapath.
>
> Signed-off-by: Chris Mi <cmi at nvidia.com>
> Reviewed-by: Eli Britstein <elibr at nvidia.com>
> ---
> lib/automake.mk | 1 +
> lib/dpif-netlink.c | 2 +-
> lib/dpif-offload-netlink.c | 210 ++++++++++++++++++++++++++++++++++++
> lib/dpif-offload-provider.h | 12 +++
> 4 files changed, 224 insertions(+), 1 deletion(-)
> create mode 100644 lib/dpif-offload-netlink.c
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index dc865b0ef..daa60c784 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -442,6 +442,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-netlink.c b/lib/dpif-netlink.c
> index 6a7defb95..676934ef1 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -4037,7 +4037,7 @@ const struct dpif_class dpif_netlink_class = {
> NULL, /* bond_add */
> NULL, /* bond_del */
> NULL, /* bond_stats_get */
> - NULL, /* dpif_offlod_api */
> + &dpif_offload_netlink,
> };
>
> static int
> diff --git a/lib/dpif-offload-netlink.c b/lib/dpif-offload-netlink.c
> new file mode 100644
> index 000000000..f02a6b0eb
> --- /dev/null
> +++ b/lib/dpif-offload-netlink.c
> @@ -0,0 +1,210 @@
> +/*
> + * 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 */
> + int group_seq; /* group sequence */
Same comment on captitals and dots.
> +};
> +
> +static void
> +dpif_offload_netlink_init(void)
> +{
> + unsigned int psample_mcgroup;
> + int err;
> +
> + if (!netdev_is_flow_api_enabled()) {
I think the parameter documentation mentions that you need to restart ovs-vswitchd when you enable hw-offload, so I think we should be good here.
Are we sure there are no corner cases where this initialization comes before the netdev_is_flow_api_enabled() setup? Maybe adding a debug log message might help, so we know we did not initialize for this specific reason?
> + return;
> + }
> +
> + if (psample_sock) {
Same here, a debug log message might be good to add.
> + return;
> + }
> +
> + err = nl_lookup_genl_family(PSAMPLE_GENL_NAME,
> + &psample_family);
> + if (err) {
> + VLOG_INFO("%s: Generic Netlink family '%s' does not exist. "
> + "Please make sure the kernel module psample is loaded",
> + __func__, PSAMPLE_GENL_NAME);
Should we return the err? Maybe the current log message can be for the specific error code and add a more specific one for the others?
> + return;
> + }
> +
> + err = nl_lookup_genl_mcgroup(PSAMPLE_GENL_NAME,
> + PSAMPLE_NL_MCGRP_SAMPLE_NAME,
> + &psample_mcgroup);
> + if (err) {
> + VLOG_INFO("%s: Failed to join multicast group '%s' for Generic "
> + "Netlink family '%s'", __func__,
Add error code, something like "Netlink family '%s' with error %"
> + PSAMPLE_NL_MCGRP_SAMPLE_NAME,
> + PSAMPLE_GENL_NAME);
> + return;
> + }
> +
> + err = nl_sock_create(NETLINK_GENERIC, &psample_sock);
> + if (err) {
> + VLOG_INFO("%s: Failed to create psample socket", __func__);
Add error code to log message
> + return;
> + }
> +
> + err = nl_sock_join_mcgroup(psample_sock, psample_mcgroup);
> + if (err) {
> + VLOG_INFO("%s: Failed to join psample mcgroup", __func__);
Add error code to log message
> + nl_sock_destroy(psample_sock);
> + return;
> + }
> +}
> +
> +static void
> +dpif_offload_netlink_uninit(void)
See previous comment on uninit() name (patch 1).
> +{
> + if (!netdev_is_flow_api_enabled()) {
> + return;
> + }
Here the check needs to be removed, as I think it could be possible to call the destroy API after offload gets disabled.
> +
> + if (!psample_sock) {
> + return;
> + }
> +
> + nl_sock_destroy(psample_sock);
> + psample_sock = NULL;
> +}
> +
> +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->group_seq = nl_attr_get_u16(a[PSAMPLE_ATTR_GROUP_SEQ]);
I never see the group_seq used in the code, do we need to store it?
> + 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, "%s: error reading or parsing netlink (%s)",
> + __func__, ovs_strerror(error));
> + nl_sock_drain(psample_sock);
> + error = ENOBUFS;
> + }
> +
> + ofpbuf_uninit(&buf);
> + if (error) {
> + return error;
> + }
> + }
> +}
> +
> +const struct dpif_offload_api dpif_offload_netlink = {
> + .init = dpif_offload_netlink_init,
> + .uninit = dpif_offload_netlink_uninit,
> + .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 b765eb9a2..803c79ed2 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 "dp-packet.h"
> #include "netlink-protocol.h"
> #include "openvswitch/packets.h"
> #include "openvswitch/types.h"
> @@ -38,6 +39,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;
I was looking at some other short struct comments, and they seem to use the first word with a capital, and ending with a dot. So "Packet data." and "Input index.", not sure what the maintainers prefer.
> +};
> +
Now that you have defined the structure, the above empty declaration "struct dpif_offload_sflow;" can be removed.
> struct dpif_offload_api {
> void (*init)(void);
> void (*uninit)(void);
> @@ -49,4 +57,8 @@ void dpif_offload_sflow_recv_wait(const struct dpif *dpif);
> int dpif_offload_sflow_recv(const struct dpif *dpif,
> struct dpif_offload_sflow *sflow);
>
> +#ifdef __linux__
> +extern const struct dpif_offload_api dpif_offload_netlink;
> +#endif
> +
> #endif /* DPIF_OFFLOAD_PROVIDER_H */
> --
> 2.21.0
More information about the dev
mailing list