Re: [dpdk-dev] [PATCH v5 0/9] ethdev: support SubFunction representor

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

On 1/27/21 6:04 AM, Xueming(Steven) Li wrote:
> Hi Andrew,
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Friday, January 22, 2021 4:21 PM
>> To: Xueming(Steven) Li <xuemingl@nvidia.com>
>> Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@nvidia.com>; Asaf Penso
>> <asafp@nvidia.com>
>> Subject: Re: [dpdk-dev] [PATCH v5 0/9] ethdev: support SubFunction
>> representor
>>
>> On 1/19/21 5:24 PM, Xueming(Steven) Li wrote:
>>>
>>>> -----Original Message-----
>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> Sent: Tuesday, January 19, 2021 4:41 PM
>>>> To: Xueming(Steven) Li <xuemingl@nvidia.com>
>>>> Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@nvidia.com>; Asaf
>>>> Penso <asafp@nvidia.com>
>>>> Subject: Re: [dpdk-dev] [PATCH v5 0/9] ethdev: support SubFunction
>>>> representor
>>>>
>>>> On 1/19/21 10:13 AM, Xueming Li wrote:
>>>>> dedicated queues(txq, rxq). A SF netdev supports E-Switch
>>>>> representation offload similar to existing PF and VF representors. A
>>>>> SF shares PCI level resources with other SFs and/or with its parent
>>>>> PCI
>>>> function.
>>>>>
>>>>> >From SmartNIC perspective, when PCI device is shared for
>>>>>> multi-host,
>>>>> representors for host controller and host PF is required.
>>>>>
>>>>> This patch set introduces new representor types in addtion to
>>>>> existing VF representor. Syntax:
>>>>>
>>>>> [[c#]pf#]vf#: VF port representor/s from controller/pf
>>>>> [[c#]pf#]sf#: SF port representor/s from controller/pf
>>>>> #: VF representor - for backwards compatibility
>>>>>
>>>>> "#" is number instance, list or range, valid examples:
>>>>>   1, [1,3,5], [0-3], [0,2-4,6]
>>>>>
>>>>> For backward compatibility, this patch also introduces new netdev
>>>>> capability to indicate the capability of supportting SF representor.
>>>>
>>>> The patch series looks really nice. Thanks.
>>>>
>>>> As before, my biggest concern is making representor ID a bitmap. See
>>>> my comments to a specific patch.
>>>> Plus absence of defined semantics of caller function.
>>>> Basically it is looks like it is assumed that controller #0 and PF #0 is the
>> caller.
>>>> However, it could be wrong.
>>>
>>> From devargs syntax perspective, a VF representor on PF1 could be
>> referenced either way:
>>>   1:  <PF0_BDF>,representor=pf1vfX
>>>   2:  <PF1_BDF>,representor=vfX
>>>   3:  <PF1_BDF>,representor=pf0vfX // works but not suggested, use PF0
>>> BDF as caller If probe a device with PF0 BDF and locate it with PF1
>>> BDF, the device iterator will fail, devargs is parsed by api, EAL
>>> layer simply compare PCI BDF and then representor ID, i.e. "c#pf#vf#". The
>> caller device should be consistent, better to use the first device.
>>>
>>> Devrg "<PF1_BDF>,representor=vfX" will work, representor controller ID
>>> and pf ID default to #0, relative to caller context PF1 BDF.
>>>
>>> Is it good to add such suggestion/behavior on rte_eth_devargs comments?
>>
>> Sorry, but I disagree. Solution with vf1 and pf0vf1 making the same
>> representor ID sounds not acceptable to me. It is definitely different things if I
>> pass it to PF1.
> 
> My understanding is that representor is an "offset" to caller(context) PF, so the
> caller PF has to be the lowest, i.e. PF0.

Sorry, how to address PF0 from PF1?

> The usage for mlx5 PMD is to probe VF 
> representor on a kernel bonding:
> 1. PF0 is the only device for PMD to probe the bonding device, PMD detects underlay PFs.
> 2. PF0_BDF,reprensetor=pf1vf0, to probe the representor for first VF on PF1.
> PF1_BDF,reprenstor=pf0vf0 works for the same representor, but not encouraged because 
> it confuses EAL device iterator which can't tell the difference, as you said: "mainly for human"
> 
> Controller ID is not used by mlx5 PMD currently, just a place holder for now, but I think 
> same policy applies: lowest as caller(context).
> 
> Your suggestion of reserving an c# and pf# ID for caller device which c# or pf# not specified looks
> good, it makes the usage of the representor ID bitmap flexible, but considering some NIC with 4 PFs,
> it's hard to choose, as you know, only 3 controller and 3 PFs left.

I'd prefer the option if we make a choice between these two.
At least it a bit more consistent.
But IMHO anyway bitmap is a road to wrong direction.

> BTW, I guess your assumption is that  representor as "absolute" to caller PF, can't default to 0.
> Is there a scenario for PF1 as caller?

Yes.

>>>>
>>>> The next biggest concert is the absence of capability reporting API.
>>>> How many controller are available?
>>>> How many PFs on each controller are available?
>>>> How many VFs on each controller/PF are available?
>>>> How many SFs?
>>>>
>>> >From the first sight it sounds not that important right now and an
>>>> extra  feature which could be added in the follow up patches, but IMHO
>>>> addition of  the API would allow to avoid making representor ID a bitmap.
>>>> Basically capabilities API can provide an array of available
>>>> functions with  representor ID assigned to each entry. Also it could
>>>> make the entire patch  series optional since it would allow to
>>>> interpret numbers in representor=[....]  as representor IDs which are
>>>> mapped to controller/PF/VF/SF by the  capabilities reporting API.
>>>>
>>>> I realize that sometimes it could be more convenient to use syntax
>>>> suggested here. Mainly for human.
>>>
>>> Agree, mostly for human. Regarding to capability reporting API, how
>>> about remove current one and enhance it later with a complete patch set?
>>>
>>
>> Current one? Sorry I don't understand.
> 
> The patch of device SF capability, but seems I misunderstood your suggestion.
> Let me explain process to create a SF:
> 1. SF can be created on the fly with scripts, unlike VF which is statically pre-created.

Is there a maximum index and maximum total number of SF's
created? How to find it?

> 2. SF is created on a PF with a SF number. SF number is named per PF, different PF may have same SF number.
> 3. For standalone PF, hot plug to DPDK using "PF#_BDF,representor=sf#", no need to use pf#sf# here.
> 4. For bonding netdev, hot plug to DPDK using "PF0_BDF,representor=pf#sf#"
> If using new api to return all representor IDs, need some way locate the new created SF by PF and SF number,
> that's why "pf#sf#" is used in this patch set.

I think the API should simply reserve/report space for maximum
number of SFs. So, IDs are stable across restart/reboot in
assumption that NIC is not reconfigured (changed maximum number
of VF or maximum number of SFs of any PF).

> 
> In the future, I think representor could be processed by PMD, so PMD could have enough flexibility
> to support more device expressions and types. But that will introduce a fundamental change of devargs and 
> device management, need a full plan.
> 
>>
>> Andrew.
>