On Wed, Jan 8, 2014 at 3:57 PM, Ben Pfaff <b...@nicira.com> wrote:
> On Wed, Jan 08, 2014 at 03:32:25PM -0800, Pravin Shelar wrote:
>> On Fri, Dec 27, 2013 at 8:03 PM, Ben Pfaff <b...@nicira.com> wrote:
>> > This will allow for parallelism in multithreaded forwarding in an upcoming
>> > commit.
>> >
>> > Signed-off-by: Ben Pfaff <b...@nicira.com>
>
> ...
>
>> > +static struct dp_netdev_flow *
>> > +dp_netdev_flow_ref(const struct dp_netdev_flow *flow_)
>> > +{
>> > + struct dp_netdev_flow *flow = CONST_CAST(struct dp_netdev_flow *,
>> > flow_);
>> > + if (flow) {
>> > + ovs_refcount_ref(&flow->ref_cnt);
>> > + }
>> > + return flow;
>> > +}
>> > +
>> Can we have lock annotations for taking ref on action?
>
> I think that we could, but I don't think it would be very useful
> because these references aren't lexically balanced in one of the three
> callers. That is, dpif_netdev_flow_dump_next() returns holding a
> reference, intentionally, which only next the next call to that
> function or to dpif_netdev_flow_dump_done() will release.
>
>> > +static void
>> > +dp_netdev_flow_unref(struct dp_netdev_flow *flow)
>> > +{
>> > + if (flow && ovs_refcount_unref(&flow->ref_cnt) == 1) {
>> > + cls_rule_destroy(CONST_CAST(struct cls_rule *, &flow->cr));
>> > + ovs_mutex_lock(&flow->mutex);
>> > + dp_netdev_actions_unref(flow->actions);
>> > + ovs_mutex_unlock(&flow->mutex);
>>
>> do we really need to take lock here ?
>
> No. Clang complains if we don't, because of the lock annotations:
>
> ../lib/dpif-netdev.c:851:39: error: reading variable 'actions' requires
> locking any mutex [-Werror,-Wthread-safety-analysis]
> dp_netdev_actions_unref(flow->actions);
>
> At some point we probably will want to introduce some "fake locking"
> functions that are no-ops but satisfy Clang, but we haven't done that
> yet and our current practice is to just take the locks anyway.
>
>> > @@ -931,20 +1101,31 @@ dpif_netdev_flow_get(const struct dpif *dpif,
>> > return error;
>> > }
>> >
>> > - ovs_mutex_lock(&dp_netdev_mutex);
>> > + ovs_rwlock_rdlock(&dp->cls.rwlock);
>> > netdev_flow = dp_netdev_find_flow(dp, &key);
>> > + ovs_rwlock_unlock(&dp->cls.rwlock);
>> > +
>>
>> This needs dp->flow_mutex, since it does lookup flow_table. or
>> hmap_insert locking needs fix.
>
> Thanks.
>
> The comments show that I intended for dp->cls.rwlock to protect both
> dp->cls and dp->flow_table, so in dp_netdev_flow_add() I moved the
> hmap_insert() inside the existing block that holds dp->cls.rwlock.
>
>> It will be less confusing if classifier and flow table lookup
>> functions are named explicitly.
>
> Do you have a suggestion? I have spent too much time looking at this
> code, myself.
>
>> > + ovs_mutex_init(&netdev_flow->mutex);
>> > + ovs_mutex_lock(&netdev_flow->mutex);
>> > +
>> > netdev_flow->actions = dp_netdev_actions_create(actions, actions_len);
>> >
>> > match_init(&match, flow, wc);
>> > - cls_rule_init(&netdev_flow->cr, &match, NETDEV_RULE_PRIORITY);
>> > + cls_rule_init(CONST_CAST(struct cls_rule *, &netdev_flow->cr),
>> > + &match, NETDEV_RULE_PRIORITY);
>> > ovs_rwlock_wrlock(&dp->cls.rwlock);
>> > - classifier_insert(&dp->cls, &netdev_flow->cr);
>> > + classifier_insert(&dp->cls,
>> > + CONST_CAST(struct cls_rule *, &netdev_flow->cr));
>> > ovs_rwlock_unlock(&dp->cls.rwlock);
>> >
>> > - hmap_insert(&dp->flow_table, &netdev_flow->node, flow_hash(flow, 0));
>> > + ovs_mutex_unlock(&netdev_flow->mutex);
>> > +
>> flow->mutex lock can be released once actions pointer is set, right?
>> or does it protect something more?
>> At this point no one can have access to this flow, so this locking
>> must be for sanity checker.
>
> Yes, all it us protects us against, at this point, is Clang warnings.
> Again, I guess we should work on that, and I'm sure we will at some
> point.
>
>> > +
>> > +exit:
>> > + ovs_rwlock_unlock(&dp->port_rwlock);
>> > + dp_netdev_unref(dp);
>> > }
>> >
>> > static void
>>
>> How much does separate flow_mutex lock helps over just cls.rwlock?
>
> I have not measured the advantage, but the idea is that cls.rwlock
> should be held for writing as briefly as possible, because it delays
> packet forwarding. However, we need some way to ensure that writers
> (and there might be several threads trying to write) can prevent the
> flow table from changing as they check for some property and then act
> based on it (e.g. add a flow only if one doesn't already exist), so
> flow_mutex allows for that without delaying forwarding threads.
Thanks for explanation. make sense.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
