DPDK patches and discussions
 help / color / mirror / Atom feed
From: Thomas Monjalon <thomas@monjalon.net>
To: Alexander Kozyrev <akozyrev@nvidia.com>
Cc: dev@dpdk.org, viacheslavo@nvidia.com, orika@nvidia.com,
	ferruh.yigit@intel.com, andrew.rybchenko@oktetlabs.ru,
	jerinjacobk@gmail.com
Subject: Re: [dpdk-dev] [PATCH v7 1/2] ethdev: introduce generic modify rte flow action
Date: Mon, 18 Jan 2021 00:15:35 +0100
Message-ID: <4720582.E7ApZDAgU3@thomas> (raw)
In-Reply-To: <20210116045054.14539-2-akozyrev@nvidia.com>

16/01/2021 05:50, Alexander Kozyrev:
> Implement the generic modify flow API to allow manipulations on
> an arbitrary header field (as well as mark, metadata or tag) using
> data from another field or a user-specified value.
> This generic modify mechanism removes the necessity to implement
> a separate RTE Flow action every time we need to modify a new packet
> field in the future.
> 
> Supported operation are:
> - set: copy data from source to destination.
> - add: integer addition, stores the result in destination.
> - sub: integer subtraction, stores the result in destination.
> 
> The field ID is used to specify the desired source/destination packet
> field in order to simplify the API for various encapsulation models.
> Specifying the packet field ID with the needed encapsulation level
> is able to quickly get a packet field for any inner packet header.
> 
> Alternatively, the special ID (ITEM_START) can be used to point to
> the very beginning of a packet. This ID in conjunction with the
> offset parameter provides great flexibility to copy/modify any part of
> a packet as needed.
> 
> The number of bits to use from a source as well as the offset can be
> be specified to allow a partial copy or dividing a big packet field
> into multiple small fields (e.g. copying 128 bits of IPv6 to 4 tags).
> 
> An immediate value (or pointer to it) can be specified instead of the
> level and the offset for the special FIELD_VALUE ID (or FIELD_POINTER).
> Can be used as a source only.
> 
> RFC: http://patches.dpdk.org/patch/85384/

You don't need to refer to the RFC in the commit message.

> Signed-off-by: Alexander Kozyrev <akozyrev@nvidia.com>
> Acked-by: Ori Kam <orika@nvidia.com>
> 
> ---
> +Action: ``MODIFY_FIELD``
> +^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Modify ``dst`` field according to ``op`` selected (move, addition,
> +subtraction) with ``width`` bits of data from ``src`` field.

"move" is changed to "set", right?

> +Any arbitrary header field (as well as mark, metadata or tag values)
> +can be used as both source and destination fields as set by ``field``.
> +The immediate value ``RTE_FLOW_FIELD_VALUE`` (or a pointer to it
> +``RTE_FLOW_FIELD_POINTER``) is allowed as a source only.
> +``RTE_FLOW_FIELD_START`` is used to point to the beginning of a packet.
> +
> +``op`` selects the operation to perform on a destination field.
> +- ``set`` copies the data from ``src`` field to ``dst`` field.
> +- ``add`` adds together ``dst`` and ``src`` and stores the result into ``dst``.
> +- ``sub`` subtracts ``src`` from ``dst`` and stores the result into ``dst``
> +
> +``width`` defines a number of bits to use from ``src`` field.
> +
> +``level`` is used to access any packet field on any encapsulation level
> +as well as any tag element in the tag array.
> +- ``0`` means the default behaviour. Depending on the packet type, it can
> +mean outermost, innermost or anything in between.

I feel the interpretation of the level 0 is driver-dependent?

> +- ``1`` requests access to the outermost packet encapsulation level.
> +- ``2`` and subsequent values requests access to the specified packet
> +encapsulation level, from outermost to innermost (lower to higher values).
> +For the tag array ``level`` translates directly into the array index.

You should define what is a tag array.

> +
> +``offset`` specifies the number of bits to skip from a field's start.
> +That allows performing a partial copy of the needed part or to divide a big
> +packet field into multiple smaller fields. Alternatively, ``offset`` allows
> +going past the specified packet field boundary to copy a field to an
> +arbitrary place in a packet, essentially providing a way to copy any part of
> +a packet to any other part of it if supported by an underlying PMD driver.

All features of this specification require PMD support,
so I think it is useless to mention here.

> +
> +``value`` sets an immediate value to be used as a source or points to a
> +location of the value in memory. It is used instead of ``level`` and ``offset``
> +for ``RTE_FLOW_FIELD_VALUE`` and ``RTE_FLOW_FIELD_POINTER`` respectively.
> +
> +.. _table_rte_flow_action_modify_field:
> +
> +.. table:: MODIFY_FIELD
> +
> +   +-----------------------------------------+
> +   | Field         | Value                   |
> +   +===============+=========================+
> +   | ``op``        | operation to perform    |
> +   | ``dst``       | destination field       |
> +   | ``src``       | source field            |
> +   | ``width``     | number of bits to use   |
> +   +---------------+-------------------------+
> +
> +.. _table_rte_flow_action_modify_data:
> +
> +.. table:: destination/source field definition
> +
> +   +--------------------------------------------------------------------------+
> +   | Field         | Value                                                    |
> +   +===============+==========================================================+
> +   | ``field``     | ID: packet field, mark, meta, tag, immediate, pointer    |
> +   | ``level``     | encapsulation level of a packet field or tag array index |
> +   | ``offset``    | number of bits to skip at the beginning                  |
> +   | ``value``     | immediate value or a pointer to this value               |
> +   +---------------+----------------------------------------------------------+

I know the whole rte_flow guide has this kind of table,
but really it looks like doxygen and can be skipped here.

If really this redundant info is required, it should be RST definition lists,
not tables.

[...]

In my opinion, the most important info is in the doxygen comments.

Please add a doxygen comment for this enum.

> +enum rte_flow_field_id {
> +	RTE_FLOW_FIELD_START = 0,

Please add a doxygen comment for the value RTE_FLOW_FIELD_START.

> +	RTE_FLOW_FIELD_MAC_DST,
> +	RTE_FLOW_FIELD_MAC_SRC,
> +	RTE_FLOW_FIELD_VLAN_TYPE,
> +	RTE_FLOW_FIELD_VLAN_ID,
> +	RTE_FLOW_FIELD_MAC_TYPE,
> +	RTE_FLOW_FIELD_IPV4_DSCP,
> +	RTE_FLOW_FIELD_IPV4_TTL,
> +	RTE_FLOW_FIELD_IPV4_SRC,
> +	RTE_FLOW_FIELD_IPV4_DST,
> +	RTE_FLOW_FIELD_IPV6_HOPLIMIT,
> +	RTE_FLOW_FIELD_IPV6_SRC,
> +	RTE_FLOW_FIELD_IPV6_DST,
> +	RTE_FLOW_FIELD_TCP_PORT_SRC,
> +	RTE_FLOW_FIELD_TCP_PORT_DST,
> +	RTE_FLOW_FIELD_TCP_SEQ_NUM,
> +	RTE_FLOW_FIELD_TCP_ACK_NUM,
> +	RTE_FLOW_FIELD_TCP_FLAGS,
> +	RTE_FLOW_FIELD_UDP_PORT_SRC,
> +	RTE_FLOW_FIELD_UDP_PORT_DST,
> +	RTE_FLOW_FIELD_VXLAN_VNI,
> +	RTE_FLOW_FIELD_GENEVE_VNI,
> +	RTE_FLOW_FIELD_GTP_TEID,
> +	RTE_FLOW_FIELD_TAG,
> +	RTE_FLOW_FIELD_MARK,
> +	RTE_FLOW_FIELD_META,
> +	RTE_FLOW_FIELD_POINTER,

Please add a doxygen comment for the value RTE_FLOW_FIELD_POINTER.

> +	RTE_FLOW_FIELD_VALUE,

Please add a doxygen comment for the value RTE_FLOW_FIELD_VALUE.

> +struct rte_flow_action_modify_data {
> +	enum rte_flow_field_id field;
> +	RTE_STD_C11
> +	union {
> +		struct {
> +			uint32_t level;
> +			uint32_t offset;
> +		};
> +		uint64_t value;
> +	};
> +};

Please add doxygen for this struct and fields.

> +
> +enum rte_flow_modify_op {
> +	RTE_FLOW_MODIFY_SET = 0,
> +	RTE_FLOW_MODIFY_ADD,
> +	RTE_FLOW_MODIFY_SUB,
> +};

Please add doxygen for this enum.

> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *
> + * RTE_FLOW_ACTION_TYPE_MODIFY_FIELD
> + *
> + * Modifies a destination header field according to the specified
> + * operation. Another packet field can be used as a source as well
> + * as tag, mark, metadata or an immediate value or a pointer to it.
> + * Width is the number of bits used from the source item.

width should be documented below as other fields.

> + */
> +struct rte_flow_action_modify_field {
> +	enum rte_flow_modify_op operation;
> +	struct rte_flow_action_modify_data dst;
> +	struct rte_flow_action_modify_data src;
> +	uint32_t width;
> +};




  reply	other threads:[~2021-01-17 23:15 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-08  6:32 [dpdk-dev] [PATCH] ethdev: introduce generic copy " Alexander Kozyrev
2021-01-10  8:00 ` Ori Kam
2021-01-10  9:36   ` Asaf Penso
2021-01-12 14:15   ` Alexander Kozyrev
2021-01-12 14:52     ` Ori Kam
2021-01-12 15:13       ` Ori Kam
2021-01-12 17:19         ` Alexander Kozyrev
2021-01-12  5:01 ` [dpdk-dev] [PATCH v2 0/2] generic copy rte flow action support Alexander Kozyrev
2021-01-12  5:01   ` [dpdk-dev] [PATCH v2 1/2] ethdev: introduce generic copy rte flow action Alexander Kozyrev
2021-01-12  5:01   ` [dpdk-dev] [PATCH v2 2/2] app/testpmd: add support for " Alexander Kozyrev
2021-01-12 14:58     ` Ori Kam
2021-01-13  3:38   ` [dpdk-dev] [PATCH v3 0/2] generic copy rte flow action support Alexander Kozyrev
2021-01-13  3:38     ` [dpdk-dev] [PATCH v3 1/2] ethdev: introduce generic copy rte flow action Alexander Kozyrev
2021-01-13 11:12       ` Ori Kam
2021-01-13  3:38     ` [dpdk-dev] [PATCH v3 2/2] app/testpmd: add support for " Alexander Kozyrev
2021-01-13 17:07     ` [dpdk-dev] [PATCH v4 0/2] generic copy rte flow action support Alexander Kozyrev
2021-01-13 17:07       ` [dpdk-dev] [PATCH v4 1/2] ethdev: introduce generic copy rte flow action Alexander Kozyrev
2021-01-14 12:22         ` Ori Kam
2021-01-13 17:07       ` [dpdk-dev] [PATCH v4 2/2] app/testpmd: add support for " Alexander Kozyrev
2021-01-14 15:18         ` Ori Kam
2021-01-15 15:37           ` Alexander Kozyrev
2021-01-15 15:42       ` [dpdk-dev] [PATCH v5 0/2] generic modify rte flow action support Alexander Kozyrev
2021-01-15 15:42         ` [dpdk-dev] [PATCH v5 1/2] ethdev: introduce generic modify rte flow action Alexander Kozyrev
2021-01-15 18:03           ` Ori Kam
2021-01-17  9:23             ` Slava Ovsiienko
2021-01-15 15:42         ` [dpdk-dev] [PATCH v5 2/2] app/testpmd: add support for modify field " Alexander Kozyrev
2021-01-15 18:04           ` Ori Kam
2021-01-16  4:44         ` [dpdk-dev] [PATCH v6 0/2] generic modify rte flow action support Alexander Kozyrev
2021-01-16  4:44           ` [dpdk-dev] [PATCH v6 1/2] ethdev: introduce generic modify rte flow action Alexander Kozyrev
2021-01-16  4:44           ` [dpdk-dev] [PATCH v6 2/2] app/testpmd: add support for modify field " Alexander Kozyrev
2021-01-16  4:50           ` [dpdk-dev] [PATCH v7 0/2] generic modify rte flow action support Alexander Kozyrev
2021-01-16  4:50             ` [dpdk-dev] [PATCH v7 1/2] ethdev: introduce generic modify rte flow action Alexander Kozyrev
2021-01-17 23:15               ` Thomas Monjalon [this message]
2021-01-18 16:03                 ` Alexander Kozyrev
2021-01-16  4:50             ` [dpdk-dev] [PATCH v7 2/2] app/testpmd: add support for modify field " Alexander Kozyrev
2021-01-18 16:18             ` [dpdk-dev] [PATCH v8 0/2] generic modify rte flow action support Alexander Kozyrev
2021-01-18 16:18               ` [dpdk-dev] [PATCH v8 1/2] ethdev: introduce generic modify rte flow action Alexander Kozyrev
2021-01-18 17:51                 ` Thomas Monjalon
2021-01-18 16:18               ` [dpdk-dev] [PATCH v8 2/2] app/testpmd: add support for modify field " Alexander Kozyrev
2021-01-18 20:05               ` [dpdk-dev] [PATCH v8 0/2] generic modify rte flow action support Ajit Khaparde
2021-01-18 21:40               ` [dpdk-dev] [PATCH v9 " Alexander Kozyrev
2021-01-18 21:40                 ` [dpdk-dev] [PATCH v9 1/2] ethdev: introduce generic modify rte flow action Alexander Kozyrev
2021-01-18 21:40                 ` [dpdk-dev] [PATCH v9 2/2] app/testpmd: add support for modify field " Alexander Kozyrev
2021-01-19  1:21                 ` [dpdk-dev] [PATCH v9 0/2] generic modify rte flow action support Ferruh Yigit
2021-01-14 13:59 ` [dpdk-dev] [PATCH] ethdev: introduce generic copy rte flow action Jerin Jacob
2021-01-14 15:02   ` Ori Kam
2021-01-15 14:00     ` Jerin Jacob
2021-01-15 15:33       ` Alexander Kozyrev

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=4720582.E7ApZDAgU3@thomas \
    --to=thomas@monjalon.net \
    --cc=akozyrev@nvidia.com \
    --cc=andrew.rybchenko@oktetlabs.ru \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=jerinjacobk@gmail.com \
    --cc=orika@nvidia.com \
    --cc=viacheslavo@nvidia.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

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git