sunvnet: Schedule maybe_tx_wakeup as a tasklet from ldc_rx path

drivers/net/ethernet/sun/sunvnet.c +2 -1

@@ -489,28 +491,13 @@ static int handle_mcast(struct vnet_port *port, void *msgbuf)
 	return 0;
 }
 
-static void maybe_tx_wakeup(struct vnet *vp)
+static void maybe_tx_wakeup(unsigned long param)
 {
+	struct vnet *vp = (struct vnet *)param;
 	struct net_device *dev = vp->dev;
 

Please do not usurp vnet_tx_timeout() to handle queue waking.

It is hooked up to netdev_ops->ndo_tx_timeout() which is invoked when the qdisc watchdog triggers after dev->watchdog_timeout time elapses after the queue is stopped without a subsequent wake.

This method is a therefore executed after a hard timeout, meaning that the device should probably be reset, whereas maybe_tx_wakeup() is normal processing meant to drive the engine of TX flow control.

In the context of sunvnet, vnet_tx_timeout() should probably reset the LDC channel(s) of the unresponsive peer(s).

Quoted 11 lines

Please do not usurp vnet_tx_timeout() to handle queue waking.

It is hooked up to netdev_ops->ndo_tx_timeout() which is invoked when the qdisc watchdog triggers after dev->watchdog_timeout time elapses after the queue is stopped without a subsequent wake.

This method is a therefore executed after a hard timeout, meaning that the device should probably be reset, whereas maybe_tx_wakeup() is normal processing meant to drive the engine of TX flow control.

Quoted 2 lines

In the context of sunvnet, vnet_tx_timeout() should probably reset the LDC channel(s) of the unresponsive peer(s).

yes, after a couple of days of reflection on the lingering issues in
http://www.spinics.net/lists/sparclinux/msg12360.html
I too was starting to arrive at similar conclusions about maybe_tx_wakeup vs vnet_tx_timeout.

First case 2 in the "todo" list (cannot send DRING_STOPPED):
If we cannot send a vnet_send_ack() we should invoke ldc_disconnect() for this peer. The ldc_disconnect() will reset hs_state and flags in the ldc_channel, so subsequent attempts to ldc_write() (e.g., from vio_ldc_send()) will return -ENOTCONN. So, for that case, a subsequent vnet_start_xmit() would *not* netif_stop_queue(), but just drop the packet, reset the dring, and keep driving.

To recover the disconnected channel, the admin would need to (ldm) unbind/bind the offender, identifiable by their mac address. And none of the other channels shold be affected (assuming inter-vnet-links is set to on)

So for case 1 we could do something very similar- I haven't tried this yet, but here's what I'm thinking: vnet_start_xmit() should not do a netif_stop_queue. Instead it should do a (new function) "vio_stop_queue()" which sets some (new) VIO_DR_FLOW_CONTROLLED state in the vio_driver_state, (or perhaps some flag bit in the ldc_channel?) and also marks a (new) boolean is_flow_controlled state variable in the struct vnet as TRUE.

The modified maybe_tx_wakeup would check is_flow_controlled on the vnet, and call a new vnet_wake_queue() to reset VIO_DR_FLOW_CONTROLLED if appropriate.

vnet_start_xmit should drop packets as long as VIO_DR_FLOW_CONTROLLED is set.

I dont know if there is a way to leverage that path and still avoid the head-of-line blocking due to one flow-controlled ldc_channel.

However, don't get ahead of yourself. All I'm saying in the context of reviewing this patch #3 is that you should leave vnet_tx_timeout() alone and just put what you were putting into vnet_tx_timeout() into maybe_tx_wakeup().

This doesn't match my understanding. My understanding is that when
an LDC connection resets, the peer on the other side should automatically try to re-bind or whatever is necessary to re-establish the LDC link.

It's supposed to be fault tolerant in as many situations as possible without the need for explicit adminstrator intervention.

But besides that, an LDC reset is a big hammer. So I would say that it should be deferred to the last possible point.

So initially we do the backoff spinning loop synchronously. If that doesn't succeed, we schedule a workqueue that can poll the LDC channel some more trying to do the write, in a sleepable context (so a loop with delays and preemption points) until a much larger timeout. Only at this second timeout do we declare things to be serious enough for an LDC channel reset.

Quoted 12 lines

So for case 1 we could do something very similar- I haven't tried this yet, but here's what I'm thinking: vnet_start_xmit() should not do a netif_stop_queue. Instead it should do a (new function) "vio_stop_queue()" which sets some (new) VIO_DR_FLOW_CONTROLLED state in the vio_driver_state, (or perhaps some flag bit in the ldc_channel?) and also marks a (new) boolean is_flow_controlled state variable in the struct vnet as TRUE.

The modified maybe_tx_wakeup would check is_flow_controlled on the vnet, and call a new vnet_wake_queue() to reset VIO_DR_FLOW_CONTROLLED if appropriate.

Quoted 2 lines

vnet_start_xmit should drop packets as long as VIO_DR_FLOW_CONTROLLED is set.

Again, as I stated yesterday, you really cannot do this. Head of line blocking when one peer gets wedged is absolutely necessary.

So the question is how to manage this on the driver side, and the most natural way I see do this would be to use multiple TX netdev queues and a custom netdev_ops->ndo_select_queue() method which selects the queue based upon the peer that would be selected.

Quoted 5 lines

However, don't get ahead of yourself. All I'm saying in the context of reviewing this patch #3 is that you should leave vnet_tx_timeout() alone and just put what you were putting into vnet_tx_timeout() into maybe_tx_wakeup().

And just leave vnet_tx_timeout() as before (i.e, "XXX implement me"?)?

Note that, as I pointed out to Raghuram already, doing ldc_disconnect does *not* send any reset events. The code may have other missing functionality.

Quoted 4 lines

So the question is how to manage this on the driver side, and the most natural way I see do this would be to use multiple TX netdev queues and a custom netdev_ops->ndo_select_queue() method which selects the queue based upon the peer that would be selected.

This might work well for the current sunvnet model (and even there, it has limitations- if all the traffic is going out via the switchport to a gateway, you are again back to the head-of-line blocking model).

But how generally useful is this model? For point-to-multipoint links like ethernet, I dont think you actually track one channel per peer.

Quoted 7 lines

Please do not usurp vnet_tx_timeout() to handle queue waking.

It is hooked up to netdev_ops->ndo_tx_timeout() which is invoked when the qdisc watchdog triggers after dev->watchdog_timeout time elapses after the queue is stopped without a subsequent wake.

Quoted 6 lines

This method is a therefore executed after a hard timeout, meaning that the device should probably be reset, whereas maybe_tx_wakeup() is normal processing meant to drive the engine of TX flow control.

In the context of sunvnet, vnet_tx_timeout() should probably reset the LDC channel(s) of the unresponsive peer(s).

yes, after a couple of days of reflection on the lingering issues in
http://www.spinics.net/lists/sparclinux/msg12360.html
I too was starting to arrive at similar conclusions about maybe_tx_wakeup vs vnet_tx_timeout.

First case 2 in the "todo" list (cannot send DRING_STOPPED):
If we cannot send a vnet_send_ack() we should invoke ldc_disconnect() for this peer. The ldc_disconnect() will reset hs_state and flags in the ldc_channel, so subsequent attempts to ldc_write() (e.g., from vio_ldc_send()) will return -ENOTCONN. So, for that case, a subsequent vnet_start_xmit() would *not* netif_stop_queue(), but just drop the packet, reset the dring, and keep driving.

To recover the disconnected channel, the admin would need to (ldm) unbind/bind the offender, identifiable by their mac address. And none of the other channels shold be affected (assuming inter-vnet-links is set to on)

From what I read the ldc_disconect() code, it seems like it is reconfiguring

So for case 1 we could do something very similar- I haven't tried this yet, but here's what I'm thinking: vnet_start_xmit() should not do a netif_stop_queue. Instead it should do a (new function) "vio_stop_queue()" which sets some (new) VIO_DR_FLOW_CONTROLLED state in the vio_driver_state, (or perhaps some flag bit in the ldc_channel?) and also marks a (new) boolean is_flow_controlled state variable in the struct vnet as TRUE.

The modified maybe_tx_wakeup would check is_flow_controlled on the vnet, and call a new vnet_wake_queue() to reset VIO_DR_FLOW_CONTROLLED if appropriate.

vnet_start_xmit should drop packets as long as VIO_DR_FLOW_CONTROLLED is set.

The above sketch has a deficiency that it doesnt avoid packet drops at the net_device layer itself - packets that can't make it out because of FLOW_CONTROLLED will get dropped - I think the Qdisc code path deals better with the back-pressure. I dont know if there is a way to leverage that path and still avoid the head-of-line blocking due to one flow-controlled ldc_channel.

I'm a little hesitant to ldc_disconnect a peer from the vnet_start_xmit path- if we have a super-efficient Tx path, we dont want to reset things merely because the receiver can't keep up- we just want to flow-control the Tx? Whereas the inability to transmit a DRING_STOPPED from the Rx side is likely to be a more unusual situation (would need the peer to send a burst of packets and then not be prepared to do even a single ldc_read()!)?

thoughts?

--Sowmini

One important point to keep in mind that packets to different peers shouldn't be blocked by one blocked peer. Any flow controlling or dropping of the packets needs to be done on a per-port basis.

From: Raghuram Kothakota <[email protected]>
Date: Tue, 12 Aug 2014 21:26:28 -0700

Quoted 3 lines

One important point to keep in mind that packets to different peers shouldn't be blocked by one blocked peer. Any flow controlling or dropping of the packets needs to be done on a per-port basis.

Until we use a big hammer and reset the LDC channel of that peer, we _absolutely_ should not reorder traffic and send to non-stuck peers.

A networking stack should avoid packet reordering in all cases when it has the ability to do so, and sending packets which came in later to unstuck peers when there are older packets still pending to the stuck peer violates that requirement.

Quoted 10 lines

From: Raghuram Kothakota <[email protected]>
Date: Tue, 12 Aug 2014 21:26:28 -0700

Quoted 3 lines

One important point to keep in mind that packets to different peers shouldn't be blocked by one blocked peer. Any flow controlling or dropping of the packets needs to be done on a per-port basis.

Until we use a big hammer and reset the LDC channel of that peer, we _absolutely_ should not reorder traffic and send to non-stuck peers.

The packet ordering requirement applies only for packets destined to a specific destination. The packets to different destinations can flow as long as they do not cause ordering issues. In case of sunvnet, each peer except the switch-port would be for a specific destination, so sending packets to other ports would not result in re-ordering of the packets.

Even if the peer to peer LDC is the one that is blocked then disconnecting that LDC channel and sending the packets via switch-port would not be expected to change the order of the packet as the LDC reset is expect to drop all packets in the ring so far.

From: Raghuram Kothakota <[email protected]>
Date: Tue, 12 Aug 2014 22:14:09 -0700

Quoted 18 lines · David Miller

Quoted 10 lines

From: Raghuram Kothakota <[email protected]>
Date: Tue, 12 Aug 2014 21:26:28 -0700

Quoted 3 lines

One important point to keep in mind that packets to different peers shouldn't be blocked by one blocked peer. Any flow controlling or dropping of the packets needs to be done on a per-port basis.

Until we use a big hammer and reset the LDC channel of that peer, we _absolutely_ should not reorder traffic and send to non-stuck peers.

The packet ordering requirement applies only for packets destined to a specific destination. The packets to different destinations can flow as long as they do not cause ordering issues. In case of sunvnet, each peer except the switch-port would be for a specific destination, so sending packets to other ports would not result in re-ordering of the packets.

Quoted 4 lines

Even if the peer to peer LDC is the one that is blocked then disconnecting that LDC channel and sending the packets via switch-port would not be expected to change the order of the packet as the LDC reset is expect to drop all packets in the ring so far.

Ok, that makes sense.

So the question is how to manage this on the driver side, and the most natural way I see do this would be to use multiple TX netdev queues and a custom netdev_ops->ndo_select_queue() method which selects the queue based upon the peer that would be selected.

Thanks, if there is a method to accomplish this with multiple Tx netdev queues would be wonderful. We will research on the custom ndo_selec_queue() method to direct traffic automatically at the network stack level. We probably also need to look for methods to increase parallelism to boost performance, I assume multiple Tx queues would be a method to accomplish that as well.

From: Raghuram Kothakota <[email protected]>
Date: Tue, 12 Aug 2014 22:44:27 -0700

Quoted 6 lines

Thanks, if there is a method to accomplish this with multiple Tx netdev queues would be wonderful. We will research on the custom ndo_selec_queue() method to direct traffic automatically at the network stack level. We probably also need to look for methods to increase parallelism to boost performance, I assume multiple Tx queues would be a method to accomplish that as well.

It will introduce parallelism only when there is traffic to multiple peers at the same time.

We do not consider a peer which couldn't process LDC messages as an offender but just a peer in a broken state. A peer could get

From what I read the ldc_disconect() code, it seems like it is reconfiguring the queues, so I assume a reset event would re-enable the communication fine, but I don't know for sure without testing this specific condition.

Quoted 3 lines · Raghuram Kothakota

We do not consider a peer which couldn't process LDC messages as an offender but just a peer in a broken state. A peer could get

So I was using "offender" in the same way that one typically talks about sending ICMP errors for "offending IP packets" (nothing pesonal :-)). I dont want to quibble over terminology here.

Quoted 3 lines

From what I read the ldc_disconect() code, it seems like it is reconfiguring the queues, so I assume a reset event would re-enable the communication fine, but I don't know for sure without testing this specific condition.

No such events were generated by the code when I tested it.

I'll send a consolidated reply to the rest of that very long thread in a bit.

--Sowmini