[ovs-dev] [PATCH v2 1/3] netdev-dpdk: Fix Rx checksum reconfigure.

[Kevin Traynor]

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.

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
>