On Fri, Dec 9, 2022 at 8:37 AM Numan Siddique <[email protected]> wrote: > > 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. > > > > > > > > > > > > > > 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 also agree with Ilya that OVN should configure OVS instead of netdev directly, but I am not familiar with the OVS native QoS support and not sure if there is a feature gap. The bugzilla was reported against VF representor port, but I think OVN should follow the same implementation if possible (i.e. configure OVS conf), and let the OVS offloading logic to figure out what to do. (In case there is any VF specific information that needs to be supplied for the VF representor to work, the VIF-plug mechanism may be utilized, which doesn't require any change in OVN code).
I am also not familiar with how the current LSP qos options are implemented in OVN. I traced back to the below patch that added the qdisc related logic in 2016: a6095f815ed9 Check and allocate free qdisc queue id for ports with qos parameters And recently the below patch improved it by adding qos_min_rate support. dbf12e5fe1f7 qos: add support for port minimum bandwidth guarantee Thanks, Han _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
