[dpdk-stable] [PATCH 1/8] net/virtio: revert "virtio: fix restart"

[dpdk-stable] [PATCH 1/8] net/virtio: revert "virtio: fix restart"

[Yuanhan Liu]

This reverts commit 9a0615af7746 ("virtio: fix restart"); conflict is
manually addressed.

Kyle reported an issue with above commit

    qemu-kvm: Guest moved used index from 5 to 1

with following steps,

    1) Start my virtio interfaces
    2) Send some traffic into/out of the interfaces
    3) Stop the interfaces
    4) Start the interfaces
    5) Send some more traffic

And here are some quotes from Kyle's analysis,

    Prior to the patch, if an interface were stopped then started, without
    restarting the application, the queues would be left as-is, because
    hw->started would be set to 1. Now, calling stop sets hw->started to 0,
    which means the next call to start will "touch the queues". This is the
    unintended side-effect that causes the problem.

Fixes: 9a0615af7746 ("virtio: fix restart")

Cc: Jianfeng Tan <jianfeng.tan@intel.com>
Cc: <stable@dpdk.org>
Reported-by: Kyle Larose <klarose@sandvine.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 drivers/net/virtio/virtio_ethdev.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index b388134..5815875 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -550,9 +550,6 @@ virtio_dev_close(struct rte_eth_dev *dev)
 
 	PMD_INIT_LOG(DEBUG, "virtio_dev_close");
 
-	if (hw->started == 1)
-		virtio_dev_stop(dev);
-
 	if (hw->cvq)
 		virtio_dev_queue_release(hw->cvq->vq);
 
@@ -560,6 +557,7 @@ virtio_dev_close(struct rte_eth_dev *dev)
 	if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
 		vtpci_irq_config(hw, VIRTIO_MSI_NO_VECTOR);
 	vtpci_reset(hw);
+	hw->started = 0;
 	virtio_dev_free_mbufs(dev);
 	virtio_free_queues(dev);
 }
@@ -1296,15 +1294,17 @@ static int
 eth_virtio_dev_uninit(struct rte_eth_dev *eth_dev)
 {
 	struct rte_pci_device *pci_dev;
+	struct virtio_hw *hw = eth_dev->data->dev_private;
 
 	PMD_INIT_FUNC_TRACE();
 
 	if (rte_eal_process_type() == RTE_PROC_SECONDARY)
 		return -EPERM;
 
-	/* Close it anyway since there's no way to know if closed */
-	virtio_dev_close(eth_dev);
-
+	if (hw->started == 1) {
+		virtio_dev_stop(eth_dev);
+		virtio_dev_close(eth_dev);
+	}
 	pci_dev = eth_dev->pci_dev;
 
 	eth_dev->dev_ops = NULL;
@@ -1543,12 +1543,9 @@ static void
 virtio_dev_stop(struct rte_eth_dev *dev)
 {
 	struct rte_eth_link link;
-	struct virtio_hw *hw = dev->data->dev_private;
 
 	PMD_INIT_LOG(DEBUG, "stop");
 
-	hw->started = 0;
-
 	if (dev->data->dev_conf.intr_conf.lsc)
 		rte_intr_disable(&dev->pci_dev->intr_handle);
 
-- 
1.9.0

Re: [dpdk-stable] [dpdk-dev] [PATCH 1/8] net/virtio: revert "virtio: fix restart"

[Maxime Coquelin]

Hi Yuanhan,

On 11/03/2016 05:09 PM, Yuanhan Liu wrote:
> This reverts commit 9a0615af7746 ("virtio: fix restart"); conflict is
> manually addressed.
>
> Kyle reported an issue with above commit
>
>     qemu-kvm: Guest moved used index from 5 to 1
>
> with following steps,
>
>     1) Start my virtio interfaces
>     2) Send some traffic into/out of the interfaces
>     3) Stop the interfaces
>     4) Start the interfaces
>     5) Send some more traffic
>
> And here are some quotes from Kyle's analysis,
>
>     Prior to the patch, if an interface were stopped then started, without
>     restarting the application, the queues would be left as-is, because
>     hw->started would be set to 1. Now, calling stop sets hw->started to 0,
>     which means the next call to start will "touch the queues". This is the
>     unintended side-effect that causes the problem.

Maybe a good idea to explain what is the problem the revert aims to fix.
It does not seem to be clearly stated in the commit message.

>
> Fixes: 9a0615af7746 ("virtio: fix restart")
>
> Cc: Jianfeng Tan <jianfeng.tan@intel.com>
> Cc: <stable@dpdk.org>
> Reported-by: Kyle Larose <klarose@sandvine.com>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

Thanks,
Maxime

Re: [dpdk-stable] [dpdk-dev] [PATCH 1/8] net/virtio: revert "virtio: fix restart"

[Yuanhan Liu]

On Thu, Nov 03, 2016 at 09:36:52PM +0100, Maxime Coquelin wrote:
> Hi Yuanhan,
> 
> On 11/03/2016 05:09 PM, Yuanhan Liu wrote:
> >This reverts commit 9a0615af7746 ("virtio: fix restart"); conflict is
> >manually addressed.
> >
> >Kyle reported an issue with above commit
> >
> >    qemu-kvm: Guest moved used index from 5 to 1
> >
> >with following steps,
> >
> >    1) Start my virtio interfaces
> >    2) Send some traffic into/out of the interfaces
> >    3) Stop the interfaces
> >    4) Start the interfaces
> >    5) Send some more traffic
> >
> >And here are some quotes from Kyle's analysis,
> >
> >    Prior to the patch, if an interface were stopped then started, without
> >    restarting the application, the queues would be left as-is, because
> >    hw->started would be set to 1. Now, calling stop sets hw->started to 0,
> >    which means the next call to start will "touch the queues". This is the
> >    unintended side-effect that causes the problem.
> 
> Maybe a good idea to explain what is the problem the revert aims to fix.

It aims to fix the issue, by "not touching the queues" on restart.

> It does not seem to be clearly stated in the commit message.

I was thinking the quote from Kyle is enough. How about following supplement:

    We should not touch the queues once the init is done, otherwise, the
    vring state of virtio PMD driver and vhost-user would be inconsistent,
    leading some issue like above.

    Thus this patch is reverted.

Better now?

	--yliu

Re: [dpdk-stable] [dpdk-dev] [PATCH 1/8] net/virtio: revert "virtio: fix restart"

[Maxime Coquelin]

On 11/04/2016 03:00 AM, Yuanhan Liu wrote:
> On Thu, Nov 03, 2016 at 09:36:52PM +0100, Maxime Coquelin wrote:
>> Hi Yuanhan,
>>
>> On 11/03/2016 05:09 PM, Yuanhan Liu wrote:
>>> This reverts commit 9a0615af7746 ("virtio: fix restart"); conflict is
>>> manually addressed.
>>>
>>> Kyle reported an issue with above commit
>>>
>>>    qemu-kvm: Guest moved used index from 5 to 1
>>>
>>> with following steps,
>>>
>>>    1) Start my virtio interfaces
>>>    2) Send some traffic into/out of the interfaces
>>>    3) Stop the interfaces
>>>    4) Start the interfaces
>>>    5) Send some more traffic
>>>
>>> And here are some quotes from Kyle's analysis,
>>>
>>>    Prior to the patch, if an interface were stopped then started, without
>>>    restarting the application, the queues would be left as-is, because
>>>    hw->started would be set to 1. Now, calling stop sets hw->started to 0,
>>>    which means the next call to start will "touch the queues". This is the
>>>    unintended side-effect that causes the problem.
>>
>> Maybe a good idea to explain what is the problem the revert aims to fix.
>
> It aims to fix the issue, by "not touching the queues" on restart.
>
>> It does not seem to be clearly stated in the commit message.
>
> I was thinking the quote from Kyle is enough. How about following supplement:
>
>     We should not touch the queues once the init is done, otherwise, the
>     vring state of virtio PMD driver and vhost-user would be inconsistent,
>     leading some issue like above.
>
>     Thus this patch is reverted.
>
> Better now?
Yes, this is much clearer from my PoV.

Thanks!
Maxime

>
> 	--yliu
>

Re: [dpdk-stable] [dpdk-dev] [PATCH 1/8] net/virtio: revert "virtio: fix restart"

[Maxime Coquelin]

On 11/03/2016 05:09 PM, Yuanhan Liu wrote:
> This reverts commit 9a0615af7746 ("virtio: fix restart"); conflict is
> manually addressed.
>
> Kyle reported an issue with above commit
>
>     qemu-kvm: Guest moved used index from 5 to 1
>
> with following steps,
>
>     1) Start my virtio interfaces
>     2) Send some traffic into/out of the interfaces
>     3) Stop the interfaces
>     4) Start the interfaces
>     5) Send some more traffic
>
> And here are some quotes from Kyle's analysis,
>
>     Prior to the patch, if an interface were stopped then started, without
>     restarting the application, the queues would be left as-is, because
>     hw->started would be set to 1. Now, calling stop sets hw->started to 0,
>     which means the next call to start will "touch the queues". This is the
>     unintended side-effect that causes the problem.
>
> Fixes: 9a0615af7746 ("virtio: fix restart")
>
> Cc: Jianfeng Tan <jianfeng.tan@intel.com>
> Cc: <stable@dpdk.org>
> Reported-by: Kyle Larose <klarose@sandvine.com>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

Re: [dpdk-stable] [dpdk-dev] [PATCH 1/8] net/virtio: revert "virtio: fix restart"

[Yuanhan Liu]

On Fri, Nov 04, 2016 at 09:09:22AM +0100, Maxime Coquelin wrote:
> 
> 
> On 11/04/2016 03:00 AM, Yuanhan Liu wrote:
> >On Thu, Nov 03, 2016 at 09:36:52PM +0100, Maxime Coquelin wrote:
> >>Hi Yuanhan,
> >>
> >>On 11/03/2016 05:09 PM, Yuanhan Liu wrote:
> >>>This reverts commit 9a0615af7746 ("virtio: fix restart"); conflict is
> >>>manually addressed.
> >>>
> >>>Kyle reported an issue with above commit
> >>>
> >>>   qemu-kvm: Guest moved used index from 5 to 1
> >>>
> >>>with following steps,
> >>>
> >>>   1) Start my virtio interfaces
> >>>   2) Send some traffic into/out of the interfaces
> >>>   3) Stop the interfaces
> >>>   4) Start the interfaces
> >>>   5) Send some more traffic
> >>>
> >>>And here are some quotes from Kyle's analysis,
> >>>
> >>>   Prior to the patch, if an interface were stopped then started, without
> >>>   restarting the application, the queues would be left as-is, because
> >>>   hw->started would be set to 1. Now, calling stop sets hw->started to 0,
> >>>   which means the next call to start will "touch the queues". This is the
> >>>   unintended side-effect that causes the problem.
> >>
> >>Maybe a good idea to explain what is the problem the revert aims to fix.
> >
> >It aims to fix the issue, by "not touching the queues" on restart.
> >
> >>It does not seem to be clearly stated in the commit message.
> >
> >I was thinking the quote from Kyle is enough. How about following supplement:
> >
> >    We should not touch the queues once the init is done, otherwise, the
> >    vring state of virtio PMD driver and vhost-user would be inconsistent,
> >    leading some issue like above.
> >
> >    Thus this patch is reverted.
> >
> >Better now?
> Yes, this is much clearer from my PoV.

Fixed, and series applied to dpdk-next-virtio.

Thanks for the review!

	--yliu

[dpdk-stable] [PATCH v2 01/10] net/virtio: revert fix restart

[Yuanhan Liu]

This reverts commit 9a0615af7746 ("virtio: fix restart"); conflict is
manually addressed.

Kyle reported an issue with above commit

    qemu-kvm: Guest moved used index from 5 to 1

with following steps,

    1) Start my virtio interfaces
    2) Send some traffic into/out of the interfaces
    3) Stop the interfaces
    4) Start the interfaces
    5) Send some more traffic

And here are some quotes from Kyle's analysis,

    Prior to the patch, if an interface were stopped then started, without
    restarting the application, the queues would be left as-is, because
    hw->started would be set to 1. Now, calling stop sets hw->started to 0,
    which means the next call to start will "touch the queues". This is the
    unintended side-effect that causes the problem.

We should not touch the queues once the init is done, otherwise, the vring
state of virtio PMD driver and vhost-user would be inconsistent, leading
some issue like above.

Thus this patch is reverted.

Fixes: 9a0615af7746 ("virtio: fix restart")

Cc: Jianfeng Tan <jianfeng.tan@intel.com>
Cc: <stable@dpdk.org>
Reported-by: Kyle Larose <klarose@sandvine.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_ethdev.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index b388134..5815875 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -550,9 +550,6 @@ virtio_dev_close(struct rte_eth_dev *dev)
 
 	PMD_INIT_LOG(DEBUG, "virtio_dev_close");
 
-	if (hw->started == 1)
-		virtio_dev_stop(dev);
-
 	if (hw->cvq)
 		virtio_dev_queue_release(hw->cvq->vq);
 
@@ -560,6 +557,7 @@ virtio_dev_close(struct rte_eth_dev *dev)
 	if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
 		vtpci_irq_config(hw, VIRTIO_MSI_NO_VECTOR);
 	vtpci_reset(hw);
+	hw->started = 0;
 	virtio_dev_free_mbufs(dev);
 	virtio_free_queues(dev);
 }
@@ -1296,15 +1294,17 @@ static int
 eth_virtio_dev_uninit(struct rte_eth_dev *eth_dev)
 {
 	struct rte_pci_device *pci_dev;
+	struct virtio_hw *hw = eth_dev->data->dev_private;
 
 	PMD_INIT_FUNC_TRACE();
 
 	if (rte_eal_process_type() == RTE_PROC_SECONDARY)
 		return -EPERM;
 
-	/* Close it anyway since there's no way to know if closed */
-	virtio_dev_close(eth_dev);
-
+	if (hw->started == 1) {
+		virtio_dev_stop(eth_dev);
+		virtio_dev_close(eth_dev);
+	}
 	pci_dev = eth_dev->pci_dev;
 
 	eth_dev->dev_ops = NULL;
@@ -1543,12 +1543,9 @@ static void
 virtio_dev_stop(struct rte_eth_dev *dev)
 {
 	struct rte_eth_link link;
-	struct virtio_hw *hw = dev->data->dev_private;
 
 	PMD_INIT_LOG(DEBUG, "stop");
 
-	hw->started = 0;
-
 	if (dev->data->dev_conf.intr_conf.lsc)
 		rte_intr_disable(&dev->pci_dev->intr_handle);
 
-- 
1.9.0