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

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

On Wed, Oct 25, 2017 at 11:44:39AM -0700, 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.
> 
> > 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.
> 

Hi Ferruh,

I won't comment on the rest of the discussion, as I see merit to both
positions and I am still undecided about what is most interesting to the
user.

On your last question:

For ether ops, the fail-safe relies on the public ether API of the
sub-device eth_dev.

For interrupts, the fail-safe relies on user request or on specific PMD
support for RMV_EVENT: it doesn't matter if the other sub-device does
not support the RMV event, if one PMD supports it it will be enabled.

For offloads, the fail-safe will do a logical AND of offload amongst the
sub-device PMDs and restrict those to this subset.
If a plugged-in PMD has a different set of supported offloads, the
fail-safe throws an error.

Generally, sub-device PMDs are expected to closely match feature-wise.

> > 
> >>>
> >>>>>
> >>>>>> OK for dropping "Stats per queue"
> >>>>>>
> >>>>>>>
> >>>>>
> >>>
> > 
> 

-- 
Gaëtan Rivet
6WIND