Regards _Sugesh
> -----Original Message----- > From: Darrell Ball [mailto:db...@vmware.com] > Sent: Sunday, July 9, 2017 5:37 PM > To: Chandran, Sugesh <sugesh.chand...@intel.com>; d...@openvswitch.org > Subject: Re: [PATCH v3 2/2] conntrack : Use Rx checksum offload feature on > DPDK ports for conntrack. > > There is no need to call checksum_valid() if the caller already knows the > checksum is valid Accordingly, the parameter ‘validate_checksum’ is not > needed for checksum_valid() as this function should only be called if the > checksum is in question. > > Hence, how about this incremental: [Sugesh] Thank you for the Suggestion Darrel. Yes that looks OK for me. I will modify the code and send v4. > > diff --git a/lib/conntrack.c b/lib/conntrack.c index 6f40df2..dcd82a7 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -1119,13 +1119,10 @@ 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, bool validate_checksum) > + const void *l3) > { > 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)) { @@ -1153,7 +1150,7 > @@ check_l4_tcp(const struct conn_key *key, const void *data, size_t size, > return false; > } > > - return checksum_valid(key, data, size, l3, validate_checksum); > + return validate_checksum ? checksum_valid(key, data, size, l3) : > + true; > } > > static inline bool > @@ -1172,23 +1169,20 @@ 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, validate_checksum); > + || (validate_checksum ? checksum_valid(key, data, size, l3) > + : true); > } > > static inline bool > check_l4_icmp(const void *data, size_t size, bool validate_checksum) { > - if (validate_checksum) { > - return csum(data, size) == 0; > - } > - return true; > + return validate_checksum ? csum(data, size) == 0 : true; > } > > static inline bool > check_l4_icmp6(const struct conn_key *key, const void *data, size_t size, > const void *l3, bool validate_checksum) { > - return checksum_valid(key, data, size, l3, validate_checksum); > + return validate_checksum ? checksum_valid(key, data, size, l3) : > + true; > } > > static inline bool > > //////////////////////////////////////////////////// > > > On 6/27/17, 7:39 AM, "Sugesh Chandran" <sugesh.chand...@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.chand...@intel.com> > --- > lib/conntrack.c | 71 ++++++++++++++++++++++++++++++++++++++------ > ------------- > lib/dp-packet.h | 22 ++++++++++++++++++ > 2 files changed, 69 insertions(+), 24 deletions(-) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 90b154a..6f40df2 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; > } > @@ -1494,17 +1502,32 @@ 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); > + bool hwol_bad_l3_csum = dp_packet_ip_checksum_bad(pkt); > + if (hwol_bad_l3_csum) { > + ok = false; > + } else { > + bool hwol_good_l3_csum = dp_packet_ip_checksum_valid(pkt); > + /* Validate the checksum only when hwol is not supported. */ > + ok = extract_l3_ipv4(&ctx->key, l3, tail - (char *) l3, NULL, > + !hwol_good_l3_csum); > + } > } else if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) { > ok = extract_l3_ipv6(&ctx->key, l3, tail - (char *) l3, NULL); > } else { > ok = false; > } > > + > if (ok) { > - if (extract_l4(&ctx->key, l4, tail - l4, &ctx->related, l3)) { > - ctx->hash = conn_key_hash(&ctx->key, ct->hash_basis); > - return true; > + bool hwol_bad_l4_csum = dp_packet_l4_checksum_bad(pkt); > + if (!hwol_bad_l4_csum) { > + bool hwol_good_l4_csum = dp_packet_l4_checksum_valid(pkt); > + /* Validate the checksum only when hwol is not supported. */ > + if (extract_l4(&ctx->key, l4, tail - l4, &ctx->related, l3, > + !hwol_good_l4_csum)) { > + ctx->hash = conn_key_hash(&ctx->key, ct->hash_basis); > + return true; > + } > } > } > > diff --git a/lib/dp-packet.h b/lib/dp-packet.h > index 84cc434..a1de0f2 100644 > --- a/lib/dp-packet.h > +++ b/lib/dp-packet.h > @@ -620,6 +620,17 @@ dp_packet_ip_checksum_valid(struct dp_packet > *p) > } > > static inline bool > +dp_packet_ip_checksum_bad(struct dp_packet *p) > +{ > +#ifdef DPDK_NETDEV > + return (p->mbuf.ol_flags & PKT_RX_IP_CKSUM_MASK) == > + PKT_RX_IP_CKSUM_BAD; > +#else > + return 0 && p; > +#endif > +} > + > +static inline bool > dp_packet_l4_checksum_valid(struct dp_packet *p) > { > #ifdef DPDK_NETDEV > @@ -630,6 +641,17 @@ dp_packet_l4_checksum_valid(struct dp_packet > *p) > #endif > } > > +static inline bool > +dp_packet_l4_checksum_bad(struct dp_packet *p) > +{ > +#ifdef DPDK_NETDEV > + return (p->mbuf.ol_flags & PKT_RX_L4_CKSUM_MASK) == > + PKT_RX_L4_CKSUM_BAD; > +#else > + return 0 && p; > +#endif > +} > + > #ifdef DPDK_NETDEV > static inline void > reset_dp_packet_checksum_ol_flags(struct dp_packet *p) > -- > 2.7.4 > > > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev