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

[Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>]

On 2021-09-01 17:55, Ferruh Yigit wrote:
> 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?

It's not an mlx5-specific problem, it's going to affect sfc as well once 
it
starts supporting representors. But that doesn't really matter as it's 
more
about the usage of representors in general, not specific to any PMD's 
internals.

Since representors are created through some device (which is probed and 
assigned
a port ID), it makes sense to query the list of representors from the 
same
device.

[snip]

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

This is true, the naming here is confusing and should be changed.
"parent_port_id" doesn't point at the represented entity, but at the 
device
that was used to create this representor. We call it the backing device, 
so
using "backer_port_id" sounds appropriate, what do you think?

>> +			/**< 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.

This information is usually kept in the device private data as well, but 
we
need to use it from the generic code to redirect the representor info 
requests
to the appropriate ports.

Using "inheritance" is a good suggestion, but it does bring a lot of 
complexity,
as you've said, and we're not sure if the result is worth the effort.

We can also avoid storing this port ID in the device data by creating a 
special
callback that PMDs would use to return it. However, this also brings 
complexity
and this information is static anyway, so having a separate callback 
might be
a little too much.

What we're doing here just seems to be the simplest option.