Hi Subash,
On Thu, Jun 23, 2016 at 06:44:06PM -0600, Subash Abhinov Kasiviswanathan wrote:
> ip[6]tables currently waits for 1 second for the xtables lock to be
> freed if the -w option is used. We have seen that the lock is held
> much less than that resulting in unnecessary delay when trying to
> acquire the lock. This problem is even severe in case of latency
> sensitive applications.
>
> Introduce a new option 'W' to specify the wait interval in decimal
> format [seconds.microseconds]. If this option is not specified,
> the command sleeps for 1 second by default.
I'm attaching a patch that applies on top of this.
That I can remember, you only needed to reduce the wait interval, not
to make it ever larger, so I'm simplifying this so -W only takes
microseconds.
I have only included some missing error check in case -W is specified
but -w is not.
I can collapse this patch to yours, unless you have any concern.
>From 4644aaabdffaafc488c80b7b81b6be45a297e9e9 Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
Date: Fri, 1 Jul 2016 12:11:59 +0200
Subject: [PATCH] iptables: revisit new -W option
* Simplify -W so it only takes the interval wait in microseconds.
* Bail out if -W is specific but -w is not.
Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
---
iptables/ip6tables.c | 35 +++++++++++++++++++-------------
iptables/iptables.c | 37 +++++++++++++++++++---------------
iptables/xshared.c | 57 +++++++++++++++++++++++++++++++++++++---------------
iptables/xshared.h | 4 +++-
iptables/xtables.c | 32 ++++++++++++++++-------------
5 files changed, 104 insertions(+), 61 deletions(-)
diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c
index 9629403..66df8e9 100644
--- a/iptables/ip6tables.c
+++ b/iptables/ip6tables.c
@@ -260,8 +260,8 @@ exit_printhelp(const struct xtables_rule_match *matches)
" network interface name ([+] for wildcard)\n"
" --table -t table table to manipulate (default: `filter')\n"
" --verbose -v verbose mode\n"
-" --wait -w [seconds] wait for the xtables lock\n"
-" --wait-interval -W [seconds.microseconds]\n"
+" --wait -w [seconds] maximum wait to acquire xtables lock before give up\n"
+" --wait-interval -W [usecs] wait time to try to acquire xtables lock\n"
" interval to wait for xtables lock\n"
" default is 1 second\n"
" --line-numbers print line numbers when listing\n"
@@ -1329,7 +1329,10 @@ int do_command6(int argc, char *argv[], char **table,
int verbose = 0;
int wait = 0;
- double wait_interval = 1;
+ struct timeval wait_interval = {
+ .tv_sec = 1,
+ };
+ bool wait_interval_set = false;
const char *chain = NULL;
const char *shostnetworkmask = NULL, *dhostnetworkmask = NULL;
const char *policy = NULL, *newname = NULL;
@@ -1365,7 +1368,7 @@ int do_command6(int argc, char *argv[], char **table,
opts = xt_params->orig_opts;
while ((cs.c = getopt_long(argc, argv,
- "-:A:C:D:R:I:L::S::M:F::Z::N:X::E:P:Vh::o:p:s:d:j:i:bvwW::nt:m:xc:g:46",
+ "-:A:C:D:R:I:L::S::M:F::Z::N:X::E:P:Vh::o:p:s:d:j:i:bvw::W::nt:m:xc:g:46",
opts, NULL)) != -1) {
switch (cs.c) {
/*
@@ -1627,15 +1630,15 @@ int do_command6(int argc, char *argv[], char **table,
"You cannot use `-W' from "
"ip6tables-restore");
}
- if (optarg) {
- if (sscanf(optarg, "%lf", &wait_interval) != 1)
- xtables_error(PARAMETER_PROBLEM,
- "wait interval not numeric");
- } else if (optind < argc && argv[optind][0] != '-'
- && argv[optind][0] != '!')
- if (sscanf(argv[optind++], "%lf", &wait_interval) != 1)
- xtables_error(PARAMETER_PROBLEM,
- "wait interval not numeric");
+ if (optarg)
+ parse_wait_interval(optarg, &wait_interval);
+ else if (optind < argc &&
+ argv[optind][0] != '-' &&
+ argv[optind][0] != '!')
+ parse_wait_interval(argv[optind++],
+ &wait_interval);
+
+ wait_interval_set = true;
break;
case 'm':
@@ -1742,6 +1745,10 @@ int do_command6(int argc, char *argv[], char **table,
cs.invert = FALSE;
}
+ if (!wait && wait_interval_set)
+ xtables_error(PARAMETER_PROBLEM,
+ "--wait-interval only makes sense with --wait\n");
+
if (strcmp(*table, "nat") == 0 &&
((policy != NULL && strcmp(policy, "DROP") == 0) ||
(cs.jumpto != NULL && strcmp(cs.jumpto, "DROP") == 0)))
@@ -1792,7 +1799,7 @@ int do_command6(int argc, char *argv[], char **table,
generic_opt_check(command, cs.options);
/* Attempt to acquire the xtables lock */
- if (!restore && !xtables_lock(wait, wait_interval)) {
+ if (!restore && !xtables_lock(wait, &wait_interval)) {
fprintf(stderr, "Another app is currently holding the xtables lock. ");
if (wait == 0)
fprintf(stderr, "Perhaps you want to use the -w option?\n");
diff --git a/iptables/iptables.c b/iptables/iptables.c
index 1b26d61..540d111 100644
--- a/iptables/iptables.c
+++ b/iptables/iptables.c
@@ -254,9 +254,8 @@ exit_printhelp(const struct xtables_rule_match *matches)
" network interface name ([+] for wildcard)\n"
" --table -t table table to manipulate (default: `filter')\n"
" --verbose -v verbose mode\n"
-" --wait -w [seconds] wait for the xtables lock\n"
-" --wait-interval -W [seconds.microseconds]\n"
-" interval to wait for xtables lock\n"
+" --wait -w [seconds] maximum wait to acquire xtables lock before give up\n"
+" --wait-interval -W [usecs] wait time to try to acquire xtables lock\n"
" default is 1 second\n"
" --line-numbers print line numbers when listing\n"
" --exact -x expand numbers (display exact values)\n"
@@ -1322,10 +1321,12 @@ int do_command4(int argc, char *argv[], char **table,
unsigned int nsaddrs = 0, ndaddrs = 0;
struct in_addr *saddrs = NULL, *smasks = NULL;
struct in_addr *daddrs = NULL, *dmasks = NULL;
-
+ struct timeval wait_interval = {
+ .tv_sec = 1,
+ };
+ bool wait_interval_set = false;
int verbose = 0;
int wait = 0;
- double wait_interval = 1;
const char *chain = NULL;
const char *shostnetworkmask = NULL, *dhostnetworkmask = NULL;
const char *policy = NULL, *newname = NULL;
@@ -1360,7 +1361,7 @@ int do_command4(int argc, char *argv[], char **table,
opterr = 0;
opts = xt_params->orig_opts;
while ((cs.c = getopt_long(argc, argv,
- "-:A:C:D:R:I:L::S::M:F::Z::N:X::E:P:Vh::o:p:s:d:j:i:fbvwW::nt:m:xc:g:46",
+ "-:A:C:D:R:I:L::S::M:F::Z::N:X::E:P:Vh::o:p:s:d:j:i:fbvw::W::nt:m:xc:g:46",
opts, NULL)) != -1) {
switch (cs.c) {
/*
@@ -1620,15 +1621,15 @@ int do_command4(int argc, char *argv[], char **table,
"You cannot use `-W' from "
"iptables-restore");
}
- if (optarg) {
- if (sscanf(optarg, "%lf", &wait_interval) != 1)
- xtables_error(PARAMETER_PROBLEM,
- "wait interval not numeric");
- } else if (optind < argc && argv[optind][0] != '-'
- && argv[optind][0] != '!')
- if (sscanf(argv[optind++], "%lf", &wait_interval) != 1)
- xtables_error(PARAMETER_PROBLEM,
- "wait interval not numeric");
+ if (optarg)
+ parse_wait_interval(optarg, &wait_interval);
+ else if (optind < argc &&
+ argv[optind][0] != '-' &&
+ argv[optind][0] != '!')
+ parse_wait_interval(argv[optind++],
+ &wait_interval);
+
+ wait_interval_set = true;
break;
case 'm':
@@ -1731,6 +1732,10 @@ int do_command4(int argc, char *argv[], char **table,
cs.invert = FALSE;
}
+ if (!wait && wait_interval_set)
+ xtables_error(PARAMETER_PROBLEM,
+ "--wait-interval only makes sense with --wait\n");
+
if (strcmp(*table, "nat") == 0 &&
((policy != NULL && strcmp(policy, "DROP") == 0) ||
(cs.jumpto != NULL && strcmp(cs.jumpto, "DROP") == 0)))
@@ -1781,7 +1786,7 @@ int do_command4(int argc, char *argv[], char **table,
generic_opt_check(command, cs.options);
/* Attempt to acquire the xtables lock */
- if (!restore && !xtables_lock(wait, wait_interval)) {
+ if (!restore && !xtables_lock(wait, &wait_interval)) {
fprintf(stderr, "Another app is currently holding the xtables lock. ");
if (wait == 0)
fprintf(stderr, "Perhaps you want to use the -w option?\n");
diff --git a/iptables/xshared.c b/iptables/xshared.c
index d810c98..cccb8ae 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -247,15 +247,13 @@ void xs_init_match(struct xtables_match *match)
match->init(match->m);
}
-bool xtables_lock(int wait, double wait_interval)
+bool xtables_lock(int wait, struct timeval *wait_interval)
{
+ struct timeval time_left, wait_time, waited_time;
int fd, i = 0;
- double wait_int, wait_fract;
- struct timeval total_time, wait_time, waited_time;
- wait_fract = modf(wait_interval, &wait_int);
- total_time.tv_sec = wait;
- total_time.tv_usec = 0;
+ time_left.tv_sec = wait;
+ time_left.tv_usec = 0;
waited_time.tv_sec = 0;
waited_time.tv_usec = 0;
@@ -266,16 +264,43 @@ bool xtables_lock(int wait, double wait_interval)
while (1) {
if (flock(fd, LOCK_EX | LOCK_NB) == 0)
return true;
- if (++i % 10 == 0)
- fprintf(stderr, "Another app is currently holding the xtables lock; "
- "waiting (%lds %ldus) for it to exit...\n", waited_time.tv_sec, waited_time.tv_usec);
- wait_time.tv_sec = wait_int;
- wait_time.tv_usec = wait_fract*BASE_MICROSECONDS;
- if (wait != -1)
- timersub(&total_time, &wait_time, &total_time);
- timeradd(&waited_time, &wait_time, &waited_time);
- if (!timerisset(&total_time))
- return false;
+ if (++i % 10 == 0) {
+ if (wait != -1)
+ fprintf(stderr, "Another app is currently holding the xtables lock; "
+ "still %lds %ldus time ahead to have a chance to grab the lock...\n",
+ time_left.tv_sec, time_left.tv_usec);
+ else
+ fprintf(stderr, "Another app is currently holding the xtables lock; "
+ "waiting for it to exit...\n");
+ }
+
+ wait_time = *wait_interval;
select(0, NULL, NULL, NULL, &wait_time);
+ if (wait == -1)
+ continue;
+
+ timeradd(&waited_time, wait_interval, &waited_time);
+ timersub(&time_left, wait_interval, &time_left);
+ if (!timerisset(&time_left))
+ return false;
+ }
+}
+
+void parse_wait_interval(const char *str, struct timeval *wait_interval)
+{
+ unsigned int usec;
+ int ret;
+
+ ret = sscanf(str, "%u", &usec);
+ if (ret == 1) {
+ if (usec > 999999)
+ xtables_error(PARAMETER_PROBLEM,
+ "too long usec wait %u > 999999 usec",
+ usec);
+
+ wait_interval->tv_sec = 0;
+ wait_interval->tv_usec = usec;
+ return;
}
+ xtables_error(PARAMETER_PROBLEM, "wait interval not numeric");
}
diff --git a/iptables/xshared.h b/iptables/xshared.h
index 964cd4c..6eb8eb8 100644
--- a/iptables/xshared.h
+++ b/iptables/xshared.h
@@ -85,7 +85,9 @@ extern struct xtables_match *load_proto(struct iptables_command_state *);
extern int subcmd_main(int, char **, const struct subcommand *);
extern void xs_init_target(struct xtables_target *);
extern void xs_init_match(struct xtables_match *);
-bool xtables_lock(int wait, double wait_interval);
+bool xtables_lock(int wait, struct timeval *wait_interval);
+
+void parse_wait_interval(const char *str, struct timeval *wait_interval);
extern const struct xtables_afinfo *afinfo;
diff --git a/iptables/xtables.c b/iptables/xtables.c
index 84f9efd..15a84a6 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -240,9 +240,8 @@ exit_printhelp(const struct xtables_rule_match *matches)
" network interface name ([+] for wildcard)\n"
" --table -t table table to manipulate (default: `filter')\n"
" --verbose -v verbose mode\n"
-" --wait -w [seconds] wait for the xtables lock\n"
-" --wait-interval -W [seconds.microseconds]\n"
-" interval to wait for xtables lock\n"
+" --wait -w [seconds] maximum wait to acquire xtables lock before give up\n"
+" --wait-interval -W [usecs] wait time to try to acquire xtables lock\n"
" default is 1 second\n"
" --line-numbers print line numbers when listing\n"
" --exact -x expand numbers (display exact values)\n"
@@ -690,9 +689,10 @@ void do_parse(struct nft_handle *h, int argc, char *argv[],
{
struct xtables_match *m;
struct xtables_rule_match *matchp;
+ bool wait_interval_set = false;
+ struct timeval wait_interval;
struct xtables_target *t;
int wait = 0;
- double wait_interval = 1;
memset(cs, 0, sizeof(*cs));
cs->jumpto = "";
@@ -722,7 +722,7 @@ void do_parse(struct nft_handle *h, int argc, char *argv[],
opts = xt_params->orig_opts;
while ((cs->c = getopt_long(argc, argv,
- "-:A:C:D:R:I:L::S::M:F::Z::N:X::E:P:Vh::o:p:s:d:j:i:fbvwW::nt:m:xc:g:46",
+ "-:A:C:D:R:I:L::S::M:F::Z::N:X::E:P:Vh::o:p:s:d:j:i:fbvw::W::nt:m:xc:g:46",
opts, NULL)) != -1) {
switch (cs->c) {
/*
@@ -1032,15 +1032,15 @@ void do_parse(struct nft_handle *h, int argc, char *argv[],
"You cannot use `-W' from "
"iptables-restore");
}
- if (optarg) {
- if (sscanf(optarg, "%lf", &wait_interval) != 1)
- xtables_error(PARAMETER_PROBLEM,
- "wait interval not numeric");
- } else if (optind < argc && argv[optind][0] != '-'
- && argv[optind][0] != '!')
- if (sscanf(argv[optind++], "%lf", &wait_interval) != 1)
- xtables_error(PARAMETER_PROBLEM,
- "wait interval not numeric");
+ if (optarg)
+ parse_wait_interval(optarg, &wait_interval);
+ else if (optind < argc &&
+ argv[optind][0] != '-' &&
+ argv[optind][0] != '!')
+ parse_wait_interval(argv[optind++],
+ &wait_interval);
+
+ wait_interval_set = true;
break;
case '0':
@@ -1125,6 +1125,10 @@ void do_parse(struct nft_handle *h, int argc, char *argv[],
"\nThe \"nat\" table is not intended for filtering, "
"the use of DROP is therefore inhibited.\n\n");
+ if (!wait && wait_interval_set)
+ xtables_error(PARAMETER_PROBLEM,
+ "--wait-interval only makes sense with --wait\n");
+
for (matchp = cs->matches; matchp; matchp = matchp->next)
xtables_option_mfcall(matchp->match);
if (cs->target != NULL)
--
2.1.4
