[ovs-dev] [flow monitor v2 12/12] ofproto: New feature to notify controllers of flow table changes.

Ben Pfaff blp at nicira.com
Thu Jul 12 21:04:37 UTC 2012


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.



More information about the dev mailing list