[dpdk-dev] [PATCH 0/2] net/tap: simplfication and servicabilty improvements

[dpdk-dev] [PATCH 0/2] net/tap: simplfication and servicabilty improvements

[Stephen Hemminger]

These are a couple of small fixes to the TAP driver. The first makes it
more robust to random signals, and the second one adds better error
reporting.

Stephen Hemminger (2):
  net/tap: simplify netlink send/receive functions
  net/tap: use netlink extended ack support

 drivers/net/tap/tap_netlink.c | 123 +++++++++++++++++++++++++---------
 1 file changed, 92 insertions(+), 31 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH 1/2] net/tap: simplify netlink send/receive functions

[Stephen Hemminger]

The tap_nl_recv() function does not need to use the full
complex recvmsg() system call, basic recv() will work here.

Ditto for tap_nl_send() full sendmsg is not needed.

Add logic to retry in case EINTR rather than forcing
error handling back in driver or worse to ethdev API.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/tap/tap_netlink.c | 45 +++++++++++------------------------
 1 file changed, 14 insertions(+), 31 deletions(-)

diff --git a/drivers/net/tap/tap_netlink.c b/drivers/net/tap/tap_netlink.c
index 14bbbec754f6..64220178a6ba 100644
--- a/drivers/net/tap/tap_netlink.c
+++ b/drivers/net/tap/tap_netlink.c
@@ -101,26 +101,17 @@ tap_nl_final(int nlsk_fd)
 int
 tap_nl_send(int nlsk_fd, struct nlmsghdr *nh)
 {
-	/* man 7 netlink EXAMPLE */
-	struct sockaddr_nl sa = {
-		.nl_family = AF_NETLINK,
-	};
-	struct iovec iov = {
-		.iov_base = nh,
-		.iov_len = nh->nlmsg_len,
-	};
-	struct msghdr msg = {
-		.msg_name = &sa,
-		.msg_namelen = sizeof(sa),
-		.msg_iov = &iov,
-		.msg_iovlen = 1,
-	};
 	int send_bytes;
 
 	nh->nlmsg_pid = 0; /* communication with the kernel uses pid 0 */
 	nh->nlmsg_seq = (uint32_t)rte_rand();
-	send_bytes = sendmsg(nlsk_fd, &msg, 0);
+
+retry:
+	send_bytes = send(nlsk_fd, nh, nh->nlmsg_len, 0);
 	if (send_bytes < 0) {
+		if (errno == EINTR)
+			goto retry;
+
 		TAP_LOG(ERR, "Failed to send netlink message: %s (%d)",
 			strerror(errno), errno);
 		return -1;
@@ -161,30 +152,22 @@ tap_nl_recv_ack(int nlsk_fd)
 int
 tap_nl_recv(int nlsk_fd, int (*cb)(struct nlmsghdr *, void *arg), void *arg)
 {
-	/* man 7 netlink EXAMPLE */
-	struct sockaddr_nl sa;
 	char buf[BUF_SIZE];
-	struct iovec iov = {
-		.iov_base = buf,
-		.iov_len = sizeof(buf),
-	};
-	struct msghdr msg = {
-		.msg_name = &sa,
-		.msg_namelen = sizeof(sa),
-		.msg_iov = &iov,
-		/* One message at a time */
-		.msg_iovlen = 1,
-	};
 	int multipart = 0;
 	int ret = 0;
 
 	do {
 		struct nlmsghdr *nh;
-		int recv_bytes = 0;
+		int recv_bytes;
 
-		recv_bytes = recvmsg(nlsk_fd, &msg, 0);
-		if (recv_bytes < 0)
+retry:
+		recv_bytes = recv(nlsk_fd, buf, sizeof(buf), 0);
+		if (recv_bytes < 0) {
+			if (errno == EINTR)
+				goto retry;
 			return -1;
+		}
+
 		for (nh = (struct nlmsghdr *)buf;
 		     NLMSG_OK(nh, (unsigned int)recv_bytes);
 		     nh = NLMSG_NEXT(nh, recv_bytes)) {
-- 
2.20.1

[dpdk-dev] [PATCH 2/2] net/tap: use netlink extended ack support

[Stephen Hemminger]

In recent Linux kernels, there is support for extended acknowledgement
to netlink messages. This is quite useful for diagnosing errors
in configuration in the kernel with TAP.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/tap/tap_netlink.c | 78 +++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/drivers/net/tap/tap_netlink.c b/drivers/net/tap/tap_netlink.c
index 64220178a6ba..2365678a9c13 100644
--- a/drivers/net/tap/tap_netlink.c
+++ b/drivers/net/tap/tap_netlink.c
@@ -9,10 +9,12 @@
 #include <string.h>
 #include <sys/socket.h>
 #include <unistd.h>
+#include <stdbool.h>
 
 #include <rte_malloc.h>
 #include <tap_netlink.h>
 #include <rte_random.h>
+
 #include "tap_log.h"
 
 /* Must be quite large to support dumping a huge list of QDISC or filters. */
@@ -43,6 +45,7 @@ tap_nl_init(uint32_t nl_groups)
 		.nl_family = AF_NETLINK,
 		.nl_groups = nl_groups,
 	};
+	int one = 1;
 
 	fd = socket(AF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, NETLINK_ROUTE);
 	if (fd < 0) {
@@ -59,6 +62,12 @@ tap_nl_init(uint32_t nl_groups)
 		close(fd);
 		return -1;
 	}
+
+#ifdef NETLINK_EXT_ACK
+	/* Ask for extended ACK response. on older kernel will ignore request. */
+	setsockopt(fd, SOL_NETLINK, NETLINK_EXT_ACK, &one, sizeof(one));
+#endif
+
 	if (bind(fd, (struct sockaddr *)&local, sizeof(local)) < 0) {
 		TAP_LOG(ERR, "Unable to bind to the netlink socket");
 		close(fd);
@@ -119,6 +128,74 @@ tap_nl_send(int nlsk_fd, struct nlmsghdr *nh)
 	return send_bytes;
 }
 
+#ifdef NETLINK_EXT_ACK
+static const struct nlattr *
+tap_nl_attr_first(const struct nlmsghdr *nh, size_t offset)
+{
+	return (const struct nlattr *)((const char *)nh + NLMSG_SPACE(offset));
+}
+
+static const struct nlattr *
+tap_nl_attr_next(const struct nlattr *attr)
+{
+	return (const struct nlattr *)((const char *)attr
+				       + NLMSG_ALIGN(attr->nla_len));
+}
+
+static bool
+tap_nl_attr_ok(const struct nlattr *attr, int len)
+{
+	if (len < (int)sizeof(struct nlattr))
+		return false; /* missing header */
+	if (attr->nla_len < sizeof(struct nlattr))
+		return false; /* attribute length should include itself */
+	if ((int)attr->nla_len  > len)
+		return false; /* attribute is truncated */
+	return true;
+}
+
+
+/* Decode extended errors from kernel */
+static void
+tap_nl_dump_ext_ack(const struct nlmsghdr *nh, const struct nlmsgerr *err)
+{
+	const struct nlattr *attr;
+	const char *tail = (const char *)nh + NLMSG_ALIGN(nh->nlmsg_len);
+	size_t hlen = sizeof(*err);
+
+	/* no TLVs, no extended response */
+	if (!(nh->nlmsg_flags & NLM_F_ACK_TLVS))
+		return;
+
+	if (!(nh->nlmsg_flags & NLM_F_CAPPED))
+		hlen += err->msg.nlmsg_len - NLMSG_HDRLEN;
+
+	for (attr = tap_nl_attr_first(nh, hlen);
+	     tap_nl_attr_ok(attr, tail - (const char *)attr);
+	     attr = tap_nl_attr_next(attr)) {
+		uint16_t type = attr->nla_type & NLA_TYPE_MASK;
+
+		if (type == NLMSGERR_ATTR_MSG) {
+			const char *msg = (const char *)attr
+				+ NLMSG_ALIGN(sizeof(*attr));
+
+			if (err->error)
+				TAP_LOG(ERR, "%s", msg);
+			else
+
+				TAP_LOG(WARNING, "%s", msg);
+			break;
+		}
+	}
+}
+#else
+/*
+ * External ACK support was added in Linux kernel 4.17
+ * on older kernels, just ignore that part of message
+ */
+#define tap_nl_dump_ext_ack(nh, err) do { } while();
+#endif
+
 /**
  * Check that the kernel sends an appropriate ACK in response
  * to an tap_nl_send().
@@ -174,6 +251,7 @@ tap_nl_recv(int nlsk_fd, int (*cb)(struct nlmsghdr *, void *arg), void *arg)
 			if (nh->nlmsg_type == NLMSG_ERROR) {
 				struct nlmsgerr *err_data = NLMSG_DATA(nh);
 
+				tap_nl_dump_ext_ack(nh, err_data);
 				if (err_data->error < 0) {
 					errno = -err_data->error;
 					return -1;
-- 
2.20.1

Re: [dpdk-dev] [PATCH 0/2] net/tap: simplfication and servicabilty improvements

[Wiles, Keith]

> On Apr 24, 2020, at 6:36 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> These are a couple of small fixes to the TAP driver. The first makes it
> more robust to random signals, and the second one adds better error
> reporting.
> 
> Stephen Hemminger (2):
>  net/tap: simplify netlink send/receive functions
>  net/tap: use netlink extended ack support
> 
> drivers/net/tap/tap_netlink.c | 123 +++++++++++++++++++++++++---------
> 1 file changed, 92 insertions(+), 31 deletions(-)
> 
> 2.20.1
> 

Acked-by: Keith Wiles <keith.wiles@intel.com>

Re: [dpdk-dev] [PATCH 2/2] net/tap: use netlink extended ack support

[Andrzej Ostruszka [C]]

On 25/04/2020 01:36, Stephen Hemminger wrote:
> In recent Linux kernels, there is support for extended acknowledgement
> to netlink messages. This is quite useful for diagnosing errors
> in configuration in the kernel with TAP.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
[...]
> +#else
> +/*
> + * External ACK support was added in Linux kernel 4.17
> + * on older kernels, just ignore that part of message
> + */
> +#define tap_nl_dump_ext_ack(nh, err) do { } while();

Maybe "while(0)" here?

With regards
Andrzej Ostruszka

Re: [dpdk-dev] [PATCH 2/2] net/tap: use netlink extended ack support

[Ferruh Yigit]

On 4/27/2020 12:32 PM, Andrzej Ostruszka [C] wrote:
> On 25/04/2020 01:36, Stephen Hemminger wrote:
>> In recent Linux kernels, there is support for extended acknowledgement
>> to netlink messages. This is quite useful for diagnosing errors
>> in configuration in the kernel with TAP.
>>
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>> ---
> [...]
>> +#else
>> +/*
>> + * External ACK support was added in Linux kernel 4.17
>> + * on older kernels, just ignore that part of message
>> + */
>> +#define tap_nl_dump_ext_ack(nh, err) do { } while();
> 
> Maybe "while(0)" here?
> 

+1, will fix while merging.

I assume since it is in #else leg it has not been tested but #if leg tested well.

Re: [dpdk-dev] [PATCH 0/2] net/tap: simplfication and servicabilty improvements

[Ferruh Yigit]

On 4/25/2020 2:14 PM, Wiles, Keith wrote:
> 
> 
>> On Apr 24, 2020, at 6:36 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
>>
>> These are a couple of small fixes to the TAP driver. The first makes it
>> more robust to random signals, and the second one adds better error
>> reporting.
>>
>> Stephen Hemminger (2):
>>  net/tap: simplify netlink send/receive functions
>>  net/tap: use netlink extended ack support
>>
>> drivers/net/tap/tap_netlink.c | 123 +++++++++++++++++++++++++---------
>> 1 file changed, 92 insertions(+), 31 deletions(-)
>>
>> 2.20.1
>>
> 
> Acked-by: Keith Wiles <keith.wiles@intel.com>
> 

Series applied to dpdk-next-net/master, thanks.

Re: [dpdk-dev] [PATCH 2/2] net/tap: use netlink extended ack support

[Stephen Hemminger]

On Wed, 6 May 2020 19:41:28 +0100
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 4/27/2020 12:32 PM, Andrzej Ostruszka [C] wrote:
> > On 25/04/2020 01:36, Stephen Hemminger wrote:  
> >> In recent Linux kernels, there is support for extended acknowledgement
> >> to netlink messages. This is quite useful for diagnosing errors
> >> in configuration in the kernel with TAP.
> >>
> >> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> >> ---  
> > [...]  
> >> +#else
> >> +/*
> >> + * External ACK support was added in Linux kernel 4.17
> >> + * on older kernels, just ignore that part of message
> >> + */
> >> +#define tap_nl_dump_ext_ack(nh, err) do { } while();  
> > 
> > Maybe "while(0)" here?
> >   
> 
> +1, will fix while merging.
> 
> I assume since it is in #else leg it has not been tested but #if leg tested well.

Yes, haven't built on distros that are old enough to hit the #else :-(