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

[Justin Pettit]

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?

> @@ -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.  :-) )

> --- 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?

> @@ -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.

> +.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.

> @@ -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?

> +    }
> +
> +    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?

> 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?

--Justin