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

Chandran, Sugesh sugesh.chandran at intel.com
Tue Aug 23 09:57:31 UTC 2016


Hi Jesse,
Please find my answers inline below.

Regards
_Sugesh

> -----Original Message-----
> From: Jesse Gross [mailto:jesse at kernel.org]
> Sent: Monday, August 22, 2016 7:50 PM
> To: Chandran, Sugesh <sugesh.chandran at intel.com>
> Cc: ovs dev <dev at 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.chandran at 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.
Please let me know.
> 
> Generally, this looks good to me though. Obviously, we won't be able to
> apply it until there is support for DPDK 16.11 in OVS. Will you hold onto the
> patch and resubmit it at that time?
[Sugesh] Yes, that’s right. I will hold this patch and will send out again once the 16.11
is out.


More information about the dev mailing list