[patch iproute2 v8 0/2] tc: Add batchsize feature to batch mode

[patch iproute2 v8 0/2] tc: Add batchsize feature to batch mode

[Chris Mi]

Currently in tc batch mode, only one command is read from the batch
file and sent to kernel to process. With this patchset, at most 128
commands can be accumulated before sending to kernel.

We introduced two new functions in patch 1 to support for sending
multiple messages. In patch 2, we add this support for filter and
actions add/delete/change/replace commands.

But please note that kernel still processes the requests one by one.
To process the requests in parallel in kernel is another effort.
The time we're saving in this patchset is the user mode and kernel mode
context switch. So this patchset works on top of the current kernel.

Using the following script in kernel, we can generate 1,000,000 rules.
	tools/testing/selftests/tc-testing/tdc_batch.py

Without this patchset, 'tc -b $file' exection time is:

real    0m15.555s
user    0m7.211s
sys     0m8.284s

With this patchset, 'tc -b $file' exection time is:

real    0m12.360s
user    0m6.082s
sys     0m6.213s

The insertion rate is improved more than 10%.

v3
==
1. Instead of hacking function rtnl_talk directly, add a new function
   rtnl_talk_msg.
2. remove most of global variables to use parameter passing
3. divide the previous patch into 4 patches.

v4
==
1. Remove function setcmdlinetotal. Now in function batch, we read one
   more line to determine if we are reaching the end of file.
2. Remove function __rtnl_check_ack. Now __rtnl_talk calls __rtnl_talk_msg
   directly.
3. if (batch_size < 1)
        batch_size = 1;

v5
==
1. Fix a bug that can't deal with batch file with blank line.
2. Describe the limitation in man page.

v6
==
1. Add support for mixed commands.
2. Fix a bug that not all messages are acked if batch size > 1.

v7
==
1. We can tell exactly which command fails.
2. Add a new function rtnl_talk_iov
3. Allocate the memory in function batch() instead of each client.
4. Remove option -bs.

v8
==
1. Replace strcmp with matches.
2. Recycle buffers.


Chris Mi (2):
  lib/libnetlink: Add functions rtnl_talk_msg and rtnl_talk_iov
  tc: Add batchsize feature for filter and actions

 include/libnetlink.h |   6 ++
 lib/libnetlink.c     |  82 +++++++++++++++++++------
 tc/m_action.c        |  60 ++++++++++++-------
 tc/tc.c              | 165 ++++++++++++++++++++++++++++++++++++++++++++++-----
 tc/tc_common.h       |   5 +-
 tc/tc_filter.c       |  97 +++++++++++++++++-------------
 6 files changed, 319 insertions(+), 96 deletions(-)

-- 
2.14.2

[patch iproute2 v8 1/2] lib/libnetlink: Add functions rtnl_talk_msg and rtnl_talk_iov

[Chris Mi]

rtnl_talk can only send a single message to kernel. Add two functions
rtnl_talk_msg and rtnl_talk_iov that can send multiple messages to kernel.
rtnl_talk_msg takes struct msghdr * as argument.
rtnl_talk_iov takes struct iovec * and iovlen as arguments.

Signed-off-by: Chris Mi <chrism@mellanox.com>
---
 include/libnetlink.h |  6 ++++
 lib/libnetlink.c     | 82 ++++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 70 insertions(+), 18 deletions(-)

diff --git a/include/libnetlink.h b/include/libnetlink.h
index a4d83b9e..e9a63dbc 100644
--- a/include/libnetlink.h
+++ b/include/libnetlink.h
@@ -96,6 +96,12 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
 int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 	      struct nlmsghdr **answer)
 	__attribute__((warn_unused_result));
+int rtnl_talk_msg(struct rtnl_handle *rtnl, struct msghdr *m,
+		  struct nlmsghdr **answer)
+	__attribute__((warn_unused_result));
+int rtnl_talk_iov(struct rtnl_handle *rtnl, struct iovec *iovec, size_t iovlen,
+		  struct nlmsghdr **answer)
+	__attribute__((warn_unused_result));
 int rtnl_talk_extack(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 	      struct nlmsghdr **answer, nl_ext_ack_fn_t errfn)
 	__attribute__((warn_unused_result));
diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index 00e6ce0c..183825c7 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -581,39 +581,43 @@ static void rtnl_talk_error(struct nlmsghdr *h, struct nlmsgerr *err,
 		strerror(-err->error));
 }
 
-static int __rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
-		       struct nlmsghdr **answer,
-		       bool show_rtnl_err, nl_ext_ack_fn_t errfn)
+static int __rtnl_talk_msg(struct rtnl_handle *rtnl, struct msghdr *m,
+			   struct nlmsghdr **answer,
+			   bool show_rtnl_err, nl_ext_ack_fn_t errfn)
 {
-	int status;
-	unsigned int seq;
-	struct nlmsghdr *h;
 	struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };
-	struct iovec iov = {
-		.iov_base = n,
-		.iov_len = n->nlmsg_len
-	};
+	int i, status, iovlen = m->msg_iovlen;
+	struct iovec iov;
 	struct msghdr msg = {
 		.msg_name = &nladdr,
 		.msg_namelen = sizeof(nladdr),
 		.msg_iov = &iov,
 		.msg_iovlen = 1,
 	};
+	unsigned int seq = 0;
+	struct nlmsghdr *h;
 	char *buf;
 
-	n->nlmsg_seq = seq = ++rtnl->seq;
-
-	if (answer == NULL)
-		n->nlmsg_flags |= NLM_F_ACK;
+	for (i = 0; i < iovlen; i++) {
+		struct iovec *v;
+		v = &m->msg_iov[i];
+		h = v->iov_base;
+		h->nlmsg_seq = seq = ++rtnl->seq;
+		if (answer == NULL)
+			h->nlmsg_flags |= NLM_F_ACK;
+	}
 
-	status = sendmsg(rtnl->fd, &msg, 0);
+	status = sendmsg(rtnl->fd, m, 0);
 	if (status < 0) {
 		perror("Cannot talk to rtnetlink");
 		return -1;
 	}
 
+	i = 0;
 	while (1) {
+next:
 		status = rtnl_recvmsg(rtnl->fd, &msg, &buf);
+		++i;
 
 		if (status < 0)
 			return status;
@@ -642,7 +646,7 @@ static int __rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 
 			if (nladdr.nl_pid != 0 ||
 			    h->nlmsg_pid != rtnl->local.nl_pid ||
-			    h->nlmsg_seq != seq) {
+			    h->nlmsg_seq > seq || h->nlmsg_seq < seq - iovlen) {
 				/* Don't forget to skip that message. */
 				status -= NLMSG_ALIGN(len);
 				h = (struct nlmsghdr *)((char *)h + NLMSG_ALIGN(len));
@@ -662,7 +666,10 @@ static int __rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 						*answer = (struct nlmsghdr *)buf;
 					else
 						free(buf);
-					return 0;
+					if (h->nlmsg_seq == seq)
+						return 0;
+					else
+						goto next;
 				}
 
 				if (rtnl->proto != NETLINK_SOCK_DIAG &&
@@ -671,7 +678,7 @@ static int __rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 
 				errno = -err->error;
 				free(buf);
-				return -1;
+				return -i;
 			}
 
 			if (answer) {
@@ -698,12 +705,51 @@ static int __rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 	}
 }
 
+static int __rtnl_talk_iov(struct rtnl_handle *rtnl, struct iovec *msg_iov,
+			   size_t iovlen, struct nlmsghdr **answer,
+			   bool show_rtnl_err, nl_ext_ack_fn_t errfn)
+{
+	struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };
+	struct msghdr msg = {
+		.msg_name = &nladdr,
+		.msg_namelen = sizeof(nladdr),
+		.msg_iov = msg_iov,
+		.msg_iovlen = iovlen,
+	};
+
+	return __rtnl_talk_msg(rtnl, &msg, answer, show_rtnl_err, errfn);
+}
+
+static int __rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
+		       struct nlmsghdr **answer,
+		       bool show_rtnl_err, nl_ext_ack_fn_t errfn)
+{
+	struct iovec iov = {
+		.iov_base = n,
+		.iov_len = n->nlmsg_len
+	};
+
+	return __rtnl_talk_iov(rtnl, &iov, 1, answer, show_rtnl_err, errfn);
+}
+
 int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 	      struct nlmsghdr **answer)
 {
 	return __rtnl_talk(rtnl, n, answer, true, NULL);
 }
 
+int rtnl_talk_msg(struct rtnl_handle *rtnl, struct msghdr *m,
+		  struct nlmsghdr **answer)
+{
+	return __rtnl_talk_msg(rtnl, m, answer, true, NULL);
+}
+
+int rtnl_talk_iov(struct rtnl_handle *rtnl, struct iovec *iovec, size_t iovlen,
+		  struct nlmsghdr **answer)
+{
+	return __rtnl_talk_iov(rtnl, iovec, iovlen, answer, true, NULL);
+}
+
 int rtnl_talk_extack(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 		     struct nlmsghdr **answer,
 		     nl_ext_ack_fn_t errfn)
-- 
2.14.2

[patch iproute2 v8 2/2] tc: Add batchsize feature for filter and actions

[Chris Mi]

Currently in tc batch mode, only one command is read from the batch
file and sent to kernel to process. With this support, at most 128
commands can be accumulated before sending to kernel.

Now it only works for the following successive commands:
filter and actions add/delete/change/replace.

Signed-off-by: Chris Mi <chrism@mellanox.com>
---
 tc/m_action.c  |  60 +++++++++++++--------
 tc/tc.c        | 165 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 tc/tc_common.h |   5 +-
 tc/tc_filter.c |  97 +++++++++++++++++++--------------
 4 files changed, 249 insertions(+), 78 deletions(-)

diff --git a/tc/m_action.c b/tc/m_action.c
index fc422364..e5c53a80 100644
--- a/tc/m_action.c
+++ b/tc/m_action.c
@@ -546,40 +546,56 @@ bad_val:
 	return ret;
 }
 
+struct tc_action_req {
+	struct nlmsghdr		n;
+	struct tcamsg		t;
+	char			buf[MAX_MSG];
+};
+
 static int tc_action_modify(int cmd, unsigned int flags,
-			    int *argc_p, char ***argv_p)
+			    int *argc_p, char ***argv_p,
+			    void *buf)
 {
-	int argc = *argc_p;
+	struct tc_action_req *req, action_req;
 	char **argv = *argv_p;
+	struct rtattr *tail;
+	int argc = *argc_p;
+	struct iovec iov;
 	int ret = 0;
-	struct {
-		struct nlmsghdr         n;
-		struct tcamsg           t;
-		char                    buf[MAX_MSG];
-	} req = {
-		.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcamsg)),
-		.n.nlmsg_flags = NLM_F_REQUEST | flags,
-		.n.nlmsg_type = cmd,
-		.t.tca_family = AF_UNSPEC,
-	};
-	struct rtattr *tail = NLMSG_TAIL(&req.n);
+
+	if (buf)
+		req = buf;
+	else
+		req = &action_req;
+
+	req->n.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcamsg));
+	req->n.nlmsg_flags = NLM_F_REQUEST | flags;
+	req->n.nlmsg_type = cmd;
+	req->t.tca_family = AF_UNSPEC;
+	tail = NLMSG_TAIL(&req->n);
 
 	argc -= 1;
 	argv += 1;
-	if (parse_action(&argc, &argv, TCA_ACT_TAB, &req.n)) {
+	if (parse_action(&argc, &argv, TCA_ACT_TAB, &req->n)) {
 		fprintf(stderr, "Illegal \"action\"\n");
 		return -1;
 	}
-	tail->rta_len = (void *) NLMSG_TAIL(&req.n) - (void *) tail;
+	tail->rta_len = (void *) NLMSG_TAIL(&req->n) - (void *) tail;
+
+	*argc_p = argc;
+	*argv_p = argv;
 
-	if (rtnl_talk(&rth, &req.n, NULL) < 0) {
+	iov.iov_base = &req->n;
+	iov.iov_len = req->n.nlmsg_len;
+
+	if (buf)
+		return 0;
+
+	if (rtnl_talk_iov(&rth, &iov, 1, NULL) < 0) {
 		fprintf(stderr, "We have an error talking to the kernel\n");
 		ret = -1;
 	}
 
-	*argc_p = argc;
-	*argv_p = argv;
-
 	return ret;
 }
 
@@ -679,7 +695,7 @@ bad_val:
 	return ret;
 }
 
-int do_action(int argc, char **argv)
+int do_action(int argc, char **argv, void *buf)
 {
 
 	int ret = 0;
@@ -689,12 +705,12 @@ int do_action(int argc, char **argv)
 		if (matches(*argv, "add") == 0) {
 			ret =  tc_action_modify(RTM_NEWACTION,
 						NLM_F_EXCL | NLM_F_CREATE,
-						&argc, &argv);
+						&argc, &argv, buf);
 		} else if (matches(*argv, "change") == 0 ||
 			  matches(*argv, "replace") == 0) {
 			ret = tc_action_modify(RTM_NEWACTION,
 					       NLM_F_CREATE | NLM_F_REPLACE,
-					       &argc, &argv);
+					       &argc, &argv, buf);
 		} else if (matches(*argv, "delete") == 0) {
 			argc -= 1;
 			argv += 1;
diff --git a/tc/tc.c b/tc/tc.c
index ad9f07e9..44277405 100644
--- a/tc/tc.c
+++ b/tc/tc.c
@@ -193,16 +193,16 @@ static void usage(void)
 			"                    -nm | -nam[es] | { -cf | -conf } path } | -j[son]\n");
 }
 
-static int do_cmd(int argc, char **argv)
+static int do_cmd(int argc, char **argv, void *buf)
 {
 	if (matches(*argv, "qdisc") == 0)
 		return do_qdisc(argc-1, argv+1);
 	if (matches(*argv, "class") == 0)
 		return do_class(argc-1, argv+1);
 	if (matches(*argv, "filter") == 0)
-		return do_filter(argc-1, argv+1);
+		return do_filter(argc-1, argv+1, buf);
 	if (matches(*argv, "actions") == 0)
-		return do_action(argc-1, argv+1);
+		return do_action(argc-1, argv+1, buf);
 	if (matches(*argv, "monitor") == 0)
 		return do_tcmonitor(argc-1, argv+1);
 	if (matches(*argv, "exec") == 0)
@@ -217,11 +217,79 @@ static int do_cmd(int argc, char **argv)
 	return -1;
 }
 
+static bool batchsize_enabled(int argc, char *argv[])
+{
+	if (argc < 2)
+		return false;
+	if ((matches(argv[0], "filter") && matches(argv[0], "actions"))
+	    || (matches(argv[1], "add") && matches(argv[1], "delete")
+	    && matches(argv[1], "change") && matches(argv[1], "replace")))
+		return false;
+	return true;
+}
+
+struct batch_buf {
+	struct batch_buf	*next;
+	char			buf[16420];	/* sizeof (struct nlmsghdr) +
+						   sizeof (struct tcmsg) +
+						   MAX_MSG */
+};
+
+static struct batch_buf *get_batch_buf(struct batch_buf **pool)
+{
+	struct batch_buf *buf;
+
+	if (*pool == NULL)
+		buf = calloc(1, sizeof (struct batch_buf));
+	else {
+		buf = *pool;
+		*pool = (*pool)->next;
+		memset(buf, 0, sizeof (struct batch_buf));
+	}
+	return buf;
+}
+
+static void put_batch_bufs(struct batch_buf **pool,
+			   struct batch_buf **head, struct batch_buf **tail)
+{
+	if (*head == NULL || *tail == NULL)
+		return;
+
+	if (*pool == NULL)
+		*pool = *head;
+	else {
+		(*tail)->next = *pool;
+		*pool = *head;
+	}
+	*head = NULL;
+	*tail = NULL;
+}
+
+static void free_batch_bufs(struct batch_buf **pool)
+{
+	struct batch_buf *buf;
+
+	for (buf = *pool; buf != NULL; buf = *pool) {
+		*pool = buf->next;
+		free(buf);
+	}
+	*pool = NULL;
+}
+
 static int batch(const char *name)
 {
-	char *line = NULL;
+	struct batch_buf *head = NULL, *tail = NULL, *buf_pool = NULL;
+	char *largv[100], *largv_next[100];
+	char *line, *line_next = NULL;
+	bool bs_enabled_next = false;
+	bool bs_enabled = false;
+	bool lastline = false;
+	int largc, largc_next;
+	bool bs_enabled_saved;
+	int batchsize = 0;
 	size_t len = 0;
 	int ret = 0;
+	bool send;
 
 	batch_mode = 1;
 	if (name && strcmp(name, "-") != 0) {
@@ -240,25 +308,94 @@ static int batch(const char *name)
 	}
 
 	cmdlineno = 0;
-	while (getcmdline(&line, &len, stdin) != -1) {
-		char *largv[100];
-		int largc;
+	if (getcmdline(&line, &len, stdin) == -1)
+		goto Exit;
+	largc = makeargs(line, largv, 100);
+	bs_enabled = batchsize_enabled(largc, largv);
+	bs_enabled_saved = bs_enabled;
+	do {
+		if (getcmdline(&line_next, &len, stdin) == -1)
+			lastline = true;
+
+		largc_next = makeargs(line_next, largv_next, 100);
+		bs_enabled_next = batchsize_enabled(largc_next, largv_next);
+		if (bs_enabled) {
+			struct batch_buf *buf;
+			buf = get_batch_buf(&buf_pool);
+			if (!buf) {
+				fprintf(stderr, "failed to allocate batch_buf\n");
+				return -1;
+			}
+			if (head == NULL)
+				head = tail = buf;
+			else {
+				tail->next = buf;
+				tail = buf;
+			}
+			++batchsize;
+		}
+
+		/*
+		 * In batch mode, if we haven't accumulated enough commands
+		 * and this is not the last command and this command & next
+		 * command both support the batchsize feature, don't send the
+		 * message immediately.
+		 */
+		if (!lastline && bs_enabled && bs_enabled_next
+		    && batchsize != MSG_IOV_MAX)
+			send = false;
+		else
+			send = true;
+
+		line = line_next;
+		line_next = NULL;
+		len = 0;
+		bs_enabled_saved = bs_enabled;
+		bs_enabled = bs_enabled_next;
+		bs_enabled_next = false;
 
-		largc = makeargs(line, largv, 100);
 		if (largc == 0)
 			continue;	/* blank line */
 
-		if (do_cmd(largc, largv)) {
-			fprintf(stderr, "Command failed %s:%d\n", name, cmdlineno);
+		ret = do_cmd(largc, largv, tail == NULL ? NULL : tail->buf);
+		if (ret != 0) {
+			fprintf(stderr, "Command failed %s:%d\n", name,
+				cmdlineno - 1);
 			ret = 1;
 			if (!force)
 				break;
 		}
-	}
-	if (line)
-		free(line);
+		largc = largc_next;
+		memcpy(largv, largv_next, largc * sizeof(char *));
+
+		if (send && bs_enabled_saved) {
+			struct iovec *iov, *iovs;
+			struct batch_buf *buf;
+			struct nlmsghdr *n;
+			iov = iovs = malloc(batchsize * sizeof (struct iovec));
+			for (buf = head; buf != NULL; buf = buf->next, ++iov) {
+				n = (struct nlmsghdr *)&buf->buf;
+				iov->iov_base = n;
+				iov->iov_len = n->nlmsg_len;
+			}
+
+			ret = rtnl_talk_iov(&rth, iovs, batchsize, NULL);
+			if (ret < 0) {
+				fprintf(stderr, "Command failed %s:%d\n", name,
+					cmdlineno - (batchsize + ret) - 1);
+				return 2;
+			}
+			put_batch_bufs(&buf_pool, &head, &tail);
+			batchsize = 0;
+			free(iovs);
+		}
+	} while (!lastline);
 
+	free_batch_bufs(&buf_pool);
+Exit:
+	free(line);
 	rtnl_close(&rth);
+
 	return ret;
 }
 
@@ -341,7 +478,7 @@ int main(int argc, char **argv)
 		goto Exit;
 	}
 
-	ret = do_cmd(argc-1, argv+1);
+	ret = do_cmd(argc-1, argv+1, NULL);
 Exit:
 	rtnl_close(&rth);
 
diff --git a/tc/tc_common.h b/tc/tc_common.h
index 264fbdac..59018da5 100644
--- a/tc/tc_common.h
+++ b/tc/tc_common.h
@@ -1,13 +1,14 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 
 #define TCA_BUF_MAX	(64*1024)
+#define MSG_IOV_MAX	128
 
 extern struct rtnl_handle rth;
 
 extern int do_qdisc(int argc, char **argv);
 extern int do_class(int argc, char **argv);
-extern int do_filter(int argc, char **argv);
-extern int do_action(int argc, char **argv);
+extern int do_filter(int argc, char **argv, void *buf);
+extern int do_action(int argc, char **argv, void *buf);
 extern int do_tcmonitor(int argc, char **argv);
 extern int do_exec(int argc, char **argv);
 
diff --git a/tc/tc_filter.c b/tc/tc_filter.c
index 545cc3a1..7db4964b 100644
--- a/tc/tc_filter.c
+++ b/tc/tc_filter.c
@@ -42,28 +42,38 @@ static void usage(void)
 		"OPTIONS := ... try tc filter add <desired FILTER_KIND> help\n");
 }
 
-static int tc_filter_modify(int cmd, unsigned int flags, int argc, char **argv)
+struct tc_filter_req {
+	struct nlmsghdr		n;
+	struct tcmsg		t;
+	char			buf[MAX_MSG];
+};
+
+static int tc_filter_modify(int cmd, unsigned int flags, int argc, char **argv,
+			    void *buf)
 {
-	struct {
-		struct nlmsghdr	n;
-		struct tcmsg		t;
-		char			buf[MAX_MSG];
-	} req = {
-		.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcmsg)),
-		.n.nlmsg_flags = NLM_F_REQUEST | flags,
-		.n.nlmsg_type = cmd,
-		.t.tcm_family = AF_UNSPEC,
-	};
+	struct tc_filter_req *req, filter_req;
 	struct filter_util *q = NULL;
-	__u32 prio = 0;
-	__u32 protocol = 0;
-	int protocol_set = 0;
-	__u32 chain_index;
+	struct tc_estimator est = {};
+	char k[FILTER_NAMESZ] = {};
 	int chain_index_set = 0;
+	char d[IFNAMSIZ] = {};
+	int protocol_set = 0;
 	char *fhandle = NULL;
-	char  d[IFNAMSIZ] = {};
-	char  k[FILTER_NAMESZ] = {};
-	struct tc_estimator est = {};
+	__u32 protocol = 0;
+	__u32 chain_index;
+	struct iovec iov;
+	__u32 prio = 0;
+	int ret;
+
+	if (buf)
+		req = buf;
+	else
+		req = &filter_req;
+
+	req->n.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcmsg));
+	req->n.nlmsg_flags = NLM_F_REQUEST | flags;
+	req->n.nlmsg_type = cmd;
+	req->t.tcm_family = AF_UNSPEC;
 
 	if (cmd == RTM_NEWTFILTER && flags & NLM_F_CREATE)
 		protocol = htons(ETH_P_ALL);
@@ -75,37 +85,37 @@ static int tc_filter_modify(int cmd, unsigned int flags, int argc, char **argv)
 				duparg("dev", *argv);
 			strncpy(d, *argv, sizeof(d)-1);
 		} else if (strcmp(*argv, "root") == 0) {
-			if (req.t.tcm_parent) {
+			if (req->t.tcm_parent) {
 				fprintf(stderr,
 					"Error: \"root\" is duplicate parent ID\n");
 				return -1;
 			}
-			req.t.tcm_parent = TC_H_ROOT;
+			req->t.tcm_parent = TC_H_ROOT;
 		} else if (strcmp(*argv, "ingress") == 0) {
-			if (req.t.tcm_parent) {
+			if (req->t.tcm_parent) {
 				fprintf(stderr,
 					"Error: \"ingress\" is duplicate parent ID\n");
 				return -1;
 			}
-			req.t.tcm_parent = TC_H_MAKE(TC_H_CLSACT,
+			req->t.tcm_parent = TC_H_MAKE(TC_H_CLSACT,
 						     TC_H_MIN_INGRESS);
 		} else if (strcmp(*argv, "egress") == 0) {
-			if (req.t.tcm_parent) {
+			if (req->t.tcm_parent) {
 				fprintf(stderr,
 					"Error: \"egress\" is duplicate parent ID\n");
 				return -1;
 			}
-			req.t.tcm_parent = TC_H_MAKE(TC_H_CLSACT,
+			req->t.tcm_parent = TC_H_MAKE(TC_H_CLSACT,
 						     TC_H_MIN_EGRESS);
 		} else if (strcmp(*argv, "parent") == 0) {
 			__u32 handle;
 
 			NEXT_ARG();
-			if (req.t.tcm_parent)
+			if (req->t.tcm_parent)
 				duparg("parent", *argv);
 			if (get_tc_classid(&handle, *argv))
 				invarg("Invalid parent ID", *argv);
-			req.t.tcm_parent = handle;
+			req->t.tcm_parent = handle;
 		} else if (strcmp(*argv, "handle") == 0) {
 			NEXT_ARG();
 			if (fhandle)
@@ -152,26 +162,26 @@ static int tc_filter_modify(int cmd, unsigned int flags, int argc, char **argv)
 		argc--; argv++;
 	}
 
-	req.t.tcm_info = TC_H_MAKE(prio<<16, protocol);
+	req->t.tcm_info = TC_H_MAKE(prio<<16, protocol);
 
 	if (chain_index_set)
-		addattr32(&req.n, sizeof(req), TCA_CHAIN, chain_index);
+		addattr32(&req->n, sizeof(*req), TCA_CHAIN, chain_index);
 
 	if (k[0])
-		addattr_l(&req.n, sizeof(req), TCA_KIND, k, strlen(k)+1);
+		addattr_l(&req->n, sizeof(*req), TCA_KIND, k, strlen(k)+1);
 
 	if (d[0])  {
 		ll_init_map(&rth);
 
-		req.t.tcm_ifindex = ll_name_to_index(d);
-		if (req.t.tcm_ifindex == 0) {
+		req->t.tcm_ifindex = ll_name_to_index(d);
+		if (req->t.tcm_ifindex == 0) {
 			fprintf(stderr, "Cannot find device \"%s\"\n", d);
 			return 1;
 		}
 	}
 
 	if (q) {
-		if (q->parse_fopt(q, fhandle, argc, argv, &req.n))
+		if (q->parse_fopt(q, fhandle, argc, argv, &req->n))
 			return 1;
 	} else {
 		if (fhandle) {
@@ -190,10 +200,17 @@ static int tc_filter_modify(int cmd, unsigned int flags, int argc, char **argv)
 	}
 
 	if (est.ewma_log)
-		addattr_l(&req.n, sizeof(req), TCA_RATE, &est, sizeof(est));
+		addattr_l(&req->n, sizeof(*req), TCA_RATE, &est, sizeof(est));
 
-	if (rtnl_talk(&rth, &req.n, NULL) < 0) {
-		fprintf(stderr, "We have an error talking to the kernel\n");
+	iov.iov_base = &req->n;
+	iov.iov_len = req->n.nlmsg_len;
+
+	if (buf)
+		return 0;
+
+	ret = rtnl_talk_iov(&rth, &iov, 1, NULL);
+	if (ret < 0) {
+		fprintf(stderr, "We have an error talking to the kernel, %d\n", ret);
 		return 2;
 	}
 
@@ -636,20 +653,20 @@ static int tc_filter_list(int argc, char **argv)
 	return 0;
 }
 
-int do_filter(int argc, char **argv)
+int do_filter(int argc, char **argv, void *buf)
 {
 	if (argc < 1)
 		return tc_filter_list(0, NULL);
 	if (matches(*argv, "add") == 0)
 		return tc_filter_modify(RTM_NEWTFILTER, NLM_F_EXCL|NLM_F_CREATE,
-					argc-1, argv+1);
+					argc-1, argv+1, buf);
 	if (matches(*argv, "change") == 0)
-		return tc_filter_modify(RTM_NEWTFILTER, 0, argc-1, argv+1);
+		return tc_filter_modify(RTM_NEWTFILTER, 0, argc-1, argv+1, buf);
 	if (matches(*argv, "replace") == 0)
 		return tc_filter_modify(RTM_NEWTFILTER, NLM_F_CREATE, argc-1,
-					argv+1);
+					argv+1, buf);
 	if (matches(*argv, "delete") == 0)
-		return tc_filter_modify(RTM_DELTFILTER, 0,  argc-1, argv+1);
+		return tc_filter_modify(RTM_DELTFILTER, 0, argc-1, argv+1, buf);
 	if (matches(*argv, "get") == 0)
 		return tc_filter_get(RTM_GETTFILTER, 0,  argc-1, argv+1);
 	if (matches(*argv, "list") == 0 || matches(*argv, "show") == 0
-- 
2.14.2

Re: [patch iproute2 v8 2/2] tc: Add batchsize feature for filter and actions

[Marcelo Ricardo Leitner]

On Wed, Jan 10, 2018 at 12:27:42PM +0900, Chris Mi wrote:
> Currently in tc batch mode, only one command is read from the batch
> file and sent to kernel to process. With this support, at most 128
> commands can be accumulated before sending to kernel.
> 
> Now it only works for the following successive commands:
> filter and actions add/delete/change/replace.
> 
> Signed-off-by: Chris Mi <chrism@mellanox.com>
> ---
>  tc/m_action.c  |  60 +++++++++++++--------
>  tc/tc.c        | 165 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  tc/tc_common.h |   5 +-
>  tc/tc_filter.c |  97 +++++++++++++++++++--------------
>  4 files changed, 249 insertions(+), 78 deletions(-)
> 
> diff --git a/tc/m_action.c b/tc/m_action.c
> index fc422364..e5c53a80 100644
> --- a/tc/m_action.c
> +++ b/tc/m_action.c
> @@ -546,40 +546,56 @@ bad_val:
>  	return ret;
>  }
>  
> +struct tc_action_req {
> +	struct nlmsghdr		n;
> +	struct tcamsg		t;
> +	char			buf[MAX_MSG];
> +};
> +
>  static int tc_action_modify(int cmd, unsigned int flags,
> -			    int *argc_p, char ***argv_p)
> +			    int *argc_p, char ***argv_p,
> +			    void *buf)
>  {
> -	int argc = *argc_p;
> +	struct tc_action_req *req, action_req;
>  	char **argv = *argv_p;
> +	struct rtattr *tail;
> +	int argc = *argc_p;
> +	struct iovec iov;
>  	int ret = 0;
> -	struct {
> -		struct nlmsghdr         n;
> -		struct tcamsg           t;
> -		char                    buf[MAX_MSG];
> -	} req = {
> -		.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcamsg)),
> -		.n.nlmsg_flags = NLM_F_REQUEST | flags,
> -		.n.nlmsg_type = cmd,
> -		.t.tca_family = AF_UNSPEC,
> -	};
> -	struct rtattr *tail = NLMSG_TAIL(&req.n);
> +
> +	if (buf)
> +		req = buf;
> +	else
> +		req = &action_req;
> +
> +	req->n.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcamsg));
> +	req->n.nlmsg_flags = NLM_F_REQUEST | flags;
> +	req->n.nlmsg_type = cmd;
> +	req->t.tca_family = AF_UNSPEC;
> +	tail = NLMSG_TAIL(&req->n);
>  
>  	argc -= 1;
>  	argv += 1;
> -	if (parse_action(&argc, &argv, TCA_ACT_TAB, &req.n)) {
> +	if (parse_action(&argc, &argv, TCA_ACT_TAB, &req->n)) {
>  		fprintf(stderr, "Illegal \"action\"\n");
>  		return -1;
>  	}
> -	tail->rta_len = (void *) NLMSG_TAIL(&req.n) - (void *) tail;
> +	tail->rta_len = (void *) NLMSG_TAIL(&req->n) - (void *) tail;
> +
> +	*argc_p = argc;
> +	*argv_p = argv;
>  
> -	if (rtnl_talk(&rth, &req.n, NULL) < 0) {
> +	iov.iov_base = &req->n;
> +	iov.iov_len = req->n.nlmsg_len;
> +
> +	if (buf)
> +		return 0;
> +
> +	if (rtnl_talk_iov(&rth, &iov, 1, NULL) < 0) {
>  		fprintf(stderr, "We have an error talking to the kernel\n");
>  		ret = -1;
>  	}
>  
> -	*argc_p = argc;
> -	*argv_p = argv;
> -
>  	return ret;
>  }
>  
> @@ -679,7 +695,7 @@ bad_val:
>  	return ret;
>  }
>  
> -int do_action(int argc, char **argv)
> +int do_action(int argc, char **argv, void *buf)
>  {
>  
>  	int ret = 0;
> @@ -689,12 +705,12 @@ int do_action(int argc, char **argv)
>  		if (matches(*argv, "add") == 0) {
>  			ret =  tc_action_modify(RTM_NEWACTION,
>  						NLM_F_EXCL | NLM_F_CREATE,
> -						&argc, &argv);
> +						&argc, &argv, buf);
>  		} else if (matches(*argv, "change") == 0 ||
>  			  matches(*argv, "replace") == 0) {
>  			ret = tc_action_modify(RTM_NEWACTION,
>  					       NLM_F_CREATE | NLM_F_REPLACE,
> -					       &argc, &argv);
> +					       &argc, &argv, buf);
>  		} else if (matches(*argv, "delete") == 0) {
>  			argc -= 1;
>  			argv += 1;
> diff --git a/tc/tc.c b/tc/tc.c
> index ad9f07e9..44277405 100644
> --- a/tc/tc.c
> +++ b/tc/tc.c
> @@ -193,16 +193,16 @@ static void usage(void)
>  			"                    -nm | -nam[es] | { -cf | -conf } path } | -j[son]\n");
>  }
>  
> -static int do_cmd(int argc, char **argv)
> +static int do_cmd(int argc, char **argv, void *buf)
>  {
>  	if (matches(*argv, "qdisc") == 0)
>  		return do_qdisc(argc-1, argv+1);
>  	if (matches(*argv, "class") == 0)
>  		return do_class(argc-1, argv+1);
>  	if (matches(*argv, "filter") == 0)
> -		return do_filter(argc-1, argv+1);
> +		return do_filter(argc-1, argv+1, buf);
>  	if (matches(*argv, "actions") == 0)
> -		return do_action(argc-1, argv+1);
> +		return do_action(argc-1, argv+1, buf);
>  	if (matches(*argv, "monitor") == 0)
>  		return do_tcmonitor(argc-1, argv+1);
>  	if (matches(*argv, "exec") == 0)
> @@ -217,11 +217,79 @@ static int do_cmd(int argc, char **argv)
>  	return -1;
>  }
>  
> +static bool batchsize_enabled(int argc, char *argv[])
> +{
> +	if (argc < 2)
> +		return false;
> +	if ((matches(argv[0], "filter") && matches(argv[0], "actions"))
> +	    || (matches(argv[1], "add") && matches(argv[1], "delete")
> +	    && matches(argv[1], "change") && matches(argv[1], "replace")))
> +		return false;
> +	return true;
> +}
> +
> +struct batch_buf {
> +	struct batch_buf	*next;
> +	char			buf[16420];	/* sizeof (struct nlmsghdr) +
> +						   sizeof (struct tcmsg) +
> +						   MAX_MSG */
> +};
> +
> +static struct batch_buf *get_batch_buf(struct batch_buf **pool)
> +{
> +	struct batch_buf *buf;
> +
> +	if (*pool == NULL)
> +		buf = calloc(1, sizeof (struct batch_buf));
> +	else {
> +		buf = *pool;
> +		*pool = (*pool)->next;
> +		memset(buf, 0, sizeof (struct batch_buf));
> +	}
> +	return buf;
> +}
> +
> +static void put_batch_bufs(struct batch_buf **pool,
> +			   struct batch_buf **head, struct batch_buf **tail)
> +{
> +	if (*head == NULL || *tail == NULL)
> +		return;
> +
> +	if (*pool == NULL)
> +		*pool = *head;
> +	else {
> +		(*tail)->next = *pool;
> +		*pool = *head;
> +	}
> +	*head = NULL;
> +	*tail = NULL;
> +}
> +
> +static void free_batch_bufs(struct batch_buf **pool)
> +{
> +	struct batch_buf *buf;
> +
> +	for (buf = *pool; buf != NULL; buf = *pool) {
> +		*pool = buf->next;
> +		free(buf);
> +	}
> +	*pool = NULL;
> +}
> +
>  static int batch(const char *name)
>  {
> -	char *line = NULL;
> +	struct batch_buf *head = NULL, *tail = NULL, *buf_pool = NULL;
> +	char *largv[100], *largv_next[100];
> +	char *line, *line_next = NULL;
> +	bool bs_enabled_next = false;
> +	bool bs_enabled = false;
> +	bool lastline = false;
> +	int largc, largc_next;
> +	bool bs_enabled_saved;
> +	int batchsize = 0;
>  	size_t len = 0;
>  	int ret = 0;
> +	bool send;
>  
>  	batch_mode = 1;
>  	if (name && strcmp(name, "-") != 0) {
> @@ -240,25 +308,94 @@ static int batch(const char *name)
>  	}
>  
>  	cmdlineno = 0;
> -	while (getcmdline(&line, &len, stdin) != -1) {
> -		char *largv[100];
> -		int largc;
> +	if (getcmdline(&line, &len, stdin) == -1)
> +		goto Exit;
> +	largc = makeargs(line, largv, 100);
> +	bs_enabled = batchsize_enabled(largc, largv);
> +	bs_enabled_saved = bs_enabled;
> +	do {
> +		if (getcmdline(&line_next, &len, stdin) == -1)
> +			lastline = true;
> +
> +		largc_next = makeargs(line_next, largv_next, 100);
> +		bs_enabled_next = batchsize_enabled(largc_next, largv_next);
> +		if (bs_enabled) {
> +			struct batch_buf *buf;
> +			buf = get_batch_buf(&buf_pool);
> +			if (!buf) {
> +				fprintf(stderr, "failed to allocate batch_buf\n");
> +				return -1;
> +			}
> +			if (head == NULL)
> +				head = tail = buf;
> +			else {
> +				tail->next = buf;
> +				tail = buf;
> +			}

What about moving this list handling to get_batch_buf(), like you did
for put_batch_buf() ? Just for the sake of consistency.

Otherwise LGTM!

  Marcelo

Re: [patch iproute2 v8 1/2] lib/libnetlink: Add functions rtnl_talk_msg and rtnl_talk_iov

[David Ahern]

[-- Attachment #1: Type: text/plain, Size: 3618 bytes --]

On 1/9/18 8:27 PM, Chris Mi wrote:
> rtnl_talk can only send a single message to kernel. Add two functions
> rtnl_talk_msg and rtnl_talk_iov that can send multiple messages to kernel.
> rtnl_talk_msg takes struct msghdr * as argument.
> rtnl_talk_iov takes struct iovec * and iovlen as arguments.
> 
> Signed-off-by: Chris Mi <chrism@mellanox.com>
> ---
>  include/libnetlink.h |  6 ++++
>  lib/libnetlink.c     | 82 ++++++++++++++++++++++++++++++++++++++++------------
>  2 files changed, 70 insertions(+), 18 deletions(-)
> 
> diff --git a/include/libnetlink.h b/include/libnetlink.h
> index a4d83b9e..e9a63dbc 100644
> --- a/include/libnetlink.h
> +++ b/include/libnetlink.h
> @@ -96,6 +96,12 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
>  int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
>  	      struct nlmsghdr **answer)
>  	__attribute__((warn_unused_result));
> +int rtnl_talk_msg(struct rtnl_handle *rtnl, struct msghdr *m,
> +		  struct nlmsghdr **answer)
> +	__attribute__((warn_unused_result));

As mentioned before rtnl_talk_msg is not needed; you only need to add
rtnl_talk_iov. The attached fixup on top of your patch removes it and
adjusts __rtnl_talk_iov. Please roll that change into your patch.


While testing this I noticed 2 other oddities:

$ perf trace -s tc -b tc.batch
(stddev column removed to shorten line width)

 Summary of events:

 tc (780), 1857 events, 97.9%

   syscall            calls    total       min       avg       max
                               (msec)    (msec)    (msec)    (msec)
   --------------- -------- --------- --------- --------- ---------
   recvmsg              530     6.532     0.008     0.012     0.218
   open                 269     5.429     0.012     0.020     0.117
   sendmsg                4     3.518     0.092     0.879     1.647



1. recvmsg is called twice - once to peek at message size, allocate a
buffer and then really receive the message. That is overkill for ACKs.

2. I am using a batch file with drop filters:

filter add dev eth2 ingress protocol ip pref 273 flower dst_ip
192.168.253.0/16 action drop

and for each command tc is trying to dlopen m_drop.so:

open("/usr/lib/tc//m_drop.so", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such
file or directory)


With a patch to use a stack buffer for ACKs, the above perf summary becomes:

$ perf trace -s tc -b tc.batch

 Summary of events:

 tc (777), 1345 events, 97.1%

   syscall            calls    total       min       avg       max
                               (msec)    (msec)    (msec)    (msec)
   --------------- -------- --------- --------- --------- ---------
   open                 269     5.510     0.013     0.020     0.160
   recvmsg              274     3.758     0.009     0.014     0.396
   sendmsg                4     3.531     0.098     0.883     1.672


Making the open errors now the dominate overhead affecting performance.
If tc had some smarts that it already tried that file it would avoid the
subsequent open calls. The end result is a significant speed up compared
to the current tc:

 Summary of events:

 tc (785), 2333 events, 98.3%

   syscall            calls    total       min       avg       max
                               (msec)    (msec)    (msec)    (msec)
   --------------- -------- --------- --------- --------- ---------
   sendmsg              256     9.832     0.029     0.038     0.181
   open                 269     5.819     0.013     0.022     0.353
   recvmsg              530     5.592     0.009     0.011     0.285


Can you look at a follow on patch (not part of this set) to cache status
of dlopen attempts?

[-- Attachment #2: rtnl_talk_fixup.patch --]
[-- Type: text/plain, Size: 3173 bytes --]

diff --git a/include/libnetlink.h b/include/libnetlink.h
index e9a63dbca259..d6322190afaa 100644
--- a/include/libnetlink.h
+++ b/include/libnetlink.h
@@ -96,9 +96,6 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
 int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 	      struct nlmsghdr **answer)
 	__attribute__((warn_unused_result));
-int rtnl_talk_msg(struct rtnl_handle *rtnl, struct msghdr *m,
-		  struct nlmsghdr **answer)
-	__attribute__((warn_unused_result));
 int rtnl_talk_iov(struct rtnl_handle *rtnl, struct iovec *iovec, size_t iovlen,
 		  struct nlmsghdr **answer)
 	__attribute__((warn_unused_result));
diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index 183825c7fe0e..cb1990fcbf1c 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -581,38 +581,40 @@ static void rtnl_talk_error(struct nlmsghdr *h, struct nlmsgerr *err,
 		strerror(-err->error));
 }
 
-static int __rtnl_talk_msg(struct rtnl_handle *rtnl, struct msghdr *m,
-			   struct nlmsghdr **answer,
+
+static int __rtnl_talk_iov(struct rtnl_handle *rtnl, struct iovec *iov,
+			   size_t iovlen, struct nlmsghdr **answer,
 			   bool show_rtnl_err, nl_ext_ack_fn_t errfn)
 {
 	struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };
-	int i, status, iovlen = m->msg_iovlen;
-	struct iovec iov;
+	int i, status;
+	struct iovec riov;
 	struct msghdr msg = {
 		.msg_name = &nladdr,
 		.msg_namelen = sizeof(nladdr),
-		.msg_iov = &iov,
-		.msg_iovlen = 1,
+		.msg_iov = iov,
+		.msg_iovlen = iovlen,
 	};
 	unsigned int seq = 0;
 	struct nlmsghdr *h;
 	char *buf;
 
 	for (i = 0; i < iovlen; i++) {
-		struct iovec *v;
-		v = &m->msg_iov[i];
-		h = v->iov_base;
+		h = iov[i].iov_base;
 		h->nlmsg_seq = seq = ++rtnl->seq;
 		if (answer == NULL)
 			h->nlmsg_flags |= NLM_F_ACK;
 	}
 
-	status = sendmsg(rtnl->fd, m, 0);
+	status = sendmsg(rtnl->fd, &msg, 0);
 	if (status < 0) {
 		perror("Cannot talk to rtnetlink");
 		return -1;
 	}
 
+	/* change msg to use the response iov */
+	msg.msg_iov = &riov;
+	msg.msg_iovlen = 1;
 	i = 0;
 	while (1) {
 next:
@@ -705,21 +707,6 @@ static int __rtnl_talk_msg(struct rtnl_handle *rtnl, struct msghdr *m,
 	}
 }
 
-static int __rtnl_talk_iov(struct rtnl_handle *rtnl, struct iovec *msg_iov,
-			   size_t iovlen, struct nlmsghdr **answer,
-			   bool show_rtnl_err, nl_ext_ack_fn_t errfn)
-{
-	struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };
-	struct msghdr msg = {
-		.msg_name = &nladdr,
-		.msg_namelen = sizeof(nladdr),
-		.msg_iov = msg_iov,
-		.msg_iovlen = iovlen,
-	};
-
-	return __rtnl_talk_msg(rtnl, &msg, answer, show_rtnl_err, errfn);
-}
-
 static int __rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 		       struct nlmsghdr **answer,
 		       bool show_rtnl_err, nl_ext_ack_fn_t errfn)
@@ -738,12 +725,6 @@ int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 	return __rtnl_talk(rtnl, n, answer, true, NULL);
 }
 
-int rtnl_talk_msg(struct rtnl_handle *rtnl, struct msghdr *m,
-		  struct nlmsghdr **answer)
-{
-	return __rtnl_talk_msg(rtnl, m, answer, true, NULL);
-}
-
 int rtnl_talk_iov(struct rtnl_handle *rtnl, struct iovec *iovec, size_t iovlen,
 		  struct nlmsghdr **answer)
 {

Re: [patch iproute2 v8 2/2] tc: Add batchsize feature for filter and actions

[David Ahern]

On 1/9/18 8:27 PM, Chris Mi wrote:
> Currently in tc batch mode, only one command is read from the batch
> file and sent to kernel to process. With this support, at most 128
> commands can be accumulated before sending to kernel.
> 
> Now it only works for the following successive commands:
> filter and actions add/delete/change/replace.
> 
> Signed-off-by: Chris Mi <chrism@mellanox.com>
> ---
>  tc/m_action.c  |  60 +++++++++++++--------
>  tc/tc.c        | 165 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  tc/tc_common.h |   5 +-
>  tc/tc_filter.c |  97 +++++++++++++++++++--------------
>  4 files changed, 249 insertions(+), 78 deletions(-)
> 
> diff --git a/tc/m_action.c b/tc/m_action.c
> index fc422364..e5c53a80 100644
> --- a/tc/m_action.c
> +++ b/tc/m_action.c
> @@ -546,40 +546,56 @@ bad_val:
>  	return ret;
>  }
>  
> +struct tc_action_req {
> +	struct nlmsghdr		n;
> +	struct tcamsg		t;
> +	char			buf[MAX_MSG];
> +};
> +
>  static int tc_action_modify(int cmd, unsigned int flags,
> -			    int *argc_p, char ***argv_p)
> +			    int *argc_p, char ***argv_p,
> +			    void *buf)

you really need a buflen; you should not make assumptions about the
length of buffer passed to these functions.

>  {
> -	int argc = *argc_p;
> +	struct tc_action_req *req, action_req;
>  	char **argv = *argv_p;
> +	struct rtattr *tail;
> +	int argc = *argc_p;
> +	struct iovec iov;
>  	int ret = 0;
> -	struct {
> -		struct nlmsghdr         n;
> -		struct tcamsg           t;
> -		char                    buf[MAX_MSG];
> -	} req = {
> -		.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcamsg)),
> -		.n.nlmsg_flags = NLM_F_REQUEST | flags,
> -		.n.nlmsg_type = cmd,
> -		.t.tca_family = AF_UNSPEC,
> -	};
> -	struct rtattr *tail = NLMSG_TAIL(&req.n);
> +
> +	if (buf)
> +		req = buf;
> +	else
> +		req = &action_req;
> +

And a memset is needed for the !buf path since action_req is not
initialized.

> +	req->n.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcamsg));
> +	req->n.nlmsg_flags = NLM_F_REQUEST | flags;
> +	req->n.nlmsg_type = cmd;
> +	req->t.tca_family = AF_UNSPEC;
> +	tail = NLMSG_TAIL(&req->n);
>  
>  	argc -= 1;
>  	argv += 1;
> -	if (parse_action(&argc, &argv, TCA_ACT_TAB, &req.n)) {
> +	if (parse_action(&argc, &argv, TCA_ACT_TAB, &req->n)) {
>  		fprintf(stderr, "Illegal \"action\"\n");
>  		return -1;
>  	}
> -	tail->rta_len = (void *) NLMSG_TAIL(&req.n) - (void *) tail;
> +	tail->rta_len = (void *) NLMSG_TAIL(&req->n) - (void *) tail;
> +
> +	*argc_p = argc;
> +	*argv_p = argv;
>  
> -	if (rtnl_talk(&rth, &req.n, NULL) < 0) {
> +	iov.iov_base = &req->n;
> +	iov.iov_len = req->n.nlmsg_len;

you can leave that as rtnl_talk; no need to change the !buf case to
rtnl_talk_iov.

> +
> +	if (buf)
> +		return 0;
> +
> +	if (rtnl_talk_iov(&rth, &iov, 1, NULL) < 0) {
>  		fprintf(stderr, "We have an error talking to the kernel\n");
>  		ret = -1;
>  	}
>  
> -	*argc_p = argc;
> -	*argv_p = argv;
> -
>  	return ret;
>  }
>  
> @@ -679,7 +695,7 @@ bad_val:
>  	return ret;
>  }
>  
> -int do_action(int argc, char **argv)
> +int do_action(int argc, char **argv, void *buf)
>  {
>  
>  	int ret = 0;
> @@ -689,12 +705,12 @@ int do_action(int argc, char **argv)
>  		if (matches(*argv, "add") == 0) {
>  			ret =  tc_action_modify(RTM_NEWACTION,
>  						NLM_F_EXCL | NLM_F_CREATE,
> -						&argc, &argv);
> +						&argc, &argv, buf);
>  		} else if (matches(*argv, "change") == 0 ||
>  			  matches(*argv, "replace") == 0) {
>  			ret = tc_action_modify(RTM_NEWACTION,
>  					       NLM_F_CREATE | NLM_F_REPLACE,
> -					       &argc, &argv);
> +					       &argc, &argv, buf);
>  		} else if (matches(*argv, "delete") == 0) {
>  			argc -= 1;
>  			argv += 1;
> diff --git a/tc/tc.c b/tc/tc.c
> index ad9f07e9..44277405 100644
> --- a/tc/tc.c
> +++ b/tc/tc.c
> @@ -193,16 +193,16 @@ static void usage(void)
>  			"                    -nm | -nam[es] | { -cf | -conf } path } | -j[son]\n");
>  }
>  
> -static int do_cmd(int argc, char **argv)
> +static int do_cmd(int argc, char **argv, void *buf)
>  {
>  	if (matches(*argv, "qdisc") == 0)
>  		return do_qdisc(argc-1, argv+1);
>  	if (matches(*argv, "class") == 0)
>  		return do_class(argc-1, argv+1);
>  	if (matches(*argv, "filter") == 0)
> -		return do_filter(argc-1, argv+1);
> +		return do_filter(argc-1, argv+1, buf);
>  	if (matches(*argv, "actions") == 0)
> -		return do_action(argc-1, argv+1);
> +		return do_action(argc-1, argv+1, buf);
>  	if (matches(*argv, "monitor") == 0)
>  		return do_tcmonitor(argc-1, argv+1);
>  	if (matches(*argv, "exec") == 0)
> @@ -217,11 +217,79 @@ static int do_cmd(int argc, char **argv)
>  	return -1;
>  }
>  
> +static bool batchsize_enabled(int argc, char *argv[])
> +{
> +	if (argc < 2)
> +		return false;
> +	if ((matches(argv[0], "filter") && matches(argv[0], "actions"))
> +	    || (matches(argv[1], "add") && matches(argv[1], "delete")
> +	    && matches(argv[1], "change") && matches(argv[1], "replace")))
> +		return false;

That is really hard to follow. It would be better to default the
response to false and test for the expected set:
    if argv[0] is filter or actions &&
       argv[1] is add or delete or change or replace
        return true;

    return false;

> +	return true;
> +}
> +
> +struct batch_buf {
> +	struct batch_buf	*next;
> +	char			buf[16420];	/* sizeof (struct nlmsghdr) +
> +						   sizeof (struct tcmsg) +

actions has a different ancillary header -- tcamsg. Use the buflen
approach is more robust.


> +						   MAX_MSG */
> +};
> +
> +static struct batch_buf *get_batch_buf(struct batch_buf **pool)
> +{
> +	struct batch_buf *buf;
> +
> +	if (*pool == NULL)
> +		buf = calloc(1, sizeof (struct batch_buf));
> +	else {
> +		buf = *pool;
> +		*pool = (*pool)->next;
> +		memset(buf, 0, sizeof (struct batch_buf));
> +	}
> +	return buf;
> +}
> +
> +static void put_batch_bufs(struct batch_buf **pool,
> +			   struct batch_buf **head, struct batch_buf **tail)
> +{
> +	if (*head == NULL || *tail == NULL)
> +		return;
> +
> +	if (*pool == NULL)
> +		*pool = *head;
> +	else {
> +		(*tail)->next = *pool;
> +		*pool = *head;
> +	}
> +	*head = NULL;
> +	*tail = NULL;
> +}
> +
> +static void free_batch_bufs(struct batch_buf **pool)
> +{
> +	struct batch_buf *buf;
> +
> +	for (buf = *pool; buf != NULL; buf = *pool) {
> +		*pool = buf->next;
> +		free(buf);
> +	}
> +	*pool = NULL;
> +}
> +
>  static int batch(const char *name)
>  {
> -	char *line = NULL;
> +	struct batch_buf *head = NULL, *tail = NULL, *buf_pool = NULL;
> +	char *largv[100], *largv_next[100];
> +	char *line, *line_next = NULL;
> +	bool bs_enabled_next = false;
> +	bool bs_enabled = false;
> +	bool lastline = false;
> +	int largc, largc_next;
> +	bool bs_enabled_saved;
> +	int batchsize = 0;
>  	size_t len = 0;
>  	int ret = 0;
> +	bool send;
>  
>  	batch_mode = 1;
>  	if (name && strcmp(name, "-") != 0) {
> @@ -240,25 +308,94 @@ static int batch(const char *name)
>  	}
>  
>  	cmdlineno = 0;
> -	while (getcmdline(&line, &len, stdin) != -1) {
> -		char *largv[100];
> -		int largc;
> +	if (getcmdline(&line, &len, stdin) == -1)
> +		goto Exit;
> +	largc = makeargs(line, largv, 100);
> +	bs_enabled = batchsize_enabled(largc, largv);
> +	bs_enabled_saved = bs_enabled;
> +	do {
> +		if (getcmdline(&line_next, &len, stdin) == -1)
> +			lastline = true;
> +
> +		largc_next = makeargs(line_next, largv_next, 100);
> +		bs_enabled_next = batchsize_enabled(largc_next, largv_next);
> +		if (bs_enabled) {
> +			struct batch_buf *buf;

space between variables and code.

> +			buf = get_batch_buf(&buf_pool);
> +			if (!buf) {
> +				fprintf(stderr, "failed to allocate batch_buf\n");
> +				return -1;
> +			}
> +			if (head == NULL)
> +				head = tail = buf;
> +			else {
> +				tail->next = buf;
> +				tail = buf;
> +			}
> +			++batchsize;
> +		}
> +
> +		/*
> +		 * In batch mode, if we haven't accumulated enough commands
> +		 * and this is not the last command and this command & next
> +		 * command both support the batchsize feature, don't send the
> +		 * message immediately.
> +		 */
> +		if (!lastline && bs_enabled && bs_enabled_next
> +		    && batchsize != MSG_IOV_MAX)
> +			send = false;
> +		else
> +			send = true;
> +
> +		line = line_next;
> +		line_next = NULL;
> +		len = 0;
> +		bs_enabled_saved = bs_enabled;
> +		bs_enabled = bs_enabled_next;
> +		bs_enabled_next = false;
>  
> -		largc = makeargs(line, largv, 100);
>  		if (largc == 0)
>  			continue;	/* blank line */
>  
> -		if (do_cmd(largc, largv)) {
> -			fprintf(stderr, "Command failed %s:%d\n", name, cmdlineno);
> +		ret = do_cmd(largc, largv, tail == NULL ? NULL : tail->buf);
> +		if (ret != 0) {
> +			fprintf(stderr, "Command failed %s:%d\n", name,
> +				cmdlineno - 1);
>  			ret = 1;
>  			if (!force)
>  				break;
>  		}
> -	}
> -	if (line)
> -		free(line);
> +		largc = largc_next;
> +		memcpy(largv, largv_next, largc * sizeof(char *));
> +
> +		if (send && bs_enabled_saved) {
> +			struct iovec *iov, *iovs;
> +			struct batch_buf *buf;
> +			struct nlmsghdr *n;
> +			iov = iovs = malloc(batchsize * sizeof (struct iovec));
> +			for (buf = head; buf != NULL; buf = buf->next, ++iov) {
> +				n = (struct nlmsghdr *)&buf->buf;
> +				iov->iov_base = n;
> +				iov->iov_len = n->nlmsg_len;
> +			}
> +
> +			ret = rtnl_talk_iov(&rth, iovs, batchsize, NULL);
> +			if (ret < 0) {
> +				fprintf(stderr, "Command failed %s:%d\n", name,
> +					cmdlineno - (batchsize + ret) - 1);
> +				return 2;
> +			}
> +			put_batch_bufs(&buf_pool, &head, &tail);
> +			batchsize = 0;
> +			free(iovs);
> +		}
> +	} while (!lastline);
>  
> +	free_batch_bufs(&buf_pool);
> +Exit:
> +	free(line);
>  	rtnl_close(&rth);
> +
>  	return ret;
>  }
>  
> @@ -341,7 +478,7 @@ int main(int argc, char **argv)
>  		goto Exit;
>  	}
>  
> -	ret = do_cmd(argc-1, argv+1);
> +	ret = do_cmd(argc-1, argv+1, NULL);
>  Exit:
>  	rtnl_close(&rth);
>  
> diff --git a/tc/tc_common.h b/tc/tc_common.h
> index 264fbdac..59018da5 100644
> --- a/tc/tc_common.h
> +++ b/tc/tc_common.h
> @@ -1,13 +1,14 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
>  
>  #define TCA_BUF_MAX	(64*1024)
> +#define MSG_IOV_MAX	128
>  
>  extern struct rtnl_handle rth;
>  
>  extern int do_qdisc(int argc, char **argv);
>  extern int do_class(int argc, char **argv);
> -extern int do_filter(int argc, char **argv);
> -extern int do_action(int argc, char **argv);
> +extern int do_filter(int argc, char **argv, void *buf);
> +extern int do_action(int argc, char **argv, void *buf);
>  extern int do_tcmonitor(int argc, char **argv);
>  extern int do_exec(int argc, char **argv);
>  
> diff --git a/tc/tc_filter.c b/tc/tc_filter.c
> index 545cc3a1..7db4964b 100644
> --- a/tc/tc_filter.c
> +++ b/tc/tc_filter.c
> @@ -42,28 +42,38 @@ static void usage(void)
>  		"OPTIONS := ... try tc filter add <desired FILTER_KIND> help\n");
>  }
>  
> -static int tc_filter_modify(int cmd, unsigned int flags, int argc, char **argv)
> +struct tc_filter_req {
> +	struct nlmsghdr		n;
> +	struct tcmsg		t;
> +	char			buf[MAX_MSG];
> +};
> +
> +static int tc_filter_modify(int cmd, unsigned int flags, int argc, char **argv,
> +			    void *buf)
>  {
> -	struct {
> -		struct nlmsghdr	n;
> -		struct tcmsg		t;
> -		char			buf[MAX_MSG];
> -	} req = {
> -		.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcmsg)),
> -		.n.nlmsg_flags = NLM_F_REQUEST | flags,
> -		.n.nlmsg_type = cmd,
> -		.t.tcm_family = AF_UNSPEC,
> -	};
> +	struct tc_filter_req *req, filter_req;
>  	struct filter_util *q = NULL;
> -	__u32 prio = 0;
> -	__u32 protocol = 0;
> -	int protocol_set = 0;
> -	__u32 chain_index;
> +	struct tc_estimator est = {};
> +	char k[FILTER_NAMESZ] = {};
>  	int chain_index_set = 0;
> +	char d[IFNAMSIZ] = {};
> +	int protocol_set = 0;
>  	char *fhandle = NULL;
> -	char  d[IFNAMSIZ] = {};
> -	char  k[FILTER_NAMESZ] = {};
> -	struct tc_estimator est = {};
> +	__u32 protocol = 0;
> +	__u32 chain_index;
> +	struct iovec iov;
> +	__u32 prio = 0;
> +	int ret;
> +
> +	if (buf)
> +		req = buf;
> +	else
> +		req = &filter_req;

same comment here about buflen and filter_req initialization.


> +
> +	req->n.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcmsg));
> +	req->n.nlmsg_flags = NLM_F_REQUEST | flags;
> +	req->n.nlmsg_type = cmd;
> +	req->t.tcm_family = AF_UNSPEC;
>  
>  	if (cmd == RTM_NEWTFILTER && flags & NLM_F_CREATE)
>  		protocol = htons(ETH_P_ALL);
> @@ -75,37 +85,37 @@ static int tc_filter_modify(int cmd, unsigned int flags, int argc, char **argv)
>  				duparg("dev", *argv);
>  			strncpy(d, *argv, sizeof(d)-1);
>  		} else if (strcmp(*argv, "root") == 0) {
> -			if (req.t.tcm_parent) {
> +			if (req->t.tcm_parent) {
>  				fprintf(stderr,
>  					"Error: \"root\" is duplicate parent ID\n");
>  				return -1;
>  			}
> -			req.t.tcm_parent = TC_H_ROOT;
> +			req->t.tcm_parent = TC_H_ROOT;
>  		} else if (strcmp(*argv, "ingress") == 0) {
> -			if (req.t.tcm_parent) {
> +			if (req->t.tcm_parent) {
>  				fprintf(stderr,
>  					"Error: \"ingress\" is duplicate parent ID\n");
>  				return -1;
>  			}
> -			req.t.tcm_parent = TC_H_MAKE(TC_H_CLSACT,
> +			req->t.tcm_parent = TC_H_MAKE(TC_H_CLSACT,
>  						     TC_H_MIN_INGRESS);
>  		} else if (strcmp(*argv, "egress") == 0) {
> -			if (req.t.tcm_parent) {
> +			if (req->t.tcm_parent) {
>  				fprintf(stderr,
>  					"Error: \"egress\" is duplicate parent ID\n");
>  				return -1;
>  			}
> -			req.t.tcm_parent = TC_H_MAKE(TC_H_CLSACT,
> +			req->t.tcm_parent = TC_H_MAKE(TC_H_CLSACT,
>  						     TC_H_MIN_EGRESS);
>  		} else if (strcmp(*argv, "parent") == 0) {
>  			__u32 handle;
>  
>  			NEXT_ARG();
> -			if (req.t.tcm_parent)
> +			if (req->t.tcm_parent)
>  				duparg("parent", *argv);
>  			if (get_tc_classid(&handle, *argv))
>  				invarg("Invalid parent ID", *argv);
> -			req.t.tcm_parent = handle;
> +			req->t.tcm_parent = handle;
>  		} else if (strcmp(*argv, "handle") == 0) {
>  			NEXT_ARG();
>  			if (fhandle)
> @@ -152,26 +162,26 @@ static int tc_filter_modify(int cmd, unsigned int flags, int argc, char **argv)
>  		argc--; argv++;
>  	}
>  
> -	req.t.tcm_info = TC_H_MAKE(prio<<16, protocol);
> +	req->t.tcm_info = TC_H_MAKE(prio<<16, protocol);
>  
>  	if (chain_index_set)
> -		addattr32(&req.n, sizeof(req), TCA_CHAIN, chain_index);
> +		addattr32(&req->n, sizeof(*req), TCA_CHAIN, chain_index);
>  
>  	if (k[0])
> -		addattr_l(&req.n, sizeof(req), TCA_KIND, k, strlen(k)+1);
> +		addattr_l(&req->n, sizeof(*req), TCA_KIND, k, strlen(k)+1);
>  
>  	if (d[0])  {
>  		ll_init_map(&rth);
>  
> -		req.t.tcm_ifindex = ll_name_to_index(d);
> -		if (req.t.tcm_ifindex == 0) {
> +		req->t.tcm_ifindex = ll_name_to_index(d);
> +		if (req->t.tcm_ifindex == 0) {
>  			fprintf(stderr, "Cannot find device \"%s\"\n", d);
>  			return 1;
>  		}
>  	}
>  
>  	if (q) {
> -		if (q->parse_fopt(q, fhandle, argc, argv, &req.n))
> +		if (q->parse_fopt(q, fhandle, argc, argv, &req->n))
>  			return 1;
>  	} else {
>  		if (fhandle) {
> @@ -190,10 +200,17 @@ static int tc_filter_modify(int cmd, unsigned int flags, int argc, char **argv)
>  	}
>  
>  	if (est.ewma_log)
> -		addattr_l(&req.n, sizeof(req), TCA_RATE, &est, sizeof(est));
> +		addattr_l(&req->n, sizeof(*req), TCA_RATE, &est, sizeof(est));
>  
> -	if (rtnl_talk(&rth, &req.n, NULL) < 0) {
> -		fprintf(stderr, "We have an error talking to the kernel\n");

And here, no need to change to iov for the !buf case.


> +	iov.iov_base = &req->n;
> +	iov.iov_len = req->n.nlmsg_len;
> +
> +	if (buf)
> +		return 0;
> +
> +	ret = rtnl_talk_iov(&rth, &iov, 1, NULL);
> +	if (ret < 0) {
> +		fprintf(stderr, "We have an error talking to the kernel, %d\n", ret);
>  		return 2;
>  	}
>  
> @@ -636,20 +653,20 @@ static int tc_filter_list(int argc, char **argv)
>  	return 0;
>  }
>  
> -int do_filter(int argc, char **argv)
> +int do_filter(int argc, char **argv, void *buf)
>  {
>  	if (argc < 1)
>  		return tc_filter_list(0, NULL);
>  	if (matches(*argv, "add") == 0)
>  		return tc_filter_modify(RTM_NEWTFILTER, NLM_F_EXCL|NLM_F_CREATE,
> -					argc-1, argv+1);
> +					argc-1, argv+1, buf);
>  	if (matches(*argv, "change") == 0)
> -		return tc_filter_modify(RTM_NEWTFILTER, 0, argc-1, argv+1);
> +		return tc_filter_modify(RTM_NEWTFILTER, 0, argc-1, argv+1, buf);
>  	if (matches(*argv, "replace") == 0)
>  		return tc_filter_modify(RTM_NEWTFILTER, NLM_F_CREATE, argc-1,
> -					argv+1);
> +					argv+1, buf);
>  	if (matches(*argv, "delete") == 0)
> -		return tc_filter_modify(RTM_DELTFILTER, 0,  argc-1, argv+1);
> +		return tc_filter_modify(RTM_DELTFILTER, 0, argc-1, argv+1, buf);
>  	if (matches(*argv, "get") == 0)
>  		return tc_filter_get(RTM_GETTFILTER, 0,  argc-1, argv+1);
>  	if (matches(*argv, "list") == 0 || matches(*argv, "show") == 0
> 

Re: [patch iproute2 v8 1/2] lib/libnetlink: Add functions rtnl_talk_msg and rtnl_talk_iov

[Phil Sutter]

On Wed, Jan 10, 2018 at 12:20:36PM -0700, David Ahern wrote:
[...]
> 2. I am using a batch file with drop filters:
> 
> filter add dev eth2 ingress protocol ip pref 273 flower dst_ip
> 192.168.253.0/16 action drop
> 
> and for each command tc is trying to dlopen m_drop.so:
> 
> open("/usr/lib/tc//m_drop.so", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such
> file or directory)

[...]

> Can you look at a follow on patch (not part of this set) to cache status
> of dlopen attempts?

IMHO the logic used in get_action_kind() for gact is the culprit here:
After trying to dlopen m_drop.so, it dlopens m_gact.so although it is
present already. (Unless I missed something.)

I guess the better (and easier) fix would be to create some more struct
action_util instances in m_gact.c for the primitives it supports so that
the lookup in action_list succeeds for consecutive uses. Note that
parse_gact() even supports this already.

Cheers, Phil

RE: [patch iproute2 v8 2/2] tc: Add batchsize feature for filter and actions

[Chris Mi]

> -----Original Message-----
> From: Marcelo Ricardo Leitner [mailto:marcelo.leitner@gmail.com]
> Sent: Wednesday, January 10, 2018 7:42 PM
> To: Chris Mi <chrism@mellanox.com>
> Cc: netdev@vger.kernel.org; gerlitz.or@gmail.com;
> stephen@networkplumber.org; dsahern@gmail.com; phil@nwl.cc
> Subject: Re: [patch iproute2 v8 2/2] tc: Add batchsize feature for filter and
> actions
> 
> On Wed, Jan 10, 2018 at 12:27:42PM +0900, Chris Mi wrote:
> > Currently in tc batch mode, only one command is read from the batch
> > file and sent to kernel to process. With this support, at most 128
> > commands can be accumulated before sending to kernel.
> >
> > Now it only works for the following successive commands:
> > filter and actions add/delete/change/replace.
> >
> > Signed-off-by: Chris Mi <chrism@mellanox.com>
> > ---
> >  tc/m_action.c  |  60 +++++++++++++--------
> >  tc/tc.c        | 165
> ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
> >  tc/tc_common.h |   5 +-
> >  tc/tc_filter.c |  97 +++++++++++++++++++--------------
> >  4 files changed, 249 insertions(+), 78 deletions(-)
> >
> > diff --git a/tc/m_action.c b/tc/m_action.c index fc422364..e5c53a80
> > 100644
> > --- a/tc/m_action.c
> > +++ b/tc/m_action.c
> > @@ -546,40 +546,56 @@ bad_val:
> >  	return ret;
> >  }
> >
> > +struct tc_action_req {
> > +	struct nlmsghdr		n;
> > +	struct tcamsg		t;
> > +	char			buf[MAX_MSG];
> > +};
> > +
> >  static int tc_action_modify(int cmd, unsigned int flags,
> > -			    int *argc_p, char ***argv_p)
> > +			    int *argc_p, char ***argv_p,
> > +			    void *buf)
> >  {
> > -	int argc = *argc_p;
> > +	struct tc_action_req *req, action_req;
> >  	char **argv = *argv_p;
> > +	struct rtattr *tail;
> > +	int argc = *argc_p;
> > +	struct iovec iov;
> >  	int ret = 0;
> > -	struct {
> > -		struct nlmsghdr         n;
> > -		struct tcamsg           t;
> > -		char                    buf[MAX_MSG];
> > -	} req = {
> > -		.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcamsg)),
> > -		.n.nlmsg_flags = NLM_F_REQUEST | flags,
> > -		.n.nlmsg_type = cmd,
> > -		.t.tca_family = AF_UNSPEC,
> > -	};
> > -	struct rtattr *tail = NLMSG_TAIL(&req.n);
> > +
> > +	if (buf)
> > +		req = buf;
> > +	else
> > +		req = &action_req;
> > +
> > +	req->n.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcamsg));
> > +	req->n.nlmsg_flags = NLM_F_REQUEST | flags;
> > +	req->n.nlmsg_type = cmd;
> > +	req->t.tca_family = AF_UNSPEC;
> > +	tail = NLMSG_TAIL(&req->n);
> >
> >  	argc -= 1;
> >  	argv += 1;
> > -	if (parse_action(&argc, &argv, TCA_ACT_TAB, &req.n)) {
> > +	if (parse_action(&argc, &argv, TCA_ACT_TAB, &req->n)) {
> >  		fprintf(stderr, "Illegal \"action\"\n");
> >  		return -1;
> >  	}
> > -	tail->rta_len = (void *) NLMSG_TAIL(&req.n) - (void *) tail;
> > +	tail->rta_len = (void *) NLMSG_TAIL(&req->n) - (void *) tail;
> > +
> > +	*argc_p = argc;
> > +	*argv_p = argv;
> >
> > -	if (rtnl_talk(&rth, &req.n, NULL) < 0) {
> > +	iov.iov_base = &req->n;
> > +	iov.iov_len = req->n.nlmsg_len;
> > +
> > +	if (buf)
> > +		return 0;
> > +
> > +	if (rtnl_talk_iov(&rth, &iov, 1, NULL) < 0) {
> >  		fprintf(stderr, "We have an error talking to the kernel\n");
> >  		ret = -1;
> >  	}
> >
> > -	*argc_p = argc;
> > -	*argv_p = argv;
> > -
> >  	return ret;
> >  }
> >
> > @@ -679,7 +695,7 @@ bad_val:
> >  	return ret;
> >  }
> >
> > -int do_action(int argc, char **argv)
> > +int do_action(int argc, char **argv, void *buf)
> >  {
> >
> >  	int ret = 0;
> > @@ -689,12 +705,12 @@ int do_action(int argc, char **argv)
> >  		if (matches(*argv, "add") == 0) {
> >  			ret =  tc_action_modify(RTM_NEWACTION,
> >  						NLM_F_EXCL |
> NLM_F_CREATE,
> > -						&argc, &argv);
> > +						&argc, &argv, buf);
> >  		} else if (matches(*argv, "change") == 0 ||
> >  			  matches(*argv, "replace") == 0) {
> >  			ret = tc_action_modify(RTM_NEWACTION,
> >  					       NLM_F_CREATE |
> NLM_F_REPLACE,
> > -					       &argc, &argv);
> > +					       &argc, &argv, buf);
> >  		} else if (matches(*argv, "delete") == 0) {
> >  			argc -= 1;
> >  			argv += 1;
> > diff --git a/tc/tc.c b/tc/tc.c
> > index ad9f07e9..44277405 100644
> > --- a/tc/tc.c
> > +++ b/tc/tc.c
> > @@ -193,16 +193,16 @@ static void usage(void)
> >  			"                    -nm | -nam[es] | { -cf | -conf } path } | -
> j[son]\n");
> >  }
> >
> > -static int do_cmd(int argc, char **argv)
> > +static int do_cmd(int argc, char **argv, void *buf)
> >  {
> >  	if (matches(*argv, "qdisc") == 0)
> >  		return do_qdisc(argc-1, argv+1);
> >  	if (matches(*argv, "class") == 0)
> >  		return do_class(argc-1, argv+1);
> >  	if (matches(*argv, "filter") == 0)
> > -		return do_filter(argc-1, argv+1);
> > +		return do_filter(argc-1, argv+1, buf);
> >  	if (matches(*argv, "actions") == 0)
> > -		return do_action(argc-1, argv+1);
> > +		return do_action(argc-1, argv+1, buf);
> >  	if (matches(*argv, "monitor") == 0)
> >  		return do_tcmonitor(argc-1, argv+1);
> >  	if (matches(*argv, "exec") == 0)
> > @@ -217,11 +217,79 @@ static int do_cmd(int argc, char **argv)
> >  	return -1;
> >  }
> >
> > +static bool batchsize_enabled(int argc, char *argv[]) {
> > +	if (argc < 2)
> > +		return false;
> > +	if ((matches(argv[0], "filter") && matches(argv[0], "actions"))
> > +	    || (matches(argv[1], "add") && matches(argv[1], "delete")
> > +	    && matches(argv[1], "change") && matches(argv[1], "replace")))
> > +		return false;
> > +	return true;
> > +}
> > +
> > +struct batch_buf {
> > +	struct batch_buf	*next;
> > +	char			buf[16420];	/* sizeof (struct nlmsghdr) +
> > +						   sizeof (struct tcmsg) +
> > +						   MAX_MSG */
> > +};
> > +
> > +static struct batch_buf *get_batch_buf(struct batch_buf **pool) {
> > +	struct batch_buf *buf;
> > +
> > +	if (*pool == NULL)
> > +		buf = calloc(1, sizeof (struct batch_buf));
> > +	else {
> > +		buf = *pool;
> > +		*pool = (*pool)->next;
> > +		memset(buf, 0, sizeof (struct batch_buf));
> > +	}
> > +	return buf;
> > +}
> > +
> > +static void put_batch_bufs(struct batch_buf **pool,
> > +			   struct batch_buf **head, struct batch_buf **tail) {
> > +	if (*head == NULL || *tail == NULL)
> > +		return;
> > +
> > +	if (*pool == NULL)
> > +		*pool = *head;
> > +	else {
> > +		(*tail)->next = *pool;
> > +		*pool = *head;
> > +	}
> > +	*head = NULL;
> > +	*tail = NULL;
> > +}
> > +
> > +static void free_batch_bufs(struct batch_buf **pool) {
> > +	struct batch_buf *buf;
> > +
> > +	for (buf = *pool; buf != NULL; buf = *pool) {
> > +		*pool = buf->next;
> > +		free(buf);
> > +	}
> > +	*pool = NULL;
> > +}
> > +
> >  static int batch(const char *name)
> >  {
> > -	char *line = NULL;
> > +	struct batch_buf *head = NULL, *tail = NULL, *buf_pool = NULL;
> > +	char *largv[100], *largv_next[100];
> > +	char *line, *line_next = NULL;
> > +	bool bs_enabled_next = false;
> > +	bool bs_enabled = false;
> > +	bool lastline = false;
> > +	int largc, largc_next;
> > +	bool bs_enabled_saved;
> > +	int batchsize = 0;
> >  	size_t len = 0;
> >  	int ret = 0;
> > +	bool send;
> >
> >  	batch_mode = 1;
> >  	if (name && strcmp(name, "-") != 0) { @@ -240,25 +308,94 @@ static
> > int batch(const char *name)
> >  	}
> >
> >  	cmdlineno = 0;
> > -	while (getcmdline(&line, &len, stdin) != -1) {
> > -		char *largv[100];
> > -		int largc;
> > +	if (getcmdline(&line, &len, stdin) == -1)
> > +		goto Exit;
> > +	largc = makeargs(line, largv, 100);
> > +	bs_enabled = batchsize_enabled(largc, largv);
> > +	bs_enabled_saved = bs_enabled;
> > +	do {
> > +		if (getcmdline(&line_next, &len, stdin) == -1)
> > +			lastline = true;
> > +
> > +		largc_next = makeargs(line_next, largv_next, 100);
> > +		bs_enabled_next = batchsize_enabled(largc_next,
> largv_next);
> > +		if (bs_enabled) {
> > +			struct batch_buf *buf;
> > +			buf = get_batch_buf(&buf_pool);
> > +			if (!buf) {
> > +				fprintf(stderr, "failed to allocate
> batch_buf\n");
> > +				return -1;
> > +			}
> > +			if (head == NULL)
> > +				head = tail = buf;
> > +			else {
> > +				tail->next = buf;
> > +				tail = buf;
> > +			}
> 
> What about moving this list handling to get_batch_buf(), like you did for
> put_batch_buf() ? Just for the sake of consistency.
Done.
> 
> Otherwise LGTM!
> 
>   Marcelo

RE: [patch iproute2 v8 1/2] lib/libnetlink: Add functions rtnl_talk_msg and rtnl_talk_iov

[Chris Mi]

> -----Original Message-----
> From: David Ahern [mailto:dsahern@gmail.com]
> Sent: Thursday, January 11, 2018 3:21 AM
> To: Chris Mi <chrism@mellanox.com>; netdev@vger.kernel.org
> Cc: gerlitz.or@gmail.com; stephen@networkplumber.org;
> marcelo.leitner@gmail.com; phil@nwl.cc
> Subject: Re: [patch iproute2 v8 1/2] lib/libnetlink: Add functions
> rtnl_talk_msg and rtnl_talk_iov
> 
> On 1/9/18 8:27 PM, Chris Mi wrote:
> > rtnl_talk can only send a single message to kernel. Add two functions
> > rtnl_talk_msg and rtnl_talk_iov that can send multiple messages to kernel.
> > rtnl_talk_msg takes struct msghdr * as argument.
> > rtnl_talk_iov takes struct iovec * and iovlen as arguments.
> >
> > Signed-off-by: Chris Mi <chrism@mellanox.com>
> > ---
> >  include/libnetlink.h |  6 ++++
> >  lib/libnetlink.c     | 82 ++++++++++++++++++++++++++++++++++++++++-
> -----------
> >  2 files changed, 70 insertions(+), 18 deletions(-)
> >
> > diff --git a/include/libnetlink.h b/include/libnetlink.h index
> > a4d83b9e..e9a63dbc 100644
> > --- a/include/libnetlink.h
> > +++ b/include/libnetlink.h
> > @@ -96,6 +96,12 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
> > int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
> >  	      struct nlmsghdr **answer)
> >  	__attribute__((warn_unused_result));
> > +int rtnl_talk_msg(struct rtnl_handle *rtnl, struct msghdr *m,
> > +		  struct nlmsghdr **answer)
> > +	__attribute__((warn_unused_result));
> 
> As mentioned before rtnl_talk_msg is not needed; you only need to add
> rtnl_talk_iov. The attached fixup on top of your patch removes it and adjusts
> __rtnl_talk_iov. Please roll that change into your patch.
Done. I misunderstood you previous comment.
Thanks for your patch, David.
> 
> 
> While testing this I noticed 2 other oddities:
> 
> $ perf trace -s tc -b tc.batch
> (stddev column removed to shorten line width)
> 
>  Summary of events:
> 
>  tc (780), 1857 events, 97.9%
> 
>    syscall            calls    total       min       avg       max
>                                (msec)    (msec)    (msec)    (msec)
>    --------------- -------- --------- --------- --------- ---------
>    recvmsg              530     6.532     0.008     0.012     0.218
>    open                 269     5.429     0.012     0.020     0.117
>    sendmsg                4     3.518     0.092     0.879     1.647
> 
> 
> 
> 1. recvmsg is called twice - once to peek at message size, allocate a buffer
> and then really receive the message. That is overkill for ACKs.
> 
> 2. I am using a batch file with drop filters:
> 
> filter add dev eth2 ingress protocol ip pref 273 flower dst_ip
> 192.168.253.0/16 action drop
> 
> and for each command tc is trying to dlopen m_drop.so:
> 
> open("/usr/lib/tc//m_drop.so", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No
> such file or directory)
> 
> 
> With a patch to use a stack buffer for ACKs, the above perf summary
> becomes:
> 
> $ perf trace -s tc -b tc.batch
> 
>  Summary of events:
> 
>  tc (777), 1345 events, 97.1%
> 
>    syscall            calls    total       min       avg       max
>                                (msec)    (msec)    (msec)    (msec)
>    --------------- -------- --------- --------- --------- ---------
>    open                 269     5.510     0.013     0.020     0.160
>    recvmsg              274     3.758     0.009     0.014     0.396
>    sendmsg                4     3.531     0.098     0.883     1.672
> 
> 
> Making the open errors now the dominate overhead affecting performance.
> If tc had some smarts that it already tried that file it would avoid the
> subsequent open calls. The end result is a significant speed up compared to
> the current tc:
> 
>  Summary of events:
> 
>  tc (785), 2333 events, 98.3%
> 
>    syscall            calls    total       min       avg       max
>                                (msec)    (msec)    (msec)    (msec)
>    --------------- -------- --------- --------- --------- ---------
>    sendmsg              256     9.832     0.029     0.038     0.181
>    open                 269     5.819     0.013     0.022     0.353
>    recvmsg              530     5.592     0.009     0.011     0.285
> 
> 
> Can you look at a follow on patch (not part of this set) to cache status of
> dlopen attempts?
Sure, I will investigate this issue.

RE: [patch iproute2 v8 2/2] tc: Add batchsize feature for filter and actions

[Chris Mi]

> -----Original Message-----
> From: David Ahern [mailto:dsahern@gmail.com]
> Sent: Thursday, January 11, 2018 3:41 AM
> To: Chris Mi <chrism@mellanox.com>; netdev@vger.kernel.org
> Cc: gerlitz.or@gmail.com; stephen@networkplumber.org;
> marcelo.leitner@gmail.com; phil@nwl.cc
> Subject: Re: [patch iproute2 v8 2/2] tc: Add batchsize feature for filter and
> actions
> 
> On 1/9/18 8:27 PM, Chris Mi wrote:
> > Currently in tc batch mode, only one command is read from the batch
> > file and sent to kernel to process. With this support, at most 128
> > commands can be accumulated before sending to kernel.
> >
> > Now it only works for the following successive commands:
> > filter and actions add/delete/change/replace.
> >
> > Signed-off-by: Chris Mi <chrism@mellanox.com>
> > ---
> >  tc/m_action.c  |  60 +++++++++++++--------
> >  tc/tc.c        | 165
> ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
> >  tc/tc_common.h |   5 +-
> >  tc/tc_filter.c |  97 +++++++++++++++++++--------------
> >  4 files changed, 249 insertions(+), 78 deletions(-)
> >
> > diff --git a/tc/m_action.c b/tc/m_action.c index fc422364..e5c53a80
> > 100644
> > --- a/tc/m_action.c
> > +++ b/tc/m_action.c
> > @@ -546,40 +546,56 @@ bad_val:
> >  	return ret;
> >  }
> >
> > +struct tc_action_req {
> > +	struct nlmsghdr		n;
> > +	struct tcamsg		t;
> > +	char			buf[MAX_MSG];
> > +};
> > +
> >  static int tc_action_modify(int cmd, unsigned int flags,
> > -			    int *argc_p, char ***argv_p)
> > +			    int *argc_p, char ***argv_p,
> > +			    void *buf)
> 
> you really need a buflen; you should not make assumptions about the length
> of buffer passed to these functions.
Done.
> 
> >  {
> > -	int argc = *argc_p;
> > +	struct tc_action_req *req, action_req;
> >  	char **argv = *argv_p;
> > +	struct rtattr *tail;
> > +	int argc = *argc_p;
> > +	struct iovec iov;
> >  	int ret = 0;
> > -	struct {
> > -		struct nlmsghdr         n;
> > -		struct tcamsg           t;
> > -		char                    buf[MAX_MSG];
> > -	} req = {
> > -		.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcamsg)),
> > -		.n.nlmsg_flags = NLM_F_REQUEST | flags,
> > -		.n.nlmsg_type = cmd,
> > -		.t.tca_family = AF_UNSPEC,
> > -	};
> > -	struct rtattr *tail = NLMSG_TAIL(&req.n);
> > +
> > +	if (buf)
> > +		req = buf;
> > +	else
> > +		req = &action_req;
> > +
> 
> And a memset is needed for the !buf path since action_req is not initialized.
Done.
> 
> > +	req->n.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcamsg));
> > +	req->n.nlmsg_flags = NLM_F_REQUEST | flags;
> > +	req->n.nlmsg_type = cmd;
> > +	req->t.tca_family = AF_UNSPEC;
> > +	tail = NLMSG_TAIL(&req->n);
> >
> >  	argc -= 1;
> >  	argv += 1;
> > -	if (parse_action(&argc, &argv, TCA_ACT_TAB, &req.n)) {
> > +	if (parse_action(&argc, &argv, TCA_ACT_TAB, &req->n)) {
> >  		fprintf(stderr, "Illegal \"action\"\n");
> >  		return -1;
> >  	}
> > -	tail->rta_len = (void *) NLMSG_TAIL(&req.n) - (void *) tail;
> > +	tail->rta_len = (void *) NLMSG_TAIL(&req->n) - (void *) tail;
> > +
> > +	*argc_p = argc;
> > +	*argv_p = argv;
> >
> > -	if (rtnl_talk(&rth, &req.n, NULL) < 0) {
> > +	iov.iov_base = &req->n;
> > +	iov.iov_len = req->n.nlmsg_len;
> 
> you can leave that as rtnl_talk; no need to change the !buf case to
> rtnl_talk_iov.
Done.
> 
> > +
> > +	if (buf)
> > +		return 0;
> > +
> > +	if (rtnl_talk_iov(&rth, &iov, 1, NULL) < 0) {
> >  		fprintf(stderr, "We have an error talking to the kernel\n");
> >  		ret = -1;
> >  	}
> >
> > -	*argc_p = argc;
> > -	*argv_p = argv;
> > -
> >  	return ret;
> >  }
> >
> > @@ -679,7 +695,7 @@ bad_val:
> >  	return ret;
> >  }
> >
> > -int do_action(int argc, char **argv)
> > +int do_action(int argc, char **argv, void *buf)
> >  {
> >
> >  	int ret = 0;
> > @@ -689,12 +705,12 @@ int do_action(int argc, char **argv)
> >  		if (matches(*argv, "add") == 0) {
> >  			ret =  tc_action_modify(RTM_NEWACTION,
> >  						NLM_F_EXCL |
> NLM_F_CREATE,
> > -						&argc, &argv);
> > +						&argc, &argv, buf);
> >  		} else if (matches(*argv, "change") == 0 ||
> >  			  matches(*argv, "replace") == 0) {
> >  			ret = tc_action_modify(RTM_NEWACTION,
> >  					       NLM_F_CREATE |
> NLM_F_REPLACE,
> > -					       &argc, &argv);
> > +					       &argc, &argv, buf);
> >  		} else if (matches(*argv, "delete") == 0) {
> >  			argc -= 1;
> >  			argv += 1;
> > diff --git a/tc/tc.c b/tc/tc.c
> > index ad9f07e9..44277405 100644
> > --- a/tc/tc.c
> > +++ b/tc/tc.c
> > @@ -193,16 +193,16 @@ static void usage(void)
> >  			"                    -nm | -nam[es] | { -cf | -conf } path } | -
> j[son]\n");
> >  }
> >
> > -static int do_cmd(int argc, char **argv)
> > +static int do_cmd(int argc, char **argv, void *buf)
> >  {
> >  	if (matches(*argv, "qdisc") == 0)
> >  		return do_qdisc(argc-1, argv+1);
> >  	if (matches(*argv, "class") == 0)
> >  		return do_class(argc-1, argv+1);
> >  	if (matches(*argv, "filter") == 0)
> > -		return do_filter(argc-1, argv+1);
> > +		return do_filter(argc-1, argv+1, buf);
> >  	if (matches(*argv, "actions") == 0)
> > -		return do_action(argc-1, argv+1);
> > +		return do_action(argc-1, argv+1, buf);
> >  	if (matches(*argv, "monitor") == 0)
> >  		return do_tcmonitor(argc-1, argv+1);
> >  	if (matches(*argv, "exec") == 0)
> > @@ -217,11 +217,79 @@ static int do_cmd(int argc, char **argv)
> >  	return -1;
> >  }
> >
> > +static bool batchsize_enabled(int argc, char *argv[]) {
> > +	if (argc < 2)
> > +		return false;
> > +	if ((matches(argv[0], "filter") && matches(argv[0], "actions"))
> > +	    || (matches(argv[1], "add") && matches(argv[1], "delete")
> > +	    && matches(argv[1], "change") && matches(argv[1], "replace")))
> > +		return false;
> 
> That is really hard to follow. It would be better to default the response to
> false and test for the expected set:
>     if argv[0] is filter or actions &&
>        argv[1] is add or delete or change or replace
>         return true;
> 
>     return false;
> 
I just realize that 'action delete' is not supported because function tc_action_gd() is called
for 'delete' instead of tc_action_modify().  So there is a good reason to use table now.

Done.
> > +	return true;
> > +}
> > +
> > +struct batch_buf {
> > +	struct batch_buf	*next;
> > +	char			buf[16420];	/* sizeof (struct nlmsghdr) +
> > +						   sizeof (struct tcmsg) +
> 
> actions has a different ancillary header -- tcamsg. Use the buflen approach is
> more robust.
Done. Actually 16420  = sizeof (struct nlmsghdr) + max(sizeof (struct tcmsg) + sizeof (struct tcamsg)) + MAX_MSG */
> 
> 
> > +						   MAX_MSG */
> > +};
> > +
> > +static struct batch_buf *get_batch_buf(struct batch_buf **pool) {
> > +	struct batch_buf *buf;
> > +
> > +	if (*pool == NULL)
> > +		buf = calloc(1, sizeof (struct batch_buf));
> > +	else {
> > +		buf = *pool;
> > +		*pool = (*pool)->next;
> > +		memset(buf, 0, sizeof (struct batch_buf));
> > +	}
> > +	return buf;
> > +}
> > +
> > +static void put_batch_bufs(struct batch_buf **pool,
> > +			   struct batch_buf **head, struct batch_buf **tail) {
> > +	if (*head == NULL || *tail == NULL)
> > +		return;
> > +
> > +	if (*pool == NULL)
> > +		*pool = *head;
> > +	else {
> > +		(*tail)->next = *pool;
> > +		*pool = *head;
> > +	}
> > +	*head = NULL;
> > +	*tail = NULL;
> > +}
> > +
> > +static void free_batch_bufs(struct batch_buf **pool) {
> > +	struct batch_buf *buf;
> > +
> > +	for (buf = *pool; buf != NULL; buf = *pool) {
> > +		*pool = buf->next;
> > +		free(buf);
> > +	}
> > +	*pool = NULL;
> > +}
> > +
> >  static int batch(const char *name)
> >  {
> > -	char *line = NULL;
> > +	struct batch_buf *head = NULL, *tail = NULL, *buf_pool = NULL;
> > +	char *largv[100], *largv_next[100];
> > +	char *line, *line_next = NULL;
> > +	bool bs_enabled_next = false;
> > +	bool bs_enabled = false;
> > +	bool lastline = false;
> > +	int largc, largc_next;
> > +	bool bs_enabled_saved;
> > +	int batchsize = 0;
> >  	size_t len = 0;
> >  	int ret = 0;
> > +	bool send;
> >
> >  	batch_mode = 1;
> >  	if (name && strcmp(name, "-") != 0) { @@ -240,25 +308,94 @@ static
> > int batch(const char *name)
> >  	}
> >
> >  	cmdlineno = 0;
> > -	while (getcmdline(&line, &len, stdin) != -1) {
> > -		char *largv[100];
> > -		int largc;
> > +	if (getcmdline(&line, &len, stdin) == -1)
> > +		goto Exit;
> > +	largc = makeargs(line, largv, 100);
> > +	bs_enabled = batchsize_enabled(largc, largv);
> > +	bs_enabled_saved = bs_enabled;
> > +	do {
> > +		if (getcmdline(&line_next, &len, stdin) == -1)
> > +			lastline = true;
> > +
> > +		largc_next = makeargs(line_next, largv_next, 100);
> > +		bs_enabled_next = batchsize_enabled(largc_next,
> largv_next);
> > +		if (bs_enabled) {
> > +			struct batch_buf *buf;
> 
> space between variables and code.
Done.
> 
> > +			buf = get_batch_buf(&buf_pool);
> > +			if (!buf) {
> > +				fprintf(stderr, "failed to allocate
> batch_buf\n");
> > +				return -1;
> > +			}
> > +			if (head == NULL)
> > +				head = tail = buf;
> > +			else {
> > +				tail->next = buf;
> > +				tail = buf;
> > +			}
> > +			++batchsize;
> > +		}
> > +
> > +		/*
> > +		 * In batch mode, if we haven't accumulated enough
> commands
> > +		 * and this is not the last command and this command & next
> > +		 * command both support the batchsize feature, don't send
> the
> > +		 * message immediately.
> > +		 */
> > +		if (!lastline && bs_enabled && bs_enabled_next
> > +		    && batchsize != MSG_IOV_MAX)
> > +			send = false;
> > +		else
> > +			send = true;
> > +
> > +		line = line_next;
> > +		line_next = NULL;
> > +		len = 0;
> > +		bs_enabled_saved = bs_enabled;
> > +		bs_enabled = bs_enabled_next;
> > +		bs_enabled_next = false;
> >
> > -		largc = makeargs(line, largv, 100);
> >  		if (largc == 0)
> >  			continue;	/* blank line */
> >
> > -		if (do_cmd(largc, largv)) {
> > -			fprintf(stderr, "Command failed %s:%d\n", name,
> cmdlineno);
> > +		ret = do_cmd(largc, largv, tail == NULL ? NULL : tail->buf);
> > +		if (ret != 0) {
> > +			fprintf(stderr, "Command failed %s:%d\n", name,
> > +				cmdlineno - 1);
> >  			ret = 1;
> >  			if (!force)
> >  				break;
> >  		}
> > -	}
> > -	if (line)
> > -		free(line);
> > +		largc = largc_next;
> > +		memcpy(largv, largv_next, largc * sizeof(char *));
> > +
> > +		if (send && bs_enabled_saved) {
> > +			struct iovec *iov, *iovs;
> > +			struct batch_buf *buf;
> > +			struct nlmsghdr *n;
> > +			iov = iovs = malloc(batchsize * sizeof (struct iovec));
> > +			for (buf = head; buf != NULL; buf = buf->next, ++iov)
> {
> > +				n = (struct nlmsghdr *)&buf->buf;
> > +				iov->iov_base = n;
> > +				iov->iov_len = n->nlmsg_len;
> > +			}
> > +
> > +			ret = rtnl_talk_iov(&rth, iovs, batchsize, NULL);
> > +			if (ret < 0) {
> > +				fprintf(stderr, "Command failed %s:%d\n",
> name,
> > +					cmdlineno - (batchsize + ret) - 1);
> > +				return 2;
> > +			}
> > +			put_batch_bufs(&buf_pool, &head, &tail);
> > +			batchsize = 0;
> > +			free(iovs);
> > +		}
> > +	} while (!lastline);
> >
> > +	free_batch_bufs(&buf_pool);
> > +Exit:
> > +	free(line);
> >  	rtnl_close(&rth);
> > +
> >  	return ret;
> >  }
> >
> > @@ -341,7 +478,7 @@ int main(int argc, char **argv)
> >  		goto Exit;
> >  	}
> >
> > -	ret = do_cmd(argc-1, argv+1);
> > +	ret = do_cmd(argc-1, argv+1, NULL);
> >  Exit:
> >  	rtnl_close(&rth);
> >
> > diff --git a/tc/tc_common.h b/tc/tc_common.h index 264fbdac..59018da5
> > 100644
> > --- a/tc/tc_common.h
> > +++ b/tc/tc_common.h
> > @@ -1,13 +1,14 @@
> >  /* SPDX-License-Identifier: GPL-2.0 */
> >
> >  #define TCA_BUF_MAX	(64*1024)
> > +#define MSG_IOV_MAX	128
> >
> >  extern struct rtnl_handle rth;
> >
> >  extern int do_qdisc(int argc, char **argv);  extern int do_class(int
> > argc, char **argv); -extern int do_filter(int argc, char **argv);
> > -extern int do_action(int argc, char **argv);
> > +extern int do_filter(int argc, char **argv, void *buf); extern int
> > +do_action(int argc, char **argv, void *buf);
> >  extern int do_tcmonitor(int argc, char **argv);  extern int
> > do_exec(int argc, char **argv);
> >
> > diff --git a/tc/tc_filter.c b/tc/tc_filter.c index 545cc3a1..7db4964b
> > 100644
> > --- a/tc/tc_filter.c
> > +++ b/tc/tc_filter.c
> > @@ -42,28 +42,38 @@ static void usage(void)
> >  		"OPTIONS := ... try tc filter add <desired FILTER_KIND>
> help\n");
> > }
> >
> > -static int tc_filter_modify(int cmd, unsigned int flags, int argc,
> > char **argv)
> > +struct tc_filter_req {
> > +	struct nlmsghdr		n;
> > +	struct tcmsg		t;
> > +	char			buf[MAX_MSG];
> > +};
> > +
> > +static int tc_filter_modify(int cmd, unsigned int flags, int argc, char **argv,
> > +			    void *buf)
> >  {
> > -	struct {
> > -		struct nlmsghdr	n;
> > -		struct tcmsg		t;
> > -		char			buf[MAX_MSG];
> > -	} req = {
> > -		.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcmsg)),
> > -		.n.nlmsg_flags = NLM_F_REQUEST | flags,
> > -		.n.nlmsg_type = cmd,
> > -		.t.tcm_family = AF_UNSPEC,
> > -	};
> > +	struct tc_filter_req *req, filter_req;
> >  	struct filter_util *q = NULL;
> > -	__u32 prio = 0;
> > -	__u32 protocol = 0;
> > -	int protocol_set = 0;
> > -	__u32 chain_index;
> > +	struct tc_estimator est = {};
> > +	char k[FILTER_NAMESZ] = {};
> >  	int chain_index_set = 0;
> > +	char d[IFNAMSIZ] = {};
> > +	int protocol_set = 0;
> >  	char *fhandle = NULL;
> > -	char  d[IFNAMSIZ] = {};
> > -	char  k[FILTER_NAMESZ] = {};
> > -	struct tc_estimator est = {};
> > +	__u32 protocol = 0;
> > +	__u32 chain_index;
> > +	struct iovec iov;
> > +	__u32 prio = 0;
> > +	int ret;
> > +
> > +	if (buf)
> > +		req = buf;
> > +	else
> > +		req = &filter_req;
> 
> same comment here about buflen and filter_req initialization.
Done.
> 
> 
> > +
> > +	req->n.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcmsg));
> > +	req->n.nlmsg_flags = NLM_F_REQUEST | flags;
> > +	req->n.nlmsg_type = cmd;
> > +	req->t.tcm_family = AF_UNSPEC;
> >
> >  	if (cmd == RTM_NEWTFILTER && flags & NLM_F_CREATE)
> >  		protocol = htons(ETH_P_ALL);
> > @@ -75,37 +85,37 @@ static int tc_filter_modify(int cmd, unsigned int flags,
> int argc, char **argv)
> >  				duparg("dev", *argv);
> >  			strncpy(d, *argv, sizeof(d)-1);
> >  		} else if (strcmp(*argv, "root") == 0) {
> > -			if (req.t.tcm_parent) {
> > +			if (req->t.tcm_parent) {
> >  				fprintf(stderr,
> >  					"Error: \"root\" is duplicate parent
> ID\n");
> >  				return -1;
> >  			}
> > -			req.t.tcm_parent = TC_H_ROOT;
> > +			req->t.tcm_parent = TC_H_ROOT;
> >  		} else if (strcmp(*argv, "ingress") == 0) {
> > -			if (req.t.tcm_parent) {
> > +			if (req->t.tcm_parent) {
> >  				fprintf(stderr,
> >  					"Error: \"ingress\" is duplicate parent
> ID\n");
> >  				return -1;
> >  			}
> > -			req.t.tcm_parent = TC_H_MAKE(TC_H_CLSACT,
> > +			req->t.tcm_parent = TC_H_MAKE(TC_H_CLSACT,
> >  						     TC_H_MIN_INGRESS);
> >  		} else if (strcmp(*argv, "egress") == 0) {
> > -			if (req.t.tcm_parent) {
> > +			if (req->t.tcm_parent) {
> >  				fprintf(stderr,
> >  					"Error: \"egress\" is duplicate parent
> ID\n");
> >  				return -1;
> >  			}
> > -			req.t.tcm_parent = TC_H_MAKE(TC_H_CLSACT,
> > +			req->t.tcm_parent = TC_H_MAKE(TC_H_CLSACT,
> >  						     TC_H_MIN_EGRESS);
> >  		} else if (strcmp(*argv, "parent") == 0) {
> >  			__u32 handle;
> >
> >  			NEXT_ARG();
> > -			if (req.t.tcm_parent)
> > +			if (req->t.tcm_parent)
> >  				duparg("parent", *argv);
> >  			if (get_tc_classid(&handle, *argv))
> >  				invarg("Invalid parent ID", *argv);
> > -			req.t.tcm_parent = handle;
> > +			req->t.tcm_parent = handle;
> >  		} else if (strcmp(*argv, "handle") == 0) {
> >  			NEXT_ARG();
> >  			if (fhandle)
> > @@ -152,26 +162,26 @@ static int tc_filter_modify(int cmd, unsigned int
> flags, int argc, char **argv)
> >  		argc--; argv++;
> >  	}
> >
> > -	req.t.tcm_info = TC_H_MAKE(prio<<16, protocol);
> > +	req->t.tcm_info = TC_H_MAKE(prio<<16, protocol);
> >
> >  	if (chain_index_set)
> > -		addattr32(&req.n, sizeof(req), TCA_CHAIN, chain_index);
> > +		addattr32(&req->n, sizeof(*req), TCA_CHAIN, chain_index);
> >
> >  	if (k[0])
> > -		addattr_l(&req.n, sizeof(req), TCA_KIND, k, strlen(k)+1);
> > +		addattr_l(&req->n, sizeof(*req), TCA_KIND, k, strlen(k)+1);
> >
> >  	if (d[0])  {
> >  		ll_init_map(&rth);
> >
> > -		req.t.tcm_ifindex = ll_name_to_index(d);
> > -		if (req.t.tcm_ifindex == 0) {
> > +		req->t.tcm_ifindex = ll_name_to_index(d);
> > +		if (req->t.tcm_ifindex == 0) {
> >  			fprintf(stderr, "Cannot find device \"%s\"\n", d);
> >  			return 1;
> >  		}
> >  	}
> >
> >  	if (q) {
> > -		if (q->parse_fopt(q, fhandle, argc, argv, &req.n))
> > +		if (q->parse_fopt(q, fhandle, argc, argv, &req->n))
> >  			return 1;
> >  	} else {
> >  		if (fhandle) {
> > @@ -190,10 +200,17 @@ static int tc_filter_modify(int cmd, unsigned int
> flags, int argc, char **argv)
> >  	}
> >
> >  	if (est.ewma_log)
> > -		addattr_l(&req.n, sizeof(req), TCA_RATE, &est, sizeof(est));
> > +		addattr_l(&req->n, sizeof(*req), TCA_RATE, &est,
> sizeof(est));
> >
> > -	if (rtnl_talk(&rth, &req.n, NULL) < 0) {
> > -		fprintf(stderr, "We have an error talking to the kernel\n");
> 
> And here, no need to change to iov for the !buf case.
Done.
> 
> 
> > +	iov.iov_base = &req->n;
> > +	iov.iov_len = req->n.nlmsg_len;
> > +
> > +	if (buf)
> > +		return 0;
> > +
> > +	ret = rtnl_talk_iov(&rth, &iov, 1, NULL);
> > +	if (ret < 0) {
> > +		fprintf(stderr, "We have an error talking to the kernel, %d\n",
> > +ret);
> >  		return 2;
> >  	}
> >
> > @@ -636,20 +653,20 @@ static int tc_filter_list(int argc, char **argv)
> >  	return 0;
> >  }
> >
> > -int do_filter(int argc, char **argv)
> > +int do_filter(int argc, char **argv, void *buf)
> >  {
> >  	if (argc < 1)
> >  		return tc_filter_list(0, NULL);
> >  	if (matches(*argv, "add") == 0)
> >  		return tc_filter_modify(RTM_NEWTFILTER,
> NLM_F_EXCL|NLM_F_CREATE,
> > -					argc-1, argv+1);
> > +					argc-1, argv+1, buf);
> >  	if (matches(*argv, "change") == 0)
> > -		return tc_filter_modify(RTM_NEWTFILTER, 0, argc-1, argv+1);
> > +		return tc_filter_modify(RTM_NEWTFILTER, 0, argc-1, argv+1,
> buf);
> >  	if (matches(*argv, "replace") == 0)
> >  		return tc_filter_modify(RTM_NEWTFILTER, NLM_F_CREATE,
> argc-1,
> > -					argv+1);
> > +					argv+1, buf);
> >  	if (matches(*argv, "delete") == 0)
> > -		return tc_filter_modify(RTM_DELTFILTER, 0,  argc-1, argv+1);
> > +		return tc_filter_modify(RTM_DELTFILTER, 0, argc-1, argv+1,
> buf);
> >  	if (matches(*argv, "get") == 0)
> >  		return tc_filter_get(RTM_GETTFILTER, 0,  argc-1, argv+1);
> >  	if (matches(*argv, "list") == 0 || matches(*argv, "show") == 0
> >

Re: [patch iproute2 v8 1/2] lib/libnetlink: Add functions rtnl_talk_msg and rtnl_talk_iov

[Phil Sutter]

[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]

On Wed, Jan 10, 2018 at 09:12:45PM +0100, Phil Sutter wrote:
> On Wed, Jan 10, 2018 at 12:20:36PM -0700, David Ahern wrote:
> [...]
> > 2. I am using a batch file with drop filters:
> > 
> > filter add dev eth2 ingress protocol ip pref 273 flower dst_ip
> > 192.168.253.0/16 action drop
> > 
> > and for each command tc is trying to dlopen m_drop.so:
> > 
> > open("/usr/lib/tc//m_drop.so", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such
> > file or directory)
> 
> [...]
> 
> > Can you look at a follow on patch (not part of this set) to cache status
> > of dlopen attempts?
> 
> IMHO the logic used in get_action_kind() for gact is the culprit here:
> After trying to dlopen m_drop.so, it dlopens m_gact.so although it is
> present already. (Unless I missed something.)

Not quite, m_gact.c is statically compiled in and there is logic around
dlopen(NULL, ...) to prevent calling it twice.

> I guess the better (and easier) fix would be to create some more struct
> action_util instances in m_gact.c for the primitives it supports so that
> the lookup in action_list succeeds for consecutive uses. Note that
> parse_gact() even supports this already.

Sadly, this doesn't fly: If a lookup for action 'drop' is successful,
that value is set as TCA_ACT_KIND and the kernel doesn't know about it.

I came up with an alternative solution, what do you think about attached
patch?

Thanks, Phil

[-- Attachment #2: dlopen_gact.diff --]
[-- Type: text/plain, Size: 1649 bytes --]

diff --git a/tc/m_action.c b/tc/m_action.c
index fc4223648e8cf..d3df93c066a89 100644
--- a/tc/m_action.c
+++ b/tc/m_action.c
@@ -194,7 +194,10 @@ int parse_action(int *argc_p, char ***argv_p, int tca_id, struct nlmsghdr *n)
 		} else {
 			struct action_util *a = NULL;
 
-			strncpy(k, *argv, sizeof(k) - 1);
+			if (!action_a2n(*argv, NULL, false))
+				strncpy(k, "gact", sizeof(k) - 1);
+			else
+				strncpy(k, *argv, sizeof(k) - 1);
 			eap = 0;
 			if (argc > 0) {
 				a = get_action_kind(k);
diff --git a/tc/tc_util.c b/tc/tc_util.c
index ee9a70aa6830c..10e5aa91168a1 100644
--- a/tc/tc_util.c
+++ b/tc/tc_util.c
@@ -511,7 +511,7 @@ static const char *action_n2a(int action)
  *
  * In error case, returns -1 and does not touch @result. Otherwise returns 0.
  */
-static int action_a2n(char *arg, int *result, bool allow_num)
+int action_a2n(char *arg, int *result, bool allow_num)
 {
 	int n;
 	char dummy;
@@ -535,13 +535,15 @@ static int action_a2n(char *arg, int *result, bool allow_num)
 	for (iter = a2n; iter->a; iter++) {
 		if (matches(arg, iter->a) != 0)
 			continue;
-		*result = iter->n;
-		return 0;
+		n = iter->n;
+		goto out_ok;
 	}
 	if (!allow_num || sscanf(arg, "%d%c", &n, &dummy) != 1)
 		return -1;
 
-	*result = n;
+out_ok:
+	if (result)
+		*result = n;
 	return 0;
 }
 
diff --git a/tc/tc_util.h b/tc/tc_util.h
index 1218610d77092..e354765ff1ed0 100644
--- a/tc/tc_util.h
+++ b/tc/tc_util.h
@@ -132,4 +132,6 @@ int prio_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt);
 int cls_names_init(char *path);
 void cls_names_uninit(void);
 
+int action_a2n(char *arg, int *result, bool allow_num);
+
 #endif

Re: [patch iproute2 v8 1/2] lib/libnetlink: Add functions rtnl_talk_msg and rtnl_talk_iov

[David Ahern]

On 1/11/18 8:08 AM, Phil Sutter wrote:
> On Wed, Jan 10, 2018 at 09:12:45PM +0100, Phil Sutter wrote:
>> On Wed, Jan 10, 2018 at 12:20:36PM -0700, David Ahern wrote:
>> [...]
>>> 2. I am using a batch file with drop filters:
>>>
>>> filter add dev eth2 ingress protocol ip pref 273 flower dst_ip
>>> 192.168.253.0/16 action drop
>>>
>>> and for each command tc is trying to dlopen m_drop.so:
>>>
>>> open("/usr/lib/tc//m_drop.so", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such
>>> file or directory)
>>
>> [...]
>>
>>> Can you look at a follow on patch (not part of this set) to cache status
>>> of dlopen attempts?
>>
>> IMHO the logic used in get_action_kind() for gact is the culprit here:
>> After trying to dlopen m_drop.so, it dlopens m_gact.so although it is
>> present already. (Unless I missed something.)
> 
> Not quite, m_gact.c is statically compiled in and there is logic around
> dlopen(NULL, ...) to prevent calling it twice.
> 
>> I guess the better (and easier) fix would be to create some more struct
>> action_util instances in m_gact.c for the primitives it supports so that
>> the lookup in action_list succeeds for consecutive uses. Note that
>> parse_gact() even supports this already.
> 
> Sadly, this doesn't fly: If a lookup for action 'drop' is successful,
> that value is set as TCA_ACT_KIND and the kernel doesn't know about it.
> 
> I came up with an alternative solution, what do you think about attached
> patch?

Looks ok to me and removes the repeated open overhead. Send it formally
and cc Jiri and Jamal.

Thanks,