Date:	Thu, 3 Sep 2015 08:30:47 +0000
From:	Kolmakov Dmitriy <kolmakov.dmitriy@...wei.com>
To:	David Miller <davem@...emloft.net>
CC:	"jon.maloy@...csson.com" <jon.maloy@...csson.com>,
	"ying.xue@...driver.com" <ying.xue@...driver.com>,
	"tipc-discussion@...ts.sourceforge.net" 
	<tipc-discussion@...ts.sourceforge.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] tipc: fix stall during bclink wakeup procedure

From: David Miller [mailto:davem@...emloft.net]
> 
> From: Kolmakov Dmitriy <kolmakov.dmitriy@...wei.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.

> 
> 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists