[ovs-dev] [PATCH v2 1/3] netdev-dpdk: Fix Rx checksum reconfigure.

Kevin Traynor ktraynor at redhat.com
Mon May 29 12:36:43 UTC 2017


On 05/26/2017 03:04 PM, Chandran, Sugesh wrote:
> 
> 
> Regards
> _Sugesh
> 
> 
>> -----Original Message-----
>> From: Chandran, Sugesh
>> Sent: Wednesday, May 17, 2017 10:50 AM
>> To: Kevin Traynor <ktraynor at redhat.com>
>> Cc: dev at openvswitch.org
>> Subject: RE: [PATCH v2 1/3] netdev-dpdk: Fix Rx checksum reconfigure.
>>
>>
>>
>> Regards
>> _Sugesh
>>
>>> -----Original Message-----
>>> From: Kevin Traynor [mailto:ktraynor at redhat.com]
>>> Sent: Wednesday, May 17, 2017 10:10 AM
>>> To: Chandran, Sugesh <sugesh.chandran at intel.com>
>>> Cc: dev at openvswitch.org
>>> Subject: Re: [PATCH v2 1/3] netdev-dpdk: Fix Rx checksum reconfigure.
>>>
>>> On 05/16/2017 05:48 PM, Chandran, Sugesh wrote:
>>>> Hi Kevin,
>>>> Thank you for sending out this patch series.
>>>> Have you tested the tunneling decap usecase with checksum offload? I
>>>> am seeing weird behavior when I testing the tunneling with Rx
>>>> checksum offload ON and OFF.(Seeing the same behavior on master as
>>>> well)
>>>>
>>>> Here is the summary of issue with the steps,
>>>>
>>>> 1) Send tunnel traffic to OVS to do the decap.
>>>> 2) Set & unset the checksum offload.
>>>> 3) I don't see any performance difference in both case.
>>>>
>>>> Now I went ahead and put some debug message to see what is
>> happening
>>>>
>>>> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c index
>>>> 2798324..49ca847 100644
>>>> --- a/lib/netdev-native-tnl.c
>>>> +++ b/lib/netdev-native-tnl.c
>>>> @@ -86,6 +86,7 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet
>>> *packet, struct flow_tnl *tnl,
>>>>          ovs_be32 ip_src, ip_dst;
>>>>
>>>>          if (OVS_UNLIKELY(!dp_packet_ip_checksum_valid(packet))) {
>>>> +            VLOG_INFO("Checksum is not validated...");
>>>>              if (csum(ip, IP_IHL(ip->ip_ihl_ver) * 4)) {
>>>>                  VLOG_WARN_RL(&err_rl, "ip packet has invalid checksum");
>>>>                  return NULL;
>>>> @@ -182,6 +183,7 @@ udp_extract_tnl_md(struct dp_packet *packet,
>>>> struct flow_tnl *tnl,
>>>>
>>>>      if (udp->udp_csum) {
>>>>          if (OVS_UNLIKELY(!dp_packet_l4_checksum_valid(packet))) {
>>>> +            VLOG_INFO("Checksum is not validated...");
>>>>              uint32_t csum;
>>>>              if (netdev_tnl_is_header_ipv6(dp_packet_data(packet))) {
>>>>                  csum =
>>>> packet_csum_pseudoheader6(dp_packet_l3(packet));
>>>> sugeshch at silpixa00389816:~/repo/ovs_master$
>>>>
>>>> These debug messages are not showing at all when I am sending the
>>>> traffic. (I tried it with rx checksum ON and OFF)
>>>>
>>>> Looks like ol_flags are always reporting checksum is good.
>>>>
>>>> I am using DPDK 17.02 for the testing.
>>>> If I remember correctly it was reporting properly at the time of rx
>>>> checksum
>>> offload.
>>>> Looks like DPDK is reporting checksum valid in all the cases even it
>>>> is
>>> disabled.
>>>>
>>>> Any inputs on this?
>>>>
>>>
>>> Hi Sugesh, I was trying to fix the reconfiguration code not applying
>>> the OVSDB value so that's all I tested. I guess it's best to roll back
>>> to your original test and take it from there? You probably tested with
>>> DPDK
>>> 16.11.0 and I see some changes since then (e.g. below). Also, maybe
>>> you were testing enabled/disabled on first configure? It's the same
>>> configure code, but perhaps there is some different state in DPDK when
>>> the port is configured initially.
>>>
>> Yes, I tried to configure initially as well as run time.
>> [Sugesh] Also,
>> At the time of Rx checksum offload implementation, one of the comments
>> suggests not to use any configuration option at all.
>> Keep it ON for all the physical ports when it is supported. The reason being
>> the configuration option is added is due to the vectorization.
>> In the earlier DPDK releases the vectorization will turned off when checksum
>> offload is enabled.
>> I feel that is not the case for the latest DPDK releases (Vectorization is ON
>> with checksum offload).
>> If there is no impact of vectorization, then there is no usecase for having this
>> configuration option at all.
>> This way there is one less thing for the user to worry about. What do you
>> think?
>> Do you think it makes any backward compatibility issues??
> [Sugesh] Now I have tested with 16.11 and 17.2 DPDK releases.
> Here are the test cases I have run
> 1) Send tunnel traffic, Rx checksum offload ON
> 2) Send tunnel traffic, Rx checksum offload OFF
> 3) Send tunnel traffic, toggle Rx checksum offload configuration at run time.
> 4) Send tunnel traffic(With invalid checksum)
> 
> In all cases, DPDK report checksum status properly. But it doesn't honor the configuration changes at all.

That sounds like a bug in DPDK, no? You should probably let the
maintainer of whichever NIC you are using know about it.

> Considering the vector Rx works with Rx checksum , will you consider removing the
> Configuration option completely and keep it ON implicitly when it can supported in the NIC.
>  

Seems that way for Intel NICs. I guess the config option could be
removed if no one from other NIC vendors thinks it will cause issues for
them.

>>
>>> commit f3a85f4ce04d9fb55ebdb392563f7c1711f3d943
>>> Author: Qi Zhang <qi.z.zhang at intel.com>
>>> Date:   Tue Jan 24 14:06:59 2017 -0500
>>>
>>>     net/i40e: fix checksum flag in x86 vector Rx
>>>
>>>     When no error reported in Rx descriptor, we should set
>>>     CKSUM_GOOD flag before return.
>>>
>>>     Fixes: 9966a00a0688 ("net/i40e: enable bad checksum flags in vector Rx")
>>>     Cc: stable at dpdk.org
>>>
>>>     Signed-off-by: Qi Zhang <qi.z.zhang at intel.com>
>>>
>>>
>>> HTH,
>>> Kevin.
>>>
>>>>
>>>>
>>>> Regards
>>>> _Sugesh
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Kevin Traynor [mailto:ktraynor at redhat.com]
>>>>> Sent: Friday, May 12, 2017 6:22 PM
>>>>> To: dev at openvswitch.org
>>>>> Cc: Chandran, Sugesh <sugesh.chandran at intel.com>; Kevin Traynor
>>>>> <ktraynor at redhat.com>
>>>>> Subject: [PATCH v2 1/3] netdev-dpdk: Fix Rx checksum reconfigure.
>>>>>
>>>>> Rx checksum offload is enabled by default where available, with
>>>>> reconfiguration through OVSDB options:rx-checksum-offload.
>>>>> However, setting rx-checksum-offload does not result in a
>>>>> reconfiguration of the NIC.
>>>>>
>>>>> Fix that by checking if the requested port config features (e.g. rx
>>>>> checksum
>>>>> offload) are currently applied on the NIC and if not, perform a
>>>>> reconfiguration.
>>>>>
>>>>> Fixes: 1a2bb11817a4 ("netdev-dpdk: Enable Rx checksum offloading
>>>>> feature on DPDK physical ports.")
>>>>> Cc: Sugesh Chandran <sugesh.chandran at intel.com>
>>>>> Signed-off-by: Kevin Traynor <ktraynor at redhat.com>
>>>>> ---
>>>>>  lib/netdev-dpdk.c | 14 +++++++++-----
>>>>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
>>>>> 609b8da..d1688ce
>>>>> 100644
>>>>> --- a/lib/netdev-dpdk.c
>>>>> +++ b/lib/netdev-dpdk.c
>>>>> @@ -374,4 +374,5 @@ struct netdev_dpdk {
>>>>>      int requested_rxq_size;
>>>>>      int requested_txq_size;
>>>>> +    uint32_t requested_hwol;
>>>>>
>>>>>      /* Number of rx/tx descriptors for physical devices */ @@
>>>>> -648,5
>>>>> +649,5 @@ dpdk_eth_dev_queue_setup(struct netdev_dpdk *dev, int
>>>>> n_rxq, int
>>>>> n_txq)
>>>>>          conf.rxmode.max_rx_pkt_len = 0;
>>>>>      }
>>>>> -    conf.rxmode.hw_ip_checksum = (dev->hw_ol_features &
>>>>> +    conf.rxmode.hw_ip_checksum = (dev->requested_hwol &
>>>>>                                    NETDEV_RX_CHECKSUM_OFFLOAD) != 0;
>>>>>      /* A device may report more queues than it makes available
>>>>> (this has @@
>>>>> -702,4 +703,5 @@ dpdk_eth_dev_queue_setup(struct netdev_dpdk
>>> *dev,
>>>>> int n_rxq, int n_txq)
>>>>>          dev->up.n_rxq = n_rxq;
>>>>>          dev->up.n_txq = n_txq;
>>>>> +        dev->hw_ol_features = dev->requested_hwol;
>>>>>
>>>>>          return 0;
>>>>> @@ -719,5 +721,5 @@ dpdk_eth_checksum_offload_configure(struct
>>>>> netdev_dpdk *dev)
>>>>>                                       DEV_RX_OFFLOAD_IPV4_CKSUM;
>>>>>      rte_eth_dev_info_get(dev->port_id, &info);
>>>>> -    rx_csum_ol_flag = (dev->hw_ol_features &
>>>>> NETDEV_RX_CHECKSUM_OFFLOAD) != 0;
>>>>> +    rx_csum_ol_flag = (dev->requested_hwol &
>>>>> + NETDEV_RX_CHECKSUM_OFFLOAD) != 0;
>>>>>
>>>>>      if (rx_csum_ol_flag &&
>>>>> @@ -726,5 +728,5 @@ dpdk_eth_checksum_offload_configure(struct
>>>>> netdev_dpdk *dev)
>>>>>          VLOG_WARN_ONCE("Rx checksum offload is not supported on
>>>>> device %d",
>>>>>                     dev->port_id);
>>>>> -        dev->hw_ol_features &= ~NETDEV_RX_CHECKSUM_OFFLOAD;
>>>>> +        dev->requested_hwol &= ~NETDEV_RX_CHECKSUM_OFFLOAD;
>>>>>          return;
>>>>>      }
>>>>> @@ -872,4 +874,5 @@ common_construct(struct netdev *netdev,
>>> unsigned
>>>>> int port_no,
>>>>>      /* Initilize the hardware offload flags to 0 */
>>>>>      dev->hw_ol_features = 0;
>>>>> +    dev->requested_hwol = 0;
>>>>>
>>>>>      dev->flags = NETDEV_UP | NETDEV_PROMISC; @@ -1260,5 +1263,5
>>> @@
>>>>> netdev_dpdk_set_config(struct netdev *netdev, const struct smap
>> *args,
>>>>>                          != 0;
>>>>>      if (temp_flag != rx_chksm_ofld) {
>>>>> -        dev->hw_ol_features ^= NETDEV_RX_CHECKSUM_OFFLOAD;
>>>>> +        dev->requested_hwol ^= NETDEV_RX_CHECKSUM_OFFLOAD;
>>>>>          dpdk_eth_checksum_offload_configure(dev);
>>>>>      }
>>>>> @@ -3124,5 +3127,6 @@ netdev_dpdk_reconfigure(struct netdev
>>> *netdev)
>>>>>          && dev->rxq_size == dev->requested_rxq_size
>>>>>          && dev->txq_size == dev->requested_txq_size
>>>>> -        && dev->socket_id == dev->requested_socket_id) {
>>>>> +        && dev->socket_id == dev->requested_socket_id
>>>>> +        && dev->hw_ol_features == dev->requested_hwol) {
>>>>>          /* Reconfiguration is unnecessary */
>>>>>
>>>>> --
>>>>> 1.8.3.1
>>>>
> 



More information about the dev mailing list