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

Kevin Traynor ktraynor at redhat.com
Thu Jun 8 17:23:51 UTC 2017


On 06/01/2017 02:19 AM, Darrell Ball wrote:
> 
> 
> On 5/31/17, 6:59 AM, "ovs-dev-bounces at openvswitch.org on behalf of Kevin Traynor" <ovs-dev-bounces at openvswitch.org on behalf of ktraynor at redhat.com> wrote:
> 
>     On 05/30/2017 05:09 PM, Chandran, Sugesh wrote:
>     > 
>     > 
>     > Regards
>     > _Sugesh
>     > 
>     > 
>     >> -----Original Message-----
>     >> From: Kevin Traynor [mailto:ktraynor at redhat.com]
>     >> Sent: Monday, May 29, 2017 1:37 PM
>     >> To: Chandran, Sugesh <sugesh.chandran at intel.com>
>     >> Cc: 'dev at openvswitch.org' <dev at openvswitch.org>
>     >> Subject: Re: [PATCH v2 1/3] netdev-dpdk: Fix Rx checksum reconfigure.
>     >>
>     >> 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.
>     >>
>     > [Sugesh] OK, I feel DPDK offers vector processing with Rx checksum offload irrespective of any NIC.(I feel that all NIC drivers supports it now)
>     > Hence removing this option and keep ON by default doesn't harm anyone.
>     > Do you have any other concern on removing the option?
>     > 
>     
>     No, not from my side. I'll give a few more days to see if anyone raises
>     concern and then I will respin my patchset to remove the user config
>     option. I'll see what parts of the patches are still useful too as we've
>     already seen patches for enabling/disabling other eth config features,
>     so I think some of the generic code can help.
> 
> 
> I am not sure waiting a few days for people to mention an issue will give
> more confidence that there are no outlier nics.
> However, it is a pretty ugly workaround option that might remain forever
> for fear of some corner case. It is probably better to just remove it. 
> 

Sure. They ended up getting a few days anyway but not by design :-)

Just sent a v3 that removes the reconfig option.

Kevin.

>     .
> 
>     >>>>
>     >>>>> 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
>     >>>>>>
>     >>>
>     > 
>     
>     _______________________________________________
>     dev mailing list
>     dev at openvswitch.org
>     https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=KZU6iMKERSoEZNioJQdb3TDmWU_srhsxK2BXDVM6pY8&s=IvwIf4CNa9CqplW_RlYO-EkoqD2AJwIHHK7j5z9kzvk&e= 
>     
> 



More information about the dev mailing list