[PATCH libinput 1/2] util: introduce ratelimit helpers

Peter Hutterer peter.hutterer at who-t.net
Wed Nov 5 03:41:09 PST 2014


On Wed, Nov 05, 2014 at 12:30:32PM +0100, David Herrmann wrote:
> On Wed, Nov 5, 2014 at 6:11 AM, Peter Hutterer <peter.hutterer at who-t.net> wrote:
> > On Tue, Nov 04, 2014 at 09:35:37AM +0100, David Herrmann wrote:
> >> This adds "struct ratelimit" and "ratelimit_test()". It's a very simple
> >> rate-limit helper modeled after Linux' lib/ratelimit.c by Dave Young.
> >>
> >> This comes in handy to limit log-messages in possible busy loops etc..
> >>
> >> Signed-off-by: David Herrmann <dh.herrmann at gmail.com>
> >> ---
> >>  src/libinput-util.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>  src/libinput-util.h | 19 +++++++++++++++++++
> >>  test/misc.c         | 37 +++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 104 insertions(+)
> >>
> >> diff --git a/src/libinput-util.c b/src/libinput-util.c
> >> index eeb9786..19594e3 100644
> >> --- a/src/libinput-util.c
> >> +++ b/src/libinput-util.c
> >> @@ -65,3 +65,51 @@ list_empty(const struct list *list)
> >>  {
> >>       return list->next == list;
> >>  }
> >> +
> >> +/*
> >> + * Perform rate-limit test. Returns true if the rate-limited action is still
> >> + * allowed, false if it should be suppressed.
> >> + *
> >> + * The ratelimit object must be initialized via RATELIMIT_INIT().
> >> + *
> >> + * Modelled after Linux' lib/ratelimit.c by Dave Young
> >> + * <hidave.darkstar at gmail.com>, which is licensed GPLv2.
> >> + */
> >> +bool ratelimit_test(struct ratelimit *r)
> >
> > libinput style is: return type on a separate line
> 
> Fixed.
> 
> >> +{
> >> +     struct timespec ts;
> >> +     uint64_t utime;
> >> +
> >> +     if (r->interval <= 0 || r->burst <= 0)
> >> +             return true;
> >> +
> >> +     clock_gettime(CLOCK_MONOTONIC, &ts);
> >> +     utime = ts.tv_sec * 1000 * 1000 + ts.tv_nsec / 1000;
> >> +
> >> +     if (r->begin <= 0 || r->begin + r->interval < utime) {
> >> +             /* reset counter */
> >> +             r->begin = utime;
> >> +             r->num = 1;
> >> +             return true;
> >> +     } else if (r->num < r->burst) {
> >> +             /* continue burst */
> >> +             r->num++;
> >> +             return true;
> >> +     }
> >> +
> >> +     /* rate-limit with overflow check */
> >> +     if (r->num + 1 > r->num)
> >> +             ++r->num;
> >> +
> >> +     return false;
> >> +}
> >> +
> >> +/*
> >> + * Return true if the ratelimit counter just crossed the cutoff value. That is,
> >> + * this function returns true iff the last call to ratelimit_test() was the
> >
> > s/iff/if/
> 
> "Iff" is widely used for "if, and only if," [1]. Should I still change it?

interesting, that's the first time I've come across it. I just know it as
the military IFF, which has some entertainment value in this context but is
not quite what we need :)
I'd still prefer a normal 'if', has the same meaning here anyway.

> >> + * first call to exceed the burst value in this interval.
> >> + */
> >> +bool ratelimit_cutoff(struct ratelimit *r)
> >
> > bool on separate line please
> 
> Fixed.
> 
> >> +{
> >> +     return r->num == r->burst + 1;
> >> +}
> >
> >
> > I'm wondering: why have two separate functions here?
> >
> > how about an
> > enum ratelimit {
> >         RATELIMIT_PASS,
> >         RATELIMIT_THRESHOLD,
> >         RATELIMIT_EXCEEDED,
> > };
> >
> > then return that from ratelimit_test and then use the return value to
> > decide on the rest of the handling?
> > so the dispatch code would be:
> > if ((rc = ratelimit_test(...)) != RATELIMIT_EXCEEDED)) {
> >      log_info("SYN_DROPPED....");
> >      if (rc == RATELIMIT_THRESHOLD) {
> >         log_info("SYN_DROPPED flood");
> >      }
> > }
> >
> > or the same with a switch statement.
> 
> Sure, can do that.
> 
> >> diff --git a/src/libinput-util.h b/src/libinput-util.h
> >> index 51759e8..8ff8778 100644
> >> --- a/src/libinput-util.h
> >> +++ b/src/libinput-util.h
> >> @@ -25,6 +25,7 @@
> >>
> >>  #include <unistd.h>
> >>  #include <math.h>
> >> +#include <stdbool.h>
> >>  #include <string.h>
> >>  #include <time.h>
> >>
> >> @@ -280,4 +281,22 @@ matrix_to_farray6(const struct matrix *m, float out[6])
> >>       out[5] = m->val[1][2];
> >>  }
> >>
> >> +struct ratelimit {
> >> +     uint64_t interval;
> >> +     uint64_t begin;
> >> +     unsigned burst;
> >> +     unsigned num;
> >
> > unsigned int please
> 
> Fixed.
> 
> >> +} RateLimit;
> >
> > well, hello. what are you doing here? are you lost? :)
> 
> Weird.. gcc didn't warn me about this unused variable.. Fixed.
> 
> >> +
> >> +#define RATELIMIT_INIT(_interval, _burst)            \
> >> +     ((struct ratelimit){                            \
> >> +             .interval = (_interval),                \
> >> +             .begin = 0,                             \
> >> +             .burst = (_burst),                      \
> >> +             .num = 0,                               \
> >> +     })
> >
> > any reason you didn't make this into a function of
> > void ratelimit_init(struct ratelimit *rl)?
> > I don't see a lot of benefits having this as a macro given that it's only
> > called once anyway (per context).
> 
> If you want it as global variable, you cannot use a function to
> initialize it. I usually prefer literals to initialize objects as it
> can be optimized by the compiler. But I can provide ratelimit_init(),
> if you want. For the single use-case we have, both are fine.

yeah, understood. for the for a single per-device call I think a function is
the better option.

> >> +
> >> +bool ratelimit_test(struct ratelimit *r);
> >> +bool ratelimit_cutoff(struct ratelimit *r);
> >> +
> >>  #endif /* LIBINPUT_UTIL_H */
> >> diff --git a/test/misc.c b/test/misc.c
> >> index 1512180..70b3e57 100644
> >> --- a/test/misc.c
> >> +++ b/test/misc.c
> >> @@ -508,6 +508,42 @@ START_TEST(matrix_helpers)
> >>  }
> >>  END_TEST
> >>
> >> +START_TEST(ratelimit_helpers)
> >> +{
> >> +     /* 10 attempts every 10ms */
> >> +     struct ratelimit rl = RATELIMIT_INIT(10000, 10);
> >> +     unsigned int i, j;
> >> +
> >> +     for (j = 0; j < 100; ++j) {
> >> +             /* a burst of 10 attempts must succeed */
> >> +             for (i = 0; i < 10; ++i) {
> >> +                     ck_assert(ratelimit_test(&rl));
> >> +                     ck_assert(!ratelimit_cutoff(&rl));
> >> +             }
> >> +
> >> +             /* ..then further attempts must fail.. */
> >> +             ck_assert(!ratelimit_test(&rl));
> >> +             ck_assert(ratelimit_cutoff(&rl));
> >
> > you could just drop the above two lines and merge the comments into one.
> 
> Ugh? I cannot drop them, as the _cutoff() is only true here, not in
> the loop below.

right, I missed that one. sorry about the noise.

Cheers,
   Peter

> 
> >> +
> >> +             /* ..regardless of how often we try. */
> >> +             for (i = 0; i < 100; ++i) {
> >> +                     ck_assert(!ratelimit_test(&rl));
> >> +                     ck_assert(!ratelimit_cutoff(&rl));
> >> +             }
> >> +
> >> +             /* ..even after waiting 5ms */
> >> +             usleep(5000);
> >
> > msleep(5) for consistency with the rest of the code please.
> 
> Sure, fixed!
> 
> >> +             for (i = 0; i < 100; ++i) {
> >> +                     ck_assert(!ratelimit_test(&rl));
> >> +                     ck_assert(!ratelimit_cutoff(&rl));
> >> +             }
> >> +
> >> +             /* but after 10ms the counter is reset */
> >> +             usleep(6000); /* +1ms to account for time drifts */
> >
> > same here
> 
> Fixed.
> 
> >> +     }
> >> +}
> >> +END_TEST
> >> +
> >>  int main (int argc, char **argv) {
> >>       litest_add_no_device("events:conversion", event_conversion_device_notify);
> >>       litest_add_no_device("events:conversion", event_conversion_pointer);
> >> @@ -519,5 +555,6 @@ int main (int argc, char **argv) {
> >>       litest_add_no_device("config:status string", config_status_string);
> >>
> >>       litest_add_no_device("misc:matrix", matrix_helpers);
> >> +     litest_add_no_device("misc:ratelimit", ratelimit_helpers);
> >
> > while you're at it, please add an empty line before the return here.
> 
> Fixed.
> 
> Thanks
> David
> 
> [1] http://en.wikipedia.org/wiki/If_and_only_if
> 
> >>       return litest_run(argc, argv);
> >>  }
> >> --
> >> 2.1.3
> >>


More information about the wayland-devel mailing list