DPDK patches and discussions
 help / color / mirror / Atom feed
From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
To: Qi Zhang <qi.z.zhang@intel.com>
Cc: dev@dpdk.org, declan.doherty@intel.com,
	sugesh.chandran@intel.com, michael.j.glynn@intel.com,
	yu.y.liu@intel.com, konstantin.ananyev@intel.com,
	bruce.richardson@intel.com
Subject: Re: [dpdk-dev] [PATCH v3 2/4] ethdev: add packet field set aciton in flow API
Date: Thu, 19 Apr 2018 16:48:43 +0200	[thread overview]
Message-ID: <20180419144843.GF4957@6wind.com> (raw)
In-Reply-To: <20180416061042.785-3-qi.z.zhang@intel.com>

Typo in commit title: aciton => action

On Mon, Apr 16, 2018 at 02:10:40PM +0800, Qi Zhang wrote:
> Add new action RTE_FLOW_ACTION_TYPE_FIELD_SET, it is used to
> modify fields of specific protocol layer of the packet, the
> action only apply on packets that contain the requireds protocol
> layer.

requireds => required

> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>

(more below)

> ---
>  doc/guides/prog_guide/rte_flow.rst | 30 +++++++++++++++++++++++++++
>  lib/librte_ether/rte_flow.h        | 42 +++++++++++++++++++++++++++++++++++++-
>  2 files changed, 71 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index 99468bf60..68deb9812 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -1574,6 +1574,36 @@ fields in the pattern items.
>     | 1     | END      |
>     +-------+----------+
>  
> +Action: ``FILED_SET``
> +^^^^^^^^^^^^^^^^^^^^^

FILED_SET => FIELD_SET

> +
> +Modify the value of fields in a protocol layer, only applies to packets that
> +contain respective protocol layer.
> +
> +.. _table_rte_flow_action_field_set:
> +
> +.. table:: FIELD_SET
> +
> +   +---------------+-------------------------------------------------------------------+
> +   | Field         | Value                                                             |
> +   +===============+===================================================================+
> +   | ``type``      | Specify the type of a protocol layer. (see RTE_FLOW_ITEM_TYPE_*)  |
> +   +---------------+-------------------------------------------------------------------+
> +   | ``dir_level`` | Specify the level of matched protocol layer.                      |
> +   |               | direction (1b)                                                    |
> +   |               | 0: match start from outermost.                                    |
> +   |               | 1: match start from innermost.                                    |

Please remove the direction part. What devices can match is always outermost
up to the point where they can't recognize an inner header. "innermost" is
almost guaranteed to never have the desired effect.

> +   |               | level: (31b)                                                      |
> +   |               | 0: outermost or innermost protocol layer that matched @type       |
> +   |               | 1: next to outmost or innermost protocol layer that matched @type |
> +   |               | 2: and so on ...                                                  |

Then you can remove any reference to dir_level from here.

> +   +---------------+-------------------------------------------------------------------+
> +   |  ``new_val``  | Pointer to specific data structure according to protocol type,    |
> +   |               | the content is the new value to updtae.                           |

updtae => update

> +   +---------------+-------------------------------------------------------------------+
> +   |  ``mask``     | Bit-mask applied to new_val                                       |
> +   +---------------+-------------------------------------------------------------------+
> +
>  Negative types
>  ~~~~~~~~~~~~~~
>  
> diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
> index f84bbfda5..2dc95b6b8 100644
> --- a/lib/librte_ether/rte_flow.h
> +++ b/lib/librte_ether/rte_flow.h
> @@ -1245,7 +1245,15 @@ enum rte_flow_action_type {
>  	 *
>  	 * See struct rte_flow_action_security.
>  	 */
> -	RTE_FLOW_ACTION_TYPE_SECURITY
> +	RTE_FLOW_ACTION_TYPE_SECURITY,
> +
> +	/**
> +	 * Modify the value of fields in a protocol layer, only applies to
> +	 * packets that contain respective protocol layer.
> +	 *
> +	 * See struct rte_flow_action_field_set.
> +	 */
> +	RTE_FLOW_ACTION_TYPE_FIELD_SET,
>  };
>  
>  /**
> @@ -1384,6 +1392,38 @@ struct rte_flow_action_security {
>  };
>  
>  /**
> + * RTE_FLOW_ACTION_TYPE_FIELD_SET
> + *
> + * Modify the value of fields in a protocol layer, only applies to
> + * packets that contain respective protocol layer.
> + */
> +struct rte_flow_action_field_set {
> +	/**
> +	 * Specify the type of a protocol layer.
> +	 */
> +	enum rte_flow_item_type type;
> +	/**
> +	 * Specify the level of matched protocol layer.
> +	 *
> +	 * direction (1b)
> +	 * 0: match start from outermost.
> +	 * 1: match start from innermost.
> +	 *
> +	 * level (31b)
> +	 * 0: outermost|innermost protocol layer that matched @type.
> +	 * 1: next to outermost|innermost protocol layer that matched @type.
> +	 * 2: and so on ...
> +	 */
> +	uint32_t dir_level;

See above regarding this field.

> +	/**
> +	 * Pointer to specific data structure according to protocol type,
> +	 * the content is the new value to update.
> +	 */
> +	const void *new_val;
> +	const void *mask; /**< Bit-mask applied to new_val. */
> +};
> +
> +/**
>   * Definition of a single action.
>   *
>   * A list of actions is terminated by a END action.
> -- 
> 2.13.6
> 

Testpmd implementation and documentation update are also missing, however
I'm still not convinced by the definition of this new action, it seems too
generic to be useful (e.g. compare this with a dedicated "update destination
IPv4 address" action for instance).

What existing HW capabilities do you intend to expose through this, what
kind of fields can be updated at this point?

If it's still unclear, I suggest to remove this patch from the series or at
the very least mark it as experimental. You can even provide a forward
declaration without the contents of struct rte_flow_action_field_set to
prevent applications from using it before it's finalized.

-- 
Adrien Mazarguil
6WIND

  reply	other threads:[~2018-04-19 14:48 UTC|newest]

Thread overview: 61+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-28 23:29 [dpdk-dev] [PATCH 0/4] rte_flow extension for vSwitch acceleration Qi Zhang
2018-03-28 23:29 ` [dpdk-dev] [PATCH 1/4] ether: add flow action to redirect packet in a switch domain Qi Zhang
2018-03-29 10:48   ` Pattan, Reshma
2018-03-29 11:20   ` Pattan, Reshma
2018-03-28 23:29 ` [dpdk-dev] [PATCH 2/4] ether: add flow last hit query support Qi Zhang
2018-03-28 23:29 ` [dpdk-dev] [PATCH 3/4] ether: add more protocol support in flow API Qi Zhang
2018-03-29 14:32   ` Pattan, Reshma
2018-03-28 23:29 ` [dpdk-dev] [PATCH 4/4] ether: add packet modification aciton " Qi Zhang
2018-03-29 15:23   ` Pattan, Reshma
2018-04-02  3:35     ` Zhang, Qi Z
2018-04-01 21:19 ` [dpdk-dev] [PATCH v2 0/4] rte_flow extension for vSwitch acceleration Qi Zhang
2018-04-01 21:19   ` [dpdk-dev] [PATCH v2 1/4] ether: add flow action to redirect packet to a port Qi Zhang
2018-04-11 16:30     ` Adrien Mazarguil
2018-04-01 21:19   ` [dpdk-dev] [PATCH v2 2/4] ether: add flow last hit query support Qi Zhang
2018-04-11 16:31     ` Adrien Mazarguil
2018-04-01 21:19   ` [dpdk-dev] [PATCH v2 3/4] ether: add more protocol support in flow API Qi Zhang
2018-04-11 16:32     ` Adrien Mazarguil
2018-04-12  5:12       ` Zhang, Qi Z
2018-04-12  9:19         ` Adrien Mazarguil
2018-04-12 10:00           ` Zhang, Qi Z
2018-04-01 21:19   ` [dpdk-dev] [PATCH v2 4/4] ether: add packet modification aciton " Qi Zhang
2018-04-12  7:03     ` Adrien Mazarguil
2018-04-12  8:50       ` Zhang, Qi Z
2018-04-12 10:22         ` Adrien Mazarguil
2018-04-13 13:47           ` Zhang, Qi Z
2018-04-16 13:30             ` Adrien Mazarguil
2018-04-16 15:03               ` Zhang, Qi Z
2018-04-17  9:55                 ` Adrien Mazarguil
2018-04-17 10:32                   ` Zhang, Qi Z
2018-04-10 14:12   ` [dpdk-dev] [PATCH v2 0/4] rte_flow extension for vSwitch acceleration Thomas Monjalon
2018-04-16  5:16 ` [dpdk-dev] [PATCH v3 " Qi Zhang
2018-04-16  5:16   ` [dpdk-dev] [PATCH v3 1/4] ethdev: add more protocol support in flow API Qi Zhang
2018-04-16  5:16   ` [dpdk-dev] [PATCH v3 2/4] ethdev: add packet field set aciton " Qi Zhang
2018-04-16  5:16   ` [dpdk-dev] [PATCH v3 3/4] ethdev: add TTL change actions " Qi Zhang
2018-04-16  5:48     ` Shahaf Shuler
2018-04-16  8:12       ` Adrien Mazarguil
2018-04-16  8:56         ` Shahaf Shuler
2018-04-16  9:59           ` Adrien Mazarguil
2018-04-16  5:16   ` [dpdk-dev] [PATCH v3 4/4] ethdev: add VLAN and MPLS pop push action " Qi Zhang
2018-04-16  6:10 ` [dpdk-dev] [PATCH v3 0/4] rte_flow extension for vSwitch acceleration Qi Zhang
2018-04-16  6:10   ` [dpdk-dev] [PATCH v3 1/4] ethdev: add more protocol support in flow API Qi Zhang
2018-04-19 14:48     ` Adrien Mazarguil
2018-04-16  6:10   ` [dpdk-dev] [PATCH v3 2/4] ethdev: add packet field set aciton " Qi Zhang
2018-04-19 14:48     ` Adrien Mazarguil [this message]
2018-04-20  2:24       ` Zhang, Qi Z
2018-04-20  8:54         ` Adrien Mazarguil
2018-04-16  6:10   ` [dpdk-dev] [PATCH v3 3/4] ethdev: add TTL change actions " Qi Zhang
2018-04-19 14:48     ` Adrien Mazarguil
2018-04-16  6:10   ` [dpdk-dev] [PATCH v3 4/4] ethdev: add VLAN and MPLS pop push action " Qi Zhang
2018-04-17  7:34     ` Shahaf Shuler
2018-04-19 14:49     ` Adrien Mazarguil
2018-04-23  6:36 ` [dpdk-dev] [PATCH v4 0/3] rte_flow extension for vSwitch acceleration Qi Zhang
2018-04-23  6:36   ` [dpdk-dev] [PATCH v4 1/3] ethdev: add more protocol support in flow API Qi Zhang
2018-04-23  6:36   ` [dpdk-dev] [PATCH v4 2/3] ethdev: add TTL change actions " Qi Zhang
2018-04-23  6:36   ` [dpdk-dev] [PATCH v4 3/3] ethdev: add VLAN and MPLS " Qi Zhang
2018-04-24 15:58   ` [dpdk-dev] [PATCH v5 0/3] rte_flow extension for vSwitch acceleration Adrien Mazarguil
2018-04-24 15:58     ` [dpdk-dev] [PATCH v5 1/3] ethdev: add neighbor discovery support to flow API Adrien Mazarguil
2018-04-24 15:59     ` [dpdk-dev] [PATCH v5 2/3] ethdev: add TTL change actions " Adrien Mazarguil
2018-04-24 15:59     ` [dpdk-dev] [PATCH v5 3/3] ethdev: add VLAN and MPLS " Adrien Mazarguil
2018-04-25 13:54     ` [dpdk-dev] [PATCH v5 0/3] rte_flow extension for vSwitch acceleration Zhang, Qi Z
2018-04-25 22:44       ` Ferruh Yigit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180419144843.GF4957@6wind.com \
    --to=adrien.mazarguil@6wind.com \
    --cc=bruce.richardson@intel.com \
    --cc=declan.doherty@intel.com \
    --cc=dev@dpdk.org \
    --cc=konstantin.ananyev@intel.com \
    --cc=michael.j.glynn@intel.com \
    --cc=qi.z.zhang@intel.com \
    --cc=sugesh.chandran@intel.com \
    --cc=yu.y.liu@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).