[patch iproute2 v3 0/4] tc: Add -bs option to batch mode

[patch iproute2 v3 0/4] 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.125s
user    0m6.982s
sys     0m8.080s

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

real    0m12.772s
user    0m5.984s
sys     0m6.723s

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.484s
user    0m6.919s
sys     0m7.498s

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

real    0m11.664s
user    0m6.017s
sys     0m5.578s

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.

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

 include/libnetlink.h |   3 ++
 include/utils.h      |   4 ++
 lib/libnetlink.c     |  92 ++++++++++++++++++++++++++++++---------
 lib/utils.c          |  20 +++++++++
 man/man8/tc.8        |   5 +++
 tc/m_action.c        |  91 +++++++++++++++++++++++++++++---------
 tc/tc.c              |  47 ++++++++++++++++----
 tc/tc_common.h       |   8 +++-
 tc/tc_filter.c       | 121 +++++++++++++++++++++++++++++++++++++--------------
 9 files changed, 307 insertions(+), 84 deletions(-)

-- 
2.14.3

[patch iproute2 v3 1/4] 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     | 92 ++++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 74 insertions(+), 21 deletions(-)

diff --git a/include/libnetlink.h b/include/libnetlink.h
index a4d83b9e..e95fad75 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..f5f675cf 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -581,36 +581,21 @@ 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_check_ack(struct rtnl_handle *rtnl, struct nlmsghdr **answer,
+		       bool show_rtnl_err, nl_ext_ack_fn_t errfn,
+		       unsigned int seq)
 {
 	int status;
-	unsigned int seq;
-	struct nlmsghdr *h;
+	char *buf;
 	struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };
-	struct iovec iov = {
-		.iov_base = n,
-		.iov_len = n->nlmsg_len
-	};
+	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;
-
-	status = sendmsg(rtnl->fd, &msg, 0);
-	if (status < 0) {
-		perror("Cannot talk to rtnetlink");
-		return -1;
-	}
 
 	while (1) {
 		status = rtnl_recvmsg(rtnl->fd, &msg, &buf);
@@ -698,12 +683,77 @@ 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)
+{
+	unsigned int seq;
+	int status;
+	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,
+	};
+
+	n->nlmsg_seq = seq = ++rtnl->seq;
+
+	if (answer == NULL)
+		n->nlmsg_flags |= NLM_F_ACK;
+
+	status = sendmsg(rtnl->fd, &msg, 0);
+	if (status < 0) {
+		perror("Cannot talk to rtnetlink");
+		return -1;
+	}
+
+	return __rtnl_check_ack(rtnl, answer, show_rtnl_err, errfn, seq);
+}
+
+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 iovlen = m->msg_iovlen;
+	unsigned int seq = 0;
+	struct nlmsghdr *n;
+	struct iovec *v;
+	int i, status;
+
+	for (i = 0; i < iovlen; i++) {
+		v = &m->msg_iov[i];
+		n = v->iov_base;
+		n->nlmsg_seq = seq = ++rtnl->seq;
+		if (answer == NULL)
+			n->nlmsg_flags |= NLM_F_ACK;
+	}
+
+	status = sendmsg(rtnl->fd, m, 0);
+	if (status < 0) {
+		perror("Cannot talk to rtnetlink");
+		return -1;
+	}
+
+	return __rtnl_check_ack(rtnl, answer, show_rtnl_err, errfn, seq);
+}
+
 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 v3 2/4] utils: Add a function setcmdlinetotal

[Chris Mi]

This function calculates how many commands a batch file has and
set it to global variable cmdlinetotal.

Signed-off-by: Chris Mi <chrism@mellanox.com>
---
 include/utils.h |  4 ++++
 lib/utils.c     | 20 ++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/utils.h b/include/utils.h
index d3895d56..113a8c31 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -235,6 +235,10 @@ void print_nlmsg_timestamp(FILE *fp, const struct nlmsghdr *n);
 
 extern int cmdlineno;
 ssize_t getcmdline(char **line, size_t *len, FILE *in);
+
+extern int cmdlinetotal;
+void setcmdlinetotal(const char *name);
+
 int makeargs(char *line, char *argv[], int maxargs);
 int inet_get_addr(const char *src, __u32 *dst, struct in6_addr *dst6);
 
diff --git a/lib/utils.c b/lib/utils.c
index 7ced8c06..53ca389f 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -1202,6 +1202,26 @@ ssize_t getcmdline(char **linep, size_t *lenp, FILE *in)
 	return cc;
 }
 
+int cmdlinetotal;
+
+void setcmdlinetotal(const char *name)
+{
+	char *line = NULL;
+	size_t len = 0;
+
+	if (name && strcmp(name, "-") != 0) {
+		if (freopen(name, "r", stdin) == NULL) {
+			fprintf(stderr, "Cannot open file \"%s\" for reading: %s\n",
+				name, strerror(errno));
+			return;
+		}
+	}
+
+	cmdlinetotal = 0;
+	while (getcmdline(&line, &len, stdin) != -1)
+		cmdlinetotal++;
+}
+
 /* split command line into argument vector */
 int makeargs(char *line, char *argv[], int maxargs)
 {
-- 
2.14.3

[patch iproute2 v3 3/4] tc: Add -bs option to batch mode

[Chris Mi]

Signed-off-by: Chris Mi <chrism@mellanox.com>
---
 tc/m_action.c  |  91 +++++++++++++++++++++++++++++++++----------
 tc/tc.c        |  47 ++++++++++++++++++----
 tc/tc_common.h |   8 +++-
 tc/tc_filter.c | 121 +++++++++++++++++++++++++++++++++++++++++----------------
 4 files changed, 204 insertions(+), 63 deletions(-)

diff --git a/tc/m_action.c b/tc/m_action.c
index fc422364..c4c3b862 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,88 @@ 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;
 	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,
+	tc_action_req *req;
+	struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };
+	struct iovec *iov = &msg_iov[index];
+
+	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;
+	struct rtattr *tail = NLMSG_TAIL(&req->n);
+
+	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);
 
 	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;
+
+	iov->iov_base = &req->n;
+	iov->iov_len = req->n.nlmsg_len;
+
+	if (!send)
+		return 0;
 
-	if (rtnl_talk(&rth, &req.n, NULL) < 0) {
+	ret = rtnl_talk_msg(&rth, &msg, NULL);
+	if (ret < 0) {
 		fprintf(stderr, "We have an error talking to the kernel\n");
 		ret = -1;
 	}
 
-	*argc_p = argc;
-	*argv_p = argv;
-
 	return ret;
 }
 
@@ -679,7 +728,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 +738,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..7ea2fc89 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,16 @@ static int do_cmd(int argc, char **argv)
 	return -1;
 }
 
-static int batch(const char *name)
+static int batch(const char *name, int batch_size)
 {
+	int msg_iov_index = 0;
 	char *line = NULL;
 	size_t len = 0;
 	int ret = 0;
+	bool send;
+
+	if (batch_size > 1)
+		setcmdlinetotal(name);
 
 	batch_mode = 1;
 	if (name && strcmp(name, "-") != 0) {
@@ -248,15 +253,30 @@ static int batch(const char *name)
 		if (largc == 0)
 			continue;	/* blank line */
 
-		if (do_cmd(largc, largv)) {
+		/*
+		 * In batch mode, if we haven't accumulated enough commands
+		 * and this is not the last command, don't send the message
+		 * immediately.
+		 */
+		if (batch_size > 1 && msg_iov_index + 1 != batch_size
+		    && cmdlineno != cmdlinetotal)
+			send = false;
+		else
+			send = true;
+
+		ret = do_cmd(largc, largv, batch_size, msg_iov_index++, send);
+		if (ret < 0) {
 			fprintf(stderr, "Command failed %s:%d\n", name, cmdlineno);
 			ret = 1;
 			if (!force)
 				break;
 		}
+		msg_iov_index %= batch_size;
 	}
 	if (line)
 		free(line);
+	free_filter_reqs();
+	free_action_reqs();
 
 	rtnl_close(&rth);
 	return ret;
@@ -267,6 +287,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 +318,14 @@ 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 (matches(argv[1], "-netns") == 0) {
 			NEXT_ARG();
 			if (netns_switch(argv[1]))
@@ -323,7 +352,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 +370,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..6fecbb45 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,18 +43,44 @@ 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)
+{
+	tc_filter_req *req;
+	int ret;
+
+	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;
@@ -65,6 +92,24 @@ static int tc_filter_modify(int cmd, unsigned int flags, int argc, char **argv)
 	char  k[FILTER_NAMESZ] = {};
 	struct tc_estimator est = {};
 
+	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;
+
+	struct msghdr msg = {
+		.msg_name = &nladdr,
+		.msg_namelen = sizeof(nladdr),
+		.msg_iov = msg_iov,
+		.msg_iovlen = index + 1,
+	};
+
 	if (cmd == RTM_NEWTFILTER && flags & NLM_F_CREATE)
 		protocol = htons(ETH_P_ALL);
 
@@ -75,37 +120,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 +197,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 +235,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 +688,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 v3 4/4] man: Add -bs option to tc manpage

[Chris Mi]

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

diff --git a/man/man8/tc.8 b/man/man8/tc.8
index ff071b33..de137e16 100644
--- a/man/man8/tc.8
+++ b/man/man8/tc.8
@@ -601,6 +601,11 @@ 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.
+
 .TP
 .BR "\-force"
 don't terminate tc on errors in batch mode.
-- 
2.14.3

Re: [patch iproute2 v3 1/4] lib/libnetlink: Add a function rtnl_talk_msg

[David Ahern]

On 12/25/17 2:46 AM, Chris Mi wrote:
> diff --git a/lib/libnetlink.c b/lib/libnetlink.c
> index 00e6ce0c..f5f675cf 100644
> --- a/lib/libnetlink.c
> +++ b/lib/libnetlink.c
> @@ -581,36 +581,21 @@ 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_check_ack(struct rtnl_handle *rtnl, struct nlmsghdr **answer,

Make this function __rtnl_talk_msg. Include the assignment of nlmsg_seq
and ack setting using the for loop below and sendmsg() call. All of that
code can be common for both the single and multiple iov case.


> +		       bool show_rtnl_err, nl_ext_ack_fn_t errfn,
> +		       unsigned int seq)
>  {
>  	int status;
> -	unsigned int seq;
> -	struct nlmsghdr *h;
> +	char *buf;

Please order variables in the reverse xmas tree style used in the net code.

>  	struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };
> -	struct iovec iov = {
> -		.iov_base = n,
> -		.iov_len = n->nlmsg_len
> -	};
> +	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;
> -
> -	status = sendmsg(rtnl->fd, &msg, 0);
> -	if (status < 0) {
> -		perror("Cannot talk to rtnetlink");
> -		return -1;
> -	}
>  
>  	while (1) {
>  		status = rtnl_recvmsg(rtnl->fd, &msg, &buf);

Re: [patch iproute2 v3 2/4] utils: Add a function setcmdlinetotal

[David Ahern]

On 12/25/17 2:46 AM, Chris Mi wrote:
> This function calculates how many commands a batch file has and
> set it to global variable cmdlinetotal.
> 
> Signed-off-by: Chris Mi <chrism@mellanox.com>
> ---
>  include/utils.h |  4 ++++
>  lib/utils.c     | 20 ++++++++++++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/include/utils.h b/include/utils.h
> index d3895d56..113a8c31 100644
> --- a/include/utils.h
> +++ b/include/utils.h
> @@ -235,6 +235,10 @@ void print_nlmsg_timestamp(FILE *fp, const struct nlmsghdr *n);
>  
>  extern int cmdlineno;
>  ssize_t getcmdline(char **line, size_t *len, FILE *in);
> +
> +extern int cmdlinetotal;
> +void setcmdlinetotal(const char *name);
> +
>  int makeargs(char *line, char *argv[], int maxargs);
>  int inet_get_addr(const char *src, __u32 *dst, struct in6_addr *dst6);
>  
> diff --git a/lib/utils.c b/lib/utils.c
> index 7ced8c06..53ca389f 100644
> --- a/lib/utils.c
> +++ b/lib/utils.c
> @@ -1202,6 +1202,26 @@ ssize_t getcmdline(char **linep, size_t *lenp, FILE *in)
>  	return cc;
>  }
>  
> +int cmdlinetotal;
> +
> +void setcmdlinetotal(const char *name)
> +{
> +	char *line = NULL;
> +	size_t len = 0;
> +
> +	if (name && strcmp(name, "-") != 0) {
> +		if (freopen(name, "r", stdin) == NULL) {
> +			fprintf(stderr, "Cannot open file \"%s\" for reading: %s\n",
> +				name, strerror(errno));
> +			return;
> +		}
> +	}
> +
> +	cmdlinetotal = 0;
> +	while (getcmdline(&line, &len, stdin) != -1)
> +		cmdlinetotal++;
> +}
> +
>  /* split command line into argument vector */
>  int makeargs(char *line, char *argv[], int maxargs)
>  {
> 

This helper should not be needed. There is no need to read what could be
a million+ line file multiple times.

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

[David Ahern]

On 12/25/17 2:46 AM, Chris Mi wrote:
> Signed-off-by: Chris Mi <chrism@mellanox.com>
> ---
>  tc/m_action.c  |  91 +++++++++++++++++++++++++++++++++----------
>  tc/tc.c        |  47 ++++++++++++++++++----
>  tc/tc_common.h |   8 +++-
>  tc/tc_filter.c | 121 +++++++++++++++++++++++++++++++++++++++++----------------
>  4 files changed, 204 insertions(+), 63 deletions(-)
> 
> diff --git a/tc/m_action.c b/tc/m_action.c
> index fc422364..c4c3b862 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,88 @@ 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;
>  	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,
> +	tc_action_req *req;
> +	struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };
> +	struct iovec *iov = &msg_iov[index];
> +
> +	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;
> +	struct rtattr *tail = NLMSG_TAIL(&req->n);
> +
> +	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);
>  
>  	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;
> +
> +	iov->iov_base = &req->n;
> +	iov->iov_len = req->n.nlmsg_len;
> +
> +	if (!send)
> +		return 0;
>  
> -	if (rtnl_talk(&rth, &req.n, NULL) < 0) {
> +	ret = rtnl_talk_msg(&rth, &msg, NULL);
> +	if (ret < 0) {
>  		fprintf(stderr, "We have an error talking to the kernel\n");
>  		ret = -1;
>  	}
>  
> -	*argc_p = argc;
> -	*argv_p = argv;
> -
>  	return ret;
>  }
>  
> @@ -679,7 +728,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 +738,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..7ea2fc89 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,16 @@ static int do_cmd(int argc, char **argv)
>  	return -1;
>  }
>  
> -static int batch(const char *name)
> +static int batch(const char *name, int batch_size)
>  {
> +	int msg_iov_index = 0;
>  	char *line = NULL;
>  	size_t len = 0;
>  	int ret = 0;
> +	bool send;
> +
> +	if (batch_size > 1)
> +		setcmdlinetotal(name);
>  
>  	batch_mode = 1;
>  	if (name && strcmp(name, "-") != 0) {
> @@ -248,15 +253,30 @@ static int batch(const char *name)
>  		if (largc == 0)
>  			continue;	/* blank line */
>  
> -		if (do_cmd(largc, largv)) {
> +		/*
> +		 * In batch mode, if we haven't accumulated enough commands
> +		 * and this is not the last command, don't send the message
> +		 * immediately.
> +		 */
> +		if (batch_size > 1 && msg_iov_index + 1 != batch_size
> +		    && cmdlineno != cmdlinetotal)
> +			send = false;
> +		else
> +			send = true;
> +
> +		ret = do_cmd(largc, largv, batch_size, msg_iov_index++, send);

What happens if tc commands are interlaced in the file -- qdisc add,
class add, filter add, then a delete, show, exec, etc.? Right now each
command is handled one at a time so an add followed by a delete will
work. Your proposed batching loop won't work for this case as some
commands are executed when that line is reached and others are batched
for later send. Not all of the tc commands need to be batched in a
single message so perhaps those commands cause the queue to be flushed
(ie, message sent), then that command is executed and you start the
batching over.

Further, I really think the batching can be done without the global
variables and without the command handlers knowing it is batching or
part of an iov. e.g., in the case of batching try having the commands
malloc the request buffer and return the pointer back to this loop in
which case this loop calls rtnl_talk_msg and frees the buffers.

> +		if (ret < 0) {
>  			fprintf(stderr, "Command failed %s:%d\n", name, cmdlineno);
>  			ret = 1;
>  			if (!force)
>  				break;
>  		}
> +		msg_iov_index %= batch_size;
>  	}
>  	if (line)
>  		free(line);
> +	free_filter_reqs();
> +	free_action_reqs();
>  
>  	rtnl_close(&rth);
>  	return ret;

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

[Marcelo Ricardo Leitner]

On Mon, Dec 25, 2017 at 05:46:57PM +0900, Chris Mi wrote:
> @@ -267,6 +287,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 +318,14 @@ 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;

what about
if (batch_size < 1)
	batch_size = 1;

>  		} else if (matches(argv[1], "-netns") == 0) {
>  			NEXT_ARG();
>  			if (netns_switch(argv[1]))

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

[Marcelo Ricardo Leitner]

On Wed, Dec 27, 2017 at 09:39:15AM -0600, David Ahern wrote:
> On 12/25/17 2:46 AM, Chris Mi wrote:
> > Signed-off-by: Chris Mi <chrism@mellanox.com>
> > ---
> >  tc/m_action.c  |  91 +++++++++++++++++++++++++++++++++----------
> >  tc/tc.c        |  47 ++++++++++++++++++----
> >  tc/tc_common.h |   8 +++-
> >  tc/tc_filter.c | 121 +++++++++++++++++++++++++++++++++++++++++----------------
> >  4 files changed, 204 insertions(+), 63 deletions(-)
> > 
> > diff --git a/tc/m_action.c b/tc/m_action.c
> > index fc422364..c4c3b862 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,88 @@ 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;
> >  	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,
> > +	tc_action_req *req;
> > +	struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };
> > +	struct iovec *iov = &msg_iov[index];
> > +
> > +	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;
> > +	struct rtattr *tail = NLMSG_TAIL(&req->n);
> > +
> > +	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);
> >  
> >  	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;
> > +
> > +	iov->iov_base = &req->n;
> > +	iov->iov_len = req->n.nlmsg_len;
> > +
> > +	if (!send)
> > +		return 0;
> >  
> > -	if (rtnl_talk(&rth, &req.n, NULL) < 0) {
> > +	ret = rtnl_talk_msg(&rth, &msg, NULL);
> > +	if (ret < 0) {
> >  		fprintf(stderr, "We have an error talking to the kernel\n");
> >  		ret = -1;
> >  	}
> >  
> > -	*argc_p = argc;
> > -	*argv_p = argv;
> > -
> >  	return ret;
> >  }
> >  
> > @@ -679,7 +728,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 +738,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..7ea2fc89 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,16 @@ static int do_cmd(int argc, char **argv)
> >  	return -1;
> >  }
> >  
> > -static int batch(const char *name)
> > +static int batch(const char *name, int batch_size)
> >  {
> > +	int msg_iov_index = 0;
> >  	char *line = NULL;
> >  	size_t len = 0;
> >  	int ret = 0;
> > +	bool send;
> > +
> > +	if (batch_size > 1)
> > +		setcmdlinetotal(name);
> >  
> >  	batch_mode = 1;
> >  	if (name && strcmp(name, "-") != 0) {
> > @@ -248,15 +253,30 @@ static int batch(const char *name)
> >  		if (largc == 0)
> >  			continue;	/* blank line */
> >  
> > -		if (do_cmd(largc, largv)) {
> > +		/*
> > +		 * In batch mode, if we haven't accumulated enough commands
> > +		 * and this is not the last command, don't send the message
> > +		 * immediately.
> > +		 */
> > +		if (batch_size > 1 && msg_iov_index + 1 != batch_size
> > +		    && cmdlineno != cmdlinetotal)

I think you can replace the cmdlineno check with a simple !feof(stdin)

> > +			send = false;
> > +		else
> > +			send = true;
> > +
> > +		ret = do_cmd(largc, largv, batch_size, msg_iov_index++, send);
> 
> What happens if tc commands are interlaced in the file -- qdisc add,
> class add, filter add, then a delete, show, exec, etc.? Right now each
> command is handled one at a time so an add followed by a delete will
> work. Your proposed batching loop won't work for this case as some
> commands are executed when that line is reached and others are batched
> for later send. Not all of the tc commands need to be batched in a
> single message so perhaps those commands cause the queue to be flushed
> (ie, message sent), then that command is executed and you start the
> batching over.
> 
> Further, I really think the batching can be done without the global
> variables and without the command handlers knowing it is batching or
> part of an iov. e.g., in the case of batching try having the commands
> malloc the request buffer and return the pointer back to this loop in
> which case this loop calls rtnl_talk_msg and frees the buffers.

Sounds like the batching is being done at the wrong level. If it was
done by rtnl_talk(), it should be easier.
We can keep rtnl_talk() for previous users and make rtnl_talk_msg() do
the batching, mostly independent of which kind of msg it it.

As you need to inform it that it was the last entry, that may be
detected with feof(stdin). Just add a 'bool flush' parameter to it.
   rtnl_talk_msg(...., flush=feof(stdin));

Next step then would be to add a memory manager layer to it, so
libnetlink wouldn't need to copy the messages but recycle pointers:
  rtnl_get_msgbuf(): returns a buffer that one can use to fill in the
    msg and use with rtnl_talk_msg()
  and the free is done by libnetlink itself when the message is
  finally sent, so no need to keep track of what one needs to free or
  can reuse.

> 
> > +		if (ret < 0) {
> >  			fprintf(stderr, "Command failed %s:%d\n", name, cmdlineno);
> >  			ret = 1;
> >  			if (!force)
> >  				break;
> >  		}
> > +		msg_iov_index %= batch_size;
> >  	}
> >  	if (line)
> >  		free(line);
> > +	free_filter_reqs();
> > +	free_action_reqs();
> >  
> >  	rtnl_close(&rth);
> >  	return ret;
> 

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

[Stephen Hemminger]

On Wed, 27 Dec 2017 18:39:29 -0200
Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote:

> > > +			send = false;
> > > +		else
> > > +			send = true;
> > > +
> > > +		ret = do_cmd(largc, largv, batch_size, msg_iov_index++, send);  
> > 
> > What happens if tc commands are interlaced in the file -- qdisc add,
> > class add, filter add, then a delete, show, exec, etc.? Right now each
> > command is handled one at a time so an add followed by a delete will
> > work. Your proposed batching loop won't work for this case as some
> > commands are executed when that line is reached and others are batched
> > for later send. Not all of the tc commands need to be batched in a
> > single message so perhaps those commands cause the queue to be flushed
> > (ie, message sent), then that command is executed and you start the
> > batching over.
> > 
> > Further, I really think the batching can be done without the global
> > variables and without the command handlers knowing it is batching or
> > part of an iov. e.g., in the case of batching try having the commands
> > malloc the request buffer and return the pointer back to this loop in
> > which case this loop calls rtnl_talk_msg and frees the buffers.  
> 
> Sounds like the batching is being done at the wrong level. If it was
> done by rtnl_talk(), it should be easier.
> We can keep rtnl_talk() for previous users and make rtnl_talk_msg() do
> the batching, mostly independent of which kind of msg it it.
> 
> As you need to inform it that it was the last entry, that may be
> detected with feof(stdin). Just add a 'bool flush' parameter to it.
>    rtnl_talk_msg(...., flush=feof(stdin));
> 
> Next step then would be to add a memory manager layer to it, so
> libnetlink wouldn't need to copy the messages but recycle pointers:
>   rtnl_get_msgbuf(): returns a buffer that one can use to fill in the
>     msg and use with rtnl_talk_msg()
>   and the free is done by libnetlink itself when the message is
>   finally sent, so no need to keep track of what one needs to free or
>   can reuse.


What about using sendmmsg instead?
That woudl allow sending multiple messages in one syscall.

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

[Marcelo Ricardo Leitner]

On Wed, Dec 27, 2017 at 01:40:24PM -0800, Stephen Hemminger wrote:
> On Wed, 27 Dec 2017 18:39:29 -0200
> Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote:
> 
> > > > +			send = false;
> > > > +		else
> > > > +			send = true;
> > > > +
> > > > +		ret = do_cmd(largc, largv, batch_size, msg_iov_index++, send);  
> > > 
> > > What happens if tc commands are interlaced in the file -- qdisc add,
> > > class add, filter add, then a delete, show, exec, etc.? Right now each
> > > command is handled one at a time so an add followed by a delete will
> > > work. Your proposed batching loop won't work for this case as some
> > > commands are executed when that line is reached and others are batched
> > > for later send. Not all of the tc commands need to be batched in a
> > > single message so perhaps those commands cause the queue to be flushed
> > > (ie, message sent), then that command is executed and you start the
> > > batching over.
> > > 
> > > Further, I really think the batching can be done without the global
> > > variables and without the command handlers knowing it is batching or
> > > part of an iov. e.g., in the case of batching try having the commands
> > > malloc the request buffer and return the pointer back to this loop in
> > > which case this loop calls rtnl_talk_msg and frees the buffers.  
> > 
> > Sounds like the batching is being done at the wrong level. If it was
> > done by rtnl_talk(), it should be easier.
> > We can keep rtnl_talk() for previous users and make rtnl_talk_msg() do
> > the batching, mostly independent of which kind of msg it it.
> > 
> > As you need to inform it that it was the last entry, that may be
> > detected with feof(stdin). Just add a 'bool flush' parameter to it.
> >    rtnl_talk_msg(...., flush=feof(stdin));
> > 
> > Next step then would be to add a memory manager layer to it, so
> > libnetlink wouldn't need to copy the messages but recycle pointers:
> >   rtnl_get_msgbuf(): returns a buffer that one can use to fill in the
> >     msg and use with rtnl_talk_msg()
> >   and the free is done by libnetlink itself when the message is
> >   finally sent, so no need to keep track of what one needs to free or
> >   can reuse.
> 
> 
> What about using sendmmsg instead?
> That woudl allow sending multiple messages in one syscall.

Could be. Although the batching effect would be very different.
sendmmsg calls cond_resched() between messages, for instance.

Re: [patch iproute2 v3 1/4] lib/libnetlink: Add a function rtnl_talk_msg

[Chris Mi]

> On 12/25/17 2:46 AM, Chris Mi wrote:
>> diff --git a/lib/libnetlink.c b/lib/libnetlink.c
>> index 00e6ce0c..f5f675cf 100644
>> --- a/lib/libnetlink.c
>> +++ b/lib/libnetlink.c
>> @@ -581,36 +581,21 @@ 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_check_ack(struct rtnl_handle *rtnl, struct nlmsghdr **answer,
> Make this function __rtnl_talk_msg. Include the assignment of nlmsg_seq
> and ack setting using the for loop below and sendmsg() call. All of that
> code can be common for both the single and multiple iov case.
Thanks for your suggestion. Done.
>
>> +		       bool show_rtnl_err, nl_ext_ack_fn_t errfn,
>> +		       unsigned int seq)
>>   {
>>   	int status;
>> -	unsigned int seq;
>> -	struct nlmsghdr *h;
>> +	char *buf;
> Please order variables in the reverse xmas tree style used in the net code.
Actually, I divide the variables in two parts, none-struct variables and 
struct variables.
Not sure if that meets the reverse xmac tree style, but I think it is 
more readable.
>
>>   	struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };
>> -	struct iovec iov = {
>> -		.iov_base = n,
>> -		.iov_len = n->nlmsg_len
>> -	};
>> +	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;
>> -
>> -	status = sendmsg(rtnl->fd, &msg, 0);
>> -	if (status < 0) {
>> -		perror("Cannot talk to rtnetlink");
>> -		return -1;
>> -	}
>>   
>>   	while (1) {
>>   		status = rtnl_recvmsg(rtnl->fd, &msg, &buf);

Re: [patch iproute2 v3 2/4] utils: Add a function setcmdlinetotal

[Chris Mi]

> On 12/25/17 2:46 AM, Chris Mi wrote:
>> This function calculates how many commands a batch file has and
>> set it to global variable cmdlinetotal.
>>
>> Signed-off-by: Chris Mi <chrism@mellanox.com>
>> ---
>>   include/utils.h |  4 ++++
>>   lib/utils.c     | 20 ++++++++++++++++++++
>>   2 files changed, 24 insertions(+)
>>
>> diff --git a/include/utils.h b/include/utils.h
>> index d3895d56..113a8c31 100644
>> --- a/include/utils.h
>> +++ b/include/utils.h
>> @@ -235,6 +235,10 @@ void print_nlmsg_timestamp(FILE *fp, const struct nlmsghdr *n);
>>   
>>   extern int cmdlineno;
>>   ssize_t getcmdline(char **line, size_t *len, FILE *in);
>> +
>> +extern int cmdlinetotal;
>> +void setcmdlinetotal(const char *name);
>> +
>>   int makeargs(char *line, char *argv[], int maxargs);
>>   int inet_get_addr(const char *src, __u32 *dst, struct in6_addr *dst6);
>>   
>> diff --git a/lib/utils.c b/lib/utils.c
>> index 7ced8c06..53ca389f 100644
>> --- a/lib/utils.c
>> +++ b/lib/utils.c
>> @@ -1202,6 +1202,26 @@ ssize_t getcmdline(char **linep, size_t *lenp, FILE *in)
>>   	return cc;
>>   }
>>   
>> +int cmdlinetotal;
>> +
>> +void setcmdlinetotal(const char *name)
>> +{
>> +	char *line = NULL;
>> +	size_t len = 0;
>> +
>> +	if (name && strcmp(name, "-") != 0) {
>> +		if (freopen(name, "r", stdin) == NULL) {
>> +			fprintf(stderr, "Cannot open file \"%s\" for reading: %s\n",
>> +				name, strerror(errno));
>> +			return;
>> +		}
>> +	}
>> +
>> +	cmdlinetotal = 0;
>> +	while (getcmdline(&line, &len, stdin) != -1)
>> +		cmdlinetotal++;
>> +}
>> +
>>   /* split command line into argument vector */
>>   int makeargs(char *line, char *argv[], int maxargs)
>>   {
>>
> This helper should not be needed. There is no need to read what could be
> a million+ line file multiple times.
Done. I removed this helper. But we can't simply use !feof directly.
I figure out a way to determine if we are reaching the end of file by
reading one more line of the batch file.

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

[Chris Mi]

> On 12/25/17 2:46 AM, Chris Mi wrote:
>> Signed-off-by: Chris Mi <chrism@mellanox.com>
>> ---
>>   tc/m_action.c  |  91 +++++++++++++++++++++++++++++++++----------
>>   tc/tc.c        |  47 ++++++++++++++++++----
>>   tc/tc_common.h |   8 +++-
>>   tc/tc_filter.c | 121 +++++++++++++++++++++++++++++++++++++++++----------------
>>   4 files changed, 204 insertions(+), 63 deletions(-)
>>
>> diff --git a/tc/m_action.c b/tc/m_action.c
>> index fc422364..c4c3b862 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,88 @@ 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;
>>   	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,
>> +	tc_action_req *req;
>> +	struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };
>> +	struct iovec *iov = &msg_iov[index];
>> +
>> +	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;
>> +	struct rtattr *tail = NLMSG_TAIL(&req->n);
>> +
>> +	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);
>>   
>>   	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;
>> +
>> +	iov->iov_base = &req->n;
>> +	iov->iov_len = req->n.nlmsg_len;
>> +
>> +	if (!send)
>> +		return 0;
>>   
>> -	if (rtnl_talk(&rth, &req.n, NULL) < 0) {
>> +	ret = rtnl_talk_msg(&rth, &msg, NULL);
>> +	if (ret < 0) {
>>   		fprintf(stderr, "We have an error talking to the kernel\n");
>>   		ret = -1;
>>   	}
>>   
>> -	*argc_p = argc;
>> -	*argv_p = argv;
>> -
>>   	return ret;
>>   }
>>   
>> @@ -679,7 +728,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 +738,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..7ea2fc89 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,16 @@ static int do_cmd(int argc, char **argv)
>>   	return -1;
>>   }
>>   
>> -static int batch(const char *name)
>> +static int batch(const char *name, int batch_size)
>>   {
>> +	int msg_iov_index = 0;
>>   	char *line = NULL;
>>   	size_t len = 0;
>>   	int ret = 0;
>> +	bool send;
>> +
>> +	if (batch_size > 1)
>> +		setcmdlinetotal(name);
>>   
>>   	batch_mode = 1;
>>   	if (name && strcmp(name, "-") != 0) {
>> @@ -248,15 +253,30 @@ static int batch(const char *name)
>>   		if (largc == 0)
>>   			continue;	/* blank line */
>>   
>> -		if (do_cmd(largc, largv)) {
>> +		/*
>> +		 * In batch mode, if we haven't accumulated enough commands
>> +		 * and this is not the last command, don't send the message
>> +		 * immediately.
>> +		 */
>> +		if (batch_size > 1 && msg_iov_index + 1 != batch_size
>> +		    && cmdlineno != cmdlinetotal)
>> +			send = false;
>> +		else
>> +			send = true;
>> +
>> +		ret = do_cmd(largc, largv, batch_size, msg_iov_index++, send);
> What happens if tc commands are interlaced in the file -- qdisc add,
> class add, filter add, then a delete, show, exec, etc.? Right now each
> command is handled one at a time so an add followed by a delete will
> work. Your proposed batching loop won't work for this case as some
> commands are executed when that line is reached and others are batched
> for later send. Not all of the tc commands need to be batched in a
> single message so perhaps those commands cause the queue to be flushed
> (ie, message sent), then that command is executed and you start the
> batching over.
Maybe you are right. But the intention of this patch is to improve the 
insertion rate.
We can use it as benchmark tool. So we only care about the filter add or 
action add.
If the user mixes many different subcommands in one batch file, I don't 
think he can
get any benfit from this patch. Maybe he should ignore the -bs option.
>
> Further, I really think the batching can be done without the global
> variables and without the command handlers knowing it is batching or
> part of an iov. e.g., in the case of batching try having the commands
> malloc the request buffer and return the pointer back to this loop in
> which case this loop calls rtnl_talk_msg and frees the buffers.
But not all of these commands will end up calling rtnl_talk. They may do 
different things.
I think current code is clear and easy to maitain, I mean the functions 
do_xxx.
If I change the code as you suggested, I think function batch will be 
very complex
and hard to maitain.
>
>> +		if (ret < 0) {
>>   			fprintf(stderr, "Command failed %s:%d\n", name, cmdlineno);
>>   			ret = 1;
>>   			if (!force)
>>   				break;
>>   		}
>> +		msg_iov_index %= batch_size;
>>   	}
>>   	if (line)
>>   		free(line);
>> +	free_filter_reqs();
>> +	free_action_reqs();
>>   
>>   	rtnl_close(&rth);
>>   	return ret;

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

[Chris Mi]

> On Mon, Dec 25, 2017 at 05:46:57PM +0900, Chris Mi wrote:
>> @@ -267,6 +287,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 +318,14 @@ 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;
> what about
> if (batch_size < 1)
> 	batch_size = 1;
Done.
>
>>   		} else if (matches(argv[1], "-netns") == 0) {
>>   			NEXT_ARG();
>>   			if (netns_switch(argv[1]))