From: Louis Luo <llouis@vmware.com>
To: Luca Boccassi <bluca@debian.org>,
Thomas Monjalon <thomas@monjalon.net>,
Yong Wang <yongwang@vmware.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, Chas Williams <3chas3@gmail.com>,
Maxime Coquelin <maxime.coquelin@redhat.com>,
"tiwei.bie@intel.com" <tiwei.bie@intel.com>,
Bruce Richardson <bruce.richardson@intel.com>,
Jianfeng Tan <jianfeng.tan@intel.com>,
"Burakov, Anatoly" <anatoly.burakov@intel.com>,
"brussell@vyatta.att-mail.com" <brussell@vyatta.att-mail.com>
Subject: Re: [dpdk-dev] [PATCH v2 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug
Date: Wed, 31 Oct 2018 18:54:02 +0000 [thread overview]
Message-ID: <AB591D64-0091-4656-A87E-41A5E5949A77@vmware.com> (raw)
In-Reply-To: <1541008002.29722.34.camel@debian.org>
Hey I'm taking paternity leave now so late on response.
The v1 was different from what Thomas asked for and (hw->adapter_stopped == 0) was ignored (see cited below). So we felt uncomfortable about that as there is no guarantee that the device has been closed before calling uninit. Now that you fail the uninit call for non-stopped device and return EBUSY, I'm fine with it.
Regards,
Louis
static int
eth_vmxnet3_dev_uninit(struct rte_eth_dev *eth_dev)
{
- struct vmxnet3_hw *hw = eth_dev->data->dev_private;
-
PMD_INIT_FUNC_TRACE();
if (rte_eal_process_type() != RTE_PROC_PRIMARY)
return 0;
- if (hw->adapter_stopped == 0)
- vmxnet3_dev_close(eth_dev);
-
eth_dev->dev_ops = NULL;
eth_dev->rx_pkt_burst = NULL;
eth_dev->tx_pkt_burst = NULL;
On 10/31/18, 10:46 AM, "Luca Boccassi" <bluca@debian.org> wrote:
Sorry, been otherwise busy - I can do what you and Chas have asked, but
the problem is that v1 already did that and the VMWare maintainers
asked to change it back. So can I assume that the v1 way is the way to
go?
On Wed, 2018-10-31 at 18:27 +0100, Thomas Monjalon wrote:
> Any update or question for this patch?
> If no update, it will miss 18.11.
>
>
> 27/10/2018 17:09, Thomas Monjalon:
> > 19/09/2018 17:47, Chas Williams:
> > > On Wed, Sep 19, 2018 at 8:58 AM Luca Boccassi <bluca@debian.org>
> > > wrote:
> > > >
> > > > The vmxnet3 driver can't call back into dev_close(), and
> > > > possibly
> > > > dev_stop(), in dev_uninit(). When dev_uninit() is called,
> > > > anything
> > > > that those routines would want to clean up has already been
> > > > released.
> > > > Further, for complete cleanup, it is necessary to release any
> > > > of the
> > > > queue resources during dev_close().
> > > > This allows a vmxnet3 device to be hot-unplugged without
> > > > leaking
> > > > queues.
> > > >
> > > > Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode
> > > > driver implementation")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Brian Russell <brussell@brocade.com>
> > > > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > > > ---
> > > > v2: add back extra close() call in uninit() for buggy
> > > > applications as
> > > > requested by the reviewers, and add debug log noting the
> > > > issue
> > > >
> > > > drivers/net/vmxnet3/vmxnet3_ethdev.c | 35
> > > > +++++++++++++++++++++++-----
> > > > 1 file changed, 29 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > > > b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > > > index f1596ab19d..98e5d01890 100644
> > > > --- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > > > +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
> > > > @@ -354,8 +354,10 @@ eth_vmxnet3_dev_uninit(struct rte_eth_dev
> > > > *eth_dev)
> > > > if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> > > > return 0;
> > >
> > > This should probably be EPERM as well. Out of scope though.
> > >
> > > >
> > > > - if (hw->adapter_stopped == 0)
> > > > + if (hw->adapter_stopped == 0) {
> > > > + PMD_INIT_LOG(DEBUG, "Device has not been
> > > > closed.");
> > > > vmxnet3_dev_close(eth_dev);
> > >
> > > This just seems wrong. You have called uninit() will the driver
> > > is
> > > still busy. Instead of "fixing" the state of the driver, return
> > > EBUSY
> > > here.
> >
> > I agree.
> > If the port is not stopped, either you stop it or you return EBUSY.
> >
> > Closing the device should be done outside of this check.
> > It is OK to close from uninit if the app did not close it.
> >
> > [...]
> > > > +static void
> > > > +vmxnet3_free_queues(struct rte_eth_dev *dev)
> > > > +{
> > > > + int i;
> > > > +
> > > > + PMD_INIT_FUNC_TRACE();
> > > > +
> > > > + for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > > > + void *rxq = dev->data->rx_queues[i];
> > > > +
> > > > + vmxnet3_dev_rx_queue_release(rxq);
> > > > + }
> > > > + dev->data->nb_rx_queues = 0;
> > > > +
> > > > + for (i = 0; i < dev->data->nb_tx_queues; i++) {
> > > > + void *txq = dev->data->tx_queues[i];
> > > > +
> > > > + vmxnet3_dev_tx_queue_release(txq);
> > > > + }
> > > > + dev->data->nb_tx_queues = 0;
> > > > }
> > > >
> > > > /*
> > > > @@ -844,12 +869,10 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev)
> > > > static void
> > > > vmxnet3_dev_close(struct rte_eth_dev *dev)
> > > > {
> > > > - struct vmxnet3_hw *hw = dev->data->dev_private;
> > > > -
> > > > PMD_INIT_FUNC_TRACE();
> > > >
> > > > vmxnet3_dev_stop(dev);
> > > > - hw->adapter_stopped = 1;
> > > > + vmxnet3_free_queues(dev);
> > > > }
> >
> > Good clean-up on dev_close.
> > You probably want to go further and set RTE_ETH_DEV_CLOSE_REMOVE
> > for allowing
> > a real release of the port on close.
> > Note: every PMDs should migrate towards this behaviour.
> >
> > To make things clear (I will write a doc for -rc2):
> > - "stop" should be called by the app but the PMD is allowed to
> > force it.
> > - "close" may be called by the app, and the PMD should enforce
> > it in uninit.
> > With RTE_ETH_DEV_CLOSE_REMOVE flag, it must completely
> > release the port.
> > - "remove" (implemented in PMD as uninit) is responsible of
> > closing
> > ethdev ports if not already done, and release the
> > shared resources
> > which are not specific to a port. It removes the whole
> > EAL rte_device.
> >
> > PS: for any hotplug patch or questions, feel free to Cc me.
>
>
>
>
>
--
Kind regards,
Luca Boccassi
next prev parent reply other threads:[~2018-10-31 18:54 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-16 13:50 [dpdk-dev] [PATCH 0/3] Fix hot plug/unplug of virtual devices Luca Boccassi
2018-08-16 13:50 ` [dpdk-dev] [PATCH 1/3] net/virtio: register/unregister intr handler on start/stop Luca Boccassi
2018-08-16 13:50 ` [dpdk-dev] [PATCH 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug Luca Boccassi
2018-09-17 19:06 ` Louis Luo
2018-09-18 13:14 ` Luca Boccassi
2018-09-18 18:14 ` Louis Luo
2018-09-18 18:29 ` Luca Boccassi
2018-09-18 18:48 ` Louis Luo
2018-09-19 12:58 ` Luca Boccassi
2018-08-16 13:50 ` [dpdk-dev] [PATCH 3/3] eal/linux: handle uio read failure in interrupt handler Luca Boccassi
2018-09-19 12:57 ` [dpdk-dev] [PATCH v2 1/3] net/virtio: register/unregister intr handler on start/stop Luca Boccassi
2018-09-19 12:57 ` [dpdk-dev] [PATCH v2 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug Luca Boccassi
2018-09-19 15:47 ` Chas Williams
2018-09-19 16:08 ` Luca Boccassi
2018-10-27 15:09 ` Thomas Monjalon
2018-10-31 17:27 ` Thomas Monjalon
2018-10-31 17:46 ` Luca Boccassi
2018-10-31 18:02 ` Thomas Monjalon
2018-10-31 18:54 ` Louis Luo [this message]
2018-09-27 8:39 ` Luca Boccassi
2018-09-19 12:57 ` [dpdk-dev] [PATCH v2 3/3] eal/linux: handle uio read failure in interrupt handler Luca Boccassi
2018-10-11 10:32 ` Thomas Monjalon
2018-09-27 8:40 ` [dpdk-dev] [PATCH v2 1/3] net/virtio: register/unregister intr handler on start/stop Luca Boccassi
2018-09-27 10:51 ` Maxime Coquelin
2018-09-27 11:14 ` Maxime Coquelin
2018-10-31 18:39 ` [dpdk-dev] [PATCH v3 " Luca Boccassi
2018-10-31 18:39 ` [dpdk-dev] [PATCH v3 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug Luca Boccassi
2018-10-31 18:39 ` [dpdk-dev] [PATCH v3 3/3] eal/linux: handle uio read failure in interrupt handler Luca Boccassi
2018-11-02 9:49 ` [dpdk-dev] [dpdk-stable] [PATCH v3 1/3] net/virtio: register/unregister intr handler on start/stop Thomas Monjalon
2018-11-02 11:14 ` Luca Boccassi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AB591D64-0091-4656-A87E-41A5E5949A77@vmware.com \
--to=llouis@vmware.com \
--cc=3chas3@gmail.com \
--cc=anatoly.burakov@intel.com \
--cc=bluca@debian.org \
--cc=bruce.richardson@intel.com \
--cc=brussell@vyatta.att-mail.com \
--cc=dev@dpdk.org \
--cc=jianfeng.tan@intel.com \
--cc=maxime.coquelin@redhat.com \
--cc=thomas@monjalon.net \
--cc=tiwei.bie@intel.com \
--cc=yongwang@vmware.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).