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

Chris Mi cmi at nvidia.com
Tue Oct 12 06:37:04 UTC 2021


On 10/1/2021 7:57 PM, Eelco Chaudron wrote:
> 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));
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' 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));
Done.
>
>> +        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)
Done.
>> +        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)
Done.
>
>> +        nl_sock_destroy(psample_sock);
>> +        return;
>> +    }
>> +}
> <SNIP>
>



More information about the dev mailing list