[2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug
Checks
Commit Message
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(-)
Comments
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
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
>
>
>
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&data=02%7C01%7Cllouis%40vmware.com%7C3d87a5482cd046679d7608d61d68bf9a%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636728733078700911&sdata=qBx7zyOJhTmVH0c2ZEBohmIHL6PkYwB6XPaw2epgS0E%3D&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
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" <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%2Fd
> oc.dpdk.org%2Fguides%2Fprog_guide%2Fport_hotplug_framework.html&d
> ata=02%7C01%7Cllouis%40vmware.com%7C3d87a5482cd046679d7608d61d68bf9a%
> 7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636728733078700911&s
> data=qBx7zyOJhTmVH0c2ZEBohmIHL6PkYwB6XPaw2epgS0E%3D&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 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
>
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" <bluca@debian.org> 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" <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%2Fd
> oc.dpdk.org%2Fguides%2Fprog_guide%2Fport_hotplug_framework.html&d
> ata=02%7C01%7Cllouis%40vmware.com%7C3d87a5482cd046679d7608d61d68bf9a%
> 7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636728733078700911&s
> data=qBx7zyOJhTmVH0c2ZEBohmIHL6PkYwB6XPaw2epgS0E%3D&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 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
>
--
Kind regards,
Luca Boccassi
Ok, I've added it back with a debug log in v2.
On Tue, 2018-09-18 at 18:48 +0000, Louis Luo wrote:
> 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" <bluca@debian.org> 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" <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%2Fd
> >
> oc.dpdk.org%2Fguides%2Fprog_guide%2Fport_hotplug_framework.html&d
> >
> ata=02%7C01%7Cllouis%40vmware.com%7C3d87a5482cd046679d7608d61d68bf9a%
> >
> 7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636728733078700911&s
> >
> data=qBx7zyOJhTmVH0c2ZEBohmIHL6PkYwB6XPaw2epgS0E%3D&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 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
> >
>
> --
> Kind regards,
> Luca Boccassi
>
@@ -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