Re: [PATCH 2/4] examples/ipsec-secgw: disable Tx chksum offload for inline

[Nithin Kumar Dabilpuram <ndabilpuram@marvell.com>]

On 2/18/22 12:47 AM, Ananyev, Konstantin wrote:
> 
>>>> Enable Tx IPv4 checksum offload only when Tx inline crypto is needed.
>>>> In other cases such as Tx Inline protocol offload, checksum computation
>>>> is implicitly taken care by HW.
>>>
>>> Why is that?
>>> These is two separate HW offload and user has to enable each of them explicitly.
>>
>>
>> In Inline IPSec protocol offload, the complete tunnel header for tunnel
>> mode is updated by HW/PMD. So it doesn't have any dependency on
>> RTE_ETH_TX_OFFLOAD_IPV4_CKSUM as there is no valid l2_len/l3_len yet in
>> the mbuf. Similarly in case of Transport mode, the IPv4 header is
>> updated by HW itself for next proto and hence the offsets and all can
>> vary based on the HW implementation.
>>
>> Hence my thought was for Inline IPsec protocol offload, there is no need
>> to explicitly say that RTE_ETH_TX_OFFLOAD_IPV4_CKSUM is enabled and need
>> not provide ol_flags RTE_MBUF_F_TX_IP_CKSUM and l2_len and l3_len which
>> might not be correct in prepare_tx_pkt().
>>
>>   >* RTE_MBUF_F_TX_IP_CKSUM.
>>   > *  - fill the mbuf offload information: l2_len, l3_len
>> (Ex: Tunnel header being inserted is IPv6 while inner header is IPv4.
>>
>> For inline crypto I agree, the packet content is all in place except for
>> plain text->cipher text translation so l2/l3 offsets are valid.
> 
> Ok, but apart from inline modes we also have lookaside modes.
> Why to disable ip cksum for them?

Ack, I missed that case. I'll make the change to enable Tx checksum 
offload enabled even for Lookaside crypto.

> 
>>
>>   > Also we can TX clear-text traffic.
>> Ok, I agree that we can have clear-text traffic. We are already handling
>> ipv4 checksum in SW in case Tx offload doesn't have IPv4 Checksum
>> offload enabled. And for clear text traffic I think that is not needed
>> as well as we are not updating ttl.
> 
> As I remember we always recalculate ip cksum if RTE_MBUF_F_TX_IP_CKSUM
> is not set:
> 
> static inline void
> prepare_tx_pkt(struct rte_mbuf *pkt, uint16_t port,
>                  const struct lcore_conf *qconf)
> {
>          struct ip *ip;
>          struct rte_ether_hdr *ethhdr;
> 
>          ip = rte_pktmbuf_mtod(pkt, struct ip *);
> 
>          ethhdr = (struct rte_ether_hdr *)
>                  rte_pktmbuf_prepend(pkt, RTE_ETHER_HDR_LEN);
> 
>          if (ip->ip_v == IPVERSION) {
>                  pkt->ol_flags |= qconf->outbound.ipv4_offloads;
>                  pkt->l3_len = sizeof(struct ip);
>                  pkt->l2_len = RTE_ETHER_HDR_LEN;
> 
>                  ip->ip_sum = 0;
> 
>                  /* calculate IPv4 cksum in SW */
>                  if ((pkt->ol_flags & RTE_MBUF_F_TX_IP_CKSUM) == 0)
>                          ip->ip_sum = rte_ipv4_cksum((struct rte_ipv4_hdr *)ip);
> 
> ...
> 

I'm working on another series to restructure fast path based on 
different mode. As part of that, I think we can avoid checksum 
recalculation in cases of inline protocol offload.

>>
>> My overall intention was to have lighter Tx burst function for Inline
>> IPsec protocol offload as mainly IPsec traffic and not plain traffic is
>> primary use case for ipsec-secgw.
>>
>>
>>
>>>
>>>> The advantage of having only necessary
>>>> offloads enabled is that Tx burst function can be as light as possible.
>>>>
>>>> Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
>>>> ---
>>>>    examples/ipsec-secgw/ipsec-secgw.c | 3 ---
>>>>    examples/ipsec-secgw/sa.c          | 9 +++++++++
>>>>    2 files changed, 9 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-secgw/ipsec-secgw.c
>>>> index 21abc0d..d8a9bfa 100644
>>>> --- a/examples/ipsec-secgw/ipsec-secgw.c
>>>> +++ b/examples/ipsec-secgw/ipsec-secgw.c
>>>> @@ -2314,9 +2314,6 @@ port_init(uint16_t portid, uint64_t req_rx_offloads, uint64_t req_tx_offloads)
>>>>    		local_port_conf.txmode.offloads |=
>>>>    			RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE;
>>>>
>>>> -	if (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_IPV4_CKSUM)
>>>> -		local_port_conf.txmode.offloads |= RTE_ETH_TX_OFFLOAD_IPV4_CKSUM;
>>>> -
>>>>    	printf("port %u configuring rx_offloads=0x%" PRIx64
>>>>    		", tx_offloads=0x%" PRIx64 "\n",
>>>>    		portid, local_port_conf.rxmode.offloads,
>>>> diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c
>>>> index 1839ac7..b878a48 100644
>>>> --- a/examples/ipsec-secgw/sa.c
>>>> +++ b/examples/ipsec-secgw/sa.c
>>>> @@ -1790,6 +1790,15 @@ sa_check_offloads(uint16_t port_id, uint64_t *rx_offloads,
>>>>    				RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL)
>>>>    				&& rule->portid == port_id) {
>>>>    			*tx_offloads |= RTE_ETH_TX_OFFLOAD_SECURITY;
>>>> +
>>>> +			/* Checksum offload is not needed for inline protocol as
>>>> +			 * all processing for Outbound IPSec packets will be
>>>> +			 * implicitly taken care and for non-IPSec packets,
>>>> +			 * there is no need of IPv4 Checksum offload.
>>>> +			 */
>>>> +			if (rule_type == RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO)
>>>> +				*tx_offloads |= RTE_ETH_TX_OFFLOAD_IPV4_CKSUM;
>>>> +
>>>>    			if (rule->mss)
>>>>    				*tx_offloads |= RTE_ETH_TX_OFFLOAD_TCP_TSO;
>>>>    		}
>>>> --
>>>> 2.8.4
>>>