| Message ID | 20221018120214.1106988-2-amorenoz@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ofproto-ipfix: use per-domain template timeouts | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
Bleep bloop. Greetings Adrian 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
#273 FILE: ofproto/ofproto-dpif-ipfix.c:2928:
/* XXX: Make frequency of the (Options) Template and Exporter Process
Lines checked: 321, Warnings: 1, Errors: 0
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
On Tue, Oct 18, 2022 at 8:02 AM Adrian Moreno <amorenoz@redhat.com> wrote: > > 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 <amorenoz@redhat.com> > --- > ofproto/ofproto-dpif-ipfix.c | 130 ++++++++++++++++++++++++++++------- > 1 file changed, 107 insertions(+), 23 deletions(-) > > diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c > index 742eed399..98a3f9b71 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.*/ > + uint32_t obs_domain_id; > + time_t last_template_set_time; Just a thought, we end up setting hmap_node.hash to obs_domain_id, do we need to hold on to that variable? It should be unique, could we ditch obs_domain_id and just use HMAP_FOR_EACH_WITH_HASH below instead? > +}; > + > 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. */ > + 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 virtual_obs_len; > collectors_destroy(exporter->collectors); > @@ -769,6 +788,40 @@ dpif_ipfix_exporter_set_options(struct dpif_ipfix_exporter *exporter, > return true; > } > > +static struct dpif_ipfix_domain * > +dpif_ipfix_exporter_find_domain(const struct dpif_ipfix_exporter *exporter, > + uint32_t domain_id) I believe this function would also call for an OVS_REQUIRES(mutex) > +{ > + struct dpif_ipfix_domain *dom; > + HMAP_FOR_EACH_IN_BUCKET (dom, hmap_node, hash_int(domain_id, 0), > + &exporter->domains) { > + if (dom->obs_domain_id == domain_id) { > + return dom; > + } > + } > + return NULL; > +} > + > +static struct dpif_ipfix_domain * > +dpif_ipfix_exporter_insert_domain(struct dpif_ipfix_exporter *exporter, > + const uint32_t domain_id) OVS_REQUIRES(mutex) > +{ > + struct dpif_ipfix_domain *dom = xmalloc(sizeof *dom); > + dom->obs_domain_id = domain_id; > + dom->last_template_set_time = 0; > + hmap_insert(&exporter->domains, &dom->hmap_node, hash_int(domain_id, 0)); > + return dom; > +} > + > +static void > +dpif_ipfix_exporter_del_domain(struct dpif_ipfix_exporter *exporter, > + struct dpif_ipfix_domain *dom) > + OVS_REQUIRES(mutex) > +{ > + hmap_remove(&exporter->domains, &dom->hmap_node); > + free(dom); > +} > + > static struct dpif_ipfix_port * > dpif_ipfix_find_port(const struct dpif_ipfix *di, > odp_port_t odp_port) OVS_REQUIRES(mutex) > @@ -909,6 +962,7 @@ dpif_ipfix_bridge_exporter_init(struct dpif_ipfix_bridge_exporter *exporter) > > static void > dpif_ipfix_bridge_exporter_clear(struct dpif_ipfix_bridge_exporter *exporter) > + OVS_REQUIRES(mutex) > { > dpif_ipfix_exporter_clear(&exporter->exporter); > ofproto_ipfix_bridge_exporter_options_destroy(exporter->options); > @@ -918,6 +972,7 @@ dpif_ipfix_bridge_exporter_clear(struct dpif_ipfix_bridge_exporter *exporter) > > static void > dpif_ipfix_bridge_exporter_destroy(struct dpif_ipfix_bridge_exporter *exporter) > + OVS_REQUIRES(mutex) > { > dpif_ipfix_bridge_exporter_clear(exporter); > dpif_ipfix_exporter_destroy(&exporter->exporter); > @@ -927,7 +982,7 @@ static void > dpif_ipfix_bridge_exporter_set_options( > struct dpif_ipfix_bridge_exporter *exporter, > const struct ofproto_ipfix_bridge_exporter_options *options, > - bool *options_changed) > + bool *options_changed) OVS_REQUIRES(mutex) > { > if (!options || sset_is_empty(&options->targets)) { > /* No point in doing any work if there are no targets. */ > @@ -1003,6 +1058,7 @@ dpif_ipfix_flow_exporter_init(struct dpif_ipfix_flow_exporter *exporter) > > static void > dpif_ipfix_flow_exporter_clear(struct dpif_ipfix_flow_exporter *exporter) > + OVS_REQUIRES(mutex) > { > dpif_ipfix_exporter_clear(&exporter->exporter); > ofproto_ipfix_flow_exporter_options_destroy(exporter->options); > @@ -1011,6 +1067,7 @@ dpif_ipfix_flow_exporter_clear(struct dpif_ipfix_flow_exporter *exporter) > > static void > dpif_ipfix_flow_exporter_destroy(struct dpif_ipfix_flow_exporter *exporter) > + OVS_REQUIRES(mutex) > { > dpif_ipfix_flow_exporter_clear(exporter); > dpif_ipfix_exporter_destroy(&exporter->exporter); > @@ -1020,7 +1077,7 @@ static bool > dpif_ipfix_flow_exporter_set_options( > struct dpif_ipfix_flow_exporter *exporter, > const struct ofproto_ipfix_flow_exporter_options *options, > - bool *options_changed) > + bool *options_changed) OVS_REQUIRES(mutex) > { > if (sset_is_empty(&options->targets)) { > /* No point in doing any work if there are no targets. */ > @@ -1071,6 +1128,7 @@ dpif_ipfix_flow_exporter_set_options( > static void > remove_flow_exporter(struct dpif_ipfix *di, > struct dpif_ipfix_flow_exporter_map_node *node) > + OVS_REQUIRES(mutex) > { > hmap_remove(&di->flow_exporter_map, &node->node); > dpif_ipfix_flow_exporter_destroy(&node->exporter); > @@ -2000,6 +2058,7 @@ static void > ipfix_cache_update(struct dpif_ipfix_exporter *exporter, > struct ipfix_flow_cache_entry *entry, > enum ipfix_sampled_packet_type sampled_pkt_type) > + OVS_REQUIRES(mutex) > { > struct ipfix_flow_cache_entry *old_entry; > size_t current_flows = 0; > @@ -2811,14 +2870,36 @@ dpif_ipfix_flow_sample(struct dpif_ipfix *di, const struct dp_packet *packet, > ovs_mutex_unlock(&mutex); > } > > +static bool > +dpif_ipfix_should_send_template(struct dpif_ipfix_exporter *exporter, > + const uint32_t observation_domain_id, > + const uint32_t export_time_sec) > + OVS_REQUIRES(mutex) > +{ > + struct dpif_ipfix_domain *domain; > + domain = dpif_ipfix_exporter_find_domain(exporter, > + observation_domain_id); > + if (!domain) { > + /* First time we see this obs_domain_id. */ > + domain = dpif_ipfix_exporter_insert_domain(exporter, > + observation_domain_id); > + } > + > + if ((domain->last_template_set_time + IPFIX_TEMPLATE_INTERVAL) > + <= export_time_sec) { > + domain->last_template_set_time = export_time_sec; > + return true; > + } > + return false; > +} > + > static void > dpif_ipfix_cache_expire(struct dpif_ipfix_exporter *exporter, > bool forced_end, const uint64_t export_time_usec, > - const uint32_t export_time_sec) > + const uint32_t export_time_sec) OVS_REQUIRES(mutex) > { > struct ipfix_flow_cache_entry *entry; > uint64_t max_flow_start_timestamp_usec; > - bool template_msg_sent = false; > enum ipfix_flow_end_reason flow_end_reason; > > if (ovs_list_is_empty(&exporter->cache_flow_start_timestamp_list)) { > @@ -2844,25 +2925,28 @@ dpif_ipfix_cache_expire(struct dpif_ipfix_exporter *exporter, > break; > } > > - ovs_list_remove(&entry->cache_flow_start_timestamp_list_node); > - hmap_remove(&exporter->cache_flow_key_map, > - &entry->flow_key_map_node); > + /* XXX: Make frequency of the (Options) Template and Exporter Process > + * Statistics transmission configurable. > + * Cf. IETF RFC 5101 Section 4.3. and 10.3.6. */ > + if ((exporter->last_stats_sent_time + IPFIX_TEMPLATE_INTERVAL) > + <= export_time_sec) { > + exporter->last_stats_sent_time = export_time_sec; > + ipfix_send_exporter_data_msg(exporter, export_time_sec); > + } > > - /* XXX: Make frequency of the (Options) Template and Exporter Process > - * Statistics transmission configurable. > - * Cf. IETF RFC 5101 Section 4.3. and 10.3.6. */ > - if (!template_msg_sent > - && (exporter->last_template_set_time + IPFIX_TEMPLATE_INTERVAL) > - <= export_time_sec) { > + if (dpif_ipfix_should_send_template(exporter, > + entry->flow_key.obs_domain_id, > + export_time_sec)) { > + VLOG_DBG("Sending templates for ObservationDomainID %"PRIu32, > + entry->flow_key.obs_domain_id); > ipfix_send_template_msgs(exporter, export_time_sec, > entry->flow_key.obs_domain_id); > - exporter->last_template_set_time = export_time_sec; > - template_msg_sent = true; > - > - /* Send Exporter Process Statistics. */ > - ipfix_send_exporter_data_msg(exporter, export_time_sec); > } > > + ovs_list_remove(&entry->cache_flow_start_timestamp_list_node); > + hmap_remove(&exporter->cache_flow_key_map, > + &entry->flow_key_map_node); > + > /* XXX: Group multiple data records for the same obs domain id > * into the same message. */ > ipfix_send_data_msg(exporter, export_time_sec, entry, flow_end_reason); > @@ -2883,7 +2967,7 @@ get_export_time_now(uint64_t *export_time_usec, uint32_t *export_time_sec) > > static void > dpif_ipfix_cache_expire_now(struct dpif_ipfix_exporter *exporter, > - bool forced_end) > + bool forced_end) OVS_REQUIRES(mutex) > { > uint64_t export_time_usec; > uint32_t export_time_sec; > -- > 2.37.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Thanks for the review Mike, I'll update the patch. On 1/13/23 06:55, Mike Pattrick wrote: > On Tue, Oct 18, 2022 at 8:02 AM Adrian Moreno <amorenoz@redhat.com> wrote: >> >> 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 <amorenoz@redhat.com> >> --- >> ofproto/ofproto-dpif-ipfix.c | 130 ++++++++++++++++++++++++++++------- >> 1 file changed, 107 insertions(+), 23 deletions(-) >> >> diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c >> index 742eed399..98a3f9b71 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.*/ >> + uint32_t obs_domain_id; >> + time_t last_template_set_time; > > Just a thought, we end up setting hmap_node.hash to obs_domain_id, do > we need to hold on to that variable? It should be unique, could we > ditch obs_domain_id and just use HMAP_FOR_EACH_WITH_HASH below > instead? > Good point! I think you're right. I'll give it a try. >> +}; >> + >> 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. */ >> + 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 virtual_obs_len; >> collectors_destroy(exporter->collectors); >> @@ -769,6 +788,40 @@ dpif_ipfix_exporter_set_options(struct dpif_ipfix_exporter *exporter, >> return true; >> } >> >> +static struct dpif_ipfix_domain * >> +dpif_ipfix_exporter_find_domain(const struct dpif_ipfix_exporter *exporter, >> + uint32_t domain_id) > > I believe this function would also call for an OVS_REQUIRES(mutex) > Yes. Good catch. Thanks. >> +{ >> + struct dpif_ipfix_domain *dom; >> + HMAP_FOR_EACH_IN_BUCKET (dom, hmap_node, hash_int(domain_id, 0), >> + &exporter->domains) { >> + if (dom->obs_domain_id == domain_id) { >> + return dom; >> + } >> + } >> + return NULL; >> +} >> + >> +static struct dpif_ipfix_domain * >> +dpif_ipfix_exporter_insert_domain(struct dpif_ipfix_exporter *exporter, >> + const uint32_t domain_id) OVS_REQUIRES(mutex) >> +{ >> + struct dpif_ipfix_domain *dom = xmalloc(sizeof *dom); >> + dom->obs_domain_id = domain_id; >> + dom->last_template_set_time = 0; >> + hmap_insert(&exporter->domains, &dom->hmap_node, hash_int(domain_id, 0)); >> + return dom; >> +} >> + >> +static void >> +dpif_ipfix_exporter_del_domain(struct dpif_ipfix_exporter *exporter, >> + struct dpif_ipfix_domain *dom) >> + OVS_REQUIRES(mutex) >> +{ >> + hmap_remove(&exporter->domains, &dom->hmap_node); >> + free(dom); >> +} >> + >> static struct dpif_ipfix_port * >> dpif_ipfix_find_port(const struct dpif_ipfix *di, >> odp_port_t odp_port) OVS_REQUIRES(mutex) >> @@ -909,6 +962,7 @@ dpif_ipfix_bridge_exporter_init(struct dpif_ipfix_bridge_exporter *exporter) >> >> static void >> dpif_ipfix_bridge_exporter_clear(struct dpif_ipfix_bridge_exporter *exporter) >> + OVS_REQUIRES(mutex) >> { >> dpif_ipfix_exporter_clear(&exporter->exporter); >> ofproto_ipfix_bridge_exporter_options_destroy(exporter->options); >> @@ -918,6 +972,7 @@ dpif_ipfix_bridge_exporter_clear(struct dpif_ipfix_bridge_exporter *exporter) >> >> static void >> dpif_ipfix_bridge_exporter_destroy(struct dpif_ipfix_bridge_exporter *exporter) >> + OVS_REQUIRES(mutex) >> { >> dpif_ipfix_bridge_exporter_clear(exporter); >> dpif_ipfix_exporter_destroy(&exporter->exporter); >> @@ -927,7 +982,7 @@ static void >> dpif_ipfix_bridge_exporter_set_options( >> struct dpif_ipfix_bridge_exporter *exporter, >> const struct ofproto_ipfix_bridge_exporter_options *options, >> - bool *options_changed) >> + bool *options_changed) OVS_REQUIRES(mutex) >> { >> if (!options || sset_is_empty(&options->targets)) { >> /* No point in doing any work if there are no targets. */ >> @@ -1003,6 +1058,7 @@ dpif_ipfix_flow_exporter_init(struct dpif_ipfix_flow_exporter *exporter) >> >> static void >> dpif_ipfix_flow_exporter_clear(struct dpif_ipfix_flow_exporter *exporter) >> + OVS_REQUIRES(mutex) >> { >> dpif_ipfix_exporter_clear(&exporter->exporter); >> ofproto_ipfix_flow_exporter_options_destroy(exporter->options); >> @@ -1011,6 +1067,7 @@ dpif_ipfix_flow_exporter_clear(struct dpif_ipfix_flow_exporter *exporter) >> >> static void >> dpif_ipfix_flow_exporter_destroy(struct dpif_ipfix_flow_exporter *exporter) >> + OVS_REQUIRES(mutex) >> { >> dpif_ipfix_flow_exporter_clear(exporter); >> dpif_ipfix_exporter_destroy(&exporter->exporter); >> @@ -1020,7 +1077,7 @@ static bool >> dpif_ipfix_flow_exporter_set_options( >> struct dpif_ipfix_flow_exporter *exporter, >> const struct ofproto_ipfix_flow_exporter_options *options, >> - bool *options_changed) >> + bool *options_changed) OVS_REQUIRES(mutex) >> { >> if (sset_is_empty(&options->targets)) { >> /* No point in doing any work if there are no targets. */ >> @@ -1071,6 +1128,7 @@ dpif_ipfix_flow_exporter_set_options( >> static void >> remove_flow_exporter(struct dpif_ipfix *di, >> struct dpif_ipfix_flow_exporter_map_node *node) >> + OVS_REQUIRES(mutex) >> { >> hmap_remove(&di->flow_exporter_map, &node->node); >> dpif_ipfix_flow_exporter_destroy(&node->exporter); >> @@ -2000,6 +2058,7 @@ static void >> ipfix_cache_update(struct dpif_ipfix_exporter *exporter, >> struct ipfix_flow_cache_entry *entry, >> enum ipfix_sampled_packet_type sampled_pkt_type) >> + OVS_REQUIRES(mutex) >> { >> struct ipfix_flow_cache_entry *old_entry; >> size_t current_flows = 0; >> @@ -2811,14 +2870,36 @@ dpif_ipfix_flow_sample(struct dpif_ipfix *di, const struct dp_packet *packet, >> ovs_mutex_unlock(&mutex); >> } >> >> +static bool >> +dpif_ipfix_should_send_template(struct dpif_ipfix_exporter *exporter, >> + const uint32_t observation_domain_id, >> + const uint32_t export_time_sec) >> + OVS_REQUIRES(mutex) >> +{ >> + struct dpif_ipfix_domain *domain; >> + domain = dpif_ipfix_exporter_find_domain(exporter, >> + observation_domain_id); >> + if (!domain) { >> + /* First time we see this obs_domain_id. */ >> + domain = dpif_ipfix_exporter_insert_domain(exporter, >> + observation_domain_id); >> + } >> + >> + if ((domain->last_template_set_time + IPFIX_TEMPLATE_INTERVAL) >> + <= export_time_sec) { >> + domain->last_template_set_time = export_time_sec; >> + return true; >> + } >> + return false; >> +} >> + >> static void >> dpif_ipfix_cache_expire(struct dpif_ipfix_exporter *exporter, >> bool forced_end, const uint64_t export_time_usec, >> - const uint32_t export_time_sec) >> + const uint32_t export_time_sec) OVS_REQUIRES(mutex) >> { >> struct ipfix_flow_cache_entry *entry; >> uint64_t max_flow_start_timestamp_usec; >> - bool template_msg_sent = false; >> enum ipfix_flow_end_reason flow_end_reason; >> >> if (ovs_list_is_empty(&exporter->cache_flow_start_timestamp_list)) { >> @@ -2844,25 +2925,28 @@ dpif_ipfix_cache_expire(struct dpif_ipfix_exporter *exporter, >> break; >> } >> >> - ovs_list_remove(&entry->cache_flow_start_timestamp_list_node); >> - hmap_remove(&exporter->cache_flow_key_map, >> - &entry->flow_key_map_node); >> + /* XXX: Make frequency of the (Options) Template and Exporter Process >> + * Statistics transmission configurable. >> + * Cf. IETF RFC 5101 Section 4.3. and 10.3.6. */ >> + if ((exporter->last_stats_sent_time + IPFIX_TEMPLATE_INTERVAL) >> + <= export_time_sec) { >> + exporter->last_stats_sent_time = export_time_sec; >> + ipfix_send_exporter_data_msg(exporter, export_time_sec); >> + } >> >> - /* XXX: Make frequency of the (Options) Template and Exporter Process >> - * Statistics transmission configurable. >> - * Cf. IETF RFC 5101 Section 4.3. and 10.3.6. */ >> - if (!template_msg_sent >> - && (exporter->last_template_set_time + IPFIX_TEMPLATE_INTERVAL) >> - <= export_time_sec) { >> + if (dpif_ipfix_should_send_template(exporter, >> + entry->flow_key.obs_domain_id, >> + export_time_sec)) { >> + VLOG_DBG("Sending templates for ObservationDomainID %"PRIu32, >> + entry->flow_key.obs_domain_id); >> ipfix_send_template_msgs(exporter, export_time_sec, >> entry->flow_key.obs_domain_id); >> - exporter->last_template_set_time = export_time_sec; >> - template_msg_sent = true; >> - >> - /* Send Exporter Process Statistics. */ >> - ipfix_send_exporter_data_msg(exporter, export_time_sec); >> } >> >> + ovs_list_remove(&entry->cache_flow_start_timestamp_list_node); >> + hmap_remove(&exporter->cache_flow_key_map, >> + &entry->flow_key_map_node); >> + >> /* XXX: Group multiple data records for the same obs domain id >> * into the same message. */ >> ipfix_send_data_msg(exporter, export_time_sec, entry, flow_end_reason); >> @@ -2883,7 +2967,7 @@ get_export_time_now(uint64_t *export_time_usec, uint32_t *export_time_sec) >> >> static void >> dpif_ipfix_cache_expire_now(struct dpif_ipfix_exporter *exporter, >> - bool forced_end) >> + bool forced_end) OVS_REQUIRES(mutex) >> { >> uint64_t export_time_usec; >> uint32_t export_time_sec; >> -- >> 2.37.3 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >
diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c index 742eed399..98a3f9b71 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.*/ + uint32_t obs_domain_id; + 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. */ + 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 virtual_obs_len; collectors_destroy(exporter->collectors); @@ -769,6 +788,40 @@ dpif_ipfix_exporter_set_options(struct dpif_ipfix_exporter *exporter, return true; } +static struct dpif_ipfix_domain * +dpif_ipfix_exporter_find_domain(const struct dpif_ipfix_exporter *exporter, + uint32_t domain_id) +{ + struct dpif_ipfix_domain *dom; + HMAP_FOR_EACH_IN_BUCKET (dom, hmap_node, hash_int(domain_id, 0), + &exporter->domains) { + if (dom->obs_domain_id == domain_id) { + return dom; + } + } + return NULL; +} + +static struct dpif_ipfix_domain * +dpif_ipfix_exporter_insert_domain(struct dpif_ipfix_exporter *exporter, + const uint32_t domain_id) OVS_REQUIRES(mutex) +{ + struct dpif_ipfix_domain *dom = xmalloc(sizeof *dom); + dom->obs_domain_id = domain_id; + dom->last_template_set_time = 0; + hmap_insert(&exporter->domains, &dom->hmap_node, hash_int(domain_id, 0)); + return dom; +} + +static void +dpif_ipfix_exporter_del_domain(struct dpif_ipfix_exporter *exporter, + struct dpif_ipfix_domain *dom) + OVS_REQUIRES(mutex) +{ + hmap_remove(&exporter->domains, &dom->hmap_node); + free(dom); +} + static struct dpif_ipfix_port * dpif_ipfix_find_port(const struct dpif_ipfix *di, odp_port_t odp_port) OVS_REQUIRES(mutex) @@ -909,6 +962,7 @@ dpif_ipfix_bridge_exporter_init(struct dpif_ipfix_bridge_exporter *exporter) static void dpif_ipfix_bridge_exporter_clear(struct dpif_ipfix_bridge_exporter *exporter) + OVS_REQUIRES(mutex) { dpif_ipfix_exporter_clear(&exporter->exporter); ofproto_ipfix_bridge_exporter_options_destroy(exporter->options); @@ -918,6 +972,7 @@ dpif_ipfix_bridge_exporter_clear(struct dpif_ipfix_bridge_exporter *exporter) static void dpif_ipfix_bridge_exporter_destroy(struct dpif_ipfix_bridge_exporter *exporter) + OVS_REQUIRES(mutex) { dpif_ipfix_bridge_exporter_clear(exporter); dpif_ipfix_exporter_destroy(&exporter->exporter); @@ -927,7 +982,7 @@ static void dpif_ipfix_bridge_exporter_set_options( struct dpif_ipfix_bridge_exporter *exporter, const struct ofproto_ipfix_bridge_exporter_options *options, - bool *options_changed) + bool *options_changed) OVS_REQUIRES(mutex) { if (!options || sset_is_empty(&options->targets)) { /* No point in doing any work if there are no targets. */ @@ -1003,6 +1058,7 @@ dpif_ipfix_flow_exporter_init(struct dpif_ipfix_flow_exporter *exporter) static void dpif_ipfix_flow_exporter_clear(struct dpif_ipfix_flow_exporter *exporter) + OVS_REQUIRES(mutex) { dpif_ipfix_exporter_clear(&exporter->exporter); ofproto_ipfix_flow_exporter_options_destroy(exporter->options); @@ -1011,6 +1067,7 @@ dpif_ipfix_flow_exporter_clear(struct dpif_ipfix_flow_exporter *exporter) static void dpif_ipfix_flow_exporter_destroy(struct dpif_ipfix_flow_exporter *exporter) + OVS_REQUIRES(mutex) { dpif_ipfix_flow_exporter_clear(exporter); dpif_ipfix_exporter_destroy(&exporter->exporter); @@ -1020,7 +1077,7 @@ static bool dpif_ipfix_flow_exporter_set_options( struct dpif_ipfix_flow_exporter *exporter, const struct ofproto_ipfix_flow_exporter_options *options, - bool *options_changed) + bool *options_changed) OVS_REQUIRES(mutex) { if (sset_is_empty(&options->targets)) { /* No point in doing any work if there are no targets. */ @@ -1071,6 +1128,7 @@ dpif_ipfix_flow_exporter_set_options( static void remove_flow_exporter(struct dpif_ipfix *di, struct dpif_ipfix_flow_exporter_map_node *node) + OVS_REQUIRES(mutex) { hmap_remove(&di->flow_exporter_map, &node->node); dpif_ipfix_flow_exporter_destroy(&node->exporter); @@ -2000,6 +2058,7 @@ static void ipfix_cache_update(struct dpif_ipfix_exporter *exporter, struct ipfix_flow_cache_entry *entry, enum ipfix_sampled_packet_type sampled_pkt_type) + OVS_REQUIRES(mutex) { struct ipfix_flow_cache_entry *old_entry; size_t current_flows = 0; @@ -2811,14 +2870,36 @@ dpif_ipfix_flow_sample(struct dpif_ipfix *di, const struct dp_packet *packet, ovs_mutex_unlock(&mutex); } +static bool +dpif_ipfix_should_send_template(struct dpif_ipfix_exporter *exporter, + const uint32_t observation_domain_id, + const uint32_t export_time_sec) + OVS_REQUIRES(mutex) +{ + struct dpif_ipfix_domain *domain; + domain = dpif_ipfix_exporter_find_domain(exporter, + observation_domain_id); + if (!domain) { + /* First time we see this obs_domain_id. */ + domain = dpif_ipfix_exporter_insert_domain(exporter, + observation_domain_id); + } + + if ((domain->last_template_set_time + IPFIX_TEMPLATE_INTERVAL) + <= export_time_sec) { + domain->last_template_set_time = export_time_sec; + return true; + } + return false; +} + static void dpif_ipfix_cache_expire(struct dpif_ipfix_exporter *exporter, bool forced_end, const uint64_t export_time_usec, - const uint32_t export_time_sec) + const uint32_t export_time_sec) OVS_REQUIRES(mutex) { struct ipfix_flow_cache_entry *entry; uint64_t max_flow_start_timestamp_usec; - bool template_msg_sent = false; enum ipfix_flow_end_reason flow_end_reason; if (ovs_list_is_empty(&exporter->cache_flow_start_timestamp_list)) { @@ -2844,25 +2925,28 @@ dpif_ipfix_cache_expire(struct dpif_ipfix_exporter *exporter, break; } - ovs_list_remove(&entry->cache_flow_start_timestamp_list_node); - hmap_remove(&exporter->cache_flow_key_map, - &entry->flow_key_map_node); + /* XXX: Make frequency of the (Options) Template and Exporter Process + * Statistics transmission configurable. + * Cf. IETF RFC 5101 Section 4.3. and 10.3.6. */ + if ((exporter->last_stats_sent_time + IPFIX_TEMPLATE_INTERVAL) + <= export_time_sec) { + exporter->last_stats_sent_time = export_time_sec; + ipfix_send_exporter_data_msg(exporter, export_time_sec); + } - /* XXX: Make frequency of the (Options) Template and Exporter Process - * Statistics transmission configurable. - * Cf. IETF RFC 5101 Section 4.3. and 10.3.6. */ - if (!template_msg_sent - && (exporter->last_template_set_time + IPFIX_TEMPLATE_INTERVAL) - <= export_time_sec) { + if (dpif_ipfix_should_send_template(exporter, + entry->flow_key.obs_domain_id, + export_time_sec)) { + VLOG_DBG("Sending templates for ObservationDomainID %"PRIu32, + entry->flow_key.obs_domain_id); ipfix_send_template_msgs(exporter, export_time_sec, entry->flow_key.obs_domain_id); - exporter->last_template_set_time = export_time_sec; - template_msg_sent = true; - - /* Send Exporter Process Statistics. */ - ipfix_send_exporter_data_msg(exporter, export_time_sec); } + ovs_list_remove(&entry->cache_flow_start_timestamp_list_node); + hmap_remove(&exporter->cache_flow_key_map, + &entry->flow_key_map_node); + /* XXX: Group multiple data records for the same obs domain id * into the same message. */ ipfix_send_data_msg(exporter, export_time_sec, entry, flow_end_reason); @@ -2883,7 +2967,7 @@ get_export_time_now(uint64_t *export_time_usec, uint32_t *export_time_sec) static void dpif_ipfix_cache_expire_now(struct dpif_ipfix_exporter *exporter, - bool forced_end) + bool forced_end) OVS_REQUIRES(mutex) { uint64_t export_time_usec; uint32_t export_time_sec;
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 <amorenoz@redhat.com> --- ofproto/ofproto-dpif-ipfix.c | 130 ++++++++++++++++++++++++++++------- 1 file changed, 107 insertions(+), 23 deletions(-)