DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 0/3] Fix hot plug/unplug of virtual devices
@ 2018-08-16 13:50 Luca Boccassi
  2018-08-16 13:50 ` [dpdk-dev] [PATCH 1/3] net/virtio: register/unregister intr handler on start/stop Luca Boccassi
                   ` (4 more replies)
  0 siblings, 5 replies; 30+ messages in thread
From: Luca Boccassi @ 2018-08-16 13:50 UTC (permalink / raw)
  To: dev
  Cc: maxime.coquelin, tiwei.bie, yongwang, 3chas3, bruce.richardson,
	jianfeng.tan, anatoly.burakov, Luca Boccassi

Although not very common and perhaps a bit strange, we do have users
that want to be able to hot plug/unplug virtio or vmxnet devices from
a running system, without shutting down the application nor the VM.

We have been enabling this in production for a year or so, so it's well
tested. The only issues found so far are fixed by this series.

Luca Boccassi (3):
  net/virtio: register/unregister intr handler on start/stop
  net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug
  eal/linux: handle uio read failure in interrupt handler

 drivers/net/virtio/virtio_ethdev.c           | 26 ++++++++------
 drivers/net/vmxnet3/vmxnet3_ethdev.c         | 36 ++++++++++++++------
 lib/librte_eal/linuxapp/eal/eal_interrupts.c | 19 ++++++++++-
 3 files changed, 59 insertions(+), 22 deletions(-)

-- 
2.18.0

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

* [dpdk-dev] [PATCH 1/3] net/virtio: register/unregister intr handler on start/stop
  2018-08-16 13:50 [dpdk-dev] [PATCH 0/3] Fix hot plug/unplug of virtual devices Luca Boccassi
@ 2018-08-16 13:50 ` Luca Boccassi
  2018-08-16 13:50 ` [dpdk-dev] [PATCH 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug Luca Boccassi
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 30+ messages in thread
From: Luca Boccassi @ 2018-08-16 13:50 UTC (permalink / raw)
  To: dev
  Cc: maxime.coquelin, tiwei.bie, yongwang, 3chas3, bruce.richardson,
	jianfeng.tan, anatoly.burakov, Luca Boccassi, stable,
	Brian Russell

Register and unregister the virtio interrupt handler when the device is
started and stopped. This allows a virtio device to be hotplugged or
unplugged.

Fixes: c1f86306a026 ("virtio: add new driver")
Cc: stable@dpdk.org

Signed-off-by: Brian Russell <brussell@brocade.com>
Signed-off-by: Luca Boccassi <bluca@debian.org>
---
 drivers/net/virtio/virtio_ethdev.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 614357da74..3195443a1b 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1679,11 +1679,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 	if (ret < 0)
 		goto out;
 
-	/* Setup interrupt callback  */
-	if (eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
-		rte_intr_callback_register(eth_dev->intr_handle,
-			virtio_interrupt_handler, eth_dev);
-
 	return 0;
 
 out:
@@ -1709,11 +1704,6 @@ eth_virtio_dev_uninit(struct rte_eth_dev *eth_dev)
 	rte_free(eth_dev->data->mac_addrs);
 	eth_dev->data->mac_addrs = NULL;
 
-	/* reset interrupt callback  */
-	if (eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
-		rte_intr_callback_unregister(eth_dev->intr_handle,
-						virtio_interrupt_handler,
-						eth_dev);
 	if (eth_dev->device)
 		rte_pci_unmap_device(RTE_ETH_DEV_TO_PCI(eth_dev));
 
@@ -1972,6 +1962,12 @@ virtio_dev_start(struct rte_eth_dev *dev)
 	    dev->data->dev_conf.intr_conf.rxq) {
 		virtio_intr_disable(dev);
 
+		/* Setup interrupt callback  */
+		if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
+			rte_intr_callback_register(dev->intr_handle,
+						   virtio_interrupt_handler,
+						   dev);
+
 		if (virtio_intr_enable(dev) < 0) {
 			PMD_DRV_LOG(ERR, "interrupt enable failed");
 			return -EIO;
@@ -2081,9 +2077,17 @@ virtio_dev_stop(struct rte_eth_dev *dev)
 	PMD_INIT_LOG(DEBUG, "stop");
 
 	rte_spinlock_lock(&hw->state_lock);
-	if (intr_conf->lsc || intr_conf->rxq)
+	if (intr_conf->lsc || intr_conf->rxq) {
 		virtio_intr_disable(dev);
 
+		/* Reset interrupt callback  */
+		if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC) {
+			rte_intr_callback_unregister(dev->intr_handle,
+						     virtio_interrupt_handler,
+						     dev);
+		}
+	}
+
 	hw->started = 0;
 	memset(&link, 0, sizeof(link));
 	rte_eth_linkstatus_set(dev, &link);
-- 
2.18.0

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

* [dpdk-dev] [PATCH 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug
  2018-08-16 13:50 [dpdk-dev] [PATCH 0/3] Fix hot plug/unplug of virtual devices Luca Boccassi
  2018-08-16 13:50 ` [dpdk-dev] [PATCH 1/3] net/virtio: register/unregister intr handler on start/stop Luca Boccassi
@ 2018-08-16 13:50 ` Luca Boccassi
  2018-09-17 19:06   ` Louis Luo
  2018-08-16 13:50 ` [dpdk-dev] [PATCH 3/3] eal/linux: handle uio read failure in interrupt handler Luca Boccassi
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 30+ messages in thread
From: Luca Boccassi @ 2018-08-16 13:50 UTC (permalink / raw)
  To: dev
  Cc: maxime.coquelin, tiwei.bie, yongwang, 3chas3, bruce.richardson,
	jianfeng.tan, anatoly.burakov, Luca Boccassi, stable,
	Brian Russell

The vmxnet3 driver can't call back into dev_close(), and possibly
dev_stop(), in dev_uninit().  When dev_uninit() is called, anything
that those routines would want to clean up has already been released.
Further, for complete cleanup, it is necessary to release any of the
queue resources during dev_close().
This allows a vmxnet3 device to be hot-unplugged without leaking
queues.

Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode driver implementation")
Cc: stable@dpdk.org

Signed-off-by: Brian Russell <brussell@brocade.com>
Signed-off-by: Luca Boccassi <bluca@debian.org>
---
 drivers/net/vmxnet3/vmxnet3_ethdev.c | 36 ++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c b/drivers/net/vmxnet3/vmxnet3_ethdev.c
index 2613cd1358..b5d4be5e24 100644
--- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
+++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
@@ -348,16 +348,11 @@ eth_vmxnet3_dev_init(struct rte_eth_dev *eth_dev)
 static int
 eth_vmxnet3_dev_uninit(struct rte_eth_dev *eth_dev)
 {
-	struct vmxnet3_hw *hw = eth_dev->data->dev_private;
-
 	PMD_INIT_FUNC_TRACE();
 
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return 0;
 
-	if (hw->adapter_stopped == 0)
-		vmxnet3_dev_close(eth_dev);
-
 	eth_dev->dev_ops = NULL;
 	eth_dev->rx_pkt_burst = NULL;
 	eth_dev->tx_pkt_burst = NULL;
@@ -803,7 +798,7 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
 	PMD_INIT_FUNC_TRACE();
 
 	if (hw->adapter_stopped == 1) {
-		PMD_INIT_LOG(DEBUG, "Device already closed.");
+		PMD_INIT_LOG(DEBUG, "Device already stopped.");
 		return;
 	}
 
@@ -827,7 +822,6 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
 	/* reset the device */
 	VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_CMD, VMXNET3_CMD_RESET_DEV);
 	PMD_INIT_LOG(DEBUG, "Device reset.");
-	hw->adapter_stopped = 0;
 
 	vmxnet3_dev_clear_queues(dev);
 
@@ -837,6 +831,30 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
 	link.link_speed = ETH_SPEED_NUM_10G;
 	link.link_autoneg = ETH_LINK_FIXED;
 	rte_eth_linkstatus_set(dev, &link);
+
+	hw->adapter_stopped = 1;
+}
+
+static void
+vmxnet3_free_queues(struct rte_eth_dev *dev)
+{
+	int i;
+
+	PMD_INIT_FUNC_TRACE();
+
+	for (i = 0; i < dev->data->nb_rx_queues; i++) {
+		void *rxq = dev->data->rx_queues[i];
+
+		vmxnet3_dev_rx_queue_release(rxq);
+	}
+	dev->data->nb_rx_queues = 0;
+
+	for (i = 0; i < dev->data->nb_tx_queues; i++) {
+		void *txq = dev->data->tx_queues[i];
+
+		vmxnet3_dev_tx_queue_release(txq);
+	}
+	dev->data->nb_tx_queues = 0;
 }
 
 /*
@@ -845,12 +863,10 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
 static void
 vmxnet3_dev_close(struct rte_eth_dev *dev)
 {
-	struct vmxnet3_hw *hw = dev->data->dev_private;
-
 	PMD_INIT_FUNC_TRACE();
 
 	vmxnet3_dev_stop(dev);
-	hw->adapter_stopped = 1;
+	vmxnet3_free_queues(dev);
 }
 
 static void
-- 
2.18.0

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

* [dpdk-dev] [PATCH 3/3] eal/linux: handle uio read failure in interrupt handler
  2018-08-16 13:50 [dpdk-dev] [PATCH 0/3] Fix hot plug/unplug of virtual devices Luca Boccassi
  2018-08-16 13:50 ` [dpdk-dev] [PATCH 1/3] net/virtio: register/unregister intr handler on start/stop Luca Boccassi
  2018-08-16 13:50 ` [dpdk-dev] [PATCH 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug Luca Boccassi
@ 2018-08-16 13:50 ` Luca Boccassi
  2018-09-19 12:57 ` [dpdk-dev] [PATCH v2 1/3] net/virtio: register/unregister intr handler on start/stop Luca Boccassi
  2018-10-31 18:39 ` [dpdk-dev] [PATCH v3 " Luca Boccassi
  4 siblings, 0 replies; 30+ messages in thread
From: Luca Boccassi @ 2018-08-16 13:50 UTC (permalink / raw)
  To: dev
  Cc: maxime.coquelin, tiwei.bie, yongwang, 3chas3, bruce.richardson,
	jianfeng.tan, anatoly.burakov, Luca Boccassi, stable,
	Brian Russell

If a device is unplugged while an interrupt is pending, the
read call to the uio device to remove it from the poll wait list
can fail resulting in it being continually polled forever. This
change checks for the read failing and if so, unregisters the device
as an interrupt source and causes the wait list to be rebuilt.

This race has been reported and observed in production.

Fixes: 0a45657a6794 ("pci: rework interrupt handling")
Cc: stable@dpdk.org

Signed-off-by: Brian Russell <brussell@brocade.com>
Signed-off-by: Luca Boccassi <bluca@debian.org>
---
 lib/librte_eal/linuxapp/eal/eal_interrupts.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
index 4076c6d6ca..34584db883 100644
--- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
+++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
@@ -627,7 +627,7 @@ eal_intr_process_interrupts(struct epoll_event *events, int nfds)
 	bool call = false;
 	int n, bytes_read;
 	struct rte_intr_source *src;
-	struct rte_intr_callback *cb;
+	struct rte_intr_callback *cb, *next;
 	union rte_intr_read_buffer buf;
 	struct rte_intr_callback active_cb;
 
@@ -701,6 +701,23 @@ eal_intr_process_interrupts(struct epoll_event *events, int nfds)
 					"descriptor %d: %s\n",
 					events[n].data.fd,
 					strerror(errno));
+				/*
+				 * The device is unplugged or buggy, remove
+				 * it as an interrupt source and return to
+				 * force the wait list to be rebuilt.
+				 */
+				rte_spinlock_lock(&intr_lock);
+				TAILQ_REMOVE(&intr_sources, src, next);
+				rte_spinlock_unlock(&intr_lock);
+
+				for (cb = TAILQ_FIRST(&src->callbacks); cb;
+							cb = next) {
+					next = TAILQ_NEXT(cb, next);
+					TAILQ_REMOVE(&src->callbacks, cb, next);
+					free(cb);
+				}
+				free(src);
+				return -1;
 			} else if (bytes_read == 0)
 				RTE_LOG(ERR, EAL, "Read nothing from file "
 					"descriptor %d\n", events[n].data.fd);
-- 
2.18.0

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

* Re: [dpdk-dev] [PATCH 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug
  2018-08-16 13:50 ` [dpdk-dev] [PATCH 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug Luca Boccassi
@ 2018-09-17 19:06   ` Louis Luo
  2018-09-18 13:14     ` Luca Boccassi
  0 siblings, 1 reply; 30+ messages in thread
From: Louis Luo @ 2018-09-17 19:06 UTC (permalink / raw)
  To: Luca Boccassi, dev
  Cc: maxime.coquelin, tiwei.bie, Yong Wang, 3chas3, bruce.richardson,
	jianfeng.tan, anatoly.burakov, stable, Brian Russell

Hi Luca,

When eth_vmxnet3_dev_uninit() is called, is it guaranteed that vmxnet3_dev_close/ vmxnet3_dev_stop must have been called? I'm not familiar with the hot-plug procedure, so just wonder if there is any chance that eth_vmxnet3_dev_uninit() is called without calling vmxnet3_dev_close/ vmxnet3_dev_stop.

Thanks,
Louis

On 8/16/18, 6:51 AM, "dev on behalf of Luca Boccassi" <dev-bounces@dpdk.org on behalf of bluca@debian.org> wrote:

    The vmxnet3 driver can't call back into dev_close(), and possibly
    dev_stop(), in dev_uninit().  When dev_uninit() is called, anything
    that those routines would want to clean up has already been released.
    Further, for complete cleanup, it is necessary to release any of the
    queue resources during dev_close().
    This allows a vmxnet3 device to be hot-unplugged without leaking
    queues.
    
    Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode driver implementation")
    Cc: stable@dpdk.org
    
    Signed-off-by: Brian Russell <brussell@brocade.com>
    Signed-off-by: Luca Boccassi <bluca@debian.org>
    ---
     drivers/net/vmxnet3/vmxnet3_ethdev.c | 36 ++++++++++++++++++++--------
     1 file changed, 26 insertions(+), 10 deletions(-)
    
    diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c b/drivers/net/vmxnet3/vmxnet3_ethdev.c
    index 2613cd1358..b5d4be5e24 100644
    --- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
    +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
    @@ -348,16 +348,11 @@ eth_vmxnet3_dev_init(struct rte_eth_dev *eth_dev)
     static int
     eth_vmxnet3_dev_uninit(struct rte_eth_dev *eth_dev)
     {
    -	struct vmxnet3_hw *hw = eth_dev->data->dev_private;
    -
     	PMD_INIT_FUNC_TRACE();
     
     	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
     		return 0;
     
    -	if (hw->adapter_stopped == 0)
    -		vmxnet3_dev_close(eth_dev);
    -
     	eth_dev->dev_ops = NULL;
     	eth_dev->rx_pkt_burst = NULL;
     	eth_dev->tx_pkt_burst = NULL;
    @@ -803,7 +798,7 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
     	PMD_INIT_FUNC_TRACE();
     
     	if (hw->adapter_stopped == 1) {
    -		PMD_INIT_LOG(DEBUG, "Device already closed.");
    +		PMD_INIT_LOG(DEBUG, "Device already stopped.");
     		return;
     	}
     
    @@ -827,7 +822,6 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
     	/* reset the device */
     	VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_CMD, VMXNET3_CMD_RESET_DEV);
     	PMD_INIT_LOG(DEBUG, "Device reset.");
    -	hw->adapter_stopped = 0;
     
     	vmxnet3_dev_clear_queues(dev);
     
    @@ -837,6 +831,30 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
     	link.link_speed = ETH_SPEED_NUM_10G;
     	link.link_autoneg = ETH_LINK_FIXED;
     	rte_eth_linkstatus_set(dev, &link);
    +
    +	hw->adapter_stopped = 1;
    +}
    +
    +static void
    +vmxnet3_free_queues(struct rte_eth_dev *dev)
    +{
    +	int i;
    +
    +	PMD_INIT_FUNC_TRACE();
    +
    +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
    +		void *rxq = dev->data->rx_queues[i];
    +
    +		vmxnet3_dev_rx_queue_release(rxq);
    +	}
    +	dev->data->nb_rx_queues = 0;
    +
    +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
    +		void *txq = dev->data->tx_queues[i];
    +
    +		vmxnet3_dev_tx_queue_release(txq);
    +	}
    +	dev->data->nb_tx_queues = 0;
     }
     
     /*
    @@ -845,12 +863,10 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
     static void
     vmxnet3_dev_close(struct rte_eth_dev *dev)
     {
    -	struct vmxnet3_hw *hw = dev->data->dev_private;
    -
     	PMD_INIT_FUNC_TRACE();
     
     	vmxnet3_dev_stop(dev);
    -	hw->adapter_stopped = 1;
    +	vmxnet3_free_queues(dev);
     }
     
     static void
    -- 
    2.18.0
    
    


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

* Re: [dpdk-dev] [PATCH 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug
  2018-09-17 19:06   ` Louis Luo
@ 2018-09-18 13:14     ` Luca Boccassi
  2018-09-18 18:14       ` Louis Luo
  0 siblings, 1 reply; 30+ messages in thread
From: Luca Boccassi @ 2018-09-18 13:14 UTC (permalink / raw)
  To: Louis Luo, dev
  Cc: maxime.coquelin, tiwei.bie, Yong Wang, 3chas3, bruce.richardson,
	jianfeng.tan, anatoly.burakov, stable, Brian Russell

Hi,

The application must already stop and close before detaching (which
will call uninit). Quoting from the documentation:

"*  Before detaching, they must be stopped and closed.

    DPDK applications must call "rte_eth_dev_stop()" and
    "rte_eth_dev_close()" APIs before detaching ports. These functions will
    start finalization sequence of the PMDs."

http://doc.dpdk.org/guides/prog_guide/port_hotplug_framework.html

On Mon, 2018-09-17 at 19:06 +0000, Louis Luo wrote:
> Hi Luca,
> 
> When eth_vmxnet3_dev_uninit() is called, is it guaranteed that
> vmxnet3_dev_close/ vmxnet3_dev_stop must have been called? I'm not
> familiar with the hot-plug procedure, so just wonder if there is any
> chance that eth_vmxnet3_dev_uninit() is called without calling
> vmxnet3_dev_close/ vmxnet3_dev_stop.
> 
> Thanks,
> Louis
> 
> On 8/16/18, 6:51 AM, "dev on behalf of Luca Boccassi" <dev-bounces@d
> pdk.org on behalf of bluca@debian.org> wrote:
> 
>     The vmxnet3 driver can't call back into dev_close(), and possibly
>     dev_stop(), in dev_uninit().  When dev_uninit() is called,
> anything
>     that those routines would want to clean up has already been
> released.
>     Further, for complete cleanup, it is necessary to release any of
> the
>     queue resources during dev_close().
>     This allows a vmxnet3 device to be hot-unplugged without leaking
>     queues.
>     
>     Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode
> driver implementation")
>     Cc: stable@dpdk.org
>     
>     Signed-off-by: Brian Russell <brussell@brocade.com>
>     Signed-off-by: Luca Boccassi <bluca@debian.org>
>     ---
>      drivers/net/vmxnet3/vmxnet3_ethdev.c | 36 ++++++++++++++++++++
> --------
>      1 file changed, 26 insertions(+), 10 deletions(-)
>     
>     diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c
> b/drivers/net/vmxnet3/vmxnet3_ethdev.c
>     index 2613cd1358..b5d4be5e24 100644
>     --- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
>     +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
>     @@ -348,16 +348,11 @@ eth_vmxnet3_dev_init(struct rte_eth_dev
> *eth_dev)
>      static int
>      eth_vmxnet3_dev_uninit(struct rte_eth_dev *eth_dev)
>      {
>     -	struct vmxnet3_hw *hw = eth_dev->data->dev_private;
>     -
>      	PMD_INIT_FUNC_TRACE();
>      
>      	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>      		return 0;
>      
>     -	if (hw->adapter_stopped == 0)
>     -		vmxnet3_dev_close(eth_dev);
>     -
>      	eth_dev->dev_ops = NULL;
>      	eth_dev->rx_pkt_burst = NULL;
>      	eth_dev->tx_pkt_burst = NULL;
>     @@ -803,7 +798,7 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
>      	PMD_INIT_FUNC_TRACE();
>      
>      	if (hw->adapter_stopped == 1) {
>     -		PMD_INIT_LOG(DEBUG, "Device already closed.");
>     +		PMD_INIT_LOG(DEBUG, "Device already stopped.");
>      		return;
>      	}
>      
>     @@ -827,7 +822,6 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
>      	/* reset the device */
>      	VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_CMD,
> VMXNET3_CMD_RESET_DEV);
>      	PMD_INIT_LOG(DEBUG, "Device reset.");
>     -	hw->adapter_stopped = 0;
>      
>      	vmxnet3_dev_clear_queues(dev);
>      
>     @@ -837,6 +831,30 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
>      	link.link_speed = ETH_SPEED_NUM_10G;
>      	link.link_autoneg = ETH_LINK_FIXED;
>      	rte_eth_linkstatus_set(dev, &link);
>     +
>     +	hw->adapter_stopped = 1;
>     +}
>     +
>     +static void
>     +vmxnet3_free_queues(struct rte_eth_dev *dev)
>     +{
>     +	int i;
>     +
>     +	PMD_INIT_FUNC_TRACE();
>     +
>     +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
>     +		void *rxq = dev->data->rx_queues[i];
>     +
>     +		vmxnet3_dev_rx_queue_release(rxq);
>     +	}
>     +	dev->data->nb_rx_queues = 0;
>     +
>     +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
>     +		void *txq = dev->data->tx_queues[i];
>     +
>     +		vmxnet3_dev_tx_queue_release(txq);
>     +	}
>     +	dev->data->nb_tx_queues = 0;
>      }
>      
>      /*
>     @@ -845,12 +863,10 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
>      static void
>      vmxnet3_dev_close(struct rte_eth_dev *dev)
>      {
>     -	struct vmxnet3_hw *hw = dev->data->dev_private;
>     -
>      	PMD_INIT_FUNC_TRACE();
>      
>      	vmxnet3_dev_stop(dev);
>     -	hw->adapter_stopped = 1;
>     +	vmxnet3_free_queues(dev);
>      }
>      
>      static void
>     -- 
>     2.18.0
>     
>     
> 

-- 
Kind regards,
Luca Boccassi

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

* Re: [dpdk-dev] [PATCH 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug
  2018-09-18 13:14     ` Luca Boccassi
@ 2018-09-18 18:14       ` Louis Luo
  2018-09-18 18:29         ` Luca Boccassi
  0 siblings, 1 reply; 30+ messages in thread
From: Louis Luo @ 2018-09-18 18:14 UTC (permalink / raw)
  To: Luca Boccassi, dev
  Cc: maxime.coquelin, tiwei.bie, Yong Wang, 3chas3, bruce.richardson,
	jianfeng.tan, anatoly.burakov, stable, Brian Russell

Hi Luca,

Thanks for pointing to the document! This "basic requirements" seems to lay the burden on application developers to correctly follow the hot-plug framework's rules, but there seems no mechanism to enforce this procedure (correct me if I'm wrong). What if a buggy application doesn't call stop/close before detaching? 

In addition, in your commit description, you said " The vmxnet3 driver can't call back into dev_close(), and possibly dev_stop(), in dev_uninit()." But actually in vmxnet3_dev_close(), we set hw->adapter_stopped = 1, and in eth_vmxnet3_dev_uninit() we call into vmxnet3_dev_close() ONLY when hw->adapter_stopped == 0. So if the application does meet the hot-plug framework requirement and calls dev_close before calling uninit, eth_vmxnet3_dev_uninit() should not call into vmxnet3_dev_close() again, right? If so, why bother removing this check? 

Or let me ask this way. If a buggy application DOES NOT call dev_close before calling eth_vmxnet3_dev_uninit(), would calling vmxnet3_dev_close() inside eth_vmxnet3_dev_uninit() cause any trouble? And if an application DOES call dev_close before calling eth_vmxnet3_dev_uninit(), have you ever seen vmxnet3_dev_close() being called again and trigger crashes like double-free or something else? If yes, then we need to investigate.

Thanks
Louis

On 9/18/18, 6:15 AM, "Luca Boccassi" <bluca@debian.org> wrote:

    Hi,
    
    The application must already stop and close before detaching (which
    will call uninit). Quoting from the documentation:
    
    "*  Before detaching, they must be stopped and closed.
    
        DPDK applications must call "rte_eth_dev_stop()" and
        "rte_eth_dev_close()" APIs before detaching ports. These functions will
        start finalization sequence of the PMDs."
    
    https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdoc.dpdk.org%2Fguides%2Fprog_guide%2Fport_hotplug_framework.html&amp;data=02%7C01%7Cllouis%40vmware.com%7C3d87a5482cd046679d7608d61d68bf9a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636728733078700911&amp;sdata=qBx7zyOJhTmVH0c2ZEBohmIHL6PkYwB6XPaw2epgS0E%3D&amp;reserved=0
    
    On Mon, 2018-09-17 at 19:06 +0000, Louis Luo wrote:
    > Hi Luca,
    > 
    > When eth_vmxnet3_dev_uninit() is called, is it guaranteed that
    > vmxnet3_dev_close/ vmxnet3_dev_stop must have been called? I'm not
    > familiar with the hot-plug procedure, so just wonder if there is any
    > chance that eth_vmxnet3_dev_uninit() is called without calling
    > vmxnet3_dev_close/ vmxnet3_dev_stop.
    > 
    > Thanks,
    > Louis
    > 
    > On 8/16/18, 6:51 AM, "dev on behalf of Luca Boccassi" <dev-bounces@d
    > pdk.org on behalf of bluca@debian.org> wrote:
    > 
    >     The vmxnet3 driver can't call back into dev_close(), and possibly
    >     dev_stop(), in dev_uninit().  When dev_uninit() is called,
    > anything
    >     that those routines would want to clean up has already been
    > released.
    >     Further, for complete cleanup, it is necessary to release any of
    > the
    >     queue resources during dev_close().
    >     This allows a vmxnet3 device to be hot-unplugged without leaking
    >     queues.
    >     
    >     Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode
    > driver implementation")
    >     Cc: stable@dpdk.org
    >     
    >     Signed-off-by: Brian Russell <brussell@brocade.com>
    >     Signed-off-by: Luca Boccassi <bluca@debian.org>
    >     ---
    >      drivers/net/vmxnet3/vmxnet3_ethdev.c | 36 ++++++++++++++++++++
    > --------
    >      1 file changed, 26 insertions(+), 10 deletions(-)
    >     
    >     diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c
    > b/drivers/net/vmxnet3/vmxnet3_ethdev.c
    >     index 2613cd1358..b5d4be5e24 100644
    >     --- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
    >     +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
    >     @@ -348,16 +348,11 @@ eth_vmxnet3_dev_init(struct rte_eth_dev
    > *eth_dev)
    >      static int
    >      eth_vmxnet3_dev_uninit(struct rte_eth_dev *eth_dev)
    >      {
    >     -	struct vmxnet3_hw *hw = eth_dev->data->dev_private;
    >     -
    >      	PMD_INIT_FUNC_TRACE();
    >      
    >      	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
    >      		return 0;
    >      
    >     -	if (hw->adapter_stopped == 0)
    >     -		vmxnet3_dev_close(eth_dev);
    >     -
    >      	eth_dev->dev_ops = NULL;
    >      	eth_dev->rx_pkt_burst = NULL;
    >      	eth_dev->tx_pkt_burst = NULL;
    >     @@ -803,7 +798,7 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
    >      	PMD_INIT_FUNC_TRACE();
    >      
    >      	if (hw->adapter_stopped == 1) {
    >     -		PMD_INIT_LOG(DEBUG, "Device already closed.");
    >     +		PMD_INIT_LOG(DEBUG, "Device already stopped.");
    >      		return;
    >      	}
    >      
    >     @@ -827,7 +822,6 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
    >      	/* reset the device */
    >      	VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_CMD,
    > VMXNET3_CMD_RESET_DEV);
    >      	PMD_INIT_LOG(DEBUG, "Device reset.");
    >     -	hw->adapter_stopped = 0;
    >      
    >      	vmxnet3_dev_clear_queues(dev);
    >      
    >     @@ -837,6 +831,30 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
    >      	link.link_speed = ETH_SPEED_NUM_10G;
    >      	link.link_autoneg = ETH_LINK_FIXED;
    >      	rte_eth_linkstatus_set(dev, &link);
    >     +
    >     +	hw->adapter_stopped = 1;
    >     +}
    >     +
    >     +static void
    >     +vmxnet3_free_queues(struct rte_eth_dev *dev)
    >     +{
    >     +	int i;
    >     +
    >     +	PMD_INIT_FUNC_TRACE();
    >     +
    >     +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
    >     +		void *rxq = dev->data->rx_queues[i];
    >     +
    >     +		vmxnet3_dev_rx_queue_release(rxq);
    >     +	}
    >     +	dev->data->nb_rx_queues = 0;
    >     +
    >     +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
    >     +		void *txq = dev->data->tx_queues[i];
    >     +
    >     +		vmxnet3_dev_tx_queue_release(txq);
    >     +	}
    >     +	dev->data->nb_tx_queues = 0;
    >      }
    >      
    >      /*
    >     @@ -845,12 +863,10 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
    >      static void
    >      vmxnet3_dev_close(struct rte_eth_dev *dev)
    >      {
    >     -	struct vmxnet3_hw *hw = dev->data->dev_private;
    >     -
    >      	PMD_INIT_FUNC_TRACE();
    >      
    >      	vmxnet3_dev_stop(dev);
    >     -	hw->adapter_stopped = 1;
    >     +	vmxnet3_free_queues(dev);
    >      }
    >      
    >      static void
    >     -- 
    >     2.18.0
    >     
    >     
    > 
    
    -- 
    Kind regards,
    Luca Boccassi


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

* Re: [dpdk-dev] [PATCH 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug
  2018-09-18 18:14       ` Louis Luo
@ 2018-09-18 18:29         ` Luca Boccassi
  2018-09-18 18:48           ` Louis Luo
  0 siblings, 1 reply; 30+ messages in thread
From: Luca Boccassi @ 2018-09-18 18:29 UTC (permalink / raw)
  To: Louis Luo, dev; +Cc: brussell

On Tue, 2018-09-18 at 18:14 +0000, Louis Luo wrote:
> Hi Luca,
> 
> Thanks for pointing to the document! This "basic requirements" seems
> to lay the burden on application developers to correctly follow the
> hot-plug framework's rules, but there seems no mechanism to enforce
> this procedure (correct me if I'm wrong). What if a buggy application
> doesn't call stop/close before detaching? 

Well DPDK in general does not have the simplest API to use, there's no
denying that. That's because what it does is very much more complicated
than the average library.
So if an application ignores the development guides and does not follow
the recommendation bad things will happen :-) But it will happen
regardless of the PMD used. That's already the case.

> In addition, in your commit description, you said " The vmxnet3
> driver can't call back into dev_close(), and possibly dev_stop(), in
> dev_uninit()." But actually in vmxnet3_dev_close(), we set hw-
> >adapter_stopped = 1, and in eth_vmxnet3_dev_uninit() we call into
> vmxnet3_dev_close() ONLY when hw->adapter_stopped == 0. So if the
> application does meet the hot-plug framework requirement and calls
> dev_close before calling uninit, eth_vmxnet3_dev_uninit() should not
> call into vmxnet3_dev_close() again, right? If so, why bother
> removing this check? 
> 
> Or let me ask this way. If a buggy application DOES NOT call
> dev_close before calling eth_vmxnet3_dev_uninit(), would calling
> vmxnet3_dev_close() inside eth_vmxnet3_dev_uninit() cause any
> trouble? And if an application DOES call dev_close before calling
> eth_vmxnet3_dev_uninit(), have you ever seen vmxnet3_dev_close()
> being called again and trigger crashes like double-free or something
> else? If yes, then we need to investigate.
> 
> Thanks
> Louis

The problem is that there are many PMDs, so the guidelines have to be
followed - so stop and close have to be called anyway before detach.
Otherwise running with other PMDs will break the application. So not
calling close is not really an option for an application.

But the main issue here is the leaking of the queues, which this patch
fixes. By re-arranging the stop/close/uninit like this, queues can be
correctly freed and thus we can hotplug/unplug devices without leaking
massive amounts of memory.

My colleague Brian, who wrote the patch and I CC'ed, might have more
information if you need it.

> On 9/18/18, 6:15 AM, "Luca Boccassi" <bluca@debian.org> wrote:
> 
>     Hi,
>     
>     The application must already stop and close before detaching
> (which
>     will call uninit). Quoting from the documentation:
>     
>     "*  Before detaching, they must be stopped and closed.
>     
>         DPDK applications must call "rte_eth_dev_stop()" and
>         "rte_eth_dev_close()" APIs before detaching ports. These
> functions will
>         start finalization sequence of the PMDs."
>     
>     https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fd
> oc.dpdk.org%2Fguides%2Fprog_guide%2Fport_hotplug_framework.html&amp;d
> ata=02%7C01%7Cllouis%40vmware.com%7C3d87a5482cd046679d7608d61d68bf9a%
> 7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636728733078700911&amp;s
> data=qBx7zyOJhTmVH0c2ZEBohmIHL6PkYwB6XPaw2epgS0E%3D&amp;reserved=0
>     
>     On Mon, 2018-09-17 at 19:06 +0000, Louis Luo wrote:
>     > Hi Luca,
>     > 
>     > When eth_vmxnet3_dev_uninit() is called, is it guaranteed that
>     > vmxnet3_dev_close/ vmxnet3_dev_stop must have been called? I'm
> not
>     > familiar with the hot-plug procedure, so just wonder if there
> is any
>     > chance that eth_vmxnet3_dev_uninit() is called without calling
>     > vmxnet3_dev_close/ vmxnet3_dev_stop.
>     > 
>     > Thanks,
>     > Louis
>     > 
>     > On 8/16/18, 6:51 AM, "dev on behalf of Luca Boccassi" <dev-boun
> ces@d
>     > pdk.org on behalf of bluca@debian.org> wrote:
>     > 
>     >     The vmxnet3 driver can't call back into dev_close(), and
> possibly
>     >     dev_stop(), in dev_uninit().  When dev_uninit() is called,
>     > anything
>     >     that those routines would want to clean up has already been
>     > released.
>     >     Further, for complete cleanup, it is necessary to release
> any of
>     > the
>     >     queue resources during dev_close().
>     >     This allows a vmxnet3 device to be hot-unplugged without
> leaking
>     >     queues.
>     >     
>     >     Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode
>     > driver implementation")
>     >     Cc: stable@dpdk.org
>     >     
>     >     Signed-off-by: Brian Russell <brussell@brocade.com>
>     >     Signed-off-by: Luca Boccassi <bluca@debian.org>
>     >     ---
>     >      drivers/net/vmxnet3/vmxnet3_ethdev.c | 36
> ++++++++++++++++++++
>     > --------
>     >      1 file changed, 26 insertions(+), 10 deletions(-)
>     >     
>     >     diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c
>     > b/drivers/net/vmxnet3/vmxnet3_ethdev.c
>     >     index 2613cd1358..b5d4be5e24 100644
>     >     --- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
>     >     +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
>     >     @@ -348,16 +348,11 @@ eth_vmxnet3_dev_init(struct
> rte_eth_dev
>     > *eth_dev)
>     >      static int
>     >      eth_vmxnet3_dev_uninit(struct rte_eth_dev *eth_dev)
>     >      {
>     >     -	struct vmxnet3_hw *hw = eth_dev->data-
> >dev_private;
>     >     -
>     >      	PMD_INIT_FUNC_TRACE();
>     >      
>     >      	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>     >      		return 0;
>     >      
>     >     -	if (hw->adapter_stopped == 0)
>     >     -		vmxnet3_dev_close(eth_dev);
>     >     -
>     >      	eth_dev->dev_ops = NULL;
>     >      	eth_dev->rx_pkt_burst = NULL;
>     >      	eth_dev->tx_pkt_burst = NULL;
>     >     @@ -803,7 +798,7 @@ vmxnet3_dev_stop(struct rte_eth_dev
> *dev)
>     >      	PMD_INIT_FUNC_TRACE();
>     >      
>     >      	if (hw->adapter_stopped == 1) {
>     >     -		PMD_INIT_LOG(DEBUG, "Device already
> closed.");
>     >     +		PMD_INIT_LOG(DEBUG, "Device already
> stopped.");
>     >      		return;
>     >      	}
>     >      
>     >     @@ -827,7 +822,6 @@ vmxnet3_dev_stop(struct rte_eth_dev
> *dev)
>     >      	/* reset the device */
>     >      	VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_CMD,
>     > VMXNET3_CMD_RESET_DEV);
>     >      	PMD_INIT_LOG(DEBUG, "Device reset.");
>     >     -	hw->adapter_stopped = 0;
>     >      
>     >      	vmxnet3_dev_clear_queues(dev);
>     >      
>     >     @@ -837,6 +831,30 @@ vmxnet3_dev_stop(struct rte_eth_dev
> *dev)
>     >      	link.link_speed = ETH_SPEED_NUM_10G;
>     >      	link.link_autoneg = ETH_LINK_FIXED;
>     >      	rte_eth_linkstatus_set(dev, &link);
>     >     +
>     >     +	hw->adapter_stopped = 1;
>     >     +}
>     >     +
>     >     +static void
>     >     +vmxnet3_free_queues(struct rte_eth_dev *dev)
>     >     +{
>     >     +	int i;
>     >     +
>     >     +	PMD_INIT_FUNC_TRACE();
>     >     +
>     >     +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
>     >     +		void *rxq = dev->data->rx_queues[i];
>     >     +
>     >     +		vmxnet3_dev_rx_queue_release(rxq);
>     >     +	}
>     >     +	dev->data->nb_rx_queues = 0;
>     >     +
>     >     +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
>     >     +		void *txq = dev->data->tx_queues[i];
>     >     +
>     >     +		vmxnet3_dev_tx_queue_release(txq);
>     >     +	}
>     >     +	dev->data->nb_tx_queues = 0;
>     >      }
>     >      
>     >      /*
>     >     @@ -845,12 +863,10 @@ vmxnet3_dev_stop(struct rte_eth_dev
> *dev)
>     >      static void
>     >      vmxnet3_dev_close(struct rte_eth_dev *dev)
>     >      {
>     >     -	struct vmxnet3_hw *hw = dev->data->dev_private;
>     >     -
>     >      	PMD_INIT_FUNC_TRACE();
>     >      
>     >      	vmxnet3_dev_stop(dev);
>     >     -	hw->adapter_stopped = 1;
>     >     +	vmxnet3_free_queues(dev);
>     >      }
>     >      
>     >      static void
>     >     -- 
>     >     2.18.0
>     >     
>     >     
>     > 
>     
>     -- 
>     Kind regards,
>     Luca Boccassi
> 

-- 
Kind regards,
Luca Boccassi

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

* Re: [dpdk-dev] [PATCH 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug
  2018-09-18 18:29         ` Luca Boccassi
@ 2018-09-18 18:48           ` Louis Luo
  2018-09-19 12:58             ` Luca Boccassi
  0 siblings, 1 reply; 30+ messages in thread
From: Louis Luo @ 2018-09-18 18:48 UTC (permalink / raw)
  To: Luca Boccassi, dev; +Cc: brussell

Hi Luca,

I'm fine with the queue free part, but just have concern about removing the check inside uninit function. If the application does follow the rules and keeping the check in uninit won't cause vmxnet3_dev_close() being called again (and if it won't cause trouble in case the application doesn't call close before calling uninit), I prefer keeping the check (and log some warning) in case any buggy application exists.

Thanks,
Louis

On 9/18/18, 11:29 AM, "Luca Boccassi" <bluca@debian.org> wrote:

    On Tue, 2018-09-18 at 18:14 +0000, Louis Luo wrote:
    > Hi Luca,
    > 
    > Thanks for pointing to the document! This "basic requirements" seems
    > to lay the burden on application developers to correctly follow the
    > hot-plug framework's rules, but there seems no mechanism to enforce
    > this procedure (correct me if I'm wrong). What if a buggy application
    > doesn't call stop/close before detaching? 
    
    Well DPDK in general does not have the simplest API to use, there's no
    denying that. That's because what it does is very much more complicated
    than the average library.
    So if an application ignores the development guides and does not follow
    the recommendation bad things will happen :-) But it will happen
    regardless of the PMD used. That's already the case.
    
    > In addition, in your commit description, you said " The vmxnet3
    > driver can't call back into dev_close(), and possibly dev_stop(), in
    > dev_uninit()." But actually in vmxnet3_dev_close(), we set hw-
    > >adapter_stopped = 1, and in eth_vmxnet3_dev_uninit() we call into
    > vmxnet3_dev_close() ONLY when hw->adapter_stopped == 0. So if the
    > application does meet the hot-plug framework requirement and calls
    > dev_close before calling uninit, eth_vmxnet3_dev_uninit() should not
    > call into vmxnet3_dev_close() again, right? If so, why bother
    > removing this check? 
    > 
    > Or let me ask this way. If a buggy application DOES NOT call
    > dev_close before calling eth_vmxnet3_dev_uninit(), would calling
    > vmxnet3_dev_close() inside eth_vmxnet3_dev_uninit() cause any
    > trouble? And if an application DOES call dev_close before calling
    > eth_vmxnet3_dev_uninit(), have you ever seen vmxnet3_dev_close()
    > being called again and trigger crashes like double-free or something
    > else? If yes, then we need to investigate.
    > 
    > Thanks
    > Louis
    
    The problem is that there are many PMDs, so the guidelines have to be
    followed - so stop and close have to be called anyway before detach.
    Otherwise running with other PMDs will break the application. So not
    calling close is not really an option for an application.
    
    But the main issue here is the leaking of the queues, which this patch
    fixes. By re-arranging the stop/close/uninit like this, queues can be
    correctly freed and thus we can hotplug/unplug devices without leaking
    massive amounts of memory.
    
    My colleague Brian, who wrote the patch and I CC'ed, might have more
    information if you need it.
    
    > On 9/18/18, 6:15 AM, "Luca Boccassi" <bluca@debian.org> wrote:
    > 
    >     Hi,
    >     
    >     The application must already stop and close before detaching
    > (which
    >     will call uninit). Quoting from the documentation:
    >     
    >     "*  Before detaching, they must be stopped and closed.
    >     
    >         DPDK applications must call "rte_eth_dev_stop()" and
    >         "rte_eth_dev_close()" APIs before detaching ports. These
    > functions will
    >         start finalization sequence of the PMDs."
    >     
    >     https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fd
    > oc.dpdk.org%2Fguides%2Fprog_guide%2Fport_hotplug_framework.html&amp;d
    > ata=02%7C01%7Cllouis%40vmware.com%7C3d87a5482cd046679d7608d61d68bf9a%
    > 7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636728733078700911&amp;s
    > data=qBx7zyOJhTmVH0c2ZEBohmIHL6PkYwB6XPaw2epgS0E%3D&amp;reserved=0
    >     
    >     On Mon, 2018-09-17 at 19:06 +0000, Louis Luo wrote:
    >     > Hi Luca,
    >     > 
    >     > When eth_vmxnet3_dev_uninit() is called, is it guaranteed that
    >     > vmxnet3_dev_close/ vmxnet3_dev_stop must have been called? I'm
    > not
    >     > familiar with the hot-plug procedure, so just wonder if there
    > is any
    >     > chance that eth_vmxnet3_dev_uninit() is called without calling
    >     > vmxnet3_dev_close/ vmxnet3_dev_stop.
    >     > 
    >     > Thanks,
    >     > Louis
    >     > 
    >     > On 8/16/18, 6:51 AM, "dev on behalf of Luca Boccassi" <dev-boun
    > ces@d
    >     > pdk.org on behalf of bluca@debian.org> wrote:
    >     > 
    >     >     The vmxnet3 driver can't call back into dev_close(), and
    > possibly
    >     >     dev_stop(), in dev_uninit().  When dev_uninit() is called,
    >     > anything
    >     >     that those routines would want to clean up has already been
    >     > released.
    >     >     Further, for complete cleanup, it is necessary to release
    > any of
    >     > the
    >     >     queue resources during dev_close().
    >     >     This allows a vmxnet3 device to be hot-unplugged without
    > leaking
    >     >     queues.
    >     >     
    >     >     Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode
    >     > driver implementation")
    >     >     Cc: stable@dpdk.org
    >     >     
    >     >     Signed-off-by: Brian Russell <brussell@brocade.com>
    >     >     Signed-off-by: Luca Boccassi <bluca@debian.org>
    >     >     ---
    >     >      drivers/net/vmxnet3/vmxnet3_ethdev.c | 36
    > ++++++++++++++++++++
    >     > --------
    >     >      1 file changed, 26 insertions(+), 10 deletions(-)
    >     >     
    >     >     diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c
    >     > b/drivers/net/vmxnet3/vmxnet3_ethdev.c
    >     >     index 2613cd1358..b5d4be5e24 100644
    >     >     --- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
    >     >     +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
    >     >     @@ -348,16 +348,11 @@ eth_vmxnet3_dev_init(struct
    > rte_eth_dev
    >     > *eth_dev)
    >     >      static int
    >     >      eth_vmxnet3_dev_uninit(struct rte_eth_dev *eth_dev)
    >     >      {
    >     >     -	struct vmxnet3_hw *hw = eth_dev->data-
    > >dev_private;
    >     >     -
    >     >      	PMD_INIT_FUNC_TRACE();
    >     >      
    >     >      	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
    >     >      		return 0;
    >     >      
    >     >     -	if (hw->adapter_stopped == 0)
    >     >     -		vmxnet3_dev_close(eth_dev);
    >     >     -
    >     >      	eth_dev->dev_ops = NULL;
    >     >      	eth_dev->rx_pkt_burst = NULL;
    >     >      	eth_dev->tx_pkt_burst = NULL;
    >     >     @@ -803,7 +798,7 @@ vmxnet3_dev_stop(struct rte_eth_dev
    > *dev)
    >     >      	PMD_INIT_FUNC_TRACE();
    >     >      
    >     >      	if (hw->adapter_stopped == 1) {
    >     >     -		PMD_INIT_LOG(DEBUG, "Device already
    > closed.");
    >     >     +		PMD_INIT_LOG(DEBUG, "Device already
    > stopped.");
    >     >      		return;
    >     >      	}
    >     >      
    >     >     @@ -827,7 +822,6 @@ vmxnet3_dev_stop(struct rte_eth_dev
    > *dev)
    >     >      	/* reset the device */
    >     >      	VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_CMD,
    >     > VMXNET3_CMD_RESET_DEV);
    >     >      	PMD_INIT_LOG(DEBUG, "Device reset.");
    >     >     -	hw->adapter_stopped = 0;
    >     >      
    >     >      	vmxnet3_dev_clear_queues(dev);
    >     >      
    >     >     @@ -837,6 +831,30 @@ vmxnet3_dev_stop(struct rte_eth_dev
    > *dev)
    >     >      	link.link_speed = ETH_SPEED_NUM_10G;
    >     >      	link.link_autoneg = ETH_LINK_FIXED;
    >     >      	rte_eth_linkstatus_set(dev, &link);
    >     >     +
    >     >     +	hw->adapter_stopped = 1;
    >     >     +}
    >     >     +
    >     >     +static void
    >     >     +vmxnet3_free_queues(struct rte_eth_dev *dev)
    >     >     +{
    >     >     +	int i;
    >     >     +
    >     >     +	PMD_INIT_FUNC_TRACE();
    >     >     +
    >     >     +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
    >     >     +		void *rxq = dev->data->rx_queues[i];
    >     >     +
    >     >     +		vmxnet3_dev_rx_queue_release(rxq);
    >     >     +	}
    >     >     +	dev->data->nb_rx_queues = 0;
    >     >     +
    >     >     +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
    >     >     +		void *txq = dev->data->tx_queues[i];
    >     >     +
    >     >     +		vmxnet3_dev_tx_queue_release(txq);
    >     >     +	}
    >     >     +	dev->data->nb_tx_queues = 0;
    >     >      }
    >     >      
    >     >      /*
    >     >     @@ -845,12 +863,10 @@ vmxnet3_dev_stop(struct rte_eth_dev
    > *dev)
    >     >      static void
    >     >      vmxnet3_dev_close(struct rte_eth_dev *dev)
    >     >      {
    >     >     -	struct vmxnet3_hw *hw = dev->data->dev_private;
    >     >     -
    >     >      	PMD_INIT_FUNC_TRACE();
    >     >      
    >     >      	vmxnet3_dev_stop(dev);
    >     >     -	hw->adapter_stopped = 1;
    >     >     +	vmxnet3_free_queues(dev);
    >     >      }
    >     >      
    >     >      static void
    >     >     -- 
    >     >     2.18.0
    >     >     
    >     >     
    >     > 
    >     
    >     -- 
    >     Kind regards,
    >     Luca Boccassi
    > 
    
    -- 
    Kind regards,
    Luca Boccassi


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

* [dpdk-dev] [PATCH v2 1/3] net/virtio: register/unregister intr handler on start/stop
  2018-08-16 13:50 [dpdk-dev] [PATCH 0/3] Fix hot plug/unplug of virtual devices Luca Boccassi
                   ` (2 preceding siblings ...)
  2018-08-16 13:50 ` [dpdk-dev] [PATCH 3/3] eal/linux: handle uio read failure in interrupt handler Luca Boccassi
@ 2018-09-19 12:57 ` Luca Boccassi
  2018-09-19 12:57   ` [dpdk-dev] [PATCH v2 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug Luca Boccassi
                     ` (3 more replies)
  2018-10-31 18:39 ` [dpdk-dev] [PATCH v3 " Luca Boccassi
  4 siblings, 4 replies; 30+ messages in thread
From: Luca Boccassi @ 2018-09-19 12:57 UTC (permalink / raw)
  To: dev
  Cc: maxime.coquelin, tiwei.bie, yongwang, 3chas3, bruce.richardson,
	jianfeng.tan, anatoly.burakov, llouis, brussell

Register and unregister the virtio interrupt handler when the device is
started and stopped. This allows a virtio device to be hotplugged or
unplugged.

Fixes: c1f86306a026 ("virtio: add new driver")
Cc: stable@dpdk.org

Signed-off-by: Brian Russell <brussell@brocade.com>
Signed-off-by: Luca Boccassi <bluca@debian.org>
---
 drivers/net/virtio/virtio_ethdev.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index b81df0a99f..adc6a30a32 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1679,11 +1679,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 	if (ret < 0)
 		goto out;
 
-	/* Setup interrupt callback  */
-	if (eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
-		rte_intr_callback_register(eth_dev->intr_handle,
-			virtio_interrupt_handler, eth_dev);
-
 	return 0;
 
 out:
@@ -1709,11 +1704,6 @@ eth_virtio_dev_uninit(struct rte_eth_dev *eth_dev)
 	rte_free(eth_dev->data->mac_addrs);
 	eth_dev->data->mac_addrs = NULL;
 
-	/* reset interrupt callback  */
-	if (eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
-		rte_intr_callback_unregister(eth_dev->intr_handle,
-						virtio_interrupt_handler,
-						eth_dev);
 	if (eth_dev->device)
 		rte_pci_unmap_device(RTE_ETH_DEV_TO_PCI(eth_dev));
 
@@ -1972,6 +1962,12 @@ virtio_dev_start(struct rte_eth_dev *dev)
 	    dev->data->dev_conf.intr_conf.rxq) {
 		virtio_intr_disable(dev);
 
+		/* Setup interrupt callback  */
+		if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
+			rte_intr_callback_register(dev->intr_handle,
+						   virtio_interrupt_handler,
+						   dev);
+
 		if (virtio_intr_enable(dev) < 0) {
 			PMD_DRV_LOG(ERR, "interrupt enable failed");
 			return -EIO;
@@ -2081,9 +2077,17 @@ virtio_dev_stop(struct rte_eth_dev *dev)
 	PMD_INIT_LOG(DEBUG, "stop");
 
 	rte_spinlock_lock(&hw->state_lock);
-	if (intr_conf->lsc || intr_conf->rxq)
+	if (intr_conf->lsc || intr_conf->rxq) {
 		virtio_intr_disable(dev);
 
+		/* Reset interrupt callback  */
+		if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC) {
+			rte_intr_callback_unregister(dev->intr_handle,
+						     virtio_interrupt_handler,
+						     dev);
+		}
+	}
+
 	hw->started = 0;
 	memset(&link, 0, sizeof(link));
 	rte_eth_linkstatus_set(dev, &link);
-- 
2.18.0

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

* [dpdk-dev] [PATCH v2 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug
  2018-09-19 12:57 ` [dpdk-dev] [PATCH v2 1/3] net/virtio: register/unregister intr handler on start/stop Luca Boccassi
@ 2018-09-19 12:57   ` Luca Boccassi
  2018-09-19 15:47     ` Chas Williams
  2018-09-27  8:39     ` Luca Boccassi
  2018-09-19 12:57   ` [dpdk-dev] [PATCH v2 3/3] eal/linux: handle uio read failure in interrupt handler Luca Boccassi
                     ` (2 subsequent siblings)
  3 siblings, 2 replies; 30+ messages in thread
From: Luca Boccassi @ 2018-09-19 12:57 UTC (permalink / raw)
  To: dev
  Cc: maxime.coquelin, tiwei.bie, yongwang, 3chas3, bruce.richardson,
	jianfeng.tan, anatoly.burakov, llouis, brussell

The vmxnet3 driver can't call back into dev_close(), and possibly
dev_stop(), in dev_uninit().  When dev_uninit() is called, anything
that those routines would want to clean up has already been released.
Further, for complete cleanup, it is necessary to release any of the
queue resources during dev_close().
This allows a vmxnet3 device to be hot-unplugged without leaking
queues.

Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode driver implementation")
Cc: stable@dpdk.org

Signed-off-by: Brian Russell <brussell@brocade.com>
Signed-off-by: Luca Boccassi <bluca@debian.org>
---
v2: add back extra close() call in uninit() for buggy applications as
    requested by the reviewers, and add debug log noting the issue

 drivers/net/vmxnet3/vmxnet3_ethdev.c | 35 +++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c b/drivers/net/vmxnet3/vmxnet3_ethdev.c
index f1596ab19d..98e5d01890 100644
--- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
+++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
@@ -354,8 +354,10 @@ eth_vmxnet3_dev_uninit(struct rte_eth_dev *eth_dev)
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return 0;
 
-	if (hw->adapter_stopped == 0)
+	if (hw->adapter_stopped == 0) {
+		PMD_INIT_LOG(DEBUG, "Device has not been closed.");
 		vmxnet3_dev_close(eth_dev);
+	}
 
 	eth_dev->dev_ops = NULL;
 	eth_dev->rx_pkt_burst = NULL;
@@ -802,7 +804,7 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
 	PMD_INIT_FUNC_TRACE();
 
 	if (hw->adapter_stopped == 1) {
-		PMD_INIT_LOG(DEBUG, "Device already closed.");
+		PMD_INIT_LOG(DEBUG, "Device already stopped.");
 		return;
 	}
 
@@ -826,7 +828,6 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
 	/* reset the device */
 	VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_CMD, VMXNET3_CMD_RESET_DEV);
 	PMD_INIT_LOG(DEBUG, "Device reset.");
-	hw->adapter_stopped = 0;
 
 	vmxnet3_dev_clear_queues(dev);
 
@@ -836,6 +837,30 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
 	link.link_speed = ETH_SPEED_NUM_10G;
 	link.link_autoneg = ETH_LINK_FIXED;
 	rte_eth_linkstatus_set(dev, &link);
+
+	hw->adapter_stopped = 1;
+}
+
+static void
+vmxnet3_free_queues(struct rte_eth_dev *dev)
+{
+	int i;
+
+	PMD_INIT_FUNC_TRACE();
+
+	for (i = 0; i < dev->data->nb_rx_queues; i++) {
+		void *rxq = dev->data->rx_queues[i];
+
+		vmxnet3_dev_rx_queue_release(rxq);
+	}
+	dev->data->nb_rx_queues = 0;
+
+	for (i = 0; i < dev->data->nb_tx_queues; i++) {
+		void *txq = dev->data->tx_queues[i];
+
+		vmxnet3_dev_tx_queue_release(txq);
+	}
+	dev->data->nb_tx_queues = 0;
 }
 
 /*
@@ -844,12 +869,10 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
 static void
 vmxnet3_dev_close(struct rte_eth_dev *dev)
 {
-	struct vmxnet3_hw *hw = dev->data->dev_private;
-
 	PMD_INIT_FUNC_TRACE();
 
 	vmxnet3_dev_stop(dev);
-	hw->adapter_stopped = 1;
+	vmxnet3_free_queues(dev);
 }
 
 static void
-- 
2.18.0

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

* [dpdk-dev] [PATCH v2 3/3] eal/linux: handle uio read failure in interrupt handler
  2018-09-19 12:57 ` [dpdk-dev] [PATCH v2 1/3] net/virtio: register/unregister intr handler on start/stop Luca Boccassi
  2018-09-19 12:57   ` [dpdk-dev] [PATCH v2 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug Luca Boccassi
@ 2018-09-19 12:57   ` Luca Boccassi
  2018-10-11 10:32     ` Thomas Monjalon
  2018-09-27  8:40   ` [dpdk-dev] [PATCH v2 1/3] net/virtio: register/unregister intr handler on start/stop Luca Boccassi
  2018-09-27 11:14   ` Maxime Coquelin
  3 siblings, 1 reply; 30+ messages in thread
From: Luca Boccassi @ 2018-09-19 12:57 UTC (permalink / raw)
  To: dev
  Cc: maxime.coquelin, tiwei.bie, yongwang, 3chas3, bruce.richardson,
	jianfeng.tan, anatoly.burakov, llouis, brussell

If a device is unplugged while an interrupt is pending, the
read call to the uio device to remove it from the poll wait list
can fail resulting in it being continually polled forever. This
change checks for the read failing and if so, unregisters the device
as an interrupt source and causes the wait list to be rebuilt.

This race has been reported and observed in production.

Fixes: 0a45657a6794 ("pci: rework interrupt handling")
Cc: stable@dpdk.org

Signed-off-by: Brian Russell <brussell@brocade.com>
Signed-off-by: Luca Boccassi <bluca@debian.org>
---
 lib/librte_eal/linuxapp/eal/eal_interrupts.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
index 4076c6d6ca..34584db883 100644
--- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
+++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
@@ -627,7 +627,7 @@ eal_intr_process_interrupts(struct epoll_event *events, int nfds)
 	bool call = false;
 	int n, bytes_read;
 	struct rte_intr_source *src;
-	struct rte_intr_callback *cb;
+	struct rte_intr_callback *cb, *next;
 	union rte_intr_read_buffer buf;
 	struct rte_intr_callback active_cb;
 
@@ -701,6 +701,23 @@ eal_intr_process_interrupts(struct epoll_event *events, int nfds)
 					"descriptor %d: %s\n",
 					events[n].data.fd,
 					strerror(errno));
+				/*
+				 * The device is unplugged or buggy, remove
+				 * it as an interrupt source and return to
+				 * force the wait list to be rebuilt.
+				 */
+				rte_spinlock_lock(&intr_lock);
+				TAILQ_REMOVE(&intr_sources, src, next);
+				rte_spinlock_unlock(&intr_lock);
+
+				for (cb = TAILQ_FIRST(&src->callbacks); cb;
+							cb = next) {
+					next = TAILQ_NEXT(cb, next);
+					TAILQ_REMOVE(&src->callbacks, cb, next);
+					free(cb);
+				}
+				free(src);
+				return -1;
 			} else if (bytes_read == 0)
 				RTE_LOG(ERR, EAL, "Read nothing from file "
 					"descriptor %d\n", events[n].data.fd);
-- 
2.18.0

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

* Re: [dpdk-dev] [PATCH 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug
  2018-09-18 18:48           ` Louis Luo
@ 2018-09-19 12:58             ` Luca Boccassi
  0 siblings, 0 replies; 30+ messages in thread
From: Luca Boccassi @ 2018-09-19 12:58 UTC (permalink / raw)
  To: Louis Luo, dev; +Cc: brussell

Ok, I've added it back with a debug log in v2.

On Tue, 2018-09-18 at 18:48 +0000, Louis Luo wrote:
> Hi Luca,
> 
> I'm fine with the queue free part, but just have concern about
> removing the check inside uninit function. If the application does
> follow the rules and keeping the check in uninit won't cause
> vmxnet3_dev_close() being called again (and if it won't cause trouble
> in case the application doesn't call close before calling uninit), I
> prefer keeping the check (and log some warning) in case any buggy
> application exists.
> 
> Thanks,
> Louis
> 
> On 9/18/18, 11:29 AM, "Luca Boccassi" <bluca@debian.org> wrote:
> 
>     On Tue, 2018-09-18 at 18:14 +0000, Louis Luo wrote:
>     > Hi Luca,
>     > 
>     > Thanks for pointing to the document! This "basic requirements"
> seems
>     > to lay the burden on application developers to correctly follow
> the
>     > hot-plug framework's rules, but there seems no mechanism to
> enforce
>     > this procedure (correct me if I'm wrong). What if a buggy
> application
>     > doesn't call stop/close before detaching? 
>     
>     Well DPDK in general does not have the simplest API to use,
> there's no
>     denying that. That's because what it does is very much more
> complicated
>     than the average library.
>     So if an application ignores the development guides and does not
> follow
>     the recommendation bad things will happen :-) But it will happen
>     regardless of the PMD used. That's already the case.
>     
>     > In addition, in your commit description, you said " The vmxnet3
>     > driver can't call back into dev_close(), and possibly
> dev_stop(), in
>     > dev_uninit()." But actually in vmxnet3_dev_close(), we set hw-
>     > >adapter_stopped = 1, and in eth_vmxnet3_dev_uninit() we call
> into
>     > vmxnet3_dev_close() ONLY when hw->adapter_stopped == 0. So if
> the
>     > application does meet the hot-plug framework requirement and
> calls
>     > dev_close before calling uninit, eth_vmxnet3_dev_uninit()
> should not
>     > call into vmxnet3_dev_close() again, right? If so, why bother
>     > removing this check? 
>     > 
>     > Or let me ask this way. If a buggy application DOES NOT call
>     > dev_close before calling eth_vmxnet3_dev_uninit(), would
> calling
>     > vmxnet3_dev_close() inside eth_vmxnet3_dev_uninit() cause any
>     > trouble? And if an application DOES call dev_close before
> calling
>     > eth_vmxnet3_dev_uninit(), have you ever seen
> vmxnet3_dev_close()
>     > being called again and trigger crashes like double-free or
> something
>     > else? If yes, then we need to investigate.
>     > 
>     > Thanks
>     > Louis
>     
>     The problem is that there are many PMDs, so the guidelines have
> to be
>     followed - so stop and close have to be called anyway before
> detach.
>     Otherwise running with other PMDs will break the application. So
> not
>     calling close is not really an option for an application.
>     
>     But the main issue here is the leaking of the queues, which this
> patch
>     fixes. By re-arranging the stop/close/uninit like this, queues
> can be
>     correctly freed and thus we can hotplug/unplug devices without
> leaking
>     massive amounts of memory.
>     
>     My colleague Brian, who wrote the patch and I CC'ed, might have
> more
>     information if you need it.
>     
>     > On 9/18/18, 6:15 AM, "Luca Boccassi" <bluca@debian.org> wrote:
>     > 
>     >     Hi,
>     >     
>     >     The application must already stop and close before
> detaching
>     > (which
>     >     will call uninit). Quoting from the documentation:
>     >     
>     >     "*  Before detaching, they must be stopped and closed.
>     >     
>     >         DPDK applications must call "rte_eth_dev_stop()" and
>     >         "rte_eth_dev_close()" APIs before detaching ports.
> These
>     > functions will
>     >         start finalization sequence of the PMDs."
>     >     
>     >     https://na01.safelinks.protection.outlook.com/?url=http%3A%
> 2F%2Fd
>     >
> oc.dpdk.org%2Fguides%2Fprog_guide%2Fport_hotplug_framework.html&amp;d
>     >
> ata=02%7C01%7Cllouis%40vmware.com%7C3d87a5482cd046679d7608d61d68bf9a%
>     >
> 7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636728733078700911&amp;s
>     >
> data=qBx7zyOJhTmVH0c2ZEBohmIHL6PkYwB6XPaw2epgS0E%3D&amp;reserved=0
>     >     
>     >     On Mon, 2018-09-17 at 19:06 +0000, Louis Luo wrote:
>     >     > Hi Luca,
>     >     > 
>     >     > When eth_vmxnet3_dev_uninit() is called, is it guaranteed
> that
>     >     > vmxnet3_dev_close/ vmxnet3_dev_stop must have been
> called? I'm
>     > not
>     >     > familiar with the hot-plug procedure, so just wonder if
> there
>     > is any
>     >     > chance that eth_vmxnet3_dev_uninit() is called without
> calling
>     >     > vmxnet3_dev_close/ vmxnet3_dev_stop.
>     >     > 
>     >     > Thanks,
>     >     > Louis
>     >     > 
>     >     > On 8/16/18, 6:51 AM, "dev on behalf of Luca Boccassi"
> <dev-boun
>     > ces@d
>     >     > pdk.org on behalf of bluca@debian.org> wrote:
>     >     > 
>     >     >     The vmxnet3 driver can't call back into dev_close(),
> and
>     > possibly
>     >     >     dev_stop(), in dev_uninit().  When dev_uninit() is
> called,
>     >     > anything
>     >     >     that those routines would want to clean up has
> already been
>     >     > released.
>     >     >     Further, for complete cleanup, it is necessary to
> release
>     > any of
>     >     > the
>     >     >     queue resources during dev_close().
>     >     >     This allows a vmxnet3 device to be hot-unplugged
> without
>     > leaking
>     >     >     queues.
>     >     >     
>     >     >     Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3
> poll mode
>     >     > driver implementation")
>     >     >     Cc: stable@dpdk.org
>     >     >     
>     >     >     Signed-off-by: Brian Russell <brussell@brocade.com>
>     >     >     Signed-off-by: Luca Boccassi <bluca@debian.org>
>     >     >     ---
>     >     >      drivers/net/vmxnet3/vmxnet3_ethdev.c | 36
>     > ++++++++++++++++++++
>     >     > --------
>     >     >      1 file changed, 26 insertions(+), 10 deletions(-)
>     >     >     
>     >     >     diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c
>     >     > b/drivers/net/vmxnet3/vmxnet3_ethdev.c
>     >     >     index 2613cd1358..b5d4be5e24 100644
>     >     >     --- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
>     >     >     +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
>     >     >     @@ -348,16 +348,11 @@ eth_vmxnet3_dev_init(struct
>     > rte_eth_dev
>     >     > *eth_dev)
>     >     >      static int
>     >     >      eth_vmxnet3_dev_uninit(struct rte_eth_dev *eth_dev)
>     >     >      {
>     >     >     -	struct vmxnet3_hw *hw = eth_dev->data-
>     > >dev_private;
>     >     >     -
>     >     >      	PMD_INIT_FUNC_TRACE();
>     >     >      
>     >     >      	if (rte_eal_process_type() !=
> RTE_PROC_PRIMARY)
>     >     >      		return 0;
>     >     >      
>     >     >     -	if (hw->adapter_stopped == 0)
>     >     >     -		vmxnet3_dev_close(eth_dev);
>     >     >     -
>     >     >      	eth_dev->dev_ops = NULL;
>     >     >      	eth_dev->rx_pkt_burst = NULL;
>     >     >      	eth_dev->tx_pkt_burst = NULL;
>     >     >     @@ -803,7 +798,7 @@ vmxnet3_dev_stop(struct
> rte_eth_dev
>     > *dev)
>     >     >      	PMD_INIT_FUNC_TRACE();
>     >     >      
>     >     >      	if (hw->adapter_stopped == 1) {
>     >     >     -		PMD_INIT_LOG(DEBUG, "Device already
>     > closed.");
>     >     >     +		PMD_INIT_LOG(DEBUG, "Device already
>     > stopped.");
>     >     >      		return;
>     >     >      	}
>     >     >      
>     >     >     @@ -827,7 +822,6 @@ vmxnet3_dev_stop(struct
> rte_eth_dev
>     > *dev)
>     >     >      	/* reset the device */
>     >     >      	VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_CMD,
>     >     > VMXNET3_CMD_RESET_DEV);
>     >     >      	PMD_INIT_LOG(DEBUG, "Device reset.");
>     >     >     -	hw->adapter_stopped = 0;
>     >     >      
>     >     >      	vmxnet3_dev_clear_queues(dev);
>     >     >      
>     >     >     @@ -837,6 +831,30 @@ vmxnet3_dev_stop(struct
> rte_eth_dev
>     > *dev)
>     >     >      	link.link_speed = ETH_SPEED_NUM_10G;
>     >     >      	link.link_autoneg = ETH_LINK_FIXED;
>     >     >      	rte_eth_linkstatus_set(dev, &link);
>     >     >     +
>     >     >     +	hw->adapter_stopped = 1;
>     >     >     +}
>     >     >     +
>     >     >     +static void
>     >     >     +vmxnet3_free_queues(struct rte_eth_dev *dev)
>     >     >     +{
>     >     >     +	int i;
>     >     >     +
>     >     >     +	PMD_INIT_FUNC_TRACE();
>     >     >     +
>     >     >     +	for (i = 0; i < dev->data->nb_rx_queues;
> i++) {
>     >     >     +		void *rxq = dev->data->rx_queues[i];
>     >     >     +
>     >     >     +		vmxnet3_dev_rx_queue_release(rxq);
>     >     >     +	}
>     >     >     +	dev->data->nb_rx_queues = 0;
>     >     >     +
>     >     >     +	for (i = 0; i < dev->data->nb_tx_queues;
> i++) {
>     >     >     +		void *txq = dev->data->tx_queues[i];
>     >     >     +
>     >     >     +		vmxnet3_dev_tx_queue_release(txq);
>     >     >     +	}
>     >     >     +	dev->data->nb_tx_queues = 0;
>     >     >      }
>     >     >      
>     >     >      /*
>     >     >     @@ -845,12 +863,10 @@ vmxnet3_dev_stop(struct
> rte_eth_dev
>     > *dev)
>     >     >      static void
>     >     >      vmxnet3_dev_close(struct rte_eth_dev *dev)
>     >     >      {
>     >     >     -	struct vmxnet3_hw *hw = dev->data-
> >dev_private;
>     >     >     -
>     >     >      	PMD_INIT_FUNC_TRACE();
>     >     >      
>     >     >      	vmxnet3_dev_stop(dev);
>     >     >     -	hw->adapter_stopped = 1;
>     >     >     +	vmxnet3_free_queues(dev);
>     >     >      }
>     >     >      
>     >     >      static void
>     >     >     -- 
>     >     >     2.18.0
>     >     >     
>     >     >     
>     >     > 
>     >     
>     >     -- 
>     >     Kind regards,
>     >     Luca Boccassi
>     > 
>     
>     -- 
>     Kind regards,
>     Luca Boccassi
> 

-- 
Kind regards,
Luca Boccassi

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

* Re: [dpdk-dev] [PATCH v2 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug
  2018-09-19 12:57   ` [dpdk-dev] [PATCH v2 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug Luca Boccassi
@ 2018-09-19 15:47     ` Chas Williams
  2018-09-19 16:08       ` Luca Boccassi
  2018-10-27 15:09       ` Thomas Monjalon
  2018-09-27  8:39     ` Luca Boccassi
  1 sibling, 2 replies; 30+ messages in thread
From: Chas Williams @ 2018-09-19 15:47 UTC (permalink / raw)
  To: bluca
  Cc: dev, Maxime Coquelin, tiwei.bie, yongwang, Bruce Richardson,
	Jianfeng Tan, Burakov, Anatoly, llouis, brussell

On Wed, Sep 19, 2018 at 8:58 AM Luca Boccassi <bluca@debian.org> wrote:
>
> The vmxnet3 driver can't call back into dev_close(), and possibly
> dev_stop(), in dev_uninit().  When dev_uninit() is called, anything
> that those routines would want to clean up has already been released.
> Further, for complete cleanup, it is necessary to release any of the
> queue resources during dev_close().
> This allows a vmxnet3 device to be hot-unplugged without leaking
> queues.
>
> Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode driver implementation")
> Cc: stable@dpdk.org
>
> Signed-off-by: Brian Russell <brussell@brocade.com>
> Signed-off-by: Luca Boccassi <bluca@debian.org>
> ---
> v2: add back extra close() call in uninit() for buggy applications as
>     requested by the reviewers, and add debug log noting the issue
>
>  drivers/net/vmxnet3/vmxnet3_ethdev.c | 35 +++++++++++++++++++++++-----
>  1 file changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> index f1596ab19d..98e5d01890 100644
> --- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
> +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> @@ -354,8 +354,10 @@ eth_vmxnet3_dev_uninit(struct rte_eth_dev *eth_dev)
>         if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>                 return 0;

This should probably be EPERM as well.  Out of scope though.

>
> -       if (hw->adapter_stopped == 0)
> +       if (hw->adapter_stopped == 0) {
> +               PMD_INIT_LOG(DEBUG, "Device has not been closed.");
>                 vmxnet3_dev_close(eth_dev);

This just seems wrong.  You have called uninit() will the driver is
still busy.  Instead of "fixing" the state of the driver, return EBUSY
here.

> +       }
>
>         eth_dev->dev_ops = NULL;
>         eth_dev->rx_pkt_burst = NULL;
> @@ -802,7 +804,7 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
>         PMD_INIT_FUNC_TRACE();
>
>         if (hw->adapter_stopped == 1) {
> -               PMD_INIT_LOG(DEBUG, "Device already closed.");
> +               PMD_INIT_LOG(DEBUG, "Device already stopped.");
>                 return;
>         }
>
> @@ -826,7 +828,6 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
>         /* reset the device */
>         VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_CMD, VMXNET3_CMD_RESET_DEV);
>         PMD_INIT_LOG(DEBUG, "Device reset.");
> -       hw->adapter_stopped = 0;
>
>         vmxnet3_dev_clear_queues(dev);
>
> @@ -836,6 +837,30 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
>         link.link_speed = ETH_SPEED_NUM_10G;
>         link.link_autoneg = ETH_LINK_FIXED;
>         rte_eth_linkstatus_set(dev, &link);
> +
> +       hw->adapter_stopped = 1;
> +}
> +
> +static void
> +vmxnet3_free_queues(struct rte_eth_dev *dev)
> +{
> +       int i;
> +
> +       PMD_INIT_FUNC_TRACE();
> +
> +       for (i = 0; i < dev->data->nb_rx_queues; i++) {
> +               void *rxq = dev->data->rx_queues[i];
> +
> +               vmxnet3_dev_rx_queue_release(rxq);
> +       }
> +       dev->data->nb_rx_queues = 0;
> +
> +       for (i = 0; i < dev->data->nb_tx_queues; i++) {
> +               void *txq = dev->data->tx_queues[i];
> +
> +               vmxnet3_dev_tx_queue_release(txq);
> +       }
> +       dev->data->nb_tx_queues = 0;
>  }
>
>  /*
> @@ -844,12 +869,10 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
>  static void
>  vmxnet3_dev_close(struct rte_eth_dev *dev)
>  {
> -       struct vmxnet3_hw *hw = dev->data->dev_private;
> -
>         PMD_INIT_FUNC_TRACE();
>
>         vmxnet3_dev_stop(dev);
> -       hw->adapter_stopped = 1;
> +       vmxnet3_free_queues(dev);
>  }
>
>  static void
> --
> 2.18.0
>

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

* Re: [dpdk-dev] [PATCH v2 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug
  2018-09-19 15:47     ` Chas Williams
@ 2018-09-19 16:08       ` Luca Boccassi
  2018-10-27 15:09       ` Thomas Monjalon
  1 sibling, 0 replies; 30+ messages in thread
From: Luca Boccassi @ 2018-09-19 16:08 UTC (permalink / raw)
  To: Chas Williams
  Cc: dev, Maxime Coquelin, tiwei.bie, yongwang, Bruce Richardson,
	Jianfeng Tan, Burakov, Anatoly, llouis, brussell

On Wed, 2018-09-19 at 11:47 -0400, Chas Williams wrote:
> On Wed, Sep 19, 2018 at 8:58 AM Luca Boccassi <bluca@debian.org>
> wrote:
> > 
> > The vmxnet3 driver can't call back into dev_close(), and possibly
> > dev_stop(), in dev_uninit().  When dev_uninit() is called, anything
> > that those routines would want to clean up has already been
> > released.
> > Further, for complete cleanup, it is necessary to release any of
> > the
> > queue resources during dev_close().
> > This allows a vmxnet3 device to be hot-unplugged without leaking
> > queues.
> > 
> > Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode driver
> > implementation")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Brian Russell <brussell@brocade.com>
> > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > ---
> > v2: add back extra close() call in uninit() for buggy applications
> > as
> >     requested by the reviewers, and add debug log noting the issue
> > 
> >  drivers/net/vmxnet3/vmxnet3_ethdev.c | 35 +++++++++++++++++++++++-
> > ----
> >  1 file changed, 29 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > index f1596ab19d..98e5d01890 100644
> > --- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > @@ -354,8 +354,10 @@ eth_vmxnet3_dev_uninit(struct rte_eth_dev
> > *eth_dev)
> >         if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> >                 return 0;
> 
> This should probably be EPERM as well.  Out of scope though.
> 
> > 
> > -       if (hw->adapter_stopped == 0)
> > +       if (hw->adapter_stopped == 0) {
> > +               PMD_INIT_LOG(DEBUG, "Device has not been closed.");
> >                 vmxnet3_dev_close(eth_dev);
> 
> This just seems wrong.  You have called uninit() will the driver is
> still busy.  Instead of "fixing" the state of the driver, return
> EBUSY
> here.

At this point that's out of scope too - it doesn't affect the ability
to hotplug or not.
So please send another patch and discuss it further with Louis, who
requested to drop that change.

> > +       }
> > 
> >         eth_dev->dev_ops = NULL;
> >         eth_dev->rx_pkt_burst = NULL;
> > @@ -802,7 +804,7 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
> >         PMD_INIT_FUNC_TRACE();
> > 
> >         if (hw->adapter_stopped == 1) {
> > -               PMD_INIT_LOG(DEBUG, "Device already closed.");
> > +               PMD_INIT_LOG(DEBUG, "Device already stopped.");
> >                 return;
> >         }
> > 
> > @@ -826,7 +828,6 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
> >         /* reset the device */
> >         VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_CMD,
> > VMXNET3_CMD_RESET_DEV);
> >         PMD_INIT_LOG(DEBUG, "Device reset.");
> > -       hw->adapter_stopped = 0;
> > 
> >         vmxnet3_dev_clear_queues(dev);
> > 
> > @@ -836,6 +837,30 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
> >         link.link_speed = ETH_SPEED_NUM_10G;
> >         link.link_autoneg = ETH_LINK_FIXED;
> >         rte_eth_linkstatus_set(dev, &link);
> > +
> > +       hw->adapter_stopped = 1;
> > +}
> > +
> > +static void
> > +vmxnet3_free_queues(struct rte_eth_dev *dev)
> > +{
> > +       int i;
> > +
> > +       PMD_INIT_FUNC_TRACE();
> > +
> > +       for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > +               void *rxq = dev->data->rx_queues[i];
> > +
> > +               vmxnet3_dev_rx_queue_release(rxq);
> > +       }
> > +       dev->data->nb_rx_queues = 0;
> > +
> > +       for (i = 0; i < dev->data->nb_tx_queues; i++) {
> > +               void *txq = dev->data->tx_queues[i];
> > +
> > +               vmxnet3_dev_tx_queue_release(txq);
> > +       }
> > +       dev->data->nb_tx_queues = 0;
> >  }
> > 
> >  /*
> > @@ -844,12 +869,10 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
> >  static void
> >  vmxnet3_dev_close(struct rte_eth_dev *dev)
> >  {
> > -       struct vmxnet3_hw *hw = dev->data->dev_private;
> > -
> >         PMD_INIT_FUNC_TRACE();
> > 
> >         vmxnet3_dev_stop(dev);
> > -       hw->adapter_stopped = 1;
> > +       vmxnet3_free_queues(dev);
> >  }
> > 
> >  static void
> > --
> > 2.18.0
> > 

-- 
Kind regards,
Luca Boccassi

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

* Re: [dpdk-dev] [PATCH v2 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug
  2018-09-19 12:57   ` [dpdk-dev] [PATCH v2 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug Luca Boccassi
  2018-09-19 15:47     ` Chas Williams
@ 2018-09-27  8:39     ` Luca Boccassi
  1 sibling, 0 replies; 30+ messages in thread
From: Luca Boccassi @ 2018-09-27  8:39 UTC (permalink / raw)
  To: dev
  Cc: maxime.coquelin, tiwei.bie, yongwang, 3chas3, bruce.richardson,
	jianfeng.tan, anatoly.burakov, llouis, brussell

On Wed, 2018-09-19 at 13:57 +0100, Luca Boccassi wrote:
> The vmxnet3 driver can't call back into dev_close(), and possibly
> dev_stop(), in dev_uninit().  When dev_uninit() is called, anything
> that those routines would want to clean up has already been released.
> Further, for complete cleanup, it is necessary to release any of the
> queue resources during dev_close().
> This allows a vmxnet3 device to be hot-unplugged without leaking
> queues.
> 
> Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode driver
> implementation")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Brian Russell <brussell@brocade.com>
> Signed-off-by: Luca Boccassi <bluca@debian.org>
> ---
> v2: add back extra close() call in uninit() for buggy applications as
>     requested by the reviewers, and add debug log noting the issue
> 
>  drivers/net/vmxnet3/vmxnet3_ethdev.c | 35 +++++++++++++++++++++++---
> --
>  1 file changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c
> b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> index f1596ab19d..98e5d01890 100644
> --- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
> +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> @@ -354,8 +354,10 @@ eth_vmxnet3_dev_uninit(struct rte_eth_dev
> *eth_dev)
>  	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>  		return 0;
>  
> -	if (hw->adapter_stopped == 0)
> +	if (hw->adapter_stopped == 0) {
> +		PMD_INIT_LOG(DEBUG, "Device has not been closed.");
>  		vmxnet3_dev_close(eth_dev);
> +	}
>  
>  	eth_dev->dev_ops = NULL;
>  	eth_dev->rx_pkt_burst = NULL;
> @@ -802,7 +804,7 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
>  	PMD_INIT_FUNC_TRACE();
>  
>  	if (hw->adapter_stopped == 1) {
> -		PMD_INIT_LOG(DEBUG, "Device already closed.");
> +		PMD_INIT_LOG(DEBUG, "Device already stopped.");
>  		return;
>  	}
>  
> @@ -826,7 +828,6 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
>  	/* reset the device */
>  	VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_CMD,
> VMXNET3_CMD_RESET_DEV);
>  	PMD_INIT_LOG(DEBUG, "Device reset.");
> -	hw->adapter_stopped = 0;
>  
>  	vmxnet3_dev_clear_queues(dev);
>  
> @@ -836,6 +837,30 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
>  	link.link_speed = ETH_SPEED_NUM_10G;
>  	link.link_autoneg = ETH_LINK_FIXED;
>  	rte_eth_linkstatus_set(dev, &link);
> +
> +	hw->adapter_stopped = 1;
> +}
> +
> +static void
> +vmxnet3_free_queues(struct rte_eth_dev *dev)
> +{
> +	int i;
> +
> +	PMD_INIT_FUNC_TRACE();
> +
> +	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> +		void *rxq = dev->data->rx_queues[i];
> +
> +		vmxnet3_dev_rx_queue_release(rxq);
> +	}
> +	dev->data->nb_rx_queues = 0;
> +
> +	for (i = 0; i < dev->data->nb_tx_queues; i++) {
> +		void *txq = dev->data->tx_queues[i];
> +
> +		vmxnet3_dev_tx_queue_release(txq);
> +	}
> +	dev->data->nb_tx_queues = 0;
>  }
>  
>  /*
> @@ -844,12 +869,10 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
>  static void
>  vmxnet3_dev_close(struct rte_eth_dev *dev)
>  {
> -	struct vmxnet3_hw *hw = dev->data->dev_private;
> -
>  	PMD_INIT_FUNC_TRACE();
>  
>  	vmxnet3_dev_stop(dev);
> -	hw->adapter_stopped = 1;
> +	vmxnet3_free_queues(dev);
>  }
>  
>  static void

Hi Louis,

Are you happy with the diff as in v2 now for vmxnet3? Thanks

-- 
Kind regards,
Luca Boccassi

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

* Re: [dpdk-dev] [PATCH v2 1/3] net/virtio: register/unregister intr handler on start/stop
  2018-09-19 12:57 ` [dpdk-dev] [PATCH v2 1/3] net/virtio: register/unregister intr handler on start/stop Luca Boccassi
  2018-09-19 12:57   ` [dpdk-dev] [PATCH v2 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug Luca Boccassi
  2018-09-19 12:57   ` [dpdk-dev] [PATCH v2 3/3] eal/linux: handle uio read failure in interrupt handler Luca Boccassi
@ 2018-09-27  8:40   ` Luca Boccassi
  2018-09-27 10:51     ` Maxime Coquelin
  2018-09-27 11:14   ` Maxime Coquelin
  3 siblings, 1 reply; 30+ messages in thread
From: Luca Boccassi @ 2018-09-27  8:40 UTC (permalink / raw)
  To: dev
  Cc: maxime.coquelin, tiwei.bie, yongwang, 3chas3, bruce.richardson,
	jianfeng.tan, anatoly.burakov, llouis, brussell

On Wed, 2018-09-19 at 13:57 +0100, Luca Boccassi wrote:
> Register and unregister the virtio interrupt handler when the device
> is
> started and stopped. This allows a virtio device to be hotplugged or
> unplugged.
> 
> Fixes: c1f86306a026 ("virtio: add new driver")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Brian Russell <brussell@brocade.com>
> Signed-off-by: Luca Boccassi <bluca@debian.org>
> ---
>  drivers/net/virtio/virtio_ethdev.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c
> b/drivers/net/virtio/virtio_ethdev.c
> index b81df0a99f..adc6a30a32 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1679,11 +1679,6 @@ eth_virtio_dev_init(struct rte_eth_dev
> *eth_dev)
>  	if (ret < 0)
>  		goto out;
>  
> -	/* Setup interrupt callback  */
> -	if (eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
> -		rte_intr_callback_register(eth_dev->intr_handle,
> -			virtio_interrupt_handler, eth_dev);
> -
>  	return 0;
>  
>  out:
> @@ -1709,11 +1704,6 @@ eth_virtio_dev_uninit(struct rte_eth_dev
> *eth_dev)
>  	rte_free(eth_dev->data->mac_addrs);
>  	eth_dev->data->mac_addrs = NULL;
>  
> -	/* reset interrupt callback  */
> -	if (eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
> -		rte_intr_callback_unregister(eth_dev->intr_handle,
> -						virtio_interrupt_han
> dler,
> -						eth_dev);
>  	if (eth_dev->device)
>  		rte_pci_unmap_device(RTE_ETH_DEV_TO_PCI(eth_dev));
>  
> @@ -1972,6 +1962,12 @@ virtio_dev_start(struct rte_eth_dev *dev)
>  	    dev->data->dev_conf.intr_conf.rxq) {
>  		virtio_intr_disable(dev);
>  
> +		/* Setup interrupt callback  */
> +		if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
> +			rte_intr_callback_register(dev->intr_handle,
> +						   virtio_interrupt_
> handler,
> +						   dev);
> +
>  		if (virtio_intr_enable(dev) < 0) {
>  			PMD_DRV_LOG(ERR, "interrupt enable failed");
>  			return -EIO;
> @@ -2081,9 +2077,17 @@ virtio_dev_stop(struct rte_eth_dev *dev)
>  	PMD_INIT_LOG(DEBUG, "stop");
>  
>  	rte_spinlock_lock(&hw->state_lock);
> -	if (intr_conf->lsc || intr_conf->rxq)
> +	if (intr_conf->lsc || intr_conf->rxq) {
>  		virtio_intr_disable(dev);
>  
> +		/* Reset interrupt callback  */
> +		if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC) {
> +			rte_intr_callback_unregister(dev-
> >intr_handle,
> +						     virtio_interrup
> t_handler,
> +						     dev);
> +		}
> +	}
> +
>  	hw->started = 0;
>  	memset(&link, 0, sizeof(link));
>  	rte_eth_linkstatus_set(dev, &link);

Hi, any chance the virtio and eal patches in this series could get a
review? Thanks!

-- 
Kind regards,
Luca Boccassi

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

* Re: [dpdk-dev] [PATCH v2 1/3] net/virtio: register/unregister intr handler on start/stop
  2018-09-27  8:40   ` [dpdk-dev] [PATCH v2 1/3] net/virtio: register/unregister intr handler on start/stop Luca Boccassi
@ 2018-09-27 10:51     ` Maxime Coquelin
  0 siblings, 0 replies; 30+ messages in thread
From: Maxime Coquelin @ 2018-09-27 10:51 UTC (permalink / raw)
  To: Luca Boccassi, dev
  Cc: tiwei.bie, yongwang, 3chas3, bruce.richardson, jianfeng.tan,
	anatoly.burakov, llouis, brussell



On 09/27/2018 10:40 AM, Luca Boccassi wrote:
> On Wed, 2018-09-19 at 13:57 +0100, Luca Boccassi wrote:
>> Register and unregister the virtio interrupt handler when the device
>> is
>> started and stopped. This allows a virtio device to be hotplugged or
>> unplugged.
>>
>> Fixes: c1f86306a026 ("virtio: add new driver")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Brian Russell <brussell@brocade.com>
>> Signed-off-by: Luca Boccassi <bluca@debian.org>
>> ---
>>   drivers/net/virtio/virtio_ethdev.c | 26 +++++++++++++++-----------
>>   1 file changed, 15 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtio_ethdev.c
>> b/drivers/net/virtio/virtio_ethdev.c
>> index b81df0a99f..adc6a30a32 100644
>> --- a/drivers/net/virtio/virtio_ethdev.c
>> +++ b/drivers/net/virtio/virtio_ethdev.c
>> @@ -1679,11 +1679,6 @@ eth_virtio_dev_init(struct rte_eth_dev
>> *eth_dev)
>>   	if (ret < 0)
>>   		goto out;
>>   
>> -	/* Setup interrupt callback  */
>> -	if (eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
>> -		rte_intr_callback_register(eth_dev->intr_handle,
>> -			virtio_interrupt_handler, eth_dev);
>> -
>>   	return 0;
>>   
>>   out:
>> @@ -1709,11 +1704,6 @@ eth_virtio_dev_uninit(struct rte_eth_dev
>> *eth_dev)
>>   	rte_free(eth_dev->data->mac_addrs);
>>   	eth_dev->data->mac_addrs = NULL;
>>   
>> -	/* reset interrupt callback  */
>> -	if (eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
>> -		rte_intr_callback_unregister(eth_dev->intr_handle,
>> -						virtio_interrupt_han
>> dler,
>> -						eth_dev);
>>   	if (eth_dev->device)
>>   		rte_pci_unmap_device(RTE_ETH_DEV_TO_PCI(eth_dev));
>>   
>> @@ -1972,6 +1962,12 @@ virtio_dev_start(struct rte_eth_dev *dev)
>>   	    dev->data->dev_conf.intr_conf.rxq) {
>>   		virtio_intr_disable(dev);
>>   
>> +		/* Setup interrupt callback  */
>> +		if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
>> +			rte_intr_callback_register(dev->intr_handle,
>> +						   virtio_interrupt_
>> handler,
>> +						   dev);
>> +
>>   		if (virtio_intr_enable(dev) < 0) {
>>   			PMD_DRV_LOG(ERR, "interrupt enable failed");
>>   			return -EIO;
>> @@ -2081,9 +2077,17 @@ virtio_dev_stop(struct rte_eth_dev *dev)
>>   	PMD_INIT_LOG(DEBUG, "stop");
>>   
>>   	rte_spinlock_lock(&hw->state_lock);
>> -	if (intr_conf->lsc || intr_conf->rxq)
>> +	if (intr_conf->lsc || intr_conf->rxq) {
>>   		virtio_intr_disable(dev);
>>   
>> +		/* Reset interrupt callback  */
>> +		if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC) {
>> +			rte_intr_callback_unregister(dev-
>>> intr_handle,
>> +						     virtio_interrup
>> t_handler,
>> +						     dev);
>> +		}
>> +	}
>> +
>>   	hw->started = 0;
>>   	memset(&link, 0, sizeof(link));
>>   	rte_eth_linkstatus_set(dev, &link);
> 
> Hi, any chance the virtio and eal patches in this series could get a
> review? Thanks!
> 

Hi,

Sorry, I missed it because not delegated to me in patchwork.
Looking at it now!

Regards,
Maxime

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

* Re: [dpdk-dev] [PATCH v2 1/3] net/virtio: register/unregister intr handler on start/stop
  2018-09-19 12:57 ` [dpdk-dev] [PATCH v2 1/3] net/virtio: register/unregister intr handler on start/stop Luca Boccassi
                     ` (2 preceding siblings ...)
  2018-09-27  8:40   ` [dpdk-dev] [PATCH v2 1/3] net/virtio: register/unregister intr handler on start/stop Luca Boccassi
@ 2018-09-27 11:14   ` Maxime Coquelin
  3 siblings, 0 replies; 30+ messages in thread
From: Maxime Coquelin @ 2018-09-27 11:14 UTC (permalink / raw)
  To: Luca Boccassi, dev
  Cc: tiwei.bie, yongwang, 3chas3, bruce.richardson, jianfeng.tan,
	anatoly.burakov, llouis, brussell



On 09/19/2018 02:57 PM, Luca Boccassi wrote:
> Register and unregister the virtio interrupt handler when the device is
> started and stopped. This allows a virtio device to be hotplugged or
> unplugged.
> 
> Fixes: c1f86306a026 ("virtio: add new driver")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Brian Russell <brussell@brocade.com>
> Signed-off-by: Luca Boccassi <bluca@debian.org>
> ---
>   drivers/net/virtio/virtio_ethdev.c | 26 +++++++++++++++-----------
>   1 file changed, 15 insertions(+), 11 deletions(-)
> 

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

Thanks,
Maxime

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

* Re: [dpdk-dev] [PATCH v2 3/3] eal/linux: handle uio read failure in interrupt handler
  2018-09-19 12:57   ` [dpdk-dev] [PATCH v2 3/3] eal/linux: handle uio read failure in interrupt handler Luca Boccassi
@ 2018-10-11 10:32     ` Thomas Monjalon
  0 siblings, 0 replies; 30+ messages in thread
From: Thomas Monjalon @ 2018-10-11 10:32 UTC (permalink / raw)
  To: dev
  Cc: Luca Boccassi, maxime.coquelin, tiwei.bie, yongwang, 3chas3,
	bruce.richardson, jianfeng.tan, anatoly.burakov, llouis,
	brussell, stephen, jingjing.wu, anatoly.burakov, qi.z.zhang

Looking for someone to review this patch please


19/09/2018 14:57, Luca Boccassi:
> If a device is unplugged while an interrupt is pending, the
> read call to the uio device to remove it from the poll wait list
> can fail resulting in it being continually polled forever. This
> change checks for the read failing and if so, unregisters the device
> as an interrupt source and causes the wait list to be rebuilt.
> 
> This race has been reported and observed in production.
> 
> Fixes: 0a45657a6794 ("pci: rework interrupt handling")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Brian Russell <brussell@brocade.com>
> Signed-off-by: Luca Boccassi <bluca@debian.org>
> ---
>  lib/librte_eal/linuxapp/eal/eal_interrupts.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
> index 4076c6d6ca..34584db883 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
> @@ -627,7 +627,7 @@ eal_intr_process_interrupts(struct epoll_event *events, int nfds)
>  	bool call = false;
>  	int n, bytes_read;
>  	struct rte_intr_source *src;
> -	struct rte_intr_callback *cb;
> +	struct rte_intr_callback *cb, *next;
>  	union rte_intr_read_buffer buf;
>  	struct rte_intr_callback active_cb;
>  
> @@ -701,6 +701,23 @@ eal_intr_process_interrupts(struct epoll_event *events, int nfds)
>  					"descriptor %d: %s\n",
>  					events[n].data.fd,
>  					strerror(errno));
> +				/*
> +				 * The device is unplugged or buggy, remove
> +				 * it as an interrupt source and return to
> +				 * force the wait list to be rebuilt.
> +				 */
> +				rte_spinlock_lock(&intr_lock);
> +				TAILQ_REMOVE(&intr_sources, src, next);
> +				rte_spinlock_unlock(&intr_lock);
> +
> +				for (cb = TAILQ_FIRST(&src->callbacks); cb;
> +							cb = next) {
> +					next = TAILQ_NEXT(cb, next);
> +					TAILQ_REMOVE(&src->callbacks, cb, next);
> +					free(cb);
> +				}
> +				free(src);
> +				return -1;
>  			} else if (bytes_read == 0)
>  				RTE_LOG(ERR, EAL, "Read nothing from file "
>  					"descriptor %d\n", events[n].data.fd);
> 

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

* Re: [dpdk-dev] [PATCH v2 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug
  2018-09-19 15:47     ` Chas Williams
  2018-09-19 16:08       ` Luca Boccassi
@ 2018-10-27 15:09       ` Thomas Monjalon
  2018-10-31 17:27         ` Thomas Monjalon
  1 sibling, 1 reply; 30+ messages in thread
From: Thomas Monjalon @ 2018-10-27 15:09 UTC (permalink / raw)
  To: Chas Williams, bluca, llouis
  Cc: dev, Maxime Coquelin, tiwei.bie, yongwang, Bruce Richardson,
	Jianfeng Tan, Burakov, Anatoly, brussell

19/09/2018 17:47, Chas Williams:
> On Wed, Sep 19, 2018 at 8:58 AM Luca Boccassi <bluca@debian.org> wrote:
> >
> > The vmxnet3 driver can't call back into dev_close(), and possibly
> > dev_stop(), in dev_uninit().  When dev_uninit() is called, anything
> > that those routines would want to clean up has already been released.
> > Further, for complete cleanup, it is necessary to release any of the
> > queue resources during dev_close().
> > This allows a vmxnet3 device to be hot-unplugged without leaking
> > queues.
> >
> > Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode driver implementation")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Brian Russell <brussell@brocade.com>
> > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > ---
> > v2: add back extra close() call in uninit() for buggy applications as
> >     requested by the reviewers, and add debug log noting the issue
> >
> >  drivers/net/vmxnet3/vmxnet3_ethdev.c | 35 +++++++++++++++++++++++-----
> >  1 file changed, 29 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > index f1596ab19d..98e5d01890 100644
> > --- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > @@ -354,8 +354,10 @@ eth_vmxnet3_dev_uninit(struct rte_eth_dev *eth_dev)
> >         if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> >                 return 0;
> 
> This should probably be EPERM as well.  Out of scope though.
> 
> >
> > -       if (hw->adapter_stopped == 0)
> > +       if (hw->adapter_stopped == 0) {
> > +               PMD_INIT_LOG(DEBUG, "Device has not been closed.");
> >                 vmxnet3_dev_close(eth_dev);
> 
> This just seems wrong.  You have called uninit() will the driver is
> still busy.  Instead of "fixing" the state of the driver, return EBUSY
> here.

I agree.
If the port is not stopped, either you stop it or you return EBUSY.

Closing the device should be done outside of this check.
It is OK to close from uninit if the app did not close it.

[...]
> > +static void
> > +vmxnet3_free_queues(struct rte_eth_dev *dev)
> > +{
> > +       int i;
> > +
> > +       PMD_INIT_FUNC_TRACE();
> > +
> > +       for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > +               void *rxq = dev->data->rx_queues[i];
> > +
> > +               vmxnet3_dev_rx_queue_release(rxq);
> > +       }
> > +       dev->data->nb_rx_queues = 0;
> > +
> > +       for (i = 0; i < dev->data->nb_tx_queues; i++) {
> > +               void *txq = dev->data->tx_queues[i];
> > +
> > +               vmxnet3_dev_tx_queue_release(txq);
> > +       }
> > +       dev->data->nb_tx_queues = 0;
> >  }
> >
> >  /*
> > @@ -844,12 +869,10 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
> >  static void
> >  vmxnet3_dev_close(struct rte_eth_dev *dev)
> >  {
> > -       struct vmxnet3_hw *hw = dev->data->dev_private;
> > -
> >         PMD_INIT_FUNC_TRACE();
> >
> >         vmxnet3_dev_stop(dev);
> > -       hw->adapter_stopped = 1;
> > +       vmxnet3_free_queues(dev);
> >  }

Good clean-up on dev_close.
You probably want to go further and set RTE_ETH_DEV_CLOSE_REMOVE for allowing
a real release of the port on close.
Note: every PMDs should migrate towards this behaviour.

To make things clear (I will write a doc for -rc2):
	- "stop" should be called by the app but the PMD is allowed to force it.
	- "close" may be called by the app, and the PMD should enforce it in uninit.
		With RTE_ETH_DEV_CLOSE_REMOVE flag, it must completely release the port.
	- "remove" (implemented in PMD as uninit) is responsible of closing
		ethdev ports if not already done, and release the shared resources
		which are not specific to a port. It removes the whole EAL rte_device.

PS: for any hotplug patch or questions, feel free to Cc me.

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

* Re: [dpdk-dev] [PATCH v2 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug
  2018-10-27 15:09       ` Thomas Monjalon
@ 2018-10-31 17:27         ` Thomas Monjalon
  2018-10-31 17:46           ` Luca Boccassi
  0 siblings, 1 reply; 30+ messages in thread
From: Thomas Monjalon @ 2018-10-31 17:27 UTC (permalink / raw)
  To: bluca, llouis, yongwang
  Cc: dev, Chas Williams, Maxime Coquelin, tiwei.bie, Bruce Richardson,
	Jianfeng Tan, Burakov, Anatoly, brussell

Any update or question for this patch?
If no update, it will miss 18.11.


27/10/2018 17:09, Thomas Monjalon:
> 19/09/2018 17:47, Chas Williams:
> > On Wed, Sep 19, 2018 at 8:58 AM Luca Boccassi <bluca@debian.org> wrote:
> > >
> > > The vmxnet3 driver can't call back into dev_close(), and possibly
> > > dev_stop(), in dev_uninit().  When dev_uninit() is called, anything
> > > that those routines would want to clean up has already been released.
> > > Further, for complete cleanup, it is necessary to release any of the
> > > queue resources during dev_close().
> > > This allows a vmxnet3 device to be hot-unplugged without leaking
> > > queues.
> > >
> > > Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode driver implementation")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Brian Russell <brussell@brocade.com>
> > > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > > ---
> > > v2: add back extra close() call in uninit() for buggy applications as
> > >     requested by the reviewers, and add debug log noting the issue
> > >
> > >  drivers/net/vmxnet3/vmxnet3_ethdev.c | 35 +++++++++++++++++++++++-----
> > >  1 file changed, 29 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > > index f1596ab19d..98e5d01890 100644
> > > --- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > > +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > > @@ -354,8 +354,10 @@ eth_vmxnet3_dev_uninit(struct rte_eth_dev *eth_dev)
> > >         if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> > >                 return 0;
> > 
> > This should probably be EPERM as well.  Out of scope though.
> > 
> > >
> > > -       if (hw->adapter_stopped == 0)
> > > +       if (hw->adapter_stopped == 0) {
> > > +               PMD_INIT_LOG(DEBUG, "Device has not been closed.");
> > >                 vmxnet3_dev_close(eth_dev);
> > 
> > This just seems wrong.  You have called uninit() will the driver is
> > still busy.  Instead of "fixing" the state of the driver, return EBUSY
> > here.
> 
> I agree.
> If the port is not stopped, either you stop it or you return EBUSY.
> 
> Closing the device should be done outside of this check.
> It is OK to close from uninit if the app did not close it.
> 
> [...]
> > > +static void
> > > +vmxnet3_free_queues(struct rte_eth_dev *dev)
> > > +{
> > > +       int i;
> > > +
> > > +       PMD_INIT_FUNC_TRACE();
> > > +
> > > +       for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > > +               void *rxq = dev->data->rx_queues[i];
> > > +
> > > +               vmxnet3_dev_rx_queue_release(rxq);
> > > +       }
> > > +       dev->data->nb_rx_queues = 0;
> > > +
> > > +       for (i = 0; i < dev->data->nb_tx_queues; i++) {
> > > +               void *txq = dev->data->tx_queues[i];
> > > +
> > > +               vmxnet3_dev_tx_queue_release(txq);
> > > +       }
> > > +       dev->data->nb_tx_queues = 0;
> > >  }
> > >
> > >  /*
> > > @@ -844,12 +869,10 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
> > >  static void
> > >  vmxnet3_dev_close(struct rte_eth_dev *dev)
> > >  {
> > > -       struct vmxnet3_hw *hw = dev->data->dev_private;
> > > -
> > >         PMD_INIT_FUNC_TRACE();
> > >
> > >         vmxnet3_dev_stop(dev);
> > > -       hw->adapter_stopped = 1;
> > > +       vmxnet3_free_queues(dev);
> > >  }
> 
> Good clean-up on dev_close.
> You probably want to go further and set RTE_ETH_DEV_CLOSE_REMOVE for allowing
> a real release of the port on close.
> Note: every PMDs should migrate towards this behaviour.
> 
> To make things clear (I will write a doc for -rc2):
> 	- "stop" should be called by the app but the PMD is allowed to force it.
> 	- "close" may be called by the app, and the PMD should enforce it in uninit.
> 		With RTE_ETH_DEV_CLOSE_REMOVE flag, it must completely release the port.
> 	- "remove" (implemented in PMD as uninit) is responsible of closing
> 		ethdev ports if not already done, and release the shared resources
> 		which are not specific to a port. It removes the whole EAL rte_device.
> 
> PS: for any hotplug patch or questions, feel free to Cc me.

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

* Re: [dpdk-dev] [PATCH v2 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug
  2018-10-31 17:27         ` Thomas Monjalon
@ 2018-10-31 17:46           ` Luca Boccassi
  2018-10-31 18:02             ` Thomas Monjalon
  2018-10-31 18:54             ` Louis Luo
  0 siblings, 2 replies; 30+ messages in thread
From: Luca Boccassi @ 2018-10-31 17:46 UTC (permalink / raw)
  To: Thomas Monjalon, llouis, yongwang
  Cc: dev, Chas Williams, Maxime Coquelin, tiwei.bie, Bruce Richardson,
	Jianfeng Tan, Burakov, Anatoly, brussell

Sorry, been otherwise busy - I can do what you and Chas have asked, but
the problem is that v1 already did that and the VMWare maintainers
asked to change it back. So can I assume that the v1 way is the way to
go?

On Wed, 2018-10-31 at 18:27 +0100, Thomas Monjalon wrote:
> Any update or question for this patch?
> If no update, it will miss 18.11.
> 
> 
> 27/10/2018 17:09, Thomas Monjalon:
> > 19/09/2018 17:47, Chas Williams:
> > > On Wed, Sep 19, 2018 at 8:58 AM Luca Boccassi <bluca@debian.org>
> > > wrote:
> > > > 
> > > > The vmxnet3 driver can't call back into dev_close(), and
> > > > possibly
> > > > dev_stop(), in dev_uninit().  When dev_uninit() is called,
> > > > anything
> > > > that those routines would want to clean up has already been
> > > > released.
> > > > Further, for complete cleanup, it is necessary to release any
> > > > of the
> > > > queue resources during dev_close().
> > > > This allows a vmxnet3 device to be hot-unplugged without
> > > > leaking
> > > > queues.
> > > > 
> > > > Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode
> > > > driver implementation")
> > > > Cc: stable@dpdk.org
> > > > 
> > > > Signed-off-by: Brian Russell <brussell@brocade.com>
> > > > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > > > ---
> > > > v2: add back extra close() call in uninit() for buggy
> > > > applications as
> > > >     requested by the reviewers, and add debug log noting the
> > > > issue
> > > > 
> > > >  drivers/net/vmxnet3/vmxnet3_ethdev.c | 35
> > > > +++++++++++++++++++++++-----
> > > >  1 file changed, 29 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > > > b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > > > index f1596ab19d..98e5d01890 100644
> > > > --- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > > > +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > > > @@ -354,8 +354,10 @@ eth_vmxnet3_dev_uninit(struct rte_eth_dev
> > > > *eth_dev)
> > > >         if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> > > >                 return 0;
> > > 
> > > This should probably be EPERM as well.  Out of scope though.
> > > 
> > > > 
> > > > -       if (hw->adapter_stopped == 0)
> > > > +       if (hw->adapter_stopped == 0) {
> > > > +               PMD_INIT_LOG(DEBUG, "Device has not been
> > > > closed.");
> > > >                 vmxnet3_dev_close(eth_dev);
> > > 
> > > This just seems wrong.  You have called uninit() will the driver
> > > is
> > > still busy.  Instead of "fixing" the state of the driver, return
> > > EBUSY
> > > here.
> > 
> > I agree.
> > If the port is not stopped, either you stop it or you return EBUSY.
> > 
> > Closing the device should be done outside of this check.
> > It is OK to close from uninit if the app did not close it.
> > 
> > [...]
> > > > +static void
> > > > +vmxnet3_free_queues(struct rte_eth_dev *dev)
> > > > +{
> > > > +       int i;
> > > > +
> > > > +       PMD_INIT_FUNC_TRACE();
> > > > +
> > > > +       for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > > > +               void *rxq = dev->data->rx_queues[i];
> > > > +
> > > > +               vmxnet3_dev_rx_queue_release(rxq);
> > > > +       }
> > > > +       dev->data->nb_rx_queues = 0;
> > > > +
> > > > +       for (i = 0; i < dev->data->nb_tx_queues; i++) {
> > > > +               void *txq = dev->data->tx_queues[i];
> > > > +
> > > > +               vmxnet3_dev_tx_queue_release(txq);
> > > > +       }
> > > > +       dev->data->nb_tx_queues = 0;
> > > >  }
> > > > 
> > > >  /*
> > > > @@ -844,12 +869,10 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
> > > >  static void
> > > >  vmxnet3_dev_close(struct rte_eth_dev *dev)
> > > >  {
> > > > -       struct vmxnet3_hw *hw = dev->data->dev_private;
> > > > -
> > > >         PMD_INIT_FUNC_TRACE();
> > > > 
> > > >         vmxnet3_dev_stop(dev);
> > > > -       hw->adapter_stopped = 1;
> > > > +       vmxnet3_free_queues(dev);
> > > >  }
> > 
> > Good clean-up on dev_close.
> > You probably want to go further and set RTE_ETH_DEV_CLOSE_REMOVE
> > for allowing
> > a real release of the port on close.
> > Note: every PMDs should migrate towards this behaviour.
> > 
> > To make things clear (I will write a doc for -rc2):
> > 	- "stop" should be called by the app but the PMD is allowed to
> > force it.
> > 	- "close" may be called by the app, and the PMD should enforce
> > it in uninit.
> > 		With RTE_ETH_DEV_CLOSE_REMOVE flag, it must completely
> > release the port.
> > 	- "remove" (implemented in PMD as uninit) is responsible of
> > closing
> > 		ethdev ports if not already done, and release the
> > shared resources
> > 		which are not specific to a port. It removes the whole
> > EAL rte_device.
> > 
> > PS: for any hotplug patch or questions, feel free to Cc me.
> 
> 
> 
> 
> 

-- 
Kind regards,
Luca Boccassi

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

* Re: [dpdk-dev] [PATCH v2 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug
  2018-10-31 17:46           ` Luca Boccassi
@ 2018-10-31 18:02             ` Thomas Monjalon
  2018-10-31 18:54             ` Louis Luo
  1 sibling, 0 replies; 30+ messages in thread
From: Thomas Monjalon @ 2018-10-31 18:02 UTC (permalink / raw)
  To: Luca Boccassi, Burakov, Anatoly
  Cc: llouis, yongwang, dev, Chas Williams, Maxime Coquelin, tiwei.bie,
	Bruce Richardson, brussell

31/10/2018 18:46, Luca Boccassi:
> Sorry, been otherwise busy - I can do what you and Chas have asked, but
> the problem is that v1 already did that and the VMWare maintainers
> asked to change it back. So can I assume that the v1 way is the way to
> go?

I am expecting an answer from the vmxnet3 maintainer,
or other VMware developpers.
If they don't reply, we should work without them,
and ask for a new maintainer in the community.

I assume you can now work on a v3 reproducing what was done in v1.


> On Wed, 2018-10-31 at 18:27 +0100, Thomas Monjalon wrote:
> > Any update or question for this patch?
> > If no update, it will miss 18.11.
> > 
> > 
> > 27/10/2018 17:09, Thomas Monjalon:
> > > 19/09/2018 17:47, Chas Williams:
> > > > On Wed, Sep 19, 2018 at 8:58 AM Luca Boccassi <bluca@debian.org>
> > > > wrote:
> > > > > 
> > > > > The vmxnet3 driver can't call back into dev_close(), and
> > > > > possibly
> > > > > dev_stop(), in dev_uninit().  When dev_uninit() is called,
> > > > > anything
> > > > > that those routines would want to clean up has already been
> > > > > released.
> > > > > Further, for complete cleanup, it is necessary to release any
> > > > > of the
> > > > > queue resources during dev_close().
> > > > > This allows a vmxnet3 device to be hot-unplugged without
> > > > > leaking
> > > > > queues.
> > > > > 
> > > > > Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode
> > > > > driver implementation")
> > > > > Cc: stable@dpdk.org
> > > > > 
> > > > > Signed-off-by: Brian Russell <brussell@brocade.com>
> > > > > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > > > > ---
> > > > > v2: add back extra close() call in uninit() for buggy
> > > > > applications as
> > > > >     requested by the reviewers, and add debug log noting the
> > > > > issue
> > > > > 
> > > > >  drivers/net/vmxnet3/vmxnet3_ethdev.c | 35
> > > > > +++++++++++++++++++++++-----
> > > > >  1 file changed, 29 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > > > > b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > > > > index f1596ab19d..98e5d01890 100644
> > > > > --- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > > > > +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > > > > @@ -354,8 +354,10 @@ eth_vmxnet3_dev_uninit(struct rte_eth_dev
> > > > > *eth_dev)
> > > > >         if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> > > > >                 return 0;
> > > > 
> > > > This should probably be EPERM as well.  Out of scope though.
> > > > 
> > > > > 
> > > > > -       if (hw->adapter_stopped == 0)
> > > > > +       if (hw->adapter_stopped == 0) {
> > > > > +               PMD_INIT_LOG(DEBUG, "Device has not been
> > > > > closed.");
> > > > >                 vmxnet3_dev_close(eth_dev);
> > > > 
> > > > This just seems wrong.  You have called uninit() will the driver
> > > > is
> > > > still busy.  Instead of "fixing" the state of the driver, return
> > > > EBUSY
> > > > here.
> > > 
> > > I agree.
> > > If the port is not stopped, either you stop it or you return EBUSY.
> > > 
> > > Closing the device should be done outside of this check.
> > > It is OK to close from uninit if the app did not close it.
> > > 
> > > [...]
> > > > > +static void
> > > > > +vmxnet3_free_queues(struct rte_eth_dev *dev)
> > > > > +{
> > > > > +       int i;
> > > > > +
> > > > > +       PMD_INIT_FUNC_TRACE();
> > > > > +
> > > > > +       for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > > > > +               void *rxq = dev->data->rx_queues[i];
> > > > > +
> > > > > +               vmxnet3_dev_rx_queue_release(rxq);
> > > > > +       }
> > > > > +       dev->data->nb_rx_queues = 0;
> > > > > +
> > > > > +       for (i = 0; i < dev->data->nb_tx_queues; i++) {
> > > > > +               void *txq = dev->data->tx_queues[i];
> > > > > +
> > > > > +               vmxnet3_dev_tx_queue_release(txq);
> > > > > +       }
> > > > > +       dev->data->nb_tx_queues = 0;
> > > > >  }
> > > > > 
> > > > >  /*
> > > > > @@ -844,12 +869,10 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
> > > > >  static void
> > > > >  vmxnet3_dev_close(struct rte_eth_dev *dev)
> > > > >  {
> > > > > -       struct vmxnet3_hw *hw = dev->data->dev_private;
> > > > > -
> > > > >         PMD_INIT_FUNC_TRACE();
> > > > > 
> > > > >         vmxnet3_dev_stop(dev);
> > > > > -       hw->adapter_stopped = 1;
> > > > > +       vmxnet3_free_queues(dev);
> > > > >  }
> > > 
> > > Good clean-up on dev_close.
> > > You probably want to go further and set RTE_ETH_DEV_CLOSE_REMOVE
> > > for allowing
> > > a real release of the port on close.
> > > Note: every PMDs should migrate towards this behaviour.
> > > 
> > > To make things clear (I will write a doc for -rc2):
> > > 	- "stop" should be called by the app but the PMD is allowed to
> > > force it.
> > > 	- "close" may be called by the app, and the PMD should enforce
> > > it in uninit.
> > > 		With RTE_ETH_DEV_CLOSE_REMOVE flag, it must completely
> > > release the port.
> > > 	- "remove" (implemented in PMD as uninit) is responsible of
> > > closing
> > > 		ethdev ports if not already done, and release the
> > > shared resources
> > > 		which are not specific to a port. It removes the whole
> > > EAL rte_device.
> > > 
> > > PS: for any hotplug patch or questions, feel free to Cc me.
> > 
> > 
> > 
> > 
> > 
> 
> 

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

* [dpdk-dev] [PATCH v3 1/3] net/virtio: register/unregister intr handler on start/stop
  2018-08-16 13:50 [dpdk-dev] [PATCH 0/3] Fix hot plug/unplug of virtual devices Luca Boccassi
                   ` (3 preceding siblings ...)
  2018-09-19 12:57 ` [dpdk-dev] [PATCH v2 1/3] net/virtio: register/unregister intr handler on start/stop Luca Boccassi
@ 2018-10-31 18:39 ` Luca Boccassi
  2018-10-31 18:39   ` [dpdk-dev] [PATCH v3 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug Luca Boccassi
                     ` (2 more replies)
  4 siblings, 3 replies; 30+ messages in thread
From: Luca Boccassi @ 2018-10-31 18:39 UTC (permalink / raw)
  To: dev
  Cc: yongwang, 3chas3, bruce.richardson, anatoly.burakov, thomas,
	llouis, Luca Boccassi, stable, Brian Russell

Register and unregister the virtio interrupt handler when the device is
started and stopped. This allows a virtio device to be hotplugged or
unplugged.

Fixes: c1f86306a026 ("virtio: add new driver")
Cc: stable@dpdk.org

Signed-off-by: Brian Russell <brussell@brocade.com>
Signed-off-by: Luca Boccassi <bluca@debian.org>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_ethdev.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 10a7e3fcc..da8717726 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1679,11 +1679,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 	if (ret < 0)
 		goto out;
 
-	/* Setup interrupt callback  */
-	if (eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
-		rte_intr_callback_register(eth_dev->intr_handle,
-			virtio_interrupt_handler, eth_dev);
-
 	return 0;
 
 out:
@@ -1706,11 +1701,6 @@ eth_virtio_dev_uninit(struct rte_eth_dev *eth_dev)
 	eth_dev->tx_pkt_burst = NULL;
 	eth_dev->rx_pkt_burst = NULL;
 
-	/* reset interrupt callback  */
-	if (eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
-		rte_intr_callback_unregister(eth_dev->intr_handle,
-						virtio_interrupt_handler,
-						eth_dev);
 	if (eth_dev->device)
 		rte_pci_unmap_device(RTE_ETH_DEV_TO_PCI(eth_dev));
 
@@ -1969,6 +1959,12 @@ virtio_dev_start(struct rte_eth_dev *dev)
 	    dev->data->dev_conf.intr_conf.rxq) {
 		virtio_intr_disable(dev);
 
+		/* Setup interrupt callback  */
+		if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
+			rte_intr_callback_register(dev->intr_handle,
+						   virtio_interrupt_handler,
+						   dev);
+
 		if (virtio_intr_enable(dev) < 0) {
 			PMD_DRV_LOG(ERR, "interrupt enable failed");
 			return -EIO;
@@ -2078,9 +2074,17 @@ virtio_dev_stop(struct rte_eth_dev *dev)
 	PMD_INIT_LOG(DEBUG, "stop");
 
 	rte_spinlock_lock(&hw->state_lock);
-	if (intr_conf->lsc || intr_conf->rxq)
+	if (intr_conf->lsc || intr_conf->rxq) {
 		virtio_intr_disable(dev);
 
+		/* Reset interrupt callback  */
+		if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC) {
+			rte_intr_callback_unregister(dev->intr_handle,
+						     virtio_interrupt_handler,
+						     dev);
+		}
+	}
+
 	hw->started = 0;
 	memset(&link, 0, sizeof(link));
 	rte_eth_linkstatus_set(dev, &link);
-- 
2.19.1

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

* [dpdk-dev] [PATCH v3 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug
  2018-10-31 18:39 ` [dpdk-dev] [PATCH v3 " Luca Boccassi
@ 2018-10-31 18:39   ` Luca Boccassi
  2018-10-31 18:39   ` [dpdk-dev] [PATCH v3 3/3] eal/linux: handle uio read failure in interrupt handler Luca Boccassi
  2018-11-02  9:49   ` [dpdk-dev] [dpdk-stable] [PATCH v3 1/3] net/virtio: register/unregister intr handler on start/stop Thomas Monjalon
  2 siblings, 0 replies; 30+ messages in thread
From: Luca Boccassi @ 2018-10-31 18:39 UTC (permalink / raw)
  To: dev
  Cc: yongwang, 3chas3, bruce.richardson, anatoly.burakov, thomas,
	llouis, Luca Boccassi, stable, Brian Russell

The vmxnet3 driver can't call back into dev_close(), and possibly
dev_stop(), in dev_uninit().  When dev_uninit() is called, anything
that those routines would want to clean up has already been released.
Further, for complete cleanup, it is necessary to release any of the
queue resources during dev_close().
This allows a vmxnet3 device to be hot-unplugged without leaking
queues.
Also set RTE_ETH_DEV_CLOSE_REMOVE on close so that the port resources
can be deallocated.
Return EBUSY if remove is called before stop.

Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode driver implementation")
Cc: stable@dpdk.org

Signed-off-by: Brian Russell <brussell@brocade.com>
Signed-off-by: Luca Boccassi <bluca@debian.org>
---
v2: add back extra close() call in uninit() for buggy applications as
    requested by the reviewers, and add debug log noting the issue
v3: remove extra close() call in uninit() as requested, and return
    -EBUSY instead.
    set RTE_ETH_DEV_CLOSE_REMOVE in close().

 drivers/net/vmxnet3/vmxnet3_ethdev.c | 43 +++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c b/drivers/net/vmxnet3/vmxnet3_ethdev.c
index 41bcd450a..84acd9dbb 100644
--- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
+++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
@@ -360,8 +360,10 @@ eth_vmxnet3_dev_uninit(struct rte_eth_dev *eth_dev)
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return 0;
 
-	if (hw->adapter_stopped == 0)
-		vmxnet3_dev_close(eth_dev);
+	if (hw->adapter_stopped == 0) {
+		PMD_INIT_LOG(DEBUG, "Device has not been closed.");
+		return -EBUSY;
+	}
 
 	eth_dev->dev_ops = NULL;
 	eth_dev->rx_pkt_burst = NULL;
@@ -805,7 +807,7 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
 	PMD_INIT_FUNC_TRACE();
 
 	if (hw->adapter_stopped == 1) {
-		PMD_INIT_LOG(DEBUG, "Device already closed.");
+		PMD_INIT_LOG(DEBUG, "Device already stopped.");
 		return;
 	}
 
@@ -829,7 +831,6 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
 	/* reset the device */
 	VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_CMD, VMXNET3_CMD_RESET_DEV);
 	PMD_INIT_LOG(DEBUG, "Device reset.");
-	hw->adapter_stopped = 0;
 
 	vmxnet3_dev_clear_queues(dev);
 
@@ -839,6 +840,30 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
 	link.link_speed = ETH_SPEED_NUM_10G;
 	link.link_autoneg = ETH_LINK_FIXED;
 	rte_eth_linkstatus_set(dev, &link);
+
+	hw->adapter_stopped = 1;
+}
+
+static void
+vmxnet3_free_queues(struct rte_eth_dev *dev)
+{
+	int i;
+
+	PMD_INIT_FUNC_TRACE();
+
+	for (i = 0; i < dev->data->nb_rx_queues; i++) {
+		void *rxq = dev->data->rx_queues[i];
+
+		vmxnet3_dev_rx_queue_release(rxq);
+	}
+	dev->data->nb_rx_queues = 0;
+
+	for (i = 0; i < dev->data->nb_tx_queues; i++) {
+		void *txq = dev->data->tx_queues[i];
+
+		vmxnet3_dev_tx_queue_release(txq);
+	}
+	dev->data->nb_tx_queues = 0;
 }
 
 /*
@@ -847,12 +872,16 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
 static void
 vmxnet3_dev_close(struct rte_eth_dev *dev)
 {
-	struct vmxnet3_hw *hw = dev->data->dev_private;
-
 	PMD_INIT_FUNC_TRACE();
 
 	vmxnet3_dev_stop(dev);
-	hw->adapter_stopped = 1;
+	vmxnet3_free_queues(dev);
+
+	/*
+	 * flag to rte_eth_dev_close() that it should release the port resources
+	 * (calling rte_eth_dev_release_port()) in addition to closing it.
+	 */
+	dev->data->dev_flags |= RTE_ETH_DEV_CLOSE_REMOVE;
 }
 
 static void
-- 
2.19.1

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

* [dpdk-dev] [PATCH v3 3/3] eal/linux: handle uio read failure in interrupt handler
  2018-10-31 18:39 ` [dpdk-dev] [PATCH v3 " Luca Boccassi
  2018-10-31 18:39   ` [dpdk-dev] [PATCH v3 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug Luca Boccassi
@ 2018-10-31 18:39   ` Luca Boccassi
  2018-11-02  9:49   ` [dpdk-dev] [dpdk-stable] [PATCH v3 1/3] net/virtio: register/unregister intr handler on start/stop Thomas Monjalon
  2 siblings, 0 replies; 30+ messages in thread
From: Luca Boccassi @ 2018-10-31 18:39 UTC (permalink / raw)
  To: dev
  Cc: yongwang, 3chas3, bruce.richardson, anatoly.burakov, thomas,
	llouis, Luca Boccassi, stable, Brian Russell

If a device is unplugged while an interrupt is pending, the
read call to the uio device to remove it from the poll wait list
can fail resulting in it being continually polled forever. This
change checks for the read failing and if so, unregisters the device
as an interrupt source and causes the wait list to be rebuilt.

This race has been reported and observed in production.

Fixes: 0a45657a6794 ("pci: rework interrupt handling")
Cc: stable@dpdk.org

Signed-off-by: Brian Russell <brussell@brocade.com>
Signed-off-by: Luca Boccassi <bluca@debian.org>
---
 lib/librte_eal/linuxapp/eal/eal_interrupts.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
index 39252a887..cbac451e1 100644
--- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
+++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
@@ -700,7 +700,7 @@ eal_intr_process_interrupts(struct epoll_event *events, int nfds)
 	bool call = false;
 	int n, bytes_read;
 	struct rte_intr_source *src;
-	struct rte_intr_callback *cb;
+	struct rte_intr_callback *cb, *next;
 	union rte_intr_read_buffer buf;
 	struct rte_intr_callback active_cb;
 
@@ -780,6 +780,23 @@ eal_intr_process_interrupts(struct epoll_event *events, int nfds)
 					"descriptor %d: %s\n",
 					events[n].data.fd,
 					strerror(errno));
+				/*
+				 * The device is unplugged or buggy, remove
+				 * it as an interrupt source and return to
+				 * force the wait list to be rebuilt.
+				 */
+				rte_spinlock_lock(&intr_lock);
+				TAILQ_REMOVE(&intr_sources, src, next);
+				rte_spinlock_unlock(&intr_lock);
+
+				for (cb = TAILQ_FIRST(&src->callbacks); cb;
+							cb = next) {
+					next = TAILQ_NEXT(cb, next);
+					TAILQ_REMOVE(&src->callbacks, cb, next);
+					free(cb);
+				}
+				free(src);
+				return -1;
 			} else if (bytes_read == 0)
 				RTE_LOG(ERR, EAL, "Read nothing from file "
 					"descriptor %d\n", events[n].data.fd);
-- 
2.19.1

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

* Re: [dpdk-dev] [PATCH v2 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug
  2018-10-31 17:46           ` Luca Boccassi
  2018-10-31 18:02             ` Thomas Monjalon
@ 2018-10-31 18:54             ` Louis Luo
  1 sibling, 0 replies; 30+ messages in thread
From: Louis Luo @ 2018-10-31 18:54 UTC (permalink / raw)
  To: Luca Boccassi, Thomas Monjalon, Yong Wang
  Cc: dev, Chas Williams, Maxime Coquelin, tiwei.bie, Bruce Richardson,
	Jianfeng Tan, Burakov, Anatoly, brussell

Hey I'm taking paternity leave now so late on response.

The v1 was different from what Thomas asked for and (hw->adapter_stopped == 0) was ignored (see cited below). So we felt uncomfortable about that as there is no guarantee that the device has been closed before calling uninit. Now that you fail the uninit call for non-stopped device and return EBUSY, I'm fine with it.

Regards,
Louis

static int
eth_vmxnet3_dev_uninit(struct rte_eth_dev *eth_dev)
{
-	struct vmxnet3_hw *hw = eth_dev->data->dev_private;
-
	PMD_INIT_FUNC_TRACE();
	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
		return 0;
-	if (hw->adapter_stopped == 0)
-		vmxnet3_dev_close(eth_dev);
-
	eth_dev->dev_ops = NULL;
	eth_dev->rx_pkt_burst = NULL;
	eth_dev->tx_pkt_burst = NULL;

On 10/31/18, 10:46 AM, "Luca Boccassi" <bluca@debian.org> wrote:

    Sorry, been otherwise busy - I can do what you and Chas have asked, but
    the problem is that v1 already did that and the VMWare maintainers
    asked to change it back. So can I assume that the v1 way is the way to
    go?
    
    On Wed, 2018-10-31 at 18:27 +0100, Thomas Monjalon wrote:
    > Any update or question for this patch?
    > If no update, it will miss 18.11.
    > 
    > 
    > 27/10/2018 17:09, Thomas Monjalon:
    > > 19/09/2018 17:47, Chas Williams:
    > > > On Wed, Sep 19, 2018 at 8:58 AM Luca Boccassi <bluca@debian.org>
    > > > wrote:
    > > > > 
    > > > > The vmxnet3 driver can't call back into dev_close(), and
    > > > > possibly
    > > > > dev_stop(), in dev_uninit().  When dev_uninit() is called,
    > > > > anything
    > > > > that those routines would want to clean up has already been
    > > > > released.
    > > > > Further, for complete cleanup, it is necessary to release any
    > > > > of the
    > > > > queue resources during dev_close().
    > > > > This allows a vmxnet3 device to be hot-unplugged without
    > > > > leaking
    > > > > queues.
    > > > > 
    > > > > Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode
    > > > > driver implementation")
    > > > > Cc: stable@dpdk.org
    > > > > 
    > > > > Signed-off-by: Brian Russell <brussell@brocade.com>
    > > > > Signed-off-by: Luca Boccassi <bluca@debian.org>
    > > > > ---
    > > > > v2: add back extra close() call in uninit() for buggy
    > > > > applications as
    > > > >     requested by the reviewers, and add debug log noting the
    > > > > issue
    > > > > 
    > > > >  drivers/net/vmxnet3/vmxnet3_ethdev.c | 35
    > > > > +++++++++++++++++++++++-----
    > > > >  1 file changed, 29 insertions(+), 6 deletions(-)
    > > > > 
    > > > > diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c
    > > > > b/drivers/net/vmxnet3/vmxnet3_ethdev.c
    > > > > index f1596ab19d..98e5d01890 100644
    > > > > --- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
    > > > > +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
    > > > > @@ -354,8 +354,10 @@ eth_vmxnet3_dev_uninit(struct rte_eth_dev
    > > > > *eth_dev)
    > > > >         if (rte_eal_process_type() != RTE_PROC_PRIMARY)
    > > > >                 return 0;
    > > > 
    > > > This should probably be EPERM as well.  Out of scope though.
    > > > 
    > > > > 
    > > > > -       if (hw->adapter_stopped == 0)
    > > > > +       if (hw->adapter_stopped == 0) {
    > > > > +               PMD_INIT_LOG(DEBUG, "Device has not been
    > > > > closed.");
    > > > >                 vmxnet3_dev_close(eth_dev);
    > > > 
    > > > This just seems wrong.  You have called uninit() will the driver
    > > > is
    > > > still busy.  Instead of "fixing" the state of the driver, return
    > > > EBUSY
    > > > here.
    > > 
    > > I agree.
    > > If the port is not stopped, either you stop it or you return EBUSY.
    > > 
    > > Closing the device should be done outside of this check.
    > > It is OK to close from uninit if the app did not close it.
    > > 
    > > [...]
    > > > > +static void
    > > > > +vmxnet3_free_queues(struct rte_eth_dev *dev)
    > > > > +{
    > > > > +       int i;
    > > > > +
    > > > > +       PMD_INIT_FUNC_TRACE();
    > > > > +
    > > > > +       for (i = 0; i < dev->data->nb_rx_queues; i++) {
    > > > > +               void *rxq = dev->data->rx_queues[i];
    > > > > +
    > > > > +               vmxnet3_dev_rx_queue_release(rxq);
    > > > > +       }
    > > > > +       dev->data->nb_rx_queues = 0;
    > > > > +
    > > > > +       for (i = 0; i < dev->data->nb_tx_queues; i++) {
    > > > > +               void *txq = dev->data->tx_queues[i];
    > > > > +
    > > > > +               vmxnet3_dev_tx_queue_release(txq);
    > > > > +       }
    > > > > +       dev->data->nb_tx_queues = 0;
    > > > >  }
    > > > > 
    > > > >  /*
    > > > > @@ -844,12 +869,10 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
    > > > >  static void
    > > > >  vmxnet3_dev_close(struct rte_eth_dev *dev)
    > > > >  {
    > > > > -       struct vmxnet3_hw *hw = dev->data->dev_private;
    > > > > -
    > > > >         PMD_INIT_FUNC_TRACE();
    > > > > 
    > > > >         vmxnet3_dev_stop(dev);
    > > > > -       hw->adapter_stopped = 1;
    > > > > +       vmxnet3_free_queues(dev);
    > > > >  }
    > > 
    > > Good clean-up on dev_close.
    > > You probably want to go further and set RTE_ETH_DEV_CLOSE_REMOVE
    > > for allowing
    > > a real release of the port on close.
    > > Note: every PMDs should migrate towards this behaviour.
    > > 
    > > To make things clear (I will write a doc for -rc2):
    > > 	- "stop" should be called by the app but the PMD is allowed to
    > > force it.
    > > 	- "close" may be called by the app, and the PMD should enforce
    > > it in uninit.
    > > 		With RTE_ETH_DEV_CLOSE_REMOVE flag, it must completely
    > > release the port.
    > > 	- "remove" (implemented in PMD as uninit) is responsible of
    > > closing
    > > 		ethdev ports if not already done, and release the
    > > shared resources
    > > 		which are not specific to a port. It removes the whole
    > > EAL rte_device.
    > > 
    > > PS: for any hotplug patch or questions, feel free to Cc me.
    > 
    > 
    > 
    > 
    > 
    
    -- 
    Kind regards,
    Luca Boccassi
    


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

* Re: [dpdk-dev] [dpdk-stable] [PATCH v3 1/3] net/virtio: register/unregister intr handler on start/stop
  2018-10-31 18:39 ` [dpdk-dev] [PATCH v3 " Luca Boccassi
  2018-10-31 18:39   ` [dpdk-dev] [PATCH v3 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug Luca Boccassi
  2018-10-31 18:39   ` [dpdk-dev] [PATCH v3 3/3] eal/linux: handle uio read failure in interrupt handler Luca Boccassi
@ 2018-11-02  9:49   ` Thomas Monjalon
  2018-11-02 11:14     ` Luca Boccassi
  2 siblings, 1 reply; 30+ messages in thread
From: Thomas Monjalon @ 2018-11-02  9:49 UTC (permalink / raw)
  To: Luca Boccassi
  Cc: stable, dev, yongwang, 3chas3, bruce.richardson, anatoly.burakov,
	llouis, Brian Russell

31/10/2018 19:39, Luca Boccassi:
> Register and unregister the virtio interrupt handler when the device is
> started and stopped. This allows a virtio device to be hotplugged or
> unplugged.
> 
> Fixes: c1f86306a026 ("virtio: add new driver")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Brian Russell <brussell@brocade.com>
> Signed-off-by: Luca Boccassi <bluca@debian.org>
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Series applied, thanks

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

* Re: [dpdk-dev] [dpdk-stable] [PATCH v3 1/3] net/virtio: register/unregister intr handler on start/stop
  2018-11-02  9:49   ` [dpdk-dev] [dpdk-stable] [PATCH v3 1/3] net/virtio: register/unregister intr handler on start/stop Thomas Monjalon
@ 2018-11-02 11:14     ` Luca Boccassi
  0 siblings, 0 replies; 30+ messages in thread
From: Luca Boccassi @ 2018-11-02 11:14 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: stable, dev, yongwang, 3chas3, bruce.richardson, anatoly.burakov,
	llouis, Brian Russell

On Fri, 2018-11-02 at 10:49 +0100, Thomas Monjalon wrote:
> 31/10/2018 19:39, Luca Boccassi:
> > Register and unregister the virtio interrupt handler when the
> > device is
> > started and stopped. This allows a virtio device to be hotplugged
> > or
> > unplugged.
> > 
> > Fixes: c1f86306a026 ("virtio: add new driver")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Brian Russell <brussell@brocade.com>
> > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> Series applied, thanks

Thanks!

-- 
Kind regards,
Luca Boccassi

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

end of thread, other threads:[~2018-11-02 11:14 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-16 13:50 [dpdk-dev] [PATCH 0/3] Fix hot plug/unplug of virtual devices Luca Boccassi
2018-08-16 13:50 ` [dpdk-dev] [PATCH 1/3] net/virtio: register/unregister intr handler on start/stop Luca Boccassi
2018-08-16 13:50 ` [dpdk-dev] [PATCH 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug Luca Boccassi
2018-09-17 19:06   ` Louis Luo
2018-09-18 13:14     ` Luca Boccassi
2018-09-18 18:14       ` Louis Luo
2018-09-18 18:29         ` Luca Boccassi
2018-09-18 18:48           ` Louis Luo
2018-09-19 12:58             ` Luca Boccassi
2018-08-16 13:50 ` [dpdk-dev] [PATCH 3/3] eal/linux: handle uio read failure in interrupt handler Luca Boccassi
2018-09-19 12:57 ` [dpdk-dev] [PATCH v2 1/3] net/virtio: register/unregister intr handler on start/stop Luca Boccassi
2018-09-19 12:57   ` [dpdk-dev] [PATCH v2 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug Luca Boccassi
2018-09-19 15:47     ` Chas Williams
2018-09-19 16:08       ` Luca Boccassi
2018-10-27 15:09       ` Thomas Monjalon
2018-10-31 17:27         ` Thomas Monjalon
2018-10-31 17:46           ` Luca Boccassi
2018-10-31 18:02             ` Thomas Monjalon
2018-10-31 18:54             ` Louis Luo
2018-09-27  8:39     ` Luca Boccassi
2018-09-19 12:57   ` [dpdk-dev] [PATCH v2 3/3] eal/linux: handle uio read failure in interrupt handler Luca Boccassi
2018-10-11 10:32     ` Thomas Monjalon
2018-09-27  8:40   ` [dpdk-dev] [PATCH v2 1/3] net/virtio: register/unregister intr handler on start/stop Luca Boccassi
2018-09-27 10:51     ` Maxime Coquelin
2018-09-27 11:14   ` Maxime Coquelin
2018-10-31 18:39 ` [dpdk-dev] [PATCH v3 " Luca Boccassi
2018-10-31 18:39   ` [dpdk-dev] [PATCH v3 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug Luca Boccassi
2018-10-31 18:39   ` [dpdk-dev] [PATCH v3 3/3] eal/linux: handle uio read failure in interrupt handler Luca Boccassi
2018-11-02  9:49   ` [dpdk-dev] [dpdk-stable] [PATCH v3 1/3] net/virtio: register/unregister intr handler on start/stop Thomas Monjalon
2018-11-02 11:14     ` Luca Boccassi

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).