| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4b8acc65-1cfd-4f46-876f-9d279d587628@mellanox.com>
Date: Tue, 2 Jan 2018 22:17:48 +0800
From: Chris Mi <chrism@...lanox.com>
To: David Ahern <dsahern@...il.com>, netdev@...r.kernel.org
Cc: gerlitz.or@...il.com, stephen@...workplumber.org
Subject: Re: [patch iproute2 v3 3/4] tc: Add -bs option to batch mode
> On 12/25/17 2:46 AM, Chris Mi wrote:
>> Signed-off-by: Chris Mi <chrism@...lanox.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;
Powered by blists - more mailing lists