session: fix race condition in fifo allocation

Under some timing conditions,VCL may receive CONNECTED/ACCEPTED
event before ADD_SEGMENT event.

Timing example:

2 threads call segment_manager_alloc_session_fifos() parallelly

         Thread 1                Thread 2

       sm read lock                 |
            |                       |
     try to alloc fifo =>failed     |
            |                       |
       sm read unlock               |
            |                       |
       sm write lock                |
            |                       |
       add segment                  |
            |                       |
       sm write unlock              |
            |                  sm read lock
            |                       |
            |               try to alloc fifo=>successful
       sm read lock                 |
            |                  sm read unlock
            |                       |
            |                 emit CONNECTED/ACCEPTED
   emit ADD_SEGMENT event
            |
       sm read unlock

This commit move ADD_SEGMENT notification under the protection
of the write lock in some scenarios.

Type: fix

Signed-off-by: liuyacan <liuyacan@corp.netease.com>
Change-Id: I25d5475c5e6d37cfccefa9506f6030c26ce8ee9b

diff --git a/src/plugins/unittest/segment_manager_test.c b/src/plugins/unittest/segment_manager_test.c
index c6f5297..31b417a 100644 (file)
--- a/src/plugins/unittest/segment_manager_test.c
+++ b/src/plugins/unittest/segment_manager_test.c
@@ -448,7 +448,7 @@ segment_manager_test_fifo_balanced_alloc (vlib_main_t * vm,
     }
 
   /* add another 2MB segment */
-  fs_index = segment_manager_add_segment (sm, size_2MB);
+  fs_index = segment_manager_add_segment (sm, size_2MB, 0);
   SEG_MGR_TEST ((fs_index == 1), "fs_index %d", fs_index);
 
   /* allocate fifos : 4KB x2

diff --git a/src/vnet/session/application_local.c b/src/vnet/session/application_local.c
index 478fc7a..1220952 100644 (file)
--- a/src/vnet/session/application_local.c
+++ b/src/vnet/session/application_local.c
@@ -188,7 +188,7 @@ ct_init_accepted_session (app_worker_t * server_wrk,
   seg_size = 4 * (round_rx_fifo_sz + round_tx_fifo_sz + margin);
 
   sm = app_worker_get_listen_segment_manager (server_wrk, ll);
-  seg_index = segment_manager_add_segment (sm, seg_size);
+  seg_index = segment_manager_add_segment (sm, seg_size, 0);
   if (seg_index < 0)
     {
       clib_warning ("failed to add new cut-through segment");

diff --git a/src/vnet/session/segment_manager.c b/src/vnet/session/segment_manager.c
index 560560c..73b2f71 100644 (file)
--- a/src/vnet/session/segment_manager.c
+++ b/src/vnet/session/segment_manager.c
@@ -88,7 +88,8 @@ segment_manager_segment_index (segment_manager_t * sm, fifo_segment_t * seg)
  * to avoid affecting any of the segments pool readers.
  */
 int
-segment_manager_add_segment (segment_manager_t * sm, uword segment_size)
+segment_manager_add_segment (segment_manager_t *sm, uword segment_size,
+                            u8 notify_app)
 {
   segment_manager_main_t *smm = &sm_main;
   segment_manager_props_t *props;
@@ -162,6 +163,16 @@ segment_manager_add_segment (segment_manager_t * sm, uword segment_size)
   fs->h->pct_first_alloc = props->pct_first_alloc;
   fs->h->flags &= ~FIFO_SEGMENT_F_MEM_LIMIT;
 
+  if (notify_app)
+    {
+      app_worker_t *app_wrk;
+      u64 fs_handle;
+      fs_handle = segment_manager_segment_handle (sm, fs);
+      app_wrk = app_worker_get (sm->app_wrk_index);
+      rv = app_worker_add_segment_notify (app_wrk, fs_handle);
+      if (rv)
+       return rv;
+    }
 done:
 
   if (vlib_num_workers ())
@@ -376,7 +387,7 @@ segment_manager_init_first (segment_manager_t * sm)
       /* Allocate the segments */
       for (i = 0; i < approx_segment_count + 1; i++)
        {
-         fs_index = segment_manager_add_segment (sm, max_seg_size);
+         fs_index = segment_manager_add_segment (sm, max_seg_size, 0);
          if (fs_index < 0)
            {
              clib_warning ("Failed to preallocate segment %d", i);
@@ -398,7 +409,7 @@ segment_manager_init_first (segment_manager_t * sm)
       return 0;
     }
 
-  fs_index = segment_manager_add_segment (sm, first_seg_size);
+  fs_index = segment_manager_add_segment (sm, first_seg_size, 0);
   if (fs_index < 0)
     {
       clib_warning ("Failed to allocate segment");
@@ -665,8 +676,6 @@ segment_manager_alloc_session_fifos (segment_manager_t * sm,
   segment_manager_props_t *props;
   fifo_segment_t *fs = 0, *cur;
   u32 sm_index, fs_index;
-  u8 added_a_segment = 0;
-  u64 fs_handle;
 
   props = segment_manager_properties_get (sm);
 
@@ -700,45 +709,12 @@ segment_manager_alloc_session_fifos (segment_manager_t * sm,
 
   segment_manager_segment_reader_unlock (sm);
 
-alloc_check:
-
-  if (!alloc_fail)
-    {
-
-    alloc_success:
-
-      ASSERT (rx_fifo && tx_fifo);
-      sm_index = segment_manager_index (sm);
-      fs_index = segment_manager_segment_index (sm, fs);
-      (*tx_fifo)->segment_manager = sm_index;
-      (*rx_fifo)->segment_manager = sm_index;
-      (*tx_fifo)->segment_index = fs_index;
-      (*rx_fifo)->segment_index = fs_index;
-
-      if (added_a_segment)
-       {
-         app_worker_t *app_wrk;
-         fs_handle = segment_manager_segment_handle (sm, fs);
-         app_wrk = app_worker_get (sm->app_wrk_index);
-         rv = app_worker_add_segment_notify (app_wrk, fs_handle);
-       }
-      /* Drop the lock after app is notified */
-      segment_manager_segment_reader_unlock (sm);
-      return rv;
-    }
-
   /*
    * Allocation failed, see if we can add a new segment
    */
   if (props->add_segment)
     {
-      if (added_a_segment)
-       {
-         clib_warning ("Added a segment, still can't allocate a fifo");
-         segment_manager_segment_reader_unlock (sm);
-         return SESSION_E_SEG_NO_SPACE2;
-       }
-      if ((new_fs_index = segment_manager_add_segment (sm, 0)) < 0)
+      if ((new_fs_index = segment_manager_add_segment (sm, 0, 1)) < 0)
        {
          clib_warning ("Failed to add new segment");
          return SESSION_E_SEG_CREATE;
@@ -748,14 +724,33 @@ alloc_check:
                                                    props->rx_fifo_size,
                                                    props->tx_fifo_size,
                                                    rx_fifo, tx_fifo);
-      added_a_segment = 1;
-      goto alloc_check;
+      if (alloc_fail)
+       {
+         clib_warning ("Added a segment, still can't allocate a fifo");
+         segment_manager_segment_reader_unlock (sm);
+         return SESSION_E_SEG_NO_SPACE2;
+       }
     }
   else
     {
       SESSION_DBG ("Can't add new seg and no space to allocate fifos!");
       return SESSION_E_SEG_NO_SPACE;
     }
+
+alloc_success:
+  ASSERT (rx_fifo && tx_fifo);
+
+  sm_index = segment_manager_index (sm);
+  fs_index = segment_manager_segment_index (sm, fs);
+  (*tx_fifo)->segment_manager = sm_index;
+  (*rx_fifo)->segment_manager = sm_index;
+  (*tx_fifo)->segment_index = fs_index;
+  (*rx_fifo)->segment_index = fs_index;
+
+  /* Drop the lock after app is notified */
+  segment_manager_segment_reader_unlock (sm);
+
+  return rv;
 }
 
 void

diff --git a/src/vnet/session/segment_manager.h b/src/vnet/session/segment_manager.h
index 7c3434b..e73a70f 100644 (file)
--- a/src/vnet/session/segment_manager.h
+++ b/src/vnet/session/segment_manager.h
@@ -101,7 +101,8 @@ segment_manager_t *segment_manager_get (u32 index);
 segment_manager_t *segment_manager_get_if_valid (u32 index);
 u32 segment_manager_index (segment_manager_t * sm);
 
-int segment_manager_add_segment (segment_manager_t * sm, uword segment_size);
+int segment_manager_add_segment (segment_manager_t *sm, uword segment_size,
+                                u8 notify_app);
 void segment_manager_del_segment (segment_manager_t * sm,
                                  fifo_segment_t * fs);
 void segment_manager_lock_and_del_segment (segment_manager_t * sm,