| Message ID | 1495746664-48559-1-git-send-email-sugesh.chandran@intel.com |
|---|---|
| State | Superseded |
| Delegated to: | Darrell Ball |
| Headers | show |
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
Ping!. Any other comments on this patch?? Regards _Sugesh > -----Original Message----- > From: Fischetti, Antonio > Sent: Friday, May 26, 2017 11:05 AM > To: Chandran, Sugesh <sugesh.chandran@intel.com>; ovs- > dev@openvswitch.org > Subject: RE: [ovs-dev] [PATCH] conntrack : Use Rx checksum offload feature > on DPDK ports for conntrack. > > 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 <antonio.fischetti@intel.com> > > > > -----Original Message----- > > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > > bounces@openvswitch.org] On Behalf Of Sugesh Chandran > > Sent: Thursday, May 25, 2017 10:11 PM > > To: ovs-dev@openvswitch.org > > 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 <sugesh.chandran@intel.com> > > --- > > 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 > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
I did a first pass, but I will look thru. it more carefully and test it.
The benefit is smaller than I had hoped, even in a simple case, but the code delta is also simple.
Thanks Darrell
On 6/6/17, 3:45 AM, "ovs-dev-bounces@openvswitch.org on behalf of Chandran, Sugesh" <ovs-dev-bounces@openvswitch.org on behalf of sugesh.chandran@intel.com> wrote:
Ping!.
Any other comments on this patch??
Regards
_Sugesh
> -----Original Message-----
> From: Fischetti, Antonio
> Sent: Friday, May 26, 2017 11:05 AM
> To: Chandran, Sugesh <sugesh.chandran@intel.com>; ovs-
> dev@openvswitch.org
> Subject: RE: [ovs-dev] [PATCH] conntrack : Use Rx checksum offload feature
> on DPDK ports for conntrack.
>
> 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 <antonio.fischetti@intel.com>
>
>
> > -----Original Message-----
> > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
> > bounces@openvswitch.org] On Behalf Of Sugesh Chandran
> > Sent: Thursday, May 25, 2017 10:11 PM
> > To: ovs-dev@openvswitch.org
> > 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 <sugesh.chandran@intel.com>
> > ---
> > 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
> > dev@openvswitch.org
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=kVLpddWgvkgbLp3fpJuodSguI0Q073H7dUQnYuL7eY0&s=9JGLm-HbUQv0mvfEWEEiiIEBHsRBn-i2IVfCH8mzbhI&e=
_______________________________________________
dev mailing list
dev@openvswitch.org
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=kVLpddWgvkgbLp3fpJuodSguI0Q073H7dUQnYuL7eY0&s=9JGLm-HbUQv0mvfEWEEiiIEBHsRBn-i2IVfCH8mzbhI&e=
Do you still intend to look closer? On Tue, Jun 06, 2017 at 04:40:14PM +0000, Darrell Ball wrote: > I did a first pass, but I will look thru. it more carefully and test it. > The benefit is smaller than I had hoped, even in a simple case, but the code delta is also simple. > > Thanks Darrell > > On 6/6/17, 3:45 AM, "ovs-dev-bounces@openvswitch.org on behalf of Chandran, Sugesh" <ovs-dev-bounces@openvswitch.org on behalf of sugesh.chandran@intel.com> wrote: > > Ping!. > Any other comments on this patch?? > > > Regards > _Sugesh > > > > -----Original Message----- > > From: Fischetti, Antonio > > Sent: Friday, May 26, 2017 11:05 AM > > To: Chandran, Sugesh <sugesh.chandran@intel.com>; ovs- > > dev@openvswitch.org > > Subject: RE: [ovs-dev] [PATCH] conntrack : Use Rx checksum offload feature > > on DPDK ports for conntrack. > > > > 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 <antonio.fischetti@intel.com> > > > > > > > -----Original Message----- > > > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > > > bounces@openvswitch.org] On Behalf Of Sugesh Chandran > > > Sent: Thursday, May 25, 2017 10:11 PM > > > To: ovs-dev@openvswitch.org > > > 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 <sugesh.chandran@intel.com> > > > --- > > > 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 > > > dev@openvswitch.org > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=kVLpddWgvkgbLp3fpJuodSguI0Q073H7dUQnYuL7eY0&s=9JGLm-HbUQv0mvfEWEEiiIEBHsRBn-i2IVfCH8mzbhI&e= > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=kVLpddWgvkgbLp3fpJuodSguI0Q073H7dUQnYuL7eY0&s=9JGLm-HbUQv0mvfEWEEiiIEBHsRBn-i2IVfCH8mzbhI&e= > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
This has been updated and I will be reviewing the changes
Thanks Darrell
On 7/6/17, 4:59 PM, "Ben Pfaff" <blp@ovn.org> wrote:
Do you still intend to look closer?
On Tue, Jun 06, 2017 at 04:40:14PM +0000, Darrell Ball wrote:
> I did a first pass, but I will look thru. it more carefully and test it.
> The benefit is smaller than I had hoped, even in a simple case, but the code delta is also simple.
>
> Thanks Darrell
>
> On 6/6/17, 3:45 AM, "ovs-dev-bounces@openvswitch.org on behalf of Chandran, Sugesh" <ovs-dev-bounces@openvswitch.org on behalf of sugesh.chandran@intel.com> wrote:
>
> Ping!.
> Any other comments on this patch??
>
>
> Regards
> _Sugesh
>
>
> > -----Original Message-----
> > From: Fischetti, Antonio
> > Sent: Friday, May 26, 2017 11:05 AM
> > To: Chandran, Sugesh <sugesh.chandran@intel.com>; ovs-
> > dev@openvswitch.org
> > Subject: RE: [ovs-dev] [PATCH] conntrack : Use Rx checksum offload feature
> > on DPDK ports for conntrack.
> >
> > 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 <antonio.fischetti@intel.com>
> >
> >
> > > -----Original Message-----
> > > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
> > > bounces@openvswitch.org] On Behalf Of Sugesh Chandran
> > > Sent: Thursday, May 25, 2017 10:11 PM
> > > To: ovs-dev@openvswitch.org
> > > 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 <sugesh.chandran@intel.com>
> > > ---
> > > 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
> > > dev@openvswitch.org
> > > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=kVLpddWgvkgbLp3fpJuodSguI0Q073H7dUQnYuL7eY0&s=9JGLm-HbUQv0mvfEWEEiiIEBHsRBn-i2IVfCH8mzbhI&e=
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=kVLpddWgvkgbLp3fpJuodSguI0Q073H7dUQnYuL7eY0&s=9JGLm-HbUQv0mvfEWEEiiIEBHsRBn-i2IVfCH8mzbhI&e=
>
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwIBAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=PttJsCkFn16WMCzEaJn4v6l5zONxHUnPbcfG4FLR94E&s=8li16gpsqoZh5uhKmVEBbvkY94ZRG-bWn5bGV-Gfq0c&e=
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; }
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> --- lib/conntrack.c | 58 ++++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 37 insertions(+), 21 deletions(-)