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