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

Jarno Rajahalme jrajahalme at nicira.com
Tue Aug 11 18:13:48 UTC 2015


> On Aug 10, 2015, at 6:46 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 Jackson <ethan at nicira.com>
> ---
> ofproto/ofproto-dpif-upcall.c | 169 +++++++++++++++++++++++++++---------------
> tests/ofproto-dpif.at         |  40 ++++++++++
> 2 files changed, 150 insertions(+), 59 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index fddb535..8ef61bc 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. */
> @@ -1663,33 +1669,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;
> @@ -1703,20 +1717,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;
>     }
> 
> @@ -1739,7 +1752,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;
> @@ -1749,18 +1762,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,
> @@ -1781,18 +1790,24 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
>         }
>     }
> 
> -    ok = true;
> +    if (!ofpbuf_equal(odp_actions, ukey->actions)) {
> +        /* The datpath mask was OK, but the actions seem to have changed.

‘datapath'

> +         * 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
> @@ -1823,6 +1838,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;
> +    op->dop.u.flow_put.actions = ukey->actions->data;
> +    op->dop.u.flow_put.actions_len = ukey->actions->size;
> +}
> +
> +static void
> push_ukey_ops__(struct udpif *udpif, struct ukey_op *ops, size_t n_ops)
> {
>     struct dpif_op *opsp[REVALIDATE_MAX_BATCH];
> @@ -1841,6 +1873,12 @@ 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_PUT) {
> +            /* This was an in place modification.  Don't bother pushing stats
> +             * for now. */

I’d prefer this instead:

        if (op->dop.type != DPIF_OP_FLOW_DEL) {
            /* Only deleted flows need their stats pushed now. */

> +            continue;
> +        }
> +
>         if (op->ukey) {
>             ovs_mutex_lock(&op->ukey->mutex);
>             push->used = MAX(stats->used, op->ukey->stats.used);
> @@ -1926,6 +1964,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;
> @@ -1973,8 +2014,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)) {
> @@ -2008,15 +2050,20 @@ 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) {
> +                ofpbuf_delete(ukey->actions);
> +                ukey->actions = ofpbuf_clone(&odp_actions);

ukey’s actions is documented to be read only, so you should check if it is safe to change it. Also, ukey is said to be RCU protected, so maybe the ukey->actions pointer should be changed to an RCU pointer and the old key postpone deleted after the new pointer is set? Or, have you checked that holding the ukey mutex while changing the actions is enough? In that case you should update the struct ukey definition accordingly.

> +                modify_op_init(&ops[n_ops++], ukey);
>             }
>             ovs_mutex_unlock(&ukey->mutex);
>         }
> @@ -2027,23 +2074,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
> @@ -2060,28 +2091,45 @@ 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];
>         size_t n_ops = 0;
> 
>         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. */
>             if (ovs_mutex_trylock(&ukey->mutex)) {
>                 continue;
>             }
> -            flow_exists = ukey->flow_exists;
> -            seq_mismatch = (ukey->dump_seq != dump_seq
> -                            && ukey->reval_seq != reval_seq);
> -
> -            if (flow_exists
> -                && (purge
> -                    || (seq_mismatch
> -                        && !handle_missed_revalidation(udpif, reval_seq,
> -                                                       ukey)))) {
> +
> +            if (!ukey->flow_exists) {
> +                ovs_mutex_lock(&umap->mutex);
> +                ukey_delete(umap, ukey);
> +                ovs_mutex_unlock(&umap->mutex);
> +                ovs_mutex_unlock(&ukey->mutex);

Should check if the lock ordering here is safe.

> +                continue;
> +            }
> +
> +            if (purge) {
> +                result = UKEY_DELETE;
> +            } else if (ukey->dump_seq == dump_seq
> +                     || ukey->reval_seq == reval_seq) {
> +                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) {
>                 struct ukey_op *op = &ops[n_ops++];
> 
>                 delete_op_init(udpif, op, ukey);
> @@ -2089,17 +2137,20 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge)
>                     push_ukey_ops(udpif, umap, ops, n_ops);

push_key_ops() will try to take the locks of the ukeys, so it would double lock the key we are currently processing.

>                     n_ops = 0;
>                 }
> -            } else if (!flow_exists) {
> -                ovs_mutex_lock(&umap->mutex);
> -                ukey_delete(umap, ukey);
> -                ovs_mutex_unlock(&umap->mutex);
> +            } else if (result == UKEY_MODIFY) {
> +                ofpbuf_delete(ukey->actions);
> +                ukey->actions = ofpbuf_clone(&odp_actions);

Same here about RCU/locking.

> +                modify_op_init(&ops[n_ops++], ukey);
>             }
> +
>             ovs_mutex_unlock(&ukey->mutex);
>         }
> 
>         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