[dpdk-dev] [PATCH] ethdev: add roughly match pattern

[Adrien Mazarguil]

Hi Zhang,

You should cram "flow API" somewhere in the title of such commits.

On Tue, May 23, 2017 at 07:28:54PM -0400, Qi Zhang wrote:
> Add new meta pattern item RTE_FLOW_TYPE_ITEM_ROUGHLY.
> 
> This is for device that support no-perfect match option.
> Usually a no-perfect match is fast but the cost is accuracy.
> i.e. Signature Match only match pattern's hash value, but it is
> possible two different patterns have the same hash value.
> 
> Matching accuracy level can be configure by subfield threshold.
> Driver can divide the range of threshold and map to different
> accuracy levels that device support.
> 
> Signed-off-by: Qi Zhang <qi.z.zhang at intel.com>

While I really like the "roughly" pattern item name since it perfectly
describes its intended purpose in my opinion, perhaps some may not find this
name appropriate. I would like to hear other people's opinion on the matter
and not be the only one to ack this patch.

Several more comments below.

> ---
>  app/test-pmd/cmdline_flow.c                 | 24 ++++++++++++++
>  app/test-pmd/config.c                       |  1 +
>  doc/guides/prog_guide/rte_flow.rst          | 50 +++++++++++++++++++++++++++++
>  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  2 ++
>  lib/librte_ether/rte_flow.h                 | 30 +++++++++++++++++
>  5 files changed, 107 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index 0fd69f9..18ffcff 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -107,6 +107,8 @@ enum index {
>  	ITEM_END,
>  	ITEM_VOID,
>  	ITEM_INVERT,
> +	ITEM_ROUGHLY,
> +	ITEM_ROUGHLY_THRESHOLD,

"Threshold" is commonly abbreviated as "thresh", I think it's fine if you
use this shorter form here and in the structure definition.

There is also an issue with the position of these enum entries. They should
come in the same order as rte_flow.h definitions like you did, but you are
supposed to add new entries at the end of the various lists in that file
instead of in the middle, otherwise you're destroying API/ABI compatibility
which is not supposed to happen. More on that topic below.

>  	ITEM_ANY,
>  	ITEM_ANY_NUM,
>  	ITEM_PF,
> @@ -426,6 +428,7 @@ static const enum index next_item[] = {
>  	ITEM_END,
>  	ITEM_VOID,
>  	ITEM_INVERT,
> +	ITEM_ROUGHLY,

This will have to be moved at the end of the list.

>  	ITEM_ANY,
>  	ITEM_PF,
>  	ITEM_VF,
> @@ -447,6 +450,12 @@ static const enum index next_item[] = {
>  	ZERO,
>  };
>  
> +static const enum index item_roughly[] = {
> +	ITEM_ROUGHLY_THRESHOLD,

I suggest "ITEM_ROUGHLY_THRESH".

> +	ITEM_NEXT,
> +	ZERO,
> +};
> +
>  static const enum index item_any[] = {
>  	ITEM_ANY_NUM,
>  	ITEM_NEXT,
> @@ -954,6 +963,21 @@ static const struct token token_list[] = {
>  		.next = NEXT(NEXT_ENTRY(ITEM_NEXT)),
>  		.call = parse_vc,
>  	},
> +	[ITEM_ROUGHLY] = {

This will have to be moved at the end of the list.

> +		.name = "roughly",
> +		.help = "match the pattern roughly",

Hehe, the question is who will go out of their way to match traffic roughly
instead of perfectly? They need a better incentive to do so, in a very short
sentence.

> +		.priv = PRIV_ITEM(ROUGHLY,
> +				sizeof(struct rte_flow_item_roughly)),
> +		.next = NEXT(item_roughly),
> +		.call = parse_vc,
> +	},
> +	[ITEM_ROUGHLY_THRESHOLD] = {
> +		.name = "threshold",

"thresh" again, I won't comment them all, you get the idea.

> +		.help = "match accuracy threshold",
> +		.next = NEXT(item_roughly, NEXT_ENTRY(UNSIGNED), item_param),
> +		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_roughly,
> +					threshold)),
> +	},
>  	[ITEM_ANY] = {
>  		.name = "any",
>  		.help = "match any protocol for the current layer",
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 4d873cd..5b0cd4d 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -954,6 +954,7 @@ static const struct {
>  	MK_FLOW_ITEM(END, 0),
>  	MK_FLOW_ITEM(VOID, 0),
>  	MK_FLOW_ITEM(INVERT, 0),
> +	MK_FLOW_ITEM(ROUGHLY, sizeof(struct rte_flow_item_roughly)),

This will have to be moved at the end of the list.

>  	MK_FLOW_ITEM(ANY, sizeof(struct rte_flow_item_any)),
>  	MK_FLOW_ITEM(PF, 0),
>  	MK_FLOW_ITEM(VF, sizeof(struct rte_flow_item_vf)),
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index b587ba9..4cc1876 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -491,6 +491,7 @@ Usage example, matching non-TCPv4 packets only:
>  
>     +-------+----------+
>     | Index | Item     |
> +

This change looks unnecessary.

>     +=======+==========+
>     | 0     | INVERT   |
>     +-------+----------+
> @@ -503,6 +504,55 @@ Usage example, matching non-TCPv4 packets only:
>     | 4     | END      |
>     +-------+----------+
>  
> +Item: ``ROUGHLY``
> +^^^^^^^^^^^^^^^^^

This will have to be moved at the end of the list (documentation also
follows the same order as rte_flow.h).

> +
> +Roughly matching, not perfect match.
> +
> +This is for device that support no-perfect match option.
> +Usually a no-perfect match is fast but the cost is accuracy.
> +i.e. Signature Match only match pattern's hash value, but it is
> +possible two different patterns have the same hash value.
> +
> +Matching accuracy level can be configure by threshold.
> +Driver can divide the range of threshold and map to different
> +accuracy levels that device support.

Please expand these paragraphs to fit 75-79 columns wide like the rest of
the file.

I think a better wording is necessary to provide incentives for applications
to use this mode. I have a few ideas but I'm not familiar enough with the
original signature mode for that. Perhaps John can help?

> +
> +.. _table_rte_flow_item_roughly:
> +
> +.. table:: ROUGHLY
> +
> +   +----------+---------------+--------------------------------------------------+
> +   | Field    |   Subfield    | Value                                            |
> +   +==========+===========+======================================================+
> +   | ``spec`` | ``threshold`` | 0 as perfect match, 0xffffffff as roughest match |
> +   +----------+---------------+--------------------------------------------------+
> +   | ``last`` | ``threshold`` | ignored                                          |
> +   +----------+-----------+------------------------------------------------------+
> +   | ``mask`` | ``threshold`` | ignored                                          |
> +   +----------+-----------+------------------------------------------------------+

Last and mask cannot be ignored. The only items where they are ignored are
those that do not even take a spec definition.

This means that a mask set to 0 is supposed to be the same as no item
provided. PMDs should retrieve the threshold value using something like:

 thresh = spec->thresh & mask->thresh;
 if (last->thresh && (last->thresh & mask->thresh) < thresh)
     complain_unsupported();

Ranges (last) can be ignored when otherwise valid because what matters is
only the lowest threshold value.

See 8.2.3 Pattern item [1] for more information.

> +

Extra empty line.

> +
> +Usage example, roughly match a TCPv4 packets:
> +
> +.. _table_rte_flow_item_roughly_example:
> +
> +.. table:: Roughly matching

How about "Rough matching".

> +
> +   +-------+----------+
> +   | Index | Item     |
> +   +=======+==========+
> +   | 0     | ROUGHLY  |
> +   +-------+----------+
> +   | 1     | Ethernet |
> +   +-------+----------+
> +   | 2     | IPv4     |
> +   +-------+----------+
> +   | 3     | TCP      |
> +   +-------+----------+
> +   | 4     | END      |
> +   +-------+----------+
> +
>  Item: ``PF``
>  ^^^^^^^^^^^^

There is a missing change in this file. You must modify the following
statement:

 - Signature mode of operation is not defined but could be handled through a
   specific item type if needed.

And mention ROUGHLY in index 3 of the table below:

    +---+-------------------+----------+-----+                       |
    | 3 | VF, PF (optional) | ``spec`` | any |                       |
    |   |                   +----------+-----+                       |
    |   |                   | ``last`` | N/A |                       |
    |   |                   +----------+-----+                       |
    |   |                   | ``mask`` | any |                       |
    +---+-------------------+----------+-----+                       |

> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> index 0e50c10..08a88f8 100644
> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> @@ -2513,6 +2513,8 @@ This section lists supported pattern items and their attributes, if any.
>  
>  - ``invert``: perform actions when pattern does not match.
>  
> +- ``roughly``: pattern is matched roughly.
> +

How about "match pattern roughly"? (remember to keep this in sync with
testpmd's inline help string though)

This will also have to be moved at the end of the list.

>  - ``any``: match any protocol for the current layer.
>  
>    - ``num {unsigned}``: number of layers covered.
> diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
> index c47edbc..4921858 100644
> --- a/lib/librte_ether/rte_flow.h
> +++ b/lib/librte_ether/rte_flow.h
> @@ -148,6 +148,18 @@ enum rte_flow_item_type {
>  	RTE_FLOW_ITEM_TYPE_INVERT,
>  
>  	/**
> +	 * [META]
> +	 *
> +	 * Roughly matching, not perfect matching
> +	 *
> +	 * This is for device that support no-perfect matching option.
> +	 * Usually a no-perfect matching is fast but the cost is accuracy.

Perhaps John can help here as well. This one should be kept mostly in sync
with the full description for struct rte_flow_item_roughly and also the
documentation in rte_flow.rst.

> +	 *
> +	 * See struct rte_flow_item_roughly

Missing colon.

> +	 */
> +	RTE_FLOW_ITEM_TYPE_ROUGHLY,
> +

This new item type *must* be moved at the end of the list to avoid breaking
API/ABI and existing applications. Remember it's append-only.

You then need to update the rest of the patch accordingly.

> +	/**
>  	 * Matches any protocol in place of the current layer, a single ANY
>  	 * may also stand for several protocol layers.
>  	 *
> @@ -300,6 +312,24 @@ enum rte_flow_item_type {
>  };
>  
>  /**
> + * RTE_FLOW_ITEM_TYPE_ROUGHLY
> + *
> + * Roughly matching, not perfect match.
> + *
> + * This is for device that support no-perfect match option.
> + * Usually a no-perfect match is fast but the cost is accuracy.
> + * i.e. Signature Match only match pattern's hash value, but it is
> + * possible two different patterns have the same hash value.
> + *
> + * Matching accuracy level can be configure by threshold.
> + * Driver can divide the range of threshold and map to different
> + * accuracy levels that device support.

John?

> + */
> +struct rte_flow_item_roughly {
> +	uint32_t threshold; /**< accuracy threshold*/

How about:

 uint32_t thresh; /**< Accuracy threshold. */

> +};

Again this structure must be defined after all the others to keep the same
order as enum rte_flow_item_type.

> +
> +/**
>   * RTE_FLOW_ITEM_TYPE_ANY
>   *
>   * Matches any protocol in place of the current layer, a single ANY may also
> -- 
> 2.7.4
> 

[1] http://dpdk.org/doc/guides/prog_guide/rte_flow.html#pattern-item

-- 
Adrien Mazarguil
6WIND