northbound: KISS always batch yang config (file read), it's faster

The backoff code assumed that yang operations always completed quickly.
It checked for > 100 YANG modeled commands happening in under 1 second
to enable batching. If 100 yang modeled commands always take longer than
1 second batching is never enabled. This is the exact opposite of what
we want to happen since batching speeds the operations up.

Here are the results for libyang2 code without and with batching.

| action        |  1K rts |  2K rts | 1K rts | 2K rts | 20k rts |
|               | nobatch | nobatch |  batch |  batch |   batch |
| Add IPv4      |    .881 |    1.28 |   .703 |   1.04 |    8.16 |
| Add Same IPv4 |    28.7 |     113 |   .590 |   .860 |    6.09 |
| Rem 1/2 IPv4  |    .376 |    .442 |   .379 |   .435 |    1.44 |
| Add Same IPv4 |    28.7 |     113 |   .576 |   .841 |    6.02 |
| Rem All IPv4  |    17.4 |    71.8 |   .559 |   .813 |    5.57 |

(IPv6 numbers are basically the same as iPv4, a couple percent slower)

Clearly we need this. Please note the growth (1K to 2K) w/o batching is
non-linear and 100 times slower than batched.

Notes on code: The use of the new `nb_cli_apply_changes_clear_pending`
is to commit any pending changes (including the current one). This is
done when the code would not correctly handle a single diff that
included the current changes with possible following changes. For
example, a "no" command followed by a new value to replace it would be
merged into a change, and the code would not deal well with that. A good
example of this is BGP neighbor peer-group changing. The other use is
after entering a router level (e.g., "router bgp") where the follow-on
command handlers expect that router object to now exists. The code
eventually needs to be cleaned up to not fail in these cases, but that
is for future NB cleanup.

Signed-off-by: Christian Hopps <chopps@labn.net>

1 files changed, 53 insertions, 53 deletions

diff --git a/lib/northbound_cli.c b/lib/northbound_cli.c
index d291a1f24d..b74a0e6c23 100644
--- a/lib/northbound_cli.c
+++ b/lib/northbound_cli.c
@@ -74,7 +74,7 @@ static int nb_cli_classic_commit(struct vty *vty)
 	default:
 		vty_out(vty, "%% Configuration failed.\n\n");
 		vty_show_nb_errors(vty, ret, errmsg);
-		if (vty->t_pending_commit)
+		if (vty->pending_commit)
 			vty_out(vty,
 				"The following commands were dynamically grouped into the same transaction and rejected:\n%s",
 				vty->pending_cmds_buf);
@@ -89,49 +89,22 @@ static int nb_cli_classic_commit(struct vty *vty)
 
 static void nb_cli_pending_commit_clear(struct vty *vty)
 {
-	THREAD_OFF(vty->t_pending_commit);
-	vty->backoff_cmd_count = 0;
+	vty->pending_commit = 0;
 	XFREE(MTYPE_TMP, vty->pending_cmds_buf);
 	vty->pending_cmds_buflen = 0;
 	vty->pending_cmds_bufpos = 0;
 }
 
-static int nb_cli_pending_commit_cb(struct thread *thread)
+int nb_cli_pending_commit_check(struct vty *vty)
 {
-	struct vty *vty = THREAD_ARG(thread);
-
-	(void)nb_cli_classic_commit(vty);
-	nb_cli_pending_commit_clear(vty);
+	int ret = CMD_SUCCESS;
 
-	return 0;
-}
-
-void nb_cli_pending_commit_check(struct vty *vty)
-{
-	if (vty->t_pending_commit) {
-		(void)nb_cli_classic_commit(vty);
+	if (vty->pending_commit) {
+		ret = nb_cli_classic_commit(vty);
 		nb_cli_pending_commit_clear(vty);
 	}
-}
-
-static bool nb_cli_backoff_start(struct vty *vty)
-{
-	struct timeval now, delta;
-
-	/*
-	 * Start the configuration backoff timer only if 100 YANG-modeled
-	 * commands or more were entered within the last second.
-	 */
-	monotime(&now);
-	if (monotime_since(&vty->backoff_start, &delta) >= 1000000) {
-		vty->backoff_start = now;
-		vty->backoff_cmd_count = 1;
-		return false;
-	}
-	if (++vty->backoff_cmd_count < 100)
-		return false;
 
-	return true;
+	return ret;
 }
 
 static int nb_cli_schedule_command(struct vty *vty)
@@ -154,9 +127,7 @@ static int nb_cli_schedule_command(struct vty *vty)
 					   vty->pending_cmds_buflen);
 
 	/* Schedule the commit operation. */
-	THREAD_OFF(vty->t_pending_commit);
-	thread_add_timer_msec(master, nb_cli_pending_commit_cb, vty, 100,
-			      &vty->t_pending_commit);
+	vty->pending_commit = 1;
 
 	return CMD_SUCCESS;
 }
@@ -180,21 +151,16 @@ void nb_cli_enqueue_change(struct vty *vty, const char *xpath,
 	change->value = value;
 }
 
-int nb_cli_apply_changes(struct vty *vty, const char *xpath_base_fmt, ...)
+static int nb_cli_apply_changes_internal(struct vty *vty,
+					 const char *xpath_base,
+					 bool clear_pending)
 {
-	char xpath_base[XPATH_MAXLEN] = {};
 	bool error = false;
 
-	VTY_CHECK_XPATH;
-
-	/* Parse the base XPath format string. */
-	if (xpath_base_fmt) {
-		va_list ap;
+	if (xpath_base == NULL)
+		xpath_base = "";
 
-		va_start(ap, xpath_base_fmt);
-		vsnprintf(xpath_base, sizeof(xpath_base), xpath_base_fmt, ap);
-		va_end(ap);
-	}
+	VTY_CHECK_XPATH;
 
 	/* Edit candidate configuration. */
 	for (size_t i = 0; i < vty->num_cfg_changes; i++) {
@@ -207,10 +173,9 @@ int nb_cli_apply_changes(struct vty *vty, const char *xpath_base_fmt, ...)
 		/* Handle relative XPaths. */
 		memset(xpath, 0, sizeof(xpath));
 		if (vty->xpath_index > 0
-		    && ((xpath_base_fmt && xpath_base[0] == '.')
-			|| change->xpath[0] == '.'))
+		    && (xpath_base[0] == '.' || change->xpath[0] == '.'))
 			strlcpy(xpath, VTY_CURR_XPATH, sizeof(xpath));
-		if (xpath_base_fmt) {
+		if (xpath_base[0]) {
 			if (xpath_base[0] == '.')
 				strlcat(xpath, xpath_base + 1, sizeof(xpath));
 			else
@@ -267,7 +232,7 @@ int nb_cli_apply_changes(struct vty *vty, const char *xpath_base_fmt, ...)
 	}
 
 	/*
-	 * Do an implicit commit when using the classic CLI mode.
+	 * Maybe do an implicit commit when using the classic CLI mode.
 	 *
 	 * NOTE: the implicit commit might be scheduled to run later when
 	 * too many commands are being sent at the same time. This is a
@@ -276,14 +241,49 @@ int nb_cli_apply_changes(struct vty *vty, const char *xpath_base_fmt, ...)
 	 * faster.
 	 */
 	if (frr_get_cli_mode() == FRR_CLI_CLASSIC) {
-		if (vty->t_pending_commit || nb_cli_backoff_start(vty))
+		if (clear_pending) {
+			if (vty->pending_commit)
+				return nb_cli_pending_commit_check(vty);
+		} else if (vty->pending_allowed)
 			return nb_cli_schedule_command(vty);
+		assert(!vty->pending_commit);
 		return nb_cli_classic_commit(vty);
 	}
 
 	return CMD_SUCCESS;
 }
 
+int nb_cli_apply_changes(struct vty *vty, const char *xpath_base_fmt, ...)
+{
+	char xpath_base[XPATH_MAXLEN] = {};
+
+	/* Parse the base XPath format string. */
+	if (xpath_base_fmt) {
+		va_list ap;
+
+		va_start(ap, xpath_base_fmt);
+		vsnprintf(xpath_base, sizeof(xpath_base), xpath_base_fmt, ap);
+		va_end(ap);
+	}
+	return nb_cli_apply_changes_internal(vty, xpath_base, false);
+}
+
+int nb_cli_apply_changes_clear_pending(struct vty *vty,
+				       const char *xpath_base_fmt, ...)
+{
+	char xpath_base[XPATH_MAXLEN] = {};
+
+	/* Parse the base XPath format string. */
+	if (xpath_base_fmt) {
+		va_list ap;
+
+		va_start(ap, xpath_base_fmt);
+		vsnprintf(xpath_base, sizeof(xpath_base), xpath_base_fmt, ap);
+		va_end(ap);
+	}
+	return nb_cli_apply_changes_internal(vty, xpath_base, true);
+}
+
 int nb_cli_rpc(struct vty *vty, const char *xpath, struct list *input,
 	       struct list *output)
 {