[PATCH] serial8250: ratelimit "too much work" error

[PATCH] serial8250: ratelimit "too much work" error

[Daniel Drake]

Running a serial console, if too many kernel messages are generated within
a short time causing a lot of serial I/O, the 8250 driver will generate
another kernel message reporting this, which just adds to the I/O. It has
a cascading effect and quickly results the system being brought to its knees
by a flood of "too much work" messages.

Ratelimit the error message to avoid this.

Signed-off-by: Daniel Drake <dsd@laptop.org>
---
 drivers/serial/8250.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 24110f6..d3c5855 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -1606,8 +1606,9 @@ static irqreturn_t serial8250_interrupt(int irq, void *dev_id)
 
 		if (l == i->head && pass_counter++ > PASS_LIMIT) {
 			/* If we hit this, we're dead. */
-			printk(KERN_ERR "serial8250: too much work for "
-				"irq%d\n", irq);
+			if (printk_ratelimit())
+				printk(KERN_ERR "serial8250: too much work for "
+					"irq%d\n", irq);
 			break;
 		}
 	} while (l != end);
-- 
1.7.2.3

Re: [PATCH] serial8250: ratelimit "too much work" error

[Alan Cox]

On Sat,  2 Oct 2010 12:04:38 +0100 (BST)
Daniel Drake <dsd@laptop.org> wrote:

> Running a serial console, if too many kernel messages are generated within
> a short time causing a lot of serial I/O, the 8250 driver will generate
> another kernel message reporting this, which just adds to the I/O. It has
> a cascading effect and quickly results the system being brought to its knees
> by a flood of "too much work" messages.
> 
> Ratelimit the error message to avoid this.
> 
> Signed-off-by: Daniel Drake <dsd@laptop.org>

Acked-by: Alan Cox <alan@linux.intel.com>

Re: [PATCH] serial8250: ratelimit "too much work" error

[Andrew Morton]

On Sat,  2 Oct 2010 12:04:38 +0100 (BST)
Daniel Drake <dsd@laptop.org> wrote:

> Running a serial console, if too many kernel messages are generated within
> a short time causing a lot of serial I/O, the 8250 driver will generate
> another kernel message reporting this, which just adds to the I/O. It has
> a cascading effect and quickly results the system being brought to its knees
> by a flood of "too much work" messages.
> 
> Ratelimit the error message to avoid this.
> 
> Signed-off-by: Daniel Drake <dsd@laptop.org>
> ---
>  drivers/serial/8250.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
> index 24110f6..d3c5855 100644
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -1606,8 +1606,9 @@ static irqreturn_t serial8250_interrupt(int irq, void *dev_id)
>  
>  		if (l == i->head && pass_counter++ > PASS_LIMIT) {
>  			/* If we hit this, we're dead. */
> -			printk(KERN_ERR "serial8250: too much work for "
> -				"irq%d\n", irq);
> +			if (printk_ratelimit())
> +				printk(KERN_ERR "serial8250: too much work for "
> +					"irq%d\n", irq);
>  			break;
>  		}
>  	} while (l != end);

printk_ratelimit() shares a common ratelimiting state between all
callers of printk_ratelimit().  This is pretty sucky because if one
printk_ratelimit() caller is going crazy then this can cause punishment
of other unrelated printk_ratelimit() callers who *aren't* going crazy.

So it's generally better to use printk_ratelimited(), which will
ratelimit this printkand no other printk, without affecting other
printk_ratelimit[ed]() users.

So, this:

--- a/drivers/serial/8250.c~serial8250-ratelimit-too-much-work-error-fix
+++ a/drivers/serial/8250.c
@@ -1606,9 +1606,8 @@ static irqreturn_t serial8250_interrupt(
 
 		if (l == i->head && pass_counter++ > PASS_LIMIT) {
 			/* If we hit this, we're dead. */
-			if (printk_ratelimit())
-				printk(KERN_ERR "serial8250: too much work for "
-					"irq%d\n", irq);
+			printk_ratelimited(KERN_ERR
+				"serial8250: too much work for irq%d\n", irq);
 			break;
 		}
 	} while (l != end);
_

which, interestingly, doesn't compile because someone stuck a
DEFINE_RATELIMIT_STATE() in include/linux/kernel.h and it ain't defined
anywhere.  Let me fix that up...

Re: [PATCH] serial8250: ratelimit "too much work" error

[Andrew Morton]

On Mon, 4 Oct 2010 14:51:01 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> someone stuck a
> DEFINE_RATELIMIT_STATE() in include/linux/kernel.h and it ain't defined
> anywhere.  Let me fix that up...

Well that's a PITA.  Can't include ratelimit.h into kernel.h because a)
it'll slow everyone's compiels down and b) ratelimit.h needs spinlock.h
which surely needs kernel.h.  Fixable by adding a new
printk_ratelimit.h and including that from 135 source files, blah.

I'll give up and pronounce that users of printk_ratelimited() need to
include ratelimit.h as well.

Re: [PATCH] serial8250: ratelimit "too much work" error

[Joe Perches]

On Mon, 2010-10-04 at 15:02 -0700, Andrew Morton wrote:
> On Mon, 4 Oct 2010 14:51:01 -0700
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > someone stuck a
> > DEFINE_RATELIMIT_STATE() in include/linux/kernel.h and it ain't defined
> > anywhere.  Let me fix that up...
> 
> Well that's a PITA.  Can't include ratelimit.h into kernel.h because a)
> it'll slow everyone's compiels down and b) ratelimit.h needs spinlock.h
> which surely needs kernel.h.  Fixable by adding a new
> printk_ratelimit.h and including that from 135 source files, blah.
> 
> I'll give up and pronounce that users of printk_ratelimited() need to
> include ratelimit.h as well.

What I suggested several months ago was to move the
macro definitions to ratelimit.h

http://lkml.org/lkml/2010/2/18/377

Re: [PATCH] serial8250: ratelimit "too much work" error

[Andrew Morton]

On Mon, 04 Oct 2010 15:10:59 -0700
Joe Perches <joe@perches.com> wrote:

> On Mon, 2010-10-04 at 15:02 -0700, Andrew Morton wrote:
> > On Mon, 4 Oct 2010 14:51:01 -0700
> > Andrew Morton <akpm@linux-foundation.org> wrote:
> > 
> > > someone stuck a
> > > DEFINE_RATELIMIT_STATE() in include/linux/kernel.h and it ain't defined
> > > anywhere.  Let me fix that up...
> > 
> > Well that's a PITA.  Can't include ratelimit.h into kernel.h because a)
> > it'll slow everyone's compiels down and b) ratelimit.h needs spinlock.h
> > which surely needs kernel.h.  Fixable by adding a new
> > printk_ratelimit.h and including that from 135 source files, blah.
> > 
> > I'll give up and pronounce that users of printk_ratelimited() need to
> > include ratelimit.h as well.
> 
> What I suggested several months ago was to move the
> macro definitions to ratelimit.h
> 
> http://lkml.org/lkml/2010/2/18/377
> 

That's a bit nasty because at present ratelimit.h is purely about
ratelimiting and knowns nothing about any of its clients.  At present
it has only one client (printk), but it could have more in the future!

Re: [PATCH] serial8250: ratelimit "too much work" error

[Joe Perches]

On Mon, 2010-10-04 at 15:21 -0700, Andrew Morton wrote:
> On Mon, 04 Oct 2010 15:10:59 -0700
> Joe Perches <joe@perches.com> wrote:
> > On Mon, 2010-10-04 at 15:02 -0700, Andrew Morton wrote:
> > > On Mon, 4 Oct 2010 14:51:01 -0700
> > > Andrew Morton <akpm@linux-foundation.org> wrote:
> > > I'll give up and pronounce that users of printk_ratelimited() need to
> > > include ratelimit.h as well.
> > What I suggested several months ago was to move the
> > macro definitions to ratelimit.h
> > http://lkml.org/lkml/2010/2/18/377
> That's a bit nasty because at present ratelimit.h is purely about
> ratelimiting and knowns nothing about any of its clients.  At present
> it has only one client (printk), but it could have more in the future!

Look at the structure, it's very specific to
message logging functionality.

struct ratelimit_state {
	spinlock_t	lock;		/* protect the state */

	int		interval;
	int		burst;
	int		printed;
	int		missed;
	unsigned long	begin;
};

I think it's likely that the current ratelimit
will not be used for any other function.

Re: [PATCH] serial8250: ratelimit "too much work" error

[Andrew Morton]

On Mon, 04 Oct 2010 15:59:29 -0700
Joe Perches <joe@perches.com> wrote:

> On Mon, 2010-10-04 at 15:21 -0700, Andrew Morton wrote:
> > On Mon, 04 Oct 2010 15:10:59 -0700
> > Joe Perches <joe@perches.com> wrote:
> > > On Mon, 2010-10-04 at 15:02 -0700, Andrew Morton wrote:
> > > > On Mon, 4 Oct 2010 14:51:01 -0700
> > > > Andrew Morton <akpm@linux-foundation.org> wrote:
> > > > I'll give up and pronounce that users of printk_ratelimited() need to
> > > > include ratelimit.h as well.
> > > What I suggested several months ago was to move the
> > > macro definitions to ratelimit.h
> > > http://lkml.org/lkml/2010/2/18/377
> > That's a bit nasty because at present ratelimit.h is purely about
> > ratelimiting and knowns nothing about any of its clients.  At present
> > it has only one client (printk), but it could have more in the future!
> 
> Look at the structure, it's very specific to
> message logging functionality.
> 
> struct ratelimit_state {
> 	spinlock_t	lock;		/* protect the state */
> 
> 	int		interval;
> 	int		burst;
> 	int		printed;
> 	int		missed;
> 	unsigned long	begin;
> };

s/printed/hit/there,fixed

This is at present a quite general facility.

> I think it's likely that the current ratelimit
> will not be used for any other function.

Filling it up with printk-specific stuff will help ensure that.

Re: [PATCH] serial8250: ratelimit "too much work" error

[Joe Perches]

On Mon, 2010-10-04 at 16:11 -0700, Andrew Morton wrote:
> On Mon, 04 Oct 2010 15:59:29 -0700
> Joe Perches <joe@perches.com> wrote:
> > On Mon, 2010-10-04 at 15:21 -0700, Andrew Morton wrote:
> > > On Mon, 04 Oct 2010 15:10:59 -0700
> > > Joe Perches <joe@perches.com> wrote:
> > > > On Mon, 2010-10-04 at 15:02 -0700, Andrew Morton wrote:
> > > > > On Mon, 4 Oct 2010 14:51:01 -0700
> > > > > Andrew Morton <akpm@linux-foundation.org> wrote:
> > > > > I'll give up and pronounce that users of printk_ratelimited() need to
> > > > > include ratelimit.h as well.
> > > > What I suggested several months ago was to move the
> > > > macro definitions to ratelimit.h
> > > > http://lkml.org/lkml/2010/2/18/377
> > > That's a bit nasty because at present ratelimit.h is purely about
> > > ratelimiting and knowns nothing about any of its clients.  At present
> > > it has only one client (printk), but it could have more in the future!
> > 
> > Look at the structure, it's very specific to
> > message logging functionality.
> > 
> > struct ratelimit_state {
> > 	spinlock_t	lock;		/* protect the state */
> > 
> > 	int		interval;
> > 	int		burst;
> > 	int		printed;
> > 	int		missed;
> > 	unsigned long	begin;
> > };
> 
> s/printed/hit/there,fixed
> 
> This is at present a quite general facility.
> 
> > I think it's likely that the current ratelimit
> > will not be used for any other function.
> 
> Filling it up with printk-specific stuff will help ensure that.

Interval too because it is in seconds and likely
should be in timespec or jiffies.

For what other facility could you see ratelimit_state
be used for?

Putting the printk specific uses in the .h file
would make sure that the users of ratelimit use
the proper file and reduce the #include dependencies.

Re: [PATCH] serial8250: ratelimit "too much work" error

[Andrew Morton]

On Mon, 04 Oct 2010 16:34:05 -0700
Joe Perches <joe@perches.com> wrote:

> On Mon, 2010-10-04 at 16:11 -0700, Andrew Morton wrote:
> > On Mon, 04 Oct 2010 15:59:29 -0700
> > Joe Perches <joe@perches.com> wrote:
> > > On Mon, 2010-10-04 at 15:21 -0700, Andrew Morton wrote:
> > > > On Mon, 04 Oct 2010 15:10:59 -0700
> > > > Joe Perches <joe@perches.com> wrote:
> > > > > On Mon, 2010-10-04 at 15:02 -0700, Andrew Morton wrote:
> > > > > > On Mon, 4 Oct 2010 14:51:01 -0700
> > > > > > Andrew Morton <akpm@linux-foundation.org> wrote:
> > > > > > I'll give up and pronounce that users of printk_ratelimited() need to
> > > > > > include ratelimit.h as well.
> > > > > What I suggested several months ago was to move the
> > > > > macro definitions to ratelimit.h
> > > > > http://lkml.org/lkml/2010/2/18/377
> > > > That's a bit nasty because at present ratelimit.h is purely about
> > > > ratelimiting and knowns nothing about any of its clients.  At present
> > > > it has only one client (printk), but it could have more in the future!
> > > 
> > > Look at the structure, it's very specific to
> > > message logging functionality.
> > > 
> > > struct ratelimit_state {
> > > 	spinlock_t	lock;		/* protect the state */
> > > 
> > > 	int		interval;
> > > 	int		burst;
> > > 	int		printed;
> > > 	int		missed;
> > > 	unsigned long	begin;
> > > };
> > 
> > s/printed/hit/there,fixed
> > 
> > This is at present a quite general facility.
> > 
> > > I think it's likely that the current ratelimit
> > > will not be used for any other function.
> > 
> > Filling it up with printk-specific stuff will help ensure that.
> 
> Interval too because it is in seconds and likely
> should be in timespec or jiffies.

It might need changes when adapted to additional uses.  These things
happen.

> For what other facility could you see ratelimit_state
> be used for?

Gee I dunno.  Sending occasional packets of accumulated counters up to
userspace via netlink?  Who knows, people do all sorts of things.
I bet there's code in the kernel right now which could use this.

Look at the file!  It all does one thing.  It encapsulates a single
concept.  It's simply a bad thing to add single-concept
application-specific material into that.  Layering, and all that.

> Putting the printk specific uses in the .h file
> would make sure that the users of ratelimit use
> the proper file and reduce the #include dependencies.

I know that.  It's the first thing I thought of, before deciding that
it would be a poor thing to do.

Re: [PATCH] serial8250: ratelimit "too much work" error

[Joe Perches]

On Mon, 2010-10-04 at 16:49 -0700, Andrew Morton wrote:
> Look at the file!  It all does one thing.  It encapsulates a single
> concept.  It's simply a bad thing to add single-concept
> application-specific material into that.  Layering, and all that.

Take a look at kernel.h recently?