[v3] doc: announce API change in ethdev offload flags

  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(+)

--
2.17.1
  

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?
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?
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
  

> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Thursday, August 8, 2019 2:53 PM
> To: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; Jerin Jacob
> Kollanukkaran <jerinj@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 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.

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
  

Hi Jerin,

> >
> > 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?

> 
> 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
  

> -----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?


> 
> >
> > 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
  

> -----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
  

> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Thursday, August 8, 2019 4:04 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
> 
> ----------------------------------------------------------------------
> 
> 
> > -----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 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

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 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.
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)

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?

> 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
  

08/08/2019 12:59, Jerin Jacob Kollanukkaran:
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > From: Jerin Jacob Kollanukkaran [mailto:jerinj@marvell.com]
> > > From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > > > > > > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > > > > 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
> 
> 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.

Just a note here: I am clearly against negative flags.
We recently cleaned up the flags to all be positive.
  

> > > > > > > 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)
  

> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Thursday, August 8, 2019 10:24 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
> > > > 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?

I agree. We don't need to.

> 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).

Yes

> 
> > 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)


The API looks good to me. We need to document the  rte_eth_dev_set_supported_ptypes()
must  be called when device is in stop state to allow PMD do slow path configuration.

To summarize:
Two options to control PTYPE lookup:
Option 1:
- If DEV_RX_OFFLOAD_PTYPE set, PMD returns mbuf->packet_type with rte_eth_dev_get_supported_ptypes()
- If DEV_RX_OFFLOAD_PTYPE is not set, PMD still can return  mbuf->packet_type with rte_eth_dev_get_supported_ptypes()
But if PMD can do some optimization, it can avoid ptype lookup and return mbuf->packet_type as zero.

Option 2:
- Introduce rte_eth_dev_set_supported_ptypes(port, needed_ptypes).

I think, rte_eth_dev_set_supported_ptypes() is better option As Konstantain suggested to
have selective control of ptype parsing by PMD at the cost of adding new API.

I think, we can avoid breaking exiting application by, If rte_eth_dev_set_supported_ptypes() is not called,
PMD must return mbuf->packet_type with rte_eth_dev_get_supported_ptypes().
If rte_eth_dev_set_supported_ptypes() and successful, PMD must return
mbuf->packet_type with rte_eth_dev_set_supported_ptypes()

If there no objection to this API, We can send updated deprecation notice.

>
  

> 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>

Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
  

Hi Jerin,

> > > > > 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?
> 
> I agree. We don't need to.
> 
> > 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).
> 
> Yes
> 
> >
> > > 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)
> 
> 
> The API looks good to me. We need to document the  rte_eth_dev_set_supported_ptypes()
> must  be called when device is in stop state to allow PMD do slow path configuration.
> 
> To summarize:
> Two options to control PTYPE lookup:
> Option 1:
> - If DEV_RX_OFFLOAD_PTYPE set, PMD returns mbuf->packet_type with rte_eth_dev_get_supported_ptypes()
> - If DEV_RX_OFFLOAD_PTYPE is not set, PMD still can return  mbuf->packet_type with rte_eth_dev_get_supported_ptypes()
> But if PMD can do some optimization, it can avoid ptype lookup and return mbuf->packet_type as zero.
> 
> Option 2:
> - Introduce rte_eth_dev_set_supported_ptypes(port, needed_ptypes).

Yes.

> 
> I think, rte_eth_dev_set_supported_ptypes() is better option As Konstantain suggested to
> have selective control of ptype parsing by PMD at the cost of adding new API.
> 
> I think, we can avoid breaking exiting application by, If rte_eth_dev_set_supported_ptypes() is not called,
> PMD must return mbuf->packet_type with rte_eth_dev_get_supported_ptypes().
> If rte_eth_dev_set_supported_ptypes() and successful, PMD must return
> mbuf->packet_type with rte_eth_dev_set_supported_ptypes()

+1

> 
> If there no objection to this API, We can send updated deprecation notice.
> 

None from me.
Konstantin