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

[Joe Stringer]

On 10 August 2015 at 18:46, Ethan J. Jackson <ethan at nicira.com> wrote:
>  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);
>      }

I think netflow should be cleared only if we delete.

> +            if (purge) {
> +                result = UKEY_DELETE;
> +            } else if (ukey->dump_seq == dump_seq
> +                     || ukey->reval_seq == reval_seq) {
> +                result = UKEY_KEEP;

Indentation in elseif.

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

Per-batch push_ukey_ops() occurs for UKEY_DELETE case, but not
UKEY_MODIFY. this may allow n_ops to exceed REVALIDATE_MAX_BATCH if a
bunch of modify operations all occur at once.

> 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

Have you tried running this test on travis or a similar environment
under high load? I wonder if it's prone to race conditions, because
the test script doesn't do any waiting to ensure that the packets are
handled before it checks the logs.