DPDK patches and discussions
 help / color / mirror / Atom feed
From: Alexander Kozyrev <akozyrev@nvidia.com>
To: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	Slava Ovsiienko <viacheslavo@nvidia.com>,
	Ori Kam <orika@nvidia.com>,
	"ferruh.yigit@intel.com" <ferruh.yigit@intel.com>,
	"andrew.rybchenko@oktetlabs.ru" <andrew.rybchenko@oktetlabs.ru>,
	"jerinjacobk@gmail.com" <jerinjacobk@gmail.com>
Subject: Re: [dpdk-dev] [PATCH v7 1/2] ethdev: introduce generic modify rte flow action
Date: Mon, 18 Jan 2021 16:03:28 +0000
Message-ID: <BN7PR12MB2707A9FB1336382BEB32E828AFA40@BN7PR12MB2707.namprd12.prod.outlook.com> (raw)
In-Reply-To: <4720582.E7ApZDAgU3@thomas>

> From: Thomas Monjalon <thomas@monjalon.net> on Sunday, January 17, 2021 18:16
> 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:
> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatch
> es.dpdk.org%2Fpatch%2F85384%2F&amp;data=04%7C01%7Cakozyrev%40nv
> idia.com%7Ce20d2c38372b4062420d08d8bb3dd549%7C43083d15727340c1b7d
> b39efd9ccc17a%7C0%7C0%7C637465221537183554%7CUnknown%7CTWFpb
> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
> 6Mn0%3D%7C1000&amp;sdata=gWEdmyAabnCCmeFyi%2FvHs0aheMmWNx
> IyR%2BTC96O3Yck%3D&amp;reserved=0
> 
> You don't need to refer to the RFC in the commit message.
Removing link to it.

> 
> > 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?
Right, thanks for catching this.

> 
> > +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?
It is driver-dependent, the behavior is the same as in case of RSS.

> > +- ``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.
Ok, but it is defined already in RTE_FLOW_ACTION_TYPE_SET_TAG action.

> > +
> > +``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.
Removing.
 
> > +
> > +``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.
I would keep these tables for the sake of consistency of the rte_flow guide.

> [...]
> 
> In my opinion, the most important info is in the doxygen comments.
Agree, adding them for all the structures/fields mentioned below.

> 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-18 16:03 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
2021-01-18 16:03                 ` Alexander Kozyrev [this message]
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=BN7PR12MB2707A9FB1336382BEB32E828AFA40@BN7PR12MB2707.namprd12.prod.outlook.com \
    --to=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=thomas@monjalon.net \
    --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 http://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/ http://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