From patchwork Tue Apr 23 07:42:47 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aravind.Sridharan@dell.com X-Patchwork-Id: 1089443 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=dell.com Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=dell.com header.i=@dell.com header.b="mkJ45SBJ"; dkim-atps=neutral Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 44pQkQ0QnHz9sNg for ; Wed, 24 Apr 2019 00:26:54 +1000 (AEST) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id A425BBDC; Tue, 23 Apr 2019 14:26:52 +0000 (UTC) X-Original-To: ovs-dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 0A59AB5F for ; Tue, 23 Apr 2019 14:26:51 +0000 (UTC) X-Greylist: delayed 06:43:53 by SQLgrey-1.7.6 Received: from mx0a-00154904.pphosted.com (mx0a-00154904.pphosted.com [148.163.133.20]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 6C7F4F4 for ; Tue, 23 Apr 2019 14:26:49 +0000 (UTC) Received: from pps.filterd (m0170389.ppops.net [127.0.0.1]) by mx0a-00154904.pphosted.com (8.16.0.27/8.16.0.27) with SMTP id x3N7eP4H025773 for ; Tue, 23 Apr 2019 03:42:55 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=dell.com; h=from : to : subject : date : message-id : content-type : mime-version; s=smtpout1; bh=+qB4PQ1Newjnb4Vr70fUEgaQ3RIBEGvfcVb1QyfV81c=; b=mkJ45SBJuKdfAdOT4RgTMILX0E1CsBK4b4pERxhLRHXZZeshhIGlkhlABXI08UycMJhG cuhV1SQL2Ztc79gn5+RYKg3dDlrz/4SYQA8BnpVvRw6ONG7AQS4i5AjNAw4++B5GLFFZ 90A3tFtajuj3JcC2A543naB6yoTmriiHfUbZGKxhh5W7hRPn3IFyIaEbG050VRezRAuB 9xbYHabeGOBavxMc6RyL1fZ6nGfbL3q/rWhFIvIyjHfUK17w5RTzsobEm7p+zt6fTywr qjW3b0idnalaeqAQJI4DgdkkURO/ECdsDqainwXjLRICbtHfM+OzK/Ljg6OvJoE6DmMG Fg== Received: from mx0b-00154901.pphosted.com (mx0b-00154901.pphosted.com [67.231.157.37]) by mx0a-00154904.pphosted.com with ESMTP id 2ryyeh6w5v-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Tue, 23 Apr 2019 03:42:54 -0400 Received: from pps.filterd (m0144103.ppops.net [127.0.0.1]) by mx0b-00154901.pphosted.com (8.16.0.27/8.16.0.27) with SMTP id x3N7grDU098920 for ; Tue, 23 Apr 2019 03:42:54 -0400 Received: from ausxipps306.us.dell.com (AUSXIPPS306.us.dell.com [143.166.148.156]) by mx0b-00154901.pphosted.com with ESMTP id 2s1u0ntesk-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Tue, 23 Apr 2019 03:42:53 -0400 X-LoopCount0: from 10.166.132.131 X-IronPort-AV: E=Sophos;i="5.60,349,1549951200"; d="scan'208,217";a="299291185" From: To: Thread-Topic: [ovs-dev] [PATCH] ofproto: Return error codes for Rule insertions Thread-Index: AdT5o2gmVfjna44cQziQ9n9Kbh2Iog== Date: Tue, 23 Apr 2019 07:42:47 +0000 Message-ID: <554e873adec6449a8efe85037f95d6b5@BLRX13MDC429.AMER.DELL.COM> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [163.244.186.30] MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:, , definitions=2019-04-22_01:, , signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1011 lowpriorityscore=0 mlxscore=0 impostorscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1810050000 definitions=main-1904230057 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1011 lowpriorityscore=0 mlxscore=0 impostorscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1810050000 definitions=main-1904230057 X-Spam-Status: No, score=-1.6 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, HTML_MESSAGE, KHOP_DYNAMIC, RCVD_IN_DNSWL_NONE autolearn=no version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org X-Content-Filtered-By: Mailman/MimeDel 2.1.12 Subject: [ovs-dev] [PATCH] ofproto: Return error codes for Rule insertions X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org Description ----------- Currently, rule_insert() API does not have return value. There are some possible scenarios where rule insertions can fail at run-time even though the static checks during rule_construct() had passed previously. Some possible scenarios for failure of rule insertions: **) Rule insertions can fail dynamically in Hybrid mode (both Openflow and Normal switch functioning coexist) where the CAM space could get suddenly filled up by Normal switch functioning and Openflow gets devoid of available space. **) Some deployments could have separate independent layers for HW rule insertions and application layer to interact with OVS. HW layer could face any dynamic issue during rule handling which application could not have predicted/captured in rule-construction phase. Rule-insert errors for bundles are handled too in this pull-request. Testing: ------- Tested with failures of rule insertions and also with bundles. Signed-off-by: Aravind Prasad S static void ofproto_flow_mod_revert(struct ofproto *, struct ofproto_flow_mod *) OVS_REQUIRES(ofproto_mutex); -static void ofproto_flow_mod_finish(struct ofproto *, - struct ofproto_flow_mod *, - const struct openflow_mod_requester *) +static enum ofperr ofproto_flow_mod_finish(struct ofproto *, + struct ofproto_flow_mod *, + const struct openflow_mod_requester *) OVS_REQUIRES(ofproto_mutex); static enum ofperr handle_flow_mod__(struct ofproto *, const struct ofputil_flow_mod *, @@ -5115,7 +5115,7 @@ add_flow_revert(struct ofproto *ofproto, struct ofproto_flow_mod *ofm) } /* To be called after version bump. */ -static void +static enum ofperr add_flow_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm, const struct openflow_mod_requester *req) OVS_REQUIRES(ofproto_mutex) @@ -5124,8 +5124,14 @@ add_flow_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm, ? rule_collection_rules(&ofm->old_rules)[0] : NULL; struct rule *new_rule = rule_collection_rules(&ofm->new_rules)[0]; struct ovs_list dead_cookies = OVS_LIST_INITIALIZER(&dead_cookies); + enum ofperr error = 0; + + error = replace_rule_finish(ofproto, ofm, req, old_rule, new_rule, + &dead_cookies); + if (error) { + return error; + } - replace_rule_finish(ofproto, ofm, req, old_rule, new_rule, &dead_cookies); learned_cookies_flush(ofproto, &dead_cookies); if (old_rule) { @@ -5138,6 +5144,8 @@ add_flow_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm, /* Send Vacancy Events for OF1.4+. */ send_table_status(ofproto, new_rule->table_id); } + + return error; } /* OFPFC_MODIFY and OFPFC_MODIFY_STRICT. */ @@ -5334,22 +5342,25 @@ ofproto_flow_mod_learn_revert(struct ofproto_flow_mod *ofm) ofproto_flow_mod_revert(rule->ofproto, ofm); } -void +enum ofperr ofproto_flow_mod_learn_finish(struct ofproto_flow_mod *ofm, struct ofproto *orig_ofproto) OVS_REQUIRES(ofproto_mutex) { struct rule *rule = rule_collection_rules(&ofm->new_rules)[0]; + enum ofperr error = 0; /* If learning on a different bridge, must bump its version * number and flush connmgr afterwards. */ if (rule->ofproto != orig_ofproto) { ofproto_bump_tables_version(rule->ofproto); } - ofproto_flow_mod_finish(rule->ofproto, ofm, NULL); + error = ofproto_flow_mod_finish(rule->ofproto, ofm, NULL); if (rule->ofproto != orig_ofproto) { ofmonitor_flush(rule->ofproto->connmgr); } + + return error; } /* Refresh 'ofm->temp_rule', for which the caller holds a reference, if already @@ -5404,7 +5415,7 @@ ofproto_flow_mod_learn(struct ofproto_flow_mod *ofm, bool keep_ref, error = ofproto_flow_mod_learn_start(ofm); if (!error) { - ofproto_flow_mod_learn_finish(ofm, NULL); + error = ofproto_flow_mod_learn_finish(ofm, NULL); } } else { static struct vlog_rate_limit rll = VLOG_RATE_LIMIT_INIT(1, 5); @@ -5504,7 +5515,7 @@ replace_rule_revert(struct ofproto *ofproto, } /* Adds the 'new_rule', replacing the 'old_rule'. */ -static void +static enum ofperr replace_rule_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm, const struct openflow_mod_requester *req, struct rule *old_rule, struct rule *new_rule, @@ -5512,6 +5523,8 @@ replace_rule_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm, OVS_REQUIRES(ofproto_mutex) { struct rule *replaced_rule; + enum ofperr error = 0; + struct oftable *table = &ofproto->tables[new_rule->table_id]; replaced_rule = (old_rule && old_rule->removed_reason != OFPRR_EVICTION) ? old_rule : NULL; @@ -5521,8 +5534,15 @@ replace_rule_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm, * link the packet and byte counts from the old rule to the new one if * 'modify_keep_counts' is 'true'. The 'replaced_rule' will be deleted * right after this call. */ - ofproto->ofproto_class->rule_insert(new_rule, replaced_rule, - ofm->modify_keep_counts); + error = ofproto->ofproto_class->rule_insert(new_rule, replaced_rule, + ofm->modify_keep_counts); + if (error) { + if (classifier_remove(&table->cls, &new_rule->cr)) { + ofproto_rule_destroy__(new_rule); + } + return error; + } + learned_cookies_inc(ofproto, rule_get_actions(new_rule)); if (old_rule) { @@ -5558,6 +5578,8 @@ replace_rule_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm, req ? req->request->xid : 0, NULL); } } + + return error; } /* ofm->temp_rule is consumed only in the successful case. */ @@ -5706,17 +5728,18 @@ modify_flows_revert(struct ofproto *ofproto, struct ofproto_flow_mod *ofm) } } -static void +static enum ofperr modify_flows_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm, const struct openflow_mod_requester *req) OVS_REQUIRES(ofproto_mutex) { struct rule_collection *old_rules = &ofm->old_rules; struct rule_collection *new_rules = &ofm->new_rules; + enum ofperr error = 0; if (rule_collection_n(old_rules) == 0 && rule_collection_n(new_rules) == 1) { - add_flow_finish(ofproto, ofm, req); + error = add_flow_finish(ofproto, ofm, req); } else if (rule_collection_n(old_rules) > 0) { struct ovs_list dead_cookies = OVS_LIST_INITIALIZER(&dead_cookies); @@ -5725,12 +5748,17 @@ modify_flows_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm, struct rule *old_rule, *new_rule; RULE_COLLECTIONS_FOR_EACH (old_rule, new_rule, old_rules, new_rules) { - replace_rule_finish(ofproto, ofm, req, old_rule, new_rule, - &dead_cookies); + error = replace_rule_finish(ofproto, ofm, req, old_rule, new_rule, + &dead_cookies); + if (error) { + return error; + } } learned_cookies_flush(ofproto, &dead_cookies); remove_rules_postponed(old_rules); } + + return error; } static enum ofperr @@ -6094,7 +6122,7 @@ handle_flow_mod__(struct ofproto *ofproto, const struct ofputil_flow_mod *fm, error = ofproto_flow_mod_start(ofproto, &ofm); if (!error) { ofproto_bump_tables_version(ofproto); - ofproto_flow_mod_finish(ofproto, &ofm, req); + error = ofproto_flow_mod_finish(ofproto, &ofm, req); ofmonitor_flush(ofproto->connmgr); } ovs_mutex_unlock(&ofproto_mutex); @@ -7960,19 +7988,21 @@ ofproto_flow_mod_revert(struct ofproto *ofproto, struct ofproto_flow_mod *ofm) rule_collection_destroy(&ofm->new_rules); } -static void +static enum ofperr ofproto_flow_mod_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm, const struct openflow_mod_requester *req) OVS_REQUIRES(ofproto_mutex) { + enum ofperr error = 0; + switch (ofm->command) { case OFPFC_ADD: - add_flow_finish(ofproto, ofm, req); + error = add_flow_finish(ofproto, ofm, req); break; case OFPFC_MODIFY: case OFPFC_MODIFY_STRICT: - modify_flows_finish(ofproto, ofm, req); + error = modify_flows_finish(ofproto, ofm, req); break; case OFPFC_DELETE: @@ -7990,6 +8020,8 @@ ofproto_flow_mod_finish(struct ofproto *ofproto, struct ofproto_flow_mod *ofm, if (req) { ofconn_report_flow_mod(req->ofconn, ofm->command); } + + return error; } /* Commit phases (all while locking ofproto_mutex): @@ -8073,11 +8105,9 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags) if (error) { /* Send error referring to the original message. */ - if (error) { - ofconn_send_error(ofconn, be->msg, error); - error = OFPERR_OFPBFC_MSG_FAILED; - } - + ofconn_send_error(ofconn, be->msg, error); + error = OFPERR_OFPBFC_MSG_FAILED; + /* 2. Revert. Undo all the changes made above. */ LIST_FOR_EACH_REVERSE_CONTINUE(be, node, &bundle->msg_list) { if (be->type == OFPTYPE_FLOW_MOD) { @@ -8119,13 +8149,34 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags) struct openflow_mod_requester req = { ofconn, be->msg }; if (be->type == OFPTYPE_FLOW_MOD) { - ofproto_flow_mod_finish(ofproto, &be->ofm, &req); + error = ofproto_flow_mod_finish(ofproto, &be->ofm, + &req); } else if (be->type == OFPTYPE_GROUP_MOD) { ofproto_group_mod_finish(ofproto, &be->ogm, &req); } else if (be->type == OFPTYPE_PACKET_OUT) { ofproto_packet_out_finish(ofproto, &be->opo); } } + if (error) { + break; + } + } + if (error) { + /* Send error referring to the original message. */ + ofconn_send_error(ofconn, be->msg, error); + error = OFPERR_OFPBFC_MSG_FAILED; + + /* Revert. Undo all the changes made above. */ + LIST_FOR_EACH_REVERSE_CONTINUE (be, node, &bundle->msg_list) { + if (be->type == OFPTYPE_FLOW_MOD) { + ofproto_flow_mod_revert(ofproto, &be->ofm); + } else if (be->type == OFPTYPE_GROUP_MOD) { + ofproto_group_mod_revert(ofproto, &be->ogm); + } else if (be->type == OFPTYPE_PACKET_OUT) { + ofproto_packet_out_revert(ofproto, &be->opo); + } + /* Nothing needs to be reverted for a port mod. */ + } } } diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index f0d387c..110789e 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -4443,7 +4443,7 @@ rule_construct(struct rule *rule_) return 0; } -static void +static enum ofperr rule_insert(struct rule *rule_, struct rule *old_rule_, bool forward_counts) OVS_REQUIRES(ofproto_mutex) { @@ -4473,6 +4473,8 @@ rule_insert(struct rule *rule_, struct rule *old_rule_, bool forward_counts) ovs_mutex_unlock(&rule->stats_mutex); ovs_mutex_unlock(&old_rule->stats_mutex); } + + return 0; } static void diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index eb62a8f..7907d4b 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -1329,8 +1329,8 @@ struct ofproto_class { struct rule *(*rule_alloc)(void); enum ofperr (*rule_construct)(struct rule *rule) /* OVS_REQUIRES(ofproto_mutex) */; - void (*rule_insert)(struct rule *rule, struct rule *old_rule, - bool forward_counts) + enum ofperr (*rule_insert)(struct rule *rule, struct rule *old_rule, + bool forward_counts) /* OVS_REQUIRES(ofproto_mutex) */; void (*rule_delete)(struct rule *rule) /* OVS_REQUIRES(ofproto_mutex) */; void (*rule_destruct)(struct rule *rule); @@ -1984,7 +1984,7 @@ enum ofperr ofproto_flow_mod_learn_start(struct ofproto_flow_mod *ofm) OVS_REQUIRES(ofproto_mutex); void ofproto_flow_mod_learn_revert(struct ofproto_flow_mod *ofm) OVS_REQUIRES(ofproto_mutex); -void ofproto_flow_mod_learn_finish(struct ofproto_flow_mod *ofm, +enum ofperr ofproto_flow_mod_learn_finish(struct ofproto_flow_mod *ofm, struct ofproto *orig_ofproto) OVS_REQUIRES(ofproto_mutex); void ofproto_add_flow(struct ofproto *, const struct match *, int priority, diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 3579aac..1d6fc00 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -247,10 +247,10 @@ static void replace_rule_revert(struct ofproto *, struct rule *old_rule, struct rule *new_rule) OVS_REQUIRES(ofproto_mutex); -static void replace_rule_finish(struct ofproto *, struct ofproto_flow_mod *, - const struct openflow_mod_requester *, - struct rule *old_rule, struct rule *new_rule, - struct ovs_list *dead_cookies) +static enum ofperr replace_rule_finish(struct ofproto *, struct ofproto_flow_mod *, + const struct openflow_mod_requester *, + struct rule *old_rule, struct rule *new_rule, + struct ovs_list *dead_cookies) OVS_REQUIRES(ofproto_mutex); static void delete_flows__(struct rule_collection *, enum ofp_flow_removed_reason, @@ -272,9 +272,9 @@ static enum ofperr ofproto_flow_mod_start(struct ofproto *,