On 06/01/2017 02:19 AM, Darrell Ball wrote: > > > On 5/31/17, 6:59 AM, "[email protected] on behalf of Kevin > Traynor" <[email protected] on behalf of [email protected]> > wrote: > > 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. > > > 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 <[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://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= > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
