[ovs-dev] [PATCH v14 5/7] dpif-offload-netlink: Implement dpif-offload-provider API
Chris Mi
cmi at nvidia.com
Wed Sep 15 10:05:38 UTC 2021
On 9/6/2021 9:20 PM, Eelco Chaudron wrote:
> 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.
Done.
>
>> +};
>> +
>> +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?
Yes, we need to restart the ovs to make sFlow offload to take effect.
Done.
>
>> + return;
>> + }
>> +
>> + if (psample_sock) {
> Same here, a debug log message might be good to add.
Done.
>
>> + 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?
We don't want to fail the whole offload if failing to initialize
psample. So we don't return err here. Just print a log.
> Maybe the current log message can be for the specific error code and add a more specific one for the others?
Done.
>
>> + 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 %"
Done.
>
>> + 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
Done.
>
>> + nl_sock_destroy(psample_sock);
>> + return;
>> + }
>> +}
>> +
>> +static void
>> +dpif_offload_netlink_uninit(void)
> See previous comment on uninit() name (patch 1).
Done.
>
>> +{
>> + 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.
Done.
>
>> +
>> + 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?
No. I removed 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.
Done.
>
>> +};
>> +
> Now that you have defined the structure, the above empty declaration "struct dpif_offload_sflow;" can be removed.
Done.
>
>> 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