diff mbox

[dpdk-dev] examples/ipsec-secgw: update incremental checksum

Message ID 20180115124212.7011-1-akhil.goyal@nxp.com (mailing list archive)
State Accepted, archived
Delegated to: Pablo de Lara Guarch
Headers

Checks

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

Commit Message

Akhil Goyal Jan. 15, 2018, 12:42 p.m. UTC 
  When TTL is decremented or ecn is updated in IP header
before forwarding the packet, checksum needs to be updated.

In this patch an incremental checksum is added for ipv4 case.

Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
---
 examples/ipsec-secgw/ipip.h | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

Comments

Akhil Goyal Jan. 15, 2018, 12:48 p.m. UTC | #1 
On 1/15/2018 6:12 PM, Akhil Goyal wrote:
> When TTL is decremented or ecn is updated in IP header
> before forwarding the packet, checksum needs to be updated.
> 
> In this patch an incremental checksum is added for ipv4 case.
> 
> Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
> ---
This patch is an update to a very old patch which was rejected earlier.
http://dpdk.org/dev/patchwork/patch/16113/

>   examples/ipsec-secgw/ipip.h | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/examples/ipsec-secgw/ipip.h b/examples/ipsec-secgw/ipip.h
> index fb6a6fa..13b8455 100644
> --- a/examples/ipsec-secgw/ipip.h
> +++ b/examples/ipsec-secgw/ipip.h
> @@ -27,6 +27,10 @@ ipip_outbound(struct rte_mbuf *m, uint32_t offset, uint32_t is_ipv6,
>   	if (inip4->ip_v == IPVERSION) {
>   		/* XXX This should be done by the forwarding engine instead */
>   		inip4->ip_ttl -= 1;
> +		if (inip4->ip_sum >= rte_cpu_to_be_16(0xffff - 0x100))
> +			inip4->ip_sum += rte_cpu_to_be_16(0x100) + 1;
> +		else
> +			inip4->ip_sum += rte_cpu_to_be_16(0x100);
>   		ds_ecn = inip4->ip_tos;
>   	} else {
>   		inip6 = (struct ip6_hdr *)inip4;
> @@ -95,8 +99,17 @@ ip6ip_outbound(struct rte_mbuf *m, uint32_t offset,
>   static inline void
>   ip4_ecn_setup(struct ip *ip4)
>   {
> -	if (ip4->ip_tos & IPTOS_ECN_MASK)
> +	if (ip4->ip_tos & IPTOS_ECN_MASK) {
> +		unsigned long sum;
> +		uint8_t old;
> +
> +		old = ip4->ip_tos;
>   		ip4->ip_tos |= IPTOS_ECN_CE;
> +		sum = old + (~(*(uint8_t *)&ip4->ip_tos) & 0xff);
> +		sum += rte_be_to_cpu_16(ip4->ip_sum);
> +		sum = (sum & 0xffff) + (sum >> 16);
> +		ip4->ip_sum = rte_cpu_to_be_16(sum + (sum >> 16));
> +	}
>   }
>   
>   static inline void
> @@ -140,6 +153,10 @@ ipip_inbound(struct rte_mbuf *m, uint32_t offset)
>   			ip4_ecn_setup(inip4);
>   		/* XXX This should be done by the forwarding engine instead */
>   		inip4->ip_ttl -= 1;
> +		if (inip4->ip_sum >= rte_cpu_to_be_16(0xffff - 0x100))
> +			inip4->ip_sum += rte_cpu_to_be_16(0x100) + 1;
> +		else
> +			inip4->ip_sum += rte_cpu_to_be_16(0x100);
>   		m->packet_type &= ~RTE_PTYPE_L4_MASK;
>   		if (inip4->ip_p == IPPROTO_UDP)
>   			m->packet_type |= RTE_PTYPE_L4_UDP;
>
Radu Nicolau Jan. 15, 2018, 2:40 p.m. UTC | #2 
> -----Original Message-----

> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]

> Sent: Monday, January 15, 2018 12:48 PM

> To: dev@dpdk.org

> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;

> hemant.agrawal@nxp.com; Nicolau, Radu <radu.nicolau@intel.com>

> Subject: Re: [PATCH] examples/ipsec-secgw: update incremental checksum

> 

> On 1/15/2018 6:12 PM, Akhil Goyal wrote:

> > When TTL is decremented or ecn is updated in IP header before

> > forwarding the packet, checksum needs to be updated.

> >

> > In this patch an incremental checksum is added for ipv4 case.

> >

> > Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>

> > ---

> This patch is an update to a very old patch which was rejected earlier.

> http://dpdk.org/dev/patchwork/patch/16113/

> 

> >   examples/ipsec-secgw/ipip.h | 19 ++++++++++++++++++-

> >   1 file changed, 18 insertions(+), 1 deletion(-)

> >

> > diff --git a/examples/ipsec-secgw/ipip.h b/examples/ipsec-secgw/ipip.h

> > index fb6a6fa..13b8455 100644

> > --- a/examples/ipsec-secgw/ipip.h

> > +++ b/examples/ipsec-secgw/ipip.h

> > @@ -27,6 +27,10 @@ ipip_outbound(struct rte_mbuf *m, uint32_t offset,

> uint32_t is_ipv6,

> >   	if (inip4->ip_v == IPVERSION) {

> >   		/* XXX This should be done by the forwarding engine instead

> */

> >   		inip4->ip_ttl -= 1;

> > +		if (inip4->ip_sum >= rte_cpu_to_be_16(0xffff - 0x100))

> > +			inip4->ip_sum += rte_cpu_to_be_16(0x100) + 1;

> > +		else

> > +			inip4->ip_sum += rte_cpu_to_be_16(0x100);

> >   		ds_ecn = inip4->ip_tos;

> >   	} else {

> >   		inip6 = (struct ip6_hdr *)inip4;

> > @@ -95,8 +99,17 @@ ip6ip_outbound(struct rte_mbuf *m, uint32_t offset,

> >   static inline void

> >   ip4_ecn_setup(struct ip *ip4)

> >   {

> > -	if (ip4->ip_tos & IPTOS_ECN_MASK)

> > +	if (ip4->ip_tos & IPTOS_ECN_MASK) {

> > +		unsigned long sum;

> > +		uint8_t old;

> > +

> > +		old = ip4->ip_tos;

> >   		ip4->ip_tos |= IPTOS_ECN_CE;

> > +		sum = old + (~(*(uint8_t *)&ip4->ip_tos) & 0xff);

> > +		sum += rte_be_to_cpu_16(ip4->ip_sum);

> > +		sum = (sum & 0xffff) + (sum >> 16);

> > +		ip4->ip_sum = rte_cpu_to_be_16(sum + (sum >> 16));

> > +	}

> >   }

> >

> >   static inline void

> > @@ -140,6 +153,10 @@ ipip_inbound(struct rte_mbuf *m, uint32_t offset)

> >   			ip4_ecn_setup(inip4);

> >   		/* XXX This should be done by the forwarding engine instead

> */

> >   		inip4->ip_ttl -= 1;

> > +		if (inip4->ip_sum >= rte_cpu_to_be_16(0xffff - 0x100))

> > +			inip4->ip_sum += rte_cpu_to_be_16(0x100) + 1;

> > +		else

> > +			inip4->ip_sum += rte_cpu_to_be_16(0x100);

> >   		m->packet_type &= ~RTE_PTYPE_L4_MASK;

> >   		if (inip4->ip_p == IPPROTO_UDP)

> >   			m->packet_type |= RTE_PTYPE_L4_UDP;

> >


I think instead of manipulating the checksum in this way it will be better to use rte_ipv4_cksum to re-compute it, unless the performance penalty is too significant.
Akhil Goyal Jan. 16, 2018, 6:29 a.m. UTC | #3 
Hi Radu,
On 1/15/2018 8:10 PM, Nicolau, Radu wrote:
> 
> 
>> -----Original Message-----
>> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
>> Sent: Monday, January 15, 2018 12:48 PM
>> To: dev@dpdk.org
>> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
>> hemant.agrawal@nxp.com; Nicolau, Radu <radu.nicolau@intel.com>
>> Subject: Re: [PATCH] examples/ipsec-secgw: update incremental checksum
>>
>> On 1/15/2018 6:12 PM, Akhil Goyal wrote:
>>> When TTL is decremented or ecn is updated in IP header before
>>> forwarding the packet, checksum needs to be updated.
>>>
>>> In this patch an incremental checksum is added for ipv4 case.
>>>
>>> Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
>>> ---
>> This patch is an update to a very old patch which was rejected earlier.
>> http://dpdk.org/dev/patchwork/patch/16113/
>>
>>>    examples/ipsec-secgw/ipip.h | 19 ++++++++++++++++++-
>>>    1 file changed, 18 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/examples/ipsec-secgw/ipip.h b/examples/ipsec-secgw/ipip.h
>>> index fb6a6fa..13b8455 100644
>>> --- a/examples/ipsec-secgw/ipip.h
>>> +++ b/examples/ipsec-secgw/ipip.h
>>> @@ -27,6 +27,10 @@ ipip_outbound(struct rte_mbuf *m, uint32_t offset,
>> uint32_t is_ipv6,
>>>    	if (inip4->ip_v == IPVERSION) {
>>>    		/* XXX This should be done by the forwarding engine instead
>> */
>>>    		inip4->ip_ttl -= 1;
>>> +		if (inip4->ip_sum >= rte_cpu_to_be_16(0xffff - 0x100))
>>> +			inip4->ip_sum += rte_cpu_to_be_16(0x100) + 1;
>>> +		else
>>> +			inip4->ip_sum += rte_cpu_to_be_16(0x100);
>>>    		ds_ecn = inip4->ip_tos;
>>>    	} else {
>>>    		inip6 = (struct ip6_hdr *)inip4;
>>> @@ -95,8 +99,17 @@ ip6ip_outbound(struct rte_mbuf *m, uint32_t offset,
>>>    static inline void
>>>    ip4_ecn_setup(struct ip *ip4)
>>>    {
>>> -	if (ip4->ip_tos & IPTOS_ECN_MASK)
>>> +	if (ip4->ip_tos & IPTOS_ECN_MASK) {
>>> +		unsigned long sum;
>>> +		uint8_t old;
>>> +
>>> +		old = ip4->ip_tos;
>>>    		ip4->ip_tos |= IPTOS_ECN_CE;
>>> +		sum = old + (~(*(uint8_t *)&ip4->ip_tos) & 0xff);
>>> +		sum += rte_be_to_cpu_16(ip4->ip_sum);
>>> +		sum = (sum & 0xffff) + (sum >> 16);
>>> +		ip4->ip_sum = rte_cpu_to_be_16(sum + (sum >> 16));
>>> +	}
>>>    }
>>>
>>>    static inline void
>>> @@ -140,6 +153,10 @@ ipip_inbound(struct rte_mbuf *m, uint32_t offset)
>>>    			ip4_ecn_setup(inip4);
>>>    		/* XXX This should be done by the forwarding engine instead
>> */
>>>    		inip4->ip_ttl -= 1;
>>> +		if (inip4->ip_sum >= rte_cpu_to_be_16(0xffff - 0x100))
>>> +			inip4->ip_sum += rte_cpu_to_be_16(0x100) + 1;
>>> +		else
>>> +			inip4->ip_sum += rte_cpu_to_be_16(0x100);
>>>    		m->packet_type &= ~RTE_PTYPE_L4_MASK;
>>>    		if (inip4->ip_p == IPPROTO_UDP)
>>>    			m->packet_type |= RTE_PTYPE_L4_UDP;
>>>
> 
> I think instead of manipulating the checksum in this way it will be better to use rte_ipv4_cksum to re-compute it, unless the performance penalty is too significant.
> 
There would be unnecessary wastage of cycles. This way of updating the 
checksum is implemented as per the RFC1141
https://tools.ietf.org/html/rfc1141
Do you see any issue in this way of updating the checksum?

-Akhil
Radu Nicolau Jan. 16, 2018, 10:56 a.m. UTC | #4 
> -----Original Message-----

> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]

> Sent: Tuesday, January 16, 2018 6:30 AM

> To: Nicolau, Radu <radu.nicolau@intel.com>; dev@dpdk.org

> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;

> hemant.agrawal@nxp.com

> Subject: Re: [PATCH] examples/ipsec-secgw: update incremental checksum

> 

> Hi Radu,

> On 1/15/2018 8:10 PM, Nicolau, Radu wrote:

> >

> >

> >> -----Original Message-----

> >> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]

> >> Sent: Monday, January 15, 2018 12:48 PM

> >> To: dev@dpdk.org

> >> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;

> >> hemant.agrawal@nxp.com; Nicolau, Radu <radu.nicolau@intel.com>

> >> Subject: Re: [PATCH] examples/ipsec-secgw: update incremental

> >> checksum

> >>

> >> On 1/15/2018 6:12 PM, Akhil Goyal wrote:

> >>> When TTL is decremented or ecn is updated in IP header before

> >>> forwarding the packet, checksum needs to be updated.

> >>>

> >>> In this patch an incremental checksum is added for ipv4 case.

> >>>

> >>> Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>

> >>> ---

> >> This patch is an update to a very old patch which was rejected earlier.

> >> http://dpdk.org/dev/patchwork/patch/16113/

> >>

> >>>    examples/ipsec-secgw/ipip.h | 19 ++++++++++++++++++-

> >>>    1 file changed, 18 insertions(+), 1 deletion(-)

> >>>

> >>> diff --git a/examples/ipsec-secgw/ipip.h

> >>> b/examples/ipsec-secgw/ipip.h index fb6a6fa..13b8455 100644

> >>> --- a/examples/ipsec-secgw/ipip.h

> >>> +++ b/examples/ipsec-secgw/ipip.h

> >>> @@ -27,6 +27,10 @@ ipip_outbound(struct rte_mbuf *m, uint32_t

> >>> offset,

> >> uint32_t is_ipv6,

> >>>    	if (inip4->ip_v == IPVERSION) {

> >>>    		/* XXX This should be done by the forwarding engine instead

> >> */

> >>>    		inip4->ip_ttl -= 1;

> >>> +		if (inip4->ip_sum >= rte_cpu_to_be_16(0xffff - 0x100))

> >>> +			inip4->ip_sum += rte_cpu_to_be_16(0x100) + 1;

> >>> +		else

> >>> +			inip4->ip_sum += rte_cpu_to_be_16(0x100);

> >>>    		ds_ecn = inip4->ip_tos;

> >>>    	} else {

> >>>    		inip6 = (struct ip6_hdr *)inip4; @@ -95,8 +99,17 @@

> >>> ip6ip_outbound(struct rte_mbuf *m, uint32_t offset,

> >>>    static inline void

> >>>    ip4_ecn_setup(struct ip *ip4)

> >>>    {

> >>> -	if (ip4->ip_tos & IPTOS_ECN_MASK)

> >>> +	if (ip4->ip_tos & IPTOS_ECN_MASK) {

> >>> +		unsigned long sum;

> >>> +		uint8_t old;

> >>> +

> >>> +		old = ip4->ip_tos;

> >>>    		ip4->ip_tos |= IPTOS_ECN_CE;

> >>> +		sum = old + (~(*(uint8_t *)&ip4->ip_tos) & 0xff);

> >>> +		sum += rte_be_to_cpu_16(ip4->ip_sum);

> >>> +		sum = (sum & 0xffff) + (sum >> 16);

> >>> +		ip4->ip_sum = rte_cpu_to_be_16(sum + (sum >> 16));

> >>> +	}

> >>>    }

> >>>

> >>>    static inline void

> >>> @@ -140,6 +153,10 @@ ipip_inbound(struct rte_mbuf *m, uint32_t

> offset)

> >>>    			ip4_ecn_setup(inip4);

> >>>    		/* XXX This should be done by the forwarding engine instead

> >> */

> >>>    		inip4->ip_ttl -= 1;

> >>> +		if (inip4->ip_sum >= rte_cpu_to_be_16(0xffff - 0x100))

> >>> +			inip4->ip_sum += rte_cpu_to_be_16(0x100) + 1;

> >>> +		else

> >>> +			inip4->ip_sum += rte_cpu_to_be_16(0x100);

> >>>    		m->packet_type &= ~RTE_PTYPE_L4_MASK;

> >>>    		if (inip4->ip_p == IPPROTO_UDP)

> >>>    			m->packet_type |= RTE_PTYPE_L4_UDP;

> >>>

> >

> > I think instead of manipulating the checksum in this way it will be better to

> use rte_ipv4_cksum to re-compute it, unless the performance penalty is too

> significant.

> >

> There would be unnecessary wastage of cycles. This way of updating the

> checksum is implemented as per the RFC1141

> https://tools.ietf.org/html/rfc1141

> Do you see any issue in this way of updating the checksum?

No, I just tought that it will make the code look nicer.

Acked-by: Radu Nicolau <radu.nicolau@intel.com>
De Lara Guarch, Pablo Jan. 16, 2018, 5:17 p.m. UTC | #5 
> -----Original Message-----

> From: Nicolau, Radu

> Sent: Tuesday, January 16, 2018 10:56 AM

> To: Akhil Goyal <akhil.goyal@nxp.com>; dev@dpdk.org

> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;

> hemant.agrawal@nxp.com

> Subject: RE: [PATCH] examples/ipsec-secgw: update incremental checksum


...

> 

> Acked-by: Radu Nicolau <radu.nicolau@intel.com>


Applied to dpdk-next-crypto.
Thanks,

Pablo
diff mbox

Patch

diff --git a/examples/ipsec-secgw/ipip.h b/examples/ipsec-secgw/ipip.h
index fb6a6fa..13b8455 100644
--- a/examples/ipsec-secgw/ipip.h
+++ b/examples/ipsec-secgw/ipip.h
@@ -27,6 +27,10 @@  ipip_outbound(struct rte_mbuf *m, uint32_t offset, uint32_t is_ipv6,
 	if (inip4->ip_v == IPVERSION) {
 		/* XXX This should be done by the forwarding engine instead */
 		inip4->ip_ttl -= 1;
+		if (inip4->ip_sum >= rte_cpu_to_be_16(0xffff - 0x100))
+			inip4->ip_sum += rte_cpu_to_be_16(0x100) + 1;
+		else
+			inip4->ip_sum += rte_cpu_to_be_16(0x100);
 		ds_ecn = inip4->ip_tos;
 	} else {
 		inip6 = (struct ip6_hdr *)inip4;
@@ -95,8 +99,17 @@  ip6ip_outbound(struct rte_mbuf *m, uint32_t offset,
 static inline void
 ip4_ecn_setup(struct ip *ip4)
 {
-	if (ip4->ip_tos & IPTOS_ECN_MASK)
+	if (ip4->ip_tos & IPTOS_ECN_MASK) {
+		unsigned long sum;
+		uint8_t old;
+
+		old = ip4->ip_tos;
 		ip4->ip_tos |= IPTOS_ECN_CE;
+		sum = old + (~(*(uint8_t *)&ip4->ip_tos) & 0xff);
+		sum += rte_be_to_cpu_16(ip4->ip_sum);
+		sum = (sum & 0xffff) + (sum >> 16);
+		ip4->ip_sum = rte_cpu_to_be_16(sum + (sum >> 16));
+	}
 }
 
 static inline void
@@ -140,6 +153,10 @@  ipip_inbound(struct rte_mbuf *m, uint32_t offset)
 			ip4_ecn_setup(inip4);
 		/* XXX This should be done by the forwarding engine instead */
 		inip4->ip_ttl -= 1;
+		if (inip4->ip_sum >= rte_cpu_to_be_16(0xffff - 0x100))
+			inip4->ip_sum += rte_cpu_to_be_16(0x100) + 1;
+		else
+			inip4->ip_sum += rte_cpu_to_be_16(0x100);
 		m->packet_type &= ~RTE_PTYPE_L4_MASK;
 		if (inip4->ip_p == IPPROTO_UDP)
 			m->packet_type |= RTE_PTYPE_L4_UDP;