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

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

[Ophir Munk]

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
+
+L4 (tcp/udp)
+^^^^^^^^^^^^
+
+- Supports only full mask
+
+  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
+
+  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.
+
+- 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)
+- Uses default constant RSS key
+- Only power of two number of queues is supported
+- Every Rx queue can be specified only once in RSS action
-- 
2.7.4

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

[Mcnamara, John]

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

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

[Adrien Mazarguil]

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?

-- 
Adrien Mazarguil
6WIND

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

[Thomas Monjalon]

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?

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

[Marcelo Ricardo Leitner]

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

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

[Shahaf Shuler]

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

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

[Adrien Mazarguil]

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.

-- 
Adrien Mazarguil
6WIND

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

[Marcelo Ricardo Leitner]

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
> 

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

[Shahaf Shuler]

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

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

[Thomas Monjalon]

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?

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

[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.

> [...]
> > > 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.

-- 
Adrien Mazarguil
6WIND

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

[Thomas Monjalon]

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..."

[dpdk-dev] [PATCH v2] doc: update mlx4 flow limitations

[Ophir Munk]

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

Signed-off-by: Moti Haimovsky <motih@mellanox.com>
Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
v1: initial version (use testpmd examples)
v2: enhance and add rte_flow examples

 doc/guides/nics/mlx4.rst | 383 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 383 insertions(+)

diff --git a/doc/guides/nics/mlx4.rst b/doc/guides/nics/mlx4.rst
index 98b9716..02cea79 100644
--- a/doc/guides/nics/mlx4.rst
+++ b/doc/guides/nics/mlx4.rst
@@ -515,3 +515,386 @@ devices managed by librte_pmd_mlx4.
       Port 3 Link Up - speed 40000 Mbps - full-duplex
       Done
       testpmd>
+
+Flow Limitations
+----------------
+
+- Flows are specified by rte_flow API (defined in **rte_flow.h**) which provides
+  a generic means to match specific traffic. Please refer to
+  http://dpdk.org/doc/guides/prog_guide/rte_flow.html
+- testpmd application **(app/test-pmd/)** has a command line interface where
+  flows can be specified. These flows are translated by testpmd
+  to rte_flow calls.
+- **drivers/net/mlx4/mlx4_flow.c** can be used as a reference to flow
+  limitations written in source code.
+
+General
+~~~~~~~
+
+.. code-block:: console
+
+    struct rte_flow_attr {
+        uint32_t group; /**< Priority group. */
+        uint32_t priority; /**< Priority level within group. */
+        uint32_t ingress:1; /**< Rule applies to ingress traffic. */
+        uint32_t egress:1; /**< Rule applies to egress traffic. */
+        uint32_t reserved:30; /**< Reserved, must be zero. */
+    }
+
+    struct rte_flow_attr *attr;
+
+- No support for mlx4 group rte_flow
+
+.. code-block:: console
+
+    if (attr->group)
+             print_err("groups are not supported");
+
+- No support for mlx4 rte_flow priority above 0xfff
+
+.. code-block:: console
+
+    if (attr->priority > 0xfff)
+             print_err("maximum priority level is 0xfff");
+
+- No support for mlx4 rte_flow egress filters
+
+.. code-block:: console
+
+    if (attr->egress)
+             print_err("egress is not supported");
+
+- Must specify mlx4 rte_flow ingress filters
+
+.. code-block:: console
+
+    if (!attr->ingress)
+             print_err("only ingress is supported");
+
+Flow rules filters
+~~~~~~~~~~~~~~~~~~
+
+Flow rules filters can be validated using testpmd **flow validate** syntax.
+
+L2 (ETH)
+^^^^^^^^
+
+This section documents flow rules limitations related to RTE_FLOW_ITEM_TYPE_ETH.
+It does not apply to an inner ETH used after VXLAN since mlx4 can't match those
+yet.
+
+.. code-block:: console
+
+    /**
+     * RTE_FLOW_ITEM_TYPE_ETH
+     *
+     * Matches an Ethernet header.
+     */
+    struct rte_flow_item_eth {
+    	struct ether_addr dst; /**< Destination MAC. */
+    	struct ether_addr src; /**< Source MAC. */
+    	rte_be16_t type; /**< EtherType. */
+    };
+
+    struct rte_flow_item_eth *mask;
+
+- Can only use real destination MAC.
+- EtherType is not taken into consideration.
+- Source MAC is not taken into consideration and should be set to 0
+
+.. code-block:: console
+
+    /**
+     * Summarize all src adrresses
+     **/
+    for (i = 0; i != sizeof(mask->src.addr_bytes); ++i)
+			sum_src += mask->src.addr_bytes[i];
+
+    if (sum_src)
+        print_err("mlx4 does not support source MAC matching");
+
+
+Using testpmd application - src mask must be 00:00:00:00:00:00
+otherwise the following command will fail.
+
+.. code-block:: console
+
+     testpmd> flow validate 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.
+- No support for partial masks, except in the specific case of matching
+  all multicast traffic (spec->dst and mask->dst equal to
+  01:00:00:00:00:00).
+
+.. code-block:: console
+
+    /**
+     * Summarize all dst adrresses
+     */
+    for (i = 0; i != sizeof(mask->dst.addr_bytes); ++i) {
+	sum_dst += mask->dst.addr_bytes[i];
+
+        if (sum_dst != (0xffui8 * ETHER_ADDR_LEN))
+            print_err("mlx4 does not support matching partial"
+                         " Ethernet fields");
+    }
+
+Using the following testpmd command with partial mask will fail.
+
+.. code-block:: console
+
+     testpmd> flow validate 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
+
+.. code-block:: console
+
+        if (flow->promisc || flow->allmulti)
+                print_err("mlx4 does not support additional matching"
+                        " criteria combined with indiscriminate"
+                        " matching on Ethernet headers");
+
+- Does not support the explicit exclusion of all multicast traffic
+
+.. code-block:: console
+
+    if (sum_dst == 1 && mask->dst.addr_bytes[0] == 1)
+        if (!(spec->dst.addr_bytes[0] & 1))
+            print_err("mlx4 does not support the explicit"
+            " exclusion of all multicast traffic");
+
+VLAN
+^^^^
+
+This section documents flow rules limitations related to
+RTE_FLOW_ITEM_TYPE_VLAN.
+
+.. code-block:: console
+
+    /**
+     * RTE_FLOW_ITEM_TYPE_VLAN
+     *
+     * Matches an 802.1Q/ad VLAN tag.
+     *
+     * This type normally follows either RTE_FLOW_ITEM_TYPE_ETH or
+     * RTE_FLOW_ITEM_TYPE_VLAN.
+     */
+    struct rte_flow_item_vlan {
+        rte_be16_t tpid; /**< Tag protocol identifier. */
+        rte_be16_t tci; /**< Tag control information. */
+    };
+
+    struct rte_flow_item_vlan mask;
+
+- TCI VID must be specified
+
+.. code-block:: console
+
+    if (!mask || !mask->tci)
+        print_err("mlx4 cannot match all VLAN traffic while excluding"
+            " non-VLAN traffic, TCI VID must be specified");
+
+- Does not support partial VLAN TCI VID matching
+
+.. code-block:: console
+
+    if (mask->tci != RTE_BE16(0x0fff))
+        print_err("mlx4 does not support partial TCI VID matching");
+
+L3 (IPv4)
+^^^^^^^^^
+
+This section documents flow rules limitations related to
+RTE_FLOW_ITEM_TYPE_IPV4.
+
+.. code-block:: console
+
+    /**
+     * RTE_FLOW_ITEM_TYPE_IPV4
+     *
+     * Matches an IPv4 header.
+     *
+     * Note: IPv4 options are handled by dedicated pattern items.
+     */
+    struct rte_flow_item_ipv4 {
+        struct ipv4_hdr hdr; /**< IPv4 header definition. */
+    };
+
+    struct rte_flow_item_ipv4 *mask;
+
+- Prerequisites: must follow eth dst spec definition.
+- Supports only zero or full one's source and destinatin masks.
+
+.. code-block:: console
+
+    if (mask &&
+        (uint32_t)(mask->hdr.src_addr + 1) > 1U ||
+        (uint32_t)(mask->hdr.dst_addr + 1) > 1U))
+            print_err("mlx4 does not support matching partial IPv4 fields");
+
+Using the following testpmd command with ipv4 prefix 16 will fail.
+
+.. code-block:: console
+
+     testpmd> flow validate 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
+
+L3 (IPv6)
+^^^^^^^^^
+
+mlx4 does not support IPv6 filters
+
+L4 UDP
+^^^^^^
+
+This section documents flow rules limitations related to RTE_FLOW_ITEM_TYPE_UDP.
+
+.. code-block:: console
+
+    /**
+     * RTE_FLOW_ITEM_TYPE_UDP.
+     *
+     * Matches a UDP header.
+     */
+    struct rte_flow_item_udp {
+        struct udp_hdr hdr; /**< UDP header definition. */
+    };
+
+    struct rte_flow_item_udp mask;
+
+- Prerequisites - must follow eth dst followed by IPv4 specs
+- Supports only zero or full source and destination ports masks.
+
+.. code-block:: console
+
+    if (mask &&
+        ((uint16_t)(mask->hdr.src_port + 1) > 1ui16 ||
+         (uint16_t)(mask->hdr.dst_port + 1) > 1ui16))
+        print_err("mlx4 does not support matching partial UDP fields");
+
+L4 TCP
+^^^^^^
+
+This section documents flow rules limitations related to RTE_FLOW_ITEM_TYPE_TCP.
+
+.. code-block:: console
+
+    /**
+     * RTE_FLOW_ITEM_TYPE_TCP.
+     *
+     * Matches a TCP header.
+     */
+    struct rte_flow_item_tcp {
+        struct tcp_hdr hdr; /**< TCP header definition. */
+    };
+
+    struct rte_flow_item_tcp *mask;
+
+- Prerequisites - must follow eth dst spec followed by IPv4 spec
+- Supports only zero or full source and destination ports masks.
+
+.. code-block:: console
+
+    if (mask &&
+        ((uint16_t)(mask->hdr.src_port + 1) > 1ui16 ||
+         (uint16_t)(mask->hdr.dst_port + 1) > 1ui16))
+        print_err("mlx4 does not support matching partial TCP fields");
+
+Flow actions
+~~~~~~~~~~~~
+
+RSS
+^^^
+
+RSS is performed on packets to spread them among several queues based on hash
+function calculation and according to provided parameters.
+
+.. code-block:: console
+
+    struct rte_eth_rss_conf {
+        uint8_t *rss_key;    /**< If not NULL, 40-byte hash key. */
+        uint8_t rss_key_len; /**< hash key length in bytes. */
+        uint64_t rss_hf;     /**< Hash functions to apply - see below. */
+    };
+
+    struct rte_flow_action_rss {
+        const struct rte_eth_rss_conf *rss_conf; /**< RSS parameters. */
+        uint16_t num; /**< Number of entries in queue[]. */
+        uint16_t queue[]; /**< Queues indices to use. */
+    };
+
+    struct rte_flow_action_rss *rss;
+
+- 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)
+- Every Rx queue can be specified only once in RSS action
+
+- Only power of two number of queues is supported
+
+.. code-block:: console
+
+    if (!rte_is_power_of_2(rss->num))
+        print_err("for RSS, mlx4 requires the number of"
+              " queues to be a power of two");
+
+Using the following RSS action with three RSS ports (0 1 2) will fail.
+
+.. code-block:: console
+
+    testpmd> flow create 0  ingress pattern eth dst is f4:52:14:7a:59:81
+    / ipv4 / tcp / end actions rss queues 0 1 2 end / end
+
+- RSS hash key must be 40 characters
+
+.. code-block:: console
+
+    if (rss_conf->rss_key_len !=
+        sizeof(flow->rss->key))
+        print_err("mlx4 supports exactly one RSS hash key"
+                  " length: 40"
+
+- Packets must be distributed over consecutive queue indeces only
+
+.. code-block:: console
+
+    for (i = 1; i < rss->num; ++i)
+            if (rss->queue[i] - rss->queue[i - 1] != 1)
+                break;
+    if (i != rss->num)
+        "mlx4 requires RSS contexts to use"
+                " consecutive queue indices only");
+
+Using the following RSS action with non-consecutive ports (0 2) will fail.
+
+.. code-block:: console
+
+    testpmd> flow create 0  ingress pattern eth dst is f4:52:14:7a:59:81
+    / ipv4 / tcp / end actions rss queues 0 2 end / end
+
+- The first queue index specified in RSS context must be aligned
+  to context size
+
+.. code-block:: console
+
+    if (rss->queue[0] % rss->num)
+        print_err("mlx4 requires the first queue of a RSS"
+                " context to be aligned on a multiple"
+                " of the context size");
+
+Using the following RSS action with a fist queue index set as 1 - will fail.
+
+.. code-block:: console
+
+    testpmd> flow create 0  ingress pattern eth dst is f4:52:14:7a:59:81
+    / ipv4 / tcp / end actions rss queues 1 2 end / end
+
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2] doc: update mlx4 flow limitations

[Adrien Mazarguil]

On Sat, Feb 24, 2018 at 10:36:23PM +0000, Ophir Munk wrote:
> This patch updates mlx4 documentation with flow
> configuration limitations imposed by NIC hardware and
> PMD implementation
> 
> Signed-off-by: Moti Haimovsky <motih@mellanox.com>
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> ---
> v1: initial version (use testpmd examples)
> v2: enhance and add rte_flow examples

I still don't agree with this version (reasons below), however if anyone
thinks it's production-ready and good enough for a start, please send your
acked-by line and I won't oppose its inclusion.

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

It's still announcing limitations instead of features, I remain unconvinced
by this approach for reasons I already stated [1].

> +
> +- Flows are specified by rte_flow API (defined in **rte_flow.h**) which provides
> +  a generic means to match specific traffic. Please refer to
> +  http://dpdk.org/doc/guides/prog_guide/rte_flow.html

This should be a relative internal link to remain consistent when
documentation is hosted on a different server or generated as a PDF file,
see the various ":ref:" examples under doc/.

> +- testpmd application **(app/test-pmd/)** has a command line interface where
> +  flows can be specified. These flows are translated by testpmd
> +  to rte_flow calls.

Irrelevant unless you also describe the testpmd syntax will be used for
subsequent examples and by providing a link to its definition.

> +- **drivers/net/mlx4/mlx4_flow.c** can be used as a reference to flow
> +  limitations written in source code.

It should be used as reference by the documentation writer, it's not really
meant for readers of this documentation. PMD developers already know where
to look.

> +
> +General
> +~~~~~~~
> +
> +.. code-block:: console
> +
> +    struct rte_flow_attr {
> +        uint32_t group; /**< Priority group. */
> +        uint32_t priority; /**< Priority level within group. */
> +        uint32_t ingress:1; /**< Rule applies to ingress traffic. */
> +        uint32_t egress:1; /**< Rule applies to egress traffic. */
> +        uint32_t reserved:30; /**< Reserved, must be zero. */
> +    }
> +
> +    struct rte_flow_attr *attr;

You shouldn't copy/paste the API nor PMD code in this documentation. It's
overly verbose and makes it difficult to maintain. Use links if you want to
refer to something documented elsewhere.

> +
> +- No support for mlx4 group rte_flow

Did you mean "No support for group values other than 0"?

Again, please focus on features instead of limitations, otherwise there's no
end to this.

> +
> +.. code-block:: console
> +
> +    if (attr->group)
> +             print_err("groups are not supported");
> +

Besides the made-up print_err() definition, is this documentation expected
to be synchronized every time error messages are updated in PMD code?

> +- No support for mlx4 rte_flow priority above 0xfff
> +
> +.. code-block:: console
> +
> +    if (attr->priority > 0xfff)
> +             print_err("maximum priority level is 0xfff");

You don't need to provide code proof to every statement. Moreover this one
is inaccurate, the maximum priority level depends on whether isolated mode
is enabled (0-4095 with rte_flow in isolated mode, 0-4094 otherwise).

> +
> +- No support for mlx4 rte_flow egress filters
> +
> +.. code-block:: console
> +
> +    if (attr->egress)
> +             print_err("egress is not supported");
> +
> +- Must specify mlx4 rte_flow ingress filters
> +
> +.. code-block:: console
> +
> +    if (!attr->ingress)
> +             print_err("only ingress is supported");
> +

Same comments as above regarding past and subsequent code-blocks...

> +Flow rules filters
> +~~~~~~~~~~~~~~~~~~
> +
> +Flow rules filters can be validated using testpmd **flow validate** syntax.

It's already part of testpmd documentation.

> +
> +L2 (ETH)
> +^^^^^^^^
> +
> +This section documents flow rules limitations related to RTE_FLOW_ITEM_TYPE_ETH.
> +It does not apply to an inner ETH used after VXLAN since mlx4 can't match those
> +yet.

OK for VXLAN, now what about NVGRE and other tunnel protocols, I assume mlx4
can match them? (see how I'm trying to make this documentation only about
supported features :)

> +
> +.. code-block:: console
> +
> +    /**
> +     * RTE_FLOW_ITEM_TYPE_ETH
> +     *
> +     * Matches an Ethernet header.
> +     */
> +    struct rte_flow_item_eth {
> +    	struct ether_addr dst; /**< Destination MAC. */
> +    	struct ether_addr src; /**< Source MAC. */
> +    	rte_be16_t type; /**< EtherType. */
> +    };
> +
> +    struct rte_flow_item_eth *mask;
> +
> +- Can only use real destination MAC.

real => exact / fully specified?

> +- EtherType is not taken into consideration.

This reads like you can put whatever in this field and PMD just ignores it,
while it's in fact not the case. Masking this field causes the PMD to
explicitly reject the requested flow rule.

> +- Source MAC is not taken into consideration and should be set to 0

Misleading since it's not the source MAC itself but its mask (like EtherType
in fact).

> +
> +.. code-block:: console
> +
> +    /**
> +     * Summarize all src adrresses

adrresses => addresses

> +     **/
> +    for (i = 0; i != sizeof(mask->src.addr_bytes); ++i)
> +			sum_src += mask->src.addr_bytes[i];
> +
> +    if (sum_src)
> +        print_err("mlx4 does not support source MAC matching");
> +
> +
> +Using testpmd application - src mask must be 00:00:00:00:00:00
> +otherwise the following command will fail.

Please remove code proofs. Code is subject to change, what HW supports on
the other hand isn't. A proper description should be enough.

> +
> +.. code-block:: console
> +
> +     testpmd> flow validate 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.
> +- No support for partial masks, except in the specific case of matching
> +  all multicast traffic (spec->dst and mask->dst equal to
> +  01:00:00:00:00:00).
> +
> +.. code-block:: console
> +
> +    /**
> +     * Summarize all dst adrresses
> +     */
> +    for (i = 0; i != sizeof(mask->dst.addr_bytes); ++i) {
> +	sum_dst += mask->dst.addr_bytes[i];
> +
> +        if (sum_dst != (0xffui8 * ETHER_ADDR_LEN))
> +            print_err("mlx4 does not support matching partial"
> +                         " Ethernet fields");
> +    }
> +
> +Using the following testpmd command with partial mask will fail.
> +
> +.. code-block:: console
> +
> +     testpmd> flow validate 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
> +

While I know the answer, an unsuspecting reader will wonder why "src spec"
is specified and the comment doesn't fail on that basis?

More generally, avoid testpmd examples, unless you use its syntax to
describe what's supported, e.g.:

 eth [dst [(spec|is) {MAC-48_spec}] [last {MAC-48_last}] [mask {MAC-48_mask}]]

 MAC-48_spec: standard MAC address.
 MAC-48_last: must be equal to MAC-48_spec when "mask" or "is" are specified.
 MAX-48_mask: must be ether full or empty, not partial.

It's just an example, I don't find it particularly clear.

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

No such modes at the rte_flow level. You have to describe in terms of
full/empty/partial/specific field masks.

> +.. code-block:: console
> +
> +        if (flow->promisc || flow->allmulti)
> +                print_err("mlx4 does not support additional matching"
> +                        " criteria combined with indiscriminate"
> +                        " matching on Ethernet headers");
> +
> +- Does not support the explicit exclusion of all multicast traffic

Same comment.

> +.. code-block:: console
> +
> +    if (sum_dst == 1 && mask->dst.addr_bytes[0] == 1)
> +        if (!(spec->dst.addr_bytes[0] & 1))
> +            print_err("mlx4 does not support the explicit"
> +            " exclusion of all multicast traffic");
> +
> +VLAN
> +^^^^
> +
> +This section documents flow rules limitations related to
> +RTE_FLOW_ITEM_TYPE_VLAN.
> +
> +.. code-block:: console
> +
> +    /**
> +     * RTE_FLOW_ITEM_TYPE_VLAN
> +     *
> +     * Matches an 802.1Q/ad VLAN tag.
> +     *
> +     * This type normally follows either RTE_FLOW_ITEM_TYPE_ETH or
> +     * RTE_FLOW_ITEM_TYPE_VLAN.
> +     */
> +    struct rte_flow_item_vlan {
> +        rte_be16_t tpid; /**< Tag protocol identifier. */
> +        rte_be16_t tci; /**< Tag control information. */
> +    };
> +
> +    struct rte_flow_item_vlan mask;
> +
> +- TCI VID must be specified
> +
> +.. code-block:: console
> +
> +    if (!mask || !mask->tci)
> +        print_err("mlx4 cannot match all VLAN traffic while excluding"
> +            " non-VLAN traffic, TCI VID must be specified");
> +
> +- Does not support partial VLAN TCI VID matching
> +
> +.. code-block:: console
> +
> +    if (mask->tci != RTE_BE16(0x0fff))
> +        print_err("mlx4 does not support partial TCI VID matching");

I'm sure all this could have been described in a single line.

> +L3 (IPv4)
> +^^^^^^^^^
> +
> +This section documents flow rules limitations related to
> +RTE_FLOW_ITEM_TYPE_IPV4.
> +
> +.. code-block:: console
> +
> +    /**
> +     * RTE_FLOW_ITEM_TYPE_IPV4
> +     *
> +     * Matches an IPv4 header.
> +     *
> +     * Note: IPv4 options are handled by dedicated pattern items.
> +     */
> +    struct rte_flow_item_ipv4 {
> +        struct ipv4_hdr hdr; /**< IPv4 header definition. */
> +    };
> +
> +    struct rte_flow_item_ipv4 *mask;
> +
> +- Prerequisites: must follow eth dst spec definition.
> +- Supports only zero or full one's source and destinatin masks.

destinatin => destination

> +
> +.. code-block:: console
> +
> +    if (mask &&
> +        (uint32_t)(mask->hdr.src_addr + 1) > 1U ||
> +        (uint32_t)(mask->hdr.dst_addr + 1) > 1U))
> +            print_err("mlx4 does not support matching partial IPv4 fields");
> +
> +Using the following testpmd command with ipv4 prefix 16 will fail.
> +
> +.. code-block:: console
> +
> +     testpmd> flow validate 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

Same comments. Note mlx4 does not support partial masks. It's always either
full or empty except for the allmulticast bit which is implemented through a
kind of workaround. This may be mentioned once at the beginning to shorten
documentation.

> +
> +L3 (IPv6)
> +^^^^^^^^^
> +
> +mlx4 does not support IPv6 filters

Not much to say regarding this section except it shouldn't exist if only
features were documented, however ending sentences with periods makes
documentation look more professional and all.

> +
> +L4 UDP
> +^^^^^^
> +
> +This section documents flow rules limitations related to RTE_FLOW_ITEM_TYPE_UDP.
> +
> +.. code-block:: console
> +
> +    /**
> +     * RTE_FLOW_ITEM_TYPE_UDP.
> +     *
> +     * Matches a UDP header.
> +     */
> +    struct rte_flow_item_udp {
> +        struct udp_hdr hdr; /**< UDP header definition. */
> +    };
> +
> +    struct rte_flow_item_udp mask;
> +
> +- Prerequisites - must follow eth dst followed by IPv4 specs

True, however what do "dst" and "specs" mean in this context?

> +- Supports only zero or full source and destination ports masks.
> +
> +.. code-block:: console
> +
> +    if (mask &&
> +        ((uint16_t)(mask->hdr.src_port + 1) > 1ui16 ||
> +         (uint16_t)(mask->hdr.dst_port + 1) > 1ui16))
> +        print_err("mlx4 does not support matching partial UDP fields");
> +

Same comments as for IPv4.

> +L4 TCP
> +^^^^^^
> +
> +This section documents flow rules limitations related to RTE_FLOW_ITEM_TYPE_TCP.
> +
> +.. code-block:: console
> +
> +    /**
> +     * RTE_FLOW_ITEM_TYPE_TCP.
> +     *
> +     * Matches a TCP header.
> +     */
> +    struct rte_flow_item_tcp {
> +        struct tcp_hdr hdr; /**< TCP header definition. */
> +    };
> +
> +    struct rte_flow_item_tcp *mask;
> +
> +- Prerequisites - must follow eth dst spec followed by IPv4 spec
> +- Supports only zero or full source and destination ports masks.
> +
> +.. code-block:: console
> +
> +    if (mask &&
> +        ((uint16_t)(mask->hdr.src_port + 1) > 1ui16 ||
> +         (uint16_t)(mask->hdr.dst_port + 1) > 1ui16))
> +        print_err("mlx4 does not support matching partial TCP fields");
> +

Same comments as for TCP.

> +Flow actions
> +~~~~~~~~~~~~
> +
> +RSS
> +^^^
> +
> +RSS is performed on packets to spread them among several queues based on hash
> +function calculation and according to provided parameters.
> +
> +.. code-block:: console
> +
> +    struct rte_eth_rss_conf {
> +        uint8_t *rss_key;    /**< If not NULL, 40-byte hash key. */
> +        uint8_t rss_key_len; /**< hash key length in bytes. */
> +        uint64_t rss_hf;     /**< Hash functions to apply - see below. */
> +    };
> +
> +    struct rte_flow_action_rss {
> +        const struct rte_eth_rss_conf *rss_conf; /**< RSS parameters. */
> +        uint16_t num; /**< Number of entries in queue[]. */
> +        uint16_t queue[]; /**< Queues indices to use. */
> +    };
> +
> +    struct rte_flow_action_rss *rss;
> +
> +- 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)

upd => UDP, also use caps for acronyms (e.g. IPv4, IPv6, TCP).

This reads like there's more than what you listed. You should be more
specific:

- Combined IPv4 source and destination addresses.
- Combined IPv6 source and destination addresses.
- Combined UDP source and destination ports.
- Combined TCP source and destination ports.

You may even throw in the related ETH_RSS_* macros for good measure.

> +- Every Rx queue can be specified only once in RSS action
> +
> +- Only power of two number of queues is supported
> +
> +.. code-block:: console
> +
> +    if (!rte_is_power_of_2(rss->num))
> +        print_err("for RSS, mlx4 requires the number of"
> +              " queues to be a power of two");
> +
> +Using the following RSS action with three RSS ports (0 1 2) will fail.

ports => queues?

> +
> +.. code-block:: console
> +
> +    testpmd> flow create 0  ingress pattern eth dst is f4:52:14:7a:59:81
> +    / ipv4 / tcp / end actions rss queues 0 1 2 end / end
> +
> +- RSS hash key must be 40 characters

characters => bytes?

> +
> +.. code-block:: console
> +
> +    if (rss_conf->rss_key_len !=
> +        sizeof(flow->rss->key))
> +        print_err("mlx4 supports exactly one RSS hash key"
> +                  " length: 40"
> +
> +- Packets must be distributed over consecutive queue indeces only

indeces => indices

> +
> +.. code-block:: console
> +
> +    for (i = 1; i < rss->num; ++i)
> +            if (rss->queue[i] - rss->queue[i - 1] != 1)
> +                break;
> +    if (i != rss->num)
> +        "mlx4 requires RSS contexts to use"
> +                " consecutive queue indices only");

This code might be difficult to compile.

You know, I'm starting to think error messages in code proofs are verbose
enough on their own, no need for documentation. Just point readers to the
source code :)

> +Using the following RSS action with non-consecutive ports (0 2) will fail.
> +
> +.. code-block:: console
> +
> +    testpmd> flow create 0  ingress pattern eth dst is f4:52:14:7a:59:81
> +    / ipv4 / tcp / end actions rss queues 0 2 end / end
> +
> +- The first queue index specified in RSS context must be aligned
> +  to context size
> +
> +.. code-block:: console
> +
> +    if (rss->queue[0] % rss->num)
> +        print_err("mlx4 requires the first queue of a RSS"
> +                " context to be aligned on a multiple"
> +                " of the context size");
> +
> +Using the following RSS action with a fist queue index set as 1 - will fail.
> +
> +.. code-block:: console
> +
> +    testpmd> flow create 0  ingress pattern eth dst is f4:52:14:7a:59:81
> +    / ipv4 / tcp / end actions rss queues 1 2 end / end
> +
> -- 
> 2.7.4

No word on the remaining limitations?

- RTE_FLOW_ITEM_TYPE_INVERT
- RTE_FLOW_ITEM_TYPE_ANY
- RTE_FLOW_ITEM_TYPE_PF
- RTE_FLOW_ITEM_TYPE_VF
- RTE_FLOW_ITEM_TYPE_PORT
- RTE_FLOW_ITEM_TYPE_RAW
- RTE_FLOW_ITEM_TYPE_SCTP
- [...] (see rte_flow.h)
- RTE_FLOW_ACTION_TYPE_PASSTHRU
- RTE_FLOW_ACTION_TYPE_MARK
- [...] (see rte_flow.h)

All right, I think you got the point...

[1] Message-ID: <20180209162124.GD4256@6wind.com>
    http://dpdk.org/ml/archives/dev/2018-February/090649.html

-- 
Adrien Mazarguil
6WIND