[ovs-dev] [PATCH RFC] netdev-dpdk: Rx checksum offloading feature on DPDK physical ports.

[Chandran, Sugesh]

Regards
_Sugesh


> -----Original Message-----
> From: Jesse Gross [mailto:jesse at kernel.org]
> Sent: Thursday, August 18, 2016 2:03 AM
> To: Chandran, Sugesh <sugesh.chandran at intel.com>
> Cc: ovs dev <dev at 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.chandran at intel.com> wrote:
> >> -----Original Message-----
> >> From: Jesse Gross [mailto:jesse at kernel.org]
> >> Sent: Monday, August 15, 2016 6:44 PM
> >> To: Chandran, Sugesh <sugesh.chandran at intel.com>
> >> Cc: ovs dev <dev at 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.chandran at 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?