[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