[dpdk-dev,v1] doc: update mlx4 flow limitations

  From: Moti Haimovsky <motih@mellanox.com>

This patch updates mlx4 documentation with flow
configuration limitations imposed by NIC hardware and
PMD implementation

Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
 doc/guides/nics/mlx4.rst | 77 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)
  

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ophir Munk
> Sent: Thursday, February 8, 2018 6:56 AM
> To: dev@dpdk.org; Adrien Mazarguil <adrien.mazarguil@6wind.com>; Moti
> Haimovsky <motih@mellanox.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; Olga Shern
> <olgas@mellanox.com>; Ophir Munk <ophirmu@mellanox.com>; Matan Azrad
> <matan@mellanox.com>
> Subject: [dpdk-dev] [PATCH v1] doc: update mlx4 flow limitations
> 
> From: Moti Haimovsky <motih@mellanox.com>
> 
> This patch updates mlx4 documentation with flow configuration limitations
> imposed by NIC hardware and PMD implementation
> 


> +  For example using testpmd command - src mask must be
> + 00:00:00:00:00:00  otherwise the following command will fail
> +
> +.. code-block:: console
> +
> +     testpmd> flow create 1 ingress pattern eth
> +              src spec 00:16:3e:2b:e6:47 src mask FF:FF:FF:FF:FF:FF
> +              / end actions drop / end

Is there a condadiction between "src mask must be 00:00:00:00:00:00"
and the mask used in the example.

Apart from that the doc looks okay.

Acked-by: John McNamara <john.mcnamara@intel.com>
  

Hi Ophir,

On Thu, Feb 08, 2018 at 06:55:54AM +0000, Ophir Munk wrote:
> From: Moti Haimovsky <motih@mellanox.com>

Relatively minor, patch author differs from the only sign off below, I don't
think it's on purpose.

> This patch updates mlx4 documentation with flow
> configuration limitations imposed by NIC hardware and
> PMD implementation
> 
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>

Another nit, don't hesitate to spread commit logs to their maximum width
of 75 chars. We're not writing poetry :)

More comments below.

> ---
>  doc/guides/nics/mlx4.rst | 77 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
> 
> diff --git a/doc/guides/nics/mlx4.rst b/doc/guides/nics/mlx4.rst
> index 98b9716..b81a875 100644
> --- a/doc/guides/nics/mlx4.rst
> +++ b/doc/guides/nics/mlx4.rst
> @@ -515,3 +515,80 @@ devices managed by librte_pmd_mlx4.
>        Port 3 Link Up - speed 40000 Mbps - full-duplex
>        Done
>        testpmd>
> +
> +Limitations
> +-----------
> +
> +Flow rules
> +~~~~~~~~~~

While documenting flow rule limitations is a good idea, I think this
approach is not ideal. rte_flow being unbounded, no PMD can support all
possible combinations. It's much easier and useful to list what is
implemented or more importantly, *tested* and known to work.

Ideally it should be written in a format that can be reused by other PMDs
for consistency and divided in sections sorted by order of usefulness,
something like:

1. Overview of supported combinations of attributes, patterns and actions.
2. Detailed description of each supported pattern (item combinations).
3. Detailed description of all supported action combinations.
4. Description of each supported pattern item and its quirks.
5. Description of each supported action and its quirks.

> +
> +L2 (eth)
> +^^^^^^^^

You need to make clear you're documenting RTE_FLOW_ITEM_TYPE_ETH when used
at the beginning of a flow rule, for instance it doesn't apply to an inner
ETH used after VXLAN since mlx4 can't match those yet.

That's why it's important to also describe the supported combinations.

> +
> +- Can only use real destination MAC
> +- Source MAC is not taken into consideration

Neither EtherType (please end all sentences with periods).

> +
> +  For example using testpmd command - src mask must be 00:00:00:00:00:00
> +  otherwise the following command will fail
> +
> +.. code-block:: console
> +
> +     testpmd> flow create 1 ingress pattern eth
> +              src spec 00:16:3e:2b:e6:47 src mask FF:FF:FF:FF:FF:FF
> +              / end actions drop / end

Remember you're documenting API support limitations primarily for
application writers who are not necessarily familiar with testpmd.

The problem here is also that "src" is in fact an attribute of either
"spec", "last" or "mask", not the other way around, hence you should refer
to struct rte_flow_item / rte_flow_item_eth and its fields instead of using
a testpmd example.

> +
> +- Supports only full MASK
> +
> +  For example the following testpmd command will fail
> +
> +.. code-block:: console
> +
> +     testpmd> flow create 1 ingress pattern eth
> +              src spec 00:16:3e:2b:e6:47
> +              dst spec 4A:11:6C:FA:60:D0 dst mask FF:00:FF:FF:FF:00
> +              / end actions drop / end
> +

Providing spec but no mask for src is valid (mask remains 0), however it's
certainly a trap for unsuspecting readers unfamiliar with the flow command.

Also providing examples is not bad in itself but they should not appear in
the middle of an enumeration list as it makes them unclear.

> +
> +- When configured to run in promiscuous or all-multicast modes does
> +  not support additional rules

This wording is misleading, the actual limitation is you can't provide
additional items in a pattern if you want to match any destination MAC
(mask.dst == 0) or only multicast traffic (spec.dst & mask.dst ==
01:00:00:00:00:00).

> +- Does not support the explicit exclusion of all multicast traffic
> +- Does not support partial VLAN TCI VID matching

This last item actually documents RTE_FLOW_ITEM_TYPE_VLAN.

> +
> +L3 (ipv4)
> +^^^^^^^^^
> +
> +- Supports only 0 or full mask. Prerequisites: Need to have eth dst spec

Matching all fields of IPv4 headers is not supported, only source and
destination.

Not a single word about the lack of IPv6 support?

> +
> +L4 (tcp/udp)
> +^^^^^^^^^^^^
> +
> +- Supports only full mask

Only on source and destination ports. Empty masks are also supported.

> +  For example the following testpmd command will fail
> +
> +.. code-block:: console
> +
> +     testpmd> flow create 0 ingress pattern eth
> +              src spec e4:1d:2d:2d:8d:22
> +              dst spec 00:15:5D:10:8D:00 dst mask FF:FF:FF:FF:FF:FF
> +              / ipv4 src spec 144.144.92.0 src prefix 16
> +              / end actions drop  / end

Neither TCP nor UDP are part of this example.

> +  Prerequisites: Need to have eth dst spec and IPv4 before it with all
> +  its limitations
> +
> +Flow actions
> +~~~~~~~~~~~~
> +
> +RSS
> +^^^
> +
> +RSS is performed on packets to spread them among several queues based on hash
> +function calculation and according to provided parameters.

You should assume readers are familiar with what RSS does at this point. You
only have to provide device-specific information on top of what is already
documented by rte_flow.rst, you can link to that documentation if deemed
necessary.

> +
> +- RSS hash is calculated on fixed packet fields including: L3 source and
> +  destination addresses (ipv4 or ipv6) and L4 source and destination addresses
> +  (upd or tcp ports)

This reads like there's no choice but to have them all in a fixed fashion.
Users can actually pick their own hash fields combinations from the
supported set (IPv4/IPv6/TCP/UDP).

> +- Uses default constant RSS key

Wrong, users can provide their own key as well. The fixed key is only used
for the default (non-rte_flow) RSS, or when one is not provided.

> +- Only power of two number of queues is supported
> +- Every Rx queue can be specified only once in RSS action

Right, should probably be described a bit more extensively though.

This section is titled "Limitations" but contains a mix of features,
limitations and quirks, more like "Random thoughts regarding rte_flow
support". I think this is not what users might expect from such a
documentation which must be exhaustive and consistent. Getting there may
involve tables.

My suggestion is to first get everyone agree on a common rte_flow
capabilities documentation format all PMDs could reuse and then fill in the
blanks. What's your opinion?
  

09/02/2018 17:21, Adrien Mazarguil:
> This section is titled "Limitations" but contains a mix of features,
> limitations and quirks, more like "Random thoughts regarding rte_flow
> support". I think this is not what users might expect from such a
> documentation which must be exhaustive and consistent. Getting there may
> involve tables.

+Cc Ferruh

> My suggestion is to first get everyone agree on a common rte_flow
> capabilities documentation format all PMDs could reuse and then fill in the
> blanks. What's your opinion?

I think it's better to have some random thoughts than nothing.
All the comments you gave in this thread deserve to be written in
the documentation as soon as possible.
Working on a better standardized documentation (longer term) should
not prevent us to write some messy notes in the meantime.

Is there already some similar rte_flow notes in other PMD docs?

About the common documentation, do you think about a generated table
like it is done for other features?
Do you plan to provide a template or an example?
  

On Thu, Feb 08, 2018 at 06:55:54AM +0000, Ophir Munk wrote:
> From: Moti Haimovsky <motih@mellanox.com>
> 
> This patch updates mlx4 documentation with flow
> configuration limitations imposed by NIC hardware and
> PMD implementation
> 
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> ---
>  doc/guides/nics/mlx4.rst | 77 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
> 
> diff --git a/doc/guides/nics/mlx4.rst b/doc/guides/nics/mlx4.rst
> index 98b9716..b81a875 100644
> --- a/doc/guides/nics/mlx4.rst
> +++ b/doc/guides/nics/mlx4.rst
> @@ -515,3 +515,80 @@ devices managed by librte_pmd_mlx4.
>        Port 3 Link Up - speed 40000 Mbps - full-duplex
>        Done
>        testpmd>
> +
> +Limitations
> +-----------
> +
> +Flow rules
> +~~~~~~~~~~
> +
> +L2 (eth)
> +^^^^^^^^
> +
> +- Can only use real destination MAC
> +- Source MAC is not taken into consideration
> +
> +  For example using testpmd command - src mask must be 00:00:00:00:00:00
> +  otherwise the following command will fail
> +
> +.. code-block:: console
> +
> +     testpmd> flow create 1 ingress pattern eth
> +              src spec 00:16:3e:2b:e6:47 src mask FF:FF:FF:FF:FF:FF
> +              / end actions drop / end
> +
> +- Supports only full MASK
> +
> +  For example the following testpmd command will fail
> +
> +.. code-block:: console
> +
> +     testpmd> flow create 1 ingress pattern eth
> +              src spec 00:16:3e:2b:e6:47
> +              dst spec 4A:11:6C:FA:60:D0 dst mask FF:00:FF:FF:FF:00
> +              / end actions drop / end
> +
> +
> +- When configured to run in promiscuous or all-multicast modes does
> +  not support additional rules
> +- Does not support the explicit exclusion of all multicast traffic
> +- Does not support partial VLAN TCI VID matching
> +
> +L3 (ipv4)
> +^^^^^^^^^
> +
> +- Supports only 0 or full mask. Prerequisites: Need to have eth dst spec

Plans on updating mlx5 guide with this info too?
AFAIK ConnectX-4 and 5 also have this limitation and it can save some
hours of debugging.

  Marcelo
  

Hi Marcelo,

Sunday, February 11, 2018 9:31 PM, Marcelo Ricardo:
> On Thu, Feb 08, 2018 at 06:55:54AM +0000, Ophir Munk wrote:
> > From: Moti Haimovsky <motih@mellanox.com>
> >
> > This patch updates mlx4 documentation with flow configuration
> > limitations imposed by NIC hardware and PMD implementation
> >
> > Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> > ---
> >  doc/guides/nics/mlx4.rst | 77
> > ++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 77 insertions(+)
> >
> > diff --git a/doc/guides/nics/mlx4.rst b/doc/guides/nics/mlx4.rst index
> > 98b9716..b81a875 100644
> > --- a/doc/guides/nics/mlx4.rst
> > +++ b/doc/guides/nics/mlx4.rst
> > @@ -515,3 +515,80 @@ devices managed by librte_pmd_mlx4.
> >        Port 3 Link Up - speed 40000 Mbps - full-duplex
> >        Done
> >        testpmd>
> > +
> > +Limitations
> > +-----------
> > +
> > +Flow rules
> > +~~~~~~~~~~
> > +
> > +L2 (eth)
> > +^^^^^^^^
> > +
> > +- Can only use real destination MAC
> > +- Source MAC is not taken into consideration
> > +
> > +  For example using testpmd command - src mask must be
> > + 00:00:00:00:00:00  otherwise the following command will fail
> > +
> > +.. code-block:: console
> > +
> > +     testpmd> flow create 1 ingress pattern eth
> > +              src spec 00:16:3e:2b:e6:47 src mask FF:FF:FF:FF:FF:FF
> > +              / end actions drop / end
> > +
> > +- Supports only full MASK
> > +
> > +  For example the following testpmd command will fail
> > +
> > +.. code-block:: console
> > +
> > +     testpmd> flow create 1 ingress pattern eth
> > +              src spec 00:16:3e:2b:e6:47
> > +              dst spec 4A:11:6C:FA:60:D0 dst mask FF:00:FF:FF:FF:00
> > +              / end actions drop / end
> > +
> > +
> > +- When configured to run in promiscuous or all-multicast modes does
> > +  not support additional rules
> > +- Does not support the explicit exclusion of all multicast traffic
> > +- Does not support partial VLAN TCI VID matching
> > +
> > +L3 (ipv4)
> > +^^^^^^^^^
> > +
> > +- Supports only 0 or full mask. Prerequisites: Need to have eth dst
> > +spec
> 
> Plans on updating mlx5 guide with this info too?
> AFAIK ConnectX-4 and 5 also have this limitation and it can save some hours
> of debugging.

Which of the above limitations you encountered? 

> 
>   Marcelo
  

On Fri, Feb 09, 2018 at 05:39:50PM +0100, Thomas Monjalon wrote:
> 09/02/2018 17:21, Adrien Mazarguil:
> > This section is titled "Limitations" but contains a mix of features,
> > limitations and quirks, more like "Random thoughts regarding rte_flow
> > support". I think this is not what users might expect from such a
> > documentation which must be exhaustive and consistent. Getting there may
> > involve tables.
> 
> +Cc Ferruh
> 
> > My suggestion is to first get everyone agree on a common rte_flow
> > capabilities documentation format all PMDs could reuse and then fill in the
> > blanks. What's your opinion?
> 
> I think it's better to have some random thoughts than nothing.
> All the comments you gave in this thread deserve to be written in
> the documentation as soon as possible.

Right, but as a side note in the meantime, more complete documentation is
already available in Doxygen format in mlx4_flow.c. *All* limitations are
also returned in plain text through error messages (rte_flow_error.message)
in the same file (git grep 'msg =' drivers/net/mlx4/mlx4_flow.c).

While not proper documentation, it shouldn't be too difficult to write an
exhaustive list based on that information.

> Working on a better standardized documentation (longer term) should
> not prevent us to write some messy notes in the meantime.

I'm not sure, misleading documentation can do more harm than good for all
parties in my opinion, with users assuming features or limitations that do
not exist, then making purchase decisions and bug reports based on that.

> Is there already some similar rte_flow notes in other PMD docs?

I'm not aware of any, but I do think it's a good idea to start with mlx4.

> About the common documentation, do you think about a generated table
> like it is done for other features?

I'm not sure about this specific format, which can be used to deal with
a bunch of concrete use cases (e.g. "matching TCPv4 traffic and sending it
to some queue = Y") but more than a dozen like that will make it too
confusing to be useful.

I think the current "Flow API = Y" is enough, the rest should be described
in a more verbose format common to all PMDs which remains to be defined.

By "tables" I mean coming up with a visual representation for flow
attributes, patterns and action lists on a single line such as:

 +---------+   +-----+------+-----+    +-------+-----+
 | ingress | : | ETH | IPV4 | UDP | => | COUNT | RSS |
 +---------+   +-----+------+-----+    +-------+-----+

And/or some kind of grammar summarizing all the possibilities, e.g. for
mlx4:

 ingress : ETH [ VLAN ] [ IPV4 [ (UDP | TCP) ] ] => [ VOID ] (DROP | QUEUE | RSS)

With features/limitation for each item described separately.

> Do you plan to provide a template or an example?

I certainly would like to submit something with the plan I suggested,
unfortunately I don't have time to work on that at the moment.
  

On Mon, Feb 12, 2018 at 07:41:03AM +0000, Shahaf Shuler wrote:
> Hi Marcelo,
> 
> Sunday, February 11, 2018 9:31 PM, Marcelo Ricardo:
> > On Thu, Feb 08, 2018 at 06:55:54AM +0000, Ophir Munk wrote:
> > > From: Moti Haimovsky <motih@mellanox.com>
> > >
> > > This patch updates mlx4 documentation with flow configuration
> > > limitations imposed by NIC hardware and PMD implementation
> > >
> > > Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> > > ---
> > >  doc/guides/nics/mlx4.rst | 77
> > > ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 77 insertions(+)
> > >
> > > diff --git a/doc/guides/nics/mlx4.rst b/doc/guides/nics/mlx4.rst index
> > > 98b9716..b81a875 100644
> > > --- a/doc/guides/nics/mlx4.rst
> > > +++ b/doc/guides/nics/mlx4.rst
> > > @@ -515,3 +515,80 @@ devices managed by librte_pmd_mlx4.
> > >        Port 3 Link Up - speed 40000 Mbps - full-duplex
> > >        Done
> > >        testpmd>
> > > +
> > > +Limitations
> > > +-----------
> > > +
> > > +Flow rules
> > > +~~~~~~~~~~
> > > +
> > > +L2 (eth)
> > > +^^^^^^^^
> > > +
> > > +- Can only use real destination MAC
> > > +- Source MAC is not taken into consideration
> > > +
> > > +  For example using testpmd command - src mask must be
> > > + 00:00:00:00:00:00  otherwise the following command will fail
> > > +
> > > +.. code-block:: console
> > > +
> > > +     testpmd> flow create 1 ingress pattern eth
> > > +              src spec 00:16:3e:2b:e6:47 src mask FF:FF:FF:FF:FF:FF
> > > +              / end actions drop / end
> > > +
> > > +- Supports only full MASK
> > > +
> > > +  For example the following testpmd command will fail
> > > +
> > > +.. code-block:: console
> > > +
> > > +     testpmd> flow create 1 ingress pattern eth
> > > +              src spec 00:16:3e:2b:e6:47
> > > +              dst spec 4A:11:6C:FA:60:D0 dst mask FF:00:FF:FF:FF:00
> > > +              / end actions drop / end
> > > +
> > > +
> > > +- When configured to run in promiscuous or all-multicast modes does
> > > +  not support additional rules
> > > +- Does not support the explicit exclusion of all multicast traffic
> > > +- Does not support partial VLAN TCI VID matching
> > > +
> > > +L3 (ipv4)
> > > +^^^^^^^^^
> > > +
> > > +- Supports only 0 or full mask. Prerequisites: Need to have eth dst
> > > +spec
> > 
> > Plans on updating mlx5 guide with this info too?
> > AFAIK ConnectX-4 and 5 also have this limitation and it can save some hours
> > of debugging.
> 
> Which of the above limitations you encountered? 

The need to have eth dst specified.

> 
> > 
> >   Marcelo
>
  

Monday, February 12, 2018 2:46 PM, Marcelo Ricardo Leitner:
> On Mon, Feb 12, 2018 at 07:41:03AM +0000, Shahaf Shuler wrote:
> The need to have eth dst specified.

This is not correct. the device can support such rules.

Are you running on VF? 
Can you paste the rule you try to create and the error? 

> 
> >
> > >
> > >   Marcelo
> >
  

12/02/2018 12:23, Adrien Mazarguil:
> On Fri, Feb 09, 2018 at 05:39:50PM +0100, Thomas Monjalon wrote:
> > I think it's better to have some random thoughts than nothing.
> > All the comments you gave in this thread deserve to be written in
> > the documentation as soon as possible.
> 
> Right, but as a side note in the meantime, more complete documentation is
> already available in Doxygen format in mlx4_flow.c. *All* limitations are
> also returned in plain text through error messages (rte_flow_error.message)
> in the same file (git grep 'msg =' drivers/net/mlx4/mlx4_flow.c).

OK, so we can just point to this file in the documentation and we are done.

[...]
> > Do you plan to provide a template or an example?
> 
> I certainly would like to submit something with the plan I suggested,
> unfortunately I don't have time to work on that at the moment.

Is it worth the effort, given it is already documented in code?
  

On Mon, Feb 12, 2018 at 02:58:31PM +0100, Thomas Monjalon wrote:
> 12/02/2018 12:23, Adrien Mazarguil:
> > On Fri, Feb 09, 2018 at 05:39:50PM +0100, Thomas Monjalon wrote:
> > > I think it's better to have some random thoughts than nothing.
> > > All the comments you gave in this thread deserve to be written in
> > > the documentation as soon as possible.
> > 
> > Right, but as a side note in the meantime, more complete documentation is
> > already available in Doxygen format in mlx4_flow.c. *All* limitations are
> > also returned in plain text through error messages (rte_flow_error.message)
> > in the same file (git grep 'msg =' drivers/net/mlx4/mlx4_flow.c).
> 
> OK, so we can just point to this file in the documentation and we are done.

One remaining issue is that we do not generate documentation out of PMDs
since they do not expose public APIs. We can still somehow point to their
source code though.

> [...]
> > > Do you plan to provide a template or an example?
> > 
> > I certainly would like to submit something with the plan I suggested,
> > unfortunately I don't have time to work on that at the moment.
> 
> Is it worth the effort, given it is already documented in code?

I don't think it's super urgent but I definitely think it would still be
useful, if only to summarize all quirks at once in one place instead of
leaving users discover them through trial & error as only one error can be
reported at once.

Keep in mind not all features/limitations are documented in Doxygen format;
PMDs are not forced to have one parse function per pattern item for
instance. In mlx4, supported actions are handled by a common function and
are not documented individually.
  

12/02/2018 15:19, Adrien Mazarguil:
> On Mon, Feb 12, 2018 at 02:58:31PM +0100, Thomas Monjalon wrote:
> > 12/02/2018 12:23, Adrien Mazarguil:
> > > On Fri, Feb 09, 2018 at 05:39:50PM +0100, Thomas Monjalon wrote:
> > > > I think it's better to have some random thoughts than nothing.
> > > > All the comments you gave in this thread deserve to be written in
> > > > the documentation as soon as possible.
> > > 
> > > Right, but as a side note in the meantime, more complete documentation is
> > > already available in Doxygen format in mlx4_flow.c. *All* limitations are
> > > also returned in plain text through error messages (rte_flow_error.message)
> > > in the same file (git grep 'msg =' drivers/net/mlx4/mlx4_flow.c).
> > 
> > OK, so we can just point to this file in the documentation and we are done.
> 
> One remaining issue is that we do not generate documentation out of PMDs
> since they do not expose public APIs. We can still somehow point to their
> source code though.

Yes, just mention the source code where the information is.
It can help.

And if you or Ophir think there is some outstanding limitations,
we can just add a note with a sentence like: "One of the limitation
important to be aware of is..."