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

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

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
>