Some inline comments below. The only thing I'm a bit concerned about it is
the constant cast issue, especially since created / modified are actually
altered in modify_group().
Also, the problem is the struct-defined variables are constant, so
CONST_CAST won't work here. (CONST_CAST works for converting constant
variables to non-constant variables). Obviously, I could initialize a
pointer to the constant variable and overwrite the pointer's value in
init_group. But this will make the code somewhat ugly for a minor gain.
>On Wed, May 21, 2014 at 2:14 PM, Ryan Wilson <wr...@nicira.com> wrote:
>> When adding support for OpenFlow group and bucket stats, a group entry
>>is added
>> to the xlate_cache. If the main thread removes the group from an
>>ofproto, we
>> need to guarantee that the group remains accessible to users of
>> xlate_group_actions and the xlate_cache, as the xlate_cache will not be
>>cleaned
>> up until the corresponding datapath flows are revalidated.
>>
>> To make modify_group compatible with group reference counting, the
>>group is
>> re-created and then replaces the old group in ofproto's ofgroup hash
>>map. Thus,
>> the group is never altered while users of the xlate module hold a
>>pointer to the
>> group. This also eliminates the need for ofgroup's lock as all
>>properties of
>> ofgroup are read-only.
>>
>> Signed-off-by: Ryan Wilson <wr...@nicira.com>
>>
>> ---
>
>Look good at high level. Comments inline:
>
>
>> static inline void rule_dpif_ref(struct rule_dpif *rule)
>> {
>> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
>> index 141adec..e2af522 100644
>> --- a/ofproto/ofproto-provider.h
>> +++ b/ofproto/ofproto-provider.h
>> @@ -474,20 +474,22 @@ bool ofoperation_has_out_port(const struct
>>ofoperation *, ofp_port_t out_port)
>> * With few exceptions, ofproto implementations may look at these
>>fields but
>> * should not modify them. */
>> struct ofgroup {
>> - /* The rwlock is used to prevent groups from being deleted while
>>child
>> - * threads are using them to xlate flows. A read lock means the
>> - * group is currently being used. A write lock means the group is
>> - * in the process of being deleted or updated. Note that since
>> - * a read lock on the groups container is held while searching, and
>> - * a group is ever write locked only while holding a write lock
>> - * on the container, the user's of groups will never face a group
>> - * in the write locked state. */
>> - struct ovs_rwlock rwlock OVS_ACQ_AFTER(ofproto_mutex);
>> struct hmap_node hmap_node; /* In struct ofproto's "groups" hmap.
>>*/
>Add OVS_GUARDED ?
Done.
>> struct ofproto *ofproto; /* The ofproto that contains this
>>group. */
>> uint32_t group_id;
>> enum ofp11_group_type type; /* One of OFPGT_*. */
>>
>> + /* Number of references.
>> + *
>> + * This is needed to keep track of references to the group in the
>>xlate
>> + * module.
>> + *
>> + * If the main thread removes the group from an ofproto, we need to
>> + * guarantee that the group remains accessible to users of
>> + * xlate_group_actions and the xlate_cache, as the xlate_cache
>>will not be
>> + * cleaned up until the corresponding datapath flows are
>>revalidated. */
>> + struct ovs_refcount ref_count;
>How about move it right below hmap_node, it now protects all the fields
>below.
>The comment above needs update about we don't change the fields once
>constructed.
Well, the ref_count doesn't really protect the fields. It moreso delays
when the object will be freed. But I did add a comment saying all
properties are constant after construction so there is no need for a lock.
>
>> +
>> long long int created; /* Creation time. */
>> long long int modified; /* Time of last modification. */
>All fields above till ofproto (except ref_count) can be made const to
>make sure they
>are not being updated during runtime,init_group() can use CONST_CAST
>to initialize them.
>>
>> @@ -496,11 +498,10 @@ struct ofgroup {
>> };
>>
>> +
>> +void
>> +ofproto_group_unref(struct ofgroup *group)
>> +{
>> + if (group && ovs_refcount_unref(&group->ref_count) == 1) {
>> + ovsrcu_postpone(group_destroy_cb, group);
>Why do we need ovsrcu_postpone? It seems we can just do the destroy in
>place.
>> + }
>> +}
>> +
>> static void
>> append_group_stats(struct ofgroup *group, struct list *replies)
>> - OVS_REQ_RDLOCK(group->rwlock)
>> {
>> struct ofputil_group_stats ogs;
>> struct ofproto *ofproto = group->ofproto;
>> @@ -5476,15 +5494,15 @@ handle_group_request(struct ofconn *ofconn,
>> if (group_id == OFPG_ALL) {
>> ovs_rwlock_rdlock(&ofproto->groups_rwlock);
>> HMAP_FOR_EACH (group, hmap_node, &ofproto->groups) {
>> - ovs_rwlock_rdlock(&group->rwlock);
>> + ofproto_group_ref(group);
>> cb(group, &replies);
>> - ovs_rwlock_unlock(&group->rwlock);
>> + ofproto_group_unref(group);
>Do we need ofproto_group_ref()/unref() here? Not sure why group would
>disappear while holding the read lock.
>>
Right, fixed it.
>
>>
>> +static void
>> +replace_group(struct ofproto *ofproto, struct ofgroup *ofgroup,
>> + struct ofgroup *new_ofgroup)
>It can use a lock annotation for the function. Personally, I think
>putting the logic
>in modify_group() makes it more readable.
Put the logic in modify_group as suggested.
>> +{
>> + hmap_remove(&ofproto->groups, &ofgroup->hmap_node);
>> + hmap_insert(&ofproto->groups, &new_ofgroup->hmap_node,
>> + hash_int(new_ofgroup->group_id, 0));
>> + if (ofgroup->type != new_ofgroup->type) {
>> + ofproto->n_groups[ofgroup->type]--;
>> + ofproto->n_groups[new_ofgroup->type]++;
>> + }
>> + ofproto_group_unref(ofgroup);
>Will this cause a unref be called twice?
Originally, I took a ref to the group in ofproto_group_write_lookup for
consistency, but since the write lock for ofproto's groups is being held,
I think its unnecessary. So I removed the unref from that function, so I
don't have to call it here.
>> +}
>> +
>> /* Implements OFPFC_MODIFY. Returns 0 on success or an OpenFlow error
>>code on
>> * failure.
>> *
>> + * Note that the group is re-created and then replaces the old group in
>> + * ofproto's ofgroup hash map. Thus, the group is never altered while
>>users of
>> + * the xlate module hold a pointer to the group.
>> + *
>> * 'ofconn' is used to retrieve the packet buffer specified in
>>fm->buffer_id,
>> * if any. */
>> static enum ofperr
>> modify_group(struct ofproto *ofproto, struct ofputil_group_mod *gm)
>> -
>> - error = ofproto->ofproto_class->group_modify(ofgroup, victim);
>> - if (!error) {
>> - ofputil_bucket_list_destroy(&victim->buckets);
>> - ofproto->n_groups[victim->type]--;
>> - ofproto->n_groups[ofgroup->type]++;
>> - ofgroup->modified = time_msec();
>> + ofproto_group_unref(new_ofgroup);
>If we fall into this error path, should we _not_ unref the existing
>ofproto?
Forgot about this error path, fixed it.
>> } else {
>> - ofputil_bucket_list_destroy(&ofgroup->buckets);
>> + /* The group creation time does not change during
>>modification. */
>> + new_ofgroup->created = ofgroup->created;
>>
>Should we also update the modified time stamp here?
Yup, added it.
>_______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>>
>>https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman
>>/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=TfBS78Vw3dzttvXidhbff
>>g%3D%3D%0A&m=8AEC%2Bt0UxqRpHBa6a%2FgOOCByqV0yQV9RMIKqjdmT9LI%3D%0A&s=02b1
>>7a6b38717efa436c9da146cb1e7a1e9c13d4ace0c0dccf277af8261e068e
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
