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