One of the NEWS entries uses a TAB instead of spaces.
Following tradition, the nx_action_fin_timeout structure should have
an OFP_ASSERT of its size.
> netflow_flow_clear(&facet->nf_flow);
> + facet->tcp_flags = 0;
Could you comment briefly on this line of code? I'm having trouble
convincing myself that it's either correct or incorrect. I'm worried
about the case where facet_flush_stats() is called due to the actions
changing on the facet. If this facet has a fin_timeout in its new
actions, and it's already seen a fin, I would expect the fin_timeout
action to apply immediately? I'm not sure though.
Looks good,
Ethan
>
> /* Searches 'ofproto''s table of facets for one exactly equal to 'flow'.
> @@ -3407,7 +3420,7 @@ facet_check_consistency(struct facet *facet)
> bool should_install;
>
> action_xlate_ctx_init(&ctx, ofproto, &facet->flow,
> - subfacet->initial_tci, rule, NULL);
> + subfacet->initial_tci, rule, 0, NULL);
> odp_actions = xlate_actions(&ctx, rule->up.actions,
> rule->up.n_actions);
>
> @@ -3521,7 +3534,7 @@ facet_revalidate(struct facet *facet)
> bool should_install;
>
> action_xlate_ctx_init(&ctx, ofproto, &facet->flow,
> - subfacet->initial_tci, new_rule, NULL);
> + subfacet->initial_tci, new_rule, 0, NULL);
> odp_actions = xlate_actions(&ctx, new_rule->up.actions,
> new_rule->up.n_actions);
> actions_changed = (subfacet->actions_len != odp_actions->size
> @@ -3563,6 +3576,7 @@ facet_revalidate(struct facet *facet)
> facet->may_install = ctx.may_set_up_flow;
> facet->has_learn = ctx.has_learn;
> facet->has_normal = ctx.has_normal;
> + facet->has_fin_timeout = ctx.has_fin_timeout;
> facet->mirrors = ctx.mirrors;
> if (new_actions) {
> i = 0;
> @@ -3672,7 +3686,7 @@ flow_push_stats(struct rule_dpif *rule,
> push.used = used;
>
> action_xlate_ctx_init(&push.ctx, ofproto, flow, flow->vlan_tci, rule,
> - NULL);
> + 0, NULL);
> push.ctx.resubmit_hook = push_resubmit;
> ofpbuf_delete(xlate_actions(&push.ctx,
> rule->up.actions, rule->up.n_actions));
> @@ -3817,12 +3831,13 @@ subfacet_make_actions(struct subfacet *subfacet,
> const struct ofpbuf *packet)
> struct action_xlate_ctx ctx;
>
> action_xlate_ctx_init(&ctx, ofproto, &facet->flow, subfacet->initial_tci,
> - rule, packet);
> + rule, 0, packet);
> odp_actions = xlate_actions(&ctx, rule->up.actions, rule->up.n_actions);
> facet->tags = ctx.tags;
> facet->may_install = ctx.may_set_up_flow;
> facet->has_learn = ctx.has_learn;
> facet->has_normal = ctx.has_normal;
> + facet->has_fin_timeout = ctx.has_fin_timeout;
> facet->nf_flow.output_iface = ctx.nf_output_iface;
> facet->mirrors = ctx.mirrors;
>
> @@ -3942,6 +3957,7 @@ subfacet_update_stats(struct subfacet *subfacet,
> subfacet_update_time(subfacet, stats->used);
> facet->packet_count += stats->n_packets;
> facet->byte_count += stats->n_bytes;
> + facet->tcp_flags |= stats->tcp_flags;
> facet_push_stats(facet);
> netflow_flow_update_flags(&facet->nf_flow, stats->tcp_flags);
> }
> @@ -4098,7 +4114,7 @@ rule_execute(struct rule *rule_, const struct flow
> *flow,
> size_t size;
>
> action_xlate_ctx_init(&ctx, ofproto, flow, flow->vlan_tci,
> - rule, packet);
> + rule, packet_get_tcp_flags(packet, flow), packet);
> odp_actions = xlate_actions(&ctx, rule->up.actions, rule->up.n_actions);
> size = packet->size;
> if (execute_odp_actions(ofproto, flow, odp_actions->data,
> @@ -4693,6 +4709,28 @@ xlate_learn_action(struct action_xlate_ctx *ctx,
> free(fm.actions);
> }
>
> +/* Reduces '*timeout' to no more than 'max'. A value of zero in either case
> + * means "infinite". */
> +static void
> +reduce_timeout(uint16_t max, uint16_t *timeout)
> +{
> + if (max && (!*timeout || *timeout > max)) {
> + *timeout = max;
> + }
> +}
> +
> +static void
> +xlate_fin_timeout(struct action_xlate_ctx *ctx,
> + const struct nx_action_fin_timeout *naft)
> +{
> + if (ctx->tcp_flags & (TCP_FIN | TCP_RST) && ctx->rule) {
> + struct rule_dpif *rule = ctx->rule;
> +
> + reduce_timeout(ntohs(naft->fin_idle_timeout),
> &rule->up.idle_timeout);
> + reduce_timeout(ntohs(naft->fin_hard_timeout),
> &rule->up.hard_timeout);
> + }
> +}
> +
> static bool
> may_receive(const struct ofport_dpif *port, struct action_xlate_ctx *ctx)
> {
> @@ -4892,6 +4930,11 @@ do_xlate_actions(const union ofp_action *in, size_t
> n_in,
> case OFPUTIL_NXAST_EXIT:
> ctx->exit = true;
> break;
> +
> + case OFPUTIL_NXAST_FIN_TIMEOUT:
> + ctx->has_fin_timeout = true;
> + xlate_fin_timeout(ctx, (const struct nx_action_fin_timeout *)
> ia);
> + break;
> }
> }
>
> @@ -4908,7 +4951,7 @@ static void
> action_xlate_ctx_init(struct action_xlate_ctx *ctx,
> struct ofproto_dpif *ofproto, const struct flow *flow,
> ovs_be16 initial_tci, struct rule_dpif *rule,
> - const struct ofpbuf *packet)
> + uint8_t tcp_flags, const struct ofpbuf *packet)
> {
> ctx->ofproto = ofproto;
> ctx->flow = *flow;
> @@ -4918,6 +4961,7 @@ action_xlate_ctx_init(struct action_xlate_ctx *ctx,
> ctx->rule = rule;
> ctx->packet = packet;
> ctx->may_learn = packet != NULL;
> + ctx->tcp_flags = tcp_flags;
> ctx->resubmit_hook = NULL;
> }
>
> @@ -4935,6 +4979,7 @@ xlate_actions(struct action_xlate_ctx *ctx,
> ctx->may_set_up_flow = true;
> ctx->has_learn = false;
> ctx->has_normal = false;
> + ctx->has_fin_timeout = false;
> ctx->nf_output_iface = NF_OUT_DROP;
> ctx->mirrors = 0;
> ctx->recurse = 0;
> @@ -5705,7 +5750,7 @@ packet_out(struct ofproto *ofproto_, struct ofpbuf
> *packet,
> odp_flow_key_from_flow(&key, flow);
>
> action_xlate_ctx_init(&push.ctx, ofproto, flow, flow->vlan_tci, NULL,
> - packet);
> + packet_get_tcp_flags(packet, flow), packet);
>
> /* Ensure that resubmits in 'ofp_actions' get accounted to their
> * matching rules. */
> @@ -6004,7 +6049,8 @@ ofproto_unixctl_trace(struct unixctl_conn *conn, int
> argc, const char *argv[],
> trace.result = &result;
> trace.flow = flow;
> action_xlate_ctx_init(&trace.ctx, ofproto, &flow, initial_tci,
> - rule, packet);
> + rule, packet_get_tcp_flags(packet, &flow),
> + packet);
> trace.ctx.resubmit_hook = trace_resubmit;
> odp_actions = xlate_actions(&trace.ctx,
> rule->up.actions, rule->up.n_actions);
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index b995831..9fbe8aa 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2009, 2010, 2011 Nicira Networks.
> + * Copyright (c) 2009, 2010, 2011, 2012 Nicira Networks.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -796,9 +796,10 @@ struct ofproto_class {
> * 'flow' reflects the flow information for 'packet'. All of the
> * information in 'flow' is extracted from 'packet', except for
> * flow->tun_id and flow->in_port, which are assigned the correct values
> - * for the incoming packet. The register values are zeroed.
> + * for the incoming packet. The register values are zeroed. 'packet''s
> + * header pointers (e.g. packet->l3) are appropriately initialized.
> *
> - * The statistics for 'packet' should be included in 'rule'.
> + * The implementation should add the statistics for 'packet' into 'rule'.
> *
> * Returns 0 if successful, otherwise an OpenFlow error code. */
> enum ofperr (*rule_execute)(struct rule *rule, const struct flow *flow,
> diff --git a/tests/learn.at b/tests/learn.at
> index 93192ab..6539c68 100644
> --- a/tests/learn.at
> +++ b/tests/learn.at
> @@ -4,12 +4,12 @@ AT_SETUP([learning action - parsing and formatting])
> AT_DATA([flows.txt], [[
> actions=learn()
> actions=learn(NXM_OF_VLAN_TCI[0..11], NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],
> output:NXM_OF_IN_PORT[], load:10->NXM_NX_REG0[5..10])
> -actions=learn(table=1,idle_timeout=1, hard_timeout=2, priority=10,
> cookie=0xfedcba9876543210,
> in_port=99,NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],load:NXM_OF_IN_PORT[]->NXM_NX_REG1[16..31])
> +actions=learn(table=1,idle_timeout=10, hard_timeout=20, fin_idle_timeout=5,
> fin_hard_timeout=10, priority=10, cookie=0xfedcba9876543210,
> in_port=99,NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],load:NXM_OF_IN_PORT[]->NXM_NX_REG1[16..31])
> ]])
> AT_CHECK([ovs-ofctl parse-flows flows.txt], [0],
> [[OFPT_FLOW_MOD (xid=0x1): ADD actions=learn(table=1)
> OFPT_FLOW_MOD (xid=0x2): ADD
> actions=learn(table=1,NXM_OF_VLAN_TCI[0..11],NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],output:NXM_OF_IN_PORT[],load:0x000a->NXM_NX_REG0[5..10])
> -OFPT_FLOW_MOD (xid=0x3): ADD
> actions=learn(table=1,idle_timeout=1,hard_timeout=2,priority=10,cookie=0xfedcba9876543210,in_port=99,NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],load:NXM_OF_IN_PORT[]->NXM_NX_REG1[16..31])
> +OFPT_FLOW_MOD (xid=0x3): ADD
> actions=learn(table=1,idle_timeout=10,hard_timeout=20,fin_idle_timeout=5,fin_hard_timeout=10,priority=10,cookie=0xfedcba9876543210,in_port=99,NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],load:NXM_OF_IN_PORT[]->NXM_NX_REG1[16..31])
> ]])
> AT_CLEANUP
>
> @@ -106,3 +106,17 @@ NXST_FLOW reply:
> ])
> OVS_VSWITCHD_STOP
> AT_CLEANUP
> +
> +AT_SETUP([learning action - fin_timeout feature])
> +# This is a totally artificial use of the "learn" action. The only purpose
> +# is to check that specifying fin_idle_timeout or fin_hard_timeout causes
> +# a corresponding fin_timeout action to end up in the learned flows.
> +OVS_VSWITCHD_START
> +AT_CHECK([[ovs-ofctl add-flow br0 'actions=learn(fin_hard_timeout=10,
> fin_idle_timeout=5, NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],
> output:NXM_OF_IN_PORT[])']])
> +AT_CHECK([ovs-appctl ofproto/trace br0
> 'in_port(1),eth(src=50:54:00:00:00:05,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=192.168.0.1,tip=192.168.0.2,op=1,sha=50:54:00:00:00:05,tha=00:00:00:00:00:00)'
> -generate], [0], [ignore])
> +AT_CHECK([ovs-ofctl dump-flows br0 table=1 | STRIP_XIDS | STRIP_DURATION],
> [0],
> +[NXST_FLOW reply:
> + cookie=0x0, duration=?s, table=1, n_packets=0, n_bytes=0,
> dl_dst=50:54:00:00:00:05
> actions=fin_timeout(idle_timeout=5,hard_timeout=10),output:1
> +])
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index bb00714..4c19ea8 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -1035,3 +1035,40 @@ echo $n_recs
> AT_CHECK([test $n_recs -ge 13])
>
> AT_CLEANUP
> +
> +AT_SETUP([ofproto-dpif - fin_timeout])
> +OVS_VSWITCHD_START
> +AT_DATA([flows.txt], [dnl
> +in_port=1 actions=output:2
> +in_port=2 actions=mod_vlan_vid:17,output:1
> +])
> +AT_CHECK([ovs-ofctl add-flow br0
> 'idle_timeout=60,actions=fin_timeout(idle_timeout=5)'])
> +AT_CHECK([ovs-ofctl dump-flows br0 | STRIP_DURATION | STRIP_XIDS], [0],
> +[NXST_FLOW reply:
> + cookie=0x0, duration=?s, table=0, n_packets=0, n_bytes=0, idle_timeout=60,
> actions=fin_timeout(idle_timeout=5)
> +])
> +# Check that a TCP SYN packet does not change the timeout. (Because
> +# flow stats updates are mainly what implements the fin_timeout
> +# feature, we warp forward a couple of times to ensure that flow stats
> +# run before re-checking the flow table.)
> +AT_CHECK([ovs-appctl netdev-dummy/receive br0
> 0021853763af0026b98cb0f908004500003c2e2440004006465dac11370dac11370b828b0016751e267b00000000a00216d017360000020405b40402080a2d25085f0000000001030307],
> [0], [success
> +])
> +AT_CHECK([ovs-appctl time/warp 1000 && ovs-appctl time/warp 1000], [0],
> [warped
> +warped
> +])
> +AT_CHECK([ovs-ofctl dump-flows br0 | STRIP_DURATION | STRIP_XIDS], [0],
> +[NXST_FLOW reply:
> + cookie=0x0, duration=?s, table=0, n_packets=1, n_bytes=74, idle_timeout=60,
> actions=fin_timeout(idle_timeout=5)
> +])
> +# Check that a TCP FIN packet does change the timeout.
> +AT_CHECK([ovs-appctl netdev-dummy/receive br0
> 0021853763af0026b98cb0f90800451000342e3e40004006463bac11370dac11370b828b0016751e319dfc96399b801100717ae800000101080a2d250a9408579588],
> [0], [success
> +])
> +AT_CHECK([ovs-appctl time/warp 1000 && ovs-appctl time/warp 1000], [0],
> [warped
> +warped
> +])
> +AT_CHECK([ovs-ofctl dump-flows br0 | STRIP_DURATION | STRIP_XIDS], [0],
> +[NXST_FLOW reply:
> + cookie=0x0, duration=?s, table=0, n_packets=2, n_bytes=140, idle_timeout=5,
> actions=fin_timeout(idle_timeout=5)
> +])
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
> index 0b58b3d..6488bd9 100644
> --- a/tests/ovs-ofctl.at
> +++ b/tests/ovs-ofctl.at
> @@ -27,6 +27,7 @@
> actions=output:1,bundle_load(eth_src,0,hrw,ofport,NXM_NX_REG0[16..31],slaves:1),
> actions=resubmit:1,resubmit(2),resubmit(,3),resubmit(2,3)
> actions=output:1,output:NXM_NX_REG0[],output:2,output:NXM_NX_REG1[16..31],output:3
> actions=output:1,exit,output:2
> +actions=fin_timeout(idle_timeout=5,hard_timeout=15)
> ]])
>
> AT_CHECK([ovs-ofctl parse-flows flows.txt
> @@ -58,6 +59,7 @@ NXT_FLOW_MOD: ADD table:255
> actions=output:1,bundle_load(eth_src,0,hrw,ofport,NX
> NXT_FLOW_MOD: ADD table:255
> actions=resubmit:1,resubmit:2,resubmit(,3),resubmit(2,3)
> NXT_FLOW_MOD: ADD table:255
> actions=output:1,output:NXM_NX_REG0[],output:2,output:NXM_NX_REG1[16..31],output:3
> NXT_FLOW_MOD: ADD table:255 actions=output:1,exit,output:2
> +NXT_FLOW_MOD: ADD table:255
> actions=fin_timeout(hard_timeout=15,idle_timeout=5)
> ]])
> AT_CLEANUP
>
> diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
> index 2a20a2f..0f57a8b 100644
> --- a/utilities/ovs-ofctl.8.in
> +++ b/utilities/ovs-ofctl.8.in
> @@ -893,6 +893,11 @@ specified.
> These key-value pairs have the same meaning as in the usual
> \fBovs\-ofctl\fR flow syntax.
> .
> +.IP \fBfin_idle_timeout=\fIseconds\fR
> +.IQ \fBfin_hard_timeout=\fIseconds\fR
> +Adds a \fBfin_timeout\fR action with the specified arguments to the
> +new flow. This feature was added in Open vSwitch 1.5.90.
> +.
> .IP \fBtable=\fInumber\fR
> The table in which the new flow should be inserted. Specify a decimal
> number between 0 and 254. The default, if \fBtable\fR is unspecified,
> @@ -943,6 +948,27 @@ with no match criteria. (This is why the default
> \fBtable\fR is 1, to
> keep the learned flows separate from the primary flow table 0.)
> .RE
> .
> +.IP "\fBfin_timeout(\fIargument\fR[\fB,\fIargument\fR]\fB)"
> +This action changes the idle timeout or hard timeout, or both, of this
> +OpenFlow rule when the rule matches a TCP packet with the FIN or RST
> +flag. When such a packet is observed, the action reduces the rule's
> +timeouts to those specified on the action. If the rule's existing
> +timeout is already shorter than the one that the action specifies,
> +then that timeout is unaffected.
> +.IP
> +\fIargument\fR takes the following forms:
> +.RS
> +.IP "\fBidle_timeout=\fIseconds\fR"
> +Causes the flow to expire after the given number of seconds of
> +inactivity.
> +.
> +.IP "\fBhard_timeout=\fIseconds\fR"
> +Causes the flow to expire after the given number of seconds,
> +regardless of activity. (\fIseconds\fR specifies time since the
> +flow's creation, not since the receipt of the FIN or RST.)
> +.RE
> +.IP
> +This action was added in Open vSwitch 1.5.90.
> .IP "\fBexit\fR"
> This action causes Open vSwitch to immediately halt execution of further
> actions. Those actions which have already been executed are unaffected. Any
> --
> 1.7.2.5
>
> _______________________________________________
> dev mailing list
> [email protected]
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
