From patchwork Fri Jul 15 10:19:08 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jarno Rajahalme X-Patchwork-Id: 648746 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from archives.nicira.com (archives.nicira.com [96.126.127.54]) by ozlabs.org (Postfix) with ESMTP id 3rrT9409p9z9s9N for ; Fri, 15 Jul 2016 20:20:23 +1000 (AEST) Received: from archives.nicira.com (localhost [127.0.0.1]) by archives.nicira.com (Postfix) with ESMTP id 9D8AD10C98; Fri, 15 Jul 2016 03:20:02 -0700 (PDT) X-Original-To: dev@openvswitch.org Delivered-To: dev@openvswitch.org Received: from mx3v3.cudamail.com (mx3.cudamail.com [64.34.241.5]) by archives.nicira.com (Postfix) with ESMTPS id 240A310C79 for ; Fri, 15 Jul 2016 03:20:00 -0700 (PDT) Received: from bar6.cudamail.com (localhost [127.0.0.1]) by mx3v3.cudamail.com (Postfix) with ESMTPS id AF58C163042 for ; Fri, 15 Jul 2016 04:19:59 -0600 (MDT) X-ASG-Debug-ID: 1468577998-0b3237207a01e70001-byXFYA Received: from mx1-pf1.cudamail.com ([192.168.24.1]) by bar6.cudamail.com with ESMTP id orWX8j2j67VggHJs (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO) for ; Fri, 15 Jul 2016 04:19:58 -0600 (MDT) X-Barracuda-Envelope-From: jarno@ovn.org X-Barracuda-RBL-Trusted-Forwarder: 192.168.24.1 Received: from unknown (HELO relay4-d.mail.gandi.net) (217.70.183.196) by mx1-pf1.cudamail.com with ESMTPS (DHE-RSA-AES256-SHA encrypted); 15 Jul 2016 10:19:57 -0000 Received-SPF: pass (mx1-pf1.cudamail.com: SPF record at ovn.org designates 217.70.183.196 as permitted sender) X-Barracuda-Apparent-Source-IP: 217.70.183.196 X-Barracuda-RBL-IP: 217.70.183.196 Received: from mfilter35-d.gandi.net (mfilter35-d.gandi.net [217.70.178.166]) by relay4-d.mail.gandi.net (Postfix) with ESMTP id 5CF8E1720D1; Fri, 15 Jul 2016 12:19:55 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at mfilter35-d.gandi.net Received: from relay4-d.mail.gandi.net ([IPv6:::ffff:217.70.183.196]) by mfilter35-d.gandi.net (mfilter35-d.gandi.net [::ffff:10.0.15.180]) (amavisd-new, port 10024) with ESMTP id xGL_z04r7AOb; Fri, 15 Jul 2016 12:19:53 +0200 (CEST) X-Originating-IP: 85.76.8.181 Received: from sc9-mailhost2.vmware.com (85-76-8-181-nat.elisa-mobile.fi [85.76.8.181]) (Authenticated sender: jarno@ovn.org) by relay4-d.mail.gandi.net (Postfix) with ESMTPSA id AECF81720E7; Fri, 15 Jul 2016 12:19:52 +0200 (CEST) X-CudaMail-Envelope-Sender: jarno@ovn.org From: Jarno Rajahalme To: dev@openvswitch.org X-CudaMail-Whitelist-To: dev@openvswitch.org X-CudaMail-MID: CM-E1-714004429 X-CudaMail-DTE: 071516 X-CudaMail-Originating-IP: 217.70.183.196 Date: Fri, 15 Jul 2016 03:19:08 -0700 X-ASG-Orig-Subj: [##CM-E1-714004429##][PATCH 02/13] ofproto: Lockless group lookups. Message-Id: <1468577959-98487-2-git-send-email-jarno@ovn.org> X-Mailer: git-send-email 2.1.4 In-Reply-To: <1468577959-98487-1-git-send-email-jarno@ovn.org> References: <1468577959-98487-1-git-send-email-jarno@ovn.org> X-Barracuda-Connect: UNKNOWN[192.168.24.1] X-Barracuda-Start-Time: 1468577998 X-Barracuda-Encrypted: ECDHE-RSA-AES256-GCM-SHA384 X-Barracuda-URL: https://web.cudamail.com:443/cgi-mod/mark.cgi X-ASG-Whitelist: Header =?UTF-8?B?eFwtY3VkYW1haWxcLXdoaXRlbGlzdFwtdG8=?= X-Barracuda-BRTS-Status: 1 X-Virus-Scanned: by bsmtpd at cudamail.com Subject: [ovs-dev] [PATCH 02/13] ofproto: Lockless group lookups. X-BeenThere: dev@openvswitch.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: dev-bounces@openvswitch.org Sender: "dev" Make groups RCU protected and make group lookups lockless. Signed-off-by: Jarno Rajahalme --- ofproto/ofproto-dpif-xlate.c | 22 +++--- ofproto/ofproto-dpif.c | 27 +++---- ofproto/ofproto-dpif.h | 7 +- ofproto/ofproto-provider.h | 12 ++-- ofproto/ofproto.c | 165 +++++++++++++++++++++---------------------- tests/ofproto.at | 16 ++--- 6 files changed, 119 insertions(+), 130 deletions(-) diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index fee11aa..39b4bc9 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -1502,7 +1502,8 @@ group_is_alive(const struct xlate_ctx *ctx, uint32_t group_id, int depth) { struct group_dpif *group; - if (group_dpif_lookup(ctx->xbridge->ofproto, group_id, &group)) { + group = group_dpif_lookup(ctx->xbridge->ofproto, group_id); + if (group) { struct ofputil_bucket *bucket; bucket = group_first_live_bucket(ctx, group, depth); @@ -1541,7 +1542,7 @@ group_first_live_bucket(const struct xlate_ctx *ctx, struct ofputil_bucket *bucket; const struct ovs_list *buckets; - group_dpif_get_buckets(group, &buckets); + buckets = group_dpif_get_buckets(group); LIST_FOR_EACH (bucket, list_node, buckets) { if (bucket_is_alive(ctx, bucket, depth)) { return bucket; @@ -1562,7 +1563,7 @@ group_best_live_bucket(const struct xlate_ctx *ctx, struct ofputil_bucket *bucket; const struct ovs_list *buckets; - group_dpif_get_buckets(group, &buckets); + buckets = group_dpif_get_buckets(group); LIST_FOR_EACH (bucket, list_node, buckets) { if (bucket_is_alive(ctx, bucket, 0)) { uint32_t score = @@ -3448,8 +3449,7 @@ xlate_all_group(struct xlate_ctx *ctx, struct group_dpif *group) struct ofputil_bucket *bucket; const struct ovs_list *buckets; - group_dpif_get_buckets(group, &buckets); - + buckets = group_dpif_get_buckets(group); LIST_FOR_EACH (bucket, list_node, buckets) { xlate_group_bucket(ctx, bucket); } @@ -3605,14 +3605,13 @@ xlate_group_action(struct xlate_ctx *ctx, uint32_t group_id) { if (xlate_resubmit_resource_check(ctx)) { struct group_dpif *group; - bool got_group; - got_group = group_dpif_lookup(ctx->xbridge->ofproto, group_id, &group); - if (got_group) { - xlate_group_action__(ctx, group); - } else { + group = group_dpif_lookup(ctx->xbridge->ofproto, group_id); + if (!group) { + /* XXX: Should set ctx->error ? */ return true; } + xlate_group_action__(ctx, group); } return false; @@ -4772,6 +4771,9 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, case OFPACT_GROUP: if (xlate_group_action(ctx, ofpact_get_GROUP(a)->group_id)) { /* Group could not be found. */ + + /* XXX: Terminates action list translation, but does not + * terminate the pipeline. */ return; } break; diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 57f3eff..ab26f38 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -4264,7 +4264,7 @@ group_construct_stats(struct group_dpif *group) group->packet_count = 0; group->byte_count = 0; - group_dpif_get_buckets(group, &buckets); + buckets = group_dpif_get_buckets(group); LIST_FOR_EACH (bucket, list_node, buckets) { bucket->stats.packet_count = 0; bucket->stats.byte_count = 0; @@ -4285,7 +4285,7 @@ group_dpif_credit_stats(struct group_dpif *group, } else { /* Credit to all buckets */ const struct ovs_list *buckets; - group_dpif_get_buckets(group, &buckets); + buckets = group_dpif_get_buckets(group); LIST_FOR_EACH (bucket, list_node, buckets) { bucket->stats.packet_count += stats->n_packets; bucket->stats.byte_count += stats->n_bytes; @@ -4335,7 +4335,7 @@ group_get_stats(const struct ofgroup *group_, struct ofputil_group_stats *ogs) ogs->packet_count = group->packet_count; ogs->byte_count = group->byte_count; - group_dpif_get_buckets(group, &buckets); + buckets = group_dpif_get_buckets(group); bucket_stats = ogs->bucket_stats; LIST_FOR_EACH (bucket, list_node, buckets) { bucket_stats->packet_count = bucket->stats.packet_count; @@ -4351,24 +4351,17 @@ group_get_stats(const struct ofgroup *group_, struct ofputil_group_stats *ogs) * * Make sure to call group_dpif_unref() after no longer needing to maintain * a reference to the group. */ -bool -group_dpif_lookup(struct ofproto_dpif *ofproto, uint32_t group_id, - struct group_dpif **group) +struct group_dpif * +group_dpif_lookup(struct ofproto_dpif *ofproto, uint32_t group_id) { - struct ofgroup *ofgroup; - bool found; - - found = ofproto_group_lookup(&ofproto->up, group_id, &ofgroup); - *group = found ? group_dpif_cast(ofgroup) : NULL; - - return found; + struct ofgroup *ofgroup = ofproto_group_lookup(&ofproto->up, group_id); + return ofgroup ? group_dpif_cast(ofgroup) : NULL; } -void -group_dpif_get_buckets(const struct group_dpif *group, - const struct ovs_list **buckets) +const struct ovs_list * +group_dpif_get_buckets(const struct group_dpif *group) { - *buckets = &group->up.buckets; + return &group->up.buckets; } enum ofp11_group_type diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h index ff07ba7..18a90b2 100644 --- a/ofproto/ofproto-dpif.h +++ b/ofproto/ofproto-dpif.h @@ -136,11 +136,10 @@ void rule_dpif_reduce_timeouts(struct rule_dpif *rule, uint16_t idle_timeout, void group_dpif_credit_stats(struct group_dpif *, struct ofputil_bucket *, const struct dpif_flow_stats *); -bool group_dpif_lookup(struct ofproto_dpif *ofproto, uint32_t group_id, - struct group_dpif **group); +struct group_dpif *group_dpif_lookup(struct ofproto_dpif *ofproto, + uint32_t group_id); +const struct ovs_list *group_dpif_get_buckets(const struct group_dpif *group); -void group_dpif_get_buckets(const struct group_dpif *group, - const struct ovs_list **buckets); enum ofp11_group_type group_dpif_get_type(const struct group_dpif *group); const char *group_dpif_get_selection_method(const struct group_dpif *group); uint64_t group_dpif_get_selection_method_param(const struct group_dpif *group); diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index 5ceabe5..e2bc6f3 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -125,7 +125,7 @@ struct ofproto { /* Groups. */ struct ovs_rwlock groups_rwlock; - struct hmap groups OVS_GUARDED; /* Contains "struct ofgroup"s. */ + struct cmap groups; /* Contains "struct ofgroup"s. */ uint32_t n_groups[4] OVS_GUARDED; /* # of existing groups of each type. */ struct ofputil_group_features ogf; }; @@ -490,12 +490,12 @@ void ofproto_rule_reduce_timeouts(struct rule *rule, uint16_t idle_timeout, uint16_t hard_timeout) OVS_EXCLUDED(ofproto_mutex); -/* A group within a "struct ofproto". +/* A group within a "struct ofproto", RCU-protected. * * With few exceptions, ofproto implementations may look at these fields but * should not modify them. */ struct ofgroup { - struct hmap_node hmap_node OVS_GUARDED; /* In ofproto's "groups" hmap. */ + struct cmap_node cmap_node; /* In ofproto's "groups" cmap. */ /* Number of references. * @@ -517,16 +517,18 @@ struct ofgroup { const long long int created; /* Creation time. */ const long long int modified; /* Time of last modification. */ + /* const ?? */ struct ovs_list buckets; /* Contains "struct ofputil_bucket"s. */ const uint32_t n_buckets; const struct ofputil_group_props props; }; -bool ofproto_group_lookup(const struct ofproto *ofproto, uint32_t group_id, - struct ofgroup **group); +struct ofgroup *ofproto_group_lookup(const struct ofproto *ofproto, + uint32_t group_id); void ofproto_group_ref(struct ofgroup *); +bool ofproto_group_try_ref(struct ofgroup *); void ofproto_group_unref(struct ofgroup *); void ofproto_group_delete_all(struct ofproto *); diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index b4876b5..fb4993f 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -293,12 +293,7 @@ static void send_buffered_packet(const struct flow_mod_requester *, uint32_t buffer_id, struct rule *) OVS_REQUIRES(ofproto_mutex); -static bool ofproto_group_exists__(const struct ofproto *ofproto, - uint32_t group_id) - OVS_REQ_RDLOCK(ofproto->groups_rwlock); -static bool ofproto_group_exists(const struct ofproto *ofproto, - uint32_t group_id) - OVS_EXCLUDED(ofproto->groups_rwlock); +static bool ofproto_group_exists(const struct ofproto *, uint32_t group_id); static enum ofperr add_group(struct ofproto *, const struct ofputil_group_mod *); static void handle_openflow(struct ofconn *, const struct ofpbuf *); @@ -564,7 +559,7 @@ ofproto_create(const char *datapath_name, const char *datapath_type, guarded_list_init(&ofproto->rule_executes); ofproto->min_mtu = INT_MAX; ovs_rwlock_init(&ofproto->groups_rwlock); - hmap_init(&ofproto->groups); + cmap_init(&ofproto->groups); ovs_mutex_unlock(&ofproto_mutex); ofproto->ogf.types = 0xf; ofproto->ogf.capabilities = OFPGFC_CHAINING | OFPGFC_SELECT_LIVENESS | @@ -1591,7 +1586,7 @@ ofproto_destroy__(struct ofproto *ofproto) guarded_list_destroy(&ofproto->rule_executes); ovs_rwlock_destroy(&ofproto->groups_rwlock); - hmap_destroy(&ofproto->groups); + cmap_destroy(&ofproto->groups); hmap_remove(&all_ofprotos, &ofproto->hmap_node); free(ofproto->name); @@ -2947,13 +2942,28 @@ ofproto_group_ref(struct ofgroup *group) } } +bool +ofproto_group_try_ref(struct ofgroup *group) +{ + if (group) { + return ovs_refcount_try_ref_rcu(&group->ref_count); + } + return false; +} + +static void +group_destroy_cb(struct ofgroup *group) +{ + group->ofproto->ofproto_class->group_destruct(group); + ofputil_bucket_list_destroy(&group->buckets); + group->ofproto->ofproto_class->group_dealloc(group); +} + void ofproto_group_unref(struct ofgroup *group) { - if (group && ovs_refcount_unref(&group->ref_count) == 1) { - group->ofproto->ofproto_class->group_destruct(group); - ofputil_bucket_list_destroy(&group->buckets); - group->ofproto->ofproto_class->group_dealloc(group); + if (group && ovs_refcount_unref_relaxed(&group->ref_count) == 1) { + ovsrcu_postpone(group_destroy_cb, group); } } @@ -6153,68 +6163,51 @@ handle_meter_request(struct ofconn *ofconn, const struct ofp_header *request, return 0; } -static bool -ofproto_group_lookup__(const struct ofproto *ofproto, uint32_t group_id, - struct ofgroup **group) - OVS_REQ_RDLOCK(ofproto->groups_rwlock) +/* Returned group is RCU protected. */ +static struct ofgroup * +ofproto_group_lookup__(const struct ofproto *ofproto, uint32_t group_id) { - HMAP_FOR_EACH_IN_BUCKET (*group, hmap_node, - hash_int(group_id, 0), &ofproto->groups) { - if ((*group)->group_id == group_id) { - return true; + struct ofgroup *group; + + CMAP_FOR_EACH_WITH_HASH (group, cmap_node, hash_int(group_id, 0), + &ofproto->groups) { + if (group->group_id == group_id) { + return group; } } - return false; + return NULL; } /* 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) +struct ofgroup * +ofproto_group_lookup(const struct ofproto *ofproto, uint32_t group_id) { - bool found; - - ovs_rwlock_rdlock(&ofproto->groups_rwlock); - found = ofproto_group_lookup__(ofproto, group_id, group); - if (found) { - ofproto_group_ref(*group); - } - ovs_rwlock_unlock(&ofproto->groups_rwlock); - return found; -} - -static bool -ofproto_group_exists__(const struct ofproto *ofproto, uint32_t group_id) - OVS_REQ_RDLOCK(ofproto->groups_rwlock) -{ - struct ofgroup *grp; + struct ofgroup *group; - HMAP_FOR_EACH_IN_BUCKET (grp, hmap_node, - hash_int(group_id, 0), &ofproto->groups) { - if (grp->group_id == group_id) { - return true; - } + group = ofproto_group_lookup__(ofproto, group_id); + if (group) { + /* Not holding a lock, so it is possible that another thread releases + * the last reference just before we manage to get one. */ + return ofproto_group_try_ref(group) ? group : NULL; } - return false; + return NULL; } +/* Caller should hold 'ofproto->groups_rwlock' if it is important that the + * group is not removed by someone else. */ static bool ofproto_group_exists(const struct ofproto *ofproto, uint32_t group_id) - OVS_EXCLUDED(ofproto->groups_rwlock) { - bool exists; - - ovs_rwlock_rdlock(&ofproto->groups_rwlock); - exists = ofproto_group_exists__(ofproto, group_id); - ovs_rwlock_unlock(&ofproto->groups_rwlock); - - return exists; + return ofproto_group_lookup__(ofproto, group_id) != NULL; } +/* XXX: This is potentially very expensive for large flow tables, and may be + * called in a loop. Maybe explicitly maintain the count of rules referring to + * the group instead?. */ static uint32_t group_get_ref_count(struct ofgroup *group) OVS_EXCLUDED(ofproto_mutex) @@ -6283,15 +6276,16 @@ handle_group_request(struct ofconn *ofconn, ofpmp_init(&replies, request); if (group_id == OFPG_ALL) { + /* Must exclude modifications to guarantee iterating all groups. */ ovs_rwlock_rdlock(&ofproto->groups_rwlock); - HMAP_FOR_EACH (group, hmap_node, &ofproto->groups) { + CMAP_FOR_EACH (group, cmap_node, &ofproto->groups) { cb(group, &replies); } ovs_rwlock_unlock(&ofproto->groups_rwlock); } else { - if (ofproto_group_lookup(ofproto, group_id, &group)) { + group = ofproto_group_lookup__(ofproto, group_id); + if (group) { cb(group, &replies); - ofproto_group_unref(group); } } ofconn_send_replies(ofconn, &replies); @@ -6496,8 +6490,8 @@ add_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm) return error; } - /* We wrlock as late as possible to minimize the time we jam any other - * threads: No visible state changes before acquiring the lock. */ + /* We take the mutex as late as possible to minimize the time we jam any + * other threads: No visible state changes before acquiring the lock. */ ovs_rwlock_wrlock(&ofproto->groups_rwlock); if (ofproto->n_groups[gm->type] >= ofproto->ogf.max_groups[gm->type]) { @@ -6505,27 +6499,22 @@ add_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm) goto unlock_out; } - if (ofproto_group_exists__(ofproto, gm->group_id)) { + if (ofproto_group_exists(ofproto, gm->group_id)) { error = OFPERR_OFPGMFC_GROUP_EXISTS; goto unlock_out; } - if (!error) { - /* Insert new group. */ - hmap_insert(&ofproto->groups, &ofgroup->hmap_node, - hash_int(ofgroup->group_id, 0)); - ofproto->n_groups[ofgroup->type]++; + /* Insert new group. */ + cmap_insert(&ofproto->groups, &ofgroup->cmap_node, + hash_int(ofgroup->group_id, 0)); + ofproto->n_groups[ofgroup->type]++; - ovs_rwlock_unlock(&ofproto->groups_rwlock); - return error; - } + ovs_rwlock_unlock(&ofproto->groups_rwlock); + return 0; unlock_out: ovs_rwlock_unlock(&ofproto->groups_rwlock); - ofproto->ofproto_class->group_destruct(ofgroup); - ofputil_bucket_list_destroy(&ofgroup->buckets); - ofproto->ofproto_class->group_dealloc(ofgroup); - + group_destroy_cb(ofgroup); return error; } @@ -6646,7 +6635,8 @@ modify_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm) retiring = new_ofgroup; ovs_rwlock_wrlock(&ofproto->groups_rwlock); - if (!ofproto_group_lookup__(ofproto, gm->group_id, &ofgroup)) { + ofgroup = ofproto_group_lookup__(ofproto, gm->group_id); + if (!ofgroup) { error = OFPERR_OFPGMFC_UNKNOWN_GROUP; goto out; } @@ -6674,6 +6664,8 @@ modify_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm) *CONST_CAST(long long int *, &(new_ofgroup->created)) = ofgroup->created; *CONST_CAST(long long int *, &(new_ofgroup->modified)) = time_msec(); + /* XXX: OK to lose old group's stats? */ + error = ofproto->ofproto_class->group_modify(new_ofgroup); if (error) { goto out; @@ -6681,9 +6673,9 @@ modify_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm) retiring = ofgroup; /* Replace ofgroup in ofproto's groups hash map with new_ofgroup. */ - hmap_remove(&ofproto->groups, &ofgroup->hmap_node); - hmap_insert(&ofproto->groups, &new_ofgroup->hmap_node, - hash_int(new_ofgroup->group_id, 0)); + cmap_replace(&ofproto->groups, &ofgroup->cmap_node, + &new_ofgroup->cmap_node, hash_int(ofgroup->group_id, 0)); + if (ofgroup->type != new_ofgroup->type) { ofproto->n_groups[ofgroup->type]--; ofproto->n_groups[new_ofgroup->type]++; @@ -6700,13 +6692,12 @@ out: static enum ofperr add_or_modify_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm) { - struct ofgroup *ofgroup; enum ofperr error; bool exists; - ovs_rwlock_rdlock(&ofproto->groups_rwlock); - exists = ofproto_group_lookup__(ofproto, gm->group_id, &ofgroup); - ovs_rwlock_unlock(&ofproto->groups_rwlock); + exists = ofproto_group_exists(ofproto, gm->group_id); + + /* XXX: Race: Should hold groups_rwlock here. */ if (!exists) { error = add_group(ofproto, gm); @@ -6731,8 +6722,10 @@ delete_group__(struct ofproto *ofproto, struct ofgroup *ofgroup) ofm.fm.table_id = OFPTT_ALL; handle_flow_mod__(ofproto, &ofm, NULL); - hmap_remove(&ofproto->groups, &ofgroup->hmap_node); - /* No-one can find this group any more. */ + cmap_remove(&ofproto->groups, &ofgroup->cmap_node, + hash_int(ofgroup->group_id, 0)); + /* No-one can find this group any more, but current users may continue to + * use it. */ ofproto->n_groups[ofgroup->type]--; ovs_rwlock_unlock(&ofproto->groups_rwlock); ofproto_group_unref(ofgroup); @@ -6747,18 +6740,18 @@ delete_group(struct ofproto *ofproto, uint32_t group_id) ovs_rwlock_wrlock(&ofproto->groups_rwlock); if (group_id == OFPG_ALL) { for (;;) { - struct hmap_node *node = hmap_first(&ofproto->groups); + struct cmap_node *node = cmap_first(&ofproto->groups); if (!node) { break; } - ofgroup = CONTAINER_OF(node, struct ofgroup, hmap_node); - delete_group__(ofproto, ofgroup); + ofgroup = CONTAINER_OF(node, struct ofgroup, cmap_node); + delete_group__(ofproto, ofgroup); /* Releases the mutex. */ /* Lock for each node separately, so that we will not jam the * other threads for too long time. */ ovs_rwlock_wrlock(&ofproto->groups_rwlock); } } else { - HMAP_FOR_EACH_IN_BUCKET (ofgroup, hmap_node, + CMAP_FOR_EACH_WITH_HASH (ofgroup, cmap_node, hash_int(group_id, 0), &ofproto->groups) { if (ofgroup->group_id == group_id) { delete_group__(ofproto, ofgroup); diff --git a/tests/ofproto.at b/tests/ofproto.at index 4abd335..e5fb54c 100644 --- a/tests/ofproto.at +++ b/tests/ofproto.at @@ -347,10 +347,10 @@ group_id=1235,type=all,bucket=actions=output:10 ]) AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn add-groups br0 groups.txt]) AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn dump-groups br0 ], [0], [stdout]) -AT_CHECK([strip_xids < stdout], [0], [dnl -OFPST_GROUP_DESC reply (OF1.1): +AT_CHECK([strip_xids < stdout | sort], [0], [dnl group_id=1234,type=all,bucket=actions=output:10 group_id=1235,type=all,bucket=actions=output:10 +OFPST_GROUP_DESC reply (OF1.1): ]) AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn del-groups br0 group_id=1234]) AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn dump-groups br0], [0], [stdout]) @@ -437,17 +437,17 @@ OFPST_GROUP_DESC reply (OF1.5): ]) AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn del-groups br0 group_id=1234]) AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn dump-groups br0], [0], [stdout]) -AT_CHECK([strip_xids < stdout], [0], [dnl -OFPST_GROUP_DESC reply (OF1.5): - group_id=1235,type=all,bucket=bucket_id:2,actions=output:12,bucket=bucket_id:0,actions=output:10,bucket=bucket_id:1,actions=output:11 +AT_CHECK([strip_xids < stdout | sort], [0], [dnl group_id=1233,type=select,selection_method=hash,bucket=bucket_id:0,actions=output:10,bucket=bucket_id:1,actions=output:11 + group_id=1235,type=all,bucket=bucket_id:2,actions=output:12,bucket=bucket_id:0,actions=output:10,bucket=bucket_id:1,actions=output:11 +OFPST_GROUP_DESC reply (OF1.5): ]) AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn del-groups br0 group_id=1234]) AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn dump-groups br0], [0], [stdout]) -AT_CHECK([strip_xids < stdout], [0], [dnl -OFPST_GROUP_DESC reply (OF1.5): - group_id=1235,type=all,bucket=bucket_id:2,actions=output:12,bucket=bucket_id:0,actions=output:10,bucket=bucket_id:1,actions=output:11 +AT_CHECK([strip_xids < stdout | sort], [0], [dnl group_id=1233,type=select,selection_method=hash,bucket=bucket_id:0,actions=output:10,bucket=bucket_id:1,actions=output:11 + group_id=1235,type=all,bucket=bucket_id:2,actions=output:12,bucket=bucket_id:0,actions=output:10,bucket=bucket_id:1,actions=output:11 +OFPST_GROUP_DESC reply (OF1.5): ]) AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn del-groups br0], [0]) AT_CHECK([ovs-ofctl -O OpenFlow15 -vwarn dump-groups br0], [0], [stdout])