diff mbox series

[v2,1/4] ethdev: add Rx offload outer UDP checksum definition

Message ID 20181002192451.19119-1-jerin.jacob@caviumnetworks.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series [v2,1/4] ethdev: add Rx offload outer UDP checksum definition |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Jerin Jacob Oct. 2, 2018, 7:24 p.m. UTC 
  Introduced DEV_RX_OFFLOAD_OUTER_UDP_CKSUM Rx offload flag and
PKT_RX_EL4_CKSUM_BAD mbuf ol_flags to detect outer UDP checksum
failure.

- To use hardware Rx outer UDP checksum offload, the user needs to
configure DEV_RX_OFFLOAD_OUTER_UDP_CKSUM offload flags in slowpath.

- Driver updates the PKT_RX_EL4_CKSUM_BAD mbuf ol_flag on checksum failure
similar to the outer L3 PKT_RX_EIP_CKSUM_BAD flag.

Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
---

v2:
- Removed DEV_RX_OFFLOAD_OUTER_TCP_CKSUM and DEV_RX_OFFLOAD_OUTER_SCTP_CKSUM
as there is no realworld use case for it.
See: http://patches.dpdk.org/patch/44692/

This patch series is depended on http://patches.dpdk.org/patch/45840/

--
 app/test-pmd/config.c          | 9 +++++++++
 doc/guides/nics/features.rst   | 3 +++
 lib/librte_ethdev/rte_ethdev.c | 1 +
 lib/librte_ethdev/rte_ethdev.h | 1 +
 lib/librte_mbuf/rte_mbuf.c     | 2 ++
 lib/librte_mbuf/rte_mbuf.h     | 3 +++
 6 files changed, 19 insertions(+)

Comments

Andrew Rybchenko Oct. 3, 2018, 7:34 a.m. UTC | #1 
On 10/2/18 10:24 PM, Jerin Jacob wrote:
> Introduced DEV_RX_OFFLOAD_OUTER_UDP_CKSUM Rx offload flag and
> PKT_RX_EL4_CKSUM_BAD mbuf ol_flags to detect outer UDP checksum
> failure.
>
> - To use hardware Rx outer UDP checksum offload, the user needs to
> configure DEV_RX_OFFLOAD_OUTER_UDP_CKSUM offload flags in slowpath.
>
> - Driver updates the PKT_RX_EL4_CKSUM_BAD mbuf ol_flag on checksum failure
> similar to the outer L3 PKT_RX_EIP_CKSUM_BAD flag.
>
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>

1. I'm not sure that it is OK that mbuf and ethdev changes go in one patch.
     It seems typically mbuf changes go separately and mbuf changes should
     be applied to main dpdk repo.

2. I'd like to see thought why single bit is used for outer L2 checksum when
     2 bits (UNKNOWN, BAD, GOOD, NONE) are used for PKT_RX_L4_CKSUM.
     May be it is OK, but it would be useful to state explicitly why it 
is decided
     to go this way.

3. PKT_RX_L4_CKSUM_MASK description says nothing if it is inner or outer.
     May be it is not directly related to changeset, but I think it 
would be really
     useful to clarify it.


Plus one nit below.

> ---
>
> v2:
> - Removed DEV_RX_OFFLOAD_OUTER_TCP_CKSUM and DEV_RX_OFFLOAD_OUTER_SCTP_CKSUM
> as there is no realworld use case for it.
> See: http://patches.dpdk.org/patch/44692/
>
> This patch series is depended on http://patches.dpdk.org/patch/45840/
>
> --
>   app/test-pmd/config.c          | 9 +++++++++
>   doc/guides/nics/features.rst   | 3 +++
>   lib/librte_ethdev/rte_ethdev.c | 1 +
>   lib/librte_ethdev/rte_ethdev.h | 1 +
>   lib/librte_mbuf/rte_mbuf.c     | 2 ++
>   lib/librte_mbuf/rte_mbuf.h     | 3 +++
>   6 files changed, 19 insertions(+)
>
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 1adc9b94b..d53c527e5 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -594,6 +594,15 @@ port_offload_cap_display(portid_t port_id)
>   			printf("off\n");
>   	}
>   
> +	if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_OUTER_UDP_CKSUM) {
> +		printf("RX Outer UDP checksum:               ");
> +		if (ports[port_id].dev_conf.rxmode.offloads &
> +		    DEV_RX_OFFLOAD_OUTER_UDP_CKSUM)
> +			printf("on\n");
> +		else
> +			printf("off\n");
> +	}
> +
>   	if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_TCP_LRO) {
>   		printf("Large receive offload:         ");
>   		if (ports[port_id].dev_conf.rxmode.offloads &
> diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
> index d42489b6d..2c2959e0b 100644
> --- a/doc/guides/nics/features.rst
> +++ b/doc/guides/nics/features.rst
> @@ -639,6 +639,9 @@ Inner L4 checksum
>   
>   Supports inner packet L4 checksum.
>   
> +* **[uses]     rte_eth_rxconf,rte_eth_rxmode**: ``offloads:DEV_RX_OFFLOAD_OUTER_UDP_CKSUM``.
> +* **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_EL4_CKSUM_BAD``.
> +* **[provides] rte_eth_dev_info**: ``rx_offload_capa,rx_queue_offload_capa:DEV_RX_OFFLOAD_OUTER_UDP_CKSUM``,
>   

One more empty line should be added here to have two empty lines between 
features.

Andrew.
Jerin Jacob Oct. 3, 2018, 7:57 a.m. UTC | #2 
-----Original Message-----
> Date: Wed, 3 Oct 2018 10:34:52 +0300
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, Wenzhuo Lu
>  <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>, Bernard
>  Iremonger <bernard.iremonger@intel.com>, John McNamara
>  <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
>  Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
>  <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>
> CC: dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
>  <konstantin.ananyev@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>  checksum definition
> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
>  Thunderbird/60.0
> 
> 
> On 10/2/18 10:24 PM, Jerin Jacob wrote:
> 
> Introduced DEV_RX_OFFLOAD_OUTER_UDP_CKSUM Rx offload flag and
> PKT_RX_EL4_CKSUM_BAD mbuf ol_flags to detect outer UDP checksum
> failure.
> 
> - To use hardware Rx outer UDP checksum offload, the user needs to
> configure DEV_RX_OFFLOAD_OUTER_UDP_CKSUM offload flags in slowpath.
> 
> - Driver updates the PKT_RX_EL4_CKSUM_BAD mbuf ol_flag on checksum failure
> similar to the outer L3 PKT_RX_EIP_CKSUM_BAD flag.
> 
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com><mailto:jerin.jacob@caviumnetworks.com>
> 
> 1. I'm not sure that it is OK that mbuf and ethdev changes go in one patch.
>    It seems typically mbuf changes go separately and mbuf changes should
>    be applied to main dpdk repo.


I don't have strong opinion on this. If there are no other objection, I
will split the patch further as mbuf and ethdev as you pointed out.

> 
> 2. I'd like to see thought why single bit is used for outer L2 checksum when
>    2 bits (UNKNOWN, BAD, GOOD, NONE) are used for PKT_RX_L4_CKSUM.
>    May be it is OK, but it would be useful to state explicitly why it is decided
>    to go this way.

I am following the scheme similar to OUTER IP checksum where we have only
one bit filed(PKT_RX_EIP_CKSUM_BAD). I will mention in the git commit.


> 
> 3. PKT_RX_L4_CKSUM_MASK description says nothing if it is inner or outer.
>    May be it is not directly related to changeset, but I think it would be really
>    useful to clarify it.

I will update the comment.

> 
> 
> Plus one nit below.
> 
> 
> 
> ---
> 
> v2:
> - Removed DEV_RX_OFFLOAD_OUTER_TCP_CKSUM and DEV_RX_OFFLOAD_OUTER_SCTP_CKSUM
> as there is no realworld use case for it.
> See: http://patches.dpdk.org/patch/44692/
> 
> This patch series is depended on http://patches.dpdk.org/patch/45840/
> 
> --
> app/test-pmd/config.c          | 9 +++++++++
> doc/guides/nics/features.rst   | 3 +++
> lib/librte_ethdev/rte_ethdev.c | 1 +
> lib/librte_ethdev/rte_ethdev.h | 1 +
> lib/librte_mbuf/rte_mbuf.c     | 2 ++
> lib/librte_mbuf/rte_mbuf.h     | 3 +++
> 6 files changed, 19 insertions(+)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 1adc9b94b..d53c527e5 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -594,6 +594,15 @@ port_offload_cap_display(portid_t port_id)
>                        printf("off\n");
>        }
> 
> +       if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_OUTER_UDP_CKSUM) {
> +               printf("RX Outer UDP checksum:               ");
> +               if (ports[port_id].dev_conf.rxmode.offloads &
> +                   DEV_RX_OFFLOAD_OUTER_UDP_CKSUM)
> +                       printf("on\n");
> +               else
> +                       printf("off\n");
> +       }
> +
>        if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_TCP_LRO) {
>                printf("Large receive offload:         ");
>                if (ports[port_id].dev_conf.rxmode.offloads &
> diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
> index d42489b6d..2c2959e0b 100644
> --- a/doc/guides/nics/features.rst
> +++ b/doc/guides/nics/features.rst
> @@ -639,6 +639,9 @@ Inner L4 checksum
> 
> Supports inner packet L4 checksum.
> 
> +* **[uses]     rte_eth_rxconf,rte_eth_rxmode**: ``offloads:DEV_RX_OFFLOAD_OUTER_UDP_CKSUM``.
> +* **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_EL4_CKSUM_BAD``.
> +* **[provides] rte_eth_dev_info**: ``rx_offload_capa,rx_queue_offload_capa:DEV_RX_OFFLOAD_OUTER_UDP_CKSUM``,
> 
> 
> One more empty line should be added here to have two empty lines between features.

OK.

Thanks for the review.

> 
> Andrew.
Thomas Monjalon Oct. 3, 2018, 8:35 a.m. UTC | #3 
03/10/2018 09:57, Jerin Jacob:
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> > 1. I'm not sure that it is OK that mbuf and ethdev changes go in one patch.
> >    It seems typically mbuf changes go separately and mbuf changes should
> >    be applied to main dpdk repo.
> 
> I don't have strong opinion on this. If there are no other objection, I
> will split the patch further as mbuf and ethdev as you pointed out.

Those flags are handled in mbuf and ethdev.
As it is closely related, I think it is better to get the changes
in one patch, as you did.
Andrew Rybchenko Oct. 3, 2018, 8:36 a.m. UTC | #4 
On 10/3/18 11:35 AM, Thomas Monjalon wrote:
> 03/10/2018 09:57, Jerin Jacob:
>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>>> 1. I'm not sure that it is OK that mbuf and ethdev changes go in one patch.
>>>     It seems typically mbuf changes go separately and mbuf changes should
>>>     be applied to main dpdk repo.
>> I don't have strong opinion on this. If there are no other objection, I
>> will split the patch further as mbuf and ethdev as you pointed out.
> Those flags are handled in mbuf and ethdev.
> As it is closely related, I think it is better to get the changes
> in one patch, as you did.

OK, thanks for clarification.
Ananyev, Konstantin Oct. 3, 2018, 8:53 a.m. UTC | #5 
Hi Jerin,

> 
> Introduced DEV_RX_OFFLOAD_OUTER_UDP_CKSUM Rx offload flag and
> PKT_RX_EL4_CKSUM_BAD mbuf ol_flags to detect outer UDP checksum
> failure.
> 
> - To use hardware Rx outer UDP checksum offload, the user needs to
> configure DEV_RX_OFFLOAD_OUTER_UDP_CKSUM offload flags in slowpath.
> 
> - Driver updates the PKT_RX_EL4_CKSUM_BAD mbuf ol_flag on checksum failure
> similar to the outer L3 PKT_RX_EIP_CKSUM_BAD flag.
> 
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>

Looks ok to me in general.
Just wonder is there any PMD that supports all these new features?
Konstantin
Jerin Jacob Oct. 3, 2018, 8:59 a.m. UTC | #6 
-----Original Message-----
> Date: Wed, 3 Oct 2018 08:53:22 +0000
> From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, "Lu, Wenzhuo"
>  <wenzhuo.lu@intel.com>, "Wu, Jingjing" <jingjing.wu@intel.com>,
>  "Iremonger, Bernard" <bernard.iremonger@intel.com>, "Mcnamara, John"
>  <john.mcnamara@intel.com>, "Kovacevic, Marko" <marko.kovacevic@intel.com>,
>  Thomas Monjalon <thomas@monjalon.net>, "Yigit, Ferruh"
>  <ferruh.yigit@intel.com>, Andrew Rybchenko <arybchenko@solarflare.com>,
>  Olivier Matz <olivier.matz@6wind.com>
> CC: "dev@dpdk.org" <dev@dpdk.org>, "shahafs@mellanox.com"
>  <shahafs@mellanox.com>
> Subject: RE: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>  checksum	definition
> 
> 
> Hi Jerin,

Hi Konstantin,

> 
> >
> > Introduced DEV_RX_OFFLOAD_OUTER_UDP_CKSUM Rx offload flag and
> > PKT_RX_EL4_CKSUM_BAD mbuf ol_flags to detect outer UDP checksum
> > failure.
> >
> > - To use hardware Rx outer UDP checksum offload, the user needs to
> > configure DEV_RX_OFFLOAD_OUTER_UDP_CKSUM offload flags in slowpath.
> >
> > - Driver updates the PKT_RX_EL4_CKSUM_BAD mbuf ol_flag on checksum failure
> > similar to the outer L3 PKT_RX_EIP_CKSUM_BAD flag.
> >
> > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> 
> Looks ok to me in general.
> Just wonder is there any PMD that supports all these new features?

octeontx2 PMD has this feature. I am planning to push the PMD for v19.02.
Before that I adding all the common code change to avoid the dependency.

> Konstantin
Ananyev, Konstantin Oct. 3, 2018, 9:17 a.m. UTC | #7 
> -----Original Message-----
> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> Sent: Wednesday, October 3, 2018 9:59 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Iremonger, Bernard <bernard.iremonger@intel.com>;
> Mcnamara, John <john.mcnamara@intel.com>; Kovacevic, Marko <marko.kovacevic@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>; Olivier Matz
> <olivier.matz@6wind.com>; dev@dpdk.org; shahafs@mellanox.com
> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP checksum definition
> 
> -----Original Message-----
> > Date: Wed, 3 Oct 2018 08:53:22 +0000
> > From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
> > To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, "Lu, Wenzhuo"
> >  <wenzhuo.lu@intel.com>, "Wu, Jingjing" <jingjing.wu@intel.com>,
> >  "Iremonger, Bernard" <bernard.iremonger@intel.com>, "Mcnamara, John"
> >  <john.mcnamara@intel.com>, "Kovacevic, Marko" <marko.kovacevic@intel.com>,
> >  Thomas Monjalon <thomas@monjalon.net>, "Yigit, Ferruh"
> >  <ferruh.yigit@intel.com>, Andrew Rybchenko <arybchenko@solarflare.com>,
> >  Olivier Matz <olivier.matz@6wind.com>
> > CC: "dev@dpdk.org" <dev@dpdk.org>, "shahafs@mellanox.com"
> >  <shahafs@mellanox.com>
> > Subject: RE: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
> >  checksum	definition
> >
> >
> > Hi Jerin,
> 
> Hi Konstantin,
> 
> >
> > >
> > > Introduced DEV_RX_OFFLOAD_OUTER_UDP_CKSUM Rx offload flag and
> > > PKT_RX_EL4_CKSUM_BAD mbuf ol_flags to detect outer UDP checksum
> > > failure.
> > >
> > > - To use hardware Rx outer UDP checksum offload, the user needs to
> > > configure DEV_RX_OFFLOAD_OUTER_UDP_CKSUM offload flags in slowpath.
> > >
> > > - Driver updates the PKT_RX_EL4_CKSUM_BAD mbuf ol_flag on checksum failure
> > > similar to the outer L3 PKT_RX_EIP_CKSUM_BAD flag.
> > >
> > > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> >
> > Looks ok to me in general.
> > Just wonder is there any PMD that supports all these new features?
> 
> octeontx2 PMD has this feature. I am planning to push the PMD for v19.02.
> Before that I adding all the common code change to avoid the dependency.

Ok, but why then ethdev/mbuf changes has to go into 18.11?
Do you plan your new PMD backward compatible with 18.11 LTS?
Konstantin
Jerin Jacob Oct. 3, 2018, 9:22 a.m. UTC | #8 
-----Original Message-----
> Date: Wed, 3 Oct 2018 09:17:18 +0000
> From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: "Lu, Wenzhuo" <wenzhuo.lu@intel.com>, "Wu, Jingjing"
>  <jingjing.wu@intel.com>, "Iremonger, Bernard"
>  <bernard.iremonger@intel.com>, "Mcnamara, John" <john.mcnamara@intel.com>,
>  "Kovacevic, Marko" <marko.kovacevic@intel.com>, Thomas Monjalon
>  <thomas@monjalon.net>, "Yigit, Ferruh" <ferruh.yigit@intel.com>, Andrew
>  Rybchenko <arybchenko@solarflare.com>, Olivier Matz
>  <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>,
>  "shahafs@mellanox.com" <shahafs@mellanox.com>
> Subject: RE: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>  checksum	definition
> 
> 
> > -----Original Message-----
> > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > Sent: Wednesday, October 3, 2018 9:59 AM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Iremonger, Bernard <bernard.iremonger@intel.com>;
> > Mcnamara, John <john.mcnamara@intel.com>; Kovacevic, Marko <marko.kovacevic@intel.com>; Thomas Monjalon
> > <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>; Olivier Matz
> > <olivier.matz@6wind.com>; dev@dpdk.org; shahafs@mellanox.com
> > Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP checksum definition
> >
> > -----Original Message-----
> > > Date: Wed, 3 Oct 2018 08:53:22 +0000
> > > From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
> > > To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, "Lu, Wenzhuo"
> > >  <wenzhuo.lu@intel.com>, "Wu, Jingjing" <jingjing.wu@intel.com>,
> > >  "Iremonger, Bernard" <bernard.iremonger@intel.com>, "Mcnamara, John"
> > >  <john.mcnamara@intel.com>, "Kovacevic, Marko" <marko.kovacevic@intel.com>,
> > >  Thomas Monjalon <thomas@monjalon.net>, "Yigit, Ferruh"
> > >  <ferruh.yigit@intel.com>, Andrew Rybchenko <arybchenko@solarflare.com>,
> > >  Olivier Matz <olivier.matz@6wind.com>
> > > CC: "dev@dpdk.org" <dev@dpdk.org>, "shahafs@mellanox.com"
> > >  <shahafs@mellanox.com>
> > > Subject: RE: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
> > >  checksum   definition
> > >
> > >
> > > Hi Jerin,
> >
> > Hi Konstantin,
> >
> > >
> > > >
> > > > Introduced DEV_RX_OFFLOAD_OUTER_UDP_CKSUM Rx offload flag and
> > > > PKT_RX_EL4_CKSUM_BAD mbuf ol_flags to detect outer UDP checksum
> > > > failure.
> > > >
> > > > - To use hardware Rx outer UDP checksum offload, the user needs to
> > > > configure DEV_RX_OFFLOAD_OUTER_UDP_CKSUM offload flags in slowpath.
> > > >
> > > > - Driver updates the PKT_RX_EL4_CKSUM_BAD mbuf ol_flag on checksum failure
> > > > similar to the outer L3 PKT_RX_EIP_CKSUM_BAD flag.
> > > >
> > > > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > >
> > > Looks ok to me in general.
> > > Just wonder is there any PMD that supports all these new features?
> >
> > octeontx2 PMD has this feature. I am planning to push the PMD for v19.02.
> > Before that I adding all the common code change to avoid the dependency.
> 
> Ok, but why then ethdev/mbuf changes has to go into 18.11?

It it is a generic change then why not? What is the real concern here?

> Do you plan your new PMD backward compatible with 18.11 LTS?

Yes.

> Konstantin
Ananyev, Konstantin Oct. 3, 2018, 10:16 a.m. UTC | #9 
> -----Original Message-----
> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> Sent: Wednesday, October 3, 2018 10:22 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Iremonger, Bernard <bernard.iremonger@intel.com>;
> Mcnamara, John <john.mcnamara@intel.com>; Kovacevic, Marko <marko.kovacevic@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>; Olivier Matz
> <olivier.matz@6wind.com>; dev@dpdk.org; shahafs@mellanox.com
> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP checksum definition
> 
> -----Original Message-----
> > Date: Wed, 3 Oct 2018 09:17:18 +0000
> > From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
> > To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > CC: "Lu, Wenzhuo" <wenzhuo.lu@intel.com>, "Wu, Jingjing"
> >  <jingjing.wu@intel.com>, "Iremonger, Bernard"
> >  <bernard.iremonger@intel.com>, "Mcnamara, John" <john.mcnamara@intel.com>,
> >  "Kovacevic, Marko" <marko.kovacevic@intel.com>, Thomas Monjalon
> >  <thomas@monjalon.net>, "Yigit, Ferruh" <ferruh.yigit@intel.com>, Andrew
> >  Rybchenko <arybchenko@solarflare.com>, Olivier Matz
> >  <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>,
> >  "shahafs@mellanox.com" <shahafs@mellanox.com>
> > Subject: RE: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
> >  checksum	definition
> >
> >
> > > -----Original Message-----
> > > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > > Sent: Wednesday, October 3, 2018 9:59 AM
> > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>;
> > > Mcnamara, John <john.mcnamara@intel.com>; Kovacevic, Marko <marko.kovacevic@intel.com>; Thomas Monjalon
> > > <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>; Olivier Matz
> > > <olivier.matz@6wind.com>; dev@dpdk.org; shahafs@mellanox.com
> > > Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP checksum definition
> > >
> > > -----Original Message-----
> > > > Date: Wed, 3 Oct 2018 08:53:22 +0000
> > > > From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
> > > > To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, "Lu, Wenzhuo"
> > > >  <wenzhuo.lu@intel.com>, "Wu, Jingjing" <jingjing.wu@intel.com>,
> > > >  "Iremonger, Bernard" <bernard.iremonger@intel.com>, "Mcnamara, John"
> > > >  <john.mcnamara@intel.com>, "Kovacevic, Marko" <marko.kovacevic@intel.com>,
> > > >  Thomas Monjalon <thomas@monjalon.net>, "Yigit, Ferruh"
> > > >  <ferruh.yigit@intel.com>, Andrew Rybchenko <arybchenko@solarflare.com>,
> > > >  Olivier Matz <olivier.matz@6wind.com>
> > > > CC: "dev@dpdk.org" <dev@dpdk.org>, "shahafs@mellanox.com"
> > > >  <shahafs@mellanox.com>
> > > > Subject: RE: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
> > > >  checksum   definition
> > > >
> > > >
> > > > Hi Jerin,
> > >
> > > Hi Konstantin,
> > >
> > > >
> > > > >
> > > > > Introduced DEV_RX_OFFLOAD_OUTER_UDP_CKSUM Rx offload flag and
> > > > > PKT_RX_EL4_CKSUM_BAD mbuf ol_flags to detect outer UDP checksum
> > > > > failure.
> > > > >
> > > > > - To use hardware Rx outer UDP checksum offload, the user needs to
> > > > > configure DEV_RX_OFFLOAD_OUTER_UDP_CKSUM offload flags in slowpath.
> > > > >
> > > > > - Driver updates the PKT_RX_EL4_CKSUM_BAD mbuf ol_flag on checksum failure
> > > > > similar to the outer L3 PKT_RX_EIP_CKSUM_BAD flag.
> > > > >
> > > > > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > >
> > > > Looks ok to me in general.
> > > > Just wonder is there any PMD that supports all these new features?
> > >
> > > octeontx2 PMD has this feature. I am planning to push the PMD for v19.02.
> > > Before that I adding all the common code change to avoid the dependency.
> >
> > Ok, but why then ethdev/mbuf changes has to go into 18.11?
> 
> It it is a generic change then why not? What is the real concern here?

If there is no implementation for it, how we can conclude that it is really 'generic' one? :)
My main concern is that we already have several features in rte_ethdev and rte_security
that supposed to be 'generic' but right now no-one support them. 
I wouldn't to object about these particular features, they look reasonable to me.
But in general I think we need some better defined policy -
about what is allowed to propagate without real evidence (particular implementation)
and what is not.

> 
> > Do you plan your new PMD backward compatible with 18.11 LTS?
> 
> Yes.

I see.
Konstantin
Iremonger, Bernard Oct. 3, 2018, 10:51 a.m. UTC | #10 
Hi Jerin,

> -----Original Message-----
> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> Sent: Tuesday, October 2, 2018 8:25 PM
> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Iremonger, Bernard <bernard.iremonger@intel.com>;
> Mcnamara, John <john.mcnamara@intel.com>; Kovacevic, Marko
> <marko.kovacevic@intel.com>; Thomas Monjalon <thomas@monjalon.net>;
> Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>; Olivier Matz <olivier.matz@6wind.com>
> Cc: dev@dpdk.org; shahafs@mellanox.com; Jerin Jacob
> <jerin.jacob@caviumnetworks.com>
> Subject: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP checksum
> definition
> 
> Introduced DEV_RX_OFFLOAD_OUTER_UDP_CKSUM Rx offload flag and
> PKT_RX_EL4_CKSUM_BAD mbuf ol_flags to detect outer UDP checksum failure.
> 
> - To use hardware Rx outer UDP checksum offload, the user needs to configure
> DEV_RX_OFFLOAD_OUTER_UDP_CKSUM offload flags in slowpath.
> 
> - Driver updates the PKT_RX_EL4_CKSUM_BAD mbuf ol_flag on checksum failure
> similar to the outer L3 PKT_RX_EIP_CKSUM_BAD flag.
> 
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> ---
> 
> v2:
> - Removed DEV_RX_OFFLOAD_OUTER_TCP_CKSUM and
> DEV_RX_OFFLOAD_OUTER_SCTP_CKSUM as there is no realworld use case for
> it.
> See: http://patches.dpdk.org/patch/44692/
> 
> This patch series is depended on http://patches.dpdk.org/patch/45840/
> 
> --
>  app/test-pmd/config.c          | 9 +++++++++
>  doc/guides/nics/features.rst   | 3 +++
>  lib/librte_ethdev/rte_ethdev.c | 1 +
>  lib/librte_ethdev/rte_ethdev.h | 1 +
>  lib/librte_mbuf/rte_mbuf.c     | 2 ++
>  lib/librte_mbuf/rte_mbuf.h     | 3 +++
>  6 files changed, 19 insertions(+)
> 

<snip>

This patch fails to apply to the latest master branch, a rebase may be needed.

Regards,

Bernard.
Jerin Jacob Oct. 3, 2018, 11:15 a.m. UTC | #11 
-----Original Message-----
> Date: Wed, 3 Oct 2018 10:16:31 +0000
> From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: "Lu, Wenzhuo" <wenzhuo.lu@intel.com>, "Wu, Jingjing"
>  <jingjing.wu@intel.com>, "Iremonger, Bernard"
>  <bernard.iremonger@intel.com>, "Mcnamara, John" <john.mcnamara@intel.com>,
>  "Kovacevic, Marko" <marko.kovacevic@intel.com>, Thomas Monjalon
>  <thomas@monjalon.net>, "Yigit, Ferruh" <ferruh.yigit@intel.com>, Andrew
>  Rybchenko <arybchenko@solarflare.com>, Olivier Matz
>  <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>,
>  "shahafs@mellanox.com" <shahafs@mellanox.com>
> Subject: RE: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>  checksum	definition
> 
> > -----Original Message-----
> > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > Sent: Wednesday, October 3, 2018 10:22 AM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Iremonger, Bernard <bernard.iremonger@intel.com>;
> > Mcnamara, John <john.mcnamara@intel.com>; Kovacevic, Marko <marko.kovacevic@intel.com>; Thomas Monjalon
> > <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>; Olivier Matz
> > <olivier.matz@6wind.com>; dev@dpdk.org; shahafs@mellanox.com
> > Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP checksum definition
> >
> > -----Original Message-----
> > > Date: Wed, 3 Oct 2018 09:17:18 +0000
> > > From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
> > > To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > CC: "Lu, Wenzhuo" <wenzhuo.lu@intel.com>, "Wu, Jingjing"
> > >  <jingjing.wu@intel.com>, "Iremonger, Bernard"
> > >  <bernard.iremonger@intel.com>, "Mcnamara, John" <john.mcnamara@intel.com>,
> > >  "Kovacevic, Marko" <marko.kovacevic@intel.com>, Thomas Monjalon
> > >  <thomas@monjalon.net>, "Yigit, Ferruh" <ferruh.yigit@intel.com>, Andrew
> > >  Rybchenko <arybchenko@solarflare.com>, Olivier Matz
> > >  <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>,
> > >  "shahafs@mellanox.com" <shahafs@mellanox.com>
> > > Subject: RE: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
> > >  checksum   definition
> > >
> > >
> > > > -----Original Message-----
> > > > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > > > Sent: Wednesday, October 3, 2018 9:59 AM
> > > > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Iremonger, Bernard
> > <bernard.iremonger@intel.com>;
> > > > Mcnamara, John <john.mcnamara@intel.com>; Kovacevic, Marko <marko.kovacevic@intel.com>; Thomas Monjalon
> > > > <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>; Olivier Matz
> > > > <olivier.matz@6wind.com>; dev@dpdk.org; shahafs@mellanox.com
> > > > Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP checksum definition
> > > >
> > > > -----Original Message-----
> > > > > Date: Wed, 3 Oct 2018 08:53:22 +0000
> > > > > From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
> > > > > To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, "Lu, Wenzhuo"
> > > > >  <wenzhuo.lu@intel.com>, "Wu, Jingjing" <jingjing.wu@intel.com>,
> > > > >  "Iremonger, Bernard" <bernard.iremonger@intel.com>, "Mcnamara, John"
> > > > >  <john.mcnamara@intel.com>, "Kovacevic, Marko" <marko.kovacevic@intel.com>,
> > > > >  Thomas Monjalon <thomas@monjalon.net>, "Yigit, Ferruh"
> > > > >  <ferruh.yigit@intel.com>, Andrew Rybchenko <arybchenko@solarflare.com>,
> > > > >  Olivier Matz <olivier.matz@6wind.com>
> > > > > CC: "dev@dpdk.org" <dev@dpdk.org>, "shahafs@mellanox.com"
> > > > >  <shahafs@mellanox.com>
> > > > > Subject: RE: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
> > > > >  checksum   definition
> > > > >
> > > > >
> > > > > Hi Jerin,
> > > >
> > > > Hi Konstantin,
> > > >
> > > > >
> > > > > >
> > > > > > Introduced DEV_RX_OFFLOAD_OUTER_UDP_CKSUM Rx offload flag and
> > > > > > PKT_RX_EL4_CKSUM_BAD mbuf ol_flags to detect outer UDP checksum
> > > > > > failure.
> > > > > >
> > > > > > - To use hardware Rx outer UDP checksum offload, the user needs to
> > > > > > configure DEV_RX_OFFLOAD_OUTER_UDP_CKSUM offload flags in slowpath.
> > > > > >
> > > > > > - Driver updates the PKT_RX_EL4_CKSUM_BAD mbuf ol_flag on checksum failure
> > > > > > similar to the outer L3 PKT_RX_EIP_CKSUM_BAD flag.
> > > > > >
> > > > > > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > > >
> > > > > Looks ok to me in general.
> > > > > Just wonder is there any PMD that supports all these new features?
> > > >
> > > > octeontx2 PMD has this feature. I am planning to push the PMD for v19.02.
> > > > Before that I adding all the common code change to avoid the dependency.
> > >
> > > Ok, but why then ethdev/mbuf changes has to go into 18.11?
> >
> > It it is a generic change then why not? What is the real concern here?
> 
> If there is no implementation for it, how we can conclude that it is really 'generic' one? :)
> My main concern is that we already have several features in rte_ethdev and rte_security
> that supposed to be 'generic' but right now no-one support them.

I think, this case is pretty straight forward changes.

> I wouldn't to object about these particular features, they look reasonable to me.

OK.

> But in general I think we need some better defined policy -
> about what is allowed to propagate without real evidence (particular implementation)
> and what is not.

I am open for the policy definition.

> 
> >
> > > Do you plan your new PMD backward compatible with 18.11 LTS?
> >
> > Yes.
> 
> I see.
> Konstantin
>
Jerin Jacob Oct. 3, 2018, 11:19 a.m. UTC | #12 
-----Original Message-----
> Date: Wed, 3 Oct 2018 10:51:36 +0000
> From: "Iremonger, Bernard" <bernard.iremonger@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, "Lu, Wenzhuo"
>  <wenzhuo.lu@intel.com>, "Wu, Jingjing" <jingjing.wu@intel.com>, "Mcnamara,
>  John" <john.mcnamara@intel.com>, "Kovacevic, Marko"
>  <marko.kovacevic@intel.com>, Thomas Monjalon <thomas@monjalon.net>,
>  "Yigit, Ferruh" <ferruh.yigit@intel.com>, Andrew Rybchenko
>  <arybchenko@solarflare.com>, Olivier Matz <olivier.matz@6wind.com>
> CC: "dev@dpdk.org" <dev@dpdk.org>, "shahafs@mellanox.com"
>  <shahafs@mellanox.com>
> Subject: RE: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>  checksum definition
> 
> 
> Hi Jerin,

Hi Iremonger,

> 
> > -----Original Message-----
> > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > Sent: Tuesday, October 2, 2018 8:25 PM
> > To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing
> > <jingjing.wu@intel.com>; Iremonger, Bernard <bernard.iremonger@intel.com>;
> > Mcnamara, John <john.mcnamara@intel.com>; Kovacevic, Marko
> > <marko.kovacevic@intel.com>; Thomas Monjalon <thomas@monjalon.net>;
> > Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko
> > <arybchenko@solarflare.com>; Olivier Matz <olivier.matz@6wind.com>
> > Cc: dev@dpdk.org; shahafs@mellanox.com; Jerin Jacob
> > <jerin.jacob@caviumnetworks.com>
> > Subject: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP checksum
> > definition
> >
> > Introduced DEV_RX_OFFLOAD_OUTER_UDP_CKSUM Rx offload flag and
> > PKT_RX_EL4_CKSUM_BAD mbuf ol_flags to detect outer UDP checksum failure.
> >
> > - To use hardware Rx outer UDP checksum offload, the user needs to configure
> > DEV_RX_OFFLOAD_OUTER_UDP_CKSUM offload flags in slowpath.
> >
> > - Driver updates the PKT_RX_EL4_CKSUM_BAD mbuf ol_flag on checksum failure
> > similar to the outer L3 PKT_RX_EIP_CKSUM_BAD flag.
> >
> > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > ---
> >
> > v2:
> > - Removed DEV_RX_OFFLOAD_OUTER_TCP_CKSUM and
> > DEV_RX_OFFLOAD_OUTER_SCTP_CKSUM as there is no realworld use case for
> > it.
> > See: http://patches.dpdk.org/patch/44692/
> >
> > This patch series is depended on http://patches.dpdk.org/patch/45840/
> >
> > --
> >  app/test-pmd/config.c          | 9 +++++++++
> >  doc/guides/nics/features.rst   | 3 +++
> >  lib/librte_ethdev/rte_ethdev.c | 1 +
> >  lib/librte_ethdev/rte_ethdev.h | 1 +
> >  lib/librte_mbuf/rte_mbuf.c     | 2 ++
> >  lib/librte_mbuf/rte_mbuf.h     | 3 +++
> >  6 files changed, 19 insertions(+)
> >
> 
> <snip>
> 
> This patch fails to apply to the latest master branch, a rebase may be needed.


Have you applies the depended series http://patches.dpdk.org/patch/45840/
as mentioned above? If yes and then still has issue then let me know.




> Regards,
> 
> Bernard.
Iremonger, Bernard Oct. 3, 2018, 1 p.m. UTC | #13 
Hi Jerin,

<snip>

> > > Subject: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
> > > checksum definition
> > >
> > > Introduced DEV_RX_OFFLOAD_OUTER_UDP_CKSUM Rx offload flag and
> > > PKT_RX_EL4_CKSUM_BAD mbuf ol_flags to detect outer UDP checksum
> failure.
> > >
> > > - To use hardware Rx outer UDP checksum offload, the user needs to
> > > configure DEV_RX_OFFLOAD_OUTER_UDP_CKSUM offload flags in
> slowpath.
> > >
> > > - Driver updates the PKT_RX_EL4_CKSUM_BAD mbuf ol_flag on checksum
> > > failure similar to the outer L3 PKT_RX_EIP_CKSUM_BAD flag.
> > >
> > > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > ---
> > >
> > > v2:
> > > - Removed DEV_RX_OFFLOAD_OUTER_TCP_CKSUM and
> > > DEV_RX_OFFLOAD_OUTER_SCTP_CKSUM as there is no realworld use case
> > > for it.
> > > See: http://patches.dpdk.org/patch/44692/
> > >
> > > This patch series is depended on
> > > http://patches.dpdk.org/patch/45840/
> > >
> > > --
> > >  app/test-pmd/config.c          | 9 +++++++++
> > >  doc/guides/nics/features.rst   | 3 +++
> > >  lib/librte_ethdev/rte_ethdev.c | 1 +
> > > lib/librte_ethdev/rte_ethdev.h | 1 +
> > >  lib/librte_mbuf/rte_mbuf.c     | 2 ++
> > >  lib/librte_mbuf/rte_mbuf.h     | 3 +++
> > >  6 files changed, 19 insertions(+)
> > >
> >
> > <snip>
> >
> > This patch fails to apply to the latest master branch, a rebase may be needed.
> 
> 
> Have you applies the depended series http://patches.dpdk.org/patch/45840/
> as mentioned above? If yes and then still has issue then let me know.
> 

The  patches apply and build fine with the above patch applied first.

Regards,

Bernard.
Jerin Jacob Oct. 3, 2018, 5:12 p.m. UTC | #14 
-----Original Message-----
> Date: Wed, 3 Oct 2018 13:27:13 +0530
> From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> To: Andrew Rybchenko <arybchenko@solarflare.com>
> CC: Wenzhuo Lu <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>,
>  Bernard Iremonger <bernard.iremonger@intel.com>, John McNamara
>  <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
>  Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
>  <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>,
>  dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
>  <konstantin.ananyev@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>  checksum definition
> User-Agent: Mutt/1.10.1 (2018-07-13)
> 
> External Email
> 
> -----Original Message-----
> > Date: Wed, 3 Oct 2018 10:34:52 +0300
> > From: Andrew Rybchenko <arybchenko@solarflare.com>
> > To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, Wenzhuo Lu
> >  <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>, Bernard
> >  Iremonger <bernard.iremonger@intel.com>, John McNamara
> >  <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
> >  Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
> >  <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>
> > CC: dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
> >  <konstantin.ananyev@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
> >  checksum definition
> > User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
> >  Thunderbird/60.0
> >
> >
> > On 10/2/18 10:24 PM, Jerin Jacob wrote:
> >
> > Introduced DEV_RX_OFFLOAD_OUTER_UDP_CKSUM Rx offload flag and
> > PKT_RX_EL4_CKSUM_BAD mbuf ol_flags to detect outer UDP checksum
> > failure.
> >
> > - To use hardware Rx outer UDP checksum offload, the user needs to
> > configure DEV_RX_OFFLOAD_OUTER_UDP_CKSUM offload flags in slowpath.
> >
> > - Driver updates the PKT_RX_EL4_CKSUM_BAD mbuf ol_flag on checksum failure
> > similar to the outer L3 PKT_RX_EIP_CKSUM_BAD flag.
> >
> > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com><mailto:jerin.jacob@caviumnetworks.com>
> >
> > 1. I'm not sure that it is OK that mbuf and ethdev changes go in one patch.
> >    It seems typically mbuf changes go separately and mbuf changes should
> >    be applied to main dpdk repo.
> 
> 
> I don't have strong opinion on this. If there are no other objection, I
> will split the patch further as mbuf and ethdev as you pointed out.
> 
> >
> > 2. I'd like to see thought why single bit is used for outer L2 checksum when
> >    2 bits (UNKNOWN, BAD, GOOD, NONE) are used for PKT_RX_L4_CKSUM.
> >    May be it is OK, but it would be useful to state explicitly why it is decided
> >    to go this way.
> 
> I am following the scheme similar to OUTER IP checksum where we have only
> one bit filed(PKT_RX_EIP_CKSUM_BAD). I will mention in the git commit.
> 
> 
> >
> > 3. PKT_RX_L4_CKSUM_MASK description says nothing if it is inner or outer.
> >    May be it is not directly related to changeset, but I think it would be really
> >    useful to clarify it.
> 
> I will update the comment.

Hi Andrew,

I looked at the other definitions in mbuf.h, according the documentation,
If nothing is mentioned it is treated as inner if the packet is
tunneled else it is outer most. So I would like avoid confusion by
adding "inner" in the exiting PKT_RX_L4_CKSUM_MASK comment.
Technically it is not correct to say "inner" if the packet is not
tunneled. So I am untouching the exiting comment.


> 
> >
> >
> > Plus one nit below.
> >
> >
> >
> > ---
> >
> > v2:
> > - Removed DEV_RX_OFFLOAD_OUTER_TCP_CKSUM and DEV_RX_OFFLOAD_OUTER_SCTP_CKSUM
> > as there is no realworld use case for it.
> > See: http://patches.dpdk.org/patch/44692/
> >
> > This patch series is depended on http://patches.dpdk.org/patch/45840/
> >
> > --
> > app/test-pmd/config.c          | 9 +++++++++
> > doc/guides/nics/features.rst   | 3 +++
> > lib/librte_ethdev/rte_ethdev.c | 1 +
> > lib/librte_ethdev/rte_ethdev.h | 1 +
> > lib/librte_mbuf/rte_mbuf.c     | 2 ++
> > lib/librte_mbuf/rte_mbuf.h     | 3 +++
> > 6 files changed, 19 insertions(+)
> >
> > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> > index 1adc9b94b..d53c527e5 100644
> > --- a/app/test-pmd/config.c
> > +++ b/app/test-pmd/config.c
> > @@ -594,6 +594,15 @@ port_offload_cap_display(portid_t port_id)
> >                        printf("off\n");
> >        }
> >
> > +       if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_OUTER_UDP_CKSUM) {
> > +               printf("RX Outer UDP checksum:               ");
> > +               if (ports[port_id].dev_conf.rxmode.offloads &
> > +                   DEV_RX_OFFLOAD_OUTER_UDP_CKSUM)
> > +                       printf("on\n");
> > +               else
> > +                       printf("off\n");
> > +       }
> > +
> >        if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_TCP_LRO) {
> >                printf("Large receive offload:         ");
> >                if (ports[port_id].dev_conf.rxmode.offloads &
> > diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
> > index d42489b6d..2c2959e0b 100644
> > --- a/doc/guides/nics/features.rst
> > +++ b/doc/guides/nics/features.rst
> > @@ -639,6 +639,9 @@ Inner L4 checksum
> >
> > Supports inner packet L4 checksum.
> >
> > +* **[uses]     rte_eth_rxconf,rte_eth_rxmode**: ``offloads:DEV_RX_OFFLOAD_OUTER_UDP_CKSUM``.
> > +* **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_EL4_CKSUM_BAD``.
> > +* **[provides] rte_eth_dev_info**: ``rx_offload_capa,rx_queue_offload_capa:DEV_RX_OFFLOAD_OUTER_UDP_CKSUM``,
> >
> >
> > One more empty line should be added here to have two empty lines between features.
> 
> OK.
> 
> Thanks for the review.
> 
> >
> > Andrew.
Andrew Rybchenko Oct. 3, 2018, 6 p.m. UTC | #15 
On 03.10.2018 20:12, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Wed, 3 Oct 2018 13:27:13 +0530
>> From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>> To: Andrew Rybchenko <arybchenko@solarflare.com>
>> CC: Wenzhuo Lu <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>,
>>   Bernard Iremonger <bernard.iremonger@intel.com>, John McNamara
>>   <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
>>   Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
>>   <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>,
>>   dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
>>   <konstantin.ananyev@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>>   checksum definition
>> User-Agent: Mutt/1.10.1 (2018-07-13)
>>
>> External Email
>>
>> -----Original Message-----
>>> Date: Wed, 3 Oct 2018 10:34:52 +0300
>>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, Wenzhuo Lu
>>>   <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>, Bernard
>>>   Iremonger <bernard.iremonger@intel.com>, John McNamara
>>>   <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
>>>   Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
>>>   <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>
>>> CC: dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
>>>   <konstantin.ananyev@intel.com>
>>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>>>   checksum definition
>>> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
>>>   Thunderbird/60.0
>>>
>>>
>>> On 10/2/18 10:24 PM, Jerin Jacob wrote:
>>>
>>> Introduced DEV_RX_OFFLOAD_OUTER_UDP_CKSUM Rx offload flag and
>>> PKT_RX_EL4_CKSUM_BAD mbuf ol_flags to detect outer UDP checksum
>>> failure.
>>>
>>> - To use hardware Rx outer UDP checksum offload, the user needs to
>>> configure DEV_RX_OFFLOAD_OUTER_UDP_CKSUM offload flags in slowpath.
>>>
>>> - Driver updates the PKT_RX_EL4_CKSUM_BAD mbuf ol_flag on checksum failure
>>> similar to the outer L3 PKT_RX_EIP_CKSUM_BAD flag.
>>>
>>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com><mailto:jerin.jacob@caviumnetworks.com>
>>>
>>> 1. I'm not sure that it is OK that mbuf and ethdev changes go in one patch.
>>>     It seems typically mbuf changes go separately and mbuf changes should
>>>     be applied to main dpdk repo.
>>
>> I don't have strong opinion on this. If there are no other objection, I
>> will split the patch further as mbuf and ethdev as you pointed out.
>>
>>> 2. I'd like to see thought why single bit is used for outer L2 checksum when
>>>     2 bits (UNKNOWN, BAD, GOOD, NONE) are used for PKT_RX_L4_CKSUM.
>>>     May be it is OK, but it would be useful to state explicitly why it is decided
>>>     to go this way.
>> I am following the scheme similar to OUTER IP checksum where we have only
>> one bit filed(PKT_RX_EIP_CKSUM_BAD). I will mention in the git commit.
>>
>>
>>> 3. PKT_RX_L4_CKSUM_MASK description says nothing if it is inner or outer.
>>>     May be it is not directly related to changeset, but I think it would be really
>>>     useful to clarify it.
>> I will update the comment.
> Hi Andrew,
>
> I looked at the other definitions in mbuf.h, according the documentation,
> If nothing is mentioned it is treated as inner if the packet is
> tunneled else it is outer most. So I would like avoid confusion by
> adding "inner" in the exiting PKT_RX_L4_CKSUM_MASK comment.
> Technically it is not correct to say "inner" if the packet is not
> tunneled. So I am untouching the exiting comment.
>

Yes, it is incorrect to say that it is inner. How does application find
how to treat PKT_RX_L4_CKSUM (inner or outer)?
Should it rely on packet type provided in mbuf?
Is it specified/mentioned somewhere?

Andrew.
Jerin Jacob Oct. 3, 2018, 6:14 p.m. UTC | #16 
-----Original Message-----
> Date: Wed, 3 Oct 2018 21:00:37 +0300
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: Wenzhuo Lu <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>,
>  Bernard Iremonger <bernard.iremonger@intel.com>, John McNamara
>  <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
>  Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
>  <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>,
>  dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
>  <konstantin.ananyev@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>  checksum definition
> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
>  Thunderbird/52.9.1
> 
> On 03.10.2018 20:12, Jerin Jacob wrote:
> > -----Original Message-----
> > > Date: Wed, 3 Oct 2018 13:27:13 +0530
> > > From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > To: Andrew Rybchenko <arybchenko@solarflare.com>
> > > CC: Wenzhuo Lu <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>,
> > >   Bernard Iremonger <bernard.iremonger@intel.com>, John McNamara
> > >   <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
> > >   Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
> > >   <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>,
> > >   dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
> > >   <konstantin.ananyev@intel.com>
> > > Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
> > >   checksum definition
> > > User-Agent: Mutt/1.10.1 (2018-07-13)
> > > 
> > > External Email
> > > 
> > > -----Original Message-----
> > > > Date: Wed, 3 Oct 2018 10:34:52 +0300
> > > > From: Andrew Rybchenko <arybchenko@solarflare.com>
> > > > To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, Wenzhuo Lu
> > > >   <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>, Bernard
> > > >   Iremonger <bernard.iremonger@intel.com>, John McNamara
> > > >   <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
> > > >   Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
> > > >   <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>
> > > > CC: dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
> > > >   <konstantin.ananyev@intel.com>
> > > > Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
> > > >   checksum definition
> > > > User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
> > > >   Thunderbird/60.0
> > > > 
> > > > 
> > > > On 10/2/18 10:24 PM, Jerin Jacob wrote:
> > > > 
> > > > Introduced DEV_RX_OFFLOAD_OUTER_UDP_CKSUM Rx offload flag and
> > > > PKT_RX_EL4_CKSUM_BAD mbuf ol_flags to detect outer UDP checksum
> > > > failure.
> > > > 
> > > > - To use hardware Rx outer UDP checksum offload, the user needs to
> > > > configure DEV_RX_OFFLOAD_OUTER_UDP_CKSUM offload flags in slowpath.
> > > > 
> > > > - Driver updates the PKT_RX_EL4_CKSUM_BAD mbuf ol_flag on checksum failure
> > > > similar to the outer L3 PKT_RX_EIP_CKSUM_BAD flag.
> > > > 
> > > > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com><mailto:jerin.jacob@caviumnetworks.com>
> > > > 
> > > > 1. I'm not sure that it is OK that mbuf and ethdev changes go in one patch.
> > > >     It seems typically mbuf changes go separately and mbuf changes should
> > > >     be applied to main dpdk repo.
> > > 
> > > I don't have strong opinion on this. If there are no other objection, I
> > > will split the patch further as mbuf and ethdev as you pointed out.
> > > 
> > > > 2. I'd like to see thought why single bit is used for outer L2 checksum when
> > > >     2 bits (UNKNOWN, BAD, GOOD, NONE) are used for PKT_RX_L4_CKSUM.
> > > >     May be it is OK, but it would be useful to state explicitly why it is decided
> > > >     to go this way.
> > > I am following the scheme similar to OUTER IP checksum where we have only
> > > one bit filed(PKT_RX_EIP_CKSUM_BAD). I will mention in the git commit.
> > > 
> > > 
> > > > 3. PKT_RX_L4_CKSUM_MASK description says nothing if it is inner or outer.
> > > >     May be it is not directly related to changeset, but I think it would be really
> > > >     useful to clarify it.
> > > I will update the comment.
> > Hi Andrew,
> > 
> > I looked at the other definitions in mbuf.h, according the documentation,
> > If nothing is mentioned it is treated as inner if the packet is
> > tunneled else it is outer most. So I would like avoid confusion by
> > adding "inner" in the exiting PKT_RX_L4_CKSUM_MASK comment.
> > Technically it is not correct to say "inner" if the packet is not
> > tunneled. So I am untouching the exiting comment.
> > 
> 
> Yes, it is incorrect to say that it is inner. How does application find
> how to treat PKT_RX_L4_CKSUM (inner or outer)?
> Should it rely on packet type provided in mbuf?

AFAIK, Finding is it a tunneled packet or not is through ptype or SW has
to parse the packet. For example, testpmd chooses later method using
"csum parse-tunnel on <port>" to detect the presence of the tunnel.

> Is it specified/mentioned somewhere?

I don't know. It it not directly related to this change set, Olivier may know
additional details.

> 
> Andrew.
Andrew Rybchenko Oct. 3, 2018, 7:47 p.m. UTC | #17 
Hi Jerin,

On 03.10.2018 21:14, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Wed, 3 Oct 2018 21:00:37 +0300
>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>> CC: Wenzhuo Lu <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>,
>>   Bernard Iremonger <bernard.iremonger@intel.com>, John McNamara
>>   <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
>>   Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
>>   <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>,
>>   dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
>>   <konstantin.ananyev@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>>   checksum definition
>> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
>>   Thunderbird/52.9.1
>>
>> On 03.10.2018 20:12, Jerin Jacob wrote:
>>> -----Original Message-----
>>>> Date: Wed, 3 Oct 2018 13:27:13 +0530
>>>> From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>>> To: Andrew Rybchenko <arybchenko@solarflare.com>
>>>> CC: Wenzhuo Lu <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>,
>>>>    Bernard Iremonger <bernard.iremonger@intel.com>, John McNamara
>>>>    <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
>>>>    Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
>>>>    <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>,
>>>>    dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
>>>>    <konstantin.ananyev@intel.com>
>>>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>>>>    checksum definition
>>>> User-Agent: Mutt/1.10.1 (2018-07-13)
>>>>
>>>> External Email
>>>>
>>>> -----Original Message-----
>>>>> Date: Wed, 3 Oct 2018 10:34:52 +0300
>>>>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>>>>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, Wenzhuo Lu
>>>>>    <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>, Bernard
>>>>>    Iremonger <bernard.iremonger@intel.com>, John McNamara
>>>>>    <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
>>>>>    Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
>>>>>    <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>
>>>>> CC: dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
>>>>>    <konstantin.ananyev@intel.com>
>>>>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>>>>>    checksum definition
>>>>> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
>>>>>    Thunderbird/60.0
>>>>>
>>>>>
>>>>> On 10/2/18 10:24 PM, Jerin Jacob wrote:
>>>>>
>>>>> Introduced DEV_RX_OFFLOAD_OUTER_UDP_CKSUM Rx offload flag and
>>>>> PKT_RX_EL4_CKSUM_BAD mbuf ol_flags to detect outer UDP checksum
>>>>> failure.
>>>>>
>>>>> - To use hardware Rx outer UDP checksum offload, the user needs to
>>>>> configure DEV_RX_OFFLOAD_OUTER_UDP_CKSUM offload flags in slowpath.
>>>>>
>>>>> - Driver updates the PKT_RX_EL4_CKSUM_BAD mbuf ol_flag on checksum failure
>>>>> similar to the outer L3 PKT_RX_EIP_CKSUM_BAD flag.
>>>>>
>>>>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com><mailto:jerin.jacob@caviumnetworks.com>
>>>>>
>>>>> 1. I'm not sure that it is OK that mbuf and ethdev changes go in one patch.
>>>>>      It seems typically mbuf changes go separately and mbuf changes should
>>>>>      be applied to main dpdk repo.
>>>> I don't have strong opinion on this. If there are no other objection, I
>>>> will split the patch further as mbuf and ethdev as you pointed out.
>>>>
>>>>> 2. I'd like to see thought why single bit is used for outer L2 checksum when
>>>>>      2 bits (UNKNOWN, BAD, GOOD, NONE) are used for PKT_RX_L4_CKSUM.
>>>>>      May be it is OK, but it would be useful to state explicitly why it is decided
>>>>>      to go this way.
>>>> I am following the scheme similar to OUTER IP checksum where we have only
>>>> one bit filed(PKT_RX_EIP_CKSUM_BAD). I will mention in the git commit.
>>>>
>>>>
>>>>> 3. PKT_RX_L4_CKSUM_MASK description says nothing if it is inner or outer.
>>>>>      May be it is not directly related to changeset, but I think it would be really
>>>>>      useful to clarify it.
>>>> I will update the comment.
>>> Hi Andrew,
>>>
>>> I looked at the other definitions in mbuf.h, according the documentation,
>>> If nothing is mentioned it is treated as inner if the packet is
>>> tunneled else it is outer most. So I would like avoid confusion by
>>> adding "inner" in the exiting PKT_RX_L4_CKSUM_MASK comment.
>>> Technically it is not correct to say "inner" if the packet is not
>>> tunneled. So I am untouching the exiting comment.
>>>
>> Yes, it is incorrect to say that it is inner. How does application find
>> how to treat PKT_RX_L4_CKSUM (inner or outer)?
>> Should it rely on packet type provided in mbuf?
> AFAIK, Finding is it a tunneled packet or not is through ptype or SW has
> to parse the packet. For example, testpmd chooses later method using
> "csum parse-tunnel on <port>" to detect the presence of the tunnel.

SW parsing of the packet cannot help, since app should be sure
that HW has classified the packet as tunneled and provided information
about inner and outer checksum checks.

>> Is it specified/mentioned somewhere?
> I don't know. It it not directly related to this change set, Olivier may know
> additional details.

I disagree. You're adding one more offload flag. Yes, it simply follows
existing RX_EIP_CKSUM_BAD pattern. But, IMHO, RX_EIP_CKSUM_BAD
has many open questions. Why should these open questions be preserved
here? It is similar to the code with a bug which is cloned once again with
the bug :)

If everyone else is fine with the description of Rx checksum offloads and
it is only me who is unhappy with it - no problem.

Thanks for your patience and I'm sorry that I'm really boring with it.
My goal is just to make it clear and as the result have less bugs in
networking PMDs and applications.

Andrew.
Thomas Monjalon Oct. 3, 2018, 8:08 p.m. UTC | #18 
03/10/2018 21:47, Andrew Rybchenko:
> On 03.10.2018 21:14, Jerin Jacob wrote:
> > From: Andrew Rybchenko <arybchenko@solarflare.com>
> >>> From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> >>>> From: Andrew Rybchenko <arybchenko@solarflare.com>
> >>>>> On 10/2/18 10:24 PM, Jerin Jacob wrote:
> >>>>>
> >>>>> Introduced DEV_RX_OFFLOAD_OUTER_UDP_CKSUM Rx offload flag and
> >>>>> PKT_RX_EL4_CKSUM_BAD mbuf ol_flags to detect outer UDP checksum
> >>>>> failure.
> >>>>>
> >>>>> - To use hardware Rx outer UDP checksum offload, the user needs to
> >>>>> configure DEV_RX_OFFLOAD_OUTER_UDP_CKSUM offload flags in slowpath.
> >>>>>
> >>>>> - Driver updates the PKT_RX_EL4_CKSUM_BAD mbuf ol_flag on checksum failure
> >>>>> similar to the outer L3 PKT_RX_EIP_CKSUM_BAD flag.
> >>>>>
> >>>>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com><mailto:jerin.jacob@caviumnetworks.com>
> >>>>>
> >>>>> 1. I'm not sure that it is OK that mbuf and ethdev changes go in one patch.
> >>>>>      It seems typically mbuf changes go separately and mbuf changes should
> >>>>>      be applied to main dpdk repo.
> >>>> I don't have strong opinion on this. If there are no other objection, I
> >>>> will split the patch further as mbuf and ethdev as you pointed out.
> >>>>
> >>>>> 2. I'd like to see thought why single bit is used for outer L2 checksum when
> >>>>>      2 bits (UNKNOWN, BAD, GOOD, NONE) are used for PKT_RX_L4_CKSUM.
> >>>>>      May be it is OK, but it would be useful to state explicitly why it is decided
> >>>>>      to go this way.
> >>>> I am following the scheme similar to OUTER IP checksum where we have only
> >>>> one bit filed(PKT_RX_EIP_CKSUM_BAD). I will mention in the git commit.
> >>>>
> >>>>
> >>>>> 3. PKT_RX_L4_CKSUM_MASK description says nothing if it is inner or outer.
> >>>>>      May be it is not directly related to changeset, but I think it would be really
> >>>>>      useful to clarify it.
> >>>> I will update the comment.
> >>> Hi Andrew,
> >>>
> >>> I looked at the other definitions in mbuf.h, according the documentation,
> >>> If nothing is mentioned it is treated as inner if the packet is
> >>> tunneled else it is outer most. So I would like avoid confusion by
> >>> adding "inner" in the exiting PKT_RX_L4_CKSUM_MASK comment.
> >>> Technically it is not correct to say "inner" if the packet is not
> >>> tunneled. So I am untouching the exiting comment.
> >>>
> >> Yes, it is incorrect to say that it is inner. How does application find
> >> how to treat PKT_RX_L4_CKSUM (inner or outer)?
> >> Should it rely on packet type provided in mbuf?
> > AFAIK, Finding is it a tunneled packet or not is through ptype or SW has
> > to parse the packet. For example, testpmd chooses later method using
> > "csum parse-tunnel on <port>" to detect the presence of the tunnel.
> 
> SW parsing of the packet cannot help, since app should be sure
> that HW has classified the packet as tunneled and provided information
> about inner and outer checksum checks.
> 
> >> Is it specified/mentioned somewhere?
> > I don't know. It it not directly related to this change set, Olivier may know
> > additional details.
> 
> I disagree. You're adding one more offload flag. Yes, it simply follows
> existing RX_EIP_CKSUM_BAD pattern. But, IMHO, RX_EIP_CKSUM_BAD
> has many open questions. Why should these open questions be preserved
> here? It is similar to the code with a bug which is cloned once again with
> the bug :)
> 
> If everyone else is fine with the description of Rx checksum offloads and
> it is only me who is unhappy with it - no problem.

I agree we must better define these checksum flags.

Olivier, please could you give your opinion here?
Jerin Jacob Oct. 4, 2018, 5:59 a.m. UTC | #19 
-----Original Message-----
> Date: Wed, 3 Oct 2018 22:47:15 +0300
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: Wenzhuo Lu <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>,
>  Bernard Iremonger <bernard.iremonger@intel.com>, John McNamara
>  <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
>  Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
>  <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>,
>  dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
>  <konstantin.ananyev@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>  checksum definition
> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
>  Thunderbird/52.9.1
> 
> 
> Hi Jerin,

Hi Andrew,

> 
> On 03.10.2018 21:14, Jerin Jacob wrote:
> > -----Original Message-----
> > > Date: Wed, 3 Oct 2018 21:00:37 +0300
> > > From: Andrew Rybchenko <arybchenko@solarflare.com>
> > > To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > CC: Wenzhuo Lu <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>,
> > >   Bernard Iremonger <bernard.iremonger@intel.com>, John McNamara
> > >   <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
> > >   Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
> > >   <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>,
> > >   dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
> > >   <konstantin.ananyev@intel.com>
> > > Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
> > >   checksum definition
> > > User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
> > >   Thunderbird/52.9.1
> > > 
> > > On 03.10.2018 20:12, Jerin Jacob wrote:
> > > > -----Original Message-----
> > > > > Date: Wed, 3 Oct 2018 13:27:13 +0530
> > > > > From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > > > To: Andrew Rybchenko <arybchenko@solarflare.com>
> > > > > CC: Wenzhuo Lu <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>,
> > > > >    Bernard Iremonger <bernard.iremonger@intel.com>, John McNamara
> > > > >    <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
> > > > >    Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
> > > > >    <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>,
> > > > >    dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
> > > > >    <konstantin.ananyev@intel.com>
> > > > > Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
> > > > >    checksum definition
> > > > > User-Agent: Mutt/1.10.1 (2018-07-13)
> > > > > 
> > > > > External Email
> > > > > 
> > > > > -----Original Message-----
> > > > > > Date: Wed, 3 Oct 2018 10:34:52 +0300
> > > > > > From: Andrew Rybchenko <arybchenko@solarflare.com>
> > > > > > To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, Wenzhuo Lu
> > > > > >    <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>, Bernard
> > > > > >    Iremonger <bernard.iremonger@intel.com>, John McNamara
> > > > > >    <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
> > > > > >    Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
> > > > > >    <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>
> > > > > > CC: dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
> > > > > >    <konstantin.ananyev@intel.com>
> > > > > > Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
> > > > > >    checksum definition
> > > > > > User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
> > > > > >    Thunderbird/60.0
> > > > > > 
> > > > > > 
> > > > > > On 10/2/18 10:24 PM, Jerin Jacob wrote:
> > > > > > 
> > > > > > Introduced DEV_RX_OFFLOAD_OUTER_UDP_CKSUM Rx offload flag and
> > > > > > PKT_RX_EL4_CKSUM_BAD mbuf ol_flags to detect outer UDP checksum
> > > > > > failure.
> > > > > > 
> > > > > > - To use hardware Rx outer UDP checksum offload, the user needs to
> > > > > > configure DEV_RX_OFFLOAD_OUTER_UDP_CKSUM offload flags in slowpath.
> > > > > > 
> > > > > > - Driver updates the PKT_RX_EL4_CKSUM_BAD mbuf ol_flag on checksum failure
> > > > > > similar to the outer L3 PKT_RX_EIP_CKSUM_BAD flag.
> > > > > > 
> > > > > > Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com><mailto:jerin.jacob@caviumnetworks.com>
> > > > > > 
> > > > > > 1. I'm not sure that it is OK that mbuf and ethdev changes go in one patch.
> > > > > >      It seems typically mbuf changes go separately and mbuf changes should
> > > > > >      be applied to main dpdk repo.
> > > > > I don't have strong opinion on this. If there are no other objection, I
> > > > > will split the patch further as mbuf and ethdev as you pointed out.
> > > > > 
> > > > > > 2. I'd like to see thought why single bit is used for outer L2 checksum when
> > > > > >      2 bits (UNKNOWN, BAD, GOOD, NONE) are used for PKT_RX_L4_CKSUM.
> > > > > >      May be it is OK, but it would be useful to state explicitly why it is decided
> > > > > >      to go this way.
> > > > > I am following the scheme similar to OUTER IP checksum where we have only
> > > > > one bit filed(PKT_RX_EIP_CKSUM_BAD). I will mention in the git commit.
> > > > > 
> > > > > 
> > > > > > 3. PKT_RX_L4_CKSUM_MASK description says nothing if it is inner or outer.
> > > > > >      May be it is not directly related to changeset, but I think it would be really
> > > > > >      useful to clarify it.
> > > > > I will update the comment.
> > > > Hi Andrew,
> > > > 
> > > > I looked at the other definitions in mbuf.h, according the documentation,
> > > > If nothing is mentioned it is treated as inner if the packet is
> > > > tunneled else it is outer most. So I would like avoid confusion by
> > > > adding "inner" in the exiting PKT_RX_L4_CKSUM_MASK comment.
> > > > Technically it is not correct to say "inner" if the packet is not
> > > > tunneled. So I am untouching the exiting comment.
> > > > 
> > > Yes, it is incorrect to say that it is inner. How does application find
> > > how to treat PKT_RX_L4_CKSUM (inner or outer)?
> > > Should it rely on packet type provided in mbuf?
> > AFAIK, Finding is it a tunneled packet or not is through ptype or SW has
> > to parse the packet. For example, testpmd chooses later method using
> > "csum parse-tunnel on <port>" to detect the presence of the tunnel.
> 
> SW parsing of the packet cannot help, since app should be sure
> that HW has classified the packet as tunneled and provided information
> about inner and outer checksum checks.


I thought the question was, How does the application find how to treat
PKT_RX_L4_CKSUM (inner or outer)?
Obviously, ptype will help here
Not sure why SW parsing won't help here if SW parses and find it is an
inner packet or it is not a tunneled packet. PKT_RX_L4_CKSUM treat as
inner checksum for former case and PKT_RX_L4_CKSUM treated as plain l4
checksum in the latter case.

> 
> > > Is it specified/mentioned somewhere?
> > I don't know. It it not directly related to this change set, Olivier may know
> > additional details.
> 
> I disagree. You're adding one more offload flag. Yes, it simply follows
> existing RX_EIP_CKSUM_BAD pattern. But, IMHO, RX_EIP_CKSUM_BAD
> has many open questions. Why should these open questions be preserved
> here? It is similar to the code with a bug which is cloned once again with
> the bug :)


No disagreement here. I choose to follow the existing scheme, because if I
do another way around, still I will get the question on why it is different
than the outer IPV4 checksum scheme.

Looking at the history, the mbuf DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM change
went part of ixgbe change
d909af8f72ca ("ixgbe: offload VxLAN and NVGRE Rx checksum on X550").

Al east in the HW I know, We can support "two bit" fields for Outer IP
checksum and Outer L4 checksum.

So I think, the decision was made based on ixgbe capability or probably
no one noticed the change as the subject was ixgbe:

Anyway, i don't have strong preferences on 1 bit vs 2 bits. I think, 1
bit can be supported by all the HW if supports this feature. Leaving the
decision to ethdev and mbuf maintainers.

> 
> If everyone else is fine with the description of Rx checksum offloads and
> it is only me who is unhappy with it - no problem.
> 
> Thanks for your patience and I'm sorry that I'm really boring with it.
> My goal is just to make it clear and as the result have less bugs in
> networking PMDs and applications.

No problem. :-)


> 
> Andrew.
>
Ferruh Yigit Oct. 5, 2018, 7:48 p.m. UTC | #20 
On 10/4/2018 6:59 AM, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Wed, 3 Oct 2018 22:47:15 +0300
>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>> CC: Wenzhuo Lu <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>,
>>  Bernard Iremonger <bernard.iremonger@intel.com>, John McNamara
>>  <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
>>  Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
>>  <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>,
>>  dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
>>  <konstantin.ananyev@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>>  checksum definition
>> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
>>  Thunderbird/52.9.1
>>
>>
>> Hi Jerin,
> 
> Hi Andrew,
> 
>>
>> On 03.10.2018 21:14, Jerin Jacob wrote:
>>> -----Original Message-----
>>>> Date: Wed, 3 Oct 2018 21:00:37 +0300
>>>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>>>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>>> CC: Wenzhuo Lu <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>,
>>>>   Bernard Iremonger <bernard.iremonger@intel.com>, John McNamara
>>>>   <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
>>>>   Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
>>>>   <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>,
>>>>   dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
>>>>   <konstantin.ananyev@intel.com>
>>>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>>>>   checksum definition
>>>> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
>>>>   Thunderbird/52.9.1
>>>>
>>>> On 03.10.2018 20:12, Jerin Jacob wrote:
>>>>> -----Original Message-----
>>>>>> Date: Wed, 3 Oct 2018 13:27:13 +0530
>>>>>> From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>>>>> To: Andrew Rybchenko <arybchenko@solarflare.com>
>>>>>> CC: Wenzhuo Lu <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>,
>>>>>>    Bernard Iremonger <bernard.iremonger@intel.com>, John McNamara
>>>>>>    <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
>>>>>>    Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
>>>>>>    <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>,
>>>>>>    dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
>>>>>>    <konstantin.ananyev@intel.com>
>>>>>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>>>>>>    checksum definition
>>>>>> User-Agent: Mutt/1.10.1 (2018-07-13)
>>>>>>
>>>>>> External Email
>>>>>>
>>>>>> -----Original Message-----
>>>>>>> Date: Wed, 3 Oct 2018 10:34:52 +0300
>>>>>>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>>>>>>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, Wenzhuo Lu
>>>>>>>    <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>, Bernard
>>>>>>>    Iremonger <bernard.iremonger@intel.com>, John McNamara
>>>>>>>    <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
>>>>>>>    Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
>>>>>>>    <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>
>>>>>>> CC: dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
>>>>>>>    <konstantin.ananyev@intel.com>
>>>>>>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>>>>>>>    checksum definition
>>>>>>> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
>>>>>>>    Thunderbird/60.0
>>>>>>>
>>>>>>>
>>>>>>> On 10/2/18 10:24 PM, Jerin Jacob wrote:
>>>>>>>
>>>>>>> Introduced DEV_RX_OFFLOAD_OUTER_UDP_CKSUM Rx offload flag and
>>>>>>> PKT_RX_EL4_CKSUM_BAD mbuf ol_flags to detect outer UDP checksum
>>>>>>> failure.
>>>>>>>
>>>>>>> - To use hardware Rx outer UDP checksum offload, the user needs to
>>>>>>> configure DEV_RX_OFFLOAD_OUTER_UDP_CKSUM offload flags in slowpath.
>>>>>>>
>>>>>>> - Driver updates the PKT_RX_EL4_CKSUM_BAD mbuf ol_flag on checksum failure
>>>>>>> similar to the outer L3 PKT_RX_EIP_CKSUM_BAD flag.
>>>>>>>
>>>>>>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com><mailto:jerin.jacob@caviumnetworks.com>
>>>>>>>
>>>>>>> 1. I'm not sure that it is OK that mbuf and ethdev changes go in one patch.
>>>>>>>      It seems typically mbuf changes go separately and mbuf changes should
>>>>>>>      be applied to main dpdk repo.
>>>>>> I don't have strong opinion on this. If there are no other objection, I
>>>>>> will split the patch further as mbuf and ethdev as you pointed out.
>>>>>>
>>>>>>> 2. I'd like to see thought why single bit is used for outer L2 checksum when
>>>>>>>      2 bits (UNKNOWN, BAD, GOOD, NONE) are used for PKT_RX_L4_CKSUM.
>>>>>>>      May be it is OK, but it would be useful to state explicitly why it is decided
>>>>>>>      to go this way.
>>>>>> I am following the scheme similar to OUTER IP checksum where we have only
>>>>>> one bit filed(PKT_RX_EIP_CKSUM_BAD). I will mention in the git commit.
>>>>>>
>>>>>>
>>>>>>> 3. PKT_RX_L4_CKSUM_MASK description says nothing if it is inner or outer.
>>>>>>>      May be it is not directly related to changeset, but I think it would be really
>>>>>>>      useful to clarify it.
>>>>>> I will update the comment.
>>>>> Hi Andrew,
>>>>>
>>>>> I looked at the other definitions in mbuf.h, according the documentation,
>>>>> If nothing is mentioned it is treated as inner if the packet is
>>>>> tunneled else it is outer most. So I would like avoid confusion by
>>>>> adding "inner" in the exiting PKT_RX_L4_CKSUM_MASK comment.
>>>>> Technically it is not correct to say "inner" if the packet is not
>>>>> tunneled. So I am untouching the exiting comment.
>>>>>
>>>> Yes, it is incorrect to say that it is inner. How does application find
>>>> how to treat PKT_RX_L4_CKSUM (inner or outer)?
>>>> Should it rely on packet type provided in mbuf?
>>> AFAIK, Finding is it a tunneled packet or not is through ptype or SW has
>>> to parse the packet. For example, testpmd chooses later method using
>>> "csum parse-tunnel on <port>" to detect the presence of the tunnel.
>>
>> SW parsing of the packet cannot help, since app should be sure
>> that HW has classified the packet as tunneled and provided information
>> about inner and outer checksum checks.
> 
> 
> I thought the question was, How does the application find how to treat
> PKT_RX_L4_CKSUM (inner or outer)?
> Obviously, ptype will help here
> Not sure why SW parsing won't help here if SW parses and find it is an
> inner packet or it is not a tunneled packet. PKT_RX_L4_CKSUM treat as
> inner checksum for former case and PKT_RX_L4_CKSUM treated as plain l4
> checksum in the latter case.
> 
>>
>>>> Is it specified/mentioned somewhere?
>>> I don't know. It it not directly related to this change set, Olivier may know
>>> additional details.
>>
>> I disagree. You're adding one more offload flag. Yes, it simply follows
>> existing RX_EIP_CKSUM_BAD pattern. But, IMHO, RX_EIP_CKSUM_BAD
>> has many open questions. Why should these open questions be preserved
>> here? It is similar to the code with a bug which is cloned once again with
>> the bug :)
> 
> 
> No disagreement here. I choose to follow the existing scheme, because if I
> do another way around, still I will get the question on why it is different
> than the outer IPV4 checksum scheme.
> 
> Looking at the history, the mbuf DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM change
> went part of ixgbe change
> d909af8f72ca ("ixgbe: offload VxLAN and NVGRE Rx checksum on X550").
> 
> Al east in the HW I know, We can support "two bit" fields for Outer IP
> checksum and Outer L4 checksum.
> 
> So I think, the decision was made based on ixgbe capability or probably
> no one noticed the change as the subject was ixgbe:
> 
> Anyway, i don't have strong preferences on 1 bit vs 2 bits. I think, 1
> bit can be supported by all the HW if supports this feature. Leaving the
> decision to ethdev and mbuf maintainers.

+1 to Andrew, only PKT_RX_EL4_CKSUM_BAD bit is not clear when it is not set.
PKT_RX_IP_CKSUM_* approach looks better.

And agreed PKT_RX_EIP_CKSUM_BAD has same problem.

> 
>>
>> If everyone else is fine with the description of Rx checksum offloads and
>> it is only me who is unhappy with it - no problem.
>>
>> Thanks for your patience and I'm sorry that I'm really boring with it.
>> My goal is just to make it clear and as the result have less bugs in
>> networking PMDs and applications.
> 
> No problem. :-)
> 
> 
>>
>> Andrew.
>>
Ferruh Yigit Oct. 5, 2018, 8:04 p.m. UTC | #21 
On 10/4/2018 6:59 AM, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Wed, 3 Oct 2018 22:47:15 +0300
>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>> CC: Wenzhuo Lu <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>,
>>  Bernard Iremonger <bernard.iremonger@intel.com>, John McNamara
>>  <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
>>  Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
>>  <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>,
>>  dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
>>  <konstantin.ananyev@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>>  checksum definition
>> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
>>  Thunderbird/52.9.1
>>
>>
>> Hi Jerin,
> 
> Hi Andrew,
> 
>>
>> On 03.10.2018 21:14, Jerin Jacob wrote:
>>> -----Original Message-----
>>>> Date: Wed, 3 Oct 2018 21:00:37 +0300
>>>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>>>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>>> CC: Wenzhuo Lu <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>,
>>>>   Bernard Iremonger <bernard.iremonger@intel.com>, John McNamara
>>>>   <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
>>>>   Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
>>>>   <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>,
>>>>   dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
>>>>   <konstantin.ananyev@intel.com>
>>>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>>>>   checksum definition
>>>> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
>>>>   Thunderbird/52.9.1
>>>>
>>>> On 03.10.2018 20:12, Jerin Jacob wrote:
>>>>> -----Original Message-----
>>>>>> Date: Wed, 3 Oct 2018 13:27:13 +0530
>>>>>> From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>>>>> To: Andrew Rybchenko <arybchenko@solarflare.com>
>>>>>> CC: Wenzhuo Lu <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>,
>>>>>>    Bernard Iremonger <bernard.iremonger@intel.com>, John McNamara
>>>>>>    <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
>>>>>>    Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
>>>>>>    <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>,
>>>>>>    dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
>>>>>>    <konstantin.ananyev@intel.com>
>>>>>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>>>>>>    checksum definition
>>>>>> User-Agent: Mutt/1.10.1 (2018-07-13)
>>>>>>
>>>>>> External Email
>>>>>>
>>>>>> -----Original Message-----
>>>>>>> Date: Wed, 3 Oct 2018 10:34:52 +0300
>>>>>>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>>>>>>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, Wenzhuo Lu
>>>>>>>    <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>, Bernard
>>>>>>>    Iremonger <bernard.iremonger@intel.com>, John McNamara
>>>>>>>    <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
>>>>>>>    Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
>>>>>>>    <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>
>>>>>>> CC: dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
>>>>>>>    <konstantin.ananyev@intel.com>
>>>>>>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>>>>>>>    checksum definition
>>>>>>> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
>>>>>>>    Thunderbird/60.0
>>>>>>>
>>>>>>>
>>>>>>> On 10/2/18 10:24 PM, Jerin Jacob wrote:
>>>>>>>
>>>>>>> Introduced DEV_RX_OFFLOAD_OUTER_UDP_CKSUM Rx offload flag and
>>>>>>> PKT_RX_EL4_CKSUM_BAD mbuf ol_flags to detect outer UDP checksum
>>>>>>> failure.
>>>>>>>
>>>>>>> - To use hardware Rx outer UDP checksum offload, the user needs to
>>>>>>> configure DEV_RX_OFFLOAD_OUTER_UDP_CKSUM offload flags in slowpath.
>>>>>>>
>>>>>>> - Driver updates the PKT_RX_EL4_CKSUM_BAD mbuf ol_flag on checksum failure
>>>>>>> similar to the outer L3 PKT_RX_EIP_CKSUM_BAD flag.
>>>>>>>
>>>>>>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com><mailto:jerin.jacob@caviumnetworks.com>
>>>>>>>
>>>>>>> 1. I'm not sure that it is OK that mbuf and ethdev changes go in one patch.
>>>>>>>      It seems typically mbuf changes go separately and mbuf changes should
>>>>>>>      be applied to main dpdk repo.
>>>>>> I don't have strong opinion on this. If there are no other objection, I
>>>>>> will split the patch further as mbuf and ethdev as you pointed out.
>>>>>>
>>>>>>> 2. I'd like to see thought why single bit is used for outer L2 checksum when
>>>>>>>      2 bits (UNKNOWN, BAD, GOOD, NONE) are used for PKT_RX_L4_CKSUM.
>>>>>>>      May be it is OK, but it would be useful to state explicitly why it is decided
>>>>>>>      to go this way.
>>>>>> I am following the scheme similar to OUTER IP checksum where we have only
>>>>>> one bit filed(PKT_RX_EIP_CKSUM_BAD). I will mention in the git commit.
>>>>>>
>>>>>>
>>>>>>> 3. PKT_RX_L4_CKSUM_MASK description says nothing if it is inner or outer.
>>>>>>>      May be it is not directly related to changeset, but I think it would be really
>>>>>>>      useful to clarify it.
>>>>>> I will update the comment.
>>>>> Hi Andrew,
>>>>>
>>>>> I looked at the other definitions in mbuf.h, according the documentation,
>>>>> If nothing is mentioned it is treated as inner if the packet is
>>>>> tunneled else it is outer most. So I would like avoid confusion by
>>>>> adding "inner" in the exiting PKT_RX_L4_CKSUM_MASK comment.
>>>>> Technically it is not correct to say "inner" if the packet is not
>>>>> tunneled. So I am untouching the exiting comment.
>>>>>
>>>> Yes, it is incorrect to say that it is inner. How does application find
>>>> how to treat PKT_RX_L4_CKSUM (inner or outer)?
>>>> Should it rely on packet type provided in mbuf?
>>> AFAIK, Finding is it a tunneled packet or not is through ptype or SW has
>>> to parse the packet. For example, testpmd chooses later method using
>>> "csum parse-tunnel on <port>" to detect the presence of the tunnel.
>>
>> SW parsing of the packet cannot help, since app should be sure
>> that HW has classified the packet as tunneled and provided information
>> about inner and outer checksum checks.
> 
> 
> I thought the question was, How does the application find how to treat
> PKT_RX_L4_CKSUM (inner or outer)?
> Obviously, ptype will help here
> Not sure why SW parsing won't help here if SW parses and find it is an
> inner packet or it is not a tunneled packet. PKT_RX_L4_CKSUM treat as
> inner checksum for former case and PKT_RX_L4_CKSUM treated as plain l4
> checksum in the latter case.

I don't know if PKT_RX_L4_CKSUM is inner or outer with current design,
but wouldn't be easier if PKT_RX_L4_CKSUM always refer to outer and if tunneled
use something like PKT_RX_INNER_L4_CKSUM, same for IP and TX.

> 
>>
>>>> Is it specified/mentioned somewhere?
>>> I don't know. It it not directly related to this change set, Olivier may know
>>> additional details.
>>
>> I disagree. You're adding one more offload flag. Yes, it simply follows
>> existing RX_EIP_CKSUM_BAD pattern. But, IMHO, RX_EIP_CKSUM_BAD
>> has many open questions. Why should these open questions be preserved
>> here? It is similar to the code with a bug which is cloned once again with
>> the bug :)
> 
> 
> No disagreement here. I choose to follow the existing scheme, because if I
> do another way around, still I will get the question on why it is different
> than the outer IPV4 checksum scheme.
> 
> Looking at the history, the mbuf DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM change
> went part of ixgbe change
> d909af8f72ca ("ixgbe: offload VxLAN and NVGRE Rx checksum on X550").
> 
> Al east in the HW I know, We can support "two bit" fields for Outer IP
> checksum and Outer L4 checksum.
> 
> So I think, the decision was made based on ixgbe capability or probably
> no one noticed the change as the subject was ixgbe:
> 
> Anyway, i don't have strong preferences on 1 bit vs 2 bits. I think, 1
> bit can be supported by all the HW if supports this feature. Leaving the
> decision to ethdev and mbuf maintainers.
> 
>>
>> If everyone else is fine with the description of Rx checksum offloads and
>> it is only me who is unhappy with it - no problem.
>>
>> Thanks for your patience and I'm sorry that I'm really boring with it.
>> My goal is just to make it clear and as the result have less bugs in
>> networking PMDs and applications.
> 
> No problem. :-)
> 
> 
>>
>> Andrew.
>>
Thomas Monjalon Oct. 5, 2018, 10:44 p.m. UTC | #22 
05/10/2018 22:04, Ferruh Yigit:
> On 10/4/2018 6:59 AM, Jerin Jacob wrote:
> > From: Andrew Rybchenko <arybchenko@solarflare.com>
> >> On 03.10.2018 21:14, Jerin Jacob wrote:
> >>> From: Andrew Rybchenko <arybchenko@solarflare.com>
> >>>> On 03.10.2018 20:12, Jerin Jacob wrote:
> >>>>> From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> >>>>>> From: Andrew Rybchenko <arybchenko@solarflare.com>
> >>>>>>> 3. PKT_RX_L4_CKSUM_MASK description says nothing if it is inner or outer.
> >>>>>>>      May be it is not directly related to changeset, but I think it would be really
> >>>>>>>      useful to clarify it.
> >>>>>> I will update the comment.
> >>>>> Hi Andrew,
> >>>>>
> >>>>> I looked at the other definitions in mbuf.h, according the documentation,
> >>>>> If nothing is mentioned it is treated as inner if the packet is
> >>>>> tunneled else it is outer most. So I would like avoid confusion by
> >>>>> adding "inner" in the exiting PKT_RX_L4_CKSUM_MASK comment.
> >>>>> Technically it is not correct to say "inner" if the packet is not
> >>>>> tunneled. So I am untouching the exiting comment.
> >>>>>
> >>>> Yes, it is incorrect to say that it is inner. How does application find
> >>>> how to treat PKT_RX_L4_CKSUM (inner or outer)?
> >>>> Should it rely on packet type provided in mbuf?
> >>> AFAIK, Finding is it a tunneled packet or not is through ptype or SW has
> >>> to parse the packet. For example, testpmd chooses later method using
> >>> "csum parse-tunnel on <port>" to detect the presence of the tunnel.
> >>
> >> SW parsing of the packet cannot help, since app should be sure
> >> that HW has classified the packet as tunneled and provided information
> >> about inner and outer checksum checks.
> > 
> > 
> > I thought the question was, How does the application find how to treat
> > PKT_RX_L4_CKSUM (inner or outer)?
> > Obviously, ptype will help here
> > Not sure why SW parsing won't help here if SW parses and find it is an
> > inner packet or it is not a tunneled packet. PKT_RX_L4_CKSUM treat as
> > inner checksum for former case and PKT_RX_L4_CKSUM treated as plain l4
> > checksum in the latter case.
> 
> I don't know if PKT_RX_L4_CKSUM is inner or outer with current design,
> but wouldn't be easier if PKT_RX_L4_CKSUM always refer to outer and if tunneled
> use something like PKT_RX_INNER_L4_CKSUM, same for IP and TX.

The return of the come back of the "inner vs outer" discussion :-)
There were some thoughts about what is the most convenient for apps doing
decapsulation of tunnels.
I think the decision was to choose inner as the default, while outer
must be explicited.

Honestly, we should not try to change again the meaning of symbols
having no outer/inner word in their name, except if we have a
very very good reason and arguments.

And for new symbols, we must follow the community decision.

However, we should re-visit the flag PKT_RX_EIP_CKSUM_BAD.
Jerin Jacob Oct. 6, 2018, 8:15 a.m. UTC | #23 
-----Original Message-----
> Date: Sat, 06 Oct 2018 00:44:52 +0200
> From: Thomas Monjalon <thomas@monjalon.net>
> To: Ferruh Yigit <ferruh.yigit@intel.com>, Jerin Jacob
>  <jerin.jacob@caviumnetworks.com>, Andrew Rybchenko
>  <arybchenko@solarflare.com>
> Cc: Wenzhuo Lu <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>,
>  Bernard Iremonger <bernard.iremonger@intel.com>, John McNamara
>  <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
>  Olivier Matz <olivier.matz@6wind.com>, dev@dpdk.org, shahafs@mellanox.com,
>  "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
>  didier.pallard@6wind.com
> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>  checksum definition
> 
> 
> 05/10/2018 22:04, Ferruh Yigit:
> > On 10/4/2018 6:59 AM, Jerin Jacob wrote:
> > > From: Andrew Rybchenko <arybchenko@solarflare.com>
> > >> On 03.10.2018 21:14, Jerin Jacob wrote:
> > >>> From: Andrew Rybchenko <arybchenko@solarflare.com>
> > >>>> On 03.10.2018 20:12, Jerin Jacob wrote:
> > >>>>> From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > >>>>>> From: Andrew Rybchenko <arybchenko@solarflare.com>
> > >>>>>>> 3. PKT_RX_L4_CKSUM_MASK description says nothing if it is inner or outer.
> > >>>>>>>      May be it is not directly related to changeset, but I think it would be really
> > >>>>>>>      useful to clarify it.
> > >>>>>> I will update the comment.
> > >>>>> Hi Andrew,
> > >>>>>
> 
> However, we should re-visit the flag PKT_RX_EIP_CKSUM_BAD.

Do we need to block this patch due to the exiting PKT_RX_EIP_CKSUM_BAD
definition?

I already added the author of the PKT_RX_EIP_CKSUM_BAD flag and ethdev and mbuf
maintainers in this list. So what else I need make forward progress
on this patch?

I think, the definition of PKT_RX_EIP_CKSUM_BAD based on HW capability. It
is safe to assume that ALL HW can support CKSUM BAD if the feature is
available and hence it is more portable.

> 
>
Ananyev, Konstantin Oct. 6, 2018, 12:18 p.m. UTC | #24 
> -----Original Message-----
> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> Sent: Saturday, October 6, 2018 9:16 AM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu,
> Jingjing <jingjing.wu@intel.com>; Iremonger, Bernard <bernard.iremonger@intel.com>; Mcnamara, John <john.mcnamara@intel.com>;
> Kovacevic, Marko <marko.kovacevic@intel.com>; Olivier Matz <olivier.matz@6wind.com>; dev@dpdk.org; shahafs@mellanox.com;
> Ananyev, Konstantin <konstantin.ananyev@intel.com>; didier.pallard@6wind.com
> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP checksum definition
> 
> -----Original Message-----
> > Date: Sat, 06 Oct 2018 00:44:52 +0200
> > From: Thomas Monjalon <thomas@monjalon.net>
> > To: Ferruh Yigit <ferruh.yigit@intel.com>, Jerin Jacob
> >  <jerin.jacob@caviumnetworks.com>, Andrew Rybchenko
> >  <arybchenko@solarflare.com>
> > Cc: Wenzhuo Lu <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>,
> >  Bernard Iremonger <bernard.iremonger@intel.com>, John McNamara
> >  <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
> >  Olivier Matz <olivier.matz@6wind.com>, dev@dpdk.org, shahafs@mellanox.com,
> >  "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
> >  didier.pallard@6wind.com
> > Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
> >  checksum definition
> >
> >
> > 05/10/2018 22:04, Ferruh Yigit:
> > > On 10/4/2018 6:59 AM, Jerin Jacob wrote:
> > > > From: Andrew Rybchenko <arybchenko@solarflare.com>
> > > >> On 03.10.2018 21:14, Jerin Jacob wrote:
> > > >>> From: Andrew Rybchenko <arybchenko@solarflare.com>
> > > >>>> On 03.10.2018 20:12, Jerin Jacob wrote:
> > > >>>>> From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > >>>>>> From: Andrew Rybchenko <arybchenko@solarflare.com>
> > > >>>>>>> 3. PKT_RX_L4_CKSUM_MASK description says nothing if it is inner or outer.
> > > >>>>>>>      May be it is not directly related to changeset, but I think it would be really
> > > >>>>>>>      useful to clarify it.
> > > >>>>>> I will update the comment.
> > > >>>>> Hi Andrew,
> > > >>>>>
> >
> > However, we should re-visit the flag PKT_RX_EIP_CKSUM_BAD.
> 
> Do we need to block this patch due to the exiting PKT_RX_EIP_CKSUM_BAD
> definition?
> 
> I already added the author of the PKT_RX_EIP_CKSUM_BAD flag and ethdev and mbuf
> maintainers in this list. So what else I need make forward progress
> on this patch?
> 
> I think, the definition of PKT_RX_EIP_CKSUM_BAD based on HW capability. It
> is safe to assume that ALL HW can support CKSUM BAD if the feature is
> available and hence it is more portable.

Yes, as I remember PKT_RX_EIP_CKSUM_BAD is based on DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM.
Konstantin

> 
> >
> >
Ferruh Yigit Oct. 8, 2018, 8:12 a.m. UTC | #25 
On 10/6/2018 1:18 PM, Ananyev, Konstantin wrote:
> 
> 
>> -----Original Message-----
>> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
>> Sent: Saturday, October 6, 2018 9:16 AM
>> To: Thomas Monjalon <thomas@monjalon.net>
>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu,
>> Jingjing <jingjing.wu@intel.com>; Iremonger, Bernard <bernard.iremonger@intel.com>; Mcnamara, John <john.mcnamara@intel.com>;
>> Kovacevic, Marko <marko.kovacevic@intel.com>; Olivier Matz <olivier.matz@6wind.com>; dev@dpdk.org; shahafs@mellanox.com;
>> Ananyev, Konstantin <konstantin.ananyev@intel.com>; didier.pallard@6wind.com
>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP checksum definition
>>
>> -----Original Message-----
>>> Date: Sat, 06 Oct 2018 00:44:52 +0200
>>> From: Thomas Monjalon <thomas@monjalon.net>
>>> To: Ferruh Yigit <ferruh.yigit@intel.com>, Jerin Jacob
>>>  <jerin.jacob@caviumnetworks.com>, Andrew Rybchenko
>>>  <arybchenko@solarflare.com>
>>> Cc: Wenzhuo Lu <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>,
>>>  Bernard Iremonger <bernard.iremonger@intel.com>, John McNamara
>>>  <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
>>>  Olivier Matz <olivier.matz@6wind.com>, dev@dpdk.org, shahafs@mellanox.com,
>>>  "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
>>>  didier.pallard@6wind.com
>>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>>>  checksum definition
>>>
>>>
>>> 05/10/2018 22:04, Ferruh Yigit:
>>>> On 10/4/2018 6:59 AM, Jerin Jacob wrote:
>>>>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>>>>>> On 03.10.2018 21:14, Jerin Jacob wrote:
>>>>>>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>>>>>>>> On 03.10.2018 20:12, Jerin Jacob wrote:
>>>>>>>>> From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>>>>>>>>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>>>>>>>>>>> 3. PKT_RX_L4_CKSUM_MASK description says nothing if it is inner or outer.
>>>>>>>>>>>      May be it is not directly related to changeset, but I think it would be really
>>>>>>>>>>>      useful to clarify it.
>>>>>>>>>> I will update the comment.
>>>>>>>>> Hi Andrew,
>>>>>>>>>
>>>
>>> However, we should re-visit the flag PKT_RX_EIP_CKSUM_BAD.
>>
>> Do we need to block this patch due to the exiting PKT_RX_EIP_CKSUM_BAD
>> definition?
>>
>> I already added the author of the PKT_RX_EIP_CKSUM_BAD flag and ethdev and mbuf
>> maintainers in this list. So what else I need make forward progress
>> on this patch?
>>
>> I think, the definition of PKT_RX_EIP_CKSUM_BAD based on HW capability. It
>> is safe to assume that ALL HW can support CKSUM BAD if the feature is
>> available and hence it is more portable.
> 
> Yes, as I remember PKT_RX_EIP_CKSUM_BAD is based on DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM.

Switching to two bit won't reduce the portability, HW supports only reporting
CKSUM_BAD can set BAD || UNKNOWN.

And I think patch is not blocked by PKT_RX_EIP_CKSUM_BAD, it can be changed
separately, for this patch question is can we represent PKT_RX_EL4_CKSUM_* with
two bits, to have BAD/GOOD/UNKNOWN?
Jerin Jacob Oct. 8, 2018, 8:24 a.m. UTC | #26 
-----Original Message-----
> Date: Mon, 8 Oct 2018 09:12:34 +0100
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, Jerin Jacob
>  <jerin.jacob@caviumnetworks.com>, Thomas Monjalon <thomas@monjalon.net>
> CC: Andrew Rybchenko <arybchenko@solarflare.com>, "Lu, Wenzhuo"
>  <wenzhuo.lu@intel.com>, "Wu, Jingjing" <jingjing.wu@intel.com>,
>  "Iremonger, Bernard" <bernard.iremonger@intel.com>, "Mcnamara, John"
>  <john.mcnamara@intel.com>, "Kovacevic, Marko" <marko.kovacevic@intel.com>,
>  Olivier Matz <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>,
>  "shahafs@mellanox.com" <shahafs@mellanox.com>, "didier.pallard@6wind.com"
>  <didier.pallard@6wind.com>
> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>  checksum definition
> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
>  Thunderbird/52.9.1
> 
> 
> On 10/6/2018 1:18 PM, Ananyev, Konstantin wrote:
> >
> >
> >> -----Original Message-----
> >> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> >> Sent: Saturday, October 6, 2018 9:16 AM
> >> To: Thomas Monjalon <thomas@monjalon.net>
> >> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu,
> >> Jingjing <jingjing.wu@intel.com>; Iremonger, Bernard <bernard.iremonger@intel.com>; Mcnamara, John <john.mcnamara@intel.com>;
> >> Kovacevic, Marko <marko.kovacevic@intel.com>; Olivier Matz <olivier.matz@6wind.com>; dev@dpdk.org; shahafs@mellanox.com;
> >> Ananyev, Konstantin <konstantin.ananyev@intel.com>; didier.pallard@6wind.com
> >> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP checksum definition
> >>
> >> -----Original Message-----
> >>> Date: Sat, 06 Oct 2018 00:44:52 +0200
> >>> From: Thomas Monjalon <thomas@monjalon.net>
> >>> To: Ferruh Yigit <ferruh.yigit@intel.com>, Jerin Jacob
> >>>  <jerin.jacob@caviumnetworks.com>, Andrew Rybchenko
> >>>  <arybchenko@solarflare.com>
> >>> Cc: Wenzhuo Lu <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>,
> >>>  Bernard Iremonger <bernard.iremonger@intel.com>, John McNamara
> >>>  <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
> >>>  Olivier Matz <olivier.matz@6wind.com>, dev@dpdk.org, shahafs@mellanox.com,
> >>>  "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
> >>>  didier.pallard@6wind.com
> >>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
> >>>  checksum definition
> >>>
> >>>
> >>> 05/10/2018 22:04, Ferruh Yigit:
> >>>> On 10/4/2018 6:59 AM, Jerin Jacob wrote:
> >>>>> From: Andrew Rybchenko <arybchenko@solarflare.com>
> >>>>>> On 03.10.2018 21:14, Jerin Jacob wrote:
> >>>>>>> From: Andrew Rybchenko <arybchenko@solarflare.com>
> >>>>>>>> On 03.10.2018 20:12, Jerin Jacob wrote:
> >>>>>>>>> From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> >>>>>>>>>> From: Andrew Rybchenko <arybchenko@solarflare.com>
> >>>>>>>>>>> 3. PKT_RX_L4_CKSUM_MASK description says nothing if it is inner or outer.
> >>>>>>>>>>>      May be it is not directly related to changeset, but I think it would be really
> >>>>>>>>>>>      useful to clarify it.
> >>>>>>>>>> I will update the comment.
> >>>>>>>>> Hi Andrew,
> >>>>>>>>>
> >>>
> >>> However, we should re-visit the flag PKT_RX_EIP_CKSUM_BAD.
> >>
> >> Do we need to block this patch due to the exiting PKT_RX_EIP_CKSUM_BAD
> >> definition?
> >>
> >> I already added the author of the PKT_RX_EIP_CKSUM_BAD flag and ethdev and mbuf
> >> maintainers in this list. So what else I need make forward progress
> >> on this patch?
> >>
> >> I think, the definition of PKT_RX_EIP_CKSUM_BAD based on HW capability. It
> >> is safe to assume that ALL HW can support CKSUM BAD if the feature is
> >> available and hence it is more portable.
> >
> > Yes, as I remember PKT_RX_EIP_CKSUM_BAD is based on DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM.
> 
> Switching to two bit won't reduce the portability, HW supports only reporting
> CKSUM_BAD can set BAD || UNKNOWN.

UNKNOWN is not a bit. It is represented as 0. It spec has 2 bit, then
driver need to report GOOD as well.

Same applies for PKT_RX_EL4_CKSUM as well.

> 
> And I think patch is not blocked by PKT_RX_EIP_CKSUM_BAD, it can be changed
> separately, for this patch question is can we represent PKT_RX_EL4_CKSUM_* with
> two bits, to have BAD/GOOD/UNKNOWN?
Thomas Monjalon Oct. 8, 2018, 9:04 a.m. UTC | #27 
08/10/2018 10:24, Jerin Jacob:
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> > On 10/6/2018 1:18 PM, Ananyev, Konstantin wrote:
> > > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > >> From: Thomas Monjalon <thomas@monjalon.net>
> > >>> However, we should re-visit the flag PKT_RX_EIP_CKSUM_BAD.
> > >>
> > >> Do we need to block this patch due to the exiting PKT_RX_EIP_CKSUM_BAD
> > >> definition?
> > >>
> > >> I already added the author of the PKT_RX_EIP_CKSUM_BAD flag and ethdev and mbuf
> > >> maintainers in this list. So what else I need make forward progress
> > >> on this patch?
> > >>
> > >> I think, the definition of PKT_RX_EIP_CKSUM_BAD based on HW capability. It
> > >> is safe to assume that ALL HW can support CKSUM BAD if the feature is
> > >> available and hence it is more portable.
> > >
> > > Yes, as I remember PKT_RX_EIP_CKSUM_BAD is based on DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM.
> > 
> > Switching to two bit won't reduce the portability, HW supports only reporting
> > CKSUM_BAD can set BAD || UNKNOWN.
> 
> UNKNOWN is not a bit. It is represented as 0. It spec has 2 bit, then
> driver need to report GOOD as well.
> 
> Same applies for PKT_RX_EL4_CKSUM as well.
> 
> > 
> > And I think patch is not blocked by PKT_RX_EIP_CKSUM_BAD, it can be changed
> > separately, for this patch question is can we represent PKT_RX_EL4_CKSUM_* with
> > two bits, to have BAD/GOOD/UNKNOWN?

Yes, exact.

PKT_RX_EIP_CKSUM_BAD must be left aside.
We should just avoid taking it as a reference.
And we can reconsider its definition later.
Jerin Jacob Oct. 8, 2018, 9:37 a.m. UTC | #28 
-----Original Message-----
> Date: Mon, 08 Oct 2018 11:04:51 +0200
> From: Thomas Monjalon <thomas@monjalon.net>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, Ferruh Yigit
>  <ferruh.yigit@intel.com>, "Ananyev, Konstantin"
>  <konstantin.ananyev@intel.com>
> Cc: Andrew Rybchenko <arybchenko@solarflare.com>, "Lu, Wenzhuo"
>  <wenzhuo.lu@intel.com>, "Wu, Jingjing" <jingjing.wu@intel.com>,
>  "Iremonger, Bernard" <bernard.iremonger@intel.com>, "Mcnamara, John"
>  <john.mcnamara@intel.com>, "Kovacevic, Marko" <marko.kovacevic@intel.com>,
>  Olivier Matz <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>,
>  "shahafs@mellanox.com" <shahafs@mellanox.com>, "didier.pallard@6wind.com"
>  <didier.pallard@6wind.com>
> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>  checksum definition
> 
> 08/10/2018 10:24, Jerin Jacob:
> > From: Ferruh Yigit <ferruh.yigit@intel.com>
> > > On 10/6/2018 1:18 PM, Ananyev, Konstantin wrote:
> > > > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > > >> From: Thomas Monjalon <thomas@monjalon.net>
> > > >>> However, we should re-visit the flag PKT_RX_EIP_CKSUM_BAD.
> > > >>
> > > >> Do we need to block this patch due to the exiting PKT_RX_EIP_CKSUM_BAD
> > > >> definition?
> > > >>
> > > >> I already added the author of the PKT_RX_EIP_CKSUM_BAD flag and ethdev and mbuf
> > > >> maintainers in this list. So what else I need make forward progress
> > > >> on this patch?
> > > >>
> > > >> I think, the definition of PKT_RX_EIP_CKSUM_BAD based on HW capability. It
> > > >> is safe to assume that ALL HW can support CKSUM BAD if the feature is
> > > >> available and hence it is more portable.
> > > >
> > > > Yes, as I remember PKT_RX_EIP_CKSUM_BAD is based on DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM.
> > >
> > > Switching to two bit won't reduce the portability, HW supports only reporting
> > > CKSUM_BAD can set BAD || UNKNOWN.
> >
> > UNKNOWN is not a bit. It is represented as 0. It spec has 2 bit, then
> > driver need to report GOOD as well.
> >
> > Same applies for PKT_RX_EL4_CKSUM as well.
> >
> > >
> > > And I think patch is not blocked by PKT_RX_EIP_CKSUM_BAD, it can be changed
> > > separately, for this patch question is can we represent PKT_RX_EL4_CKSUM_* with
> > > two bits, to have BAD/GOOD/UNKNOWN?
> 
> Yes, exact.
> 
> PKT_RX_EIP_CKSUM_BAD must be left aside.
> We should just avoid taking it as a reference.
> And we can reconsider its definition later.

OK.

IMO, Using 2 bit scheme for tunneled checksum has following performance
issue from driver side.

Driver need to mark the packet as GOOD. All the HW can support
detection of BAD. That not necessary mean GOOD in case of tunnel packet,
so driver has to detect the packet is tunneled and packet is not BAD
then mark GOOD.



> 
> 
>
Ferruh Yigit Oct. 8, 2018, 10:53 a.m. UTC | #29 
On 10/8/2018 10:37 AM, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Mon, 08 Oct 2018 11:04:51 +0200
>> From: Thomas Monjalon <thomas@monjalon.net>
>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, Ferruh Yigit
>>  <ferruh.yigit@intel.com>, "Ananyev, Konstantin"
>>  <konstantin.ananyev@intel.com>
>> Cc: Andrew Rybchenko <arybchenko@solarflare.com>, "Lu, Wenzhuo"
>>  <wenzhuo.lu@intel.com>, "Wu, Jingjing" <jingjing.wu@intel.com>,
>>  "Iremonger, Bernard" <bernard.iremonger@intel.com>, "Mcnamara, John"
>>  <john.mcnamara@intel.com>, "Kovacevic, Marko" <marko.kovacevic@intel.com>,
>>  Olivier Matz <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>,
>>  "shahafs@mellanox.com" <shahafs@mellanox.com>, "didier.pallard@6wind.com"
>>  <didier.pallard@6wind.com>
>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>>  checksum definition
>>
>> 08/10/2018 10:24, Jerin Jacob:
>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>> On 10/6/2018 1:18 PM, Ananyev, Konstantin wrote:
>>>>> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>>>>> However, we should re-visit the flag PKT_RX_EIP_CKSUM_BAD.
>>>>>>
>>>>>> Do we need to block this patch due to the exiting PKT_RX_EIP_CKSUM_BAD
>>>>>> definition?
>>>>>>
>>>>>> I already added the author of the PKT_RX_EIP_CKSUM_BAD flag and ethdev and mbuf
>>>>>> maintainers in this list. So what else I need make forward progress
>>>>>> on this patch?
>>>>>>
>>>>>> I think, the definition of PKT_RX_EIP_CKSUM_BAD based on HW capability. It
>>>>>> is safe to assume that ALL HW can support CKSUM BAD if the feature is
>>>>>> available and hence it is more portable.
>>>>>
>>>>> Yes, as I remember PKT_RX_EIP_CKSUM_BAD is based on DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM.
>>>>
>>>> Switching to two bit won't reduce the portability, HW supports only reporting
>>>> CKSUM_BAD can set BAD || UNKNOWN.
>>>
>>> UNKNOWN is not a bit. It is represented as 0. It spec has 2 bit, then
>>> driver need to report GOOD as well.
>>>
>>> Same applies for PKT_RX_EL4_CKSUM as well.
>>>
>>>>
>>>> And I think patch is not blocked by PKT_RX_EIP_CKSUM_BAD, it can be changed
>>>> separately, for this patch question is can we represent PKT_RX_EL4_CKSUM_* with
>>>> two bits, to have BAD/GOOD/UNKNOWN?
>>
>> Yes, exact.
>>
>> PKT_RX_EIP_CKSUM_BAD must be left aside.
>> We should just avoid taking it as a reference.
>> And we can reconsider its definition later.
> 
> OK.
> 
> IMO, Using 2 bit scheme for tunneled checksum has following performance
> issue from driver side.
> 
> Driver need to mark the packet as GOOD. All the HW can support
> detection of BAD. That not necessary mean GOOD in case of tunnel packet,
> so driver has to detect the packet is tunneled and packet is not BAD
> then mark GOOD.

Yes UNKNOWN is not a bit, but a state, why don't use it? Why driver has to check
it is GOOD?

0x0 => UNKNOWN
0x1 => BAD
0x2 => GOOD
0x3 => ? (invalid perhaps)

HW that supports detecting good packets can set BAD || GOOD state, HW can detect
only BAD packet can set BAD || UNKNOWN state.

If BAD is not set, there is an ambiguity of state, lets clarify it in lower
level, if it is UNKNOWN, let application know it is UNKNOWN.
Jerin Jacob Oct. 8, 2018, 11:55 a.m. UTC | #30 
-----Original Message-----
> Date: Mon, 8 Oct 2018 11:53:01 +0100
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, Thomas Monjalon
>  <thomas@monjalon.net>
> CC: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, Andrew Rybchenko
>  <arybchenko@solarflare.com>, "Lu, Wenzhuo" <wenzhuo.lu@intel.com>, "Wu,
>  Jingjing" <jingjing.wu@intel.com>, "Iremonger, Bernard"
>  <bernard.iremonger@intel.com>, "Mcnamara, John" <john.mcnamara@intel.com>,
>  "Kovacevic, Marko" <marko.kovacevic@intel.com>, Olivier Matz
>  <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>,
>  "shahafs@mellanox.com" <shahafs@mellanox.com>, "didier.pallard@6wind.com"
>  <didier.pallard@6wind.com>
> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>  checksum definition
> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
>  Thunderbird/52.9.1
> 
> On 10/8/2018 10:37 AM, Jerin Jacob wrote:
> > -----Original Message-----
> >> Date: Mon, 08 Oct 2018 11:04:51 +0200
> >> From: Thomas Monjalon <thomas@monjalon.net>
> >> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, Ferruh Yigit
> >>  <ferruh.yigit@intel.com>, "Ananyev, Konstantin"
> >>  <konstantin.ananyev@intel.com>
> >> Cc: Andrew Rybchenko <arybchenko@solarflare.com>, "Lu, Wenzhuo"
> >>  <wenzhuo.lu@intel.com>, "Wu, Jingjing" <jingjing.wu@intel.com>,
> >>  "Iremonger, Bernard" <bernard.iremonger@intel.com>, "Mcnamara, John"
> >>  <john.mcnamara@intel.com>, "Kovacevic, Marko" <marko.kovacevic@intel.com>,
> >>  Olivier Matz <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>,
> >>  "shahafs@mellanox.com" <shahafs@mellanox.com>, "didier.pallard@6wind.com"
> >>  <didier.pallard@6wind.com>
> >> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
> >>  checksum definition
> >>
> >> 08/10/2018 10:24, Jerin Jacob:
> >>> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >>>> On 10/6/2018 1:18 PM, Ananyev, Konstantin wrote:
> >>>>> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> >>>>>> From: Thomas Monjalon <thomas@monjalon.net>
> >>>>>>> However, we should re-visit the flag PKT_RX_EIP_CKSUM_BAD.
> >>>>>>
> >>>>>> Do we need to block this patch due to the exiting PKT_RX_EIP_CKSUM_BAD
> >>>>>> definition?
> >>>>>>
> >>>>>> I already added the author of the PKT_RX_EIP_CKSUM_BAD flag and ethdev and mbuf
> >>>>>> maintainers in this list. So what else I need make forward progress
> >>>>>> on this patch?
> >>>>>>
> >>>>>> I think, the definition of PKT_RX_EIP_CKSUM_BAD based on HW capability. It
> >>>>>> is safe to assume that ALL HW can support CKSUM BAD if the feature is
> >>>>>> available and hence it is more portable.
> >>>>>
> >>>>> Yes, as I remember PKT_RX_EIP_CKSUM_BAD is based on DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM.
> >>>>
> >>>> Switching to two bit won't reduce the portability, HW supports only reporting
> >>>> CKSUM_BAD can set BAD || UNKNOWN.
> >>>
> >>> UNKNOWN is not a bit. It is represented as 0. It spec has 2 bit, then
> >>> driver need to report GOOD as well.
> >>>
> >>> Same applies for PKT_RX_EL4_CKSUM as well.
> >>>
> >>>>
> >>>> And I think patch is not blocked by PKT_RX_EIP_CKSUM_BAD, it can be changed
> >>>> separately, for this patch question is can we represent PKT_RX_EL4_CKSUM_* with
> >>>> two bits, to have BAD/GOOD/UNKNOWN?
> >>
> >> Yes, exact.
> >>
> >> PKT_RX_EIP_CKSUM_BAD must be left aside.
> >> We should just avoid taking it as a reference.
> >> And we can reconsider its definition later.
> >
> > OK.
> >
> > IMO, Using 2 bit scheme for tunneled checksum has following performance
> > issue from driver side.
> >
> > Driver need to mark the packet as GOOD. All the HW can support
> > detection of BAD. That not necessary mean GOOD in case of tunnel packet,
> > so driver has to detect the packet is tunneled and packet is not BAD
> > then mark GOOD.
> 
> Yes UNKNOWN is not a bit, but a state, why don't use it? Why driver has to check
> it is GOOD?

The application is going to check is it GOOD or not. Not the driver,
Right? My concern was, If application starts dropping the packet instead checking the BAD, if
it checks == !GOOD.

> 
> 0x0 => UNKNOWN
> 0x1 => BAD
> 0x2 => GOOD
> 0x3 => ? (invalid perhaps)
> 
> HW that supports detecting good packets can set BAD || GOOD state, HW can detect
> only BAD packet can set BAD || UNKNOWN state.
> 
> If BAD is not set, there is an ambiguity of state, lets clarify it in lower
> level, if it is UNKNOWN, let application know it is UNKNOWN.

OK.

How about the following then?

/**
 * Mask of bits used to determine the status of outer RX L4 checksum.
 * - PKT_RX_EL4_CKSUM_UNKNOWN: no information about the outer RX L4 checksum
 * - PKT_RX_EL4_CKSUM_BAD: the outer L4 checksum in the packet is wrong
 * - PKT_RX_EL4_CKSUM_GOOD: the outer L4 checksum in the packet is valid
 * - PKT_RX_EL4_CKSUM_INVALID: invalid outer L4 checksum state.
 *
 * The detection of PKT_RX_EL4_CKSUM_GOOD shall be based on the given
 * HW capability, At minimum, the PMD should support
 * PKT_RX_EL4_CKSUM_UNKNOWN  and PKT_RX_EL4_CKSUM_BAD states
 * if the offload is available.
 */
#define PKT_RX_EL4_CKSUM_MASK   ((1ULL << 21) | (1ULL << 22))

#define PKT_RX_IP_CKSUM_UNKNOWN 0
#define PKT_RX_IP_CKSUM_BAD     (1ULL << 21)
#define PKT_RX_IP_CKSUM_GOOD    (1ULL << 22)
#define PKT_RX_IP_CKSUM_INVALID ((1ULL << 21) | (1ULL << 22))
Ferruh Yigit Oct. 8, 2018, 12:13 p.m. UTC | #31 
On 10/8/2018 12:55 PM, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Mon, 8 Oct 2018 11:53:01 +0100
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, Thomas Monjalon
>>  <thomas@monjalon.net>
>> CC: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, Andrew Rybchenko
>>  <arybchenko@solarflare.com>, "Lu, Wenzhuo" <wenzhuo.lu@intel.com>, "Wu,
>>  Jingjing" <jingjing.wu@intel.com>, "Iremonger, Bernard"
>>  <bernard.iremonger@intel.com>, "Mcnamara, John" <john.mcnamara@intel.com>,
>>  "Kovacevic, Marko" <marko.kovacevic@intel.com>, Olivier Matz
>>  <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>,
>>  "shahafs@mellanox.com" <shahafs@mellanox.com>, "didier.pallard@6wind.com"
>>  <didier.pallard@6wind.com>
>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>>  checksum definition
>> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
>>  Thunderbird/52.9.1
>>
>> On 10/8/2018 10:37 AM, Jerin Jacob wrote:
>>> -----Original Message-----
>>>> Date: Mon, 08 Oct 2018 11:04:51 +0200
>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, Ferruh Yigit
>>>>  <ferruh.yigit@intel.com>, "Ananyev, Konstantin"
>>>>  <konstantin.ananyev@intel.com>
>>>> Cc: Andrew Rybchenko <arybchenko@solarflare.com>, "Lu, Wenzhuo"
>>>>  <wenzhuo.lu@intel.com>, "Wu, Jingjing" <jingjing.wu@intel.com>,
>>>>  "Iremonger, Bernard" <bernard.iremonger@intel.com>, "Mcnamara, John"
>>>>  <john.mcnamara@intel.com>, "Kovacevic, Marko" <marko.kovacevic@intel.com>,
>>>>  Olivier Matz <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>,
>>>>  "shahafs@mellanox.com" <shahafs@mellanox.com>, "didier.pallard@6wind.com"
>>>>  <didier.pallard@6wind.com>
>>>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>>>>  checksum definition
>>>>
>>>> 08/10/2018 10:24, Jerin Jacob:
>>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>>> On 10/6/2018 1:18 PM, Ananyev, Konstantin wrote:
>>>>>>> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
>>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>>>>>>> However, we should re-visit the flag PKT_RX_EIP_CKSUM_BAD.
>>>>>>>>
>>>>>>>> Do we need to block this patch due to the exiting PKT_RX_EIP_CKSUM_BAD
>>>>>>>> definition?
>>>>>>>>
>>>>>>>> I already added the author of the PKT_RX_EIP_CKSUM_BAD flag and ethdev and mbuf
>>>>>>>> maintainers in this list. So what else I need make forward progress
>>>>>>>> on this patch?
>>>>>>>>
>>>>>>>> I think, the definition of PKT_RX_EIP_CKSUM_BAD based on HW capability. It
>>>>>>>> is safe to assume that ALL HW can support CKSUM BAD if the feature is
>>>>>>>> available and hence it is more portable.
>>>>>>>
>>>>>>> Yes, as I remember PKT_RX_EIP_CKSUM_BAD is based on DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM.
>>>>>>
>>>>>> Switching to two bit won't reduce the portability, HW supports only reporting
>>>>>> CKSUM_BAD can set BAD || UNKNOWN.
>>>>>
>>>>> UNKNOWN is not a bit. It is represented as 0. It spec has 2 bit, then
>>>>> driver need to report GOOD as well.
>>>>>
>>>>> Same applies for PKT_RX_EL4_CKSUM as well.
>>>>>
>>>>>>
>>>>>> And I think patch is not blocked by PKT_RX_EIP_CKSUM_BAD, it can be changed
>>>>>> separately, for this patch question is can we represent PKT_RX_EL4_CKSUM_* with
>>>>>> two bits, to have BAD/GOOD/UNKNOWN?
>>>>
>>>> Yes, exact.
>>>>
>>>> PKT_RX_EIP_CKSUM_BAD must be left aside.
>>>> We should just avoid taking it as a reference.
>>>> And we can reconsider its definition later.
>>>
>>> OK.
>>>
>>> IMO, Using 2 bit scheme for tunneled checksum has following performance
>>> issue from driver side.
>>>
>>> Driver need to mark the packet as GOOD. All the HW can support
>>> detection of BAD. That not necessary mean GOOD in case of tunnel packet,
>>> so driver has to detect the packet is tunneled and packet is not BAD
>>> then mark GOOD.
>>
>> Yes UNKNOWN is not a bit, but a state, why don't use it? Why driver has to check
>> it is GOOD?
> 
> The application is going to check is it GOOD or not. Not the driver,
> Right? My concern was, If application starts dropping the packet instead checking the BAD, if
> it checks == !GOOD.

Got it, but when 2 bits state introduced, app should check if check == BAD for
drop decision, because it is not GOOD || BAD anymore.

> 
>>
>> 0x0 => UNKNOWN
>> 0x1 => BAD
>> 0x2 => GOOD
>> 0x3 => ? (invalid perhaps)
>>
>> HW that supports detecting good packets can set BAD || GOOD state, HW can detect
>> only BAD packet can set BAD || UNKNOWN state.
>>
>> If BAD is not set, there is an ambiguity of state, lets clarify it in lower
>> level, if it is UNKNOWN, let application know it is UNKNOWN.
> 
> OK.
> 
> How about the following then?
> 
> /**
>  * Mask of bits used to determine the status of outer RX L4 checksum.
>  * - PKT_RX_EL4_CKSUM_UNKNOWN: no information about the outer RX L4 checksum
>  * - PKT_RX_EL4_CKSUM_BAD: the outer L4 checksum in the packet is wrong
>  * - PKT_RX_EL4_CKSUM_GOOD: the outer L4 checksum in the packet is valid
>  * - PKT_RX_EL4_CKSUM_INVALID: invalid outer L4 checksum state.
>  *
>  * The detection of PKT_RX_EL4_CKSUM_GOOD shall be based on the given
>  * HW capability, At minimum, the PMD should support
>  * PKT_RX_EL4_CKSUM_UNKNOWN  and PKT_RX_EL4_CKSUM_BAD states
>  * if the offload is available.
>  */
> #define PKT_RX_EL4_CKSUM_MASK   ((1ULL << 21) | (1ULL << 22))
> 
> #define PKT_RX_IP_CKSUM_UNKNOWN 0
> #define PKT_RX_IP_CKSUM_BAD     (1ULL << 21)
> #define PKT_RX_IP_CKSUM_GOOD    (1ULL << 22)
> #define PKT_RX_IP_CKSUM_INVALID ((1ULL << 21) | (1ULL << 22))

Looks good to me.


> 
> 
>
Jerin Jacob Oct. 8, 2018, 12:25 p.m. UTC | #32 
-----Original Message-----
> Date: Mon, 8 Oct 2018 13:13:31 +0100
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: Thomas Monjalon <thomas@monjalon.net>, "Ananyev, Konstantin"
>  <konstantin.ananyev@intel.com>, Andrew Rybchenko
>  <arybchenko@solarflare.com>, "Lu, Wenzhuo" <wenzhuo.lu@intel.com>, "Wu,
>  Jingjing" <jingjing.wu@intel.com>, "Iremonger, Bernard"
>  <bernard.iremonger@intel.com>, "Mcnamara, John" <john.mcnamara@intel.com>,
>  "Kovacevic, Marko" <marko.kovacevic@intel.com>, Olivier Matz
>  <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>,
>  "shahafs@mellanox.com" <shahafs@mellanox.com>, "didier.pallard@6wind.com"
>  <didier.pallard@6wind.com>
> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>  checksum definition
> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
>  Thunderbird/52.9.1
> 
> On 10/8/2018 12:55 PM, Jerin Jacob wrote:
> > -----Original Message-----
> >> Date: Mon, 8 Oct 2018 11:53:01 +0100
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, Thomas Monjalon
> >>  <thomas@monjalon.net>
> >> CC: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, Andrew Rybchenko
> >>  <arybchenko@solarflare.com>, "Lu, Wenzhuo" <wenzhuo.lu@intel.com>, "Wu,
> >>  Jingjing" <jingjing.wu@intel.com>, "Iremonger, Bernard"
> >>  <bernard.iremonger@intel.com>, "Mcnamara, John" <john.mcnamara@intel.com>,
> >>  "Kovacevic, Marko" <marko.kovacevic@intel.com>, Olivier Matz
> >>  <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>,
> >>  "shahafs@mellanox.com" <shahafs@mellanox.com>, "didier.pallard@6wind.com"
> >>  <didier.pallard@6wind.com>
> >> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
> >>  checksum definition
> >> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
> >>  Thunderbird/52.9.1
> >>
> >> On 10/8/2018 10:37 AM, Jerin Jacob wrote:
> >>> -----Original Message-----
> >>>> Date: Mon, 08 Oct 2018 11:04:51 +0200
> >>>> From: Thomas Monjalon <thomas@monjalon.net>
> >>>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, Ferruh Yigit
> >>>>  <ferruh.yigit@intel.com>, "Ananyev, Konstantin"
> >>>>  <konstantin.ananyev@intel.com>
> >>>> Cc: Andrew Rybchenko <arybchenko@solarflare.com>, "Lu, Wenzhuo"
> >>>>  <wenzhuo.lu@intel.com>, "Wu, Jingjing" <jingjing.wu@intel.com>,
> >>>>  "Iremonger, Bernard" <bernard.iremonger@intel.com>, "Mcnamara, John"
> >>>>  <john.mcnamara@intel.com>, "Kovacevic, Marko" <marko.kovacevic@intel.com>,
> >>>>  Olivier Matz <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>,
> >>>>  "shahafs@mellanox.com" <shahafs@mellanox.com>, "didier.pallard@6wind.com"
> >>>>  <didier.pallard@6wind.com>
> >>>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
> >>>>  checksum definition
> >>>>
> >>>> 08/10/2018 10:24, Jerin Jacob:
> >>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >>>>>> On 10/6/2018 1:18 PM, Ananyev, Konstantin wrote:
> >>>>>>> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> >>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
> >>>>>>>>> However, we should re-visit the flag PKT_RX_EIP_CKSUM_BAD.
> >>>>>>>>
> >>>>>>>> Do we need to block this patch due to the exiting PKT_RX_EIP_CKSUM_BAD
> >>>>>>>> definition?
> >>>>>>>>
> >>>>>>>> I already added the author of the PKT_RX_EIP_CKSUM_BAD flag and ethdev and mbuf
> >>>>>>>> maintainers in this list. So what else I need make forward progress
> >>>>>>>> on this patch?
> >>>>>>>>
> >>>>>>>> I think, the definition of PKT_RX_EIP_CKSUM_BAD based on HW capability. It
> >>>>>>>> is safe to assume that ALL HW can support CKSUM BAD if the feature is
> >>>>>>>> available and hence it is more portable.
> >>>>>>>
> >>>>>>> Yes, as I remember PKT_RX_EIP_CKSUM_BAD is based on DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM.
> >>>>>>
> >>>>>> Switching to two bit won't reduce the portability, HW supports only reporting
> >>>>>> CKSUM_BAD can set BAD || UNKNOWN.
> >>>>>
> >>>>> UNKNOWN is not a bit. It is represented as 0. It spec has 2 bit, then
> >>>>> driver need to report GOOD as well.
> >>>>>
> >>>>> Same applies for PKT_RX_EL4_CKSUM as well.
> >>>>>
> >>>>>>
> >>>>>> And I think patch is not blocked by PKT_RX_EIP_CKSUM_BAD, it can be changed
> >>>>>> separately, for this patch question is can we represent PKT_RX_EL4_CKSUM_* with
> >>>>>> two bits, to have BAD/GOOD/UNKNOWN?
> >>>>
> >>>> Yes, exact.
> >>>>
> >>>> PKT_RX_EIP_CKSUM_BAD must be left aside.
> >>>> We should just avoid taking it as a reference.
> >>>> And we can reconsider its definition later.
> >>>
> >>> OK.
> >>>
> >>> IMO, Using 2 bit scheme for tunneled checksum has following performance
> >>> issue from driver side.
> >>>
> >>> Driver need to mark the packet as GOOD. All the HW can support
> >>> detection of BAD. That not necessary mean GOOD in case of tunnel packet,
> >>> so driver has to detect the packet is tunneled and packet is not BAD
> >>> then mark GOOD.
> >>
> >> Yes UNKNOWN is not a bit, but a state, why don't use it? Why driver has to check
> >> it is GOOD?
> >
> > The application is going to check is it GOOD or not. Not the driver,
> > Right? My concern was, If application starts dropping the packet instead checking the BAD, if
> > it checks == !GOOD.
> 
> Got it, but when 2 bits state introduced, app should check if check == BAD for
> drop decision, because it is not GOOD || BAD anymore.

Got it.

> 
> >
> >>
> >> 0x0 => UNKNOWN
> >> 0x1 => BAD
> >> 0x2 => GOOD
> >> 0x3 => ? (invalid perhaps)
> >>
> >> HW that supports detecting good packets can set BAD || GOOD state, HW can detect
> >> only BAD packet can set BAD || UNKNOWN state.
> >>
> >> If BAD is not set, there is an ambiguity of state, lets clarify it in lower
> >> level, if it is UNKNOWN, let application know it is UNKNOWN.
> >
> > OK.
> >
> > How about the following then?
> >
> > /**
> >  * Mask of bits used to determine the status of outer RX L4 checksum.
> >  * - PKT_RX_EL4_CKSUM_UNKNOWN: no information about the outer RX L4 checksum
> >  * - PKT_RX_EL4_CKSUM_BAD: the outer L4 checksum in the packet is wrong
> >  * - PKT_RX_EL4_CKSUM_GOOD: the outer L4 checksum in the packet is valid
> >  * - PKT_RX_EL4_CKSUM_INVALID: invalid outer L4 checksum state.
> >  *
> >  * The detection of PKT_RX_EL4_CKSUM_GOOD shall be based on the given
> >  * HW capability, At minimum, the PMD should support
> >  * PKT_RX_EL4_CKSUM_UNKNOWN  and PKT_RX_EL4_CKSUM_BAD states
> >  * if the offload is available.
> >  */
> > #define PKT_RX_EL4_CKSUM_MASK   ((1ULL << 21) | (1ULL << 22))
> >
> > #define PKT_RX_IP_CKSUM_UNKNOWN 0
> > #define PKT_RX_IP_CKSUM_BAD     (1ULL << 21)
> > #define PKT_RX_IP_CKSUM_GOOD    (1ULL << 22)
> > #define PKT_RX_IP_CKSUM_INVALID ((1ULL << 21) | (1ULL << 22))
> 
> Looks good to me.

If there is no objection with above flag definition, I will send the v3 with that.

> 
> 
> >
> >
> >
>
Thomas Monjalon Oct. 8, 2018, 1:03 p.m. UTC | #33 
08/10/2018 14:25, Jerin Jacob:
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> > On 10/8/2018 12:55 PM, Jerin Jacob wrote:
> > > From: Ferruh Yigit <ferruh.yigit@intel.com>
> > >> On 10/8/2018 10:37 AM, Jerin Jacob wrote:
> > >>> From: Thomas Monjalon <thomas@monjalon.net>
> > >>>> 08/10/2018 10:24, Jerin Jacob:
> > >>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
> > >>>>>> On 10/6/2018 1:18 PM, Ananyev, Konstantin wrote:
> > >>>>>>> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > >>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
> > >>>>>>>>> However, we should re-visit the flag PKT_RX_EIP_CKSUM_BAD.
> > >>>>>>>>
> > >>>>>>>> Do we need to block this patch due to the exiting PKT_RX_EIP_CKSUM_BAD
> > >>>>>>>> definition?
> > >>>>>>>>
> > >>>>>>>> I already added the author of the PKT_RX_EIP_CKSUM_BAD flag and ethdev and mbuf
> > >>>>>>>> maintainers in this list. So what else I need make forward progress
> > >>>>>>>> on this patch?
> > >>>>>>>>
> > >>>>>>>> I think, the definition of PKT_RX_EIP_CKSUM_BAD based on HW capability. It
> > >>>>>>>> is safe to assume that ALL HW can support CKSUM BAD if the feature is
> > >>>>>>>> available and hence it is more portable.
> > >>>>>>>
> > >>>>>>> Yes, as I remember PKT_RX_EIP_CKSUM_BAD is based on DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM.
> > >>>>>>
> > >>>>>> Switching to two bit won't reduce the portability, HW supports only reporting
> > >>>>>> CKSUM_BAD can set BAD || UNKNOWN.
> > >>>>>
> > >>>>> UNKNOWN is not a bit. It is represented as 0. It spec has 2 bit, then
> > >>>>> driver need to report GOOD as well.
> > >>>>>
> > >>>>> Same applies for PKT_RX_EL4_CKSUM as well.
> > >>>>>
> > >>>>>>
> > >>>>>> And I think patch is not blocked by PKT_RX_EIP_CKSUM_BAD, it can be changed
> > >>>>>> separately, for this patch question is can we represent PKT_RX_EL4_CKSUM_* with
> > >>>>>> two bits, to have BAD/GOOD/UNKNOWN?
> > >>>>
> > >>>> Yes, exact.
> > >>>>
> > >>>> PKT_RX_EIP_CKSUM_BAD must be left aside.
> > >>>> We should just avoid taking it as a reference.
> > >>>> And we can reconsider its definition later.
> > >>>
> > >>> OK.
> > >>>
> > >>> IMO, Using 2 bit scheme for tunneled checksum has following performance
> > >>> issue from driver side.
> > >>>
> > >>> Driver need to mark the packet as GOOD. All the HW can support
> > >>> detection of BAD. That not necessary mean GOOD in case of tunnel packet,
> > >>> so driver has to detect the packet is tunneled and packet is not BAD
> > >>> then mark GOOD.
> > >>
> > >> Yes UNKNOWN is not a bit, but a state, why don't use it? Why driver has to check
> > >> it is GOOD?
> > >
> > > The application is going to check is it GOOD or not. Not the driver,
> > > Right? My concern was, If application starts dropping the packet instead checking the BAD, if
> > > it checks == !GOOD.
> > 
> > Got it, but when 2 bits state introduced, app should check if check == BAD for
> > drop decision, because it is not GOOD || BAD anymore.
> 
> Got it.
> 
> > 
> > >
> > >>
> > >> 0x0 => UNKNOWN
> > >> 0x1 => BAD
> > >> 0x2 => GOOD
> > >> 0x3 => ? (invalid perhaps)
> > >>
> > >> HW that supports detecting good packets can set BAD || GOOD state, HW can detect
> > >> only BAD packet can set BAD || UNKNOWN state.
> > >>
> > >> If BAD is not set, there is an ambiguity of state, lets clarify it in lower
> > >> level, if it is UNKNOWN, let application know it is UNKNOWN.
> > >
> > > OK.
> > >
> > > How about the following then?
> > >
> > > /**
> > >  * Mask of bits used to determine the status of outer RX L4 checksum.
> > >  * - PKT_RX_EL4_CKSUM_UNKNOWN: no information about the outer RX L4 checksum
> > >  * - PKT_RX_EL4_CKSUM_BAD: the outer L4 checksum in the packet is wrong
> > >  * - PKT_RX_EL4_CKSUM_GOOD: the outer L4 checksum in the packet is valid
> > >  * - PKT_RX_EL4_CKSUM_INVALID: invalid outer L4 checksum state.
> > >  *
> > >  * The detection of PKT_RX_EL4_CKSUM_GOOD shall be based on the given
> > >  * HW capability, At minimum, the PMD should support
> > >  * PKT_RX_EL4_CKSUM_UNKNOWN  and PKT_RX_EL4_CKSUM_BAD states
> > >  * if the offload is available.
> > >  */
> > > #define PKT_RX_EL4_CKSUM_MASK   ((1ULL << 21) | (1ULL << 22))
> > >
> > > #define PKT_RX_IP_CKSUM_UNKNOWN 0
> > > #define PKT_RX_IP_CKSUM_BAD     (1ULL << 21)
> > > #define PKT_RX_IP_CKSUM_GOOD    (1ULL << 22)
> > > #define PKT_RX_IP_CKSUM_INVALID ((1ULL << 21) | (1ULL << 22))
> > 
> > Looks good to me.
> 
> If there is no objection with above flag definition, I will send the v3 with that.

Just one objection about the name.
Why naming it EL4 and commenting as outer L4?
I think we should choose between "external" and "outer".
Convention seems to be choosing "outer" word.
So I suggest PKT_RX_OUTER_L4_CKSUM_*.
Jerin Jacob Oct. 8, 2018, 1:08 p.m. UTC | #34 
-----Original Message-----
> Date: Mon, 08 Oct 2018 15:03:49 +0200
> From: Thomas Monjalon <thomas@monjalon.net>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>, "Ananyev, Konstantin"
>  <konstantin.ananyev@intel.com>, Andrew Rybchenko
>  <arybchenko@solarflare.com>, "Lu, Wenzhuo" <wenzhuo.lu@intel.com>, "Wu,
>  Jingjing" <jingjing.wu@intel.com>, "Iremonger, Bernard"
>  <bernard.iremonger@intel.com>, "Mcnamara, John" <john.mcnamara@intel.com>,
>  "Kovacevic, Marko" <marko.kovacevic@intel.com>, Olivier Matz
>  <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>,
>  "shahafs@mellanox.com" <shahafs@mellanox.com>, "didier.pallard@6wind.com"
>  <didier.pallard@6wind.com>
> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>  checksum definition
> 
> 08/10/2018 14:25, Jerin Jacob:
> > From: Ferruh Yigit <ferruh.yigit@intel.com>
> > > On 10/8/2018 12:55 PM, Jerin Jacob wrote:
> > > > From: Ferruh Yigit <ferruh.yigit@intel.com>
> > > >> On 10/8/2018 10:37 AM, Jerin Jacob wrote:
> > > >>> From: Thomas Monjalon <thomas@monjalon.net>
> > > >>>> 08/10/2018 10:24, Jerin Jacob:
> > > >>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
> > > >>>>>> On 10/6/2018 1:18 PM, Ananyev, Konstantin wrote:
> > > >>>>>>> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
> > > >>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
> > > >>>>>>>>> However, we should re-visit the flag PKT_RX_EIP_CKSUM_BAD.
> > > >>>>>>>>
> > > >>>>>>>> Do we need to block this patch due to the exiting PKT_RX_EIP_CKSUM_BAD
> > > >>>>>>>> definition?
> > > >>>>>>>>
> > > >>>>>>>> I already added the author of the PKT_RX_EIP_CKSUM_BAD flag and ethdev and mbuf
> > > >>>>>>>> maintainers in this list. So what else I need make forward progress
> > > >>>>>>>> on this patch?
> > > >>>>>>>>
> > > >>>>>>>> I think, the definition of PKT_RX_EIP_CKSUM_BAD based on HW capability. It
> > > >>>>>>>> is safe to assume that ALL HW can support CKSUM BAD if the feature is
> > > >>>>>>>> available and hence it is more portable.
> > > >>>>>>>
> > > >>>>>>> Yes, as I remember PKT_RX_EIP_CKSUM_BAD is based on DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM.
> > > >>>>>>
> > > >>>>>> Switching to two bit won't reduce the portability, HW supports only reporting
> > > >>>>>> CKSUM_BAD can set BAD || UNKNOWN.
> > > >>>>>
> > > >>>>> UNKNOWN is not a bit. It is represented as 0. It spec has 2 bit, then
> > > >>>>> driver need to report GOOD as well.
> > > >>>>>
> > > >>>>> Same applies for PKT_RX_EL4_CKSUM as well.
> > > >>>>>
> > > >>>>>>
> > > >>>>>> And I think patch is not blocked by PKT_RX_EIP_CKSUM_BAD, it can be changed
> > > >>>>>> separately, for this patch question is can we represent PKT_RX_EL4_CKSUM_* with
> > > >>>>>> two bits, to have BAD/GOOD/UNKNOWN?
> > > >>>>
> > > >>>> Yes, exact.
> > > >>>>
> > > >>>> PKT_RX_EIP_CKSUM_BAD must be left aside.
> > > >>>> We should just avoid taking it as a reference.
> > > >>>> And we can reconsider its definition later.
> > > >>>
> > > >>> OK.
> > > >>>
> > > >>> IMO, Using 2 bit scheme for tunneled checksum has following performance
> > > >>> issue from driver side.
> > > >>>
> > > >>> Driver need to mark the packet as GOOD. All the HW can support
> > > >>> detection of BAD. That not necessary mean GOOD in case of tunnel packet,
> > > >>> so driver has to detect the packet is tunneled and packet is not BAD
> > > >>> then mark GOOD.
> > > >>
> > > >> Yes UNKNOWN is not a bit, but a state, why don't use it? Why driver has to check
> > > >> it is GOOD?
> > > >
> > > > The application is going to check is it GOOD or not. Not the driver,
> > > > Right? My concern was, If application starts dropping the packet instead checking the BAD, if
> > > > it checks == !GOOD.
> > >
> > > Got it, but when 2 bits state introduced, app should check if check == BAD for
> > > drop decision, because it is not GOOD || BAD anymore.
> >
> > Got it.
> >
> > >
> > > >
> > > >>
> > > >> 0x0 => UNKNOWN
> > > >> 0x1 => BAD
> > > >> 0x2 => GOOD
> > > >> 0x3 => ? (invalid perhaps)
> > > >>
> > > >> HW that supports detecting good packets can set BAD || GOOD state, HW can detect
> > > >> only BAD packet can set BAD || UNKNOWN state.
> > > >>
> > > >> If BAD is not set, there is an ambiguity of state, lets clarify it in lower
> > > >> level, if it is UNKNOWN, let application know it is UNKNOWN.
> > > >
> > > > OK.
> > > >
> > > > How about the following then?
> > > >
> > > > /**
> > > >  * Mask of bits used to determine the status of outer RX L4 checksum.
> > > >  * - PKT_RX_EL4_CKSUM_UNKNOWN: no information about the outer RX L4 checksum
> > > >  * - PKT_RX_EL4_CKSUM_BAD: the outer L4 checksum in the packet is wrong
> > > >  * - PKT_RX_EL4_CKSUM_GOOD: the outer L4 checksum in the packet is valid
> > > >  * - PKT_RX_EL4_CKSUM_INVALID: invalid outer L4 checksum state.
> > > >  *
> > > >  * The detection of PKT_RX_EL4_CKSUM_GOOD shall be based on the given
> > > >  * HW capability, At minimum, the PMD should support
> > > >  * PKT_RX_EL4_CKSUM_UNKNOWN  and PKT_RX_EL4_CKSUM_BAD states
> > > >  * if the offload is available.
> > > >  */
> > > > #define PKT_RX_EL4_CKSUM_MASK   ((1ULL << 21) | (1ULL << 22))
> > > >
> > > > #define PKT_RX_IP_CKSUM_UNKNOWN 0
> > > > #define PKT_RX_IP_CKSUM_BAD     (1ULL << 21)
> > > > #define PKT_RX_IP_CKSUM_GOOD    (1ULL << 22)
> > > > #define PKT_RX_IP_CKSUM_INVALID ((1ULL << 21) | (1ULL << 22))
> > >
> > > Looks good to me.
> >
> > If there is no objection with above flag definition, I will send the v3 with that.
> 
> Just one objection about the name.
> Why naming it EL4 and commenting as outer L4?
> I think we should choose between "external" and "outer".
> Convention seems to be choosing "outer" word.
> So I suggest PKT_RX_OUTER_L4_CKSUM_*.

OK. I will change to PKT_RX_OUTER_L4_CKSUM_*

> 
> 
>
diff mbox series

Patch

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 1adc9b94b..d53c527e5 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -594,6 +594,15 @@  port_offload_cap_display(portid_t port_id)
 			printf("off\n");
 	}
 
+	if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_OUTER_UDP_CKSUM) {
+		printf("RX Outer UDP checksum:               ");
+		if (ports[port_id].dev_conf.rxmode.offloads &
+		    DEV_RX_OFFLOAD_OUTER_UDP_CKSUM)
+			printf("on\n");
+		else
+			printf("off\n");
+	}
+
 	if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_TCP_LRO) {
 		printf("Large receive offload:         ");
 		if (ports[port_id].dev_conf.rxmode.offloads &
diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst
index d42489b6d..2c2959e0b 100644
--- a/doc/guides/nics/features.rst
+++ b/doc/guides/nics/features.rst
@@ -639,6 +639,9 @@  Inner L4 checksum
 
 Supports inner packet L4 checksum.
 
+* **[uses]     rte_eth_rxconf,rte_eth_rxmode**: ``offloads:DEV_RX_OFFLOAD_OUTER_UDP_CKSUM``.
+* **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_EL4_CKSUM_BAD``.
+* **[provides] rte_eth_dev_info**: ``rx_offload_capa,rx_queue_offload_capa:DEV_RX_OFFLOAD_OUTER_UDP_CKSUM``,
 
 .. _nic_features_packet_type_parsing:
 
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index e9a82fe7f..a630c4fda 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -127,6 +127,7 @@  static const struct {
 	RTE_RX_OFFLOAD_BIT2STR(SECURITY),
 	RTE_RX_OFFLOAD_BIT2STR(KEEP_CRC),
 	RTE_RX_OFFLOAD_BIT2STR(SCTP_CKSUM),
+	RTE_RX_OFFLOAD_BIT2STR(OUTER_UDP_CKSUM),
 };
 
 #undef RTE_RX_OFFLOAD_BIT2STR
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index d02db14ad..821d371c3 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -889,6 +889,7 @@  struct rte_eth_conf {
 #define DEV_RX_OFFLOAD_SECURITY         0x00008000
 #define DEV_RX_OFFLOAD_KEEP_CRC		0x00010000
 #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
+#define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
 
 #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
 				 DEV_RX_OFFLOAD_UDP_CKSUM | \
diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index e714c5a59..022e92b3c 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -301,6 +301,7 @@  const char *rte_get_rx_ol_flag_name(uint64_t mask)
 	case PKT_RX_TIMESTAMP: return "PKT_RX_TIMESTAMP";
 	case PKT_RX_SEC_OFFLOAD: return "PKT_RX_SEC_OFFLOAD";
 	case PKT_RX_SEC_OFFLOAD_FAILED: return "PKT_RX_SEC_OFFLOAD_FAILED";
+	case PKT_RX_EL4_CKSUM_BAD: return "PKT_RX_EL4_CKSUM_BAD";
 	default: return NULL;
 	}
 }
@@ -339,6 +340,7 @@  rte_get_rx_ol_flag_list(uint64_t mask, char *buf, size_t buflen)
 		{ PKT_RX_SEC_OFFLOAD, PKT_RX_SEC_OFFLOAD, NULL },
 		{ PKT_RX_SEC_OFFLOAD_FAILED, PKT_RX_SEC_OFFLOAD_FAILED, NULL },
 		{ PKT_RX_QINQ, PKT_RX_QINQ, NULL },
+		{ PKT_RX_EL4_CKSUM_BAD, PKT_RX_EL4_CKSUM_BAD, NULL },
 	};
 	const char *name;
 	unsigned int i;
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index eb11779e7..5c03e98b3 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -177,6 +177,9 @@  extern "C" {
  */
 #define PKT_RX_QINQ          (1ULL << 20)
 
+/**< External/Outer Layer4 header checksum error. */
+#define PKT_RX_EL4_CKSUM_BAD (1ULL << 21)
+
 /* add new RX flags here */
 
 /* add new TX flags here */