[dpdk-stable] [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs

[dpdk-stable] [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs

[zhiyong.yang]

For vdev, just calling rte_eth_dev_close() isn't enough to free all
the resources allocated during device probe, e.g. for virtio-user,
virtio_user_pmd_remove(), i.e. the remove() method of a vdev driver,
needs to be called to unlink the socket file created during device
probe. So this patch calls the rte_eth_dev_detach() for vdev when
quitting testpmd.

Cc: maxime.coquelin@redhat.com
Cc: ferruh.yigit@intel.com
Cc: tiwei.bie@intel.com
Cc: lei.a.yao@intel.com
Cc: bernard.iremonger@intel.com
Cc: stable@dpdk.org

Fixes: af75078fece3 ("first public release")
Fixes: bd8f50a45d0f ("net/virtio-user: support server mode")

Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
---

changes in V2:
1. change the pache title and add a fixes line.

 app/test-pmd/testpmd.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 134401603..1d308f056 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2011,6 +2011,8 @@ detach_port(portid_t port_id)
 void
 pmd_test_exit(void)
 {
+	const struct rte_bus *bus;
+	struct rte_device *device;
 	portid_t pt_id;
 	int ret;
 
@@ -2020,10 +2022,14 @@ pmd_test_exit(void)
 	if (ports != NULL) {
 		no_link_check = 1;
 		RTE_ETH_FOREACH_DEV(pt_id) {
+			device = rte_eth_devices[pt_id].device;
+			bus = rte_bus_find_by_device(device);
 			printf("\nShutting down port %d...\n", pt_id);
 			fflush(stdout);
 			stop_port(pt_id);
 			close_port(pt_id);
+			if (bus && !strcmp(bus->name, "vdev"))
+				detach_port(pt_id);
 		}
 	}
 
-- 
2.14.3

Re: [dpdk-stable] [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs

[Yao, Lei A]

> -----Original Message-----
> From: Yang, Zhiyong
> Sent: Friday, May 18, 2018 6:00 PM
> To: dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Yigit, Ferruh <ferruh.yigit@intel.com>;
> Bie, Tiwei <tiwei.bie@intel.com>; Yao, Lei A <lei.a.yao@intel.com>;
> Iremonger, Bernard <bernard.iremonger@intel.com>; stable@dpdk.org;
> Yang, Zhiyong <zhiyong.yang@intel.com>
> Subject: [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs
> 
> For vdev, just calling rte_eth_dev_close() isn't enough to free all
> the resources allocated during device probe, e.g. for virtio-user,
> virtio_user_pmd_remove(), i.e. the remove() method of a vdev driver,
> needs to be called to unlink the socket file created during device
> probe. So this patch calls the rte_eth_dev_detach() for vdev when
> quitting testpmd.
> 
> Cc: maxime.coquelin@redhat.com
> Cc: ferruh.yigit@intel.com
> Cc: tiwei.bie@intel.com
> Cc: lei.a.yao@intel.com
> Cc: bernard.iremonger@intel.com
> Cc: stable@dpdk.org
> 
> Fixes: af75078fece3 ("first public release")
> Fixes: bd8f50a45d0f ("net/virtio-user: support server mode")
> 
> Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
Tested-by: Lei Yao<lei.a.yao@intel.com>
This patch pass the test for virtio-user server mode. The socket file can 
be deleted after quit testpmd.
> ---
> 
> changes in V2:
> 1. change the pache title and add a fixes line.
> 
>  app/test-pmd/testpmd.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 134401603..1d308f056 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2011,6 +2011,8 @@ detach_port(portid_t port_id)
>  void
>  pmd_test_exit(void)
>  {
> +	const struct rte_bus *bus;
> +	struct rte_device *device;
>  	portid_t pt_id;
>  	int ret;
> 
> @@ -2020,10 +2022,14 @@ pmd_test_exit(void)
>  	if (ports != NULL) {
>  		no_link_check = 1;
>  		RTE_ETH_FOREACH_DEV(pt_id) {
> +			device = rte_eth_devices[pt_id].device;
> +			bus = rte_bus_find_by_device(device);
>  			printf("\nShutting down port %d...\n", pt_id);
>  			fflush(stdout);
>  			stop_port(pt_id);
>  			close_port(pt_id);
> +			if (bus && !strcmp(bus->name, "vdev"))
> +				detach_port(pt_id);
>  		}
>  	}
> 
> --
> 2.14.3

Re: [dpdk-stable] [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs

[Iremonger, Bernard]

Hi Ferruh, Zhiyong,

<snip> 

> > Subject: [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs
> >
> > For vdev, just calling rte_eth_dev_close() isn't enough to free all
> > the resources allocated during device probe, e.g. for virtio-user,
> > virtio_user_pmd_remove(), i.e. the remove() method of a vdev driver,
> > needs to be called to unlink the socket file created during device
> > probe. So this patch calls the rte_eth_dev_detach() for vdev when
> > quitting testpmd.
> >
> > Cc: maxime.coquelin@redhat.com
> > Cc: ferruh.yigit@intel.com
> > Cc: tiwei.bie@intel.com
> > Cc: lei.a.yao@intel.com
> > Cc: bernard.iremonger@intel.com
> > Cc: stable@dpdk.org
> >
> > Fixes: af75078fece3 ("first public release")
> > Fixes: bd8f50a45d0f ("net/virtio-user: support server mode")
> >
> > Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
> Tested-by: Lei Yao<lei.a.yao@intel.com>
> This patch pass the test for virtio-user server mode. The socket file can be
> deleted after quit testpmd.

Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>

Hi Ferruh,

Check-git-log is showing some errors:

./devtools/check-git-log.sh -1
Wrong headline format:
        app/testpmd: fix pmd_test_exit function for vdevs
Wrong tag:
        Tested-by: Lei Yao<lei.a.yao@intel.com>

Can you fix these while merging or is a v3 needed?

Regards,

Bernard.

Re: [dpdk-stable] [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs

[Ferruh Yigit]

On 5/18/2018 11:18 AM, Iremonger, Bernard wrote:
> Hi Ferruh, Zhiyong,
> 
> <snip> 
> 
>>> Subject: [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs
>>>
>>> For vdev, just calling rte_eth_dev_close() isn't enough to free all
>>> the resources allocated during device probe, e.g. for virtio-user,
>>> virtio_user_pmd_remove(), i.e. the remove() method of a vdev driver,
>>> needs to be called to unlink the socket file created during device
>>> probe. So this patch calls the rte_eth_dev_detach() for vdev when
>>> quitting testpmd.
>>>
>>> Cc: maxime.coquelin@redhat.com
>>> Cc: ferruh.yigit@intel.com
>>> Cc: tiwei.bie@intel.com
>>> Cc: lei.a.yao@intel.com
>>> Cc: bernard.iremonger@intel.com
>>> Cc: stable@dpdk.org
>>>
>>> Fixes: af75078fece3 ("first public release")
>>> Fixes: bd8f50a45d0f ("net/virtio-user: support server mode")
>>>
>>> Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
>> Tested-by: Lei Yao<lei.a.yao@intel.com>
>> This patch pass the test for virtio-user server mode. The socket file can be
>> deleted after quit testpmd.
> 
> Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>
> 
> Hi Ferruh,
> 
> Check-git-log is showing some errors:
> 
> ./devtools/check-git-log.sh -1
> Wrong headline format:
>         app/testpmd: fix pmd_test_exit function for vdevs
> Wrong tag:
>         Tested-by: Lei Yao<lei.a.yao@intel.com>
> 
> Can you fix these while merging or is a v3 needed?

Thanks Bernard, I can fix them merging, no new version required.

> 
> Regards,
> 
> Bernard.
> 

Re: [dpdk-stable] [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs

[Ferruh Yigit]

On 5/18/2018 11:18 AM, Iremonger, Bernard wrote:
> Hi Ferruh, Zhiyong,
> 
> <snip> 
> 
>>> Subject: [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs
>>>
>>> For vdev, just calling rte_eth_dev_close() isn't enough to free all
>>> the resources allocated during device probe, e.g. for virtio-user,
>>> virtio_user_pmd_remove(), i.e. the remove() method of a vdev driver,
>>> needs to be called to unlink the socket file created during device
>>> probe. So this patch calls the rte_eth_dev_detach() for vdev when
>>> quitting testpmd.
>>>
>>> Cc: maxime.coquelin@redhat.com
>>> Cc: ferruh.yigit@intel.com
>>> Cc: tiwei.bie@intel.com
>>> Cc: lei.a.yao@intel.com
>>> Cc: bernard.iremonger@intel.com
>>> Cc: stable@dpdk.org
>>>
>>> Fixes: af75078fece3 ("first public release")
>>> Fixes: bd8f50a45d0f ("net/virtio-user: support server mode")
>>>
>>> Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
>> Tested-by: Lei Yao<lei.a.yao@intel.com>
>> This patch pass the test for virtio-user server mode. The socket file can be
>> deleted after quit testpmd.
> 
> Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>

Applied to dpdk-next-net/master, thanks.


> Check-git-log is showing some errors:
> 
> ./devtools/check-git-log.sh -1
> Wrong headline format:
>         app/testpmd: fix pmd_test_exit function for vdevs
> Wrong tag:
>         Tested-by: Lei Yao<lei.a.yao@intel.com>

fixed while applying.

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs

[Matan Azrad]

Hi all

While this patch also applied I don't understand it.
Is it mandatory for each PMD to free all its resources in dev_close()?
Or it should be done by the rte_device remove function?

If the resource cleanup should be done by the remove function I think it
should be called for all the devices (pci, vdev, etc).

Is there an exit function for EAL to clean rte_eal_init()? If no, looks like we need it...
 
From:  Ferruh Yigit
> 
> On 5/18/2018 11:18 AM, Iremonger, Bernard wrote:
> > Hi Ferruh, Zhiyong,
> >
> > <snip>
> >
> >>> Subject: [PATCH v2] app/testpmd: fix pmd_test_exit function for
> >>> vdevs
> >>>
> >>> For vdev, just calling rte_eth_dev_close() isn't enough to free all
> >>> the resources allocated during device probe, e.g. for virtio-user,
> >>> virtio_user_pmd_remove(), i.e. the remove() method of a vdev driver,
> >>> needs to be called to unlink the socket file created during device
> >>> probe. So this patch calls the rte_eth_dev_detach() for vdev when
> >>> quitting testpmd.
> >>>
> >>> Cc: maxime.coquelin@redhat.com
> >>> Cc: ferruh.yigit@intel.com
> >>> Cc: tiwei.bie@intel.com
> >>> Cc: lei.a.yao@intel.com
> >>> Cc: bernard.iremonger@intel.com
> >>> Cc: stable@dpdk.org
> >>>
> >>> Fixes: af75078fece3 ("first public release")
> >>> Fixes: bd8f50a45d0f ("net/virtio-user: support server mode")
> >>>
> >>> Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
> >> Tested-by: Lei Yao<lei.a.yao@intel.com> This patch pass the test for
> >> virtio-user server mode. The socket file can be deleted after quit
> >> testpmd.
> >
> > Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>
> 
> Applied to dpdk-next-net/master, thanks.
> 
> 
> > Check-git-log is showing some errors:
> >
> > ./devtools/check-git-log.sh -1
> > Wrong headline format:
> >         app/testpmd: fix pmd_test_exit function for vdevs Wrong tag:
> >         Tested-by: Lei Yao<lei.a.yao@intel.com>
> 
> fixed while applying.

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs

[Ferruh Yigit]

On 5/18/2018 4:55 PM, Matan Azrad wrote:
> Hi all
> 
> While this patch also applied I don't understand it.
> Is it mandatory for each PMD to free all its resources in dev_close()?
> Or it should be done by the rte_device remove function?
> 
> If the resource cleanup should be done by the remove function I think it
> should be called for all the devices (pci, vdev, etc).
> 
> Is there an exit function for EAL to clean rte_eal_init()? If no, looks like we need it...

Hi Matan,

I believe there is a gap in resource cleanup.
dev_close() it not for resource cleanup, it should be in PMD remove() functions,
and PMDs have it. The problem is remove path is not called in application exit.

As far as I know there is no simple API to clean the resources, having it may
help application to do the cleanup.

I have seen the rte_eal_cleanup() API by Harry, that can be extended to cover
PMD resource cleanup if there is enough motivation for it.

>  
> From:  Ferruh Yigit
>>
>> On 5/18/2018 11:18 AM, Iremonger, Bernard wrote:
>>> Hi Ferruh, Zhiyong,
>>>
>>> <snip>
>>>
>>>>> Subject: [PATCH v2] app/testpmd: fix pmd_test_exit function for
>>>>> vdevs
>>>>>
>>>>> For vdev, just calling rte_eth_dev_close() isn't enough to free all
>>>>> the resources allocated during device probe, e.g. for virtio-user,
>>>>> virtio_user_pmd_remove(), i.e. the remove() method of a vdev driver,
>>>>> needs to be called to unlink the socket file created during device
>>>>> probe. So this patch calls the rte_eth_dev_detach() for vdev when
>>>>> quitting testpmd.
>>>>>
>>>>> Cc: maxime.coquelin@redhat.com
>>>>> Cc: ferruh.yigit@intel.com
>>>>> Cc: tiwei.bie@intel.com
>>>>> Cc: lei.a.yao@intel.com
>>>>> Cc: bernard.iremonger@intel.com
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Fixes: af75078fece3 ("first public release")
>>>>> Fixes: bd8f50a45d0f ("net/virtio-user: support server mode")
>>>>>
>>>>> Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
>>>> Tested-by: Lei Yao<lei.a.yao@intel.com> This patch pass the test for
>>>> virtio-user server mode. The socket file can be deleted after quit
>>>> testpmd.
>>>
>>> Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>
>>
>> Applied to dpdk-next-net/master, thanks.
>>
>>
>>> Check-git-log is showing some errors:
>>>
>>> ./devtools/check-git-log.sh -1
>>> Wrong headline format:
>>>         app/testpmd: fix pmd_test_exit function for vdevs Wrong tag:
>>>         Tested-by: Lei Yao<lei.a.yao@intel.com>
>>
>> fixed while applying.

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs

[Thomas Monjalon]

18/05/2018 18:29, Ferruh Yigit:
> On 5/18/2018 4:55 PM, Matan Azrad wrote:
> > Hi all
> > 
> > While this patch also applied I don't understand it.
> > Is it mandatory for each PMD to free all its resources in dev_close()?
> > Or it should be done by the rte_device remove function?
> > 
> > If the resource cleanup should be done by the remove function I think it
> > should be called for all the devices (pci, vdev, etc).
> > 
> > Is there an exit function for EAL to clean rte_eal_init()? If no, looks like we need it...
> 
> Hi Matan,
> 
> I believe there is a gap in resource cleanup.
> dev_close() it not for resource cleanup, it should be in PMD remove() functions,
> and PMDs have it. The problem is remove path is not called in application exit.
> 
> As far as I know there is no simple API to clean the resources, having it may
> help application to do the cleanup.
> 
> I have seen the rte_eal_cleanup() API by Harry, that can be extended to cover
> PMD resource cleanup if there is enough motivation for it.

Yes, EAL resources should be removed by the function rte_eal_cleanup().
And ethdev ports must be removed by rte_eth_dev_close().

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs

[Thomas Monjalon]

19/05/2018 16:19, Thomas Monjalon:
> 18/05/2018 18:29, Ferruh Yigit:
> > On 5/18/2018 4:55 PM, Matan Azrad wrote:
> > > Hi all
> > > 
> > > While this patch also applied I don't understand it.
> > > Is it mandatory for each PMD to free all its resources in dev_close()?
> > > Or it should be done by the rte_device remove function?
> > > 
> > > If the resource cleanup should be done by the remove function I think it
> > > should be called for all the devices (pci, vdev, etc).
> > > 
> > > Is there an exit function for EAL to clean rte_eal_init()? If no, looks like we need it...
> > 
> > Hi Matan,
> > 
> > I believe there is a gap in resource cleanup.
> > dev_close() it not for resource cleanup, it should be in PMD remove() functions,
> > and PMDs have it. The problem is remove path is not called in application exit.
> > 
> > As far as I know there is no simple API to clean the resources, having it may
> > help application to do the cleanup.
> > 
> > I have seen the rte_eal_cleanup() API by Harry, that can be extended to cover
> > PMD resource cleanup if there is enough motivation for it.
> 
> Yes, EAL resources should be removed by the function rte_eal_cleanup().
> And ethdev ports must be removed by rte_eth_dev_close().

This patch is relying on rte_eth_dev_detach() to remove the EAL device.
It should be done in rte_eal_cleanup().

I am concerned that this patch is workarounding a miss in rte_eal_cleanup,
and takes a different action only for vdev. It is a bad example.

And the function rte_eth_dev_detach() is fundamentally wrong and should be deprecated:
	http://dpdk.org/commit/b05b444d22
	http://dpdk.org/commit/b0fb266855
	http://dpdk.org/commit/df3e8ad73f

One more concern: it seems this patch is breaking failsafe use case.
Note: bonding is managed as an exception in rte_eth_dev_detach().

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs

[Ferruh Yigit]

On 5/21/2018 11:54 AM, Thomas Monjalon wrote:
> 19/05/2018 16:19, Thomas Monjalon:
>> 18/05/2018 18:29, Ferruh Yigit:
>>> On 5/18/2018 4:55 PM, Matan Azrad wrote:
>>>> Hi all
>>>>
>>>> While this patch also applied I don't understand it.
>>>> Is it mandatory for each PMD to free all its resources in dev_close()?
>>>> Or it should be done by the rte_device remove function?
>>>>
>>>> If the resource cleanup should be done by the remove function I think it
>>>> should be called for all the devices (pci, vdev, etc).
>>>>
>>>> Is there an exit function for EAL to clean rte_eal_init()? If no, looks like we need it...
>>>
>>> Hi Matan,
>>>
>>> I believe there is a gap in resource cleanup.
>>> dev_close() it not for resource cleanup, it should be in PMD remove() functions,
>>> and PMDs have it. The problem is remove path is not called in application exit.
>>>
>>> As far as I know there is no simple API to clean the resources, having it may
>>> help application to do the cleanup.
>>>
>>> I have seen the rte_eal_cleanup() API by Harry, that can be extended to cover
>>> PMD resource cleanup if there is enough motivation for it.
>>
>> Yes, EAL resources should be removed by the function rte_eal_cleanup().
>> And ethdev ports must be removed by rte_eth_dev_close().
> 
> This patch is relying on rte_eth_dev_detach() to remove the EAL device.
> It should be done in rte_eal_cleanup().
> 
> I am concerned that this patch is workarounding a miss in rte_eal_cleanup,
> and takes a different action only for vdev. It is a bad example.

Indeed it does workaround, but it is needed to fix a defect in virtio-user.

And currently rte_eal_cleanup() is not complete, it is not doing any device
related cleanup.

> 
> And the function rte_eth_dev_detach() is fundamentally wrong and should be deprecated:
> 	http://dpdk.org/commit/b05b444d22
> 	http://dpdk.org/commit/b0fb266855
> 	http://dpdk.org/commit/df3e8ad73f
> 
> One more concern: it seems this patch is breaking failsafe use case.
> Note: bonding is managed as an exception in rte_eth_dev_detach().
> 
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs

[Thomas Monjalon]

21/05/2018 18:32, Ferruh Yigit:
> On 5/21/2018 11:54 AM, Thomas Monjalon wrote:
> > 19/05/2018 16:19, Thomas Monjalon:
> >> 18/05/2018 18:29, Ferruh Yigit:
> >>> On 5/18/2018 4:55 PM, Matan Azrad wrote:
> >>>> Hi all
> >>>>
> >>>> While this patch also applied I don't understand it.
> >>>> Is it mandatory for each PMD to free all its resources in dev_close()?
> >>>> Or it should be done by the rte_device remove function?
> >>>>
> >>>> If the resource cleanup should be done by the remove function I think it
> >>>> should be called for all the devices (pci, vdev, etc).
> >>>>
> >>>> Is there an exit function for EAL to clean rte_eal_init()? If no, looks like we need it...
> >>>
> >>> Hi Matan,
> >>>
> >>> I believe there is a gap in resource cleanup.
> >>> dev_close() it not for resource cleanup, it should be in PMD remove() functions,
> >>> and PMDs have it. The problem is remove path is not called in application exit.
> >>>
> >>> As far as I know there is no simple API to clean the resources, having it may
> >>> help application to do the cleanup.
> >>>
> >>> I have seen the rte_eal_cleanup() API by Harry, that can be extended to cover
> >>> PMD resource cleanup if there is enough motivation for it.
> >>
> >> Yes, EAL resources should be removed by the function rte_eal_cleanup().
> >> And ethdev ports must be removed by rte_eth_dev_close().
> > 
> > This patch is relying on rte_eth_dev_detach() to remove the EAL device.
> > It should be done in rte_eal_cleanup().
> > 
> > I am concerned that this patch is workarounding a miss in rte_eal_cleanup,
> > and takes a different action only for vdev. It is a bad example.
> 
> Indeed it does workaround, but it is needed to fix a defect in virtio-user.

The defect is still in virtio-user after this patch.
To make this workaround acceptable, you need:
	1/ add the virtio-user known issue in release notes
	2/ add a FIXME comment in testpmd code explaining the workaround
	3/ commit to work on rte_eal_cleanup() in 18.08

> And currently rte_eal_cleanup() is not complete, it is not doing any device
> related cleanup.

Yes, we need to add more code in rte_eal_cleanup().

> > And the function rte_eth_dev_detach() is fundamentally wrong and should be deprecated:
> > 	http://dpdk.org/commit/b05b444d22
> > 	http://dpdk.org/commit/b0fb266855
> > 	http://dpdk.org/commit/df3e8ad73f
> > 
> > One more concern: it seems this patch is breaking failsafe use case.
> > Note: bonding is managed as an exception in rte_eth_dev_detach().

I have sent a fix in vdev code for the failsafe issue.

No matter the vdev fix is applied or not, this workaround could target only virtio-user.

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs

[Thomas Monjalon]

Any update to improve this workaround?

21/05/2018 18:38, Thomas Monjalon:
> 21/05/2018 18:32, Ferruh Yigit:
> > On 5/21/2018 11:54 AM, Thomas Monjalon wrote:
> > > 19/05/2018 16:19, Thomas Monjalon:
> > >> 18/05/2018 18:29, Ferruh Yigit:
> > >>> On 5/18/2018 4:55 PM, Matan Azrad wrote:
> > >>>> Hi all
> > >>>>
> > >>>> While this patch also applied I don't understand it.
> > >>>> Is it mandatory for each PMD to free all its resources in dev_close()?
> > >>>> Or it should be done by the rte_device remove function?
> > >>>>
> > >>>> If the resource cleanup should be done by the remove function I think it
> > >>>> should be called for all the devices (pci, vdev, etc).
> > >>>>
> > >>>> Is there an exit function for EAL to clean rte_eal_init()? If no, looks like we need it...
> > >>>
> > >>> Hi Matan,
> > >>>
> > >>> I believe there is a gap in resource cleanup.
> > >>> dev_close() it not for resource cleanup, it should be in PMD remove() functions,
> > >>> and PMDs have it. The problem is remove path is not called in application exit.
> > >>>
> > >>> As far as I know there is no simple API to clean the resources, having it may
> > >>> help application to do the cleanup.
> > >>>
> > >>> I have seen the rte_eal_cleanup() API by Harry, that can be extended to cover
> > >>> PMD resource cleanup if there is enough motivation for it.
> > >>
> > >> Yes, EAL resources should be removed by the function rte_eal_cleanup().
> > >> And ethdev ports must be removed by rte_eth_dev_close().
> > > 
> > > This patch is relying on rte_eth_dev_detach() to remove the EAL device.
> > > It should be done in rte_eal_cleanup().
> > > 
> > > I am concerned that this patch is workarounding a miss in rte_eal_cleanup,
> > > and takes a different action only for vdev. It is a bad example.
> > 
> > Indeed it does workaround, but it is needed to fix a defect in virtio-user.
> 
> The defect is still in virtio-user after this patch.
> To make this workaround acceptable, you need:
> 	1/ add the virtio-user known issue in release notes
> 	2/ add a FIXME comment in testpmd code explaining the workaround
> 	3/ commit to work on rte_eal_cleanup() in 18.08
> 
> > And currently rte_eal_cleanup() is not complete, it is not doing any device
> > related cleanup.
> 
> Yes, we need to add more code in rte_eal_cleanup().
> 
> > > And the function rte_eth_dev_detach() is fundamentally wrong and should be deprecated:
> > > 	http://dpdk.org/commit/b05b444d22
> > > 	http://dpdk.org/commit/b0fb266855
> > > 	http://dpdk.org/commit/df3e8ad73f
> > > 
> > > One more concern: it seems this patch is breaking failsafe use case.
> > > Note: bonding is managed as an exception in rte_eth_dev_detach().
> 
> I have sent a fix in vdev code for the failsafe issue.
> 
> No matter the vdev fix is applied or not, this workaround could target only virtio-user.

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs

[Ferruh Yigit]

On 5/22/2018 2:12 PM, Thomas Monjalon wrote:
> Any update to improve this workaround?
> 
> 21/05/2018 18:38, Thomas Monjalon:
>> 21/05/2018 18:32, Ferruh Yigit:
>>> On 5/21/2018 11:54 AM, Thomas Monjalon wrote:
>>>> 19/05/2018 16:19, Thomas Monjalon:
>>>>> 18/05/2018 18:29, Ferruh Yigit:
>>>>>> On 5/18/2018 4:55 PM, Matan Azrad wrote:
>>>>>>> Hi all
>>>>>>>
>>>>>>> While this patch also applied I don't understand it.
>>>>>>> Is it mandatory for each PMD to free all its resources in dev_close()?
>>>>>>> Or it should be done by the rte_device remove function?
>>>>>>>
>>>>>>> If the resource cleanup should be done by the remove function I think it
>>>>>>> should be called for all the devices (pci, vdev, etc).
>>>>>>>
>>>>>>> Is there an exit function for EAL to clean rte_eal_init()? If no, looks like we need it...
>>>>>>
>>>>>> Hi Matan,
>>>>>>
>>>>>> I believe there is a gap in resource cleanup.
>>>>>> dev_close() it not for resource cleanup, it should be in PMD remove() functions,
>>>>>> and PMDs have it. The problem is remove path is not called in application exit.
>>>>>>
>>>>>> As far as I know there is no simple API to clean the resources, having it may
>>>>>> help application to do the cleanup.
>>>>>>
>>>>>> I have seen the rte_eal_cleanup() API by Harry, that can be extended to cover
>>>>>> PMD resource cleanup if there is enough motivation for it.
>>>>>
>>>>> Yes, EAL resources should be removed by the function rte_eal_cleanup().
>>>>> And ethdev ports must be removed by rte_eth_dev_close().
>>>>
>>>> This patch is relying on rte_eth_dev_detach() to remove the EAL device.
>>>> It should be done in rte_eal_cleanup().
>>>>
>>>> I am concerned that this patch is workarounding a miss in rte_eal_cleanup,
>>>> and takes a different action only for vdev. It is a bad example.
>>>
>>> Indeed it does workaround, but it is needed to fix a defect in virtio-user.
>>
>> The defect is still in virtio-user after this patch.
>> To make this workaround acceptable, you need:
>> 	1/ add the virtio-user known issue in release notes
>> 	2/ add a FIXME comment in testpmd code explaining the workaround
>> 	3/ commit to work on rte_eal_cleanup() in 18.08

I have submitted following patch [1], can it cover 2/ for the v18.05-rc5?

[1]
https://dpdk.org/dev/patchwork/patch/40350/

>>
>>> And currently rte_eal_cleanup() is not complete, it is not doing any device
>>> related cleanup.
>>
>> Yes, we need to add more code in rte_eal_cleanup().
>>
>>>> And the function rte_eth_dev_detach() is fundamentally wrong and should be deprecated:
>>>> 	http://dpdk.org/commit/b05b444d22
>>>> 	http://dpdk.org/commit/b0fb266855
>>>> 	http://dpdk.org/commit/df3e8ad73f
>>>>
>>>> One more concern: it seems this patch is breaking failsafe use case.
>>>> Note: bonding is managed as an exception in rte_eth_dev_detach().
>>
>> I have sent a fix in vdev code for the failsafe issue.
>>
>> No matter the vdev fix is applied or not, this workaround could target only virtio-user.
> 
> 
> 
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs

[Thomas Monjalon]

22/05/2018 20:38, Ferruh Yigit:
> On 5/22/2018 2:12 PM, Thomas Monjalon wrote:
> > Any update to improve this workaround?
> > 
> > 21/05/2018 18:38, Thomas Monjalon:
> >> 21/05/2018 18:32, Ferruh Yigit:
> >>> On 5/21/2018 11:54 AM, Thomas Monjalon wrote:
> >>>> 19/05/2018 16:19, Thomas Monjalon:
> >>>>> 18/05/2018 18:29, Ferruh Yigit:
> >>>>>> On 5/18/2018 4:55 PM, Matan Azrad wrote:
> >>>>>>> Hi all
> >>>>>>>
> >>>>>>> While this patch also applied I don't understand it.
> >>>>>>> Is it mandatory for each PMD to free all its resources in dev_close()?
> >>>>>>> Or it should be done by the rte_device remove function?
> >>>>>>>
> >>>>>>> If the resource cleanup should be done by the remove function I think it
> >>>>>>> should be called for all the devices (pci, vdev, etc).
> >>>>>>>
> >>>>>>> Is there an exit function for EAL to clean rte_eal_init()? If no, looks like we need it...
> >>>>>>
> >>>>>> Hi Matan,
> >>>>>>
> >>>>>> I believe there is a gap in resource cleanup.
> >>>>>> dev_close() it not for resource cleanup, it should be in PMD remove() functions,
> >>>>>> and PMDs have it. The problem is remove path is not called in application exit.
> >>>>>>
> >>>>>> As far as I know there is no simple API to clean the resources, having it may
> >>>>>> help application to do the cleanup.
> >>>>>>
> >>>>>> I have seen the rte_eal_cleanup() API by Harry, that can be extended to cover
> >>>>>> PMD resource cleanup if there is enough motivation for it.
> >>>>>
> >>>>> Yes, EAL resources should be removed by the function rte_eal_cleanup().
> >>>>> And ethdev ports must be removed by rte_eth_dev_close().
> >>>>
> >>>> This patch is relying on rte_eth_dev_detach() to remove the EAL device.
> >>>> It should be done in rte_eal_cleanup().
> >>>>
> >>>> I am concerned that this patch is workarounding a miss in rte_eal_cleanup,
> >>>> and takes a different action only for vdev. It is a bad example.
> >>>
> >>> Indeed it does workaround, but it is needed to fix a defect in virtio-user.
> >>
> >> The defect is still in virtio-user after this patch.
> >> To make this workaround acceptable, you need:
> >> 	1/ add the virtio-user known issue in release notes
> >> 	2/ add a FIXME comment in testpmd code explaining the workaround
> >> 	3/ commit to work on rte_eal_cleanup() in 18.08
> 
> I have submitted following patch [1], can it cover 2/ for the v18.05-rc5?
> 
> [1]
> https://dpdk.org/dev/patchwork/patch/40350/

Yes, it covers item 2/. Thanks

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs

[Yang, Zhiyong]

Hi Thomas, 

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, May 23, 2018 3:49 AM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: Yang, Zhiyong <zhiyong.yang@intel.com>; dev@dpdk.org; Matan Azrad
> <matan@mellanox.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>; Yao, Lei A <lei.a.yao@intel.com>;
> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>;
> stable@dpdk.org; Van Haaren, Harry <harry.van.haaren@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2] app/testpmd: fix pmd_test_exit function for
> vdevs
> 
> 22/05/2018 20:38, Ferruh Yigit:
> > On 5/22/2018 2:12 PM, Thomas Monjalon wrote:
> > > Any update to improve this workaround?
> > >
> > > 21/05/2018 18:38, Thomas Monjalon:
> > >> 21/05/2018 18:32, Ferruh Yigit:
> > >>> On 5/21/2018 11:54 AM, Thomas Monjalon wrote:
> > >>>> 19/05/2018 16:19, Thomas Monjalon:
> > >>>>> 18/05/2018 18:29, Ferruh Yigit:
> > >>>>>> On 5/18/2018 4:55 PM, Matan Azrad wrote:
> > >>>>>>> Hi all
> > >>>>>>>
> > >>>>>>> While this patch also applied I don't understand it.
> > >>>>>>> Is it mandatory for each PMD to free all its resources in dev_close()?
> > >>>>>>> Or it should be done by the rte_device remove function?
> > >>>>>>>
> > >>>>>>> If the resource cleanup should be done by the remove function
> > >>>>>>> I think it should be called for all the devices (pci, vdev, etc).
> > >>>>>>>
> > >>>>>>> Is there an exit function for EAL to clean rte_eal_init()? If no, looks
> like we need it...
> > >>>>>>
> > >>>>>> Hi Matan,
> > >>>>>>
> > >>>>>> I believe there is a gap in resource cleanup.
> > >>>>>> dev_close() it not for resource cleanup, it should be in PMD
> > >>>>>> remove() functions, and PMDs have it. The problem is remove path is
> not called in application exit.
> > >>>>>>
> > >>>>>> As far as I know there is no simple API to clean the resources,
> > >>>>>> having it may help application to do the cleanup.
> > >>>>>>
> > >>>>>> I have seen the rte_eal_cleanup() API by Harry, that can be
> > >>>>>> extended to cover PMD resource cleanup if there is enough
> motivation for it.
> > >>>>>
> > >>>>> Yes, EAL resources should be removed by the function
> rte_eal_cleanup().
> > >>>>> And ethdev ports must be removed by rte_eth_dev_close().
> > >>>>
> > >>>> This patch is relying on rte_eth_dev_detach() to remove the EAL device.
> > >>>> It should be done in rte_eal_cleanup().
> > >>>>
> > >>>> I am concerned that this patch is workarounding a miss in
> > >>>> rte_eal_cleanup, and takes a different action only for vdev. It is a bad
> example.
> > >>>
> > >>> Indeed it does workaround, but it is needed to fix a defect in virtio-user.
> > >>
> > >> The defect is still in virtio-user after this patch.
> > >> To make this workaround acceptable, you need:
> > >> 	1/ add the virtio-user known issue in release notes

I don't think it is a bug for virtio-user. Just no proper implemented API can free the virtio-user resources  allocated by probe method.
Socket file is just one example,  other resources are freed in the remove method.  Testpmd was always missing the functionality.

> > >> 	2/ add a FIXME comment in testpmd code explaining the workaround
> > >> 	3/ commit to work on rte_eal_cleanup() in 18.08
> >
> > I have submitted following patch [1], can it cover 2/ for the v18.05-rc5?
> >
> > [1]
> > https://dpdk.org/dev/patchwork/patch/40350/
> 
> Yes, it covers item 2/. Thanks
> 
> 

rte_eal_cleanup() is a proper API,  which is needed to clean up resources.

Thanks
Zhiyong

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs

[Yang, Zhiyong]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Yang, Zhiyong
> Sent: Wednesday, May 23, 2018 9:52 AM
> To: Thomas Monjalon <thomas@monjalon.net>; Yigit, Ferruh
> <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; Matan Azrad <matan@mellanox.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>; Yao, Lei A <lei.a.yao@intel.com>;
> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>;
> stable@dpdk.org; Van Haaren, Harry <harry.van.haaren@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2] app/testpmd: fix pmd_test_exit function for
> vdevs
> 
> Hi Thomas,
> 
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Wednesday, May 23, 2018 3:49 AM
> > To: Yigit, Ferruh <ferruh.yigit@intel.com>
> > Cc: Yang, Zhiyong <zhiyong.yang@intel.com>; dev@dpdk.org; Matan Azrad
> > <matan@mellanox.com>; Iremonger, Bernard
> > <bernard.iremonger@intel.com>; Yao, Lei A <lei.a.yao@intel.com>;
> > maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>;
> > stable@dpdk.org; Van Haaren, Harry <harry.van.haaren@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH v2] app/testpmd: fix pmd_test_exit
> > function for vdevs
> >
> > 22/05/2018 20:38, Ferruh Yigit:
> > > On 5/22/2018 2:12 PM, Thomas Monjalon wrote:
> > > > Any update to improve this workaround?
> > > >
> > > > 21/05/2018 18:38, Thomas Monjalon:
> > > >> 21/05/2018 18:32, Ferruh Yigit:
> > > >>> On 5/21/2018 11:54 AM, Thomas Monjalon wrote:
> > > >>>> 19/05/2018 16:19, Thomas Monjalon:
> > > >>>>> 18/05/2018 18:29, Ferruh Yigit:
> > > >>>>>> On 5/18/2018 4:55 PM, Matan Azrad wrote:
> > > >>>>>>> Hi all
> > > >>>>>>>
> > > >>>>>>> While this patch also applied I don't understand it.
> > > >>>>>>> Is it mandatory for each PMD to free all its resources in
> dev_close()?
> > > >>>>>>> Or it should be done by the rte_device remove function?
> > > >>>>>>>
> > > >>>>>>> If the resource cleanup should be done by the remove
> > > >>>>>>> function I think it should be called for all the devices (pci, vdev,
> etc).
> > > >>>>>>>
> > > >>>>>>> Is there an exit function for EAL to clean rte_eal_init()?
> > > >>>>>>> If no, looks
> > like we need it...
> > > >>>>>>
> > > >>>>>> Hi Matan,
> > > >>>>>>
> > > >>>>>> I believe there is a gap in resource cleanup.
> > > >>>>>> dev_close() it not for resource cleanup, it should be in PMD
> > > >>>>>> remove() functions, and PMDs have it. The problem is remove
> > > >>>>>> path is
> > not called in application exit.
> > > >>>>>>
> > > >>>>>> As far as I know there is no simple API to clean the
> > > >>>>>> resources, having it may help application to do the cleanup.
> > > >>>>>>
> > > >>>>>> I have seen the rte_eal_cleanup() API by Harry, that can be
> > > >>>>>> extended to cover PMD resource cleanup if there is enough
> > motivation for it.
> > > >>>>>
> > > >>>>> Yes, EAL resources should be removed by the function
> > rte_eal_cleanup().
> > > >>>>> And ethdev ports must be removed by rte_eth_dev_close().
> > > >>>>
> > > >>>> This patch is relying on rte_eth_dev_detach() to remove the EAL
> device.
> > > >>>> It should be done in rte_eal_cleanup().
> > > >>>>
> > > >>>> I am concerned that this patch is workarounding a miss in
> > > >>>> rte_eal_cleanup, and takes a different action only for vdev. It
> > > >>>> is a bad
> > example.
> > > >>>
> > > >>> Indeed it does workaround, but it is needed to fix a defect in virtio-user.
> > > >>
> > > >> The defect is still in virtio-user after this patch.
> > > >> To make this workaround acceptable, you need:
> > > >> 	1/ add the virtio-user known issue in release notes
> 
> I don't think it is a bug for virtio-user. Just no proper implemented API can free
> the virtio-user resources  allocated by probe method.
> Socket file is just one example,  other resources are freed in the remove
> method.  Testpmd was always missing the functionality.
> 
> > > >> 	2/ add a FIXME comment in testpmd code explaining the workaround
> > > >> 	3/ commit to work on rte_eal_cleanup() in 18.08

Do you mean to work on rte_eal_cleanup() for all drivers or for virtio user only?

> > >
> > > I have submitted following patch [1], can it cover 2/ for the v18.05-rc5?
> > >
> > > [1]
> > > https://dpdk.org/dev/patchwork/patch/40350/
> >
> > Yes, it covers item 2/. Thanks
> >
> >
> 
> rte_eal_cleanup() is a proper API,  which is needed to clean up resources.
> 
> Thanks
> Zhiyong

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs

[Thomas Monjalon]

23/05/2018 13:37, Yang, Zhiyong:
> From: Yang, Zhiyong
> > 
> > Hi Thomas,
> > 
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > 22/05/2018 20:38, Ferruh Yigit:
> > > > On 5/22/2018 2:12 PM, Thomas Monjalon wrote:
> > > > > Any update to improve this workaround?
> > > > >
> > > > > 21/05/2018 18:38, Thomas Monjalon:
> > > > >> 21/05/2018 18:32, Ferruh Yigit:
> > > > >>> On 5/21/2018 11:54 AM, Thomas Monjalon wrote:
> > > > >>>> 19/05/2018 16:19, Thomas Monjalon:
> > > > >>>>> 18/05/2018 18:29, Ferruh Yigit:
> > > > >>>>>> On 5/18/2018 4:55 PM, Matan Azrad wrote:
> > > > >>>>>>> Hi all
> > > > >>>>>>>
> > > > >>>>>>> While this patch also applied I don't understand it.
> > > > >>>>>>> Is it mandatory for each PMD to free all its resources in
> > dev_close()?
> > > > >>>>>>> Or it should be done by the rte_device remove function?
> > > > >>>>>>>
> > > > >>>>>>> If the resource cleanup should be done by the remove
> > > > >>>>>>> function I think it should be called for all the devices (pci, vdev,
> > etc).
> > > > >>>>>>>
> > > > >>>>>>> Is there an exit function for EAL to clean rte_eal_init()?
> > > > >>>>>>> If no, looks
> > > like we need it...
> > > > >>>>>>
> > > > >>>>>> Hi Matan,
> > > > >>>>>>
> > > > >>>>>> I believe there is a gap in resource cleanup.
> > > > >>>>>> dev_close() it not for resource cleanup, it should be in PMD
> > > > >>>>>> remove() functions, and PMDs have it. The problem is remove
> > > > >>>>>> path is
> > > not called in application exit.
> > > > >>>>>>
> > > > >>>>>> As far as I know there is no simple API to clean the
> > > > >>>>>> resources, having it may help application to do the cleanup.
> > > > >>>>>>
> > > > >>>>>> I have seen the rte_eal_cleanup() API by Harry, that can be
> > > > >>>>>> extended to cover PMD resource cleanup if there is enough
> > > motivation for it.
> > > > >>>>>
> > > > >>>>> Yes, EAL resources should be removed by the function
> > > rte_eal_cleanup().
> > > > >>>>> And ethdev ports must be removed by rte_eth_dev_close().
> > > > >>>>
> > > > >>>> This patch is relying on rte_eth_dev_detach() to remove the EAL
> > device.
> > > > >>>> It should be done in rte_eal_cleanup().
> > > > >>>>
> > > > >>>> I am concerned that this patch is workarounding a miss in
> > > > >>>> rte_eal_cleanup, and takes a different action only for vdev. It
> > > > >>>> is a bad
> > > example.
> > > > >>>
> > > > >>> Indeed it does workaround, but it is needed to fix a defect in virtio-user.
> > > > >>
> > > > >> The defect is still in virtio-user after this patch.
> > > > >> To make this workaround acceptable, you need:
> > > > >> 	1/ add the virtio-user known issue in release notes
> > 
> > I don't think it is a bug for virtio-user. Just no proper implemented API can free
> > the virtio-user resources  allocated by probe method.
> > Socket file is just one example,  other resources are freed in the remove
> > method.  Testpmd was always missing the functionality.
> > 
> > > > >> 	2/ add a FIXME comment in testpmd code explaining the workaround
> > > > >> 	3/ commit to work on rte_eal_cleanup() in 18.08
> 
> Do you mean to work on rte_eal_cleanup() for all drivers or for virtio user only?

Yes, we need to call the remove() functions of all drivers from rte_eal_cleanup().


> > > > I have submitted following patch [1], can it cover 2/ for the v18.05-rc5?
> > > >
> > > > [1]
> > > > https://dpdk.org/dev/patchwork/patch/40350/
> > >
> > > Yes, it covers item 2/. Thanks
> > >
> > >
> > 
> > rte_eal_cleanup() is a proper API,  which is needed to clean up resources.
> > 
> > Thanks
> > Zhiyong

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs

[Ferruh Yigit]

On 5/19/2018 3:19 PM, Thomas Monjalon wrote:
> 18/05/2018 18:29, Ferruh Yigit:
>> On 5/18/2018 4:55 PM, Matan Azrad wrote:
>>> Hi all
>>>
>>> While this patch also applied I don't understand it.
>>> Is it mandatory for each PMD to free all its resources in dev_close()?
>>> Or it should be done by the rte_device remove function?
>>>
>>> If the resource cleanup should be done by the remove function I think it
>>> should be called for all the devices (pci, vdev, etc).
>>>
>>> Is there an exit function for EAL to clean rte_eal_init()? If no, looks like we need it...
>>
>> Hi Matan,
>>
>> I believe there is a gap in resource cleanup.
>> dev_close() it not for resource cleanup, it should be in PMD remove() functions,
>> and PMDs have it. The problem is remove path is not called in application exit.
>>
>> As far as I know there is no simple API to clean the resources, having it may
>> help application to do the cleanup.
>>
>> I have seen the rte_eal_cleanup() API by Harry, that can be extended to cover
>> PMD resource cleanup if there is enough motivation for it.
> 
> Yes, EAL resources should be removed by the function rte_eal_cleanup().
> And ethdev ports must be removed by rte_eth_dev_close().

There is probe() and remove() functions.
There is dev_close devops, called by rte_eth_dev_close(), but there is no open()
or equivalent.

For example an ethdev allocated its private data during probe(), if
rte_eth_dev_close() free it, how can a new ethdev can be allocated?

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs

[Thomas Monjalon]

21/05/2018 18:37, Ferruh Yigit:
> On 5/19/2018 3:19 PM, Thomas Monjalon wrote:
> > 18/05/2018 18:29, Ferruh Yigit:
> >> On 5/18/2018 4:55 PM, Matan Azrad wrote:
> >>> Hi all
> >>>
> >>> While this patch also applied I don't understand it.
> >>> Is it mandatory for each PMD to free all its resources in dev_close()?
> >>> Or it should be done by the rte_device remove function?
> >>>
> >>> If the resource cleanup should be done by the remove function I think it
> >>> should be called for all the devices (pci, vdev, etc).
> >>>
> >>> Is there an exit function for EAL to clean rte_eal_init()? If no, looks like we need it...
> >>
> >> Hi Matan,
> >>
> >> I believe there is a gap in resource cleanup.
> >> dev_close() it not for resource cleanup, it should be in PMD remove() functions,
> >> and PMDs have it. The problem is remove path is not called in application exit.
> >>
> >> As far as I know there is no simple API to clean the resources, having it may
> >> help application to do the cleanup.
> >>
> >> I have seen the rte_eal_cleanup() API by Harry, that can be extended to cover
> >> PMD resource cleanup if there is enough motivation for it.
> > 
> > Yes, EAL resources should be removed by the function rte_eal_cleanup().
> > And ethdev ports must be removed by rte_eth_dev_close().
> 
> There is probe() and remove() functions.
> There is dev_close devops, called by rte_eth_dev_close(), but there is no open()
> or equivalent.
> 
> For example an ethdev allocated its private data during probe(), if
> rte_eth_dev_close() free it, how can a new ethdev can be allocated?

I don't understand the question.
You say closing a port and opening a new one.
So we allocate private data in the new probe.

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs

[Ferruh Yigit]

On 5/21/2018 5:40 PM, Thomas Monjalon wrote:
> 21/05/2018 18:37, Ferruh Yigit:
>> On 5/19/2018 3:19 PM, Thomas Monjalon wrote:
>>> 18/05/2018 18:29, Ferruh Yigit:
>>>> On 5/18/2018 4:55 PM, Matan Azrad wrote:
>>>>> Hi all
>>>>>
>>>>> While this patch also applied I don't understand it.
>>>>> Is it mandatory for each PMD to free all its resources in dev_close()?
>>>>> Or it should be done by the rte_device remove function?
>>>>>
>>>>> If the resource cleanup should be done by the remove function I think it
>>>>> should be called for all the devices (pci, vdev, etc).
>>>>>
>>>>> Is there an exit function for EAL to clean rte_eal_init()? If no, looks like we need it...
>>>>
>>>> Hi Matan,
>>>>
>>>> I believe there is a gap in resource cleanup.
>>>> dev_close() it not for resource cleanup, it should be in PMD remove() functions,
>>>> and PMDs have it. The problem is remove path is not called in application exit.
>>>>
>>>> As far as I know there is no simple API to clean the resources, having it may
>>>> help application to do the cleanup.
>>>>
>>>> I have seen the rte_eal_cleanup() API by Harry, that can be extended to cover
>>>> PMD resource cleanup if there is enough motivation for it.
>>>
>>> Yes, EAL resources should be removed by the function rte_eal_cleanup().
>>> And ethdev ports must be removed by rte_eth_dev_close().
>>
>> There is probe() and remove() functions.
>> There is dev_close devops, called by rte_eth_dev_close(), but there is no open()
>> or equivalent.
>>
>> For example an ethdev allocated its private data during probe(), if
>> rte_eth_dev_close() free it, how can a new ethdev can be allocated?
> 
> I don't understand the question.
> You say closing a port and opening a new one.
> So we allocate private data in the new probe.

the question is why resources allocated in probe() but freed in close() instead
of remove()?

Or should we have something like rte_eth_dev_open() ?

Re: [dpdk-stable] [dpdk-dev] [PATCH v2] app/testpmd: fix pmd_test_exit function for vdevs

[Thomas Monjalon]

21/05/2018 18:44, Ferruh Yigit:
> On 5/21/2018 5:40 PM, Thomas Monjalon wrote:
> > 21/05/2018 18:37, Ferruh Yigit:
> >> On 5/19/2018 3:19 PM, Thomas Monjalon wrote:
> >>> 18/05/2018 18:29, Ferruh Yigit:
> >>>> On 5/18/2018 4:55 PM, Matan Azrad wrote:
> >>>>> Hi all
> >>>>>
> >>>>> While this patch also applied I don't understand it.
> >>>>> Is it mandatory for each PMD to free all its resources in dev_close()?
> >>>>> Or it should be done by the rte_device remove function?
> >>>>>
> >>>>> If the resource cleanup should be done by the remove function I think it
> >>>>> should be called for all the devices (pci, vdev, etc).
> >>>>>
> >>>>> Is there an exit function for EAL to clean rte_eal_init()? If no, looks like we need it...
> >>>>
> >>>> Hi Matan,
> >>>>
> >>>> I believe there is a gap in resource cleanup.
> >>>> dev_close() it not for resource cleanup, it should be in PMD remove() functions,
> >>>> and PMDs have it. The problem is remove path is not called in application exit.
> >>>>
> >>>> As far as I know there is no simple API to clean the resources, having it may
> >>>> help application to do the cleanup.
> >>>>
> >>>> I have seen the rte_eal_cleanup() API by Harry, that can be extended to cover
> >>>> PMD resource cleanup if there is enough motivation for it.
> >>>
> >>> Yes, EAL resources should be removed by the function rte_eal_cleanup().
> >>> And ethdev ports must be removed by rte_eth_dev_close().
> >>
> >> There is probe() and remove() functions.
> >> There is dev_close devops, called by rte_eth_dev_close(), but there is no open()
> >> or equivalent.
> >>
> >> For example an ethdev allocated its private data during probe(), if
> >> rte_eth_dev_close() free it, how can a new ethdev can be allocated?
> > 
> > I don't understand the question.
> > You say closing a port and opening a new one.
> > So we allocate private data in the new probe.
> 
> the question is why resources allocated in probe() but freed in close() instead
> of remove()?

Because close() is the opposite of probe().

The function remove() does not exist in ethdev.
However it exists in EAL layer for rte_device.
Reminder: one rte_device can be common to several ethdev ports,
or other class of ports.

> Or should we have something like rte_eth_dev_open() ?

I don't think it is needed.