On 16 Apr 2022, at 13:09, lic121 wrote:
> ipfix cfg creation/deleting triggers revalidation. But this does
> not cover the case where ipfix options changes, which also suppose
> to trigger revalidation.
Some small comments below, and I think we should add a test case to verify the
cases that could yield a reconfigure.
> Fixes: a9f5ee1199e1 ("ofproto-dpif: Trigger revalidation when ipfix config
> set.")
> Signed-off-by: lic121 <[email protected]>
> ---
> ofproto/ofproto-dpif-ipfix.c | 46
> ++++++++++++++++++++++++++++----------------
> ofproto/ofproto-dpif-ipfix.h | 2 +-
> ofproto/ofproto-dpif.c | 7 +++----
> 3 files changed, 33 insertions(+), 22 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> index fc927fe..7d316ca 100644
> --- a/ofproto/ofproto-dpif-ipfix.c
> +++ b/ofproto/ofproto-dpif-ipfix.c
> @@ -926,17 +926,19 @@ dpif_ipfix_bridge_exporter_destroy(struct
> dpif_ipfix_bridge_exporter *exporter)
> static void
> dpif_ipfix_bridge_exporter_set_options(
> struct dpif_ipfix_bridge_exporter *exporter,
> - const struct ofproto_ipfix_bridge_exporter_options *options)
> + const struct ofproto_ipfix_bridge_exporter_options *options,
> + bool *options_changed)
> {
> - bool options_changed;
> -
> if (!options || sset_is_empty(&options->targets)) {
> /* No point in doing any work if there are no targets. */
> - dpif_ipfix_bridge_exporter_clear(exporter);
> + if (exporter->options) {
> + dpif_ipfix_bridge_exporter_clear(exporter);
> + *options_changed = true;
> + }
This can return without the option changed being set, so it should become:
} else {
*options_changed = false;
}
> return;
> }
>
> - options_changed = (
> + *options_changed = (
> !exporter->options
> || !ofproto_ipfix_bridge_exporter_options_equal(
> options, exporter->options));
> @@ -945,7 +947,7 @@ dpif_ipfix_bridge_exporter_set_options(
> * shortchanged in collectors (which indicates that opening one or
> * more of the configured collectors failed, so that we should
> * retry). */
> - if (options_changed
> + if (*options_changed
> || collectors_count(exporter->exporter.collectors)
> < sset_count(&options->targets)) {
> if (!dpif_ipfix_exporter_set_options(
> @@ -957,7 +959,7 @@ dpif_ipfix_bridge_exporter_set_options(
> }
>
> /* Avoid reconfiguring if options didn't change. */
> - if (!options_changed) {
> + if (!*options_changed) {
> return;
> }
>
> @@ -1015,17 +1017,19 @@ dpif_ipfix_flow_exporter_destroy(struct
> dpif_ipfix_flow_exporter *exporter)
> static bool
> dpif_ipfix_flow_exporter_set_options(
> struct dpif_ipfix_flow_exporter *exporter,
> - const struct ofproto_ipfix_flow_exporter_options *options)
> + const struct ofproto_ipfix_flow_exporter_options *options,
> + bool *options_changed)
> {
> - bool options_changed;
> -
> if (sset_is_empty(&options->targets)) {
> /* No point in doing any work if there are no targets. */
> - dpif_ipfix_flow_exporter_clear(exporter);
> + if (exporter->options) {
> + dpif_ipfix_flow_exporter_clear(exporter);
> + *options_changed = true;
> + }
This can return without the option changed being set, so it should become:
} else {
*options_changed = false;
}
> return true;
> }
>
> - options_changed = (
> + *options_changed = (
> !exporter->options
> || !ofproto_ipfix_flow_exporter_options_equal(
> options, exporter->options));
> @@ -1034,7 +1038,7 @@ dpif_ipfix_flow_exporter_set_options(
> * shortchanged in collectors (which indicates that opening one or
> * more of the configured collectors failed, so that we should
> * retry). */
> - if (options_changed
> + if (*options_changed
> || collectors_count(exporter->exporter.collectors)
> < sset_count(&options->targets)) {
> if (!dpif_ipfix_exporter_set_options(
> @@ -1046,7 +1050,7 @@ dpif_ipfix_flow_exporter_set_options(
> }
>
> /* Avoid reconfiguring if options didn't change. */
> - if (!options_changed) {
> + if (!*options_changed) {
> return true;
> }
>
> @@ -1069,7 +1073,7 @@ remove_flow_exporter(struct dpif_ipfix *di,
> free(node);
> }
>
> -void
> +bool
> dpif_ipfix_set_options(
> struct dpif_ipfix *di,
> const struct ofproto_ipfix_bridge_exporter_options
> *bridge_exporter_options,
> @@ -1077,12 +1081,14 @@ dpif_ipfix_set_options(
> size_t n_flow_exporters_options) OVS_EXCLUDED(mutex)
> {
> int i;
> + bool beo_changed, feo_changed, entry_changed;
I’m not a compiler but looking at the sequence below I think feo_changed needs
to be initialized to false.
Wondering why we do not get an error on this…
> struct ofproto_ipfix_flow_exporter_options *options;
> struct dpif_ipfix_flow_exporter_map_node *node;
>
> ovs_mutex_lock(&mutex);
> dpif_ipfix_bridge_exporter_set_options(&di->bridge_exporter,
> - bridge_exporter_options);
> + bridge_exporter_options,
> + &beo_changed);
>
> /* Add new flow exporters and update current flow exporters. */
> options = (struct ofproto_ipfix_flow_exporter_options *)
> @@ -1095,10 +1101,14 @@ dpif_ipfix_set_options(
> dpif_ipfix_flow_exporter_init(&node->exporter);
> hmap_insert(&di->flow_exporter_map, &node->node,
> hash_int(options->collector_set_id, 0));
> + feo_changed = true;
> }
> - if (!dpif_ipfix_flow_exporter_set_options(&node->exporter, options))
> {
> + if (!dpif_ipfix_flow_exporter_set_options(&node->exporter,
> + options,
> + &entry_changed)) {
> remove_flow_exporter(di, node);
> }
> + feo_changed = entry_changed ? true : feo_changed;
> options++;
> }
>
> @@ -1117,10 +1127,12 @@ dpif_ipfix_set_options(
> }
> if (i == n_flow_exporters_options) { /* Not found. */
> remove_flow_exporter(di, node);
> + feo_changed = true;
> }
> }
>
> ovs_mutex_unlock(&mutex);
> + return beo_changed || feo_changed;
> }
>
> struct dpif_ipfix *
> diff --git a/ofproto/ofproto-dpif-ipfix.h b/ofproto/ofproto-dpif-ipfix.h
> index 1f42cd5..75c0ab8 100644
> --- a/ofproto/ofproto-dpif-ipfix.h
> +++ b/ofproto/ofproto-dpif-ipfix.h
> @@ -48,7 +48,7 @@ bool dpif_ipfix_get_bridge_exporter_output_sampling(const
> struct dpif_ipfix *);
> bool dpif_ipfix_get_flow_exporter_tunnel_sampling(const struct dpif_ipfix *,
> const uint32_t);
> bool dpif_ipfix_is_tunnel_port(const struct dpif_ipfix *, odp_port_t);
> -void dpif_ipfix_set_options(
> +bool dpif_ipfix_set_options(
> struct dpif_ipfix *,
> const struct ofproto_ipfix_bridge_exporter_options *,
> const struct ofproto_ipfix_flow_exporter_options *, size_t);
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 6601f23..b42a30c 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -2337,6 +2337,7 @@ set_ipfix(
> struct dpif_ipfix *di = ofproto->ipfix;
> bool has_options = bridge_exporter_options || flow_exporters_options;
> bool new_di = false;
> + bool option_changed = false;
bool options_changed = false;
You use the variable options_changed above, so I would use the same here.
>
> if (has_options && !di) {
> di = ofproto->ipfix = dpif_ipfix_create();
> @@ -2346,7 +2347,7 @@ set_ipfix(
> if (di) {
> /* Call set_options in any case to cleanly flush the flow
> * caches in the last exporters that are to be destroyed. */
> - dpif_ipfix_set_options(
> + option_changed = dpif_ipfix_set_options(
> di, bridge_exporter_options, flow_exporters_options,
> n_flow_exporters_options);
>
> @@ -2363,9 +2364,7 @@ set_ipfix(
> ofproto->ipfix = NULL;
> }
>
> - /* TODO: need to consider ipfix option changes more than
> - * enable/disable */
> - if (new_di || !ofproto->ipfix) {
> + if (new_di || option_changed) {
> ofproto->backer->need_revalidate = REV_RECONFIGURE;
> }
> }
> --
> 1.8.3.1
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
