Looks right to me, Acked-by: Jarno Rajahalme <ja...@ovn.org>
> On Feb 23, 2017, at 1:31 PM, Andy Zhou <az...@ovn.org> wrote: > > When bond is removed or when its configuration changes, > the post recirculation rules that are installed by current > bond configuration, if any, should be also be removed. > > Reported-by: Huanle Han <hanxue...@gmail.com> > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-dev/2017-February/328969.html > CC: Huanle Han <hanxue...@gmail.com> > Signed-off-by: Andy Zhou <az...@ovn.org> > --- > ofproto/bond.c | 36 ++++++++++++++++++++++++++---------- > 1 file changed, 26 insertions(+), 10 deletions(-) > > diff --git a/ofproto/bond.c b/ofproto/bond.c > index 6e10c5143c0e..5bb124bda5ad 100644 > --- a/ofproto/bond.c > +++ b/ofproto/bond.c > @@ -190,6 +190,7 @@ static struct bond_slave *choose_output_slave(const > struct bond *, > struct flow_wildcards *, > uint16_t vlan) > OVS_REQ_RDLOCK(rwlock); > +static void update_recirc_rules__(struct bond *bond); > > /* Attempts to parse 's' as the name of a bond balancing mode. If successful, > * stores the mode in '*balance' and returns true. Otherwise returns false > @@ -264,7 +265,6 @@ bond_ref(const struct bond *bond_) > void > bond_unref(struct bond *bond) > { > - struct bond_pr_rule_op *pr_op; > struct bond_slave *slave; > > if (!bond || ovs_refcount_unref_relaxed(&bond->ref_cnt) != 1) { > @@ -283,18 +283,18 @@ bond_unref(struct bond *bond) > hmap_destroy(&bond->slaves); > > ovs_mutex_destroy(&bond->mutex); > - free(bond->hash); > - free(bond->name); > - > - HMAP_FOR_EACH_POP (pr_op, hmap_node, &bond->pr_rule_ops) { > - free(pr_op); > - } > - hmap_destroy(&bond->pr_rule_ops); > > + /* Free bond resources. Remove existing post recirc rules. */ > if (bond->recirc_id) { > recirc_free_id(bond->recirc_id); > + bond->recirc_id = 0; > } > + free(bond->hash); > + bond->hash = NULL; > + update_recirc_rules__(bond); > > + hmap_destroy(&bond->pr_rule_ops); > + free(bond->name); > free(bond); > } > > @@ -322,9 +322,17 @@ add_pr_rule(struct bond *bond, const struct match *match, > hmap_insert(&bond->pr_rule_ops, &pr_op->hmap_node, hash); > } > > +/* This function should almost never be called directly. > + * 'update_recirc_rules()' should be called instead. Since > + * this function modifies 'bond->pr_rule_ops', it is only > + * safe when 'rwlock' is held. > + * > + * However, when the 'bond' is the only reference in the system, > + * calling this function avoid acquiring lock only to satisfy > + * lock annotation. Currently, only 'bond_unref()' calls > + * this function directly. */ > static void > -update_recirc_rules(struct bond *bond) > - OVS_REQ_WRLOCK(rwlock) > +update_recirc_rules__(struct bond *bond) > { > struct match match; > struct bond_pr_rule_op *pr_op, *next_op; > @@ -394,6 +402,12 @@ update_recirc_rules(struct bond *bond) > ofpbuf_uninit(&ofpacts); > } > > +static void > +update_recirc_rules(struct bond *bond) > + OVS_REQ_RDLOCK(rwlock) > +{ > + update_recirc_rules__(bond); > +} > > /* Updates 'bond''s overall configuration to 's'. > * > @@ -1640,6 +1654,8 @@ bond_entry_reset(struct bond *bond) > } else { > free(bond->hash); > bond->hash = NULL; > + /* Remove existing post recirc rules. */ > + update_recirc_rules(bond); > } > } > > -- > 1.9.1 > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
