DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>,
	"Pavan Nikhilesh Bhagavatula" <pbhagavatula@marvell.com>,
	"stephen@networkplumber.org" <stephen@networkplumber.org>,
	"arybchenko@solarflare.com" <arybchenko@solarflare.com>,
	"hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>,
	"thomas@monjalon.net" <thomas@monjalon.net>,
	"Yigit, Ferruh" <ferruh.yigit@intel.com>,
	"Richardson, Bruce" <bruce.richardson@intel.com>,
	Neil Horman <nhorman@tuxdriver.com>,
	"Mcnamara, John" <john.mcnamara@intel.com>,
	"Kovacevic, Marko" <marko.kovacevic@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [patch v3] doc: announce API change in ethdev offload	flags
Date: Thu, 8 Aug 2019 16:53:48 +0000	[thread overview]
Message-ID: <2601191342CEEE43887BDE71AB9772580168A63C89@irsmsx105.ger.corp.intel.com> (raw)
In-Reply-To: <BYAPR18MB2424675A22DB5BA733AEC587C8D70@BYAPR18MB2424.namprd18.prod.outlook.com>

> > > > > > > Add new offload flags ``DEV_RX_OFFLOAD_PTYPE``,
> > > > > > ``DEV_RX_OFFLOAD_RSS``
> > > > > > > and ``DEV_RX_OFFLOAD_FLOW_MARK``.
> > > > > > >
> > > > > > > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > > > > > Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> > > > > > > Acked-by: Jerin Jacob <jerinj@marvell.com>
> > > > > > > ---
> > > > > > >  v3 Changes:
> > > > > > >  - DEV_RX_OFFLOAD_RSS -> DEV_RX_OFFLOAD_RSS_HASH
> > (anndrew).
> > > > > > >
> > > > > > >  v2 Changes:
> > > > > > >  - Reword for clarity.
> > > > > > >
> > > > > > >  doc/guides/rel_notes/deprecation.rst | 13 +++++++++++++
> > > > > > >  1 file changed, 13 insertions(+)
> > > > > > >
> > > > > > > diff --git a/doc/guides/rel_notes/deprecation.rst
> > > > > > > b/doc/guides/rel_notes/deprecation.rst
> > > > > > > index 37b8592b6..056c5709f 100644
> > > > > > > --- a/doc/guides/rel_notes/deprecation.rst
> > > > > > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > > > > > @@ -78,3 +78,16 @@ Deprecation Notices
> > > > > > >    to set new power environment if power environment was
> > > > > > > already
> > > > > > initialized.
> > > > > > >    In this case the function will return -1 unless the
> > > > > > > environment is unset
> > > > > > first
> > > > > > >    (using ``rte_power_unset_env``). Other function usage
> > > > > > > scenarios will not
> > > > > > change.
> > > > > > > +
> > > > > > > +* ethdev: New offload flags ``DEV_RX_OFFLOAD_PTYPE``,
> > > > > > > +``DEV_RX_OFFLOAD_RSS_HASH``
> > > > > > > +  and ``DEV_RX_OFFLOAD_FLOW_MARK`` will be added in 19.11.
> > > > > >
> > > > > > One question about DEV_RX_OFFLOAD_PTYPE:
> > > > > > Does it mean that new ol_flags value (PKT_RX_PTYPE) will be
> > > > > > introduced to indicate that mbuf.packet_type value is set?
> > > > > > Or PMD will have to set  mbuf.packet_type to zero, when
> > > > > > DEV_RX_OFFLOAD_PTYPE was not enabled by user?
> > > > >
> > > > > I was thinking when DEV_RX_OFFLOAD_PTYPE is set
> > > > > - mbuf.packet_type will be valid and mbuf.packet_type will have
> > > > > parsed
> > > > packet type.
> > > > > If not set
> > > > > - mbuf.packet_type can be anything application should not use
> > > > mbuf.packet_type field.
> > > >
> > > > But in that case, we do need a new value for ol_flags, PKT_RX_PTYPE
> > > > or so, right?
> > >
> > > Since application has two knobs rte_eth_dev_get_supported_ptypes() and
> > > DEV_RX_OFFLOAD_PTYPE. We may not need to new ol_flags for this
> > change. Right?
> > > i.e if application sets the DEV_RX_OFFLOAD_PTYPE, The application will
> > > get the parsed ptypes by the driver(=
> > rte_eth_dev_get_supported_ptypes()).
> > > So there is no scope ambiguity. Right?
> >
> > I still think there is:
> > Imagine user has 2 eth devices, one does support DEV_RX_OFFLOAD_PTYPE,
> > second doesn't.  Now he has a mix of packets from both devices, that you
> > want t process.
> > How would he figure out for which of them ptype values are valid, and for
> > each are not?
> > Trace back from what port he has received them?
> > Not very convenient, and not always possible.
> 
> I thought so. But in that case, application can always set DEV_RX_OFFLOAD_PTYPE
> Flags for all the ethdev ports. Right? Rather having any complicated ol_flags
> or port based parsing. If limit the _contract_ to following, we are good. Right?
> # when DEV_RX_OFFLOAD_PTYPE is set, mbuf.packet_type will be valid
> and mbuf.packet_type will have parsed packet type

Yes sure in principle user can calculate smallest common subset of RX offloads
supported by all devs in the system and use only  them.
Then he can use some global value for ol_flags that will be setup at initialization time,
instead of checking ol_flags for every mbuf. 
Though inside DPDK we don't use that method for other offloads (cksum, vlan, rss).
Why we should do different here?
Again how to deal with hot-plugged devices with such approach?

> 
> or the negative offload(This contract is pretty clear, I don't think any ambiguity at all)
> # when DEV_RX_OFFLOAD_NO_PTYPE(something similar) is set,
> mbuf.packet_type will be invalid.
> 
> > I think we need either to introduce new ol_flag value (as we usually do for
> > other RX offloads),
> > or force PMD to always set ptype value.
> 
> Setting new  ol_flag value may effect performance for existing drivers
> which don't planning to use this offload

If the driver doesn't support DEV_RX_OFFLOAD_PTYPE, it wouldn't need to set anything
(neither ol_flags, neither packet_type).

> and it complicates the
> application to have additional check based on ol_flag. If you see any corner case with above section,
> 
> How about just setting as ptype as 0 incase it is not parsed by driver.

As I said above - ok by me.
AFAIK, this is current behavior, so no changes in PMD will be required.

> Actual lookup is the costly one, writing 0 to pytpe is not costly
> as there are plenty of writes in Rx and it will be write merged(No CPU stall)

Yes packet_type is at first 64B, so shouldn't cause any extra overhead.

> 
> I did not get the complete picture of "rte_eth_dev_set_supported_ptypes(uint16_t port_id, uint32_t
> ptype_mask); instead of DEV_RX_OFFLOAD_PTYPE? scheme", Does it help?

I thought about it as just a different way to disable(/limit) requested by user PTYPE support.
If let say user is not interested in ptype information at all, he can ask PMD to just 
always set ptype value to 0:
rte_eth_dev_set_supported_ptypes(port, RTE_PTYPE_UNKNOWN);

if he is interested just in L2/L3 layer info,
he can ask PMD to provide ptype information only for L2/L3:
rte_eth_dev_set_supported_ptypes(port, RTE_PTYPE_L2_MASK | RTE_PTYPE_L3_MASK);

Or to enable all supported by PMD ptypes: 
rte_eth_dev_set_supported_ptypes(port, UINT32_MAX)



  parent reply	other threads:[~2019-08-08 16:53 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-07 16:09 [dpdk-dev] [patch] " pbhagavatula
2019-08-07 19:36 ` Andrew Rybchenko
2019-08-08  8:17 ` [dpdk-dev] [patch v2] " pbhagavatula
2019-08-08  8:33   ` Jerin Jacob Kollanukkaran
2019-08-08  8:55     ` Pavan Nikhilesh Bhagavatula
2019-08-08  8:58   ` [dpdk-dev] [patch v3] " pbhagavatula
2019-08-08  9:23     ` Ananyev, Konstantin
2019-08-08 10:00       ` Jerin Jacob Kollanukkaran
2019-08-08 10:08         ` Ananyev, Konstantin
2019-08-08 10:23           ` Jerin Jacob Kollanukkaran
2019-08-08 10:33             ` Ananyev, Konstantin
2019-08-08 10:59               ` Jerin Jacob Kollanukkaran
2019-08-08 11:08                 ` Thomas Monjalon
2019-08-08 16:53                 ` Ananyev, Konstantin [this message]
2019-08-09  3:48                   ` Jerin Jacob Kollanukkaran
2019-08-09  8:24                     ` Ananyev, Konstantin
2019-08-09  8:13     ` Hemant Agrawal
2019-08-09  8:17     ` [dpdk-dev] [patch v4] " pbhagavatula
2019-08-09  8:47       ` Ananyev, Konstantin
2019-08-09  9:07         ` Pavan Nikhilesh Bhagavatula
2019-08-09  9:13       ` Tom Barbette
2019-08-09  9:22         ` Andrew Rybchenko
2019-08-09  9:28         ` Jerin Jacob Kollanukkaran
2019-08-09  9:55       ` [dpdk-dev] [patch v5] " pbhagavatula
2019-08-09 10:13         ` Ananyev, Konstantin
2019-08-10 21:10           ` Thomas Monjalon

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=2601191342CEEE43887BDE71AB9772580168A63C89@irsmsx105.ger.corp.intel.com \
    --to=konstantin.ananyev@intel.com \
    --cc=arybchenko@solarflare.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=hemant.agrawal@nxp.com \
    --cc=jerinj@marvell.com \
    --cc=john.mcnamara@intel.com \
    --cc=marko.kovacevic@intel.com \
    --cc=nhorman@tuxdriver.com \
    --cc=pbhagavatula@marvell.com \
    --cc=stephen@networkplumber.org \
    --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
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).