From 56a7b9f8b05981e929becb45a826558779bca6f6 Mon Sep 17 00:00:00 2001 From: "Paul E. McKenney" Date: Thu, 13 Mar 2025 15:31:50 -0700 Subject: [PATCH 01/19] ratelimit: Create functions to handle ratelimit_state internals A number of ratelimit use cases do open-coded access to the ratelimit_state structure's ->missed field. This works, but is a bit messy and makes it more annoying to make changes to this field. Therefore, provide a ratelimit_state_inc_miss() function that increments the ->missed field, a ratelimit_state_get_miss() function that reads out the ->missed field, and a ratelimit_state_reset_miss() function that reads out that field, but that also resets its value to zero. These functions will replace client-code open-coded uses of ->missed. In addition, a new ratelimit_state_reset_interval() function encapsulates what was previously open-coded lock acquisition and direct field updates. [ paulmck: Apply kernel test robot feedback. ] Link: https://lore.kernel.org/all/fbe93a52-365e-47fe-93a4-44a44547d601@paulmck-laptop/ Link: https://lore.kernel.org/all/20250423115409.3425-1-spasswolf@web.de/ Signed-off-by: Paul E. McKenney Reviewed-by: Petr Mladek Cc: Andrew Morton Cc: Kuniyuki Iwashima Cc: Mateusz Guzik Cc: Steven Rostedt Cc: John Ogness Cc: Sergey Senozhatsky --- include/linux/ratelimit.h | 40 ++++++++++++++++++++++++++++++++++----- lib/ratelimit.c | 8 ++++---- 2 files changed, 39 insertions(+), 9 deletions(-) diff --git a/include/linux/ratelimit.h b/include/linux/ratelimit.h index b17e0cd0a30c..8400c5356c18 100644 --- a/include/linux/ratelimit.h +++ b/include/linux/ratelimit.h @@ -22,16 +22,46 @@ static inline void ratelimit_default_init(struct ratelimit_state *rs) DEFAULT_RATELIMIT_BURST); } +static inline void ratelimit_state_inc_miss(struct ratelimit_state *rs) +{ + rs->missed++; +} + +static inline int ratelimit_state_get_miss(struct ratelimit_state *rs) +{ + return rs->missed; +} + +static inline int ratelimit_state_reset_miss(struct ratelimit_state *rs) +{ + int ret = rs->missed; + + rs->missed = 0; + return ret; +} + +static inline void ratelimit_state_reset_interval(struct ratelimit_state *rs, int interval_init) +{ + unsigned long flags; + + raw_spin_lock_irqsave(&rs->lock, flags); + rs->interval = interval_init; + rs->begin = 0; + rs->printed = 0; + ratelimit_state_reset_miss(rs); + raw_spin_unlock_irqrestore(&rs->lock, flags); +} + static inline void ratelimit_state_exit(struct ratelimit_state *rs) { + int m; + if (!(rs->flags & RATELIMIT_MSG_ON_RELEASE)) return; - if (rs->missed) { - pr_warn("%s: %d output lines suppressed due to ratelimiting\n", - current->comm, rs->missed); - rs->missed = 0; - } + m = ratelimit_state_reset_miss(rs); + if (m) + pr_warn("%s: %d output lines suppressed due to ratelimiting\n", current->comm, m); } static inline void diff --git a/lib/ratelimit.c b/lib/ratelimit.c index ce945c17980b..85e22f00180c 100644 --- a/lib/ratelimit.c +++ b/lib/ratelimit.c @@ -51,12 +51,12 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func) rs->begin = jiffies; if (time_is_before_jiffies(rs->begin + interval)) { - if (rs->missed) { + int m = ratelimit_state_reset_miss(rs); + + if (m) { if (!(rs->flags & RATELIMIT_MSG_ON_RELEASE)) { printk_deferred(KERN_WARNING - "%s: %d callbacks suppressed\n", - func, rs->missed); - rs->missed = 0; + "%s: %d callbacks suppressed\n", func, m); } } rs->begin = jiffies; From 48e864ae865744d1451b1776a14acfa422e8a115 Mon Sep 17 00:00:00 2001 From: "Paul E. McKenney" Date: Thu, 13 Mar 2025 16:11:38 -0700 Subject: [PATCH 02/19] random: Avoid open-coded use of ratelimit_state structure's ->missed field The _credit_init_bits() function directly accesses the ratelimit_state structure's ->missed field, which works, but which also makes it more difficult to change this field. Therefore, make use of the ratelimit_state_get_miss() and ratelimit_state_inc_miss() functions instead of directly accessing the ->missed field. Link: https://lore.kernel.org/all/fbe93a52-365e-47fe-93a4-44a44547d601@paulmck-laptop/ Link: https://lore.kernel.org/all/20250423115409.3425-1-spasswolf@web.de/ Signed-off-by: Paul E. McKenney Reviewed-by: Petr Mladek Cc: "Theodore Ts'o" "Jason A. Donenfeld" --- drivers/char/random.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/char/random.c b/drivers/char/random.c index 38f2fab29c56..416dac0ab565 100644 --- a/drivers/char/random.c +++ b/drivers/char/random.c @@ -726,6 +726,7 @@ static void __cold _credit_init_bits(size_t bits) static DECLARE_WORK(set_ready, crng_set_ready); unsigned int new, orig, add; unsigned long flags; + int m; if (!bits) return; @@ -748,9 +749,9 @@ static void __cold _credit_init_bits(size_t bits) wake_up_interruptible(&crng_init_wait); kill_fasync(&fasync, SIGIO, POLL_IN); pr_notice("crng init done\n"); - if (urandom_warning.missed) - pr_notice("%d urandom warning(s) missed due to ratelimiting\n", - urandom_warning.missed); + m = ratelimit_state_get_miss(&urandom_warning); + if (m) + pr_notice("%d urandom warning(s) missed due to ratelimiting\n", m); } else if (orig < POOL_EARLY_BITS && new >= POOL_EARLY_BITS) { spin_lock_irqsave(&base_crng.lock, flags); /* Check if crng_init is CRNG_EMPTY, to avoid race with crng_reseed(). */ @@ -1466,7 +1467,7 @@ static ssize_t urandom_read_iter(struct kiocb *kiocb, struct iov_iter *iter) if (!crng_ready()) { if (!ratelimit_disable && maxwarn <= 0) - ++urandom_warning.missed; + ratelimit_state_inc_miss(&urandom_warning); else if (ratelimit_disable || __ratelimit(&urandom_warning)) { --maxwarn; pr_notice("%s: uninitialized urandom read (%zu bytes read)\n", From 25228c60999f848fdeb291031858de49c115b64e Mon Sep 17 00:00:00 2001 From: "Paul E. McKenney" Date: Thu, 13 Mar 2025 16:24:36 -0700 Subject: [PATCH 03/19] drm/i915: Avoid open-coded use of ratelimit_state structure's ->missed field The i915_oa_stream_destroy() function directly accesses the ratelimit_state structure's ->missed field, which works, but which also makes it more difficult to change this field. Therefore, make use of the ratelimit_state_get_miss() function instead of directly accessing the ->missed field. Link: https://lore.kernel.org/all/fbe93a52-365e-47fe-93a4-44a44547d601@paulmck-laptop/ Link: https://lore.kernel.org/all/20250423115409.3425-1-spasswolf@web.de/ Signed-off-by: Paul E. McKenney Reviewed-by: Petr Mladek Acked-by: Jani Nikula Cc: Joonas Lahtinen Cc: Rodrigo Vivi Cc: Tvrtko Ursulin Cc: David Airlie Cc: Simona Vetter Cc: Cc: --- drivers/gpu/drm/i915/i915_perf.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c index de0b413600a1..1658f1246c6f 100644 --- a/drivers/gpu/drm/i915/i915_perf.c +++ b/drivers/gpu/drm/i915/i915_perf.c @@ -1666,6 +1666,7 @@ static void i915_oa_stream_destroy(struct i915_perf_stream *stream) struct i915_perf *perf = stream->perf; struct intel_gt *gt = stream->engine->gt; struct i915_perf_group *g = stream->engine->oa_group; + int m; if (WARN_ON(stream != g->exclusive_stream)) return; @@ -1690,10 +1691,9 @@ static void i915_oa_stream_destroy(struct i915_perf_stream *stream) free_oa_configs(stream); free_noa_wait(stream); - if (perf->spurious_report_rs.missed) { - gt_notice(gt, "%d spurious OA report notices suppressed due to ratelimiting\n", - perf->spurious_report_rs.missed); - } + m = ratelimit_state_get_miss(&perf->spurious_report_rs); + if (m) + gt_notice(gt, "%d spurious OA report notices suppressed due to ratelimiting\n", m); } static void gen7_init_oa_buffer(struct i915_perf_stream *stream) From c6f7f1b2c0ff46b9069a8fbc7d167c9ead66dfce Mon Sep 17 00:00:00 2001 From: "Paul E. McKenney" Date: Mon, 17 Mar 2025 20:28:40 -0700 Subject: [PATCH 04/19] drm/amd/pm: Avoid open-coded use of ratelimit_state structure's internals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The amdgpu_set_thermal_throttling_logging() function directly accesses the ratelimit_state structure's ->missed field, which works, but which also makes it more difficult to change this field. Therefore, make use of the ratelimit_state_reset_interval() function instead of directly accessing the ->missed field. Nevertheless, open-coded use of ->burst and ->interval is still permitted, for example, for runtime sysfs adjustment of these fields. Link: https://lore.kernel.org/all/fbe93a52-365e-47fe-93a4-44a44547d601@paulmck-laptop/ Link: https://lore.kernel.org/all/20250423115409.3425-1-spasswolf@web.de/ Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202503180826.EiekA1MB-lkp@intel.com/ Signed-off-by: Paul E. McKenney Acked-by: Alex Deucher Reviewed-by: Petr Mladek Cc: Kenneth Feng Cc: "Christian König" Cc: Xinhui Pan Cc: David Airlie Cc: Simona Vetter Cc: Cc: --- drivers/gpu/drm/amd/pm/amdgpu_pm.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c b/drivers/gpu/drm/amd/pm/amdgpu_pm.c index 922def51685b..d533c79f7e21 100644 --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c @@ -1606,7 +1606,6 @@ static ssize_t amdgpu_set_thermal_throttling_logging(struct device *dev, struct drm_device *ddev = dev_get_drvdata(dev); struct amdgpu_device *adev = drm_to_adev(ddev); long throttling_logging_interval; - unsigned long flags; int ret = 0; ret = kstrtol(buf, 0, &throttling_logging_interval); @@ -1617,18 +1616,12 @@ static ssize_t amdgpu_set_thermal_throttling_logging(struct device *dev, return -EINVAL; if (throttling_logging_interval > 0) { - raw_spin_lock_irqsave(&adev->throttling_logging_rs.lock, flags); /* * Reset the ratelimit timer internals. * This can effectively restart the timer. */ - adev->throttling_logging_rs.interval = - (throttling_logging_interval - 1) * HZ; - adev->throttling_logging_rs.begin = 0; - adev->throttling_logging_rs.printed = 0; - adev->throttling_logging_rs.missed = 0; - raw_spin_unlock_irqrestore(&adev->throttling_logging_rs.lock, flags); - + ratelimit_state_reset_interval(&adev->throttling_logging_rs, + (throttling_logging_interval - 1) * HZ); atomic_set(&adev->throttling_logging_enabled, 1); } else { atomic_set(&adev->throttling_logging_enabled, 0); From 78bf44de47b3cec72ee8ddc14161d5b3212f25e0 Mon Sep 17 00:00:00 2001 From: "Paul E. McKenney" Date: Fri, 14 Mar 2025 09:07:13 -0700 Subject: [PATCH 05/19] ratelimit: Convert the ->missed field to atomic_t The ratelimit_state structure's ->missed field is sometimes incremented locklessly, and it would be good to avoid lost counts. This is also needed to count the number of misses due to trylock failure. Therefore, convert the ratelimit_state structure's ->missed field to atomic_t. Link: https://lore.kernel.org/all/fbe93a52-365e-47fe-93a4-44a44547d601@paulmck-laptop/ Link: https://lore.kernel.org/all/20250423115409.3425-1-spasswolf@web.de/ Signed-off-by: Paul E. McKenney Reviewed-by: Petr Mladek Cc: Andrew Morton Cc: Kuniyuki Iwashima Cc: Mateusz Guzik Cc: Steven Rostedt Cc: John Ogness Cc: Sergey Senozhatsky --- include/linux/ratelimit.h | 9 +++------ include/linux/ratelimit_types.h | 2 +- lib/ratelimit.c | 2 +- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/include/linux/ratelimit.h b/include/linux/ratelimit.h index 8400c5356c18..c78b92b3e5cd 100644 --- a/include/linux/ratelimit.h +++ b/include/linux/ratelimit.h @@ -24,20 +24,17 @@ static inline void ratelimit_default_init(struct ratelimit_state *rs) static inline void ratelimit_state_inc_miss(struct ratelimit_state *rs) { - rs->missed++; + atomic_inc(&rs->missed); } static inline int ratelimit_state_get_miss(struct ratelimit_state *rs) { - return rs->missed; + return atomic_read(&rs->missed); } static inline int ratelimit_state_reset_miss(struct ratelimit_state *rs) { - int ret = rs->missed; - - rs->missed = 0; - return ret; + return atomic_xchg_relaxed(&rs->missed, 0); } static inline void ratelimit_state_reset_interval(struct ratelimit_state *rs, int interval_init) diff --git a/include/linux/ratelimit_types.h b/include/linux/ratelimit_types.h index 765232ce0b5e..d21fe82b67f6 100644 --- a/include/linux/ratelimit_types.h +++ b/include/linux/ratelimit_types.h @@ -18,7 +18,7 @@ struct ratelimit_state { int interval; int burst; int printed; - int missed; + atomic_t missed; unsigned int flags; unsigned long begin; }; diff --git a/lib/ratelimit.c b/lib/ratelimit.c index 85e22f00180c..18703f92d73e 100644 --- a/lib/ratelimit.c +++ b/lib/ratelimit.c @@ -66,7 +66,7 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func) rs->printed++; ret = 1; } else { - rs->missed++; + ratelimit_state_inc_miss(rs); ret = 0; } raw_spin_unlock_irqrestore(&rs->lock, flags); From d343732ddbfa15db88a628bf3284eb088efbaaf7 Mon Sep 17 00:00:00 2001 From: "Paul E. McKenney" Date: Fri, 14 Mar 2025 11:26:59 -0700 Subject: [PATCH 06/19] ratelimit: Count misses due to lock contention The ___ratelimit() function simply returns zero ("do ratelimiting") if the trylock fails, but does not adjust the ->missed field. This means that the resulting dropped printk()s are dropped silently, which could seriously confuse people trying to do console-log-based debugging. Therefore, increment the ->missed field upon trylock failure. Link: https://lore.kernel.org/all/fbe93a52-365e-47fe-93a4-44a44547d601@paulmck-laptop/ Link: https://lore.kernel.org/all/20250423115409.3425-1-spasswolf@web.de/ Signed-off-by: Paul E. McKenney Reviewed-by: Petr Mladek Cc: Andrew Morton Cc: Kuniyuki Iwashima Cc: Mateusz Guzik Cc: Steven Rostedt Cc: John Ogness Cc: Sergey Senozhatsky --- lib/ratelimit.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/ratelimit.c b/lib/ratelimit.c index 18703f92d73e..19ad3cdbd171 100644 --- a/lib/ratelimit.c +++ b/lib/ratelimit.c @@ -44,8 +44,10 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func) * in addition to the one that will be printed by * the entity that is holding the lock already: */ - if (!raw_spin_trylock_irqsave(&rs->lock, flags)) + if (!raw_spin_trylock_irqsave(&rs->lock, flags)) { + ratelimit_state_inc_miss(rs); return 0; + } if (!rs->begin) rs->begin = jiffies; From e64a348dc148af41cec266b2143305a2d0228ee7 Mon Sep 17 00:00:00 2001 From: "Paul E. McKenney" Date: Tue, 25 Mar 2025 15:32:54 -0700 Subject: [PATCH 07/19] ratelimit: Avoid jiffies=0 special case The ___ratelimit() function special-cases the jiffies-counter value of zero as "uninitialized". This works well on 64-bit systems, where the jiffies counter is not going to return to zero for more than half a billion years on systems with HZ=1000, but similar 32-bit systems take less than 50 days to wrap the jiffies counter. And although the consequences of wrapping the jiffies counter seem to be limited to minor confusion on the duration of the rate-limiting interval that happens to end at time zero, it is almost no work to avoid this confusion. Therefore, introduce a RATELIMIT_INITIALIZED bit to the ratelimit_state structure's ->flags field so that a ->begin value of zero is no longer special. Link: https://lore.kernel.org/all/fbe93a52-365e-47fe-93a4-44a44547d601@paulmck-laptop/ Link: https://lore.kernel.org/all/20250423115409.3425-1-spasswolf@web.de/ Signed-off-by: Paul E. McKenney Reviewed-by: Petr Mladek Cc: Andrew Morton Cc: Kuniyuki Iwashima Cc: Mateusz Guzik Cc: Steven Rostedt Cc: John Ogness Cc: Sergey Senozhatsky --- include/linux/ratelimit.h | 2 +- include/linux/ratelimit_types.h | 1 + lib/ratelimit.c | 4 +++- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/include/linux/ratelimit.h b/include/linux/ratelimit.h index c78b92b3e5cd..adfec24061d1 100644 --- a/include/linux/ratelimit.h +++ b/include/linux/ratelimit.h @@ -43,7 +43,7 @@ static inline void ratelimit_state_reset_interval(struct ratelimit_state *rs, in raw_spin_lock_irqsave(&rs->lock, flags); rs->interval = interval_init; - rs->begin = 0; + rs->flags &= ~RATELIMIT_INITIALIZED; rs->printed = 0; ratelimit_state_reset_miss(rs); raw_spin_unlock_irqrestore(&rs->lock, flags); diff --git a/include/linux/ratelimit_types.h b/include/linux/ratelimit_types.h index d21fe82b67f6..ef6711b6b229 100644 --- a/include/linux/ratelimit_types.h +++ b/include/linux/ratelimit_types.h @@ -11,6 +11,7 @@ /* issue num suppressed message on exit */ #define RATELIMIT_MSG_ON_RELEASE BIT(0) +#define RATELIMIT_INITIALIZED BIT(1) struct ratelimit_state { raw_spinlock_t lock; /* protect the state */ diff --git a/lib/ratelimit.c b/lib/ratelimit.c index 19ad3cdbd171..bd6e3b429e33 100644 --- a/lib/ratelimit.c +++ b/lib/ratelimit.c @@ -49,8 +49,10 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func) return 0; } - if (!rs->begin) + if (!(rs->flags & RATELIMIT_INITIALIZED)) { rs->begin = jiffies; + rs->flags |= RATELIMIT_INITIALIZED; + } if (time_is_before_jiffies(rs->begin + interval)) { int m = ratelimit_state_reset_miss(rs); From cf8cfa8a9978bfe77f2648a04824a46d58258e68 Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Wed, 9 Apr 2025 16:54:33 -0700 Subject: [PATCH 08/19] ratelimit: Reduce ___ratelimit() false-positive rate limiting Retain the locked design, but check rate-limiting even when the lock could not be acquired. Link: https://lore.kernel.org/all/Z_VRo63o2UsVoxLG@pathway.suse.cz/ Link: https://lore.kernel.org/all/fbe93a52-365e-47fe-93a4-44a44547d601@paulmck-laptop/ Link: https://lore.kernel.org/all/20250423115409.3425-1-spasswolf@web.de/ Signed-off-by: Petr Mladek Signed-off-by: Paul E. McKenney Cc: Petr Mladek Cc: Andrew Morton Cc: Kuniyuki Iwashima Cc: Mateusz Guzik Cc: Steven Rostedt Cc: John Ogness Cc: Sergey Senozhatsky --- include/linux/ratelimit.h | 2 +- include/linux/ratelimit_types.h | 2 +- lib/ratelimit.c | 51 ++++++++++++++++++++++++--------- 3 files changed, 40 insertions(+), 15 deletions(-) diff --git a/include/linux/ratelimit.h b/include/linux/ratelimit.h index adfec24061d1..7aaad158ee37 100644 --- a/include/linux/ratelimit.h +++ b/include/linux/ratelimit.h @@ -44,7 +44,7 @@ static inline void ratelimit_state_reset_interval(struct ratelimit_state *rs, in raw_spin_lock_irqsave(&rs->lock, flags); rs->interval = interval_init; rs->flags &= ~RATELIMIT_INITIALIZED; - rs->printed = 0; + atomic_set(&rs->rs_n_left, rs->burst); ratelimit_state_reset_miss(rs); raw_spin_unlock_irqrestore(&rs->lock, flags); } diff --git a/include/linux/ratelimit_types.h b/include/linux/ratelimit_types.h index ef6711b6b229..b19c4354540a 100644 --- a/include/linux/ratelimit_types.h +++ b/include/linux/ratelimit_types.h @@ -18,7 +18,7 @@ struct ratelimit_state { int interval; int burst; - int printed; + atomic_t rs_n_left; atomic_t missed; unsigned int flags; unsigned long begin; diff --git a/lib/ratelimit.c b/lib/ratelimit.c index bd6e3b429e33..90c9fe57eb42 100644 --- a/lib/ratelimit.c +++ b/lib/ratelimit.c @@ -39,12 +39,22 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func) 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 we contend on this state's lock then just check if + * the current burst is used or not. It might cause + * false positive when we are past the interval and + * the current lock owner is just about to reset it. */ if (!raw_spin_trylock_irqsave(&rs->lock, flags)) { + unsigned int rs_flags = READ_ONCE(rs->flags); + + if (rs_flags & RATELIMIT_INITIALIZED && burst) { + int n_left; + + n_left = atomic_dec_return(&rs->rs_n_left); + if (n_left >= 0) + return 1; + } + ratelimit_state_inc_miss(rs); return 0; } @@ -52,27 +62,42 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func) if (!(rs->flags & RATELIMIT_INITIALIZED)) { rs->begin = jiffies; rs->flags |= RATELIMIT_INITIALIZED; + atomic_set(&rs->rs_n_left, rs->burst); } if (time_is_before_jiffies(rs->begin + interval)) { - int m = ratelimit_state_reset_miss(rs); + int m; + /* + * Reset rs_n_left ASAP to reduce false positives + * in parallel calls, see above. + */ + atomic_set(&rs->rs_n_left, rs->burst); + rs->begin = jiffies; + + m = ratelimit_state_reset_miss(rs); if (m) { if (!(rs->flags & RATELIMIT_MSG_ON_RELEASE)) { printk_deferred(KERN_WARNING "%s: %d callbacks suppressed\n", func, m); } } - rs->begin = jiffies; - rs->printed = 0; } - if (burst && burst > rs->printed) { - rs->printed++; - ret = 1; - } else { - ratelimit_state_inc_miss(rs); - ret = 0; + if (burst) { + int n_left; + + /* The burst might have been taken by a parallel call. */ + n_left = atomic_dec_return(&rs->rs_n_left); + if (n_left >= 0) { + ret = 1; + goto unlock_ret; + } } + + ratelimit_state_inc_miss(rs); + ret = 0; + +unlock_ret: raw_spin_unlock_irqrestore(&rs->lock, flags); return ret; From 084a990ded6337fb603f39b43b67ec68c2da0695 Mon Sep 17 00:00:00 2001 From: "Paul E. McKenney" Date: Wed, 9 Apr 2025 16:58:53 -0700 Subject: [PATCH 09/19] ratelimit: Allow zero ->burst to disable ratelimiting If ->interval is zero, then rate-limiting will be disabled. Alternatively, if interval is greater than zero and ->burst is zero, then rate-limiting will be applied unconditionally. The point of this distinction is to handle current users that pass zero-initialized ratelimit_state structures to ___ratelimit(), and in such cases the ->lock field will be uninitialized. Acquiring ->lock in this case is clearly not a strategy to win. Therefore, make this classification be lockless. Note that although negative ->interval and ->burst happen to be treated as if they were zero, this is an accident of the current implementation. The semantics of negative values for these fields is subject to change without notice. Especially given that Bert Karwatzki determined that no current calls to ___ratelimit() ever have negative values for these fields. This commit replaces an earlier buggy versions. Link: https://lore.kernel.org/all/fbe93a52-365e-47fe-93a4-44a44547d601@paulmck-laptop/ Link: https://lore.kernel.org/all/20250423115409.3425-1-spasswolf@web.de/ Reported-by: Bert Karwatzki Reported-by: "Aithal, Srikanth" Closes: https://lore.kernel.org/all/20250423115409.3425-1-spasswolf@web.de/ Reported-by: Mark Brown Closes: https://lore.kernel.org/all/257c3b91-e30f-48be-9788-d27a4445a416@sirena.org.uk/ Signed-off-by: Paul E. McKenney Tested-by: "Aithal, Srikanth" Reviewed-by: Petr Mladek Cc: Andrew Morton Cc: Kuniyuki Iwashima Cc: Mateusz Guzik Cc: Steven Rostedt Cc: John Ogness Cc: Sergey Senozhatsky --- lib/ratelimit.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/ratelimit.c b/lib/ratelimit.c index 90c9fe57eb42..7a7ba4835639 100644 --- a/lib/ratelimit.c +++ b/lib/ratelimit.c @@ -35,8 +35,12 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func) unsigned long flags; int ret; - if (!interval) - return 1; + if (interval <= 0 || burst <= 0) { + ret = interval == 0 || burst > 0; + if (!ret) + ratelimit_state_inc_miss(rs); + return ret; + } /* * If we contend on this state's lock then just check if From aa2cc356f8791438761efa0cb95fd48f3e2721c2 Mon Sep 17 00:00:00 2001 From: "Paul E. McKenney" Date: Wed, 23 Apr 2025 12:03:59 -0700 Subject: [PATCH 10/19] ratelimit: Force re-initialization when rate-limiting re-enabled Currently, if rate limiting is disabled, ___ratelimit() does an immediate early return with no state changes. This can result in false-positive drops when re-enabling rate limiting. Therefore, mark the ratelimit_state structure "uninitialized" when rate limiting is disabled. [ paulmck: Apply Petr Mladek feedback. ] Link: https://lore.kernel.org/all/fbe93a52-365e-47fe-93a4-44a44547d601@paulmck-laptop/ Link: https://lore.kernel.org/all/20250423115409.3425-1-spasswolf@web.de/ Signed-off-by: Paul E. McKenney Reviewed-by: Petr Mladek Cc: Andrew Morton Cc: Kuniyuki Iwashima Cc: Mateusz Guzik Cc: Steven Rostedt Cc: John Ogness Cc: Sergey Senozhatsky --- lib/ratelimit.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/lib/ratelimit.c b/lib/ratelimit.c index 7a7ba4835639..7d4f4e241213 100644 --- a/lib/ratelimit.c +++ b/lib/ratelimit.c @@ -35,11 +35,24 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func) unsigned long flags; int ret; + /* + * Zero interval says never limit, otherwise, non-positive burst + * says always limit. + */ if (interval <= 0 || burst <= 0) { ret = interval == 0 || burst > 0; + if (!(READ_ONCE(rs->flags) & RATELIMIT_INITIALIZED) || (!interval && !burst) || + !raw_spin_trylock_irqsave(&rs->lock, flags)) { + if (!ret) + ratelimit_state_inc_miss(rs); + return ret; + } + + /* Force re-initialization once re-enabled. */ + rs->flags &= ~RATELIMIT_INITIALIZED; if (!ret) ratelimit_state_inc_miss(rs); - return ret; + goto unlock_ret; } /* From 21ac6e5edac569df3938b136471c59c1d3d01f09 Mon Sep 17 00:00:00 2001 From: "Paul E. McKenney" Date: Thu, 10 Apr 2025 14:37:31 -0700 Subject: [PATCH 11/19] ratelimit: Don't flush misses counter if RATELIMIT_MSG_ON_RELEASE Restore the previous semantics where the misses counter is unchanged if the RATELIMIT_MSG_ON_RELEASE flag is set. Link: https://lore.kernel.org/all/fbe93a52-365e-47fe-93a4-44a44547d601@paulmck-laptop/ Link: https://lore.kernel.org/all/20250423115409.3425-1-spasswolf@web.de/ Signed-off-by: Paul E. McKenney Reviewed-by: Petr Mladek Cc: Andrew Morton Cc: Kuniyuki Iwashima Cc: Mateusz Guzik Cc: Steven Rostedt Cc: John Ogness Cc: Sergey Senozhatsky --- lib/ratelimit.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/ratelimit.c b/lib/ratelimit.c index 7d4f4e241213..4e520d029d28 100644 --- a/lib/ratelimit.c +++ b/lib/ratelimit.c @@ -92,9 +92,9 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func) atomic_set(&rs->rs_n_left, rs->burst); rs->begin = jiffies; - m = ratelimit_state_reset_miss(rs); - if (m) { - if (!(rs->flags & RATELIMIT_MSG_ON_RELEASE)) { + if (!(rs->flags & RATELIMIT_MSG_ON_RELEASE)) { + m = ratelimit_state_reset_miss(rs); + if (m) { printk_deferred(KERN_WARNING "%s: %d callbacks suppressed\n", func, m); } From 123a1d97b2baf9eba9662a5f65660edc317e0bb8 Mon Sep 17 00:00:00 2001 From: "Paul E. McKenney" Date: Mon, 14 Apr 2025 15:43:47 -0700 Subject: [PATCH 12/19] ratelimit: Avoid atomic decrement if already rate-limited Currently, if the lock could not be acquired, the code unconditionally does an atomic decrement on ->rs_n_left, even if that atomic operation is guaranteed to return a limit-rate verdict. This incurs needless overhead and also raises the spectre of counter wrap. Therefore, do the atomic decrement only if there is some chance that rates won't be limited. Link: https://lore.kernel.org/all/fbe93a52-365e-47fe-93a4-44a44547d601@paulmck-laptop/ Link: https://lore.kernel.org/all/20250423115409.3425-1-spasswolf@web.de/ Signed-off-by: Paul E. McKenney Reviewed-by: Petr Mladek Cc: Andrew Morton Cc: Kuniyuki Iwashima Cc: Mateusz Guzik Cc: Steven Rostedt Cc: John Ogness Cc: Sergey Senozhatsky --- lib/ratelimit.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/ratelimit.c b/lib/ratelimit.c index 4e520d029d28..a7aaebb7a718 100644 --- a/lib/ratelimit.c +++ b/lib/ratelimit.c @@ -65,8 +65,10 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func) unsigned int rs_flags = READ_ONCE(rs->flags); if (rs_flags & RATELIMIT_INITIALIZED && burst) { - int n_left; + int n_left = atomic_read(&rs->rs_n_left); + if (n_left <= 0) + return 0; n_left = atomic_dec_return(&rs->rs_n_left); if (n_left >= 0) return 1; From 96d366048fed6a70accf4ddb55c4c65ab27aa48f Mon Sep 17 00:00:00 2001 From: "Paul E. McKenney" Date: Wed, 16 Apr 2025 09:34:14 -0700 Subject: [PATCH 13/19] ratelimit: Avoid atomic decrement under lock if already rate-limited Currently, if the lock is acquired, the code unconditionally does an atomic decrement on ->rs_n_left, even if that atomic operation is guaranteed to return a limit-rate verdict. A limit-rate verdict will in fact be the common case when something is spewing into a rate limit. This unconditional atomic operation incurs needless overhead and also raises the spectre of counter wrap. Therefore, do the atomic decrement only if there is some chance that rates won't be limited. Link: https://lore.kernel.org/all/fbe93a52-365e-47fe-93a4-44a44547d601@paulmck-laptop/ Link: https://lore.kernel.org/all/20250423115409.3425-1-spasswolf@web.de/ Signed-off-by: Paul E. McKenney Reviewed-by: Petr Mladek Cc: Andrew Morton Cc: Kuniyuki Iwashima Cc: Mateusz Guzik Cc: Steven Rostedt Cc: John Ogness Cc: Sergey Senozhatsky --- lib/ratelimit.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/ratelimit.c b/lib/ratelimit.c index a7aaebb7a718..ab8472edeb1d 100644 --- a/lib/ratelimit.c +++ b/lib/ratelimit.c @@ -103,13 +103,16 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func) } } if (burst) { - int n_left; + int n_left = atomic_read(&rs->rs_n_left); /* The burst might have been taken by a parallel call. */ - n_left = atomic_dec_return(&rs->rs_n_left); - if (n_left >= 0) { - ret = 1; - goto unlock_ret; + + if (n_left > 0) { + n_left = atomic_dec_return(&rs->rs_n_left); + if (n_left >= 0) { + ret = 1; + goto unlock_ret; + } } } From a940d145cc381c8cde0d0f7f9faf282fbab89bfb Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Thu, 24 Apr 2025 11:19:13 -0700 Subject: [PATCH 14/19] ratelimit: Warn if ->interval or ->burst are negative Currently, ___ratelimit() treats a negative ->interval or ->burst as if it was zero, but this is an accident of the current implementation. Therefore, splat in this case, which might have the benefit of detecting use of uninitialized ratelimit_state structures on the one hand or easing addition of new features on the other. Link: https://lore.kernel.org/all/fbe93a52-365e-47fe-93a4-44a44547d601@paulmck-laptop/ Link: https://lore.kernel.org/all/20250423115409.3425-1-spasswolf@web.de/ Signed-off-by: Petr Mladek Signed-off-by: Paul E. McKenney Cc: Andrew Morton Cc: Kuniyuki Iwashima Cc: Mateusz Guzik Cc: Steven Rostedt Cc: John Ogness Cc: Sergey Senozhatsky --- lib/ratelimit.c | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/ratelimit.c b/lib/ratelimit.c index ab8472edeb1d..6a5cb0541301 100644 --- a/lib/ratelimit.c +++ b/lib/ratelimit.c @@ -40,6 +40,7 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func) * says always limit. */ if (interval <= 0 || burst <= 0) { + WARN_ONCE(interval < 0 || burst < 0, "Negative interval (%d) or burst (%d): Uninitialized ratelimit_state structure?\n", interval, burst); ret = interval == 0 || burst > 0; if (!(READ_ONCE(rs->flags) & RATELIMIT_INITIALIZED) || (!interval && !burst) || !raw_spin_trylock_irqsave(&rs->lock, flags)) { From f2d0ea0f086ae8811bb957ae2f781387557f80bd Mon Sep 17 00:00:00 2001 From: "Paul E. McKenney" Date: Thu, 24 Apr 2025 12:00:10 -0700 Subject: [PATCH 15/19] ratelimit: Simplify common-case exit path By making "ret" always be initialized, and moving the final call to ratelimit_state_inc_miss() out from under the lock, we save a goto and a couple lines of code. This also saves a couple of lines of code from the unconditional enable/disable slowpath. Link: https://lore.kernel.org/all/fbe93a52-365e-47fe-93a4-44a44547d601@paulmck-laptop/ Link: https://lore.kernel.org/all/20250423115409.3425-1-spasswolf@web.de/ Signed-off-by: Paul E. McKenney Reviewed-by: Petr Mladek Cc: Andrew Morton Cc: Kuniyuki Iwashima Cc: Mateusz Guzik Cc: Steven Rostedt Cc: John Ogness Cc: Sergey Senozhatsky --- lib/ratelimit.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/lib/ratelimit.c b/lib/ratelimit.c index 6a5cb0541301..7c6e864306db 100644 --- a/lib/ratelimit.c +++ b/lib/ratelimit.c @@ -33,7 +33,7 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func) int interval = READ_ONCE(rs->interval); int burst = READ_ONCE(rs->burst); unsigned long flags; - int ret; + int ret = 0; /* * Zero interval says never limit, otherwise, non-positive burst @@ -51,8 +51,6 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func) /* Force re-initialization once re-enabled. */ rs->flags &= ~RATELIMIT_INITIALIZED; - if (!ret) - ratelimit_state_inc_miss(rs); goto unlock_ret; } @@ -110,19 +108,17 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func) if (n_left > 0) { n_left = atomic_dec_return(&rs->rs_n_left); - if (n_left >= 0) { + if (n_left >= 0) ret = 1; - goto unlock_ret; - } } } - ratelimit_state_inc_miss(rs); - ret = 0; - unlock_ret: raw_spin_unlock_irqrestore(&rs->lock, flags); + if (!ret) + ratelimit_state_inc_miss(rs); + return ret; } EXPORT_SYMBOL(___ratelimit); From a69114c2a12c906957ddd3409af48f40e047ef53 Mon Sep 17 00:00:00 2001 From: "Paul E. McKenney" Date: Thu, 24 Apr 2025 12:06:43 -0700 Subject: [PATCH 16/19] ratelimit: Use nolock_ret label to save a couple of lines of code Create a nolock_ret label in order to start consolidating the unlocked return paths that conditionally invoke ratelimit_state_inc_miss(). Link: https://lore.kernel.org/all/fbe93a52-365e-47fe-93a4-44a44547d601@paulmck-laptop/ Link: https://lore.kernel.org/all/20250423115409.3425-1-spasswolf@web.de/ Signed-off-by: Paul E. McKenney Reviewed-by: Petr Mladek Cc: Andrew Morton Cc: Kuniyuki Iwashima Cc: Mateusz Guzik Cc: Steven Rostedt Cc: John Ogness Cc: Sergey Senozhatsky --- lib/ratelimit.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/ratelimit.c b/lib/ratelimit.c index 7c6e864306db..e7101a79c697 100644 --- a/lib/ratelimit.c +++ b/lib/ratelimit.c @@ -43,11 +43,8 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func) WARN_ONCE(interval < 0 || burst < 0, "Negative interval (%d) or burst (%d): Uninitialized ratelimit_state structure?\n", interval, burst); ret = interval == 0 || burst > 0; if (!(READ_ONCE(rs->flags) & RATELIMIT_INITIALIZED) || (!interval && !burst) || - !raw_spin_trylock_irqsave(&rs->lock, flags)) { - if (!ret) - ratelimit_state_inc_miss(rs); - return ret; - } + !raw_spin_trylock_irqsave(&rs->lock, flags)) + goto nolock_ret; /* Force re-initialization once re-enabled. */ rs->flags &= ~RATELIMIT_INITIALIZED; @@ -116,6 +113,7 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func) unlock_ret: raw_spin_unlock_irqrestore(&rs->lock, flags); +nolock_ret: if (!ret) ratelimit_state_inc_miss(rs); From 743a1942d52f6827d39d8f6745d006b5bb8bbe6b Mon Sep 17 00:00:00 2001 From: "Paul E. McKenney" Date: Thu, 24 Apr 2025 12:16:21 -0700 Subject: [PATCH 17/19] ratelimit: Use nolock_ret label to collapse lock-failure code Now that we have a nolock_ret label that handles ->missed correctly based on the value of ret, we can eliminate a local variable and collapse several "if" statements on the lock-acquisition-failure code path. Link: https://lore.kernel.org/all/fbe93a52-365e-47fe-93a4-44a44547d601@paulmck-laptop/ Link: https://lore.kernel.org/all/20250423115409.3425-1-spasswolf@web.de/ Signed-off-by: Paul E. McKenney Reviewed-by: Petr Mladek Cc: Andrew Morton Cc: Kuniyuki Iwashima Cc: Mateusz Guzik Cc: Steven Rostedt Cc: John Ogness Cc: Sergey Senozhatsky --- lib/ratelimit.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/lib/ratelimit.c b/lib/ratelimit.c index e7101a79c697..bcda7c61fc6f 100644 --- a/lib/ratelimit.c +++ b/lib/ratelimit.c @@ -58,20 +58,10 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func) * the current lock owner is just about to reset it. */ if (!raw_spin_trylock_irqsave(&rs->lock, flags)) { - unsigned int rs_flags = READ_ONCE(rs->flags); - - if (rs_flags & RATELIMIT_INITIALIZED && burst) { - int n_left = atomic_read(&rs->rs_n_left); - - if (n_left <= 0) - return 0; - n_left = atomic_dec_return(&rs->rs_n_left); - if (n_left >= 0) - return 1; - } - - ratelimit_state_inc_miss(rs); - return 0; + if (READ_ONCE(rs->flags) & RATELIMIT_INITIALIZED && burst && + atomic_read(&rs->rs_n_left) > 0 && atomic_dec_return(&rs->rs_n_left) >= 0) + ret = 1; + goto nolock_ret; } if (!(rs->flags & RATELIMIT_INITIALIZED)) { From 4b2cce999c8f5a03916b11b1e4ce192d223ab4f5 Mon Sep 17 00:00:00 2001 From: "Paul E. McKenney" Date: Thu, 24 Apr 2025 12:28:24 -0700 Subject: [PATCH 18/19] ratelimit: Use nolock_ret restructuring to collapse common case code Now that unlock_ret releases the lock, then falls into nolock_ret, which handles ->missed based on the value of ret, the common-case lock-held code can be collapsed into a single "if" statement with a single-statement "then" clause. Yes, we could go further and just assign the "if" condition to ret, but in the immortal words of MSDOS, "Are you sure?". Link: https://lore.kernel.org/all/fbe93a52-365e-47fe-93a4-44a44547d601@paulmck-laptop/ Link: https://lore.kernel.org/all/20250423115409.3425-1-spasswolf@web.de/ Signed-off-by: Paul E. McKenney Reviewed-by: Petr Mladek Cc: Andrew Morton Cc: Kuniyuki Iwashima Cc: Mateusz Guzik Cc: Steven Rostedt Cc: John Ogness Cc: Sergey Senozhatsky --- lib/ratelimit.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/lib/ratelimit.c b/lib/ratelimit.c index bcda7c61fc6f..dcc063af195e 100644 --- a/lib/ratelimit.c +++ b/lib/ratelimit.c @@ -88,17 +88,10 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func) } } } - if (burst) { - int n_left = atomic_read(&rs->rs_n_left); - /* The burst might have been taken by a parallel call. */ - - if (n_left > 0) { - n_left = atomic_dec_return(&rs->rs_n_left); - if (n_left >= 0) - ret = 1; - } - } + /* Note that the burst might be taken by a parallel call. */ + if (burst && atomic_read(&rs->rs_n_left) > 0 && atomic_dec_return(&rs->rs_n_left) >= 0) + ret = 1; unlock_ret: raw_spin_unlock_irqrestore(&rs->lock, flags); From ba575cea29fd82a0e6836fefcd51db36f1ff8a92 Mon Sep 17 00:00:00 2001 From: "Paul E. McKenney" Date: Thu, 24 Apr 2025 12:36:19 -0700 Subject: [PATCH 19/19] ratelimit: Drop redundant accesses to burst Now that there is the "burst <= 0" fastpath, for all later code, burst must be strictly greater than zero. Therefore, drop the redundant checks of this local variable. Link: https://lore.kernel.org/all/fbe93a52-365e-47fe-93a4-44a44547d601@paulmck-laptop/ Link: https://lore.kernel.org/all/20250423115409.3425-1-spasswolf@web.de/ Signed-off-by: Paul E. McKenney Reviewed-by: Petr Mladek Cc: Andrew Morton Cc: Kuniyuki Iwashima Cc: Mateusz Guzik Cc: Steven Rostedt Cc: John Ogness Cc: Sergey Senozhatsky --- lib/ratelimit.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/ratelimit.c b/lib/ratelimit.c index dcc063af195e..859c251b23ce 100644 --- a/lib/ratelimit.c +++ b/lib/ratelimit.c @@ -58,7 +58,7 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func) * the current lock owner is just about to reset it. */ if (!raw_spin_trylock_irqsave(&rs->lock, flags)) { - if (READ_ONCE(rs->flags) & RATELIMIT_INITIALIZED && burst && + if (READ_ONCE(rs->flags) & RATELIMIT_INITIALIZED && atomic_read(&rs->rs_n_left) > 0 && atomic_dec_return(&rs->rs_n_left) >= 0) ret = 1; goto nolock_ret; @@ -90,7 +90,7 @@ int ___ratelimit(struct ratelimit_state *rs, const char *func) } /* Note that the burst might be taken by a parallel call. */ - if (burst && atomic_read(&rs->rs_n_left) > 0 && atomic_dec_return(&rs->rs_n_left) >= 0) + if (atomic_read(&rs->rs_n_left) > 0 && atomic_dec_return(&rs->rs_n_left) >= 0) ret = 1; unlock_ret: