> -----Original Message-----
> From: Marcelo Ricardo Leitner [mailto:marcelo.leit...@gmail.com]
> Sent: Saturday, January 6, 2018 3:15 AM
> To: David Ahern <dsah...@gmail.com>
> Cc: Chris Mi <chr...@mellanox.com>; netdev@vger.kernel.org;
> gerlitz...@gmail.com; step...@networkplumber.org
> Subject: Re: [patch iproute2 v6 2/3] tc: Add -bs option to batch mode
>
> 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 <chr...@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.
Done.
