Re: [dpdk-dev] [PATCH] ethdev: add namespace

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 6/30/2021 7:29 AM, David Marchand wrote:
> Hello Ferruh,
> 
> On Tue, Jun 29, 2021 at 3:46 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>
>> Add 'RTE_ETH' namespace to all enums & macros in a backward compatible
>> way. The macros for backward compatibility can be removed in next LTS.
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> - I did not do a full check but I noticed that ETH_RSS compat macro at
> least is removed.
> Is this intentional?
> 

Yes, two groups of macros was remaining from 2013 for backward compatibility,
with this patch it would be two layer of redicrection, so I moved to old ones.
The only changes in the examples/app are because of these removed macros, since
rest are all backward compatible.

Removed ones:
/**
 * for rx mq mode backward compatible
 */
#define ETH_RSS                       ETH_MQ_RX_RSS
#define VMDQ_DCB                      ETH_MQ_RX_VMDQ_DCB
#define ETH_DCB_RX                    ETH_MQ_RX_DCB

/**
 * for tx mq mode backward compatible
 */
#define ETH_DCB_NONE                ETH_MQ_TX_NONE
#define ETH_VMDQ_DCB_TX             ETH_MQ_TX_VMDQ_DCB
#define ETH_DCB_TX                  ETH_MQ_TX_DCB



> 
> - libabigail is not happy because of enum names changes.
> Example:
> 
>   [C] 'function int rte_eth_dev_configure(uint16_t, uint16_t,
> uint16_t, const rte_eth_conf*)' at rte_ethdev.c:1326:1 has some
> indirect sub-type changes:
>     parameter 4 of type 'const rte_eth_conf*' has sub-type changes:
>       in pointed to type 'const rte_eth_conf':
>         in unqualified underlying type 'struct rte_eth_conf' at
> rte_ethdev.h:1491:1:
>           type size hasn't changed
>           5 data member changes (1 filtered):
>             type of 'rte_eth_rxmode rxmode' changed:
>               type size hasn't changed
>               1 data member change:
>                 type of 'rte_eth_rx_mq_mode mq_mode' changed:
>                   type size hasn't changed
>                   8 enumerator deletions:
>                     'rte_eth_rx_mq_mode::ETH_MQ_RX_NONE' value '0'
>                     'rte_eth_rx_mq_mode::ETH_MQ_RX_RSS' value '1'
>                     'rte_eth_rx_mq_mode::ETH_MQ_RX_DCB' value '2'
>                     'rte_eth_rx_mq_mode::ETH_MQ_RX_DCB_RSS' value '3'
>                     'rte_eth_rx_mq_mode::ETH_MQ_RX_VMDQ_ONLY' value '4'
>                     'rte_eth_rx_mq_mode::ETH_MQ_RX_VMDQ_RSS' value '5'
>                     'rte_eth_rx_mq_mode::ETH_MQ_RX_VMDQ_DCB' value '6'
>                     'rte_eth_rx_mq_mode::ETH_MQ_RX_VMDQ_DCB_RSS' value '7'
>                   8 enumerator insertions:
>                     'rte_eth_rx_mq_mode::RTE_ETH_MQ_RX_NONE' value '0'
>                     'rte_eth_rx_mq_mode::RTE_ETH_MQ_RX_RSS' value '1'
>                     'rte_eth_rx_mq_mode::RTE_ETH_MQ_RX_DCB' value '2'
>                     'rte_eth_rx_mq_mode::RTE_ETH_MQ_RX_DCB_RSS' value '3'
>                     'rte_eth_rx_mq_mode::RTE_ETH_MQ_RX_VMDQ_ONLY' value '4'
>                     'rte_eth_rx_mq_mode::RTE_ETH_MQ_RX_VMDQ_RSS' value '5'
>                     'rte_eth_rx_mq_mode::RTE_ETH_MQ_RX_VMDQ_DCB' value '6'
>                     'rte_eth_rx_mq_mode::RTE_ETH_MQ_RX_VMDQ_DCB_RSS' value '7'
> [snip]
> 
> 
> I guess libabigail is lost because the symbol
> rte_eth_rx_mq_mode::ETH_MQ_RX_NONE simply disappeared (because we used
> a macro to wrap to the new name).

Yes.

> Maybe we could go the other way: leave the current enums defined as is
> and put in place wrappers for new names pointing as old names. 
> The rest of the code in DPDK would use the new names only.

It works to prevent libabigail warnings but I think it can be confusing for
users on figuring out which ones are the correct ones to use.

> This comment applies if we want to merge this change in 21.08 and/or
> we want to backport this change.
> 
> This won't be a problem if we merge this patch in 21.11.
> 

OK to have it on v21.11. In that case I assume all internal components also
needs to be updated to use new macros/enums during v21.11 release.

Let me send a deprecation notice for it.

Meanwhile, we can use this patch to discuss the prefix/namespace, if it should
be 'RTE_ETH_' or 'RTE_ETH_DEV_', or mix of both (as done now).

And if mixed prefix used, we can try to define when to have DEV_ part and when
not to have it.

> 
>> ---
>> We can get the update on v21.11 and remove backward compatibility macros
>> on v22.11.
> 
> 
>