Re: [dpdk-dev] [patch v3] doc: announce API change in ethdev offload flags

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> -----Original Message-----
> From: Jerin Jacob Kollanukkaran [mailto:jerinj@marvell.com]
> Sent: Thursday, August 8, 2019 11:23 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>;
> stephen@networkplumber.org; arybchenko@solarflare.com; hemant.agrawal@nxp.com; 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
> Subject: RE: [dpdk-dev] [patch v3] doc: announce API change in ethdev offload flags
> 
> > -----Original Message-----
> > From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > Sent: Thursday, August 8, 2019 3:39 PM
> > To: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Pavan Nikhilesh
> > Bhagavatula <pbhagavatula@marvell.com>; stephen@networkplumber.org;
> > arybchenko@solarflare.com; hemant.agrawal@nxp.com;
> > 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
> > Subject: [EXT] RE: [dpdk-dev] [patch v3] doc: announce API change in ethdev
> > offload flags
> >
> > External Email
> >
> > ----------------------------------------------------------------------
> > Hi Jerin,
> 
> Hi Konstantin,
> 
> 
> >
> > > >
> > > > Hi guys,
> > > >
> > > > >
> > > > > From: Pavan Nikhilesh <pbhagavatula@marvell.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 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.   
Konstantin

> 
> 
> >
> > >
> > > This will avoid writes 0 to mbuf.packet_type and packet_type parsing if
> > offload is not set.
> > >
> > >
> > > > If so, what is the advantage?
> > > > Again in that case, would it be more plausible to introduce something like:
> > > > rte_eth_dev_set_supported_ptypes(uint16_t port_id, uint32_t
> > > > ptype_mask); instead of DEV_RX_OFFLOAD_PTYPE?
> > >
> > > Any scheme is fine where we can skip the  write 0 to mbuf.packet_type
> > > and packet_type parsing If application is NOT interested in packet_type.
> > >
> > > > Konstantin
> > > >
> > > > > +  This will allow application to enable or disable PMDs from
> > > > > + updating ``rte_mbuf`` fields ``rte_mbuf::packet_type``,
> > > > > + ``rte_mbuf::hash::rss`` and  ``rte_mbuf::hash::fdir`` respectively.
> > > > > +  This scheme will allow PMDs to avoid writes to ``rte_mbuf``
> > > > > + fields on Rx and  thereby improve Rx performance if application
> > wishes do so.
> > > > > +  In 19.11 PMDs will still update the fields even when the
> > > > > + offloads are not  enabled.
> > > > > +  The exact semantics of the flags will be worked out later
> > > > > + either by making  them negative offloads to avoid application
> > > > > + change or positive offload to  align with existing offload flag
> > semantics.
> > > > > --
> > > > > 2.17.1