Re: [patch iproute2 v8 2/2] tc: Add batchsize feature for filter and actions

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

On Wed, Jan 10, 2018 at 12:27:42PM +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, at most 128
> commands can be accumulated before sending to kernel.
> 
> Now it only works for the following successive commands:
> filter and actions add/delete/change/replace.
> 
> Signed-off-by: Chris Mi <chrism@mellanox.com>
> ---
>  tc/m_action.c  |  60 +++++++++++++--------
>  tc/tc.c        | 165 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  tc/tc_common.h |   5 +-
>  tc/tc_filter.c |  97 +++++++++++++++++++--------------
>  4 files changed, 249 insertions(+), 78 deletions(-)
> 
> diff --git a/tc/m_action.c b/tc/m_action.c
> index fc422364..e5c53a80 100644
> --- a/tc/m_action.c
> +++ b/tc/m_action.c
> @@ -546,40 +546,56 @@ bad_val:
>  	return ret;
>  }
>  
> +struct tc_action_req {
> +	struct nlmsghdr		n;
> +	struct tcamsg		t;
> +	char			buf[MAX_MSG];
> +};
> +
>  static int tc_action_modify(int cmd, unsigned int flags,
> -			    int *argc_p, char ***argv_p)
> +			    int *argc_p, char ***argv_p,
> +			    void *buf)
>  {
> -	int argc = *argc_p;
> +	struct tc_action_req *req, action_req;
>  	char **argv = *argv_p;
> +	struct rtattr *tail;
> +	int argc = *argc_p;
> +	struct iovec iov;
>  	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 rtattr *tail = NLMSG_TAIL(&req.n);
> +
> +	if (buf)
> +		req = buf;
> +	else
> +		req = &action_req;
> +
> +	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;
> +
> +	*argc_p = argc;
> +	*argv_p = argv;
>  
> -	if (rtnl_talk(&rth, &req.n, NULL) < 0) {
> +	iov.iov_base = &req->n;
> +	iov.iov_len = req->n.nlmsg_len;
> +
> +	if (buf)
> +		return 0;
> +
> +	if (rtnl_talk_iov(&rth, &iov, 1, 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 +695,7 @@ bad_val:
>  	return ret;
>  }
>  
> -int do_action(int argc, char **argv)
> +int do_action(int argc, char **argv, void *buf)
>  {
>  
>  	int ret = 0;
> @@ -689,12 +705,12 @@ 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, buf);
>  		} 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, buf);
>  		} else if (matches(*argv, "delete") == 0) {
>  			argc -= 1;
>  			argv += 1;
> diff --git a/tc/tc.c b/tc/tc.c
> index ad9f07e9..44277405 100644
> --- a/tc/tc.c
> +++ b/tc/tc.c
> @@ -193,16 +193,16 @@ static void usage(void)
>  			"                    -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, void *buf)
>  {
>  	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, buf);
>  	if (matches(*argv, "actions") == 0)
> -		return do_action(argc-1, argv+1);
> +		return do_action(argc-1, argv+1, buf);
>  	if (matches(*argv, "monitor") == 0)
>  		return do_tcmonitor(argc-1, argv+1);
>  	if (matches(*argv, "exec") == 0)
> @@ -217,11 +217,79 @@ static int do_cmd(int argc, char **argv)
>  	return -1;
>  }
>  
> +static bool batchsize_enabled(int argc, char *argv[])
> +{
> +	if (argc < 2)
> +		return false;
> +	if ((matches(argv[0], "filter") && matches(argv[0], "actions"))
> +	    || (matches(argv[1], "add") && matches(argv[1], "delete")
> +	    && matches(argv[1], "change") && matches(argv[1], "replace")))
> +		return false;
> +	return true;
> +}
> +
> +struct batch_buf {
> +	struct batch_buf	*next;
> +	char			buf[16420];	/* sizeof (struct nlmsghdr) +
> +						   sizeof (struct tcmsg) +
> +						   MAX_MSG */
> +};
> +
> +static struct batch_buf *get_batch_buf(struct batch_buf **pool)
> +{
> +	struct batch_buf *buf;
> +
> +	if (*pool == NULL)
> +		buf = calloc(1, sizeof (struct batch_buf));
> +	else {
> +		buf = *pool;
> +		*pool = (*pool)->next;
> +		memset(buf, 0, sizeof (struct batch_buf));
> +	}
> +	return buf;
> +}
> +
> +static void put_batch_bufs(struct batch_buf **pool,
> +			   struct batch_buf **head, struct batch_buf **tail)
> +{
> +	if (*head == NULL || *tail == NULL)
> +		return;
> +
> +	if (*pool == NULL)
> +		*pool = *head;
> +	else {
> +		(*tail)->next = *pool;
> +		*pool = *head;
> +	}
> +	*head = NULL;
> +	*tail = NULL;
> +}
> +
> +static void free_batch_bufs(struct batch_buf **pool)
> +{
> +	struct batch_buf *buf;
> +
> +	for (buf = *pool; buf != NULL; buf = *pool) {
> +		*pool = buf->next;
> +		free(buf);
> +	}
> +	*pool = NULL;
> +}
> +
>  static int batch(const char *name)
>  {
> -	char *line = NULL;
> +	struct batch_buf *head = NULL, *tail = NULL, *buf_pool = NULL;
> +	char *largv[100], *largv_next[100];
> +	char *line, *line_next = NULL;
> +	bool bs_enabled_next = false;
> +	bool bs_enabled = false;
> +	bool lastline = false;
> +	int largc, largc_next;
> +	bool bs_enabled_saved;
> +	int batchsize = 0;
>  	size_t len = 0;
>  	int ret = 0;
> +	bool send;
>  
>  	batch_mode = 1;
>  	if (name && strcmp(name, "-") != 0) {
> @@ -240,25 +308,94 @@ static int batch(const char *name)
>  	}
>  
>  	cmdlineno = 0;
> -	while (getcmdline(&line, &len, stdin) != -1) {
> -		char *largv[100];
> -		int largc;
> +	if (getcmdline(&line, &len, stdin) == -1)
> +		goto Exit;
> +	largc = makeargs(line, largv, 100);
> +	bs_enabled = batchsize_enabled(largc, largv);
> +	bs_enabled_saved = bs_enabled;
> +	do {
> +		if (getcmdline(&line_next, &len, stdin) == -1)
> +			lastline = true;
> +
> +		largc_next = makeargs(line_next, largv_next, 100);
> +		bs_enabled_next = batchsize_enabled(largc_next, largv_next);
> +		if (bs_enabled) {
> +			struct batch_buf *buf;
> +			buf = get_batch_buf(&buf_pool);
> +			if (!buf) {
> +				fprintf(stderr, "failed to allocate batch_buf\n");
> +				return -1;
> +			}
> +			if (head == NULL)
> +				head = tail = buf;
> +			else {
> +				tail->next = buf;
> +				tail = buf;
> +			}

What about moving this list handling to get_batch_buf(), like you did
for put_batch_buf() ? Just for the sake of consistency.

Otherwise LGTM!

  Marcelo