[ovs-dev] [PATCH v3 2/2] conntrack : Use Rx checksum offload feature on DPDK ports for conntrack.
Chandran, Sugesh
sugesh.chandran at intel.com
Mon Jul 10 10:28:43 UTC 2017
Regards
_Sugesh
> -----Original Message-----
> From: Darrell Ball [mailto:dball at vmware.com]
> Sent: Sunday, July 9, 2017 5:37 PM
> To: Chandran, Sugesh <sugesh.chandran at intel.com>; dev at 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.chandran at 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 at 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
>
>
>
>
More information about the dev
mailing list