On Fri, Apr 22, 2022 at 11:41:25AM +0200, Eelco Chaudron wrote:
>
>
> 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.
Thanks for the review, will address the commnents in the next version.
>
> > Fixes: a9f5ee1199e1 ("ofproto-dpif: Trigger revalidation when ipfix config
> > set.")
> > Signed-off-by: lic121 <lic...@chinatelecom.cn>
> > ---
> > 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
