[ovs-dev] [PATCH v3 2/2] ofproto: Allow in-place modifications of datapath flows.

Jarno Rajahalme jrajahalme at nicira.com
Thu Aug 13 00:34:08 UTC 2015


This seems good to go for me,

  Jarno

Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>

> On Aug 12, 2015, at 4:13 PM, Ethan J. Jackson <ethan at nicira.com> wrote:
> 
> From: Ethan Jackson <ethan at nicira.com>
> 
> There are certain use cases (such as bond rebalancing) where a
> datapath flow's actions may change, while it's wildcard pattern
> remains the same.  Before this patch, revalidators would note the
> change, delete the flow, and wait for the handlers to install an
> updated version.  This is inefficient, as many packets could get
> punted to userspace before the new flow is finally installed.
> 
> To improve the situation, this patch implements in place modification
> of datapath flows.  If the revalidators detect the only change to a
> given ukey is its actions, instead of deleting it, it does a put with
> the MODIFY flag set.
> 
> Signed-off-by: Ethan J. Jackson <ethan at nicira.com>
> ---
> ofproto/ofproto-dpif-upcall.c | 164 +++++++++++++++++++++++++++---------------
> tests/ofproto-dpif.at         |  40 +++++++++++
> 2 files changed, 146 insertions(+), 58 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index c57cebd..f22fb05 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -150,6 +150,12 @@ enum upcall_type {
>     IPFIX_UPCALL                /* Per-bridge sampling. */
> };
> 
> +enum reval_result {
> +    UKEY_KEEP,
> +    UKEY_DELETE,
> +    UKEY_MODIFY
> +};
> +
> struct upcall {
>     struct ofproto_dpif *ofproto;  /* Parent ofproto. */
>     const struct recirc_id_node *recirc; /* Recirculation context. */
> @@ -1694,33 +1700,41 @@ should_revalidate(const struct udpif *udpif, uint64_t packets,
>     return false;
> }
> 
> -static bool
> +/* Verifies that the datapath actions of 'ukey' are still correct, and pushes
> + * 'stats' for it.
> + *
> + * Returns a recommended action for 'ukey', options include:
> + *      UKEY_DELETE The ukey should be deleted.
> + *      UKEY_KEEP   The ukey is fine as is.
> + *      UKEY_MODIFY The ukey's actions should be changed but is otherwise
> + *                  fine.  Callers should change the actions to those found
> + *                  in the caller supplied 'odp_actions' buffer. */
> +static enum reval_result
> revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
> -                const struct dpif_flow_stats *stats, uint64_t reval_seq)
> +                const struct dpif_flow_stats *stats,
> +                struct ofpbuf *odp_actions, uint64_t reval_seq)
>     OVS_REQUIRES(ukey->mutex)
> {
> -    uint64_t odp_actions_stub[1024 / 8];
> -    struct ofpbuf odp_actions = OFPBUF_STUB_INITIALIZER(odp_actions_stub);
> -
>     struct xlate_out xout, *xoutp;
>     struct netflow *netflow;
>     struct ofproto_dpif *ofproto;
>     struct dpif_flow_stats push;
>     struct flow flow, dp_mask;
>     struct flow_wildcards wc;
> +    enum reval_result result;
>     uint64_t *dp64, *xout64;
>     ofp_port_t ofp_in_port;
>     struct xlate_in xin;
>     long long int last_used;
>     int error;
>     size_t i;
> -    bool ok;
>     bool need_revalidate;
> 
> -    ok = false;
> +    result = UKEY_DELETE;
>     xoutp = NULL;
>     netflow = NULL;
> 
> +    ofpbuf_clear(odp_actions);
>     need_revalidate = (ukey->reval_seq != reval_seq);
>     last_used = ukey->stats.used;
>     push.used = stats->used;
> @@ -1734,20 +1748,19 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
> 
>     if (need_revalidate && last_used
>         && !should_revalidate(udpif, push.n_packets, last_used)) {
> -        ok = false;
>         goto exit;
>     }
> 
>     /* We will push the stats, so update the ukey stats cache. */
>     ukey->stats = *stats;
>     if (!push.n_packets && !need_revalidate) {
> -        ok = true;
> +        result = UKEY_KEEP;
>         goto exit;
>     }
> 
>     if (ukey->xcache && !need_revalidate) {
>         xlate_push_stats(ukey->xcache, &push);
> -        ok = true;
> +        result = UKEY_KEEP;
>         goto exit;
>     }
> 
> @@ -1770,7 +1783,7 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
>     }
> 
>     xlate_in_init(&xin, ofproto, &flow, ofp_in_port, NULL, push.tcp_flags,
> -                  NULL, need_revalidate ? &wc : NULL, &odp_actions);
> +                  NULL, need_revalidate ? &wc : NULL, odp_actions);
>     if (push.n_packets) {
>         xin.resubmit_stats = &push;
>         xin.may_learn = true;
> @@ -1780,18 +1793,14 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
>     xoutp = &xout;
> 
>     if (!need_revalidate) {
> -        ok = true;
> +        result = UKEY_KEEP;
>         goto exit;
>     }
> 
>     if (xout.slow) {
> -        ofpbuf_clear(&odp_actions);
> +        ofpbuf_clear(odp_actions);
>         compose_slow_path(udpif, &xout, &flow, flow.in_port.odp_port,
> -                          &odp_actions);
> -    }
> -
> -    if (!ofpbuf_equal(&odp_actions, ukey->actions)) {
> -        goto exit;
> +                          odp_actions);
>     }
> 
>     if (odp_flow_key_to_mask(ukey->mask, ukey->mask_len, ukey->key,
> @@ -1812,18 +1821,24 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
>         }
>     }
> 
> -    ok = true;
> +    if (!ofpbuf_equal(odp_actions, ukey->actions)) {
> +        /* The datapath mask was OK, but the actions seem to have changed.
> +         * Let's modify it in place. */
> +        result = UKEY_MODIFY;
> +        goto exit;
> +    }
> +
> +    result = UKEY_KEEP;
> 
> exit:
> -    if (ok) {
> +    if (result != UKEY_DELETE) {
>         ukey->reval_seq = reval_seq;
>     }
> -    if (netflow && !ok) {
> +    if (netflow && result == UKEY_DELETE) {
>         netflow_flow_clear(netflow, &flow);
>     }
>     xlate_out_uninit(xoutp);
> -    ofpbuf_uninit(&odp_actions);
> -    return ok;
> +    return result;
> }
> 
> static void
> @@ -1854,6 +1869,23 @@ delete_op_init(struct udpif *udpif, struct ukey_op *op, struct udpif_key *ukey)
> }
> 
> static void
> +modify_op_init(struct ukey_op *op, struct udpif_key *ukey)
> +{
> +    op->ukey = ukey;
> +    op->dop.type = DPIF_OP_FLOW_PUT;
> +    op->dop.u.flow_put.flags = DPIF_FP_MODIFY;
> +    op->dop.u.flow_put.key = ukey->key;
> +    op->dop.u.flow_put.key_len = ukey->key_len;
> +    op->dop.u.flow_put.mask = ukey->mask;
> +    op->dop.u.flow_put.mask_len = ukey->mask_len;
> +    op->dop.u.flow_put.ufid = &ukey->ufid;
> +    op->dop.u.flow_put.pmd_id = ukey->pmd_id;
> +    op->dop.u.flow_put.stats = NULL;
> +    ukey_get_actions(ukey, &op->dop.u.flow_put.actions,
> +                     &op->dop.u.flow_put.actions_len);
> +}
> +
> +static void
> push_ukey_ops__(struct udpif *udpif, struct ukey_op *ops, size_t n_ops)
> {
>     struct dpif_op *opsp[REVALIDATE_MAX_BATCH];
> @@ -1872,6 +1904,11 @@ push_ukey_ops__(struct udpif *udpif, struct ukey_op *ops, size_t n_ops)
>         stats = op->dop.u.flow_del.stats;
>         push = &push_buf;
> 
> +        if (op->dop.type != DPIF_OP_FLOW_DEL) {
> +            /* Only deleted flows need their stats pushed. */
> +            continue;
> +        }
> +
>         if (op->ukey) {
>             ovs_mutex_lock(&op->ukey->mutex);
>             push->used = MAX(stats->used, op->ukey->stats.used);
> @@ -1957,6 +1994,9 @@ log_unexpected_flow(const struct dpif_flow *flow, int error)
> static void
> revalidate(struct revalidator *revalidator)
> {
> +    uint64_t odp_actions_stub[1024 / 8];
> +    struct ofpbuf odp_actions = OFPBUF_STUB_INITIALIZER(odp_actions_stub);
> +
>     struct udpif *udpif = revalidator->udpif;
>     struct dpif_flow_dump_thread *dump_thread;
>     uint64_t dump_seq, reval_seq;
> @@ -2004,8 +2044,9 @@ revalidate(struct revalidator *revalidator)
> 
>         for (f = flows; f < &flows[n_dumped]; f++) {
>             long long int used = f->stats.used;
> +            enum reval_result result;
>             struct udpif_key *ukey;
> -            bool already_dumped, keep;
> +            bool already_dumped;
>             int error;
> 
>             if (ukey_acquire(udpif, f, &ukey, &error)) {
> @@ -2039,15 +2080,19 @@ revalidate(struct revalidator *revalidator)
>                 used = ukey->created;
>             }
>             if (kill_them_all || (used && used < now - max_idle)) {
> -                keep = false;
> +                result = UKEY_DELETE;
>             } else {
> -                keep = revalidate_ukey(udpif, ukey, &f->stats, reval_seq);
> +                result = revalidate_ukey(udpif, ukey, &f->stats, &odp_actions,
> +                                         reval_seq);
>             }
>             ukey->dump_seq = dump_seq;
> -            ukey->flow_exists = keep;
> +            ukey->flow_exists = result != UKEY_DELETE;
> 
> -            if (!keep) {
> +            if (result == UKEY_DELETE) {
>                 delete_op_init(udpif, &ops[n_ops++], ukey);
> +            } else if (result == UKEY_MODIFY) {
> +                ukey_set_actions(ukey, &odp_actions);
> +                modify_op_init(&ops[n_ops++], ukey);
>             }
>             ovs_mutex_unlock(&ukey->mutex);
>         }
> @@ -2058,23 +2103,7 @@ revalidate(struct revalidator *revalidator)
>         ovsrcu_quiesce();
>     }
>     dpif_flow_dump_thread_destroy(dump_thread);
> -}
> -
> -static bool
> -handle_missed_revalidation(struct udpif *udpif, uint64_t reval_seq,
> -                           struct udpif_key *ukey)
> -{
> -    struct dpif_flow_stats stats;
> -    bool keep;
> -
> -    COVERAGE_INC(revalidate_missed_dp_flow);
> -
> -    memset(&stats, 0, sizeof stats);
> -    ovs_mutex_lock(&ukey->mutex);
> -    keep = revalidate_ukey(udpif, ukey, &stats, reval_seq);
> -    ovs_mutex_unlock(&ukey->mutex);
> -
> -    return keep;
> +    ofpbuf_uninit(&odp_actions);
> }
> 
> static void
> @@ -2091,6 +2120,9 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge)
>     ovs_assert(slice < udpif->n_revalidators);
> 
>     for (int i = slice; i < N_UMAPS; i += udpif->n_revalidators) {
> +        uint64_t odp_actions_stub[1024 / 8];
> +        struct ofpbuf odp_actions = OFPBUF_STUB_INITIALIZER(odp_actions_stub);
> +
>         struct ukey_op ops[REVALIDATE_MAX_BATCH];
>         struct udpif_key *ukey;
>         struct umap *umap = &udpif->ukeys[i];
> @@ -2098,6 +2130,7 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge)
> 
>         CMAP_FOR_EACH(ukey, cmap_node, &umap->cmap) {
>             bool flow_exists, seq_mismatch;
> +            enum reval_result result;
> 
>             /* Handler threads could be holding a ukey lock while it installs a
>              * new flow, so don't hang around waiting for access to it. */
> @@ -2109,19 +2142,32 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge)
>                             && ukey->reval_seq != reval_seq);
>             ovs_mutex_unlock(&ukey->mutex);
> 
> -            if (flow_exists
> -                && (purge
> -                    || (seq_mismatch
> -                        && !handle_missed_revalidation(udpif, reval_seq,
> -                                                       ukey)))) {
> -                struct ukey_op *op = &ops[n_ops++];
> -
> -                delete_op_init(udpif, op, ukey);
> -                if (n_ops == REVALIDATE_MAX_BATCH) {
> -                    push_ukey_ops(udpif, umap, ops, n_ops);
> -                    n_ops = 0;
> -                }
> -            } else if (!flow_exists) {
> +
> +            if (purge) {
> +                result = UKEY_DELETE;
> +            } else if (!seq_mismatch) {
> +                result = UKEY_KEEP;
> +            } else {
> +                struct dpif_flow_stats stats;
> +                COVERAGE_INC(revalidate_missed_dp_flow);
> +                memset(&stats, 0, sizeof stats);
> +                result = revalidate_ukey(udpif, ukey, &stats, &odp_actions,
> +                                         reval_seq);
> +            }
> +
> +            if (result == UKEY_DELETE) {
> +                delete_op_init(udpif, &ops[n_ops++], ukey);
> +            } else if (result == UKEY_MODIFY) {
> +                ukey_set_actions(ukey, &odp_actions);
> +                modify_op_init(&ops[n_ops++], ukey);
> +            }
> +
> +            if (n_ops == REVALIDATE_MAX_BATCH) {
> +                push_ukey_ops(udpif, umap, ops, n_ops);
> +                n_ops = 0;
> +            }
> +
> +            if (!flow_exists) {
>                 ovs_mutex_lock(&umap->mutex);
>                 ukey_delete(umap, ukey);
>                 ovs_mutex_unlock(&umap->mutex);
> @@ -2131,6 +2177,8 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge)
>         if (n_ops) {
>             push_ukey_ops(udpif, umap, ops, n_ops);
>         }
> +
> +        ofpbuf_uninit(&odp_actions);
>         ovsrcu_quiesce();
>     }
> }
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 58c426b..121f84d 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -6985,3 +6985,43 @@ recirc_id=0,ip,in_port=1,dl_vlan=10,nw_frag=no, actions: <del>
> ])
> OVS_VSWITCHD_STOP
> AT_CLEANUP
> +
> +# Tests in place modification of installed datapath flows.
> +AT_SETUP([ofproto-dpif - in place modification])
> +OVS_VSWITCHD_START(
> +  [add-port br0 p1 -- set Interface p1 type=dummy ofport_request=1])
> +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
> +
> +AT_CHECK([ovs-ofctl del-flows br0])
> +AT_CHECK([ovs-ofctl add-flow br0 in_port=1,actions=mod_vlan_vid:3,output:local])
> +
> +ovs-appctl vlog/set PATTERN:ANY:'%c|%p|%m'
> +
> +ovs-appctl time/stop
> +
> +for i in 1 2 3; do
> +    ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x1234)'
> +done
> +
> +AT_CHECK([ovs-appctl dpif/dump-flows br0 | STRIP_UFID | STRIP_USED | sort], [0], [dnl
> +recirc_id(0),in_port(1),eth_type(0x1234), packets:2, bytes:120, used:0.0s, actions:push_vlan(vid=3,pcp=0),100
> +])
> +
> +AT_CHECK([ovs-ofctl add-flow br0 priority=60000,in_port=1,actions=mod_vlan_vid:4,output:local])
> +
> +ovs-appctl time/warp 500
> +ovs-appctl time/warp 500
> +
> +for i in 1 2 3; do
> +    ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x1234)'
> +done
> +
> +AT_CHECK([ovs-appctl dpif/dump-flows br0 | STRIP_UFID | STRIP_USED | sort], [0], [dnl
> +recirc_id(0),in_port(1),eth_type(0x1234), packets:5, bytes:300, used:0.0s, actions:push_vlan(vid=4,pcp=0),100
> +])
> +
> +AT_CHECK([cat ovs-vswitchd.log | grep 'modify' | STRIP_UFID ], [0], [dnl
> +dpif|DBG|dummy at ovs-dummy: put[[modify]] skb_priority(0/0),skb_mark(0/0),recirc_id(0),dp_hash(0/0),in_port(1),eth(src=50:54:00:00:00:09/00:00:00:00:00:00,dst=50:54:00:00:00:0a/00:00:00:00:00:00),eth_type(0x1234), actions:push_vlan(vid=4,pcp=0),100
> +])
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> -- 
> 2.1.0
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev




More information about the dev mailing list