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

[Anshu Kumari <anskuma@redhat.com>]

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

> +             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);
>       }
>

Yes, this log-level is changed to info based on the discusion attached with
the ticket
https://archives.passt.top/passt-dev/20250912082344.01888466@elisabeth/

On Mon, Mar 23, 2026 at 5:48 PM Laurent Vivier <lvivier@redhat.com> wrote:

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

[-- Attachment #2: Type: text/html, Size: 9705 bytes --]