> On Fri, Dec 9, 2022 at 9:43 AM Lorenzo Bianconi
> <[email protected]> wrote:
> >
> > > On Sun, Dec 4, 2022 at 5:06 PM Lorenzo Bianconi
> > > <[email protected]> wrote:
> > > >
> > > > Introduce the capability to apply QoS rules for logical switch ports
> > > > claimed by ovn-controller. Rely on shash instead of sset for
> > > > egress_ifaces.
> > > >
> > > > Acked-by: Mark Michelson <[email protected]>
> > > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129742
> > > > Signed-off-by: Lorenzo Bianconi <[email protected]>
> > >
> > > Hi Lorenzo,
> >
> > Hi Numan,
> >
> > thx for the review :)
> >
> > >
> > > Thanks for addressing the comments.
> > >
> > > I tested this version and compared with the present main.
> > >
> > > I see a few differences.
> > >
> > > With the main version, when I configure qos params to a localnet
> > > logical port, ovn-controller creates a qdisc on the tunnel interface
> > > which I guess is expected.
> > > But I'm still not sure why we need to configure qdiscs on the tunnel
> > > interface. But that's a story. Perhaps I need to see the original
> > > QoS commits and understand why it was added.
> > >
> > > With your patch, I don't see that happening. Perhaps there is a bug
> > > in setup_qos() function now that 'shash' is used to store the egress
> > > ifaces with the logical port and
> > > there is no logical port for the tunnel interfaces.
> >
> > uhm, I can look into this. Do you think we should keep this behaviour?
>
> Frankly I don't know. We need to find out why qos was configured in
> the first place.
>
> I do agree with Ilya's comment that it is odd to configure qos on the
> tunnel interface
> and that could impact the other traffic.
>
>
> >
> > >
> > > Regarding the option - "ovn-egress-iface". I suppose the expectation
> > > from the CMS is to set the qos parameters in the logical port and also
> > > set this option in the
> > > ovs interface right ? I don't really see a need for this. I think if
> > > CMS configures the qos parameters in the logical port options,
> > > ovn-controller should configure the qos.
> > > I think otherwise this would complicate the CMS implementation. Thoughts
> > > ?
> >
> > ovn-egress-iface is mandatory since it is used in setup_qos() to look for
> > the
> > proper netdevice pointer to configure ovs qdisc (please note it is used to
> > identify ovs interfaces and not ovn ones).
> > I think it can be doable (not sure about the complexity) to avoid it for
> > logical switch ports (since we have iface-id in them external_ids column of
> > ovs interface table) but I do not think we can avoid to set
> > "ovn-egress-iface"
> > for localnet port since afaik there is no direct link between ovn localnet
> > port
> > and ovs interface used to connect the underlay network, right? If so I guess
> > it is better to keep it for both localnet and lsp ports. Do you agree?
>
> I think it's ok to expect CMS to set "ovn-egress-iface" for the
> physical nic connected to the
> provider bridge. But I don't think we should do the same for the
> normal logical port VIFs.
does it worth to introduce a difference here?
>
>
>
> >
> > >
> > >
> > > Please see a few comments below
> > >
> > > > ---
> > > > Changes since v3:
> > > > - fix typo in new system-ovn test
> > > > Changes since v2:
> > > > - fix qos configuration restarting ovn-controller
> > > > Changes since v1:
> > > > - improve ovs interface lookup
> > > > - improve system-tests
> > > > ---
> > > > controller/binding.c | 155 ++++++++++++++++++++++--------------
> > > > controller/binding.h | 5 +-
> > > > controller/ovn-controller.c | 15 ++--
> > > > tests/system-ovn.at | 48 +++++++++++
> > > > 4 files changed, 156 insertions(+), 67 deletions(-)
> > > >
> > > > diff --git a/controller/binding.c b/controller/binding.c
> > > > index 5df62baef..53520263c 100644
> > > > --- a/controller/binding.c
> > > > +++ b/controller/binding.c
> > > > @@ -115,6 +115,7 @@ struct qos_queue {
> > > > uint32_t min_rate;
> > > > uint32_t max_rate;
> > > > uint32_t burst;
> > > > + char *port_name;
> > > > };
> > > >
> > > > void
> > > > @@ -147,25 +148,50 @@ static void update_lport_tracking(const struct
> > > > sbrec_port_binding *pb,
> > > > struct hmap *tracked_dp_bindings,
> > > > bool claimed);
> > > >
> > > > +static bool is_lport_vif(const struct sbrec_port_binding *pb);
> > > > +
> > > > +static struct qos_queue *
> > > > +get_qos_map_entry(struct hmap *queue_map, const char *name)
> > > > +{
> > > > + struct qos_queue *qos_node;
> > > > + HMAP_FOR_EACH (qos_node, node, queue_map) {
> > > > + if (!strcmp(qos_node->port_name, name)) {
> > > > + return qos_node;
> > > > + }
> > > > + }
> > > > +
> > > > + return NULL;
> > > > +}
> > > > +
> > > > static void
> > > > -get_qos_params(const struct sbrec_port_binding *pb, struct hmap
> > > > *queue_map)
> > > > +update_qos_params(const struct sbrec_port_binding *pb, struct hmap
> > > > *queue_map)
> > > > {
> > > > uint32_t min_rate = smap_get_int(&pb->options, "qos_min_rate", 0);
> > > > uint32_t max_rate = smap_get_int(&pb->options, "qos_max_rate", 0);
> > > > uint32_t burst = smap_get_int(&pb->options, "qos_burst", 0);
> > > > uint32_t queue_id = smap_get_int(&pb->options, "qdisc_queue_id",
> > > > 0);
> > > >
> > > > + struct qos_queue *node = get_qos_map_entry(queue_map,
> > > > pb->logical_port);
> > > > +
> > > > if ((!min_rate && !max_rate && !burst) || !queue_id) {
> > > > /* Qos is not configured for this port. */
> > > > + if (node) {
> > > > + hmap_remove(queue_map, &node->node);
> > > > + free(node->port_name);
> > > > + free(node);
> > > > + }
> > > > return;
> > > > }
> > > >
> > > > - struct qos_queue *node = xzalloc(sizeof *node);
> > > > - hmap_insert(queue_map, &node->node, hash_int(queue_id, 0));
> > > > + if (!node) {
> > > > + node = xzalloc(sizeof *node);
> > > > + hmap_insert(queue_map, &node->node, hash_int(queue_id, 0));
> > > > + node->port_name = xstrdup(pb->logical_port);
> > > > + }
> > > > + node->queue_id = queue_id;
> > > > node->min_rate = min_rate;
> > > > node->max_rate = max_rate;
> > > > node->burst = burst;
> > > > - node->queue_id = queue_id;
> > > > }
> > > >
> > > > static const struct ovsrec_qos *
> > > > @@ -191,7 +217,7 @@ static bool
> > > > set_noop_qos(struct ovsdb_idl_txn *ovs_idl_txn,
> > > > const struct ovsrec_port_table *port_table,
> > > > const struct ovsrec_qos_table *qos_table,
> > > > - struct sset *egress_ifaces)
> > > > + struct shash *egress_ifaces)
> > > > {
> > > > if (!ovs_idl_txn) {
> > > > return false;
> > > > @@ -206,11 +232,11 @@ set_noop_qos(struct ovsdb_idl_txn *ovs_idl_txn,
> > > > size_t count = 0;
> > > >
> > > > OVSREC_PORT_TABLE_FOR_EACH (port, port_table) {
> > > > - if (sset_contains(egress_ifaces, port->name)) {
> > > > + if (shash_find(egress_ifaces, port->name)) {
> > > > ovsrec_port_set_qos(port, noop_qos);
> > > > count++;
> > > > }
> > > > - if (sset_count(egress_ifaces) == count) {
> > > > + if (shash_count(egress_ifaces) == count) {
> > > > break;
> > > > }
> > > > }
> > > > @@ -236,7 +262,8 @@ set_qos_type(struct netdev *netdev, const char
> > > > *type)
> > > > }
> > > >
> > > > static void
> > > > -setup_qos(const char *egress_iface, struct hmap *queue_map)
> > > > +setup_qos(const char *egress_iface, const char *logical_port,
> > > > + struct hmap *queue_map)
> > > > {
> > > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
> > > > struct netdev *netdev_phy;
> > > > @@ -281,7 +308,7 @@ setup_qos(const char *egress_iface, struct hmap
> > > > *queue_map)
> > > > * a configuration setting.
> > > > *
> > > > * - Otherwise leave the qdisc alone. */
> > > > - if (hmap_is_empty(queue_map)) {
> > > > + if (!get_qos_map_entry(queue_map, logical_port)) {
> > > > if (!strcmp(qdisc_type, OVN_QOS_TYPE)) {
> > > > set_qos_type(netdev_phy, "");
> > > > }
> > > > @@ -338,6 +365,10 @@ setup_qos(const char *egress_iface, struct hmap
> > > > *queue_map)
> > > > continue;
> > > > }
> > > >
> > > > + if (strcmp(sb_info->port_name, logical_port)) {
> > > > + continue;
> > > > + }
> > > > +
> > > > smap_clear(&queue_details);
> > > > smap_add_format(&queue_details, "min-rate", "%d",
> > > > sb_info->min_rate);
> > > > smap_add_format(&queue_details, "max-rate", "%d",
> > > > sb_info->max_rate);
> > > > @@ -354,11 +385,12 @@ setup_qos(const char *egress_iface, struct hmap
> > > > *queue_map)
> > > > netdev_close(netdev_phy);
> > > > }
> > > >
> > > > -static void
> > > > +void
> > > > destroy_qos_map(struct hmap *qos_map)
> > > > {
> > > > struct qos_queue *qos_queue;
> > > > HMAP_FOR_EACH_POP (qos_queue, node, qos_map) {
> > > > + free(qos_queue->port_name);
> > > > free(qos_queue);
> > > > }
> > > >
> > > > @@ -404,7 +436,7 @@ sbrec_get_port_encap(const struct sbrec_chassis
> > > > *chassis_rec,
> > > > static void
> > > > add_localnet_egress_interface_mappings(
> > > > const struct sbrec_port_binding *port_binding,
> > > > - struct shash *bridge_mappings, struct sset *egress_ifaces)
> > > > + struct shash *bridge_mappings, struct shash *egress_ifaces)
> > > > {
> > > > const char *network = smap_get(&port_binding->options,
> > > > "network_name");
> > > > if (!network) {
> > > > @@ -429,7 +461,8 @@ add_localnet_egress_interface_mappings(
> > > > if (!is_egress_iface) {
> > > > continue;
> > > > }
> > > > - sset_add(egress_ifaces, iface_rec->name);
> > > > + shash_add(egress_ifaces, iface_rec->name,
> > > > + port_binding->logical_port);
> > > > }
> > > > }
> > > > }
> > > > @@ -474,7 +507,7 @@ update_ld_multichassis_ports(const struct
> > > > sbrec_port_binding *binding_rec,
> > > > static void
> > > > update_ld_localnet_port(const struct sbrec_port_binding *binding_rec,
> > > > struct shash *bridge_mappings,
> > > > - struct sset *egress_ifaces,
> > > > + struct shash *egress_ifaces,
> > > > struct hmap *local_datapaths)
> > > > {
> > > > /* Ignore localnet ports for unplugged networks. */
> > > > @@ -1456,7 +1489,7 @@ consider_vif_lport_(const struct
> > > > sbrec_port_binding *pb,
> > > >
> > > > b_ctx_out->tracked_dp_bindings);
> > > > }
> > > > if (b_lport->lbinding->iface && qos_map &&
> > > > b_ctx_in->ovs_idl_txn) {
> > > > - get_qos_params(pb, qos_map);
> > > > + update_qos_params(pb, qos_map);
> > > > }
> > > > } else {
> > > > /* We could, but can't claim the lport. */
> > > > @@ -1519,6 +1552,16 @@ consider_vif_lport(const struct
> > > > sbrec_port_binding *pb,
> > > > b_lport = local_binding_add_lport(binding_lports,
> > > > lbinding, pb,
> > > > LP_VIF);
> > > > }
> > > > +
> > > > + if (lbinding->iface &&
> > > > + smap_get(&lbinding->iface->external_ids,
> > > > "ovn-egress-iface")) {
> > > > + const char *iface_id =
> > > > smap_get(&lbinding->iface->external_ids,
> > > > + "iface-id");
> > > > + if (iface_id) {
> > > > + shash_add(b_ctx_out->egress_ifaces,
> > > > lbinding->iface->name,
> > > > + iface_id);
> > >
> > > When a qos is configured on a logical port, this patch is adding 2
> > > entries in the 'egress_ifaces' shash for the same logical port.
> > > I think it's because of the above 'shash_add'. I think you should use
> > > shash_replace instead.
> >
> > ack, I will fix it.
> >
> > >
> > > When a full recompute happens, the function build_local_bindings()
> > > also adds the qos configured logical ports to the 'egress_ifaces'
> > > and later if there are any updates to the logical port, this function
> > > - consider_vif_lport() also adds it to the shash.
> > >
> > > IMO the qos support in OVN needs a different approach. Instead of
> > > configuring the qos using netdev() perhaps we should rely on the OVS
> > > meters.
> > > Maybe this was brought up earlier too.
> >
> > >
> > > I'm of the opinion that instead of supporting Qos for logical ports
> > > using netdev, we should use OVS meters (not just for logical ports,
> > > even for localnet ports). Thoughts ?
> >
> > ovn qos is used for egress traffic (shaping) while ovn meters are used
> > for incoming one (policing).
> > What we can do is to avoid configuring directly netdev qdisc and rely on
> > ovs to perform this configuration but this would be quite a massive rework.
> > Do you think we should do it now before adding this feature or it is ok
> > to do it after we added this feature? I do not have a strong opinion about
> > it.
>
> IMO its better to add this new feature using the native OVS QoS support.
> And along with that we can remove the netdev implementation from OVN.
>
> This is what I think. Anyway this feature is out of window for 22.12.
> We can perhaps target this feature with the new approach in 23.03.
>
> Although I will not object if other maintainers are fine getting this
> feature first
> and then reimplement using OVS native QoS support.
I am fine to work on a proper solution for lsp and then move localnet port too.
Regards,
Lorenzo
>
> Thanks
> Numan
>
> >
> > Regards,
> > Lorenzo
> >
> > >
> > > @Dumitru Ceara @Han Zhou @Mark Michelson @Ilya Maximets Do you have
> > > any comments or suggestions here ?
> > >
> > > Thanks
> > > Numan
> > >
> > >
> > >
> > > > + }
> > > > + }
> > > > }
> > > >
> > > > return consider_vif_lport_(pb, can_bind, b_ctx_in, b_ctx_out,
> > > > @@ -1785,7 +1828,7 @@ consider_localnet_lport(const struct
> > > > sbrec_port_binding *pb,
> > > > update_local_lports(pb->logical_port, b_ctx_out);
> > > >
> > > > if (qos_map && b_ctx_in->ovs_idl_txn) {
> > > > - get_qos_params(pb, qos_map);
> > > > + update_qos_params(pb, qos_map);
> > > > }
> > > >
> > > > update_related_lport(pb, b_ctx_out);
> > > > @@ -1861,14 +1904,14 @@ build_local_bindings(struct binding_ctx_in
> > > > *b_ctx_in,
> > > > &b_ctx_out->lbinding_data->bindings;
> > > > for (j = 0; j < port_rec->n_interfaces; j++) {
> > > > const struct ovsrec_interface *iface_rec;
> > > > + struct local_binding *lbinding = NULL;
> > > >
> > > > iface_rec = port_rec->interfaces[j];
> > > > iface_id = smap_get(&iface_rec->external_ids, "iface-id");
> > > > int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport
> > > > : 0;
> > > >
> > > > if (iface_id && ofport > 0) {
> > > > - struct local_binding *lbinding =
> > > > - local_binding_find(local_bindings, iface_id);
> > > > + lbinding = local_binding_find(local_bindings,
> > > > iface_id);
> > > > if (!lbinding) {
> > > > lbinding = local_binding_create(iface_id,
> > > > iface_rec);
> > > > local_binding_add(local_bindings, lbinding);
> > > > @@ -1895,8 +1938,11 @@ build_local_bindings(struct binding_ctx_in
> > > > *b_ctx_in,
> > > > const char *tunnel_iface
> > > > = smap_get(&iface_rec->status,
> > > > "tunnel_egress_iface");
> > > > if (tunnel_iface) {
> > > > - sset_add(b_ctx_out->egress_ifaces, tunnel_iface);
> > > > + shash_add(b_ctx_out->egress_ifaces, tunnel_iface,
> > > > "");
> > > > }
> > > > + } else if (lbinding && smap_get(&iface_rec->external_ids,
> > > > + "ovn-egress-iface")) {
> > > > + shash_add(b_ctx_out->egress_ifaces, iface_rec->name,
> > > > iface_id);
> > > > }
> > > > }
> > > > }
> > > > @@ -1910,16 +1956,11 @@ binding_run(struct binding_ctx_in *b_ctx_in,
> > > > struct binding_ctx_out *b_ctx_out)
> > > > }
> > > >
> > > > struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings);
> > > > - struct hmap qos_map;
> > > >
> > > > - hmap_init(&qos_map);
> > > > if (b_ctx_in->br_int) {
> > > > build_local_bindings(b_ctx_in, b_ctx_out);
> > > > }
> > > >
> > > > - struct hmap *qos_map_ptr =
> > > > - !sset_is_empty(b_ctx_out->egress_ifaces) ? &qos_map : NULL;
> > > > -
> > > > struct ovs_list localnet_lports =
> > > > OVS_LIST_INITIALIZER(&localnet_lports);
> > > > struct ovs_list external_lports =
> > > > OVS_LIST_INITIALIZER(&external_lports);
> > > > struct ovs_list multichassis_ports = OVS_LIST_INITIALIZER(
> > > > @@ -1956,7 +1997,8 @@ binding_run(struct binding_ctx_in *b_ctx_in,
> > > > struct binding_ctx_out *b_ctx_out)
> > > > break;
> > > >
> > > > case LP_VIF:
> > > > - consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL,
> > > > qos_map_ptr);
> > > > + consider_vif_lport(pb, b_ctx_in, b_ctx_out, NULL,
> > > > + b_ctx_out->qos_map);
> > > > if (pb->additional_chassis) {
> > > > struct lport *multichassis_lport = xmalloc(
> > > > sizeof *multichassis_lport);
> > > > @@ -1967,11 +2009,13 @@ binding_run(struct binding_ctx_in *b_ctx_in,
> > > > struct binding_ctx_out *b_ctx_out)
> > > > break;
> > > >
> > > > case LP_CONTAINER:
> > > > - consider_container_lport(pb, b_ctx_in, b_ctx_out,
> > > > qos_map_ptr);
> > > > + consider_container_lport(pb, b_ctx_in, b_ctx_out,
> > > > + b_ctx_out->qos_map);
> > > > break;
> > > >
> > > > case LP_VIRTUAL:
> > > > - consider_virtual_lport(pb, b_ctx_in, b_ctx_out,
> > > > qos_map_ptr);
> > > > + consider_virtual_lport(pb, b_ctx_in, b_ctx_out,
> > > > + b_ctx_out->qos_map);
> > > > break;
> > > >
> > > > case LP_L2GATEWAY:
> > > > @@ -1994,7 +2038,8 @@ binding_run(struct binding_ctx_in *b_ctx_in,
> > > > struct binding_ctx_out *b_ctx_out)
> > > > break;
> > > >
> > > > case LP_LOCALNET: {
> > > > - consider_localnet_lport(pb, b_ctx_in, b_ctx_out, &qos_map);
> > > > + consider_localnet_lport(pb, b_ctx_in, b_ctx_out,
> > > > + b_ctx_out->qos_map);
> > > > struct lport *lnet_lport = xmalloc(sizeof *lnet_lport);
> > > > lnet_lport->pb = pb;
> > > > ovs_list_push_back(&localnet_lports,
> > > > &lnet_lport->list_node);
> > > > @@ -2051,17 +2096,15 @@ binding_run(struct binding_ctx_in *b_ctx_in,
> > > > struct binding_ctx_out *b_ctx_out)
> > > >
> > > > shash_destroy(&bridge_mappings);
> > > >
> > > > - if (!sset_is_empty(b_ctx_out->egress_ifaces)
> > > > + if (!shash_is_empty(b_ctx_out->egress_ifaces)
> > > > && set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table,
> > > > b_ctx_in->qos_table,
> > > > b_ctx_out->egress_ifaces)) {
> > > > - const char *entry;
> > > > - SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) {
> > > > - setup_qos(entry, &qos_map);
> > > > + struct shash_node *entry;
> > > > + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) {
> > > > + setup_qos(entry->name, entry->data, b_ctx_out->qos_map);
> > > > }
> > > > }
> > > >
> > > > - destroy_qos_map(&qos_map);
> > > > -
> > > > cleanup_claimed_port_timestamps();
> > > > }
> > > >
> > > > @@ -2447,7 +2490,7 @@ binding_handle_ovs_interface_changes(struct
> > > > binding_ctx_in *b_ctx_in,
> > > > }
> > > >
> > > > if (smap_get(&iface_rec->external_ids, "ovn-egress-iface") ||
> > > > - sset_contains(b_ctx_out->egress_ifaces, iface_rec->name)) {
> > > > + shash_find(b_ctx_out->egress_ifaces, iface_rec->name)) {
> > > > handled = false;
> > > > break;
> > > > }
> > > > @@ -2493,10 +2536,6 @@ binding_handle_ovs_interface_changes(struct
> > > > binding_ctx_in *b_ctx_in,
> > > > return false;
> > > > }
> > > >
> > > > - struct hmap qos_map = HMAP_INITIALIZER(&qos_map);
> > > > - struct hmap *qos_map_ptr =
> > > > - sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map;
> > > > -
> > > > /*
> > > > * We consider an OVS interface for claiming if the following
> > > > * 2 conditions are met:
> > > > @@ -2525,24 +2564,22 @@ binding_handle_ovs_interface_changes(struct
> > > > binding_ctx_in *b_ctx_in,
> > > > if (iface_id && ofport > 0 &&
> > > > is_iface_in_int_bridge(iface_rec, b_ctx_in->br_int)) {
> > > > handled = consider_iface_claim(iface_rec, iface_id,
> > > > b_ctx_in,
> > > > - b_ctx_out, qos_map_ptr);
> > > > + b_ctx_out,
> > > > b_ctx_out->qos_map);
> > > > if (!handled) {
> > > > break;
> > > > }
> > > > }
> > > > }
> > > >
> > > > - if (handled && qos_map_ptr && set_noop_qos(b_ctx_in->ovs_idl_txn,
> > > > - b_ctx_in->port_table,
> > > > - b_ctx_in->qos_table,
> > > > -
> > > > b_ctx_out->egress_ifaces)) {
> > > > - const char *entry;
> > > > - SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) {
> > > > - setup_qos(entry, &qos_map);
> > > > + if (handled &&
> > > > + set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table,
> > > > + b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) {
> > > > + struct shash_node *entry;
> > > > + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) {
> > > > + setup_qos(entry->name, entry->data, b_ctx_out->qos_map);
> > > > }
> > > > }
> > > >
> > > > - destroy_qos_map(&qos_map);
> > > > return handled;
> > > > }
> > > >
> > > > @@ -2977,10 +3014,6 @@ delete_done:
> > > > return false;
> > > > }
> > > >
> > > > - struct hmap qos_map = HMAP_INITIALIZER(&qos_map);
> > > > - struct hmap *qos_map_ptr =
> > > > - sset_is_empty(b_ctx_out->egress_ifaces) ? NULL : &qos_map;
> > > > -
> > > > SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb,
> > > >
> > > > b_ctx_in->port_binding_table) {
> > > > /* Loop to handle create and update changes only. */
> > > > @@ -2992,7 +3025,8 @@ delete_done:
> > > > update_ld_peers(pb, b_ctx_out->local_datapaths);
> > > > }
> > > >
> > > > - handled = handle_updated_port(b_ctx_in, b_ctx_out, pb,
> > > > qos_map_ptr);
> > > > + handled = handle_updated_port(b_ctx_in, b_ctx_out, pb,
> > > > + b_ctx_out->qos_map);
> > > > if (!handled) {
> > > > break;
> > > > }
> > > > @@ -3009,7 +3043,8 @@ delete_done:
> > > > sset_find_and_delete(b_ctx_out->postponed_ports,
> > > > port_name);
> > > > continue;
> > > > }
> > > > - handled = handle_updated_port(b_ctx_in, b_ctx_out, pb,
> > > > qos_map_ptr);
> > > > + handled = handle_updated_port(b_ctx_in, b_ctx_out, pb,
> > > > + b_ctx_out->qos_map);
> > > > if (!handled) {
> > > > break;
> > > > }
> > > > @@ -3055,17 +3090,15 @@ delete_done:
> > > > shash_destroy(&bridge_mappings);
> > > > }
> > > >
> > > > - if (handled && qos_map_ptr && set_noop_qos(b_ctx_in->ovs_idl_txn,
> > > > - b_ctx_in->port_table,
> > > > - b_ctx_in->qos_table,
> > > > -
> > > > b_ctx_out->egress_ifaces)) {
> > > > - const char *entry;
> > > > - SSET_FOR_EACH (entry, b_ctx_out->egress_ifaces) {
> > > > - setup_qos(entry, &qos_map);
> > > > + if (handled &&
> > > > + set_noop_qos(b_ctx_in->ovs_idl_txn, b_ctx_in->port_table,
> > > > + b_ctx_in->qos_table, b_ctx_out->egress_ifaces)) {
> > > > + struct shash_node *entry;
> > > > + SHASH_FOR_EACH (entry, b_ctx_out->egress_ifaces) {
> > > > + setup_qos(entry->name, entry->data, b_ctx_out->qos_map);
> > > > }
> > > > }
> > > >
> > > > - destroy_qos_map(&qos_map);
> > > > return handled;
> > > > }
> > > >
> > > > diff --git a/controller/binding.h b/controller/binding.h
> > > > index 6c3a98b02..8be33eddb 100644
> > > > --- a/controller/binding.h
> > > > +++ b/controller/binding.h
> > > > @@ -91,7 +91,8 @@ struct binding_ctx_out {
> > > > */
> > > > bool non_vif_ports_changed;
> > > >
> > > > - struct sset *egress_ifaces;
> > > > + struct shash *egress_ifaces;
> > > > + struct hmap *qos_map;
> > > > /* smap of OVS interface name as key and
> > > > * OVS interface external_ids:iface-id as value. */
> > > > struct smap *local_iface_ids;
> > > > @@ -195,6 +196,8 @@ void set_pb_chassis_in_sbrec(const struct
> > > > sbrec_port_binding *pb,
> > > > const struct sbrec_chassis *chassis_rec,
> > > > bool is_set);
> > > >
> > > > +void destroy_qos_map(struct hmap *qos_map);
> > > > +
> > > > /* Corresponds to each Port_Binding.type. */
> > > > enum en_lport_type {
> > > > LP_UNKNOWN,
> > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > > > index d6251afb8..066d76869 100644
> > > > --- a/controller/ovn-controller.c
> > > > +++ b/controller/ovn-controller.c
> > > > @@ -1276,7 +1276,8 @@ struct ed_type_runtime_data {
> > > > struct sset active_tunnels;
> > > >
> > > > /* runtime data engine private data. */
> > > > - struct sset egress_ifaces;
> > > > + struct shash egress_ifaces;
> > > > + struct hmap qos_map;
> > > > struct smap local_iface_ids;
> > > >
> > > > /* Tracked data. See below for more details and comments. */
> > > > @@ -1372,7 +1373,8 @@ en_runtime_data_init(struct engine_node *node
> > > > OVS_UNUSED,
> > > > sset_init(&data->local_lports);
> > > > related_lports_init(&data->related_lports);
> > > > sset_init(&data->active_tunnels);
> > > > - sset_init(&data->egress_ifaces);
> > > > + shash_init(&data->egress_ifaces);
> > > > + hmap_init(&data->qos_map);
> > > > smap_init(&data->local_iface_ids);
> > > > local_binding_data_init(&data->lbinding_data);
> > > > shash_init(&data->local_active_ports_ipv6_pd);
> > > > @@ -1392,7 +1394,8 @@ en_runtime_data_cleanup(void *data)
> > > > sset_destroy(&rt_data->local_lports);
> > > > related_lports_destroy(&rt_data->related_lports);
> > > > sset_destroy(&rt_data->active_tunnels);
> > > > - sset_destroy(&rt_data->egress_ifaces);
> > > > + shash_destroy(&rt_data->egress_ifaces);
> > > > + destroy_qos_map(&rt_data->qos_map);
> > > > smap_destroy(&rt_data->local_iface_ids);
> > > > local_datapaths_destroy(&rt_data->local_datapaths);
> > > > shash_destroy(&rt_data->local_active_ports_ipv6_pd);
> > > > @@ -1481,6 +1484,7 @@ init_binding_ctx(struct engine_node *node,
> > > > b_ctx_out->related_lports_changed = false;
> > > > b_ctx_out->non_vif_ports_changed = false;
> > > > b_ctx_out->egress_ifaces = &rt_data->egress_ifaces;
> > > > + b_ctx_out->qos_map = &rt_data->qos_map;
> > > > b_ctx_out->lbinding_data = &rt_data->lbinding_data;
> > > > b_ctx_out->local_iface_ids = &rt_data->local_iface_ids;
> > > > b_ctx_out->postponed_ports = rt_data->postponed_ports;
> > > > @@ -1510,13 +1514,14 @@ en_runtime_data_run(struct engine_node *node,
> > > > void *data)
> > > > sset_destroy(local_lports);
> > > > related_lports_destroy(&rt_data->related_lports);
> > > > sset_destroy(active_tunnels);
> > > > - sset_destroy(&rt_data->egress_ifaces);
> > > > + shash_clear(&rt_data->egress_ifaces);
> > > > + destroy_qos_map(&rt_data->qos_map);
> > > > + hmap_init(&rt_data->qos_map);
> > > > smap_destroy(&rt_data->local_iface_ids);
> > > > hmap_init(local_datapaths);
> > > > sset_init(local_lports);
> > > > related_lports_init(&rt_data->related_lports);
> > > > sset_init(active_tunnels);
> > > > - sset_init(&rt_data->egress_ifaces);
> > > > smap_init(&rt_data->local_iface_ids);
> > > > local_binding_data_init(&rt_data->lbinding_data);
> > > > }
> > > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > > > index 3e904c9dc..c986198a2 100644
> > > > --- a/tests/system-ovn.at
> > > > +++ b/tests/system-ovn.at
> > > > @@ -6335,6 +6335,10 @@ ADD_NAMESPACES(sw01)
> > > > ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24", "f0:00:00:01:02:03")
> > > > ovn-nbctl lsp-add sw0 sw01 \
> > > > -- lsp-set-addresses sw01 "f0:00:00:01:02:03 192.168.1.2"
> > > > +ADD_NAMESPACES(sw02)
> > > > +ADD_VETH(sw02, sw02, br-int, "192.168.1.3/24", "f2:00:00:01:02:03")
> > > > +ovn-nbctl lsp-add sw0 sw02 \
> > > > + -- lsp-set-addresses sw02 "f2:00:00:01:02:03 192.168.1.3"
> > > >
> > > > ADD_NAMESPACES(public)
> > > > ADD_VETH(public, public, br-ext, "192.168.2.2/24", "f0:00:00:01:02:05")
> > > > @@ -6345,6 +6349,7 @@ ovn-nbctl lsp-add sw0 public \
> > > > -- lsp-set-type public localnet \
> > > > -- lsp-set-options public network_name=phynet
> > > >
> > > > +# Setup QoS on a localnet port
> > > > AT_CHECK([ovn-nbctl set Logical_Switch_Port public
> > > > options:qos_min_rate=200000])
> > > > AT_CHECK([ovn-nbctl set Logical_Switch_Port public
> > > > options:qos_max_rate=300000])
> > > > AT_CHECK([ovn-nbctl set Logical_Switch_Port public
> > > > options:qos_burst=3000000])
> > > > @@ -6353,15 +6358,58 @@ OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1:
> > > > dev ovs-public'])
> > > > OVS_WAIT_UNTIL([tc class show dev ovs-public | \
> > > > grep -q 'class htb .* rate 200Kbit ceil 300Kbit burst
> > > > 375000b cburst 375000b'])
> > > >
> > > > +# Setup QoS on a logical switch ports
> > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01
> > > > options:qos_min_rate=400000])
> > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01
> > > > options:qos_max_rate=800000])
> > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01
> > > > options:qos_burst=5000000])
> > > > +AT_CHECK([ovs-vsctl set interface ovs-sw01
> > > > external-ids:ovn-egress-iface=true])
> > > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw01'])
> > > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw01 | \
> > > > + grep -q 'class htb .* rate 400Kbit ceil 800Kbit burst
> > > > 625000b cburst 625000b'])
> > > > +
> > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02
> > > > options:qos_min_rate=600000])
> > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02
> > > > options:qos_max_rate=6000000])
> > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02
> > > > options:qos_burst=6000000])
> > > > +AT_CHECK([ovs-vsctl set interface ovs-sw02
> > > > external-ids:ovn-egress-iface=true])
> > > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw02'])
> > > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw02 | \
> > > > + grep -q 'class htb .* rate 600Kbit ceil 6Mbit burst
> > > > 750000b cburst 750000b'])
> > > > +
> > > > +AT_CHECK([ovn-appctl -t ovn-controller exit --restart])
> > > > +OVS_WAIT_UNTIL([test "$(pidof ovn-controller)" = ""])
> > > > +start_daemon ovn-controller
> > > > +OVS_WAIT_UNTIL([test "$(pidof ovn-controller)" != ""])
> > > > +
> > > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-public'])
> > > > +OVS_WAIT_UNTIL([tc class show dev ovs-public | \
> > > > + grep -q 'class htb .* rate 200Kbit ceil 300Kbit burst
> > > > 375000b cburst 375000b'])
> > > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw01'])
> > > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw01 | \
> > > > + grep -q 'class htb .* rate 400Kbit ceil 800Kbit burst
> > > > 625000b cburst 625000b'])
> > > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw02'])
> > > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw02 | \
> > > > + grep -q 'class htb .* rate 600Kbit ceil 6Mbit burst
> > > > 750000b cburst 750000b'])
> > > >
> > > > AT_CHECK([ovn-nbctl remove Logical_Switch_Port public options
> > > > qos_max_rate=300000])
> > > > OVS_WAIT_UNTIL([tc class show dev ovs-public | \
> > > > grep -q 'class htb .* rate 200Kbit ceil 34359Mbit
> > > > burst 375000b .*'])
> > > >
> > > > AT_CHECK([ovn-nbctl remove Logical_Switch_Port public options
> > > > qos_min_rate=200000])
> > > > +AT_CHECK([ovn-nbctl remove Logical_Switch_Port public options
> > > > qos_max_rate=300000])
> > > > AT_CHECK([ovn-nbctl remove Logical_Switch_Port public options
> > > > qos_burst=3000000])
> > > > OVS_WAIT_UNTIL([test "$(tc qdisc show | grep 'htb 1: dev ovs-public')"
> > > > = ""])
> > > >
> > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01
> > > > options:qos_min_rate=200000])
> > > > +OVS_WAIT_UNTIL([tc qdisc show | grep -q 'htb 1: dev ovs-sw01'])
> > > > +OVS_WAIT_UNTIL([tc class show dev ovs-sw01 | \
> > > > + grep -q 'class htb .* rate 200Kbit ceil 800Kbit burst
> > > > 625000b cburst 625000b'])
> > > > +
> > > > +# Disable QoS rules from logical switch ports
> > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw01
> > > > options:qdisc_queue_id=0])
> > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port sw02
> > > > options:qdisc_queue_id=0])
> > > > +OVS_WAIT_UNTIL([test "$(tc qdisc show | grep 'htb 1: dev ovs-sw01')" =
> > > > ""])
> > > > +OVS_WAIT_UNTIL([test "$(tc qdisc show | grep 'htb 1: dev ovs-sw02')" =
> > > > ""])
> > > > +
> > > > kill $(pidof ovn-controller)
> > > >
> > > > as ovn-sb
> > > > --
> > > > 2.38.1
> > > >
> > > > _______________________________________________
> > > > dev mailing list
> > > > [email protected]
> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > >
> > >
> > _______________________________________________
> > dev mailing list
> > [email protected]
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
