DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] vhost: flush used->idx update before reading avail->flags
@ 2015-04-22 16:33 Huawei Xie
  2015-04-23 13:57 ` Luke Gorrie
                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Huawei Xie @ 2015-04-22 16:33 UTC (permalink / raw)
  To: dev; +Cc: mst

update of used->idx and read of avail->flags could be reordered.
memory fence should be used to ensure the order, otherwise guest could see a stale used->idx value after it toggles the interrupt suppression flag.

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
 lib/librte_vhost/vhost_rxtx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
index 510ffe8..6afba35 100644
--- a/lib/librte_vhost/vhost_rxtx.c
+++ b/lib/librte_vhost/vhost_rxtx.c
@@ -178,6 +178,9 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
 	*(volatile uint16_t *)&vq->used->idx += count;
 	vq->last_used_idx = res_end_idx;
 
+	/* flush used->idx update before we read avail->flags. */
+	rte_mb();
+
 	/* Kick the guest if necessary. */
 	if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT))
 		eventfd_write((int)vq->callfd, 1);
-- 
1.8.1.4

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: flush used->idx update before reading avail->flags
  2015-04-22 16:33 [dpdk-dev] [PATCH] vhost: flush used->idx update before reading avail->flags Huawei Xie
@ 2015-04-23 13:57 ` Luke Gorrie
  2015-04-24  1:01 ` Linhaifeng
  2015-04-29 11:11 ` [dpdk-dev] [PATCH v2] " Huawei Xie
  2 siblings, 0 replies; 17+ messages in thread
From: Luke Gorrie @ 2015-04-23 13:57 UTC (permalink / raw)
  To: Huawei Xie; +Cc: dev, Michael S. Tsirkin

On 22 April 2015 at 18:33, Huawei Xie <huawei.xie@intel.com> wrote:

> update of used->idx and read of avail->flags could be reordered.
> memory fence should be used to ensure the order, otherwise guest could see
> a stale used->idx value after it toggles the interrupt suppression flag.
>

This patch looks right to me.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: flush used->idx update before reading avail->flags
  2015-04-22 16:33 [dpdk-dev] [PATCH] vhost: flush used->idx update before reading avail->flags Huawei Xie
  2015-04-23 13:57 ` Luke Gorrie
@ 2015-04-24  1:01 ` Linhaifeng
  2015-04-24  2:40   ` Xie, Huawei
  2015-04-24  7:27   ` Luke Gorrie
  2015-04-29 11:11 ` [dpdk-dev] [PATCH v2] " Huawei Xie
  2 siblings, 2 replies; 17+ messages in thread
From: Linhaifeng @ 2015-04-24  1:01 UTC (permalink / raw)
  To: Huawei Xie, dev; +Cc: mst



On 2015/4/23 0:33, Huawei Xie wrote:
> update of used->idx and read of avail->flags could be reordered.
> memory fence should be used to ensure the order, otherwise guest could see a stale used->idx value after it toggles the interrupt suppression flag.
> 
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> ---
>  lib/librte_vhost/vhost_rxtx.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
> index 510ffe8..6afba35 100644
> --- a/lib/librte_vhost/vhost_rxtx.c
> +++ b/lib/librte_vhost/vhost_rxtx.c
> @@ -178,6 +178,9 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
>  	*(volatile uint16_t *)&vq->used->idx += count;
>  	vq->last_used_idx = res_end_idx;
>  
> +	/* flush used->idx update before we read avail->flags. */
> +	rte_mb();
> +
>  	/* Kick the guest if necessary. */
>  	if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT))
>  		eventfd_write((int)vq->callfd, 1);
> 

If not add memory fence what would happen? Packets loss or interrupt loss?How to test it ?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: flush used->idx update before reading avail->flags
  2015-04-24  1:01 ` Linhaifeng
@ 2015-04-24  2:40   ` Xie, Huawei
  2015-04-24  7:27   ` Luke Gorrie
  1 sibling, 0 replies; 17+ messages in thread
From: Xie, Huawei @ 2015-04-24  2:40 UTC (permalink / raw)
  To: Linhaifeng, dev; +Cc: mst



> -----Original Message-----
> From: Linhaifeng [mailto:haifeng.lin@huawei.com]
> Sent: Friday, April 24, 2015 9:01 AM
> To: Xie, Huawei; dev@dpdk.org
> Cc: luke@snabb.co; mst@redhat.com
> Subject: Re: [PATCH] vhost: flush used->idx update before reading avail-
> >flags
> 
> 
> 
> On 2015/4/23 0:33, Huawei Xie wrote:
> > update of used->idx and read of avail->flags could be reordered.
> > memory fence should be used to ensure the order, otherwise guest could
> see a stale used->idx value after it toggles the interrupt suppression flag.
> >
> > Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> > ---
> >  lib/librte_vhost/vhost_rxtx.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
> > index 510ffe8..6afba35 100644
> > --- a/lib/librte_vhost/vhost_rxtx.c
> > +++ b/lib/librte_vhost/vhost_rxtx.c
> > @@ -178,6 +178,9 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t
> queue_id,
> >  	*(volatile uint16_t *)&vq->used->idx += count;
> >  	vq->last_used_idx = res_end_idx;
> >
> > +	/* flush used->idx update before we read avail->flags. */
> > +	rte_mb();
> > +
> >  	/* Kick the guest if necessary. */
> >  	if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT))
> >  		eventfd_write((int)vq->callfd, 1);
> >
> 
> If not add memory fence what would happen? Packets loss or interrupt
> loss?How to test it ?

If those two store and load are reordered,  guest toggles the interrupt suppression flag, then it checks that there is no more work to do.
Actually there is.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: flush used->idx update before reading avail->flags
  2015-04-24  1:01 ` Linhaifeng
  2015-04-24  2:40   ` Xie, Huawei
@ 2015-04-24  7:27   ` Luke Gorrie
  2015-06-09  7:04     ` Linhaifeng
  1 sibling, 1 reply; 17+ messages in thread
From: Luke Gorrie @ 2015-04-24  7:27 UTC (permalink / raw)
  To: Linhaifeng; +Cc: dev, Michael S. Tsirkin

On 24 April 2015 at 03:01, Linhaifeng <haifeng.lin@huawei.com> wrote:

> If not add memory fence what would happen? Packets loss or interrupt
> loss?How to test it ?
>

You should be able to test it like this:

1. Boot two Linux kernel (e.g. 3.13) guests.
2. Connect them via vhost switch.
3. Run continuous traffic between them (e.g. iperf).

I would expect that within a reasonable timeframe (< 1 hour) one of the
guests' network interfaces will hang indefinitely due to a missed interrupt.

You won't be able to reproduce this using DPDK guests because they are not
using the same interrupt suppression method.

This is a serious real-world problem. I wouldn't deploy the vhost
implementation without this fix.

Cheers,
-Luke

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [dpdk-dev] [PATCH v2] vhost: flush used->idx update before reading avail->flags
  2015-04-22 16:33 [dpdk-dev] [PATCH] vhost: flush used->idx update before reading avail->flags Huawei Xie
  2015-04-23 13:57 ` Luke Gorrie
  2015-04-24  1:01 ` Linhaifeng
@ 2015-04-29 11:11 ` Huawei Xie
  2015-05-13 10:46   ` Thomas Monjalon
  2 siblings, 1 reply; 17+ messages in thread
From: Huawei Xie @ 2015-04-29 11:11 UTC (permalink / raw)
  To: dev; +Cc: mst

update of used->idx and read of avail->flags could be reordered.
memory fence should be used to ensure the order, otherwise guest could see a stale used->idx value after it toggles the interrupt suppression flag.
After guest sets the interrupt suppression flag, it will check if there is more buffer to process through used->idx. If it sees a stale value, it will exit the processing while host willn't send interrupt to guest.

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
 lib/librte_vhost/vhost_rxtx.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
index 510ffe8..4809d32 100644
--- a/lib/librte_vhost/vhost_rxtx.c
+++ b/lib/librte_vhost/vhost_rxtx.c
@@ -178,6 +178,9 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
 	*(volatile uint16_t *)&vq->used->idx += count;
 	vq->last_used_idx = res_end_idx;
 
+	/* flush used->idx update before we read avail->flags. */
+	rte_mb();
+
 	/* Kick the guest if necessary. */
 	if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT))
 		eventfd_write((int)vq->callfd, 1);
@@ -505,6 +508,9 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t queue_id,
 		*(volatile uint16_t *)&vq->used->idx += entry_success;
 		vq->last_used_idx = res_end_idx;
 
+		/* flush used->idx update before we read avail->flags. */
+		rte_mb();
+
 		/* Kick the guest if necessary. */
 		if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT))
 			eventfd_write((int)vq->callfd, 1);
-- 
1.8.1.4

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH v2] vhost: flush used->idx update before reading avail->flags
  2015-04-29 11:11 ` [dpdk-dev] [PATCH v2] " Huawei Xie
@ 2015-05-13 10:46   ` Thomas Monjalon
  2015-05-15 13:43     ` Nikita Kalyazin
  0 siblings, 1 reply; 17+ messages in thread
From: Thomas Monjalon @ 2015-05-13 10:46 UTC (permalink / raw)
  To: Huawei Xie; +Cc: dev, mst

2015-04-29 19:11, Huawei Xie:
> update of used->idx and read of avail->flags could be reordered.
> memory fence should be used to ensure the order, otherwise guest could see a stale used->idx value after it toggles the interrupt suppression flag.
> After guest sets the interrupt suppression flag, it will check if there is more buffer to process through used->idx. If it sees a stale value, it will exit the processing while host willn't send interrupt to guest.
> 
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>

Applied with following title, thanks
	vhost: fix virtio freeze due to missed interrupt

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH v2] vhost: flush used->idx update before reading avail->flags
  2015-05-13 10:46   ` Thomas Monjalon
@ 2015-05-15 13:43     ` Nikita Kalyazin
  2015-05-15 15:23       ` Michael S. Tsirkin
  0 siblings, 1 reply; 17+ messages in thread
From: Nikita Kalyazin @ 2015-05-15 13:43 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev, mst

Hi,


Maybe I missed a part of the discussion, but is there any special purpose for using rte_mb (both read and write fence) here rather than rte_wmb (write fence only)?

-- 

Best regards,

Nikita Kalyazin,
n.kalyazin@samsung.com

Software Engineer
CE OS Group
Samsung R&D Institute Russia
Tel: +7 (495) 797-25-00 #3816
Tel: +7 (495) 797-25-03
Office #1501, 12-1, Dvintsev str.,
Moscow, 127018, Russia

On Wed, May 13, 2015 at 12:46:30PM +0200, Thomas Monjalon wrote:
> 2015-04-29 19:11, Huawei Xie:
> > update of used->idx and read of avail->flags could be reordered.
> > memory fence should be used to ensure the order, otherwise guest could see a stale used->idx value after it toggles the interrupt suppression flag.
> > After guest sets the interrupt suppression flag, it will check if there is more buffer to process through used->idx. If it sees a stale value, it will exit the processing while host willn't send interrupt to guest.
> > 
> > Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> 
> Applied with following title, thanks
> 	vhost: fix virtio freeze due to missed interrupt
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH v2] vhost: flush used->idx update before reading avail->flags
  2015-05-15 13:43     ` Nikita Kalyazin
@ 2015-05-15 15:23       ` Michael S. Tsirkin
  2015-05-18  6:18         ` Nikita Kalyazin
  0 siblings, 1 reply; 17+ messages in thread
From: Michael S. Tsirkin @ 2015-05-15 15:23 UTC (permalink / raw)
  To: Nikita Kalyazin; +Cc: dev

On Fri, May 15, 2015 at 04:43:33PM +0300, Nikita Kalyazin wrote:
> Hi,
> 
> 
> Maybe I missed a part of the discussion, but is there any special purpose for using rte_mb (both read and write fence) here rather than rte_wmb (write fence only)?

The fence is between write of used->idx and read of avail->flags, so
rte_wmb won't do anything useful.

> -- 
> 
> Best regards,
> 
> Nikita Kalyazin,
> n.kalyazin@samsung.com
> 
> Software Engineer
> CE OS Group
> Samsung R&D Institute Russia
> Tel: +7 (495) 797-25-00 #3816
> Tel: +7 (495) 797-25-03
> Office #1501, 12-1, Dvintsev str.,
> Moscow, 127018, Russia
> 
> On Wed, May 13, 2015 at 12:46:30PM +0200, Thomas Monjalon wrote:
> > 2015-04-29 19:11, Huawei Xie:
> > > update of used->idx and read of avail->flags could be reordered.
> > > memory fence should be used to ensure the order, otherwise guest could see a stale used->idx value after it toggles the interrupt suppression flag.
> > > After guest sets the interrupt suppression flag, it will check if there is more buffer to process through used->idx. If it sees a stale value, it will exit the processing while host willn't send interrupt to guest.
> > > 
> > > Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> > 
> > Applied with following title, thanks
> > 	vhost: fix virtio freeze due to missed interrupt
> > 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH v2] vhost: flush used->idx update before reading avail->flags
  2015-05-15 15:23       ` Michael S. Tsirkin
@ 2015-05-18  6:18         ` Nikita Kalyazin
  0 siblings, 0 replies; 17+ messages in thread
From: Nikita Kalyazin @ 2015-05-18  6:18 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: dev

Ah, sorry. I looked at it without the context. Thanks.

-- 

Best regards,

Nikita Kalyazin,
n.kalyazin@samsung.com

Software Engineer
CE OS Group
Samsung R&D Institute Russia
Tel: +7 (495) 797-25-00 #3816
Tel: +7 (495) 797-25-03
Office #1501, 12-1, Dvintsev str.,
Moscow, 127018, Russia

On Fri, May 15, 2015 at 05:23:35PM +0200, Michael S. Tsirkin wrote:
> On Fri, May 15, 2015 at 04:43:33PM +0300, Nikita Kalyazin wrote:
> > Hi,
> > 
> > 
> > Maybe I missed a part of the discussion, but is there any special purpose for using rte_mb (both read and write fence) here rather than rte_wmb (write fence only)?
> 
> The fence is between write of used->idx and read of avail->flags, so
> rte_wmb won't do anything useful.
> 
> > -- 
> > 
> > Best regards,
> > 
> > Nikita Kalyazin,
> > n.kalyazin@samsung.com
> > 
> > Software Engineer
> > CE OS Group
> > Samsung R&D Institute Russia
> > Tel: +7 (495) 797-25-00 #3816
> > Tel: +7 (495) 797-25-03
> > Office #1501, 12-1, Dvintsev str.,
> > Moscow, 127018, Russia
> > 
> > On Wed, May 13, 2015 at 12:46:30PM +0200, Thomas Monjalon wrote:
> > > 2015-04-29 19:11, Huawei Xie:
> > > > update of used->idx and read of avail->flags could be reordered.
> > > > memory fence should be used to ensure the order, otherwise guest could see a stale used->idx value after it toggles the interrupt suppression flag.
> > > > After guest sets the interrupt suppression flag, it will check if there is more buffer to process through used->idx. If it sees a stale value, it will exit the processing while host willn't send interrupt to guest.
> > > > 
> > > > Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> > > 
> > > Applied with following title, thanks
> > > 	vhost: fix virtio freeze due to missed interrupt
> > > 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: flush used->idx update before reading avail->flags
  2015-04-24  7:27   ` Luke Gorrie
@ 2015-06-09  7:04     ` Linhaifeng
  2015-06-09  7:45       ` Luke Gorrie
  2015-06-09  8:46       ` Michael S. Tsirkin
  0 siblings, 2 replies; 17+ messages in thread
From: Linhaifeng @ 2015-06-09  7:04 UTC (permalink / raw)
  To: Luke Gorrie; +Cc: dev, Michael S. Tsirkin



On 2015/4/24 15:27, Luke Gorrie wrote:
> On 24 April 2015 at 03:01, Linhaifeng <haifeng.lin@huawei.com> wrote:
> 
>> If not add memory fence what would happen? Packets loss or interrupt
>> loss?How to test it ?
>>
> 
> You should be able to test it like this:
> 
> 1. Boot two Linux kernel (e.g. 3.13) guests.
> 2. Connect them via vhost switch.
> 3. Run continuous traffic between them (e.g. iperf).
> 
> I would expect that within a reasonable timeframe (< 1 hour) one of the
> guests' network interfaces will hang indefinitely due to a missed interrupt.
> 
> You won't be able to reproduce this using DPDK guests because they are not
> using the same interrupt suppression method.
> 
> This is a serious real-world problem. I wouldn't deploy the vhost
> implementation without this fix.
> 
> Cheers,
> -Luke
> 

I think this patch can't resole this problem. On the other hand we still would miss interrupt.

After add rte_mb() function the we want the case is :
1.write used->idx. ring is full or empty.
2.virtio_net open interrupt.
3.read avail->flags.

but this case(miss interrupt) would happen too:
1.write used->idx. ring is full or empty.
2.read avail->flags.
3.virtio_net open interrupt.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: flush used->idx update before reading avail->flags
  2015-06-09  7:04     ` Linhaifeng
@ 2015-06-09  7:45       ` Luke Gorrie
  2015-06-09  8:46       ` Michael S. Tsirkin
  1 sibling, 0 replies; 17+ messages in thread
From: Luke Gorrie @ 2015-06-09  7:45 UTC (permalink / raw)
  To: Linhaifeng; +Cc: dev, Michael S. Tsirkin

On 9 June 2015 at 09:04, Linhaifeng <haifeng.lin@huawei.com> wrote:

> On 2015/4/24 15:27, Luke Gorrie wrote:
> > You should be able to test it like this:
> >
> > 1. Boot two Linux kernel (e.g. 3.13) guests.
> > 2. Connect them via vhost switch.
> > 3. Run continuous traffic between them (e.g. iperf).
> >
> > I would expect that within a reasonable timeframe (< 1 hour) one of the
> > guests' network interfaces will hang indefinitely due to a missed
> interrupt.
> >
> > You won't be able to reproduce this using DPDK guests because they are
> not
> > using the same interrupt suppression method.
>
> I think this patch can't resole this problem. On the other hand we still
> would miss interrupt.
>

For what it is worth, we were able to reproduce the problem as described
above with older Snabb Switch releases and we were also able to verify that
inserting a memory barrier fixes this problem.

This is the relevant commit in the snabbswitch repo for reference:
https://github.com/SnabbCo/snabbswitch/commit/c33cdd8704246887e11d7c353f773f7b488a47f2

In a nutshell, we added an MFENCE instruction after writing used->idx and
before checking VRING_F_NO_INTERRUPT.

I have not tested this case under DPDK myself and so I am not really
certain which memory barrier operations are sufficient/insufficient in that
context. I hope that our experience is relevant/helpful though and I am
happy to explain more about that if I have missed any important details.

Cheers,
-Luke

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: flush used->idx update before reading avail->flags
  2015-06-09  7:04     ` Linhaifeng
  2015-06-09  7:45       ` Luke Gorrie
@ 2015-06-09  8:46       ` Michael S. Tsirkin
  2015-06-09 13:34         ` Xie, Huawei
  2015-06-10  8:30         ` Luke Gorrie
  1 sibling, 2 replies; 17+ messages in thread
From: Michael S. Tsirkin @ 2015-06-09  8:46 UTC (permalink / raw)
  To: Linhaifeng; +Cc: dev

On Tue, Jun 09, 2015 at 03:04:02PM +0800, Linhaifeng wrote:
> 
> 
> On 2015/4/24 15:27, Luke Gorrie wrote:
> > On 24 April 2015 at 03:01, Linhaifeng <haifeng.lin@huawei.com> wrote:
> > 
> >> If not add memory fence what would happen? Packets loss or interrupt
> >> loss?How to test it ?
> >>
> > 
> > You should be able to test it like this:
> > 
> > 1. Boot two Linux kernel (e.g. 3.13) guests.
> > 2. Connect them via vhost switch.
> > 3. Run continuous traffic between them (e.g. iperf).
> > 
> > I would expect that within a reasonable timeframe (< 1 hour) one of the
> > guests' network interfaces will hang indefinitely due to a missed interrupt.
> > 
> > You won't be able to reproduce this using DPDK guests because they are not
> > using the same interrupt suppression method.
> > 
> > This is a serious real-world problem. I wouldn't deploy the vhost
> > implementation without this fix.
> > 
> > Cheers,
> > -Luke
> > 
> 
> I think this patch can't resole this problem. On the other hand we still would miss interrupt.
> 
> After add rte_mb() function the we want the case is :
> 1.write used->idx. ring is full or empty.
> 2.virtio_net open interrupt.
> 3.read avail->flags.
> 
> but this case(miss interrupt) would happen too:
> 1.write used->idx. ring is full or empty.
> 2.read avail->flags.
> 3.virtio_net open interrupt.
> 

That's why a correct guest, after detecting an empty used ring, must always
re-check used idx at least once after writing avail->flags.

By the way, similarly, host side must re-check avail idx after writing
used flags. I don't see where snabbswitch does it - is that a bug
in snabbswitch?

-- 
MST

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: flush used->idx update before reading avail->flags
  2015-06-09  8:46       ` Michael S. Tsirkin
@ 2015-06-09 13:34         ` Xie, Huawei
  2015-06-10  2:36           ` Linhaifeng
  2015-06-10  8:30         ` Luke Gorrie
  1 sibling, 1 reply; 17+ messages in thread
From: Xie, Huawei @ 2015-06-09 13:34 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: dev

On 6/9/2015 4:47 PM, Michael S. Tsirkin wrote:
> On Tue, Jun 09, 2015 at 03:04:02PM +0800, Linhaifeng wrote:
>>
>> On 2015/4/24 15:27, Luke Gorrie wrote:
>>> On 24 April 2015 at 03:01, Linhaifeng <haifeng.lin@huawei.com> wrote:
>>>
>>>> If not add memory fence what would happen? Packets loss or interrupt
>>>> loss?How to test it ?
>>>>
>>> You should be able to test it like this:
>>>
>>> 1. Boot two Linux kernel (e.g. 3.13) guests.
>>> 2. Connect them via vhost switch.
>>> 3. Run continuous traffic between them (e.g. iperf).
>>>
>>> I would expect that within a reasonable timeframe (< 1 hour) one of the
>>> guests' network interfaces will hang indefinitely due to a missed interrupt.
>>>
>>> You won't be able to reproduce this using DPDK guests because they are not
>>> using the same interrupt suppression method.
>>>
>>> This is a serious real-world problem. I wouldn't deploy the vhost
>>> implementation without this fix.
>>>
>>> Cheers,
>>> -Luke
>>>
>> I think this patch can't resole this problem. On the other hand we still would miss interrupt.
>>
>> After add rte_mb() function the we want the case is :
>> 1.write used->idx. ring is full or empty.
>> 2.virtio_net open interrupt.
>> 3.read avail->flags.
>>
>> but this case(miss interrupt) would happen too:
>> 1.write used->idx. ring is full or empty.
>> 2.read avail->flags.
>> 3.virtio_net open interrupt.
>>
> That's why a correct guest, after detecting an empty used ring, must always
> re-check used idx at least once after writing avail->flags.
>
> By the way, similarly, host side must re-check avail idx after writing
> used flags. I don't see where snabbswitch does it - is that a bug
> in snabbswitch?
>
yes, both host and guest should recheck if there is more work added
after they toggle the flag.
For DPDK vHost, as it runs in polling mode, we will recheck avail idx
soon, so we don't need recheck.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: flush used->idx update before reading avail->flags
  2015-06-09 13:34         ` Xie, Huawei
@ 2015-06-10  2:36           ` Linhaifeng
  0 siblings, 0 replies; 17+ messages in thread
From: Linhaifeng @ 2015-06-10  2:36 UTC (permalink / raw)
  To: Xie, Huawei, Michael S. Tsirkin; +Cc: dev



On 2015/6/9 21:34, Xie, Huawei wrote:
> On 6/9/2015 4:47 PM, Michael S. Tsirkin wrote:
>> On Tue, Jun 09, 2015 at 03:04:02PM +0800, Linhaifeng wrote:
>>>
>>> On 2015/4/24 15:27, Luke Gorrie wrote:
>>>> On 24 April 2015 at 03:01, Linhaifeng <haifeng.lin@huawei.com> wrote:
>>>>
>>>>> If not add memory fence what would happen? Packets loss or interrupt
>>>>> loss?How to test it ?
>>>>>
>>>> You should be able to test it like this:
>>>>
>>>> 1. Boot two Linux kernel (e.g. 3.13) guests.
>>>> 2. Connect them via vhost switch.
>>>> 3. Run continuous traffic between them (e.g. iperf).
>>>>
>>>> I would expect that within a reasonable timeframe (< 1 hour) one of the
>>>> guests' network interfaces will hang indefinitely due to a missed interrupt.
>>>>
>>>> You won't be able to reproduce this using DPDK guests because they are not
>>>> using the same interrupt suppression method.
>>>>
>>>> This is a serious real-world problem. I wouldn't deploy the vhost
>>>> implementation without this fix.
>>>>
>>>> Cheers,
>>>> -Luke
>>>>
>>> I think this patch can't resole this problem. On the other hand we still would miss interrupt.
>>>
>>> After add rte_mb() function the we want the case is :
>>> 1.write used->idx. ring is full or empty.
>>> 2.virtio_net open interrupt.
>>> 3.read avail->flags.
>>>
>>> but this case(miss interrupt) would happen too:
>>> 1.write used->idx. ring is full or empty.
>>> 2.read avail->flags.
>>> 3.virtio_net open interrupt.
>>>
>> That's why a correct guest, after detecting an empty used ring, must always
>> re-check used idx at least once after writing avail->flags.
>>
>> By the way, similarly, host side must re-check avail idx after writing
>> used flags. I don't see where snabbswitch does it - is that a bug
>> in snabbswitch?
>>
> yes, both host and guest should recheck if there is more work added
> after they toggle the flag.
> For DPDK vHost, as it runs in polling mode, we will recheck avail idx
> soon, so we don't need recheck.
> 
> 

DPDK does check the avail idx but does nothing like this:
if (vq->last_used_idx == avail_idx) {
	return;
}

If we miss an interrupt after calling rte_mb(),
(!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT)) is False;
while (vq->last_used_idx == avail_idx) is True,

then the guest will miss the interrupt forever and virtio-net would stop working.

Would this case happen?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: flush used->idx update before reading avail->flags
  2015-06-09  8:46       ` Michael S. Tsirkin
  2015-06-09 13:34         ` Xie, Huawei
@ 2015-06-10  8:30         ` Luke Gorrie
  2015-06-11  0:50           ` Linhaifeng
  1 sibling, 1 reply; 17+ messages in thread
From: Luke Gorrie @ 2015-06-10  8:30 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: dev

On 9 June 2015 at 10:46, Michael S. Tsirkin <mst@redhat.com> wrote:

> By the way, similarly, host side must re-check avail idx after writing
> used flags. I don't see where snabbswitch does it - is that a bug
> in snabbswitch?


Good question.

Snabb Switch does not use interrupts from the guest. We always set
VRING_F_NO_NOTIFY to tell the guest that it need not interrupt us. Then we
run in poll mode and in practice check the avail ring for new descriptors
every 20us or so.

So the argument for not needing this check in both Snabb Switch and DPDK is
that we are running poll mode and don't notice whether interrupts are being
sent or not.

Is that a solid argument or do I misunderstand what the race condition is?

Cheers,
-Luke

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: flush used->idx update before reading avail->flags
  2015-06-10  8:30         ` Luke Gorrie
@ 2015-06-11  0:50           ` Linhaifeng
  0 siblings, 0 replies; 17+ messages in thread
From: Linhaifeng @ 2015-06-11  0:50 UTC (permalink / raw)
  To: Luke Gorrie, Michael S. Tsirkin; +Cc: dev



On 2015/6/10 16:30, Luke Gorrie wrote:
> On 9 June 2015 at 10:46, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
>> By the way, similarly, host side must re-check avail idx after writing
>> used flags. I don't see where snabbswitch does it - is that a bug
>> in snabbswitch?
> 
> 
> Good question.
> 
> Snabb Switch does not use interrupts from the guest. We always set
> VRING_F_NO_NOTIFY to tell the guest that it need not interrupt us. Then we
> run in poll mode and in practice check the avail ring for new descriptors
> every 20us or so.

Yes, host not need guest to notify when in poll mode but host also need to
notify guest who use virtio_net when vring is full or emtpy.If host loss this
notification guest would stop working.

> 
> So the argument for not needing this check in both Snabb Switch and DPDK is
> that we are running poll mode and don't notice whether interrupts are being
> sent or not.
> 
> Is that a solid argument or do I misunderstand what the race condition is?
> 
> Cheers,
> -Luke
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2015-06-11  0:51 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-22 16:33 [dpdk-dev] [PATCH] vhost: flush used->idx update before reading avail->flags Huawei Xie
2015-04-23 13:57 ` Luke Gorrie
2015-04-24  1:01 ` Linhaifeng
2015-04-24  2:40   ` Xie, Huawei
2015-04-24  7:27   ` Luke Gorrie
2015-06-09  7:04     ` Linhaifeng
2015-06-09  7:45       ` Luke Gorrie
2015-06-09  8:46       ` Michael S. Tsirkin
2015-06-09 13:34         ` Xie, Huawei
2015-06-10  2:36           ` Linhaifeng
2015-06-10  8:30         ` Luke Gorrie
2015-06-11  0:50           ` Linhaifeng
2015-04-29 11:11 ` [dpdk-dev] [PATCH v2] " Huawei Xie
2015-05-13 10:46   ` Thomas Monjalon
2015-05-15 13:43     ` Nikita Kalyazin
2015-05-15 15:23       ` Michael S. Tsirkin
2015-05-18  6:18         ` Nikita Kalyazin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).