On Wed, May 07, 2014 at 04:58:59PM +1200, Simon Horman wrote:
> Hi Ben,
>
> On Fri, May 02, 2014 at 07:34:58AM -0700, Ben Pfaff wrote:
> > On Fri, May 02, 2014 at 05:41:35PM +0900, Simon Horman wrote:
> > > This is to allow a recirculation id to be associated with a rule
> > > in the case that its actions cause recirculation.
> > >
> > > In such a case if the recirc_id field is non-zero then that value should
> > > be
> > > used, otherwise a value should be obtained using
> > > ofproto_dpif_alloc_recirc_id and saved in recirc_id field.
> > >
> > > When destructing the rule if the recirc_id field is non-zero then
> > > the associated internal flow should be deleted.
> > >
> > > This is in preparation for using the same helper as part of support
> > > for using recirculation in conjunction series of actions including
> > > with MPLS actions that are currently not able to be translated.
> > >
> > > Signed-off-by: Simon Horman <[email protected]>
> >
> > Has this been tested? It appears that rule_dpif_get_recirc_id()
> > requires rule->up.mutex:
>
> I have exercised the code that uses this code.
> But not specifically the locking portions.
> I will look into this.
Sorry for the delay, I have been on vacation.
> > > +uint32_t
> > > +rule_dpif_get_recirc_id(struct rule_dpif *rule)
> > > + OVS_REQUIRES(rule->up.mutex)
> > > +{
> > > + if (!rule->recirc_id) {
> > > + struct ofproto_dpif *ofproto =
> > > ofproto_dpif_cast(rule->up.ofproto);
> > > +
> > > + rule_dpif_set_recirc_id(rule,
> > > ofproto_dpif_alloc_recirc_id(ofproto));
> > > + }
> > > + return rule->recirc_id;
> > > +}
> >
> > but that rule_dpif_get_recirc_id() sometimes calls rule_set_recirc_id(),
> > which attempts to acquire rule->up.mutex:
rule_set_recirc_id() attempts to acquire rule->up.mutex.
However, (the confusingly named) rule_dpif_set_recirc_id()
requires rather than attempting to acquire that lock.
/* Sets 'rule''s recirculation id. */
static void
rule_dpif_set_recirc_id(struct rule_dpif *rule, uint32_t id)
OVS_REQUIRES(rule->up.mutex)
{
ovs_assert(!rule->recirc_id);
rule->recirc_id = id;
}
> >
> > > +/* Sets 'rule''s recirculation id. */
> > > +void
> > > +rule_set_recirc_id(struct rule *rule_, uint32_t id)
> > > +{
> > > + struct rule_dpif *rule = rule_dpif_cast(rule_);
> > > +
> > > + ovs_mutex_lock(&rule->up.mutex);
> > > + rule_dpif_set_recirc_id(rule, id);
> > > + ovs_mutex_unlock(&rule->up.mutex);
> > > +}
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
