[ovs-dev,v2] conntrack : Use Rx checksum offload feature on DPDK ports for conntrack.

Hi Sugesh

I have a few questions:

1) There is no checking for bad hwol determined checksums
If the dpdk documentation is correct, then this is indicated by both good and bad
flags set for both L3 and L4.
2) If hwol can reliably indicate when a checksum is known to be bad, then we should
stop right there and say the packet is bad, which we don’t do here.
We are checking if it is not known to be good and if not, do checksum verify in software.
I added a dp-packet API to check for bad, assuming I understood the rte documentation
and the documentation is accurate.
Please confirm if a bad checksum can reliably be determined by rte hwol.
3) Now, if the checksum is not bad according to hwol, we can proceed to check if it is
known to be good by hwol. It seems that the existing dp-packet API does not
definitely check for good, since if the rte documentation is correct, the good flag 
is set in both good and bad checksum cases, but in the bad case, the bad flag is
also set.  I updated the API according to my reading of the rte documentation.
Please verify if the documentation is correct and my reading is correct.
4) If the checksum is not known to bad or good by hwol, then it is undetermined
and must be checked by OVS.

I have some code below, which is an incremental from yours, that describes ‘1-4’ above.
Let me know if my read of the rte documentation is correct and the documentation
itself is accurate. The diff includes a bit of coding standard changes.

////////////////////////////////////////////////////////////////////

On 6/16/17, 4:02 AM, "Sugesh Chandran" <sugesh.chandran@intel.com> wrote:

    Avoiding checksum validation in conntrack module if it is already verified
    in DPDK physical NIC ports.

    Signed-off-by: Sugesh Chandran <sugesh.chandran@intel.com>

    Acked-by: Antonio Fischetti <antonio.fischetti@intel.com>

    Tested-by: Antonio Fischetti <antonio.fischetti@intel.com>

    ----
    v1->v2
     - Rebased on master
     - Added acked-by and tested-by tags in commit message
    ---
     lib/conntrack.c | 60 ++++++++++++++++++++++++++++++++++++---------------------
     1 file changed, 38 insertions(+), 22 deletions(-)

    diff --git a/lib/conntrack.c b/lib/conntrack.c
    index 90b154a..38084e9 100644
    --- a/lib/conntrack.c
    +++ b/lib/conntrack.c
    @@ -1119,10 +1119,13 @@ extract_l3_ipv6(struct conn_key *key, const void *data, size_t size,

     static inline bool
     checksum_valid(const struct conn_key *key, const void *data, size_t size,
    -               const void *l3)
    +               const void *l3, bool validate_checksum)
     {
         uint32_t csum = 0;

    +    if (!validate_checksum) {
    +       return true;
    +    }
         if (key->dl_type == htons(ETH_TYPE_IP)) {
             csum = packet_csum_pseudoheader(l3);
         } else if (key->dl_type == htons(ETH_TYPE_IPV6)) {
    @@ -1138,7 +1141,7 @@ checksum_valid(const struct conn_key *key, const void *data, size_t size,

     static inline bool
     check_l4_tcp(const struct conn_key *key, const void *data, size_t size,
    -             const void *l3)
    +             const void *l3, bool validate_checksum)
     {
         const struct tcp_header *tcp = data;
         if (size < sizeof *tcp) {
    @@ -1150,12 +1153,12 @@ check_l4_tcp(const struct conn_key *key, const void *data, size_t size,
             return false;
         }

    -    return checksum_valid(key, data, size, l3);
    +    return checksum_valid(key, data, size, l3, validate_checksum);
     }

     static inline bool
     check_l4_udp(const struct conn_key *key, const void *data, size_t size,
    -             const void *l3)
    +             const void *l3, bool validate_checksum)
     {
         const struct udp_header *udp = data;
         if (size < sizeof *udp) {
    @@ -1169,20 +1172,23 @@ check_l4_udp(const struct conn_key *key, const void *data, size_t size,

         /* Validation must be skipped if checksum is 0 on IPv4 packets */
         return (udp->udp_csum == 0 && key->dl_type == htons(ETH_TYPE_IP))
    -           || checksum_valid(key, data, size, l3);
    +           || checksum_valid(key, data, size, l3, validate_checksum);
     }

     static inline bool
    -check_l4_icmp(const void *data, size_t size)
    +check_l4_icmp(const void *data, size_t size, bool validate_checksum)
     {
    -    return csum(data, size) == 0;
    +    if (validate_checksum) {
    +        return csum(data, size) == 0;
    +    }
    +    return true;
     }

     static inline bool
     check_l4_icmp6(const struct conn_key *key, const void *data, size_t size,
    -               const void *l3)
    +               const void *l3, bool validate_checksum)
     {
    -    return checksum_valid(key, data, size, l3);
    +    return checksum_valid(key, data, size, l3, validate_checksum);
     }

     static inline bool
    @@ -1218,7 +1224,8 @@ extract_l4_udp(struct conn_key *key, const void *data, size_t size)
     }

     static inline bool extract_l4(struct conn_key *key, const void *data,
    -                              size_t size, bool *related, const void *l3);
    +                              size_t size, bool *related, const void *l3,
    +                              bool validate_checksum);

     static uint8_t
     reverse_icmp_type(uint8_t type)
    @@ -1306,7 +1313,7 @@ extract_l4_icmp(struct conn_key *key, const void *data, size_t size,
             key->dst = inner_key.dst;
             key->nw_proto = inner_key.nw_proto;

    -        ok = extract_l4(key, l4, tail - l4, NULL, l3);
    +        ok = extract_l4(key, l4, tail - l4, NULL, l3, false);
             if (ok) {
                 conn_key_reverse(key);
                 *related = true;
    @@ -1396,7 +1403,7 @@ extract_l4_icmp6(struct conn_key *key, const void *data, size_t size,
             key->dst = inner_key.dst;
             key->nw_proto = inner_key.nw_proto;

    -        ok = extract_l4(key, l4, tail - l4, NULL, l3);
    +        ok = extract_l4(key, l4, tail - l4, NULL, l3, false);
             if (ok) {
                 conn_key_reverse(key);
                 *related = true;
    @@ -1421,22 +1428,23 @@ extract_l4_icmp6(struct conn_key *key, const void *data, size_t size,
      * in an ICMP error.  In this case, we skip checksum and length validation. */
     static inline bool
     extract_l4(struct conn_key *key, const void *data, size_t size, bool *related,
    -           const void *l3)
    +           const void *l3, bool validate_checksum)
     {
         if (key->nw_proto == IPPROTO_TCP) {
    -        return (!related || check_l4_tcp(key, data, size, l3))
    -               && extract_l4_tcp(key, data, size);
    +        return (!related || check_l4_tcp(key, data, size, l3,
    +                validate_checksum)) && extract_l4_tcp(key, data, size);
         } else if (key->nw_proto == IPPROTO_UDP) {
    -        return (!related || check_l4_udp(key, data, size, l3))
    -               && extract_l4_udp(key, data, size);
    +        return (!related || check_l4_udp(key, data, size, l3,
    +                validate_checksum)) && extract_l4_udp(key, data, size);
         } else if (key->dl_type == htons(ETH_TYPE_IP)
                    && key->nw_proto == IPPROTO_ICMP) {
    -        return (!related || check_l4_icmp(data, size))
    +        return (!related || check_l4_icmp(data, size, validate_checksum))
                    && extract_l4_icmp(key, data, size, related);
         } else if (key->dl_type == htons(ETH_TYPE_IPV6)
                    && key->nw_proto == IPPROTO_ICMPV6) {
    -        return (!related || check_l4_icmp6(key, data, size, l3))
    -               && extract_l4_icmp6(key, data, size, related);
    +        return (!related || check_l4_icmp6(key, data, size, l3,
    +                validate_checksum)) && extract_l4_icmp6(key, data, size,
    +                related);
         } else {
             return false;
         }
    @@ -1451,6 +1459,8 @@ conn_key_extract(struct conntrack *ct, struct dp_packet *pkt, ovs_be16 dl_type,
         const char *l4 = dp_packet_l4(pkt);
         const char *tail = dp_packet_tail(pkt);
         bool ok;
    +    bool valid_l4_csum = 0;
    +    bool  valid_l3_csum = 0;

         memset(ctx, 0, sizeof *ctx);

    @@ -1494,7 +1504,10 @@ conn_key_extract(struct conntrack *ct, struct dp_packet *pkt, ovs_be16 dl_type,
          */
         ctx->key.dl_type = dl_type;
         if (ctx->key.dl_type == htons(ETH_TYPE_IP)) {
    -        ok = extract_l3_ipv4(&ctx->key, l3, tail - (char *) l3, NULL, true);
    +        valid_l3_csum = dp_packet_ip_checksum_valid(pkt);
    +        /* Validate the checksum only when the csum is invalid */
    +        ok = extract_l3_ipv4(&ctx->key, l3, tail - (char *) l3, NULL,
    +                             !valid_l3_csum);
         } else if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) {
             ok = extract_l3_ipv6(&ctx->key, l3, tail - (char *) l3, NULL);
         } else {
    @@ -1502,7 +1515,10 @@ conn_key_extract(struct conntrack *ct, struct dp_packet *pkt, ovs_be16 dl_type,
         }

         if (ok) {
    -        if (extract_l4(&ctx->key, l4, tail - l4, &ctx->related, l3)) {
    +        valid_l4_csum = dp_packet_l4_checksum_valid(pkt);
    +        /* Validate the ckecksum only when the checksum is not valid/unset */
    +        if (extract_l4(&ctx->key, l4, tail - l4, &ctx->related, l3,
    +            !valid_l4_csum)) {
                 ctx->hash = conn_key_hash(&ctx->key, ct->hash_basis);
                 return true;
             }
    -- 
    2.7.4

[ovs-dev,v2] conntrack : Use Rx checksum offload feature on DPDK ports for conntrack.

Commit Message

Comments

Patch