On Tue, Jan 24, 2023 at 08:21:28PM +0100, Adrián Moreno wrote:
> From: Adrian Moreno <amore...@redhat.com>
>
> IPFIX templates have to be sent for each Observation Domain ID.
> Currently, a timer is kept at each dpif_ipfix_exporter to send them.
> This works fine for per-bridge sampling where there is only one
> Observation Domain ID per exporter. However, this is does not work for
> per-flow sampling where more than one Observation Domain IDs can be
> specified by the controller. In this case, ovs-vswitchd will only send
> template information for one (arbitrary) DomainID.
>
> Fix per-flow sampling by using an hmap to keep a timer for each
> Observation Domain ID.
>
> Signed-off-by: Adrian Moreno <amore...@redhat.com>
>
> ---
> - v2:
> - Fixed a potential race condition in unit test.
> - v1:
> - Added unit test.
> - Drop domain_id from "struct dpif_ipfix_domain" since the hashmap
> already uses it as index.
> - Added OVS_REQUES(mutex) to dpif_ipfix_exporter_find_domain.
> ---
> ofproto/ofproto-dpif-ipfix.c | 127 ++++++++++++++++++++++++++++-------
> tests/system-traffic.at | 52 ++++++++++++++
> 2 files changed, 156 insertions(+), 23 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> index 742eed399..fa71fda0f 100644
> --- a/ofproto/ofproto-dpif-ipfix.c
> +++ b/ofproto/ofproto-dpif-ipfix.c
> @@ -124,11 +124,18 @@ struct dpif_ipfix_port {
> uint32_t ifindex;
> };
>
> +struct dpif_ipfix_domain {
> + struct hmap_node hmap_node; /* In struct dpif_ipfix_exporter's domains.
> */
> + time_t last_template_set_time;
> +};
> +
> struct dpif_ipfix_exporter {
> uint32_t exporter_id; /* Exporting Process identifier */
> struct collectors *collectors;
> uint32_t seq_number;
> - time_t last_template_set_time;
> + struct hmap domains; /* Contains struct dpif_ipfix_domain indexed by
> + observation domain id. */
> + time_t last_stats_sent_time;
> struct hmap cache_flow_key_map; /* ipfix_flow_cache_entry. */
> struct ovs_list cache_flow_start_timestamp_list; /*
> ipfix_flow_cache_entry. */
> uint32_t cache_active_timeout; /* In seconds. */
I'm not sure if it is important, but this change bumps the
cache_flow_key_map from the 1st to 2nd cacheline (on x86_64).
This can be avoided by moving seq_number above collectors,
as there is a 32-bit hole there.
Possibly there is further scope for making this structure more
cache-fiendly. But again, I'm not sure if it is important.
Before:
$ pahole ofproto/libofproto_la-ofproto-dpif-ipfix.o
...
struct dpif_ipfix_exporter {
uint32_t exporter_id; /* 0 4 */
/* XXX 4 bytes hole, try to pack */
struct collectors * collectors; /* 8 8 */
uint32_t seq_number; /* 16 4 */
/* XXX 4 bytes hole, try to pack */
time_t last_template_set_time; /* 24 8 */
struct hmap cache_flow_key_map; /* 32 32 */
/* --- cacheline 1 boundary (64 bytes) --- */
struct ovs_list cache_flow_start_timestamp_list; /* 64
16 */
uint32_t cache_active_timeout; /* 80 4 */
uint32_t cache_max_flows; /* 84 4 */
char * virtual_obs_id; /* 88 8 */
uint8_t virtual_obs_len; /* 96 1 */
/* XXX 7 bytes hole, try to pack */
ofproto_ipfix_stats ofproto_stats; /* 104 88 */
/* --- cacheline 3 boundary (192 bytes) --- */
struct dpif_ipfix_global_stats ipfix_global_stats; /* 192 152 */
/* size: 344, cachelines: 6, members: 12 */
/* sum members: 329, holes: 3, sum holes: 15 */
/* last cacheline: 24 bytes */
}
...
After:
$ pahole ofproto/libofproto_la-ofproto-dpif-ipfix.o
...
struct dpif_ipfix_exporter {
uint32_t exporter_id; /* 0 4 */
/* XXX 4 bytes hole, try to pack */
struct collectors * collectors; /* 8 8 */
uint32_t seq_number; /* 16 4 */
/* XXX 4 bytes hole, try to pack */
struct hmap domains; /* 24 32 */
time_t last_stats_sent_time; /* 56 8 */
/* --- cacheline 1 boundary (64 bytes) --- */
struct hmap cache_flow_key_map; /* 64 32 */
struct ovs_list cache_flow_start_timestamp_list; /* 96
16 */
uint32_t cache_active_timeout; /* 112 4 */
uint32_t cache_max_flows; /* 116 4 */
char * virtual_obs_id; /* 120 8 */
/* --- cacheline 2 boundary (128 bytes) --- */
uint8_t virtual_obs_len; /* 128 1 */
/* XXX 7 bytes hole, try to pack */
ofproto_ipfix_stats ofproto_stats; /* 136 88 */
/* --- cacheline 3 boundary (192 bytes) was 32 bytes ago --- */
struct dpif_ipfix_global_stats ipfix_global_stats; /* 224 152 */
/* size: 376, cachelines: 6, members: 13 */
/* sum members: 361, holes: 3, sum holes: 15 */
/* last cacheline: 56 bytes */
};
...
Proposal:
$ git diff
diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index fa71fda0f276..1701d91e8a72 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -131,8 +131,8 @@ struct dpif_ipfix_domain {
struct dpif_ipfix_exporter {
uint32_t exporter_id; /* Exporting Process identifier */
- struct collectors *collectors;
uint32_t seq_number;
+ struct collectors *collectors;
struct hmap domains; /* Contains struct dpif_ipfix_domain indexed by
observation domain id. */
time_t last_stats_sent_time;
$ pahole ofproto/libofproto_la-ofproto-dpif-ipfix.o
...
struct dpif_ipfix_exporter {
uint32_t exporter_id; /* 0 4 */
uint32_t seq_number; /* 4 4 */
struct collectors * collectors; /* 8 8 */
struct hmap domains; /* 16 32 */
time_t last_stats_sent_time; /* 48 8 */
struct hmap cache_flow_key_map; /* 56 32 */
/* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
struct ovs_list cache_flow_start_timestamp_list; /* 88
16 */
uint32_t cache_active_timeout; /* 104 4 */
uint32_t cache_max_flows; /* 108 4 */
char * virtual_obs_id; /* 112 8 */
uint8_t virtual_obs_len; /* 120 1 */
/* XXX 7 bytes hole, try to pack */
/* --- cacheline 2 boundary (128 bytes) --- */
ofproto_ipfix_stats ofproto_stats; /* 128 88 */
/* --- cacheline 3 boundary (192 bytes) was 24 bytes ago --- */
struct dpif_ipfix_global_stats ipfix_global_stats; /* 216 152 */
/* size: 368, cachelines: 6, members: 13 */
/* sum members: 361, holes: 1, sum holes: 7 */
/* last cacheline: 48 bytes */
};
...
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 08c78ff57..1421ab0ab 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -7488,3 +7488,55 @@ OVS_WAIT_UNTIL([cat p2.pcap | grep -E "0x0050: *0000
> *0000 *5002 *2000 *b85e *00
>
> OVS_TRAFFIC_VSWITCHD_STOP
> AT_CLEANUP
> +
> +AT_SETUP([ipfix - muliple domains])
> +AT_SKIP_IF([test $HAVE_TCPDUMP = no])
> +OVS_TRAFFIC_VSWITCHD_START()
> +
> +ADD_NAMESPACES(at_ns0, at_ns1)
> +
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
> +
> +dnl Create a dummy IPFIX collector just in localhost so that port seems open.
> +OVS_DAEMONIZE([nc -l -u 127.0.0.1 5555 > /dev/null], [nc.pid])
> +
> +OVS_DAEMONIZE([tcpdump -n -i lo udp port 5555 -w ipfix.pcap], [tcpdump.pid])
> +sleep 1
> +
> +dnl Configure a Flow Sample Collector Set with id = 1.
> +AT_CHECK([ovs-vsctl -- --id=@br get Bridge br0 \
> + -- --id=@i create IPFIX targets=\"127.0.0.1:5555\" \
> + -- create Flow_Sample_Collector_Set bridge=@br id=1
> ipfix=@i],
> + [ignore], [ignore])
> +
> +dnl Push flows that will allow communication between the network namespaces
> +dnl and sample all ICMP traffic with multiple observation domain IDs.
> +AT_DATA([flows.txt], [dnl
> +table=0,priority=100,in_port=ovs-p0,ip,icmp
> actions=sample(probability=65535,collector_set_id=1,obs_domain_id=1),NORMAL
> +table=0,priority=100,in_port=ovs-p1,ip,icmp
> actions=sample(probability=65535,collector_set_id=1,obs_domain_id=2),NORMAL
> +table=0,priority=0,actions=NORMAL
> +])
> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> +
> +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.2 | FORMAT_PING],
> [0], [dnl
> +3 packets transmitted, 3 received, 0% packet loss, time 0ms
> +])
> +
> +sleep 1
> +kill $(cat tcpdump.pid)
> +wait $(cat tcpdump.pid) 2> /dev/null
> +
> +dnl Check templates (Flow ID = 0x02) where sent to ObservationDomain 1.
> +AT_CHECK([test $(tcpdump -r ipfix.pcap --count "udp port 5555 and
> udp[[20:4]] = 0x00000001 and udp[[24:2]] = 0x02" 2>/dev/null | tr -d
> "packets") -gt 0])
> +dnl Check that exactly 3 samples (Flow ID != 0x02) where sent to
> ObservationDomain 1.
> +AT_CHECK([test $(tcpdump -r ipfix.pcap --count "udp port 5555 and
> udp[[20:4]] = 0x00000001 and udp[[24:2]] != 0x02" 2>/dev/null | tr -d
> "packets") -eq 3])
> +
> +dnl Check templates (Flow ID = 0x02) where sent to ObservationDomain 2.
> +AT_CHECK([test $(tcpdump -r ipfix.pcap --count "udp port 5555 and
> udp[[20:4]] = 0x00000002 and udp[[24:2]] = 0x02" 2>/dev/null | tr -d
> "packets") -gt 0])
> +dnl Check that exactly 3 samples (Flow ID != 0x02) where sent to
> ObservationDomain 2.
> +AT_CHECK([test $(tcpdump -r ipfix.pcap --count "udp port 5555 and
> udp[[20:4]] = 0x00000002 and udp[[24:2]] != 0x02" 2>/dev/null | tr -d
> "packets") -eq 3])
> +
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CLEANUP
> +
nit: git complains, I think it is about the trailing blank line above.
$ git am a.patch
Applying: ofproto-ipfix: use per-domain template timeouts
.git/rebase-apply/patch:366: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
