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 <c...@nvidia.com> > Reviewed-by: Eli Britstein <el...@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> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev