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

[Chris Mi]

> -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;
> > +   charbuf[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;
> > -   charbuf[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();
> > +
> > +   if (buf)
> > +   req = buf;
> > +   else
> > +   req = _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(>n);
> >
> > argc -= 1;
> > argv += 1;
> > -   if (parse_action(, , TCA_ACT_TAB, )) {
> > +   if (parse_action(, , TCA_ACT_TAB, >n)) {
> > fprintf(stderr, "Illegal \"action\"\n");
> > return -1;
> > }
> > -   tail->rta_len = (void *) NLMSG_TAIL() - (void *) tail;
> > +   tail->rta_len = (void *) NLMSG_TAIL(>n) - (void *) tail;
> > +
> > +   *argc_p = argc;
> > +   *argv_p = argv;
> >
> > -   if (rtnl_talk(, , NULL) < 0) {
> > +   iov.iov_base = >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(, , 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,
> >   

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

[Chris Mi]

> -Original Message-
> From: Marcelo Ricardo Leitner [mailto:marcelo.leit...@gmail.com]
> Sent: Wednesday, January 10, 2018 7:42 PM
> To: Chris Mi <chr...@mellanox.com>
> Cc: netdev@vger.kernel.org; gerlitz...@gmail.com;
> step...@networkplumber.org; dsah...@gmail.com; p...@nwl.cc
> Subject: Re: [patch iproute2 v8 2/2] tc: Add batchsize feature for filter and
> actions
> 
> 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 <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;
> > +   charbuf[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;
> > -   charbuf[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();
> > +
> > +   if (buf)
> > +   req = buf;
> > +   else
> > +   req = _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(>n);
> >
> > argc -= 1;
> > argv += 1;
> > -   if (parse_action(, , TCA_ACT_TAB, )) {
> > +   if (parse_action(, , TCA_ACT_TAB, >n)) {
> > fprintf(stderr, "Illegal \"action\"\n");
> > return -1;
> > }
> > -   tail->rta_len = (void *) NLMSG_TAIL() - (void *) tail;
> > +   tail->rta_len = (void *) NLMSG_TAIL(>n) - (void *) tail;
> > +
> > +   *argc_p = argc;
> > +   *argv_p = argv;
> >
> > -   if (rtnl_talk(, , NULL) < 0) {
> > +   iov.iov_base = >n;
> > +   iov.iov_len = req->n.nlmsg_len;
> > +
> > +   if (buf)
> > +   return 0;
> > +
> > +   if (rtnl_talk_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,
> > -   , );
> > +   , , buf);
> > } else if (matches(*argv, "change") == 0 ||
> >   matches(*argv, "replace") == 0) {
> > ret =

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

[David Ahern]

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 
> ---
>  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;
> + charbuf[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.

>  {
> - 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;
> - charbuf[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();
> +
> + if (buf)
> + req = buf;
> + else
> + req = _req;
> +

And a memset is needed for the !buf path since action_req is not
initialized.

> + 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(>n);
>  
>   argc -= 1;
>   argv += 1;
> - if (parse_action(, , TCA_ACT_TAB, )) {
> + if (parse_action(, , TCA_ACT_TAB, >n)) {
>   fprintf(stderr, "Illegal \"action\"\n");
>   return -1;
>   }
> - tail->rta_len = (void *) NLMSG_TAIL() - (void *) tail;
> + tail->rta_len = (void *) NLMSG_TAIL(>n) - (void *) tail;
> +
> + *argc_p = argc;
> + *argv_p = argv;
>  
> - if (rtnl_talk(, , NULL) < 0) {
> + iov.iov_base = >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.

> +
> + if (buf)
> + return 0;
> +
> + if (rtnl_talk_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,
> - , );
> + , , buf);
>   } else if (matches(*argv, "change") == 0 ||
> matches(*argv, "replace") == 0) {
>   ret = tc_action_modify(RTM_NEWACTION,
>  NLM_F_CREATE | NLM_F_REPLACE,
> -, );
> +, , 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, 

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

[Marcelo Ricardo Leitner]

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 
> ---
>  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;
> + charbuf[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;
> - charbuf[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();
> +
> + if (buf)
> + req = buf;
> + else
> + req = _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(>n);
>  
>   argc -= 1;
>   argv += 1;
> - if (parse_action(, , TCA_ACT_TAB, )) {
> + if (parse_action(, , TCA_ACT_TAB, >n)) {
>   fprintf(stderr, "Illegal \"action\"\n");
>   return -1;
>   }
> - tail->rta_len = (void *) NLMSG_TAIL() - (void *) tail;
> + tail->rta_len = (void *) NLMSG_TAIL(>n) - (void *) tail;
> +
> + *argc_p = argc;
> + *argv_p = argv;
>  
> - if (rtnl_talk(, , NULL) < 0) {
> + iov.iov_base = >n;
> + iov.iov_len = req->n.nlmsg_len;
> +
> + if (buf)
> + return 0;
> +
> + if (rtnl_talk_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,
> - , );
> + , , buf);
>   } else if (matches(*argv, "change") == 0 ||
> matches(*argv, "replace") == 0) {
>   ret = tc_action_modify(RTM_NEWACTION,
>  NLM_F_CREATE | NLM_F_REPLACE,
> -, );
> +, , 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,