Fix hot plug/unplug of virtual devices

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 [email protected]> 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: [email protected]

Signed-off-by Brian Russell <[email protected]>

Signed-off-by Luca Boccassi <[email protected]>

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

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-

Quoted 1 line

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" <[email protected]> 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 [email protected]> 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: [email protected]
    >
    >     Signed-off-by: Brian Russell <[email protected]> >     Signed-off-by: Luca Boccassi <[email protected]> >     ---
    >      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-

Quoted 1 line

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

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" <[email protected]> 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" <[email protected]> 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 [email protected]> 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: [email protected]
    >     >
    >     >     Signed-off-by: Brian Russell <[email protected]> >     >     Signed-off-by: Luca Boccassi <[email protected]> >     >     ---
    >     >      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-

Quoted 1 line

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

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 [email protected]

Signed-off-by Brian Russell <[email protected]>

Signed-off-by Luca Boccassi <[email protected]>

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

drivers/net/vmxnet3/vmxnet3_ethdev.c +0 -0

index f1596ab19d..98e5d01890 100644

drivers/net/vmxnet3/vmxnet3_ethdev.c +27 -6

+       }
 
        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

Quoted 32 lines · 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: [email protected]

Signed-off-by Brian Russell <[email protected]>

Signed-off-by Luca Boccassi <[email protected]>

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

drivers/net/vmxnet3/vmxnet3_ethdev.c +0 -0

index f1596ab19d..98e5d01890 100644

drivers/net/vmxnet3/vmxnet3_ethdev.c eth_vmxnet3_dev_uninit(struct rte_eth_dev · line 356

Quoted 3 lines

drivers/net/vmxnet3/vmxnet3_ethdev.c +0 -0

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

drivers/net/vmxnet3/vmxnet3_ethdev.c eth_vmxnet3_dev_uninit(struct rte_eth_dev *eth_dev) · line 359

Quoted 5 lines

drivers/net/vmxnet3/vmxnet3_ethdev.c +2 -1

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

Quoted 70 lines

drivers/net/vmxnet3/vmxnet3_ethdev.c +26 -2

+       }
 
         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

Quoted 26 lines

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 [email protected]

Signed-off-by Brian Russell <[email protected]>

Signed-off-by Luca Boccassi <[email protected]>

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

drivers/net/vmxnet3/vmxnet3_ethdev.c +0 -0

index f1596ab19d..98e5d01890 100644

drivers/net/vmxnet3/vmxnet3_ethdev.c eth_vmxnet3_dev_uninit(struct rte_eth_dev *eth_dev) · line 355

Quoted 2 lines

drivers/net/vmxnet3/vmxnet3_ethdev.c +0 -0

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

Quoted 1 line

drivers/net/vmxnet3/vmxnet3_ethdev.c eth_vmxnet3_dev_uninit(struct rte_eth_dev *eth_dev) · line 359

Quoted 4 lines

drivers/net/vmxnet3/vmxnet3_ethdev.c +2 -1

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

Quoted 31 lines

drivers/net/vmxnet3/vmxnet3_ethdev.c +20 -2

+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();
 

drivers/net/vmxnet3/vmxnet3_ethdev.c vmxnet3_dev_stop(struct rte_eth_dev *dev) · line 876

Quoted 4 lines

drivers/net/vmxnet3/vmxnet3_ethdev.c +1 -1

        vmxnet3_dev_stop(dev);
-       hw->adapter_stopped = 1;
+       vmxnet3_free_queues(dev);
 }

19/09/2018 17:47, Chas Williams:

Quoted 33 lines · Luca Boccassi

Quoted 26 lines

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 [email protected]

Signed-off-by Brian Russell <[email protected]>

Signed-off-by Luca Boccassi <[email protected]>

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

drivers/net/vmxnet3/vmxnet3_ethdev.c +0 -0

index f1596ab19d..98e5d01890 100644

drivers/net/vmxnet3/vmxnet3_ethdev.c eth_vmxnet3_dev_uninit(struct rte_eth_dev *eth_dev) · line 355

Quoted 2 lines

drivers/net/vmxnet3/vmxnet3_ethdev.c +0 -0

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

Quoted 1 line

Quoted 8 lines

drivers/net/vmxnet3/vmxnet3_ethdev.c eth_vmxnet3_dev_uninit(struct rte_eth_dev *eth_dev) · line 359

Quoted 4 lines

drivers/net/vmxnet3/vmxnet3_ethdev.c +2 -1

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

[...]

Quoted 31 lines

Quoted 31 lines

drivers/net/vmxnet3/vmxnet3_ethdev.c +20 -2

+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();
 

Quoted 4 lines

drivers/net/vmxnet3/vmxnet3_ethdev.c vmxnet3_dev_stop(struct rte_eth_dev *dev) · line 876

Quoted 4 lines

drivers/net/vmxnet3/vmxnet3_ethdev.c +1 -1

        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.

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

27/10/2018 17:09, Thomas Monjalon:

Quoted 122 lines

19/09/2018 17:47, Chas Williams:

Quoted 51 lines

Quoted 36 lines · 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: [email protected]

Signed-off-by Brian Russell <[email protected]>

Signed-off-by Luca Boccassi <[email protected]>

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

drivers/net/vmxnet3/vmxnet3_ethdev.c +0 -0

index f1596ab19d..98e5d01890 100644

drivers/net/vmxnet3/vmxnet3_ethdev.c eth_vmxnet3_dev_uninit(struct rte_eth_dev · line 356

Quoted 3 lines

drivers/net/vmxnet3/vmxnet3_ethdev.c +0 -0

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

drivers/net/vmxnet3/vmxnet3_ethdev.c eth_vmxnet3_dev_uninit(struct rte_eth_dev *eth_dev) · line 359

Quoted 6 lines

drivers/net/vmxnet3/vmxnet3_ethdev.c +2 -1

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

Quoted 5 lines

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.

[...]

Quoted 31 lines

Quoted 31 lines

drivers/net/vmxnet3/vmxnet3_ethdev.c +20 -0

+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;

 }

 /*

drivers/net/vmxnet3/vmxnet3_ethdev.c +0 -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();

Quoted 4 lines

Quoted 4 lines

        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.

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?

Quoted 132 lines

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

27/10/2018 17:09, Thomas Monjalon:

Quoted 122 lines

19/09/2018 17:47, Chas Williams:

Quoted 51 lines

Quoted 36 lines · 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: [email protected]

Signed-off-by Brian Russell <[email protected]>

Signed-off-by Luca Boccassi <[email protected]>

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

drivers/net/vmxnet3/vmxnet3_ethdev.c +0 -0

index f1596ab19d..98e5d01890 100644

drivers/net/vmxnet3/vmxnet3_ethdev.c eth_vmxnet3_dev_uninit(struct rte_eth_dev · line 356

Quoted 3 lines

drivers/net/vmxnet3/vmxnet3_ethdev.c +0 -0

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

drivers/net/vmxnet3/vmxnet3_ethdev.c eth_vmxnet3_dev_uninit(struct rte_eth_dev *eth_dev) · line 359

Quoted 6 lines

drivers/net/vmxnet3/vmxnet3_ethdev.c +2 -1

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

Quoted 5 lines

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.

[...]

Quoted 31 lines

Quoted 31 lines

drivers/net/vmxnet3/vmxnet3_ethdev.c +20 -2

+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();
 

Quoted 4 lines

drivers/net/vmxnet3/vmxnet3_ethdev.c vmxnet3_dev_stop(struct rte_eth_dev *dev) · line 876

Quoted 4 lines

drivers/net/vmxnet3/vmxnet3_ethdev.c +1 -1

        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.

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: [email protected]

Signed-off-by Brian Russell <[email protected]>

Signed-off-by Luca Boccassi <[email protected]>

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

drivers/net/vmxnet3/vmxnet3_ethdev.c +0 -0

index f1596ab19d..98e5d01890 100644
@@ -354,8 +354,10 @@ eth_vmxnet3_dev_uninit(struct rte_eth_dev
 *eth_dev)
  	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
  		return 0;

drivers/net/vmxnet3/vmxnet3_ethdev.c +2 -1

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

  vmxnet3_dev_close(eth_dev);

drivers/net/vmxnet3/vmxnet3_ethdev.c +1 -0

+	}

  eth_dev->dev_ops = NULL;
  eth_dev->rx_pkt_burst = NULL;

drivers/net/vmxnet3/vmxnet3_ethdev.c +0 -0

@@ -802,7 +804,7 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
  	PMD_INIT_FUNC_TRACE();

  if (hw->adapter_stopped == 1) {

drivers/net/vmxnet3/vmxnet3_ethdev.c +1 -1

-		PMD_INIT_LOG(DEBUG, "Device already closed.");
+		PMD_INIT_LOG(DEBUG, "Device already stopped.");

  return;
  }

drivers/net/vmxnet3/vmxnet3_ethdev.c +0 -1

@@ -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);

drivers/net/vmxnet3/vmxnet3_ethdev.c +24 -0

@@ -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;
  }

 /*

drivers/net/vmxnet3/vmxnet3_ethdev.c +0 -0

@@ -844,12 +869,10 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)

 static void
 vmxnet3_dev_close(struct rte_eth_dev *dev)
 {

drivers/net/vmxnet3/vmxnet3_ethdev.c +0 -2

-	struct vmxnet3_hw *hw = dev->data->dev_private;
-

  PMD_INIT_FUNC_TRACE();

  vmxnet3_dev_stop(dev);

drivers/net/vmxnet3/vmxnet3_ethdev.c +1 -1

-	hw->adapter_stopped = 1;
+	vmxnet3_free_queues(dev);

 }

 static void

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 [email protected]

Signed-off-by Brian Russell <[email protected]>

Signed-off-by Luca Boccassi <[email protected]>

---
 lib/librte_eal/linuxapp/eal/eal_interrupts.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

lib/librte_eal/linuxapp/eal/eal_interrupts.c +18 -1

index 4076c6d6ca..34584db883 100644
@@ -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);

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 [email protected]

Signed-off-by Brian Russell <[email protected]>

Signed-off-by Luca Boccassi <[email protected]>

---
 drivers/net/virtio/virtio_ethdev.c | 26 +++++++++++++++-----------

 1 file changed, 15 insertions(+), 11 deletions(-)

drivers/net/virtio/virtio_ethdev.c +0 -0

index b81df0a99f..adc6a30a32 100644
@@ -1679,11 +1679,6 @@ eth_virtio_dev_init(struct rte_eth_dev
 *eth_dev)
  	if (ret < 0)
  		goto out;

drivers/net/virtio/virtio_ethdev.c +0 -5

-	/* 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:

drivers/net/virtio/virtio_ethdev.c +0 -0

@@ -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;

drivers/net/virtio/virtio_ethdev.c +0 -4

-	/* 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,

drivers/net/virtio/virtio_ethdev.c +0 -1

-						eth_dev);

  if (eth_dev->device) rte_pci_unmap_device(RTE_ETH_DEV_TO_PCI(eth_dev));

drivers/net/virtio/virtio_ethdev.c +0 -0

@@ -1972,6 +1962,12 @@ virtio_dev_start(struct rte_eth_dev *dev)
  	    dev->data->dev_conf.intr_conf.rxq) {
  		virtio_intr_disable(dev);

drivers/net/virtio/virtio_ethdev.c +4 -0

+		/* Setup interrupt callback  */
+		if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
+			rte_intr_callback_register(dev->intr_handle,
+						   virtio_interrupt_

handler,

drivers/net/virtio/virtio_ethdev.c +2 -0

+						   dev);
+

  if (virtio_intr_enable(dev) < 0) { PMD_DRV_LOG(ERR, "interrupt enable failed"); return -EIO;

drivers/net/virtio/virtio_ethdev.c +0 -0

@@ -2081,9 +2077,17 @@ virtio_dev_stop(struct rte_eth_dev *dev)
  	PMD_INIT_LOG(DEBUG, "stop");

  rte_spinlock_lock(&hw->state_lock);

drivers/net/virtio/virtio_ethdev.c +1 -1

-	if (intr_conf->lsc || intr_conf->rxq)
+	if (intr_conf->lsc || intr_conf->rxq) {

  virtio_intr_disable(dev);

drivers/net/virtio/virtio_ethdev.c +3 -0

+		/* Reset interrupt callback  */
+		if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC) {
+			rte_intr_callback_unregister(dev-

Quoted 1 line

intr_handle,

drivers/net/virtio/virtio_ethdev.c +1 -0

+						     virtio_interrup

t_handler,

drivers/net/virtio/virtio_ethdev.c +4 -0

+						     dev);
+		}
+	}
+

  hw->started = 0;
  memset(&link, 0, sizeof(link));
  rte_eth_linkstatus_set(dev, &link);

Quoted 72 lines · 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 [email protected]

Signed-off-by Brian Russell <[email protected]>

Signed-off-by Luca Boccassi <[email protected]>

---
  drivers/net/virtio/virtio_ethdev.c | 26 +++++++++++++++-----------
  1 file changed, 15 insertions(+), 11 deletions(-)

drivers/net/virtio/virtio_ethdev.c +0 -5

index b81df0a99f..adc6a30a32 100644
@@ -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:

drivers/net/virtio/virtio_ethdev.c +0 -5

@@ -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));

drivers/net/virtio/virtio_ethdev.c +6 -0

@@ -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;

drivers/net/virtio/virtio_ethdev.c +4 -1

@@ -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,

Quoted 9 lines

drivers/net/virtio/virtio_ethdev.c +5 -0

+						     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!

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 [email protected]

Signed-off-by Brian Russell <[email protected]>

Signed-off-by Luca Boccassi <[email protected]>

---
  drivers/net/virtio/virtio_ethdev.c | 26 +++++++++++++++-----------
  1 file changed, 15 insertions(+), 11 deletions(-)

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 [email protected]

Signed-off-by Brian Russell <[email protected]>

Signed-off-by Luca Boccassi <[email protected]>

Reviewed-by Maxime Coquelin <[email protected]>

31/10/2018 19:39, Luca Boccassi:

Quoted 6 lines

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

Quoted 6 lines

Fixes c1f86306a026 ("virtio: add new driver")

Cc [email protected]

Signed-off-by Brian Russell <[email protected]>

Signed-off-by Luca Boccassi <[email protected]>

Reviewed-by Maxime Coquelin <[email protected]>

Series applied, thanks