DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] app/testpmd: fix hotplug
@ 2020-01-23 15:15 Ferruh Yigit
  2020-01-23 16:00 ` Matan Azrad
  2020-01-23 17:01 ` Iremonger, Bernard
  0 siblings, 2 replies; 7+ messages in thread
From: Ferruh Yigit @ 2020-01-23 15:15 UTC (permalink / raw)
  To: Wenzhuo Lu, Jingjing Wu, Bernard Iremonger, Matan Azrad; +Cc: dev, stable

The 'port_id_is_invalid()' check in the 'detach_port_device()' is
breaking the hotplug support, since at that stage port will be closed
and validity check always fail [1] and removing the device is not really
completed.

But this cause the vfio request interrupt keep triggered continuously
and makes the application unusable, since port is closed but device is
not removed, the remove path keep generating error log:

EAL: can not get port by device 0000:00:05.0!
EAL: can not get port by device 0000:00:05.0!
EAL: can not get port by device 0000:00:05.0!
EAL: can not get port by device 0000:00:05.0!
EAL: can not get port by device 0000:00:05.0!
EAL: can not get port by device 0000:00:05.0!

Fixed by removing 'port_id_is_invalid()' check from
'detach_port_device()', anyway it shouldn't be required. Without this
check device remove works as expected.

Only "Invalid port_id=0" logs seen a few times, which is because the
actual removal not done synchronously but an alarm set for it, until the
alarm fired application may receive many interrupts, expect the first
ones cause the error.
So this patch also removes the logging from checking the invalid port in
'rmv_port_callback()' to reduce the noise.

[1]
rmv_port_callback()
  stop_port(port_id);
  close_port(port_id);
  detach_port_device(port_id);

Fixes: 43d0e304980a ("app/testpmd: fix invalid port detaching")
Cc: stable@dpdk.org

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 app/test-pmd/testpmd.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index f9f4cd1d3..3323013bb 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2641,9 +2641,6 @@ detach_port_device(portid_t port_id)
 
 	printf("Removing a device...\n");
 
-	if (port_id_is_invalid(port_id, ENABLED_WARN))
-		return;
-
 	dev = rte_eth_devices[port_id].device;
 	if (dev == NULL) {
 		printf("Device already removed\n");
@@ -2875,7 +2872,8 @@ rmv_port_callback(void *arg)
 	int org_no_link_check = no_link_check;
 	portid_t port_id = (intptr_t)arg;
 
-	RTE_ETH_VALID_PORTID_OR_RET(port_id);
+	if (!rte_eth_dev_is_valid_port(port_id))
+		return;
 
 	if (!test_done && port_is_forwarding(port_id)) {
 		need_to_start = 1;
-- 
2.24.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] app/testpmd: fix hotplug
  2020-01-23 15:15 [dpdk-dev] [PATCH] app/testpmd: fix hotplug Ferruh Yigit
@ 2020-01-23 16:00 ` Matan Azrad
  2020-01-23 17:01 ` Iremonger, Bernard
  1 sibling, 0 replies; 7+ messages in thread
From: Matan Azrad @ 2020-01-23 16:00 UTC (permalink / raw)
  To: Ferruh Yigit, Wenzhuo Lu, Jingjing Wu, Bernard Iremonger; +Cc: dev, stable


Hi

From: Ferruh Yigit
> The 'port_id_is_invalid()' check in the 'detach_port_device()' is breaking the
> hotplug support, since at that stage port will be closed and validity check
> always fail [1] and removing the device is not really completed.
> 
> But this cause the vfio request interrupt keep triggered continuously and
> makes the application unusable, since port is closed but device is not
> removed, the remove path keep generating error log:
> 
> EAL: can not get port by device 0000:00:05.0!
> EAL: can not get port by device 0000:00:05.0!
> EAL: can not get port by device 0000:00:05.0!
> EAL: can not get port by device 0000:00:05.0!
> EAL: can not get port by device 0000:00:05.0!
> EAL: can not get port by device 0000:00:05.0!
> 
> Fixed by removing 'port_id_is_invalid()' check from 'detach_port_device()',
> anyway it shouldn't be required. Without this check device remove works as
> expected.
> 
> Only "Invalid port_id=0" logs seen a few times, which is because the actual
> removal not done synchronously but an alarm set for it, until the alarm fired
> application may receive many interrupts, expect the first ones cause the
> error.
> So this patch also removes the logging from checking the invalid port in
> 'rmv_port_callback()' to reduce the noise.
> 
> [1]
> rmv_port_callback()
>   stop_port(port_id);
>   close_port(port_id);
>   detach_port_device(port_id);
> 
> Fixes: 43d0e304980a ("app/testpmd: fix invalid port detaching")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
>  app/test-pmd/testpmd.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> f9f4cd1d3..3323013bb 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2641,9 +2641,6 @@ detach_port_device(portid_t port_id)
> 
>  	printf("Removing a device...\n");
> 
> -	if (port_id_is_invalid(port_id, ENABLED_WARN))
> -		return;
> -

I don't think this is the correct fix.

See the line below - you use the port structure, it makes sense to check its validity before.

>  	dev = rte_eth_devices[port_id].device;
>  	if (dev == NULL) {
>  		printf("Device already removed\n");
> @@ -2875,7 +2872,8 @@ rmv_port_callback(void *arg)
>  	int org_no_link_check = no_link_check;
>  	portid_t port_id = (intptr_t)arg;
> 
> -	RTE_ETH_VALID_PORTID_OR_RET(port_id);
> +	if (!rte_eth_dev_is_valid_port(port_id))
> +		return;
> 
>  	if (!test_done && port_is_forwarding(port_id)) {
>  		need_to_start = 1;

IMO the correct fix is to change this function to get pointer to rte_device instead of port ID and change the Testpmd command line to get the device instead of port ID.

Matan


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] app/testpmd: fix hotplug
  2020-01-23 15:15 [dpdk-dev] [PATCH] app/testpmd: fix hotplug Ferruh Yigit
  2020-01-23 16:00 ` Matan Azrad
@ 2020-01-23 17:01 ` Iremonger, Bernard
  2020-01-23 17:25   ` Ferruh Yigit
  1 sibling, 1 reply; 7+ messages in thread
From: Iremonger, Bernard @ 2020-01-23 17:01 UTC (permalink / raw)
  To: Yigit, Ferruh, Lu, Wenzhuo, Wu, Jingjing, Matan Azrad; +Cc: dev, stable

Hi Ferruh,

> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Thursday, January 23, 2020 3:15 PM
> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>; Matan Azrad <matan@mellanox.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: [PATCH] app/testpmd: fix hotplug
> 
This is reverting the original patch, which breaks hotplug.
I think the commit message should make this clear.
This patch is not fixing hotplug, the original patch seems to have revealed a problem with hotplug.
Reverting this patch will not fix the underlying hotplug issue. 

> The 'port_id_is_invalid()' check in the 'detach_port_device()' is breaking the
> hotplug support, since at that stage port will be closed and validity check
> always fail [1] and removing the device is not really completed.
> 
> But this cause the vfio request interrupt keep triggered continuously and
> makes the application unusable, since port is closed but device is not
> removed, the remove path keep generating error log:
> 
> EAL: can not get port by device 0000:00:05.0!
> EAL: can not get port by device 0000:00:05.0!
> EAL: can not get port by device 0000:00:05.0!
> EAL: can not get port by device 0000:00:05.0!
> EAL: can not get port by device 0000:00:05.0!
> EAL: can not get port by device 0000:00:05.0!
> 
> Fixed by removing 'port_id_is_invalid()' check from 'detach_port_device()',
> anyway it shouldn't be required. Without this check device remove works as
> expected.
> 
> Only "Invalid port_id=0" logs seen a few times, which is because the actual
> removal not done synchronously but an alarm set for it, until the alarm fired
> application may receive many interrupts, expect the first ones cause the
> error.
> So this patch also removes the logging from checking the invalid port in
> 'rmv_port_callback()' to reduce the noise.
> 
> [1]
> rmv_port_callback()
>   stop_port(port_id);
>   close_port(port_id);
>   detach_port_device(port_id);
> 
> Fixes: 43d0e304980a ("app/testpmd: fix invalid port detaching")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
>  app/test-pmd/testpmd.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> f9f4cd1d3..3323013bb 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2641,9 +2641,6 @@ detach_port_device(portid_t port_id)
> 
>  	printf("Removing a device...\n");
> 
> -	if (port_id_is_invalid(port_id, ENABLED_WARN))
> -		return;
> -
>  	dev = rte_eth_devices[port_id].device;
>  	if (dev == NULL) {
>  		printf("Device already removed\n");
> @@ -2875,7 +2872,8 @@ rmv_port_callback(void *arg)
>  	int org_no_link_check = no_link_check;
>  	portid_t port_id = (intptr_t)arg;
> 
> -	RTE_ETH_VALID_PORTID_OR_RET(port_id);
> +	if (!rte_eth_dev_is_valid_port(port_id))
> +		return;
> 
>  	if (!test_done && port_is_forwarding(port_id)) {
>  		need_to_start = 1;
> --
> 2.24.1

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] app/testpmd: fix hotplug
  2020-01-23 17:01 ` Iremonger, Bernard
@ 2020-01-23 17:25   ` Ferruh Yigit
  2020-01-23 17:34     ` Iremonger, Bernard
  0 siblings, 1 reply; 7+ messages in thread
From: Ferruh Yigit @ 2020-01-23 17:25 UTC (permalink / raw)
  To: Iremonger, Bernard, Lu, Wenzhuo, Wu, Jingjing, Matan Azrad; +Cc: dev, stable

On 1/23/2020 5:01 PM, Iremonger, Bernard wrote:
> Hi Ferruh,
> 
>> -----Original Message-----
>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
>> Sent: Thursday, January 23, 2020 3:15 PM
>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing
>> <jingjing.wu@intel.com>; Iremonger, Bernard
>> <bernard.iremonger@intel.com>; Matan Azrad <matan@mellanox.com>
>> Cc: dev@dpdk.org; stable@dpdk.org
>> Subject: [PATCH] app/testpmd: fix hotplug
>>
> This is reverting the original patch, which breaks hotplug.
> I think the commit message should make this clear.
> This patch is not fixing hotplug, the original patch seems to have revealed a problem with hotplug.
> Reverting this patch will not fix the underlying hotplug issue.

What is the problem revealed by the original patch? Did you able to reproduce it?

> 
>> The 'port_id_is_invalid()' check in the 'detach_port_device()' is breaking the
>> hotplug support, since at that stage port will be closed and validity check
>> always fail [1] and removing the device is not really completed.
>>
>> But this cause the vfio request interrupt keep triggered continuously and
>> makes the application unusable, since port is closed but device is not
>> removed, the remove path keep generating error log:
>>
>> EAL: can not get port by device 0000:00:05.0!
>> EAL: can not get port by device 0000:00:05.0!
>> EAL: can not get port by device 0000:00:05.0!
>> EAL: can not get port by device 0000:00:05.0!
>> EAL: can not get port by device 0000:00:05.0!
>> EAL: can not get port by device 0000:00:05.0!
>>
>> Fixed by removing 'port_id_is_invalid()' check from 'detach_port_device()',
>> anyway it shouldn't be required. Without this check device remove works as
>> expected.
>>
>> Only "Invalid port_id=0" logs seen a few times, which is because the actual
>> removal not done synchronously but an alarm set for it, until the alarm fired
>> application may receive many interrupts, expect the first ones cause the
>> error.
>> So this patch also removes the logging from checking the invalid port in
>> 'rmv_port_callback()' to reduce the noise.
>>
>> [1]
>> rmv_port_callback()
>>   stop_port(port_id);
>>   close_port(port_id);
>>   detach_port_device(port_id);
>>
>> Fixes: 43d0e304980a ("app/testpmd: fix invalid port detaching")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> ---
>>  app/test-pmd/testpmd.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>> f9f4cd1d3..3323013bb 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -2641,9 +2641,6 @@ detach_port_device(portid_t port_id)
>>
>>  printf("Removing a device...\n");
>>
>> -if (port_id_is_invalid(port_id, ENABLED_WARN))
>> -return;
>> -
>>  dev = rte_eth_devices[port_id].device;
>>  if (dev == NULL) {
>>  printf("Device already removed\n");
>> @@ -2875,7 +2872,8 @@ rmv_port_callback(void *arg)
>>  int org_no_link_check = no_link_check;
>>  portid_t port_id = (intptr_t)arg;
>>
>> -RTE_ETH_VALID_PORTID_OR_RET(port_id);
>> +if (!rte_eth_dev_is_valid_port(port_id))
>> +return;
>>
>>  if (!test_done && port_is_forwarding(port_id)) {
>>  need_to_start = 1;
>> --
>> 2.24.1
> 
> Otherwise
> Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] app/testpmd: fix hotplug
  2020-01-23 17:25   ` Ferruh Yigit
@ 2020-01-23 17:34     ` Iremonger, Bernard
  2020-01-23 17:46       ` Iremonger, Bernard
  0 siblings, 1 reply; 7+ messages in thread
From: Iremonger, Bernard @ 2020-01-23 17:34 UTC (permalink / raw)
  To: Yigit, Ferruh, Lu, Wenzhuo, Wu, Jingjing, Matan Azrad; +Cc: dev, stable

Hi Ferruh,

<snip>

> >> Subject: [PATCH] app/testpmd: fix hotplug
> >>
> > This is reverting the original patch, which breaks hotplug.
> > I think the commit message should make this clear.
> > This patch is not fixing hotplug, the original patch seems to have revealed a
> problem with hotplug.
> > Reverting this patch will not fix the underlying hotplug issue.
> 
> What is the problem revealed by the original patch? Did you able to
> reproduce it?

No, I thought you had.
There is a lot of information in the commit message, but is not clear that you are reverting a patch. 
I am ok with reverting the original patch as it has caused a problem.

> 
> >
> >> The 'port_id_is_invalid()' check in the 'detach_port_device()' is
> >> breaking the hotplug support, since at that stage port will be closed
> >> and validity check always fail [1] and removing the device is not really
> completed.
> >>
> >> But this cause the vfio request interrupt keep triggered continuously
> >> and makes the application unusable, since port is closed but device
> >> is not removed, the remove path keep generating error log:
> >>
> >> EAL: can not get port by device 0000:00:05.0!
> >> EAL: can not get port by device 0000:00:05.0!
> >> EAL: can not get port by device 0000:00:05.0!
> >> EAL: can not get port by device 0000:00:05.0!
> >> EAL: can not get port by device 0000:00:05.0!
> >> EAL: can not get port by device 0000:00:05.0!
> >>
> >> Fixed by removing 'port_id_is_invalid()' check from
> >> 'detach_port_device()', anyway it shouldn't be required. Without this
> >> check device remove works as expected.
> >>
> >> Only "Invalid port_id=0" logs seen a few times, which is because the
> >> actual removal not done synchronously but an alarm set for it, until
> >> the alarm fired application may receive many interrupts, expect the
> >> first ones cause the error.
> >> So this patch also removes the logging from checking the invalid port
> >> in 'rmv_port_callback()' to reduce the noise.
> >>
> >> [1]
> >> rmv_port_callback()
> >>   stop_port(port_id);
> >>   close_port(port_id);
> >>   detach_port_device(port_id);
> >>
> >> Fixes: 43d0e304980a ("app/testpmd: fix invalid port detaching")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >> ---
> >>  app/test-pmd/testpmd.c | 6 ++----
> >>  1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> >> f9f4cd1d3..3323013bb 100644
> >> --- a/app/test-pmd/testpmd.c
> >> +++ b/app/test-pmd/testpmd.c
> >> @@ -2641,9 +2641,6 @@ detach_port_device(portid_t port_id)
> >>
> >>  printf("Removing a device...\n");
> >>
> >> -if (port_id_is_invalid(port_id, ENABLED_WARN)) -return;
> >> -
> >>  dev = rte_eth_devices[port_id].device;  if (dev == NULL) {
> >> printf("Device already removed\n"); @@ -2875,7 +2872,8 @@
> >> rmv_port_callback(void *arg)  int org_no_link_check = no_link_check;
> >> portid_t port_id = (intptr_t)arg;
> >>
> >> -RTE_ETH_VALID_PORTID_OR_RET(port_id);
> >> +if (!rte_eth_dev_is_valid_port(port_id))
> >> +return;
> >>
> >>  if (!test_done && port_is_forwarding(port_id)) {  need_to_start = 1;
> >> --
> >> 2.24.1
> >
> > Otherwise
> > Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>
> >
Regards,

Bernard.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] app/testpmd: fix hotplug
  2020-01-23 17:34     ` Iremonger, Bernard
@ 2020-01-23 17:46       ` Iremonger, Bernard
  2020-01-23 18:25         ` Ferruh Yigit
  0 siblings, 1 reply; 7+ messages in thread
From: Iremonger, Bernard @ 2020-01-23 17:46 UTC (permalink / raw)
  To: Iremonger, Bernard, Yigit, Ferruh, Lu, Wenzhuo, Wu, Jingjing,
	Matan Azrad
  Cc: dev, stable

Hi Ferruh,

> <snip>
> 
> > >> Subject: [PATCH] app/testpmd: fix hotplug
> > >>
> > > This is reverting the original patch, which breaks hotplug.
> > > I think the commit message should make this clear.
> > > This patch is not fixing hotplug, the original patch seems to have
> > > revealed a
> > problem with hotplug.
> > > Reverting this patch will not fix the underlying hotplug issue.
> >
> > What is the problem revealed by the original patch? Did you able to
> > reproduce it?
> 
> No, I thought you had.
> There is a lot of information in the commit message, but is not clear that you
> are reverting a patch.
> I am ok with reverting the original patch as it has caused a problem.

Just had another look at the patch, there are two changes.
One reverts the original patch.
The second is a fix.
Suggest splitting patch into two, one for revert and second one for fix.

> 
> >
> > >
> > >> The 'port_id_is_invalid()' check in the 'detach_port_device()' is
> > >> breaking the hotplug support, since at that stage port will be
> > >> closed and validity check always fail [1] and removing the device
> > >> is not really
> > completed.
> > >>
> > >> But this cause the vfio request interrupt keep triggered
> > >> continuously and makes the application unusable, since port is
> > >> closed but device is not removed, the remove path keep generating
> error log:
> > >>
> > >> EAL: can not get port by device 0000:00:05.0!
> > >> EAL: can not get port by device 0000:00:05.0!
> > >> EAL: can not get port by device 0000:00:05.0!
> > >> EAL: can not get port by device 0000:00:05.0!
> > >> EAL: can not get port by device 0000:00:05.0!
> > >> EAL: can not get port by device 0000:00:05.0!
> > >>
> > >> Fixed by removing 'port_id_is_invalid()' check from
> > >> 'detach_port_device()', anyway it shouldn't be required. Without
> > >> this check device remove works as expected.
> > >>
> > >> Only "Invalid port_id=0" logs seen a few times, which is because
> > >> the actual removal not done synchronously but an alarm set for it,
> > >> until the alarm fired application may receive many interrupts,
> > >> expect the first ones cause the error.
> > >> So this patch also removes the logging from checking the invalid
> > >> port in 'rmv_port_callback()' to reduce the noise.
> > >>
> > >> [1]
> > >> rmv_port_callback()
> > >>   stop_port(port_id);
> > >>   close_port(port_id);
> > >>   detach_port_device(port_id);
> > >>
> > >> Fixes: 43d0e304980a ("app/testpmd: fix invalid port detaching")
> > >> Cc: stable@dpdk.org
> > >>
> > >> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > >> ---
> > >>  app/test-pmd/testpmd.c | 6 ++----
> > >>  1 file changed, 2 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > >> f9f4cd1d3..3323013bb 100644
> > >> --- a/app/test-pmd/testpmd.c
> > >> +++ b/app/test-pmd/testpmd.c
> > >> @@ -2641,9 +2641,6 @@ detach_port_device(portid_t port_id)
> > >>
> > >>  printf("Removing a device...\n");
> > >>
> > >> -if (port_id_is_invalid(port_id, ENABLED_WARN)) -return;
> > >> -
> > >>  dev = rte_eth_devices[port_id].device;  if (dev == NULL) {
> > >> printf("Device already removed\n"); @@ -2875,7 +2872,8 @@
> > >> rmv_port_callback(void *arg)  int org_no_link_check =
> > >> no_link_check; portid_t port_id = (intptr_t)arg;
> > >>
> > >> -RTE_ETH_VALID_PORTID_OR_RET(port_id);
> > >> +if (!rte_eth_dev_is_valid_port(port_id))
> > >> +return;
> > >>
> > >>  if (!test_done && port_is_forwarding(port_id)) {  need_to_start =
> > >> 1;
> > >> --
> > >> 2.24.1
> > >
> > > Otherwise
> > > Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>
> > >
 Regards,
 
 Bernard.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] app/testpmd: fix hotplug
  2020-01-23 17:46       ` Iremonger, Bernard
@ 2020-01-23 18:25         ` Ferruh Yigit
  0 siblings, 0 replies; 7+ messages in thread
From: Ferruh Yigit @ 2020-01-23 18:25 UTC (permalink / raw)
  To: Iremonger, Bernard, Lu, Wenzhuo, Wu, Jingjing, Matan Azrad; +Cc: dev, stable

On 1/23/2020 5:46 PM, Iremonger, Bernard wrote:
> Hi Ferruh,
> 
>> <snip>
>>
>>>>> Subject: [PATCH] app/testpmd: fix hotplug
>>>>>
>>>> This is reverting the original patch, which breaks hotplug.
>>>> I think the commit message should make this clear.
>>>> This patch is not fixing hotplug, the original patch seems to have
>>>> revealed a
>>> problem with hotplug.
>>>> Reverting this patch will not fix the underlying hotplug issue.
>>>
>>> What is the problem revealed by the original patch? Did you able to
>>> reproduce it?
>>
>> No, I thought you had.

I can't reproduce the problem mentioned in original patch, but can reproduce
now, so from my perspective "fixing testpmd hotplug"

>> There is a lot of information in the commit message, but is not clear that you
>> are reverting a patch.

OK I can add a note

>> I am ok with reverting the original patch as it has caused a problem.
> 
> Just had another look at the patch, there are two changes.
> One reverts the original patch.
> The second is a fix.

Not exactly, fix is reverting, second change is removing log which is to reduce
the noise.
Both changes mentioned in the commit log.

> Suggest splitting patch into two, one for revert and second one for fix.
> 
>>
>>>
>>>>
>>>>> The 'port_id_is_invalid()' check in the 'detach_port_device()' is
>>>>> breaking the hotplug support, since at that stage port will be
>>>>> closed and validity check always fail [1] and removing the device
>>>>> is not really
>>> completed.
>>>>>
>>>>> But this cause the vfio request interrupt keep triggered
>>>>> continuously and makes the application unusable, since port is
>>>>> closed but device is not removed, the remove path keep generating
>> error log:
>>>>>
>>>>> EAL: can not get port by device 0000:00:05.0!
>>>>> EAL: can not get port by device 0000:00:05.0!
>>>>> EAL: can not get port by device 0000:00:05.0!
>>>>> EAL: can not get port by device 0000:00:05.0!
>>>>> EAL: can not get port by device 0000:00:05.0!
>>>>> EAL: can not get port by device 0000:00:05.0!
>>>>>
>>>>> Fixed by removing 'port_id_is_invalid()' check from
>>>>> 'detach_port_device()', anyway it shouldn't be required. Without
>>>>> this check device remove works as expected.
>>>>>
>>>>> Only "Invalid port_id=0" logs seen a few times, which is because
>>>>> the actual removal not done synchronously but an alarm set for it,
>>>>> until the alarm fired application may receive many interrupts,
>>>>> expect the first ones cause the error.
>>>>> So this patch also removes the logging from checking the invalid
>>>>> port in 'rmv_port_callback()' to reduce the noise.
>>>>>
>>>>> [1]
>>>>> rmv_port_callback()
>>>>>   stop_port(port_id);
>>>>>   close_port(port_id);
>>>>>   detach_port_device(port_id);
>>>>>
>>>>> Fixes: 43d0e304980a ("app/testpmd: fix invalid port detaching")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>> ---
>>>>>  app/test-pmd/testpmd.c | 6 ++----
>>>>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>>>> f9f4cd1d3..3323013bb 100644
>>>>> --- a/app/test-pmd/testpmd.c
>>>>> +++ b/app/test-pmd/testpmd.c
>>>>> @@ -2641,9 +2641,6 @@ detach_port_device(portid_t port_id)
>>>>>
>>>>>  printf("Removing a device...\n");
>>>>>
>>>>> -if (port_id_is_invalid(port_id, ENABLED_WARN)) -return;
>>>>> -
>>>>>  dev = rte_eth_devices[port_id].device;  if (dev == NULL) {
>>>>> printf("Device already removed\n"); @@ -2875,7 +2872,8 @@
>>>>> rmv_port_callback(void *arg)  int org_no_link_check =
>>>>> no_link_check; portid_t port_id = (intptr_t)arg;
>>>>>
>>>>> -RTE_ETH_VALID_PORTID_OR_RET(port_id);
>>>>> +if (!rte_eth_dev_is_valid_port(port_id))
>>>>> +return;
>>>>>
>>>>>  if (!test_done && port_is_forwarding(port_id)) {  need_to_start =
>>>>> 1;
>>>>> --
>>>>> 2.24.1
>>>>
>>>> Otherwise
>>>> Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>
>>>>
>  Regards,
> 
>  Bernard.
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-01-23 18:32 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-23 15:15 [dpdk-dev] [PATCH] app/testpmd: fix hotplug Ferruh Yigit
2020-01-23 16:00 ` Matan Azrad
2020-01-23 17:01 ` Iremonger, Bernard
2020-01-23 17:25   ` Ferruh Yigit
2020-01-23 17:34     ` Iremonger, Bernard
2020-01-23 17:46       ` Iremonger, Bernard
2020-01-23 18:25         ` Ferruh Yigit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).