From patchwork Wed Jul 26 13:43:55 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Xavier Simonart X-Patchwork-Id: 1813152 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.138; helo=smtp1.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: legolas.ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=SOcgoUlx; dkim-atps=neutral Received: from smtp1.osuosl.org (smtp1.osuosl.org [140.211.166.138]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4R9w9L6JL8z1yYc for ; Wed, 26 Jul 2023 23:44:06 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id 58F7781EC0; Wed, 26 Jul 2023 13:44:04 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org 58F7781EC0 Authentication-Results: smtp1.osuosl.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=SOcgoUlx X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp1.osuosl.org ([127.0.0.1]) by localhost (smtp1.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id mBuvgA0nSKi3; Wed, 26 Jul 2023 13:44:03 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp1.osuosl.org (Postfix) with ESMTPS id EF8DD81BA8; Wed, 26 Jul 2023 13:44:01 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org EF8DD81BA8 Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id BF461C0072; Wed, 26 Jul 2023 13:44:01 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp2.osuosl.org (smtp2.osuosl.org [140.211.166.133]) by lists.linuxfoundation.org (Postfix) with ESMTP id E0885C0032 for ; Wed, 26 Jul 2023 13:44:00 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp2.osuosl.org (Postfix) with ESMTP id B608F4037E for ; Wed, 26 Jul 2023 13:44:00 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org B608F4037E Authentication-Results: smtp2.osuosl.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=SOcgoUlx X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp2.osuosl.org ([127.0.0.1]) by localhost (smtp2.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id dBHeSKlB3ArH for ; Wed, 26 Jul 2023 13:43:59 +0000 (UTC) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by smtp2.osuosl.org (Postfix) with ESMTPS id 70C1140204 for ; Wed, 26 Jul 2023 13:43:59 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org 70C1140204 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1690379038; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=0gBsmWckNalTDBgaw9h1+JnOQDfF3xSu55uFaPcLFr8=; b=SOcgoUlxOOJh4uX9azB+KZZ0vqd/+jg7Q2SDxlyHxR7qKMB/YEXHe6q8DIM69GwRrtsnFJ MG143SHzddbTFR8JSnxuZj5Jt6BfBoK6qVp2j9L7RPi+XCh4FrFc4ie2WYzqD+E+T0zgR/ f/koLACtrLEzyBS9MhemekBVf2mSUdk= Received: from mimecast-mx02.redhat.com (66.187.233.73 [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-260-3IVCycU4NH-o7UJ0tDvusw-1; Wed, 26 Jul 2023 09:43:56 -0400 X-MC-Unique: 3IVCycU4NH-o7UJ0tDvusw-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 65B3829DD99D for ; Wed, 26 Jul 2023 13:43:56 +0000 (UTC) Received: from wsfd-netdev90.ntdv.lab.eng.bos.redhat.com (wsfd-netdev90.ntdv.lab.eng.bos.redhat.com [10.19.188.196]) by smtp.corp.redhat.com (Postfix) with ESMTP id 469E61121339; Wed, 26 Jul 2023 13:43:56 +0000 (UTC) From: Xavier Simonart To: xsimonar@redhat.com, dev@openvswitch.org Date: Wed, 26 Jul 2023 15:43:55 +0200 Message-Id: <20230726134355.3012154-1-xsimonar@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.3 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Subject: [ovs-dev] [PATCH ovn] QoS: Properly set qos when ovs db is read only X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" QoS was not configured in OVS db when db was read only: the configuration was just ignored and not done later when OVS db became writable. It was sometimes set later, if/when a recompute happened. This is now fixed: when OVS db is read only, the ports on which qos must be applied and stored and qos will be applied when OVS db becomes writable. To avoid race conditions between delayed qos and new qos changes (e.g. a qos configuration delayed in one loop as ovs is ro, followed in next loop, when ovs becomes rw, by another qos on the same port), all qos changes are done at the same time. This issue was identified by some random failures in system test "egress qos". Signed-off-by: Xavier Simonart --- controller/binding.c | 131 ++++++++++++++++++++++++------------ controller/binding.h | 8 +++ controller/ovn-controller.c | 7 ++ tests/ovn.at | 1 - tests/system-ovn.at | 22 ++++++ 5 files changed, 124 insertions(+), 45 deletions(-) diff --git a/controller/binding.c b/controller/binding.c index 9aa3fc6c4..0e13624c1 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -55,8 +55,13 @@ struct claimed_port { long long int last_claimed; }; +struct qos_port { + bool added; +}; + static struct shash _claimed_ports = SHASH_INITIALIZER(&_claimed_ports); static struct sset _postponed_ports = SSET_INITIALIZER(&_postponed_ports); +static struct shash _qos_ports = SHASH_INITIALIZER(&_qos_ports); static void remove_additional_chassis(const struct sbrec_port_binding *pb, @@ -218,6 +223,17 @@ get_qos_egress_port_interface(struct shash *bridge_mappings, return NULL; } +static void +add_or_del_qos_port(const char *ovn_port, bool add) +{ + struct qos_port *qos_port = shash_find_data(&_qos_ports, ovn_port); + if (!qos_port) { + qos_port = xzalloc(sizeof *qos_port); + shash_add(&_qos_ports, ovn_port, qos_port); + } + qos_port->added = add; +} + /* 34359738360 == (2^32 - 1) * 8. netdev_set_qos() doesn't support * 64-bit rate netlink attributes, so the maximum value is 2^32 - 1 * bytes. The 'max-rate' config option is in bits, so multiplying by 8. @@ -225,7 +241,7 @@ get_qos_egress_port_interface(struct shash *bridge_mappings, * can be unrecognized for certain NICs or reported too low for virtual * interfaces. */ #define OVN_QOS_MAX_RATE 34359738360ULL -static void +static bool add_ovs_qos_table_entry(struct ovsdb_idl_txn *ovs_idl_txn, const struct ovsrec_port *port, unsigned long long min_rate, @@ -239,7 +255,7 @@ add_ovs_qos_table_entry(struct ovsdb_idl_txn *ovs_idl_txn, const struct ovsrec_qos *qos = port->qos; if (qos && !smap_get_bool(&qos->external_ids, "ovn_qos", false)) { /* External configured QoS, do not overwrite it. */ - return; + return false; } if (!qos) { @@ -282,22 +298,18 @@ add_ovs_qos_table_entry(struct ovsdb_idl_txn *ovs_idl_txn, ovsrec_queue_verify_external_ids(queue); ovsrec_queue_set_external_ids(queue, &external_ids); smap_destroy(&external_ids); + return true; } static void -remove_stale_qos_entry(struct ovsdb_idl_txn *ovs_idl_txn, - const struct sbrec_port_binding *pb, +remove_stale_qos_entry( const char *logical_port, struct ovsdb_idl_index *ovsrec_port_by_qos, const struct ovsrec_qos_table *qos_table, struct hmap *queue_map) { - if (!ovs_idl_txn) { - return; - } - struct qos_queue *q = find_qos_queue( - queue_map, hash_string(pb->logical_port, 0), - pb->logical_port); + queue_map, hash_string(logical_port, 0), + logical_port); if (!q) { return; } @@ -338,8 +350,12 @@ remove_stale_qos_entry(struct ovsdb_idl_txn *ovs_idl_txn, static void configure_qos(const struct sbrec_port_binding *pb, - struct binding_ctx_in *b_ctx_in, - struct binding_ctx_out *b_ctx_out) + struct ovsdb_idl_txn *ovs_idl_txn, + struct ovsdb_idl_index *ovsrec_port_by_qos, + const struct ovsrec_qos_table *qos_table, + struct hmap *qos_map, + const struct ovsrec_open_vswitch_table *ovs_table, + const struct ovsrec_bridge_table *bridge_table) { unsigned long long min_rate = smap_get_ullong( &pb->options, "qos_min_rate", 0); @@ -351,20 +367,20 @@ configure_qos(const struct sbrec_port_binding *pb, if ((!min_rate && !max_rate && !burst) || !queue_id) { /* Qos is not configured for this port. */ - remove_stale_qos_entry(b_ctx_in->ovs_idl_txn, pb, - b_ctx_in->ovsrec_port_by_qos, - b_ctx_in->qos_table, b_ctx_out->qos_map); + remove_stale_qos_entry(pb->logical_port, + ovsrec_port_by_qos, + qos_table, qos_map); return; } const char *network = smap_get(&pb->options, "qos_physical_network"); uint32_t hash = hash_string(pb->logical_port, 0); - struct qos_queue *q = find_qos_queue(b_ctx_out->qos_map, hash, + struct qos_queue *q = find_qos_queue(qos_map, hash, pb->logical_port); if (!q || q->min_rate != min_rate || q->max_rate != max_rate || q->burst != burst || (network && strcmp(network, q->network))) { struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings); - add_ovs_bridge_mappings(b_ctx_in->ovs_table, b_ctx_in->bridge_table, + add_ovs_bridge_mappings(ovs_table, bridge_table, &bridge_mappings); const struct ovsrec_port *port = NULL; @@ -375,25 +391,58 @@ configure_qos(const struct sbrec_port_binding *pb, } if (iface) { /* Add new QoS entries. */ - add_ovs_qos_table_entry(b_ctx_in->ovs_idl_txn, port, min_rate, + if (add_ovs_qos_table_entry(ovs_idl_txn, port, min_rate, max_rate, burst, queue_id, - pb->logical_port); - if (!q) { - q = xzalloc(sizeof *q); - hmap_insert(b_ctx_out->qos_map, &q->node, hash); - q->port = xstrdup(pb->logical_port); - q->queue_id = queue_id; + pb->logical_port)) { + if (!q) { + q = xzalloc(sizeof *q); + hmap_insert(qos_map, &q->node, hash); + q->port = xstrdup(pb->logical_port); + q->queue_id = queue_id; + } + free(q->network); + q->network = network ? xstrdup(network) : NULL; + q->min_rate = min_rate; + q->max_rate = max_rate; + q->burst = burst; } - free(q->network); - q->network = network ? xstrdup(network) : NULL; - q->min_rate = min_rate; - q->max_rate = max_rate; - q->burst = burst; } shash_destroy(&bridge_mappings); } } +void +update_qos(struct ovsdb_idl_index * sbrec_port_binding_by_name, + struct ovsdb_idl_txn *ovs_idl_txn, + struct ovsdb_idl_index *ovsrec_port_by_qos, + const struct ovsrec_qos_table *qos_table, + struct hmap *qos_map, + const struct ovsrec_open_vswitch_table *ovs_table, + const struct ovsrec_bridge_table *bridge_table) +{ + /* Remove qos for any ports for which we could not do it before */ + const struct sbrec_port_binding *pb; + + struct shash_node *node; + SHASH_FOR_EACH_SAFE (node, &_qos_ports) { + struct qos_port *qos_port = (struct qos_port *) node->data; + if (qos_port->added) { + pb = lport_lookup_by_name(sbrec_port_binding_by_name, + node->name); + if (pb) { + configure_qos(pb, ovs_idl_txn, ovsrec_port_by_qos, qos_table, + qos_map, ovs_table, bridge_table); + } + } else { + remove_stale_qos_entry(node->name, + ovsrec_port_by_qos, + qos_table, qos_map); + } + free(qos_port); + shash_delete(&_qos_ports, node); + } +} + static const struct ovsrec_queue * find_qos_queue_by_external_ids(const struct smap *external_ids, struct ovsdb_idl_index *ovsrec_queue_by_external_ids) @@ -1599,8 +1648,8 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, tracked_datapath_lport_add(pb, TRACKED_RESOURCE_UPDATED, b_ctx_out->tracked_dp_bindings); } - if (b_lport->lbinding->iface && b_ctx_in->ovs_idl_txn) { - configure_qos(pb, b_ctx_in, b_ctx_out); + if (b_lport->lbinding->iface) { + add_or_del_qos_port(pb->logical_port, true); } } else { /* We could, but can't claim the lport. */ @@ -1926,17 +1975,13 @@ consider_l3gw_lport(const struct sbrec_port_binding *pb, static void consider_localnet_lport(const struct sbrec_port_binding *pb, - struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) { /* Add all localnet ports to local_ifaces so that we allocate ct zones * for them. */ update_local_lports(pb->logical_port, b_ctx_out); - if (b_ctx_in->ovs_idl_txn) { - configure_qos(pb, b_ctx_in, b_ctx_out); - } - + add_or_del_qos_port(pb->logical_port, true); update_related_lport(pb, b_ctx_out); } @@ -2057,6 +2102,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) return; } + shash_clear_free_data(&_qos_ports); struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings); if (b_ctx_in->br_int) { @@ -2143,7 +2189,7 @@ 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); + consider_localnet_lport(pb, b_ctx_out); struct lport *lnet_lport = xmalloc(sizeof *lnet_lport); lnet_lport->pb = pb; ovs_list_push_back(&localnet_lports, &lnet_lport->list_node); @@ -2432,9 +2478,7 @@ consider_iface_release(const struct ovsrec_interface *iface_rec, b_ctx_out, ld); } - remove_stale_qos_entry(b_ctx_in->ovs_idl_txn, b_lport->pb, - b_ctx_in->ovsrec_port_by_qos, - b_ctx_in->qos_table, b_ctx_out->qos_map); + add_or_del_qos_port(b_lport->pb->logical_port, false); /* Release the primary binding lport and other children lports if * any. */ @@ -2992,7 +3036,7 @@ handle_updated_port(struct binding_ctx_in *b_ctx_in, break; case LP_LOCALNET: { - consider_localnet_lport(pb, b_ctx_in, b_ctx_out); + consider_localnet_lport(pb, b_ctx_out); struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings); @@ -3080,9 +3124,7 @@ binding_handle_port_binding_changes(struct binding_ctx_in *b_ctx_in, shash_add(&deleted_other_pbs, pb->logical_port, pb); } - remove_stale_qos_entry(b_ctx_in->ovs_idl_txn, pb, - b_ctx_in->ovsrec_port_by_qos, - b_ctx_in->qos_table, b_ctx_out->qos_map); + add_or_del_qos_port(pb->logical_port, false); } struct shash_node *node; @@ -3667,5 +3709,6 @@ void binding_destroy(void) { shash_destroy_free_data(&_claimed_ports); + shash_destroy_free_data(&_qos_ports); sset_clear(&_postponed_ports); } diff --git a/controller/binding.h b/controller/binding.h index 235e5860d..59a4654fb 100644 --- a/controller/binding.h +++ b/controller/binding.h @@ -268,4 +268,12 @@ void binding_destroy(void); void destroy_qos_map(struct hmap *); +void update_qos(struct ovsdb_idl_index * sbrec_port_binding_by_name, + struct ovsdb_idl_txn *ovs_idl_txn, + struct ovsdb_idl_index *ovsrec_port_by_qos, + const struct ovsrec_qos_table *qos_table, + struct hmap *qos_map, + const struct ovsrec_open_vswitch_table *ovs_table, + const struct ovsrec_bridge_table *bridge_table); + #endif /* controller/binding.h */ diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 236974f4f..21e03273c 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -5434,6 +5434,13 @@ main(int argc, char *argv[]) ovsrec_interface_table_get( ovs_idl_loop.idl), !ovnsb_idl_txn, !ovs_idl_txn); + if (runtime_data && ovs_idl_txn) { + update_qos(sbrec_port_binding_by_name, ovs_idl_txn, + ovsrec_port_by_qos, + ovsrec_qos_table_get(ovs_idl_loop.idl), + &runtime_data->qos_map, + ovs_table, bridge_table); + } stopwatch_stop(IF_STATUS_MGR_RUN_STOPWATCH_NAME, time_msec()); } diff --git a/tests/ovn.at b/tests/ovn.at index 24da9174e..c8dd0978b 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -36596,7 +36596,6 @@ check ovn-nbctl lsp-add ls2 public2 check ovn-nbctl lsp-set-addresses public2 unknown check ovn-nbctl lsp-set-type public2 localnet check ovn-nbctl --wait=sb set Logical_Switch_Port public2 options:qos_min_rate=6000000000 options:qos_max_rate=7000000000 options:qos_burst=8000000000 options:network_name=phys -check ovn-nbctl --wait=sb lsp-set-options public2 qos_min_rate=6000000000 qos_max_rate=7000000000 qos_burst=8000000000 # Let's now send ovn controller to sleep, so it will receive both ofport notification and ls deletion simultaneously sleep_controller hv-1 diff --git a/tests/system-ovn.at b/tests/system-ovn.at index fef3cfbf0..8533149f8 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -6630,6 +6630,28 @@ AT_CHECK([ovn-nbctl set Logical_Switch_Port ext options:qos_min_rate=400000]) AT_CHECK([ovn-nbctl set Logical_Switch_Port ext options:qos_max_rate=600000]) AT_CHECK([ovn-nbctl set Logical_Switch_Port ext options:qos_burst=6000000]) +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-ext']) +OVS_WAIT_UNTIL([tc class show dev ovs-ext | \ + grep -q 'class htb .* rate 400Kbit ceil 600Kbit burst 750000b cburst 750000b']) + +# The same now with ovs db read only +# +AT_CHECK([ovn-nbctl remove Logical_Switch_Port ext options qos_min_rate=400000]) +AT_CHECK([ovn-nbctl remove Logical_Switch_Port ext options qos_max_rate=600000]) +AT_CHECK([ovn-nbctl remove Logical_Switch_Port ext options qos_burst=6000000]) +OVS_WAIT_UNTIL([test "$(tc class show dev ovs-ext | grep 'class htb')" == ""]) + +sleep_ovsdb . + +AT_CHECK([ovn-nbctl set Logical_Switch_Port ext options:qos_min_rate=400000]) +AT_CHECK([ovn-nbctl set Logical_Switch_Port ext options:qos_max_rate=600000]) +AT_CHECK([ovn-nbctl set Logical_Switch_Port ext options:qos_burst=6000000]) +wake_up_ovsdb . + 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'])