[PATCH net-next 0/2] xen-netback: Changes around carrier handling

This series starts using carrier off as a way to purge packets when the guest is
not able (or willing) to receive them. It is a much faster way to get rid of
packets waiting for an overwhelmed guest.
The first patch changes current netback code where it relies currently on
netif_carrier_ok.
The second turns off the carrier if the guest times out on a queue, and only
turn it on again if that queue (or queues) resurrects.

Signed-off-by: Zoltan Kiss <redacted>
Signed-off-by: David Vrabel <redacted>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: xen-devel@lists.xenproject.org 

This patch introduces a new state bit VIF_STATUS_CONNECTED to track whether the
vif is in a connected state. Using carrier will not work with the next patch
in this series, which aims to turn the carrier temporarily off if the guest
doesn't seem to be able to receive packets.

Signed-off-by: Zoltan Kiss <redacted>
Signed-off-by: David Vrabel <redacted>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: xen-devel@lists.xenproject.org 

v2:
- rename the bitshift type to "enum state_bit_shift" here, not in the next patch

diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index 28c9822..4a92fc1 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h

@@ -198,6 +198,11 @@ struct xenvif_queue { /* Per-queue data for xenvif */
 	struct xenvif_stats stats;
 };
 
+enum state_bit_shift {
+	/* This bit marks that the vif is connected */
+	VIF_STATUS_CONNECTED
+};
+
 struct xenvif {
 	/* Unique identifier for this interface. */
 	domid_t          domid;

@@ -220,6 +225,7 @@ struct xenvif {
 	 * frontend is rogue.
 	 */
 	bool disabled;
+	unsigned long status;
 
 	/* Queues */
 	struct xenvif_queue *queues;

diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index bd59d9d..fbdadb3 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c

@@ -55,7 +55,8 @@ static inline void xenvif_stop_queue(struct xenvif_queue *queue)
 
 int xenvif_schedulable(struct xenvif *vif)
 {
-	return netif_running(vif->dev) && netif_carrier_ok(vif->dev);
+	return netif_running(vif->dev) &&
+		test_bit(VIF_STATUS_CONNECTED, &vif->status);
 }
 
 static irqreturn_t xenvif_tx_interrupt(int irq, void *dev_id)

@@ -267,7 +268,7 @@ static void xenvif_down(struct xenvif *vif)
 static int xenvif_open(struct net_device *dev)
 {
 	struct xenvif *vif = netdev_priv(dev);
-	if (netif_carrier_ok(dev))
+	if (test_bit(VIF_STATUS_CONNECTED, &vif->status))
 		xenvif_up(vif);
 	netif_tx_start_all_queues(dev);
 	return 0;

@@ -276,7 +277,7 @@ static int xenvif_open(struct net_device *dev)
 static int xenvif_close(struct net_device *dev)
 {
 	struct xenvif *vif = netdev_priv(dev);
-	if (netif_carrier_ok(dev))
+	if (test_bit(VIF_STATUS_CONNECTED, &vif->status))
 		xenvif_down(vif);
 	netif_tx_stop_all_queues(dev);
 	return 0;

@@ -528,6 +529,7 @@ void xenvif_carrier_on(struct xenvif *vif)
 	if (!vif->can_sg && vif->dev->mtu > ETH_DATA_LEN)
 		dev_set_mtu(vif->dev, ETH_DATA_LEN);
 	netdev_update_features(vif->dev);
+	set_bit(VIF_STATUS_CONNECTED, &vif->status);
 	netif_carrier_on(vif->dev);
 	if (netif_running(vif->dev))
 		xenvif_up(vif);

@@ -625,9 +627,11 @@ void xenvif_carrier_off(struct xenvif *vif)
 	struct net_device *dev = vif->dev;
 
 	rtnl_lock();
-	netif_carrier_off(dev); /* discard queued packets */
-	if (netif_running(dev))
-		xenvif_down(vif);
+	if (test_and_clear_bit(VIF_STATUS_CONNECTED, &vif->status)) {
+		netif_carrier_off(dev); /* discard queued packets */
+		if (netif_running(dev))
+			xenvif_down(vif);
+	}
 	rtnl_unlock();
 }
 

@@ -656,8 +660,7 @@ void xenvif_disconnect(struct xenvif *vif)
 	unsigned int num_queues = vif->num_queues;
 	unsigned int queue_index;
 
-	if (netif_carrier_ok(vif->dev))
-		xenvif_carrier_off(vif);
+	xenvif_carrier_off(vif);
 
 	for (queue_index = 0; queue_index < num_queues; ++queue_index) {
 		queue = &vif->queues[queue_index];

diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 769e553..6c4cc0f 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c

@@ -1953,7 +1953,7 @@ int xenvif_kthread_guest_rx(void *data)
 		 * context so we defer it here, if this thread is
 		 * associated with queue 0.
 		 */
-		if (unlikely(queue->vif->disabled && netif_carrier_ok(queue->vif->dev) && queue->id == 0))
+		if (unlikely(queue->vif->disabled && queue->id == 0))
 			xenvif_carrier_off(queue->vif);
 
 		if (kthread_should_stop())

Currently when the guest is not able to receive more packets, qdisc layer starts
a timer, and when it goes off, qdisc is started again to deliver a packet again.
This is a very slow way to drain the queues, consumes unnecessary resources and
slows down other guests shutdown.
This patch change the behaviour by turning the carrier off when that timer
fires, so all the packets are freed up which were stucked waiting for that vif.
Instead of the rx_queue_purge bool it uses the VIF_STATUS_RX_PURGE_EVENT bit to
signal the thread that either the timeout happened or an RX interrupt arrived,
so the thread can check what it should do. It also disables NAPI, so the guest
can't transmit, but leaves the interrupts on, so it can resurrect.
Only the queues which brought down the interface can enable it again, the bit
QUEUE_STATUS_RX_STALLED makes sure of that.

Signed-off-by: Zoltan Kiss <redacted>
Signed-off-by: David Vrabel <redacted>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: xen-devel@lists.xenproject.org 

diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index 4a92fc1..ef3026f 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h

@@ -176,9 +176,9 @@ struct xenvif_queue { /* Per-queue data for xenvif */
 	struct xen_netif_rx_back_ring rx;
 	struct sk_buff_head rx_queue;
 	RING_IDX rx_last_skb_slots;
-	bool rx_queue_purge;
+	unsigned long status;
 
-	struct timer_list wake_queue;
+	struct timer_list rx_stalled;
 
 	struct gnttab_copy grant_copy_op[MAX_GRANT_COPY_OPS];
 

@@ -200,7 +200,16 @@ struct xenvif_queue { /* Per-queue data for xenvif */
 
 enum state_bit_shift {
 	/* This bit marks that the vif is connected */
-	VIF_STATUS_CONNECTED
+	VIF_STATUS_CONNECTED,
+	/* This bit signals the RX thread that queuing was stopped (in
+	 * start_xmit), and either the timer fired or an RX interrupt came
+	 */
+	QUEUE_STATUS_RX_PURGE_EVENT,
+	/* This bit tells the interrupt handler that this queue was the reason
+	 * for the carrier off, so it should kick the thread. Only queues which
+	 * brought it down can turn on the carrier.
+	 */
+	QUEUE_STATUS_RX_STALLED
 };
 
 struct xenvif {

diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index fbdadb3..48a55cd 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c

@@ -78,8 +78,12 @@ int xenvif_poll(struct napi_struct *napi, int budget)
 	/* This vif is rogue, we pretend we've there is nothing to do
 	 * for this vif to deschedule it from NAPI. But this interface
 	 * will be turned off in thread context later.
+	 * Also, if a guest doesn't post enough slots to receive data on one of
+	 * its queues, the carrier goes down and NAPI is descheduled here so
+	 * the guest can't send more packets until it's ready to receive.
 	 */
-	if (unlikely(queue->vif->disabled)) {
+	if (unlikely(queue->vif->disabled ||
+		     !netif_carrier_ok(queue->vif->dev))) {
 		napi_complete(napi);
 		return 0;
 	}

@@ -97,7 +101,16 @@ int xenvif_poll(struct napi_struct *napi, int budget)
 static irqreturn_t xenvif_rx_interrupt(int irq, void *dev_id)
 {
 	struct xenvif_queue *queue = dev_id;
+	struct netdev_queue *net_queue =
+		netdev_get_tx_queue(queue->vif->dev, queue->id);
 
+	/* QUEUE_STATUS_RX_PURGE_EVENT is only set if either QDisc was off OR
+	 * the carrier went down and this queue was previously blocked
+	 */
+	if (unlikely(netif_tx_queue_stopped(net_queue) ||
+		     (!netif_carrier_ok(queue->vif->dev) &&
+		      test_bit(QUEUE_STATUS_RX_STALLED, &queue->status))))
+		set_bit(QUEUE_STATUS_RX_PURGE_EVENT, &queue->status);
 	xenvif_kick_thread(queue);
 
 	return IRQ_HANDLED;

@@ -125,16 +138,14 @@ void xenvif_wake_queue(struct xenvif_queue *queue)
 	netif_tx_wake_queue(netdev_get_tx_queue(dev, id));
 }
 
-/* Callback to wake the queue and drain it on timeout */
-static void xenvif_wake_queue_callback(unsigned long data)
+/* Callback to wake the queue's thread and turn the carrier off on timeout */
+static void xenvif_rx_stalled(unsigned long data)
 {
 	struct xenvif_queue *queue = (struct xenvif_queue *)data;
 
 	if (xenvif_queue_stopped(queue)) {
-		netdev_err(queue->vif->dev, "draining TX queue\n");
-		queue->rx_queue_purge = true;
+		set_bit(QUEUE_STATUS_RX_PURGE_EVENT, &queue->status);
 		xenvif_kick_thread(queue);
-		xenvif_wake_queue(queue);
 	}
 }
 

@@ -183,11 +194,11 @@ static int xenvif_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	 * drain.
 	 */
 	if (!xenvif_rx_ring_slots_available(queue, min_slots_needed)) {
-		queue->wake_queue.function = xenvif_wake_queue_callback;
-		queue->wake_queue.data = (unsigned long)queue;
+		queue->rx_stalled.function = xenvif_rx_stalled;
+		queue->rx_stalled.data = (unsigned long)queue;
 		xenvif_stop_queue(queue);
-		mod_timer(&queue->wake_queue,
-			jiffies + rx_drain_timeout_jiffies);
+		mod_timer(&queue->rx_stalled,
+			  jiffies + rx_drain_timeout_jiffies);
 	}
 
 	skb_queue_tail(&queue->rx_queue, skb);

@@ -515,7 +526,7 @@ int xenvif_init_queue(struct xenvif_queue *queue)
 		queue->grant_tx_handle[i] = NETBACK_INVALID_HANDLE;
 	}
 
-	init_timer(&queue->wake_queue);
+	init_timer(&queue->rx_stalled);
 
 	netif_napi_add(queue->vif->dev, &queue->napi, xenvif_poll,
 			XENVIF_NAPI_WEIGHT);

@@ -666,7 +677,7 @@ void xenvif_disconnect(struct xenvif *vif)
 		queue = &vif->queues[queue_index];
 
 		if (queue->task) {
-			del_timer_sync(&queue->wake_queue);
+			del_timer_sync(&queue->rx_stalled);
 			kthread_stop(queue->task);
 			queue->task = NULL;
 		}

@@ -708,16 +719,12 @@ void xenvif_free(struct xenvif *vif)
 	/* Here we want to avoid timeout messages if an skb can be legitimately
 	 * stuck somewhere else. Realistically this could be an another vif's
 	 * internal or QDisc queue. That another vif also has this
-	 * rx_drain_timeout_msecs timeout, but the timer only ditches the
-	 * internal queue. After that, the QDisc queue can put in worst case
-	 * XEN_NETIF_RX_RING_SIZE / MAX_SKB_FRAGS skbs into that another vif's
-	 * internal queue, so we need several rounds of such timeouts until we
-	 * can be sure that no another vif should have skb's from us. We are
-	 * not sending more skb's, so newly stuck packets are not interesting
-	 * for us here.
+	 * rx_drain_timeout_msecs timeout, so give it time to drain out.
+	 * Although if that other guest wakes up just before its timeout happens
+	 * and takes only one skb from QDisc, it can hold onto other skbs for a
+	 * longer period.
 	 */
-	unsigned int worst_case_skb_lifetime = (rx_drain_timeout_msecs/1000) *
-		DIV_ROUND_UP(XENVIF_QUEUE_LENGTH, (XEN_NETIF_RX_RING_SIZE / MAX_SKB_FRAGS));
+	unsigned int worst_case_skb_lifetime = (rx_drain_timeout_msecs/1000);
 
 	unregister_netdev(vif->dev);
 

diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 6c4cc0f..aa20933 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c

@@ -1869,8 +1869,7 @@ void xenvif_idx_unmap(struct xenvif_queue *queue, u16 pending_idx)
 static inline int rx_work_todo(struct xenvif_queue *queue)
 {
 	return (!skb_queue_empty(&queue->rx_queue) &&
-	       xenvif_rx_ring_slots_available(queue, queue->rx_last_skb_slots)) ||
-	       queue->rx_queue_purge;
+	       xenvif_rx_ring_slots_available(queue, queue->rx_last_skb_slots));
 }
 
 static inline int tx_work_todo(struct xenvif_queue *queue)

@@ -1935,6 +1934,75 @@ static void xenvif_start_queue(struct xenvif_queue *queue)
 		xenvif_wake_queue(queue);
 }
 
+/* Only called from the queue's thread, it handles the situation when the guest
+ * doesn't post enough requests on the receiving ring.
+ * First xenvif_start_xmit disables QDisc and start a timer, and then either the
+ * timer fires, or the guest send an interrupt after posting new request. If it
+ * is the timer, the carrier is turned off here.
+ * */
+static void xenvif_rx_purge_event(struct xenvif_queue *queue)
+{
+	/* Either the last unsuccesful skb or at least 1 slot should fit */
+	int needed = queue->rx_last_skb_slots ?
+		     queue->rx_last_skb_slots : 1;
+
+	/* It is assumed that if the guest post new slots after this, the RX
+	 * interrupt will set the QUEUE_STATUS_RX_PURGE_EVENT bit and wake up
+	 * the thread again
+	 */
+	set_bit(QUEUE_STATUS_RX_STALLED, &queue->status);
+	if (!xenvif_rx_ring_slots_available(queue, needed)) {
+		rtnl_lock();
+		if (netif_carrier_ok(queue->vif->dev)) {
+			/* Timer fired and there are still no slots. Turn off
+			 * everything except the interrupts
+			 */
+			netif_carrier_off(queue->vif->dev);
+			skb_queue_purge(&queue->rx_queue);
+			queue->rx_last_skb_slots = 0;
+			if (net_ratelimit())
+				netdev_err(queue->vif->dev, "Carrier off due to lack of guest response on queue %d\n", queue->id);
+		} else {
+			/* Probably an another queue already turned the carrier
+			 * off, make sure nothing is stucked in the internal
+			 * queue of this queue
+			 */
+			skb_queue_purge(&queue->rx_queue);
+			queue->rx_last_skb_slots = 0;
+		}
+		rtnl_unlock();
+	} else if (!netif_carrier_ok(queue->vif->dev)) {
+		unsigned int num_queues = queue->vif->num_queues;
+		unsigned int i;
+		/* The carrier was down, but an interrupt kicked
+		 * the thread again after new requests were
+		 * posted
+		 */
+		clear_bit(QUEUE_STATUS_RX_STALLED,
+			  &queue->status);
+		rtnl_lock();
+		netif_carrier_on(queue->vif->dev);
+		netif_tx_wake_all_queues(queue->vif->dev);
+		rtnl_unlock();
+
+		for (i = 0; i < num_queues; i++) {
+			struct xenvif_queue *temp = &queue->vif->queues[i];
+
+			xenvif_napi_schedule_or_enable_events(temp);
+		}
+		if (net_ratelimit())
+			netdev_err(queue->vif->dev, "Carrier on again\n");
+	} else {
+		/* Queuing were stopped, but the guest posted
+		 * new requests and sent an interrupt
+		 */
+		clear_bit(QUEUE_STATUS_RX_STALLED,
+			  &queue->status);
+		del_timer_sync(&queue->rx_stalled);
+		xenvif_start_queue(queue);
+	}
+}
+
 int xenvif_kthread_guest_rx(void *data)
 {
 	struct xenvif_queue *queue = data;

@@ -1944,8 +2012,12 @@ int xenvif_kthread_guest_rx(void *data)
 		wait_event_interruptible(queue->wq,
 					 rx_work_todo(queue) ||
 					 queue->vif->disabled ||
+					 test_bit(QUEUE_STATUS_RX_PURGE_EVENT, &queue->status) ||
 					 kthread_should_stop());
 
+		if (kthread_should_stop())
+			break;
+
 		/* This frontend is found to be rogue, disable it in
 		 * kthread context. Currently this is only set when
 		 * netback finds out frontend sends malformed packet,

@@ -1955,24 +2027,21 @@ int xenvif_kthread_guest_rx(void *data)
 		 */
 		if (unlikely(queue->vif->disabled && queue->id == 0))
 			xenvif_carrier_off(queue->vif);
-
-		if (kthread_should_stop())
-			break;
-
-		if (queue->rx_queue_purge) {
+		else if (unlikely(test_and_clear_bit(QUEUE_STATUS_RX_PURGE_EVENT,
+						     &queue->status))) {
+			xenvif_rx_purge_event(queue);
+		} else if (!netif_carrier_ok(queue->vif->dev)) {
+			/* Another queue stalled and turned the carrier off, so
+			 * purge the internal queue of queues which were not
+			 * blocked
+			 */
 			skb_queue_purge(&queue->rx_queue);
-			queue->rx_queue_purge = false;
+			queue->rx_last_skb_slots = 0;
 		}
 
 		if (!skb_queue_empty(&queue->rx_queue))
 			xenvif_rx_action(queue);
 
-		if (skb_queue_empty(&queue->rx_queue) &&
-		    xenvif_queue_stopped(queue)) {
-			del_timer_sync(&queue->wake_queue);
-			xenvif_start_queue(queue);
-		}
-
 		cond_resched();
 	}
 

On 04/08/14 16:20, Zoltan Kiss wrote:

This series starts using carrier off as a way to purge packets when the guest is
not able (or willing) to receive them. It is a much faster way to get rid of
packets waiting for an overwhelmed guest.
The first patch changes current netback code where it relies currently on
netif_carrier_ok.
The second turns off the carrier if the guest times out on a queue, and only
turn it on again if that queue (or queues) resurrects.

Signed-off-by: Zoltan Kiss <redacted>
Signed-off-by: David Vrabel <redacted>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: xen-devel@lists.xenproject.org

Sorry, I forgot to include v2 in the subject header, and the version 
history for the second patch:

v2:
- added a lot more comments
- fixing the bit checking in RX interrupt
- move out the bulk of the code from the main thread function into
   xenvif_rx_purge_event

Zoli

On Mon, Aug 04, 2014 at 04:20:57PM +0100, Zoltan Kiss wrote:

This bit shift applies to vif.  In the following patch you introduce two
more bits specifically for queues. IMHO we should avoid mixing things
up. What about having two enums

  enum vif_state_bit_shift {}
  enum queue_state_bit_shift {}

?

Wei.

On Mon, Aug 04, 2014 at 04:20:58PM +0100, Zoltan Kiss wrote:
[...]

Could you change "blocked" to "stalled" so that the comment matches the
code closely?

[...]

Please remove that extra "*".

+static void xenvif_rx_purge_event(struct xenvif_queue *queue)
+{
+	/* Either the last unsuccesful skb or at least 1 slot should fit */
+	int needed = queue->rx_last_skb_slots ?
+		     queue->rx_last_skb_slots : 1;
+
+	/* It is assumed that if the guest post new slots after this, the RX
+	 * interrupt will set the QUEUE_STATUS_RX_PURGE_EVENT bit and wake up
+	 * the thread again
+	 */

Basically in this state machine you have a tuple (RX_STALLED bit,
PURGE_EVENT bit, carrier state). This whole state transaction is very
scary, any chance you can draw a graph like the xenbus state machine in
xenbus.c?

I fear that after three month noone can easily understand this code
unless he / she spends half a day reading the code. And without defining
what state is legal it's very hard to tell what behavior is expected and
what is not.

+	set_bit(QUEUE_STATUS_RX_STALLED, &queue->status);
+	if (!xenvif_rx_ring_slots_available(queue, needed)) {
+		rtnl_lock();
+		if (netif_carrier_ok(queue->vif->dev)) {
+			/* Timer fired and there are still no slots. Turn off
+			 * everything except the interrupts
+			 */
+			netif_carrier_off(queue->vif->dev);
+			skb_queue_purge(&queue->rx_queue);
+			queue->rx_last_skb_slots = 0;
+			if (net_ratelimit())
+				netdev_err(queue->vif->dev, "Carrier off due to lack of guest response on queue %d\n", queue->id);

Line too long.

Line too long.

I think you also need to check vif->disabled flag in your following code
so that you don't accidently re-enable a rogue vif in a queue whose id
!= 0.

Further more "disabled" can be transformed to a bit in vif->status.
You can incorporate such change in your previous patch or a separate
prerequisite patch.

-
-		if (kthread_should_stop())
-			break;
-
-		if (queue->rx_queue_purge) {
+		else if (unlikely(test_and_clear_bit(QUEUE_STATUS_RX_PURGE_EVENT,
+						     &queue->status))) {
+			xenvif_rx_purge_event(queue);
+		} else if (!netif_carrier_ok(queue->vif->dev)) {
+			/* Another queue stalled and turned the carrier off, so
+			 * purge the internal queue of queues which were not
+			 * blocked
+			 */

"blocked" -> "stalled"?

In theory even one queue stalls all other queues can still make
progress, isn't it?

Wei.

From: Zoltan Kiss <redacted>
Date: Mon, 4 Aug 2014 16:20:56 +0100

This series starts using carrier off as a way to purge packets when the guest is
not able (or willing) to receive them. It is a much faster way to get rid of
packets waiting for an overwhelmed guest.
The first patch changes current netback code where it relies currently on
netif_carrier_ok.
The second turns off the carrier if the guest times out on a queue, and only
turn it on again if that queue (or queues) resurrects.

Signed-off-by: Zoltan Kiss <redacted>
Signed-off-by: David Vrabel <redacted>

Applied, but I have some reservations:

1) This is starting to bleed what is normally qdisc type policy into the
   driver.

2) There are other drivers that could run into this kind of situation and
   have similar concerns, therefore we should make sure we have a consistent
   approach that such entities use to handle this problem.

Part of the problem is that netif_carrier_off() only partially mimicks
the situation.  It expresses the "transmitter is down so packets
aren't going onto the wire" part, which keeps the watchdog from
spitting out log messages ever time it fires.  But it doesn't deal
with packet freeing policy meanwhile, which I guess is the part that
this patch series is largely trying to address.

Thanks.

On Tue, Aug 05, 2014 at 04:07:48PM -0700, David Miller wrote:

From: Zoltan Kiss <redacted>
Date: Mon, 4 Aug 2014 16:20:56 +0100

quoted

This series starts using carrier off as a way to purge packets when the guest is
not able (or willing) to receive them. It is a much faster way to get rid of
packets waiting for an overwhelmed guest.
The first patch changes current netback code where it relies currently on
netif_carrier_ok.
The second turns off the carrier if the guest times out on a queue, and only
turn it on again if that queue (or queues) resurrects.

Signed-off-by: Zoltan Kiss <redacted>
Signed-off-by: David Vrabel <redacted>

Applied, but I have some reservations:

Wow, this is fast. I appreciate your speed, but there's still
outstanding issues in the series.

Apart from some comments on maintainability, the biggest problem is that
there's a bug in second patch that needs to be fixed; or I need to be
proved wrong.  We need to set aside some more time to get those
questions answered. I don't think this series is suitable to go in as
is.

DaveM, could you please advise how to deal with this situation?

Wei.

From: Wei Liu <redacted>
Date: Wed, 6 Aug 2014 01:00:59 +0100

DaveM, could you please advise how to deal with this situation?

The merge window is just openning, you have two months to fix any
problems.

On Tue, Aug 05, 2014 at 06:50:20PM -0700, David Miller wrote:

From: Wei Liu <redacted>
Date: Wed, 6 Aug 2014 01:00:59 +0100

quoted

DaveM, could you please advise how to deal with this situation?

The merge window is just openning, you have two months to fix any
problems.

OK, then I shall wait for some other incremental patches this time. But
I still prefer to get obvious issues in patches addressed before
applying them.

Wei.

On 05/08/14 13:45, Wei Liu wrote:

On Mon, Aug 04, 2014 at 04:20:57PM +0100, Zoltan Kiss wrote:

quoted

This patch introduces a new state bit VIF_STATUS_CONNECTED to track whether the
vif is in a connected state. Using carrier will not work with the next patch
in this series, which aims to turn the carrier temporarily off if the guest
doesn't seem to be able to receive packets.

Signed-off-by: Zoltan Kiss <redacted>
Signed-off-by: David Vrabel <redacted>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: xen-devel@lists.xenproject.org

v2:
- rename the bitshift type to "enum state_bit_shift" here, not in the next patch

diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index 28c9822..4a92fc1 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h

@@ -198,6 +198,11 @@ struct xenvif_queue { /* Per-queue data for xenvif */
  	struct xenvif_stats stats;
  };

+enum state_bit_shift {
+	/* This bit marks that the vif is connected */
+	VIF_STATUS_CONNECTED

This bit shift applies to vif.  In the following patch you introduce two
more bits specifically for queues. IMHO we should avoid mixing things
up. What about having two enums

   enum vif_state_bit_shift {}
   enum queue_state_bit_shift {}

I think it would be a bit overdoing it, this enum type is never used in 
declaration.

On 05/08/14 13:45, Wei Liu wrote:

On Mon, Aug 04, 2014 at 04:20:58PM +0100, Zoltan Kiss wrote:
[...]

quoted

  struct xenvif {

diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index fbdadb3..48a55cd 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c

@@ -97,7 +101,16 @@ int xenvif_poll(struct napi_struct *napi, int budget)
  static irqreturn_t xenvif_rx_interrupt(int irq, void *dev_id)
  {
  	struct xenvif_queue *queue = dev_id;
+	struct netdev_queue *net_queue =
+		netdev_get_tx_queue(queue->vif->dev, queue->id);

+	/* QUEUE_STATUS_RX_PURGE_EVENT is only set if either QDisc was off OR
+	 * the carrier went down and this queue was previously blocked
+	 */

Could you change "blocked" to "stalled" so that the comment matches the
code closely?

Ok

quoted

@@ -1935,6 +1934,75 @@ static void xenvif_start_queue(struct xenvif_queue *queue)
  		xenvif_wake_queue(queue);
  }

+/* Only called from the queue's thread, it handles the situation when the guest
+ * doesn't post enough requests on the receiving ring.
+ * First xenvif_start_xmit disables QDisc and start a timer, and then either the
+ * timer fires, or the guest send an interrupt after posting new request. If it
+ * is the timer, the carrier is turned off here.
+ * */

Please remove that extra "*".

Ok

quoted

+static void xenvif_rx_purge_event(struct xenvif_queue *queue)
+{
+	/* Either the last unsuccesful skb or at least 1 slot should fit */
+	int needed = queue->rx_last_skb_slots ?
+		     queue->rx_last_skb_slots : 1;
+
+	/* It is assumed that if the guest post new slots after this, the RX
+	 * interrupt will set the QUEUE_STATUS_RX_PURGE_EVENT bit and wake up
+	 * the thread again
+	 */

Basically in this state machine you have a tuple (RX_STALLED bit,
PURGE_EVENT bit, carrier state). This whole state transaction is very
scary, any chance you can draw a graph like the xenbus state machine in
xenbus.c?

I fear that after three month noone can easily understand this code
unless he / she spends half a day reading the code. And without defining
what state is legal it's very hard to tell what behavior is expected and
what is not.

Ok

quoted

+	set_bit(QUEUE_STATUS_RX_STALLED, &queue->status);
+	if (!xenvif_rx_ring_slots_available(queue, needed)) {
+		rtnl_lock();
+		if (netif_carrier_ok(queue->vif->dev)) {
+			/* Timer fired and there are still no slots. Turn off
+			 * everything except the interrupts
+			 */
+			netif_carrier_off(queue->vif->dev);
+			skb_queue_purge(&queue->rx_queue);
+			queue->rx_last_skb_slots = 0;
+			if (net_ratelimit())
+				netdev_err(queue->vif->dev, "Carrier off due to lack of guest response on queue %d\n", queue->id);

Line too long.

Ok

quoted

@@ -1944,8 +2012,12 @@ int xenvif_kthread_guest_rx(void *data)
  		wait_event_interruptible(queue->wq,
  					 rx_work_todo(queue) ||
  					 queue->vif->disabled ||
+					 test_bit(QUEUE_STATUS_RX_PURGE_EVENT, &queue->status) ||

Line too long.

Ok

quoted

  					 kthread_should_stop());

+		if (kthread_should_stop())
+			break;
+
  		/* This frontend is found to be rogue, disable it in
  		 * kthread context. Currently this is only set when
  		 * netback finds out frontend sends malformed packet,

@@ -1955,24 +2027,21 @@ int xenvif_kthread_guest_rx(void *data)
  		 */
  		if (unlikely(queue->vif->disabled && queue->id == 0))
  			xenvif_carrier_off(queue->vif);

I think you also need to check vif->disabled flag in your following code
so that you don't accidently re-enable a rogue vif in a queue whose id
!= 0.

Yes.

Further more "disabled" can be transformed to a bit in vif->status.
You can incorporate such change in your previous patch or a separate
prerequisite patch.

Yes, I've already done that on my non-multiqueue branch.

quoted

-
-		if (kthread_should_stop())
-			break;
-
-		if (queue->rx_queue_purge) {
+		else if (unlikely(test_and_clear_bit(QUEUE_STATUS_RX_PURGE_EVENT,
+						     &queue->status))) {
+			xenvif_rx_purge_event(queue);
+		} else if (!netif_carrier_ok(queue->vif->dev)) {
+			/* Another queue stalled and turned the carrier off, so
+			 * purge the internal queue of queues which were not
+			 * blocked
+			 */

"blocked" -> "stalled"?

Ok

In theory even one queue stalls all other queues can still make
progress, isn't it?

This patch makes sure that if a queue is stalled, none of the others can 
transmit, even if they would be able to do so. It is documented at the 
definition of QUEUE_STATUS_RX_STALLED.

Wei.

On 06/08/14 00:07, David Miller wrote:

From: Zoltan Kiss <redacted>
Date: Mon, 4 Aug 2014 16:20:56 +0100

quoted

This series starts using carrier off as a way to purge packets when the guest is
not able (or willing) to receive them. It is a much faster way to get rid of
packets waiting for an overwhelmed guest.
The first patch changes current netback code where it relies currently on
netif_carrier_ok.
The second turns off the carrier if the guest times out on a queue, and only
turn it on again if that queue (or queues) resurrects.

Signed-off-by: Zoltan Kiss <redacted>
Signed-off-by: David Vrabel <redacted>

Applied, but I have some reservations:

1) This is starting to bleed what is normally qdisc type policy into the
    driver.

Yeah. The fundamental problem with netback that start_xmit place the 
packet into an internal queue, and then the thread does the actual 
transmission from that queue, but it doesn't know whether it will 
succeed in a finite time period.
There is slot estimation in start_xmit to prevent QDisc pouring more 
packets into the internal queue when it seems it won't fit. When that 
happens, we stop QDisc and start a timer, and when the timer fires, 
before this patch we just ditched the internal queue and started QDisc 
again. If the frontend were dead, it lead to a quite long delay until 
packets destined to it were freed.
I just noticed a possible problem with start_xmit: if this slot 
estimation fails, and the packet is likely to be stalled at least for a 
while, it still place the skb into the internal queue and return 
NETDEV_TX_OK. Shouldn't we return NETDEV_TX_BUSY and not placing the 
packet into the internal queue? It will be requeued later, or dropped by 
QDisc. I think it will be more natural. But it can decrease performance 
if these "not enough slot" situations are very frequent and short 
living, and by the time the RX thread wakes up
My long term idea is to move part of the thread's work into start_xmit, 
so it can set up the grant operations and if there isn't enough slot it 
can return the skb to QDisc with NETDEV_TX_BUSY for requeueing. Then the 
thread can only do the batching of the grant copy operations and 
releasing the skbs. And we can ditch a good part of the code ...

2) There are other drivers that could run into this kind of situation and
    have similar concerns, therefore we should make sure we have a consistent
    approach that such entities use to handle this problem.

Part of the problem is that netif_carrier_off() only partially mimicks
the situation.  It expresses the "transmitter is down so packets
aren't going onto the wire" part, which keeps the watchdog from
spitting out log messages ever time it fires.  But it doesn't deal
with packet freeing policy meanwhile, which I guess is the part that
this patch series is largely trying to address.

Thanks.

From: Zoltan Kiss <redacted>
Date: Wed, 6 Aug 2014 20:20:55 +0100

The fundamental problem with netback that start_xmit place the
packet into an internal queue, and then the thread does the actual
transmission from that queue, but it doesn't know whether it will
succeed in a finite time period.

A hardware device acts the same way when the link goes down or the
transmitter hangs.  I do not see this situation, therefore, as
fundamentally unique to xen-netback.

I just noticed a possible problem with start_xmit: if this slot
estimation fails, and the packet is likely to be stalled at least for
a while, it still place the skb into the internal queue and return
NETDEV_TX_OK. Shouldn't we return NETDEV_TX_BUSY and not placing the
packet into the internal queue? It will be requeued later, or dropped
by QDisc. I think it will be more natural. But it can decrease
performance if these "not enough slot" situations are very frequent
and short living, and by the time the RX thread wakes up
My long term idea is to move part of the thread's work into
start_xmit, so it can set up the grant operations and if there isn't
enough slot it can return the skb to QDisc with NETDEV_TX_BUSY for
requeueing. Then the thread can only do the batching of the grant copy
operations and releasing the skbs. And we can ditch a good part of the
code ...

Yes, it would be a slight improvement if slot availability was
detected at ->ndo_start_xmit() time.

But best would be to properly stop the queue at the _end_ of
->ndo_start_xmit() like nearly all ethernet drivers do.

And we can't do that in netback because..... your queues are too
small.

If your queues were large enough you could say "now that I've queued
up SKB for transmit, do I still have enough slots available for a
maxed out SKB?"  and stop the queue if the answer to that question is
no.

The queue state was not meant to be a "maybe I can queue a new packet"
indication.  It's supposed to mean that you can absolutely take at
least one more SKB of any size or configuration.

Returning TX_BUSY from ->ndo_start_xmit() is fundamentally, therefore,
more like an error condition rather than something that should occur
under normal circumstances.  If your queue is up, you should be able
to accept any one single packet.  That's the rule.

Requeueing back into the qdisc is expensive and takes a lot of locks
awkwardly.  You do not want the kernel taking this code path.

On 06/08/14 22:01, David Miller wrote:

From: Zoltan Kiss <redacted>
Date: Wed, 6 Aug 2014 20:20:55 +0100

quoted

The fundamental problem with netback that start_xmit place the
packet into an internal queue, and then the thread does the actual
transmission from that queue, but it doesn't know whether it will
succeed in a finite time period.

A hardware device acts the same way when the link goes down or the
transmitter hangs.  I do not see this situation, therefore, as
fundamentally unique to xen-netback.

quoted

I just noticed a possible problem with start_xmit: if this slot
estimation fails, and the packet is likely to be stalled at least for
a while, it still place the skb into the internal queue and return
NETDEV_TX_OK. Shouldn't we return NETDEV_TX_BUSY and not placing the
packet into the internal queue? It will be requeued later, or dropped
by QDisc. I think it will be more natural. But it can decrease
performance if these "not enough slot" situations are very frequent
and short living, and by the time the RX thread wakes up
My long term idea is to move part of the thread's work into
start_xmit, so it can set up the grant operations and if there isn't
enough slot it can return the skb to QDisc with NETDEV_TX_BUSY for
requeueing. Then the thread can only do the batching of the grant copy
operations and releasing the skbs. And we can ditch a good part of the
code ...

Yes, it would be a slight improvement if slot availability was
detected at ->ndo_start_xmit() time.

But best would be to properly stop the queue at the _end_ of
->ndo_start_xmit() like nearly all ethernet drivers do.

And we can't do that in netback because..... your queues are too
small.

If your queues were large enough you could say "now that I've queued
up SKB for transmit, do I still have enough slots available for a
maxed out SKB?"  and stop the queue if the answer to that question is
no.

The queue state was not meant to be a "maybe I can queue a new packet"
indication.  It's supposed to mean that you can absolutely take at
least one more SKB of any size or configuration.

Ok, how about this:
* ndo_start_xmit tries to set up the grant copy operations, something 
which is done now in the thread
* no estimation madness, just go ahead and try to do it
* if the skb can fit, kick the thread
* if it fails (not enough slots to complete the TX), then:
   * call netif_tx_stop_queue on that queue (just like now)
   * set up timer rx_stalled (just like now)
   * save the state of the current skb (where the grant copy op setup is 
halted)
* if new slots coming in, continue to create the grant copy ops for the 
stalled skb, and if it succeeds, kick the thread plus call 
netif_tx_start_queue. (just like now)
* if the timer fires, drop the stalled skb, and set the carrier off, so 
QDisc won't bother to queue packets for a stalled interface
* the thread will only do the actual grant copy hypercall and releasing 
the skb
* in any case, ndo_start_xmit should return NETDEV_TX_OK, just like now

Returning TX_BUSY from ->ndo_start_xmit() is fundamentally, therefore,
more like an error condition rather than something that should occur
under normal circumstances.  If your queue is up, you should be able
to accept any one single packet.  That's the rule.

Requeueing back into the qdisc is expensive and takes a lot of locks
awkwardly.  You do not want the kernel taking this code path.

Ok, thanks for clearing that up, I thought it works differently.

On 06/08/14 00:07, David Miller wrote:

From: Zoltan Kiss <redacted>
Date: Mon, 4 Aug 2014 16:20:56 +0100

quoted

This series starts using carrier off as a way to purge packets when the guest is
not able (or willing) to receive them. It is a much faster way to get rid of
packets waiting for an overwhelmed guest.
The first patch changes current netback code where it relies currently on
netif_carrier_ok.
The second turns off the carrier if the guest times out on a queue, and only
turn it on again if that queue (or queues) resurrects.

Signed-off-by: Zoltan Kiss <redacted>
Signed-off-by: David Vrabel <redacted>

Applied, but I have some reservations:

1) This is starting to bleed what is normally qdisc type policy into the
    driver.

2) There are other drivers that could run into this kind of situation and
    have similar concerns, therefore we should make sure we have a consistent
    approach that such entities use to handle this problem.

Part of the problem is that netif_carrier_off() only partially mimicks
the situation.  It expresses the "transmitter is down so packets
aren't going onto the wire" part, which keeps the watchdog from
spitting out log messages ever time it fires.  But it doesn't deal
with packet freeing policy meanwhile, which I guess is the part that
this patch series is largely trying to address.

David Vrabel pointed out an important question in a reply to the 
previous version of this series: this patch deschedule NAPI if the 
carrier goes down. The backend doesn't receive packets from the guest. 
DavidVr and others said we shouldn't do this, the guest should be able 
to transmit even if it's not able/willing to receive. Other drivers 
doesn't deschedule NAPI at carrier off as well, however the "carrier 
off" information comes from the hardware, not from an untrusted guest 
who is not posting buffers on the receive ring.
I don't have any good argument why I did it the current way, other than 
a hunch that it feels more natural.
David, do you have an opinion on that?

Zoli

From: Zoltan Kiss <redacted>
Date: Thu, 7 Aug 2014 16:51:17 +0100

Ok, how about this:
* ndo_start_xmit tries to set up the grant copy operations, something
* which is done now in the thread
* no estimation madness, just go ahead and try to do it
* if the skb can fit, kick the thread
* if it fails (not enough slots to complete the TX), then:
  * call netif_tx_stop_queue on that queue (just like now)
  * set up timer rx_stalled (just like now)
  * save the state of the current skb (where the grant copy op setup is
  * halted)
* if new slots coming in, continue to create the grant copy ops for the
* stalled skb, and if it succeeds, kick the thread plus call
* netif_tx_start_queue. (just like now)
* if the timer fires, drop the stalled skb, and set the carrier off, so
* QDisc won't bother to queue packets for a stalled interface
* the thread will only do the actual grant copy hypercall and releasing
* the skb
* in any case, ndo_start_xmit should return NETDEV_TX_OK, just like now

It sounds like this would work, and indeed it would abide by the intended
rules of netif_{stop,wake}_queue() and ->ndo_start_xmit()'s return
values.

From: Zoltan Kiss <redacted>
Date: Thu, 7 Aug 2014 17:49:37 +0100

David Vrabel pointed out an important question in a reply to the
previous version of this series: this patch deschedule NAPI if the
carrier goes down. The backend doesn't receive packets from the
guest. DavidVr and others said we shouldn't do this, the guest should
be able to transmit even if it's not able/willing to receive. Other
drivers doesn't deschedule NAPI at carrier off as well, however the
"carrier off" information comes from the hardware, not from an
untrusted guest who is not posting buffers on the receive ring.
I don't have any good argument why I did it the current way, other
than a hunch that it feels more natural.
David, do you have an opinion on that?

Unless you have a strong reason for doing so, I don't think disabling
receives when the TX path backs up is necessary.