[dpdk-dev] [PATCH 0/1] proposed minor change in rte_flow_validate semantics

[dpdk-dev] [PATCH 0/1] proposed minor change in rte_flow_validate semantics

[John Daley]

Hi,

In implementing rte_flow_validate() for the Cisco enic, I got to wondering
if the semantics might be slightly off given how I see apps using it.

Please forgive me if this has already been discussed, but during runtime, I
can't see any reason why rte_flow_validate() would be called when
rte_flow_create() returns the same errors as validate, but also creates
the filter if all is good (calling validate first is just extra overhead).
What are the use cases for calling validate when the app is up and running?

I see rte_flow_validate() being run at startup for answering these 2 questions:
1.) Given enough resources, is the device capable of handling flows
a, b, c, ..,  which the app may want to create during runtime?
2.) How many concurrent flows of type a, b, c, .. can the device handle? To
answer this app needs to do a bunch of rte_flow_create()'s (followed by a bunch of rte_flow_destroy()s).

The answer to these questions will allow the app to decide beforehand which
filters to offload (if any) and set up function pointers or whatever to run
as efficiently as possible on the target device.

rte_flow_validate() has comments that imply it should be run on the fly,like
"the flow rule is validated against its current configuration state and the
returned value should be considered valid by the caller for that state only".
The only real way to implement this is for enic (and I think other nics) is
to actually do a flow create/destroy within rte_flow_validate(). But, see
paragraph 2 above- why bother? I looked at the i40e and mlx5 implementations
and I didn't see any calls to the nics to check a flow against current state
so they might not be implemented as the comment imply they should be either.

If there are no use cases for calling validate at runtime,  I think we ought
to change the semantics of rte_flow_validate() a little to define success to
mean that the device will accept the flow IF there are enough resources,
rather than it will accept the flow given the current state. We can safely
remove the -EEXIST return code since no other drivers set them yet. This
makes the function easier to implement correctly without trying to do
something that's not useful anyway.

The following patch just changes comments but I think that is all that is
required.

Thanks,
John

John Daley (1):
  ethdev: don't consider device state when validating flows

 lib/librte_ether/rte_flow.h | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

-- 
2.12.0

[dpdk-dev] [PATCH 1/1] ethdev: don't consider device state when validating flows

[John Daley]

PMDs only consider if a flow would be accepted or not the assuming the
device had all it's resources available to it. Since state is not
considered, -EEXIST and -EBUSY return codes no longer make sense and
are removed. Also clarify the -ENOMEM has nothig to do with device
resouces, only host resources needed rte_flow_validate().

Signed-off-by: John Daley <johndale@cisco.com>
---
 lib/librte_ether/rte_flow.h | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
index 171a5698e..16846449d 100644
--- a/lib/librte_ether/rte_flow.h
+++ b/lib/librte_ether/rte_flow.h
@@ -932,15 +932,10 @@ struct rte_flow_error {
 /**
  * Check whether a flow rule can be created on a given port.
  *
- * While this function has no effect on the target device, the flow rule is
- * validated against its current configuration state and the returned value
- * should be considered valid by the caller for that state only.
- *
- * The returned value is guaranteed to remain valid only as long as no
- * successful calls to rte_flow_create() or rte_flow_destroy() are made in
- * the meantime and no device parameter affecting flow rules in any way are
- * modified, due to possible collisions or resource limitations (although in
- * such cases EINVAL should not be returned).
+ * The flow rule is validated against the target device. There is no check
+ * against the current state of the device- creating the flow could still
+ * fail due to a lack of resources on the device. This function has no effect
+ * on the target device.
  *
  * @param port_id
  *   Port identifier of Ethernet device.
@@ -965,13 +960,7 @@ struct rte_flow_error {
  *   -ENOTSUP: valid but unsupported rule specification (e.g. partial
  *   bit-masks are unsupported).
  *
- *   -EEXIST: collision with an existing rule.
- *
- *   -ENOMEM: not enough resources.
- *
- *   -EBUSY: action cannot be performed due to busy device resources, may
- *   succeed if the affected queues or even the entire port are in a stopped
- *   state (see rte_eth_dev_rx_queue_stop() and rte_eth_dev_stop()).
+ *   -ENOMEM: not enough host resources to execute this funtion.
  */
 int
 rte_flow_validate(uint8_t port_id,
-- 
2.12.0

Re: [dpdk-dev] [PATCH 1/1] ethdev: don't consider device state when validating flows

[Thomas Monjalon]

Ping,
any progress on this patch?


2017-03-23 19:36, John Daley:
> PMDs only consider if a flow would be accepted or not the assuming the
> device had all it's resources available to it. Since state is not
> considered, -EEXIST and -EBUSY return codes no longer make sense and
> are removed. Also clarify the -ENOMEM has nothig to do with device
> resouces, only host resources needed rte_flow_validate().
> 
> Signed-off-by: John Daley <johndale@cisco.com>

[dpdk-dev] [PATCH v2 0/1] fix flow validate comments

[John Daley]

Adrien,

Here is another crack at the comments for rte_flow_validate. Does this
capture what you were explaining to me?

I'm still not crazy about multiple meanings for EEXIST or ENOMEM since
it makes them unusable by apps, but at least the comments try to explain it. In 17.08 what about having PMDs indicate if they support flow
collision and resouce checking, or drop those return codes all together?

cheers,
john

John Daley (1):
  ethdev: fix flow validate comments

 lib/librte_ether/rte_flow.h | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

-- 
2.12.0

[dpdk-dev] [PATCH v2 1/1] ethdev: fix flow validate comments

[John Daley]

Change comments for rte_flow_validate() function to indicate that flow
rule collision and resource validation is optional for PMD and therefore
the return codes may have different meanings.

Fixes: b1a4b4cbc0a8 ("ethdev: introduce generic flow API")

Signed-off-by: John Daley <johndale@cisco.com>
---
 lib/librte_ether/rte_flow.h | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
index 8013ecab2..bbc5ec2e3 100644
--- a/lib/librte_ether/rte_flow.h
+++ b/lib/librte_ether/rte_flow.h
@@ -983,9 +983,10 @@ struct rte_flow_error {
 /**
  * Check whether a flow rule can be created on a given port.
  *
- * While this function has no effect on the target device, the flow rule is
- * validated against its current configuration state and the returned value
- * should be considered valid by the caller for that state only.
+ * The flow rule is validated for correctness and whether it could be accepted
+ * by the device given sufficient resources. The rule is checked against the
+ * current device mode and queue configuration. The flow rule may optionally
+ * be validated against existing flow rules and resources.
  *
  * The returned value is guaranteed to remain valid only as long as no
  * successful calls to rte_flow_create() or rte_flow_destroy() are made in
@@ -1016,9 +1017,15 @@ struct rte_flow_error {
  *   -ENOTSUP: valid but unsupported rule specification (e.g. partial
  *   bit-masks are unsupported).
  *
- *   -EEXIST: collision with an existing rule.
+ *   -EEXIST: collision with an existing rule. Only returned if device
+ *   supports flow rule collision checking and there was a flow rule
+ *   collision. Not receiving this return code is no gaurantee that creating
+ *   the rule will not fail due to a collision.
+ *
+ *   -ENOMEM: If the device supports resource checking: device resouce
+ *   limitation. If not supported: not enough resouces to execute the validate
+ *   command.
  *
- *   -ENOMEM: not enough resources.
  *
  *   -EBUSY: action cannot be performed due to busy device resources, may
  *   succeed if the affected queues or even the entire port are in a stopped
-- 
2.12.0

[dpdk-dev] [PATCH v3] ethdev: fix flow validate comments

[John Daley]

Change comments for rte_flow_validate() function to indicate that flow
rule collision and resource validation is optional for PMD and therefore
the return codes may have different meanings.

Fixes: b1a4b4cbc0a8 ("ethdev: introduce generic flow API")

Signed-off-by: John Daley <johndale@cisco.com>
---
v2: another crack at the comments
v3: fix typos, rewording, put back a sentence omitted in v2

 lib/librte_ether/rte_flow.h | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
index 8013ecab2..85ce4ec90 100644
--- a/lib/librte_ether/rte_flow.h
+++ b/lib/librte_ether/rte_flow.h
@@ -983,9 +983,11 @@ struct rte_flow_error {
 /**
  * Check whether a flow rule can be created on a given port.
  *
- * While this function has no effect on the target device, the flow rule is
- * validated against its current configuration state and the returned value
- * should be considered valid by the caller for that state only.
+ * The flow rule is validated for correctness and whether it could be accepted
+ * by the device given sufficient resources. The rule is checked against the
+ * current device mode and queue configuration. The flow rule may also
+ * optionally be validated against existing flow rules and device resources.
+ * This function has no effect on the target device.
  *
  * The returned value is guaranteed to remain valid only as long as no
  * successful calls to rte_flow_create() or rte_flow_destroy() are made in
@@ -1016,9 +1018,13 @@ struct rte_flow_error {
  *   -ENOTSUP: valid but unsupported rule specification (e.g. partial
  *   bit-masks are unsupported).
  *
- *   -EEXIST: collision with an existing rule.
+ *   -EEXIST: collision with an existing rule. Only returned if device
+ *   supports flow rule collision checking and there was a flow rule
+ *   collision. Not receiving this return code is no guarantee that creating
+ *   the rule will not fail due to a collision.
  *
- *   -ENOMEM: not enough resources.
+ *   -ENOMEM: Not enough memory to execute the function, or if the device
+ *   supports resource validation, resource limitation on the device.
  *
  *   -EBUSY: action cannot be performed due to busy device resources, may
  *   succeed if the affected queues or even the entire port are in a stopped
-- 
2.12.0

Re: [dpdk-dev] [PATCH v3] ethdev: fix flow validate comments

[Adrien Mazarguil]

Hi John,

On Thu, Apr 06, 2017 at 05:23:00PM -0700, John Daley wrote:
> Change comments for rte_flow_validate() function to indicate that flow
> rule collision and resource validation is optional for PMD and therefore
> the return codes may have different meanings.
> 
> Fixes: b1a4b4cbc0a8 ("ethdev: introduce generic flow API")
> 
> Signed-off-by: John Daley <johndale@cisco.com>
> ---
> v2: another crack at the comments
> v3: fix typos, rewording, put back a sentence omitted in v2

This version is fine and it clarifies the original intent, it's only missing
associated changes in doc/guides/prog_guide/rte_flow.rst (look for
rte_flow_validate).

Also the commit title could start with "doc:" as there is no API change.

I also have one minor nit, see below.

>  lib/librte_ether/rte_flow.h | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
> index 8013ecab2..85ce4ec90 100644
> --- a/lib/librte_ether/rte_flow.h
> +++ b/lib/librte_ether/rte_flow.h
> @@ -983,9 +983,11 @@ struct rte_flow_error {
>  /**
>   * Check whether a flow rule can be created on a given port.
>   *
> - * While this function has no effect on the target device, the flow rule is
> - * validated against its current configuration state and the returned value
> - * should be considered valid by the caller for that state only.
> + * The flow rule is validated for correctness and whether it could be accepted
> + * by the device given sufficient resources. The rule is checked against the
> + * current device mode and queue configuration. The flow rule may also
> + * optionally be validated against existing flow rules and device resources.
> + * This function has no effect on the target device.
>   *
>   * The returned value is guaranteed to remain valid only as long as no
>   * successful calls to rte_flow_create() or rte_flow_destroy() are made in
> @@ -1016,9 +1018,13 @@ struct rte_flow_error {
>   *   -ENOTSUP: valid but unsupported rule specification (e.g. partial
>   *   bit-masks are unsupported).
>   *
> - *   -EEXIST: collision with an existing rule.
> + *   -EEXIST: collision with an existing rule. Only returned if device
> + *   supports flow rule collision checking and there was a flow rule
> + *   collision. Not receiving this return code is no guarantee that creating
> + *   the rule will not fail due to a collision.
>   *
> - *   -ENOMEM: not enough resources.
> + *   -ENOMEM: Not enough memory to execute the function, or if the device

"Not" should be lowercase (why, yes, that's all).

> + *   supports resource validation, resource limitation on the device.
>   *
>   *   -EBUSY: action cannot be performed due to busy device resources, may
>   *   succeed if the affected queues or even the entire port are in a stopped
> -- 
> 2.12.0
> 

-- 
Adrien Mazarguil
6WIND

[dpdk-dev] [PATCH v4] doc: fix flow validate comments

[John Daley]

Change comments for rte_flow_validate() function to indicate that flow
rule collision and resource validation is optional for PMDs and
therefore the return codes may have different meanings.

Fixes: b1a4b4cbc0a8 ("ethdev: introduce generic flow API")

Signed-off-by: John Daley <johndale@cisco.com>
---
v2: another crack at the comments
v3: fix typos, rewording, put back a sentence omitted in v2
v4: fixes per Adrien Mazarguil- update guide, typo, commit title

 doc/guides/prog_guide/rte_flow.rst | 17 ++++++++++++-----
 lib/librte_ether/rte_flow.h        | 16 +++++++++++-----
 2 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index 5ee045ee8..9f3d3b096 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -1332,9 +1332,11 @@ supported and can be created.
                      const struct rte_flow_action actions[],
                      struct rte_flow_error *error);
 
-While this function has no effect on the target device, the flow rule is
-validated against its current configuration state and the returned value
-should be considered valid by the caller for that state only.
+The flow rule is validated for correctness and whether it could be accepted
+by the device given sufficient resources. The rule is checked against the
+current device mode and queue configuration. The flow rule may also
+optionally be validated against existing flow rules and device resources.
+This function has no effect on the target device.
 
 The returned value is guaranteed to remain valid only as long as no
 successful calls to ``rte_flow_create()`` or ``rte_flow_destroy()`` are made
@@ -1360,8 +1362,13 @@ Return values:
 - ``-EINVAL``: unknown or invalid rule specification.
 - ``-ENOTSUP``: valid but unsupported rule specification (e.g. partial
   bit-masks are unsupported).
-- ``-EEXIST``: collision with an existing rule.
-- ``-ENOMEM``: not enough resources.
+- ``EEXIST``: collision with an existing rule. Only returned if device
+  supports flow rule collision checking and there was a flow rule
+  collision. Not receiving this return code is no guarantee that creating
+  the rule will not fail due to a collision.
+- ``ENOMEM``: not enough memory to execute the function, or if the device
+  supports resource validation, resource limitation on the device.
+
 - ``-EBUSY``: action cannot be performed due to busy device resources, may
   succeed if the affected queues or even the entire port are in a stopped
   state (see ``rte_eth_dev_rx_queue_stop()`` and ``rte_eth_dev_stop()``).
diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
index 8013ecab2..7749491cb 100644
--- a/lib/librte_ether/rte_flow.h
+++ b/lib/librte_ether/rte_flow.h
@@ -983,9 +983,11 @@ struct rte_flow_error {
 /**
  * Check whether a flow rule can be created on a given port.
  *
- * While this function has no effect on the target device, the flow rule is
- * validated against its current configuration state and the returned value
- * should be considered valid by the caller for that state only.
+ * The flow rule is validated for correctness and whether it could be accepted
+ * by the device given sufficient resources. The rule is checked against the
+ * current device mode and queue configuration. The flow rule may also
+ * optionally be validated against existing flow rules and device resources.
+ * This function has no effect on the target device.
  *
  * The returned value is guaranteed to remain valid only as long as no
  * successful calls to rte_flow_create() or rte_flow_destroy() are made in
@@ -1016,9 +1018,13 @@ struct rte_flow_error {
  *   -ENOTSUP: valid but unsupported rule specification (e.g. partial
  *   bit-masks are unsupported).
  *
- *   -EEXIST: collision with an existing rule.
+ *   -EEXIST: collision with an existing rule. Only returned if device
+ *   supports flow rule collision checking and there was a flow rule
+ *   collision. Not receiving this return code is no guarantee that creating
+ *   the rule will not fail due to a collision.
  *
- *   -ENOMEM: not enough resources.
+ *   -ENOMEM: not enough memory to execute the function, or if the device
+ *   supports resource validation, resource limitation on the device.
  *
  *   -EBUSY: action cannot be performed due to busy device resources, may
  *   succeed if the affected queues or even the entire port are in a stopped
-- 
2.12.0

Re: [dpdk-dev] [PATCH v4] doc: fix flow validate comments

[Adrien Mazarguil]

On Thu, Apr 20, 2017 at 11:49:33AM -0700, John Daley wrote:
> Change comments for rte_flow_validate() function to indicate that flow
> rule collision and resource validation is optional for PMDs and
> therefore the return codes may have different meanings.
> 
> Fixes: b1a4b4cbc0a8 ("ethdev: introduce generic flow API")
> 
> Signed-off-by: John Daley <johndale@cisco.com>

One last nit below (not sure if you need to send a new version). In any
case:

Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

[...]
> @@ -1360,8 +1362,13 @@ Return values:
>  - ``-EINVAL``: unknown or invalid rule specification.
>  - ``-ENOTSUP``: valid but unsupported rule specification (e.g. partial
>    bit-masks are unsupported).
> -- ``-EEXIST``: collision with an existing rule.
> -- ``-ENOMEM``: not enough resources.
> +- ``EEXIST``: collision with an existing rule. Only returned if device
> +  supports flow rule collision checking and there was a flow rule
> +  collision. Not receiving this return code is no guarantee that creating
> +  the rule will not fail due to a collision.
> +- ``ENOMEM``: not enough memory to execute the function, or if the device
> +  supports resource validation, resource limitation on the device.
> +

This new empty line should be removed.

>  - ``-EBUSY``: action cannot be performed due to busy device resources, may
>    succeed if the affected queues or even the entire port are in a stopped
>    state (see ``rte_eth_dev_rx_queue_stop()`` and ``rte_eth_dev_stop()``).
[...]

Thanks.

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v4] doc: fix flow validate comments

[Thomas Monjalon]

21/04/2017 10:11, Adrien Mazarguil:
> On Thu, Apr 20, 2017 at 11:49:33AM -0700, John Daley wrote:
> > Change comments for rte_flow_validate() function to indicate that flow
> > rule collision and resource validation is optional for PMDs and
> > therefore the return codes may have different meanings.
> > 
> > Fixes: b1a4b4cbc0a8 ("ethdev: introduce generic flow API")
> > 
> > Signed-off-by: John Daley <johndale@cisco.com>
> 
> One last nit below (not sure if you need to send a new version). In any
> case:
> 
> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

Applied (with empty line removed), thanks

Re: [dpdk-dev] [PATCH 0/1] proposed minor change in rte_flow_validate semantics

[Adrien Mazarguil]

Hi John,

On Thu, Mar 23, 2017 at 07:36:58PM -0700, John Daley wrote:
> Hi,
> 
> In implementing rte_flow_validate() for the Cisco enic, I got to wondering
> if the semantics might be slightly off given how I see apps using it.
> 
> Please forgive me if this has already been discussed, but during runtime, I
> can't see any reason why rte_flow_validate() would be called when
> rte_flow_create() returns the same errors as validate, but also creates
> the filter if all is good (calling validate first is just extra overhead).
> What are the use cases for calling validate when the app is up and running?

In short (I'll elaborate below), to get a rough idea of the capabilities of
a given port at initialization time. One should use rte_flow_validate() with
nonspecific rules to determine if the PMD understands them at all *given the
circumstances* (the current configuration state of the device).

Nothing prevents one from using it just before calling rte_flow_create(),
however doing so is indeed redundant.

> I see rte_flow_validate() being run at startup for answering these 2 questions:
> 1.) Given enough resources, is the device capable of handling flows
> a, b, c, ..,  which the app may want to create during runtime?

Yes, breaking this down means all the following:

1. Recognize all pattern items and actions.
2. Make sure items are properly stacked and actions can be combined.
3. Check for pattern item specification structures (e.g. limits, masks,
   ranges, and so on).
4. Check for actions configuration structures (e.g. target queue exists).

Depending on the flow rule, 3. and 4. may have to check the current device
configuration in order to validate the rule.

> 2.) How many concurrent flows of type a, b, c, .. can the device handle? To
> answer this app needs to do a bunch of rte_flow_create()'s (followed by a bunch of rte_flow_destroy()s).

Hehe, this is the brute force approach, actually the rte_flow API in its
current form is not supposed to answer this question, and I don't think we
should document it that way (note you could use rte_flow_flush() to destroy
all flows at once faster).

Some devices (like mlx5) do not really have an upper limit on the number of
possible flows one can configure, they're basically limited by the available
memory on the host system.

> The answer to these questions will allow the app to decide beforehand which
> filters to offload (if any) and set up function pointers or whatever to run
> as efficiently as possible on the target device.
> 
> rte_flow_validate() has comments that imply it should be run on the fly,like
> "the flow rule is validated against its current configuration state and the
> returned value should be considered valid by the caller for that state only".
> The only real way to implement this is for enic (and I think other nics) is
> to actually do a flow create/destroy within rte_flow_validate(). But, see
> paragraph 2 above- why bother? I looked at the i40e and mlx5 implementations
> and I didn't see any calls to the nics to check a flow against current state
> so they might not be implemented as the comment imply they should be either.

Then you're probably right there is an issue with the documentation, it's
not the intent (more below).

> If there are no use cases for calling validate at runtime,  I think we ought
> to change the semantics of rte_flow_validate() a little to define success to
> mean that the device will accept the flow IF there are enough resources,
> rather than it will accept the flow given the current state. We can safely
> remove the -EEXIST return code since no other drivers set them yet. This
> makes the function easier to implement correctly without trying to do
> something that's not useful anyway.
> 
> The following patch just changes comments but I think that is all that is
> required.

rte_flow_validate() and rte_flow_create() share the same error codes to make
their implementation simpler. rte_flow_validate() basically exposes the
validation part necessarily present in rte_flow_create().

EEXIST and other error codes *may* be returned by rte_flow_validate(), but
this does not force PMDs to check possible collisions during the validation
step, I'll take documentation is not clear enough regarding this.

So to be completely clear, rte_flow_validate() already does *not* guarantee
a rule can be created afterward, only that rte_flow_create() will
understand that rule.

The other reason "configuration state" is mentioned in the documentation
(besides target queues should exist) is that existing rules may have
switched the device in a mode that does not allow different rule types.

If after initialization, matching TCP, UDP and ICMP is possible, creating a
UDP rule might subsequently prevent the creation of otherwise valid TCP and
ICMP rules. rte_flow_validate() should (but is not forced to) check for
that.

What do you think about keeping the defined error codes as is and merging
somehow my above statements in the documentation instead?

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH 0/1] proposed minor change in rte_flow_validate semantics

[John Daley (johndale)]

Hi Adrien,

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Friday, March 24, 2017 2:47 AM
> To: John Daley (johndale) <johndale@cisco.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH 0/1] proposed minor change in rte_flow_validate
> semantics
> 
> Hi John,
> 
> On Thu, Mar 23, 2017 at 07:36:58PM -0700, John Daley wrote:
> > Hi,
> >
> > In implementing rte_flow_validate() for the Cisco enic, I got to
> > wondering if the semantics might be slightly off given how I see apps using
> it.
> >
> > Please forgive me if this has already been discussed, but during
> > runtime, I can't see any reason why rte_flow_validate() would be
> > called when
> > rte_flow_create() returns the same errors as validate, but also
> > creates the filter if all is good (calling validate first is just extra overhead).
> > What are the use cases for calling validate when the app is up and running?
> 
> In short (I'll elaborate below), to get a rough idea of the capabilities of a given
> port at initialization time. One should use rte_flow_validate() with
> nonspecific rules to determine if the PMD understands them at all *given the
> circumstances* (the current configuration state of the device).
> 
> Nothing prevents one from using it just before calling rte_flow_create(),
> however doing so is indeed redundant.
> 
> > I see rte_flow_validate() being run at startup for answering these 2
> questions:
> > 1.) Given enough resources, is the device capable of handling flows a,
> > b, c, ..,  which the app may want to create during runtime?
> 
> Yes, breaking this down means all the following:
> 
> 1. Recognize all pattern items and actions.
> 2. Make sure items are properly stacked and actions can be combined.
> 3. Check for pattern item specification structures (e.g. limits, masks,
>    ranges, and so on).
> 4. Check for actions configuration structures (e.g. target queue exists).
> 
> Depending on the flow rule, 3. and 4. may have to check the current device
> configuration in order to validate the rule.

Agreed. You didn't include checking for duplicate patterns with conflicting actions. Is that a requirement also? I hope some of that and some of 1, 2 and 3 can be pushed up into rte_ether/flow someday. I.e. only present flows to the PMDs which are already valid, so the PMD just needs to see if it is acceptable to the device.

> 
> > 2.) How many concurrent flows of type a, b, c, .. can the device
> > handle? To answer this app needs to do a bunch of rte_flow_create()'s
> (followed by a bunch of rte_flow_destroy()s).
> 
> Hehe, this is the brute force approach, actually the rte_flow API in its current
> form is not supposed to answer this question, and I don't think we should
> document it that way (note you could use rte_flow_flush() to destroy all
> flows at once faster).
> 
> Some devices (like mlx5) do not really have an upper limit on the number of
> possible flows one can configure, they're basically limited by the available
> memory on the host system.
> 
> > The answer to these questions will allow the app to decide beforehand
> > which filters to offload (if any) and set up function pointers or
> > whatever to run as efficiently as possible on the target device.
> >
> > rte_flow_validate() has comments that imply it should be run on the
> > fly,like "the flow rule is validated against its current configuration
> > state and the returned value should be considered valid by the caller for
> that state only".
> > The only real way to implement this is for enic (and I think other
> > nics) is to actually do a flow create/destroy within
> > rte_flow_validate(). But, see paragraph 2 above- why bother? I looked
> > at the i40e and mlx5 implementations and I didn't see any calls to the
> > nics to check a flow against current state so they might not be
> implemented as the comment imply they should be either.
> 
> Then you're probably right there is an issue with the documentation, it's not
> the intent (more below).
> 
> > If there are no use cases for calling validate at runtime,  I think we
> > ought to change the semantics of rte_flow_validate() a little to
> > define success to mean that the device will accept the flow IF there
> > are enough resources, rather than it will accept the flow given the
> > current state. We can safely remove the -EEXIST return code since no
> > other drivers set them yet. This makes the function easier to
> > implement correctly without trying to do something that's not useful
> anyway.
> >
> > The following patch just changes comments but I think that is all that
> > is required.
> 
> rte_flow_validate() and rte_flow_create() share the same error codes to
> make their implementation simpler. rte_flow_validate() basically exposes
> the validation part necessarily present in rte_flow_create().
> 
> EEXIST and other error codes *may* be returned by rte_flow_validate(), but
> this does not force PMDs to check possible collisions during the validation
> step, I'll take documentation is not clear enough regarding this.

My scan of the other PMDs didn't turn up any EEXIXT or EBUSY return codes from rte_flow_validate(). I suppose future implementation could, but I doubt they will since intel/mlx PMDs set the standard and they do not.

> 
> So to be completely clear, rte_flow_validate() already does *not* guarantee
> a rule can be created afterward, only that rte_flow_create() will understand
> that rule.
> 
> The other reason "configuration state" is mentioned in the documentation
> (besides target queues should exist) is that existing rules may have switched
> the device in a mode that does not allow different rule types.
> 
> If after initialization, matching TCP, UDP and ICMP is possible, creating a UDP
> rule might subsequently prevent the creation of otherwise valid TCP and
> ICMP rules. rte_flow_validate() should (but is not forced to) check for that.
> 
> What do you think about keeping the defined error codes as is and merging
> somehow my above statements in the documentation instead?

I guess I still don't see much value of checking a rule against the current state and no other PMDs are doing this. I still think just having a rte_flow_validate that answers my question 1 above has a lot of value and implying it does more (and having different PMDs do different things) will just confuse users (not to mention PMD maintainers ;). Maybe just cleaning up the documentations as you say is good enough. I think removing return error codes that can't be counted on is even better though. Otherwise the documentation gets harder, right? We would have to basically say "don't count on ENOMEM and EXIST doing what you think because they probably won't...." . My 2 cents, thanks!

> 
> --
> Adrien Mazarguil
> 6WIND