[dpdk-stable] [PATCH 1/3] net/virtio: register/unregister intr handler on start/stop

[dpdk-stable] [PATCH 1/3] net/virtio: register/unregister intr handler on start/stop

[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>
---
 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

[dpdk-stable] [PATCH 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug

[Luca Boccassi]

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

Re: [dpdk-stable] [dpdk-dev] [PATCH 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug

[Louis Luo]

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
    
    

Re: [dpdk-stable] [dpdk-dev] [PATCH 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug

[Luca Boccassi]

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

Re: [dpdk-stable] [dpdk-dev] [PATCH 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug

[Louis Luo]

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

[dpdk-stable] [PATCH 3/3] eal/linux: handle uio read failure in interrupt handler

[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);
-- 
2.18.0

[dpdk-stable] [PATCH v3 1/3] net/virtio: register/unregister intr handler on start/stop

[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>
---
 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

[dpdk-stable] [PATCH v3 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug

[Luca Boccassi]

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

[dpdk-stable] [PATCH v3 3/3] eal/linux: handle uio read failure in interrupt handler

[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 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

Re: [dpdk-stable] [PATCH v3 1/3] net/virtio: register/unregister intr handler on start/stop

[Thomas Monjalon]

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

Re: [dpdk-stable] [PATCH v3 1/3] net/virtio: register/unregister intr handler on start/stop

[Luca Boccassi]

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