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

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

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?

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.

And with same logic, should we add 'all_tunnel_tso' check to the UDP case?


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