[patch iproute2 v6 0/3] tc: Add -bs option to batch mode

[patch iproute2 v6 0/3] tc: Add -bs option 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, we can accumulate
several commands before sending to kernel. The batch size is specified
using option -bs or -batchsize.

To accumulate the commands in tc, client should allocate an array of
struct iovec. If batchsize is bigger than 1, only after the client
has accumulated enough commands, can the client call rtnl_talk_msg
to send the message that includes the iov array. One exception is
that there is no more command in the batch file.

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 -bs 10' exection time is:

real    0m13.043s
user    0m6.479s
sys     0m6.504s

The insertion rate is improved more than 10%.

In this patchset, we still ack for every rule. If we don't ack at all,
'tc -b $file' exection time is:

real    0m14.748s
user    0m6.944s
sys     0m7.740s

'tc -b $file -bs 10' exection time is:

real    0m12.535s
user    0m6.587s
sys     0m5.888s

We can see that the performance win is to send multiple messages instead
of no acking. I think that's because in tc, we don't spend too much time
processing the ack message.


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.


Chris Mi (3):
  lib/libnetlink: Add a function rtnl_talk_msg
  tc: Add -bs option to batch mode
  man: Add -bs option to tc manpage


 include/libnetlink.h |   3 ++
 lib/libnetlink.c     |  66 +++++++++++++++++++-------
 man/man8/tc.8        |   7 +++
 tc/m_action.c        |  93 +++++++++++++++++++++++++++---------
 tc/tc.c              |  96 ++++++++++++++++++++++++++++++++-----
 tc/tc_common.h       |   8 +++-
 tc/tc_filter.c       | 132 +++++++++++++++++++++++++++++++++++----------------
 7 files changed, 310 insertions(+), 95 deletions(-)

-- 
2.14.3

[patch iproute2 v6 1/3] lib/libnetlink: Add a function rtnl_talk_msg

[Chris Mi]

rtnl_talk can only send a single message to kernel. Add a new function
rtnl_talk_msg that can send multiple messages to kernel.

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

diff --git a/include/libnetlink.h b/include/libnetlink.h
index a4d83b9e..01d98b16 100644
--- a/include/libnetlink.h
+++ b/include/libnetlink.h
@@ -96,6 +96,9 @@ 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_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..49ee1208 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(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;
+	unsigned int seq = 0;
+	struct nlmsghdr *h;
+	struct iovec iov;
 	struct msghdr msg = {
 		.msg_name = &nladdr,
 		.msg_namelen = sizeof(nladdr),
 		.msg_iov = &iov,
 		.msg_iovlen = 1,
 	};
-	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;
 	}
 
 	while (1) {
+		char *buf;
+next:
 		status = rtnl_recvmsg(rtnl->fd, &msg, &buf);
 
 		if (status < 0)
@@ -642,7 +644,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 +664,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 &&
@@ -698,12 +703,37 @@ static int __rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 	}
 }
 
+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 sockaddr_nl nladdr = { .nl_family = AF_NETLINK };
+	struct iovec iov = {
+		.iov_base = n,
+		.iov_len = n->nlmsg_len
+	};
+	struct msghdr msg = {
+		.msg_name = &nladdr,
+		.msg_namelen = sizeof(nladdr),
+		.msg_iov = &iov,
+		.msg_iovlen = 1,
+	};
+
+	return __rtnl_talk_msg(rtnl, &msg, 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_extack(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 		     struct nlmsghdr **answer,
 		     nl_ext_ack_fn_t errfn)
-- 
2.14.3

[patch iproute2 v6 2/3] tc: Add -bs option 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 support, we can accumulate
several commands before sending to kernel.

Now it only works for the following successive rules,
1. filter add
2. filter delete
3. actions add
4. actions delete

Otherwise, the batch size is still 1.

Signed-off-by: Chris Mi <chrism@mellanox.com>
---
 tc/m_action.c  |  93 ++++++++++++++++++++++++++++++----------
 tc/tc.c        |  96 +++++++++++++++++++++++++++++++++++------
 tc/tc_common.h |   8 +++-
 tc/tc_filter.c | 132 ++++++++++++++++++++++++++++++++++++++++-----------------
 4 files changed, 252 insertions(+), 77 deletions(-)

diff --git a/tc/m_action.c b/tc/m_action.c
index fc422364..cf5cc95d 100644
--- a/tc/m_action.c
+++ b/tc/m_action.c
@@ -23,6 +23,7 @@
 #include <arpa/inet.h>
 #include <string.h>
 #include <dlfcn.h>
+#include <errno.h>
 
 #include "utils.h"
 #include "tc_common.h"
@@ -546,40 +547,86 @@ bad_val:
 	return ret;
 }
 
+typedef struct {
+	struct nlmsghdr		n;
+	struct tcamsg		t;
+	char			buf[MAX_MSG];
+} tc_action_req;
+
+static tc_action_req *action_reqs;
+static struct iovec msg_iov[MSG_IOV_MAX];
+
+void free_action_reqs(void)
+{
+	free(action_reqs);
+}
+
+static tc_action_req *get_action_req(int batch_size, int index)
+{
+	tc_action_req *req;
+
+	if (action_reqs == NULL) {
+		action_reqs = malloc(batch_size * sizeof (tc_action_req));
+		if (action_reqs == NULL)
+			return NULL;
+	}
+	req = &action_reqs[index];
+	memset(req, 0, sizeof (*req));
+
+	return req;
+}
+
 static int tc_action_modify(int cmd, unsigned int flags,
-			    int *argc_p, char ***argv_p)
+			    int *argc_p, char ***argv_p,
+			    int batch_size, int index, bool send)
 {
-	int argc = *argc_p;
+	struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };
+	struct iovec *iov = &msg_iov[index];
 	char **argv = *argv_p;
-	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 msghdr msg = {
+		.msg_name = &nladdr,
+		.msg_namelen = sizeof(nladdr),
+		.msg_iov = msg_iov,
+		.msg_iovlen = index + 1,
 	};
-	struct rtattr *tail = NLMSG_TAIL(&req.n);
+	struct rtattr *tail;
+	tc_action_req *req;
+	int argc = *argc_p;
+	int ret = 0;
+
+	req = get_action_req(batch_size, index);
+	if (req == NULL) {
+		fprintf(stderr, "get_action_req error: not enough buffer\n");
+		return -ENOMEM;
+	}
+	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;
 
-	if (rtnl_talk(&rth, &req.n, NULL) < 0) {
+	*argc_p = argc;
+	*argv_p = argv;
+
+	iov->iov_base = &req->n;
+	iov->iov_len = req->n.nlmsg_len;
+
+	if (!send)
+		return 0;
+
+	if (rtnl_talk_msg(&rth, &msg, 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 +726,7 @@ bad_val:
 	return ret;
 }
 
-int do_action(int argc, char **argv)
+int do_action(int argc, char **argv, int batch_size, int index, bool send)
 {
 
 	int ret = 0;
@@ -689,12 +736,14 @@ 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, batch_size,
+						index, send);
 		} 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, batch_size,
+					       index, send);
 		} else if (matches(*argv, "delete") == 0) {
 			argc -= 1;
 			argv += 1;
diff --git a/tc/tc.c b/tc/tc.c
index ad9f07e9..67c6bfb4 100644
--- a/tc/tc.c
+++ b/tc/tc.c
@@ -189,20 +189,20 @@ static void usage(void)
 	fprintf(stderr, "Usage: tc [ OPTIONS ] OBJECT { COMMAND | help }\n"
 			"       tc [-force] -batch filename\n"
 			"where  OBJECT := { qdisc | class | filter | action | monitor | exec }\n"
-	                "       OPTIONS := { -s[tatistics] | -d[etails] | -r[aw] | -p[retty] | -b[atch] [filename] | -n[etns] name |\n"
+	                "       OPTIONS := { -s[tatistics] | -d[etails] | -r[aw] | -p[retty] | -b[atch] [filename] | -bs | -batchsize [size] | -n[etns] name |\n"
 			"                    -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, int batch_size, int index, bool send)
 {
 	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, batch_size, index, send);
 	if (matches(*argv, "actions") == 0)
-		return do_action(argc-1, argv+1);
+		return do_action(argc-1, argv+1, batch_size, index, send);
 	if (matches(*argv, "monitor") == 0)
 		return do_tcmonitor(argc-1, argv+1);
 	if (matches(*argv, "exec") == 0)
@@ -217,11 +217,25 @@ static int do_cmd(int argc, char **argv)
 	return -1;
 }
 
-static int batch(const char *name)
+static bool batchsize_enabled(int argc, char *argv[])
 {
+	if (argc < 2)
+		return false;
+	if (((strcmp(argv[0], "filter") != 0) && strcmp(argv[0], "action") != 0)
+	    || ((strcmp(argv[1], "add") != 0) && strcmp(argv[1], "delete") != 0))
+		return false;
+	return true;
+}
+
+static int batch(const char *name, int batch_size)
+{
+	bool lastline = false;
+	int msg_iov_index = 0;
+	char *line2 = NULL;
 	char *line = NULL;
 	size_t len = 0;
 	int ret = 0;
+	bool send;
 
 	batch_mode = 1;
 	if (name && strcmp(name, "-") != 0) {
@@ -240,23 +254,66 @@ static int batch(const char *name)
 	}
 
 	cmdlineno = 0;
-	while (getcmdline(&line, &len, stdin) != -1) {
+	if (getcmdline(&line, &len, stdin) == -1)
+		goto Exit;
+	do {
+		char *largv2[100];
 		char *largv[100];
+		int largc2;
 		int largc;
 
+		if (getcmdline(&line2, &len, stdin) == -1)
+			lastline = true;
+
+		if (batch_size > 1)
+			largc2 = makeargs(line2, largv2, 100);
 		largc = makeargs(line, largv, 100);
+
+		/*
+		 * 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 (batch_size > 1 && msg_iov_index + 1 != batch_size
+		    && !lastline && batchsize_enabled(largc, largv)
+		    && batchsize_enabled(largc2, largv2))
+			send = false;
+		else
+			send = true;
+
+		line = line2;
+		line2 = NULL;
+		len = 0;
+
 		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, batch_size, msg_iov_index, send);
+		if (ret != 0) {
+			if (batch_size == 1)
+				fprintf(stderr, "Command failed %s:%d\n",
+					name, cmdlineno - 1);
+			else
+				fprintf(stderr, "Command failed %s:%d-%d\n",
+					name, cmdlineno - msg_iov_index - 1,
+					cmdlineno - 1);
 			ret = 1;
 			if (!force)
 				break;
 		}
-	}
-	if (line)
-		free(line);
+		if (batch_size > 1) {
+			++msg_iov_index;
+			msg_iov_index %= batch_size;
+		}
+		if (send)
+			msg_iov_index = 0;
+	} while (!lastline);
+
+	free_filter_reqs();
+	free_action_reqs();
+Exit:
+	free(line);
 
 	rtnl_close(&rth);
 	return ret;
@@ -267,6 +324,7 @@ int main(int argc, char **argv)
 {
 	int ret;
 	char *batch_file = NULL;
+	int batch_size = 1;
 
 	while (argc > 1) {
 		if (argv[1][0] != '-')
@@ -297,6 +355,16 @@ int main(int argc, char **argv)
 			if (argc <= 1)
 				usage();
 			batch_file = argv[1];
+		} else if (matches(argv[1], "-batchsize") == 0 ||
+				matches(argv[1], "-bs") == 0) {
+			argc--;	argv++;
+			if (argc <= 1)
+				usage();
+			batch_size = atoi(argv[1]);
+			if (batch_size > MSG_IOV_MAX)
+				batch_size = MSG_IOV_MAX;
+			else if (batch_size < 0)
+				batch_size = 1;
 		} else if (matches(argv[1], "-netns") == 0) {
 			NEXT_ARG();
 			if (netns_switch(argv[1]))
@@ -323,7 +391,7 @@ int main(int argc, char **argv)
 	}
 
 	if (batch_file)
-		return batch(batch_file);
+		return batch(batch_file, batch_size);
 
 	if (argc <= 1) {
 		usage();
@@ -341,7 +409,9 @@ int main(int argc, char **argv)
 		goto Exit;
 	}
 
-	ret = do_cmd(argc-1, argv+1);
+	ret = do_cmd(argc-1, argv+1, 1, 0, true);
+	free_filter_reqs();
+	free_action_reqs();
 Exit:
 	rtnl_close(&rth);
 
diff --git a/tc/tc_common.h b/tc/tc_common.h
index 264fbdac..8a82439f 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	256
 
 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, int batch_size, int index, bool send);
+extern int do_action(int argc, char **argv, int batch_size, int index, bool send);
 extern int do_tcmonitor(int argc, char **argv);
 extern int do_exec(int argc, char **argv);
 
@@ -24,5 +25,8 @@ struct tc_sizespec;
 extern int parse_size_table(int *p_argc, char ***p_argv, struct tc_sizespec *s);
 extern int check_size_table_opts(struct tc_sizespec *s);
 
+extern void free_filter_reqs(void);
+extern void free_action_reqs(void);
+
 extern int show_graph;
 extern bool use_names;
diff --git a/tc/tc_filter.c b/tc/tc_filter.c
index 545cc3a1..6e80ed2c 100644
--- a/tc/tc_filter.c
+++ b/tc/tc_filter.c
@@ -19,6 +19,7 @@
 #include <arpa/inet.h>
 #include <string.h>
 #include <linux/if_ether.h>
+#include <errno.h>
 
 #include "rt_names.h"
 #include "utils.h"
@@ -42,28 +43,69 @@ 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)
+typedef struct {
+	struct nlmsghdr		n;
+	struct tcmsg		t;
+	char			buf[MAX_MSG];
+} tc_filter_req;
+
+static tc_filter_req *filter_reqs;
+static struct iovec msg_iov[MSG_IOV_MAX];
+
+void free_filter_reqs(void)
 {
-	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,
-	};
+	free(filter_reqs);
+}
+
+static tc_filter_req *get_filter_req(int batch_size, int index)
+{
+	tc_filter_req *req;
+
+	if (filter_reqs == NULL) {
+		filter_reqs = malloc(batch_size * sizeof (tc_filter_req));
+		if (filter_reqs == NULL)
+			return NULL;
+	}
+	req = &filter_reqs[index];
+	memset(req, 0, sizeof (*req));
+
+	return req;
+}
+
+static int tc_filter_modify(int cmd, unsigned int flags, int argc, char **argv,
+			    int batch_size, int index, bool send)
+{
+	struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };
+	struct iovec *iov = &msg_iov[index];
 	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 *fhandle = NULL;
 	char  d[IFNAMSIZ] = {};
-	char  k[FILTER_NAMESZ] = {};
-	struct tc_estimator est = {};
+	struct msghdr msg = {
+		.msg_name = &nladdr,
+		.msg_namelen = sizeof(nladdr),
+		.msg_iov = msg_iov,
+		.msg_iovlen = index + 1,
+	};
+	int protocol_set = 0;
+	char *fhandle = NULL;
+	tc_filter_req *req;
+	__u32 protocol = 0;
+	__u32 chain_index;
+	__u32 prio = 0;
+	int ret;
+
+	req = get_filter_req(batch_size, index);
+	if (req == NULL) {
+		fprintf(stderr, "get_filter_req error: not enough buffer\n");
+		return -ENOMEM;
+	}
+
+	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 +117,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 +194,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 +232,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 (!send)
+		return 0;
+
+	ret = rtnl_talk_msg(&rth, &msg, NULL);
+	if (ret < 0) {
+		fprintf(stderr, "We have an error talking to the kernel, %d\n", ret);
 		return 2;
 	}
 
@@ -636,20 +685,23 @@ 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, int batch_size, int index, bool send)
 {
 	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,
+					batch_size, index, send);
 	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,
+					batch_size, index, send);
 	if (matches(*argv, "replace") == 0)
 		return tc_filter_modify(RTM_NEWTFILTER, NLM_F_CREATE, argc-1,
-					argv+1);
+					argv+1, batch_size, index, send);
 	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,
+					batch_size, index, send);
 	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.3

[patch iproute2 v6 3/3] man: Add -bs option to tc manpage

[Chris Mi]

Signed-off-by: Chris Mi <chrism@mellanox.com>
---
 man/man8/tc.8 | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/man/man8/tc.8 b/man/man8/tc.8
index ff071b33..23db730c 100644
--- a/man/man8/tc.8
+++ b/man/man8/tc.8
@@ -601,6 +601,13 @@ must exist already.
 read commands from provided file or standard input and invoke them.
 First failure will cause termination of tc.
 
+.TP
+.BR "\-bs", " \-bs size", " \-batchsize", " \-batchsize size"
+How many commands are accumulated before sending to kernel.
+By default, it is 1. It only takes effect in batch mode.
+Only successive rules of filter add and delete are supported.
+Otherwise, batch size is still 1.
+
 .TP
 .BR "\-force"
 don't terminate tc on errors in batch mode.
-- 
2.14.3

Re: [patch iproute2 v6 3/3] man: Add -bs option to tc manpage

[Marcelo Ricardo Leitner]

On Thu, Jan 04, 2018 at 04:34:54PM +0900, Chris Mi wrote:
> Signed-off-by: Chris Mi <chrism@mellanox.com>
> ---
>  man/man8/tc.8 | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/man/man8/tc.8 b/man/man8/tc.8
> index ff071b33..23db730c 100644
> --- a/man/man8/tc.8
> +++ b/man/man8/tc.8
> @@ -601,6 +601,13 @@ must exist already.
>  read commands from provided file or standard input and invoke them.
>  First failure will cause termination of tc.
>  
> +.TP
> +.BR "\-bs", " \-bs size", " \-batchsize", " \-batchsize size"
> +How many commands are accumulated before sending to kernel.
> +By default, it is 1. It only takes effect in batch mode.
> +Only successive rules of filter add and delete are supported.

"filter or action, add and delete, are supported."
(it's not mentioning action)

> +Otherwise, batch size is still 1.

"The batch will be flushed if other commands are found in order to
preserve the ordering."
or so.

> +
>  .TP
>  .BR "\-force"
>  don't terminate tc on errors in batch mode.
> -- 
> 2.14.3
> 

Re: [patch iproute2 v6 2/3] tc: Add -bs option to batch mode

[Marcelo Ricardo Leitner]

On Thu, Jan 04, 2018 at 04:34:53PM +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, we can accumulate
> several commands before sending to kernel.
> 
> Now it only works for the following successive rules,
> 1. filter add
> 2. filter delete
> 3. actions add
> 4. actions delete
> 
> Otherwise, the batch size is still 1.
> 
> Signed-off-by: Chris Mi <chrism@mellanox.com>
> ---
>  tc/m_action.c  |  93 ++++++++++++++++++++++++++++++----------
>  tc/tc.c        |  96 +++++++++++++++++++++++++++++++++++------
>  tc/tc_common.h |   8 +++-
>  tc/tc_filter.c | 132 ++++++++++++++++++++++++++++++++++++++++-----------------
>  4 files changed, 252 insertions(+), 77 deletions(-)
> 
> diff --git a/tc/m_action.c b/tc/m_action.c
> index fc422364..cf5cc95d 100644
> --- a/tc/m_action.c
> +++ b/tc/m_action.c
> @@ -23,6 +23,7 @@
>  #include <arpa/inet.h>
>  #include <string.h>
>  #include <dlfcn.h>
> +#include <errno.h>
>  
>  #include "utils.h"
>  #include "tc_common.h"
> @@ -546,40 +547,86 @@ bad_val:
>  	return ret;
>  }
>  
> +typedef struct {
> +	struct nlmsghdr		n;
> +	struct tcamsg		t;
> +	char			buf[MAX_MSG];
> +} tc_action_req;
> +
> +static tc_action_req *action_reqs;
> +static struct iovec msg_iov[MSG_IOV_MAX];
> +
> +void free_action_reqs(void)
> +{
> +	free(action_reqs);
> +}
> +
> +static tc_action_req *get_action_req(int batch_size, int index)
> +{
> +	tc_action_req *req;
> +
> +	if (action_reqs == NULL) {
> +		action_reqs = malloc(batch_size * sizeof (tc_action_req));
> +		if (action_reqs == NULL)
> +			return NULL;
> +	}
> +	req = &action_reqs[index];
> +	memset(req, 0, sizeof (*req));
> +
> +	return req;
> +}
> +
>  static int tc_action_modify(int cmd, unsigned int flags,
> -			    int *argc_p, char ***argv_p)
> +			    int *argc_p, char ***argv_p,
> +			    int batch_size, int index, bool send)
>  {
> -	int argc = *argc_p;
> +	struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };
> +	struct iovec *iov = &msg_iov[index];
>  	char **argv = *argv_p;
> -	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 msghdr msg = {
> +		.msg_name = &nladdr,
> +		.msg_namelen = sizeof(nladdr),
> +		.msg_iov = msg_iov,
> +		.msg_iovlen = index + 1,
>  	};
> -	struct rtattr *tail = NLMSG_TAIL(&req.n);
> +	struct rtattr *tail;
> +	tc_action_req *req;
> +	int argc = *argc_p;
> +	int ret = 0;
> +
> +	req = get_action_req(batch_size, index);
> +	if (req == NULL) {
> +		fprintf(stderr, "get_action_req error: not enough buffer\n");
> +		return -ENOMEM;
> +	}
> +	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;
>  
> -	if (rtnl_talk(&rth, &req.n, NULL) < 0) {
> +	*argc_p = argc;
> +	*argv_p = argv;
> +
> +	iov->iov_base = &req->n;
> +	iov->iov_len = req->n.nlmsg_len;
> +
> +	if (!send)
> +		return 0;
> +
> +	if (rtnl_talk_msg(&rth, &msg, 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 +726,7 @@ bad_val:
>  	return ret;
>  }
>  
> -int do_action(int argc, char **argv)
> +int do_action(int argc, char **argv, int batch_size, int index, bool send)
>  {
>  
>  	int ret = 0;
> @@ -689,12 +736,14 @@ 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, batch_size,
> +						index, send);
>  		} 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, batch_size,
> +					       index, send);
>  		} else if (matches(*argv, "delete") == 0) {
>  			argc -= 1;
>  			argv += 1;
> diff --git a/tc/tc.c b/tc/tc.c
> index ad9f07e9..67c6bfb4 100644
> --- a/tc/tc.c
> +++ b/tc/tc.c
> @@ -189,20 +189,20 @@ static void usage(void)
>  	fprintf(stderr, "Usage: tc [ OPTIONS ] OBJECT { COMMAND | help }\n"
>  			"       tc [-force] -batch filename\n"
>  			"where  OBJECT := { qdisc | class | filter | action | monitor | exec }\n"
> -	                "       OPTIONS := { -s[tatistics] | -d[etails] | -r[aw] | -p[retty] | -b[atch] [filename] | -n[etns] name |\n"
> +	                "       OPTIONS := { -s[tatistics] | -d[etails] | -r[aw] | -p[retty] | -b[atch] [filename] | -bs | -batchsize [size] | -n[etns] name |\n"
>  			"                    -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, int batch_size, int index, bool send)
>  {
>  	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, batch_size, index, send);
>  	if (matches(*argv, "actions") == 0)
> -		return do_action(argc-1, argv+1);
> +		return do_action(argc-1, argv+1, batch_size, index, send);
>  	if (matches(*argv, "monitor") == 0)
>  		return do_tcmonitor(argc-1, argv+1);
>  	if (matches(*argv, "exec") == 0)
> @@ -217,11 +217,25 @@ static int do_cmd(int argc, char **argv)
>  	return -1;
>  }
>  
> -static int batch(const char *name)
> +static bool batchsize_enabled(int argc, char *argv[])
>  {
> +	if (argc < 2)
> +		return false;
> +	if (((strcmp(argv[0], "filter") != 0) && strcmp(argv[0], "action") != 0)
> +	    || ((strcmp(argv[1], "add") != 0) && strcmp(argv[1], "delete") != 0))

Not sure about iproute2 coding style, but removing these != 0 makes it
much more readable IMHO:

        if ((strcmp(argv[0], "filter") && strcmp(argv[0], "action"))
            || (strcmp(argv[1], "add") && strcmp(argv[1], "delete")))

> +		return false;
> +	return true;
> +}
> +
> +static int batch(const char *name, int batch_size)
> +{
> +	bool lastline = false;
> +	int msg_iov_index = 0;
> +	char *line2 = NULL;

Please lets call it 'line_next' or so.

>  	char *line = NULL;
>  	size_t len = 0;
>  	int ret = 0;
> +	bool send;
>  
>  	batch_mode = 1;
>  	if (name && strcmp(name, "-") != 0) {
> @@ -240,23 +254,66 @@ static int batch(const char *name)
>  	}
>  
>  	cmdlineno = 0;
> -	while (getcmdline(&line, &len, stdin) != -1) {
> +	if (getcmdline(&line, &len, stdin) == -1)
> +		goto Exit;
> +	do {
> +		char *largv2[100];
>  		char *largv[100];
> +		int largc2;
>  		int largc;
>  
> +		if (getcmdline(&line2, &len, stdin) == -1)
> +			lastline = true;
> +
> +		if (batch_size > 1)
> +			largc2 = makeargs(line2, largv2, 100);
>  		largc = makeargs(line, largv, 100);

makeargs() is a pretty expensive call. Do we really need to call it
twice per iteration?

> +
> +		/*
> +		 * 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 (batch_size > 1 && msg_iov_index + 1 != batch_size
> +		    && !lastline && batchsize_enabled(largc, largv)
> +		    && batchsize_enabled(largc2, largv2))

Also here. Caching the result for the next/previous line could help.

> +			send = false;
> +		else
> +			send = true;
> +
> +		line = line2;
> +		line2 = NULL;
> +		len = 0;
> +
>  		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, batch_size, msg_iov_index, send);
> +		if (ret != 0) {
> +			if (batch_size == 1)
> +				fprintf(stderr, "Command failed %s:%d\n",
> +					name, cmdlineno - 1);
> +			else
> +				fprintf(stderr, "Command failed %s:%d-%d\n",
> +					name, cmdlineno - msg_iov_index - 1,
> +					cmdlineno - 1);
>  			ret = 1;
>  			if (!force)
>  				break;
>  		}
> -	}
> -	if (line)
> -		free(line);
> +		if (batch_size > 1) {
> +			++msg_iov_index;
> +			msg_iov_index %= batch_size;
> +		}
> +		if (send)
> +			msg_iov_index = 0;
> +	} while (!lastline);
> +
> +	free_filter_reqs();
> +	free_action_reqs();
> +Exit:
> +	free(line);
>  
>  	rtnl_close(&rth);
>  	return ret;
> @@ -267,6 +324,7 @@ int main(int argc, char **argv)
>  {
>  	int ret;
>  	char *batch_file = NULL;
> +	int batch_size = 1;
>  
>  	while (argc > 1) {
>  		if (argv[1][0] != '-')
> @@ -297,6 +355,16 @@ int main(int argc, char **argv)
>  			if (argc <= 1)
>  				usage();
>  			batch_file = argv[1];
> +		} else if (matches(argv[1], "-batchsize") == 0 ||
> +				matches(argv[1], "-bs") == 0) {
> +			argc--;	argv++;
> +			if (argc <= 1)
> +				usage();
> +			batch_size = atoi(argv[1]);
> +			if (batch_size > MSG_IOV_MAX)
> +				batch_size = MSG_IOV_MAX;
> +			else if (batch_size < 0)
> +				batch_size = 1;
>  		} else if (matches(argv[1], "-netns") == 0) {
>  			NEXT_ARG();
>  			if (netns_switch(argv[1]))
> @@ -323,7 +391,7 @@ int main(int argc, char **argv)
>  	}
>  
>  	if (batch_file)
> -		return batch(batch_file);
> +		return batch(batch_file, batch_size);
>  
>  	if (argc <= 1) {
>  		usage();
> @@ -341,7 +409,9 @@ int main(int argc, char **argv)
>  		goto Exit;
>  	}
>  
> -	ret = do_cmd(argc-1, argv+1);
> +	ret = do_cmd(argc-1, argv+1, 1, 0, true);
> +	free_filter_reqs();
> +	free_action_reqs();
>  Exit:
>  	rtnl_close(&rth);
>  
> diff --git a/tc/tc_common.h b/tc/tc_common.h
> index 264fbdac..8a82439f 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	256
>  
>  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, int batch_size, int index, bool send);
> +extern int do_action(int argc, char **argv, int batch_size, int index, bool send);
>  extern int do_tcmonitor(int argc, char **argv);
>  extern int do_exec(int argc, char **argv);
>  
> @@ -24,5 +25,8 @@ struct tc_sizespec;
>  extern int parse_size_table(int *p_argc, char ***p_argv, struct tc_sizespec *s);
>  extern int check_size_table_opts(struct tc_sizespec *s);
>  
> +extern void free_filter_reqs(void);
> +extern void free_action_reqs(void);
> +
>  extern int show_graph;
>  extern bool use_names;
> diff --git a/tc/tc_filter.c b/tc/tc_filter.c
> index 545cc3a1..6e80ed2c 100644
> --- a/tc/tc_filter.c
> +++ b/tc/tc_filter.c
> @@ -19,6 +19,7 @@
>  #include <arpa/inet.h>
>  #include <string.h>
>  #include <linux/if_ether.h>
> +#include <errno.h>
>  
>  #include "rt_names.h"
>  #include "utils.h"
> @@ -42,28 +43,69 @@ 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)
> +typedef struct {
> +	struct nlmsghdr		n;
> +	struct tcmsg		t;
> +	char			buf[MAX_MSG];
> +} tc_filter_req;
> +
> +static tc_filter_req *filter_reqs;
> +static struct iovec msg_iov[MSG_IOV_MAX];
> +
> +void free_filter_reqs(void)
>  {
> -	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,
> -	};
> +	free(filter_reqs);
> +}
> +
> +static tc_filter_req *get_filter_req(int batch_size, int index)
> +{
> +	tc_filter_req *req;
> +
> +	if (filter_reqs == NULL) {
> +		filter_reqs = malloc(batch_size * sizeof (tc_filter_req));
> +		if (filter_reqs == NULL)
> +			return NULL;
> +	}
> +	req = &filter_reqs[index];
> +	memset(req, 0, sizeof (*req));
> +
> +	return req;
> +}
> +
> +static int tc_filter_modify(int cmd, unsigned int flags, int argc, char **argv,
> +			    int batch_size, int index, bool send)
> +{
> +	struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };
> +	struct iovec *iov = &msg_iov[index];
>  	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 *fhandle = NULL;
>  	char  d[IFNAMSIZ] = {};
> -	char  k[FILTER_NAMESZ] = {};
> -	struct tc_estimator est = {};
> +	struct msghdr msg = {
> +		.msg_name = &nladdr,
> +		.msg_namelen = sizeof(nladdr),
> +		.msg_iov = msg_iov,
> +		.msg_iovlen = index + 1,
> +	};
> +	int protocol_set = 0;
> +	char *fhandle = NULL;
> +	tc_filter_req *req;
> +	__u32 protocol = 0;
> +	__u32 chain_index;
> +	__u32 prio = 0;
> +	int ret;
> +
> +	req = get_filter_req(batch_size, index);
> +	if (req == NULL) {
> +		fprintf(stderr, "get_filter_req error: not enough buffer\n");
> +		return -ENOMEM;
> +	}
> +
> +	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 +117,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 +194,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 +232,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 (!send)
> +		return 0;
> +
> +	ret = rtnl_talk_msg(&rth, &msg, NULL);
> +	if (ret < 0) {
> +		fprintf(stderr, "We have an error talking to the kernel, %d\n", ret);
>  		return 2;
>  	}
>  
> @@ -636,20 +685,23 @@ 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, int batch_size, int index, bool send)
>  {
>  	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,
> +					batch_size, index, send);
>  	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,
> +					batch_size, index, send);
>  	if (matches(*argv, "replace") == 0)
>  		return tc_filter_modify(RTM_NEWTFILTER, NLM_F_CREATE, argc-1,
> -					argv+1);
> +					argv+1, batch_size, index, send);
>  	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,
> +					batch_size, index, send);
>  	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.3
> 

Re: [patch iproute2 v6 0/3] tc: Add -bs option to batch mode

[Phil Sutter]

Hi Chris,

On Thu, Jan 04, 2018 at 04:34:51PM +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 patchset, we can accumulate
> several commands before sending to kernel. The batch size is specified
> using option -bs or -batchsize.
> 
> To accumulate the commands in tc, client should allocate an array of
> struct iovec. If batchsize is bigger than 1, only after the client
> has accumulated enough commands, can the client call rtnl_talk_msg
> to send the message that includes the iov array. One exception is
> that there is no more command in the batch file.
> 
> 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 -bs 10' exection time is:
> 
> real    0m13.043s
> user    0m6.479s
> sys     0m6.504s
> 
> The insertion rate is improved more than 10%.

Did you measure the effect of increasing batch sizes?

I wonder whether specifying the batch size is necessary at all. Couldn't
batch mode just collect messages until either EOF or an incompatible
command is encountered which then triggers a commit to kernel? This
might simplify code quite a bit.

Cheers, Phil

Re: [patch iproute2 v6 0/3] tc: Add -bs option to batch mode

[David Ahern]

On 1/5/18 10:25 AM, Phil Sutter wrote:
> I wonder whether specifying the batch size is necessary at all. Couldn't
> batch mode just collect messages until either EOF or an incompatible
> command is encountered which then triggers a commit to kernel? This
> might simplify code quite a bit.
> 

agreed.

Re: [patch iproute2 v6 1/3] lib/libnetlink: Add a function rtnl_talk_msg

[David Ahern]

On 1/4/18 12:34 AM, Chris Mi wrote:
> rtnl_talk can only send a single message to kernel. Add a new function
> rtnl_talk_msg that can send multiple messages to kernel.
> 
> Signed-off-by: Chris Mi <chrism@mellanox.com>
> ---
>  include/libnetlink.h |  3 +++
>  lib/libnetlink.c     | 66 ++++++++++++++++++++++++++++++++++++++--------------
>  2 files changed, 51 insertions(+), 18 deletions(-)
> 

I think you should add an argument to rtnl_talk_msg to return the number
of messages processed. That can be used to refine which line failed. As
batch size increases the current design puts the burden on the user to
scan a lot of lines to find the one that fails:

tc -b tc.batch  -bs 50
RTNETLINK answers: File exists
We have an error talking to the kernel, -1
Command failed tc.batch:2-51

We should be able to tell them exactly which line failed.

Also, it would be better to call this rtnl_talk_iov, take an iov as an
argument and have a common rtnl_talk_msg for existing code and this new one.

As it stands you are having to add:
   struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };

to tc functions when it really only needs to know about iov's.

Re: [patch iproute2 v6 2/3] tc: Add -bs option to batch mode

[David Ahern]

On 1/4/18 12:34 AM, 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, we can accumulate
> several commands before sending to kernel.
> 
> Now it only works for the following successive rules,
> 1. filter add
> 2. filter delete
> 3. actions add
> 4. actions delete
> 
> Otherwise, the batch size is still 1.
> 
> Signed-off-by: Chris Mi <chrism@mellanox.com>
> ---
>  tc/m_action.c  |  93 ++++++++++++++++++++++++++++++----------
>  tc/tc.c        |  96 +++++++++++++++++++++++++++++++++++------
>  tc/tc_common.h |   8 +++-
>  tc/tc_filter.c | 132 ++++++++++++++++++++++++++++++++++++++++-----------------
>  4 files changed, 252 insertions(+), 77 deletions(-)
> 
> diff --git a/tc/m_action.c b/tc/m_action.c
> index fc422364..cf5cc95d 100644
> --- a/tc/m_action.c
> +++ b/tc/m_action.c
> @@ -23,6 +23,7 @@
>  #include <arpa/inet.h>
>  #include <string.h>
>  #include <dlfcn.h>
> +#include <errno.h>
>  
>  #include "utils.h"
>  #include "tc_common.h"
> @@ -546,40 +547,86 @@ bad_val:
>  	return ret;
>  }
>  
> +typedef struct {
> +	struct nlmsghdr		n;
> +	struct tcamsg		t;
> +	char			buf[MAX_MSG];
> +} tc_action_req;
> +
> +static tc_action_req *action_reqs;
> +static struct iovec msg_iov[MSG_IOV_MAX];
> +
> +void free_action_reqs(void)
> +{
> +	free(action_reqs);
> +}
> +
> +static tc_action_req *get_action_req(int batch_size, int index)
> +{
> +	tc_action_req *req;
> +
> +	if (action_reqs == NULL) {
> +		action_reqs = malloc(batch_size * sizeof (tc_action_req));
> +		if (action_reqs == NULL)
> +			return NULL;
> +	}
> +	req = &action_reqs[index];
> +	memset(req, 0, sizeof (*req));
> +
> +	return req;
> +}
> +
>  static int tc_action_modify(int cmd, unsigned int flags,
> -			    int *argc_p, char ***argv_p)
> +			    int *argc_p, char ***argv_p,
> +			    int batch_size, int index, bool send)
>  {
> -	int argc = *argc_p;
> +	struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };
> +	struct iovec *iov = &msg_iov[index];
>  	char **argv = *argv_p;
> -	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 msghdr msg = {
> +		.msg_name = &nladdr,
> +		.msg_namelen = sizeof(nladdr),
> +		.msg_iov = msg_iov,
> +		.msg_iovlen = index + 1,
>  	};
> -	struct rtattr *tail = NLMSG_TAIL(&req.n);
> +	struct rtattr *tail;
> +	tc_action_req *req;
> +	int argc = *argc_p;
> +	int ret = 0;
> +
> +	req = get_action_req(batch_size, index);
> +	if (req == NULL) {
> +		fprintf(stderr, "get_action_req error: not enough buffer\n");
> +		return -ENOMEM;
> +	}
> +	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;
>  
> -	if (rtnl_talk(&rth, &req.n, NULL) < 0) {
> +	*argc_p = argc;
> +	*argv_p = argv;
> +
> +	iov->iov_base = &req->n;
> +	iov->iov_len = req->n.nlmsg_len;
> +
> +	if (!send)
> +		return 0;
> +
> +	if (rtnl_talk_msg(&rth, &msg, 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 +726,7 @@ bad_val:
>  	return ret;
>  }
>  
> -int do_action(int argc, char **argv)
> +int do_action(int argc, char **argv, int batch_size, int index, bool send)
>  {
>  
>  	int ret = 0;
> @@ -689,12 +736,14 @@ 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, batch_size,
> +						index, send);
>  		} 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, batch_size,
> +					       index, send);
>  		} else if (matches(*argv, "delete") == 0) {
>  			argc -= 1;
>  			argv += 1;
> diff --git a/tc/tc.c b/tc/tc.c
> index ad9f07e9..67c6bfb4 100644
> --- a/tc/tc.c
> +++ b/tc/tc.c
> @@ -189,20 +189,20 @@ static void usage(void)
>  	fprintf(stderr, "Usage: tc [ OPTIONS ] OBJECT { COMMAND | help }\n"
>  			"       tc [-force] -batch filename\n"
>  			"where  OBJECT := { qdisc | class | filter | action | monitor | exec }\n"
> -	                "       OPTIONS := { -s[tatistics] | -d[etails] | -r[aw] | -p[retty] | -b[atch] [filename] | -n[etns] name |\n"
> +	                "       OPTIONS := { -s[tatistics] | -d[etails] | -r[aw] | -p[retty] | -b[atch] [filename] | -bs | -batchsize [size] | -n[etns] name |\n"
>  			"                    -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, int batch_size, int index, bool send)
>  {
>  	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, batch_size, index, send);
>  	if (matches(*argv, "actions") == 0)
> -		return do_action(argc-1, argv+1);
> +		return do_action(argc-1, argv+1, batch_size, index, send);
>  	if (matches(*argv, "monitor") == 0)
>  		return do_tcmonitor(argc-1, argv+1);
>  	if (matches(*argv, "exec") == 0)
> @@ -217,11 +217,25 @@ static int do_cmd(int argc, char **argv)
>  	return -1;
>  }
>  
> -static int batch(const char *name)
> +static bool batchsize_enabled(int argc, char *argv[])
>  {
> +	if (argc < 2)
> +		return false;
> +	if (((strcmp(argv[0], "filter") != 0) && strcmp(argv[0], "action") != 0)
> +	    || ((strcmp(argv[1], "add") != 0) && strcmp(argv[1], "delete") != 0))
> +		return false;
> +	return true;
> +}
> +
> +static int batch(const char *name, int batch_size)
> +{
> +	bool lastline = false;
> +	int msg_iov_index = 0;
> +	char *line2 = NULL;
>  	char *line = NULL;
>  	size_t len = 0;
>  	int ret = 0;
> +	bool send;
>  
>  	batch_mode = 1;
>  	if (name && strcmp(name, "-") != 0) {
> @@ -240,23 +254,66 @@ static int batch(const char *name)
>  	}
>  
>  	cmdlineno = 0;
> -	while (getcmdline(&line, &len, stdin) != -1) {
> +	if (getcmdline(&line, &len, stdin) == -1)
> +		goto Exit;
> +	do {
> +		char *largv2[100];
>  		char *largv[100];
> +		int largc2;
>  		int largc;
>  
> +		if (getcmdline(&line2, &len, stdin) == -1)
> +			lastline = true;
> +
> +		if (batch_size > 1)
> +			largc2 = makeargs(line2, largv2, 100);
>  		largc = makeargs(line, largv, 100);
> +
> +		/*
> +		 * 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 (batch_size > 1 && msg_iov_index + 1 != batch_size
> +		    && !lastline && batchsize_enabled(largc, largv)
> +		    && batchsize_enabled(largc2, largv2))
> +			send = false;
> +		else
> +			send = true;
> +
> +		line = line2;
> +		line2 = NULL;
> +		len = 0;
> +
>  		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, batch_size, msg_iov_index, send);

Perhaps I am missing something, but this design seems to assume the file
contains a series of filters or actions. What happens if actions and
filters are interlaced in the file?

I think you need to look at having do_cmd return the generated request
as a buffer and length or have this batch function supply a buffer that
do_cmd fills instead of using its local req variable. The latter would
have better memory management. Then this batch function puts the buffer
into the iov and calls rtnl_talk_iov when it is ready to send the message.

Re: [patch iproute2 v6 0/3] tc: Add -bs option to batch mode

[Marcelo Ricardo Leitner]

On Fri, Jan 05, 2018 at 10:27:52AM -0700, David Ahern wrote:
> On 1/5/18 10:25 AM, Phil Sutter wrote:
> > I wonder whether specifying the batch size is necessary at all. Couldn't
> > batch mode just collect messages until either EOF or an incompatible
> > command is encountered which then triggers a commit to kernel? This
> > might simplify code quite a bit.
> > 
> 
> agreed.

Agreed too, especially now that it supports flushing because of other
rules.

  Marcelo

Re: [patch iproute2 v6 2/3] tc: Add -bs option to batch mode

[Marcelo Ricardo Leitner]

On Fri, Jan 05, 2018 at 11:15:59AM -0700, David Ahern wrote:
> On 1/4/18 12:34 AM, 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, we can accumulate
> > several commands before sending to kernel.
> > 
> > Now it only works for the following successive rules,
> > 1. filter add
> > 2. filter delete
> > 3. actions add
> > 4. actions delete
> > 
> > Otherwise, the batch size is still 1.
> > 
> > Signed-off-by: Chris Mi <chrism@mellanox.com>
> > ---
> >  tc/m_action.c  |  93 ++++++++++++++++++++++++++++++----------
> >  tc/tc.c        |  96 +++++++++++++++++++++++++++++++++++------
> >  tc/tc_common.h |   8 +++-
> >  tc/tc_filter.c | 132 ++++++++++++++++++++++++++++++++++++++++-----------------
> >  4 files changed, 252 insertions(+), 77 deletions(-)
> > 
> > diff --git a/tc/m_action.c b/tc/m_action.c
> > index fc422364..cf5cc95d 100644
> > --- a/tc/m_action.c
> > +++ b/tc/m_action.c
> > @@ -23,6 +23,7 @@
> >  #include <arpa/inet.h>
> >  #include <string.h>
> >  #include <dlfcn.h>
> > +#include <errno.h>
> >  
> >  #include "utils.h"
> >  #include "tc_common.h"
> > @@ -546,40 +547,86 @@ bad_val:
> >  	return ret;
> >  }
> >  
> > +typedef struct {
> > +	struct nlmsghdr		n;
> > +	struct tcamsg		t;
> > +	char			buf[MAX_MSG];
> > +} tc_action_req;
> > +
> > +static tc_action_req *action_reqs;
> > +static struct iovec msg_iov[MSG_IOV_MAX];
> > +
> > +void free_action_reqs(void)
> > +{
> > +	free(action_reqs);
> > +}
> > +
> > +static tc_action_req *get_action_req(int batch_size, int index)
> > +{
> > +	tc_action_req *req;
> > +
> > +	if (action_reqs == NULL) {
> > +		action_reqs = malloc(batch_size * sizeof (tc_action_req));
> > +		if (action_reqs == NULL)
> > +			return NULL;
> > +	}
> > +	req = &action_reqs[index];
> > +	memset(req, 0, sizeof (*req));
> > +
> > +	return req;
> > +}
> > +
> >  static int tc_action_modify(int cmd, unsigned int flags,
> > -			    int *argc_p, char ***argv_p)
> > +			    int *argc_p, char ***argv_p,
> > +			    int batch_size, int index, bool send)
> >  {
> > -	int argc = *argc_p;
> > +	struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };
> > +	struct iovec *iov = &msg_iov[index];
> >  	char **argv = *argv_p;
> > -	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 msghdr msg = {
> > +		.msg_name = &nladdr,
> > +		.msg_namelen = sizeof(nladdr),
> > +		.msg_iov = msg_iov,
> > +		.msg_iovlen = index + 1,
> >  	};
> > -	struct rtattr *tail = NLMSG_TAIL(&req.n);
> > +	struct rtattr *tail;
> > +	tc_action_req *req;
> > +	int argc = *argc_p;
> > +	int ret = 0;
> > +
> > +	req = get_action_req(batch_size, index);
> > +	if (req == NULL) {
> > +		fprintf(stderr, "get_action_req error: not enough buffer\n");
> > +		return -ENOMEM;
> > +	}
> > +	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;
> >  
> > -	if (rtnl_talk(&rth, &req.n, NULL) < 0) {
> > +	*argc_p = argc;
> > +	*argv_p = argv;
> > +
> > +	iov->iov_base = &req->n;
> > +	iov->iov_len = req->n.nlmsg_len;
> > +
> > +	if (!send)
> > +		return 0;
> > +
> > +	if (rtnl_talk_msg(&rth, &msg, 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 +726,7 @@ bad_val:
> >  	return ret;
> >  }
> >  
> > -int do_action(int argc, char **argv)
> > +int do_action(int argc, char **argv, int batch_size, int index, bool send)
> >  {
> >  
> >  	int ret = 0;
> > @@ -689,12 +736,14 @@ 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, batch_size,
> > +						index, send);
> >  		} 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, batch_size,
> > +					       index, send);
> >  		} else if (matches(*argv, "delete") == 0) {
> >  			argc -= 1;
> >  			argv += 1;
> > diff --git a/tc/tc.c b/tc/tc.c
> > index ad9f07e9..67c6bfb4 100644
> > --- a/tc/tc.c
> > +++ b/tc/tc.c
> > @@ -189,20 +189,20 @@ static void usage(void)
> >  	fprintf(stderr, "Usage: tc [ OPTIONS ] OBJECT { COMMAND | help }\n"
> >  			"       tc [-force] -batch filename\n"
> >  			"where  OBJECT := { qdisc | class | filter | action | monitor | exec }\n"
> > -	                "       OPTIONS := { -s[tatistics] | -d[etails] | -r[aw] | -p[retty] | -b[atch] [filename] | -n[etns] name |\n"
> > +	                "       OPTIONS := { -s[tatistics] | -d[etails] | -r[aw] | -p[retty] | -b[atch] [filename] | -bs | -batchsize [size] | -n[etns] name |\n"
> >  			"                    -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, int batch_size, int index, bool send)
> >  {
> >  	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, batch_size, index, send);
> >  	if (matches(*argv, "actions") == 0)
> > -		return do_action(argc-1, argv+1);
> > +		return do_action(argc-1, argv+1, batch_size, index, send);
> >  	if (matches(*argv, "monitor") == 0)
> >  		return do_tcmonitor(argc-1, argv+1);
> >  	if (matches(*argv, "exec") == 0)
> > @@ -217,11 +217,25 @@ static int do_cmd(int argc, char **argv)
> >  	return -1;
> >  }
> >  
> > -static int batch(const char *name)
> > +static bool batchsize_enabled(int argc, char *argv[])
> >  {
> > +	if (argc < 2)
> > +		return false;
> > +	if (((strcmp(argv[0], "filter") != 0) && strcmp(argv[0], "action") != 0)
> > +	    || ((strcmp(argv[1], "add") != 0) && strcmp(argv[1], "delete") != 0))
> > +		return false;
> > +	return true;
> > +}
> > +
> > +static int batch(const char *name, int batch_size)
> > +{
> > +	bool lastline = false;
> > +	int msg_iov_index = 0;
> > +	char *line2 = NULL;
> >  	char *line = NULL;
> >  	size_t len = 0;
> >  	int ret = 0;
> > +	bool send;
> >  
> >  	batch_mode = 1;
> >  	if (name && strcmp(name, "-") != 0) {
> > @@ -240,23 +254,66 @@ static int batch(const char *name)
> >  	}
> >  
> >  	cmdlineno = 0;
> > -	while (getcmdline(&line, &len, stdin) != -1) {
> > +	if (getcmdline(&line, &len, stdin) == -1)
> > +		goto Exit;
> > +	do {
> > +		char *largv2[100];
> >  		char *largv[100];
> > +		int largc2;
> >  		int largc;
> >  
> > +		if (getcmdline(&line2, &len, stdin) == -1)
> > +			lastline = true;
> > +
> > +		if (batch_size > 1)
> > +			largc2 = makeargs(line2, largv2, 100);
> >  		largc = makeargs(line, largv, 100);
> > +
> > +		/*
> > +		 * 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 (batch_size > 1 && msg_iov_index + 1 != batch_size
> > +		    && !lastline && batchsize_enabled(largc, largv)
> > +		    && batchsize_enabled(largc2, largv2))
> > +			send = false;
> > +		else
> > +			send = true;
> > +
> > +		line = line2;
> > +		line2 = NULL;
> > +		len = 0;
> > +
> >  		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, batch_size, msg_iov_index, send);
> 
> Perhaps I am missing something, but this design seems to assume the file
> contains a series of filters or actions. What happens if actions and
> filters are interlaced in the file?
> 
> I think you need to look at having do_cmd return the generated request
> as a buffer and length or have this batch function supply a buffer that
> do_cmd fills instead of using its local req variable. The latter would
> have better memory management. Then this batch function puts the buffer
> into the iov and calls rtnl_talk_iov when it is ready to send the message.

Yes!   (That's pretty much what I tried to mean on Dec 27th)
and then when rtnl_talk_iov returns, it knows all those buffers can be
returned to the pool, regardless of the type of cmd.
With this change, 'index' doesn't need to be here but somewhere below
this function.

  Marcelo

RE: [patch iproute2 v6 0/3] tc: Add -bs option to batch mode

[Chris Mi]

> -----Original Message-----
> From: n0-1@orbyte.nwl.cc [mailto:n0-1@orbyte.nwl.cc] On Behalf Of Phil
> Sutter
> Sent: Saturday, January 6, 2018 1:25 AM
> To: Chris Mi <chrism@mellanox.com>
> Cc: netdev@vger.kernel.org; gerlitz.or@gmail.com;
> stephen@networkplumber.org; dsahern@gmail.com;
> marcelo.leitner@gmail.com
> Subject: Re: [patch iproute2 v6 0/3] tc: Add -bs option to batch mode
> 
> Hi Chris,
> 
> On Thu, Jan 04, 2018 at 04:34:51PM +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 patchset, we can
> > accumulate several commands before sending to kernel. The batch size
> > is specified using option -bs or -batchsize.
> >
> > To accumulate the commands in tc, client should allocate an array of
> > struct iovec. If batchsize is bigger than 1, only after the client has
> > accumulated enough commands, can the client call rtnl_talk_msg to send
> > the message that includes the iov array. One exception is that there
> > is no more command in the batch file.
> >
> > 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 -bs 10' exection time is:
> >
> > real    0m13.043s
> > user    0m6.479s
> > sys     0m6.504s
> >
> > The insertion rate is improved more than 10%.
> 
> Did you measure the effect of increasing batch sizes?
Yes. Even if we enlarge the batch size bigger than 10, there is no big improvement.
I think that's because current kernel doesn't process the requests in parallel.
If kernel processes the requests in parallel, I believe specifying a bigger batch size
will get a better result.
> 
> I wonder whether specifying the batch size is necessary at all. Couldn't batch
> mode just collect messages until either EOF or an incompatible command is
> encountered which then triggers a commit to kernel? This might simplify
> code quite a bit.
That's a good suggestion.

-Chris
> 
> Cheers, Phil

Re: [patch iproute2 v6 0/3] tc: Add -bs option to batch mode

[David Ahern]

On 1/7/18 7:03 PM, Chris Mi wrote:

>> Did you measure the effect of increasing batch sizes?
> Yes. Even if we enlarge the batch size bigger than 10, there is no big improvement.

That will change over time so the tc command should allow auto-batching
to work up to the message size limit.


> I think that's because current kernel doesn't process the requests in parallel.
> If kernel processes the requests in parallel, I believe specifying a bigger batch size
> will get a better result.
>>
>> I wonder whether specifying the batch size is necessary at all. Couldn't batch
>> mode just collect messages until either EOF or an incompatible command is
>> encountered which then triggers a commit to kernel? This might simplify
>> code quite a bit.
> That's a good suggestion.

Thanks for your time on this, Chris.

RE: [patch iproute2 v6 0/3] tc: Add -bs option to batch mode

[Chris Mi]

> >> I wonder whether specifying the batch size is necessary at all.
> >> Couldn't batch mode just collect messages until either EOF or an
> >> incompatible command is encountered which then triggers a commit to
> >> kernel? This might simplify code quite a bit.
> > That's a good suggestion.
> 
> Thanks for your time on this, Chris.
After testing, I find that the message passed to kernel should not be too big.
If it is bigger than about 64K, sendmsg returns -1, errno is 90 (EMSGSIZE).
That is about 400 commands.  So how about set batch size to 128 which is big enough?

Re: [patch iproute2 v6 0/3] tc: Add -bs option to batch mode

[Phil Sutter]

Hi Chris,

On Mon, Jan 08, 2018 at 02:03:53AM +0000, Chris Mi wrote:
> > On Thu, Jan 04, 2018 at 04:34:51PM +0900, Chris Mi wrote:
> > > The insertion rate is improved more than 10%.
> > 
> > Did you measure the effect of increasing batch sizes?
> Yes. Even if we enlarge the batch size bigger than 10, there is no big improvement.
> I think that's because current kernel doesn't process the requests in parallel.
> If kernel processes the requests in parallel, I believe specifying a bigger batch size
> will get a better result.

But throughput doesn't regress at some point, right? I think that's the
critical aspect when considering an "unlimited" batch size.

On Mon, Jan 08, 2018 at 08:00:00AM +0000, Chris Mi wrote:
> After testing, I find that the message passed to kernel should not be too big.
> If it is bigger than about 64K, sendmsg returns -1, errno is 90 (EMSGSIZE).
> That is about 400 commands.  So how about set batch size to 128 which is big enough?

If that's the easiest way, why not. At first, I thought one could maybe
send the collected messages in chunks of suitable size, but that's
probably not worth the effort.

Cheers, Phil

Re: [patch iproute2 v6 0/3] tc: Add -bs option to batch mode

[Stephen Hemminger]

On Mon, 8 Jan 2018 08:00:00 +0000
Chris Mi <chrism@mellanox.com> wrote:

> > >> I wonder whether specifying the batch size is necessary at all.
> > >> Couldn't batch mode just collect messages until either EOF or an
> > >> incompatible command is encountered which then triggers a commit to
> > >> kernel? This might simplify code quite a bit.  
> > > That's a good suggestion.  
> > 
> > Thanks for your time on this, Chris.  
> After testing, I find that the message passed to kernel should not be too big.
> If it is bigger than about 64K, sendmsg returns -1, errno is 90 (EMSGSIZE).
> That is about 400 commands.  So how about set batch size to 128 which is big enough?


Use sendmmsg?

RE: [patch iproute2 v6 0/3] tc: Add -bs option to batch mode

[Chris Mi]

> -----Original Message-----
> From: n0-1@orbyte.nwl.cc [mailto:n0-1@orbyte.nwl.cc] On Behalf Of Phil
> Sutter
> Sent: Monday, January 8, 2018 9:32 PM
> To: Chris Mi <chrism@mellanox.com>
> Cc: dsahern@gmail.com; marcelo.leitner@gmail.com;
> netdev@vger.kernel.org; gerlitz.or@gmail.com;
> stephen@networkplumber.org
> Subject: Re: [patch iproute2 v6 0/3] tc: Add -bs option to batch mode
> 
> Hi Chris,
> 
> On Mon, Jan 08, 2018 at 02:03:53AM +0000, Chris Mi wrote:
> > > On Thu, Jan 04, 2018 at 04:34:51PM +0900, Chris Mi wrote:
> > > > The insertion rate is improved more than 10%.
> > >
> > > Did you measure the effect of increasing batch sizes?
> > Yes. Even if we enlarge the batch size bigger than 10, there is no big
> improvement.
> > I think that's because current kernel doesn't process the requests in
> parallel.
> > If kernel processes the requests in parallel, I believe specifying a
> > bigger batch size will get a better result.
> 
> But throughput doesn't regress at some point, right? I think that's the critical
> aspect when considering an "unlimited" batch size.
Yes.
> 
> On Mon, Jan 08, 2018 at 08:00:00AM +0000, Chris Mi wrote:
> > After testing, I find that the message passed to kernel should not be too
> big.
> > If it is bigger than about 64K, sendmsg returns -1, errno is 90 (EMSGSIZE).
> > That is about 400 commands.  So how about set batch size to 128 which is
> big enough?
> 
> If that's the easiest way, why not. At first, I thought one could maybe send
> the collected messages in chunks of suitable size, but that's probably not
> worth the effort.
OK.

-Chris

RE: [patch iproute2 v6 0/3] tc: Add -bs option to batch mode

[Chris Mi]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Monday, January 8, 2018 11:40 PM
> To: Chris Mi <chrism@mellanox.com>
> Cc: David Ahern <dsahern@gmail.com>; Phil Sutter <phil@nwl.cc>;
> marcelo.leitner@gmail.com; netdev@vger.kernel.org; gerlitz.or@gmail.com
> Subject: Re: [patch iproute2 v6 0/3] tc: Add -bs option to batch mode
> 
> On Mon, 8 Jan 2018 08:00:00 +0000
> Chris Mi <chrism@mellanox.com> wrote:
> 
> > > >> I wonder whether specifying the batch size is necessary at all.
> > > >> Couldn't batch mode just collect messages until either EOF or an
> > > >> incompatible command is encountered which then triggers a commit
> > > >> to kernel? This might simplify code quite a bit.
> > > > That's a good suggestion.
> > >
> > > Thanks for your time on this, Chris.
> > After testing, I find that the message passed to kernel should not be too
> big.
> > If it is bigger than about 64K, sendmsg returns -1, errno is 90 (EMSGSIZE).
> > That is about 400 commands.  So how about set batch size to 128 which is
> big enough?
> 
> 
> Use sendmmsg?
Maybe we can try that, but there is also a limit on it.

RE: [patch iproute2 v6 0/3] tc: Add -bs option to batch mode

[Chris Mi]

> -----Original Message-----
> From: n0-1@orbyte.nwl.cc [mailto:n0-1@orbyte.nwl.cc] On Behalf Of Phil
> Sutter
> Sent: Monday, January 8, 2018 9:32 PM
> To: Chris Mi <chrism@mellanox.com>
> Cc: dsahern@gmail.com; marcelo.leitner@gmail.com;
> netdev@vger.kernel.org; gerlitz.or@gmail.com;
> stephen@networkplumber.org
> Subject: Re: [patch iproute2 v6 0/3] tc: Add -bs option to batch mode
> 
> Hi Chris,
> 
> On Mon, Jan 08, 2018 at 02:03:53AM +0000, Chris Mi wrote:
> > > On Thu, Jan 04, 2018 at 04:34:51PM +0900, Chris Mi wrote:
> > > > The insertion rate is improved more than 10%.
> > >
> > > Did you measure the effect of increasing batch sizes?
> > Yes. Even if we enlarge the batch size bigger than 10, there is no big
> improvement.
> > I think that's because current kernel doesn't process the requests in
> parallel.
> > If kernel processes the requests in parallel, I believe specifying a
> > bigger batch size will get a better result.
> 
> But throughput doesn't regress at some point, right? I think that's the critical
> aspect when considering an "unlimited" batch size.
> 
> On Mon, Jan 08, 2018 at 08:00:00AM +0000, Chris Mi wrote:
> > After testing, I find that the message passed to kernel should not be too
> big.
> > If it is bigger than about 64K, sendmsg returns -1, errno is 90 (EMSGSIZE).
> > That is about 400 commands.  So how about set batch size to 128 which is
> big enough?
> 
> If that's the easiest way, why not. At first, I thought one could maybe send
> the collected messages in chunks of suitable size, but that's probably not
> worth the effort.
I did a testing. If we read a million commands in memory and send them in chunks of 128,
we'll have a big regression. It takes about 21 seconds.

RE: [patch iproute2 v6 1/3] lib/libnetlink: Add a function rtnl_talk_msg

[Chris Mi]

> -----Original Message-----
> From: David Ahern [mailto:dsahern@gmail.com]
> Sent: Saturday, January 6, 2018 1:51 AM
> To: Chris Mi <chrism@mellanox.com>; netdev@vger.kernel.org
> Cc: gerlitz.or@gmail.com; stephen@networkplumber.org;
> marcelo.leitner@gmail.com
> Subject: Re: [patch iproute2 v6 1/3] lib/libnetlink: Add a function
> rtnl_talk_msg
> 
> On 1/4/18 12:34 AM, Chris Mi wrote:
> > rtnl_talk can only send a single message to kernel. Add a new function
> > rtnl_talk_msg that can send multiple messages to kernel.
> >
> > Signed-off-by: Chris Mi <chrism@mellanox.com>
> > ---
> >  include/libnetlink.h |  3 +++
> >  lib/libnetlink.c     | 66 ++++++++++++++++++++++++++++++++++++++----
> ----------
> >  2 files changed, 51 insertions(+), 18 deletions(-)
> >
> 
> I think you should add an argument to rtnl_talk_msg to return the number of
> messages processed. That can be used to refine which line failed. As batch
> size increases the current design puts the burden on the user to scan a lot of
> lines to find the one that fails:
> 
> tc -b tc.batch  -bs 50
> RTNETLINK answers: File exists
> We have an error talking to the kernel, -1 Command failed tc.batch:2-51
> 
> We should be able to tell them exactly which line failed.
Done.
> 
> Also, it would be better to call this rtnl_talk_iov, take an iov as an argument
> and have a common rtnl_talk_msg for existing code and this new one.
> 
> As it stands you are having to add:
>    struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };
> 
> to tc functions when it really only needs to know about iov's.
Done.

RE: [patch iproute2 v6 2/3] tc: Add -bs option to batch mode

[Chris Mi]

> -----Original Message-----
> From: Marcelo Ricardo Leitner [mailto:marcelo.leitner@gmail.com]
> Sent: Saturday, January 6, 2018 3:15 AM
> To: David Ahern <dsahern@gmail.com>
> Cc: Chris Mi <chrism@mellanox.com>; netdev@vger.kernel.org;
> gerlitz.or@gmail.com; stephen@networkplumber.org
> Subject: Re: [patch iproute2 v6 2/3] tc: Add -bs option to batch mode
> 
> On Fri, Jan 05, 2018 at 11:15:59AM -0700, David Ahern wrote:
> > On 1/4/18 12:34 AM, 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, we can
> > > accumulate several commands before sending to kernel.
> > >
> > > Now it only works for the following successive rules, 1. filter add
> > > 2. filter delete 3. actions add 4. actions delete
> > >
> > > Otherwise, the batch size is still 1.
> > >
> > > Signed-off-by: Chris Mi <chrism@mellanox.com>
> > > ---
> > >  tc/m_action.c  |  93 ++++++++++++++++++++++++++++++----------
> > >  tc/tc.c        |  96 +++++++++++++++++++++++++++++++++++------
> > >  tc/tc_common.h |   8 +++-
> > >  tc/tc_filter.c | 132
> > > ++++++++++++++++++++++++++++++++++++++++-----------------
> > >  4 files changed, 252 insertions(+), 77 deletions(-)
> > >
> > > diff --git a/tc/m_action.c b/tc/m_action.c index fc422364..cf5cc95d
> > > 100644
> > > --- a/tc/m_action.c
> > > +++ b/tc/m_action.c
> > > @@ -23,6 +23,7 @@
> > >  #include <arpa/inet.h>
> > >  #include <string.h>
> > >  #include <dlfcn.h>
> > > +#include <errno.h>
> > >
> > >  #include "utils.h"
> > >  #include "tc_common.h"
> > > @@ -546,40 +547,86 @@ bad_val:
> > >  	return ret;
> > >  }
> > >
> > > +typedef struct {
> > > +	struct nlmsghdr		n;
> > > +	struct tcamsg		t;
> > > +	char			buf[MAX_MSG];
> > > +} tc_action_req;
> > > +
> > > +static tc_action_req *action_reqs;
> > > +static struct iovec msg_iov[MSG_IOV_MAX];
> > > +
> > > +void free_action_reqs(void)
> > > +{
> > > +	free(action_reqs);
> > > +}
> > > +
> > > +static tc_action_req *get_action_req(int batch_size, int index) {
> > > +	tc_action_req *req;
> > > +
> > > +	if (action_reqs == NULL) {
> > > +		action_reqs = malloc(batch_size * sizeof (tc_action_req));
> > > +		if (action_reqs == NULL)
> > > +			return NULL;
> > > +	}
> > > +	req = &action_reqs[index];
> > > +	memset(req, 0, sizeof (*req));
> > > +
> > > +	return req;
> > > +}
> > > +
> > >  static int tc_action_modify(int cmd, unsigned int flags,
> > > -			    int *argc_p, char ***argv_p)
> > > +			    int *argc_p, char ***argv_p,
> > > +			    int batch_size, int index, bool send)
> > >  {
> > > -	int argc = *argc_p;
> > > +	struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };
> > > +	struct iovec *iov = &msg_iov[index];
> > >  	char **argv = *argv_p;
> > > -	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 msghdr msg = {
> > > +		.msg_name = &nladdr,
> > > +		.msg_namelen = sizeof(nladdr),
> > > +		.msg_iov = msg_iov,
> > > +		.msg_iovlen = index + 1,
> > >  	};
> > > -	struct rtattr *tail = NLMSG_TAIL(&req.n);
> > > +	struct rtattr *tail;
> > > +	tc_action_req *req;
> > > +	int argc = *argc_p;
> > > +	int ret = 0;
> > > +
> > > +	req = get_action_req(batch_size, index);
> > > +	if (req == NULL) {
> > > +		fprintf(stderr, "get_action_req error: not enough buffer\n");
> > > +		return -ENOMEM;
> > > +	}
> > > +	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;
> > >
> > > -	if (rtnl_talk(&rth, &req.n, NULL) < 0) {
> > > +	*argc_p = argc;
> > > +	*argv_p = argv;
> > > +
> > > +	iov->iov_base = &req->n;
> > > +	iov->iov_len = req->n.nlmsg_len;
> > > +
> > > +	if (!send)
> > > +		return 0;
> > > +
> > > +	if (rtnl_talk_msg(&rth, &msg, 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 +726,7 @@ bad_val:
> > >  	return ret;
> > >  }
> > >
> > > -int do_action(int argc, char **argv)
> > > +int do_action(int argc, char **argv, int batch_size, int index,
> > > +bool send)
> > >  {
> > >
> > >  	int ret = 0;
> > > @@ -689,12 +736,14 @@ 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, batch_size,
> > > +						index, send);
> > >  		} 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, batch_size,
> > > +					       index, send);
> > >  		} else if (matches(*argv, "delete") == 0) {
> > >  			argc -= 1;
> > >  			argv += 1;
> > > diff --git a/tc/tc.c b/tc/tc.c
> > > index ad9f07e9..67c6bfb4 100644
> > > --- a/tc/tc.c
> > > +++ b/tc/tc.c
> > > @@ -189,20 +189,20 @@ static void usage(void)
> > >  	fprintf(stderr, "Usage: tc [ OPTIONS ] OBJECT { COMMAND |
> help }\n"
> > >  			"       tc [-force] -batch filename\n"
> > >  			"where  OBJECT := { qdisc | class | filter | action |
> monitor | exec }\n"
> > > -	                "       OPTIONS := { -s[tatistics] | -d[etails] | -r[aw] | -p[retty] |
> -b[atch] [filename] | -n[etns] name |\n"
> > > +	                "       OPTIONS := { -s[tatistics] | -d[etails] | -r[aw] | -p[retty] |
> -b[atch] [filename] | -bs | -batchsize [size] | -n[etns] name |\n"
> > >  			"                    -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, int batch_size, int index,
> > > +bool send)
> > >  {
> > >  	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, batch_size, index, send);
> > >  	if (matches(*argv, "actions") == 0)
> > > -		return do_action(argc-1, argv+1);
> > > +		return do_action(argc-1, argv+1, batch_size, index, send);
> > >  	if (matches(*argv, "monitor") == 0)
> > >  		return do_tcmonitor(argc-1, argv+1);
> > >  	if (matches(*argv, "exec") == 0)
> > > @@ -217,11 +217,25 @@ static int do_cmd(int argc, char **argv)
> > >  	return -1;
> > >  }
> > >
> > > -static int batch(const char *name)
> > > +static bool batchsize_enabled(int argc, char *argv[])
> > >  {
> > > +	if (argc < 2)
> > > +		return false;
> > > +	if (((strcmp(argv[0], "filter") != 0) && strcmp(argv[0], "action") != 0)
> > > +	    || ((strcmp(argv[1], "add") != 0) && strcmp(argv[1], "delete") != 0))
> > > +		return false;
> > > +	return true;
> > > +}
> > > +
> > > +static int batch(const char *name, int batch_size) {
> > > +	bool lastline = false;
> > > +	int msg_iov_index = 0;
> > > +	char *line2 = NULL;
> > >  	char *line = NULL;
> > >  	size_t len = 0;
> > >  	int ret = 0;
> > > +	bool send;
> > >
> > >  	batch_mode = 1;
> > >  	if (name && strcmp(name, "-") != 0) { @@ -240,23 +254,66 @@ static
> > > int batch(const char *name)
> > >  	}
> > >
> > >  	cmdlineno = 0;
> > > -	while (getcmdline(&line, &len, stdin) != -1) {
> > > +	if (getcmdline(&line, &len, stdin) == -1)
> > > +		goto Exit;
> > > +	do {
> > > +		char *largv2[100];
> > >  		char *largv[100];
> > > +		int largc2;
> > >  		int largc;
> > >
> > > +		if (getcmdline(&line2, &len, stdin) == -1)
> > > +			lastline = true;
> > > +
> > > +		if (batch_size > 1)
> > > +			largc2 = makeargs(line2, largv2, 100);
> > >  		largc = makeargs(line, largv, 100);
> > > +
> > > +		/*
> > > +		 * 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 (batch_size > 1 && msg_iov_index + 1 != batch_size
> > > +		    && !lastline && batchsize_enabled(largc, largv)
> > > +		    && batchsize_enabled(largc2, largv2))
> > > +			send = false;
> > > +		else
> > > +			send = true;
> > > +
> > > +		line = line2;
> > > +		line2 = NULL;
> > > +		len = 0;
> > > +
> > >  		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, batch_size, msg_iov_index, send);
> >
> > Perhaps I am missing something, but this design seems to assume the
> > file contains a series of filters or actions. What happens if actions
> > and filters are interlaced in the file?
> >
> > I think you need to look at having do_cmd return the generated request
> > as a buffer and length or have this batch function supply a buffer
> > that do_cmd fills instead of using its local req variable. The latter
> > would have better memory management. Then this batch function puts
> the
> > buffer into the iov and calls rtnl_talk_iov when it is ready to send the
> message.
> 
> Yes!   (That's pretty much what I tried to mean on Dec 27th)
> and then when rtnl_talk_iov returns, it knows all those buffers can be
> returned to the pool, regardless of the type of cmd.
> With this change, 'index' doesn't need to be here but somewhere below this
> function.
Done.