On 2021-09-01 17:55, Ferruh Yigit wrote:
On 8/31/2021 5:06 PM, Andrew Rybchenko wrote:
From: Viacheslav Galaktionov <viacheslav.galaktio...@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.
