[ovs-dev] [PATCH 4/4] ovs-controller: Improve QoS abilities.

[Ben Pfaff]

On Wed, Sep 29, 2010 at 04:40:01PM -0700, Justin Pettit wrote:
> On Sep 23, 2010, at 3:05 PM, Ben Pfaff wrote:
> 
> > +    /* Queue distribution. */
> > +    uint32_t default_queue;     /* Default OpenFlow queue, or UINT32_MAX. */
> > +    struct hmap queue_numbers;  /* Map from port number to queue ID. */
> 
> Isn't this really a map from port number to lswitch_port, which is
> being used to determine the queue ID?

OK, I changed the comment to read "to lswitch_port".

> > @@ -247,8 +281,28 @@ process_switch_features(struct lswitch *sw, struct rconn *rconn OVS_UNUSED,
> >                         void *osf_)
> 
> Since queue configuration is based on name, do you think it's
> worthwhile to be a bit more dynamic about handling physical port
> changes.  For example, it would be pretty trivial to send an
> OFPT_FEATURES_REQUEST message anytime we get an OFPT_PORT_STATUS
> message that adds a port.  (Saying this is a dumb controller and the
> switch should be statically configured is a perfectly reasonable
> answer, too, though.  :-) )

I think I'll leave this as an exercise to the reader.

> > --- a/utilities/ovs-controller.8.in
> > +++ b/utilities/ovs-controller.8.in
> 
> In general, did you want to escape the hyphen in "port-name" and
> "queue-id" below?

Not really.  They're not literal syntax, they're just text, so I don't
think it's necessary.

> > @@ -98,7 +98,26 @@ sending packets and setting up flows.  Use one of these options,
> > supplying \fIid\fR as an OpenFlow queue ID as a decimal number, to
> > instead use that specific queue.
> > .IP
> > -This option may be useful for debugging quality of service setups.
> > +This option is incompatible with \fB\-N\fR or \fB\-\-normal\fR and
> > +with \fB\-H\fR or \fB\-\-hub\fR.  If more than one is specified then
> > +this option takes precedence.
> > +.IP
> > +This option may be useful for testing or debugging quality of service
> > +setups.
> > +.
> > +.IP "\fB\-Q \fIport-name\fB:\fIqueue-id\fR"
> > +.IP "\fB\-\-port\-queue \fIport-name\fB:\fIqueue-id\fR"
> > +Configures packets received on the port named \fIport-name\fR
> > +(e.g. \fBeth0\fR) to be output on OpenFlow queue ID \fIqueue-id\fR
> > +(specified as a decimal number).  This option overrides the default
> > +specified on \fB\-q\fR or \fB\-\-queue\fR.
> 
> It might be nice to say that it only overrides the default for the
> specified port.

OK, done.

> > +.IP
> > +This option is incompatible with \fB\-N\fR or \fB\-\-normal\fR and
> > +with \fB\-H\fR or \fB\-\-hub\fR.  If more than one is specified then
> > +this option takes precedence.
> > +.IP
> > +This option may be useful for testing or debugging quality of service
> > +setups.
> 
> It may be useful to explicitly mention that the "-Q" option can be
> specified multiple times for different ports.

Good point, done.

> > @@ -270,6 +276,23 @@ read_flow_file(const char *name)
> > }
> > 
> > static void
> > +add_port_queue(char *s)
> > +{
> > ...
> > +        ovs_fatal(0, "argument to -Q or --port-queue should take the form "
> > +                  "\"<port>:<queue id>\"");
> 
> How about "port-name" and "queue-id" to be consistent with the man page?

Done, thanks.

> > +    }
> > +
> > +    shash_add(&port_queues, port_name, (void *) (uintptr_t) atoi(queue_id));
> 
> Do you think it's worth notifying the user if they attempt to add the
> same port_name twice?

Good idea, done.

> > static void
> > @@ -398,7 +440,7 @@ usage(void)
> >            "  -H, --hub               act as hub instead of learning switch\n"
> >            "  -n, --noflow            pass traffic, but don't add flows\n"
> >            "  --max-idle=SECS         max idle time for new flows\n"
> > -           "  -N, --normal            use OFPAT_NORMAL action\n"
> > +           "  -N, --normal            use OFPP_NORMAL action\n"
> >            "  -w, --wildcard          use wildcards, not exact-match rules\n"
> >            "  -q, --queue=QUEUE       OpenFlow queue ID to use for output\n"
> >            "  --with-flows FILE       use the flows from FILE\n"
> 
> Did you want to add a "-Q, --port-queue" usage description?

Good idea, thanks.