[ovs-dev] [PATCH v2 1/3] netdev-dpdk: Fix Rx checksum reconfigure.
Chandran, Sugesh
sugesh.chandran at intel.com
Tue May 30 16:09:38 UTC 2017
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?
> >>
> >>> 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
> >>>>
> >
More information about the dev
mailing list