This seems good to go for me,
Jarno
Acked-by: Jarno Rajahalme <jrajaha...@nicira.com>
> On Aug 12, 2015, at 4:13 PM, Ethan J. Jackson <et...@nicira.com> wrote:
>
> From: Ethan Jackson <et...@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 <et...@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@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@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
