[Dnsmasq-discuss] [PATCH] Add number of forks for TCP to metrics and dump

[Simon Kelley]

Looks good. Patch applied.


Cheers,

Simon.

On 24/11/2023 11:13, Damian Sawicki via Dnsmasq-discuss wrote:
> Hello dnsmasq experts,
> 
> Following up on the recent addition of the flag --max-tcp-connections, I'd like to propose a patch with monitoring of the number of TCP connections. This way, a user can actually estimate their usage and adjust --max-tcp-connections accordingly to prevent overload.
> 
> The patch adds the relevant information to the metrics and to the output of dump_cache() (which is called when dnsmasq receives SIGUSR1). Hence, users not collecting metrics will still be able to troubleshoot with SIGUSR1. In addition to the current usage, dump_cache() contains the information on the highest usage since it was last called.
> 
> Please kindly let me know what you think.
> 
> Best regards,
> 
> Damian Sawicki
> 
> *
> 
> Add to the metrics and dump_cache() the info on the utilisation of
> the limit on concurrent TCP connections (the variable
> daemon->max_procs, customisable via  --max-tcp-connections).
> ---
>   man/dnsmasq.8 |  2 +-
>   src/cache.c   |  5 +++++
>   src/dnsmasq.c | 11 ++++++++++-
>   src/dnsmasq.h |  1 +
>   src/metrics.c |  1 +
>   src/metrics.h |  1 +
>   src/option.c  |  1 +
>   7 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/man/dnsmasq.8 b/man/dnsmasq.8
> index 6d37360..1b5ebda 100644
> --- a/man/dnsmasq.8
> +++ b/man/dnsmasq.8
> @@ -2303,7 +2303,7 @@ they expired in order to make room for new names and the total number
>   of names that have been inserted into the cache. The number of cache hits and
>   misses and the number of authoritative queries answered are also given. For each upstream
>   server it gives the number of queries sent, and the number which
> -resulted in an error. In
> +resulted in an error. It also gives information on the number of forks for TCP connections. In
>   .B --no-daemon
>   mode or when full logging is enabled (\fB--log-queries\fP), a complete dump of the
>   contents of the cache is made.
> diff --git a/src/cache.c b/src/cache.c
> index 5342ce2..b95e3c7 100644
> --- a/src/cache.c
> +++ b/src/cache.c
> @@ -1895,6 +1895,11 @@ void dump_cache(time_t now)
>   #endif
>   
>     blockdata_report();
> +  my_syslog(LOG_INFO, _("child processes for TCP requests: in use %zu, highest since last SIGUSR1 %zu, max %zu."),
> +      daemon->metrics[METRIC_TCP_CONNECTIONS],
> +      daemon->max_procs_used,
> +      daemon->max_procs);
> +  daemon->max_procs_used = daemon->metrics[METRIC_TCP_CONNECTIONS];
>   
>     /* sum counts from different records for same server */
>     for (serv = daemon->servers; serv; serv = serv->next)
> diff --git a/src/dnsmasq.c b/src/dnsmasq.c
> index 65ba334..39aaf19 100644
> --- a/src/dnsmasq.c
> +++ b/src/dnsmasq.c
> @@ -1534,7 +1534,11 @@ static void async_event(int pipe, time_t now)
>   	  else if (daemon->port != 0)
>   	    for (i = 0 ; i < daemon->max_procs; i++)
>   	      if (daemon->tcp_pids[i] == p)
> -		daemon->tcp_pids[i] = 0;
> +    {
> +		  daemon->tcp_pids[i] = 0;
> +      if (daemon->tcp_pipes[i] == -1)
> +        daemon->metrics[METRIC_TCP_CONNECTIONS]--;
> +    }
>   	break;
>   	
>   #if defined(HAVE_SCRIPT)	
> @@ -1844,6 +1848,8 @@ static void check_dns_listeners(time_t now)
>   	{
>   	  close(daemon->tcp_pipes[i]);
>   	  daemon->tcp_pipes[i] = -1;	
> +    if (daemon->tcp_pids[i] == 0)
> +      daemon->metrics[METRIC_TCP_CONNECTIONS]--;
>   	}
>   	
>     for (listener = daemon->listeners; listener; listener = listener->next)
> @@ -1972,6 +1978,9 @@ static void check_dns_listeners(time_t now)
>   		  /* i holds index of free slot */
>   		  daemon->tcp_pids[i] = p;
>   		  daemon->tcp_pipes[i] = pipefd[0];
> +      daemon->metrics[METRIC_TCP_CONNECTIONS]++;
> +      if (daemon->metrics[METRIC_TCP_CONNECTIONS] > daemon->max_procs_used)
> +        daemon->max_procs_used = daemon->metrics[METRIC_TCP_CONNECTIONS];
>   		}
>   	      close(confd);
>   
> diff --git a/src/dnsmasq.h b/src/dnsmasq.h
> index 67c083b..3d85b73 100644
> --- a/src/dnsmasq.h
> +++ b/src/dnsmasq.h
> @@ -1314,6 +1314,7 @@ extern struct daemon {
>     int dumpfd;
>   #endif
>     int max_procs;
> +  uint max_procs_used;
>   } *daemon;
>   
>   struct server_details {
> diff --git a/src/metrics.c b/src/metrics.c
> index 050ccb3..1a9a4d3 100644
> --- a/src/metrics.c
> +++ b/src/metrics.c
> @@ -39,6 +39,7 @@ const char * metric_names[] = {
>       "leases_pruned_4",
>       "leases_allocated_6",
>       "leases_pruned_6",
> +    "tcp_connections",
>   };
>   
>   const char* get_metric_name(int i) {
> diff --git a/src/metrics.h b/src/metrics.h
> index 22e9e48..e1cdd87 100644
> --- a/src/metrics.h
> +++ b/src/metrics.h
> @@ -38,6 +38,7 @@ enum {
>     METRIC_LEASES_PRUNED_4,
>     METRIC_LEASES_ALLOCATED_6,
>     METRIC_LEASES_PRUNED_6,
> +  METRIC_TCP_CONNECTIONS,
>     
>     __METRIC_MAX,
>   };
> diff --git a/src/option.c b/src/option.c
> index 9423582..3eeda18 100644
> --- a/src/option.c
> +++ b/src/option.c
> @@ -5855,6 +5855,7 @@ void read_opts(int argc, char **argv, char *compile_opts)
>     daemon->randport_limit = 1;
>     daemon->host_index = SRC_AH;
>     daemon->max_procs = MAX_PROCS;
> +  daemon->max_procs_used = 0;
>     
>     /* See comment above make_servers(). Optimises server-read code. */
>     mark_servers(0);