diff mbox

[ovs-dev] ofproto-dpif-ipfix: add support for per-flow TCP counters

Message ID 1494497607-195457-1-git-send-email-przemyslawx.szczerbik@intel.com
State Changes Requested
Headers show

Commit Message

Przemyslaw Szczerbik May 11, 2017, 10:13 a.m. UTC 
This patch implements support for per-flow TCP IPFIX counters. It's based on RFC
5102, section 5.10.

Signed-off-by: Przemyslaw Szczerbik <przemyslawx.szczerbik@intel.com>
---
 ofproto/ofproto-dpif-ipfix.c | 144 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 126 insertions(+), 18 deletions(-)

Comments

Przemyslaw Szczerbik May 19, 2017, 10:57 a.m. UTC | #1 
> -----Original Message-----
> From: Ben Pfaff [mailto:blp@ovn.org]
> Sent: Friday, May 19, 2017 4:39 AM
> To: Szczerbik, PrzemyslawX <przemyslawx.szczerbik@intel.com>
> Cc: dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH] ofproto-dpif-ipfix: add support for per-flow TCP
> counters
> 
> On Thu, May 11, 2017 at 11:13:27AM +0100, Przemyslaw Szczerbik wrote:
> > This patch implements support for per-flow TCP IPFIX counters. It's based on
> RFC
> > 5102, section 5.10.
> >
> > Signed-off-by: Przemyslaw Szczerbik <przemyslawx.szczerbik@intel.com>
> 
> Thanks for working on IPFIX!
> 
> I suggest folding in an update to NEWS to mention this new feature,

Sure thing, I can update NEWS file.
> e.g.:
> 
> diff --git a/NEWS b/NEWS
> index 7a2b185bbd84..ab2faec02e57 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -10,7 +10,9 @@ Post-v2.7.0
>         Log level can be changed in a usual OVS way using
>         'ovs-appctl vlog' commands for 'dpdk' module. Lower bound
>         still can be configured via extra arguments for DPDK EAL.
> -   - IPFIX now provides additional counters for totals since startup.
> +   - IPFIX now provides additional counters:
> +     * Total counters since metering process startup.
> +     * Per-flow TCP flag counters.
>     - New support for multiple VLANs (802.1ad or "QinQ"), including a new
>       "dot1q-tunnel" port VLAN mode.
>     - In ovn-vsctl and vtep-ctl, record UUIDs in commands may now be
> 
> However, I am worried about this feature.  I suspect that it does not
> report correct statistics.  It infers that, if a flow's tcp_flags
> contains a given flag, then that flag has been seen on every packet.
> But this is incorrect: in fact, it only means that the flag has been
> seen on at least one packet.

Flow represents exact content of the packet that is going to be sampled.
If packet, is it in fact a TCP packet, then tcp_flags member will contain appropriate values.
I'm assuming that all matched packets had exactly the same flags set, which really boils down to a single issue - IPFIX counters are based on probability.

Sampling probability is used to approximate number of matched packets, which is then used as a baseline for majority of other counters.
For instance, octet_delta_count counter implementation assumes that all matched packets have the same length.

I run a few tests on my patch and it worked quite well.

Is there a specific scenario that you would like me to test?
Do you have a suggestion how to improve implementation of TCP counters?

Regards,
Przemek



--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.
Ben Pfaff May 20, 2017, 6:29 p.m. UTC | #2 
On Fri, May 19, 2017 at 10:57:17AM +0000, Szczerbik, PrzemyslawX wrote:
> 
> 
> > -----Original Message-----
> > From: Ben Pfaff [mailto:blp@ovn.org]
> > Sent: Friday, May 19, 2017 4:39 AM
> > To: Szczerbik, PrzemyslawX <przemyslawx.szczerbik@intel.com>
> > Cc: dev@openvswitch.org
> > Subject: Re: [ovs-dev] [PATCH] ofproto-dpif-ipfix: add support for per-flow TCP
> > counters
> > 
> > On Thu, May 11, 2017 at 11:13:27AM +0100, Przemyslaw Szczerbik wrote:
> > > This patch implements support for per-flow TCP IPFIX counters. It's based on
> > RFC
> > > 5102, section 5.10.
> > >
> > > Signed-off-by: Przemyslaw Szczerbik <przemyslawx.szczerbik@intel.com>
> > 
> > Thanks for working on IPFIX!
> > 
> > I suggest folding in an update to NEWS to mention this new feature,
> 
> Sure thing, I can update NEWS file.
> > e.g.:
> > 
> > diff --git a/NEWS b/NEWS
> > index 7a2b185bbd84..ab2faec02e57 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -10,7 +10,9 @@ Post-v2.7.0
> >         Log level can be changed in a usual OVS way using
> >         'ovs-appctl vlog' commands for 'dpdk' module. Lower bound
> >         still can be configured via extra arguments for DPDK EAL.
> > -   - IPFIX now provides additional counters for totals since startup.
> > +   - IPFIX now provides additional counters:
> > +     * Total counters since metering process startup.
> > +     * Per-flow TCP flag counters.
> >     - New support for multiple VLANs (802.1ad or "QinQ"), including a new
> >       "dot1q-tunnel" port VLAN mode.
> >     - In ovn-vsctl and vtep-ctl, record UUIDs in commands may now be
> > 
> > However, I am worried about this feature.  I suspect that it does not
> > report correct statistics.  It infers that, if a flow's tcp_flags
> > contains a given flag, then that flag has been seen on every packet.
> > But this is incorrect: in fact, it only means that the flag has been
> > seen on at least one packet.
> 
> Flow represents exact content of the packet that is going to be sampled.
> If packet, is it in fact a TCP packet, then tcp_flags member will contain appropriate values.
> I'm assuming that all matched packets had exactly the same flags set, which really boils down to a single issue - IPFIX counters are based on probability.
> 
> Sampling probability is used to approximate number of matched packets, which is then used as a baseline for majority of other counters.
> For instance, octet_delta_count counter implementation assumes that all matched packets have the same length.
> 
> I run a few tests on my patch and it worked quite well.

OK, I understand the context now.  Thanks for the explanation.

I applied this to master.  Thank you!
diff mbox

Patch

diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index 3de81a9..23fc51b 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -89,6 +89,12 @@  struct dpif_ipfix_global_stats {
     uint64_t octet_total_count;
     uint64_t octet_total_sum_of_squares;
     uint64_t layer2_octet_total_count;
+    uint64_t tcp_ack_total_count;
+    uint64_t tcp_fin_total_count;
+    uint64_t tcp_psh_total_count;
+    uint64_t tcp_rst_total_count;
+    uint64_t tcp_syn_total_count;
+    uint64_t tcp_urg_total_count;
 };
 
 struct dpif_ipfix_port {
@@ -183,7 +189,9 @@  enum ipfix_proto_l3 {
 };
 enum ipfix_proto_l4 {
     IPFIX_PROTO_L4_UNKNOWN = 0,
-    IPFIX_PROTO_L4_TCP_UDP_SCTP,
+    IPFIX_PROTO_L4_TCP,
+    IPFIX_PROTO_L4_UDP,
+    IPFIX_PROTO_L4_SCTP,
     IPFIX_PROTO_L4_ICMP,
     NUM_IPFIX_PROTO_L4
 };
@@ -356,9 +364,9 @@  struct ipfix_data_record_aggregated_common {
     ovs_be32 flow_start_delta_microseconds; /* FLOW_START_DELTA_MICROSECONDS */
     ovs_be32 flow_end_delta_microseconds; /* FLOW_END_DELTA_MICROSECONDS */
     ovs_be64 packet_delta_count;  /* PACKET_DELTA_COUNT */
-    ovs_be64 packet_total_count;  /* PACKET_DELTA_COUNT */
+    ovs_be64 packet_total_count;  /* PACKET_TOTAL_COUNT */
     ovs_be64 layer2_octet_delta_count;  /* LAYER2_OCTET_DELTA_COUNT */
-    ovs_be64 layer2_octet_total_count;  /* LAYER2_OCTET_DELTA_COUNT */
+    ovs_be64 layer2_octet_total_count;  /* LAYER2_OCTET_TOTAL_COUNT */
     uint8_t flow_end_reason;  /* FLOW_END_REASON */
 });
 BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_aggregated_common) == 41);
@@ -375,6 +383,18 @@  struct ipfix_data_record_aggregated_ip {
 });
 BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_aggregated_ip) == 48);
 
+/* Part of data record for TCP aggregated elements. */
+OVS_PACKED(
+struct ipfix_data_record_aggregated_tcp {
+    ovs_be64 tcp_ack_total_count;  /* TCP_ACK_TOTAL_COUNT */
+    ovs_be64 tcp_fin_total_count;  /* TCP_FIN_TOTAL_COUNT */
+    ovs_be64 tcp_psh_total_count;  /* TCP_PSH_TOTAL_COUNT */
+    ovs_be64 tcp_rst_total_count;  /* TCP_RST_TOTAL_COUNT */
+    ovs_be64 tcp_syn_total_count;  /* TCP_SYN_TOTAL_COUNT */
+    ovs_be64 tcp_urg_total_count;  /* TCP_URG_TOTAL_COUNT */
+});
+BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_aggregated_tcp) == 48);
+
 /*
  * Refer to RFC 7011, the length of Variable length element is 0~65535:
  * In most case, it should be less than 255 octets:
@@ -424,7 +444,8 @@  BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_aggregated_ip) == 48);
 #define MAX_DATA_RECORD_LEN                                 \
     (MAX_FLOW_KEY_LEN                                       \
      + sizeof(struct ipfix_data_record_aggregated_common)   \
-     + sizeof(struct ipfix_data_record_aggregated_ip))
+     + sizeof(struct ipfix_data_record_aggregated_ip)       \
+     + sizeof(struct ipfix_data_record_aggregated_tcp))
 
 /* Max length of a data set.  To simplify the implementation, each
  * data record is sent in a separate data set, so each data set
@@ -465,6 +486,13 @@  struct ipfix_flow_cache_entry {
     uint64_t octet_total_sum_of_squares;  /* 0 if not IP. */
     uint16_t minimum_ip_total_length;  /* 0 if not IP. */
     uint16_t maximum_ip_total_length;  /* 0 if not IP. */
+    uint64_t tcp_packet_delta_count;
+    uint64_t tcp_ack_total_count;
+    uint64_t tcp_fin_total_count;
+    uint64_t tcp_psh_total_count;
+    uint64_t tcp_rst_total_count;
+    uint64_t tcp_syn_total_count;
+    uint64_t tcp_urg_total_count;
 };
 
 static void dpif_ipfix_cache_expire(struct dpif_ipfix_exporter *, bool,
@@ -1180,7 +1208,9 @@  ipfix_define_template_fields(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
         if (l3 == IPFIX_PROTO_L3_IPV4) {
             DEF(SOURCE_IPV4_ADDRESS);
             DEF(DESTINATION_IPV4_ADDRESS);
-            if (l4 == IPFIX_PROTO_L4_TCP_UDP_SCTP) {
+            if (l4 == IPFIX_PROTO_L4_TCP
+                || l4 == IPFIX_PROTO_L4_UDP
+                || l4 == IPFIX_PROTO_L4_SCTP) {
                 DEF(SOURCE_TRANSPORT_PORT);
                 DEF(DESTINATION_TRANSPORT_PORT);
             } else if (l4 == IPFIX_PROTO_L4_ICMP) {
@@ -1191,7 +1221,9 @@  ipfix_define_template_fields(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
             DEF(SOURCE_IPV6_ADDRESS);
             DEF(DESTINATION_IPV6_ADDRESS);
             DEF(FLOW_LABEL_IPV6);
-            if (l4 == IPFIX_PROTO_L4_TCP_UDP_SCTP) {
+            if (l4 == IPFIX_PROTO_L4_TCP
+                || l4 == IPFIX_PROTO_L4_UDP
+                || l4 == IPFIX_PROTO_L4_SCTP) {
                 DEF(SOURCE_TRANSPORT_PORT);
                 DEF(DESTINATION_TRANSPORT_PORT);
             } else if (l4 == IPFIX_PROTO_L4_ICMP) {
@@ -1234,6 +1266,15 @@  ipfix_define_template_fields(enum ipfix_proto_l2 l2, enum ipfix_proto_l3 l3,
         DEF(MINIMUM_IP_TOTAL_LENGTH);
         DEF(MAXIMUM_IP_TOTAL_LENGTH);
     }
+
+    if (l4 == IPFIX_PROTO_L4_TCP) {
+        DEF(TCP_ACK_TOTAL_COUNT);
+        DEF(TCP_FIN_TOTAL_COUNT);
+        DEF(TCP_PSH_TOTAL_COUNT);
+        DEF(TCP_RST_TOTAL_COUNT);
+        DEF(TCP_SYN_TOTAL_COUNT);
+        DEF(TCP_URG_TOTAL_COUNT);
+    }
 #undef DEF
 
     return count;
@@ -1450,6 +1491,14 @@  ipfix_cache_aggregate_entries(struct ipfix_flow_cache_entry *from_entry,
     if (*to_max_len < *from_max_len) {
         *to_max_len = *from_max_len;
     }
+
+    to_entry->tcp_packet_delta_count += from_entry->tcp_packet_delta_count;
+    to_entry->tcp_ack_total_count = from_entry->tcp_ack_total_count;
+    to_entry->tcp_fin_total_count = from_entry->tcp_fin_total_count;
+    to_entry->tcp_psh_total_count = from_entry->tcp_psh_total_count;
+    to_entry->tcp_rst_total_count = from_entry->tcp_rst_total_count;
+    to_entry->tcp_syn_total_count = from_entry->tcp_syn_total_count;
+    to_entry->tcp_urg_total_count = from_entry->tcp_urg_total_count;
 }
 
 /* Get statistics */
@@ -1597,7 +1646,7 @@  ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
                        enum nx_action_sample_direction direction,
                        const struct dpif_ipfix_port *tunnel_port,
                        const struct flow_tnl *tunnel_key,
-                       struct dpif_ipfix_global_stats * stats)
+                       struct dpif_ipfix_global_stats *stats)
 {
     struct ipfix_flow_key *flow_key;
     struct dp_packet msg;
@@ -1620,16 +1669,19 @@  ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
     switch(ntohs(flow->dl_type)) {
     case ETH_TYPE_IP:
         l3 = IPFIX_PROTO_L3_IPV4;
+        sampled_pkt_type = IPFIX_SAMPLED_PKT_IPV4_OK;
         switch(flow->nw_proto) {
         case IPPROTO_TCP:
+            l4 = IPFIX_PROTO_L4_TCP;
+            break;
         case IPPROTO_UDP:
+            l4 = IPFIX_PROTO_L4_UDP;
+            break;
         case IPPROTO_SCTP:
-            l4 = IPFIX_PROTO_L4_TCP_UDP_SCTP;
-            sampled_pkt_type = IPFIX_SAMPLED_PKT_IPV4_OK;
+            l4 = IPFIX_PROTO_L4_SCTP;
             break;
         case IPPROTO_ICMP:
             l4 = IPFIX_PROTO_L4_ICMP;
-            sampled_pkt_type = IPFIX_SAMPLED_PKT_IPV4_OK;
             break;
         default:
             l4 = IPFIX_PROTO_L4_UNKNOWN;
@@ -1638,16 +1690,19 @@  ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
         break;
     case ETH_TYPE_IPV6:
         l3 = IPFIX_PROTO_L3_IPV6;
+        sampled_pkt_type = IPFIX_SAMPLED_PKT_IPV6_OK;
         switch(flow->nw_proto) {
         case IPPROTO_TCP:
+            l4 = IPFIX_PROTO_L4_TCP;
+            break;
         case IPPROTO_UDP:
+            l4 = IPFIX_PROTO_L4_UDP;
+            break;
         case IPPROTO_SCTP:
-            l4 = IPFIX_PROTO_L4_TCP_UDP_SCTP;
-            sampled_pkt_type = IPFIX_SAMPLED_PKT_IPV6_OK;
+            l4 = IPFIX_PROTO_L4_SCTP;
             break;
         case IPPROTO_ICMPV6:
             l4 = IPFIX_PROTO_L4_ICMP;
-            sampled_pkt_type = IPFIX_SAMPLED_PKT_IPV6_OK;
             break;
         default:
             l4 = IPFIX_PROTO_L4_UNKNOWN;
@@ -1731,7 +1786,9 @@  ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
         }
     }
 
-    if (l4 == IPFIX_PROTO_L4_TCP_UDP_SCTP) {
+    if (l4 == IPFIX_PROTO_L4_TCP
+        || l4 == IPFIX_PROTO_L4_UDP
+        || l4 == IPFIX_PROTO_L4_SCTP) {
         struct ipfix_data_record_flow_key_transport *data_transport;
 
         data_transport = dp_packet_put_zeros(&msg, sizeof *data_transport);
@@ -1810,17 +1867,49 @@  ipfix_cache_entry_init(struct ipfix_flow_cache_entry *entry,
 
         stats->octet_total_count += octet_delta_count;
         stats->octet_total_sum_of_squares += entry->octet_delta_sum_of_squares;
-        entry->octet_total_count = stats->octet_total_count;
-        entry->octet_total_sum_of_squares = stats->octet_total_sum_of_squares;
 
     } else {
         entry->octet_delta_sum_of_squares = 0;
-        entry->octet_total_sum_of_squares = stats->octet_total_sum_of_squares;
-        entry->octet_total_count = stats->octet_total_count;
         entry->minimum_ip_total_length = 0;
         entry->maximum_ip_total_length = 0;
     }
 
+    entry->octet_total_sum_of_squares = stats->octet_total_sum_of_squares;
+    entry->octet_total_count = stats->octet_total_count;
+
+    if (l4 == IPFIX_PROTO_L4_TCP) {
+        uint16_t tcp_flags = ntohs(flow->tcp_flags);
+        entry->tcp_packet_delta_count = packet_delta_count;
+
+        if (tcp_flags & TCP_ACK) {
+            stats->tcp_ack_total_count += packet_delta_count;
+        }
+        if (tcp_flags & TCP_FIN) {
+            stats->tcp_fin_total_count += packet_delta_count;
+        }
+        if (tcp_flags & TCP_PSH) {
+            stats->tcp_psh_total_count += packet_delta_count;
+        }
+        if (tcp_flags & TCP_RST) {
+            stats->tcp_rst_total_count += packet_delta_count;
+        }
+        if (tcp_flags & TCP_SYN) {
+            stats->tcp_syn_total_count += packet_delta_count;
+        }
+        if (tcp_flags & TCP_URG) {
+            stats->tcp_urg_total_count += packet_delta_count;
+        }
+    } else {
+        entry->tcp_packet_delta_count = 0;
+    }
+
+    entry->tcp_ack_total_count = stats->tcp_ack_total_count;
+    entry->tcp_fin_total_count = stats->tcp_fin_total_count;
+    entry->tcp_psh_total_count = stats->tcp_psh_total_count;
+    entry->tcp_rst_total_count = stats->tcp_rst_total_count;
+    entry->tcp_syn_total_count = stats->tcp_syn_total_count;
+    entry->tcp_urg_total_count = stats->tcp_urg_total_count;
+
     return sampled_pkt_type;
 }
 
@@ -1906,6 +1995,25 @@  ipfix_put_data_set(uint32_t export_time_sec,
             entry->maximum_ip_total_length);
     }
 
+    if (entry->tcp_packet_delta_count) {
+        struct ipfix_data_record_aggregated_tcp *data_aggregated_tcp;
+
+        data_aggregated_tcp = dp_packet_put_zeros(
+            msg, sizeof *data_aggregated_tcp);
+        data_aggregated_tcp->tcp_ack_total_count = htonll(
+            entry->tcp_ack_total_count);
+        data_aggregated_tcp->tcp_fin_total_count = htonll(
+            entry->tcp_fin_total_count);
+        data_aggregated_tcp->tcp_psh_total_count = htonll(
+            entry->tcp_psh_total_count);
+        data_aggregated_tcp->tcp_rst_total_count = htonll(
+            entry->tcp_rst_total_count);
+        data_aggregated_tcp->tcp_syn_total_count = htonll(
+            entry->tcp_syn_total_count);
+        data_aggregated_tcp->tcp_urg_total_count = htonll(
+            entry->tcp_urg_total_count);
+    }
+
     set_hdr = (struct ipfix_set_header*)((uint8_t*)dp_packet_data(msg) + set_hdr_offset);
     set_hdr->length = htons(dp_packet_size(msg) - set_hdr_offset);
 }