On Tue, Jun 26, 2018 at 7:24 PM, Dmitry Safonov <dima@xxxxxxxxxx> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
