[dpdk-dev] [PATCH 0/5] implement new Rx checksum flag

[dpdk-dev] [PATCH 0/5] implement new Rx checksum flag

[Xiao Wang]

Following http://dpdk.org/dev/patchwork/patch/14941/, this patch set
implements newly defined Rx checksum flag for igb, ixgbe, i40e and fm10k.

Currently, fm10k supports Rx checksum offload in both scalar and vector
pmd, while the other three don't, this patch set keeps the situation.

This patch set has dependency on the following patch:
"mbuf: add new Rx checksum mbuf flags"
(http://dpdk.org/dev/patchwork/patch/14941/)

Xiao Wang (5):
  net/fm10k: add back Rx checksum offload
  net/fm10k: implement new Rx checksum flag
  net/e1000: implement new Rx checksum flag
  net/ixgbe: implement new Rx checksum flag
  net/i40e: implement new Rx checksum flag

 drivers/net/e1000/igb_rxtx.c       |  4 +++-
 drivers/net/fm10k/fm10k_rxtx.c     | 14 ++++++++++++++
 drivers/net/fm10k/fm10k_rxtx_vec.c |  7 +++++--
 drivers/net/i40e/i40e_rxtx.c       |  6 ++++++
 drivers/net/ixgbe/ixgbe_rxtx.c     |  4 +++-
 5 files changed, 31 insertions(+), 4 deletions(-)

-- 
1.9.3

[dpdk-dev] [PATCH 1/5] net/fm10k: add back Rx checksum offload

[Xiao Wang]

A previous patch (http://dpdk.org/dev/patchwork/patch/12937/) removed
some necessary lines about Rx checksum by mistake, this patch adds
them back.

Fixes: 6046898f5097 ("net/mbuf: remove unused Rx error flags")

Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>
---
 drivers/net/fm10k/fm10k_rxtx.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/fm10k/fm10k_rxtx.c b/drivers/net/fm10k/fm10k_rxtx.c
index 5b2d04b..bf5888b 100644
--- a/drivers/net/fm10k/fm10k_rxtx.c
+++ b/drivers/net/fm10k/fm10k_rxtx.c
@@ -96,6 +96,16 @@ rx_desc_to_ol_flags(struct rte_mbuf *m, const union fm10k_rx_desc *d)
 
 	if (d->w.pkt_info & FM10K_RXD_RSSTYPE_MASK)
 		m->ol_flags |= PKT_RX_RSS_HASH;
+
+	if (unlikely((d->d.staterr &
+		(FM10K_RXD_STATUS_IPCS | FM10K_RXD_STATUS_IPE)) ==
+		(FM10K_RXD_STATUS_IPCS | FM10K_RXD_STATUS_IPE)))
+		m->ol_flags |= PKT_RX_IP_CKSUM_BAD;
+
+	if (unlikely((d->d.staterr &
+		(FM10K_RXD_STATUS_L4CS | FM10K_RXD_STATUS_L4E)) ==
+		(FM10K_RXD_STATUS_L4CS | FM10K_RXD_STATUS_L4E)))
+		m->ol_flags |= PKT_RX_L4_CKSUM_BAD;
 }
 
 uint16_t
-- 
1.9.3

Re: [dpdk-dev] [PATCH 1/5] net/fm10k: add back Rx checksum offload

[Olivier Matz]

Hi Xiao,

On 08/25/2016 07:48 PM, Xiao Wang wrote:
> A previous patch (http://dpdk.org/dev/patchwork/patch/12937/) removed
> some necessary lines about Rx checksum by mistake, this patch adds
> them back.
> 
> Fixes: 6046898f5097 ("net/mbuf: remove unused Rx error flags")
> 
> Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>

Just a small comment about the title, what do you thinkg about:
"net/fm10k: fix Rx checksum flags" ?
I think having the word "fix" in the title would help for finding these
commits for the stable version.

Apart from that, thanks for fixing my mistake :)


Regards,
Olivier

Re: [dpdk-dev] [PATCH 1/5] net/fm10k: add back Rx checksum offload

[Wang, Xiao W]

Hi Olivier,

> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Friday, August 26, 2016 3:27 PM
> To: Wang, Xiao W <xiao.w.wang@intel.com>; Chen, Jing D
> <jing.d.chen@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH 1/5] net/fm10k: add back Rx checksum offload
> 
> Hi Xiao,
> 
> On 08/25/2016 07:48 PM, Xiao Wang wrote:
> > A previous patch (http://dpdk.org/dev/patchwork/patch/12937/) removed
> > some necessary lines about Rx checksum by mistake, this patch adds
> > them back.
> >
> > Fixes: 6046898f5097 ("net/mbuf: remove unused Rx error flags")
> >
> > Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>
> 
> Just a small comment about the title, what do you thinkg about:
> "net/fm10k: fix Rx checksum flags" ?
> I think having the word "fix" in the title would help for finding these
> commits for the stable version.
> 
> Apart from that, thanks for fixing my mistake :)
> 
> 
> Regards,
> Olivier

I agree with you, will change the commit log in V2.

Best Regards,
Xiao

[dpdk-dev] [PATCH 2/5] net/fm10k: implement new Rx checksum flag

[Xiao Wang]

Add CKSUM_GOOD flag to distinguish a good checksum from an unknown one.

Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>
---
 drivers/net/fm10k/fm10k_rxtx.c     | 4 ++++
 drivers/net/fm10k/fm10k_rxtx_vec.c | 7 +++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/fm10k/fm10k_rxtx.c b/drivers/net/fm10k/fm10k_rxtx.c
index bf5888b..32cc7ff 100644
--- a/drivers/net/fm10k/fm10k_rxtx.c
+++ b/drivers/net/fm10k/fm10k_rxtx.c
@@ -101,11 +101,15 @@ rx_desc_to_ol_flags(struct rte_mbuf *m, const union fm10k_rx_desc *d)
 		(FM10K_RXD_STATUS_IPCS | FM10K_RXD_STATUS_IPE)) ==
 		(FM10K_RXD_STATUS_IPCS | FM10K_RXD_STATUS_IPE)))
 		m->ol_flags |= PKT_RX_IP_CKSUM_BAD;
+	else
+		m->ol_flags |= PKT_RX_IP_CKSUM_GOOD;
 
 	if (unlikely((d->d.staterr &
 		(FM10K_RXD_STATUS_L4CS | FM10K_RXD_STATUS_L4E)) ==
 		(FM10K_RXD_STATUS_L4CS | FM10K_RXD_STATUS_L4E)))
 		m->ol_flags |= PKT_RX_L4_CKSUM_BAD;
+	else
+		m->ol_flags |= PKT_RX_L4_CKSUM_GOOD;
 }
 
 uint16_t
diff --git a/drivers/net/fm10k/fm10k_rxtx_vec.c b/drivers/net/fm10k/fm10k_rxtx_vec.c
index 9ea747e..8c08b44 100644
--- a/drivers/net/fm10k/fm10k_rxtx_vec.c
+++ b/drivers/net/fm10k/fm10k_rxtx_vec.c
@@ -95,8 +95,10 @@ fm10k_desc_to_olflags_v(__m128i descs[4], struct rte_mbuf **rx_pkts)
 	const __m128i l3l4cksum_flag = _mm_set_epi8(0, 0, 0, 0,
 			0, 0, 0, 0,
 			0, 0, 0, 0,
-			PKT_RX_IP_CKSUM_BAD | PKT_RX_L4_CKSUM_BAD,
-			PKT_RX_IP_CKSUM_BAD, PKT_RX_L4_CKSUM_BAD, 0);
+			(PKT_RX_IP_CKSUM_BAD | PKT_RX_L4_CKSUM_BAD) >> 1,
+			(PKT_RX_IP_CKSUM_BAD | PKT_RX_L4_CKSUM_GOOD) >> 1,
+			(PKT_RX_IP_CKSUM_GOOD | PKT_RX_L4_CKSUM_BAD) >> 1,
+			(PKT_RX_IP_CKSUM_GOOD | PKT_RX_L4_CKSUM_GOOD) >> 1);
 
 	const __m128i rxe_flag = _mm_set_epi8(0, 0, 0, 0,
 			0, 0, 0, 0,
@@ -139,6 +141,7 @@ fm10k_desc_to_olflags_v(__m128i descs[4], struct rte_mbuf **rx_pkts)
 	/* Process L4/L3 checksum error flags */
 	cksumflag = _mm_srli_epi16(cksumflag, L3L4EFLAG_SHIFT);
 	cksumflag = _mm_shuffle_epi8(l3l4cksum_flag, cksumflag);
+	cksumflag = _mm_slli_epi16(cksumflag, 1);
 	vtag1 = _mm_or_si128(cksumflag, vtag1);
 
 	vol.dword = _mm_cvtsi128_si64(vtag1);
-- 
1.9.3

Re: [dpdk-dev] [PATCH 2/5] net/fm10k: implement new Rx checksum flag

[Chen, Jing D]

Hi,

>  uint16_t
> diff --git a/drivers/net/fm10k/fm10k_rxtx_vec.c
> b/drivers/net/fm10k/fm10k_rxtx_vec.c
> index 9ea747e..8c08b44 100644
> --- a/drivers/net/fm10k/fm10k_rxtx_vec.c
> +++ b/drivers/net/fm10k/fm10k_rxtx_vec.c
> @@ -95,8 +95,10 @@ fm10k_desc_to_olflags_v(__m128i descs[4], struct rte_mbuf
> **rx_pkts)
>  	const __m128i l3l4cksum_flag = _mm_set_epi8(0, 0, 0, 0,
>  			0, 0, 0, 0,
>  			0, 0, 0, 0,
> -			PKT_RX_IP_CKSUM_BAD | PKT_RX_L4_CKSUM_BAD,
> -			PKT_RX_IP_CKSUM_BAD, PKT_RX_L4_CKSUM_BAD, 0);
> +			(PKT_RX_IP_CKSUM_BAD | PKT_RX_L4_CKSUM_BAD) >> 1,
> +			(PKT_RX_IP_CKSUM_BAD | PKT_RX_L4_CKSUM_GOOD) >>
> 1,
> +			(PKT_RX_IP_CKSUM_GOOD | PKT_RX_L4_CKSUM_BAD) >>
> 1,
> +			(PKT_RX_IP_CKSUM_GOOD |
> PKT_RX_L4_CKSUM_GOOD) >> 1);

Can we define a macro, like "#define RTE_CKSUM_SHIFT 1" to avoid numeric?

> 
>  	const __m128i rxe_flag = _mm_set_epi8(0, 0, 0, 0,
>  			0, 0, 0, 0,
> @@ -139,6 +141,7 @@ fm10k_desc_to_olflags_v(__m128i descs[4], struct
> rte_mbuf **rx_pkts)
>  	/* Process L4/L3 checksum error flags */
>  	cksumflag = _mm_srli_epi16(cksumflag, L3L4EFLAG_SHIFT);
>  	cksumflag = _mm_shuffle_epi8(l3l4cksum_flag, cksumflag);
> +	cksumflag = _mm_slli_epi16(cksumflag, 1);
>  	vtag1 = _mm_or_si128(cksumflag, vtag1);
> 
>  	vol.dword = _mm_cvtsi128_si64(vtag1);
> --
> 1.9.3

Besides that, just realize we should remove "hw_ip_checksum" check in func
fm10k_rx_vec_condition_check() since we already support it.
Can you help to make the change?

Re: [dpdk-dev] [PATCH 2/5] net/fm10k: implement new Rx checksum flag

[Wang, Xiao W]

Hi Mark,

> -----Original Message-----
> From: Chen, Jing D
> Sent: Monday, August 29, 2016 5:33 PM
> To: Wang, Xiao W <xiao.w.wang@intel.com>; olivier.matz@6wind.com
> Cc: dev@dpdk.org
> Subject: RE: [PATCH 2/5] net/fm10k: implement new Rx checksum flag
> 
> Hi,
> 
> >  uint16_t
> > diff --git a/drivers/net/fm10k/fm10k_rxtx_vec.c
> > b/drivers/net/fm10k/fm10k_rxtx_vec.c
> > index 9ea747e..8c08b44 100644
> > --- a/drivers/net/fm10k/fm10k_rxtx_vec.c
> > +++ b/drivers/net/fm10k/fm10k_rxtx_vec.c
> > @@ -95,8 +95,10 @@ fm10k_desc_to_olflags_v(__m128i descs[4], struct
> rte_mbuf
> > **rx_pkts)
> >  	const __m128i l3l4cksum_flag = _mm_set_epi8(0, 0, 0, 0,
> >  			0, 0, 0, 0,
> >  			0, 0, 0, 0,
> > -			PKT_RX_IP_CKSUM_BAD | PKT_RX_L4_CKSUM_BAD,
> > -			PKT_RX_IP_CKSUM_BAD, PKT_RX_L4_CKSUM_BAD, 0);
> > +			(PKT_RX_IP_CKSUM_BAD |
> PKT_RX_L4_CKSUM_BAD) >> 1,
> > +			(PKT_RX_IP_CKSUM_BAD |
> PKT_RX_L4_CKSUM_GOOD) >>
> > 1,
> > +			(PKT_RX_IP_CKSUM_GOOD |
> PKT_RX_L4_CKSUM_BAD) >>
> > 1,
> > +			(PKT_RX_IP_CKSUM_GOOD |
> > PKT_RX_L4_CKSUM_GOOD) >> 1);
> 
> Can we define a macro, like "#define RTE_CKSUM_SHIFT 1" to avoid numeric?

Yes, I'll add a macro for this, but since this shift operation isn't commonly used
by other pmds (igb and i40e don't support cksum offload in vector Rx, ixgbe cannot
do this shift due to VLAN offload), I will make it a local macro definition.

> 
> >
> >  	const __m128i rxe_flag = _mm_set_epi8(0, 0, 0, 0,
> >  			0, 0, 0, 0,
> > @@ -139,6 +141,7 @@ fm10k_desc_to_olflags_v(__m128i descs[4], struct
> > rte_mbuf **rx_pkts)
> >  	/* Process L4/L3 checksum error flags */
> >  	cksumflag = _mm_srli_epi16(cksumflag, L3L4EFLAG_SHIFT);
> >  	cksumflag = _mm_shuffle_epi8(l3l4cksum_flag, cksumflag);
> > +	cksumflag = _mm_slli_epi16(cksumflag, 1);
> >  	vtag1 = _mm_or_si128(cksumflag, vtag1);
> >
> >  	vol.dword = _mm_cvtsi128_si64(vtag1);
> > --
> > 1.9.3
> 
> Besides that, just realize we should remove "hw_ip_checksum" check in func
> fm10k_rx_vec_condition_check() since we already support it.
> Can you help to make the change?

Sure, will remove it in V2.

Thanks for the comments,
Xiao

[dpdk-dev] [PATCH 3/5] net/e1000: implement new Rx checksum flag

[Xiao Wang]

Add CKSUM_GOOD flag to distinguish a good checksum from an unknown one.

Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>
---
 drivers/net/e1000/igb_rxtx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/e1000/igb_rxtx.c b/drivers/net/e1000/igb_rxtx.c
index 9d80a0b..bc33aed 100644
--- a/drivers/net/e1000/igb_rxtx.c
+++ b/drivers/net/e1000/igb_rxtx.c
@@ -748,7 +748,9 @@ rx_desc_error_to_pkt_flags(uint32_t rx_status)
 	 */
 
 	static uint64_t error_to_pkt_flags_map[4] = {
-		0,  PKT_RX_L4_CKSUM_BAD, PKT_RX_IP_CKSUM_BAD,
+		PKT_RX_IP_CKSUM_GOOD | PKT_RX_L4_CKSUM_GOOD,
+		PKT_RX_IP_CKSUM_GOOD | PKT_RX_L4_CKSUM_BAD,
+		PKT_RX_IP_CKSUM_BAD | PKT_RX_L4_CKSUM_GOOD,
 		PKT_RX_IP_CKSUM_BAD | PKT_RX_L4_CKSUM_BAD
 	};
 	return error_to_pkt_flags_map[(rx_status >>
-- 
1.9.3

[dpdk-dev] [PATCH 4/5] net/ixgbe: implement new Rx checksum flag

[Xiao Wang]

Add CKSUM_GOOD flag to distinguish a good checksum from an unknown one.

Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>
---
 drivers/net/ixgbe/ixgbe_rxtx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index 8a306b0..d2dc82a 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -1345,7 +1345,9 @@ rx_desc_error_to_pkt_flags(uint32_t rx_status)
 	 * Bit 30: L4I, L4I integrity error
 	 */
 	static uint64_t error_to_pkt_flags_map[4] = {
-		0,  PKT_RX_L4_CKSUM_BAD, PKT_RX_IP_CKSUM_BAD,
+		PKT_RX_IP_CKSUM_GOOD | PKT_RX_L4_CKSUM_GOOD,
+		PKT_RX_IP_CKSUM_GOOD | PKT_RX_L4_CKSUM_BAD,
+		PKT_RX_IP_CKSUM_BAD | PKT_RX_L4_CKSUM_GOOD,
 		PKT_RX_IP_CKSUM_BAD | PKT_RX_L4_CKSUM_BAD
 	};
 	pkt_flags = error_to_pkt_flags_map[(rx_status >>
-- 
1.9.3

Re: [dpdk-dev] [PATCH 4/5] net/ixgbe: implement new Rx checksum flag

[Olivier Matz]

Hi Xiao,

On 08/25/2016 07:48 PM, Xiao Wang wrote:
> Add CKSUM_GOOD flag to distinguish a good checksum from an unknown one.
> 
> Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>
> ---
>  drivers/net/ixgbe/ixgbe_rxtx.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
> index 8a306b0..d2dc82a 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> @@ -1345,7 +1345,9 @@ rx_desc_error_to_pkt_flags(uint32_t rx_status)
>  	 * Bit 30: L4I, L4I integrity error
>  	 */
>  	static uint64_t error_to_pkt_flags_map[4] = {
> -		0,  PKT_RX_L4_CKSUM_BAD, PKT_RX_IP_CKSUM_BAD,
> +		PKT_RX_IP_CKSUM_GOOD | PKT_RX_L4_CKSUM_GOOD,
> +		PKT_RX_IP_CKSUM_GOOD | PKT_RX_L4_CKSUM_BAD,
> +		PKT_RX_IP_CKSUM_BAD | PKT_RX_L4_CKSUM_GOOD,
>  		PKT_RX_IP_CKSUM_BAD | PKT_RX_L4_CKSUM_BAD
>  	};
>  	pkt_flags = error_to_pkt_flags_map[(rx_status >>
> 

I think this would somehow conflict with the patch adding the support of
Rx checksum offload in vector receive function:

http://dpdk.org/dev/patchwork/patch/14630/

Depending on which one is pushed first, the second one would need to be
reworked.

Olivier

Re: [dpdk-dev] [PATCH 4/5] net/ixgbe: implement new Rx checksum flag

[Wang, Xiao W]

Hi Olivier,

> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Friday, August 26, 2016 3:30 PM
> To: Wang, Xiao W <xiao.w.wang@intel.com>; Chen, Jing D
> <jing.d.chen@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH 4/5] net/ixgbe: implement new Rx checksum flag
> 
> Hi Xiao,
> 
> On 08/25/2016 07:48 PM, Xiao Wang wrote:
> > Add CKSUM_GOOD flag to distinguish a good checksum from an unknown
> one.
> >
> > Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>
> > ---
> >  drivers/net/ixgbe/ixgbe_rxtx.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
> > index 8a306b0..d2dc82a 100644
> > --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> > +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> > @@ -1345,7 +1345,9 @@ rx_desc_error_to_pkt_flags(uint32_t rx_status)
> >  	 * Bit 30: L4I, L4I integrity error
> >  	 */
> >  	static uint64_t error_to_pkt_flags_map[4] = {
> > -		0,  PKT_RX_L4_CKSUM_BAD, PKT_RX_IP_CKSUM_BAD,
> > +		PKT_RX_IP_CKSUM_GOOD | PKT_RX_L4_CKSUM_GOOD,
> > +		PKT_RX_IP_CKSUM_GOOD | PKT_RX_L4_CKSUM_BAD,
> > +		PKT_RX_IP_CKSUM_BAD | PKT_RX_L4_CKSUM_GOOD,
> >  		PKT_RX_IP_CKSUM_BAD | PKT_RX_L4_CKSUM_BAD
> >  	};
> >  	pkt_flags = error_to_pkt_flags_map[(rx_status >>
> >
> 
> I think this would somehow conflict with the patch adding the support of
> Rx checksum offload in vector receive function:
> 
> http://dpdk.org/dev/patchwork/patch/14630/
> 
> Depending on which one is pushed first, the second one would need to be
> reworked.
> 
> Olivier

Yes, since your patch supports cksum in vector Rx, I'll add the new cksum flags for it
based on your patch.

Best Regards,
Xiao

[dpdk-dev] [PATCH 5/5] net/i40e: implement new Rx checksum flag

[Xiao Wang]

Add CKSUM_GOOD flag to distinguish a good checksum from an unknown one.

Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>
---
 drivers/net/i40e/i40e_rxtx.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 554d167..b49d9dc 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -142,8 +142,14 @@ i40e_rxd_error_to_pkt_flags(uint64_t qword)
 		return flags;
 	if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_IPE_SHIFT)))
 		flags |= PKT_RX_IP_CKSUM_BAD;
+	else
+		flags |= PKT_RX_IP_CKSUM_GOOD;
+
 	if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_L4E_SHIFT)))
 		flags |= PKT_RX_L4_CKSUM_BAD;
+	else
+		flags |= PKT_RX_L4_CKSUM_GOOD;
+
 	if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_EIPE_SHIFT)))
 		flags |= PKT_RX_EIP_CKSUM_BAD;
 
-- 
1.9.3