On Fri, Dec 27, 2013 at 8:03 PM, Ben Pfaff <b...@nicira.com> wrote:
> 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;
> + }
actions is checked twice.
Otherwise looks good.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
