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

[Louis Luo]

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 at 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 at 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 at dpdk.org
    > > > > 
    > > > > Signed-off-by: Brian Russell <brussell at brocade.com>
    > > > > Signed-off-by: Luca Boccassi <bluca at 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