Re: [PATCH 2/2] ipoib: Ratelimit messages which can flood a host.

[Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

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

On Tue, 2016-07-26 at 21:00 +0300, Yuval Shaia wrote:
> On Tue, Jul 26, 2016 at 05:38:49PM +0300, Nikolay Borisov wrote:
> > 
> > In case an infiniband outtage occurs the messages modified
> > in this patchset can flood the host with a rate of 1 message per
> 
> s/patchset/patch
> 
> > 
> > ms which is a lot. Using the ratelimited version of ipoib_warn
> > fixes the issue by limiting at most 10 messages in 5 seconds.
> > 
> > Signed-off-by: Nikolay Borisov <kernel-6AxghH7DbtA@public.gmane.org>
> > ---
> >  drivers/infiniband/ulp/ipoib/ipoib_main.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > index 7d3281866ffc..dd5b4afbc00b 100644
> > --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > @@ -1034,9 +1034,9 @@ static void ipoib_timeout(struct net_device
> > *dev)
> >  {
> >  	struct ipoib_dev_priv *priv = netdev_priv(dev);
> >  
> > -	ipoib_warn(priv, "transmit timeout: latency %d msecs\n",
> > +	ipoib_warn_rl(priv, "transmit timeout: latency %d
> > msecs\n",
> >  		   jiffies_to_msecs(jiffies - dev->trans_start));
> > -	ipoib_warn(priv, "queue stopped %d, tx_head %u, tx_tail
> > %u\n",
> > +	ipoib_warn_rl(priv, "queue stopped %d, tx_head %u, tx_tail
> > %u\n",
> 
> So this is just to hide the hide the problem,

No, that's not true.  To hide the problem, he would have to remove the
printk entirely.  Linus absolutely despises when programmers use
BUG_ON() for situations where it is possible for the system to
continue, albeit with reduced capability or what not.  Similarly, it is
entirely unacceptable that a simple issue, such as a hardware queue
stoppage on a network device, can render a system mostly or entirely or
even just somewhat unusable due to a flood of kernel printk messages.

Just so we're clear, I absolutely despise printk floods.  There is
never a valid reason for them.  If someone in the kernel community
suggested that we make the default printk implementation be flood
proof, I would support it.  In the meantime, I'm all for these patches.
 And if there are other places in the ipoib code that can cause floods
too, I would like to see them switched over to use the rate limited
version too.

Now that I think about it, I'm just as likely to be receptive to a
patch that makes the default ipoib_warn() always be rate limited, and
if there are exceptions in the ipoib stack where we truly want every
message, then change just those places to a different
ipoib_warn_no_rate_limit() implementation.  I would probably modify the
default rate though...10 in 5 seconds is probably too low for some of
the messages, as we can legitimately get a burst of 50 in less than a
second when doing things like joining lots of multicast groups and
there is an issue, or something like that.  Maybe 100 in 60 to allow
for bursts but to still prevent runaway printks like you are having?

>  the real question is what is
> causing this to happen a lot.
> 
> > 
> >  		   netif_queue_stopped(dev),
> >  		   priv->tx_head, priv->tx_tail);
> >  	/* XXX reset QP, etc. */
> > -- 
> > 2.5.0
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-
> > rdma" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]