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

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

在 2023/11/6 18:09, Ferruh Yigit 写道:
> 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.
get it.
I will check "cmd_tunnel_tso_set_parsed" after this patch and see if we 
need to do something for tunnel tso in testpmd.
Will send v3 according to your advice ASAP.
>
>
>
>>> 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;
>>>>> .
>>> .
> .