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

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

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