[v5,1/3] ethdev: add generic TTL rewrite actions

Message ID 20181013032348.26380-2-jackmin@mellanox.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev: add generic TTL rewrite actions |

Checks

Context Check Description
ci/Intel-compilation success Compilation OK

Commit Message

Xiaoyu Min Oct. 13, 2018, 3:24 a.m. UTC
  rewrite TTL by decrease or just set it directly
it's not necessary to check if the final result
is zero or not

This is slightly different from the one defined
by openflow and more generic

Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
Acked-by: Yongseok Koh <yskoh@mellanox.com>
---
 doc/guides/prog_guide/rte_flow.rst | 30 +++++++++++++++++++++++++++++
 lib/librte_ethdev/rte_flow.c       |  2 ++
 lib/librte_ethdev/rte_flow.h       | 31 ++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+)
  

Comments

Andrew Rybchenko Oct. 14, 2018, 7:35 a.m. UTC | #1
On 13.10.2018 06:24, Jack Min wrote:
> rewrite TTL by decrease or just set it directly
> it's not necessary to check if the final result
> is zero or not
>
> This is slightly different from the one defined
> by openflow and more generic
>
> Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
> Acked-by: Yongseok Koh <yskoh@mellanox.com>
> ---

[...]

> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index 68bbf57d0..f102e6939 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -1579,6 +1579,26 @@ enum rte_flow_action_type {
>   	 * No associated configuration structure.
>   	 */
>   	RTE_FLOW_ACTION_TYPE_MAC_SWAP,
> +
> +	/**
> +	 * Decrease TTL value directly
> +	 *
> +	 * If flow pattern doesn't define a valid RTE_FLOW_ITEM_TYPE_IPV4, or
> +	 * RTE_FLOW_ITEM_TYPE_IPV6, the PMD should return a
> +	 * RTE_FLOW_ERROR_TYPE_ACTION error.
> +	 */
> +	RTE_FLOW_ACTION_TYPE_DEC_TTL,
> +
> +	/**
> +	 * Set TTL value
> +	 *
> +	 * If flow pattern doesn't define a valid RTE_FLOW_ITEM_TYPE_IPV4, or
> +	 * RTE_FLOW_ITEM_TYPE_IPV6, the PMD should return a
> +	 * RTE_FLOW_ERROR_TYPE_ACTION error.
> +	 *
> +	 * See struct rte_flow_action_set_ttl
> +	 */
> +	RTE_FLOW_ACTION_TYPE_SET_TTL,
>   };
>   
>   /**

As I understand it is one more case where the following note from Adrien
is applicable [1]:

<begin quote>

Another problem is that you must not require actions to rely on specific
pattern content:

  [...]
  *
  * Decapsulate outer most tunnel from matched flow.
  *
  * The flow pattern must have a valid tunnel header
  */
  RTE_FLOW_ACTION_TYPE_TUNNEL_DECAP,

For maximum flexibility, all actions should be usable on their own on empty
pattern. On the other hand, you can document undefined behavior when
performing some action on traffic that doesn't contain something.

Reason is that invalid traffic may have already been removed by other flow
rules (or whatever happens) before such a rule is reached; it's a user's
responsibility to provide such guarantee.

When parsing an action, a PMD is not supposed to look at the pattern. Action
list should contain all the needed info, otherwise it means the API is badly
defined.

I'm aware the above makes it tough to implement something like
RTE_FLOW_ACTION_TYPE_TUNNEL_DECAP as defined in this series, but that's
unfortunately why I think it must not be defined like that.

<end quote>

[1] https://mails.dpdk.org/archives/dev/2018-October/115267.html
  
Xiaoyu Min Oct. 16, 2018, 2:03 a.m. UTC | #2
On 18-10-14 10:35:30, Andrew Rybchenko wrote:
> On 13.10.2018 06:24, Jack Min wrote:
> > rewrite TTL by decrease or just set it directly
> > it's not necessary to check if the final result
> > is zero or not
> > 
> > This is slightly different from the one defined
> > by openflow and more generic
> > 
> > Signed-off-by: Xiaoyu Min <jackmin@mellanox.com>
> > Acked-by: Yongseok Koh <yskoh@mellanox.com>
> > ---
> 
> [...]
> 
> > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> > index 68bbf57d0..f102e6939 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -1579,6 +1579,26 @@ enum rte_flow_action_type {
> >   	 * No associated configuration structure.
> >   	 */
> >   	RTE_FLOW_ACTION_TYPE_MAC_SWAP,
> > +
> > +	/**
> > +	 * Decrease TTL value directly
> > +	 *
> > +	 * If flow pattern doesn't define a valid RTE_FLOW_ITEM_TYPE_IPV4, or
> > +	 * RTE_FLOW_ITEM_TYPE_IPV6, the PMD should return a
> > +	 * RTE_FLOW_ERROR_TYPE_ACTION error.
> > +	 */
> > +	RTE_FLOW_ACTION_TYPE_DEC_TTL,
> > +
> > +	/**
> > +	 * Set TTL value
> > +	 *
> > +	 * If flow pattern doesn't define a valid RTE_FLOW_ITEM_TYPE_IPV4, or
> > +	 * RTE_FLOW_ITEM_TYPE_IPV6, the PMD should return a
> > +	 * RTE_FLOW_ERROR_TYPE_ACTION error.
> > +	 *
> > +	 * See struct rte_flow_action_set_ttl
> > +	 */
> > +	RTE_FLOW_ACTION_TYPE_SET_TTL,
> >   };
> >   /**
> 
> As I understand it is one more case where the following note from Adrien
> is applicable [1]:
> 
> <begin quote>
> 
> Another problem is that you must not require actions to rely on specific
> pattern content:
> 
>  [...]
>  *
>  * Decapsulate outer most tunnel from matched flow.
>  *
>  * The flow pattern must have a valid tunnel header
>  */
>  RTE_FLOW_ACTION_TYPE_TUNNEL_DECAP,
> 
> For maximum flexibility, all actions should be usable on their own on empty
> pattern. On the other hand, you can document undefined behavior when
> performing some action on traffic that doesn't contain something.
> 
> Reason is that invalid traffic may have already been removed by other flow
> rules (or whatever happens) before such a rule is reached; it's a user's
> responsibility to provide such guarantee.
> 
> When parsing an action, a PMD is not supposed to look at the pattern. Action
> list should contain all the needed info, otherwise it means the API is badly
> defined.
> 
> I'm aware the above makes it tough to implement something like
> RTE_FLOW_ACTION_TYPE_TUNNEL_DECAP as defined in this series, but that's
> unfortunately why I think it must not be defined like that.
> 
> <end quote>
> 
> [1] https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmails.dpdk.org%2Farchives%2Fdev%2F2018-October%2F115267.html&amp;data=02%7C01%7Cjackmin%40mellanox.com%7C8297d07c910e4aa6b4f508d631a7ab8e%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636750993538615097&amp;sdata=q0ywvMqqtRr%2BHp%2FBHLGtMF3x84kwPooZ7eYfdxmHtUs%3D&amp;reserved=0
> 
Alright, I'will document this as PMD's limitation not API's.

-Jack
  

Patch

diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index a5ec441c9..12f3eb794 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -2203,6 +2203,36 @@  Otherwise, RTE_FLOW_ERROR_TYPE_ACTION error will be returned.
    | no properties |
    +---------------+
 
+Action: ``DEC_TTL``
+^^^^^^^^^^^^^^^^^^^
+
+Decrease TTL value.
+
+.. _table_rte_flow_action_dec_ttl:
+
+.. table:: DEC_TTL
+
+   +---------------+
+   | Field         |
+   +===============+
+   | no properties |
+   +---------------+
+
+Action: ``SET_TTL``
+^^^^^^^^^^^^^^^^^^^
+
+Assigns a new TTL value.
+
+.. _table_rte_flow_action_set_ttl:
+
+.. table:: SET_TTL
+
+   +---------------+--------------------+
+   | Field         | Value              |
+   +===============+====================+
+   | ``ttl_value`` | new TTL value      |
+   +---------------+--------------------+
+
 Negative types
 ~~~~~~~~~~~~~~
 
diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
index bc9e719dc..5040c7667 100644
--- a/lib/librte_ethdev/rte_flow.c
+++ b/lib/librte_ethdev/rte_flow.c
@@ -136,6 +136,8 @@  static const struct rte_flow_desc_data rte_flow_desc_action[] = {
 	MK_FLOW_ACTION(SET_TP_DST,
 		       sizeof(struct rte_flow_action_set_tp)),
 	MK_FLOW_ACTION(MAC_SWAP, 0),
+	MK_FLOW_ACTION(DEC_TTL, 0),
+	MK_FLOW_ACTION(SET_TTL, sizeof(struct rte_flow_action_set_ttl)),
 };
 
 static int
diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
index 68bbf57d0..f102e6939 100644
--- a/lib/librte_ethdev/rte_flow.h
+++ b/lib/librte_ethdev/rte_flow.h
@@ -1579,6 +1579,26 @@  enum rte_flow_action_type {
 	 * No associated configuration structure.
 	 */
 	RTE_FLOW_ACTION_TYPE_MAC_SWAP,
+
+	/**
+	 * Decrease TTL value directly
+	 *
+	 * If flow pattern doesn't define a valid RTE_FLOW_ITEM_TYPE_IPV4, or
+	 * RTE_FLOW_ITEM_TYPE_IPV6, the PMD should return a
+	 * RTE_FLOW_ERROR_TYPE_ACTION error.
+	 */
+	RTE_FLOW_ACTION_TYPE_DEC_TTL,
+
+	/**
+	 * Set TTL value
+	 *
+	 * If flow pattern doesn't define a valid RTE_FLOW_ITEM_TYPE_IPV4, or
+	 * RTE_FLOW_ITEM_TYPE_IPV6, the PMD should return a
+	 * RTE_FLOW_ERROR_TYPE_ACTION error.
+	 *
+	 * See struct rte_flow_action_set_ttl
+	 */
+	RTE_FLOW_ACTION_TYPE_SET_TTL,
 };
 
 /**
@@ -1987,6 +2007,17 @@  struct rte_flow_action_set_tp {
 	rte_be16_t port;
 };
 
+/**
+ * RTE_FLOW_ACTION_TYPE_SET_TTL
+ *
+ * Set the TTL value directly for IPv4 or IPv6
+ * The RTE_FLOW_ITEM_TYPE_IPV4 or RTE_FLOW_ITEM_TYPE_IPV6
+ * must be present in pattern
+ */
+struct rte_flow_action_set_ttl {
+	uint8_t ttl_value;
+};
+
 /*
  * Definition of a single action.
  *