| Message ID | 20180712180447.5015-1-raja.avi@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [ovs-dev] ofproto: Return error codes for Rule insertions | expand |
Hi Aaron/Ben/All, If possible, Kindly review the patch and let me know your views. On Thu, Jul 12, 2018 at 11:34 PM, Aravind Prasad S <raja.avi@gmail.com> wrote: > 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. > > Signed-off-by: Aravind Prasad S <raja.avi@gmail.com> > --- > ofproto/ofproto-dpif.c | 4 +- > ofproto/ofproto-provider.h | 6 +-- > ofproto/ofproto.c | 105 ++++++++++++++++++++++++++++++ > ++++----------- > 3 files changed, 85 insertions(+), 30 deletions(-) > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index ad1e8af..d1678ed 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 2b77b89..3f3d110 100644 > --- a/ofproto/ofproto-provider.h > +++ b/ofproto/ofproto-provider.h > @@ -1297,8 +1297,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); > @@ -1952,7 +1952,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 f946e27..cb09ee6 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -245,10 +245,12 @@ 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, > @@ -270,7 +272,7 @@ static enum ofperr ofproto_flow_mod_start(struct > ofproto *, > static void ofproto_flow_mod_revert(struct ofproto *, > struct ofproto_flow_mod *) > OVS_REQUIRES(ofproto_mutex); > -static void ofproto_flow_mod_finish(struct ofproto *, > +static enum ofperr ofproto_flow_mod_finish(struct ofproto *, > struct ofproto_flow_mod *, > const struct openflow_mod_requester *) > OVS_REQUIRES(ofproto_mutex); > @@ -4855,7 +4857,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) > @@ -4864,8 +4866,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) { > @@ -4878,6 +4886,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. */ > @@ -5074,22 +5084,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 > @@ -5144,7 +5157,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); > @@ -5244,7 +5257,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, > @@ -5252,6 +5265,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; > @@ -5261,8 +5276,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) { > @@ -5298,6 +5320,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. */ > @@ -5448,17 +5472,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); > > @@ -5467,12 +5492,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 > @@ -5838,7 +5868,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); > @@ -7668,19 +7698,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: > @@ -7698,6 +7730,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): > @@ -7781,10 +7815,8 @@ 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) { > @@ -7827,13 +7859,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. */ > + } > } > } > > -- > 1.9.1 > >
Hi Ben/Aaron,All, Kindly review the patch and let me know your views. On Thu, Jul 12, 2018 at 11:34 PM, Aravind Prasad S <raja.avi@gmail.com> wrote: > 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. > > Signed-off-by: Aravind Prasad S <raja.avi@gmail.com> > --- > ofproto/ofproto-dpif.c | 4 +- > ofproto/ofproto-provider.h | 6 +-- > ofproto/ofproto.c | 105 ++++++++++++++++++++++++++++++ > ++++----------- > 3 files changed, 85 insertions(+), 30 deletions(-) > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index ad1e8af..d1678ed 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 2b77b89..3f3d110 100644 > --- a/ofproto/ofproto-provider.h > +++ b/ofproto/ofproto-provider.h > @@ -1297,8 +1297,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); > @@ -1952,7 +1952,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 f946e27..cb09ee6 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -245,10 +245,12 @@ 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, > @@ -270,7 +272,7 @@ static enum ofperr ofproto_flow_mod_start(struct > ofproto *, > static void ofproto_flow_mod_revert(struct ofproto *, > struct ofproto_flow_mod *) > OVS_REQUIRES(ofproto_mutex); > -static void ofproto_flow_mod_finish(struct ofproto *, > +static enum ofperr ofproto_flow_mod_finish(struct ofproto *, > struct ofproto_flow_mod *, > const struct openflow_mod_requester *) > OVS_REQUIRES(ofproto_mutex); > @@ -4855,7 +4857,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) > @@ -4864,8 +4866,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) { > @@ -4878,6 +4886,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. */ > @@ -5074,22 +5084,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 > @@ -5144,7 +5157,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); > @@ -5244,7 +5257,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, > @@ -5252,6 +5265,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; > @@ -5261,8 +5276,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) { > @@ -5298,6 +5320,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. */ > @@ -5448,17 +5472,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); > > @@ -5467,12 +5492,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 > @@ -5838,7 +5868,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); > @@ -7668,19 +7698,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: > @@ -7698,6 +7730,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): > @@ -7781,10 +7815,8 @@ 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) { > @@ -7827,13 +7859,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. */ > + } > } > } > > -- > 1.9.1 > >
Hi Ben/Aaron/All, Kindly review the patch and let me know your views. On Thu, Jul 12, 2018 at 11:34 PM, Aravind Prasad S <raja.avi@gmail.com> wrote: > 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. > > Signed-off-by: Aravind Prasad S <raja.avi@gmail.com> > --- > ofproto/ofproto-dpif.c | 4 +- > ofproto/ofproto-provider.h | 6 +-- > ofproto/ofproto.c | 105 ++++++++++++++++++++++++++++++ > ++++----------- > 3 files changed, 85 insertions(+), 30 deletions(-) > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index ad1e8af..d1678ed 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 2b77b89..3f3d110 100644 > --- a/ofproto/ofproto-provider.h > +++ b/ofproto/ofproto-provider.h > @@ -1297,8 +1297,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); > @@ -1952,7 +1952,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 f946e27..cb09ee6 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -245,10 +245,12 @@ 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, > @@ -270,7 +272,7 @@ static enum ofperr ofproto_flow_mod_start(struct > ofproto *, > static void ofproto_flow_mod_revert(struct ofproto *, > struct ofproto_flow_mod *) > OVS_REQUIRES(ofproto_mutex); > -static void ofproto_flow_mod_finish(struct ofproto *, > +static enum ofperr ofproto_flow_mod_finish(struct ofproto *, > struct ofproto_flow_mod *, > const struct openflow_mod_requester *) > OVS_REQUIRES(ofproto_mutex); > @@ -4855,7 +4857,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) > @@ -4864,8 +4866,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) { > @@ -4878,6 +4886,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. */ > @@ -5074,22 +5084,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 > @@ -5144,7 +5157,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); > @@ -5244,7 +5257,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, > @@ -5252,6 +5265,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; > @@ -5261,8 +5276,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) { > @@ -5298,6 +5320,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. */ > @@ -5448,17 +5472,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); > > @@ -5467,12 +5492,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 > @@ -5838,7 +5868,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); > @@ -7668,19 +7698,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: > @@ -7698,6 +7730,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): > @@ -7781,10 +7815,8 @@ 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) { > @@ -7827,13 +7859,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. */ > + } > } > } > > -- > 1.9.1 > >
Hi Aaron/Ben/All, If possible, Kindly review the patch and let me know your views. On Thu, Jul 12, 2018 at 11:34 PM, Aravind Prasad S <raja.avi@gmail.com> wrote: > 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. > > Signed-off-by: Aravind Prasad S <raja.avi@gmail.com> > --- > ofproto/ofproto-dpif.c | 4 +- > ofproto/ofproto-provider.h | 6 +-- > ofproto/ofproto.c | 105 ++++++++++++++++++++++++++++++ > ++++----------- > 3 files changed, 85 insertions(+), 30 deletions(-) > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index ad1e8af..d1678ed 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 2b77b89..3f3d110 100644 > --- a/ofproto/ofproto-provider.h > +++ b/ofproto/ofproto-provider.h > @@ -1297,8 +1297,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); > @@ -1952,7 +1952,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 f946e27..cb09ee6 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -245,10 +245,12 @@ 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, > @@ -270,7 +272,7 @@ static enum ofperr ofproto_flow_mod_start(struct > ofproto *, > static void ofproto_flow_mod_revert(struct ofproto *, > struct ofproto_flow_mod *) > OVS_REQUIRES(ofproto_mutex); > -static void ofproto_flow_mod_finish(struct ofproto *, > +static enum ofperr ofproto_flow_mod_finish(struct ofproto *, > struct ofproto_flow_mod *, > const struct openflow_mod_requester *) > OVS_REQUIRES(ofproto_mutex); > @@ -4855,7 +4857,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) > @@ -4864,8 +4866,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) { > @@ -4878,6 +4886,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. */ > @@ -5074,22 +5084,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 > @@ -5144,7 +5157,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); > @@ -5244,7 +5257,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, > @@ -5252,6 +5265,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; > @@ -5261,8 +5276,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) { > @@ -5298,6 +5320,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. */ > @@ -5448,17 +5472,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); > > @@ -5467,12 +5492,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 > @@ -5838,7 +5868,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); > @@ -7668,19 +7698,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: > @@ -7698,6 +7730,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): > @@ -7781,10 +7815,8 @@ 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) { > @@ -7827,13 +7859,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. */ > + } > } > } > > -- > 1.9.1 > >
Hi Ben/Aaron/All, Kindly review the patch and let me know your views. On Thu, Jul 12, 2018 at 11:34 PM, Aravind Prasad S <raja.avi@gmail.com> wrote: > 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. > > Signed-off-by: Aravind Prasad S <raja.avi@gmail.com> > --- > ofproto/ofproto-dpif.c | 4 +- > ofproto/ofproto-provider.h | 6 +-- > ofproto/ofproto.c | 105 ++++++++++++++++++++++++++++++ > ++++----------- > 3 files changed, 85 insertions(+), 30 deletions(-) > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index ad1e8af..d1678ed 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 2b77b89..3f3d110 100644 > --- a/ofproto/ofproto-provider.h > +++ b/ofproto/ofproto-provider.h > @@ -1297,8 +1297,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); > @@ -1952,7 +1952,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 f946e27..cb09ee6 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -245,10 +245,12 @@ 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, > @@ -270,7 +272,7 @@ static enum ofperr ofproto_flow_mod_start(struct > ofproto *, > static void ofproto_flow_mod_revert(struct ofproto *, > struct ofproto_flow_mod *) > OVS_REQUIRES(ofproto_mutex); > -static void ofproto_flow_mod_finish(struct ofproto *, > +static enum ofperr ofproto_flow_mod_finish(struct ofproto *, > struct ofproto_flow_mod *, > const struct openflow_mod_requester *) > OVS_REQUIRES(ofproto_mutex); > @@ -4855,7 +4857,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) > @@ -4864,8 +4866,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) { > @@ -4878,6 +4886,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. */ > @@ -5074,22 +5084,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 > @@ -5144,7 +5157,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); > @@ -5244,7 +5257,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, > @@ -5252,6 +5265,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; > @@ -5261,8 +5276,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) { > @@ -5298,6 +5320,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. */ > @@ -5448,17 +5472,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); > > @@ -5467,12 +5492,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 > @@ -5838,7 +5868,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); > @@ -7668,19 +7698,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: > @@ -7698,6 +7730,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): > @@ -7781,10 +7815,8 @@ 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) { > @@ -7827,13 +7859,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. */ > + } > } > } > > -- > 1.9.1 > >
On Thu, Jul 12, 2018 at 06:04:47PM +0000, Aravind Prasad S wrote: > 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. > > Signed-off-by: Aravind Prasad S <raja.avi@gmail.com> Which switches does this help?
It is super annoying to send a nagging message every day. Do not do it. On Fri, Jul 13, 2018 at 08:03:22AM +0530, Aravind Prasad wrote: > Hi Aaron/Ben/All, > If possible, Kindly review the patch and let me know > your views. > > On Thu, Jul 12, 2018 at 11:34 PM, Aravind Prasad S <raja.avi@gmail.com> > wrote: > > > 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. > > > > Signed-off-by: Aravind Prasad S <raja.avi@gmail.com> > > --- > > ofproto/ofproto-dpif.c | 4 +- > > ofproto/ofproto-provider.h | 6 +-- > > ofproto/ofproto.c | 105 ++++++++++++++++++++++++++++++ > > ++++----------- > > 3 files changed, 85 insertions(+), 30 deletions(-) > > > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > > index ad1e8af..d1678ed 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 2b77b89..3f3d110 100644 > > --- a/ofproto/ofproto-provider.h > > +++ b/ofproto/ofproto-provider.h > > @@ -1297,8 +1297,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); > > @@ -1952,7 +1952,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 f946e27..cb09ee6 100644 > > --- a/ofproto/ofproto.c > > +++ b/ofproto/ofproto.c > > @@ -245,10 +245,12 @@ 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, > > @@ -270,7 +272,7 @@ static enum ofperr ofproto_flow_mod_start(struct > > ofproto *, > > static void ofproto_flow_mod_revert(struct ofproto *, > > struct ofproto_flow_mod *) > > OVS_REQUIRES(ofproto_mutex); > > -static void ofproto_flow_mod_finish(struct ofproto *, > > +static enum ofperr ofproto_flow_mod_finish(struct ofproto *, > > struct ofproto_flow_mod *, > > const struct openflow_mod_requester *) > > OVS_REQUIRES(ofproto_mutex); > > @@ -4855,7 +4857,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) > > @@ -4864,8 +4866,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) { > > @@ -4878,6 +4886,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. */ > > @@ -5074,22 +5084,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 > > @@ -5144,7 +5157,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); > > @@ -5244,7 +5257,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, > > @@ -5252,6 +5265,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; > > @@ -5261,8 +5276,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) { > > @@ -5298,6 +5320,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. */ > > @@ -5448,17 +5472,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); > > > > @@ -5467,12 +5492,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 > > @@ -5838,7 +5868,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); > > @@ -7668,19 +7698,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: > > @@ -7698,6 +7730,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): > > @@ -7781,10 +7815,8 @@ 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) { > > @@ -7827,13 +7859,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. */ > > + } > > } > > } > > > > -- > > 1.9.1 > > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 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. > > Signed-off-by: Aravind Prasad S <raja.avi@gmail.com> >Which switches does this help? Hi Ben, These type of errors are possible in actual Hardware implementations of OVS. It is possible that ofproto and netdev providers be implemented for an actual HW/NPU. Usually, in such cases, in the rule construct phase, all the static checks like verifying the qualifiers/ actions, CAM full checks could be done and the other related verifications. But during the rule insert phase, it is possible that the rule insertion may get failed in HW (runtime errors, HW errors and so on). Also, since HW switches may support Hybrid mode (coexistence of Normal and Openflow functioning), the possibility of this issue could be even more. Further, when rule-insertion fails, it results in a stale state where the Controller and Switch Flow-DB differ. Hence, we need a way to rollback for rule-insert phase also. Kindly let me know your views. And sorry for re-sending the review requests many times over. Will avoid in future. On Fri, Jul 20, 2018 at 10:20 PM, Ben Pfaff <blp@ovn.org> wrote: On Thu, Jul 12, 2018 at 06:04:47PM +0000, Aravind Prasad S wrote: > > 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. > > > > Signed-off-by: Aravind Prasad S <raja.avi@gmail.com> > > Which switches does this help? >
> 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. > > Signed-off-by: Aravind Prasad S <raja.avi@gmail.com> >Which switches does this help? >Hi Ben, >These type of errors are possible in actual Hardware implementations >of OVS. It is possible that ofproto and netdev providers be >implemented for an actual HW/NPU. Usually, in such cases, in the rule >construct phase, all the static checks like verifying the qualifiers/ >actions, CAM full checks could be done and the other related >verifications. >But during the rule insert phase, it is possible that >the rule insertion may get failed in HW (runtime errors, >HW errors and so on). Also, since HW switches may support Hybrid >mode (coexistence of Normal and Openflow functioning), the >possibility of this issue could be even more. Further, when >rule-insertion fails, it results in a stale state where the >Controller and Switch Flow-DB differ. >Hence, we need a way to rollback for rule-insert phase also. >Kindly let me know your views. >And sorry for re-sending the review requests many times over. >Will avoid in future. Hi Ben/All, Kindly let me know your views. On Sat, Jul 21, 2018 at 9:22 AM, Aravind Prasad <raja.avi@gmail.com> wrote: > > > 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. > > > > Signed-off-by: Aravind Prasad S <raja.avi@gmail.com> > > >Which switches does this help? > > Hi Ben, These type of errors are possible in actual Hardware > implementations of OVS. It is possible that ofproto and netdev providers be > implemented for an actual HW/NPU. Usually, in such cases, in the rule > construct phase, all the static checks like verifying the qualifiers/ > actions, CAM full checks could be done and the other related verifications. > But during the rule insert phase, it is possible that the rule insertion > may get failed in HW (runtime errors, HW errors and so on). Also, since HW > switches may support Hybrid mode (coexistence of Normal and Openflow > functioning), the possibility of this issue could be even more. Further, > when rule-insertion fails, it results in a stale state where the Controller > and Switch Flow-DB differ. Hence, we need a way to rollback for rule-insert > phase also. Kindly let me know your views. And sorry for re-sending the > review requests many times over. Will avoid in future. > > > On Fri, Jul 20, 2018 at 10:20 PM, Ben Pfaff <blp@ovn.org> wrote: > > On Thu, Jul 12, 2018 at 06:04:47PM +0000, Aravind Prasad S wrote: >> > 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. >> > >> > Signed-off-by: Aravind Prasad S <raja.avi@gmail.com> >> >> Which switches does this help? >> > >
> 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. > > Signed-off-by: Aravind Prasad S <raja.avi@gmail.com> > Which switches does this help? > Hi Ben, > These type of errors are possible in actual Hardware implementations > of OVS. It is possible that ofproto and netdev providers be > implemented for an actual HW/NPU. Usually, in such cases, in the rule > construct phase, all the static checks like verifying the qualifiers/ > actions, CAM full checks could be done and the other related > verifications. > But during the rule insert phase, it is possible that > the rule insertion may get failed in HW (runtime errors, > HW errors and so on). Also, since HW switches may support Hybrid > mode (coexistence of Normal and Openflow functioning), the > possibility of this issue could be even more. Further, when > rule-insertion fails, it results in a stale state where the > Controller and Switch Flow-DB differ. > Hence, we need a way to rollback for rule-insert phase also. > Kindly let me know your views. Hi Ben/All, Kindly review and let me know your views. On Tue, Jul 24, 2018 at 5:15 AM, Aravind Prasad <raja.avi@gmail.com> wrote: > > 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. > > > > Signed-off-by: Aravind Prasad S <raja.avi@gmail.com> > > >Which switches does this help? > > >Hi Ben, > >These type of errors are possible in actual Hardware implementations > >of OVS. It is possible that ofproto and netdev providers be > >implemented for an actual HW/NPU. Usually, in such cases, in the rule > >construct phase, all the static checks like verifying the qualifiers/ > >actions, CAM full checks could be done and the other related > >verifications. > >But during the rule insert phase, it is possible that > >the rule insertion may get failed in HW (runtime errors, > >HW errors and so on). Also, since HW switches may support Hybrid > >mode (coexistence of Normal and Openflow functioning), the > >possibility of this issue could be even more. Further, when > >rule-insertion fails, it results in a stale state where the > >Controller and Switch Flow-DB differ. > >Hence, we need a way to rollback for rule-insert phase also. > >Kindly let me know your views. > >And sorry for re-sending the review requests many times over. > >Will avoid in future. > > Hi Ben/All, > > Kindly let me know your views. > > > On Sat, Jul 21, 2018 at 9:22 AM, Aravind Prasad <raja.avi@gmail.com> > wrote: > >> >> > 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. >> > >> > Signed-off-by: Aravind Prasad S <raja.avi@gmail.com> >> >> >Which switches does this help? >> >> Hi Ben, These type of errors are possible in actual Hardware >> implementations of OVS. It is possible that ofproto and netdev providers be >> implemented for an actual HW/NPU. Usually, in such cases, in the rule >> construct phase, all the static checks like verifying the qualifiers/ >> actions, CAM full checks could be done and the other related verifications. >> But during the rule insert phase, it is possible that the rule insertion >> may get failed in HW (runtime errors, HW errors and so on). Also, since HW >> switches may support Hybrid mode (coexistence of Normal and Openflow >> functioning), the possibility of this issue could be even more. Further, >> when rule-insertion fails, it results in a stale state where the Controller >> and Switch Flow-DB differ. Hence, we need a way to rollback for rule-insert >> phase also. Kindly let me know your views. And sorry for re-sending the >> review requests many times over. Will avoid in future. >> >> >> On Fri, Jul 20, 2018 at 10:20 PM, Ben Pfaff <blp@ovn.org> wrote: >> >> On Thu, Jul 12, 2018 at 06:04:47PM +0000, Aravind Prasad S wrote: >>> > 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. >>> > >>> > Signed-off-by: Aravind Prasad S <raja.avi@gmail.com> >>> >>> Which switches does this help? >>> >> >> >
> 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. > > Signed-off-by: Aravind Prasad S <raja.avi at gmail.com> > Which switches does this help? > Hi Ben, > These type of errors are possible in actual Hardware implementations > of OVS. It is possible that ofproto and netdev providers be > implemented for an actual HW/NPU. Usually, in such cases, in the rule > construct phase, all the static checks like verifying the qualifiers/ > actions, CAM full checks could be done and the other related > verifications. > But during the rule insert phase, it is possible that > the rule insertion may get failed in HW (runtime errors, > HW errors and so on). Also, since HW switches may support Hybrid > mode (coexistence of Normal and Openflow functioning), the > possibility of this issue could be even more. Further, when > rule-insertion fails, it results in a stale state where the > Controller and Switch Flow-DB differ. > Hence, we need a way to rollback for rule-insert phase also. > Kindly let me know your views. Hi Ben/All, Kindly review and let me know your views. Sorry for the previous junk mail. On Sat, Jul 21, 2018 at 9:22 AM, Aravind Prasad <raja.avi@gmail.com> wrote: > > > 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. > > > > Signed-off-by: Aravind Prasad S <raja.avi@gmail.com> > > >Which switches does this help? > > Hi Ben, These type of errors are possible in actual Hardware > implementations of OVS. It is possible that ofproto and netdev providers be > implemented for an actual HW/NPU. Usually, in such cases, in the rule > construct phase, all the static checks like verifying the qualifiers/ > actions, CAM full checks could be done and the other related verifications. > But during the rule insert phase, it is possible that the rule insertion > may get failed in HW (runtime errors, HW errors and so on). Also, since HW > switches may support Hybrid mode (coexistence of Normal and Openflow > functioning), the possibility of this issue could be even more. Further, > when rule-insertion fails, it results in a stale state where the Controller > and Switch Flow-DB differ. Hence, we need a way to rollback for rule-insert > phase also. Kindly let me know your views. And sorry for re-sending the > review requests many times over. Will avoid in future. > > > On Fri, Jul 20, 2018 at 10:20 PM, Ben Pfaff <blp@ovn.org> wrote: > > On Thu, Jul 12, 2018 at 06:04:47PM +0000, Aravind Prasad S wrote: >> > 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. >> > >> > Signed-off-by: Aravind Prasad S <raja.avi@gmail.com> >> >> Which switches does this help? >> > >
On Sat, Jul 21, 2018 at 09:22:06AM +0530, Aravind Prasad wrote: > > 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. > > > > Signed-off-by: Aravind Prasad S <raja.avi@gmail.com> > > >Which switches does this help? > > Hi Ben, These type of errors are possible in actual Hardware > implementations of OVS. It is possible that ofproto and netdev providers be > implemented for an actual HW/NPU. Usually, in such cases, in the rule > construct phase, all the static checks like verifying the qualifiers/ > actions, CAM full checks could be done and the other related verifications. > But during the rule insert phase, it is possible that the rule insertion > may get failed in HW (runtime errors, HW errors and so on). Also, since HW > switches may support Hybrid mode (coexistence of Normal and Openflow > functioning), the possibility of this issue could be even more. Further, > when rule-insertion fails, it results in a stale state where the Controller > and Switch Flow-DB differ. Hence, we need a way to rollback for rule-insert > phase also. Kindly let me know your views. And sorry for re-sending the > review requests many times over. Will avoid in future. Which switches does this help?
> > 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. > > > > Signed-off-by: Aravind Prasad S <raja.avi@gmail.com> > > >Which switches does this help? > > Hi Ben, These type of errors are possible in actual Hardware > implementations of OVS. It is possible that ofproto and netdev providers be > implemented for an actual HW/NPU. Usually, in such cases, in the rule > construct phase, all the static checks like verifying the qualifiers/ > actions, CAM full checks could be done and the other related verifications. > But during the rule insert phase, it is possible that the rule insertion > may get failed in HW (runtime errors, HW errors and so on). Also, since HW > switches may support Hybrid mode (coexistence of Normal and Openflow > functioning), the possibility of this issue could be even more. Further, > when rule-insertion fails, it results in a stale state where the Controller > and Switch Flow-DB differ. Hence, we need a way to rollback for rule-insert > phase also. Kindly let me know your views. And sorry for re-sending the > review requests many times over. Will avoid in future. > >Which switches does this help? Hi Ben, By "which" switches, you mean to ask to which vendor switches ? In general, this patch will be very useful for all Switches which implement OVS for their HW implementations, specifically, will be useful for Switches which had implemented ofproto and netdev provider APIs for their respective HW/NPU and Kernel. And, in case of HW, the possibility of dynamic rule insertion failures are usually higher. Static checks during rule-construct phase are not sufficient and predicting future failure predictions in construct phase may not be possible. Hence, this patch will make OVS more flexible to handle such rule insertion failures.
On Wed, Aug 01, 2018 at 07:29:06AM +0530, Aravind Prasad wrote: > > > 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. > > > > > > Signed-off-by: Aravind Prasad S <raja.avi@gmail.com> > > > > >Which switches does this help? > > > > Hi Ben, These type of errors are possible in actual Hardware > > implementations of OVS. It is possible that ofproto and netdev providers > be > > implemented for an actual HW/NPU. Usually, in such cases, in the rule > > construct phase, all the static checks like verifying the qualifiers/ > > actions, CAM full checks could be done and the other related > verifications. > > But during the rule insert phase, it is possible that the rule insertion > > may get failed in HW (runtime errors, HW errors and so on). Also, since HW > > switches may support Hybrid mode (coexistence of Normal and Openflow > > functioning), the possibility of this issue could be even more. Further, > > when rule-insertion fails, it results in a stale state where the > Controller > > and Switch Flow-DB differ. Hence, we need a way to rollback for > rule-insert > > phase also. Kindly let me know your views. And sorry for re-sending the > > review requests many times over. Will avoid in future. > > > >Which switches does this help? > > Hi Ben, > > By "which" switches, you mean to ask to which vendor switches ? > In general, this patch will be very useful for all Switches which > implement OVS for their HW implementations, specifically, > will be useful for Switches which had implemented ofproto > and netdev provider APIs for their respective HW/NPU and > Kernel. And, in case of HW, the possibility of dynamic rule > insertion failures are usually higher. Static checks during > rule-construct phase are not sufficient and predicting > future failure predictions in construct phase may not be possible. > Hence, this patch will make OVS more flexible to handle such > rule insertion failures. So, basically, you're not willing to name any switches. I'm tired of the vendor secrecy game. They get to see all of our code but they're never willing even to name themselves. Sorry, I'm not going to willingly take this patch.
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index ad1e8af..d1678ed 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 2b77b89..3f3d110 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -1297,8 +1297,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); @@ -1952,7 +1952,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 f946e27..cb09ee6 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -245,10 +245,12 @@ 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, @@ -270,7 +272,7 @@ static enum ofperr ofproto_flow_mod_start(struct ofproto *, static void ofproto_flow_mod_revert(struct ofproto *, struct ofproto_flow_mod *) OVS_REQUIRES(ofproto_mutex); -static void ofproto_flow_mod_finish(struct ofproto *, +static enum ofperr ofproto_flow_mod_finish(struct ofproto *, struct ofproto_flow_mod *, const struct openflow_mod_requester *) OVS_REQUIRES(ofproto_mutex); @@ -4855,7 +4857,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) @@ -4864,8 +4866,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) { @@ -4878,6 +4886,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. */ @@ -5074,22 +5084,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 @@ -5144,7 +5157,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); @@ -5244,7 +5257,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, @@ -5252,6 +5265,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; @@ -5261,8 +5276,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) { @@ -5298,6 +5320,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. */ @@ -5448,17 +5472,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); @@ -5467,12 +5492,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 @@ -5838,7 +5868,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); @@ -7668,19 +7698,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: @@ -7698,6 +7730,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): @@ -7781,10 +7815,8 @@ 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) { @@ -7827,13 +7859,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. */ + } } }
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. Signed-off-by: Aravind Prasad S <raja.avi@gmail.com> --- ofproto/ofproto-dpif.c | 4 +- ofproto/ofproto-provider.h | 6 +-- ofproto/ofproto.c | 105 ++++++++++++++++++++++++++++++++++----------- 3 files changed, 85 insertions(+), 30 deletions(-)