stats: fix state counter removal

Avoid using vec_del1() for directory vector to keep indexes valid all
the time.

There are state counters for each slave in LACP bond mode which can be
dynamically created and removed. Vector index is used to access these
counters. But also vec_del1() is used to remove counter from vector.
This function changes the index of the last element, so after this we
are unable to access ex-last element using old index.

As a result it is not possible to add-del-add two interfaces to the LACP
bond:

DBGvpp# create bond mode lacp
BondEthernet0
DBGvpp# create packet-generator interface pg1
DBGvpp# create packet-generator interface pg2
DBGvpp# bond add BondEthernet0 pg1
DBGvpp# bond add BondEthernet0 pg2
DBGvpp# bond del pg1
DBGvpp# bond del pg2
DBGvpp# bond add BondEthernet0 pg1
DBGvpp# bond add BondEthernet0 pg2
bond add: /if/lacp/1/3/partner-state is already register

Type: fix

Signed-off-by: Vladimir Isaev <visaev@netgate.com>
Change-Id: I2c86e13905eefdef6233369cd4ab5c1b53d123bd

diff --git a/extras/vom/vom/stat_client.cpp b/extras/vom/vom/stat_client.cpp
index 394c6ee..00751dd 100644 (file)
--- a/extras/vom/vom/stat_client.cpp
+++ b/extras/vom/vom/stat_client.cpp
@@ -38,6 +38,8 @@ stat_client::stat_data_t::stat_data_t(const stat_segment_data_t& stat_seg_data)
       break;
     case STAT_DIR_TYPE_ILLEGAL:
       break;
+    case STAT_DIR_TYPE_EMPTY:
+      break;
   }
 }
 
@@ -95,8 +97,7 @@ stat_client::stat_client(std::vector<std::string>& pattern)
   , m_counter_vec()
   , m_stat_seg_data(nullptr)
   , m_stat_data()
-{
-}
+{}
 
 stat_client::stat_client(std::string socket_name,
                          std::vector<std::string> patterns)
@@ -106,8 +107,7 @@ stat_client::stat_client(std::string socket_name,
   , m_counter_vec()
   , m_stat_seg_data(nullptr)
   , m_stat_data()
-{
-}
+{}
 
 stat_client::stat_client()
   : m_socket_name("/run/vpp/stats.sock")
@@ -131,8 +131,7 @@ stat_client::~stat_client()
 stat_client::stat_client(const stat_client& o)
   : m_socket_name(o.m_socket_name)
   , m_patterns(o.m_patterns)
-{
-}
+{}
 
 int
 stat_client::connect()

diff --git a/extras/vom/vom/stat_reader.cpp b/extras/vom/vom/stat_reader.cpp
index 0edbc70..50a25d2 100644 (file)
--- a/extras/vom/vom/stat_reader.cpp
+++ b/extras/vom/vom/stat_reader.cpp
@@ -22,17 +22,13 @@ stat_reader::stat_indexes_t stat_reader::m_stat_itf_indexes;
 
 stat_reader::stat_reader()
   : m_client()
-{
-}
+{}
 
 stat_reader::stat_reader(stat_client sc)
   : m_client(sc)
-{
-}
+{}
 
-stat_reader::~stat_reader()
-{
-}
+stat_reader::~stat_reader() {}
 
 int
 stat_reader::connect()
@@ -80,6 +76,7 @@ stat_reader::read()
       case STAT_DIR_TYPE_SCALAR_INDEX:
       case STAT_DIR_TYPE_NAME_VECTOR:
       case STAT_DIR_TYPE_ILLEGAL:
+      case STAT_DIR_TYPE_EMPTY:
         break;
 
       case STAT_DIR_TYPE_COUNTER_VECTOR_SIMPLE: {

diff --git a/src/vpp-api/client/stat_client.c b/src/vpp-api/client/stat_client.c
index 9a7a0e5..9c6ff33 100644 (file)
--- a/src/vpp-api/client/stat_client.c
+++ b/src/vpp-api/client/stat_client.c
@@ -274,6 +274,9 @@ copy_data (stat_segment_directory_entry_t * ep, stat_client_main_t * sm)
        }
       break;
 
+    case STAT_DIR_TYPE_EMPTY:
+      break;
+
     default:
       fprintf (stderr, "Unknown type: %d\n", ep->type);
     }

diff --git a/src/vpp/app/vpp_get_stats.c b/src/vpp/app/vpp_get_stats.c
index c00fb83..d13e4d9 100644 (file)
--- a/src/vpp/app/vpp_get_stats.c
+++ b/src/vpp/app/vpp_get_stats.c
@@ -89,6 +89,9 @@ stat_poll_loop (u8 ** patterns)
              fformat (stdout, "%.2f %s\n", res[i].scalar_value, res[i].name);
              break;
 
+           case STAT_DIR_TYPE_EMPTY:
+             break;
+
            default:
              printf ("Unknown value\n");
              ;
@@ -233,6 +236,9 @@ reconnect:
                           res[i].name);
              break;
 
+           case STAT_DIR_TYPE_EMPTY:
+             break;
+
            default:
              ;
            }

diff --git a/src/vpp/app/vpp_prometheus_export.c b/src/vpp/app/vpp_prometheus_export.c
index 06f1a91..9994491 100644 (file)
--- a/src/vpp/app/vpp_prometheus_export.c
+++ b/src/vpp/app/vpp_prometheus_export.c
@@ -112,6 +112,9 @@ retry:
                   res[i].scalar_value);
          break;
 
+       case STAT_DIR_TYPE_EMPTY:
+         break;
+
        default:
          fformat (stderr, "Unknown value %d\n", res[i].type);
          ;

diff --git a/src/vpp/stats/stat_segment.c b/src/vpp/stats/stat_segment.c
index f4758a7..b96e667 100644 (file)
--- a/src/vpp/stats/stat_segment.c
+++ b/src/vpp/stats/stat_segment.c
@@ -60,24 +60,17 @@ vlib_stats_push_heap (void *old)
 }
 
 static u32
-lookup_or_create_hash_index (u8 * name, u32 next_vector_index)
+lookup_hash_index (u8 * name)
 {
   stat_segment_main_t *sm = &stat_segment_main;
-  u32 index;
+  u32 index = STAT_SEGMENT_INDEX_INVALID;
   hash_pair_t *hp;
 
   /* Must be called in the context of the main heap */
   ASSERT (clib_mem_get_heap () != sm->heap);
 
   hp = hash_get_pair (sm->directory_vector_by_name, name);
-  if (!hp)
-    {
-      /* we allocate our private copy of 'name' */
-      hash_set (sm->directory_vector_by_name, format (0, "%s%c", name, 0),
-               next_vector_index);
-      index = next_vector_index;
-    }
-  else
+  if (hp)
     {
       index = hp->value[0];
     }
@@ -85,6 +78,76 @@ lookup_or_create_hash_index (u8 * name, u32 next_vector_index)
   return index;
 }
 
+static void
+create_hash_index (u8 * name, u32 index)
+{
+  stat_segment_main_t *sm = &stat_segment_main;
+
+  /* Must be called in the context of the main heap */
+  ASSERT (clib_mem_get_heap () != sm->heap);
+
+  hash_set (sm->directory_vector_by_name, format (0, "%s%c", name, 0), index);
+}
+
+static u32
+vlib_stats_get_next_vector_index ()
+{
+  stat_segment_main_t *sm = &stat_segment_main;
+  u32 next_vector_index = vec_len (sm->directory_vector);
+
+  ssize_t i;
+  vec_foreach_index_backwards (i, sm->directory_vector)
+  {
+    if (sm->directory_vector[i].type == STAT_DIR_TYPE_EMPTY)
+      {
+       next_vector_index = i;
+       break;
+      }
+  }
+
+  return next_vector_index;
+}
+
+static u32
+vlib_stats_create_counter (stat_segment_directory_entry_t * e, void *oldheap)
+{
+  stat_segment_main_t *sm = &stat_segment_main;
+
+  ASSERT (clib_mem_get_heap () == sm->heap);
+
+  u32 index = vlib_stats_get_next_vector_index ();
+
+  clib_mem_set_heap (oldheap);
+  create_hash_index ((u8 *) e->name, index);
+  clib_mem_set_heap (sm->heap);
+
+  vec_validate (sm->directory_vector, index);
+  sm->directory_vector[index] = *e;
+
+  return index;
+}
+
+static void
+vlib_stats_delete_counter (u32 index, void *oldheap)
+{
+  stat_segment_main_t *sm = &stat_segment_main;
+  stat_segment_directory_entry_t *e;
+
+  ASSERT (clib_mem_get_heap () == sm->heap);
+
+  if (index > vec_len (sm->directory_vector))
+    return;
+
+  e = &sm->directory_vector[index];
+
+  clib_mem_set_heap (oldheap);
+  hash_unset (sm->directory_vector_by_name, &e->name);
+  clib_mem_set_heap (sm->heap);
+
+  memset (e, 0, sizeof (*e));
+  e->type = STAT_DIR_TYPE_EMPTY;
+}
+
 void
 vlib_stats_pop_heap (void *cm_arg, void *oldheap, u32 cindex,
                     stat_directory_type_t type)
@@ -109,20 +172,18 @@ vlib_stats_pop_heap (void *cm_arg, void *oldheap, u32 cindex,
   /* Lookup hash-table is on the main heap */
   stat_segment_name =
     cm->stat_segment_name ? cm->stat_segment_name : cm->name;
-  u32 next_vector_index = vec_len (sm->directory_vector);
   clib_mem_set_heap (oldheap); /* Exit stats segment */
-  u32 vector_index = lookup_or_create_hash_index ((u8 *) stat_segment_name,
-                                                 next_vector_index);
+  u32 vector_index = lookup_hash_index ((u8 *) stat_segment_name);
   /* Back to stats segment */
   clib_mem_set_heap (sm->heap);        /* Re-enter stat segment */
 
 
   /* Update the vector */
-  if (vector_index == next_vector_index)
+  if (vector_index == STAT_SEGMENT_INDEX_INVALID)
     {                          /* New */
       strncpy (e.name, stat_segment_name, 128 - 1);
       e.type = type;
-      vec_add1 (sm->directory_vector, e);
+      vector_index = vlib_stats_create_counter (&e, oldheap);
     }
 
   stat_segment_directory_entry_t *ep = &sm->directory_vector[vector_index];
@@ -168,23 +229,19 @@ vlib_stats_register_error_index (void *oldheap, u8 * name, u64 * em_vec,
   ASSERT (shared_header);
 
   vlib_stat_segment_lock ();
-  u32 next_vector_index = vec_len (sm->directory_vector);
   clib_mem_set_heap (oldheap); /* Exit stats segment */
-
-  u32 vector_index = lookup_or_create_hash_index (name,
-                                                 next_vector_index);
-
+  u32 vector_index = lookup_hash_index (name);
   /* Back to stats segment */
   clib_mem_set_heap (sm->heap);        /* Re-enter stat segment */
 
-  if (next_vector_index == vector_index)
+  if (vector_index == STAT_SEGMENT_INDEX_INVALID)
     {
       memcpy (e.name, name, vec_len (name));
       e.name[vec_len (name)] = '\0';
       e.type = STAT_DIR_TYPE_ERROR_INDEX;
       e.offset = index;
       e.offset_vector = 0;
-      vec_add1 (sm->directory_vector, e);
+      vector_index = vlib_stats_create_counter (&e, oldheap);
 
       /* Warn clients to refresh any pointers they might be holding */
       shared_header->directory_offset =
@@ -377,6 +434,10 @@ format_stat_dir_entry (u8 * s, va_list * args)
       type_name = "NameVector";
       break;
 
+    case STAT_DIR_TYPE_EMPTY:
+      type_name = "empty";
+      break;
+
     default:
       type_name = "illegal!";
       break;
@@ -410,6 +471,11 @@ show_stat_segment_command_fn (vlib_main_t * vm,
 
   for (i = 0; i < vec_len (show_data); i++)
     {
+      stat_segment_directory_entry_t *ep = vec_elt_at_index (show_data, i);
+
+      if (ep->type == STAT_DIR_TYPE_EMPTY)
+       continue;
+
       vlib_cli_output (vm, "%-100U", format_stat_dir_entry,
                       vec_elt_at_index (show_data, i));
     }
@@ -761,21 +827,18 @@ stat_segment_register_gauge (u8 * name, stat_segment_update_fn update_fn,
 
   ASSERT (shared_header);
 
-  u32 next_vector_index = vec_len (sm->directory_vector);
-  u32 vector_index = lookup_or_create_hash_index (name,
-                                                 next_vector_index);
-
-  if (vector_index < next_vector_index)        /* Already registered */
-    return clib_error_return (0, "%v is alreadty registered", name);
+  u32 vector_index = lookup_hash_index (name);
 
-  oldheap = vlib_stats_push_heap (NULL);
-  vlib_stat_segment_lock ();
+  if (vector_index != STAT_SEGMENT_INDEX_INVALID)      /* Already registered */
+    return clib_error_return (0, "%v is already registered", name);
 
   memset (&e, 0, sizeof (e));
   e.type = STAT_DIR_TYPE_SCALAR_INDEX;
-
   memcpy (e.name, name, vec_len (name));
-  vec_add1 (sm->directory_vector, e);
+
+  oldheap = vlib_stats_push_heap (NULL);
+  vlib_stat_segment_lock ();
+  vector_index = vlib_stats_create_counter (&e, oldheap);
 
   shared_header->directory_offset =
     stat_segment_offset (shared_header, sm->directory_vector);
@@ -787,7 +850,7 @@ stat_segment_register_gauge (u8 * name, stat_segment_update_fn update_fn,
   pool_get (sm->gauges, gauge);
   gauge->fn = update_fn;
   gauge->caller_index = caller_index;
-  gauge->directory_index = next_vector_index;
+  gauge->directory_index = vector_index;
 
   return NULL;
 }
@@ -803,21 +866,19 @@ stat_segment_register_state_counter (u8 * name, u32 * index)
   ASSERT (shared_header);
   ASSERT (vlib_get_thread_index () == 0);
 
-  u32 next_vector_index = vec_len (sm->directory_vector);
-  u32 vector_index = lookup_or_create_hash_index (name,
-                                                 next_vector_index);
+  u32 vector_index = lookup_hash_index (name);
 
-  if (vector_index < next_vector_index)        /* Already registered */
+  if (vector_index != STAT_SEGMENT_INDEX_INVALID)      /* Already registered */
     return clib_error_return (0, "%v is already registered", name);
 
-  oldheap = vlib_stats_push_heap (NULL);
-  vlib_stat_segment_lock ();
-
   memset (&e, 0, sizeof (e));
   e.type = STAT_DIR_TYPE_SCALAR_INDEX;
-
   memcpy (e.name, name, vec_len (name));
-  vec_add1 (sm->directory_vector, e);
+
+  oldheap = vlib_stats_push_heap (NULL);
+  vlib_stat_segment_lock ();
+
+  vector_index = vlib_stats_create_counter (&e, oldheap);
 
   shared_header->directory_offset =
     stat_segment_offset (shared_header, sm->directory_vector);
@@ -825,7 +886,7 @@ stat_segment_register_state_counter (u8 * name, u32 * index)
   vlib_stat_segment_unlock ();
   clib_mem_set_heap (oldheap);
 
-  *index = next_vector_index;
+  *index = vector_index;
   return 0;
 }
 
@@ -835,6 +896,7 @@ stat_segment_deregister_state_counter (u32 index)
   stat_segment_main_t *sm = &stat_segment_main;
   stat_segment_shared_header_t *shared_header = sm->shared_header;
   stat_segment_directory_entry_t *e;
+  void *oldheap;
 
   ASSERT (shared_header);
 
@@ -845,8 +907,14 @@ stat_segment_deregister_state_counter (u32 index)
   if (e->type != STAT_DIR_TYPE_SCALAR_INDEX)
     return clib_error_return (0, "%u index cannot be deleted", index);
 
-  hash_unset (sm->directory_vector_by_name, &e->name);
-  vec_del1 (sm->directory_vector, index);
+  oldheap = vlib_stats_push_heap (NULL);
+  vlib_stat_segment_lock ();
+
+  vlib_stats_delete_counter (index, oldheap);
+
+  vlib_stat_segment_unlock ();
+  clib_mem_set_heap (oldheap);
+
   return 0;
 }
 

diff --git a/src/vpp/stats/stat_segment.h b/src/vpp/stats/stat_segment.h
index 83d2334..28e9ca3 100644 (file)
--- a/src/vpp/stats/stat_segment.h
+++ b/src/vpp/stats/stat_segment.h
@@ -64,6 +64,8 @@ typedef enum
 /* Shared segment memory layout version */
 #define STAT_SEGMENT_VERSION           1
 
+#define STAT_SEGMENT_INDEX_INVALID     UINT32_MAX
+
 static inline uint64_t
 stat_segment_offset (void *start, void *data)
 {

diff --git a/src/vpp/stats/stat_segment_shared.h b/src/vpp/stats/stat_segment_shared.h
index 719cf59..982bddd 100644 (file)
--- a/src/vpp/stats/stat_segment_shared.h
+++ b/src/vpp/stats/stat_segment_shared.h
@@ -24,6 +24,7 @@ typedef enum
   STAT_DIR_TYPE_COUNTER_VECTOR_COMBINED,
   STAT_DIR_TYPE_ERROR_INDEX,
   STAT_DIR_TYPE_NAME_VECTOR,
+  STAT_DIR_TYPE_EMPTY,
 } stat_directory_type_t;
 
 typedef struct

diff --git a/test/test_bond.py b/test/test_bond.py
index 5ef865f..dd4a645 100644 (file)
--- a/test/test_bond.py
+++ b/test/test_bond.py
@@ -166,50 +166,51 @@ class TestBondInterface(VppTestCase):
     def test_bond_enslave(self):
         """ Bond enslave/detach slave test """
 
-        # create interface (BondEthernet0)
+        # create interface (BondEthernet0) and set bond mode to LACP
         self.logger.info("create bond")
-        bond0 = VppBondInterface(self, mode=3)
+        bond0 = VppBondInterface(self, mode=5)
         bond0.add_vpp_config()
         bond0.admin_up()
 
-        # verify pg0 and pg1 not in BondEthernet0
-        if_dump = self.vapi.sw_interface_slave_dump(bond0.sw_if_index)
-        self.assertFalse(self.pg0.is_interface_config_in_dump(if_dump))
-        self.assertFalse(self.pg1.is_interface_config_in_dump(if_dump))
-
-        # enslave pg0 and pg1 to BondEthernet0
-        self.logger.info("bond enslave interface pg0 to BondEthernet0")
-        bond0.enslave_vpp_bond_interface(sw_if_index=self.pg0.sw_if_index,
-                                         is_passive=0,
-                                         is_long_timeout=0)
-
-        self.logger.info("bond enslave interface pg1 to BondEthernet0")
-        bond0.enslave_vpp_bond_interface(sw_if_index=self.pg1.sw_if_index,
-                                         is_passive=0,
-                                         is_long_timeout=0)
-
-        # verify both slaves in BondEthernet0
-        if_dump = self.vapi.sw_interface_slave_dump(bond0.sw_if_index)
-        self.assertTrue(self.pg0.is_interface_config_in_dump(if_dump))
-        self.assertTrue(self.pg1.is_interface_config_in_dump(if_dump))
-
-        # detach interface pg0
-        self.logger.info("detach interface pg0")
-        bond0.detach_vpp_bond_interface(sw_if_index=self.pg0.sw_if_index)
-
-        # verify pg0 is not in BondEthernet0, but pg1 is
-        if_dump = self.vapi.sw_interface_slave_dump(bond0.sw_if_index)
-        self.assertFalse(self.pg0.is_interface_config_in_dump(if_dump))
-        self.assertTrue(self.pg1.is_interface_config_in_dump(if_dump))
-
-        # detach interface pg1
-        self.logger.info("detach interface pg1")
-        bond0.detach_vpp_bond_interface(sw_if_index=self.pg1.sw_if_index)
-
-        # verify pg0 and pg1 not in BondEthernet0
-        if_dump = self.vapi.sw_interface_slave_dump(bond0.sw_if_index)
-        self.assertFalse(self.pg0.is_interface_config_in_dump(if_dump))
-        self.assertFalse(self.pg1.is_interface_config_in_dump(if_dump))
+        # verify that interfaces can be enslaved and detached two times
+        for i in range(2):
+            # verify pg0 and pg1 not in BondEthernet0
+            if_dump = self.vapi.sw_interface_slave_dump(bond0.sw_if_index)
+            self.assertFalse(self.pg0.is_interface_config_in_dump(if_dump))
+            self.assertFalse(self.pg1.is_interface_config_in_dump(if_dump))
+
+            # enslave pg0 and pg1 to BondEthernet0
+            self.logger.info("bond enslave interface pg0 to BondEthernet0")
+            bond0.enslave_vpp_bond_interface(sw_if_index=self.pg0.sw_if_index,
+                                             is_passive=0,
+                                             is_long_timeout=0)
+
+            self.logger.info("bond enslave interface pg1 to BondEthernet0")
+            bond0.enslave_vpp_bond_interface(sw_if_index=self.pg1.sw_if_index,
+                                             is_passive=0,
+                                             is_long_timeout=0)
+            # verify both slaves in BondEthernet0
+            if_dump = self.vapi.sw_interface_slave_dump(bond0.sw_if_index)
+            self.assertTrue(self.pg0.is_interface_config_in_dump(if_dump))
+            self.assertTrue(self.pg1.is_interface_config_in_dump(if_dump))
+
+            # detach interface pg0
+            self.logger.info("detach interface pg0")
+            bond0.detach_vpp_bond_interface(sw_if_index=self.pg0.sw_if_index)
+
+            # verify pg0 is not in BondEthernet0, but pg1 is
+            if_dump = self.vapi.sw_interface_slave_dump(bond0.sw_if_index)
+            self.assertFalse(self.pg0.is_interface_config_in_dump(if_dump))
+            self.assertTrue(self.pg1.is_interface_config_in_dump(if_dump))
+
+            # detach interface pg1
+            self.logger.info("detach interface pg1")
+            bond0.detach_vpp_bond_interface(sw_if_index=self.pg1.sw_if_index)
+
+            # verify pg0 and pg1 not in BondEthernet0
+            if_dump = self.vapi.sw_interface_slave_dump(bond0.sw_if_index)
+            self.assertFalse(self.pg0.is_interface_config_in_dump(if_dump))
+            self.assertFalse(self.pg1.is_interface_config_in_dump(if_dump))
 
         bond0.remove_vpp_config()