[2/2] net/cxgbe: release port resources during port close
diff mbox series

Message ID ceb95db6242808b5f5e568a0da60c91a3bc2d9a6.1598979347.git.rahul.lakkireddy@chelsio.com
State Accepted
Delegated to: Ferruh Yigit
Headers show
Series
  • net/cxgbe: release port resources during port close
Related show

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/travis-robot success Travis build: passed
ci/iol-broadcom-Performance success Performance Testing PASS

Commit Message

Rahul Lakkireddy Sept. 1, 2020, 5:16 p.m. UTC
Enable RTE_ETH_DEV_CLOSE_REMOVE during PCI probe for all ports
enumerated under the PF. Free up the underlying port Virtual
Identifier (VI) and associated resources during port close.
Once all the ports under the PF are closed, free up the PF-wide
shared resources. Invoke port close function of all ports under
the PF, in PCI remove too.

Signed-off-by: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
---
 drivers/net/cxgbe/cxgbe_ethdev.c   | 28 ++++++++++++++++++++++++----
 drivers/net/cxgbe/cxgbe_main.c     | 12 ++----------
 drivers/net/cxgbe/cxgbevf_ethdev.c |  8 +++++---
 drivers/net/cxgbe/cxgbevf_main.c   |  2 ++
 4 files changed, 33 insertions(+), 17 deletions(-)

Comments

Ferruh Yigit Sept. 17, 2020, 12:46 p.m. UTC | #1
On 9/1/2020 6:16 PM, Rahul Lakkireddy wrote:
> Enable RTE_ETH_DEV_CLOSE_REMOVE during PCI probe for all ports
> enumerated under the PF. Free up the underlying port Virtual
> Identifier (VI) and associated resources during port close.
> Once all the ports under the PF are closed, free up the PF-wide
> shared resources. Invoke port close function of all ports under
> the PF, in PCI remove too.
> 
> Signed-off-by: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>

<...>

> @@ -1204,11 +1222,13 @@ static int eth_cxgbe_dev_init(struct rte_eth_dev *eth_dev)
>   
>   static int eth_cxgbe_dev_uninit(struct rte_eth_dev *eth_dev)
>   {
> -	struct port_info *pi = eth_dev->data->dev_private;
> -	struct adapter *adap = pi->adapter;
> +	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
> +	uint16_t port_id;
>   
>   	/* Free up other ports and all resources */
> -	cxgbe_close(adap);
> +	RTE_ETH_FOREACH_DEV_OF(port_id, &pci_dev->device)
> +		rte_eth_dev_close(port_id);

Is there a reason to call the ethdev wrapper API here? Why not calling 
'cxgbe_dev_close()' directly?
Rahul Lakkireddy Sept. 18, 2020, 1:18 p.m. UTC | #2
On Thursday, September 09/17/20, 2020 at 13:46:24 +0100, Ferruh Yigit wrote:
> On 9/1/2020 6:16 PM, Rahul Lakkireddy wrote:
> >Enable RTE_ETH_DEV_CLOSE_REMOVE during PCI probe for all ports
> >enumerated under the PF. Free up the underlying port Virtual
> >Identifier (VI) and associated resources during port close.
> >Once all the ports under the PF are closed, free up the PF-wide
> >shared resources. Invoke port close function of all ports under
> >the PF, in PCI remove too.
> >
> >Signed-off-by: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
> 
> <...>
> 
> >@@ -1204,11 +1222,13 @@ static int eth_cxgbe_dev_init(struct rte_eth_dev *eth_dev)
> >  static int eth_cxgbe_dev_uninit(struct rte_eth_dev *eth_dev)
> >  {
> >-	struct port_info *pi = eth_dev->data->dev_private;
> >-	struct adapter *adap = pi->adapter;
> >+	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
> >+	uint16_t port_id;
> >  	/* Free up other ports and all resources */
> >-	cxgbe_close(adap);
> >+	RTE_ETH_FOREACH_DEV_OF(port_id, &pci_dev->device)
> >+		rte_eth_dev_close(port_id);
> 
> Is there a reason to call the ethdev wrapper API here? Why not calling
> 'cxgbe_dev_close()' directly?


Chelsio NICs enumerate all physical ports under a single Physical
Function 4 (PF4). When PCI remove is invoked without
rte_eth_dev_close() first, the rte_eth_dev_release_port() is only
invoked for the first port instantiated on PF4. So, to ensure
rte_eth_dev_release_port() is invoked for the remaining ports,
we're explicitly calling rte_eth_dev_close() for all ports here.

Here's a testpmd example that leads to the problem.

testpmd> port stop all
Stopping ports...
Checking link statuses...
Done
testpmd> device detach 0000:03:00.4
Removing a device...
Port 0 is now closed
Port 1 is now closed
Device 0000:03:00.4 is detached
Now total ports is 1
Done
testpmd> 

The Chelsio NIC has 2 ports enumerated under 0000:03:00.4. When PCI
remove is invoked, rte_eth_dev_release_port() is only invoked once
for the first port. You can see that the other port is still active,
as printed by "Now total ports is 1".

The sequence of calls is as follows from driver's PCI remove:

eth_cxgbe_pci_remove()
rte_eth_dev_pci_generic_remove()
rte_eth_dev_pci_release()
rte_eth_dev_release_port() <--- Only invoked once

If I explicitly invoke rte_eth_dev_close() in PCI remove for all
ports, as done in this patch, rte_eth_dev_release_port() is invoked
for both the ports.

testpmd> port stop all
Stopping ports...
Checking link statuses...
Done
testpmd> device detach 0000:03:00.4
Removing a device...
Port 0 is now closed
Port 1 is now closed
Device 0000:03:00.4 is detached
Now total ports is 0
Done

Now, both ports are released properly as printed by "Now total ports
is 0".

Thanks,
Rahul
Ferruh Yigit Sept. 18, 2020, 2:38 p.m. UTC | #3
On 9/18/2020 2:18 PM, Rahul Lakkireddy wrote:
> On Thursday, September 09/17/20, 2020 at 13:46:24 +0100, Ferruh Yigit wrote:
>> On 9/1/2020 6:16 PM, Rahul Lakkireddy wrote:
>>> Enable RTE_ETH_DEV_CLOSE_REMOVE during PCI probe for all ports
>>> enumerated under the PF. Free up the underlying port Virtual
>>> Identifier (VI) and associated resources during port close.
>>> Once all the ports under the PF are closed, free up the PF-wide
>>> shared resources. Invoke port close function of all ports under
>>> the PF, in PCI remove too.
>>>
>>> Signed-off-by: Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>
>>
>> <...>
>>
>>> @@ -1204,11 +1222,13 @@ static int eth_cxgbe_dev_init(struct rte_eth_dev *eth_dev)
>>>   static int eth_cxgbe_dev_uninit(struct rte_eth_dev *eth_dev)
>>>   {
>>> -	struct port_info *pi = eth_dev->data->dev_private;
>>> -	struct adapter *adap = pi->adapter;
>>> +	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
>>> +	uint16_t port_id;
>>>   	/* Free up other ports and all resources */
>>> -	cxgbe_close(adap);
>>> +	RTE_ETH_FOREACH_DEV_OF(port_id, &pci_dev->device)
>>> +		rte_eth_dev_close(port_id);
>>
>> Is there a reason to call the ethdev wrapper API here? Why not calling
>> 'cxgbe_dev_close()' directly?
> 
> 
> Chelsio NICs enumerate all physical ports under a single Physical
> Function 4 (PF4). When PCI remove is invoked without
> rte_eth_dev_close() first, the rte_eth_dev_release_port() is only
> invoked for the first port instantiated on PF4. So, to ensure
> rte_eth_dev_release_port() is invoked for the remaining ports,
> we're explicitly calling rte_eth_dev_close() for all ports here.
> 
> Here's a testpmd example that leads to the problem.
> 
> testpmd> port stop all
> Stopping ports...
> Checking link statuses...
> Done
> testpmd> device detach 0000:03:00.4
> Removing a device...
> Port 0 is now closed
> Port 1 is now closed
> Device 0000:03:00.4 is detached
> Now total ports is 1
> Done
> testpmd>
> 
> The Chelsio NIC has 2 ports enumerated under 0000:03:00.4. When PCI
> remove is invoked, rte_eth_dev_release_port() is only invoked once
> for the first port. You can see that the other port is still active,
> as printed by "Now total ports is 1".
> 
> The sequence of calls is as follows from driver's PCI remove:
> 
> eth_cxgbe_pci_remove()
> rte_eth_dev_pci_generic_remove()
> rte_eth_dev_pci_release()
> rte_eth_dev_release_port() <--- Only invoked once
> 
> If I explicitly invoke rte_eth_dev_close() in PCI remove for all
> ports, as done in this patch, rte_eth_dev_release_port() is invoked
> for both the ports.
> 
> testpmd> port stop all
> Stopping ports...
> Checking link statuses...
> Done
> testpmd> device detach 0000:03:00.4
> Removing a device...
> Port 0 is now closed
> Port 1 is now closed
> Device 0000:03:00.4 is detached
> Now total ports is 0
> Done
> 
> Now, both ports are released properly as printed by "Now total ports
> is 0".
> 

Hi Rahul,

Thanks for detailed explanation.

Patch
diff mbox series

diff --git a/drivers/net/cxgbe/cxgbe_ethdev.c b/drivers/net/cxgbe/cxgbe_ethdev.c
index 913af2df7..60d325723 100644
--- a/drivers/net/cxgbe/cxgbe_ethdev.c
+++ b/drivers/net/cxgbe/cxgbe_ethdev.c
@@ -317,16 +317,34 @@  int cxgbe_dev_mtu_set(struct rte_eth_dev *eth_dev, uint16_t mtu)
  */
 void cxgbe_dev_close(struct rte_eth_dev *eth_dev)
 {
-	struct port_info *pi = eth_dev->data->dev_private;
+	struct port_info *temp_pi, *pi = eth_dev->data->dev_private;
 	struct adapter *adapter = pi->adapter;
+	u8 i;
 
 	CXGBE_FUNC_TRACE();
 
 	if (!(adapter->flags & FULL_INIT_DONE))
 		return;
 
+	if (!pi->viid)
+		return;
+
 	cxgbe_down(pi);
 	t4_sge_eth_release_queues(pi);
+	t4_free_vi(adapter, adapter->mbox, adapter->pf, 0, pi->viid);
+	pi->viid = 0;
+
+	/* Free up the adapter-wide resources only after all the ports
+	 * under this PF have been closed.
+	 */
+	for_each_port(adapter, i) {
+		temp_pi = adap2pinfo(adapter, i);
+		if (temp_pi->viid)
+			return;
+	}
+
+	cxgbe_close(adapter);
+	rte_free(adapter);
 }
 
 /* Start the device.
@@ -1204,11 +1222,13 @@  static int eth_cxgbe_dev_init(struct rte_eth_dev *eth_dev)
 
 static int eth_cxgbe_dev_uninit(struct rte_eth_dev *eth_dev)
 {
-	struct port_info *pi = eth_dev->data->dev_private;
-	struct adapter *adap = pi->adapter;
+	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
+	uint16_t port_id;
 
 	/* Free up other ports and all resources */
-	cxgbe_close(adap);
+	RTE_ETH_FOREACH_DEV_OF(port_id, &pci_dev->device)
+		rte_eth_dev_close(port_id);
+
 	return 0;
 }
 
diff --git a/drivers/net/cxgbe/cxgbe_main.c b/drivers/net/cxgbe/cxgbe_main.c
index 2656369c5..d0a64229c 100644
--- a/drivers/net/cxgbe/cxgbe_main.c
+++ b/drivers/net/cxgbe/cxgbe_main.c
@@ -1975,9 +1975,6 @@  int cxgbe_down(struct port_info *pi)
  */
 void cxgbe_close(struct adapter *adapter)
 {
-	struct port_info *pi;
-	int i;
-
 	if (adapter->flags & FULL_INIT_DONE) {
 		tid_free(&adapter->tids);
 		t4_cleanup_mpstcam(adapter);
@@ -1988,13 +1985,6 @@  void cxgbe_close(struct adapter *adapter)
 			t4_intr_disable(adapter);
 		t4_sge_tx_monitor_stop(adapter);
 		t4_free_sge_resources(adapter);
-		for_each_port(adapter, i) {
-			pi = adap2pinfo(adapter, i);
-			if (pi->viid != 0)
-				t4_free_vi(adapter, adapter->mbox,
-					   adapter->pf, 0, pi->viid);
-			rte_eth_dev_release_port(pi->eth_dev);
-		}
 		adapter->flags &= ~FULL_INIT_DONE;
 	}
 
@@ -2156,6 +2146,8 @@  int cxgbe_probe(struct adapter *adapter)
 			goto out_free;
 		}
 
+		pi->eth_dev->data->dev_flags |= RTE_ETH_DEV_CLOSE_REMOVE;
+
 		if (i > 0) {
 			/* First port will be notified by upper layer */
 			rte_eth_dev_probing_finish(eth_dev);
diff --git a/drivers/net/cxgbe/cxgbevf_ethdev.c b/drivers/net/cxgbe/cxgbevf_ethdev.c
index 4165ba986..b3c885d6e 100644
--- a/drivers/net/cxgbe/cxgbevf_ethdev.c
+++ b/drivers/net/cxgbe/cxgbevf_ethdev.c
@@ -181,11 +181,13 @@  static int eth_cxgbevf_dev_init(struct rte_eth_dev *eth_dev)
 
 static int eth_cxgbevf_dev_uninit(struct rte_eth_dev *eth_dev)
 {
-	struct port_info *pi = eth_dev->data->dev_private;
-	struct adapter *adap = pi->adapter;
+	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
+	uint16_t port_id;
 
 	/* Free up other ports and all resources */
-	cxgbe_close(adap);
+	RTE_ETH_FOREACH_DEV_OF(port_id, &pci_dev->device)
+		rte_eth_dev_close(port_id);
+
 	return 0;
 }
 
diff --git a/drivers/net/cxgbe/cxgbevf_main.c b/drivers/net/cxgbe/cxgbevf_main.c
index 66fb92375..9fe0ec6f6 100644
--- a/drivers/net/cxgbe/cxgbevf_main.c
+++ b/drivers/net/cxgbe/cxgbevf_main.c
@@ -261,6 +261,8 @@  int cxgbevf_probe(struct adapter *adapter)
 			goto out_free;
 		}
 
+		pi->eth_dev->data->dev_flags |= RTE_ETH_DEV_CLOSE_REMOVE;
+
 		if (i > 0) {
 			/* First port will be notified by upper layer */
 			rte_eth_dev_probing_finish(eth_dev);