DPDK-dev Archive on lore.kernel.org
 help / color / Atom feed
From: Shahaf Shuler <shahafs@mellanox.com>
To: Andrew Rybchenko <arybchenko@solarflare.com>,
	"pbhagavatula@marvell.com" <pbhagavatula@marvell.com>,
	"jerinj@marvell.com" <jerinj@marvell.com>,
	"ferruh.yigit@intel.com" <ferruh.yigit@intel.com>,
	John McNamara <john.mcnamara@intel.com>,
	Marko Kovacevic <marko.kovacevic@intel.com>,
	Thomas Monjalon <thomas@monjalon.net>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH 3/7] ethdev: add flow action type update as an offload
Date: Sun, 18 Aug 2019 06:20:41 +0000
Message-ID: <AM0PR0502MB379530AB78CD68D1EE572D22C3A90@AM0PR0502MB3795.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <3e4b3c49-842e-4d99-fdc0-7647a4897296@solarflare.com>

Sunday, August 18, 2019 8:57 AM, Andrew Rybchenko:
> Subject: Re: [dpdk-dev] [PATCH 3/7] ethdev: add flow action type update as
> an offload
> 
> On 8/18/19 7:59 AM, Shahaf Shuler wrote:
> > Friday, August 16, 2019 11:05 AM, Andrew Rybchenko:
> >> <marko.kovacevic@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>
> >> Cc: dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH 3/7] ethdev: add flow action type
> >> update as an offload
> >>
> >> On 8/16/19 8:55 AM, pbhagavatula@marvell.com wrote:
> >>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >>>
> >>> Add new Rx offload flag `DEV_RX_OFFLOAD_FLOW_MARK` that can be
> >> used to
> >>> enable/disable PMDs write to `rte_mbuf::hash::fdir::hi`.
> >> Notes similar to RSS hash.
> >>
> >> It requires better motivation why. It lets Rx queue know that it will
> >> be used as flow action MARK target and the queue should be configured
> >> to deliver the mark from NIC to PMD and processed in the driver.
> > This one is even worse than the RSS (sorry).
> >
> > First - the API breakage exists also here and if we want to include such
> patch we should have proper doc on RN.
> 
> Yes, there is a deprecation notice for it in v19.08 and I already mentioned in
> review notes for the patch [1/7] that release notes are required.
> 
> > Second - the user explicitly inserted a rte_flow rule w/ action mark. Its
> expectation is to receive his mark on the mbuf (otherwise why would it set
> this action?).  it is not expected from the user to set another offload flag just
> to enable the mark set on the mbuf. It makes the user experience very
> convoluted.
> > Third - so far we never reported rte_flow capabilities, and there is a good
> reason for it - the cap matrix is too big. For rte_flow the chosen approach was
> trail and error. If we start w/ the flow mark, the amount of cap bits the
> application will need to monitor will be huge.
> 
> The feature differs a lot from other rte_flow features since it adds Rx meta
> information.
> And yes, rte_flow rule to set mark should fail with appropriate diagnostics if
> the offload is supported by not enabled or not supported at all. So,
> application will be informed and I think it is less worse than RSS hash.
> 
> > My suggestion here, for PMD that wants to optimize their datapath is to
> check if flow w/ mark action was inserted on the queue. So long there is no
> such flow they can disable the set of the mark.
> 
> Unfortunately the information is required on Rx queue setup stage and
> rte_rule insertion is too late.

See my comments on the RSS patch. Same point of discussion.

The main question we need to answer -
User set flow mark action by rte_flow (that was accepted). Why does it need to set more flag? 

> 
> Anyway, the important point here is all Rx offloads consistency:
> want something - enable it. The only remaining exception is packet type
> which default behaviour is preserved since really flexible solution suggested
> by Konstantin.
> 
> >> Also I think that flow API action MARK documentation should be
> >> updated to mentioned the offload.
> >>
> >>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >>> ---
> >>>    doc/guides/nics/features.rst   | 12 ++++++++++++
> >>>    lib/librte_ethdev/rte_ethdev.h |  1 +
> >>>    2 files changed, 13 insertions(+)
> >>>
> >>> diff --git a/doc/guides/nics/features.rst
> >>> b/doc/guides/nics/features.rst index f79b69b38..d67430d90 100644
> >>> --- a/doc/guides/nics/features.rst
> >>> +++ b/doc/guides/nics/features.rst
> >>> @@ -594,6 +594,18 @@ application to set ptypes it is interested in.
> >>>    * **[provides]   mbuf**: ``mbuf.packet_type``.
> >>>
> >>>
> >>> +.. _nic_features_flow_action_type_update:
> >> May be  _nic_features_flow_mark ?
> >>
> >>> +
> >>> +Flow type update
> >> May be "Flow mark delivery" ?
> >>
> >>> +----------------
> >>> +
> >>> +Supports flow action type update to ``mbuf.ol_flags`` and
> >> ``mbuf.hash.fdir.hi``.
> >>> +
> >>> +* **[uses]     rte_eth_rxconf,rte_eth_rxmode**:
> >> ``offloads:DEV_RX_OFFLOAD_FLOW_TYPE``.
> >>
> >> DEV_RX_OFFLOAD_FLOW_MARK, not TYPE.
> >>
> >>
> >>
> >>> +* **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_FDIR``,
> >>> +``mbuf.ol_flags:PKT_RX_FDIR_ID;``,
> >>> +  ``mbuf.hash.fdir.hi``
> >>> +
> >>> +
> >>>    .. _nic_features_timesync:
> >>>
> >>>    Timesync
> >>> diff --git a/lib/librte_ethdev/rte_ethdev.h
> >>> b/lib/librte_ethdev/rte_ethdev.h index 889486a11..4a0cff830 100644
> >>> --- a/lib/librte_ethdev/rte_ethdev.h
> >>> +++ b/lib/librte_ethdev/rte_ethdev.h
> >>> @@ -1014,6 +1014,7 @@ struct rte_eth_conf {
> >>>    #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
> >>>    #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
> >>>    #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
> >>> +#define DEV_RX_OFFLOAD_FLOW_MARK	0x00100000
> >>>
> >>>    #define DEV_RX_OFFLOAD_CHECKSUM
> >> (DEV_RX_OFFLOAD_IPV4_CKSUM | \
> >>>    				 DEV_RX_OFFLOAD_UDP_CKSUM | \
> >> Add to rte_rx_offload_names


  reply index

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-16  5:55 [dpdk-dev] [PATCH 0/7] ethdev: add new Rx offload flags pbhagavatula
2019-08-16  5:55 ` [dpdk-dev] [PATCH 1/7] ethdev: add set ptype function pbhagavatula
2019-08-16  8:22   ` Andrew Rybchenko
2019-08-17 16:27     ` [dpdk-dev] [EXT] " Pavan Nikhilesh Bhagavatula
2019-08-16  5:55 ` [dpdk-dev] [PATCH 2/7] ethdev: add mbuf RSS update as a offload pbhagavatula
2019-08-16  7:48   ` Andrew Rybchenko
2019-08-17 11:47     ` Pavan Nikhilesh Bhagavatula
2019-08-18  4:52     ` Shahaf Shuler
2019-08-18  5:38       ` Andrew Rybchenko
2019-08-18  6:18         ` Shahaf Shuler
2019-08-18  7:00           ` Andrew Rybchenko
2019-08-18 12:11             ` Shahaf Shuler
2019-08-16  5:55 ` [dpdk-dev] [PATCH 3/7] ethdev: add flow action type update as an offload pbhagavatula
2019-08-16  8:05   ` Andrew Rybchenko
2019-08-17 14:23     ` [dpdk-dev] [EXT] " Pavan Nikhilesh Bhagavatula
2019-08-18  9:46       ` Andrew Rybchenko
2019-08-18  4:59     ` [dpdk-dev] " Shahaf Shuler
2019-08-18  5:57       ` Andrew Rybchenko
2019-08-18  6:20         ` Shahaf Shuler [this message]
2019-08-18  7:08           ` Andrew Rybchenko
2019-08-16  5:55 ` [dpdk-dev] [PATCH 4/7] net: update Rx RSS hash offload capabilities pbhagavatula
2019-08-16  7:49   ` Andrew Rybchenko
2019-08-17 12:26     ` Pavan Nikhilesh Bhagavatula
2019-08-16  5:55 ` [dpdk-dev] [PATCH 5/7] net: update Rx flow action " pbhagavatula
2019-08-16  7:50   ` Andrew Rybchenko
2019-08-16  5:55 ` [dpdk-dev] [PATCH 6/7] net: add ptype set default functionality pbhagavatula
2019-08-16  8:30   ` Andrew Rybchenko
2019-08-17 17:24     ` Pavan Nikhilesh Bhagavatula
2019-08-16  5:55 ` [dpdk-dev] [PATCH 7/7] examples/eventdev_pipeline: add new Rx RSS hash offload pbhagavatula
2019-08-16  6:01   ` Jerin Jacob Kollanukkaran
2019-08-16  6:02 ` [dpdk-dev] [PATCH 0/7] ethdev: add new Rx offload flags Jerin Jacob Kollanukkaran

Reply instructions:

You may reply publically 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=AM0PR0502MB379530AB78CD68D1EE572D22C3A90@AM0PR0502MB3795.eurprd05.prod.outlook.com \
    --to=shahafs@mellanox.com \
    --cc=arybchenko@solarflare.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=jerinj@marvell.com \
    --cc=john.mcnamara@intel.com \
    --cc=marko.kovacevic@intel.com \
    --cc=pbhagavatula@marvell.com \
    --cc=thomas@monjalon.net \
    /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-dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dpdk-dev/0 dpdk-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 dpdk-dev dpdk-dev/ https://lore.kernel.org/dpdk-dev \
		dev@dpdk.org dpdk-dev@archiver.kernel.org
	public-inbox-index dpdk-dev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.dpdk.dev


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