> -----Original Message-----
> From: David Ahern [mailto:dsah...@gmail.com]
> Sent: Thursday, January 11, 2018 3:41 AM
> To: Chris Mi <chr...@mellanox.com>; netdev@vger.kernel.org
> Cc: gerlitz...@gmail.com; step...@networkplumber.org;
> marcelo.leit...@gmail.com; p...@nwl.cc
> Subject: Re: [patch iproute2 v8 2/2] tc: Add batchsize feature for filter and
> actions
>
> On 1/9/18 8:27 PM, 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 <chr...@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)
>
> you really need a buflen; you should not make assumptions about the length
> of buffer passed to these functions.
Done.
>
> > {
> > - 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;
> > +
>
> And a memset is needed for the !buf path since action_req is not initialized.
Done.
>
> > + 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;
>
> you can leave that as rtnl_talk; no need to change the !buf case to
> rtnl_talk_iov.
Done.
>
> > +
> > + 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;
>
> That is really hard to follow. It would be better to default the response to
> false and test for the expected set:
> if argv[0] is filter or actions &&
> argv[1] is add or delete or change or replace
> return true;
>
> return false;
>
I just realize that 'action delete' is not supported because function
tc_action_gd() is called
for 'delete' instead of tc_action_modify(). So there is a good reason to use
table now.
Done.
> > + return true;
> > +}
> > +
> > +struct batch_buf {
> > + struct batch_buf *next;
> > + char buf[16420]; /* sizeof (struct nlmsghdr) +
> > + sizeof (struct tcmsg) +
>
> actions has a different ancillary header -- tcamsg. Use the buflen approach is
> more robust.
Done. Actually 16420 = sizeof (struct nlmsghdr) + max(sizeof (struct tcmsg) +
sizeof (struct tcamsg)) + MAX_MSG */
>
>
> > + 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;
>
> space between variables and code.
Done.
>
> > + 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;
> > + }
> > + ++batchsize;
> > + }
> > +
> > + /*
> > + * 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 (!lastline && bs_enabled && bs_enabled_next
> > + && batchsize != MSG_IOV_MAX)
> > + send = false;
> > + else
> > + send = true;
> > +
> > + line = line_next;
> > + line_next = NULL;
> > + len = 0;
> > + bs_enabled_saved = bs_enabled;
> > + bs_enabled = bs_enabled_next;
> > + bs_enabled_next = false;
> >
> > - largc = makeargs(line, largv, 100);
> > 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, tail == NULL ? NULL : tail->buf);
> > + if (ret != 0) {
> > + fprintf(stderr, "Command failed %s:%d\n", name,
> > + cmdlineno - 1);
> > ret = 1;
> > if (!force)
> > break;
> > }
> > - }
> > - if (line)
> > - free(line);
> > + largc = largc_next;
> > + memcpy(largv, largv_next, largc * sizeof(char *));
> > +
> > + if (send && bs_enabled_saved) {
> > + struct iovec *iov, *iovs;
> > + struct batch_buf *buf;
> > + struct nlmsghdr *n;
> > + iov = iovs = malloc(batchsize * sizeof (struct iovec));
> > + for (buf = head; buf != NULL; buf = buf->next, ++iov)
> {
> > + n = (struct nlmsghdr *)&buf->buf;
> > + iov->iov_base = n;
> > + iov->iov_len = n->nlmsg_len;
> > + }
> > +
> > + ret = rtnl_talk_iov(&rth, iovs, batchsize, NULL);
> > + if (ret < 0) {
> > + fprintf(stderr, "Command failed %s:%d\n",
> name,
> > + cmdlineno - (batchsize + ret) - 1);
> > + return 2;
> > + }
> > + put_batch_bufs(&buf_pool, &head, &tail);
> > + batchsize = 0;
> > + free(iovs);
> > + }
> > + } while (!lastline);
> >
> > + free_batch_bufs(&buf_pool);
> > +Exit:
> > + free(line);
> > rtnl_close(&rth);
> > +
> > return ret;
> > }
> >
> > @@ -341,7 +478,7 @@ int main(int argc, char **argv)
> > goto Exit;
> > }
> >
> > - ret = do_cmd(argc-1, argv+1);
> > + ret = do_cmd(argc-1, argv+1, NULL);
> > Exit:
> > rtnl_close(&rth);
> >
> > diff --git a/tc/tc_common.h b/tc/tc_common.h index 264fbdac..59018da5
> > 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 128
> >
> > 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, void *buf); extern int
> > +do_action(int argc, char **argv, void *buf);
> > extern int do_tcmonitor(int argc, char **argv); extern int
> > do_exec(int argc, char **argv);
> >
> > diff --git a/tc/tc_filter.c b/tc/tc_filter.c index 545cc3a1..7db4964b
> > 100644
> > --- a/tc/tc_filter.c
> > +++ b/tc/tc_filter.c
> > @@ -42,28 +42,38 @@ 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)
> > +struct tc_filter_req {
> > + struct nlmsghdr n;
> > + struct tcmsg t;
> > + char buf[MAX_MSG];
> > +};
> > +
> > +static int tc_filter_modify(int cmd, unsigned int flags, int argc, char
> > **argv,
> > + void *buf)
> > {
> > - 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,
> > - };
> > + struct tc_filter_req *req, filter_req;
> > 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 d[IFNAMSIZ] = {};
> > + int protocol_set = 0;
> > char *fhandle = NULL;
> > - char d[IFNAMSIZ] = {};
> > - char k[FILTER_NAMESZ] = {};
> > - struct tc_estimator est = {};
> > + __u32 protocol = 0;
> > + __u32 chain_index;
> > + struct iovec iov;
> > + __u32 prio = 0;
> > + int ret;
> > +
> > + if (buf)
> > + req = buf;
> > + else
> > + req = &filter_req;
>
> same comment here about buflen and filter_req initialization.
Done.
>
>
> > +
> > + 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 +85,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 +162,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 +200,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");
>
> And here, no need to change to iov for the !buf case.
Done.
>
>
> > + iov.iov_base = &req->n;
> > + iov.iov_len = req->n.nlmsg_len;
> > +
> > + if (buf)
> > + return 0;
> > +
> > + ret = rtnl_talk_iov(&rth, &iov, 1, NULL);
> > + if (ret < 0) {
> > + fprintf(stderr, "We have an error talking to the kernel, %d\n",
> > +ret);
> > return 2;
> > }
> >
> > @@ -636,20 +653,20 @@ 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, void *buf)
> > {
> > 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, buf);
> > 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,
> buf);
> > if (matches(*argv, "replace") == 0)
> > return tc_filter_modify(RTM_NEWTFILTER, NLM_F_CREATE,
> argc-1,
> > - argv+1);
> > + argv+1, buf);
> > 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,
> buf);
> > 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
> >
