Re: [ovs-dev] [patch v1] conntrack: Support global drop statistics.
On Tue, Jul 10, 2018 at 10:38 AM, Ben Pfaff wrote: > On Tue, Jun 26, 2018 at 04:16:42PM -0700, Darrell Ball wrote: > > Signed-off-by: Darrell Ball > > I wonder whether coverage counters would be a better alternative? They > use thread-local data so incrementing them is cheaper than atomic > increments, at least if they are accessed from multiple CPUs. > Thanks for the pointer on the coverage counters; since the counters are for sanity failures, the performance does not matter much. That being said, I have recently made some changes to the Fragmentation series where some same sanity failures also need counting, since some now bypass conntrack. It is probably best to do both together, after the Fragmentation series. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v1] conntrack: Support global drop statistics.
On Tue, Jun 26, 2018 at 04:16:42PM -0700, Darrell Ball wrote: > Signed-off-by: Darrell Ball I wonder whether coverage counters would be a better alternative? They use thread-local data so incrementing them is cheaper than atomic increments, at least if they are accessed from multiple CPUs. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v1] conntrack: Support global drop statistics.
I probably will change the name of these stats to 'invalid' and add a
comment to the effect
that the associated packets are usually dropped. I guess it would also be
good to specify that
only packet sanity invalids are tracked, which should be rare.
Thanks
Darrell
On Tue, Jun 26, 2018 at 4:16 PM, Darrell Ball wrote:
> Signed-off-by: Darrell Ball
> ---
> NEWS| 2 +
> lib/conntrack.c | 114 ++
> +++---
> lib/conntrack.h | 13 ++
> lib/ct-dpif.c | 24 +++
> lib/ct-dpif.h | 13 ++
> lib/dpctl.c | 62
> lib/dpctl.man | 2 +-
> lib/dpif-netdev.c | 26
> lib/dpif-netlink.c | 1 +
> lib/dpif-provider.h | 5 +++
> 10 files changed, 256 insertions(+), 6 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index cd15a33..04e6a53 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -40,6 +40,8 @@ Post-v2.9.0
> ovs-appctl dpif-netdev/pmd-perf-show
> * Supervision of PMD performance metrics and logging of suspicious
> iterations
> + * ovs-appctl dpctl/ct-stats-show now prints conntrack global drop
> + statistics.
> - ERSPAN:
> * Implemented ERSPAN protocol (draft-foschiano-erspan-00.txt) for
> both kernel datapath and userspace datapath.
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 825f1ad..ecc39de 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -228,6 +228,25 @@ long long ct_timeout_val[] = {
> * are accepted; this is for CT_CONN_TYPE_DEFAULT connections. */
> #define DEFAULT_N_CONN_LIMIT 300
>
> +/* IPv4 sanity drops. */
> +static atomic_count min_hdr_err_v4;
> +static atomic_count size_err_v4;
> +static atomic_count cksum_err_v4;
> +
> +/* IPv6 sanity drops. */
> +static atomic_count min_hdr_err_v6;
> +static atomic_count hdr_parse_err_v6;
> +
> +/* L4 sanity drops. */
> +static atomic_count hdr_size_err_tcp;
> +static atomic_count size_err_tcp;
> +static atomic_count cksum_err_tcp;
> +static atomic_count hdr_size_err_udp;
> +static atomic_count size_err_udp;
> +static atomic_count cksum_err_udp;
> +static atomic_count cksum_err_icmp;
> +static atomic_count cksum_err_icmp6;
> +
> /* Does a member by member comparison of two conn_keys; this
> * function must be kept in sync with struct conn_key; returns 0
> * if the keys are equal or 1 if the keys are not equal. */
> @@ -339,6 +358,20 @@ conntrack_init(struct conntrack *ct)
> ct->hash_basis = random_uint32();
> atomic_count_init(>n_conn, 0);
> atomic_init(>n_conn_limit, DEFAULT_N_CONN_LIMIT);
> +atomic_count_init(_hdr_err_v4, 0);
> +atomic_count_init(_err_v4, 0);
> +atomic_count_init(_err_v4, 0);
> +atomic_count_init(_hdr_err_v6, 0);
> +atomic_count_init(_parse_err_v6, 0);
> +atomic_count_init(_size_err_tcp, 0);
> +atomic_count_init(_err_tcp, 0);
> +atomic_count_init(_err_tcp, 0);
> +atomic_count_init(_size_err_udp, 0);
> +atomic_count_init(_err_udp, 0);
> +atomic_count_init(_err_udp, 0);
> +atomic_count_init(_err_icmp, 0);
> +atomic_count_init(_err_icmp6, 0);
> +
> latch_init(>clean_thread_exit);
> ct->clean_thread = ovs_thread_create("ct_clean", clean_thread_main,
> ct);
> }
> @@ -1502,6 +1535,7 @@ extract_l3_ipv4(const void *data, size_t size, bool
> validate_checksum,
> struct conn_key *key, const char **new_data)
> {
> if (OVS_UNLIKELY(size < IP_HEADER_LEN)) {
> +atomic_count_inc(_hdr_err_v4);
> return false;
> }
>
> @@ -1509,14 +1543,17 @@ extract_l3_ipv4(const void *data, size_t size,
> bool validate_checksum,
> size_t ip_len = IP_IHL(ip->ip_ihl_ver) * 4;
>
> if (OVS_UNLIKELY(ip_len < IP_HEADER_LEN)) {
> +atomic_count_inc(_hdr_err_v4);
> return false;
> }
>
> if (OVS_UNLIKELY(size < ip_len)) {
> +atomic_count_inc(_err_v4);
> return false;
> }
>
> if (OVS_UNLIKELY(validate_checksum && csum(data, ip_len) != 0)) {
> +atomic_count_inc(_err_v4);
> return false;
> }
>
> @@ -1542,6 +1579,7 @@ extract_l3_ipv6(const void *data, size_t size,
> struct conn_key *key,
> const struct ovs_16aligned_ip6_hdr *ip6 = data;
>
> if (OVS_UNLIKELY(size < sizeof *ip6)) {
> +atomic_count_inc(_hdr_err_v6);
> return false;
> }
>
> @@ -1552,6 +1590,7 @@ extract_l3_ipv6(const void *data, size_t size,
> struct conn_key *key,
>
> if (OVS_UNLIKELY(!parse_ipv6_ext_hdrs(, , _proto,
>_frag))) {
> +atomic_count_inc(_parse_err_v6);
> return false;
> }
>
> @@ -1595,15 +1634,24 @@ check_l4_tcp(const struct conn_key *key, const
> void *data, size_t size,
> {
> const struct tcp_header *tcp = data;
> if (size < sizeof *tcp) {
> +atomic_count_inc(_size_err_tcp);
> return false;
> }
>
> size_t tcp_len =
[ovs-dev] [patch v1] conntrack: Support global drop statistics.
Signed-off-by: Darrell Ball
---
NEWS| 2 +
lib/conntrack.c | 114 +---
lib/conntrack.h | 13 ++
lib/ct-dpif.c | 24 +++
lib/ct-dpif.h | 13 ++
lib/dpctl.c | 62
lib/dpctl.man | 2 +-
lib/dpif-netdev.c | 26
lib/dpif-netlink.c | 1 +
lib/dpif-provider.h | 5 +++
10 files changed, 256 insertions(+), 6 deletions(-)
diff --git a/NEWS b/NEWS
index cd15a33..04e6a53 100644
--- a/NEWS
+++ b/NEWS
@@ -40,6 +40,8 @@ Post-v2.9.0
ovs-appctl dpif-netdev/pmd-perf-show
* Supervision of PMD performance metrics and logging of suspicious
iterations
+ * ovs-appctl dpctl/ct-stats-show now prints conntrack global drop
+ statistics.
- ERSPAN:
* Implemented ERSPAN protocol (draft-foschiano-erspan-00.txt) for
both kernel datapath and userspace datapath.
diff --git a/lib/conntrack.c b/lib/conntrack.c
index 825f1ad..ecc39de 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -228,6 +228,25 @@ long long ct_timeout_val[] = {
* are accepted; this is for CT_CONN_TYPE_DEFAULT connections. */
#define DEFAULT_N_CONN_LIMIT 300
+/* IPv4 sanity drops. */
+static atomic_count min_hdr_err_v4;
+static atomic_count size_err_v4;
+static atomic_count cksum_err_v4;
+
+/* IPv6 sanity drops. */
+static atomic_count min_hdr_err_v6;
+static atomic_count hdr_parse_err_v6;
+
+/* L4 sanity drops. */
+static atomic_count hdr_size_err_tcp;
+static atomic_count size_err_tcp;
+static atomic_count cksum_err_tcp;
+static atomic_count hdr_size_err_udp;
+static atomic_count size_err_udp;
+static atomic_count cksum_err_udp;
+static atomic_count cksum_err_icmp;
+static atomic_count cksum_err_icmp6;
+
/* Does a member by member comparison of two conn_keys; this
* function must be kept in sync with struct conn_key; returns 0
* if the keys are equal or 1 if the keys are not equal. */
@@ -339,6 +358,20 @@ conntrack_init(struct conntrack *ct)
ct->hash_basis = random_uint32();
atomic_count_init(>n_conn, 0);
atomic_init(>n_conn_limit, DEFAULT_N_CONN_LIMIT);
+atomic_count_init(_hdr_err_v4, 0);
+atomic_count_init(_err_v4, 0);
+atomic_count_init(_err_v4, 0);
+atomic_count_init(_hdr_err_v6, 0);
+atomic_count_init(_parse_err_v6, 0);
+atomic_count_init(_size_err_tcp, 0);
+atomic_count_init(_err_tcp, 0);
+atomic_count_init(_err_tcp, 0);
+atomic_count_init(_size_err_udp, 0);
+atomic_count_init(_err_udp, 0);
+atomic_count_init(_err_udp, 0);
+atomic_count_init(_err_icmp, 0);
+atomic_count_init(_err_icmp6, 0);
+
latch_init(>clean_thread_exit);
ct->clean_thread = ovs_thread_create("ct_clean", clean_thread_main, ct);
}
@@ -1502,6 +1535,7 @@ extract_l3_ipv4(const void *data, size_t size, bool
validate_checksum,
struct conn_key *key, const char **new_data)
{
if (OVS_UNLIKELY(size < IP_HEADER_LEN)) {
+atomic_count_inc(_hdr_err_v4);
return false;
}
@@ -1509,14 +1543,17 @@ extract_l3_ipv4(const void *data, size_t size, bool
validate_checksum,
size_t ip_len = IP_IHL(ip->ip_ihl_ver) * 4;
if (OVS_UNLIKELY(ip_len < IP_HEADER_LEN)) {
+atomic_count_inc(_hdr_err_v4);
return false;
}
if (OVS_UNLIKELY(size < ip_len)) {
+atomic_count_inc(_err_v4);
return false;
}
if (OVS_UNLIKELY(validate_checksum && csum(data, ip_len) != 0)) {
+atomic_count_inc(_err_v4);
return false;
}
@@ -1542,6 +1579,7 @@ extract_l3_ipv6(const void *data, size_t size, struct
conn_key *key,
const struct ovs_16aligned_ip6_hdr *ip6 = data;
if (OVS_UNLIKELY(size < sizeof *ip6)) {
+atomic_count_inc(_hdr_err_v6);
return false;
}
@@ -1552,6 +1590,7 @@ extract_l3_ipv6(const void *data, size_t size, struct
conn_key *key,
if (OVS_UNLIKELY(!parse_ipv6_ext_hdrs(, , _proto,
_frag))) {
+atomic_count_inc(_parse_err_v6);
return false;
}
@@ -1595,15 +1634,24 @@ check_l4_tcp(const struct conn_key *key, const void
*data, size_t size,
{
const struct tcp_header *tcp = data;
if (size < sizeof *tcp) {
+atomic_count_inc(_size_err_tcp);
return false;
}
size_t tcp_len = TCP_OFFSET(tcp->tcp_ctl) * 4;
if (OVS_UNLIKELY(tcp_len < TCP_HEADER_LEN || tcp_len > size)) {
+atomic_count_inc(_err_tcp);
return false;
}
-return validate_checksum ? checksum_valid(key, data, size, l3) : true;
+if (!validate_checksum) {
+return true;
+} else if (checksum_valid(key, data, size, l3)) {
+return true;
+} else {
+atomic_count_inc(_err_tcp);
+return false;
+}
}
static inline bool
@@ -1612,30 +1660,53 @@ check_l4_udp(const struct conn_key *key, const void
*data, size_t size,
{
const
