Incremental looks good, thanks.
Ethan
On Tue, Feb 7, 2012 at 10:15, Ben Pfaff <b...@nicira.com> wrote:
> On Mon, Feb 06, 2012 at 02:59:12PM -0800, Ethan Jackson wrote:
>> In learning switch, should OFPUTIL_NXT_FLOW_AGE return an error as
>> per the spec?
>
> learning-switch.c implements a controller (that implements a
> MAC-learning switch), so it shouldn't get NXT_FLOW_AGE requests at
> all.
>
>> > + cookie=0x0, duration=2.88s, table=0, n_packets=1, n_bytes=60,
>> > idle_timeout=5,hard_timeout=10,idle_age=2,hard_age=7,priority=65535,tcp,in_port=3,vlan_tci=0x0000,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:06,nw_src=192.168.0.1,nw_dst=192.168
>>
>> This particular case looked very strange to me for a few seconds
>> because the hard_age is larger than the duration which shouldn't be
>> possible. Of course, this is only testing the parser, but perhaps it
>> makes sense to use a smaller value in the test just so it looks
>> normal.
>
> OK, I changed the hard_age to 2.
>
>> Also in the unit tests you've added, you check that the duration,
>> idle_age, and hard_age are increasing. Does it make sense to also
>> check that at each step duration >= hard_age >= idle_age?
>
> You're right, we can check more invariants here. The one you suggest
> isn't always true, though, because "mod-flows" does not reset the
> idle_age to 0. Also, we need to make sure that duration, hard_age,
> and idle_age are calculated relative to the same value of the current
> time (which is a good idea anyway).
>
> I applied this:
>
> diff --git a/lib/ofp-print.c b/lib/ofp-print.c
> index 4937040..2712d61 100644
> --- a/lib/ofp-print.c
> +++ b/lib/ofp-print.c
> @@ -1045,10 +1045,10 @@ ofp_print_flow_stats_reply(struct ds *string, const
> struct ofp_header *oh)
> ds_put_format(string, "hard_timeout=%"PRIu16",", fs.hard_timeout);
> }
> if (fs.idle_age >= 0) {
> - ds_put_format(string, "idle_age=%"PRIu16",", fs.idle_age);
> + ds_put_format(string, "idle_age=%d,", fs.idle_age);
> }
> if (fs.hard_age >= 0 && fs.hard_age != fs.duration_sec) {
> - ds_put_format(string, "hard_age=%"PRIu16",", fs.hard_age);
> + ds_put_format(string, "hard_age=%d,", fs.hard_age);
> }
>
> cls_rule_format(&fs.rule, string);
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 3cf6b07..473ae41 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -2174,9 +2174,10 @@ handle_port_stats_request(struct ofconn *ofconn,
> }
>
> static void
> -calc_flow_duration__(long long int start, uint32_t *sec, uint32_t *nsec)
> +calc_flow_duration__(long long int start, long long int now,
> + uint32_t *sec, uint32_t *nsec)
> {
> - long long int msecs = time_msec() - start;
> + long long int msecs = now - start;
> *sec = msecs / 1000;
> *nsec = (msecs % 1000) * (1000 * 1000);
> }
> @@ -2337,11 +2338,11 @@ collect_rules_strict(struct ofproto *ofproto, uint8_t
> table_id,
> return 0;
> }
>
> -/* Returns the number of whole seconds since 'start'. */
> +/* Returns 'age_ms' (a duration in milliseconds), converted to seconds and
> + * forced into the range of a uint16_t. */
> static int
> -age_secs(long long int start)
> +age_secs(long long int age_ms)
> {
> - long long int age_ms = time_msec() - start;
> return (age_ms < 0 ? 0
> : age_ms >= UINT16_MAX * 1000 ? UINT16_MAX
> : (unsigned int) age_ms / 1000);
> @@ -2372,17 +2373,18 @@ handle_flow_stats_request(struct ofconn *ofconn,
>
> ofputil_start_stats_reply(osm, &replies);
> LIST_FOR_EACH (rule, ofproto_node, &rules) {
> + long long int now = time_msec();
> struct ofputil_flow_stats fs;
>
> fs.rule = rule->cr;
> fs.cookie = rule->flow_cookie;
> fs.table_id = rule->table_id;
> - calc_flow_duration__(rule->created, &fs.duration_sec,
> + calc_flow_duration__(rule->created, now, &fs.duration_sec,
> &fs.duration_nsec);
> fs.idle_timeout = rule->idle_timeout;
> fs.hard_timeout = rule->hard_timeout;
> - fs.idle_age = age_secs(rule->used);
> - fs.hard_age = age_secs(rule->modified);
> + fs.idle_age = age_secs(now - rule->used);
> + fs.hard_age = age_secs(now - rule->modified);
> ofproto->ofproto_class->rule_get_stats(rule, &fs.packet_count,
> &fs.byte_count);
> fs.actions = rule->actions;
> @@ -2942,7 +2944,8 @@ ofproto_rule_send_removed(struct rule *rule, uint8_t
> reason)
> fr.rule = rule->cr;
> fr.cookie = rule->flow_cookie;
> fr.reason = reason;
> - calc_flow_duration__(rule->created, &fr.duration_sec, &fr.duration_nsec);
> + calc_flow_duration__(rule->created, time_msec(),
> + &fr.duration_sec, &fr.duration_nsec);
> fr.idle_timeout = rule->idle_timeout;
> rule->ofproto->ofproto_class->rule_get_stats(rule, &fr.packet_count,
> &fr.byte_count);
> diff --git a/tests/ofp-print.at b/tests/ofp-print.at
> index facba2b..2595900 100644
> --- a/tests/ofp-print.at
> +++ b/tests/ofp-print.at
> @@ -823,7 +823,7 @@ a8 00 01 00 00 0c 01 06 00 00 12 02 00 00 00 00 \
> a8 00 01 00 00 10 04 c0 a8 00 02 00 00 0c 01 06 \
> 00 00 12 02 09 e3 00 00 14 02 00 00 00 00 00 00 \
> 00 00 00 08 00 01 00 00 00 88 00 00 00 00 00 02 \
> -34 73 bc 00 ff ff 00 05 00 0a 00 4c 00 03 00 08 \
> +34 73 bc 00 ff ff 00 05 00 0a 00 4c 00 03 00 03 \
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 \
> 00 00 00 00 00 00 00 3c 00 00 00 02 00 03 00 00 \
> 02 06 50 54 00 00 00 06 00 00 04 06 50 54 00 00 \
> @@ -923,7 +923,7 @@ ff ff 00 18 00 00 23 20 00 07 00 1f 00 01 00 04 \
> cookie=0x0, duration=3.84s, table=0, n_packets=1, n_bytes=60,
> idle_timeout=5,idle_age=2,priority=65535,tcp,in_port=3,vlan_tci=0x0000,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:06,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_tos=0,tp_src=2532,tp_dst=0
> actions=output:1
> cookie=0x0, duration=2.872s, table=0, n_packets=1, n_bytes=60,
> idle_timeout=5,idle_age=4,priority=65535,tcp,in_port=1,vlan_tci=0x0000,dl_src=50:54:00:00:00:06,dl_dst=50:54:00:00:00:05,nw_src=192.168.0.2,nw_dst=192.168.0.1,nw_tos=0,tp_src=0,tp_dst=2533
> actions=output:3
> cookie=0x0, duration=4.756s, table=0, n_packets=1, n_bytes=60,
> idle_timeout=5,idle_age=0,priority=65535,tcp,in_port=3,vlan_tci=0x0000,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:06,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_tos=0,tp_src=2531,tp_dst=0
> actions=output:1
> - cookie=0x0, duration=2.88s, table=0, n_packets=1, n_bytes=60,
> idle_timeout=5,hard_timeout=10,idle_age=2,hard_age=7,priority=65535,tcp,in_port=3,vlan_tci=0x0000,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:06,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_tos=0,tp_src=2533,tp_dst=0
> actions=output:1
> + cookie=0x0, duration=2.88s, table=0, n_packets=1, n_bytes=60,
> idle_timeout=5,hard_timeout=10,idle_age=2,priority=65535,tcp,in_port=3,vlan_tci=0x0000,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:06,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_tos=0,tp_src=2533,tp_dst=0
> actions=output:1
> cookie=0x0, duration=5.672s, table=0, n_packets=1, n_bytes=60,
> idle_timeout=5,priority=65535,tcp,in_port=3,vlan_tci=0x0000,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:06,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_tos=0,tp_src=2530,tp_dst=0
> actions=output:1
> cookie=0x0, duration=1.04s, table=0, n_packets=1, n_bytes=60,
> idle_timeout=5,priority=65535,tcp,in_port=1,vlan_tci=0x0000,dl_src=50:54:00:00:00:06,dl_dst=50:54:00:00:00:05,nw_src=192.168.0.2,nw_dst=192.168.0.1,nw_tos=0,tp_src=0,tp_dst=2535
> actions=output:3
> cookie=0x0, duration=1.952s, table=0, n_packets=1, n_bytes=60,
> idle_timeout=5,priority=65535,tcp,in_port=1,vlan_tci=0x0000,dl_src=50:54:00:00:00:06,dl_dst=50:54:00:00:00:05,nw_src=192.168.0.2,nw_dst=192.168.0.1,nw_tos=0,tp_src=0,tp_dst=2534
> actions=output:3
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index b8f48de..f5c1358 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -1088,11 +1088,10 @@ get_ages duration3 hard3 idle3
> # occasionally.
> ovs-appctl time/warp 10000
> ovs-appctl netdev-dummy/receive br0
> 'in_port(0),eth(src=50:54:00:00:00:07,dst=50:54:00:00:00:05),eth_type(0x0800),ipv4(src=192.168.0.2,dst=192.168.0.1,proto=6,tos=0,ttl=64,frag=no),tcp(src=80,dst=1234)'
> -ovs-ofctl dump-flows br0
> -get_ages duration4 hard4 idle4
> ovs-appctl time/warp 1000
> ovs-appctl time/warp 1000
> ovs-appctl time/warp 1000
> +get_ages duration4 hard4 idle4
>
> printf "duration: %4s => %4s => %4s => %4s\n" $duration1 $duration2
> $duration3 $duration4
> printf "hard_age: %4s => %4s => %4s => %4s\n" $hard1 $hard2 $hard3 $hard4
> @@ -1116,5 +1115,13 @@ AT_CHECK([test $idle1 -lt $idle2])
> AT_CHECK([test $idle2 -lt $idle3])
> AT_CHECK([test $idle3 -gt $idle4])
>
> +# Check some invariant relationships.
> +AT_CHECK([test $duration1 = $idle1])
> +AT_CHECK([test $duration2 = $idle2])
> +AT_CHECK([test $duration3 = $idle3])
> +AT_CHECK([test $idle3 -gt $hard3])
> +AT_CHECK([test $idle4 -lt $hard4])
> +AT_CHECK([test $hard4 -lt $duration4])
> +
> OVS_VSWITCHD_STOP
> AT_CLEANUP
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
