I posted this review on the wrong patch, but here it is on the right one:
FWIW I think this is the right choice given the options. May be worth
adding a comment in the actual code as to why we can't just execute
the rule and have to queue it instead.
Acked-by: Ethan Jackson <et...@nicira.com>
On Tue, Sep 10, 2013 at 10:27 PM, Ben Pfaff <b...@nicira.com> wrote:
> One goal we're moving toward is to be able to execute "learn" actions
> in each thread that handles packets that arrive on an interface just before
> we forward those packets. The planned strategy there is to have a global
> lock that protects everything required to modify the flow table. Generally
> this works out well, but rule_execute() is a trouble spot. That's because
> it's called during a flow table modification (when that global lock is
> held) which means that a "learn" action triggered by the executing the
> packet would try to recursively modify the flow table and reacquire the
> global lock.
>
> I can see two ways out of this issue. One would be to make the global lock
> a recursive one, or otherwise deal with handling recursive flow_mods. The
> other is to just queue up flow_mods due to rule_execute(), which itself is
> a corner case (it only happens when a flow_mod sent via OpenFlow includes
> a buffer_id). (I guess there could be other more radical solutions, like
> just dropping packets that contain "learn" actions if they occur in this
> situation.)
>
> This commit implements the second solution because it seems less likely to
> be wrong in a way that crashes the switch.
>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> ofproto/ofproto-provider.h | 4 ++
> ofproto/ofproto.c | 141
> +++++++++++++++++++++++++++++++++-----------
> 2 files changed, 109 insertions(+), 36 deletions(-)
>
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 7c71aad..d23ff9c 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -21,6 +21,7 @@
>
> #include "cfm.h"
> #include "classifier.h"
> +#include "guarded-list.h"
> #include "heap.h"
> #include "hindex.h"
> #include "list.h"
> @@ -98,6 +99,9 @@ struct ofproto {
> unsigned int n_pending; /* list_size(&pending). */
> struct hmap deletions; /* All OFOPERATION_DELETE "ofoperation"s. */
>
> + /* Delayed rule executions. */
> + struct guarded_list rule_executes;
> +
> /* Flow table operation logging. */
> int n_add, n_delete, n_modify; /* Number of unreported ops of each kind.
> */
> long long int first_op, last_op; /* Range of times for unreported ops. */
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 7cdca26..10c803c 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -188,6 +188,17 @@ static uint32_t rule_eviction_priority(struct rule *);
> static void eviction_group_add_rule(struct rule *);
> static void eviction_group_remove_rule(struct rule *);
>
> +/* A packet that needs to be passed to rule_execute(). */
> +struct rule_execute {
> + struct list list_node; /* In struct ofproto's "rule_executes" list.
> */
> + struct rule *rule; /* Owns a reference to the rule. */
> + ofp_port_t in_port;
> + struct ofpbuf *packet; /* Owns the packet. */
> +};
> +
> +static void run_rule_executes(struct ofproto *);
> +static void destroy_rule_executes(struct ofproto *);
> +
> /* ofport. */
> static void ofport_destroy__(struct ofport *);
> static void ofport_destroy(struct ofport *);
> @@ -449,6 +460,7 @@ ofproto_create(const char *datapath_name, const char
> *datapath_type,
> list_init(&ofproto->pending);
> ofproto->n_pending = 0;
> hmap_init(&ofproto->deletions);
> + guarded_list_init(&ofproto->rule_executes);
> ofproto->n_add = ofproto->n_delete = ofproto->n_modify = 0;
> ofproto->first_op = ofproto->last_op = LLONG_MIN;
> ofproto->next_op_report = LLONG_MAX;
> @@ -1146,6 +1158,9 @@ ofproto_destroy__(struct ofproto *ofproto)
> ovs_assert(list_is_empty(&ofproto->pending));
> ovs_assert(!ofproto->n_pending);
>
> + destroy_rule_executes(ofproto);
> + guarded_list_destroy(&ofproto->rule_executes);
> +
> if (ofproto->meters) {
> meter_delete(ofproto, 1, ofproto->meter_features.max_meters);
> free(ofproto->meters);
> @@ -1287,6 +1302,8 @@ ofproto_run(struct ofproto *p)
> VLOG_ERR_RL(&rl, "%s: run failed (%s)", p->name,
> ovs_strerror(error));
> }
>
> + run_rule_executes(p);
> +
> /* Restore the eviction group heap invariant occasionally. */
> if (p->eviction_group_timer < time_msec()) {
> size_t i;
> @@ -2416,22 +2433,48 @@ ofoperation_has_out_port(const struct ofoperation
> *op, ofp_port_t out_port)
> NOT_REACHED();
> }
>
> -/* Executes the actions indicated by 'rule' on 'packet' and credits 'rule''s
> - * statistics appropriately.
> - *
> - * 'packet' doesn't necessarily have to match 'rule'. 'rule' will be
> credited
> - * with statistics for 'packet' either way.
> - *
> - * Takes ownership of 'packet'. */
> -static int
> -rule_execute(struct rule *rule, ofp_port_t in_port, struct ofpbuf *packet)
> +static void
> +rule_execute_destroy(struct rule_execute *e)
> {
> - struct flow flow;
> - union flow_in_port in_port_;
> + ofproto_rule_unref(e->rule);
> + list_remove(&e->list_node);
> + free(e);
> +}
> +
> +/* Executes all "rule_execute" operations queued up in
> ofproto->rule_executes,
> + * by passing them to the ofproto provider. */
> +static void
> +run_rule_executes(struct ofproto *ofproto)
> +{
> + struct rule_execute *e, *next;
> + struct list executes;
> +
> + guarded_list_steal_all(&ofproto->rule_executes, &executes);
> + LIST_FOR_EACH_SAFE (e, next, list_node, &executes) {
> + union flow_in_port in_port_;
> + struct flow flow;
> +
> + in_port_.ofp_port = e->in_port;
> + flow_extract(e->packet, 0, 0, NULL, &in_port_, &flow);
> + ofproto->ofproto_class->rule_execute(e->rule, &flow, e->packet);
>
> - in_port_.ofp_port = in_port;
> - flow_extract(packet, 0, 0, NULL, &in_port_, &flow);
> - return rule->ofproto->ofproto_class->rule_execute(rule, &flow, packet);
> + rule_execute_destroy(e);
> + }
> +}
> +
> +/* Destroys and discards all "rule_execute" operations queued up in
> + * ofproto->rule_executes. */
> +static void
> +destroy_rule_executes(struct ofproto *ofproto)
> +{
> + struct rule_execute *e, *next;
> + struct list executes;
> +
> + guarded_list_steal_all(&ofproto->rule_executes, &executes);
> + LIST_FOR_EACH_SAFE (e, next, list_node, &executes) {
> + ofpbuf_delete(e->packet);
> + rule_execute_destroy(e);
> + }
> }
>
> /* Returns true if 'rule' should be hidden from the controller.
> @@ -4027,35 +4070,46 @@ static enum ofperr
> handle_flow_mod__(struct ofproto *ofproto, struct ofconn *ofconn,
> struct ofputil_flow_mod *fm, const struct ofp_header *oh)
> {
> - if (ofproto->n_pending >= 50) {
> - ovs_assert(!list_is_empty(&ofproto->pending));
> - return OFPROTO_POSTPONE;
> - }
> + enum ofperr error;
>
> - switch (fm->command) {
> - case OFPFC_ADD:
> - return add_flow(ofproto, ofconn, fm, oh);
> + if (ofproto->n_pending < 50) {
> + switch (fm->command) {
> + case OFPFC_ADD:
> + error = add_flow(ofproto, ofconn, fm, oh);
> + break;
>
> - case OFPFC_MODIFY:
> - return modify_flows_loose(ofproto, ofconn, fm, oh);
> + case OFPFC_MODIFY:
> + error = modify_flows_loose(ofproto, ofconn, fm, oh);
> + break;
>
> - case OFPFC_MODIFY_STRICT:
> - return modify_flow_strict(ofproto, ofconn, fm, oh);
> + case OFPFC_MODIFY_STRICT:
> + error = modify_flow_strict(ofproto, ofconn, fm, oh);
> + break;
>
> - case OFPFC_DELETE:
> - return delete_flows_loose(ofproto, ofconn, fm, oh);
> + case OFPFC_DELETE:
> + error = delete_flows_loose(ofproto, ofconn, fm, oh);
> + break;
>
> - case OFPFC_DELETE_STRICT:
> - return delete_flow_strict(ofproto, ofconn, fm, oh);
> + case OFPFC_DELETE_STRICT:
> + error = delete_flow_strict(ofproto, ofconn, fm, oh);
> + break;
>
> - default:
> - if (fm->command > 0xff) {
> - VLOG_WARN_RL(&rl, "%s: flow_mod has explicit table_id but "
> - "flow_mod_table_id extension is not enabled",
> - ofproto->name);
> + default:
> + if (fm->command > 0xff) {
> + VLOG_WARN_RL(&rl, "%s: flow_mod has explicit table_id but "
> + "flow_mod_table_id extension is not enabled",
> + ofproto->name);
> + }
> + error = OFPERR_OFPFMFC_BAD_COMMAND;
> + break;
> }
> - return OFPERR_OFPFMFC_BAD_COMMAND;
> + } else {
> + ovs_assert(!list_is_empty(&ofproto->pending));
> + error = OFPROTO_POSTPONE;
> }
> +
> + run_rule_executes(ofproto);
> + return error;
> }
>
> static enum ofperr
> @@ -5480,8 +5534,23 @@ ofopgroup_complete(struct ofopgroup *group)
> error = ofconn_pktbuf_retrieve(group->ofconn,
> group->buffer_id,
> &packet, &in_port);
> if (packet) {
> + struct rule_execute *re;
> +
> ovs_assert(!error);
> - error = rule_execute(op->rule, in_port, packet);
> +
> + ofproto_rule_ref(op->rule);
> +
> + re = xmalloc(sizeof *re);
> + re->rule = op->rule;
> + re->in_port = in_port;
> + re->packet = packet;
> +
> + if (!guarded_list_append(&ofproto->rule_executes,
> + &re->list_node, 1024)) {
> + ofproto_rule_unref(op->rule);
> + ofpbuf_delete(re->packet);
> + free(re);
> + }
> }
> break;
> }
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
