Regards _Sugesh
> -----Original Message----- > From: Jesse Gross [mailto:je...@kernel.org] > Sent: Thursday, August 18, 2016 2:03 AM > To: Chandran, Sugesh <sugesh.chand...@intel.com> > Cc: ovs dev <dev@openvswitch.org> > Subject: Re: [PATCH RFC] netdev-dpdk: Rx checksum offloading feature on > DPDK physical ports. > > On Tue, Aug 16, 2016 at 3:06 AM, Chandran, Sugesh > <sugesh.chand...@intel.com> wrote: > >> -----Original Message----- > >> From: Jesse Gross [mailto:je...@kernel.org] > >> Sent: Monday, August 15, 2016 6:44 PM > >> To: Chandran, Sugesh <sugesh.chand...@intel.com> > >> Cc: ovs dev <dev@openvswitch.org> > >> Subject: Re: [PATCH RFC] netdev-dpdk: Rx checksum offloading feature > >> on DPDK physical ports. > >> > >> On Mon, Aug 8, 2016 at 3:03 AM, Sugesh Chandran > >> <sugesh.chand...@intel.com> wrote: > >> > To enable checksum offloading at rx side while adding a port, add > >> > the 'rx-checksum-offload' option to the 'ovs-vsctl add-port' > >> > command-line as below, > >> > > >> > 'ovs-vsctl add-port br0 dpdk0 -- \ > >> > set Interface dpdk0 type=dpdk options:rx-checksum-offload=true' > >> > >> Is there a reason not to enable this by default? It seems like it > >> would be beneficial to more users that way. > >> > >> I think we should also add some documentation for this new option. > >> > > [Sugesh] : I will add the documentation in the v2 release of patch. > > My concern over make it enabled by default is, the possible > > performance impact of being Vectorization OFF which is completely > > depends on the traffic pattern and packet size. Though I haven’t find > > any thing as such in my testing. This will avoid any surprises for the > customer. Any comments? > > In this case we are comparing two different optimizations - checksum offload > and vectorization - which both depend on the traffic pattern. > Potentially either could have cases where they are less effective, so it seems > like we should pick the one that is the most effective in the majority of > cases. > Since it looks like checksum offload helps in all cases that we know about, I > think that we should turn it on by default. [Sugesh] Agreed, Sending out v2 patch with Rx checksum is ON by default. > > >> > 2) Normally, OVS generates checksum for tunnel packets. It is more > >> > efficient to calculate the checksum at 'tunnel_push' operation > >> > itself as all the relevant fields are already present in the cache > >> > and also no additional validation overhead is involved as in checksum > offloading. > >> > >> I don't quite understand this comment. This is more efficient > >> compared to what? Presumably, at least for large packets, there would > >> still be some benefit if the issue with losing vectorization wasn't > >> present. > > [Sugesh] Sorry for the confusion. This is about generating checksum > > for tunnel packets at encap side(i.e tx checksum offload). What I > > meant here is generating checksum for tunnel packets at the time of > > 'tunnel_push' is more efficient than offloading into the NIC. > > In detail, At the time of packet encapsulation in 'tunnel_push' , > > the egress port for the tunnel packet is unknown. The tunnel packets > > are recirculated in OVS to decide the action, which can be either > > send to a phy port with tx checksum offload or send to phy port without tx > offload, or even send to any other virtual/kernel port. > > So to offload the Tx checksum in NIC, > > *) Mark the packet for checksum offload at the time of tunnel_push > > *) At 'netdev_send' validate every packet for tx_checksum offload > > flag, > > *) If tx checksum offload-flag is set, Calculate the checksum in > > software or hardware based on NIC offload configuration. > > > > While implementing, we found that the validating every packet at > > netdev_send > > + vectorization OFF make performance worse than calculating checksum > > + at the time of > > 'tunnel_push' because the cache is warm. > > The worst case is sending packet to a port with no tx checksum > > offload. It makes additional overhead of > > 1) Marking every packet with offload flag at the time of tunnel_push > > 2) validate every packet for offload flag at netdev_send, > > 3) Since the port doesn’t support offload, load all the fields into > > cache and calculate the checksum in software before sending out. > > Please let me know if anything is wrong / not clear? > > I think I generally understand and clearly attempting to offload is worse if > ultimately we won't actually be able to offload the packet. > At least for physical NICs, the vast majority do have checksum offloading so > my guess is that the main issue here is loss of vectorization. At least for > largish packets, I would think that the benefits of not touching the payload > data would outweigh the additional checks. > > In any case, I think it might be helpful to make the commit message a bit > more clear on the tradeoffs. [Sugesh] Will change the commit message section accordingly. > > >> > diff --git a/lib/dp-packet.h b/lib/dp-packet.h index > >> > 7c1e637..f2e9d4f > >> > 100644 > >> > --- a/lib/dp-packet.h > >> > +++ b/lib/dp-packet.h > >> > @@ -38,6 +38,14 @@ enum OVS_PACKED_ENUM dp_packet_source { > >> > > >> > #define DP_PACKET_CONTEXT_SIZE 64 > >> > > >> > +enum hw_pkt_ol_flags { > >> > + OL_RX_IP_CKSUM_GOOD = (1 << 0), > >> > + OL_RX_L4_CKSUM_GOOD = (1 << 1), > >> > + OL_RX_IP_CKSUM_BAD = (1 << 2), > >> > + OL_RX_L4_CKSUM_BAD = (1 << 3), > >> > >> Is it necessary to redefine this fields in OVS instead of just using > >> them from the DPDK mbuf directly? This means that we have to do > >> mapping and other checks at runtime, which if I remember correctly > >> resulted in a performance hit when you posted numbers before. > > [Sugesh] : Yes, the performance improvement will be slightly less due > > to these new flags. The idea here is to generalize the hardware > > offloading , so that it can be extended for other hardware offloading > > as well. Something like exposing the hardware offloading to virtual ports/tx > offloading as well in the future? Comments? > > I think in this case it might be best to just keep things simple and do what > makes sense for now. Usually, I would also prefer to generalize things but in > this case there aren't any public interfaces that we have to worry about > maintaining and in fact the infrastructure isn't really necessary at all right > now. If we only add receive checksum offloading the patch becomes almost > trivially simple, so that is nice. When it comes time to add additional > offloading we might know more about what is necessary. > > > Just to confirm, if we are not using the new flags, still we need to > > verify the source of packet like below, > > if (packet->source == DPBUF_DPDK) > > /* Do the checksum validation */ > > If the source isn't DPDK, won't these flags just have a value of zero and > therefore not show up as checksum good? _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
