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....@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....@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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
