Re: [PATCH] Add --stats option to display event statistics

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 18 Sep 2025 12:59:10 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> Introduce a new --stats DELAY option that displays event statistics
> tables showing counts by epoll event type. Statistics are printed to
> stderr with a minimum delay between updates, and only when events occur.

I guess we'll need more and more of these quirks in the future, so
maybe *at some point* we should think of optionally exporting those to
a machine-readable format.

Given that we'll probably need a small binary tool to dynamically
configure port forwarding, and address/port translations, maybe we
could, that day, have this also handled by... passtctl?

But meanwhile:

> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  conf.c  |  6 ++++++
>  passt.1 |  6 ++++++
>  passt.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  passt.h |  1 +
>  4 files changed, 77 insertions(+)
> 
> diff --git a/conf.c b/conf.c
> index 02e903b5cf70..9aba27cef806 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -835,6 +835,8 @@ static void usage(const char *name, FILE *f, int status)
>  		"\n"
>  		"  -d, --debug		Be verbose\n"
>  		"      --trace		Be extra verbose, implies --debug\n"
> +		"  --stats DELAY  	Display events statistics\n"
> +		"    minimum DELAY seconds between updates\n"
>  		"  -q, --quiet		Don't print informational messages\n"
>  		"  -f, --foreground	Don't run in background\n"
>  		"    default: run in background\n"
> @@ -1480,6 +1482,7 @@ void conf(struct ctx *c, int argc, char **argv)
>  		{"repair-path",	required_argument,	NULL,		28 },
>  		{"migrate-exit", no_argument,		NULL,		29 },
>  		{"migrate-no-linger", no_argument,	NULL,		30 },
> +		{"stats", required_argument,		NULL,		31 },
>  		{ 0 },
>  	};
>  	const char *optstring = "+dqfel:hs:F:I:p:P:m:a:n:M:g:i:o:D:S:H:461t:u:T:U:";
> @@ -1708,6 +1711,9 @@ void conf(struct ctx *c, int argc, char **argv)
>  				die("--migrate-no-linger is for vhost-user mode only");
>  			c->migrate_no_linger = true;
>  
> +			break;
> +		case 31:
> +			c->stats = strtol(optarg, NULL, 0);

We should make sure that we print to standard error only if it's there,
and not to, say, a TCP socket that happened to be number 2. See also
commit b74801645c23 ("conf, passt: Don't try to log to stderr after we
close it") for how badly I broke this in the past.

We could probably go the same way as that commit for c->stats as well.

>  			break;
>  		case 'd':
>  			c->debug = 1;
> diff --git a/passt.1 b/passt.1
> index af5726a7f1c5..d5cde2c424d0 100644
> --- a/passt.1
> +++ b/passt.1
> @@ -84,6 +84,12 @@ Be verbose, don't log to the system logger.
>  .BR \-\-trace
>  Be extra verbose, show single packets. Implies \fB--debug\fR.
>  
> +.TP
> +.BR \-\-stats " " \fIDELAY\fR
> +Display events statistics with a 
> +minimum \fIDELAY\fR seconds between updates. If there is no event,
> +statistics are not displayed.

Unnecessary padding/wrapping I guess? This could be two lines.

> +
>  .TP
>  .BR \-q ", " \-\-quiet
>  Don't print informational messages.
> diff --git a/passt.c b/passt.c
> index a4ec115d1784..afd7f709e58a 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -83,6 +83,31 @@ char *epoll_type_str[] = {
>  static_assert(ARRAY_SIZE(epoll_type_str) == EPOLL_NUM_TYPES,
>  	      "epoll_type_str[] doesn't match enum epoll_type");
>  
> +char *epoll_type_tag[] = {

This could be const, and I'm a bit surprised none of the static
checkers suggests that, and that's the same for epoll_type_str[],
actually.

But I find those names a bit hard to decode, so, instead of having this
array, what about my proposal below? See comment to print_stats().

> +	[EPOLL_TYPE_TCP]		= "TCP",
> +	[EPOLL_TYPE_TCP_SPLICE]		= "TCPSPL",
> +	[EPOLL_TYPE_TCP_LISTEN]		= "TCPLSN",
> +	[EPOLL_TYPE_TCP_TIMER]		= "TCPTMR",
> +	[EPOLL_TYPE_UDP_LISTEN]		= "UDPLSN",
> +	[EPOLL_TYPE_UDP]		= "UDP",
> +	[EPOLL_TYPE_PING]		= "PING",
> +	[EPOLL_TYPE_NSQUIT_INOTIFY]	= "INOTF",
> +	[EPOLL_TYPE_NSQUIT_TIMER]	= "TIMER",
> +	[EPOLL_TYPE_TAP_PASTA]		= "PASTA",
> +	[EPOLL_TYPE_TAP_PASST]		= "PASST",
> +	[EPOLL_TYPE_TAP_LISTEN]		= "TAPLSN",
> +	[EPOLL_TYPE_VHOST_CMD]		= "VUCMD",
> +	[EPOLL_TYPE_VHOST_KICK]		= "VUKICK",
> +	[EPOLL_TYPE_REPAIR_LISTEN]	= "REPLSN",
> +	[EPOLL_TYPE_REPAIR]		= "REPAIR",
> +};
> +static_assert(ARRAY_SIZE(epoll_type_tag) == EPOLL_NUM_TYPES,
> +	      "epoll_type_tag[] doesn't match enum epoll_type");
> +

This should have the usual kerneldoc-style description.

> +struct passt_stats {
> +	unsigned long events[EPOLL_NUM_TYPES];
> +};
> +
>  /**
>   * post_handler() - Run periodic and deferred tasks for L4 protocol handlers
>   * @c:		Execution context
> @@ -174,6 +199,42 @@ static void exit_handler(int signal)
>  	_exit(EXIT_SUCCESS);
>  }
>  
> +/**
> + * print_stats() - Print event statistics table to stderr
> + * @c:		Execution context
> + * @stats:	Event counters
> + * @now:	Current timestamp
> + */
> +static void print_stats(const struct ctx *c, const struct passt_stats *stats,
> +			const struct timespec *now)
> +{
> +	int i;
> +	static struct timespec before;
> +	long long elapsed_ns;
> +
> +	if (!c->stats)
> +		return;
> +
> +	elapsed_ns = (now->tv_sec - before.tv_sec) * 1000000000LL +
> +		     (now->tv_nsec - before.tv_nsec);
> +
> +	if (elapsed_ns < c->stats * 1000000000LL)
> +		return;
> +
> +	before = *now;
> +
> +	/* table header */
> +	FPRINTF(stderr, "\t");
> +	for (i = 1; i < EPOLL_NUM_TYPES; i++)
> +		FPRINTF(stderr, "%6s\t", epoll_type_tag[i]);

Instead of being forced to mangle header fields to fit, what about
having a multi-line header that's only printed every now and then, say,
every slightly less than the canonical 22 rows? That is (quickly
tested):

---
	...
	static int lines_printed;
	long long elapsed_ns;

	if (!c->stats)
		return;

	elapsed_ns = (now->tv_sec - before.tv_sec) * 1E9 +
		     (now->tv_nsec - before.tv_nsec);

	if (elapsed_ns < c->stats * 1E9)
		return;

	before = *now;

	if (!(lines_printed % 20)) {
		/* Table header */
		for (i = 1; i < EPOLL_NUM_TYPES; i++) {
			int j;

			for (j = 0; j < i * (6 + 1); j++) {
				if (j && !(j % (6 + 1)))
					FPRINTF(stderr, "|");
				else
					FPRINTF(stderr, " ");
			}
			FPRINTF(stderr, "%s\n", epoll_type_str[i]);
		}
	}

	FPRINTF(stderr, " ");
	for (i = 1; i < EPOLL_NUM_TYPES; i++)
		FPRINTF(stderr, " %6lu", stats->events[i]);
	FPRINTF(stderr, "\n");
	lines_printed++;
	...
---

?

> +	FPRINTF(stderr, "\n");
> +	/* main loop */
> +	FPRINTF(stderr, "MAIN\t");
> +	for (i = 1;  i < EPOLL_NUM_TYPES; i++)
> +		FPRINTF(stderr, "%6lu\t", stats->events[i]);
> +	FPRINTF(stderr, "\n");
> +}
> +
>  /**
>   * main() - Entry point and main loop
>   * @argc:	Argument count
> @@ -196,6 +257,7 @@ int main(int argc, char **argv)
>  	struct rlimit limit;
>  	struct timespec now;
>  	struct sigaction sa;
> +	struct passt_stats stats = { 0 };

This should go a bit above.

>  
>  	if (clock_gettime(CLOCK_MONOTONIC, &log_start))
>  		die_perror("Failed to get CLOCK_MONOTONIC time");
> @@ -362,6 +424,8 @@ loop:
>  			/* Can't happen */
>  			ASSERT(0);
>  		}
> +		stats.events[ref.type]++;
> +		print_stats(&c, &stats, &now);
>  	}
>  
>  	post_handler(&c, &now);
> diff --git a/passt.h b/passt.h
> index 3ffc19fdae05..283e7c8a832c 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -248,6 +248,7 @@ struct ctx {
>  	enum passt_modes mode;
>  	int debug;
>  	int trace;
> +	int stats;

And this should be documented in the struct comment.

>  	int quiet;
>  	int foreground;
>  	int nofile;

The rest looks good and quite useful to me (I guess it's much more
useful to you, of course ;)).

-- 
Stefano