vlib: increase the stats epoch only when necessary

When the counter vectors are validated and they are already long enough
to fit the given index in memory, there is no need to increase the stats
segment epoch. In this case, the counter vectors do not change as a
result of the validation.

This optimization is necessary for the case when the configuration is
changed at multiple thousands per second rate. The counter vectors grow
at the beginning and their size stabilizes after a while. Without this
improvement, it can still take several seconds for a stats reader to
succeed.

Type: improvement
Signed-off-by: Miklos Tirpak <miklos.tirpak@gmail.com>
Change-Id: I5a6c30255832716a1460018d0bd0f63031de102b

diff --git a/src/plugins/unittest/CMakeLists.txt b/src/plugins/unittest/CMakeLists.txt
index 2b358a5..64a7706 100644 (file)
--- a/src/plugins/unittest/CMakeLists.txt
+++ b/src/plugins/unittest/CMakeLists.txt
@@ -50,4 +50,5 @@ add_vpp_plugin(unittest
   unittest.c
   util_test.c
   vlib_test.c
+  counter_test.c
 )

diff --git a/src/plugins/unittest/counter_test.c b/src/plugins/unittest/counter_test.c
new file mode 100644 (file)
index 0000000..24b9e1e
--- /dev/null
+++ b/src/plugins/unittest/counter_test.c
@@ -0,0 +1,262 @@
+/*
+ * Copyright (c) 2021 EMnify.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#include <vlib/vlib.h>
+#include <vppinfra/time.h>
+#include <vppinfra/cache.h>
+#include <vppinfra/error.h>
+
+#include <vlib/counter.h>
+#include <vpp/stats/stat_segment.h>
+
+enum
+{
+  type_simple = 0,
+  type_combined,
+};
+
+enum
+{
+  test_expand = 0,
+};
+
+/*
+ * Return the stats segment epoch value.
+ */
+static uint64_t
+get_stats_epoch ()
+{
+  stat_segment_main_t *sm = &stat_segment_main;
+  return sm->shared_header->epoch;
+}
+
+/*
+ * Return the maximum element count of the vector based on its allocated
+ * memory.
+ */
+static int
+get_vec_mem_size (void *v, uword data_size)
+{
+  stat_segment_main_t *sm = &stat_segment_main;
+
+  if (v == 0)
+    return 0;
+
+  uword aligned_header_bytes = vec_header_bytes (0);
+  void *p = v - aligned_header_bytes;
+  void *oldheap = clib_mem_set_heap (sm->heap);
+  int mem_size = (clib_mem_size (p) - aligned_header_bytes) / data_size;
+  clib_mem_set_heap (oldheap);
+
+  return mem_size;
+}
+
+/* number of times to repeat the counter expand tests */
+#define EXPAND_TEST_ROUNDS 3
+
+/*
+ * Let a simple counter vector grow and verify that
+ * the stats epoch is increased only when the vector
+ * is expanded.
+ */
+static clib_error_t *
+test_simple_counter_expand (vlib_main_t *vm)
+{
+  vlib_simple_counter_main_t counter = {
+    .name = "test-simple-counter-expand",
+    .stat_segment_name = "/vlib/test-simple-counter-expand",
+  };
+  int i, index;
+  uint64_t epoch, new_epoch;
+
+  // Create one counter to allocate the vector.
+  vlib_validate_simple_counter (&counter, 0);
+  epoch = get_stats_epoch ();
+
+  for (i = 0; i < EXPAND_TEST_ROUNDS; i++)
+    {
+      // Check how many elements fit into the counter vector without expanding
+      // that. The next validate calls should not increase the stats segment
+      // epoch.
+      int mem_size = get_vec_mem_size (counter.counters[0],
+                                      sizeof ((counter.counters[0])[0]));
+      for (index = 1; index <= mem_size - 1; index++)
+       {
+         vlib_validate_simple_counter (&counter, index);
+         new_epoch = get_stats_epoch ();
+         if (new_epoch != epoch)
+           return clib_error_return (
+             0, "Stats segment epoch should not increase");
+       }
+
+      // The next counter index does not fit and it will extend the vector.
+      // The stats segment epoch should increase.
+      vlib_validate_simple_counter (&counter, index + 1);
+      new_epoch = get_stats_epoch ();
+      if (new_epoch == epoch)
+       return clib_error_return (0,
+                                 "Stats segment epoch should have increased");
+      epoch = new_epoch;
+    }
+
+  return 0;
+}
+
+/*
+ * Let a combined counter vector grow and verify that
+ * the stats epoch is increased only when the vector
+ * is expanded.
+ */
+static clib_error_t *
+test_combined_counter_expand (vlib_main_t *vm)
+{
+  vlib_combined_counter_main_t counter = {
+    .name = "test-combined-counter-expand",
+    .stat_segment_name = "/vlib/test-combined-counter-expand",
+  };
+  int i, index;
+  uint64_t epoch, new_epoch;
+
+  // Create one counter to allocate the vector.
+  vlib_validate_combined_counter (&counter, 0);
+  epoch = get_stats_epoch ();
+
+  for (i = 0; i < EXPAND_TEST_ROUNDS; i++)
+    {
+      // Check how many elements fit into the counter vector without expanding
+      // that. The next validate calls should not increase the stats segment
+      // epoch.
+      int mem_size = get_vec_mem_size (counter.counters[0],
+                                      sizeof ((counter.counters[0])[0]));
+      for (index = 1; index <= mem_size - 1; index++)
+       {
+         vlib_validate_combined_counter (&counter, index);
+         new_epoch = get_stats_epoch ();
+         if (new_epoch != epoch)
+           return clib_error_return (
+             0, "Stats segment epoch should not increase");
+       }
+
+      // The next counter index does not fit and it will extend the vector.
+      // The stats segment epoch should increase.
+      vlib_validate_combined_counter (&counter, index + 1);
+      new_epoch = get_stats_epoch ();
+      if (new_epoch == epoch)
+       return clib_error_return (0,
+                                 "Stats segment epoch should have increased");
+      epoch = new_epoch;
+    }
+
+  return 0;
+}
+
+static clib_error_t *
+test_simple_counter (vlib_main_t *vm, int test_case)
+{
+  clib_error_t *error;
+
+  switch (test_case)
+    {
+    case test_expand:
+      error = test_simple_counter_expand (vm);
+      break;
+
+    default:
+      return clib_error_return (0, "no such test");
+    }
+
+  return error;
+}
+
+static clib_error_t *
+test_combined_counter (vlib_main_t *vm, int test_case)
+{
+  clib_error_t *error;
+
+  switch (test_case)
+    {
+    case test_expand:
+      error = test_combined_counter_expand (vm);
+      break;
+
+    default:
+      return clib_error_return (0, "no such test");
+    }
+
+  return error;
+}
+
+static clib_error_t *
+test_counter_command_fn (vlib_main_t *vm, unformat_input_t *input,
+                        vlib_cli_command_t *cmd)
+{
+  clib_error_t *error;
+  int counter_type = -1;
+  int test_case = -1;
+
+  while (unformat_check_input (input) != UNFORMAT_END_OF_INPUT)
+    {
+      if (unformat (input, "simple"))
+       counter_type = type_simple;
+      else if (unformat (input, "combined"))
+       counter_type = type_combined;
+      else if (unformat (input, "expand"))
+       test_case = test_expand;
+      else
+       return clib_error_return (0, "unknown input '%U'",
+                                 format_unformat_error, input);
+    }
+
+  if (test_case == -1)
+    return clib_error_return (0, "no such test");
+
+  switch (counter_type)
+    {
+    case type_simple:
+      error = test_simple_counter (vm, test_case);
+      break;
+
+    case type_combined:
+      error = test_combined_counter (vm, test_case);
+      break;
+
+    default:
+      return clib_error_return (0, "no such test");
+    }
+
+  return error;
+}
+
+VLIB_CLI_COMMAND (test_counter_command, static) = {
+  .path = "test counter",
+  .short_help = "test counter [simple | combined] expand",
+  .function = test_counter_command_fn,
+};
+
+static clib_error_t *
+test_counter_init (vlib_main_t *vm)
+{
+  return (0);
+}
+
+VLIB_INIT_FUNCTION (test_counter_init);
+
+/*
+ * fd.io coding-style-patch-verification: ON
+ *
+ * Local Variables:
+ * eval: (c-set-style "gnu")
+ * End:
+ */

diff --git a/src/vlib/counter.c b/src/vlib/counter.c
index 81c81ae..186b48d 100644 (file)
--- a/src/vlib/counter.c
+++ b/src/vlib/counter.c
@@ -79,15 +79,26 @@ void
 vlib_validate_simple_counter (vlib_simple_counter_main_t * cm, u32 index)
 {
   vlib_thread_main_t *tm = vlib_get_thread_main ();
-  int i;
+  int i, resized = 0;
   void *oldheap = vlib_stats_push_heap (cm->counters);
 
   vec_validate (cm->counters, tm->n_vlib_mains - 1);
   for (i = 0; i < tm->n_vlib_mains; i++)
-    vec_validate_aligned (cm->counters[i], index, CLIB_CACHE_LINE_BYTES);
-
-  vlib_stats_pop_heap (cm, oldheap, index,
-                      2 /* STAT_DIR_TYPE_COUNTER_VECTOR_SIMPLE */ );
+    if (index >= vec_len (cm->counters[i]))
+      {
+       if (vec_resize_will_expand (cm->counters[i],
+                                   index - vec_len (cm->counters[i]) +
+                                     1 /* length_increment */))
+         resized++;
+       vec_validate_aligned (cm->counters[i], index, CLIB_CACHE_LINE_BYTES);
+      }
+
+  /* Avoid the epoch increase when there was no counter vector resize. */
+  if (resized)
+    vlib_stats_pop_heap (cm, oldheap, index,
+                        2 /* STAT_DIR_TYPE_COUNTER_VECTOR_SIMPLE */);
+  else
+    clib_mem_set_heap (oldheap);
 }
 
 void
@@ -108,15 +119,26 @@ void
 vlib_validate_combined_counter (vlib_combined_counter_main_t * cm, u32 index)
 {
   vlib_thread_main_t *tm = vlib_get_thread_main ();
-  int i;
+  int i, resized = 0;
   void *oldheap = vlib_stats_push_heap (cm->counters);
 
   vec_validate (cm->counters, tm->n_vlib_mains - 1);
   for (i = 0; i < tm->n_vlib_mains; i++)
-    vec_validate_aligned (cm->counters[i], index, CLIB_CACHE_LINE_BYTES);
-
-  vlib_stats_pop_heap (cm, oldheap, index,
-                      3 /*STAT_DIR_TYPE_COUNTER_VECTOR_COMBINED */ );
+    if (index >= vec_len (cm->counters[i]))
+      {
+       if (vec_resize_will_expand (cm->counters[i],
+                                   index - vec_len (cm->counters[i]) +
+                                     1 /* length_increment */))
+         resized++;
+       vec_validate_aligned (cm->counters[i], index, CLIB_CACHE_LINE_BYTES);
+      }
+
+  /* Avoid the epoch increase when there was no counter vector resize. */
+  if (resized)
+    vlib_stats_pop_heap (cm, oldheap, index,
+                        3 /*STAT_DIR_TYPE_COUNTER_VECTOR_COMBINED */);
+  else
+    clib_mem_set_heap (oldheap);
 }
 
 int
@@ -130,8 +152,7 @@ int
   /* Possibly once in recorded history */
   if (PREDICT_FALSE (vec_len (cm->counters) == 0))
     {
-      vlib_stats_pop_heap (cm, oldheap, index,
-                          3 /*STAT_DIR_TYPE_COUNTER_VECTOR_COMBINED */ );
+      clib_mem_set_heap (oldheap);
       return 1;
     }
 
@@ -144,13 +165,11 @@ int
                                  index - vec_len (cm->counters[i]) +
                                    1 /* length_increment */))
        {
-         vlib_stats_pop_heap (cm, oldheap, index,
-                              3 /*STAT_DIR_TYPE_COUNTER_VECTOR_COMBINED */ );
+         clib_mem_set_heap (oldheap);
          return 1;
        }
     }
-  vlib_stats_pop_heap (cm, oldheap, index,
-                      3 /*STAT_DIR_TYPE_COUNTER_VECTOR_COMBINED */ );
+  clib_mem_set_heap (oldheap);
   return 0;
 }
 

diff --git a/src/vlib/test/test_counters.py b/src/vlib/test/test_counters.py
new file mode 100644 (file)
index 0000000..5c63308
--- /dev/null
+++ b/src/vlib/test/test_counters.py
@@ -0,0 +1,37 @@
+#!/usr/bin/env python3
+
+from framework import VppTestCase
+
+
+class TestCounters(VppTestCase):
+    """ Counters C Unit Tests """
+
+    @classmethod
+    def setUpClass(cls):
+        super(TestCounters, cls).setUpClass()
+
+    @classmethod
+    def tearDownClass(cls):
+        super(TestCounters, cls).tearDownClass()
+
+    def setUp(self):
+        super(TestCounters, self).setUp()
+
+    def tearDown(self):
+        super(TestCounters, self).tearDown()
+
+    def test_counter_simple_expand(self):
+        """ Simple Counter Expand """
+        error = self.vapi.cli("test counter simple expand")
+
+        if error:
+            self.logger.critical(error)
+            self.assertNotIn('failed', error)
+
+    def test_counter_combined_expand(self):
+        """ Combined Counter Expand """
+        error = self.vapi.cli("test counter combined expand")
+
+        if error:
+            self.logger.critical(error)
+            self.assertNotIn('failed', error)