Re: [Intel-gfx] [PATCHv3] lib/ratelimit: Lockless ratelimiting
Hi Petr, thanks for review,
On Wed, 2018-08-15 at 17:10 +0200, Petr Mladek wrote:
> On Tue 2018-07-03 23:56:28, Dmitry Safonov 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):
>
> I am still confused by this paragraph. Does this patch change the
> behavior? What is the original and what is the new one, please?
Yeah, I could be a bit cleaner about the change.
Originally more than `burst` messages could be printed if the previous
period hasn't ended:
>
> >msg0 msg1-msg4 msg0-msg4
> > | | |
> > | | |
> > |--o-o-|-o|--> (t)
> > <--->
> >Lesser than burst
But now, because I check:
+ if (atomic_add_unless(>printed, 1, rs->burst))
+ return 1;
*before* checking the end of interval, the maximum number of messages
in the peak will be the same, burst*2 (burst*2 - 1 in original).
Why do I check it before the end of interval?
I thought it would be a nice optimization, making atomic_add_unless()
the only called function on the fast-path (when we haven't hit the
limit yet). If you want, I can move it into a separate patch..
> >
> > Dropped dev/random patch since v1 version:
> > lkml.kernel.org/r/<20180510125211.12583-1-d...@arista.com>
> >
> > Dropped `name' in as it's unused in RATELIMIT_STATE_INIT()
> >
> > diff --git a/lib/ratelimit.c b/lib/ratelimit.c
> > index d01f47135239..d9b749d40108 100644
> > --- a/lib/ratelimit.c
> > +++ b/lib/ratelimit.c
> > @@ -13,6 +13,18 @@
> > #include
> > #include
> >
> > +static void ratelimit_end_interval(struct ratelimit_state *rs,
> > const char *func)
> > +{
> > + rs->begin = jiffies;
> > +
> > + if (!(rs->flags & RATELIMIT_MSG_ON_RELEASE)) {
> > + unsigned int missed = atomic_xchg(>missed, 0);
> > +
> > + if (missed)
> > + pr_warn("%s: %u callbacks suppressed\n",
> > func, missed);
> > + }
> > +}
> > +
> > /*
> > * __ratelimit - rate limiting
> > * @rs: ratelimit_state data
> > @@ -27,45 +39,30 @@
> > */
> > int ___ratelimit(struct ratelimit_state *rs, const char *func)
> > {
> > - unsigned long flags;
> > - int ret;
> > -
> > if (!rs->interval)
> > return 1;
> >
> > - /*
> > -* If we contend on this state's lock then almost
> > -* by definition we are too busy to print a message,
> > -* in addition to the one that will be printed by
> > -* the entity that is holding the lock already:
> > -*/
> > - if (!raw_spin_trylock_irqsave(>lock, flags))
> > + if (unlikely(!rs->burst)) {
> > + atomic_add_unless(>missed, 1, -1);
> > + if (time_is_before_jiffies(rs->begin + rs-
> > >interval))
> > + ratelimit_end_interval(rs, func);
>
> This is racy. time_is_before_jiffies() might be valid on two
> CPUs in parallel. They would both call ratelimit_end_interval().
> This is no
Re: [Intel-gfx] [PATCHv3] lib/ratelimit: Lockless ratelimiting
Hi Steven,
Thanks for your reply,
On Wed, 2018-08-01 at 21:48 -0400, Steven Rostedt wrote:
> I'm just catching up from my vacation. What about making rs->missed
> into an atomic, and have:
>
> if (!raw_spin_trylock_irqsave(>lock, flags)) {
> atomic_inc(>missed);
> return 0;
> }
>
> ?
Uhm. Do you mean as a preparation patch to split this on two patches?
Because it will not solve the issue where one CPU has taken rs->lock,
and is updating rs->printed, checking burst and whatnot; while the
second CPU will loose the message which was even *under* burst limit.
I.e.: there are enough of printk_ratelimit() users in tree and a
message from one can be suppressed, while shouldn't.
> You would also need to do:
>
> if (time_is_before_jiffies(rs->begin + rs->interval)) {
> int missed = atomic_xchg(>missed, 0);
> if (missed) {
>
> So that you don't have a race between checking rs->missed and setting
> it
> to zero.
--
Thanks,
Dmitry
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCHv3] lib/ratelimit: Lockless ratelimiting
On Fri, 2018-07-20 at 17:09 +0200, Petr Mladek wrote: > > > On Tue, 2018-07-03 at 23:56 +0100, Dmitry Safonov 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 > > > > > > I am a bit confused. Does this patch fix two problems? > >+ Lost rc->missed update when try_spin_lock() fails >+ printing up to burst*2 messages in a give interval What I tried to solve here (maybe I could better point it in the commit message): is the situation where ratelimit::burst haven't been hit yet and there are calls for __ratelimit() from different CPUs. So, neither of the messages should be suppressed, but as the spinlock is taken already - one of them is dropped and even without updating missed counter. > It would make the review much easier if you split it into more > patches and fix the problems separately. Hmm, not really sure: the patch changes spinlock to atomics and I'm not sure it make much sense to add atomics before removing the spinlock.. > Otherwise, it looks promissing... > > Best Regards, > Petr > > PS: I have vacation the following two weeks. Anyway, please, CC me > if you send any new version. Surely, thanks for your attention to the patch (and time). -- Thanks, Dmitry ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCHv3] lib/ratelimit: Lockless ratelimiting
I would be glad if someone helps/bothers to review the change :C
Thanks,
Dmitry
On Tue, 2018-07-03 at 23:56 +0100, Dmitry Safonov 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
>
> Dropped dev/random patch since v1 version:
> lkml.kernel.org/r/<20180510125211.12583-1-d...@arista.com>
>
> Dropped `name' in as it's unused in RATELIMIT_STATE_INIT()
>
> Cc: Andy Shevchenko
> Cc: Arnd Bergmann
> Cc: David Airlie
> Cc: Greg Kroah-Hartman
> Cc: Jani Nikula
> Cc: Joonas Lahtinen
> Cc: Rodrigo Vivi
> Cc: "Theodore Ts'o"
> Cc: Thomas Gleixner
> Cc: intel-gfx@lists.freedesktop.org
> Cc: dri-de...@lists.freedesktop.org
> Signed-off-by: Dmitry Safonov
> ---
> drivers/char/random.c| 28 ---
> drivers/gpu/drm/i915/i915_perf.c | 8 --
> fs/btrfs/super.c | 16 +--
> fs/xfs/scrub/scrub.c | 2 +-
> include/linux/ratelimit.h| 31 -
> kernel/user.c| 2 +-
> lib/ratelimit.c | 59 +++---
> --
> 7 files changed, 73 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index cd888d4ee605..0be31b3eadab 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -439,10 +439,8 @@ static void _crng_backtrack_protect(struct
> crng_state *crng,
> static void process_random_ready_list(void);
> static void _get_random_bytes(void *buf, int nbytes);
>
> -static struct ratelimit_state unseeded_warning =
> - RATELIMIT_STATE_INIT("warn_unseeded_randomness", HZ, 3);
> -static struct ratelimit_state urandom_warning =
> - RATELIMIT_STATE_INIT("warn_urandom_randomness", HZ, 3);
> +static struct ratelimit_state unseeded_warning =
> RATELIMIT_STATE_INIT(HZ, 3);
> +static struct ratelimit_state urandom_warning =
> RATELIMIT_STATE_INIT(HZ, 3);
>
> static int ratelimit_disable __read_mostly;
>
> @@ -937,24 +935,22 @@ static void crng_reseed(struct crng_state
> *crng, struct entropy_store *r)
> crng->init_time = jiffies;
> spin_unlock_irqrestore(>lock, flags);
> if (crng == _crng && crng_init < 2) {
> + unsigned int unseeded_miss, urandom_miss;
> +
> invalidate_batched_entropy();
> numa_crng_init();
> crng_init = 2;
> process_random_ready_list();
> wake_up_interruptible(_init_wait);
> pr_notice("random: crng init done\n");
> - if (unseeded_warning.missed) {
> - pr_notice("random: %d get_random_xx
> warning(s) missed "
> - "due to ratelimiting\n",
> - unseeded_warning.missed);
> - unseeded_warning.missed = 0;
> - }
> - if (urandom_warning.missed) {
> - pr_notice("random: %d urandom warning(s)
> missed "
> - "due to ratelimiting\n",
> -
[Intel-gfx] [PATCHv3] lib/ratelimit: Lockless ratelimiting
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
Dropped dev/random patch since v1 version:
lkml.kernel.org/r/<20180510125211.12583-1-d...@arista.com>
Dropped `name' in as it's unused in RATELIMIT_STATE_INIT()
Cc: Andy Shevchenko
Cc: Arnd Bergmann
Cc: David Airlie
Cc: Greg Kroah-Hartman
Cc: Jani Nikula
Cc: Joonas Lahtinen
Cc: Rodrigo Vivi
Cc: "Theodore Ts'o"
Cc: Thomas Gleixner
Cc: intel-gfx@lists.freedesktop.org
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Dmitry Safonov
---
drivers/char/random.c| 28 ---
drivers/gpu/drm/i915/i915_perf.c | 8 --
fs/btrfs/super.c | 16 +--
fs/xfs/scrub/scrub.c | 2 +-
include/linux/ratelimit.h| 31 -
kernel/user.c| 2 +-
lib/ratelimit.c | 59 +++-
7 files changed, 73 insertions(+), 73 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index cd888d4ee605..0be31b3eadab 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -439,10 +439,8 @@ static void _crng_backtrack_protect(struct crng_state
*crng,
static void process_random_ready_list(void);
static void _get_random_bytes(void *buf, int nbytes);
-static struct ratelimit_state unseeded_warning =
- RATELIMIT_STATE_INIT("warn_unseeded_randomness", HZ, 3);
-static struct ratelimit_state urandom_warning =
- RATELIMIT_STATE_INIT("warn_urandom_randomness", HZ, 3);
+static struct ratelimit_state unseeded_warning = RATELIMIT_STATE_INIT(HZ, 3);
+static struct ratelimit_state urandom_warning = RATELIMIT_STATE_INIT(HZ, 3);
static int ratelimit_disable __read_mostly;
@@ -937,24 +935,22 @@ static void crng_reseed(struct crng_state *crng, struct
entropy_store *r)
crng->init_time = jiffies;
spin_unlock_irqrestore(>lock, flags);
if (crng == _crng && crng_init < 2) {
+ unsigned int unseeded_miss, urandom_miss;
+
invalidate_batched_entropy();
numa_crng_init();
crng_init = 2;
process_random_ready_list();
wake_up_interruptible(_init_wait);
pr_notice("random: crng init done\n");
- if (unseeded_warning.missed) {
- pr_notice("random: %d get_random_xx warning(s) missed "
- "due to ratelimiting\n",
- unseeded_warning.missed);
- unseeded_warning.missed = 0;
- }
- if (urandom_warning.missed) {
- pr_notice("random: %d urandom warning(s) missed "
- "due to ratelimiting\n",
- urandom_warning.missed);
- urandom_warning.missed = 0;
- }
+ unseeded_miss = atomic_xchg(_warning.missed, 0);
+ if (unseeded_miss)
+ pr_notice("random: %u get_random_xx warning(s) missed "
+ "due to ratelimiting\n", unseeded_miss);
+ urandom_miss = atomic_xchg(_warning.missed, 0);
+ if (urandom_miss)
+ pr_notice("random: %u urandom warning(s) missed "
+ "due to ratelimiting\n", urandom_miss);
Re: [Intel-gfx] [PATCHv2] lib/ratelimit: Lockless ratelimiting
On Tue, 2018-06-26 at 21:41 +0300, Andy Shevchenko wrote:
> > > > @@ -42,9 +41,10 @@ static inline void
> > > > ratelimit_state_init(struct
> > > > ratelimit_state *rs,
> > > > {
> > > > memset(rs, 0, sizeof(*rs));
> > > >
> > > > - raw_spin_lock_init(>lock);
> > > > rs->interval= interval;
> > > > rs->burst = burst;
> > > > + atomic_set(>printed, 0);
> > > > + atomic_set(>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)
> >
> > Sure.
>
> This part, by the way, potentially can be split into preparatory
> patch. Please, double check if it possible to do this way.
Hmm, I tried this way:
:#define RATELIMIT_STATE_INIT(name, interval_init, burst_init) ({ \
: struct ratelimit_state name = { \
: .lock = __RAW_SPIN_LOCK_UNLOCKED(name.lock), \
: .interval = interval_init, \
: .burst = burst_init, \
: };\
: name; \
: })
but the expression becomes non-constant, so it fails to compile in
definitions of globals.
I think I'll change it to
struct ratelimit_state tmp = RATELIMIT_STATE_INIT(...);
*rs = tmp;
Not perfect, but we did memset() and set elements after, so it's kinda
the same.
--
Thanks,
Dmitry
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCHv2] lib/ratelimit: Lockless ratelimiting
Hi Andy, thanks for the review,
On Tue, 2018-06-26 at 20:04 +0300, Andy Shevchenko wrote
[..]
> > #define RATELIMIT_STATE_INIT(name, interval_init, burst_init)
> > {\
> > - .lock =
> > __RAW_SPIN_LOCK_UNLOCKED(name.lock), \
>
> name is now redundant, isn't it?
It is. Worth to split on the second patch or keep callers changes in
this patch?
> > @@ -42,9 +41,10 @@ static inline void ratelimit_state_init(struct
> > ratelimit_state *rs,
> > {
> > memset(rs, 0, sizeof(*rs));
> >
> > - raw_spin_lock_init(>lock);
> > rs->interval= interval;
> > rs->burst = burst;
> > + atomic_set(>printed, 0);
> > + atomic_set(>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)
Sure.
> > 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(>missed, 0)))
>
> Perhaps
>
> missed = ...
> if (missed)
>
> ?
Ok, will change - checkpatch has warned me, but I thought it's just a
preference than a rule.
>
> > 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(
> > >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?
No, I do:
atomic_add_unless(>missed, 1, -1);
So, it's guard against overflow, but not against negative.
That's why I do print it as unsigned.
--
Thanks,
Dmitry
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCHv2] lib/ratelimit: Lockless ratelimiting
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
Dropped dev/random patch since v1 version:
lkml.kernel.org/r/<20180510125211.12583-1-d...@arista.com>
Cc: Arnd Bergmann
Cc: David Airlie
Cc: Dmitry Safonov <0x7f454...@gmail.com>
Cc: Greg Kroah-Hartman
Cc: Jani Nikula
Cc: Joonas Lahtinen
Cc: Rodrigo Vivi
Cc: "Theodore Ts'o"
Cc: Thomas Gleixner
Cc: intel-gfx@lists.freedesktop.org
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Dmitry Safonov
---
drivers/char/random.c| 16 ---
drivers/gpu/drm/i915/i915_perf.c | 4 +--
include/linux/ratelimit.h| 24 +---
lib/ratelimit.c | 59 +++-
4 files changed, 50 insertions(+), 53 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index a8fb0020ba5c..ba67ea0dc568 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -936,24 +936,20 @@ static void crng_reseed(struct crng_state *crng, struct
entropy_store *r)
crng->init_time = jiffies;
spin_unlock_irqrestore(>lock, flags);
if (crng == _crng && crng_init < 2) {
+ int unseeded_miss, urandom_miss;
+
invalidate_batched_entropy();
numa_crng_init();
crng_init = 2;
process_random_ready_list();
wake_up_interruptible(_init_wait);
pr_notice("random: crng init done\n");
- if (unseeded_warning.missed) {
+ if ((unseeded_miss = atomic_xchg(_warning.missed, 0)))
pr_notice("random: %d get_random_xx warning(s) missed "
- "due to ratelimiting\n",
- unseeded_warning.missed);
- unseeded_warning.missed = 0;
- }
- if (urandom_warning.missed) {
+ "due to ratelimiting\n", unseeded_miss);
+ if ((urandom_miss = atomic_xchg(_warning.missed, 0)))
pr_notice("random: %d urandom warning(s) missed "
- "due to ratelimiting\n",
- urandom_warning.missed);
- urandom_warning.missed = 0;
- }
+ "due to ratelimiting\n", urandom_miss);
}
}
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 019bd2d073ad..75f6203f6e8e 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -1317,9 +1317,9 @@ static void i915_oa_stream_destroy(struct
i915_perf_stream *stream)
put_oa_config(dev_priv, stream->oa_config);
- if (dev_priv->perf.oa.spurious_report_rs.missed) {
+ if (atomic_read(_priv->perf.oa.spurious_report_rs.missed)) {
DRM_NOTE("%d spurious OA report notices suppressed due to
ratelimiting\n",
-dev_priv->perf.oa.spurious_report_rs.missed);
+
atomic_read(_priv->perf.oa.spurious_report_rs.missed));
}
}
diff --git a/include/linux/ratelimit.h b/include/linux/ratelimit.h
index 8ddf79e9207a..7b5914e12d5b 100644
--- a/include/linux/ratelimit.h
+++ b/include/linux/ratelimit.h
@@ -4,7 +4,6 @@
#include
#include
-#include
#define DEFAULT_RATELIMIT_INTERVAL (5 * HZ)
#define DEFAULT_RATEL
[Intel-gfx] [PATCH 0/2] ratelimit: Do not lose messages under limit
There are two issues with ratelimiting as far a I can see: 1. Messages may be lost even if their amount fit burst limit; 2. "suppressed" message may not be printed if there is no call to ___ratelimit() after interval ends. I address (1) issue in the second patch. While the (2) issue will require adding timers to print "suppressed" message and care if ratelimit is on stack and no more exists. Which looks too much at this point. Dmitry Safonov (2): random: Omit double-printing ratelimit messages lib/ratelimit: Lockless ratelimiting drivers/char/random.c| 22 +++ drivers/gpu/drm/i915/i915_perf.c | 4 +-- include/linux/ratelimit.h| 34 ++- lib/ratelimit.c | 59 +++- 4 files changed, 61 insertions(+), 58 deletions(-) -- 2.13.6 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] lib/ratelimit: Lockless ratelimiting
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: in the rare corner-case when (rs->burst == 0) and concurrent
access suppressed message might be printed from both (several) CPUs,
that are accessing ratelimit.
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).
Cc: Arnd Bergmann <a...@arndb.de>
Cc: David Airlie <airl...@linux.ie>
Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Cc: Jani Nikula <jani.nik...@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.v...@intel.com>
Cc: "Theodore Ts'o" <ty...@mit.edu>
Cc: Thomas Gleixner <t...@linutronix.de>
Cc: intel-gfx@lists.freedesktop.org
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Dmitry Safonov <d...@arista.com>
---
drivers/char/random.c| 16 ---
drivers/gpu/drm/i915/i915_perf.c | 4 +--
include/linux/ratelimit.h| 24 +---
lib/ratelimit.c | 59 +++-
4 files changed, 50 insertions(+), 53 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index c1c40c7ed0e8..3694cbcb04a0 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -937,25 +937,21 @@ static void crng_reseed(struct crng_state *crng, struct
entropy_store *r)
crng->init_time = jiffies;
spin_unlock_irqrestore(>lock, flags);
if (crng == _crng && crng_init < 2) {
+ int unseeded_miss, urandom_miss;
+
invalidate_batched_entropy();
numa_crng_init();
crng_init = 2;
process_random_ready_list();
wake_up_interruptible(_init_wait);
pr_notice("random: crng init done\n");
- if (unseeded_warning.missed) {
+ if ((unseeded_miss = atomic_xchg(_warning.missed, 0)))
pr_notice("random: %d get_random_xx warning(s) missed "
- "due to ratelimiting\n",
- unseeded_warning.missed);
- unseeded_warning.missed = 0;
- }
+ "due to ratelimiting\n", unseeded_miss);
unseeded_warning.flags = 0;
- if (urandom_warning.missed) {
+ if ((urandom_miss = atomic_xchg(_warning.missed, 0)))
pr_notice("random: %d urandom warning(s) missed "
- "due to ratelimiting\n",
- urandom_warning.missed);
- urandom_warning.missed = 0;
- }
+ "due to ratelimiting\n", urandom_miss);
urandom_warning.flags = 0;
}
}
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index abaca6edeb71..a8e00913bdb9 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -1316,9 +1316,9 @@ static void i915_oa_stream_destroy(struct
i915_perf_stream *stream)
put_oa_config(dev_priv, stream->oa_config);
- if (dev_priv->perf.oa.spurious_report_rs.missed) {
+ if (atomic_read(_priv->perf.oa.spurious_report_rs.missed)) {
DRM_NOTE("%d spurious OA report notices suppressed due to
ratelimiting\n",
-dev_priv->perf.oa.spurious_report_rs.missed);
+
atomic_read(_priv->perf.oa.spurious_report_rs.missed));
}
}
diff --git a/include/linux/ratelimit.h b/include/linux/ratelimit.h
index e9a14a7641e0..ddc572389f08 100644
--- a/include/linux/ratelimit.h
+++ b/include/linux/ratelimit.h
@@ -4,7 +4,6 @@
#include
#inclu
