Ben Pfaff <b...@ovn.org> wrote on 06/02/2016 07:02:29 PM:
> From: Ben Pfaff <b...@ovn.org>
> To: Ryan Moats/Omaha/IBM@IBMUS
> Cc: dev@openvswitch.org
> Date: 06/02/2016 07:02 PM
> Subject: Re: [ovs-dev,v17,2/5] Convert binding_run to incremental
processing.
>
> On Sun, May 22, 2016 at 04:36:19PM -0500, Ryan Moats wrote:
> > Ensure that the entire port binding table is processed
> > when chassis are added/removed or when get_local_iface_ids
> > finds new ports on the local vswitch.
> >
> > Side effects:
> > - Persist local_datapaths and patch_datapaths across runs so
> > that changes to either can be used as a trigger to reset
> > incremental flow processing.
> > - Persist all_lports structure
> > - Revert commit 9baaabfff3c7df014e9acbd4c68189b568552ca9
>
> In a commit message, please give the title of the commit in addition to
> the commit number. (Then, if for some reason we move to a rebased repo
> later, or switch away from Git (!), it is still possible to find
> commits.)
So noted
>
> > as these changes are not desirable once local_datatpath
> > is persisted.
> >
> > Signed-off-by: Ryan Moats <rmo...@us.ibm.com>
>
> Thanks for the revision!
>
> "sparse" pointed out that local_datapaths_by_uuid should be static:
> ../ovn/controller/binding.c:140:13: warning: symbol
> 'local_datapaths_by_uuid' was not declared. Should it be static?
>
> s/datpath/datapath/ in this comment in ovn-controller.c:
> /* Contains "struct local_datpath" nodes whose hash values are the
> * tunnel_key of datapaths with at least one local port binding. */
Ugh, I thought I caught all of these - back to double checking again.
> main() calls hmap_clear() on a couple of hmaps. Doesn't it need to
> iterate through and free their elements first? Could it alternatively
> just call reset_binding_processing()?
That smells like something else is going on - let me revisit ...
> The OVS name prefix convention would be better served with the name
> binding_reset_processing().
Sure, I can apply that across the patch set series
> Same question as in patch 1 about potential memory leak here in
> binding.c:
> + HMAP_FOR_EACH_POP(old_ld, hmap_node,
&keep_local_datapath_by_uuid) {
> + ;
> + }
> + hmap_destroy(&keep_local_datapath_by_uuid);
>
Ack - as I said in my previous review - that is a common
potential anti-pattern
> consider_local_datapath() is indented 4 spaces too far.
>
> The following changes weren't immediately obvious to me, can you hint
> about why they are needed?
>
> > diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
> > index 4808146..e07e829 100644
> > --- a/ovn/controller/patch.c
> > +++ b/ovn/controller/patch.c
> > @@ -185,7 +185,8 @@ add_bridge_mappings(struct controller_ctx *ctx,
> > * to create patch ports for it. */
> > continue;
> > }
> > - if (ld->localnet_port) {
> > + if (ld->localnet_port && strcmp
> (ld->localnet_port->logical_port,
> > + binding->logical_port)) {
> > static struct vlog_rate_limit rl =
> VLOG_RATE_LIMIT_INIT(5, 1);
> > VLOG_WARN_RL(&rl, "localnet port '%s' already set
> for datapath "
> > "'%"PRId64"', skipping the new port
'%s'.",
When I was running through the logic, I came across cases where I could
re-enter this block with a logical port that had already been recorded
in binding->logical_port previously. Rather than emit spurious warnings,
I added the check on the logical_port name. I'm happy to add a comment
to that effect in the next respin.
> > diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
> > index 576c695..d1e8479 100644
> > --- a/ovn/controller/physical.c
> > +++ b/ovn/controller/physical.c
> > @@ -310,7 +310,7 @@ physical_run(struct controller_ctx *ctx, enum
> mf_field_id mff_ovn_geneve,
> > if (!binding->chassis) {
> > continue;
> > }
> > - if (localnet_port) {
> > + if (localnet_port && localnet_port->logical_port) {
> > ofport = u16_to_ofp(simap_get(&localvif_to_ofport,
> >
> localnet_port->logical_port));
> > if (!ofport) {
The issue I was running into was getting localnet_port structures with
the logical_port pointer being invalidated and this was a check to make
sure I didn't dump core. However I ended up reverting the change that
caused this problem, so I think *this* one can go away. I'll double
check and if it can't, I'll add a comment to that effect.
Thanks,
Ryan
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
