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" <bl...@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" <bl...@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 bl...@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: sta...@dpdk.org > > > > > > Signed-off-by: Brian Russell <bruss...@brocade.com> > > > Signed-off-by: Luca Boccassi <bl...@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
