diff mbox series

[v1] raw/ifpga: fix ifpga devices cleanup function

Message ID 20220126032953.24695-1-wei.huang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Qi Zhang
Headers
Series [v1] raw/ifpga: fix ifpga devices cleanup function |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/github-robot: build success github build: passed
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-abi-testing warning Testing issues
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS

Commit Message

Wei Huang Jan. 26, 2022, 3:29 a.m. UTC 
  Use rte_dev_remove() to replace rte_rawdev_pmd_release()
in ifpga_rawdev_cleanup(), resources occupied by ifpga
raw devices such as threads can be released correctly.

Fixes: f724a802 ("raw/ifpga: add miscellaneous APIs")

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

Comments

Zhang, Tianfei Jan. 26, 2022, 6:57 a.m. UTC | #1 
> -----Original Message-----
> From: Huang, Wei <wei.huang@intel.com>
> Sent: Wednesday, January 26, 2022 11:30 AM
> To: dev@dpdk.org; Xu, Rosen <rosen.xu@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Cc: stable@dpdk.org; Zhang, Tianfei <tianfei.zhang@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Huang, Wei <wei.huang@intel.com>
> Subject: [PATCH v1] raw/ifpga: fix ifpga devices cleanup function
> 
> Use rte_dev_remove() to replace rte_rawdev_pmd_release() in
> ifpga_rawdev_cleanup(), resources occupied by ifpga raw devices such as
> threads can be released correctly.
> 
> Fixes: f724a802 ("raw/ifpga: add miscellaneous APIs")
> 
> Signed-off-by: Wei Huang <wei.huang@intel.com>
> ---
>  drivers/raw/ifpga/ifpga_rawdev.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/raw/ifpga/ifpga_rawdev.c
> b/drivers/raw/ifpga/ifpga_rawdev.c
> index fdf3c23..88c38aa 100644
> --- a/drivers/raw/ifpga/ifpga_rawdev.c
> +++ b/drivers/raw/ifpga/ifpga_rawdev.c
> @@ -1787,12 +1787,14 @@ int ifpga_rawdev_partial_reconfigure(struct
> rte_rawdev *dev, int port,  void ifpga_rawdev_cleanup(void)  {
>  	struct ifpga_rawdev *dev;
> +	struct rte_rawdev *rdev;
>  	unsigned int i;
> 
>  	for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
>  		dev = &ifpga_rawdevices[i];
>  		if (dev->rawdev) {
> -			rte_rawdev_pmd_release(dev->rawdev);
> +			rdev = dev->rawdev;
> +			rte_dev_remove(rdev->device);
>  			dev->rawdev = NULL;
>  		}
>  	}
> --

It looks good for me.

Acked-by: Tianfei Zhang <tianfei.zhang@intel.com>
Xu, Rosen Jan. 26, 2022, 7:07 a.m. UTC | #2 
Hi,

> -----Original Message-----
> From: Huang, Wei <wei.huang@intel.com>
> Sent: Wednesday, January 26, 2022 11:30
> To: dev@dpdk.org; Xu, Rosen <rosen.xu@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Cc: stable@dpdk.org; Zhang, Tianfei <tianfei.zhang@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Huang, Wei <wei.huang@intel.com>
> Subject: [PATCH v1] raw/ifpga: fix ifpga devices cleanup function
> 
> Use rte_dev_remove() to replace rte_rawdev_pmd_release() in
> ifpga_rawdev_cleanup(), resources occupied by ifpga raw devices such as
> threads can be released correctly.
> 
> Fixes: f724a802 ("raw/ifpga: add miscellaneous APIs")
> 
> Signed-off-by: Wei Huang <wei.huang@intel.com>
> ---
>  drivers/raw/ifpga/ifpga_rawdev.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/raw/ifpga/ifpga_rawdev.c
> b/drivers/raw/ifpga/ifpga_rawdev.c
> index fdf3c23..88c38aa 100644
> --- a/drivers/raw/ifpga/ifpga_rawdev.c
> +++ b/drivers/raw/ifpga/ifpga_rawdev.c
> @@ -1787,12 +1787,14 @@ int ifpga_rawdev_partial_reconfigure(struct
> rte_rawdev *dev, int port,  void ifpga_rawdev_cleanup(void)  {
>  	struct ifpga_rawdev *dev;
> +	struct rte_rawdev *rdev;
>  	unsigned int i;
> 
>  	for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
>  		dev = &ifpga_rawdevices[i];
>  		if (dev->rawdev) {
> -			rte_rawdev_pmd_release(dev->rawdev);
> +			rdev = dev->rawdev;
> +			rte_dev_remove(rdev->device);
>  			dev->rawdev = NULL;
>  		}
>  	}
> --
> 1.8.3.1

Acked-by: Rosen Xu <rosen.xu@intel.com>
Qi Zhang Jan. 26, 2022, 9:46 a.m. UTC | #3 
> -----Original Message-----
> From: Xu, Rosen <rosen.xu@intel.com>
> Sent: Wednesday, January 26, 2022 3:08 PM
> To: Huang, Wei <wei.huang@intel.com>; dev@dpdk.org; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Cc: stable@dpdk.org; Zhang, Tianfei <tianfei.zhang@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>
> Subject: RE: [PATCH v1] raw/ifpga: fix ifpga devices cleanup function
> 
> Hi,
> 
> > -----Original Message-----
> > From: Huang, Wei <wei.huang@intel.com>
> > Sent: Wednesday, January 26, 2022 11:30
> > To: dev@dpdk.org; Xu, Rosen <rosen.xu@intel.com>; Zhang, Qi Z
> > <qi.z.zhang@intel.com>
> > Cc: stable@dpdk.org; Zhang, Tianfei <tianfei.zhang@intel.com>; Yigit,
> > Ferruh <ferruh.yigit@intel.com>; Huang, Wei <wei.huang@intel.com>
> > Subject: [PATCH v1] raw/ifpga: fix ifpga devices cleanup function
> >
> > Use rte_dev_remove() to replace rte_rawdev_pmd_release() in
> > ifpga_rawdev_cleanup(), resources occupied by ifpga raw devices such
> > as threads can be released correctly.
> >
> > Fixes: f724a802 ("raw/ifpga: add miscellaneous APIs")
> >
> > Signed-off-by: Wei Huang <wei.huang@intel.com>
> > ---
> >  drivers/raw/ifpga/ifpga_rawdev.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/raw/ifpga/ifpga_rawdev.c
> > b/drivers/raw/ifpga/ifpga_rawdev.c
> > index fdf3c23..88c38aa 100644
> > --- a/drivers/raw/ifpga/ifpga_rawdev.c
> > +++ b/drivers/raw/ifpga/ifpga_rawdev.c
> > @@ -1787,12 +1787,14 @@ int ifpga_rawdev_partial_reconfigure(struct
> > rte_rawdev *dev, int port,  void ifpga_rawdev_cleanup(void)  {
> >  	struct ifpga_rawdev *dev;
> > +	struct rte_rawdev *rdev;
> >  	unsigned int i;
> >
> >  	for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
> >  		dev = &ifpga_rawdevices[i];
> >  		if (dev->rawdev) {
> > -			rte_rawdev_pmd_release(dev->rawdev);
> > +			rdev = dev->rawdev;
> > +			rte_dev_remove(rdev->device);
> >  			dev->rawdev = NULL;
> >  		}
> >  	}
> > --
> > 1.8.3.1
> 
> Acked-by: Rosen Xu <rosen.xu@intel.com>

Applied to dpdk-next-net-intel.

Thanks
Qi
Ferruh Yigit Jan. 26, 2022, 1:25 p.m. UTC | #4 
On 1/26/2022 3:29 AM, Wei Huang wrote:
> Use rte_dev_remove() to replace rte_rawdev_pmd_release()
> in ifpga_rawdev_cleanup(), resources occupied by ifpga
> raw devices such as threads can be released correctly.
> 

As far as I understand you are fixing an issue that not all resources
are released, is this correct?
What are these not released resources?

And 'rte_rawdev_pmd_release()' rawdev API seems intended to do the cleanup,
is it expected that some resources are not freed after this call, or
should we fix that API?
If the device remove API needs to be used, what is the point of
'rte_rawdev_pmd_release()' API?

cc'ed rawdev maintainers for comment.

> Fixes: f724a802 ("raw/ifpga: add miscellaneous APIs")
> 
> Signed-off-by: Wei Huang <wei.huang@intel.com>
> ---
>   drivers/raw/ifpga/ifpga_rawdev.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/raw/ifpga/ifpga_rawdev.c b/drivers/raw/ifpga/ifpga_rawdev.c
> index fdf3c23..88c38aa 100644
> --- a/drivers/raw/ifpga/ifpga_rawdev.c
> +++ b/drivers/raw/ifpga/ifpga_rawdev.c
> @@ -1787,12 +1787,14 @@ int ifpga_rawdev_partial_reconfigure(struct rte_rawdev *dev, int port,
>   void ifpga_rawdev_cleanup(void)
>   {
>   	struct ifpga_rawdev *dev;
> +	struct rte_rawdev *rdev;
>   	unsigned int i;
>   
>   	for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
>   		dev = &ifpga_rawdevices[i];
>   		if (dev->rawdev) {
> -			rte_rawdev_pmd_release(dev->rawdev);
> +			rdev = dev->rawdev;
> +			rte_dev_remove(rdev->device);
>   			dev->rawdev = NULL;
>   		}
>   	}
Wei Huang Jan. 27, 2022, 8:57 a.m. UTC | #5 
Hi,

> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Wednesday, January 26, 2022 21:25
> To: Huang, Wei <wei.huang@intel.com>; dev@dpdk.org; Xu, Rosen
> <rosen.xu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Nipun Gupta
> <nipun.gupta@nxp.com>; Hemant Agrawal <hemant.agrawal@nxp.com>
> Cc: stable@dpdk.org; Zhang, Tianfei <tianfei.zhang@intel.com>
> Subject: Re: [PATCH v1] raw/ifpga: fix ifpga devices cleanup function
> 
> On 1/26/2022 3:29 AM, Wei Huang wrote:
> > Use rte_dev_remove() to replace rte_rawdev_pmd_release() in
> > ifpga_rawdev_cleanup(), resources occupied by ifpga raw devices such
> > as threads can be released correctly.
> >
> 
> As far as I understand you are fixing an issue that not all resources are
> released, is this correct?
> What are these not released resources?
> 
> And 'rte_rawdev_pmd_release()' rawdev API seems intended to do the
> cleanup, is it expected that some resources are not freed after this call, or
> should we fix that API?
> If the device remove API needs to be used, what is the point of
> 'rte_rawdev_pmd_release()' API?
> 
> cc'ed rawdev maintainers for comment.

Yes, this patch is to release all the resources of ifpga_rawdev after testpmd exit, the not released resources are interrupt and thread.

rte_rawdev_pmd_release implemented in ifpga_rawdev only release memory allocated by ifpga driver, that's the expected behavior.

I think it's a simple and safe way to release resources completely by calling rte_dev_remove.

> 
> > Fixes: f724a802 ("raw/ifpga: add miscellaneous APIs")
> >
> > Signed-off-by: Wei Huang <wei.huang@intel.com>
> > ---
> >   drivers/raw/ifpga/ifpga_rawdev.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/raw/ifpga/ifpga_rawdev.c
> > b/drivers/raw/ifpga/ifpga_rawdev.c
> > index fdf3c23..88c38aa 100644
> > --- a/drivers/raw/ifpga/ifpga_rawdev.c
> > +++ b/drivers/raw/ifpga/ifpga_rawdev.c
> > @@ -1787,12 +1787,14 @@ int ifpga_rawdev_partial_reconfigure(struct
> rte_rawdev *dev, int port,
> >   void ifpga_rawdev_cleanup(void)
> >   {
> >   	struct ifpga_rawdev *dev;
> > +	struct rte_rawdev *rdev;
> >   	unsigned int i;
> >
> >   	for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
> >   		dev = &ifpga_rawdevices[i];
> >   		if (dev->rawdev) {
> > -			rte_rawdev_pmd_release(dev->rawdev);
> > +			rdev = dev->rawdev;
> > +			rte_dev_remove(rdev->device);
> >   			dev->rawdev = NULL;
> >   		}
> >   	}
Ferruh Yigit Jan. 27, 2022, 12:33 p.m. UTC | #6 
On 1/27/2022 8:57 AM, Huang, Wei wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
>> Sent: Wednesday, January 26, 2022 21:25
>> To: Huang, Wei <wei.huang@intel.com>; dev@dpdk.org; Xu, Rosen
>> <rosen.xu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Nipun Gupta
>> <nipun.gupta@nxp.com>; Hemant Agrawal <hemant.agrawal@nxp.com>
>> Cc: stable@dpdk.org; Zhang, Tianfei <tianfei.zhang@intel.com>
>> Subject: Re: [PATCH v1] raw/ifpga: fix ifpga devices cleanup function
>>
>> On 1/26/2022 3:29 AM, Wei Huang wrote:
>>> Use rte_dev_remove() to replace rte_rawdev_pmd_release() in
>>> ifpga_rawdev_cleanup(), resources occupied by ifpga raw devices such
>>> as threads can be released correctly.
>>>
>>
>> As far as I understand you are fixing an issue that not all resources are
>> released, is this correct?
>> What are these not released resources?
>>
>> And 'rte_rawdev_pmd_release()' rawdev API seems intended to do the
>> cleanup, is it expected that some resources are not freed after this call, or
>> should we fix that API?
>> If the device remove API needs to be used, what is the point of
>> 'rte_rawdev_pmd_release()' API?
>>
>> cc'ed rawdev maintainers for comment.
> 
> Yes, this patch is to release all the resources of ifpga_rawdev after testpmd exit, the not released resources are interrupt and thread.
> 
> rte_rawdev_pmd_release implemented in ifpga_rawdev only release memory allocated by ifpga driver, that's the expected behavior.
> 
> I think it's a simple and safe way to release resources completely by calling rte_dev_remove.
> 

If device hot remove is better option, why 'rte_rawdev_pmd_release()' API exists?

>>
>>> Fixes: f724a802 ("raw/ifpga: add miscellaneous APIs")
>>>
>>> Signed-off-by: Wei Huang <wei.huang@intel.com>
>>> ---
>>>    drivers/raw/ifpga/ifpga_rawdev.c | 4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/raw/ifpga/ifpga_rawdev.c
>>> b/drivers/raw/ifpga/ifpga_rawdev.c
>>> index fdf3c23..88c38aa 100644
>>> --- a/drivers/raw/ifpga/ifpga_rawdev.c
>>> +++ b/drivers/raw/ifpga/ifpga_rawdev.c
>>> @@ -1787,12 +1787,14 @@ int ifpga_rawdev_partial_reconfigure(struct
>> rte_rawdev *dev, int port,
>>>    void ifpga_rawdev_cleanup(void)
>>>    {
>>>    	struct ifpga_rawdev *dev;
>>> +	struct rte_rawdev *rdev;
>>>    	unsigned int i;
>>>
>>>    	for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
>>>    		dev = &ifpga_rawdevices[i];
>>>    		if (dev->rawdev) {
>>> -			rte_rawdev_pmd_release(dev->rawdev);
>>> +			rdev = dev->rawdev;
>>> +			rte_dev_remove(rdev->device);
>>>    			dev->rawdev = NULL;
>>>    		}
>>>    	}
>
Wei Huang Jan. 28, 2022, 6:44 a.m. UTC | #7 
Hi, 

> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Thursday, January 27, 2022 20:34
> To: Huang, Wei <wei.huang@intel.com>; dev@dpdk.org; Xu, Rosen
> <rosen.xu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Nipun Gupta
> <nipun.gupta@nxp.com>; Hemant Agrawal <hemant.agrawal@nxp.com>
> Cc: stable@dpdk.org; Zhang, Tianfei <tianfei.zhang@intel.com>
> Subject: Re: [PATCH v1] raw/ifpga: fix ifpga devices cleanup function
> 
> On 1/27/2022 8:57 AM, Huang, Wei wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> >> Sent: Wednesday, January 26, 2022 21:25
> >> To: Huang, Wei <wei.huang@intel.com>; dev@dpdk.org; Xu, Rosen
> >> <rosen.xu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Nipun Gupta
> >> <nipun.gupta@nxp.com>; Hemant Agrawal <hemant.agrawal@nxp.com>
> >> Cc: stable@dpdk.org; Zhang, Tianfei <tianfei.zhang@intel.com>
> >> Subject: Re: [PATCH v1] raw/ifpga: fix ifpga devices cleanup function
> >>
> >> On 1/26/2022 3:29 AM, Wei Huang wrote:
> >>> Use rte_dev_remove() to replace rte_rawdev_pmd_release() in
> >>> ifpga_rawdev_cleanup(), resources occupied by ifpga raw devices such
> >>> as threads can be released correctly.
> >>>
> >>
> >> As far as I understand you are fixing an issue that not all resources
> >> are released, is this correct?
> >> What are these not released resources?
> >>
> >> And 'rte_rawdev_pmd_release()' rawdev API seems intended to do the
> >> cleanup, is it expected that some resources are not freed after this
> >> call, or should we fix that API?
> >> If the device remove API needs to be used, what is the point of
> >> 'rte_rawdev_pmd_release()' API?
> >>
> >> cc'ed rawdev maintainers for comment.
> >
> > Yes, this patch is to release all the resources of ifpga_rawdev after testpmd
> exit, the not released resources are interrupt and thread.
> >
> > rte_rawdev_pmd_release implemented in ifpga_rawdev only release
> memory allocated by ifpga driver, that's the expected behavior.
> >
> > I think it's a simple and safe way to release resources completely by calling
> rte_dev_remove.
> >
> 
> If device hot remove is better option, why 'rte_rawdev_pmd_release()' API
> exists?

Agree, let me try to free all resources in rte_rawdev_pmd_release().

> 
> >>
> >>> Fixes: f724a802 ("raw/ifpga: add miscellaneous APIs")
> >>>
> >>> Signed-off-by: Wei Huang <wei.huang@intel.com>
> >>> ---
> >>>    drivers/raw/ifpga/ifpga_rawdev.c | 4 +++-
> >>>    1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/raw/ifpga/ifpga_rawdev.c
> >>> b/drivers/raw/ifpga/ifpga_rawdev.c
> >>> index fdf3c23..88c38aa 100644
> >>> --- a/drivers/raw/ifpga/ifpga_rawdev.c
> >>> +++ b/drivers/raw/ifpga/ifpga_rawdev.c
> >>> @@ -1787,12 +1787,14 @@ int ifpga_rawdev_partial_reconfigure(struct
> >> rte_rawdev *dev, int port,
> >>>    void ifpga_rawdev_cleanup(void)
> >>>    {
> >>>    	struct ifpga_rawdev *dev;
> >>> +	struct rte_rawdev *rdev;
> >>>    	unsigned int i;
> >>>
> >>>    	for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
> >>>    		dev = &ifpga_rawdevices[i];
> >>>    		if (dev->rawdev) {
> >>> -			rte_rawdev_pmd_release(dev->rawdev);
> >>> +			rdev = dev->rawdev;
> >>> +			rte_dev_remove(rdev->device);
> >>>    			dev->rawdev = NULL;
> >>>    		}
> >>>    	}
> >
diff mbox series

Patch

diff --git a/drivers/raw/ifpga/ifpga_rawdev.c b/drivers/raw/ifpga/ifpga_rawdev.c
index fdf3c23..88c38aa 100644
--- a/drivers/raw/ifpga/ifpga_rawdev.c
+++ b/drivers/raw/ifpga/ifpga_rawdev.c
@@ -1787,12 +1787,14 @@  int ifpga_rawdev_partial_reconfigure(struct rte_rawdev *dev, int port,
 void ifpga_rawdev_cleanup(void)
 {
 	struct ifpga_rawdev *dev;
+	struct rte_rawdev *rdev;
 	unsigned int i;
 
 	for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
 		dev = &ifpga_rawdevices[i];
 		if (dev->rawdev) {
-			rte_rawdev_pmd_release(dev->rawdev);
+			rdev = dev->rawdev;
+			rte_dev_remove(rdev->device);
 			dev->rawdev = NULL;
 		}
 	}