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? > 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.galaktio...@oktetlabs.ru> > Signed-off-by: Andrew Rybchenko <andrew.rybche...@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.
