On Wed, Sep 04, 2013 at 12:37:36PM -0700, Ethan Jackson wrote:
> There are a few fields in struct rule which are accessible by
> functions in ofproto-dpif and therefore need to accessed in a thread
> safe manner. This patch achieves this by generalizing the rules evict
> rwlock and requiring a writelock to be held to edit them.
>
> Signed-off-by: Ethan Jackson <et...@nicira.com>
> struct rule {
> struct list ofproto_node; /* Owned by ofproto base code. */
> struct ofproto *ofproto; /* The ofproto that contains this rule. */
I wonder whether it makes sense to speak of 'cr' as being "guarded" by
anything. Most of the data that its users really look at, notably
'priority' and 'match', is immutable. It is inserted into a
classifier at construction time, and removed at destruction time, and
other changes to the classifier can change 'hmap_node' and 'list', but
few of the users really care:
> - struct cls_rule cr; /* In owning ofproto's classifier. */
> + struct cls_rule cr; /* In owning ofproto's classifier. Guarded
> by
> + rwlock. */
>
> struct ofoperation *pending; /* Operation now in progress, if nonnull. */
Marking 'flow_cookie' as OVS_GUARDED, as I imagine it should be, makes
tons of new Clang warnings pop up:
> - ovs_be64 flow_cookie; /* Controller-issued identifier. */
> + ovs_be64 flow_cookie; /* Controller-issued identifier. Guarded by
> + rwlock. */
> struct hindex_node cookie_node; /* In owning ofproto's 'cookies' index.
> */
>
> long long int created; /* Creation time. */
The following comment needs proofreading, I see a number of typos:
> + /* The rwlock is used to protect those elements in struct rule which are
> + * accessed by multiple threads. While maintaing a pointer to struct
> rule,
> + * threads are requires to hold a readlock. The main ofproto code is
> + * guarnated not to evict the rule, or change any of the elements
> "Guarded
> + * by rwlock" without holding the writelock.
> + *
> + * A rule will not be evicted unless both its own and its classifier's
> + * write locks are held. Therefore, while holding a classifier readlock,
> + * one can be assured that ewrite locked rules are safe to reference. */
> + struct ovs_rwlock rwlock;
Lots of warnings if I add OVS_GUARDED to the following:
> + /* Guarded by rwlock. */
> struct ofpact *ofpacts; /* Sequence of "struct ofpacts". */
> unsigned int ofpacts_len; /* Size of 'ofpacts', in bytes. */
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
