mbox series

[V4,0/5] app/testpmd: support mulitple process attach and detach port

Message ID 20221206092649.8287-1-lihuisong@huawei.com (mailing list archive)
Headers
Series app/testpmd: support mulitple process attach and detach port |

Message

lihuisong (C) Dec. 6, 2022, 9:26 a.m. UTC 
  This patchset fix some bugs and support attaching and detaching port
in primary and secondary.

---
 -v4: fix a misspelling. 
 -v3:
   1) merge patch 1/6 and patch 2/6 into patch 1/5, and add modification
      for other bus type.
   2) add a RTE_ETH_DEV_ALLOCATED state in rte_eth_dev_state to resolve
      the probelm in patch 2/5. 
 -v2: resend due to CI unexplained failure.

Huisong Li (5):
  drivers/bus: restore driver assignment at front of probing
  ethdev: fix skip valid port in probing callback
  app/testpmd: check the validity of the port
  app/testpmd: add attach and detach port for multiple process
  app/testpmd: stop forwarding in new or destroy event

 app/test-pmd/testpmd.c                   | 47 +++++++++++++++---------
 app/test-pmd/testpmd.h                   |  1 -
 drivers/bus/auxiliary/auxiliary_common.c |  9 ++++-
 drivers/bus/dpaa/dpaa_bus.c              |  9 ++++-
 drivers/bus/fslmc/fslmc_bus.c            |  8 +++-
 drivers/bus/ifpga/ifpga_bus.c            | 12 ++++--
 drivers/bus/pci/pci_common.c             |  9 ++++-
 drivers/bus/vdev/vdev.c                  | 10 ++++-
 drivers/bus/vmbus/vmbus_common.c         |  9 ++++-
 drivers/net/bnxt/bnxt_ethdev.c           |  3 +-
 drivers/net/bonding/bonding_testpmd.c    |  1 -
 drivers/net/mlx5/mlx5.c                  |  2 +-
 lib/ethdev/ethdev_driver.c               | 13 +++++--
 lib/ethdev/ethdev_driver.h               | 12 ++++++
 lib/ethdev/ethdev_pci.h                  |  2 +-
 lib/ethdev/rte_class_eth.c               |  2 +-
 lib/ethdev/rte_ethdev.c                  |  4 +-
 lib/ethdev/rte_ethdev.h                  |  4 +-
 lib/ethdev/version.map                   |  1 +
 19 files changed, 114 insertions(+), 44 deletions(-)

Comments

lihuisong (C) Jan. 9, 2023, 12:38 p.m. UTC | #1 
Hi Thomas and Ferruh,

Could you take a look at this patch series?
I have modified it according to our last discussion.

Best,
Huisong

在 2022/12/6 17:26, Huisong Li 写道:
> This patchset fix some bugs and support attaching and detaching port
> in primary and secondary.
>
> ---
>   -v4: fix a misspelling.
>   -v3:
>     1) merge patch 1/6 and patch 2/6 into patch 1/5, and add modification
>        for other bus type.
>     2) add a RTE_ETH_DEV_ALLOCATED state in rte_eth_dev_state to resolve
>        the probelm in patch 2/5.
>   -v2: resend due to CI unexplained failure.
>
> Huisong Li (5):
>    drivers/bus: restore driver assignment at front of probing
>    ethdev: fix skip valid port in probing callback
>    app/testpmd: check the validity of the port
>    app/testpmd: add attach and detach port for multiple process
>    app/testpmd: stop forwarding in new or destroy event
>
>   app/test-pmd/testpmd.c                   | 47 +++++++++++++++---------
>   app/test-pmd/testpmd.h                   |  1 -
>   drivers/bus/auxiliary/auxiliary_common.c |  9 ++++-
>   drivers/bus/dpaa/dpaa_bus.c              |  9 ++++-
>   drivers/bus/fslmc/fslmc_bus.c            |  8 +++-
>   drivers/bus/ifpga/ifpga_bus.c            | 12 ++++--
>   drivers/bus/pci/pci_common.c             |  9 ++++-
>   drivers/bus/vdev/vdev.c                  | 10 ++++-
>   drivers/bus/vmbus/vmbus_common.c         |  9 ++++-
>   drivers/net/bnxt/bnxt_ethdev.c           |  3 +-
>   drivers/net/bonding/bonding_testpmd.c    |  1 -
>   drivers/net/mlx5/mlx5.c                  |  2 +-
>   lib/ethdev/ethdev_driver.c               | 13 +++++--
>   lib/ethdev/ethdev_driver.h               | 12 ++++++
>   lib/ethdev/ethdev_pci.h                  |  2 +-
>   lib/ethdev/rte_class_eth.c               |  2 +-
>   lib/ethdev/rte_ethdev.c                  |  4 +-
>   lib/ethdev/rte_ethdev.h                  |  4 +-
>   lib/ethdev/version.map                   |  1 +
>   19 files changed, 114 insertions(+), 44 deletions(-)
>
Ferruh Yigit Jan. 10, 2023, 4:51 p.m. UTC | #2 
On 12/6/2022 9:26 AM, Huisong Li wrote:
> This patchset fix some bugs and support attaching and detaching port
> in primary and secondary.
> 
> ---
>  -v4: fix a misspelling. 
>  -v3:
>    1) merge patch 1/6 and patch 2/6 into patch 1/5, and add modification
>       for other bus type.
>    2) add a RTE_ETH_DEV_ALLOCATED state in rte_eth_dev_state to resolve
>       the probelm in patch 2/5. 
>  -v2: resend due to CI unexplained failure.
> 
> Huisong Li (5):
>   drivers/bus: restore driver assignment at front of probing
>   ethdev: fix skip valid port in probing callback
>   app/testpmd: check the validity of the port
>   app/testpmd: add attach and detach port for multiple process
>   app/testpmd: stop forwarding in new or destroy event
> 

Hi Huisong,

I haven't checked the patch in detail yet, but I can see it gives some
ABI compatibility warnings, is this expected:


1 function with some indirect sub-type change:

  [C] 'function int dpaa_eth_eventq_attach(const rte_eth_dev*, int, u16,
const rte_event_eth_rx_adapter_queue_conf*)' at dpaa_ethdev.c:1149:1 has
some indirect sub-type changes:
    parameter 1 of type 'const rte_eth_dev*' has sub-type changes:
      in pointed to type 'const rte_eth_dev':
        in unqualified underlying type 'struct rte_eth_dev' at
ethdev_driver.h:50:1:
          type size hasn't changed
          1 data member change:
            type of 'rte_eth_dev_state state' changed:
              type size hasn't changed
              1 enumerator insertion:
                'rte_eth_dev_state::RTE_ETH_DEV_ALLOCATED' value '1'
              2 enumerator changes:
                'rte_eth_dev_state::RTE_ETH_DEV_ATTACHED' from value '1'
to '2' at rte_ethdev.h:2000:1
                'rte_eth_dev_state::RTE_ETH_DEV_REMOVED' from value '2'
to '3' at rte_ethdev.h:2000:1

1 function with some indirect sub-type change:

  [C] 'function int rte_pmd_i40e_set_switch_dev(uint16_t, rte_eth_dev*)'
at rte_pmd_i40e.c:3266:1 has some indirect sub-type changes:
    parameter 2 of type 'rte_eth_dev*' has sub-type changes:
      in pointed to type 'struct rte_eth_dev' at ethdev_driver.h:50:1:
        type size hasn't changed
        1 data member change:
          type of 'rte_eth_dev_state state' changed:
            type size hasn't changed
            1 enumerator insertion:
              'rte_eth_dev_state::RTE_ETH_DEV_ALLOCATED' value '1'
            2 enumerator changes:
              'rte_eth_dev_state::RTE_ETH_DEV_ATTACHED' from value '1'
to '2' at rte_ethdev.h:2000:1
              'rte_eth_dev_state::RTE_ETH_DEV_REMOVED' from value '2' to
'3' at rte_ethdev.h:2000:1

1 function with some indirect sub-type change:

  [C] 'function rte_eth_dev* rte_eth_dev_allocate(const char*)' at
ethdev_driver.c:72:1 has some indirect sub-type changes:
    return type changed:
      in pointed to type 'struct rte_eth_dev' at ethdev_driver.h:50:1:
        type size hasn't changed
        1 data member change:
          type of 'rte_eth_dev_state state' changed:
            type size hasn't changed
            1 enumerator insertion:
              'rte_eth_dev_state::RTE_ETH_DEV_ALLOCATED' value '1'
            2 enumerator changes:
              'rte_eth_dev_state::RTE_ETH_DEV_ATTACHED' from value '1'
to '2' at rte_ethdev.h:2000:1
              'rte_eth_dev_state::RTE_ETH_DEV_REMOVED' from value '2' to
'3' at rte_ethdev.h:2000:1

... there are more warnings for same issue ...
lihuisong (C) Jan. 11, 2023, 12:53 a.m. UTC | #3 
在 2023/1/11 0:51, Ferruh Yigit 写道:
> On 12/6/2022 9:26 AM, Huisong Li wrote:
>> This patchset fix some bugs and support attaching and detaching port
>> in primary and secondary.
>>
>> ---
>>   -v4: fix a misspelling.
>>   -v3:
>>     1) merge patch 1/6 and patch 2/6 into patch 1/5, and add modification
>>        for other bus type.
>>     2) add a RTE_ETH_DEV_ALLOCATED state in rte_eth_dev_state to resolve
>>        the probelm in patch 2/5.
>>   -v2: resend due to CI unexplained failure.
>>
>> Huisong Li (5):
>>    drivers/bus: restore driver assignment at front of probing
>>    ethdev: fix skip valid port in probing callback
>>    app/testpmd: check the validity of the port
>>    app/testpmd: add attach and detach port for multiple process
>>    app/testpmd: stop forwarding in new or destroy event
>>
> Hi Huisong,
>
> I haven't checked the patch in detail yet, but I can see it gives some
> ABI compatibility warnings, is this expected:
This is to be expected. Because we insert a device state, 
RTE_ETH_DEV_ALLOCATED,
before RTE_ETH_DEV_ATTACHED for resolving the issue patch 2/5 mentioned.
We may have to announce it. What do you think?
>
> 1 function with some indirect sub-type change:
>
>    [C] 'function int dpaa_eth_eventq_attach(const rte_eth_dev*, int, u16,
> const rte_event_eth_rx_adapter_queue_conf*)' at dpaa_ethdev.c:1149:1 has
> some indirect sub-type changes:
>      parameter 1 of type 'const rte_eth_dev*' has sub-type changes:
>        in pointed to type 'const rte_eth_dev':
>          in unqualified underlying type 'struct rte_eth_dev' at
> ethdev_driver.h:50:1:
>            type size hasn't changed
>            1 data member change:
>              type of 'rte_eth_dev_state state' changed:
>                type size hasn't changed
>                1 enumerator insertion:
>                  'rte_eth_dev_state::RTE_ETH_DEV_ALLOCATED' value '1'
>                2 enumerator changes:
>                  'rte_eth_dev_state::RTE_ETH_DEV_ATTACHED' from value '1'
> to '2' at rte_ethdev.h:2000:1
>                  'rte_eth_dev_state::RTE_ETH_DEV_REMOVED' from value '2'
> to '3' at rte_ethdev.h:2000:1
>
> 1 function with some indirect sub-type change:
>
>    [C] 'function int rte_pmd_i40e_set_switch_dev(uint16_t, rte_eth_dev*)'
> at rte_pmd_i40e.c:3266:1 has some indirect sub-type changes:
>      parameter 2 of type 'rte_eth_dev*' has sub-type changes:
>        in pointed to type 'struct rte_eth_dev' at ethdev_driver.h:50:1:
>          type size hasn't changed
>          1 data member change:
>            type of 'rte_eth_dev_state state' changed:
>              type size hasn't changed
>              1 enumerator insertion:
>                'rte_eth_dev_state::RTE_ETH_DEV_ALLOCATED' value '1'
>              2 enumerator changes:
>                'rte_eth_dev_state::RTE_ETH_DEV_ATTACHED' from value '1'
> to '2' at rte_ethdev.h:2000:1
>                'rte_eth_dev_state::RTE_ETH_DEV_REMOVED' from value '2' to
> '3' at rte_ethdev.h:2000:1
>
> 1 function with some indirect sub-type change:
>
>    [C] 'function rte_eth_dev* rte_eth_dev_allocate(const char*)' at
> ethdev_driver.c:72:1 has some indirect sub-type changes:
>      return type changed:
>        in pointed to type 'struct rte_eth_dev' at ethdev_driver.h:50:1:
>          type size hasn't changed
>          1 data member change:
>            type of 'rte_eth_dev_state state' changed:
>              type size hasn't changed
>              1 enumerator insertion:
>                'rte_eth_dev_state::RTE_ETH_DEV_ALLOCATED' value '1'
>              2 enumerator changes:
>                'rte_eth_dev_state::RTE_ETH_DEV_ATTACHED' from value '1'
> to '2' at rte_ethdev.h:2000:1
>                'rte_eth_dev_state::RTE_ETH_DEV_REMOVED' from value '2' to
> '3' at rte_ethdev.h:2000:1
>
> ... there are more warnings for same issue ...
>
> .
Ferruh Yigit Jan. 11, 2023, 10:27 a.m. UTC | #4 
On 1/11/2023 12:53 AM, lihuisong (C) wrote:
> 
> 在 2023/1/11 0:51, Ferruh Yigit 写道:
>> On 12/6/2022 9:26 AM, Huisong Li wrote:
>>> This patchset fix some bugs and support attaching and detaching port
>>> in primary and secondary.
>>>
>>> ---
>>>   -v4: fix a misspelling.
>>>   -v3:
>>>     1) merge patch 1/6 and patch 2/6 into patch 1/5, and add
>>> modification
>>>        for other bus type.
>>>     2) add a RTE_ETH_DEV_ALLOCATED state in rte_eth_dev_state to resolve
>>>        the probelm in patch 2/5.
>>>   -v2: resend due to CI unexplained failure.
>>>
>>> Huisong Li (5):
>>>    drivers/bus: restore driver assignment at front of probing
>>>    ethdev: fix skip valid port in probing callback
>>>    app/testpmd: check the validity of the port
>>>    app/testpmd: add attach and detach port for multiple process
>>>    app/testpmd: stop forwarding in new or destroy event
>>>
>> Hi Huisong,
>>
>> I haven't checked the patch in detail yet, but I can see it gives some
>> ABI compatibility warnings, is this expected:
> This is to be expected. Because we insert a device state,
> RTE_ETH_DEV_ALLOCATED,
> before RTE_ETH_DEV_ATTACHED for resolving the issue patch 2/5 mentioned.
> We may have to announce it. What do you think?

If there is an actual ABI break, it can't go in this release, need to
wait LTS release and yes needs deprecation notice in advance.

But not all enum value change warnings are real break, need to
investigate all warnings one by one.
Need to investigate if old application & new dpdk library may cause any
unexpected behavior for application.

>>
>> 1 function with some indirect sub-type change:
>>
>>    [C] 'function int dpaa_eth_eventq_attach(const rte_eth_dev*, int, u16,
>> const rte_event_eth_rx_adapter_queue_conf*)' at dpaa_ethdev.c:1149:1 has
>> some indirect sub-type changes:
>>      parameter 1 of type 'const rte_eth_dev*' has sub-type changes:
>>        in pointed to type 'const rte_eth_dev':
>>          in unqualified underlying type 'struct rte_eth_dev' at
>> ethdev_driver.h:50:1:
>>            type size hasn't changed
>>            1 data member change:
>>              type of 'rte_eth_dev_state state' changed:
>>                type size hasn't changed
>>                1 enumerator insertion:
>>                  'rte_eth_dev_state::RTE_ETH_DEV_ALLOCATED' value '1'
>>                2 enumerator changes:
>>                  'rte_eth_dev_state::RTE_ETH_DEV_ATTACHED' from value '1'
>> to '2' at rte_ethdev.h:2000:1
>>                  'rte_eth_dev_state::RTE_ETH_DEV_REMOVED' from value '2'
>> to '3' at rte_ethdev.h:2000:1
>>
>> 1 function with some indirect sub-type change:
>>
>>    [C] 'function int rte_pmd_i40e_set_switch_dev(uint16_t, rte_eth_dev*)'
>> at rte_pmd_i40e.c:3266:1 has some indirect sub-type changes:
>>      parameter 2 of type 'rte_eth_dev*' has sub-type changes:
>>        in pointed to type 'struct rte_eth_dev' at ethdev_driver.h:50:1:
>>          type size hasn't changed
>>          1 data member change:
>>            type of 'rte_eth_dev_state state' changed:
>>              type size hasn't changed
>>              1 enumerator insertion:
>>                'rte_eth_dev_state::RTE_ETH_DEV_ALLOCATED' value '1'
>>              2 enumerator changes:
>>                'rte_eth_dev_state::RTE_ETH_DEV_ATTACHED' from value '1'
>> to '2' at rte_ethdev.h:2000:1
>>                'rte_eth_dev_state::RTE_ETH_DEV_REMOVED' from value '2' to
>> '3' at rte_ethdev.h:2000:1
>>
>> 1 function with some indirect sub-type change:
>>
>>    [C] 'function rte_eth_dev* rte_eth_dev_allocate(const char*)' at
>> ethdev_driver.c:72:1 has some indirect sub-type changes:
>>      return type changed:
>>        in pointed to type 'struct rte_eth_dev' at ethdev_driver.h:50:1:
>>          type size hasn't changed
>>          1 data member change:
>>            type of 'rte_eth_dev_state state' changed:
>>              type size hasn't changed
>>              1 enumerator insertion:
>>                'rte_eth_dev_state::RTE_ETH_DEV_ALLOCATED' value '1'
>>              2 enumerator changes:
>>                'rte_eth_dev_state::RTE_ETH_DEV_ATTACHED' from value '1'
>> to '2' at rte_ethdev.h:2000:1
>>                'rte_eth_dev_state::RTE_ETH_DEV_REMOVED' from value '2' to
>> '3' at rte_ethdev.h:2000:1
>>
>> ... there are more warnings for same issue ...
>>
>> .
Ferruh Yigit Jan. 11, 2023, 10:46 a.m. UTC | #5 
On 1/11/2023 10:27 AM, Ferruh Yigit wrote:
> On 1/11/2023 12:53 AM, lihuisong (C) wrote:
>>
>> 在 2023/1/11 0:51, Ferruh Yigit 写道:
>>> On 12/6/2022 9:26 AM, Huisong Li wrote:
>>>> This patchset fix some bugs and support attaching and detaching port
>>>> in primary and secondary.
>>>>
>>>> ---
>>>>   -v4: fix a misspelling.
>>>>   -v3:
>>>>     1) merge patch 1/6 and patch 2/6 into patch 1/5, and add
>>>> modification
>>>>        for other bus type.
>>>>     2) add a RTE_ETH_DEV_ALLOCATED state in rte_eth_dev_state to resolve
>>>>        the probelm in patch 2/5.
>>>>   -v2: resend due to CI unexplained failure.
>>>>
>>>> Huisong Li (5):
>>>>    drivers/bus: restore driver assignment at front of probing
>>>>    ethdev: fix skip valid port in probing callback
>>>>    app/testpmd: check the validity of the port
>>>>    app/testpmd: add attach and detach port for multiple process
>>>>    app/testpmd: stop forwarding in new or destroy event
>>>>
>>> Hi Huisong,
>>>
>>> I haven't checked the patch in detail yet, but I can see it gives some
>>> ABI compatibility warnings, is this expected:
>> This is to be expected. Because we insert a device state,
>> RTE_ETH_DEV_ALLOCATED,
>> before RTE_ETH_DEV_ATTACHED for resolving the issue patch 2/5 mentioned.
>> We may have to announce it. What do you think?
> 
> If there is an actual ABI break, it can't go in this release, need to
> wait LTS release and yes needs deprecation notice in advance.
> 
> But not all enum value change warnings are real break, need to
> investigate all warnings one by one.
> Need to investigate if old application & new dpdk library may cause any
> unexpected behavior for application.
> 

OR, appending new enum item, `RTE_ETH_DEV_ALLOCATED`, to the end of the
enum solves the issue, although logically it won't look nice.
Perhaps order can be fixed in next LTS, to have more logical order, but
not quite sure if order worth the disturbance may cause in application.

>>>
>>> 1 function with some indirect sub-type change:
>>>
>>>    [C] 'function int dpaa_eth_eventq_attach(const rte_eth_dev*, int, u16,
>>> const rte_event_eth_rx_adapter_queue_conf*)' at dpaa_ethdev.c:1149:1 has
>>> some indirect sub-type changes:
>>>      parameter 1 of type 'const rte_eth_dev*' has sub-type changes:
>>>        in pointed to type 'const rte_eth_dev':
>>>          in unqualified underlying type 'struct rte_eth_dev' at
>>> ethdev_driver.h:50:1:
>>>            type size hasn't changed
>>>            1 data member change:
>>>              type of 'rte_eth_dev_state state' changed:
>>>                type size hasn't changed
>>>                1 enumerator insertion:
>>>                  'rte_eth_dev_state::RTE_ETH_DEV_ALLOCATED' value '1'
>>>                2 enumerator changes:
>>>                  'rte_eth_dev_state::RTE_ETH_DEV_ATTACHED' from value '1'
>>> to '2' at rte_ethdev.h:2000:1
>>>                  'rte_eth_dev_state::RTE_ETH_DEV_REMOVED' from value '2'
>>> to '3' at rte_ethdev.h:2000:1
>>>
>>> 1 function with some indirect sub-type change:
>>>
>>>    [C] 'function int rte_pmd_i40e_set_switch_dev(uint16_t, rte_eth_dev*)'
>>> at rte_pmd_i40e.c:3266:1 has some indirect sub-type changes:
>>>      parameter 2 of type 'rte_eth_dev*' has sub-type changes:
>>>        in pointed to type 'struct rte_eth_dev' at ethdev_driver.h:50:1:
>>>          type size hasn't changed
>>>          1 data member change:
>>>            type of 'rte_eth_dev_state state' changed:
>>>              type size hasn't changed
>>>              1 enumerator insertion:
>>>                'rte_eth_dev_state::RTE_ETH_DEV_ALLOCATED' value '1'
>>>              2 enumerator changes:
>>>                'rte_eth_dev_state::RTE_ETH_DEV_ATTACHED' from value '1'
>>> to '2' at rte_ethdev.h:2000:1
>>>                'rte_eth_dev_state::RTE_ETH_DEV_REMOVED' from value '2' to
>>> '3' at rte_ethdev.h:2000:1
>>>
>>> 1 function with some indirect sub-type change:
>>>
>>>    [C] 'function rte_eth_dev* rte_eth_dev_allocate(const char*)' at
>>> ethdev_driver.c:72:1 has some indirect sub-type changes:
>>>      return type changed:
>>>        in pointed to type 'struct rte_eth_dev' at ethdev_driver.h:50:1:
>>>          type size hasn't changed
>>>          1 data member change:
>>>            type of 'rte_eth_dev_state state' changed:
>>>              type size hasn't changed
>>>              1 enumerator insertion:
>>>                'rte_eth_dev_state::RTE_ETH_DEV_ALLOCATED' value '1'
>>>              2 enumerator changes:
>>>                'rte_eth_dev_state::RTE_ETH_DEV_ATTACHED' from value '1'
>>> to '2' at rte_ethdev.h:2000:1
>>>                'rte_eth_dev_state::RTE_ETH_DEV_REMOVED' from value '2' to
>>> '3' at rte_ethdev.h:2000:1
>>>
>>> ... there are more warnings for same issue ...
>>>
>>> .
>
lihuisong (C) Jan. 12, 2023, 2:26 a.m. UTC | #6 
在 2023/1/11 18:46, Ferruh Yigit 写道:
> On 1/11/2023 10:27 AM, Ferruh Yigit wrote:
>> On 1/11/2023 12:53 AM, lihuisong (C) wrote:
>>> 在 2023/1/11 0:51, Ferruh Yigit 写道:
>>>> On 12/6/2022 9:26 AM, Huisong Li wrote:
>>>>> This patchset fix some bugs and support attaching and detaching port
>>>>> in primary and secondary.
>>>>>
>>>>> ---
>>>>>    -v4: fix a misspelling.
>>>>>    -v3:
>>>>>      1) merge patch 1/6 and patch 2/6 into patch 1/5, and add
>>>>> modification
>>>>>         for other bus type.
>>>>>      2) add a RTE_ETH_DEV_ALLOCATED state in rte_eth_dev_state to resolve
>>>>>         the probelm in patch 2/5.
>>>>>    -v2: resend due to CI unexplained failure.
>>>>>
>>>>> Huisong Li (5):
>>>>>     drivers/bus: restore driver assignment at front of probing
>>>>>     ethdev: fix skip valid port in probing callback
>>>>>     app/testpmd: check the validity of the port
>>>>>     app/testpmd: add attach and detach port for multiple process
>>>>>     app/testpmd: stop forwarding in new or destroy event
>>>>>
>>>> Hi Huisong,
>>>>
>>>> I haven't checked the patch in detail yet, but I can see it gives some
>>>> ABI compatibility warnings, is this expected:
>>> This is to be expected. Because we insert a device state,
>>> RTE_ETH_DEV_ALLOCATED,
>>> before RTE_ETH_DEV_ATTACHED for resolving the issue patch 2/5 mentioned.
>>> We may have to announce it. What do you think?
>> If there is an actual ABI break, it can't go in this release, need to
>> wait LTS release and yes needs deprecation notice in advance.
>>
>> But not all enum value change warnings are real break, need to
>> investigate all warnings one by one.
>> Need to investigate if old application & new dpdk library may cause any
>> unexpected behavior for application.
>>
> OR, appending new enum item, `RTE_ETH_DEV_ALLOCATED`, to the end of the
> enum solves the issue, although logically it won't look nice.
Thank you for your suggestion, Ferruh. It seems to be ok. I will fix it.
> Perhaps order can be fixed in next LTS, to have more logical order, but
> not quite sure if order worth the disturbance may cause in application.
Probably not worth it.
>>>> 1 function with some indirect sub-type change:
>>>>
>>>>     [C] 'function int dpaa_eth_eventq_attach(const rte_eth_dev*, int, u16,
>>>> const rte_event_eth_rx_adapter_queue_conf*)' at dpaa_ethdev.c:1149:1 has
>>>> some indirect sub-type changes:
>>>>       parameter 1 of type 'const rte_eth_dev*' has sub-type changes:
>>>>         in pointed to type 'const rte_eth_dev':
>>>>           in unqualified underlying type 'struct rte_eth_dev' at
>>>> ethdev_driver.h:50:1:
>>>>             type size hasn't changed
>>>>             1 data member change:
>>>>               type of 'rte_eth_dev_state state' changed:
>>>>                 type size hasn't changed
>>>>                 1 enumerator insertion:
>>>>                   'rte_eth_dev_state::RTE_ETH_DEV_ALLOCATED' value '1'
>>>>                 2 enumerator changes:
>>>>                   'rte_eth_dev_state::RTE_ETH_DEV_ATTACHED' from value '1'
>>>> to '2' at rte_ethdev.h:2000:1
>>>>                   'rte_eth_dev_state::RTE_ETH_DEV_REMOVED' from value '2'
>>>> to '3' at rte_ethdev.h:2000:1
>>>>
>>>> 1 function with some indirect sub-type change:
>>>>
>>>>     [C] 'function int rte_pmd_i40e_set_switch_dev(uint16_t, rte_eth_dev*)'
>>>> at rte_pmd_i40e.c:3266:1 has some indirect sub-type changes:
>>>>       parameter 2 of type 'rte_eth_dev*' has sub-type changes:
>>>>         in pointed to type 'struct rte_eth_dev' at ethdev_driver.h:50:1:
>>>>           type size hasn't changed
>>>>           1 data member change:
>>>>             type of 'rte_eth_dev_state state' changed:
>>>>               type size hasn't changed
>>>>               1 enumerator insertion:
>>>>                 'rte_eth_dev_state::RTE_ETH_DEV_ALLOCATED' value '1'
>>>>               2 enumerator changes:
>>>>                 'rte_eth_dev_state::RTE_ETH_DEV_ATTACHED' from value '1'
>>>> to '2' at rte_ethdev.h:2000:1
>>>>                 'rte_eth_dev_state::RTE_ETH_DEV_REMOVED' from value '2' to
>>>> '3' at rte_ethdev.h:2000:1
>>>>
>>>> 1 function with some indirect sub-type change:
>>>>
>>>>     [C] 'function rte_eth_dev* rte_eth_dev_allocate(const char*)' at
>>>> ethdev_driver.c:72:1 has some indirect sub-type changes:
>>>>       return type changed:
>>>>         in pointed to type 'struct rte_eth_dev' at ethdev_driver.h:50:1:
>>>>           type size hasn't changed
>>>>           1 data member change:
>>>>             type of 'rte_eth_dev_state state' changed:
>>>>               type size hasn't changed
>>>>               1 enumerator insertion:
>>>>                 'rte_eth_dev_state::RTE_ETH_DEV_ALLOCATED' value '1'
>>>>               2 enumerator changes:
>>>>                 'rte_eth_dev_state::RTE_ETH_DEV_ATTACHED' from value '1'
>>>> to '2' at rte_ethdev.h:2000:1
>>>>                 'rte_eth_dev_state::RTE_ETH_DEV_REMOVED' from value '2' to
>>>> '3' at rte_ethdev.h:2000:1
>>>>
>>>> ... there are more warnings for same issue ...
>>>>
>>>> .
> .
Thomas Monjalon Jan. 18, 2023, 2:12 p.m. UTC | #7 
11/01/2023 11:46, Ferruh Yigit:
> On 1/11/2023 10:27 AM, Ferruh Yigit wrote:
> > On 1/11/2023 12:53 AM, lihuisong (C) wrote:
> >> 在 2023/1/11 0:51, Ferruh Yigit 写道:
> >>> Hi Huisong,
> >>>
> >>> I haven't checked the patch in detail yet, but I can see it gives some
> >>> ABI compatibility warnings, is this expected:
> >> This is to be expected. Because we insert a device state,
> >> RTE_ETH_DEV_ALLOCATED,
> >> before RTE_ETH_DEV_ATTACHED for resolving the issue patch 2/5 mentioned.
> >> We may have to announce it. What do you think?
> > 
> > If there is an actual ABI break, it can't go in this release, need to
> > wait LTS release and yes needs deprecation notice in advance.
> > 
> > But not all enum value change warnings are real break, need to
> > investigate all warnings one by one.
> > Need to investigate if old application & new dpdk library may cause any
> > unexpected behavior for application.
> > 
> 
> OR, appending new enum item, `RTE_ETH_DEV_ALLOCATED`, to the end of the
> enum solves the issue, although logically it won't look nice.
> Perhaps order can be fixed in next LTS, to have more logical order, but
> not quite sure if order worth the disturbance may cause in application.

It is a state with a logical order, so it would be nice to be able to do
if (state > RTE_ETH_DEV_ALLOCATED)
but given there is RTE_ETH_DEV_REMOVED later in the enum, not sure it is useful.
lihuisong (C) Jan. 19, 2023, 10:31 a.m. UTC | #8 
在 2023/1/18 22:12, Thomas Monjalon 写道:
> 11/01/2023 11:46, Ferruh Yigit:
>> On 1/11/2023 10:27 AM, Ferruh Yigit wrote:
>>> On 1/11/2023 12:53 AM, lihuisong (C) wrote:
>>>> 在 2023/1/11 0:51, Ferruh Yigit 写道:
>>>>> Hi Huisong,
>>>>>
>>>>> I haven't checked the patch in detail yet, but I can see it gives some
>>>>> ABI compatibility warnings, is this expected:
>>>> This is to be expected. Because we insert a device state,
>>>> RTE_ETH_DEV_ALLOCATED,
>>>> before RTE_ETH_DEV_ATTACHED for resolving the issue patch 2/5 mentioned.
>>>> We may have to announce it. What do you think?
>>> If there is an actual ABI break, it can't go in this release, need to
>>> wait LTS release and yes needs deprecation notice in advance.
>>>
>>> But not all enum value change warnings are real break, need to
>>> investigate all warnings one by one.
>>> Need to investigate if old application & new dpdk library may cause any
>>> unexpected behavior for application.
>>>
>> OR, appending new enum item, `RTE_ETH_DEV_ALLOCATED`, to the end of the
>> enum solves the issue, although logically it won't look nice.
>> Perhaps order can be fixed in next LTS, to have more logical order, but
>> not quite sure if order worth the disturbance may cause in application.
> It is a state with a logical order, so it would be nice to be able to do
> if (state > RTE_ETH_DEV_ALLOCATED)
> but given there is RTE_ETH_DEV_REMOVED later in the enum, not sure it is useful.
The device state is internel. Applications should not access it 
directly, right?
Currently, ethdev layer or PMD use it by enum value instead of the way like
'state > RTE_ETH_DEV_ALLOCATED'.

But, I encapsulated an API, rte_eth_dev_is_used(), for ethdev or PMD to 
call.
I'm not sure if it can help to eliminate our concerns.
>
>
> .
Thomas Monjalon Jan. 19, 2023, 2:35 p.m. UTC | #9 
19/01/2023 11:31, lihuisong (C):
> 在 2023/1/18 22:12, Thomas Monjalon 写道:
> > 11/01/2023 11:46, Ferruh Yigit:
> >> On 1/11/2023 10:27 AM, Ferruh Yigit wrote:
> >>> On 1/11/2023 12:53 AM, lihuisong (C) wrote:
> >>>> 在 2023/1/11 0:51, Ferruh Yigit 写道:
> >>>>> Hi Huisong,
> >>>>>
> >>>>> I haven't checked the patch in detail yet, but I can see it gives some
> >>>>> ABI compatibility warnings, is this expected:
> >>>> This is to be expected. Because we insert a device state,
> >>>> RTE_ETH_DEV_ALLOCATED,
> >>>> before RTE_ETH_DEV_ATTACHED for resolving the issue patch 2/5 mentioned.
> >>>> We may have to announce it. What do you think?
> >>> If there is an actual ABI break, it can't go in this release, need to
> >>> wait LTS release and yes needs deprecation notice in advance.
> >>>
> >>> But not all enum value change warnings are real break, need to
> >>> investigate all warnings one by one.
> >>> Need to investigate if old application & new dpdk library may cause any
> >>> unexpected behavior for application.
> >>>
> >> OR, appending new enum item, `RTE_ETH_DEV_ALLOCATED`, to the end of the
> >> enum solves the issue, although logically it won't look nice.
> >> Perhaps order can be fixed in next LTS, to have more logical order, but
> >> not quite sure if order worth the disturbance may cause in application.
> > It is a state with a logical order, so it would be nice to be able to do
> > if (state > RTE_ETH_DEV_ALLOCATED)
> > but given there is RTE_ETH_DEV_REMOVED later in the enum, not sure it is useful.
> 
> The device state is internel. Applications should not access it 
> directly, right?

Right

> Currently, ethdev layer or PMD use it by enum value instead of the way like
> 'state > RTE_ETH_DEV_ALLOCATED'.

Right

> But, I encapsulated an API, rte_eth_dev_is_used(), for ethdev or PMD to 
> call.
> I'm not sure if it can help to eliminate our concerns.

Yes I think it's OK.
lihuisong (C) Jan. 28, 2023, 1:39 a.m. UTC | #10 
在 2023/1/19 22:35, Thomas Monjalon 写道:
> 19/01/2023 11:31, lihuisong (C):
>> 在 2023/1/18 22:12, Thomas Monjalon 写道:
>>> 11/01/2023 11:46, Ferruh Yigit:
>>>> On 1/11/2023 10:27 AM, Ferruh Yigit wrote:
>>>>> On 1/11/2023 12:53 AM, lihuisong (C) wrote:
>>>>>> 在 2023/1/11 0:51, Ferruh Yigit 写道:
>>>>>>> Hi Huisong,
>>>>>>>
>>>>>>> I haven't checked the patch in detail yet, but I can see it gives some
>>>>>>> ABI compatibility warnings, is this expected:
>>>>>> This is to be expected. Because we insert a device state,
>>>>>> RTE_ETH_DEV_ALLOCATED,
>>>>>> before RTE_ETH_DEV_ATTACHED for resolving the issue patch 2/5 mentioned.
>>>>>> We may have to announce it. What do you think?
>>>>> If there is an actual ABI break, it can't go in this release, need to
>>>>> wait LTS release and yes needs deprecation notice in advance.
>>>>>
>>>>> But not all enum value change warnings are real break, need to
>>>>> investigate all warnings one by one.
>>>>> Need to investigate if old application & new dpdk library may cause any
>>>>> unexpected behavior for application.
>>>>>
>>>> OR, appending new enum item, `RTE_ETH_DEV_ALLOCATED`, to the end of the
>>>> enum solves the issue, although logically it won't look nice.
>>>> Perhaps order can be fixed in next LTS, to have more logical order, but
>>>> not quite sure if order worth the disturbance may cause in application.
>>> It is a state with a logical order, so it would be nice to be able to do
>>> if (state > RTE_ETH_DEV_ALLOCATED)
>>> but given there is RTE_ETH_DEV_REMOVED later in the enum, not sure it is useful.
>> The device state is internel. Applications should not access it
>> directly, right?
> Right
>
>> Currently, ethdev layer or PMD use it by enum value instead of the way like
>> 'state > RTE_ETH_DEV_ALLOCATED'.
> Right
>
>> But, I encapsulated an API, rte_eth_dev_is_used(), for ethdev or PMD to
>> call.
>> I'm not sure if it can help to eliminate our concerns.
> Yes I think it's OK.
ok, I will fix it based on our discussion.
>
>
> .