On Fri, Jun 24, 2016 at 02:59:15PM +0530, bscha...@redhat.com wrote:
> ovn-northd processes the list of Port_Bindings and hashes the list of
> queues per chassis. When it finds a port with qos_parameters and without
> a queue_id, it allocates a free queue for the chassis that this port belongs.
> The queue_id information is stored in the options field of Port_binding table.
> Adds an action set_queue to the ingress table 0 of the logical flows
> which will be translated to openflow set_queue by ovn-controller
>
> ovn-controller opens the netdev corresponding to the tunnel interface's
> status:tunnel_egress_iface value and configures a HTB qdisc on it. Then for
> each SB port_binding that has queue_id set, it allocates a queue with the
> qos_parameters of that port. It also frees up unused queues.
>
> This patch replaces the older approach of policing
>
> Signed-off-by: Babu Shanmugam <bscha...@redhat.com>
Thanks for the new version.
I'm passing along an incremental to fold in. Some of the changes are
style. Others:
* Put hmap_node at the start of structs because it makes valgrind's
memory leak checker confident about pointers instead of calling
them "possible leaks".
* Avoid trying to modify the OVS database when there's no transaction
open.
* Log errors from netdev operations.
* Don't log a warning when setting up QoS.
* Fix a couple of memory leaks.
I was going to just apply this but there's an ongoing situation where we
might need to revert patches in this same area (see
http://openvswitch.org/pipermail/dev/2016-June/073608.html) and I don't
want to make that harder. So please sit on this until that situation
resolves.
--8<--------------------------cut here-------------------------->8--
diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index 97f9aee..c53554f 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -41,10 +41,10 @@ binding_reset_processing(void)
}
struct qos_queue {
+ struct hmap_node node;
uint32_t queue_id;
uint32_t max_rate;
uint32_t burst;
- struct hmap_node node;
};
void
@@ -189,37 +189,42 @@ get_qos_params(const struct sbrec_port_binding *pb,
struct hmap *queue_map)
}
struct qos_queue *node = xzalloc(sizeof *node);
-
+ hmap_insert(queue_map, &node->node, hash_int(queue_id, 0));
node->max_rate = max_rate;
node->burst = burst;
node->queue_id = queue_id;
- hmap_insert(queue_map, &node->node, hash_int(queue_id, 0));
}
static const struct ovsrec_qos *
get_noop_qos(struct controller_ctx *ctx)
{
const struct ovsrec_qos *qos;
-
- OVSREC_QOS_FOR_EACH(qos, ctx->ovs_idl) {
+ OVSREC_QOS_FOR_EACH (qos, ctx->ovs_idl) {
if (!strcmp(qos->type, "linux-noop")) {
return qos;
}
}
+ if (!ctx->ovs_idl_txn) {
+ return NULL;
+ }
qos = ovsrec_qos_insert(ctx->ovs_idl_txn);
ovsrec_qos_set_type(qos, "linux-noop");
return qos;
}
-static void
+static bool
set_noop_qos(struct controller_ctx *ctx, struct sset *egress_ifaces)
{
const struct ovsrec_qos *noop_qos = get_noop_qos(ctx);
+ if (!noop_qos) {
+ return false;
+ }
+
const struct ovsrec_port *port;
size_t count = 0;
- OVSREC_PORT_FOR_EACH(port, ctx->ovs_idl) {
+ OVSREC_PORT_FOR_EACH (port, ctx->ovs_idl) {
if (sset_contains(egress_ifaces, port->name)) {
ovsrec_port_set_qos(port, noop_qos);
count++;
@@ -228,22 +233,24 @@ set_noop_qos(struct controller_ctx *ctx, struct sset
*egress_ifaces)
break;
}
}
+ return true;
}
static void
setup_qos(const char *egress_iface, struct hmap *queue_map)
{
+ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
struct netdev *netdev_phy;
if (!egress_iface) {
/* Queues cannot be configured. */
return;
}
- VLOG_WARN("Setting up qos on %s", egress_iface);
- if (netdev_open(egress_iface, NULL, &netdev_phy) != 0) {
- static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
- VLOG_WARN_RL(&rl, "Unable to open netdev %s\n", egress_iface);
+ int error = netdev_open(egress_iface, NULL, &netdev_phy);
+ if (error) {
+ VLOG_WARN_RL(&rl, "%s: could not open netdev (%s)",
+ egress_iface, ovs_strerror(error));
return;
}
@@ -258,7 +265,11 @@ setup_qos(const char *egress_iface, struct hmap *queue_map)
return;
}
if (strcmp(qdisc_type, OVN_QOS_TYPE)) {
- netdev_set_qos(netdev_phy, OVN_QOS_TYPE, &qdisc_details);
+ error = netdev_set_qos(netdev_phy, OVN_QOS_TYPE, &qdisc_details);
+ if (error) {
+ VLOG_WARN_RL(&rl, "%s: could not configure QoS (%s)",
+ egress_iface, ovs_strerror(error));
+ }
}
/* Check and delete if needed. */
@@ -270,7 +281,7 @@ setup_qos(const char *egress_iface, struct hmap *queue_map)
smap_init(&queue_details);
hmap_init(&consistent_queues);
- NETDEV_QUEUE_FOR_EACH(&queue_id, &queue_details, &dump, netdev_phy) {
+ NETDEV_QUEUE_FOR_EACH (&queue_id, &queue_details, &dump, netdev_phy) {
bool is_queue_needed = false;
HMAP_FOR_EACH_WITH_HASH (sb_info, node, hash_int(queue_id, 0),
@@ -287,12 +298,16 @@ setup_qos(const char *egress_iface, struct hmap
*queue_map)
}
if (!is_queue_needed) {
- netdev_delete_queue(netdev_phy, queue_id);
+ error = netdev_delete_queue(netdev_phy, queue_id);
+ if (error) {
+ VLOG_WARN_RL(&rl, "%s: could not delete queue %u (%s)",
+ egress_iface, queue_id, ovs_strerror(error));
+ }
}
}
/* Create/Update queues. */
- HMAP_FOR_EACH(sb_info, node, queue_map) {
+ HMAP_FOR_EACH (sb_info, node, queue_map) {
if (hmap_contains(&consistent_queues, &sb_info->node)) {
hmap_remove(&consistent_queues, &sb_info->node);
continue;
@@ -301,7 +316,12 @@ setup_qos(const char *egress_iface, struct hmap *queue_map)
smap_clear(&queue_details);
smap_add_format(&queue_details, "max-rate", "%d", sb_info->max_rate);
smap_add_format(&queue_details, "burst", "%d", sb_info->burst);
- netdev_set_queue(netdev_phy, sb_info->queue_id, &queue_details);
+ error = netdev_set_queue(netdev_phy, sb_info->queue_id,
+ &queue_details);
+ if (error) {
+ VLOG_WARN_RL(&rl, "%s: could not configure queue %u (%s)",
+ egress_iface, sb_info->queue_id, ovs_strerror(error));
+ }
}
smap_destroy(&queue_details);
hmap_destroy(&consistent_queues);
@@ -419,11 +439,9 @@ binding_run(struct controller_ctx *ctx, const struct
ovsrec_bridge *br_int,
}
}
- if (!sset_is_empty(&egress_ifaces)) {
+ if (!sset_is_empty(&egress_ifaces) && set_noop_qos(ctx, &egress_ifaces)) {
const char *entry;
-
- set_noop_qos(ctx, &egress_ifaces);
- SSET_FOR_EACH(entry, &egress_ifaces) {
+ SSET_FOR_EACH (entry, &egress_ifaces) {
setup_qos(entry, &qos_map);
}
}
diff --git a/ovn/lib/actions.h b/ovn/lib/actions.h
index 14c23e8..81a38c5 100644
--- a/ovn/lib/actions.h
+++ b/ovn/lib/actions.h
@@ -22,8 +22,8 @@
#include "compiler.h"
#include "util.h"
-#define QDISC_MIN_QUEUE_ID (1)
-#define QDISC_MAX_QUEUE_ID (0xf000)
+#define QDISC_MIN_QUEUE_ID 0
+#define QDISC_MAX_QUEUE_ID 0xf000
struct expr;
struct lexer;
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 9da8614..f3495d7 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -239,8 +239,8 @@ allocate_tnlid(struct hmap *set, const char *name, uint32_t
max,
}
struct ovn_chassis_qdisc_queues {
- uint32_t queue_id;
struct hmap_node key_node;
+ uint32_t queue_id;
};
static void
@@ -305,7 +305,8 @@ free_chassis_queueid(struct hmap *set, const char *
chassis, uint32_t queue_id)
}
static inline bool
-port_has_qos_params(struct smap * opts) {
+port_has_qos_params(const struct smap *opts)
+{
return (smap_get(opts, "qos_max_rate") ||
smap_get(opts, "qos_burst"));
}
@@ -803,16 +804,11 @@ ovn_port_update_sbrec(const struct ovn_port *op,
if (strcmp(op->nbs->type, "router")) {
uint32_t queue_id = smap_get_int(
&op->sb->options, "qdisc_queue_id", 0);
- struct smap options;
-
- smap_clone(&options, &op->nbs->options);
- if (op->sb->chassis && port_has_qos_params(&options)
- && !queue_id) {
+ bool has_qos = port_has_qos_params(&op->nbs->options);
+ if (op->sb->chassis && has_qos && !queue_id) {
queue_id = allocate_chassis_queueid(chassis_qdisc_queues,
op->sb->chassis->name);
- }
- if (!port_has_qos_params(&options) && queue_id) {
- /* Free this queue. */
+ } else if (!has_qos && queue_id) {
free_chassis_queueid(chassis_qdisc_queues,
op->sb->chassis->name,
queue_id);
@@ -820,12 +816,16 @@ ovn_port_update_sbrec(const struct ovn_port *op,
}
if (queue_id) {
- /* Only when there is a valid queue. */
+ struct smap options;
+ smap_clone(&options, &op->nbs->options);
smap_add_format(&options,
"qdisc_queue_id", "%d", queue_id);
+ sbrec_port_binding_set_options(op->sb, &options);
+ smap_destroy(&options);
+ } else {
+ sbrec_port_binding_set_options(op->sb, &op->sb->options);
}
sbrec_port_binding_set_type(op->sb, op->nbs->type);
- sbrec_port_binding_set_options(op->sb, &options);
} else {
const char *chassis = NULL;
if (op->peer && op->peer->od && op->peer->od->nbr) {
@@ -1656,12 +1656,13 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap
*ports,
&match);
const char *queue_id = smap_get(&op->sb->options, "qdisc_queue_id");
if (queue_id) {
- ds_put_format(&action, "set_queue(%s);", queue_id);
+ ds_put_format(&action, "set_queue(%s); ", queue_id);
}
ds_put_cstr(&action, "next;");
ovn_lflow_add(lflows, op->od, S_SWITCH_IN_PORT_SEC_L2, 50,
ds_cstr(&match), ds_cstr(&action));
ds_destroy(&match);
+ ds_destroy(&action);
if (op->nbs->n_port_security) {
build_port_security_ip(P_IN, op, lflows);
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index 6655fab..c559d56 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -1070,12 +1070,18 @@
<dd>
<p>
- <b>Parameters</b>: Queue number <var>queue_number</var>, 32-bit
+ <b>Parameters</b>: Queue number <var>queue_number</var>, in the
range 0 to 61440.
</p>
<p>
- This is equivalent to Openflow set_queue. queue_number should be
- in the range of 1 to 61440
+ This is a logical equivalent of the OpenFlow <code>set_queue</code>
+ action. It affects packets that egress a hypervisor through a
+ physical interface. For nonzero <var>queue_number</var>, it
+ configures packet queuing to match the settings configured for the
+ <ref table="Port_Binding"/> with
+ <code>options:qdisc_queue_id</code> matching
+ <var>queue_number</var>. When <var>queue_number</var> is zero, it
+ resets queuing to the default strategy.
</p>
<p><b>Example:</b> <code>set_queue(10);</code></p>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
