On Jul 6, 2012, at 2:49 PM, Ben Pfaff wrote:
> + * 3. Whenever a change to a flow table entry matches some outstanding
> monitor
> + * request's criteria and flags, the switch sends a notification to the
> + * controller as an additional NXST_FLOW_MONITOR replies with xid 0.
I think "an" should either be dropped or "replies" should be "reply".
> + * When Open vSwitch's notification buffer space reaches a limiting
> threshold,
> + * OVS reacts as follows:
> + *
> + * 1. OVS sends an NXT_FLOW_MONITOR_PAUSED message to the controller,
> following
> + * all the already queued notifications.
> + *
> + * 2. As long as the notification buffer is not empty:
> + *
> + * - NXMFE_ADD and NXFME_MODIFIED notifications will not be sent.
> + *
> + * - NXFME_DELETED notifications will still be sent, but only for
> flows
> + * that existed before OVS sent NXT_FLOW_MONITOR_PAUSED.
> + *
> + * - NXFME_ABBREV notifications will not be sent. They are treated as
> + * the expanded version (and therefore only the NXFME_DELETED
> + * components, if any, are sent).
> + *
> + * 3. When the notification buffer empties, OVS sends NXFME_ADD notifications
> + * for flows added since the buffer reached its limit and NXFME_MODIFIED
> + * notifications for flows that existed before the limit was reached and
> + * changed after the limit was reached.
> + *
> + * 4. OVS sends an NXT_FLOW_MONITOR_RESUMED message to the controller.
> + *
> + * This allows the maximum buffer space requirement for notifications to be
> + * bounded by the limit plus the maximum number of supported flows.
As we discussed in person, it might be nice to provide more explanation about
why RESUMED is sent after messages are caught up. For example, that the
controller knows after receiving the RESUMED that it is now caught up.
> +struct nx_flow_update_abbrev {
> + ovs_be16 length; /* Length of this entry. */
In the past, we've referred to fixed length records with the actual length
("Length is 8.")
> +/* Converts an NXST_FLOW_MONITOR reply (also known as a flow update) in 'msg'
> + * into an abstract ofputil_flow_update in 'update'. The caller must have
> + * initialized update->match to point to space allocated for a cls_rule.
> + *
> + * Multiple flow updates can be packed into a single OpenFlow message.
> Calling
> + * this function multiple times for a single 'msg' iterates through the
> + * updates. The caller must initially leave 'msg''s layer pointers null and
> + * not modify them between calls.
> + *
> + * Returns 0 if successful, EOF if no updates were left in this 'msg',
> + * otherwise an OFPERR_* value. */
> +int
> +ofputil_decode_flow_update(struct ofputil_flow_update *update,
> + struct ofpbuf *msg, struct ofpbuf *ofpacts)
It might be good to explain the "ofpacts" argument in the description.
> +AT_SETUP([ofproto - flow monitoring pause and resume])
> +AT_KEYWORDS([monitor])
> +
> +# With a Linux kernel, this file has the maximum socket receive buffer
> +# size. That's important for this test, which tests behavior when the
> +# receive buffer overflows.
> +AT_SKIP_IF([test ! -e /proc/sys/net/core/rmem_max])
> +
> +# Calculate the total amount of queuing: rmem_max in the kernel, 128 kB
> +# in ofproto sending userspace (see ofmonitor_flush() in connmgr.c).
> +rmem_max=`cat /proc/sys/net/core/rmem_max`
> +queue_size=`expr $rmem_max + 128 \* 1024`
> +echo rmem_max=$rmem_max queue_size=$queue_size
> +
> +# Each flow update message takes up at least 48 bytes of space in queues
> +# and in practice more than that.
> +n_msgs=`expr $queue_size / 48`
> +echo n_msgs=$n_msgs
> +
> +OVS_VSWITCHD_START
> +
> +# Start a monitor watching the flow table, then make it block.
> +ovs-ofctl monitor br0 watch: --detach --no-chdir --pidfile >monitor.log 2>&1
> +AT_CAPTURE_FILE([monitor.log])
> +ovs-appctl -t ovs-ofctl ofctl/block
> +
> +# Add $n_msgs flows.
> +(echo "in_port=2,actions=output:2"
> +perl -e '
> + for ($i = 0; $i < '$n_msgs'; $i++) {
> + print "cookie=1,reg1=$i,actions=drop\n";
> + }
> +') > flows.txt
> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> +AT_CHECK([ovs-ofctl add-flow br0 in_port=1,cookie=3,actions=drop])
> +AT_CHECK([ovs-ofctl mod-flows br0 in_port=2,cookie=2,actions=output:2])
> +AT_CHECK([ovs-ofctl del-flows br0 cookie=1/-1])
> +
> +ovs-appctl -t ovs-ofctl ofctl/unblock
> +ovs-appctl -t ovs-ofctl ofctl/barrier
> +
> +ovs-appctl -t ovs-ofctl exit
> +
> +# Check that the flow monitor reported the same number of flows
> +# added and deleted, but fewer than we actually added and deleted.
> +adds=`grep -c 'ADDED.*reg1=' monitor.log`
> +deletes=`grep -c 'DELETED.*reg1=' monitor.log`
> +echo adds=$adds deletes=$deletes
> +AT_CHECK([test $adds -gt 100 && test $adds -lt $n_msgs])
> +AT_CHECK([test $adds = $deletes])
> +
> +# Check that the flow monitor reported everything in the expected order.
> +AT_CHECK([ofctl_strip < monitor.log | sed -n -e '
> +/reg1=0x22\b/p
> +/cookie=0x[[23]]/p
> +/NXT_FLOW_MONITOR_PAUSED:/p
> +/NXT_FLOW_MONITOR_RESUMED:/p
> +'], [0],
> +[ event=ADDED table=0 cookie=0x1 reg1=0x22
> +NXT_FLOW_MONITOR_PAUSED:
> + event=DELETED reason=delete table=0 cookie=0x1 reg1=0x22
> + event=ADDED table=0 cookie=0x3 in_port=1
> + event=MODIFIED table=0 cookie=0x2 in_port=2 actions=output:2
> +NXT_FLOW_MONITOR_RESUMED:
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
My god, that's an impressive unit test.
> +\fBwatch\fR[\fB:\fIspec\fR...] causes \fBovs\-ofctl\fR to send a
I believe the colon is required, so I think you want to move it outside the
square brackets to match the earlier definition
("[\fBwatch:\fR[\fIspec\fR...]]").
> + unixctl_command_register("ofctl/block", "", 0, 0, ofctl_block, &block);
> + unixctl_command_register("ofctl/unblock", "", 0, 0, ofctl_unblock,
> &block);
Did you want to document these commands in the ovs-ofctl man page?
Otherwise, it looks great. Thanks!
--Justin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
