ldc_connect() should not return EINVAL when handshake is in progress.

The LDC handshake could have been asynchronously triggered after ldc_bind() enables the ldc_rx() receive interrupt-handler (and thus intercepts incoming control packets)
and before vio_port_up() calls ldc_connect(). If that is the case, ldc_connect() should return 0 and let the state-machine progress.

---
Resending to the correct list this time.

 arch/sparc/kernel/ldc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index e01d75d..66dacd5 100644
@@ -1336,7 +1336,7 @@ int ldc_connect(struct ldc_channel *lp)
 	if (!(lp->flags & LDC_FLAG_ALLOCED_QUEUES) ||
 	    !(lp->flags & LDC_FLAG_REGISTERED_QUEUES) ||
 	    lp->hs_state != LDC_HS_OPEN)
-		err = -EINVAL;
+		err = ((lp->hs_state > LDC_HS_OPEN) ? 0 : -EINVAL);
 	else
 		err = start_handshake(lp);
 

1.8.4.2

From: Sowmini Varadhan <[email protected]>
Date: Fri, 1 Aug 2014 09:50:40 -0400

Applied and queued up for -stable, thanks.

Please, in the future, format your Subject line something like the following:

[PATCH] $subsystem: $description

Everything after "[xxx] " goes into the commit log header line, and putting a subsystem prefix allows people scanning the shortlog to get a good idea what area a change touches.

In this case I added "sparc64: " for the subsystem prefix when commiting your change.

BTW, you'll probably come to notice that not a lot of work has gone into handling hard disconnect failures properly in LDC and the drivers that use it. It's the one area where the LDC framework is very weak. Unfortunately a lot of cooperation is required by the drivers themselves, because what to do when a LDC connection goes down is different in different drivers (networking drivers can drop packets, but block drivers have to redo the I/O when the LDC link comes back up, for example)

Thanks, I'll keep that in mind for the future- I wasnt sure of the taxonomy for ldc.

Noted - I've not run into this personally, but we'll add it to the list.

FWIW, what I'm trying to sort out at the moment is the potential for triggering a soft lockup on a peer (i.e, a DoS vector).
If a node sends a burst of data and then never drains its incoming ldc channel, the targetted peer will eventually encounter a tx_has_no_space_for()/EWOULDBLOCK from __vnet_tx_trigger/vnet_send_ack (and also __vdc_tx_trigger?) - all of these are infinite loops, and vnet_send_ack is particularly vicious because it's actually a paradoxical blocking Tx in the Rx path (due to the vnet protocol design, I suppose).

Carefully asserting netif_stop_queue() for __vnet_tx_trigger() might possibly help that case (though it flow-controls all ldc_channels, instead of just the blocked one), but recovering gracefully for the other cases needs thought.

--Sowmini

From: Sowmini Varadhan <[email protected]>
Date: Tue, 5 Aug 2014 09:52:38 -0400

Other virtualization drivers tend to have this issue. In fact this was just being discussed over the past week on the netdev list wrt. the xen-netback driver.

The scheme used there basically is to mark the carrier off when the peer stops sinking packets we are TX'ing to it, and queue up a timer.

If the timer fires, we free up all of the packets in the transmit queue so they don't linger forever and take up resources.

Usually there is some "event" that can let us know that data is being sinked again, and we can use that to turn the carrier on and start transmitting again. Otherwise we'll have to poll for such things in the timer.

(Maybe this discussion belongs in netdev?)

so in the case of sunvnet, there are 2 writes that can block- either !vnet_tx_dring_avail() (i.e., no descriptor rings) , or !tx_has_space_for() (i.e., cannot send the ldc trigger)

vnet_start_xmit() already seems to have most of the smarts to handle both failures, the one missing piece is that __vnet_tx_trigger() should not loop infinitely on the ldc_write, but give up with EWOULDBLOCK after some finite number of tries- at that point vnet_start_xmit() recovers correctly (afaict).

But then there's another problem at the tail of vnet_event()- if we hit the maybe_tx_wakeup() condition, we try to take the netif_tx_lock() in the recv-interrupt-context and can deadlock with dev_watchdog().

I could move the contents of maybe_tx_wakeup() into vnet_tx_timeout(), and that avoids deadlocks. However I think that now matches the description of polling-to-recover-from-blocked-tx above. And one side-effect of moving from the synchronous maybe_tx_wakeup() to the reliance on watchdog-timer to unblock things is that things like iperf throughput can fluctuate wildly. But given that this actually *is* a performance weakness, perhaps its not worth over-optimizing this case?

Another data-point in favor of not over-optimizing the wrong thing:
in separate experiments, when I try to move the data-handling code in vnet_event off to a bh-handler, performance improves noticeably- I rarely encounter the netif_queue_stopped(), so relying on vnet_tx_timeout() for recovery is not an issue with that prototype.

Unless there's a simple way to kick off netif_wake_queue() from vnet_event(), it seems better to just make vnet_tx_timeout() do this job.

--Sowmini

From: Sowmini Varadhan <[email protected]>
Date: Thu, 7 Aug 2014 16:17:08 -0400

Actually, if we look at the vio_ldc_send() code path, it does it's own delay loop waiting for ldc_write() to stop signalling -EAGAIN. It loops up to 1000 times, doing a udelay(1) each iteration.

So firstly the backoff based delay loop in __vnet_tx_trigger() should probably be removed entirely.

Now, besides taking the error path in vnet_start_xmit() and dropping the packet, we need to think about what we should do subsequently when this condition triggers.

We probably want to take the carrier down via netif_carrier_off(), and then when we see the head pointer advancing (I guess via vnet_event()'s logic), the networking driver can netif_carrir_on() in response.

Indeed, that's a bad deadlock.

You can just use a tasklet to make it execute in a software interrupt "right now".

I think just the time it takes to unwind the HW interrupt handler and enter the software interrupt code path is enough time to let the other side of the LDC link make a bunch of progresss.

And yes, hitting stop queue conditions less often will improve performance if only because it means we have less overhead going in and out of stopped queue state.

Anyways, see my tasklet suggestion above.

Also it's a real pain that one port backing up can head-of-line block the entire interface.

Aren't we talking about one ldc port (out of many) for the device here? I'm not sure we want to drop carrier if only one port isn't making progress, unless maybe that's the only switch port.

						+-DLS

From: David L Stevens <[email protected]>
Date: Fri, 08 Aug 2014 09:55:55 -0400

Right now we already are stopping the entire TX queue of the device if just one port backs up.

Effectively the link is down at that point, and as long as it persists no traffic is going to pass over the device.

The problem with not taking the carrier off is that the qdisc layer is going to spam messages into the log each time the TX watchdog timer goes off.