RE: [PATCH] tipc: fix stall during bclink wakeup procedure

[Jon Maloy <jon.maloy@ericsson.com>]

> -----Original Message-----
> From: Kolmakov Dmitriy [mailto:kolmakov.dmitriy@huawei.com]
> Sent: Thursday, 03 September, 2015 04:31
> To: David Miller
> Cc: Jon Maloy; Ying Xue; tipc-discussion@lists.sourceforge.net;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH] tipc: fix stall during bclink wakeup procedure
> 
> From: David Miller [mailto:davem@davemloft.net]
> >
> > From: Kolmakov Dmitriy <kolmakov.dmitriy@huawei.com>
> > Date: Wed, 2 Sep 2015 15:33:00 +0000
> >
> > > If an attempt to wake up users of broadcast link is made when there
> > is
> > > no enough place in send queue than it may hang up inside the
> > > tipc_sk_rcv() function since the loop breaks only after the wake up
> > > queue becomes empty. This can lead to complete CPU stall with the
> > > following message generated by RCU:
> >
> > I don't understand how it can loop forever.
> >
> > It should either successfully deliver each packet to the socket, or
> > respond with a TIPC_ERR_OVERLOAD.
> >
> > In both cases, the SKB is dequeued from the queue and forward progress
> > is made.
> 
> The issue occurs only when tipc_sk_rcv() is used to wake up postponed
> senders. In this case the call stack is following:
> 
> 	tipc_bclink_wakeup_users()
> 		// wakeupq - is a queue consist of special
> 		// 		 messages with SOCK_WAKEUP type.
> 		tipc_sk_rcv(wakeupq)
> 			...
> 			while (skb_queue_len(inputq)) {
> 				filter_rcv(skb)
> 					// Here the type of message is
> checked
> 					// and if it is SOCK_WAKEUP than
> 					// it tries to wake up a sender.
> 					tipc_write_space(sk)
> 
> 	wake_up_interruptible_sync_poll()
> 			}
> 
> After the sender thread is woke up it can gather control and perform an
> attempt to send a message. But if there is no enough place in send queue it
> will call link_schedule_user() function which puts a message of type
> SOCK_WAKEUP to the wakeup queue and put the sender to sleep. Thus the
> size of the queue actually is not changed and the while() loop never exits.
> 
> The approach I proposed is to wake up only senders for which there is
> enough place in send queue so the described issue can't occur. Moreover
> the same approach is already used to wake up senders on unicast links so it
> was possible to reuse existed code.

The issue seems real, but I don't feel comfortable with your solution, because
of its possible effect on the unicast code. I would prefer that you write a separate
 "prepare_wakeup()" inside bcast.c, and uses that one.  Then, post it to "net" so it 
gets applied as far back as possible.

You may not like the code duplication this entails, but it is only temporary.  I am
just doing the final testing on a series that basically redesigns the broadcast 
implementation, and where this issue is inherently solved.  This will be delivered
when net-next re-opens.

///jon

> 
> >
> > If there really is a problem somewhere in here, then two things:
> >
> > 1) You need to describe exactly the sequence of tests and conditions
> >    that lead to the endless loop in this code, because I cannot see
> >    it.
> 
> I have got into the issue on our product code but to reproduce the issue I
> changed a benchmark test application (from tipcutils/demos/benchmark) to
> perform the following scenario:
> 	1. Run 64 instances of test application (nodes). It can be done on the
> one physical machine.
> 	2. Each application connects to all other using TIPC sockets in RDM
> mode.
> 	3. When setup is done all nodes start simultaneously send broadcast
> messages.
> 	4. Everything hangs up.
> 
> The issue is reproducible only when a congestion on broadcast link occurs.
> For example, when there are only 8 nodes it works fine since congestion
> doesn't occur. Send queue limit is 40 in my case (I use a critical importance
> level) and when 64 nodes send a message at the same moment a congestion
> occurs every time.
> 
> >
> > 2) I suspect the fix is more likely to be appropriate in tipc_sk_rcv()
> >    or similar, rather than creating a dummy queue to workaround it's
> >    behavior.
> >
> > Thanks.
> 
> BR,
> DK