Re: [PATCH v2] app/testpmd: fix UDP cksum error for UFO enable

["lihuisong (C)" <lihuisong@huawei.com>]

在 2023/11/3 18:42, Ferruh Yigit 写道:
> On 11/3/2023 9:09 AM, lihuisong (C) wrote:
>> Hi Ferruh,
>>
>> Thanks for you review.
>>
>>
>> 在 2023/11/3 9:31, Ferruh Yigit 写道:
>>> On 8/2/2023 3:55 AM, Huisong Li wrote:
>>>> The command "tso set <tso_segsz> <port_id>" is used to enable UFO,
>>>> please
>>>> see commit ce8e6e742807 ("app/testpmd: support UFO in checksum engine")
>>>>
>>>> The above patch configures the RTE_MBUF_F_TX_UDP_SEG to enable UFO
>>>> only if
>>>> tso_segsz is set.
>>>>
>>> "The above patch sets the RTE_MBUF_F_TX_UDP_SEG in mbuf ol_flags, only
>>> by checking if 'tso_segsz' is set, but missing check if UFO offload
>>> (RTE_ETH_TX_OFFLOAD_UDP_TSO) supported by device."
>> Ack
>>>
>>>> Then tx_prepare() may call rte_net_intel_cksum_prepare()
>>>> to compute pseudo header checksum (because some PMDs may supports TSO).
>>>>
>>> Not sure what do you mean by '(because some PMDs may supports TSO)'?
>>>
>>> Do you mean something like following:
>>> "RTE_MBUF_F_TX_UDP_SEG flag causes driver that supports TSO/UFO to
>>> compute pseudo header checksum."
>> Ack
>>>
>>>> As a result, if the peer sends UDP packets, all packets with UDP
>>>> checksum
>>>> error are received for the PMDs only supported TSO.
>>>>
>>> "As a result, if device only supports TSO, but not UFO, UDP packet
>>> checksum will be wrong."
>> Ack
>>>
>>>> So enabling UFO also depends on if driver has RTE_ETH_TX_OFFLOAD_UDP_TSO
>>>> capability. Similarly, TSO also need to do like this.
>>>>
>>>> In addition, this patch also fixes cmd_tso_set_parsed() for UFO to make
>>>> it better to support TSO and UFO.
>>>>
>>>> Fixes: ce8e6e742807 ("app/testpmd: support UFO in checksum engine")
>>>>
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> ---
>>>>    v2: add handle for tunnel TSO offload in process_inner_cksums
>>>>
>>>> ---
>>>>    app/test-pmd/cmdline.c  | 47 +++++++++++++++++++++--------------------
>>>>    app/test-pmd/csumonly.c | 11 ++++++++--
>>>>    2 files changed, 33 insertions(+), 25 deletions(-)
>>>>
>>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>>>> index 0d0723f659..8be593d405 100644
>>>> --- a/app/test-pmd/cmdline.c
>>>> +++ b/app/test-pmd/cmdline.c
>>>> @@ -4906,6 +4906,7 @@ cmd_tso_set_parsed(void *parsed_result,
>>>>    {
>>>>        struct cmd_tso_set_result *res = parsed_result;
>>>>        struct rte_eth_dev_info dev_info;
>>>> +    uint64_t offloads;
>>>>        int ret;
>>>>          if (port_id_is_invalid(res->port_id, ENABLED_WARN))
>>>> @@ -4922,37 +4923,37 @@ cmd_tso_set_parsed(void *parsed_result,
>>>>        if (ret != 0)
>>>>            return;
>>>>    -    if ((ports[res->port_id].tso_segsz != 0) &&
>>>> -        (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0) {
>>>> -        fprintf(stderr, "Error: TSO is not supported by port %d\n",
>>>> -            res->port_id);
>>>> -        return;
>>>> +    if (ports[res->port_id].tso_segsz != 0) {
>>>> +        if ((dev_info.tx_offload_capa & (RTE_ETH_TX_OFFLOAD_TCP_TSO |
>>>> +                    RTE_ETH_TX_OFFLOAD_UDP_TSO)) == 0) {
>>>> +            fprintf(stderr, "Error: both TSO and UFO are not
>>>> supported by port %d\n",
>>>> +                res->port_id);
>>>> +            return;
>>>> +        }
>>>> +        /* display warnings if configuration is not supported by the
>>>> NIC */
>>>> +        if ((dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO)
>>>> == 0)
>>>> +            fprintf(stderr, "Warning: port %d doesn't support TSO\n",
>>>> +                res->port_id);
>>>> +        if ((dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TSO)
>>>> == 0)
>>>> +            fprintf(stderr, "Warning: port %d doesn't support UFO\n",
>>>> +                res->port_id);
>>>>
>>> Requesting TSO/UFO by setting 'tso_segsz', but device capability missing
>>> is an error case, so OK to have first message.
>>>
>>> But only supporting TSO or UFO is not an error case, not sure about
>>> logging this. But even it is logged, I think it shouldn't be to stderr
>>> or it should say "Warning: ", a regular logging can be done.
>> All right, will fix it in next version.
>>>
>>>>        }
>>>>          if (ports[res->port_id].tso_segsz == 0) {
>>>>            ports[res->port_id].dev_conf.txmode.offloads &=
>>>> -                        ~RTE_ETH_TX_OFFLOAD_TCP_TSO;
>>>> -        printf("TSO for non-tunneled packets is disabled\n");
>>>> +            ~(RTE_ETH_TX_OFFLOAD_TCP_TSO | RTE_ETH_TX_OFFLOAD_UDP_TSO);
>>>> +        printf("TSO and UFO for non-tunneled packets is disabled\n");
>>>>        } else {
>>>> -        ports[res->port_id].dev_conf.txmode.offloads |=
>>>> -                        RTE_ETH_TX_OFFLOAD_TCP_TSO;
>>>> -        printf("TSO segment size for non-tunneled packets is %d\n",
>>>> +        offloads = (dev_info.tx_offload_capa &
>>>> RTE_ETH_TX_OFFLOAD_TCP_TSO) ?
>>>> +                    RTE_ETH_TX_OFFLOAD_TCP_TSO : 0;
>>>> +        offloads |= (dev_info.tx_offload_capa &
>>>> RTE_ETH_TX_OFFLOAD_UDP_TSO) ?
>>>> +                    RTE_ETH_TX_OFFLOAD_UDP_TSO : 0;
>>>> +        ports[res->port_id].dev_conf.txmode.offloads |= offloads;
>>>> +        printf("segment size for non-tunneled packets is %d\n",
>>>>                ports[res->port_id].tso_segsz);
>>>>        }
>>>> -    cmd_config_queue_tx_offloads(&ports[res->port_id]);
>>>> -
>>>> -    /* display warnings if configuration is not supported by the NIC */
>>>> -    ret = eth_dev_info_get_print_err(res->port_id, &dev_info);
>>>> -    if (ret != 0)
>>>> -        return;
>>>> -
>>>> -    if ((ports[res->port_id].tso_segsz != 0) &&
>>>> -        (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0) {
>>>> -        fprintf(stderr,
>>>> -            "Warning: TSO enabled but not supported by port %d\n",
>>>> -            res->port_id);
>>>> -    }
>>>>    
>>> Above is redundant check, and introduced with commit [1], I assume by
>>> mistake.
>> Yes, it is a redundant check indeed.
>> This check is introduced in the first patch[1]. But the patch [2] add
>> offload capabilities check but don't delete the old check.
>>
>>
>> [1] Fixes: b51c47536a9e ("app/testpmd: support TSO in checksum forward
>> engine")
>> [2] Fixes: 3926dd2b6668 ("app/testpmd: enforce offload capabilities check")
>>> Since removing above check is not related to UFO, what do you
>>> think to separate it to its own patch?
>> ok, will separate it from this patch.
>>> [1]
>>> Fixes: 3926dd2b6668 ("app/testpmd: enforce offload capabilities check")
>>>
>>>> +    cmd_config_queue_tx_offloads(&ports[res->port_id]);
>>>>        cmd_reconfig_device_queue(res->port_id, 1, 1);
>>>>    }
>>>>    diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
>>>> index c103e54111..21210aff43 100644
>>>> --- a/app/test-pmd/csumonly.c
>>>> +++ b/app/test-pmd/csumonly.c
>>>> @@ -466,6 +466,12 @@ process_inner_cksums(void *l3_hdr, const struct
>>>> testpmd_offload_info *info,
>>>>        uint64_t ol_flags = 0;
>>>>        uint32_t max_pkt_len, tso_segsz = 0;
>>>>        uint16_t l4_off;
>>>> +    uint64_t all_tunnel_tso = RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
>>>> +                RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO |
>>>> +                RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO |
>>>> +                RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO |
>>>> +                RTE_ETH_TX_OFFLOAD_IP_TNL_TSO |
>>>> +                RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO;
>>>>          /* ensure packet is large enough to require tso */
>>>>        if (!info->is_tunnel) {
>>>> @@ -505,7 +511,7 @@ process_inner_cksums(void *l3_hdr, const struct
>>>> testpmd_offload_info *info,
>>>>            udp_hdr = (struct rte_udp_hdr *)((char *)l3_hdr +
>>>> info->l3_len);
>>>>            /* do not recalculate udp cksum if it was 0 */
>>>>            if (udp_hdr->dgram_cksum != 0) {
>>>> -            if (tso_segsz)
>>>> +            if (tso_segsz && (tx_offloads &
>>>> RTE_ETH_TX_OFFLOAD_UDP_TSO))
>>>>                    ol_flags |= RTE_MBUF_F_TX_UDP_SEG;
>>>>                else if (tx_offloads & RTE_ETH_TX_OFFLOAD_UDP_CKSUM) {
>>>>                    ol_flags |= RTE_MBUF_F_TX_UDP_CKSUM;
>>>> @@ -528,7 +534,8 @@ process_inner_cksums(void *l3_hdr, const struct
>>>> testpmd_offload_info *info,
>>>>    #endif
>>>>        } else if (info->l4_proto == IPPROTO_TCP) {
>>>>            tcp_hdr = (struct rte_tcp_hdr *)((char *)l3_hdr +
>>>> info->l3_len);
>>>> -        if (tso_segsz)
>>>> +        if (tso_segsz &&
>>>> +            (tx_offloads & (RTE_ETH_TX_OFFLOAD_TCP_TSO |
>>>> all_tunnel_tso)))
>>>>
>>> Should we check 'all_tunnel_tso', and why they are checked only for TCP?
>> Yes, this patch is just for TCP_TSO and UDP_TSO.
>> But here is necessary for tunnel_tso, or this doesn't set
>> 'RTE_MBUF_F_TX_TCP_SEG' flag for tunnel tso.
>>
> Lets say 'RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO' is requested, but
> 'RTE_ETH_TX_OFFLOAD_TCP_TSO' is not requested, should we still set the
> 'RTE_MBUF_F_TX_TCP_SEG' flag?
Yes, RTE_MBUF_F_TX_TCP_SEG flag still should be set for tunnel tso.
Driver compute pseudo header checksum and fill hw descriptor based on 
this flag.
>
> I am not really clear how to handle tunnel TSO offloads, but considering
> previous implementation was only relying on 'tso_segsz', continue with
> all TSO offload will be similar to previous implementation, so OK to
> have it.
Yes
>
> And with same logic, should we add 'all_tunnel_tso' check to the UDP case?
I didn't see some offloads about UDP_TSO for tunnel packet.
And testpmd just support a command (please see 
cmd_tunnel_tso_set_parsed) to set these tunnel tso offloads this patch 
mentioned.
>
>
> And agree that setting other tunnel related mbuf flags is out of scope
> for this patch, but probably that part is missing in this code, and only
What specific thing is missing in this code?
> a few drivers support these flags anyway.
>
>>> As far as I can see some tunnel TSO offloads should case setting
>>> relevant mbuf flags, like RTE_MBUF_F_TX_TUNNEL_UDP or
>>> RTE_ETH_TX_OFFLOAD_IP_TNL_TSO.
>>>
>>> With above check, if RTE_ETH_TX_OFFLOAD_TCP_TSO  not set but only
>>> RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO set, we still set 'RTE_MBUF_F_TX_TCP_SEG'
>>> flag but not 'RTE_MBUF_F_TX_TUNNEL_UDP' flag.
>> At least here didn't change the original behavior for tunnel tso.
>> I'm not still clear how to set these flag for tunnel tso.
>> But I can ensure that 'RTE_MBUF_F_TX_TCP_SEG' flag is must for tunnel tso.
>>> I assume intention is to be close to previous implementation, where only
>>> tso_segsz checked, and cover as much as possible TSO offload requests,
>>> but not sure if this is accurate with expected usage.
>> we may need to do something for tunnel tso command as this patch did.
>> I will take a look at it after this patch.
>>>>                ol_flags |= RTE_MBUF_F_TX_TCP_SEG;
>>>>            else if (tx_offloads & RTE_ETH_TX_OFFLOAD_TCP_CKSUM) {
>>>>                ol_flags |= RTE_MBUF_F_TX_TCP_CKSUM;
>>> .
> .