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

[Luca Boccassi <bluca@debian.org>]

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

-- 
Kind regards,
Luca Boccassi