DPDK patches and discussions
 help / color / mirror / Atom feed
From: Olivier Matz <olivier.matz@6wind.com>
To: Slava Ovsiienko <viacheslavo@mellanox.com>
Cc: Andrew Rybchenko <arybchenko@solarflare.com>,
	"dev@dpdk.org" <dev@dpdk.org>, Matan Azrad <matan@mellanox.com>,
	Raslan Darawsheh <rasland@mellanox.com>,
	Thomas Monjalon <thomas@monjalon.net>
Subject: Re: [dpdk-dev] [PATCH v5] ethdev: extend flow metadata
Date: Wed, 30 Oct 2019 17:13:15 +0100	[thread overview]
Message-ID: <20191030161315.slvgrwi42ry2exwd@platinum> (raw)
In-Reply-To: <AM4PR05MB3265977FD6D09B4DF7A68CE8D2600@AM4PR05MB3265.eurprd05.prod.outlook.com>

On Wed, Oct 30, 2019 at 03:58:04PM +0000, Slava Ovsiienko wrote:
> > -----Original Message-----
> > From: Olivier Matz <olivier.matz@6wind.com>
> > Sent: Wednesday, October 30, 2019 17:20
> > To: Slava Ovsiienko <viacheslavo@mellanox.com>
> > Cc: Andrew Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org;
> > Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> > <rasland@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>
> > Subject: Re: [PATCH v5] ethdev: extend flow metadata
> > 
> > Hi,
> > 
> > On Wed, Oct 30, 2019 at 02:46:06PM +0000, Slava Ovsiienko wrote:
> > > > -----Original Message-----
> > > > From: Slava Ovsiienko
> > > > Sent: Wednesday, October 30, 2019 16:41
> > > > To: Andrew Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org
> > > > Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> > > > <rasland@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>;
> > > > olivier.matz@6wind.com
> > > > Subject: RE: [PATCH v5] ethdev: extend flow metadata
> > > >
> > > > > -----Original Message-----
> > > > > From: Andrew Rybchenko <arybchenko@solarflare.com>
> > > > > Sent: Wednesday, October 30, 2019 10:02
> > > > > To: Slava Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org
> > > > > Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> > > > > <rasland@mellanox.com>; Thomas Monjalon
> > <thomas@monjalon.net>;
> > > > > olivier.matz@6wind.com; Yongseok Koh <yskoh@mellanox.com>
> > > > > Subject: Re: [PATCH v5] ethdev: extend flow metadata
> > > > >
> > > > > On 10/29/19 10:31 PM, Viacheslav Ovsiienko wrote:
> > > > > > Currently, metadata can be set on egress path via mbuf
> > > > > > tx_metadata field with PKT_TX_METADATA flag and
> > > > > > RTE_FLOW_ITEM_TYPE_META
> > > > > matches metadata.
> > > > > >
> > > > > > This patch extends the metadata feature usability.
> > > > > >
> > > > > > 1) RTE_FLOW_ACTION_TYPE_SET_META
> > > > > >
> > > > > > When supporting multiple tables, Tx metadata can also be set by
> > > > > > a rule and matched by another rule. This new action allows
> > > > > > metadata to be set as a result of flow match.
> > > > > >
> > > > > > 2) Metadata on ingress
> > > > > >
> > > > > > There's also need to support metadata on ingress. Metadata can
> > > > > > be set by SET_META action and matched by META item like Tx. The
> > > > > > final value set by the action will be delivered to application
> > > > > > via metadata dynamic field of mbuf which can be accessed by
> > > > > > RTE_FLOW_DYNF_METADATA() macro or with
> > > > > > rte_flow_dynf_metadata_set() and rte_flow_dynf_metadata_get()
> > > > > > helper routines. PKT_RX_DYNF_METADATA flag will be set along with
> > the data.
> > > >
> > > > We have a problem with PKT_RX_DYNF_METADATA/
> > PKT_TX_DYNF_METADATA.
> > > > These ones are referencing to global variable
> > > > "rte_flow_dynf_metadata_mask".
> > > > And there are routines in rte_mbuf.c  (rte_get_rx_ol_flag_list)
> > > > which show the names of flags. It is not good if rte_mbuf.c would
> > > > require including of rte_flow.h and  linking rte_flow.c (not all apps use
> > rte_flow or even ethdev).
> > > >
> > > > What do you think? Should we rename rte_flow_dynf_xxxxx variables to
> > > > rte_mbuf_dynf_flow_metadata_xxxx and put ones into the
> > rte_mbuf_dyn.c?
> > > > The same about PKT_RX_DYNF_METADATA definition, is it candidate to
> > > > move to rte_mbuf_dyn.h ? It would allow not to link or include
> > > > rte_flow.c/h into rte_mbuf.c
> > > >
> > 
> > In rte_mbuf_dyn.c, we maintain a list of registered flags. I think it wouldn't
> > be too difficult to introduce the equivalent of
> > rte_get_*_ol_flag_list() and *rte_get_*_ol_flag_name() for dynamic flags.
> > There is already a dump function (which does both fields and flags), and a
> > lookup by name function.
> 
> Nice idea, thanks. I think existing lookup by name is OK, I'll update
> flag list routines with dynamic flag support.
> 
> > 
> > Maybe we could split the dump into fields and flags, and add a lookup by
> > offset/bitnum. Would it work for your use-case?
> 
> Not needed right now, I think.
> 
> > 
> > > It is interesting to note that despite metadata field looks to be
> > > related to rte_flow, there is no any reference to this field or flags inside
> > rte_flow API implementation.
> > > Only datapath references this field. Metadata is gateway between flow
> > > HW space and datapath, it tends to be mostly on datapath side not on
> > rte_flow.
> > 
> > Yes, only the registration of the field is related to rte_flow. But I don't get
> > where are you going with this. Is it a problem?
> 
> It is not a problem, rather some concern.
> IMO, the datapath related entities (flag mask and field offset) are put in not appropriate place.
> Now we have to add lookup to flag list routines, which we could avoid. The next candidates,
> like timestamp or timesync might introduce some new issues.

If the underlying question is should we centralize or not centralize the
definitions of dynamic fields/flags helpers, I'll tend to say that we
should not centralize. The reason is because it will not always be
possible: an application or an external library is allowed to register a
private dynamic field.

Nevertheless, as you say, introducing the next dynamic fields like
timestamp may show up some new issues, and we should be ready to rework
what has been done when we'll have more experience, with more usages of
dynamic fields.

  reply	other threads:[~2019-10-30 16:13 UTC|newest]

Thread overview: 98+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-03 21:32 [dpdk-dev] [RFC 1/3] " Yongseok Koh
2019-06-03 21:32 ` [dpdk-dev] [RFC 2/3] ethdev: add flow modify mark action Yongseok Koh
2019-06-06 10:35   ` Jerin Jacob Kollanukkaran
2019-06-06 18:33     ` Yongseok Koh
2019-06-03 21:32 ` [dpdk-dev] [RFC 3/3] ethdev: add flow tag Yongseok Koh
2019-07-04 23:23   ` [dpdk-dev] [PATCH] " Yongseok Koh
2019-07-05 13:54     ` Adrien Mazarguil
2019-07-05 18:05       ` Yongseok Koh
2019-07-08 23:32         ` Yongseok Koh
2019-07-09  8:38         ` Adrien Mazarguil
2019-07-11  1:59           ` Yongseok Koh
2019-10-08 12:57             ` Yigit, Ferruh
2019-10-08 13:18               ` Slava Ovsiienko
2019-10-10 16:09     ` [dpdk-dev] [PATCH v2] " Viacheslav Ovsiienko
2019-10-24 13:12       ` [dpdk-dev] [PATCH v3] " Viacheslav Ovsiienko
2019-10-27 16:38         ` Ori Kam
2019-10-27 18:42         ` [dpdk-dev] [PATCH v4] " Viacheslav Ovsiienko
2019-10-27 19:11           ` Ori Kam
2019-10-31 18:57             ` Ferruh Yigit
2019-06-09 14:23 ` [dpdk-dev] [RFC 1/3] ethdev: extend flow metadata Andrew Rybchenko
2019-06-10  3:19   ` Wang, Haiyue
2019-06-10  7:20     ` Andrew Rybchenko
2019-06-11  0:06       ` Yongseok Koh
2019-06-19  9:05         ` Andrew Rybchenko
2019-07-04 23:21 ` [dpdk-dev] [PATCH] " Yongseok Koh
2019-07-10  9:31   ` Olivier Matz
2019-07-10  9:55     ` Bruce Richardson
2019-07-10 10:07       ` Olivier Matz
2019-07-10 12:01         ` Bruce Richardson
2019-07-10 12:26           ` Thomas Monjalon
2019-07-10 16:37             ` Yongseok Koh
2019-07-11  7:44               ` Adrien Mazarguil
2019-07-14 11:46                 ` Andrew Rybchenko
2019-07-29 15:06                   ` Adrien Mazarguil
2019-10-08 12:51                     ` Yigit, Ferruh
2019-10-08 13:17                       ` Slava Ovsiienko
2019-10-10 16:02   ` [dpdk-dev] [PATCH v2] " Viacheslav Ovsiienko
2019-10-18  9:22     ` Olivier Matz
2019-10-19 19:47       ` Slava Ovsiienko
2019-10-21 16:37         ` Olivier Matz
2019-10-24  6:49           ` Slava Ovsiienko
2019-10-24  9:22             ` Olivier Matz
2019-10-24 12:30               ` Slava Ovsiienko
2019-10-24 13:08     ` [dpdk-dev] [PATCH v3] " Viacheslav Ovsiienko
2019-10-27 16:56       ` Ori Kam
2019-10-27 18:40       ` [dpdk-dev] [PATCH v4] " Viacheslav Ovsiienko
2019-10-27 19:10         ` Ori Kam
2019-10-29 16:22         ` Andrew Rybchenko
2019-10-29 17:19           ` Slava Ovsiienko
2019-10-29 18:30             ` Thomas Monjalon
2019-10-29 18:35               ` Slava Ovsiienko
2019-10-30  6:28               ` Andrew Rybchenko
2019-10-30  7:35             ` Andrew Rybchenko
2019-10-30  8:59               ` Slava Ovsiienko
2019-10-30  9:20                 ` Andrew Rybchenko
2019-10-30 10:05                   ` Slava Ovsiienko
2019-10-30 10:03                 ` Slava Ovsiienko
2019-10-30 15:49               ` Olivier Matz
2019-10-31  9:25                 ` Andrew Rybchenko
2019-10-29 16:25         ` Olivier Matz
2019-10-29 16:33           ` Olivier Matz
2019-10-29 17:53             ` Slava Ovsiienko
2019-10-29 17:43           ` Slava Ovsiienko
2019-10-29 19:31         ` [dpdk-dev] [PATCH v5] " Viacheslav Ovsiienko
2019-10-30  8:02           ` Andrew Rybchenko
2019-10-30 14:40             ` Slava Ovsiienko
2019-10-30 14:46               ` Slava Ovsiienko
2019-10-30 15:20                 ` Olivier Matz
2019-10-30 15:57                   ` Thomas Monjalon
2019-10-30 15:58                   ` Slava Ovsiienko
2019-10-30 16:13                     ` Olivier Matz [this message]
2019-10-30  8:35           ` Ori Kam
2019-10-30 17:12           ` [dpdk-dev] [PATCH v6 0/2] extend flow metadata feature Viacheslav Ovsiienko
2019-10-30 17:12             ` [dpdk-dev] [PATCH v6 1/2] ethdev: extend flow metadata Viacheslav Ovsiienko
2019-10-31  9:19               ` Andrew Rybchenko
2019-10-31 13:05               ` [dpdk-dev] [PATCH v7 0/2] extend flow metadata feature Viacheslav Ovsiienko
2019-10-31 13:05                 ` [dpdk-dev] [PATCH v7 1/2] ethdev: extend flow metadata Viacheslav Ovsiienko
2019-10-31 15:47                   ` Olivier Matz
2019-10-31 16:13                     ` Slava Ovsiienko
2019-10-31 16:48                   ` [dpdk-dev] [PATCH v8 0/2] extend flow metadata feature Viacheslav Ovsiienko
2019-10-31 16:48                     ` [dpdk-dev] [PATCH v8 1/2] ethdev: extend flow metadata Viacheslav Ovsiienko
2019-11-04  6:13                       ` [dpdk-dev] [PATCH v9 0/2] extend flow metadata feature Viacheslav Ovsiienko
2019-11-04  6:13                         ` [dpdk-dev] [PATCH v9 1/2] ethdev: extend flow metadata Viacheslav Ovsiienko
2019-11-05 14:19                           ` [dpdk-dev] [PATCH v10 0/2] extend flow metadata feature Viacheslav Ovsiienko
2019-11-05 14:19                             ` [dpdk-dev] [PATCH v10 1/2] ethdev: extend flow metadata Viacheslav Ovsiienko
2019-11-05 14:19                             ` [dpdk-dev] [PATCH v10 2/2] ethdev: move egress metadata to dynamic field Viacheslav Ovsiienko
2019-11-06 15:49                             ` [dpdk-dev] [PATCH v10 0/2] extend flow metadata feature Ferruh Yigit
2019-11-04  6:13                         ` [dpdk-dev] [PATCH v9 2/2] ethdev: move egress metadata to dynamic field Viacheslav Ovsiienko
2019-10-31 16:48                     ` [dpdk-dev] [PATCH v8 " Viacheslav Ovsiienko
2019-10-31 17:21                       ` Olivier Matz
2019-11-01 12:34                       ` Andrew Rybchenko
2019-10-31 13:05                 ` [dpdk-dev] [PATCH v7 " Viacheslav Ovsiienko
2019-10-31 13:33                   ` Ori Kam
2019-10-31 15:51                   ` Olivier Matz
2019-10-31 16:07                     ` Slava Ovsiienko
2019-10-30 17:12             ` [dpdk-dev] [PATCH v6 " Viacheslav Ovsiienko
2019-10-31  9:01               ` Andrew Rybchenko
2019-10-31 10:54                 ` Slava Ovsiienko

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=20191030161315.slvgrwi42ry2exwd@platinum \
    --to=olivier.matz@6wind.com \
    --cc=arybchenko@solarflare.com \
    --cc=dev@dpdk.org \
    --cc=matan@mellanox.com \
    --cc=rasland@mellanox.com \
    --cc=thomas@monjalon.net \
    --cc=viacheslavo@mellanox.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).