On Tue, Aug 23, 2016 at 2:57 AM, Chandran, Sugesh <sugesh.chand...@intel.com> wrote: >> -----Original Message----- >> From: Jesse Gross [mailto:je...@kernel.org] >> Sent: Monday, August 22, 2016 7:50 PM >> To: Chandran, Sugesh <sugesh.chand...@intel.com> >> Cc: ovs dev <dev@openvswitch.org> >> Subject: Re: [RFC PATCHv3] netdev-dpdk: Enable Rx checksum offloading >> feature on DPDK physical ports. >> >> On Mon, Aug 22, 2016 at 6:40 AM, Sugesh Chandran >> <sugesh.chand...@intel.com> wrote: >> > diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c index >> > ce2582f..78ce0c9 100644 >> > --- a/lib/netdev-native-tnl.c >> > +++ b/lib/netdev-native-tnl.c >> > @@ -85,11 +85,17 @@ 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; >> > + } >> > } >> > >> > + /* Reset the IP checksum offload flags if present, to avoid wrong >> > + * interpretation in the further packet processing when >> recirculated.*/ >> > + reset_dp_packet_ip_checksum_ol_flags(packet); >> > + >> > if (ntohs(ip->ip_tot_len) > l3_size) { >> > VLOG_WARN_RL(&err_rl, "ip packet is truncated (IP length %d, >> actual %d)", >> > ntohs(ip->ip_tot_len), l3_size); @@ -179,20 >> > +185,26 @@ 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; >> > + >> > + /* Reset the udp checksum offload flags if present, to avoid wrong >> > + * interpretation in the further packet processing when >> recirculated.*/ >> > + reset_dp_packet_l4_checksum_ol_flags(packet); >> > } >> >> Just one final comment on this - I think it's probably better to >> unconditionally >> clear the checksum offload flags (both L3 and L4) whenever we pop off a >> tunnel. Those headers will no longer exist in the packet and therefore those >> flags can't have any meaning. In theory this shouldn't make any difference >> compared to what you have here but it seems a little bit more robust. > [Sugesh] I kept the reset logic separate for each layer intentionally to > provide better modularity > and maintainability. For eg: In future to implement IP-in-IP tunneling, the > checksum > flags has to be cleared in IP layer, not in UDP. > Also considering your previous comment, the chances of having different > combination > of checksum offload, its nice to keep the reset logic specific to layers for > better readability. > What do you think? > As you said, for all the existing tunneling implementation it should be fine > to clear off the flags in one place > unconditionally. If you feel, it make more sense to separate the reset logic > at the time of need comes, I am OK > to send out next version with proposed changes.
The IPIP tunnel case is actually the type of situation that makes me a little nervous - if the UDP checksum flag was set in that case, what does it refer to? I suppose it's possible that the driver is capable of understand IPIP tunnels and it's an inner L4 checksum and we could use that later. However, at the moment, the infrastructure isn't really there for us to track what the checksum is referring to so it seems like it could be error prone. In practice, hopefully none of these situations will really end up being an issue and we can always change this code later if necessary. I still think it is generally safer to clear all of the checksum offloads whenever we pop off a tunnel but the only one that I think is really important now is the UDP checksum flag - we should clear that whenever we remove a UDP header, regardless of whether udp->udp_csum is non-zero. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
