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

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

On 2/18/22 7:28 PM, Nithin Kumar Dabilpuram wrote:
> 
> 
> 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.

I removed this patch from v3 of this series. I'll included it in other 
series that I'm working on to have separate worker thread when all
SA's are of particular mode. There we can avoid checksum recalculation
completely both in SW and HW.

Thanks
Nithin

> 
>>>
>>> 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
>>>>