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

[Ben Pfaff]

On Fri, May 19, 2017 at 10:57:17AM +, Szczerbik, PrzemyslawX wrote:
> 
> 
> > -Original Message-
> > From: Ben Pfaff [mailto:b...@ovn.org]
> > Sent: Friday, May 19, 2017 4:39 AM
> > To: Szczerbik, PrzemyslawX <przemyslawx.szczer...@intel.com>
> > Cc: d...@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.szczer...@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!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Ben Pfaff]

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 

Thanks for working on IPFIX!

I suggest folding in an update to NEWS to mention this new feature,
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.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Przemyslaw Szczerbik]

This patch implements support for per-flow TCP IPFIX counters. It's based on RFC
5102, section 5.10.

Signed-off-by: Przemyslaw Szczerbik 
---
 ofproto/ofproto-dpif-ipfix.c | 144 +--
 1 file changed, 126 insertions(+), 18 deletions(-)

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