Hi Sugesh, it looks good to me, it makes sense to leverage the csum info when present.
I've tested it with the firewall rules - see below for details - I saw a ~+3% improvement in my testbench with 10 UDP connections. Traffic Gen: IXIA IxExplorer 10 UDP different flows, 64B pkts Original OvS: 3.0 Mpps With this Patch: 3.1 Mpps Below some details of my testbench. =================================== BUILD ----- make -j 28 CFLAGS="-O2 -march=native -g" #I didn't use intrinsics, I expect in that case the benefit will be smaller. FLOW DUMP --------- NXST_FLOW reply (xid=0x4): cookie=0x0, duration=0.064s, table=0, n_packets=0, n_bytes=0, idle_age=0, priority=100,ct_state=-trk,ip actions=ct(table=1) cookie=0x0, duration=0.075s, table=0, n_packets=0, n_bytes=0, idle_age=0, priority=10,arp actions=NORMAL cookie=0x0, duration=0.085s, table=0, n_packets=0, n_bytes=0, idle_age=0, priority=1 actions=drop cookie=0x0, duration=0.054s, table=1, n_packets=0, n_bytes=0, idle_age=0, ct_state=+new+trk,ip,in_port=1 actions=ct(commit),output:2 cookie=0x0, duration=0.033s, table=1, n_packets=0, n_bytes=0, idle_age=0, ct_state=+new+trk,ip,in_port=2 actions=drop cookie=0x0, duration=0.043s, table=1, n_packets=0, n_bytes=0, idle_age=0, ct_state=+est+trk,ip,in_port=1 actions=output:2 cookie=0x0, duration=0.023s, table=1, n_packets=0, n_bytes=0, idle_age=0, ct_state=+est+trk,ip,in_port=2 actions=output:1 HugePages_Total: 20480 HugePages_Free: 20480 HugePages_Rsvd: 0 HugePages_Surp: 0 _______________________________________________ DPDK: HEAD detached at v16.11 OvS: On my local branch ConnTrack_01 _______________________________________________ PID PSR COMMAND %CPU 20509 0 ovsdb-server 0.0 20522 2 ovs-vswitchd 78.1 20522 4 pmd62 80.8 20522 5 pmd61 71.6 PDM threads: 2 configured_tx_queues=3, configured_tx_queues=3, ======================== Regards, Antonio Acked-by: Antonio Fischetti <[email protected]> > -----Original Message----- > From: [email protected] [mailto:ovs-dev- > [email protected]] On Behalf Of Sugesh Chandran > Sent: Thursday, May 25, 2017 10:11 PM > To: [email protected] > Subject: [ovs-dev] [PATCH] conntrack : Use Rx checksum offload feature on > DPDK ports for conntrack. > > Avoiding checksum validation in conntrack module if it is already verified > in DPDK physical NIC ports. > > Signed-off-by: Sugesh Chandran <[email protected]> > --- > lib/conntrack.c | 58 ++++++++++++++++++++++++++++++++++++---------------- > ----- > 1 file changed, 37 insertions(+), 21 deletions(-) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index cb30ac7..af6a372 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -642,10 +642,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)) { > @@ -661,7 +664,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) { > @@ -673,12 +676,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) { > @@ -692,20 +695,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 > @@ -741,7 +747,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) > @@ -830,7 +837,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; > @@ -920,7 +927,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; > @@ -945,21 +952,22 @@ 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)) > + return (!related || check_l4_icmp6(key, data, size, l3, > + validate_checksum)) > && extract_l4_icmp6(key, data, size, related); > } else { > return false; > @@ -975,6 +983,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); > > @@ -1018,7 +1028,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 { > @@ -1026,7 +1039,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 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
