Regards _Sugesh
> -----Original Message----- > From: Jesse Gross [mailto:je...@kernel.org] > Sent: Wednesday, August 24, 2016 3:59 AM > 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 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. > [Sugesh] Make sense, probably when the support is available is the future , we may change the code accordingly that time. > 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. [Sugesh] Thank you for the response Jesse, I will send out next version of patch with those changes. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
