lkml.org 
[lkml]   [2018]   [Jun]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCHv2] lib/ratelimit: Lockless ratelimiting
On Tue, Jun 26, 2018 at 7:24 PM, Dmitry Safonov <dima@arista.com> wrote:
> Currently ratelimit_state is protected with spin_lock. If the .lock is
> taken at the moment of ___ratelimit() call, the message is suppressed to
> make ratelimiting robust.
>
> That results in the following issue issue:
> CPU0 CPU1
> ------------------ ------------------
> printk_ratelimit() printk_ratelimit()
> | |
> try_spin_lock() try_spin_lock()
> | |
> time_is_before_jiffies() return 0; // suppress
>
> So, concurrent call of ___ratelimit() results in silently suppressing
> one of the messages, regardless if the limit is reached or not.
> And rc->missed is not increased in such case so the issue is covered
> from user.
>
> Convert ratelimiting to use atomic counters and drop spin_lock.
>
> Note: That might be unexpected, but with the first interval of messages
> storm one can print up to burst*2 messages. So, it doesn't guarantee
> that in *any* interval amount of messages is lesser than burst.
> But, that differs lightly from previous behavior where one can start
> burst=5 interval and print 4 messages on the last milliseconds of
> interval + new 5 messages from new interval (totally 9 messages in
> lesser than interval value):
>
> msg0 msg1-msg4 msg0-msg4
> | | |
> | | |
> |--o---------------------o-|-----o--------------------|--> (t)
> <------->
> Lesser than burst

> #define RATELIMIT_STATE_INIT(name, interval_init, burst_init) { \
> - .lock = __RAW_SPIN_LOCK_UNLOCKED(name.lock), \

name is now redundant, isn't it?

> .interval = interval_init, \
> .burst = burst_init, \
> + .printed = ATOMIC_INIT(0), \
> + .missed = ATOMIC_INIT(0), \
> }
>
> #define RATELIMIT_STATE_INIT_DISABLED \
> @@ -42,9 +41,10 @@ static inline void ratelimit_state_init(struct ratelimit_state *rs,
> {
> memset(rs, 0, sizeof(*rs));
>
> - raw_spin_lock_init(&rs->lock);
> rs->interval = interval;
> rs->burst = burst;
> + atomic_set(&rs->printed, 0);
> + atomic_set(&rs->missed, 0);

Can it be

*rs = RATELIMIT_STATE_INIT(interval, burst);

?

(Yes, the '(struct ratelimit_state)' has to be added to macro to allow this)

> }


> static inline void ratelimit_state_exit(struct ratelimit_state *rs)
> {
> + int missed;
> +
> if (!(rs->flags & RATELIMIT_MSG_ON_RELEASE))
> return;
>
> - if (rs->missed) {
> + if ((missed = atomic_xchg(&rs->missed, 0)))

Perhaps

missed = ...
if (missed)

?

> pr_warn("%s: %d output lines suppressed due to ratelimiting\n",
> - current->comm, rs->missed);
> - rs->missed = 0;
> - }
> + current->comm, missed);
> }

> +static void ratelimit_end_interval(struct ratelimit_state *rs, const char *func)
> +{
> + rs->begin = jiffies;
> +
> + if (!(rs->flags & RATELIMIT_MSG_ON_RELEASE)) {

> + unsigned missed = (unsigned)atomic_xchg(&rs->missed, 0);
> +
> + if (missed)
> + pr_warn("%s: %u callbacks suppressed\n", func, missed);

Instead of casting, perhaps

int missed = ...

I think you already has a guard against going it below zero. Or I
missed something?

> + }
> +}

--
With Best Regards,
Andy Shevchenko

\
 
 \ /
  Last update: 2018-06-26 19:05    [W:0.129 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site