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

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 11/6/2023 4:13 AM, lihuisong (C) wrote:
> 
> 在 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?
>

I don't mean this patch, but existing code. It doesn't set
'RTE_MBUF_F_TX_TUNNEL_UDP' or 'RTE_MBUF_F_TX_TUNNEL_IP' mbuf flags when
relevant TSO offload requested.



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