mbox series

[v4,0/2] Fix testpmd interrupt regression

Message ID 20230315173132.4044-1-stephen@networkplumber.org (mailing list archive)
Headers
Series Fix testpmd interrupt regression |

Message

Stephen Hemminger March 15, 2023, 5:31 p.m. UTC 
  Resolve issues from using control-C in testpmd.
Fixes regression from recent change to use cmdline_poll().

v4 - drop sig_atomic_t. Not required requires changes on
     some platforms.

Stephen Hemminger (2):
  testpmd: go back to using cmdline_interact
  testpmd: enable interrupt in interactive mode

 app/test-pmd/cmdline.c           | 27 ++++++++++++++-------------
 app/test-pmd/testpmd.c           | 11 +++++++++++
 lib/cmdline/cmdline.h            | 10 ++++++++++
 lib/cmdline/cmdline_os_unix.c    |  8 +++++++-
 lib/cmdline/cmdline_os_windows.c | 18 ++++++++++++++++--
 lib/cmdline/cmdline_private.h    |  2 +-
 lib/cmdline/version.map          |  3 +++
 7 files changed, 62 insertions(+), 17 deletions(-)

Comments

Pier Damouny March 16, 2023, 8:16 a.m. UTC | #1 
> External email: Use caution opening links or attachments
> 
> 
> Resolve issues from using control-C in testpmd.
> Fixes regression from recent change to use cmdline_poll().
> 
> v4 - drop sig_atomic_t. Not required requires changes on
>      some platforms.
> 
> Stephen Hemminger (2):
>   testpmd: go back to using cmdline_interact
>   testpmd: enable interrupt in interactive mode
> 
>  app/test-pmd/cmdline.c           | 27 ++++++++++++++-------------
>  app/test-pmd/testpmd.c           | 11 +++++++++++
>  lib/cmdline/cmdline.h            | 10 ++++++++++
>  lib/cmdline/cmdline_os_unix.c    |  8 +++++++-
>  lib/cmdline/cmdline_os_windows.c | 18 ++++++++++++++++--
>  lib/cmdline/cmdline_private.h    |  2 +-
>  lib/cmdline/version.map          |  3 +++
>  7 files changed, 62 insertions(+), 17 deletions(-)
> 
> --
> 2.39.2

Confirming that this patch fixes Bug: 1180 over Windows.
And CTRL-C works as expected.

Tested-by: Pier Damouny <pdamouny@nvidia.com>
Ferruh Yigit March 16, 2023, 12:20 p.m. UTC | #2 
On 3/15/2023 5:31 PM, Stephen Hemminger wrote:
> Resolve issues from using control-C in testpmd.
> Fixes regression from recent change to use cmdline_poll().
> 
> v4 - drop sig_atomic_t. Not required requires changes on
>      some platforms.
> 
> Stephen Hemminger (2):
>   testpmd: go back to using cmdline_interact
>   testpmd: enable interrupt in interactive mode
> 
>  app/test-pmd/cmdline.c           | 27 ++++++++++++++-------------
>  app/test-pmd/testpmd.c           | 11 +++++++++++
>  lib/cmdline/cmdline.h            | 10 ++++++++++
>  lib/cmdline/cmdline_os_unix.c    |  8 +++++++-
>  lib/cmdline/cmdline_os_windows.c | 18 ++++++++++++++++--
>  lib/cmdline/cmdline_private.h    |  2 +-
>  lib/cmdline/version.map          |  3 +++
>  7 files changed, 62 insertions(+), 17 deletions(-)
> 

This solution is adding new cmdline API for -rc3 and there are some
testpmd changes,
can it be possible to have a simple workaround specific to window for
this release and get this set for next release?
Stephen Hemminger March 16, 2023, 3:31 p.m. UTC | #3 
On Thu, 16 Mar 2023 12:20:41 +0000
Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> On 3/15/2023 5:31 PM, Stephen Hemminger wrote:
> > Resolve issues from using control-C in testpmd.
> > Fixes regression from recent change to use cmdline_poll().
> > 
> > v4 - drop sig_atomic_t. Not required requires changes on
> >      some platforms.
> > 
> > Stephen Hemminger (2):
> >   testpmd: go back to using cmdline_interact
> >   testpmd: enable interrupt in interactive mode
> > 
> >  app/test-pmd/cmdline.c           | 27 ++++++++++++++-------------
> >  app/test-pmd/testpmd.c           | 11 +++++++++++
> >  lib/cmdline/cmdline.h            | 10 ++++++++++
> >  lib/cmdline/cmdline_os_unix.c    |  8 +++++++-
> >  lib/cmdline/cmdline_os_windows.c | 18 ++++++++++++++++--
> >  lib/cmdline/cmdline_private.h    |  2 +-
> >  lib/cmdline/version.map          |  3 +++
> >  7 files changed, 62 insertions(+), 17 deletions(-)
> >   
> 
> This solution is adding new cmdline API for -rc3 and there are some
> testpmd changes,
> can it be possible to have a simple workaround specific to window for
> this release and get this set for next release?


Not really. cmdline_poll() is broken in several ways.
Don't want to fix it or use it.
Ferruh Yigit March 16, 2023, 5:01 p.m. UTC | #4 
On 3/16/2023 3:31 PM, Stephen Hemminger wrote:
> On Thu, 16 Mar 2023 12:20:41 +0000
> Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> 
>> On 3/15/2023 5:31 PM, Stephen Hemminger wrote:
>>> Resolve issues from using control-C in testpmd.
>>> Fixes regression from recent change to use cmdline_poll().
>>>
>>> v4 - drop sig_atomic_t. Not required requires changes on
>>>      some platforms.
>>>
>>> Stephen Hemminger (2):
>>>   testpmd: go back to using cmdline_interact
>>>   testpmd: enable interrupt in interactive mode
>>>
>>>  app/test-pmd/cmdline.c           | 27 ++++++++++++++-------------
>>>  app/test-pmd/testpmd.c           | 11 +++++++++++
>>>  lib/cmdline/cmdline.h            | 10 ++++++++++
>>>  lib/cmdline/cmdline_os_unix.c    |  8 +++++++-
>>>  lib/cmdline/cmdline_os_windows.c | 18 ++++++++++++++++--
>>>  lib/cmdline/cmdline_private.h    |  2 +-
>>>  lib/cmdline/version.map          |  3 +++
>>>  7 files changed, 62 insertions(+), 17 deletions(-)
>>>   
>>
>> This solution is adding new cmdline API for -rc3 and there are some
>> testpmd changes,
>> can it be possible to have a simple workaround specific to window for
>> this release and get this set for next release?
> 
> 
> Not really. cmdline_poll() is broken in several ways.
> Don't want to fix it or use it.

What about to revert the original fix [^1] in this release and get a new
version of it at early next release?

@Thomas, what do you think? I think better to decide before -rc3.


[^1]
Fixes: 0fd1386c30c3 ("app/testpmd: cleanup cleanly from signal")
Thomas Monjalon March 16, 2023, 5:05 p.m. UTC | #5 
16/03/2023 18:01, Ferruh Yigit:
> On 3/16/2023 3:31 PM, Stephen Hemminger wrote:
> > On Thu, 16 Mar 2023 12:20:41 +0000
> > Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> > 
> >> On 3/15/2023 5:31 PM, Stephen Hemminger wrote:
> >>> Resolve issues from using control-C in testpmd.
> >>> Fixes regression from recent change to use cmdline_poll().
> >>>
> >>> v4 - drop sig_atomic_t. Not required requires changes on
> >>>      some platforms.
> >>>
> >>> Stephen Hemminger (2):
> >>>   testpmd: go back to using cmdline_interact
> >>>   testpmd: enable interrupt in interactive mode
> >>>
> >>>  app/test-pmd/cmdline.c           | 27 ++++++++++++++-------------
> >>>  app/test-pmd/testpmd.c           | 11 +++++++++++
> >>>  lib/cmdline/cmdline.h            | 10 ++++++++++
> >>>  lib/cmdline/cmdline_os_unix.c    |  8 +++++++-
> >>>  lib/cmdline/cmdline_os_windows.c | 18 ++++++++++++++++--
> >>>  lib/cmdline/cmdline_private.h    |  2 +-
> >>>  lib/cmdline/version.map          |  3 +++
> >>>  7 files changed, 62 insertions(+), 17 deletions(-)
> >>>   
> >>
> >> This solution is adding new cmdline API for -rc3 and there are some
> >> testpmd changes,
> >> can it be possible to have a simple workaround specific to window for
> >> this release and get this set for next release?
> > 
> > 
> > Not really. cmdline_poll() is broken in several ways.
> > Don't want to fix it or use it.
> 
> What about to revert the original fix [^1] in this release and get a new
> version of it at early next release?
> 
> @Thomas, what do you think? I think better to decide before -rc3.

We should not add risky changes at this stage.
Also I would prefer to see a review from Olivier, the cmdline maintainer.

The reason of this breakage was a cleanup.
I think it is more reasonnable to revert.
Ferruh Yigit March 16, 2023, 5:36 p.m. UTC | #6 
On 3/16/2023 5:05 PM, Thomas Monjalon wrote:
> 16/03/2023 18:01, Ferruh Yigit:
>> On 3/16/2023 3:31 PM, Stephen Hemminger wrote:
>>> On Thu, 16 Mar 2023 12:20:41 +0000
>>> Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>>>
>>>> On 3/15/2023 5:31 PM, Stephen Hemminger wrote:
>>>>> Resolve issues from using control-C in testpmd.
>>>>> Fixes regression from recent change to use cmdline_poll().
>>>>>
>>>>> v4 - drop sig_atomic_t. Not required requires changes on
>>>>>      some platforms.
>>>>>
>>>>> Stephen Hemminger (2):
>>>>>   testpmd: go back to using cmdline_interact
>>>>>   testpmd: enable interrupt in interactive mode
>>>>>
>>>>>  app/test-pmd/cmdline.c           | 27 ++++++++++++++-------------
>>>>>  app/test-pmd/testpmd.c           | 11 +++++++++++
>>>>>  lib/cmdline/cmdline.h            | 10 ++++++++++
>>>>>  lib/cmdline/cmdline_os_unix.c    |  8 +++++++-
>>>>>  lib/cmdline/cmdline_os_windows.c | 18 ++++++++++++++++--
>>>>>  lib/cmdline/cmdline_private.h    |  2 +-
>>>>>  lib/cmdline/version.map          |  3 +++
>>>>>  7 files changed, 62 insertions(+), 17 deletions(-)
>>>>>   
>>>>
>>>> This solution is adding new cmdline API for -rc3 and there are some
>>>> testpmd changes,
>>>> can it be possible to have a simple workaround specific to window for
>>>> this release and get this set for next release?
>>>
>>>
>>> Not really. cmdline_poll() is broken in several ways.
>>> Don't want to fix it or use it.
>>
>> What about to revert the original fix [^1] in this release and get a new
>> version of it at early next release?
>>
>> @Thomas, what do you think? I think better to decide before -rc3.
> 
> We should not add risky changes at this stage.
> Also I would prefer to see a review from Olivier, the cmdline maintainer.
> 
> The reason of this breakage was a cleanup.
> I think it is more reasonnable to revert.
> 
>

Sent a revert patch [^1], and assigned to Thomas.

[^1]
https://patches.dpdk.org/project/dpdk/patch/20230316172739.77933-1-ferruh.yigit@amd.com/