[PATCH net v2 0/3] xen-netback: synchronisation between core driver and netback

[PATCH net v2 0/3] xen-netback: synchronisation between core driver and netback

[Wei Liu]

The zero-copy netback has far more interactions with core network driver than
the old copying backend. One significant thing is that netback now relies on
a callback from core driver to correctly release resources.

However correct synchronisation between core driver and netback is missing.
Currently netback relies on a loop to wait for core driver to release
resources. This is proven not enough and erronous recently, partly due to code
structure, partly due to missing synchronisation. Short-live domains like
OpenMirage unikernels can easily trigger race in backend, rendering backend
unresponsive.

This patch series aims to slove this issue by introducing proper
synchronisation between core driver and netback.

Wei.

Change in v2: fix Zoltan's email address in commit message

Wei Liu (3):
  xen-netback: move NAPI add/remove calls
  xen-netback: don't stop dealloc kthread too early
  xen-netback: remove loop waiting function

 drivers/net/xen-netback/common.h    |    5 +++
 drivers/net/xen-netback/interface.c |   57 +++++++++++++++--------------------
 drivers/net/xen-netback/netback.c   |   24 ++++++++++++---
 3 files changed, 49 insertions(+), 37 deletions(-)

-- 
1.7.10.4

[PATCH net v2 1/3] xen-netback: move NAPI add/remove calls

[Wei Liu]

Originally napi_add was in init_queue and napi_del was in deinit_queue,
while kthreads were handled in _connect and _disconnect. Move napi_add
and napi_remove to _connect and _disconnect so that they reside togother
with kthread operations.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Zoltan Kiss <zoltan.kiss@citrix.com>
---
 drivers/net/xen-netback/interface.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index 48a55cd..fdb4fca 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -528,9 +528,6 @@ int xenvif_init_queue(struct xenvif_queue *queue)
 
 	init_timer(&queue->rx_stalled);
 
-	netif_napi_add(queue->vif->dev, &queue->napi, xenvif_poll,
-			XENVIF_NAPI_WEIGHT);
-
 	return 0;
 }
 
@@ -618,6 +615,9 @@ int xenvif_connect(struct xenvif_queue *queue, unsigned long tx_ring_ref,
 	wake_up_process(queue->task);
 	wake_up_process(queue->dealloc_task);
 
+	netif_napi_add(queue->vif->dev, &queue->napi, xenvif_poll,
+			XENVIF_NAPI_WEIGHT);
+
 	return 0;
 
 err_rx_unbind:
@@ -675,6 +675,11 @@ void xenvif_disconnect(struct xenvif *vif)
 
 	for (queue_index = 0; queue_index < num_queues; ++queue_index) {
 		queue = &vif->queues[queue_index];
+		netif_napi_del(&queue->napi);
+	}
+
+	for (queue_index = 0; queue_index < num_queues; ++queue_index) {
+		queue = &vif->queues[queue_index];
 
 		if (queue->task) {
 			del_timer_sync(&queue->rx_stalled);
@@ -708,7 +713,6 @@ void xenvif_disconnect(struct xenvif *vif)
 void xenvif_deinit_queue(struct xenvif_queue *queue)
 {
 	free_xenballooned_pages(MAX_PENDING_REQS, queue->mmap_pages);
-	netif_napi_del(&queue->napi);
 }
 
 void xenvif_free(struct xenvif *vif)
-- 
1.7.10.4

[PATCH net v2 2/3] xen-netback: don't stop dealloc kthread too early

[Wei Liu]

Reference count the number of packets in host stack, so that we don't
stop the deallocation thread too early. If not, we can end up with
xenvif_free permanently waiting for deallocation thread to unmap grefs.

Reported-by: Thomas Leonard <talex5@gmail.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Zoltan Kiss <zoltan.kiss@citrix.com>
---
 drivers/net/xen-netback/common.h    |    5 +++++
 drivers/net/xen-netback/interface.c |   16 ++++++++++++++++
 drivers/net/xen-netback/netback.c   |   24 ++++++++++++++++++++----
 3 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index ef3026f..d9b7f85 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -165,6 +165,8 @@ struct xenvif_queue { /* Per-queue data for xenvif */
 	u16 dealloc_ring[MAX_PENDING_REQS];
 	struct task_struct *dealloc_task;
 	wait_queue_head_t dealloc_wq;
+	wait_queue_head_t inflight_wq;
+	atomic_t inflight_packets;
 
 	/* Use kthread for guest RX */
 	struct task_struct *task;
@@ -329,4 +331,7 @@ extern unsigned int xenvif_max_queues;
 extern struct dentry *xen_netback_dbg_root;
 #endif
 
+void xenvif_inc_inflight_packets(struct xenvif_queue *queue);
+void xenvif_dec_inflight_packets(struct xenvif_queue *queue);
+
 #endif /* __XEN_NETBACK__COMMON_H__ */
diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index fdb4fca..6488801 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -43,6 +43,17 @@
 #define XENVIF_QUEUE_LENGTH 32
 #define XENVIF_NAPI_WEIGHT  64
 
+void xenvif_inc_inflight_packets(struct xenvif_queue *queue)
+{
+	atomic_inc(&queue->inflight_packets);
+}
+
+void xenvif_dec_inflight_packets(struct xenvif_queue *queue)
+{
+	if (atomic_dec_and_test(&queue->inflight_packets))
+		wake_up(&queue->inflight_wq);
+}
+
 static inline void xenvif_stop_queue(struct xenvif_queue *queue)
 {
 	struct net_device *dev = queue->vif->dev;
@@ -561,6 +572,8 @@ int xenvif_connect(struct xenvif_queue *queue, unsigned long tx_ring_ref,
 
 	init_waitqueue_head(&queue->wq);
 	init_waitqueue_head(&queue->dealloc_wq);
+	init_waitqueue_head(&queue->inflight_wq);
+	atomic_set(&queue->inflight_packets, 0);
 
 	if (tx_evtchn == rx_evtchn) {
 		/* feature-split-event-channels == 0 */
@@ -687,6 +700,9 @@ void xenvif_disconnect(struct xenvif *vif)
 			queue->task = NULL;
 		}
 
+		wait_event(queue->inflight_wq,
+			   atomic_read(&queue->inflight_packets) == 0);
+
 		if (queue->dealloc_task) {
 			kthread_stop(queue->dealloc_task);
 			queue->dealloc_task = NULL;
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 4734472..d2f0c7d7 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -107,6 +107,18 @@ static inline unsigned long idx_to_kaddr(struct xenvif_queue *queue,
 #define callback_param(vif, pending_idx) \
 	(vif->pending_tx_info[pending_idx].callback_struct)
 
+/* This function is used to set SKBTX_DEV_ZEROCOPY as well as
+ * increasing the inflight counter. We need to increase the inflight
+ * counter because core driver calls into xenvif_zerocopy_callback
+ * which calls xenvif_dec_inflight_packets.
+ */
+static void set_skb_zerocopy(struct xenvif_queue *queue,
+			     struct sk_buff *skb)
+{
+	skb_shinfo(skb)->tx_flags |= SKBTX_DEV_ZEROCOPY;
+	xenvif_inc_inflight_packets(queue);
+}
+
 /* Find the containing VIF's structure from a pointer in pending_tx_info array
  */
 static inline struct xenvif_queue *ubuf_to_queue(const struct ubuf_info *ubuf)
@@ -1525,10 +1537,13 @@ static int xenvif_handle_frag_list(struct xenvif_queue *queue, struct sk_buff *s
 	/* remove traces of mapped pages and frag_list */
 	skb_frag_list_init(skb);
 	uarg = skb_shinfo(skb)->destructor_arg;
+	/* See comment on set_skb_zerocopy */
+	if (uarg->callback == xenvif_zerocopy_callback)
+		xenvif_inc_inflight_packets(queue);
 	uarg->callback(uarg, true);
 	skb_shinfo(skb)->destructor_arg = NULL;
 
-	skb_shinfo(nskb)->tx_flags |= SKBTX_DEV_ZEROCOPY;
+	set_skb_zerocopy(queue, nskb);
 	kfree_skb(nskb);
 
 	return 0;
@@ -1589,7 +1604,7 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
 				if (net_ratelimit())
 					netdev_err(queue->vif->dev,
 						   "Not enough memory to consolidate frag_list!\n");
-				skb_shinfo(skb)->tx_flags |= SKBTX_DEV_ZEROCOPY;
+				set_skb_zerocopy(queue, skb);
 				kfree_skb(skb);
 				continue;
 			}
@@ -1609,7 +1624,7 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
 				   "Can't setup checksum in net_tx_action\n");
 			/* We have to set this flag to trigger the callback */
 			if (skb_shinfo(skb)->destructor_arg)
-				skb_shinfo(skb)->tx_flags |= SKBTX_DEV_ZEROCOPY;
+				set_skb_zerocopy(queue, skb);
 			kfree_skb(skb);
 			continue;
 		}
@@ -1641,7 +1656,7 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
 		 * skb. E.g. the __pskb_pull_tail earlier can do such thing.
 		 */
 		if (skb_shinfo(skb)->destructor_arg) {
-			skb_shinfo(skb)->tx_flags |= SKBTX_DEV_ZEROCOPY;
+			set_skb_zerocopy(queue, skb);
 			queue->stats.tx_zerocopy_sent++;
 		}
 
@@ -1681,6 +1696,7 @@ void xenvif_zerocopy_callback(struct ubuf_info *ubuf, bool zerocopy_success)
 		queue->stats.tx_zerocopy_success++;
 	else
 		queue->stats.tx_zerocopy_fail++;
+	xenvif_dec_inflight_packets(queue);
 }
 
 static inline void xenvif_tx_dealloc_action(struct xenvif_queue *queue)
-- 
1.7.10.4

[PATCH net v2 3/3] xen-netback: remove loop waiting function

[Wei Liu]

The original implementation relies on a loop to check if all inflight
packets are freed. Now we have proper reference counting, there's no
need to use loop anymore.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Zoltan Kiss <zoltan.kiss@citrix.com>
---
 drivers/net/xen-netback/interface.c |   29 -----------------------------
 1 file changed, 29 deletions(-)

diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index 6488801..3bb1104 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -659,25 +659,6 @@ void xenvif_carrier_off(struct xenvif *vif)
 	rtnl_unlock();
 }
 
-static void xenvif_wait_unmap_timeout(struct xenvif_queue *queue,
-				      unsigned int worst_case_skb_lifetime)
-{
-	int i, unmap_timeout = 0;
-
-	for (i = 0; i < MAX_PENDING_REQS; ++i) {
-		if (queue->grant_tx_handle[i] != NETBACK_INVALID_HANDLE) {
-			unmap_timeout++;
-			schedule_timeout(msecs_to_jiffies(1000));
-			if (unmap_timeout > worst_case_skb_lifetime &&
-			    net_ratelimit())
-				netdev_err(queue->vif->dev,
-					   "Page still granted! Index: %x\n",
-					   i);
-			i = -1;
-		}
-	}
-}
-
 void xenvif_disconnect(struct xenvif *vif)
 {
 	struct xenvif_queue *queue = NULL;
@@ -736,21 +717,11 @@ void xenvif_free(struct xenvif *vif)
 	struct xenvif_queue *queue = NULL;
 	unsigned int num_queues = vif->num_queues;
 	unsigned int queue_index;
-	/* 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, 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);
 
 	unregister_netdev(vif->dev);
 
 	for (queue_index = 0; queue_index < num_queues; ++queue_index) {
 		queue = &vif->queues[queue_index];
-		xenvif_wait_unmap_timeout(queue, worst_case_skb_lifetime);
 		xenvif_deinit_queue(queue);
 	}
 
-- 
1.7.10.4

Re: [PATCH net v2 1/3] xen-netback: move NAPI add/remove calls

[Sergei Shtylyov]

Hello.

On 08/08/2014 08:37 PM, Wei Liu wrote:

> Originally napi_add was in init_queue and napi_del was in deinit_queue,
> while kthreads were handled in _connect and _disconnect. Move napi_add
> and napi_remove

    netif_napi_{add|del}()?

 > to _connect and _disconnect so that they reside togother

    Together.

> with kthread operations.

> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Zoltan Kiss <zoltan.kiss@citrix.com>

WBR, Sergei

Re: [PATCH net v2 1/3] xen-netback: move NAPI add/remove calls

[Wei Liu]

On Fri, Aug 08, 2014 at 08:49:10PM +0400, Sergei Shtylyov wrote:
> Hello.
> 
> On 08/08/2014 08:37 PM, Wei Liu wrote:
> 
> >Originally napi_add was in init_queue and napi_del was in deinit_queue,
> >while kthreads were handled in _connect and _disconnect. Move napi_add
> >and napi_remove
> 
>    netif_napi_{add|del}()?
> 
> > to _connect and _disconnect so that they reside togother
> 
>    Together.
> 

Thanks, I will fix these issues.

Wei.

Re: [Xen-devel] [PATCH net v2 2/3] xen-netback: don't stop dealloc kthread too early

[David Vrabel]

On 08/08/14 17:37, Wei Liu wrote:
> Reference count the number of packets in host stack, so that we don't
> stop the deallocation thread too early. If not, we can end up with
> xenvif_free permanently waiting for deallocation thread to unmap grefs.
[...]
> --- a/drivers/net/xen-netback/interface.c
> +++ b/drivers/net/xen-netback/interface.c
> @@ -43,6 +43,17 @@
>  #define XENVIF_QUEUE_LENGTH 32
>  #define XENVIF_NAPI_WEIGHT  64
>  
> +void xenvif_inc_inflight_packets(struct xenvif_queue *queue)
> +{
> +	atomic_inc(&queue->inflight_packets);
> +}
> +
> +void xenvif_dec_inflight_packets(struct xenvif_queue *queue)
> +{
> +	if (atomic_dec_and_test(&queue->inflight_packets))
> +		wake_up(&queue->inflight_wq);
> +}

You don't need these wrappers if you remove the inflight_wq (see below).

>  static inline void xenvif_stop_queue(struct xenvif_queue *queue)
>  {
>  	struct net_device *dev = queue->vif->dev;
> @@ -561,6 +572,8 @@ int xenvif_connect(struct xenvif_queue *queue, unsigned long tx_ring_ref,
>  
>  	init_waitqueue_head(&queue->wq);
>  	init_waitqueue_head(&queue->dealloc_wq);
> +	init_waitqueue_head(&queue->inflight_wq);
> +	atomic_set(&queue->inflight_packets, 0);
>  
>  	if (tx_evtchn == rx_evtchn) {
>  		/* feature-split-event-channels == 0 */
> @@ -687,6 +700,9 @@ void xenvif_disconnect(struct xenvif *vif)
>  			queue->task = NULL;
>  		}
>  
> +		wait_event(queue->inflight_wq,
> +			   atomic_read(&queue->inflight_packets) == 0);

Just make the dealloc task not stop unless it has deallocated all
outstanding requests.  There's no need for another wait queue here.

> +
>  		if (queue->dealloc_task) {
>  			kthread_stop(queue->dealloc_task);
>  			queue->dealloc_task = NULL;
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index 4734472..d2f0c7d7 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -107,6 +107,18 @@ static inline unsigned long idx_to_kaddr(struct xenvif_queue *queue,
>  #define callback_param(vif, pending_idx) \
>  	(vif->pending_tx_info[pending_idx].callback_struct)
>  
> +/* This function is used to set SKBTX_DEV_ZEROCOPY as well as
> + * increasing the inflight counter. We need to increase the inflight
> + * counter because core driver calls into xenvif_zerocopy_callback
> + * which calls xenvif_dec_inflight_packets.
> + */
> +static void set_skb_zerocopy(struct xenvif_queue *queue,
> +			     struct sk_buff *skb)
> +{
> +	skb_shinfo(skb)->tx_flags |= SKBTX_DEV_ZEROCOPY;
> +	xenvif_inc_inflight_packets(queue);
> +}

This name makes this look like a core function instead of a netback
specific one.

I would suggest a pair of functions:

xenvif_skb_zerocopy_prepare()
xenvif_skb_zerocopy_complete()

Or similar.

David

Re: [Xen-devel] [PATCH net v2 1/3] xen-netback: move NAPI add/remove calls

[David Vrabel]

On 08/08/14 17:37, Wei Liu wrote:
> Originally napi_add was in init_queue and napi_del was in deinit_queue,
> while kthreads were handled in _connect and _disconnect. Move napi_add
> and napi_remove to _connect and _disconnect so that they reside togother
> with kthread operations.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Zoltan Kiss <zoltan.kiss@citrix.com>
> ---
>  drivers/net/xen-netback/interface.c |   12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
> index 48a55cd..fdb4fca 100644
> --- a/drivers/net/xen-netback/interface.c
> +++ b/drivers/net/xen-netback/interface.c
> @@ -528,9 +528,6 @@ int xenvif_init_queue(struct xenvif_queue *queue)
>  
>  	init_timer(&queue->rx_stalled);
>  
> -	netif_napi_add(queue->vif->dev, &queue->napi, xenvif_poll,
> -			XENVIF_NAPI_WEIGHT);
> -
>  	return 0;
>  }
>  
> @@ -618,6 +615,9 @@ int xenvif_connect(struct xenvif_queue *queue, unsigned long tx_ring_ref,
>  	wake_up_process(queue->task);
>  	wake_up_process(queue->dealloc_task);
>  
> +	netif_napi_add(queue->vif->dev, &queue->napi, xenvif_poll,
> +			XENVIF_NAPI_WEIGHT);
> +
>  	return 0;
>  
>  err_rx_unbind:
> @@ -675,6 +675,11 @@ void xenvif_disconnect(struct xenvif *vif)
>  
>  	for (queue_index = 0; queue_index < num_queues; ++queue_index) {
>  		queue = &vif->queues[queue_index];
> +		netif_napi_del(&queue->napi);
> +	}

Why have you added an additional loop over all the queues?  The ordering
looks wrong as well.  I think you want

  1. unbind from irqhandler
  2. napi del
  3. stop task
  4. stop dealloc task
  5. unmap frontend rings.

David

Re: [Xen-devel] [PATCH net v2 1/3] xen-netback: move NAPI add/remove calls

[Zoltan Kiss]

On 11/08/14 13:35, David Vrabel wrote:
> On 08/08/14 17:37, Wei Liu wrote:
>> Originally napi_add was in init_queue and napi_del was in deinit_queue,
>> while kthreads were handled in _connect and _disconnect. Move napi_add
>> and napi_remove to _connect and _disconnect so that they reside togother
>> with kthread operations.
>>
>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Zoltan Kiss <zoltan.kiss@citrix.com>
>> ---
>>   drivers/net/xen-netback/interface.c |   12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
>> index 48a55cd..fdb4fca 100644
>> --- a/drivers/net/xen-netback/interface.c
>> +++ b/drivers/net/xen-netback/interface.c
>> @@ -528,9 +528,6 @@ int xenvif_init_queue(struct xenvif_queue *queue)
>>
>>   	init_timer(&queue->rx_stalled);
>>
>> -	netif_napi_add(queue->vif->dev, &queue->napi, xenvif_poll,
>> -			XENVIF_NAPI_WEIGHT);
>> -
>>   	return 0;
>>   }
>>
>> @@ -618,6 +615,9 @@ int xenvif_connect(struct xenvif_queue *queue, unsigned long tx_ring_ref,
>>   	wake_up_process(queue->task);
>>   	wake_up_process(queue->dealloc_task);
>>
>> +	netif_napi_add(queue->vif->dev, &queue->napi, xenvif_poll,
>> +			XENVIF_NAPI_WEIGHT);
>> +
>>   	return 0;
>>
>>   err_rx_unbind:
>> @@ -675,6 +675,11 @@ void xenvif_disconnect(struct xenvif *vif)
>>
>>   	for (queue_index = 0; queue_index < num_queues; ++queue_index) {
>>   		queue = &vif->queues[queue_index];
>> +		netif_napi_del(&queue->napi);
>> +	}
>
> Why have you added an additional loop over all the queues?  The ordering
> looks wrong as well.  I think you want
>
>    1. unbind from irqhandler
>    2. napi del
>    3. stop task
>    4. stop dealloc task
>    5. unmap frontend rings.
And that's how they are ordered. The idea for having the netif_napi_del 
as a separate loop came from me: it could be more efficient to start 
tearing down all the NAPI instances first, so by the time we stop the 
dealloc thread, it is likely it already done most of the work.
But now I realized that netif_napi_del just delete the instance from a 
list, the real thing happens in xenvif_carrier_off: xenvif_down calls 
napi_disable on all queues, and it waits until all the work is done. So 
it doesn't makes sense to have the netif_napi_del in a separate loop any 
more.
>
> David
>

Re: [Xen-devel] [PATCH net v2 1/3] xen-netback: move NAPI add/remove calls

[David Vrabel]

On 11/08/14 13:49, Zoltan Kiss wrote:
> On 11/08/14 13:35, David Vrabel wrote:
>> On 08/08/14 17:37, Wei Liu wrote:
>>> Originally napi_add was in init_queue and napi_del was in deinit_queue,
>>> while kthreads were handled in _connect and _disconnect. Move napi_add
>>> and napi_remove to _connect and _disconnect so that they reside togother
>>> with kthread operations.
>>>
>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>> Cc: Ian Campbell <ian.campbell@citrix.com>
>>> Cc: Zoltan Kiss <zoltan.kiss@citrix.com>
>>> ---
>>>   drivers/net/xen-netback/interface.c |   12 ++++++++----
>>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/xen-netback/interface.c
>>> b/drivers/net/xen-netback/interface.c
>>> index 48a55cd..fdb4fca 100644
>>> --- a/drivers/net/xen-netback/interface.c
>>> +++ b/drivers/net/xen-netback/interface.c
>>> @@ -528,9 +528,6 @@ int xenvif_init_queue(struct xenvif_queue *queue)
>>>
>>>       init_timer(&queue->rx_stalled);
>>>
>>> -    netif_napi_add(queue->vif->dev, &queue->napi, xenvif_poll,
>>> -            XENVIF_NAPI_WEIGHT);
>>> -
>>>       return 0;
>>>   }
>>>
>>> @@ -618,6 +615,9 @@ int xenvif_connect(struct xenvif_queue *queue,
>>> unsigned long tx_ring_ref,
>>>       wake_up_process(queue->task);
>>>       wake_up_process(queue->dealloc_task);
>>>
>>> +    netif_napi_add(queue->vif->dev, &queue->napi, xenvif_poll,
>>> +            XENVIF_NAPI_WEIGHT);
>>> +
>>>       return 0;
>>>
>>>   err_rx_unbind:
>>> @@ -675,6 +675,11 @@ void xenvif_disconnect(struct xenvif *vif)
>>>
>>>       for (queue_index = 0; queue_index < num_queues; ++queue_index) {
>>>           queue = &vif->queues[queue_index];
>>> +        netif_napi_del(&queue->napi);
>>> +    }
>>
>> Why have you added an additional loop over all the queues?  The ordering
>> looks wrong as well.  I think you want
>>
>>    1. unbind from irqhandler
>>    2. napi del
>>    3. stop task
>>    4. stop dealloc task
>>    5. unmap frontend rings.
> And that's how they are ordered.

No, it isn't.  Did you mistakenly look at netfront which is correctly
ordered already?

You must unbind the irq handler before calling netif_napi_del() or an
interrupt may occur and the handler may call napi_schedule() with a
deleted instance.

David

Re: [Xen-devel] [PATCH net v2 1/3] xen-netback: move NAPI add/remove calls

[Zoltan Kiss]

On 11/08/14 14:01, David Vrabel wrote:
> On 11/08/14 13:49, Zoltan Kiss wrote:
>> On 11/08/14 13:35, David Vrabel wrote:
>>> On 08/08/14 17:37, Wei Liu wrote:
>>>> Originally napi_add was in init_queue and napi_del was in deinit_queue,
>>>> while kthreads were handled in _connect and _disconnect. Move napi_add
>>>> and napi_remove to _connect and _disconnect so that they reside togother
>>>> with kthread operations.
>>>>
>>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>>> Cc: Ian Campbell <ian.campbell@citrix.com>
>>>> Cc: Zoltan Kiss <zoltan.kiss@citrix.com>
>>>> ---
>>>>    drivers/net/xen-netback/interface.c |   12 ++++++++----
>>>>    1 file changed, 8 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/net/xen-netback/interface.c
>>>> b/drivers/net/xen-netback/interface.c
>>>> index 48a55cd..fdb4fca 100644
>>>> --- a/drivers/net/xen-netback/interface.c
>>>> +++ b/drivers/net/xen-netback/interface.c
>>>> @@ -528,9 +528,6 @@ int xenvif_init_queue(struct xenvif_queue *queue)
>>>>
>>>>        init_timer(&queue->rx_stalled);
>>>>
>>>> -    netif_napi_add(queue->vif->dev, &queue->napi, xenvif_poll,
>>>> -            XENVIF_NAPI_WEIGHT);
>>>> -
>>>>        return 0;
>>>>    }
>>>>
>>>> @@ -618,6 +615,9 @@ int xenvif_connect(struct xenvif_queue *queue,
>>>> unsigned long tx_ring_ref,
>>>>        wake_up_process(queue->task);
>>>>        wake_up_process(queue->dealloc_task);
>>>>
>>>> +    netif_napi_add(queue->vif->dev, &queue->napi, xenvif_poll,
>>>> +            XENVIF_NAPI_WEIGHT);
>>>> +
>>>>        return 0;
>>>>
>>>>    err_rx_unbind:
>>>> @@ -675,6 +675,11 @@ void xenvif_disconnect(struct xenvif *vif)
>>>>
>>>>        for (queue_index = 0; queue_index < num_queues; ++queue_index) {
>>>>            queue = &vif->queues[queue_index];
>>>> +        netif_napi_del(&queue->napi);
>>>> +    }
>>>
>>> Why have you added an additional loop over all the queues?  The ordering
>>> looks wrong as well.  I think you want
>>>
>>>     1. unbind from irqhandler
>>>     2. napi del
>>>     3. stop task
>>>     4. stop dealloc task
>>>     5. unmap frontend rings.
>> And that's how they are ordered.
>
> No, it isn't.  Did you mistakenly look at netfront which is correctly
> ordered already?
>
> You must unbind the irq handler before calling netif_napi_del() or an
> interrupt may occur and the handler may call napi_schedule() with a
> deleted instance.
I think xenvif_carrier_off (which call xenvif_down) does that. It is 
right before this new napi_del in xenvif_disconnect.

Zoli

Re: [Xen-devel] [PATCH net v2 1/3] xen-netback: move NAPI add/remove calls

[Wei Liu]

In short, there's no need to reorder disconnect logic and no need have a
dedicated loop for netif_napi_del.

Wei.

Re: [Xen-devel] [PATCH net v2 2/3] xen-netback: don't stop dealloc kthread too early

[Wei Liu]

On Mon, Aug 11, 2014 at 01:10:12PM +0100, David Vrabel wrote:
> On 08/08/14 17:37, Wei Liu wrote:
[...]
> >  	if (tx_evtchn == rx_evtchn) {
> >  		/* feature-split-event-channels == 0 */
> > @@ -687,6 +700,9 @@ void xenvif_disconnect(struct xenvif *vif)
> >  			queue->task = NULL;
> >  		}
> >  
> > +		wait_event(queue->inflight_wq,
> > +			   atomic_read(&queue->inflight_packets) == 0);
> 
> Just make the dealloc task not stop unless it has deallocated all
> outstanding requests.  There's no need for another wait queue here.
> 

Are you suggesting something like

  while (atomic_read(&queue->inflight_packets) !=0)
  	schedule_timeout(SOME_TIMEOUT);

?

> > +
> >  		if (queue->dealloc_task) {
> >  			kthread_stop(queue->dealloc_task);
> >  			queue->dealloc_task = NULL;
> > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> > index 4734472..d2f0c7d7 100644
> > --- a/drivers/net/xen-netback/netback.c
> > +++ b/drivers/net/xen-netback/netback.c
> > @@ -107,6 +107,18 @@ static inline unsigned long idx_to_kaddr(struct xenvif_queue *queue,
> >  #define callback_param(vif, pending_idx) \
> >  	(vif->pending_tx_info[pending_idx].callback_struct)
> >  
> > +/* This function is used to set SKBTX_DEV_ZEROCOPY as well as
> > + * increasing the inflight counter. We need to increase the inflight
> > + * counter because core driver calls into xenvif_zerocopy_callback
> > + * which calls xenvif_dec_inflight_packets.
> > + */
> > +static void set_skb_zerocopy(struct xenvif_queue *queue,
> > +			     struct sk_buff *skb)
> > +{
> > +	skb_shinfo(skb)->tx_flags |= SKBTX_DEV_ZEROCOPY;
> > +	xenvif_inc_inflight_packets(queue);
> > +}
> 
> This name makes this look like a core function instead of a netback
> specific one.
> 
> I would suggest a pair of functions:
> 
> xenvif_skb_zerocopy_prepare()
> xenvif_skb_zerocopy_complete()
> 

This will do.

Wei.

> Or similar.
> 
> David

Re: [Xen-devel] [PATCH net v2 2/3] xen-netback: don't stop dealloc kthread too early

[David Vrabel]

On 11/08/14 14:48, Wei Liu wrote:
> On Mon, Aug 11, 2014 at 01:10:12PM +0100, David Vrabel wrote:
>> On 08/08/14 17:37, Wei Liu wrote:
> [...]
>>>  	if (tx_evtchn == rx_evtchn) {
>>>  		/* feature-split-event-channels == 0 */
>>> @@ -687,6 +700,9 @@ void xenvif_disconnect(struct xenvif *vif)
>>>  			queue->task = NULL;
>>>  		}
>>>  
>>> +		wait_event(queue->inflight_wq,
>>> +			   atomic_read(&queue->inflight_packets) == 0);
>>
>> Just make the dealloc task not stop unless it has deallocated all
>> outstanding requests.  There's no need for another wait queue here.
>>
> 
> Are you suggesting something like
> 
>   while (atomic_read(&queue->inflight_packets) !=0)
>   	schedule_timeout(SOME_TIMEOUT);

No. That would be awful!

Add the test to the kthread itself:

int xenvif_dealloc_kthread(void *data)
{
	struct xenvif_queue *queue = data;

	while (atomic_read(&queue->inflight_packets) > 0
               || !kthread_should_stop()) {
            [...]

etc.

Although, the main loop is a bit confused, so I suggest adding:

static bool xenvif_dealloc_thread_should_stop(struct xenvif_queue *q)
{
    /* Dealloc thread must remain running if there are any inflight
     * packets so it can properly dealloc them when they complete.
     */
    return atomic_read(&queue->inflight_packets) == 0
        && kthread_should_stop();
}

And cleaning it up a bit (the while() could be a for(;;)).

David

Re: [Xen-devel] [PATCH net v2 1/3] xen-netback: move NAPI add/remove calls

[David Vrabel]

On 11/08/14 14:43, Wei Liu wrote:
> In short, there's no need to reorder disconnect logic and no need have a
> dedicated loop for netif_napi_del.

Not for now.  But I would prefer it if it was re-ordered.  And similarly
in xenvif_down(), irq_disable() should be before napi_disable().

David

Re: [Xen-devel] [PATCH net v2 1/3] xen-netback: move NAPI add/remove calls

[Wei Liu]

On Mon, Aug 11, 2014 at 02:59:52PM +0100, David Vrabel wrote:
> On 11/08/14 14:43, Wei Liu wrote:
> > In short, there's no need to reorder disconnect logic and no need have a
> > dedicated loop for netif_napi_del.
> 
> Not for now.  But I would prefer it if it was re-ordered.  And similarly
> in xenvif_down(), irq_disable() should be before napi_disable().
> 

Something for another day then. In any case, even if napi_disable goes
before irq_disable, it's still safe if a tx interrupt takes place in
between. napi_schedule has no effect on a disabled instance.

Wei.

> David

Re: [Xen-devel] [PATCH net v2 2/3] xen-netback: don't stop dealloc kthread too early

[Zoltan Kiss]

On 11/08/14 14:58, David Vrabel wrote:
> On 11/08/14 14:48, Wei Liu wrote:
>> On Mon, Aug 11, 2014 at 01:10:12PM +0100, David Vrabel wrote:
>>> On 08/08/14 17:37, Wei Liu wrote:
>> [...]
>>>>   	if (tx_evtchn == rx_evtchn) {
>>>>   		/* feature-split-event-channels == 0 */
>>>> @@ -687,6 +700,9 @@ void xenvif_disconnect(struct xenvif *vif)
>>>>   			queue->task = NULL;
>>>>   		}
>>>>   
>>>> +		wait_event(queue->inflight_wq,
>>>> +			   atomic_read(&queue->inflight_packets) == 0);
>>>
>>> Just make the dealloc task not stop unless it has deallocated all
>>> outstanding requests.  There's no need for another wait queue here.
>>>
>>
>> Are you suggesting something like
>>
>>    while (atomic_read(&queue->inflight_packets) !=0)
>>    	schedule_timeout(SOME_TIMEOUT);
> 
> No. That would be awful!
> 
> Add the test to the kthread itself:
> 
> int xenvif_dealloc_kthread(void *data)
> {
> 	struct xenvif_queue *queue = data;
> 
> 	while (atomic_read(&queue->inflight_packets) > 0
>                 || !kthread_should_stop()) {
>              [...]
> 
> etc.
> 
> Although, the main loop is a bit confused, so I suggest adding:
> 
> static bool xenvif_dealloc_thread_should_stop(struct xenvif_queue *q)
> {
>      /* Dealloc thread must remain running if there are any inflight
>       * packets so it can properly dealloc them when they complete.
>       */
>      return atomic_read(&queue->inflight_packets) == 0
>          && kthread_should_stop();
> }
> 
> And cleaning it up a bit (the while() could be a for(;;)).

I would recommend this:
---
@@ -2066,7 +2066,7 @@ int xenvif_dealloc_kthread(void *data)
 		wait_event_interruptible(queue->dealloc_wq,
 					 tx_dealloc_work_todo(queue) ||
 					 kthread_should_stop());
-		if (kthread_should_stop())
+		if (kthread_should_stop() && !atomic_read(&queue->inflight_packets))
 			break;
 
 		xenvif_tx_dealloc_action(queue);
---
If kthread_stop called, this will keep the main loop running until all callbacks are called.
Then it proceeds to the exit branch, otherwise doesn't disrupt normal operation.

Re: [Xen-devel] [PATCH net v2 2/3] xen-netback: don't stop dealloc kthread too early

[Wei Liu]

On Mon, Aug 11, 2014 at 02:58:13PM +0100, David Vrabel wrote:
> On 11/08/14 14:48, Wei Liu wrote:
> > On Mon, Aug 11, 2014 at 01:10:12PM +0100, David Vrabel wrote:
> >> On 08/08/14 17:37, Wei Liu wrote:
> > [...]
> >>>  	if (tx_evtchn == rx_evtchn) {
> >>>  		/* feature-split-event-channels == 0 */
> >>> @@ -687,6 +700,9 @@ void xenvif_disconnect(struct xenvif *vif)
> >>>  			queue->task = NULL;
> >>>  		}
> >>>  
> >>> +		wait_event(queue->inflight_wq,
> >>> +			   atomic_read(&queue->inflight_packets) == 0);
> >>
> >> Just make the dealloc task not stop unless it has deallocated all
> >> outstanding requests.  There's no need for another wait queue here.
> >>
> > 
> > Are you suggesting something like
> > 
> >   while (atomic_read(&queue->inflight_packets) !=0)
> >   	schedule_timeout(SOME_TIMEOUT);
> 
> No. That would be awful!
> 
> Add the test to the kthread itself:
> 
> int xenvif_dealloc_kthread(void *data)
> {
> 	struct xenvif_queue *queue = data;
> 
> 	while (atomic_read(&queue->inflight_packets) > 0
>                || !kthread_should_stop()) {
>             [...]
> 
> etc.
> 
> Although, the main loop is a bit confused, so I suggest adding:
> 
> static bool xenvif_dealloc_thread_should_stop(struct xenvif_queue *q)
> {
>     /* Dealloc thread must remain running if there are any inflight
>      * packets so it can properly dealloc them when they complete.
>      */
>     return atomic_read(&queue->inflight_packets) == 0
>         && kthread_should_stop();
> }
> 
> And cleaning it up a bit (the while() could be a for(;;)).
> 

Unfortunately this approach is bogus. If xenbus thread is not blocked it
can free up various resources while dealloc thread is running -- queue
can be gone under dealloc thread's feet.

Wei.

> David

Re: [Xen-devel] [PATCH net v2 2/3] xen-netback: don't stop dealloc kthread too early

[David Vrabel]

On 11/08/14 15:31, Wei Liu wrote:
> On Mon, Aug 11, 2014 at 02:58:13PM +0100, David Vrabel wrote:
>> On 11/08/14 14:48, Wei Liu wrote:
>>> On Mon, Aug 11, 2014 at 01:10:12PM +0100, David Vrabel wrote:
>>>> On 08/08/14 17:37, Wei Liu wrote:
>>> [...]
>>>>>  	if (tx_evtchn == rx_evtchn) {
>>>>>  		/* feature-split-event-channels == 0 */
>>>>> @@ -687,6 +700,9 @@ void xenvif_disconnect(struct xenvif *vif)
>>>>>  			queue->task = NULL;
>>>>>  		}
>>>>>  
>>>>> +		wait_event(queue->inflight_wq,
>>>>> +			   atomic_read(&queue->inflight_packets) == 0);
>>>>
>>>> Just make the dealloc task not stop unless it has deallocated all
>>>> outstanding requests.  There's no need for another wait queue here.
>>>>
>>>
>>> Are you suggesting something like
>>>
>>>   while (atomic_read(&queue->inflight_packets) !=0)
>>>   	schedule_timeout(SOME_TIMEOUT);
>>
>> No. That would be awful!
>>
>> Add the test to the kthread itself:
>>
>> int xenvif_dealloc_kthread(void *data)
>> {
>> 	struct xenvif_queue *queue = data;
>>
>> 	while (atomic_read(&queue->inflight_packets) > 0
>>                || !kthread_should_stop()) {
>>             [...]
>>
>> etc.
>>
>> Although, the main loop is a bit confused, so I suggest adding:
>>
>> static bool xenvif_dealloc_thread_should_stop(struct xenvif_queue *q)
>> {
>>     /* Dealloc thread must remain running if there are any inflight
>>      * packets so it can properly dealloc them when they complete.
>>      */
>>     return atomic_read(&queue->inflight_packets) == 0
>>         && kthread_should_stop();
>> }
>>
>> And cleaning it up a bit (the while() could be a for(;;)).
>>
> 
> Unfortunately this approach is bogus. If xenbus thread is not blocked it
> can free up various resources while dealloc thread is running -- queue
> can be gone under dealloc thread's feet.

kthread_stop() waits until the thread exits (like pthread_join()).

/**
 * kthread_stop - stop a thread created by kthread_create().
 * @k: thread created by kthread_create().
 *
 * Sets kthread_should_stop() for @k to return true, wakes it, and
 * waits for it to exit.

David

Re: [Xen-devel] [PATCH net v2 2/3] xen-netback: don't stop dealloc kthread too early

[Wei Liu]

On Mon, Aug 11, 2014 at 03:34:48PM +0100, David Vrabel wrote:
[...]
> >>
> >> And cleaning it up a bit (the while() could be a for(;;)).
> >>
> > 
> > Unfortunately this approach is bogus. If xenbus thread is not blocked it
> > can free up various resources while dealloc thread is running -- queue
> > can be gone under dealloc thread's feet.
> 
> kthread_stop() waits until the thread exits (like pthread_join()).
> 
> /**
>  * kthread_stop - stop a thread created by kthread_create().
>  * @k: thread created by kthread_create().
>  *
>  * Sets kthread_should_stop() for @k to return true, wakes it, and
>  * waits for it to exit.
> 

Ah, misremeber the behaviour of kthread_stop. Sorry for the noise.

Wei.

> David

Re: [Xen-devel] [PATCH net v2 2/3] xen-netback: don't stop dealloc kthread too early

[Wei Liu]

On Mon, Aug 11, 2014 at 03:13:41PM +0100, Zoltan Kiss wrote:
[...]
> > And cleaning it up a bit (the while() could be a for(;;)).
> 
> I would recommend this:
> ---
> @@ -2066,7 +2066,7 @@ int xenvif_dealloc_kthread(void *data)
>  		wait_event_interruptible(queue->dealloc_wq,
>  					 tx_dealloc_work_todo(queue) ||
>  					 kthread_should_stop());
> -		if (kthread_should_stop())
> +		if (kthread_should_stop() && !atomic_read(&queue->inflight_packets))
>  			break;
>  
>  		xenvif_tx_dealloc_action(queue);
> ---
> If kthread_stop called, this will keep the main loop running until all callbacks are called.
> Then it proceeds to the exit branch, otherwise doesn't disrupt normal operation.

This snippet lacks change to while().

I would generally go for a shorter solution if the code is
self-explanatory.

@@ -2078,21 +2066,19 @@ int xenvif_dealloc_kthread(void *data)
 {
        struct xenvif_queue *queue = data;
 
-       while (!kthread_should_stop()) {
+       for (;;) {
                wait_event_interruptible(queue->dealloc_wq,
                                         tx_dealloc_work_todo(queue) ||
                                         kthread_should_stop());
-               if (kthread_should_stop())
+               if (kthread_should_stop() &&
+                   !atomic_read(&queue->inflight_packets) &&
+                   !tx_dealloc_work_todo(queue))
                        break;
 
                xenvif_tx_dealloc_action(queue);
                cond_resched();
        }
 
-       /* Unmap anything remaining*/
-       if (tx_dealloc_work_todo(queue))
-               xenvif_tx_dealloc_action(queue);
-
        return 0;
 }
 

Re: [Xen-devel] [PATCH net v2 2/3] xen-netback: don't stop dealloc kthread too early

[David Vrabel]

On 11/08/14 15:44, Wei Liu wrote:
> On Mon, Aug 11, 2014 at 03:13:41PM +0100, Zoltan Kiss wrote:
> [...]
>>> And cleaning it up a bit (the while() could be a for(;;)).
>>
>> I would recommend this:
>> ---
>> @@ -2066,7 +2066,7 @@ int xenvif_dealloc_kthread(void *data)
>>  		wait_event_interruptible(queue->dealloc_wq,
>>  					 tx_dealloc_work_todo(queue) ||
>>  					 kthread_should_stop());
>> -		if (kthread_should_stop())
>> +		if (kthread_should_stop() && !atomic_read(&queue->inflight_packets))
>>  			break;
>>  
>>  		xenvif_tx_dealloc_action(queue);
>> ---
>> If kthread_stop called, this will keep the main loop running until all callbacks are called.
>> Then it proceeds to the exit branch, otherwise doesn't disrupt normal operation.
> 
> This snippet lacks change to while().
> 
> I would generally go for a shorter solution if the code is
> self-explanatory.
> 
> @@ -2078,21 +2066,19 @@ int xenvif_dealloc_kthread(void *data)
>  {
>         struct xenvif_queue *queue = data;
>  
> -       while (!kthread_should_stop()) {
> +       for (;;) {
>                 wait_event_interruptible(queue->dealloc_wq,
>                                          tx_dealloc_work_todo(queue) ||
>                                          kthread_should_stop());

This will never sleep if the thread is being stopped when there are
packets in flight.

> -               if (kthread_should_stop())
> +               if (kthread_should_stop() &&
> +                   !atomic_read(&queue->inflight_packets) &&
> +                   !tx_dealloc_work_todo(queue))
>                         break;

Moving the final dealloc into the loop adds a cond_resched() call.  This
is harmless but not really necessary when the thread is about to stop.

>  
>                 xenvif_tx_dealloc_action(queue);
>                 cond_resched();
>         }
>  
> -       /* Unmap anything remaining*/
> -       if (tx_dealloc_work_todo(queue))
> -               xenvif_tx_dealloc_action(queue);
> -
>         return 0;
>  }

David

Re: [Xen-devel] [PATCH net v2 2/3] xen-netback: don't stop dealloc kthread too early

[Wei Liu]

On Mon, Aug 11, 2014 at 04:23:28PM +0100, David Vrabel wrote:
> On 11/08/14 15:44, Wei Liu wrote:
> > On Mon, Aug 11, 2014 at 03:13:41PM +0100, Zoltan Kiss wrote:
> > [...]
> >>> And cleaning it up a bit (the while() could be a for(;;)).
> >>
> >> I would recommend this:
> >> ---
> >> @@ -2066,7 +2066,7 @@ int xenvif_dealloc_kthread(void *data)
> >>  		wait_event_interruptible(queue->dealloc_wq,
> >>  					 tx_dealloc_work_todo(queue) ||
> >>  					 kthread_should_stop());
> >> -		if (kthread_should_stop())
> >> +		if (kthread_should_stop() && !atomic_read(&queue->inflight_packets))
> >>  			break;
> >>  
> >>  		xenvif_tx_dealloc_action(queue);
> >> ---
> >> If kthread_stop called, this will keep the main loop running until all callbacks are called.
> >> Then it proceeds to the exit branch, otherwise doesn't disrupt normal operation.
> > 
> > This snippet lacks change to while().
> > 
> > I would generally go for a shorter solution if the code is
> > self-explanatory.
> > 
> > @@ -2078,21 +2066,19 @@ int xenvif_dealloc_kthread(void *data)
> >  {
> >         struct xenvif_queue *queue = data;
> >  
> > -       while (!kthread_should_stop()) {
> > +       for (;;) {
> >                 wait_event_interruptible(queue->dealloc_wq,
> >                                          tx_dealloc_work_todo(queue) ||
> >                                          kthread_should_stop());
> 
> This will never sleep if the thread is being stopped when there are
> packets in flight.
> 

Good catch.