Re: [PATCH v3] tap, tcp, udp: Use rate-limited logging

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

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

On Fri, Apr 10, 2026 at 04:07:37PM +0530, Anshu Kumari wrote:
> Now that rate-limited logging macros are available, promote several
> debug messages to higher severity levels.  These messages were
> previously kept at debug to prevent guests from flooding host
> logs, but with rate limiting they can safely be made visible in
> normal operation.
> 
> In tap.c, refactor tap4_is_fragment() to use warn_ratelimit() instead
> of its ad-hoc rate limiting, and promote the guest MAC address change
> message to info level.
> 
> In tcp.c, promote the invalid TCP SYN endpoint message to warn level.
> 
> In udp.c, promote dropped datagram messages to warn level, and
> rate-limit the unrecoverable socket error message.
> 
> In udp_flow.c, promote flow allocation failures to err_ratelimit.
> 
> Link: https://bugs.passt.top/show_bug.cgi?id=134
> Signed-off-by: Anshu Kumari <anskuma@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  v3:
>    - Promote flow allocation failures to err_ratelimit from
>      warn_ratelimit.

Nit: doesn't affect the validity of the patch itself, but I'd consider
"promotion" in this context as going from a less severe to more severe
error level.  So going from 'err' to 'warn' is a demotion, not a
promotion.

>    - Fix remaining alignment issues.
>    - Cc Volker
> 
> ---
>  tap.c      | 16 +++++-----------
>  tcp.c      |  6 +++---
>  udp.c      | 12 ++++++------
>  udp_flow.c | 13 +++++++------
>  4 files changed, 21 insertions(+), 26 deletions(-)
> 
> diff --git a/tap.c b/tap.c
> index 1049e02..7d06189 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -99,7 +99,6 @@ static PACKET_POOL_NOINIT(pool_tap4, TAP_MSGS_IP4);
>  static PACKET_POOL_NOINIT(pool_tap6, TAP_MSGS_IP6);
>  
>  #define TAP_SEQS		128 /* Different L4 tuples in one batch */
> -#define FRAGMENT_MSG_RATE	10  /* # seconds between fragment warnings */
>  
>  /**
>   * tap_l2_max_len() - Maximum frame size (including L2 header) for current mode
> @@ -686,17 +685,11 @@ 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, "Can't process IPv4 fragments (%u dropped)",
> +			       num_dropped);
>  		return true;
>  	}
>  	return false;
> @@ -1115,8 +1108,9 @@ 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, "New guest MAC address observed: %s",
> +			       eth_ntop(c->guest_mac, bufmac,
> +					sizeof(bufmac)));
>  		proto_update_l2_buf(c->guest_mac);
>  	}
>  
> diff --git a/tcp.c b/tcp.c
> index 8ea9be8..071bcae 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -1688,9 +1688,9 @@ static void tcp_conn_from_tap(const struct ctx *c, sa_family_t af,
>  	    !inany_is_unicast(&ini->oaddr) || ini->oport == 0) {
>  		char sstr[INANY_ADDRSTRLEN], dstr[INANY_ADDRSTRLEN];
>  
> -		debug("Invalid endpoint in TCP SYN: %s:%hu -> %s:%hu",
> -		      inany_ntop(&ini->eaddr, sstr, sizeof(sstr)), ini->eport,
> -		      inany_ntop(&ini->oaddr, dstr, sizeof(dstr)), ini->oport);
> +		warn_ratelimit(now, "Invalid endpoint in TCP SYN: %s:%hu -> %s:%hu",
> +			       inany_ntop(&ini->eaddr, sstr, sizeof(sstr)), ini->eport,
> +			       inany_ntop(&ini->oaddr, dstr, sizeof(dstr)), ini->oport);
>  		goto cancel;
>  	}
>  
> diff --git a/udp.c b/udp.c
> index 1fc5a42..52260b9 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -871,7 +871,7 @@ void udp_sock_fwd(const struct ctx *c, int s, int rule_hint,
>  			/* Clear errors & carry on */
>  			if (udp_sock_errs(c, s, FLOW_SIDX_NONE,
>  					  frompif, port) < 0) {
> -				err(
> +				err_ratelimit(now,
>  "UDP: Unrecoverable error on listening socket: (%s port %hu)",
>  				    pif_name(frompif), port);
>  				/* FIXME: what now?  close/re-open socket? */
> @@ -898,7 +898,7 @@ void udp_sock_fwd(const struct ctx *c, int s, int rule_hint,
>  				 pif_name(frompif), pif_name(topif));
>  			discard = true;
>  		} else {
> -			debug("Discarding datagram without flow");
> +			warn_ratelimit(now, "Discarding datagram without flow");
>  			discard = true;
>  		}
>  
> @@ -1042,10 +1042,10 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif,
>  	if (!(uflow = udp_at_sidx(tosidx))) {
>  		char sstr[INET6_ADDRSTRLEN], dstr[INET6_ADDRSTRLEN];
>  
> -		debug("Dropping datagram with no flow %s %s:%hu -> %s:%hu",
> -		      pif_name(pif),
> -		      inet_ntop(af, saddr, sstr, sizeof(sstr)), src,
> -		      inet_ntop(af, daddr, dstr, sizeof(dstr)), dst);
> +		warn_ratelimit(now, "Dropping datagram with no flow %s %s:%hu -> %s:%hu",
> +			       pif_name(pif),
> +			       inet_ntop(af, saddr, sstr, sizeof(sstr)), src,
> +			       inet_ntop(af, daddr, dstr, sizeof(dstr)), dst);
>  		return 1;
>  	}
>  
> diff --git a/udp_flow.c b/udp_flow.c
> index 7e2453e..35417bc 100644
> --- a/udp_flow.c
> +++ b/udp_flow.c
> @@ -235,8 +235,9 @@ flow_sidx_t udp_flow_from_sock(const struct ctx *c, uint8_t pif,
>  	if (!(flow = flow_alloc())) {
>  		char sastr[SOCKADDR_STRLEN];
>  
> -		debug("Couldn't allocate flow for UDP datagram from %s %s",
> -		      pif_name(pif), sockaddr_ntop(s_in, sastr, sizeof(sastr)));
> +		err_ratelimit(now, "Couldn't allocate flow for UDP datagram from %s %s",
> +			      pif_name(pif),
> +			      sockaddr_ntop(s_in, sastr, sizeof(sastr)));
>  		return FLOW_SIDX_NONE;
>  	}
>  
> @@ -292,10 +293,10 @@ flow_sidx_t udp_flow_from_tap(const struct ctx *c,
>  	if (!(flow = flow_alloc())) {
>  		char sstr[INET6_ADDRSTRLEN], dstr[INET6_ADDRSTRLEN];
>  
> -		debug("Couldn't allocate flow for UDP datagram from %s %s:%hu -> %s:%hu",
> -		      pif_name(pif),
> -		      inet_ntop(af, saddr, sstr, sizeof(sstr)), srcport,
> -		      inet_ntop(af, daddr, dstr, sizeof(dstr)), dstport);
> +		err_ratelimit(now, "Couldn't allocate flow for UDP datagram from %s %s:%hu -> %s:%hu",
> +			      pif_name(pif),
> +			      inet_ntop(af, saddr, sstr, sizeof(sstr)), srcport,
> +			      inet_ntop(af, daddr, dstr, sizeof(dstr)), dstport);
>  		return FLOW_SIDX_NONE;
>  	}
>  
> -- 
> 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 --]