[ovs-dev] [PATCH v2 1/3] netdev-dpdk: Fix Rx checksum reconfigure.
Darrell Ball
dball at vmware.com
Thu Jun 1 01:19:47 UTC 2017
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.
.
>>>>
>>>>> 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