[dpdk-dev] ethdev: add roughly match pattern
Checks
Commit Message
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@intel.com>
---
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(+)
Comments
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@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
On Tue, May 30, 2017 at 02:46:30PM +0200, Adrien Mazarguil wrote:
>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@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.
"no-perfect" has been used a few times in the documentation. How about
"IMPERFECT" as item name?
> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Wednesday, May 31, 2017 3:52 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>; Mcnamara, John
> <john.mcnamara@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] ethdev: add roughly match pattern
>
> On Tue, May 30, 2017 at 02:46:30PM +0200, Adrien Mazarguil wrote:
> >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@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.
>
> "no-perfect" has been used a few times in the documentation. How about
> "IMPERFECT" as item name?
>
"Imperfect" looks better for me,
If no other objection, I will use this in V2.
Thanks
Qi
> --
> Gaëtan Rivet
> 6WIND
On Thu, Jun 01, 2017 at 01:44:56AM +0000, Zhang, Qi Z wrote:
> > -----Original Message-----
> > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > Sent: Wednesday, May 31, 2017 3:52 PM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> > Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>; Mcnamara, John
> > <john.mcnamara@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH] ethdev: add roughly match pattern
> >
> > On Tue, May 30, 2017 at 02:46:30PM +0200, Adrien Mazarguil wrote:
> > >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@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.
> >
> > "no-perfect" has been used a few times in the documentation. How about
> > "IMPERFECT" as item name?
> >
> "Imperfect" looks better for me,
> If no other objection, I will use this in V2.
An "imperfect threshold" doesn't make a lot more sense than a roughness
one. Who wants to match flows imperfectly by the way?
How about "fuzzy" then? Fuzzy matching, fuzzy thresholds, those are pretty
well understood concepts. And people like fuzzy things.
The patchset is based on
http://dpdk.org/dev/patchwork/patch/25292/
v3:
- will check fuzzy match's last and mask value.
v2:
- add comment to explain macro replacement
- fix wrong flow type assignment
Qi Zhang (3):
net/ixgbe: replace macro with inline function
net/ixgbe: enable signature match for consistent API
net/ixgbe: enable IPv6 for consistent API
drivers/net/ixgbe/ixgbe_flow.c | 397 ++++++++++++++++++++++++++---------------
1 file changed, 249 insertions(+), 148 deletions(-)
@@ -107,6 +107,8 @@ enum index {
ITEM_END,
ITEM_VOID,
ITEM_INVERT,
+ ITEM_ROUGHLY,
+ ITEM_ROUGHLY_THRESHOLD,
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,
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,
+ 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] = {
+ .name = "roughly",
+ .help = "match the pattern roughly",
+ .priv = PRIV_ITEM(ROUGHLY,
+ sizeof(struct rte_flow_item_roughly)),
+ .next = NEXT(item_roughly),
+ .call = parse_vc,
+ },
+ [ITEM_ROUGHLY_THRESHOLD] = {
+ .name = "threshold",
+ .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",
@@ -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)),
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)),
@@ -491,6 +491,7 @@ Usage example, matching non-TCPv4 packets only:
+-------+----------+
| Index | Item |
+
+=======+==========+
| 0 | INVERT |
+-------+----------+
@@ -503,6 +504,55 @@ Usage example, matching non-TCPv4 packets only:
| 4 | END |
+-------+----------+
+Item: ``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.
+
+.. _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 |
+ +----------+-----------+------------------------------------------------------+
+
+
+Usage example, roughly match a TCPv4 packets:
+
+.. _table_rte_flow_item_roughly_example:
+
+.. table:: Roughly matching
+
+ +-------+----------+
+ | Index | Item |
+ +=======+==========+
+ | 0 | ROUGHLY |
+ +-------+----------+
+ | 1 | Ethernet |
+ +-------+----------+
+ | 2 | IPv4 |
+ +-------+----------+
+ | 3 | TCP |
+ +-------+----------+
+ | 4 | END |
+ +-------+----------+
+
Item: ``PF``
^^^^^^^^^^^^
@@ -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.
+
- ``any``: match any protocol for the current layer.
- ``num {unsigned}``: number of layers covered.
@@ -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.
+ *
+ * See struct rte_flow_item_roughly
+ */
+ RTE_FLOW_ITEM_TYPE_ROUGHLY,
+
+ /**
* 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.
+ */
+struct rte_flow_item_roughly {
+ uint32_t threshold; /**< accuracy threshold*/
+};
+
+/**
* RTE_FLOW_ITEM_TYPE_ANY
*
* Matches any protocol in place of the current layer, a single ANY may also