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

[Laurent Vivier <lvivier@redhat.com>]

On 3/23/26 10:03, Anshu Kumari wrote:
> Hi Everyone,

Hi Anshu,

> 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.

This should be a message describing the patch, not an email text.
This will be commited and part of the change log of passt.

If you want to introduce your patch with a message you can write a cover letter (you can 
put " Hi Everyone, Please review patch for Bug 134" in it)

Put the bug link in the message body with Link: tag

Link: https://bugs.passt.top/show_bug.cgi?id=134

Something like:

"log: Add rate-limiting macros for log messages

In order 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.

Link: https://bugs.passt.top/show_bug.cgi?id=134
Signed-off-by: Anshu Kumari <anskuma@redhat.com>
"

> 
> Signed-off-by: Anshu Kumari <anskuma@redhat.com>
> ---
>   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;					\
> +		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) {

I think you could remove FRAGMENT_MSG_RATE and use LOG_RATELIMIT_INTERVAL, or better 
remove the intv parameter and use LOG_RATELIMIT_INTERVAL internally.

> -			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,

You changed "debug" to "info". Is this intentional?

> +			       "New guest MAC address observed: %s",
> +			       eth_ntop(c->guest_mac, bufmac,
> +					sizeof(bufmac)));
>   		proto_update_l2_buf(c->guest_mac);
>   	}
>   

Thanks,
Laurent