On Fri, Aug 19, 2016 at 3:40 AM, Sugesh Chandran
<sugesh.chand...@intel.com> wrote:
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index e5f2cdd..113e6d8 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1090,6 +1127,15 @@ netdev_dpdk_set_config(struct netdev *netdev, const
> struct smap *args)
>
> dpdk_eth_flow_ctrl_setup(dev);
>
> + /* Rx checksum offload configuration */
> + /* By default the Rx checksum offload is ON */
> + rx_chksm_ofld = smap_get_bool(args, "rx-checksum-offload", true);
> + temp_flag = (dev->hw_ol_features & NETDEV_RX_CHECKSUM_OFFLOAD)
> + != 0;
> + if (temp_flag != rx_chksm_ofld) {
> + dev->hw_ol_features ^= NETDEV_RX_CHECKSUM_OFFLOAD;
> + }
> + dpdk_eth_checksum_offload_configure(dev);
> ovs_mutex_unlock(&dev->mutex);
I think that while we are going through the trouble of checking
whether the old flag is different from the new one, we might as well
only call dpdk_eth_checksum_offload_configure() if there is a
difference.
> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
> index ce2582f..23e987c 100644
> --- a/lib/netdev-native-tnl.c
> +++ b/lib/netdev-native-tnl.c
> @@ -85,9 +85,11 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet *packet,
> struct flow_tnl *tnl,
>
> ovs_be32 ip_src, ip_dst;
>
> - if (csum(ip, IP_IHL(ip->ip_ihl_ver) * 4)) {
> - VLOG_WARN_RL(&err_rl, "ip packet has invalid checksum");
> - return NULL;
> + if(OVS_UNLIKELY(!dp_packet_ip_checksum_valid(packet))) {
> + if (csum(ip, IP_IHL(ip->ip_ihl_ver) * 4)) {
> + VLOG_WARN_RL(&err_rl, "ip packet has invalid checksum");
> + return NULL;
> + }
> }
>
> if (ntohs(ip->ip_tot_len) > l3_size) {
> @@ -179,18 +181,20 @@ udp_extract_tnl_md(struct dp_packet *packet, struct
> flow_tnl *tnl,
> }
>
> if (udp->udp_csum) {
> - uint32_t csum;
> - if (netdev_tnl_is_header_ipv6(dp_packet_data(packet))) {
> - csum = packet_csum_pseudoheader6(dp_packet_l3(packet));
> - } else {
> - csum = packet_csum_pseudoheader(dp_packet_l3(packet));
> - }
> -
> - csum = csum_continue(csum, udp, dp_packet_size(packet) -
> - ((const unsigned char *)udp -
> - (const unsigned char *)dp_packet_l2(packet)));
> - if (csum_finish(csum)) {
> - return NULL;
> + if(OVS_UNLIKELY(!dp_packet_l4_checksum_valid(packet))) {
> + uint32_t csum;
> + if (netdev_tnl_is_header_ipv6(dp_packet_data(packet))) {
> + csum = packet_csum_pseudoheader6(dp_packet_l3(packet));
> + } else {
> + csum = packet_csum_pseudoheader(dp_packet_l3(packet));
> + }
> +
> + csum = csum_continue(csum, udp, dp_packet_size(packet) -
> + ((const unsigned char *)udp -
> + (const unsigned char
> *)dp_packet_l2(packet)));
> + if (csum_finish(csum)) {
> + return NULL;
> + }
> }
> tnl->flags |= FLOW_TNL_F_CSUM;
> }
I think the previous version of the patch cleared the offload flags
after popping off the tunnel headers. I think that's probably a good
idea here as well. Since we're not terminating the payload it's
probably relatively rare that we would have multiple checksums but
it's theoretically possible that we might have a tunnel-in-tunnel
situation (and I suppose that it's that we might want to use checksum
offload for higher level services - connection tracking/NAT/load
balancing, etc.).
In the future, if DPDK offloads become tunnel aware some of these
issues can become quite complicated due to the presence of multiple
checksums being offloaded and how that is represented. On the Linux
side there was a fair amount of work put into this over the past
several years, so it might be worth taking a look at that for some
inspiration before the DPDK interfaces get locked down.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
