[ovs-dev] [PATCH v15 5/7] dpif-offload-netlink: Implement dpif-offload-provider API

[Eelco Chaudron]

Some comments on user-facing log messages, the rest look ok.

//Eelco


On 15 Sep 2021, at 14:43, 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  | 208 ++++++++++++++++++++++++++++++++++++
>  lib/dpif-offload-provider.h |  13 ++-
>  4 files changed, 222 insertions(+), 2 deletions(-)
>  create mode 100644 lib/dpif-offload-netlink.c
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index 9259f57de..18cffa827 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -446,6 +446,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 f546b48e5..19ae543e6 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -4341,7 +4341,7 @@ const struct dpif_class dpif_netlink_class = {
>      NULL,                       /* bond_add */
>      NULL,                       /* bond_del */
>      NULL,                       /* bond_stats_get */
> -    NULL,                       /* dpif_offload_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..fc3712764
> --- /dev/null
> +++ b/lib/dpif-offload-netlink.c
> @@ -0,0 +1,208 @@
> +/*
> + * 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. */
> +};
> +
> +static void
> +dpif_offload_netlink_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_INFO("%s: Generic Netlink family '%s' does not exist. "
> +                  "Please make sure the kernel module psample is loaded. "
> +                  "Error %d", __func__, PSAMPLE_GENL_NAME, err);

As these are use level log messages, adding a function name does not make much sense. They are already marked as dpif_offload_netlink as the component.
What about the following, it will also make the error more clear.

“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_INFO("%s: Failed to join multicast group '%s' for Generic "
> +                  "Netlink family '%s' with error %d", __func__,
> +                  PSAMPLE_NL_MCGRP_SAMPLE_NAME,
> +                  PSAMPLE_GENL_NAME, err);

Same as above, shorter and more clear:

“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_INFO("%s: Failed to create psample socket with error %d",
> +                  __func__, err);

“Failed to create psample socket: %s”, ovs_strerror(err)

> +        return;
> +    }
> +
> +    err = nl_sock_join_mcgroup(psample_sock, psample_mcgroup);
> +    if (err) {
> +        VLOG_INFO("%s: Failed to join psample mcgroup with error %d",
> +                  __func__, err);

“Failed to join psample mcgroup: %s”, ovs_strerror(err)

> +        nl_sock_destroy(psample_sock);
> +        return;
> +    }
> +}

<SNIP>