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
