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

[Akhil Goyal <akhil.goyal@nxp.com>]

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