Re: [PATCH v3] xtables: Add a smaller delay option when waiting for xtables lock

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi Subash,

Almost there, more comments on this round.

On Thu, Jun 02, 2016 at 06:46:34PM -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' with 10ms granularity (experimentally
> determined) by specifying the delay in [seconds.milliseconds].
> If milliseconds are not specified, the command sleeps for 1 second
> similar to option 'w'.
> 
> v1->v2: Change behavior to take millisecond sleep as an argument to
> -w as suggested by Pablo. Also maintain current behavior for -w to
> sleep for 1 second as mentioned by Liping.
> 
> v2->v3: Move the millisecond behavior to a new option as suggested
> by Pablo.
> 
> Cc: Liping Zhang <zlpnobody@gmail.com>
> Cc: Pablo Neira Ayuso <pablo@netfilter.org>
> Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
> ---
>  iptables/ip6tables.c | 28 +++++++++++++++++++++++++---
>  iptables/iptables.c  | 28 +++++++++++++++++++++++++---
>  iptables/xshared.c   | 27 ++++++++++++++++++++-------
>  iptables/xshared.h   |  2 +-
>  iptables/xtables.c   | 27 +++++++++++++++++++++++++--
>  5 files changed, 96 insertions(+), 16 deletions(-)
> 
> diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c
> index 2731209..c0f481f 100644
> --- a/iptables/ip6tables.c
> +++ b/iptables/ip6tables.c
> @@ -103,6 +103,7 @@ static struct option original_opts[] = {
>  	{.name = "out-interface", .has_arg = 1, .val = 'o'},
>  	{.name = "verbose",       .has_arg = 0, .val = 'v'},
>  	{.name = "wait",          .has_arg = 2, .val = 'w'},
> +	{.name = "wait-ms",       .has_arg = 2, .val = 'W'},

OK, so let's summarize what we have after this:

--wait tells us the maximum (global) wait time, default is to retry
every 1 second.

Your proposed --wait-ms allow us to change the default time to retry
(instead of the predefined 1 second wait).

If this description is accurate, then I think --wait-interval is a
better name for this, right?

It would be good to document this in the manpage.

>  	{.name = "exact",         .has_arg = 0, .val = 'x'},
>  	{.name = "version",       .has_arg = 0, .val = 'V'},
>  	{.name = "help",          .has_arg = 2, .val = 'h'},
> @@ -260,6 +261,9 @@ exit_printhelp(const struct xtables_rule_match *matches)
>  "  --table	-t table	table to manipulate (default: `filter')\n"
>  "  --verbose	-v		verbose mode\n"
>  "  --wait	-w [seconds]	wait for the xtables lock\n"
> +"  --wait-ms	-W [seconds.milliseconds]\n"
> +"				wait for the xtables lock\n"
> +"				10ms is the minimum millisecond resolution\n"
>  "  --line-numbers		print line numbers when listing\n"
>  "  --exact	-x		expand numbers (display exact values)\n"
>  /*"[!] --fragment	-f		match second or further fragments only\n"*/
[...]
> @@ -255,12 +259,21 @@ bool xtables_lock(int wait)
>  	while (1) {
>  		if (flock(fd, LOCK_EX | LOCK_NB) == 0)
>  			return true;
> -		else if (wait >= 0 && waited >= wait)
> +		else if (wait >= 0 && waited >= (int)wait && us_waited >= us_wait)
>  			return false;
> -		if (++i % 2 == 0)
> +		if ((++i % 2 == 0 && !us_wait) || (++i % 10 == 0))
>  			fprintf(stderr, "Another app is currently holding the xtables lock; "
> -				"waiting (%ds) for it to exit...\n", waited);
> -		waited++;
> -		sleep(1);
> +				"waiting (%ds %dms) for it to exit...\n", waited, us_waited/1000);
> +		if (us_wait) {
> +			us_waited += BASE_MICROSECOND_WAIT;
> +			usleep(BASE_MICROSECOND_WAIT);
> +			if (us_waited == 1000000) {
> +				waited++;
> +				us_waited = 0;
> +			}
> +		} else {
> +			waited++;
> +			sleep(1);
> +		}

I really think you can simplify this via:

        select(0, NULL, NULL, NULL, &tv);

where tv default is 1 second.

Then, use timersub() to subtract the time --wait-interval we already
waited.