diff mbox series

[v2,3/4] raw/ifpga/base: cleanup ifpga raw devices when process quit

Message ID 1601257218-6606-4-git-send-email-tianfei.zhang@intel.com (mailing list archive)
State Changes Requested, archived
Delegated to: Qi Zhang
Headers
Series raw/ifpga/base: An improvement for multi-process |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Zhang, Tianfei Sept. 28, 2020, 1:40 a.m. UTC 
  From: Wei Huang <wei.huang@intel.com>

Add function ifpga_rawdev_cleanup() to cleanup all ifpga
raw devices and register it as RTE_FINI function to make
it called after main().

Signed-off-by: Wei Huang <wei.huang@intel.com>
Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
---
 drivers/raw/ifpga/ifpga_rawdev.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Xu, Rosen Sept. 29, 2020, 1:43 a.m. UTC | #1 
Hi,

> -----Original Message-----
> From: Zhang, Tianfei <tianfei.zhang@intel.com>
> Sent: Monday, September 28, 2020 9:40
> To: dev@dpdk.org; Xu, Rosen <rosen.xu@intel.com>; Huang, Wei
> <wei.huang@intel.com>
> Cc: Zhang, Tianfei <tianfei.zhang@intel.com>
> Subject: [PATCH v2 3/4] raw/ifpga/base: cleanup ifpga raw devices when
> process quit
> 
> From: Wei Huang <wei.huang@intel.com>
> 
> Add function ifpga_rawdev_cleanup() to cleanup all ifpga raw devices and
> register it as RTE_FINI function to make it called after main().
> 
> Signed-off-by: Wei Huang <wei.huang@intel.com>
> Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
> ---
>  drivers/raw/ifpga/ifpga_rawdev.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/raw/ifpga/ifpga_rawdev.c
> b/drivers/raw/ifpga/ifpga_rawdev.c
> index 98b02b5fa..1bc500a2a 100644
> --- a/drivers/raw/ifpga/ifpga_rawdev.c
> +++ b/drivers/raw/ifpga/ifpga_rawdev.c
> @@ -1609,6 +1609,26 @@
> RTE_PMD_REGISTER_PCI_TABLE(ifpga_rawdev_pci_driver,
> rte_ifpga_rawdev_pmd);
> RTE_PMD_REGISTER_KMOD_DEP(ifpga_rawdev_pci_driver, "* igb_uio |
> uio_pci_generic | vfio-pci");  RTE_LOG_REGISTER(ifpga_rawdev_logtype,
> driver.raw.init, NOTICE);
> 
> +RTE_FINI(ifpga_rawdev_cleanup)
> +{
> +	struct ifpga_rawdev *dev;
> +	struct opae_adapter *adapter;
> +	unsigned int i;
> +
> +	for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
> +		dev = &ifpga_rawdevices[i];
> +		if (dev->rawdev) {
> +			adapter = ifpga_rawdev_get_priv(dev->rawdev);
> +			if (adapter) {
> +				opae_adapter_destroy(adapter);
> +				opae_adapter_data_free(adapter->data);
> +			}
> +			rte_rawdev_pmd_release(dev->rawdev);
> +			dev->rawdev = NULL;
> +		}
> +	}
> +}
> +
>  static const char * const valid_args[] = {
>  #define IFPGA_ARG_NAME         "ifpga"
>  	IFPGA_ARG_NAME,
> --
> 2.17.1

Acked-by: Rosen Xu <rosen.xu@intel.com>
Qi Zhang Oct. 15, 2020, 1:15 p.m. UTC | #2 
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Xu, Rosen
> Sent: Tuesday, September 29, 2020 9:43 AM
> To: Zhang, Tianfei <tianfei.zhang@intel.com>; dev@dpdk.org; Huang, Wei
> <wei.huang@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 3/4] raw/ifpga/base: cleanup ifpga raw
> devices when process quit
> 
> Hi,
> 
> > -----Original Message-----
> > From: Zhang, Tianfei <tianfei.zhang@intel.com>
> > Sent: Monday, September 28, 2020 9:40
> > To: dev@dpdk.org; Xu, Rosen <rosen.xu@intel.com>; Huang, Wei
> > <wei.huang@intel.com>
> > Cc: Zhang, Tianfei <tianfei.zhang@intel.com>
> > Subject: [PATCH v2 3/4] raw/ifpga/base: cleanup ifpga raw devices when
> > process quit
> >
> > From: Wei Huang <wei.huang@intel.com>
> >
> > Add function ifpga_rawdev_cleanup() to cleanup all ifpga raw devices
> > and register it as RTE_FINI function to make it called after main().
> >
> > Signed-off-by: Wei Huang <wei.huang@intel.com>
> > Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
> > ---
> >  drivers/raw/ifpga/ifpga_rawdev.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/drivers/raw/ifpga/ifpga_rawdev.c
> > b/drivers/raw/ifpga/ifpga_rawdev.c
> > index 98b02b5fa..1bc500a2a 100644
> > --- a/drivers/raw/ifpga/ifpga_rawdev.c
> > +++ b/drivers/raw/ifpga/ifpga_rawdev.c
> > @@ -1609,6 +1609,26 @@
> > RTE_PMD_REGISTER_PCI_TABLE(ifpga_rawdev_pci_driver,
> > rte_ifpga_rawdev_pmd);
> > RTE_PMD_REGISTER_KMOD_DEP(ifpga_rawdev_pci_driver, "* igb_uio |
> > uio_pci_generic | vfio-pci");  RTE_LOG_REGISTER(ifpga_rawdev_logtype,
> > driver.raw.init, NOTICE);
> >
> > +RTE_FINI(ifpga_rawdev_cleanup)
> > +{
> > +	struct ifpga_rawdev *dev;
> > +	struct opae_adapter *adapter;
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
> > +		dev = &ifpga_rawdevices[i];
> > +		if (dev->rawdev) {
> > +			adapter = ifpga_rawdev_get_priv(dev->rawdev);
> > +			if (adapter) {
> > +				opae_adapter_destroy(adapter);
> > +				opae_adapter_data_free(adapter->data);
> > +			}
> > +			rte_rawdev_pmd_release(dev->rawdev);
> > +			dev->rawdev = NULL;
> > +		}
> > +	}
> > +}
> > +
> >  static const char * const valid_args[] = {
> >  #define IFPGA_ARG_NAME         "ifpga"
> >  	IFPGA_ARG_NAME,
> > --
> > 2.17.1
> 
> Acked-by: Rosen Xu <rosen.xu@intel.com>

Applied to dpdk-next-net-intel.

Thanks
Qi
Ferruh Yigit Oct. 15, 2020, 6:57 p.m. UTC | #3 
On 9/28/2020 2:40 AM, Tianfei zhang wrote:
> From: Wei Huang <wei.huang@intel.com>
> 
> Add function ifpga_rawdev_cleanup() to cleanup all ifpga
> raw devices and register it as RTE_FINI function to make
> it called after main().
> 
> Signed-off-by: Wei Huang <wei.huang@intel.com>
> Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
> ---
>   drivers/raw/ifpga/ifpga_rawdev.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/raw/ifpga/ifpga_rawdev.c b/drivers/raw/ifpga/ifpga_rawdev.c
> index 98b02b5fa..1bc500a2a 100644
> --- a/drivers/raw/ifpga/ifpga_rawdev.c
> +++ b/drivers/raw/ifpga/ifpga_rawdev.c
> @@ -1609,6 +1609,26 @@ RTE_PMD_REGISTER_PCI_TABLE(ifpga_rawdev_pci_driver, rte_ifpga_rawdev_pmd);
>   RTE_PMD_REGISTER_KMOD_DEP(ifpga_rawdev_pci_driver, "* igb_uio | uio_pci_generic | vfio-pci");
>   RTE_LOG_REGISTER(ifpga_rawdev_logtype, driver.raw.init, NOTICE);
>   
> +RTE_FINI(ifpga_rawdev_cleanup)
> +{
> +	struct ifpga_rawdev *dev;
> +	struct opae_adapter *adapter;
> +	unsigned int i;
> +
> +	for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
> +		dev = &ifpga_rawdevices[i];
> +		if (dev->rawdev) {
> +			adapter = ifpga_rawdev_get_priv(dev->rawdev);
> +			if (adapter) {
> +				opae_adapter_destroy(adapter);
> +				opae_adapter_data_free(adapter->data);
> +			}
> +			rte_rawdev_pmd_release(dev->rawdev);
> +			dev->rawdev = NULL;
> +		}
> +	}
> +}
> +
>   static const char * const valid_args[] = {
>   #define IFPGA_ARG_NAME         "ifpga"
>   	IFPGA_ARG_NAME,
> 

Not sure about each driver adding destructors for cleanup, instead better to 
have a proper cleanup path for application exit.

What is the motivation of the patch, does not cleaning has negative impact, 
something like not able to start app again if it is not cleaned properly etc...

If there is negative impact but to be able to clean driver properly, I would 
suggest doing this by improving exit path.
Zhang, Tianfei Oct. 16, 2020, 5:54 a.m. UTC | #4 
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: 2020年10月16日 2:57
> To: Zhang, Tianfei <tianfei.zhang@intel.com>; dev@dpdk.org; Xu, Rosen
> <rosen.xu@intel.com>; Huang, Wei <wei.huang@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 3/4] raw/ifpga/base: cleanup ifpga raw
> devices when process quit
> 
> On 9/28/2020 2:40 AM, Tianfei zhang wrote:
> > From: Wei Huang <wei.huang@intel.com>
> >
> > Add function ifpga_rawdev_cleanup() to cleanup all ifpga raw devices
> > and register it as RTE_FINI function to make it called after main().
> >
> > Signed-off-by: Wei Huang <wei.huang@intel.com>
> > Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
> > ---
> >   drivers/raw/ifpga/ifpga_rawdev.c | 20 ++++++++++++++++++++
> >   1 file changed, 20 insertions(+)
> >
> > diff --git a/drivers/raw/ifpga/ifpga_rawdev.c
> > b/drivers/raw/ifpga/ifpga_rawdev.c
> > index 98b02b5fa..1bc500a2a 100644
> > --- a/drivers/raw/ifpga/ifpga_rawdev.c
> > +++ b/drivers/raw/ifpga/ifpga_rawdev.c
> > @@ -1609,6 +1609,26 @@
> RTE_PMD_REGISTER_PCI_TABLE(ifpga_rawdev_pci_driver,
> rte_ifpga_rawdev_pmd);
> >   RTE_PMD_REGISTER_KMOD_DEP(ifpga_rawdev_pci_driver, "* igb_uio |
> uio_pci_generic | vfio-pci");
> >   RTE_LOG_REGISTER(ifpga_rawdev_logtype, driver.raw.init, NOTICE);
> >
> > +RTE_FINI(ifpga_rawdev_cleanup)
> > +{
> > +	struct ifpga_rawdev *dev;
> > +	struct opae_adapter *adapter;
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
> > +		dev = &ifpga_rawdevices[i];
> > +		if (dev->rawdev) {
> > +			adapter = ifpga_rawdev_get_priv(dev->rawdev);
> > +			if (adapter) {
> > +				opae_adapter_destroy(adapter);
> > +				opae_adapter_data_free(adapter->data);
> > +			}
> > +			rte_rawdev_pmd_release(dev->rawdev);
> > +			dev->rawdev = NULL;
> > +		}
> > +	}
> > +}
> > +
> >   static const char * const valid_args[] = {
> >   #define IFPGA_ARG_NAME         "ifpga"
> >   	IFPGA_ARG_NAME,
> >
> 
> Not sure about each driver adding destructors for cleanup, instead better to
> have a proper cleanup path for application exit.
> 
> What is the motivation of the patch, does not cleaning has negative impact,
> something like not able to start app again if it is not cleaned properly etc...
> 
> If there is negative impact but to be able to clean driver properly, I would
> suggest doing this by improving exit path.

Good suggestion, in V3, we will add cleanup in rte_rawdev_ops-> dev_close ops,
For example, in ifpga driver, we will add it in ifpga_rawdev_close().
We will fix in V3.
diff mbox series

Patch

diff --git a/drivers/raw/ifpga/ifpga_rawdev.c b/drivers/raw/ifpga/ifpga_rawdev.c
index 98b02b5fa..1bc500a2a 100644
--- a/drivers/raw/ifpga/ifpga_rawdev.c
+++ b/drivers/raw/ifpga/ifpga_rawdev.c
@@ -1609,6 +1609,26 @@  RTE_PMD_REGISTER_PCI_TABLE(ifpga_rawdev_pci_driver, rte_ifpga_rawdev_pmd);
 RTE_PMD_REGISTER_KMOD_DEP(ifpga_rawdev_pci_driver, "* igb_uio | uio_pci_generic | vfio-pci");
 RTE_LOG_REGISTER(ifpga_rawdev_logtype, driver.raw.init, NOTICE);
 
+RTE_FINI(ifpga_rawdev_cleanup)
+{
+	struct ifpga_rawdev *dev;
+	struct opae_adapter *adapter;
+	unsigned int i;
+
+	for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
+		dev = &ifpga_rawdevices[i];
+		if (dev->rawdev) {
+			adapter = ifpga_rawdev_get_priv(dev->rawdev);
+			if (adapter) {
+				opae_adapter_destroy(adapter);
+				opae_adapter_data_free(adapter->data);
+			}
+			rte_rawdev_pmd_release(dev->rawdev);
+			dev->rawdev = NULL;
+		}
+	}
+}
+
 static const char * const valid_args[] = {
 #define IFPGA_ARG_NAME         "ifpga"
 	IFPGA_ARG_NAME,