app/testpmd: fix hotplug

  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(-)
  

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
  

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>
  

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>
>
  

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.
  

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.
  

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.
>