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

[Thomas Monjalon <thomas@monjalon.net>]

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.