On 12/9/22 04:18, Numan Siddique 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,
>
> 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.
I'm not sure how that is supposed to work, but adding qdiscs
to tunnel interfaces sounds wrong. It will affect all the
traffic between nodes, not only particular ports, right?
>
> 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.
>
> 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 ?
>
>
> 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.
>
> 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 ?
>
> @Dumitru Ceara @Han Zhou @Mark Michelson @Ilya Maximets Do you have
> any comments or suggestions here ?
I think, meters and what we call QoS in OVS/OVN world are a bit different
things. Meters only provide policing, while QoS provides egress traffic
shaping ('egress' from the point of view of OVS the switch). One can not
be a direct replacemnt of the other, but it depends on a use-case of course.
OVS also allows to configure ingress policing separately from meters or
egress shaping (QoS).
What I definitely agree with is that OVN should not call netdev_* API.
Instead, it should configure QoS table in the local OVS database and
let ovs-vswitchd to create and control qdiscs on actual interfaces.
Besides being a more robust solution, that will also allow using OVN QoS
with OVS userspace datapath.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
