Re: [dpdk-dev] [PATCH] net/tap: release port upon close

[wangyunjian <wangyunjian@huawei.com>]

> -----Original Message-----
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Wednesday, September 16, 2020 11:48 PM
> To: wangyunjian <wangyunjian@huawei.com>; dev@dpdk.org
> Cc: thomas@monjalon.net; Lilijun (Jerry) <jerry.lilijun@huawei.com>; xudingke
> <xudingke@huawei.com>
> Subject: Re: [dpdk-dev] [PATCH] net/tap: release port upon close
> 
> On 9/16/2020 8:20 AM, wangyunjian wrote:
> >> -----Original Message-----
> >> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> >> Sent: Tuesday, September 15, 2020 10:53 PM
> >> To: wangyunjian <wangyunjian@huawei.com>; dev@dpdk.org
> >> Cc: thomas@monjalon.net; Lilijun (Jerry) <jerry.lilijun@huawei.com>;
> >> xudingke <xudingke@huawei.com>
> >> Subject: Re: [dpdk-dev] [PATCH] net/tap: release port upon close
> >>
> >> On 8/28/2020 1:37 PM, wangyunjian wrote:
> >>> From: Yunjian Wang <wangyunjian@huawei.com>
> >>>
> >>> Set RTE_ETH_DEV_CLOSE_REMOVE upon probe so all the private
> resources
> >>> for the port can be freed by rte_eth_dev_close().
> >>>
> >>> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> >>
> >> <...>
> >>
> >>> @@ -1040,6 +1044,9 @@ tap_dev_close(struct rte_eth_dev *dev)
> >>>   	struct pmd_process_private *process_private =
> dev->process_private;
> >>>   	struct rx_queue *rxq;
> >>>
> >>> +	if (process_private == NULL)
> >>> +		return;
> >>
> >> Why this check is required?
> >
> > When user first call 'close()' and later call 'remove()' the tap PMD,
> > in this case, the tap_dev_close() will be called twice. The second
> > call of tap_dev_close() shouldn't do any process, we can use this check to
> return immediately.
> >
> 
> 
> When first call is 'close()', it memset the 'eth_dev->data', so the in the later
> 'remove()' call, 'rte_eth_dev_allocated()' will always return NULL and 'remove()'
> will exit without calling 'close()'.
> 
> Also multiple 'close()' calls look safe, because of
> 'RTE_ETH_VALID_PORTID_OR_RET()' checks is 'rte_eth_dev_close()'.
> 
> So, as far as I can see this additional check is not needed, but please double
> check.

I have checked, you're right. This additional check is not needed.
I will remove it in V2.

Thanks,
Yunjian