Re: [dpdk-dev] [PATCH v4] ethdev: fix representor port ID search by name

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

On 8/31/2021 5:06 PM, Andrew Rybchenko wrote:
> From: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
> 
> Getting a list of representors from a representor does not make sense.
> Instead, a parent device should be used.
> 

Which code is getting list of the representors?

As far as I can see impacted APIs are:
'rte_eth_representor_id_get()'
'rte_eth_representor_info_get()'

Which are now getting 'parent_port_id' as argument, instead of representro port id.

'rte_eth_representor_info_get()' is using 'representor_info_get()' dev_ops,
which is only implemented by 'mlx5', so is this problem only valid for 'mlx5'
and can it be solved within PMD implementation?


> To this end, extend the rte_eth_dev_data structure to include the port ID
> of the backing device for representors.
> 
> Signed-off-by: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> ---
> The new field is added into the hole in rte_eth_dev_data structure.
> The patch does not change ABI, but extra care is required since ABI
> check is disabled for the structure because of the libabigail bug [1].
> 
> Potentially it is bad for out-of-tree drivers which implement
> representors but do not fill in a new parert_port_id field in
> rte_eth_dev_data structure. Do we care?
> 
> mlx5 changes should be reviwed by maintainers very carefully, since
> we are not sure if we patch it correctly.
> 
> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=28060
> 
> v4:
>     - apply mlx5 review notes: remove fallback from generic ethdev
>       code and add fallback to mlx5 code to handle legacy usecase
> 
> v3:
>     - fix mlx5 build breakage
> 
> v2:
>     - fix mlx5 review notes
>     - try device port ID first before parent in order to address
>       backward compatibility issue
> 

<...>

> index edf96de2dc..72fefa59c2 100644
> --- a/lib/ethdev/rte_ethdev_core.h
> +++ b/lib/ethdev/rte_ethdev_core.h
> @@ -185,6 +185,12 @@ struct rte_eth_dev_data {
>  			/**< Switch-specific identifier.
>  			 *   Valid if RTE_ETH_DEV_REPRESENTOR in dev_flags.
>  			 */
> +	uint16_t parent_port_id;

Why 'parent'? Isn't this for the port that port representator represents, does
it called 'parent'? Above that device mentioned as 'backing device' a few times,
so would something like 'peer_port_id' better?




> +			/**< Port ID of the backing device.
> +			 *   This device will be used to query representor
> +			 *   info and calculate representor IDs.
> +			 *   Valid if RTE_ETH_DEV_REPRESENTOR in dev_flags.
> +			 */
>  


Overall I am not feeling good about adding representor port related information
withing the device data struct.
I wonder if this information can be kept in the device private data.

Or, it is hard to explain but can we use something like inheritance, a
representor specific dev_data derived from original dev_data. We can store
dev_data pointers in 'struct rte_eth_dev' but can cast it to representor
specific dev_data when type is representor.

struct rte_eth_dev_data_rep
	struct rte_eth_dev_data
	<representor specific fields>

This brings lots of complexity though, specially in allocating/freeing this
struct, not sure if it worth to the effort.