[ovs-dev] [PATCH v3 1/2] ofproto-ipfix: use per-domain template timeouts

2023-02-10 Thread Adrián Moreno
From: Adrian Moreno 

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 

---
- v3:
  - Removed unit tests which generate errors in Intel CI. Will submit in
independent series.
  - Added Simon Horman's proposal for incresing cache efficiency.
- 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 | 129 ---
 1 file changed, 105 insertions(+), 24 deletions(-)

diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index 742eed399..1701d91e8 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 collectors *collectors;
+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. */
@@ -617,6 +624,9 @@ static void get_export_time_now(uint64_t *, uint32_t *);
 
 static void dpif_ipfix_cache_expire_now(struct dpif_ipfix_exporter *, bool);
 
+static void dpif_ipfix_exporter_del_domain(struct dpif_ipfix_exporter * ,
+   struct dpif_ipfix_domain * );
+
 static bool
 ofproto_ipfix_bridge_exporter_options_equal(
 const struct ofproto_ipfix_bridge_exporter_options *a,
@@ -697,13 +707,14 @@ dpif_ipfix_exporter_init(struct dpif_ipfix_exporter 
*exporter)
 exporter->exporter_id = ++exporter_total_count;
 exporter->collectors = NULL;
 exporter->seq_number = 1;
-exporter->last_template_set_time = 0;
+exporter->last_stats_sent_time = 0;
 hmap_init(&exporter->cache_flow_key_map);
 ovs_list_init(&exporter->cache_flow_start_timestamp_list);
 exporter->cache_active_timeout = 0;
 exporter->cache_max_flows = 0;
 exporter->virtual_obs_id = NULL;
 exporter->virtual_obs_len = 0;
+hmap_init(&exporter->domains);
 
 memset(&exporter->ipfix_global_stats, 0,
sizeof(struct dpif_ipfix_global_stats));
@@ -711,6 +722,7 @@ dpif_ipfix_exporter_init(struct dpif_ipfix_exporter 
*exporter)
 
 static void
 dpif_ipfix_exporter_clear(struct dpif_ipfix_exporter *exporter)
+OVS_REQUIRES(mutex)
 {
 /* Flush the cache with flow end reason "forced end." */
 dpif_ipfix_cache_expire_now(exporter, true);
@@ -719,22 +731,29 @@ dpif_ipfix_exporter_clear(struct dpif_ipfix_exporter 
*exporter)
 exporter->exporter_id = 0;
 exporter->collectors = NULL;
 exporter->seq_number = 1;
-exporter->last_template_set_time = 0;
+exporter->last_stats_sent_time = 0;
 exporter->cache_active_timeout = 0;
 exporter->cache_max_flows = 0;
 free(exporter->virtual_obs_id);
 exporter->virtual_obs_id = NULL;
 exporter->virtual_obs_len = 0;
 
+struct dpif_ipfix_domain *dom;
+HMAP_FOR_EACH_SAFE (dom, hmap_node, &exporter->domains) {
+dpif_ipfix_exporter_del_domain(exporter, dom);
+}
+
 memset(&exporter->ipfix_global_stats, 0,
sizeof(struct dpif_ipfix_global_stats));
 }
 
 static void
 dpif_ipfix_exporter_destroy(struct dpif_ipfix_exporter *exporter)
+OVS_REQUIRES(mutex)
 {
 dpif_ipfix_exporter_clear(exporter);
 hmap_destroy(&exporter->cache_flow_key_map);
+hmap_destroy(&exporter->domains);
 }
 
 static bool
@@ -742,7 +761,7 @@ dpif_ipfix_exporter_set_options(struct dpif_ipfix_exporter 
*exporter,
 const struct sset *targets,
 const uint32_t cache_active_timeout,
 const uint32_t cache_max_flows,
-const char *virtual_obs_id)
+const char *virtual_obs_id) OVS_REQUIRES(mutex)
 {
 size_t virt

Re: [ovs-dev] [PATCH v3 1/2] ofproto-ipfix: use per-domain template timeouts

2023-02-13 Thread 0-day Robot
Bleep bloop.  Greetings Adrián Moreno, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Comment with 'xxx' marker
#271 FILE: ofproto/ofproto-dpif-ipfix.c:2925:
/* XXX: Make frequency of the (Options) Template and Exporter Process

Lines checked: 319, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH v3 1/2] ofproto-ipfix: use per-domain template timeouts

2023-02-20 Thread Simon Horman
On Fri, Feb 10, 2023 at 05:03:13PM +0100, Adrián Moreno wrote:
> From: Adrian Moreno 
> 
> 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 

Reviewed-by: Simon Horman 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH v3 1/2] ofproto-ipfix: use per-domain template timeouts

2023-02-21 Thread Ilya Maximets
On 2/20/23 16:41, Simon Horman wrote:
> On Fri, Feb 10, 2023 at 05:03:13PM +0100, Adrián Moreno wrote:
>> From: Adrian Moreno 
>>
>> 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 
> 
> Reviewed-by: Simon Horman 

Thanks, Adrian and Simon!

I applied this one patch.  Also backported down to 2.17 since it's
actually a bug fix.

The second patch of the set needs a minor change in the database schema
before it can be accepted.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev