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

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 7/29/21 7:13 AM, Xueming(Steven) Li wrote:
> 
> 
>> -----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.

Got it.

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

Logically even in legacy backward compatibility PF1 VFs representors
have parent port ID - PF0 which is a bond of PF0 & PF1. So,
parent_port_id should be filled in. In this case eth_representor_cmp()
will do rte_eth_representor_id_get(PF0-bond-id, -1, -1, VF, &id) which
will return PF0 VF representor ID. Most likely it will even match and
everything works, but it is still incorrect.

In fact, I have another idea. Try to do:
rte_eth_representor_id_get(representor-port-id, ...) first
for the backward compatibility case and, if not supported, do
it on parent port ID.