From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=Lw9EKfx/; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTPS id 9BEB55A0271 for ; Fri, 19 Sep 2025 13:11:04 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1758280263; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=yk0XTeBDnVbHJOZQMtSpRoeW3Kn2Im/8WUlDi91LafA=; b=Lw9EKfx/riUPURF8aZ4TKfSskD2k88GrrbCBeamEaD4JM7RdYSdVLMwj112Zv9P+henQ28 wqWEnKpL9jS/aR4Uujsn77GKvDJ0t6gStaktpzRSJHOacQdYNNsDNJz0YCoflvu6PsdE4s PoVOHfrLzHNcBKU15BBwOZXk/dVGimY= Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-575-0ghuFfuMODCl3i22ZGSDfw-1; Fri, 19 Sep 2025 07:11:02 -0400 X-MC-Unique: 0ghuFfuMODCl3i22ZGSDfw-1 X-Mimecast-MFC-AGG-ID: 0ghuFfuMODCl3i22ZGSDfw_1758280261 Received: by mail-wr1-f69.google.com with SMTP id ffacd0b85a97d-3ece14b9231so1108131f8f.0 for ; Fri, 19 Sep 2025 04:11:01 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1758280260; x=1758885060; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=yk0XTeBDnVbHJOZQMtSpRoeW3Kn2Im/8WUlDi91LafA=; b=NYVWzcv1pd0jNdWhIAcVoSqiQzX9HZqiIn9k/jGimAku14ym295FVOnTZPkqI2X3TU NFLYZHJUYkPrZBx03l2Zaq5D4uNhb1/YAAhkiozPpdwjUlYuW3dJ51zUjgEAUqCbiUGs qc/IwoMQhdbse3kmK+xh3nNuzgsIIZyMk8UvvqQrgmNYpc0sO/c0eWiUbZWxnte4TCIR gVXDP0FvkvGKqSvq0/DFlctduXK5gsXKR99rcs7bJ2H3WrVjmXsbvLOnyeGNsOCBLShm RrjOp6IJiwsUPH3A4eEXb1pYOoq8J3T7HTKO4CcopFL9EJ5zXzpX3JSXrxEmc/6Fq9Yv 0evQ== X-Gm-Message-State: AOJu0Yyosn5/YVCNpudm8xSD9Y/nUl5iiLVyVLpoNh6847gG3QXU8yuX YEOtW9We/AGHqfIDyjNGaKukyUgaissrWFEwo3JXW5k2GJO4rAqDBIinyVWhj9sC0rNc5jKNdJu mvmhDjcLM9L4b5fpbORrutiaDCuL8A4RKn9H3bOUKC29St7bhGv+CvzU1myJh7CuFsUHWVkN9r0 PdH5zJMZeqGjKqbPFrHmOAt9QjrOcUx0xwHrJa X-Gm-Gg: ASbGncuH4nY5hs0VavVCUwDRR7RfxUIHHolZ3GQT/vR/VMU9Oybjp5uS7pIfOVIBwfl EKPGQcTsaVWLMO7vIsEKAn7e8bWf6vFb3oAU29WPBgd4GFC04it4lWrck5ZlVGe0qfg5i+Qv/ps CrRB/aiZbcIpifvnvG0NynR09YKPuBM8HBaUtDIwrISsTc+qmEBTRaZv3/Wwe89iofpPGniiIJr G22260YcOFjs2QNLwvUKwfTJSQOYURZSWBbekNkO0a3fYX3XIbZKoejnpuaW5XlQyo4phLlNVdS A8TRJYnOvRH0swG8YSBSGGRPvmAPsOILsBmprBOh0Sp6gPY+vE0= X-Received: by 2002:a05:6000:4210:b0:3e7:6197:9947 with SMTP id ffacd0b85a97d-3ee868a74bamr2407899f8f.53.1758280259993; Fri, 19 Sep 2025 04:10:59 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFZxLVP0DpLJRyUnvZfoE10vhmc00NQtsK5PaSyMfXjfmt2zTkgkgCRsy86QI6LysUmV7hctg== X-Received: by 2002:a05:6000:4210:b0:3e7:6197:9947 with SMTP id ffacd0b85a97d-3ee868a74bamr2407859f8f.53.1758280259324; Fri, 19 Sep 2025 04:10:59 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-3ee0fbc7188sm7742549f8f.37.2025.09.19.04.10.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 19 Sep 2025 04:10:58 -0700 (PDT) Date: Fri, 19 Sep 2025 13:10:52 +0200 From: Stefano Brivio To: Laurent Vivier Subject: Re: [PATCH] Add --stats option to display event statistics Message-ID: <20250919131052.5ff169de@elisabeth> In-Reply-To: <20250918105910.4134694-1-lvivier@redhat.com> References: <20250918105910.4134694-1-lvivier@redhat.com> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: oKRbA7xJ7jCiASm4-0oT7pxoTJ8Bj2vamcTYu_0RPww_1758280261 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: JSKHPB7TIWIM2BKWA7GI3IUWVZ2JSBOK X-Message-ID-Hash: JSKHPB7TIWIM2BKWA7GI3IUWVZ2JSBOK X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Thu, 18 Sep 2025 12:59:10 +0200 Laurent Vivier 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 > --- > 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