Re: [dpdk-dev] [PATCH] doc: update failsafe feature list

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 10/25/2017 11:44 AM, Ferruh Yigit wrote:
> On 9/23/2017 10:55 PM, Matan Azrad wrote:
>> Hi Ferruh
>>
>>> -----Original Message-----
>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>>> Sent: Friday, September 22, 2017 1:32 PM
>>> To: Matan Azrad <matan@mellanox.com>; Gaetan Rivet
>>> <gaetan.rivet@6wind.com>
>>> Cc: dev@dpdk.org; john.mcnamara@intel.com
>>> Subject: Re: [dpdk-dev] [PATCH] doc: update failsafe feature list
>>>
>>> On 9/19/2017 12:39 PM, Matan Azrad wrote:
>>>> Hi Ferruh
>>>>
>>>>> -----Original Message-----
>>>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>>>>> Sent: Tuesday, September 19, 2017 2:00 PM
>>>>> To: Matan Azrad <matan@mellanox.com>; Gaetan Rivet
>>>>> <gaetan.rivet@6wind.com>
>>>>> Cc: dev@dpdk.org; john.mcnamara@intel.com
>>>>> Subject: Re: [dpdk-dev] [PATCH] doc: update failsafe feature list
>>>>>
>>>>> On 9/19/2017 11:04 AM, Matan Azrad wrote:
>>>>>>
>>>>>> Hi Ferruh
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>>>>>>> Sent: Tuesday, September 19, 2017 12:27 PM
>>>>>>> To: Matan Azrad <matan@mellanox.com>; Gaetan Rivet
>>>>>>> <gaetan.rivet@6wind.com>
>>>>>>> Cc: dev@dpdk.org
>>>>>>> Subject: Re: [dpdk-dev] [PATCH] doc: update failsafe feature list
>>>>>>>
>>>>>>> On 9/14/2017 4:32 PM, Matan Azrad wrote:
>>>>>>>> Add supported failsafe features to feature list.
>>>>>>>> Remove stats per queue feature from failsafe feature list since
>>>>>>>> queue_stats_mapping_set dev op has not implemented yet.
>>>>>>>>
>>>>>>>> Signed-off-by: Matan Azrad <matan@mellanox.com>
>>>>>>>> ---
>>>>>>>>  doc/guides/nics/features/failsafe.ini | 15 ++++++++++++++-
>>>>>>>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/doc/guides/nics/features/failsafe.ini
>>>>>>>> b/doc/guides/nics/features/failsafe.ini
>>>>>>>> index a42e344..9f48455 100644
>>>>>>>> --- a/doc/guides/nics/features/failsafe.ini
>>>>>>>> +++ b/doc/guides/nics/features/failsafe.ini
>>>>>>>> @@ -4,20 +4,33 @@
>>>>>>>>  ; Refer to default.ini for the full list of available PMD features.
>>>>>>>>  ;
>>>>>>>>  [Features]
>>>>>>>> +Speed capabilities   = Y
>>>>>>>>  Link status          = Y
>>>>>>>>  Link status event    = Y
>>>>>>>>  MTU update           = Y
>>>>>>>>  Jumbo frame          = Y
>>>>>>>> +Scattered Rx         = Y
>>>>>>>> +LRO                  = Y
>>>>>>>> +TSO                  = Y
>>>>>>>>  Promiscuous mode     = Y
>>>>>>>>  Allmulticast mode    = Y
>>>>>>>>  Unicast MAC filter   = Y
>>>>>>>>  Multicast MAC filter = Y
>>>>>>>>  VLAN filter          = Y
>>>>>>>> +Ethertype filter     = Y
>>>>>>>> +N-tuple filter       = Y
>>>>>>>> +SYN filter           = Y
>>>>>>>> +Tunnel filter        = Y
>>>>>>>> +Flexible filter      = Y
>>>>>>>> +Hash filter          = Y
>>>>>>>> +Flow director        = Y
>>>>>>>>  Flow control         = Y
>>>>>>>>  Flow API             = Y
>>>>>>>> +QinQ offload         = Y
>>>>>>>> +L3 checksum offload  = Y
>>>>>>>> +L4 checksum offload  = Y
>>>>>>>>  Packet type parsing  = Y
>>>>>>>>  Basic stats          = Y
>>>>>>>> -Stats per queue      = Y
>>>>>>>>  ARMv7                = Y
>>>>>>>>  ARMv8                = Y
>>>>>>>>  Power8               = Y
>>>>>>>
>>>>>>> I am not sure if claiming support for these features is correct.
>>>>>>> Failsafe itself doesn't provide these features, but relies
>>>>>>> underlying hardware which we don't really know what they supports
>>>>>>> or not in this
>>>>> stage.
>>>>>>>
>>>>>>
>>>>>> Don't you think that almost all failsafe features rely underlying
>>>>>> hardware or
>>>>> sub PMDs?
>>>>>
>>>>> You are right, perhaps we should remove all. This is helpful to show
>>>>> what device features are supported. For failsafe, is this information
>>> useful?
>>>>>
>>>> Since there are features that failsafe cannot support without any sub
>>>> PMD dependences (for example "Stats per queue") it is useful.
>>>
>>> Sorry, I missed your point.
>>>
>>> Device feature list documentation is good for:
>>> - End user can easily see what to expect from a device/driver.
>>> - To trace what features implemented for a device.
>>> - To find out which device has a specific desired feature.
>>>
>>> For failsafe, it is a virtual overlay device on other physical devices.
>>>
>>> The supported architectures and provided documents features can be
>>> useful. But why/how NIC related features can be useful since all they are
>>> coming form underlay devices?
>>>
>> My point is that someone can understand from this list all failsafe PMD features which are not
>> supported even if the failsafe sub devices PMD may support them.
> 
> It may be possible to document them as "Feature = N" if you sure they are not
> supported,
> instead of saying the features that may or may not be supported as supported.

Nak for the patch, its status updated in patchwork.

> 
>> Please read section 31.1 in failsafe documentation: http://dpdk.org/doc/guides/nics/fail_safe.html
>> Actually, the failsafe supported features is the logical AND between all its sub devices supported features and failsafe default features.
>> I think this table should reflect the failsafe default features. 
> 
> So if any PMD doesn't support that feature, it won't be supported, right? If so
> why we are documented it as supported.
> 
> Agree to document default features independent from what PMD supports, as I said
> before "supported architectures and provided documents" etc..
> 
>> It is very useful for failsafe user to compare failsafe features and sub devices features to infer which feature is going to be supported with failsafe combination.
>>
>> In addition,
>> Even if the PMD part is only to set capability bit (NIC related features capability) user must know that failsafe is going to set it.
> 
> Still this depends on if PMD supports it or not.
> 
> Out of curiosity, how failsafe set the underlying PMD features, automatically or
> based on user request? I guess it checks that all PMDs support feature before
> setting it.
> 
>>
>>>>
>>>>>>
>>>>>>> OK for dropping "Stats per queue"
>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>
>