On Thu, Jul 12, 2012 at 01:03:09PM -0700, Justin Pettit wrote:
> 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".
I did the latter, thanks.
> > + * 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.
OK, I made this change:
diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
index 9393bf3..c369cf4 100644
--- a/include/openflow/nicira-ext.h
+++ b/include/openflow/nicira-ext.h
@@ -2034,7 +2034,9 @@ OFP_ASSERT(sizeof(struct nx_action_controller) == 16);
* OVS reacts as follows:
*
* 1. OVS sends an NXT_FLOW_MONITOR_PAUSED message to the controller, following
- * all the already queued notifications.
+ * all the already queued notifications. After it receives this message,
+ * the controller knows that its view of the flow table, as represented by
+ * flow monitor notifications, is incomplete.
*
* 2. As long as the notification buffer is not empty:
*
@@ -2052,7 +2054,9 @@ OFP_ASSERT(sizeof(struct nx_action_controller) == 16);
* 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.
+ * 4. OVS sends an NXT_FLOW_MONITOR_RESUMED message to the controller. After
+ * it receives this message, the controller knows that its view of the flow
+ * table, as represented by flow monitor notifications, is again complete.
*
* This allows the maximum buffer space requirement for notifications to be
* bounded by the limit plus the maximum number of supported flows.
> > +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.")
OK, fixed.
> > +/* 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.
I added:
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 2f5830a..25a571f 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -3203,6 +3203,11 @@ ofputil_append_flow_monitor_request(
* into an abstract ofputil_flow_update in 'update'. The caller must have
* initialized update->match to point to space allocated for a cls_rule.
*
+ * Uses 'ofpacts' to store the abstract OFPACT_* version of the update's
+ * actions (except for NXFME_ABBREV, which never includes actions). The caller
+ * must initialize 'ofpacts' and retains ownership of it. 'update->ofpacts'
+ * will point into the 'ofpacts' buffer.
+ *
* 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
> My god, that's an impressive unit test.
Thanks.
> > +\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...]]").
Thanks, fixed.
> > + 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?
No, I don't see a purpose for them outside of testing.
I'll push this soon.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
