On Fri, Aug 12, 2016 at 12:21:27PM +0530, bscha...@redhat.com wrote:
> From: Babu Shanmugam <bscha...@redhat.com>
>
> 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 revision. I have a few minor suggestions, appended as an
incremental diff.
Upon studying the code in ovn-northd, I find myself really confused by
the new ovn_chassis_qdisc_queues data structure. This hash table is
hashed on the basis of a chassis name, but the chassis name is not
included in the data structure for comparison purposes. That means that
chassis whose names hash to the same value will be considered the same.
In addition, it's possible to have more than one queue-id per chassis,
and each of those will of course hash to the same value (because the
queue_id is not part of the hash), so that will cause huge numbers of
collisions in the hmap. I don't understand the intent here, and
whatever it is needs to be reconsidered and reworked.
Thanks,
Ben.
--8<--------------------------cut here-------------------------->8--
diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index 02f55ae..192274a 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -117,8 +117,7 @@ get_local_iface_ids(const struct ovsrec_bridge *br_int,
const char *tunnel_iface
= smap_get(&iface_rec->status, "tunnel_egress_iface");
if (tunnel_iface) {
- sset_add(egress_ifaces,
- tunnel_iface);
+ sset_add(egress_ifaces, tunnel_iface);
}
}
}
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index beb9a79..26b3a12 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -831,15 +831,9 @@ parse_set_queue_action(struct action_context *ctx)
{
int queue_id;
- if (!lexer_match(ctx->lexer, LEX_T_LPAREN)) {
- action_syntax_error(ctx, "expecting `('");
- return;
- }
- if (!action_get_int(ctx, &queue_id)) {
- return;
- }
- if (!lexer_match(ctx->lexer, LEX_T_RPAREN)) {
- action_syntax_error(ctx, "expecting `)'");
+ if (!action_force_match(ctx->lexer, LEX_T_LPAREN)
+ || !action_get_int(ctx, &queue_id)
+ || !action_force_match(ctx->lexer, LEX_T_RPAREN)) {
return;
}
if (queue_id < QDISC_MIN_QUEUE_ID || queue_id > QDISC_MAX_QUEUE_ID) {
@@ -849,8 +843,7 @@ parse_set_queue_action(struct action_context *ctx)
return;
}
- struct ofpact_queue *set_queue = ofpact_put_SET_QUEUE(ctx->ofpacts);
- set_queue->queue_id = queue_id;
+ ofpact_put_SET_QUEUE(ctx->ofpacts)->queue_id = queue_id;
}
static void
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index 8a1ef43..b846f47 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -1792,7 +1792,8 @@ tcp.flags = RST;
<column name="options" key="qdisc_queue_id"
type='{"type": "integer", "minInteger": 1, "maxInteger": 61440}'>
Indicates the queue number on the physical device. This is same as the
- queue_id used in OpenFlow in struct ofp_action_enqueue.
+ <code>queue_id</code> used in OpenFlow in <code>struct
+ ofp_action_enqueue</code>.
</column>
</group>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
