Date:   Wed, 27 Dec 2017 18:39:29 -0200
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     David Ahern <dsahern@...il.com>
Cc:     Chris Mi <chrism@...lanox.com>, netdev@...r.kernel.org,
        gerlitz.or@...il.com, stephen@...workplumber.org
Subject: Re: [patch iproute2 v3 3/4] tc: Add -bs option to batch mode

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@...lanox.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;
> 

Powered by blists - more mailing lists