Thanks Ben and Ryan for the review. All 4 patches applied.
On Thu, May 22, 2014 at 11:16 AM, Ryan Wilson 76511 <wr...@vmware.com> wrote:
> This is a much cleaner improvement, thanks!
>
> Acked-by: Ryan Wilson <wr...@nicira.com>
>
> On 5/22/14 11:04 AM, "Andy Zhou" <az...@nicira.com> wrote:
>
>>ofproto_group_write_lookup() slightly different from
>>ofproto_group_lookup() in handling reference counting.
>>Currently, it has only one caller: modify_group().
>>It seems the abstraction is not adding value here.
>>
>>Remove the function, along with some refactoring, makes modify_group()
>>easier to understand.
>>
>>Signed-off-by: Andy Zhou <az...@nicira.com>
>>---
>> ofproto/ofproto.c | 45 ++++++++++++++++++++++-----------------------
>> 1 file changed, 22 insertions(+), 23 deletions(-)
>>
>>diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>>index 7952984..7086960 100644
>>--- a/ofproto/ofproto.c
>>+++ b/ofproto/ofproto.c
>>@@ -5352,40 +5352,38 @@ handle_meter_request(struct ofconn *ofconn, const
>>struct ofp_header *request,
>> return 0;
>> }
>>
>>-/* If the group exists, this function increments the groups's reference
>>count.
>>- *
>>- * Make sure to call ofproto_group_unref() after no longer needing to
>>maintain
>>- * a reference to the group. */
>>-bool
>>-ofproto_group_lookup(const struct ofproto *ofproto, uint32_t group_id,
>>- struct ofgroup **group)
>>+static bool
>>+ofproto_group_lookup__(const struct ofproto *ofproto, uint32_t group_id,
>>+ struct ofgroup **group)
>>+ OVS_REQ_RDLOCK(ofproto->groups_rwlock)
>> {
>>- ovs_rwlock_rdlock(&ofproto->groups_rwlock);
>> HMAP_FOR_EACH_IN_BUCKET (*group, hmap_node,
>> hash_int(group_id, 0), &ofproto->groups) {
>> if ((*group)->group_id == group_id) {
>>- ofproto_group_ref(*group);
>>- ovs_rwlock_unlock(&ofproto->groups_rwlock);
>> return true;
>> }
>> }
>>- ovs_rwlock_unlock(&ofproto->groups_rwlock);
>>+
>> return false;
>> }
>>
>>-static bool
>>-ofproto_group_write_lookup(const struct ofproto *ofproto, uint32_t
>>group_id,
>>- struct ofgroup **group)
>>- OVS_ACQUIRES(ofproto->groups_rwlock)
>>+/* If the group exists, this function increments the groups's reference
>>count.
>>+ *
>>+ * Make sure to call ofproto_group_unref() after no longer needing to
>>maintain
>>+ * a reference to the group. */
>>+bool
>>+ofproto_group_lookup(const struct ofproto *ofproto, uint32_t group_id,
>>+ struct ofgroup **group)
>> {
>>- ovs_rwlock_wrlock(&ofproto->groups_rwlock);
>>- HMAP_FOR_EACH_IN_BUCKET (*group, hmap_node,
>>- hash_int(group_id, 0), &ofproto->groups) {
>>- if ((*group)->group_id == group_id) {
>>- return true;
>>- }
>>+ bool found;
>>+
>>+ ovs_rwlock_rdlock(&ofproto->groups_rwlock);
>>+ found = ofproto_group_lookup__(ofproto, group_id, group);
>>+ if (found) {
>>+ ofproto_group_ref(*group);
>> }
>>- return false;
>>+ ovs_rwlock_unlock(&ofproto->groups_rwlock);
>>+ return found;
>> }
>>
>> static bool
>>@@ -5712,7 +5710,8 @@ modify_group(struct ofproto *ofproto, struct
>>ofputil_group_mod *gm)
>>
>> retiring = new_ofgroup;
>>
>>- if (!ofproto_group_write_lookup(ofproto, gm->group_id, &ofgroup)) {
>>+ ovs_rwlock_wrlock(&ofproto->groups_rwlock);
>>+ if (!ofproto_group_lookup__(ofproto, gm->group_id, &ofgroup)) {
>> error = OFPERR_OFPGMFC_UNKNOWN_GROUP;
>> goto out;
>> }
>>--
>>1.9.1
>>
>>_______________________________________________
>>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=TfBS78Vw3dzttvXidhbffg%
>>3D%3D%0A&m=zh8wMPw9xcbpj4PL38Ny0FxIVZca52l1YMLz2Bd8XTg%3D%0A&s=5cbf10fbd63
>>99c57836e3dcdba2ac6c567f16aaba5a2658c52298cc2ecc21e89
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
