From patchwork Fri Jul 15 10:19:15 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jarno Rajahalme X-Patchwork-Id: 648750 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 3rrTB21N7pz9s9N for ; Fri, 15 Jul 2016 20:21:14 +1000 (AEST) Received: from archives.nicira.com (localhost [127.0.0.1]) by archives.nicira.com (Postfix) with ESMTP id 8BC8E10CD6; Fri, 15 Jul 2016 03:20:06 -0700 (PDT) X-Original-To: dev@openvswitch.org Delivered-To: dev@openvswitch.org Received: from mx1e4.cudamail.com (mx1.cudamail.com [69.90.118.67]) by archives.nicira.com (Postfix) with ESMTPS id 29B5F10CA5 for ; Fri, 15 Jul 2016 03:20:03 -0700 (PDT) Received: from bar5.cudamail.com (unknown [192.168.21.12]) by mx1e4.cudamail.com (Postfix) with ESMTPS id 924071E009C for ; Fri, 15 Jul 2016 04:20:02 -0600 (MDT) X-ASG-Debug-ID: 1468578001-09eadd107c11af0001-byXFYA Received: from mx3-pf1.cudamail.com ([192.168.14.2]) by bar5.cudamail.com with ESMTP id pFEFISBeo30n19E7 (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Fri, 15 Jul 2016 04:20:02 -0600 (MDT) X-Barracuda-Envelope-From: jarno@ovn.org X-Barracuda-RBL-Trusted-Forwarder: 192.168.14.2 Received: from unknown (HELO relay4-d.mail.gandi.net) (217.70.183.196) by mx3-pf1.cudamail.com with ESMTPS (DHE-RSA-AES256-SHA encrypted); 15 Jul 2016 10:20:01 -0000 Received-SPF: pass (mx3-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 mfilter47-d.gandi.net (mfilter47-d.gandi.net [217.70.178.178]) by relay4-d.mail.gandi.net (Postfix) with ESMTP id 3CD0317209A; Fri, 15 Jul 2016 12:20:00 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at mfilter47-d.gandi.net Received: from relay4-d.mail.gandi.net ([IPv6:::ffff:217.70.183.196]) by mfilter47-d.gandi.net (mfilter47-d.gandi.net [::ffff:10.0.15.180]) (amavisd-new, port 10024) with ESMTP id n8HebzymyF-D; Fri, 15 Jul 2016 12:19:58 +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 B3F9B17209B; Fri, 15 Jul 2016 12:19:57 +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-V1-714003043 X-CudaMail-DTE: 071516 X-CudaMail-Originating-IP: 217.70.183.196 Date: Fri, 15 Jul 2016 03:19:15 -0700 X-ASG-Orig-Subj: [##CM-V1-714003043##][PATCH 09/13] ofproto: refactor group mods. Message-Id: <1468577959-98487-9-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.14.2] X-Barracuda-Start-Time: 1468578002 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://web.cudamail.com:443/cgi-mod/mark.cgi X-ASG-Whitelist: Header =?UTF-8?B?eFwtY3VkYW1haWxcLXdoaXRlbGlzdFwtdG8=?= X-Virus-Scanned: by bsmtpd at cudamail.com X-Barracuda-BRTS-Status: 1 Subject: [ovs-dev] [PATCH 09/13] ofproto: refactor group mods. 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" This changes ofproto providers modify_group() to never fail. Separating major refactoring to a separate patch should make following patches easier to review. Signed-off-by: Jarno Rajahalme --- ofproto/ofproto-dpif.c | 4 +- ofproto/ofproto-provider.h | 27 +++- ofproto/ofproto.c | 319 +++++++++++++++++++++++++-------------------- 3 files changed, 206 insertions(+), 144 deletions(-) diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 3ca4aa0..1b0d51c 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -4313,14 +4313,12 @@ group_destruct(struct ofgroup *group_) ovs_mutex_destroy(&group->stats_mutex); } -static enum ofperr +static void group_modify(struct ofgroup *group_) { struct ofproto_dpif *ofproto = ofproto_dpif_cast(group_->ofproto); ofproto->backer->need_revalidate = REV_FLOW_TABLE; - - return 0; } static enum ofperr diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index ebc0ee3..8963e4f 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -624,6 +624,23 @@ void ofproto_group_unref(struct ofgroup *); void ofproto_group_delete_all(struct ofproto *); +DECL_COLLECTION(struct ofgroup *, group) + +#define GROUP_COLLECTION_FOR_EACH(GROUP, GROUPS) \ + for (size_t i__ = 0; \ + i__ < group_collection_n(GROUPS) \ + ? (GROUP = group_collection_groups(GROUPS)[i__]) : false; \ + i__++) + +#define GROUP_COLLECTIONS_FOR_EACH(GROUP1, GROUP2, GROUPS1, GROUPS2) \ + for (size_t i__ = 0; \ + i__ < group_collection_n(GROUPS1) \ + ? ((GROUP1 = group_collection_groups(GROUPS1)[i__]), \ + (GROUP2 = group_collection_groups(GROUPS2)[i__])) \ + : false; \ + i__++) + + /* ofproto class structure, to be defined by each ofproto implementation. * * @@ -1854,7 +1871,7 @@ struct ofproto_class { void (*group_destruct)(struct ofgroup *); void (*group_dealloc)(struct ofgroup *); - enum ofperr (*group_modify)(struct ofgroup *); + void (*group_modify)(struct ofgroup *); enum ofperr (*group_get_stats)(const struct ofgroup *, struct ofputil_group_stats *); @@ -1895,6 +1912,14 @@ struct ofproto_port_mod { struct ofport *port; /* Affected port. */ }; +/* flow_mod with execution context. */ +struct ofproto_group_mod { + struct ofputil_group_mod gm; + + struct ofgroup *new_group; /* New group. */ + struct group_collection old_groups; /* Affected groups. */ +}; + enum ofperr ofproto_flow_mod(struct ofproto *, struct ofproto_flow_mod *) OVS_EXCLUDED(ofproto_mutex); void ofproto_add_flow(struct ofproto *, const struct match *, int priority, diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index e96aeb7..39abfdb 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -294,9 +294,6 @@ static void send_buffered_packet(const struct openflow_mod_requester *, OVS_REQUIRES(ofproto_mutex); static bool ofproto_group_exists(const struct ofproto *, uint32_t group_id); -static enum ofperr add_group(struct ofproto *, - const struct ofputil_group_mod *) - OVS_REQUIRES(ofproto_mutex); static void handle_openflow(struct ofconn *, const struct ofpbuf *); static enum ofperr ofproto_flow_mod_start(struct ofproto *, struct ofproto_flow_mod *) @@ -1574,9 +1571,6 @@ ofproto_flush__(struct ofproto *ofproto) ovs_mutex_unlock(&ofproto_mutex); } -static void delete_group(struct ofproto *ofproto, uint32_t group_id) - OVS_REQUIRES(ofproto_mutex); - static void ofproto_destroy__(struct ofproto *ofproto) OVS_EXCLUDED(ofproto_mutex) @@ -6513,37 +6507,28 @@ init_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm, * 'ofproto''s group table. Returns 0 on success or an OpenFlow error code on * failure. */ static enum ofperr -add_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm) +add_group_start(struct ofproto *ofproto, struct ofproto_group_mod *ogm) OVS_REQUIRES(ofproto_mutex) { - struct ofgroup *ofgroup; enum ofperr error; - /* Allocate new group and initialize it. */ - error = init_group(ofproto, gm, &ofgroup); - if (error) { - return error; + if (ofproto_group_exists(ofproto, ogm->gm.group_id)) { + return OFPERR_OFPGMFC_GROUP_EXISTS; } - if (ofproto->n_groups[gm->type] >= ofproto->ogf.max_groups[gm->type]) { - error = OFPERR_OFPGMFC_OUT_OF_GROUPS; - goto unlock_out; + if (ofproto->n_groups[ogm->gm.type] + >= ofproto->ogf.max_groups[ogm->gm.type]) { + return OFPERR_OFPGMFC_OUT_OF_GROUPS; } - if (ofproto_group_exists(ofproto, gm->group_id)) { - error = OFPERR_OFPGMFC_GROUP_EXISTS; - goto unlock_out; + /* Allocate new group and initialize it. */ + error = init_group(ofproto, &ogm->gm, &ogm->new_group); + if (!error) { + /* Insert new group. */ + cmap_insert(&ofproto->groups, &ogm->new_group->cmap_node, + hash_int(ogm->new_group->group_id, 0)); + ofproto->n_groups[ogm->new_group->type]++; } - - /* Insert new group. */ - cmap_insert(&ofproto->groups, &ofgroup->cmap_node, - hash_int(ofgroup->group_id, 0)); - ofproto->n_groups[ofgroup->type]++; - - return 0; - - unlock_out: - group_destroy_cb(ofgroup); return error; } @@ -6651,215 +6636,269 @@ copy_buckets_for_remove_bucket(const struct ofgroup *ofgroup, * ofproto's ofgroup hash map. Thus, the group is never altered while users of * the xlate module hold a pointer to the group. */ static enum ofperr -modify_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm) +modify_group_start(struct ofproto *ofproto, struct ofproto_group_mod *ogm) OVS_REQUIRES(ofproto_mutex) { - struct ofgroup *ofgroup, *new_ofgroup, *retiring; + struct ofgroup *old_group; /* Modified group. */ + struct ofgroup *new_group; enum ofperr error; - error = init_group(ofproto, gm, &new_ofgroup); - if (error) { - return error; + old_group = ofproto_group_lookup__(ofproto, ogm->gm.group_id); + if (!old_group) { + return OFPERR_OFPGMFC_UNKNOWN_GROUP; } - retiring = new_ofgroup; - - ofgroup = ofproto_group_lookup__(ofproto, gm->group_id); - if (!ofgroup) { - error = OFPERR_OFPGMFC_UNKNOWN_GROUP; - goto out; + if (old_group->type != ogm->gm.type + && (ofproto->n_groups[ogm->gm.type] + >= ofproto->ogf.max_groups[ogm->gm.type])) { + return OFPERR_OFPGMFC_OUT_OF_GROUPS; } - /* Ofproto's group write lock is held now. */ - if (ofgroup->type != gm->type - && ofproto->n_groups[gm->type] >= ofproto->ogf.max_groups[gm->type]) { - error = OFPERR_OFPGMFC_OUT_OF_GROUPS; - goto out; + error = init_group(ofproto, &ogm->gm, &ogm->new_group); + if (error) { + return error; } + new_group = ogm->new_group; /* Manipulate bucket list for bucket commands */ - if (gm->command == OFPGC15_INSERT_BUCKET) { - error = copy_buckets_for_insert_bucket(ofgroup, new_ofgroup, - gm->command_bucket_id); - } else if (gm->command == OFPGC15_REMOVE_BUCKET) { - error = copy_buckets_for_remove_bucket(ofgroup, new_ofgroup, - gm->command_bucket_id); + if (ogm->gm.command == OFPGC15_INSERT_BUCKET) { + error = copy_buckets_for_insert_bucket(old_group, new_group, + ogm->gm.command_bucket_id); + } else if (ogm->gm.command == OFPGC15_REMOVE_BUCKET) { + error = copy_buckets_for_remove_bucket(old_group, new_group, + ogm->gm.command_bucket_id); } if (error) { goto out; } /* The group creation time does not change during modification. */ - *CONST_CAST(long long int *, &(new_ofgroup->created)) = ofgroup->created; - *CONST_CAST(long long int *, &(new_ofgroup->modified)) = time_msec(); + *CONST_CAST(long long int *, &(new_group->created)) = old_group->created; + *CONST_CAST(long long int *, &(new_group->modified)) = time_msec(); - /* XXX: OK to lose old group's stats? */ + group_collection_add(&ogm->old_groups, old_group); - error = ofproto->ofproto_class->group_modify(new_ofgroup); - if (error) { - goto out; - } - - retiring = ofgroup; /* Replace ofgroup in ofproto's groups hash map with new_ofgroup. */ - cmap_replace(&ofproto->groups, &ofgroup->cmap_node, - &new_ofgroup->cmap_node, hash_int(ofgroup->group_id, 0)); + cmap_replace(&ofproto->groups, &old_group->cmap_node, + &new_group->cmap_node, hash_int(new_group->group_id, 0)); + /* Transfer rules. */ - rule_collection_move(&new_ofgroup->rules, &ofgroup->rules); + rule_collection_move(&new_group->rules, &old_group->rules); - if (ofgroup->type != new_ofgroup->type) { - ofproto->n_groups[ofgroup->type]--; - ofproto->n_groups[new_ofgroup->type]++; + if (old_group->type != new_group->type) { + ofproto->n_groups[old_group->type]--; + ofproto->n_groups[new_group->type]++; } + return 0; out: - ofproto_group_unref(retiring); + ofproto_group_unref(new_group); return error; } /* Implements the OFPGC11_ADD_OR_MOD command which creates the group when it does not * exist yet and modifies it otherwise */ static enum ofperr -add_or_modify_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm) +add_or_modify_group_start(struct ofproto *ofproto, + struct ofproto_group_mod *ogm) OVS_REQUIRES(ofproto_mutex) { enum ofperr error; - bool exists; - - exists = ofproto_group_exists(ofproto, gm->group_id); - if (!exists) { - error = add_group(ofproto, gm); + if (!ofproto_group_exists(ofproto, ogm->gm.group_id)) { + error = add_group_start(ofproto, ogm); } else { - error = modify_group(ofproto, gm); + error = modify_group_start(ofproto, ogm); } return error; } static void -delete_group__(struct ofproto *ofproto, struct ofgroup *ofgroup) +delete_group_start(struct ofproto *ofproto, struct group_collection *groups, + struct ofgroup *group) OVS_REQUIRES(ofproto_mutex) { - /* Makes flow deletion code leave the rule pointers in group's 'rules' + /* Makes flow deletion code leave the rule pointers in 'group->rules' * intact, so that we can later refer to the rules deleted due to the group * deletion. Rule pointers will be removed from all other groups, if any, * so we will never try to delete the same rule twice. */ - ofgroup->being_deleted = true; + group->being_deleted = true; - /* Delete all flow entries containing this group in a group action. */ - delete_flows__(&ofgroup->rules, OFPRR_GROUP_DELETE, NULL); + /* Mark all the referring groups for deletion. */ + delete_flows_start__(ofproto, ofproto->tables_version + 1, + &group->rules); + group_collection_add(groups, group); + ofproto->n_groups[group->type]--; +} - 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]--; - ofproto_group_unref(ofgroup); +static void +delete_group_finish(struct ofproto *ofproto, struct ofgroup *group) + OVS_REQUIRES(ofproto_mutex) +{ + /* Finish deletion of all flow entries containing this group in a group + * action. */ + delete_flows_finish__(ofproto, &group->rules, OFPRR_GROUP_DELETE, NULL); + + cmap_remove(&ofproto->groups, &group->cmap_node, + hash_int(group->group_id, 0)); + /* No-one can find this group any more, but current users may + * continue to use it. */ + ofproto_group_unref(group); } /* Implements OFPGC11_DELETE. */ static void -delete_group(struct ofproto *ofproto, uint32_t group_id) +delete_groups_start(struct ofproto *ofproto, struct ofproto_group_mod *ogm) OVS_REQUIRES(ofproto_mutex) { - struct ofgroup *ofgroup; + struct ofgroup *group; - if (group_id == OFPG_ALL) { - for (;;) { - struct cmap_node *node = cmap_first(&ofproto->groups); - if (!node) { - break; - } - ofgroup = CONTAINER_OF(node, struct ofgroup, cmap_node); - delete_group__(ofproto, ofgroup); + if (ogm->gm.group_id == OFPG_ALL) { + CMAP_FOR_EACH (group, cmap_node, &ofproto->groups) { + delete_group_start(ofproto, &ogm->old_groups, group); } } else { - 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); + CMAP_FOR_EACH_WITH_HASH (group, cmap_node, + hash_int(ogm->gm.group_id, 0), + &ofproto->groups) { + if (group->group_id == ogm->gm.group_id) { + delete_group_start(ofproto, &ogm->old_groups, group); return; } } } } -/* Delete all groups from 'ofproto'. - * - * This is intended for use within an ofproto provider's 'destruct' - * function. */ -void -ofproto_group_delete_all(struct ofproto *ofproto) - OVS_EXCLUDED(ofproto_mutex) -{ - ovs_mutex_lock(&ofproto_mutex); - delete_group(ofproto, OFPG_ALL); - ovs_mutex_unlock(&ofproto_mutex); -} - static enum ofperr -handle_group_mod(struct ofconn *ofconn, const struct ofp_header *oh) - OVS_EXCLUDED(ofproto_mutex) +ofproto_group_mod_start(struct ofproto *ofproto, struct ofproto_group_mod *ogm) + OVS_REQUIRES(ofproto_mutex) { - struct ofproto *ofproto = ofconn_get_ofproto(ofconn); - struct ofputil_group_mod gm; enum ofperr error; - error = reject_slave_controller(ofconn); - if (error) { - return error; - } + ogm->new_group = NULL; + group_collection_init(&ogm->old_groups); - error = ofputil_decode_group_mod(oh, &gm); - if (error) { - return error; - } - - ovs_mutex_lock(&ofproto_mutex); - switch (gm.command) { + switch (ogm->gm.command) { case OFPGC11_ADD: - error = add_group(ofproto, &gm); + error = add_group_start(ofproto, ogm); break; case OFPGC11_MODIFY: - error = modify_group(ofproto, &gm); + error = modify_group_start(ofproto, ogm); break; case OFPGC11_ADD_OR_MOD: - error = add_or_modify_group(ofproto, &gm); + error = add_or_modify_group_start(ofproto, ogm); break; case OFPGC11_DELETE: - delete_group(ofproto, gm.group_id); + delete_groups_start(ofproto, ogm); error = 0; break; case OFPGC15_INSERT_BUCKET: - error = modify_group(ofproto, &gm); + error = modify_group_start(ofproto, ogm); break; case OFPGC15_REMOVE_BUCKET: - error = modify_group(ofproto, &gm); + error = modify_group_start(ofproto, ogm); break; default: - if (gm.command > OFPGC11_DELETE) { + if (ogm->gm.command > OFPGC11_DELETE) { VLOG_INFO_RL(&rl, "%s: Invalid group_mod command type %d", - ofproto->name, gm.command); + ofproto->name, ogm->gm.command); } error = OFPERR_OFPGMFC_BAD_COMMAND; + break; } - ovs_mutex_unlock(&ofproto_mutex); + return error; +} - if (!error) { +static void +ofproto_group_mod_finish(struct ofproto *ofproto, + struct ofproto_group_mod *ogm, + const struct openflow_mod_requester *req) + OVS_REQUIRES(ofproto_mutex) +{ + struct ofgroup *new_group = ogm->new_group; + struct ofgroup *old_group; + + if (!new_group) { + /* Delete old groups. */ + ofproto_bump_tables_version(ofproto); + GROUP_COLLECTION_FOR_EACH(old_group, &ogm->old_groups) { + delete_group_finish(ofproto, old_group); + } + } else if (group_collection_n(&ogm->old_groups)) { + /* Modify a group. */ + ovs_assert(group_collection_n(&ogm->old_groups) == 1); + old_group = group_collection_groups(&ogm->old_groups)[0]; + + /* XXX: OK to lose old group's stats? */ + ofproto->ofproto_class->group_modify(new_group); + + ofproto_group_unref(old_group); + } + group_collection_destroy(&ogm->old_groups); + + if (req) { struct ofputil_requestforward rf; - rf.xid = oh->xid; + rf.xid = req->request->xid; rf.reason = OFPRFR_GROUP_MOD; - rf.group_mod = &gm; - connmgr_send_requestforward(ofproto->connmgr, ofconn, &rf); + rf.group_mod = &ogm->gm; + connmgr_send_requestforward(ofproto->connmgr, req->ofconn, &rf); + } +} + +/* Delete all groups from 'ofproto'. + * + * This is intended for use within an ofproto provider's 'destruct' + * function. */ +void +ofproto_group_delete_all(struct ofproto *ofproto) + OVS_EXCLUDED(ofproto_mutex) +{ + struct ofproto_group_mod ogm; + + ogm.gm.command = OFPGC11_DELETE; + ogm.gm.group_id = OFPG_ALL; + + ovs_mutex_lock(&ofproto_mutex); + ofproto_group_mod_start(ofproto, &ogm); + ofproto_group_mod_finish(ofproto, &ogm, NULL); + ovs_mutex_unlock(&ofproto_mutex); +} + +static enum ofperr +handle_group_mod(struct ofconn *ofconn, const struct ofp_header *oh) + OVS_EXCLUDED(ofproto_mutex) +{ + struct ofproto *ofproto = ofconn_get_ofproto(ofconn); + struct ofproto_group_mod ogm; + enum ofperr error; + + error = reject_slave_controller(ofconn); + if (error) { + return error; + } + + error = ofputil_decode_group_mod(oh, &ogm.gm); + if (error) { + return error; } - ofputil_bucket_list_destroy(&gm.buckets); + + ovs_mutex_lock(&ofproto_mutex); + error = ofproto_group_mod_start(ofproto, &ogm); + if (!error) { + struct openflow_mod_requester req = { ofconn, oh }; + ofproto_group_mod_finish(ofproto, &ogm, &req); + } + ofmonitor_flush(ofproto->connmgr); + ovs_mutex_unlock(&ofproto_mutex); + + ofputil_bucket_list_destroy(&ogm.gm.buckets); return error; }