Re: [PATCH] Bug 134: message rate limiting https://bugs.passt.top/show_bug.cgi?id=134

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4492 bytes --]

On Mon, Mar 23, 2026 at 02:33:07PM +0530, Anshu Kumari wrote:
> Hi Everyone,
> 
> Please review patch for Bug 134.
> 
> Description:- Inorder to rate limit the messages, each logging function uses per-call-site static variables, so each macro expansion tracks its own rate independently.  Allows up to LOG_RATELIMIT_BURST messages per window.  When a new window starts after suppression, a summary of suppressed messages is logged.
> 
> Signed-off-by: Anshu Kumari <anskuma@redhat.com>

Logic looks good to me, and more straightforward than I feared.  Nice!
Stefano had a bunch of boring procedural things to fix, and I have one
more, see below.

> ---
>  log.h | 41 +++++++++++++++++++++++++++++++++++++++++
>  tap.c | 19 ++++++-------------
>  2 files changed, 47 insertions(+), 13 deletions(-)
> 
> diff --git a/log.h b/log.h
> index 6ceb686..e989760 100644
> --- a/log.h
> +++ b/log.h
> @@ -48,6 +48,47 @@ void logmsg_perror(int pri, const char *format, ...)
>  		passt_exit(EXIT_FAILURE);				\
>  	} while (0)
>  
> +#define LOG_RATELIMIT_BURST	5  /* Max messages per window per call site */
> +#define LOG_RATELIMIT_INTERVAL	1  /* Default rate limit window in seconds */
> +
> +/**
> + * logmsg_ratelimit() - Rate-limited log message
> + * @fn:		Logging function
> + * @now:	current timestamp
> + * @intv:	Minimum interval in seconds between allowed messages
> + */
> +#define logmsg_ratelimit(fn, now, intv, ...)				\
> +	do {								\
> +		static time_t _rl_last;					\

Identifiers starting with _ are reserved for "the system" (vague, I
know).  The kernel can get away with this, because it controls every
part of the build.  We're a normal userspace binary, though, linked
against the C library, so we should avoid this.  Doing something to
avoiding shadowing regular variables in the macro is wise though.  For
this, I usually use _ as a suffix instead of a prefix (see,
e.g. flow_log()).

> +		static unsigned int _rl_printed;			\
> +		static unsigned int _rl_suppressed;			\
> +									\
> +		if ((now)->tv_sec - _rl_last > (intv)) {		\
> +			if (_rl_suppressed)				\
> +				fn("(suppressed %u similar messages)",	\
> +				   _rl_suppressed);			\
> +			_rl_last = (now)->tv_sec;			\
> +			_rl_printed = 0;				\
> +			_rl_suppressed = 0;				\
> +		}							\
> +									\
> +		if (_rl_printed < LOG_RATELIMIT_BURST) {		\
> +			fn(__VA_ARGS__);				\
> +			_rl_printed++;					\
> +		} else {						\
> +			_rl_suppressed++;				\
> +		}							\
> +	} while (0)
> +
> +#define err_ratelimit(now, intv, ...)					\
> +	logmsg_ratelimit(err, now, intv, __VA_ARGS__)
> +#define warn_ratelimit(now, intv, ...)					\
> +	logmsg_ratelimit(warn, now, intv, __VA_ARGS__)
> +#define info_ratelimit(now, intv, ...)					\
> +	logmsg_ratelimit(info, now, intv, __VA_ARGS__)
> +#define debug_ratelimit(now, intv, ...)					\
> +	logmsg_ratelimit(debug, now, intv, __VA_ARGS__)
> +
>  extern int log_file;
>  extern int log_trace;
>  extern bool log_conf_parsed;
> diff --git a/tap.c b/tap.c
> index 1049e02..0812a8a 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -686,17 +686,8 @@ static bool tap4_is_fragment(const struct iphdr *iph,
>  			     const struct timespec *now)
>  {
>  	if (ntohs(iph->frag_off) & ~IP_DF) {
> -		/* Ratelimit messages */
> -		static time_t last_message;
> -		static unsigned num_dropped;
> -
> -		num_dropped++;
> -		if (now->tv_sec - last_message > FRAGMENT_MSG_RATE) {
> -			warn("Can't process IPv4 fragments (%u dropped)",
> -			     num_dropped);
> -			last_message = now->tv_sec;
> -			num_dropped = 0;
> -		}
> +		warn_ratelimit(now, FRAGMENT_MSG_RATE,
> +			"Can't process IPv4 fragment");
>  		return true;
>  	}
>  	return false;
> @@ -1115,8 +1106,10 @@ void tap_add_packet(struct ctx *c, struct iov_tail *data,
>  		char bufmac[ETH_ADDRSTRLEN];
>  
>  		memcpy(c->guest_mac, eh->h_source, ETH_ALEN);
> -		debug("New guest MAC address observed: %s",
> -		      eth_ntop(c->guest_mac, bufmac, sizeof(bufmac)));
> +		info_ratelimit(now, LOG_RATELIMIT_INTERVAL,
> +			       "New guest MAC address observed: %s",
> +			       eth_ntop(c->guest_mac, bufmac,
> +					sizeof(bufmac)));
>  		proto_update_l2_buf(c->guest_mac);
>  	}
>  
> -- 
> 2.53.0
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]