Thanks, I will push this in a moment.
On Tue, Feb 07, 2012 at 01:25:33PM -0800, Ethan Jackson wrote:
> 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
