Hi Mohammad,
Correct me if I'm wrong, but I don't think this code handles the I-P
recompute case properly. In ls_handle_lsp_changes(), you add a tracked
virtual port in the case where a logical switch port is added, and you
remove the tracked virtual port in the case where a logical switch port
is removed. However, in the case where a logical switch port changes and
results in a recompute, I think that should result in the destruction of
all tracked virtual ports. Then when the recompute happens,
build_ports() will reconstruct the hmap of tracked virtual ports.
Currently, if a recompute is needed, since the tracked virtual ports are
not cleared, it can result in duplicated entries in the hmap, or worse,
it could result in stale entries.
There are additional notes inline below.
On 8/1/24 06:44, Mohammad Heib wrote:
Northd handles virtual port binding requests received by ovn-controllers
without tracking those requests or saving any info about the last
binding requests and the number of requests received for an individual
virtual port.
This patch adds a basic tracking mechanism for each virtual port that
future patches will use to limit/pause the controller from sending
binding requests for a specific virtual port if this port overflows the
system by such requests.
Signed-off-by: Mohammad Heib <[email protected]>
---
northd/northd.c | 88 +++++++++++++++++++++++++++++++++++++++++++++
northd/northd.h | 2 ++
northd/ovn-northd.c | 3 ++
3 files changed, 93 insertions(+)
diff --git a/northd/northd.c b/northd/northd.c
index a8a0b6f94..89d5b2936 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3757,6 +3757,79 @@ build_lb_port_related_data(
build_lswitch_lbs_from_lrouter(lr_datapaths, lb_dps_map,
lb_group_dps_map);
}
+/*
+ * These functions implements the binding request tracking for a virtual
+ * port which can be used to limit virtual port binding requests
+ * and avoid system overflow.
+ *
+ * Virtual port binding requests must not exceed
+ * VPORT_MAX_BINDING_REQUEST_TRESHOLD within a VPORT_BINDING_TIMEFRAME,
+ * otherwise, this vport must be defined as overflowed and should limit
+ * the binding request in this port for a certain time.
+ */
+#define VPORT_BINDING_TIMEFRAME 10000
+#define VPORT_MAX_BINDING_REQUEST_TRESHOLD 15
+
+static struct hmap tracked_virtual_ports;
+
+struct tracked_virtual_port {
+ struct hmap_node node;
+ /*
+ * Use port name instaed of ovn_port refrence to make
s/instaed/instead/
s/refrence/reference/
+ * sure that virtual port tracking data will be permanent accross
s/accross/across/
+ * northd loops and we can keep track the target ports.
+ */
+ char *name;
+ long long int First_bind_in_tframe;
+ size_t Bind_request_cnt;
Please do not capitalize the names of struct fields.
+};
+
+static struct tracked_virtual_port *
+find_tracked_virtual_port(const char *name) {
+ struct tracked_virtual_port *vport;
+ HMAP_FOR_EACH (vport, node, &tracked_virtual_ports) {
The hmap uses the name as the hash function, so there is no reason to
traverse the entire hmap to find the port. Instead, hash the input
"name" parameter and use HMAP_FOR_EACH_WITH_HASH() so that less of the
hmap has to be traversed.
+ if (!strcmp(name, vport->name)) {
+ return vport;
+ }
+ }
+ return NULL;
+}
+
+static void
+add_to_tracked_virtual_ports(const char *name) {
+ struct tracked_virtual_port *vport = find_tracked_virtual_port(name);
+ if (!vport) {
+ vport = xmalloc(sizeof *vport);
+ vport->name = xstrdup(name);
+ vport->First_bind_in_tframe = 0;
+ vport->Bind_request_cnt = 0;
+ hmap_insert(&tracked_virtual_ports, &vport->node,
+ hash_string(name, 0));
+ }
+}
+
+static void
+remove_from_tracked_virtual_ports(const char *name) {
+ struct tracked_virtual_port *vport = find_tracked_virtual_port(name);
+ if (vport) {
+ free(vport->name);
+ hmap_remove(&tracked_virtual_ports, &vport->node);
+ free(vport);
+ }
+}
+
+void init_tracked_virtual_ports(void) {
+ hmap_init(&tracked_virtual_ports);
+}
+
+void destroy_tracked_virtual_ports(void) {
+ struct tracked_virtual_port *vport;
+ HMAP_FOR_EACH_SAFE (vport, node, &tracked_virtual_ports) {
+ remove_from_tracked_virtual_ports(vport->name);
This construct results in an O(n^2) complexity algorithm since the hmap
is traversed, and then remove_from_tracked_virtual_ports() traverses the
hmap again to find the vport before removing it.
One way to fix this would be to separate
remove_from_tracked_virtual_ports() into two steps:
step 1 (inline): Find the vport and remove it from the hmap
step 2: Create a function (e.g. tracked_vport_destroy()) that frees a
vport. Call that function here.
Then here in destroy_tracked_virtual_ports(), you can use
HMAP_FOR_EACH_POP() and call tracked_vport_destroy() on each vport directly.
+ }
+ hmap_destroy(&tracked_virtual_ports);
+}
+
/* Syncs the SB port binding for the ovn_port 'op' of a logical switch port.
* Caller should make sure that the OVN SB IDL txn is not NULL. Presently it
* only syncs the nat column of port binding corresponding to the 'op->nbsp'
*/
@@ -4163,6 +4236,9 @@ build_ports(struct ovsdb_idl_txn *ovnsb_txn,
op, queue_id_bitmap,
&active_ha_chassis_grps);
sbrec_port_binding_set_logical_port(op->sb, op->key);
+ if (!strcmp(op->sb->type, "virtual")) {
+ add_to_tracked_virtual_ports(op->sb->logical_port);
+ }
ovs_list_remove(&op->list);
}
@@ -4170,6 +4246,9 @@ build_ports(struct ovsdb_idl_txn *ovnsb_txn,
if (!ovs_list_is_empty(&sb_only)) {
LIST_FOR_EACH_SAFE (op, list, &sb_only) {
ovs_list_remove(&op->list);
+ if (!strcmp(op->sb->type, "virtual")) {
+ remove_from_tracked_virtual_ports(op->sb->logical_port);
+ }
sbrec_port_binding_delete(op->sb);
ovn_port_destroy(ports, op);
}
@@ -4554,6 +4633,12 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn
*ovnsb_idl_txn,
if (!op) {
goto fail;
}
+
+ if (!strcmp(new_nbsp->type, "virtual")) {
+ /* Add to virtual port tracking map */
+ add_to_tracked_virtual_ports(op->nbsp->name);
+ }
+
add_op_to_northd_tracked_ports(&trk_lsps->created, op);
} else if (ls_port_has_changed(new_nbsp)) {
/* Existing port updated */
@@ -4614,6 +4699,9 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn *ovnsb_idl_txn,
add_op_to_northd_tracked_ports(&trk_lsps->deleted, op);
hmap_remove(&nd->ls_ports, &op->key_node);
hmap_remove(&od->ports, &op->dp_node);
+ if (!strcmp(op->sb->type, "virtual")) {
+ remove_from_tracked_virtual_ports(op->sb->logical_port);
+ }
sbrec_port_binding_delete(op->sb);
delete_fdb_entry(ni->sbrec_fdb_by_dp_and_port, od->tunnel_key,
op->tunnel_key);
diff --git a/northd/northd.h b/northd/northd.h
index d4a8d75ab..5129f4afe 100644
--- a/northd/northd.h
+++ b/northd/northd.h
@@ -789,4 +789,6 @@ is_vxlan_mode(const struct smap *nb_options,
uint32_t get_ovn_max_dp_key_local(bool _vxlan_mode);
+void init_tracked_virtual_ports(void);
+void destroy_tracked_virtual_ports(void);
#endif /* NORTHD_H */
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index d71114f35..e58abdbcd 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -891,6 +891,8 @@ main(int argc, char *argv[])
/* Initialize incremental processing engine for ovn-northd */
inc_proc_northd_init(&ovnnb_idl_loop, &ovnsb_idl_loop);
+ init_tracked_virtual_ports();
+
unsigned int ovnnb_cond_seqno = UINT_MAX;
unsigned int ovnsb_cond_seqno = UINT_MAX;
@@ -1079,6 +1081,7 @@ main(int argc, char *argv[])
stopwatch_start(NORTHD_LOOP_STOPWATCH_NAME, time_msec());
}
inc_proc_northd_cleanup();
+ destroy_tracked_virtual_ports();
ovsdb_idl_loop_destroy(&ovnnb_idl_loop);
ovsdb_idl_loop_destroy(&ovnsb_idl_loop);
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
