| Message ID | 1469388799-8738-1-git-send-email-rmoats@us.ibm.com |
|---|---|
| State | Deferred |
| Headers | show |
On Sun, Jul 24, 2016 at 2:33 PM, Ryan Moats <rmoats@us.ibm.com> wrote: > The various fixes in handling physical and binding changes > have addressed the problems that the original attempt to > incrementally process the multicast group table ran into. > So, re-add incremental processing of the multicast group > table. > > Notes: > - This patch renders the short term changes in [1] taken > for multicast group processing unnecessary (and so it > conflicts with [1]). > - Running through the ovn unit tests 50 times with -j4 > didn't show increased test case failures over the > baseline code prior to adding this patch. > > [1] http://patchwork.ozlabs.org/patch/652117/ > > Signed-off-by: Ryan Moats <rmoats@us.ibm.com> > short of nitpicks below... Acked-by: Flavio Fernandes <flavio@flaviof.com> > --- > ovn/controller/physical.c | 26 +++++++++++++++++++++++--- > 1 file changed, 23 insertions(+), 3 deletions(-) > > diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c > index a104e33..b10baed 100644 > --- a/ovn/controller/physical.c > +++ b/ovn/controller/physical.c > @@ -61,11 +61,13 @@ static struct hmap tunnels = > HMAP_INITIALIZER(&tunnels); > /* UUID to identify OF flows not associated with ovsdb rows. */ > static struct uuid *hc_uuid = NULL; > static bool full_binding_processing = false; > +static bool full_mcgroup_processing = false; > > void > physical_reset_processing(void) > { > full_binding_processing = true; > + full_mcgroup_processing = true; > } > > /* Maps from a chassis to the OpenFlow port number of the tunnel that can > be > @@ -675,6 +677,7 @@ physical_run(struct controller_ctx *ctx, enum > mf_field_id mff_ovn_geneve, > tun->ofport = u16_to_ofp(ofport); > tun->type = tunnel_type; > nitpick... > full_binding_processing = true; > + full_mcgroup_processing = true; > how about replacing the new line and the line above with: physical_reset_processing(); > binding_reset_processing(); > > /* Reprocess logical flow table immediately. */ > @@ -715,6 +718,7 @@ physical_run(struct controller_ctx *ctx, enum > mf_field_id mff_ovn_geneve, > } > if (localvif_map_changed) { > > full_binding_processing = true; > + full_mcgroup_processing = true; > same as previous comment: just call physical_reset_processing(); > > /* Reprocess logical flow table immediately. */ > lflow_reset_processing(); > @@ -751,9 +755,25 @@ physical_run(struct controller_ctx *ctx, enum > mf_field_id mff_ovn_geneve, > const struct sbrec_multicast_group *mc; > struct ofpbuf remote_ofpacts; > ofpbuf_init(&remote_ofpacts, 0); > - SBREC_MULTICAST_GROUP_FOR_EACH (mc, ctx->ovnsb_idl) { > - consider_mc_group(mff_ovn_geneve, ct_zones, > - local_datapaths, mc, &ofpacts, &remote_ofpacts); > + if (full_mcgroup_processing) { > + SBREC_MULTICAST_GROUP_FOR_EACH (mc, ctx->ovnsb_idl) { > + consider_mc_group(mff_ovn_geneve, ct_zones, > + local_datapaths, mc, &ofpacts, > &remote_ofpacts); > + } > + full_mcgroup_processing = false; > + } else { > + SBREC_MULTICAST_GROUP_FOR_EACH_TRACKED (mc, ctx->ovnsb_idl) { > + if (sbrec_multicast_group_is_deleted(mc)) { > + ofctrl_remove_flows(&mc->header_.uuid); > + } else { > + if (!sbrec_multicast_group_is_new(mc)) { > + ofctrl_remove_flows(&mc->header_.uuid); > + } > + consider_mc_group(mff_ovn_geneve, ct_zones, > + local_datapaths, mc, &ofpacts, > + &remote_ofpacts); > + } > + } > } > > ofpbuf_uninit(&remote_ofpacts); > -- > 1.9.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev >
Flaviof <flavio@flaviof.com> wrote on 07/24/2016 08:14:19 PM: > From: Flaviof <flavio@flaviof.com> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: ovs dev <dev@openvswitch.org> > Date: 07/24/2016 08:14 PM > Subject: Re: [ovs-dev] [PATCH] ovn-controller: Add incremental > processing for multicast groups > > On Sun, Jul 24, 2016 at 2:33 PM, Ryan Moats <rmoats@us.ibm.com> wrote: > The various fixes in handling physical and binding changes > have addressed the problems that the original attempt to > incrementally process the multicast group table ran into. > So, re-add incremental processing of the multicast group > table. > > Notes: > - This patch renders the short term changes in [1] taken > for multicast group processing unnecessary (and so it > conflicts with [1]). > - Running through the ovn unit tests 50 times with -j4 > didn't show increased test case failures over the > baseline code prior to adding this patch. > > [1] http://patchwork.ozlabs.org/patch/652117/ > > Signed-off-by: Ryan Moats <rmoats@us.ibm.com> > > short of nitpicks below... > > Acked-by: Flavio Fernandes <flavio@flaviof.com> > > > --- > ovn/controller/physical.c | 26 +++++++++++++++++++++++--- > 1 file changed, 23 insertions(+), 3 deletions(-) > > diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c > index a104e33..b10baed 100644 > --- a/ovn/controller/physical.c > +++ b/ovn/controller/physical.c > @@ -61,11 +61,13 @@ static struct hmap tunnels = HMAP_INITIALIZER (&tunnels); > /* UUID to identify OF flows not associated with ovsdb rows. */ > static struct uuid *hc_uuid = NULL; > static bool full_binding_processing = false; > +static bool full_mcgroup_processing = false; > > void > physical_reset_processing(void) > { > full_binding_processing = true; > + full_mcgroup_processing = true; > } > > /* Maps from a chassis to the OpenFlow port number of the tunnel that can be > @@ -675,6 +677,7 @@ physical_run(struct controller_ctx *ctx, enum > mf_field_id mff_ovn_geneve, > tun->ofport = u16_to_ofp(ofport); > tun->type = tunnel_type; > > nitpick... > > full_binding_processing = true; > + full_mcgroup_processing = true; > > how about replacing the new line and the line above with: > > physical_reset_processing(); physical_reset_processing is there for other modules to call... I'm pretty "meh" on trading two lines of code for a potential stack push and pop (depending on what the optimizer decides)...
On Sun, Jul 24, 2016 at 8:20 PM, Ryan Moats <rmoats@us.ibm.com> wrote: > Flaviof <flavio@flaviof.com> wrote on 07/24/2016 08:14:19 PM: > > > From: Flaviof <flavio@flaviof.com> > > To: Ryan Moats/Omaha/IBM@IBMUS > > Cc: ovs dev <dev@openvswitch.org> > > Date: 07/24/2016 08:14 PM > > Subject: Re: [ovs-dev] [PATCH] ovn-controller: Add incremental > > processing for multicast groups > > > > > On Sun, Jul 24, 2016 at 2:33 PM, Ryan Moats <rmoats@us.ibm.com> wrote: > > The various fixes in handling physical and binding changes > > have addressed the problems that the original attempt to > > incrementally process the multicast group table ran into. > > So, re-add incremental processing of the multicast group > > table. > > > > Notes: > > - This patch renders the short term changes in [1] taken > > for multicast group processing unnecessary (and so it > > conflicts with [1]). > > - Running through the ovn unit tests 50 times with -j4 > > didn't show increased test case failures over the > > baseline code prior to adding this patch. > > > > [1] http://patchwork.ozlabs.org/patch/652117/ > > > > Signed-off-by: Ryan Moats <rmoats@us.ibm.com> > > > > short of nitpicks below... > > > > Acked-by: Flavio Fernandes <flavio@flaviof.com> > > > > > > --- > > ovn/controller/physical.c | 26 +++++++++++++++++++++++--- > > 1 file changed, 23 insertions(+), 3 deletions(-) > > > > diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c > > index a104e33..b10baed 100644 > > --- a/ovn/controller/physical.c > > +++ b/ovn/controller/physical.c > > @@ -61,11 +61,13 @@ static struct hmap tunnels = > HMAP_INITIALIZER(&tunnels); > > /* UUID to identify OF flows not associated with ovsdb rows. */ > > static struct uuid *hc_uuid = NULL; > > static bool full_binding_processing = false; > > +static bool full_mcgroup_processing = false; > > > > void > > physical_reset_processing(void) > > { > > full_binding_processing = true; > > + full_mcgroup_processing = true; > > } > > > > /* Maps from a chassis to the OpenFlow port number of the tunnel that > can be > > @@ -675,6 +677,7 @@ physical_run(struct controller_ctx *ctx, enum > > mf_field_id mff_ovn_geneve, > > tun->ofport = u16_to_ofp(ofport); > > tun->type = tunnel_type; > > > > nitpick... > > > > full_binding_processing = true; > > + full_mcgroup_processing = true; > > > > how about replacing the new line and the line above with: > > > > physical_reset_processing(); > > physical_reset_processing is there for other modules to call... > > I'm pretty "meh" on trading two lines of code for a potential > stack push and pop (depending on what the optimizer decides)... > > Ack. Like I said, it is a 'nitpick'. :) Just not sure if a new variable is going to be needed later on; in which case, all you would need would be to add it to the function. -- flaviof
Ryan Moats/Omaha/IBM@IBMUS wrote on 07/24/2016 02:33:19 PM: > From: Ryan Moats/Omaha/IBM@IBMUS > To: dev@openvswitch.org > Cc: Ryan Moats/Omaha/IBM@IBMUS > Date: 07/24/2016 02:33 PM > Subject: [PATCH] ovn-controller: Add incremental processing for > multicast groups > > The various fixes in handling physical and binding changes > have addressed the problems that the original attempt to > incrementally process the multicast group table ran into. > So, re-add incremental processing of the multicast group > table. > > Notes: > - This patch renders the short term changes in [1] taken > for multicast group processing unnecessary (and so it > conflicts with [1]). > - Running through the ovn unit tests 50 times with -j4 > didn't show increased test case failures over the > baseline code prior to adding this patch. > > [1] http://patchwork.ozlabs.org/patch/652117/ > > Signed-off-by: Ryan Moats <rmoats@us.ibm.com> Please disregard this patch, it is being abandoned for the time being. Ryan
diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c index a104e33..b10baed 100644 --- a/ovn/controller/physical.c +++ b/ovn/controller/physical.c @@ -61,11 +61,13 @@ static struct hmap tunnels = HMAP_INITIALIZER(&tunnels); /* UUID to identify OF flows not associated with ovsdb rows. */ static struct uuid *hc_uuid = NULL; static bool full_binding_processing = false; +static bool full_mcgroup_processing = false; void physical_reset_processing(void) { full_binding_processing = true; + full_mcgroup_processing = true; } /* Maps from a chassis to the OpenFlow port number of the tunnel that can be @@ -675,6 +677,7 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, tun->ofport = u16_to_ofp(ofport); tun->type = tunnel_type; full_binding_processing = true; + full_mcgroup_processing = true; binding_reset_processing(); /* Reprocess logical flow table immediately. */ @@ -715,6 +718,7 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, } if (localvif_map_changed) { full_binding_processing = true; + full_mcgroup_processing = true; /* Reprocess logical flow table immediately. */ lflow_reset_processing(); @@ -751,9 +755,25 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, const struct sbrec_multicast_group *mc; struct ofpbuf remote_ofpacts; ofpbuf_init(&remote_ofpacts, 0); - SBREC_MULTICAST_GROUP_FOR_EACH (mc, ctx->ovnsb_idl) { - consider_mc_group(mff_ovn_geneve, ct_zones, - local_datapaths, mc, &ofpacts, &remote_ofpacts); + if (full_mcgroup_processing) { + SBREC_MULTICAST_GROUP_FOR_EACH (mc, ctx->ovnsb_idl) { + consider_mc_group(mff_ovn_geneve, ct_zones, + local_datapaths, mc, &ofpacts, &remote_ofpacts); + } + full_mcgroup_processing = false; + } else { + SBREC_MULTICAST_GROUP_FOR_EACH_TRACKED (mc, ctx->ovnsb_idl) { + if (sbrec_multicast_group_is_deleted(mc)) { + ofctrl_remove_flows(&mc->header_.uuid); + } else { + if (!sbrec_multicast_group_is_new(mc)) { + ofctrl_remove_flows(&mc->header_.uuid); + } + consider_mc_group(mff_ovn_geneve, ct_zones, + local_datapaths, mc, &ofpacts, + &remote_ofpacts); + } + } } ofpbuf_uninit(&remote_ofpacts);
The various fixes in handling physical and binding changes have addressed the problems that the original attempt to incrementally process the multicast group table ran into. So, re-add incremental processing of the multicast group table. Notes: - This patch renders the short term changes in [1] taken for multicast group processing unnecessary (and so it conflicts with [1]). - Running through the ovn unit tests 50 times with -j4 didn't show increased test case failures over the baseline code prior to adding this patch. [1] http://patchwork.ozlabs.org/patch/652117/ Signed-off-by: Ryan Moats <rmoats@us.ibm.com> --- ovn/controller/physical.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-)