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