This is analogous to the split between rule and rule_actions in ofproto. As there, it will allow retaining a reference to a rule's actions, while processing them, without having to retain a reference to the rule itself.
Signed-off-by: Ben Pfaff <b...@nicira.com> --- lib/dpif-netdev.c | 136 ++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 92 insertions(+), 44 deletions(-) diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 62837b0..d0f6dab 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -138,10 +138,34 @@ struct dp_netdev_flow { uint16_t tcp_flags; /* Bitwise-OR of seen tcp_flags values. */ /* Actions. */ - struct nlattr *actions; - size_t actions_len; + struct dp_netdev_actions *actions; }; +/* A set of datapath actions within a "struct dp_netdev_flow". + * + * + * Thread-safety + * ============= + * + * A struct dp_netdev_actions 'actions' may be accessed without a risk of being + * freed by code that holds a read-lock or write-lock on 'flow->mutex' (where + * 'flow' is the dp_netdev_flow for which 'flow->actions == actions') or that + * owns a reference to 'actions->ref_cnt' (or both). */ +struct dp_netdev_actions { + struct ovs_refcount ref_cnt; + + /* These members are immutable: they do not change during the struct's + * lifetime. */ + struct nlattr *actions; /* Sequence of OVS_ACTION_ATTR_* attributes. */ + unsigned int size; /* Size of 'actions', in bytes. */ +}; + +struct dp_netdev_actions *dp_netdev_actions_create(const struct nlattr *, + size_t); +struct dp_netdev_actions *dp_netdev_actions_ref( + const struct dp_netdev_actions *); +void dp_netdev_actions_unref(struct dp_netdev_actions *); + /* Interface to netdev-based datapath. */ struct dpif_netdev { struct dpif dpif; @@ -901,8 +925,8 @@ dpif_netdev_flow_get(const struct dpif *dpif, get_dpif_flow_stats(netdev_flow, stats); } if (actionsp) { - *actionsp = ofpbuf_clone_data(netdev_flow->actions, - netdev_flow->actions_len); + *actionsp = ofpbuf_clone_data(netdev_flow->actions->actions, + netdev_flow->actions->size); } } else { error = ENOENT; @@ -913,16 +937,6 @@ dpif_netdev_flow_get(const struct dpif *dpif, } static int -set_flow_actions(struct dp_netdev_flow *netdev_flow, - const struct nlattr *actions, size_t actions_len) -{ - netdev_flow->actions = xrealloc(netdev_flow->actions, actions_len); - netdev_flow->actions_len = actions_len; - memcpy(netdev_flow->actions, actions, actions_len); - return 0; -} - -static int dp_netdev_flow_add(struct dp_netdev *dp, const struct flow *flow, const struct flow_wildcards *wc, const struct nlattr *actions, @@ -930,10 +944,10 @@ dp_netdev_flow_add(struct dp_netdev *dp, const struct flow *flow, { struct dp_netdev_flow *netdev_flow; struct match match; - int error; netdev_flow = xzalloc(sizeof *netdev_flow); netdev_flow->flow = *flow; + netdev_flow->actions = dp_netdev_actions_create(actions, actions_len); match_init(&match, flow, wc); cls_rule_init(&netdev_flow->cr, &match, NETDEV_RULE_PRIORITY); @@ -941,17 +955,6 @@ dp_netdev_flow_add(struct dp_netdev *dp, const struct flow *flow, classifier_insert(&dp->cls, &netdev_flow->cr); ovs_rwlock_unlock(&dp->cls.rwlock); - error = set_flow_actions(netdev_flow, actions, actions_len); - if (error) { - ovs_rwlock_wrlock(&dp->cls.rwlock); - classifier_remove(&dp->cls, &netdev_flow->cr); - ovs_rwlock_unlock(&dp->cls.rwlock); - cls_rule_destroy(&netdev_flow->cr); - - free(netdev_flow); - return error; - } - hmap_insert(&dp->flow_table, &netdev_flow->node, flow_hash(flow, 0)); return 0; } @@ -1004,15 +1007,14 @@ dpif_netdev_flow_put(struct dpif *dpif, const struct dpif_flow_put *put) } else { if (put->flags & DPIF_FP_MODIFY && flow_equal(&flow, &netdev_flow->flow)) { - error = set_flow_actions(netdev_flow, put->actions, - put->actions_len); - if (!error) { - if (put->stats) { - get_dpif_flow_stats(netdev_flow, put->stats); - } - if (put->flags & DPIF_FP_ZERO_STATS) { - clear_stats(netdev_flow); - } + dp_netdev_actions_unref(netdev_flow->actions); + netdev_flow->actions = dp_netdev_actions_create(put->actions, + put->actions_len); + if (put->stats) { + get_dpif_flow_stats(netdev_flow, put->stats); + } + if (put->flags & DPIF_FP_ZERO_STATS) { + clear_stats(netdev_flow); } } else if (put->flags & DPIF_FP_CREATE) { error = EEXIST; @@ -1057,7 +1059,7 @@ dpif_netdev_flow_del(struct dpif *dpif, const struct dpif_flow_del *del) struct dp_netdev_flow_state { uint32_t bucket; uint32_t offset; - struct nlattr *actions; + struct dp_netdev_actions *actions; struct odputil_keybuf keybuf; struct odputil_keybuf maskbuf; struct dpif_flow_stats stats; @@ -1121,12 +1123,14 @@ dpif_netdev_flow_dump_next(const struct dpif *dpif, void *state_, } if (actions) { - free(state->actions); - state->actions = xmemdup(netdev_flow->actions, - netdev_flow->actions_len); + dp_netdev_actions_unref(state->actions); + state->actions = NULL; - *actions = state->actions; - *actions_len = netdev_flow->actions_len; + if (actions) { + state->actions = dp_netdev_actions_ref(netdev_flow->actions); + *actions = state->actions->actions; + *actions_len = state->actions->size; + } } if (stats) { @@ -1143,7 +1147,7 @@ dpif_netdev_flow_dump_done(const struct dpif *dpif OVS_UNUSED, void *state_) { struct dp_netdev_flow_state *state = state_; - free(state->actions); + dp_netdev_actions_unref(state->actions); free(state); return 0; } @@ -1256,6 +1260,46 @@ dpif_netdev_recv_purge(struct dpif *dpif) ovs_mutex_unlock(&dp_netdev_mutex); } +/* Creates and returns a new 'struct dp_netdev_actions', with a reference count + * of 1, whose actions are a copy of from the 'ofpacts_len' bytes of + * 'ofpacts'. */ +struct dp_netdev_actions * +dp_netdev_actions_create(const struct nlattr *actions, size_t size) +{ + struct dp_netdev_actions *netdev_actions; + + netdev_actions = xmalloc(sizeof *netdev_actions); + ovs_refcount_init(&netdev_actions->ref_cnt); + netdev_actions->actions = xmemdup(actions, size); + netdev_actions->size = size; + + return netdev_actions; +} + +/* Increments 'actions''s refcount. */ +struct dp_netdev_actions * +dp_netdev_actions_ref(const struct dp_netdev_actions *actions_) +{ + struct dp_netdev_actions *actions; + + actions = CONST_CAST(struct dp_netdev_actions *, actions_); + if (actions) { + ovs_refcount_ref(&actions->ref_cnt); + } + return actions; +} + +/* Decrements 'actions''s refcount and frees 'actions' if the refcount reaches + * 0. */ +void +dp_netdev_actions_unref(struct dp_netdev_actions *actions) +{ + if (actions && ovs_refcount_unref(&actions->ref_cnt) == 1) { + free(actions->actions); + free(actions); + } +} + static void dp_netdev_flow_used(struct dp_netdev_flow *netdev_flow, const struct ofpbuf *packet) @@ -1281,10 +1325,14 @@ dp_netdev_port_input(struct dp_netdev *dp, struct dp_netdev_port *port, flow_extract(packet, 0, 0, NULL, &in_port_, &key); netdev_flow = dp_netdev_lookup_flow(dp, &key); if (netdev_flow) { + struct dp_netdev_actions *actions; + dp_netdev_flow_used(netdev_flow, packet); + actions = dp_netdev_actions_ref(netdev_flow->actions); dp_netdev_execute_actions(dp, &key, packet, - netdev_flow->actions, - netdev_flow->actions_len); + actions->actions, actions->size); + dp_netdev_actions_unref(actions); + ovsthread_counter_inc(dp->n_hit, 1); } else { ovsthread_counter_inc(dp->n_missed, 1); -- 1.7.10.4 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev