On 05/30/2017 05:09 PM, Chandran, Sugesh wrote: > > > Regards > _Sugesh > > >> -----Original Message----- >> From: Kevin Traynor [mailto:[email protected]] >> Sent: Monday, May 29, 2017 1:37 PM >> To: Chandran, Sugesh <[email protected]> >> Cc: '[email protected]' <[email protected]> >> 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 <[email protected]> >>>> Cc: [email protected] >>>> Subject: RE: [PATCH v2 1/3] netdev-dpdk: Fix Rx checksum reconfigure. >>>> >>>> >>>> >>>> Regards >>>> _Sugesh >>>> >>>>> -----Original Message----- >>>>> From: Kevin Traynor [mailto:[email protected]] >>>>> Sent: Wednesday, May 17, 2017 10:10 AM >>>>> To: Chandran, Sugesh <[email protected]> >>>>> Cc: [email protected] >>>>> 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@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. >>>> >>>>> commit f3a85f4ce04d9fb55ebdb392563f7c1711f3d943 >>>>> Author: Qi Zhang <[email protected]> >>>>> 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: [email protected] >>>>> >>>>> Signed-off-by: Qi Zhang <[email protected]> >>>>> >>>>> >>>>> HTH, >>>>> Kevin. >>>>> >>>>>> >>>>>> >>>>>> Regards >>>>>> _Sugesh >>>>>> >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: Kevin Traynor [mailto:[email protected]] >>>>>>> Sent: Friday, May 12, 2017 6:22 PM >>>>>>> To: [email protected] >>>>>>> Cc: Chandran, Sugesh <[email protected]>; Kevin Traynor >>>>>>> <[email protected]> >>>>>>> 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 <[email protected]> >>>>>>> Signed-off-by: Kevin Traynor <[email protected]> >>>>>>> --- >>>>>>> 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
