Re: [dpdk-dev] [PATCH v3 2/2] examples/ipsec-secgw: add support for TSO

["Nicolau, Radu" <radu.nicolau@intel.com>]

I'm seeing this problem: if we support both TCP and UDP TSO then we will 
check the TX offloads for both (and IPsec) and so far the only PMDs that 
support IPsec and TSO only support TCP TSO. So I will rework the patch 
to only enable TSO for TCP Inline crypto.

On 10/27/2021 1:44 PM, Nicolau, Radu wrote:
>
> On 10/27/2021 1:02 PM, Ananyev, Konstantin wrote:
>>
>>> Add support to allow user to specific MSS for TSO offload on a per SA
>>> basis. MSS configuration in the context of IPsec is only supported for
>>> outbound SA's in the context of an inline IPsec Crypto offload.
>>>
>>> Signed-off-by: Declan Doherty <declan.doherty@intel.com>
>>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
>>> ---
>>>   doc/guides/rel_notes/release_21_11.rst   |  4 ++++
>>>   doc/guides/sample_app_ug/ipsec_secgw.rst | 11 +++++++++++
>>>   examples/ipsec-secgw/ipsec-secgw.c       |  4 ++++
>>>   examples/ipsec-secgw/ipsec.h             |  1 +
>>>   examples/ipsec-secgw/ipsec_process.c     | 25 
>>> ++++++++++++++++++++++++
>>>   examples/ipsec-secgw/sa.c                | 21 ++++++++++++++++++--
>>>   6 files changed, 64 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/doc/guides/rel_notes/release_21_11.rst 
>>> b/doc/guides/rel_notes/release_21_11.rst
>>> index b5b5abadee..8d1767b084 100644
>>> --- a/doc/guides/rel_notes/release_21_11.rst
>>> +++ b/doc/guides/rel_notes/release_21_11.rst
>>> @@ -306,6 +306,10 @@ New Features
>>>       * Pcapng format with timestamps and meta-data.
>>>       * Fixes packet capture with stripped VLAN tags.
>>>
>>> +* **IPsec Security Gateway sample application new features.**
>>> +
>>> +  * Added support for TSO
>>> +
>>>
>>>   Removed Items
>>>   -------------
>>> diff --git a/doc/guides/sample_app_ug/ipsec_secgw.rst 
>>> b/doc/guides/sample_app_ug/ipsec_secgw.rst
>>> index 782574dd39..639d309a6e 100644
>>> --- a/doc/guides/sample_app_ug/ipsec_secgw.rst
>>> +++ b/doc/guides/sample_app_ug/ipsec_secgw.rst
>>> @@ -720,6 +720,17 @@ where each options means:
>>>
>>>      * *udp-encap*
>>>
>>> + ``<mss>``
>>> +
>>> + * Maximum segment size for TSO offload, available for egress SAs 
>>> only.
>>> +
>>> + * Optional: Yes, TSO offload not set by default
>>> +
>>> + * Syntax:
>>> +
>>> +   * *mss N* N is the segment size in bytes
>>> +
>>> +
>>>   Example SA rules:
>>>
>>>   .. code-block:: console
>>> diff --git a/examples/ipsec-secgw/ipsec-secgw.c 
>>> b/examples/ipsec-secgw/ipsec-secgw.c
>>> index 4bdf99b62b..5fcf424efe 100644
>>> --- a/examples/ipsec-secgw/ipsec-secgw.c
>>> +++ b/examples/ipsec-secgw/ipsec-secgw.c
>>> @@ -398,6 +398,10 @@ prepare_one_packet(struct rte_mbuf *pkt, struct 
>>> ipsec_traffic *t)
>>>           pkt->l2_len = 0;
>>>           pkt->l3_len = sizeof(*iph4);
>>>           pkt->packet_type |= RTE_PTYPE_L3_IPV4;
>>> +        if  (pkt->packet_type & RTE_PTYPE_L4_TCP)
>>> +            pkt->l4_len = sizeof(struct rte_tcp_hdr);
>>> +        else if (pkt->packet_type & RTE_PTYPE_L4_UDP)
>>> +            pkt->l4_len = sizeof(struct rte_udp_hdr);
>>>       } else if (eth->ether_type == 
>>> rte_cpu_to_be_16(RTE_ETHER_TYPE_IPV6)) {
>>>           int next_proto;
>>>           size_t l3len, ext_len;
>>> diff --git a/examples/ipsec-secgw/ipsec.h 
>>> b/examples/ipsec-secgw/ipsec.h
>>> index 8405c48171..2c3640833d 100644
>>> --- a/examples/ipsec-secgw/ipsec.h
>>> +++ b/examples/ipsec-secgw/ipsec.h
>>> @@ -137,6 +137,7 @@ struct ipsec_sa {
>>>       enum rte_security_ipsec_sa_direction direction;
>>>       uint8_t udp_encap;
>>>       uint16_t portid;
>>> +    uint16_t mss;
>>>       uint8_t fdir_qid;
>>>       uint8_t fdir_flag;
>>>
>>> diff --git a/examples/ipsec-secgw/ipsec_process.c 
>>> b/examples/ipsec-secgw/ipsec_process.c
>>> index 5012e1a6a4..26c6c2fe84 100644
>>> --- a/examples/ipsec-secgw/ipsec_process.c
>>> +++ b/examples/ipsec-secgw/ipsec_process.c
>>> @@ -222,6 +222,31 @@ prep_process_group(void *sa, struct rte_mbuf 
>>> *mb[], uint32_t cnt)
>>>       for (j = 0; j != cnt; j++) {
>>>           priv = get_priv(mb[j]);
>>>           priv->sa = sa;
>>> +        /* setup TSO related fields if TSO enabled*/
>>> +        if (priv->sa->mss) {
>>> +            mb[j]->tso_segsz = priv->sa->mss;
>>> +
>>> +            if ((IS_TUNNEL(priv->sa->flags))) {
>>> +                mb[j]->outer_l3_len = mb[j]->l3_len;
>>> +                mb[j]->outer_l2_len = mb[j]->l2_len;
>>> +                mb[j]->ol_flags |=
>>> +                (RTE_MBUF_F_TX_OUTER_IP_CKSUM |
>>> +                        RTE_MBUF_F_TX_TUNNEL_ESP);
>>> +            }
>>> +            uint32_t ptype = mb[j]->packet_type;
>>> +            if  (ptype & RTE_PTYPE_L4_TCP)
>>> +                mb[j]->ol_flags |=
>>> +                        (RTE_MBUF_F_TX_TCP_SEG |
>>> +                        RTE_MBUF_F_TX_TCP_CKSUM);
>>> +            else
>>> +                mb[j]->ol_flags |=
>>> +                    (RTE_MBUF_F_TX_UDP_SEG |
>>> +                        RTE_MBUF_F_TX_UDP_CKSUM);
>> Could it be that packet is neither TCP nor UDP?
> Yes, I will add a third branch for unsupported packet type.
>>
>>> +            if (RTE_ETH_IS_IPV4_HDR(ptype))
>>> +                mb[j]->ol_flags |= RTE_MBUF_F_TX_OUTER_IPV4;
>>> +            else
>>> +                mb[j]->ol_flags |= RTE_MBUF_F_TX_OUTER_IPV6;
>>> +        }
>>>       }
>>>   }
>>>
>>> diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c
>>> index 88dd30464f..e8815dffe7 100644
>>> --- a/examples/ipsec-secgw/sa.c
>>> +++ b/examples/ipsec-secgw/sa.c
>>> @@ -677,6 +677,16 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
>>>               continue;
>>>           }
>>>
>>> +        if (strcmp(tokens[ti], "mss") == 0) {
>>> +            INCREMENT_TOKEN_INDEX(ti, n_tokens, status);
>>> +            if (status->status < 0)
>>> +                return;
>>> +            rule->mss = atoi(tokens[ti]);
>>> +            if (status->status < 0)
>>> +                return;
>>> +            continue;
>>> +        }
>>> +
>>>           if (strcmp(tokens[ti], "fallback") == 0) {
>>>               struct rte_ipsec_session *fb;
>>>
>>> @@ -970,7 +980,7 @@ sa_create(const char *name, int32_t socket_id, 
>>> uint32_t nb_sa)
>>>   }
>>>
>>>   static int
>>> -check_eth_dev_caps(uint16_t portid, uint32_t inbound)
>>> +check_eth_dev_caps(uint16_t portid, uint32_t inbound, uint32_t tso)
>>>   {
>>>       struct rte_eth_dev_info dev_info;
>>>       int retval;
>>> @@ -999,6 +1009,13 @@ check_eth_dev_caps(uint16_t portid, uint32_t 
>>> inbound)
>>>                   "hardware TX IPSec offload is not supported\n");
>>>               return -EINVAL;
>>>           }
>>> +        if (tso && (dev_info.tx_offload_capa &
>>> +                (RTE_ETH_TX_OFFLOAD_TCP_TSO |
>>> +                    RTE_ETH_TX_OFFLOAD_UDP_TSO)) == 0) {
>> Shouldn't it be:
>> dev_info.tx_offload_capa & (RTE_ETH_TX_OFFLOAD_TCP_TSO | 
>> RTE_ETH_TX_OFFLOAD_UDP_TSO)) ==
>> RTE_ETH_TX_OFFLOAD_TCP_TSO | RTE_ETH_TX_OFFLOAD_UDP_TSO)
>> ?
> It should be that but negated, if none of the flags are set we exit - 
> we can't tell at this point if we need TCP or UDP.
>>
>>> +            RTE_LOG(WARNING, PORT,
>>> +                "hardware TSO offload is not supported\n");
>>> +            return -EINVAL;
>>> +        }
>>>       }
>>>       return 0;
>>>   }
>> I think you missed changes in a_check_offloads().
>> That's where we specify which HW offloads should be enabled for given 
>> port.
> Yes.
>>
>>
>>> @@ -1127,7 +1144,7 @@ sa_add_rules(struct sa_ctx *sa_ctx, const 
>>> struct ipsec_sa entries[],
>>>
>>>           if (ips->type == RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL ||
>>>               ips->type == RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO) {
>>> -            if (check_eth_dev_caps(sa->portid, inbound))
>>> +            if (check_eth_dev_caps(sa->portid, inbound, sa->mss))
>>>                   return -EINVAL;
>>>           }
>>>
>>> -- 
>>> 2.25.1