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

["Xueming(Steven) Li" <xuemingl@nvidia.com>]

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Tuesday, July 20, 2021 5:00 PM
> To: Xueming(Steven) Li <xuemingl@nvidia.com>; Ajit Khaparde <ajit.khaparde@broadcom.com>; Somnath Kotur
> <somnath.kotur@broadcom.com>; John Daley <johndale@cisco.com>; Hyong Youb Kim <hyonkim@cisco.com>; Beilei Xing
> <beilei.xing@intel.com>; Qiming Yang <qiming.yang@intel.com>; Qi Zhang <qi.z.zhang@intel.com>; Haiyue Wang
> <haiyue.wang@intel.com>; Matan Azrad <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>; Slava Ovsiienko
> <viacheslavo@nvidia.com>; NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>; stable@dpdk.org
> Subject: Re: [PATCH] ethdev: fix representor port ID search by name
> 
> On 7/19/21 3:50 PM, Xueming(Steven) Li wrote:
> >
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >> Sent: Monday, July 19, 2021 8:36 PM
> >> To: Xueming(Steven) Li <xuemingl@nvidia.com>; Ajit Khaparde
> >> <ajit.khaparde@broadcom.com>; Somnath Kotur
> >> <somnath.kotur@broadcom.com>; John Daley <johndale@cisco.com>; Hyong
> >> Youb Kim <hyonkim@cisco.com>; Beilei Xing <beilei.xing@intel.com>;
> >> Qiming Yang <qiming.yang@intel.com>; Qi Zhang <qi.z.zhang@intel.com>;
> >> Haiyue Wang <haiyue.wang@intel.com>; Matan Azrad <matan@nvidia.com>;
> >> Shahaf Shuler <shahafs@nvidia.com>; Slava Ovsiienko
> >> <viacheslavo@nvidia.com>; NBU-Contact-Thomas Monjalon
> >> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>
> >> Cc: dev@dpdk.org; Viacheslav Galaktionov
> >> <viacheslav.galaktionov@oktetlabs.ru>; stable@dpdk.org
> >> Subject: Re: [PATCH] ethdev: fix representor port ID search by name
> >>
> >> On 7/19/21 2:54 PM, Xueming(Steven) Li wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >>>> Sent: Monday, July 19, 2021 4:46 PM
> >>>> To: Xueming(Steven) Li <xuemingl@nvidia.com>; Ajit Khaparde
> >>>> <ajit.khaparde@broadcom.com>; Somnath Kotur
> >>>> <somnath.kotur@broadcom.com>; John Daley <johndale@cisco.com>;
> >>>> Hyong Youb Kim <hyonkim@cisco.com>; Beilei Xing
> >>>> <beilei.xing@intel.com>; Qiming Yang <qiming.yang@intel.com>; Qi
> >>>> Zhang <qi.z.zhang@intel.com>; Haiyue Wang <haiyue.wang@intel.com>;
> >>>> Matan Azrad <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>;
> >>>> Slava Ovsiienko <viacheslavo@nvidia.com>; NBU-Contact-Thomas
> >>>> Monjalon <thomas@monjalon.net>; Ferruh Yigit
> >>>> <ferruh.yigit@intel.com>
> >>>> Cc: dev@dpdk.org; Viacheslav Galaktionov
> >>>> <viacheslav.galaktionov@oktetlabs.ru>; stable@dpdk.org
> >>>> Subject: Re: [PATCH] ethdev: fix representor port ID search by name
> >>>>
> >>>> On 7/19/21 9:58 AM, Xueming(Steven) Li wrote:
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >>>>>> Sent: Tuesday, July 13, 2021 12:18 AM
> >>>>>> To: Ajit Khaparde <ajit.khaparde@broadcom.com>; Somnath Kotur
> >>>>>> <somnath.kotur@broadcom.com>; John Daley <johndale@cisco.com>;
> >>>>>> Hyong Youb Kim <hyonkim@cisco.com>; Beilei Xing
> >>>>>> <beilei.xing@intel.com>; Qiming Yang <qiming.yang@intel.com>; Qi
> >>>>>> Zhang <qi.z.zhang@intel.com>; Haiyue Wang
> >>>>>> <haiyue.wang@intel.com>; Matan Azrad <matan@nvidia.com>; Shahaf
> >>>>>> Shuler <shahafs@nvidia.com>; Slava Ovsiienko
> >>>>>> <viacheslavo@nvidia.com>; NBU-Contact-Thomas Monjalon
> >>>>>> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
> >>>>>> Xueming(Steven) Li <xuemingl@nvidia.com>
> >>>>>> Cc: dev@dpdk.org; Viacheslav Galaktionov
> >>>>>> <viacheslav.galaktionov@oktetlabs.ru>; stable@dpdk.org
> >>>>>> Subject: [PATCH] ethdev: fix representor port ID search by name
> >>>>>>
> >>>>>> From: Viacheslav Galaktionov
> >>>>>> <viacheslav.galaktionov@oktetlabs.ru>
> >>>>>>
> >>>>>> Fix representor port ID search by name if the representor itself
> >>>>>> does not provide representors info. Getting a list of
> >>>>>> representors from a representor does not make sense. Instead, a
> >>>>>> parent device
> >>>> should be used.
> >>>>>>
> >>>>>> To this end, extend the rte_eth_dev_data structure to include the port ID of the parent device for representors.
> >>>>>>
> >>>>>> Fixes: df7547a6a2cc ("ethdev: add helper function to get
> >>>>>> representor
> >>>>>> ID")
> >>>>>> Cc: stable@dpdk.org
> >>>>>>
> >>>>>> 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?
> >>>>>>
> >>>>>> May be the patch should add lines to release notes, but I'd like to get initial feedback first.
> >>>>>>
> >>>>>> 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
> >>>>
> >>>> [snip]
> >>>>
> >>>>>> --- a/lib/ethdev/ethdev_driver.h
> >>>>>> +++ b/lib/ethdev/ethdev_driver.h
> >>>>>> @@ -1248,8 +1248,8 @@ struct rte_eth_devargs {
> >>>>>>      * For backward compatibility, if no representor info, direct
> >>>>>>      * map legacy VF (no controller and pf).
> >>>>>>      *
> >>>>>> - * @param ethdev
> >>>>>> - *  Handle of ethdev port.
> >>>>>> + * @param parent_port_id
> >>>>>> + *  Port ID of the backing device.
> >>>>>>      * @param type
> >>>>>>      *  Representor type.
> >>>>>>      * @param controller
> >>>>>> @@ -1266,7 +1266,7 @@ struct rte_eth_devargs {
> >>>>>>      */
> >>>>>>     __rte_internal
> >>>>>>     int
> >>>>>> -rte_eth_representor_id_get(const struct rte_eth_dev *ethdev,
> >>>>>> +rte_eth_representor_id_get(uint16_t parent_port_id,
> >>>>>
> >>>>> It make more sense to get representor info from parent port.
> >>>>> Representor is a member of switch domain, PMD owns the information
> >>>>> of the representor owner port and info of representors. This
> >>>>> change looks better, but not sure whether it valuable to introduce
> >>>>> a new
> >>>> member to the EAL data structure.
> >>>>
> >>>> IMHO, it is simply incorrect to return representors info on a
> >>>> representor itself. Representor info is an information which representors may be populated using the device.
> >>>>
> >>>> If above statement is correct, we need a way to get parent device
> >>>> by representor to do name to representor ID mapping. I see two options to do it:
> >>>>     A. Dedicated field in rte_eth_dev_data as the patch does.
> >>>>     B. Dedicated ethdev op (since representor knows parent port ID anyway).
> >>>> We have chosen (A) because of simplicity.
> >>>
> >>> Just recalled that representor port could be probed w/o owner PF, is a force for parent port?
> >>
> >> I thought that it is impossible and parent port is absolutely
> >> required for a representor. Could you provide an example and explain how will it work?
> >
> > In case of bonding, PF0 and PF1 become one PF port `bond0`, PCI address is PF0.
> > 	-a <PF0>,representor=pf[0-1]vf[0-99] // this is the syntax we proposed.
> 
> Is it net/bonding or vendor-specific bonding in HW?
> If I remember correctly in the case of net/bonding we have ethdev ports for bonded devices.

Not net/bonding pmd, it's Linux bonding, supported by hw driver.

> 
> >
> > To be backward compatible, also support the following 2 devargs:
> > 	-a <pf0>,representor=[0-99] // probe bond0 and representor on pf0
> > 	-a <pf1>,representor=[0-99] // probe representors on pf1.
> > If devargs start with PF1 devargs, no owner PF1 created as it disabled
> > in bonding. Can't create bond0(PF0) automatically here as device is located by PCI address(PF1) from devargs.
> 
> So, I guess the problem is vendor-specific bonding in HW. Anyway legacy backward compatible representor spec should not require
> representors info since it worked before without it. So, it does not sound like a reason to have representors info on a representor
> itself.

Legacy backward logic could be something like this: if PF owner port found, use it, fallback to current representor.
This won't break anything I guess, how do you think?