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

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

[SteveX Yang]

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: SteveX Yang <stevex.yang@intel.com>
---
 drivers/net/iavf/iavf_ethdev.c | 43 ++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c
index c3aa4cd72..ebff4d40b 100644
--- a/drivers/net/iavf/iavf_ethdev.c
+++ b/drivers/net/iavf/iavf_ethdev.c
@@ -1362,6 +1362,11 @@ iavf_dev_init(struct rte_eth_dev *eth_dev)
 	adapter->eth_dev = eth_dev;
 	adapter->stopped = 1;
 
+	/* Pass the information to the rte_eth_dev_close() that it should also
+	 * release the private port resources.
+	 */
+	eth_dev->data->dev_flags |= RTE_ETH_DEV_CLOSE_REMOVE;
+
 	if (iavf_init_vf(eth_dev) != 0) {
 		PMD_INIT_LOG(ERR, "Init vf failed");
 		return -1;
@@ -1416,6 +1421,7 @@ iavf_dev_close(struct rte_eth_dev *dev)
 	struct rte_intr_handle *intr_handle = &pci_dev->intr_handle;
 	struct iavf_adapter *adapter =
 		IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
+	struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
 
 	iavf_dev_stop(dev);
 	iavf_flow_flush(dev, NULL);
@@ -1428,20 +1434,21 @@ iavf_dev_close(struct rte_eth_dev *dev)
 	rte_intr_callback_unregister(intr_handle,
 				     iavf_dev_interrupt_handler, dev);
 	iavf_disable_irq0(hw);
-}
-
-static int
-iavf_dev_uninit(struct rte_eth_dev *dev)
-{
-	struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
-
-	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
-		return -EPERM;
 
 	dev->dev_ops = NULL;
 	dev->rx_pkt_burst = NULL;
 	dev->tx_pkt_burst = NULL;
-	iavf_dev_close(dev);
+
+	if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_RSS_PF) {
+		if (vf->rss_lut) {
+			rte_free(vf->rss_lut);
+			vf->rss_lut = NULL;
+		}
+		if (vf->rss_key) {
+			rte_free(vf->rss_key);
+			vf->rss_key = NULL;
+		}
+	}
 
 	rte_free(vf->vf_res);
 	vf->vsi_res = NULL;
@@ -1449,15 +1456,15 @@ iavf_dev_uninit(struct rte_eth_dev *dev)
 
 	rte_free(vf->aq_resp);
 	vf->aq_resp = NULL;
+}
 
-	if (vf->rss_lut) {
-		rte_free(vf->rss_lut);
-		vf->rss_lut = NULL;
-	}
-	if (vf->rss_key) {
-		rte_free(vf->rss_key);
-		vf->rss_key = NULL;
-	}
+static int
+iavf_dev_uninit(struct rte_eth_dev *dev)
+{
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return -EPERM;
+
+	iavf_dev_close(dev);
 
 	return 0;
 }
-- 
2.17.1

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

[Zhang, Qi Z]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of SteveX Yang
> Sent: Tuesday, August 11, 2020 3:28 PM
> To: Yang, Qiming <qiming.yang@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> dev@dpdk.org
> Cc: Yang, SteveX <stevex.yang@intel.com>
> Subject: [dpdk-dev] [PATCH] net/iavf: release port upon close
> 
> 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: SteveX Yang <stevex.yang@intel.com>

Acked-by: Qi Zhang <qi.z.zhang@intel.com>
Applied to dpdk-next-net-intel.

Thanks
Qi

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

[Thomas Monjalon]

Hi,

SteveX Yang <stevex.yang@intel.com> wrote:
> 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: SteveX Yang <stevex.yang@intel.com>

I guess the X is not part of your name.

[...]
> -static int
> -iavf_dev_uninit(struct rte_eth_dev *dev)
> -{
> -       struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> -
> -       if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> -               return -EPERM;

All the code below is moved from iavf_dev_uninit() where it was
restricted to the primary process, to iavf_dev_close() which can be
called from the primary process.
It looks inconsistent.

> 
>         dev->dev_ops = NULL;
>         dev->rx_pkt_burst = NULL;
>         dev->tx_pkt_burst = NULL;
> 
> -       iavf_dev_close(dev);
> +
> +       if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_RSS_PF) {
> +               if (vf->rss_lut) {
> +                       rte_free(vf->rss_lut);
> +                       vf->rss_lut = NULL;
> +               }
> +               if (vf->rss_key) {
> +                       rte_free(vf->rss_key);
> +                       vf->rss_key = NULL;
> +               }
> +       }
> 
>         rte_free(vf->vf_res);
>         vf->vsi_res = NULL;
> 
> @@ -1449,15 +1456,15 @@ iavf_dev_uninit(struct rte_eth_dev *dev)
> 
>         rte_free(vf->aq_resp);
>         vf->aq_resp = NULL;
> 
> +}
> 
> -       if (vf->rss_lut) {
> -               rte_free(vf->rss_lut);
> -               vf->rss_lut = NULL;
> -       }
> -       if (vf->rss_key) {
> -               rte_free(vf->rss_key);
> -               vf->rss_key = NULL;
> -       }
> +static int
> +iavf_dev_uninit(struct rte_eth_dev *dev)
> +{
> +       if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +               return -EPERM;
> +
> +       iavf_dev_close(dev);

Are you sure about what should be freed in the secondary process?
If iavf_dev_close() should not be called by the secondary process,
you should move the check inside the function,
because iavf_dev_close() is also called by rte_eth_dev_close().

> 
>         return 0;
>  
>  }

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

[Thomas Monjalon]

As you may notice, I have included a slightly modified version
of this patch in my series in order to cover the full picture:
	https://patches.dpdk.org/patch/77559/

Feel free to continue improving your patch in this thread or the other,
as you prefer, as long as the secondary process issue is adressed.

Thanks


13/09/2020 10:53, Thomas Monjalon:
> Hi,
> 
> SteveX Yang <stevex.yang@intel.com> wrote:
> > 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: SteveX Yang <stevex.yang@intel.com>
> 
> I guess the X is not part of your name.
> 
> [...]
> > -static int
> > -iavf_dev_uninit(struct rte_eth_dev *dev)
> > -{
> > -       struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> > -
> > -       if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> > -               return -EPERM;
> 
> All the code below is moved from iavf_dev_uninit() where it was
> restricted to the primary process, to iavf_dev_close() which can be
> called from the primary process.
> It looks inconsistent.
> 
> > 
> >         dev->dev_ops = NULL;
> >         dev->rx_pkt_burst = NULL;
> >         dev->tx_pkt_burst = NULL;
> > 
> > -       iavf_dev_close(dev);
> > +
> > +       if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_RSS_PF) {
> > +               if (vf->rss_lut) {
> > +                       rte_free(vf->rss_lut);
> > +                       vf->rss_lut = NULL;
> > +               }
> > +               if (vf->rss_key) {
> > +                       rte_free(vf->rss_key);
> > +                       vf->rss_key = NULL;
> > +               }
> > +       }
> > 
> >         rte_free(vf->vf_res);
> >         vf->vsi_res = NULL;
> > 
> > @@ -1449,15 +1456,15 @@ iavf_dev_uninit(struct rte_eth_dev *dev)
> > 
> >         rte_free(vf->aq_resp);
> >         vf->aq_resp = NULL;
> > 
> > +}
> > 
> > -       if (vf->rss_lut) {
> > -               rte_free(vf->rss_lut);
> > -               vf->rss_lut = NULL;
> > -       }
> > -       if (vf->rss_key) {
> > -               rte_free(vf->rss_key);
> > -               vf->rss_key = NULL;
> > -       }
> > +static int
> > +iavf_dev_uninit(struct rte_eth_dev *dev)
> > +{
> > +       if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> > +               return -EPERM;
> > +
> > +       iavf_dev_close(dev);
> 
> Are you sure about what should be freed in the secondary process?
> If iavf_dev_close() should not be called by the secondary process,
> you should move the check inside the function,
> because iavf_dev_close() is also called by rte_eth_dev_close().
> 
> > 
> >         return 0;
> >  
> >  }