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.
>> > 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.
>> > 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
