Re: [REGRESSION] RLIMIT_DATA crashes named

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 1548 bytes --]

On Fri, Sep 16, 2016 at 3:30 PM, Sam Varshavchik <mrsam@courier-mta.com> wrote:
>>
>> Sam, do you end up seeing the kernel warning in your logs if you just
>> go back earlier in the boot?
>
> Yes, I found it.
>
> Sep 10 07:36:29 shorty kernel: mmap: named (1108): VmData 52588544 exceed
> data ulimit 20971520. Update limits or use boot option ignore_rlimit_data.
>
> Now that I know what to search for: this appeared about 300 lines earlier in
> /var/log/messages.

Ok, so that's a pretty strong argument that we shouldn't just warn once.

Maybe the warning happened at bootup, and it is now three months
later, and somebody notices that something doesn't work. It might not
be *critical* (three months without working implies it isn't), but it
sure is silly for the kernel to say "yeah, I already warned you, I'm
not going to tell you why it's not working any more".

So it sounds like if the kernel had just had a that warning be
rate-limited instead of happening only once, there would never have
been any confusion about the RLIMIT_DATA change.

Doing a grep for "pr_warn_once()", I get the feeling that we could
just change the definition of "once" to be "at most once per minute"
and everybody would be happy.

Maybe we could change all the "pr_xyz_once()" to consider "once" to be
a softer "at most once per minute" thing. After all, these things are
*supposed* to be very uncommon to begin with, but when they do happen
we do want the user to be aware of them.

Here's a totally untested patch. What do people say?

                 Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 1579 bytes --]

 include/linux/printk.h | 38 +++++++++++++++-----------------------
 1 file changed, 15 insertions(+), 23 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index 696a56be7d3e..ae98c388a377 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -316,31 +316,23 @@ extern asmlinkage void dump_stack(void) __cold;
 
 /*
  * Print a one-time message (analogous to WARN_ONCE() et al):
+ *
+ * "once" here is a misnomer. It's shorthand for "at most once a minute".
  */
-
 #ifdef CONFIG_PRINTK
-#define printk_once(fmt, ...)					\
-({								\
-	static bool __print_once __read_mostly;			\
-	bool __ret_print_once = !__print_once;			\
-								\
-	if (!__print_once) {					\
-		__print_once = true;				\
-		printk(fmt, ##__VA_ARGS__);			\
-	}							\
-	unlikely(__ret_print_once);				\
-})
-#define printk_deferred_once(fmt, ...)				\
-({								\
-	static bool __print_once __read_mostly;			\
-	bool __ret_print_once = !__print_once;			\
-								\
-	if (!__print_once) {					\
-		__print_once = true;				\
-		printk_deferred(fmt, ##__VA_ARGS__);		\
-	}							\
-	unlikely(__ret_print_once);				\
-})
+
+#define do_just_once(stmt)	({			\
+	static DEFINE_RATELIMIT_STATE(_rs, HZ*60, 1);	\
+	bool __do_it = __ratelimit(&_rs);		\
+	if (unlikely(__do_it))				\
+		stmt;					\
+	unlikely(__do_it); })
+
+#define printk_once(fmt, ...) \
+	do_just_once(printk(fmt, ##__VA_ARGS__))
+#define printk_deferred_once(fmt, ...) \
+	do_just_once(printk_deferred(fmt, ##__VA_ARGS__))
+
 #else
 #define printk_once(fmt, ...)					\
 	no_printk(fmt, ##__VA_ARGS__)