Merge remote-tracking branch 'public/no_itime_queue_025'

changes/bug10870

@@ -0,0 +1,6 @@
+  o Code simplification and refactoring:
+    - Remove data structures which were introduced to implement the
+      CellStatistics option: they are now redundant with the addition
+      of timestamp to the regular packed_cell_t data structure, which
+      we did in 0.2.4.18-rc in order to resolve #9093. Fixes bug
+      10870.

src/or/or.h

@@ -1121,48 +1121,12 @@ typedef struct packed_cell_t {
                            * bits truncated) when this cell was inserted. */
 } packed_cell_t;
 
-/* XXXX This next structure may be obsoleted by inserted_time in
- * packed_cell_t */
-
-/** Number of cells added to a circuit queue including their insertion
- * time on 10 millisecond detail; used for buffer statistics. */
-typedef struct insertion_time_elem_t {
-  struct insertion_time_elem_t *next; /**< Next element in queue. */
-  uint32_t insertion_time; /**< When were cells inserted (in 10 ms steps
-                             * starting at 0:00 of the current day)? */
-  unsigned counter; /**< How many cells were inserted? */
-} insertion_time_elem_t;
-
-/** Queue of insertion times. */
-typedef struct insertion_time_queue_t {
-  struct insertion_time_elem_t *first; /**< First element in queue. */
-  struct insertion_time_elem_t *last; /**< Last element in queue. */
-} insertion_time_queue_t;
-
-/** Number of cells with the same command consecutively added to a circuit
- * queue; used for cell statistics only if CELL_STATS events are enabled. */
-typedef struct insertion_command_elem_t {
-  struct insertion_command_elem_t *next; /**< Next element in queue. */
-  /** Which command did these consecutively added cells have? */
-  uint8_t command;
-  unsigned counter; /**< How many cells were inserted? */
-} insertion_command_elem_t;
-
-/** Queue of insertion commands. */
-typedef struct insertion_command_queue_t {
-  struct insertion_command_elem_t *first; /**< First element in queue. */
-  struct insertion_command_elem_t *last; /**< Last element in queue. */
-} insertion_command_queue_t;
-
 /** A queue of cells on a circuit, waiting to be added to the
  * or_connection_t's outbuf. */
 typedef struct cell_queue_t {
   /** Linked list of packed_cell_t*/
   TOR_SIMPLEQ_HEAD(cell_simpleq, packed_cell_t) head;
   int n; /**< The number of cells in the queue. */
-  insertion_time_queue_t *insertion_times; /**< Insertion times of cells. */
- /** Commands of inserted cells. */
-  insertion_command_queue_t *insertion_commands;
 } cell_queue_t;
 
 /** Beginning of a RELAY cell payload. */

src/or/relay.c

@@ -2047,14 +2047,6 @@ static size_t total_cells_allocated = 0;
 /** A memory pool to allocate packed_cell_t objects. */
 static mp_pool_t *cell_pool = NULL;
 
-/** Memory pool to allocate insertion_time_elem_t objects used for cell
- * statistics. */
-static mp_pool_t *it_pool = NULL;
-
-/** Memory pool to allocate insertion_command_elem_t objects used for cell
- * statistics if CELL_STATS events are enabled. */
-static mp_pool_t *ic_pool = NULL;
-
 /** Allocate structures to hold cells. */
 void
 init_cell_pool(void)
@@ -2073,14 +2065,6 @@ free_cell_pool(void)
     mp_pool_destroy(cell_pool);
     cell_pool = NULL;
   }
-  if (it_pool) {
-    mp_pool_destroy(it_pool);
-    it_pool = NULL;
-  }
-  if (ic_pool) {
-    mp_pool_destroy(ic_pool);
-    ic_pool = NULL;
-  }
 }
 
 /** Free excess storage in cell pool. */
@@ -2153,57 +2137,6 @@ cell_queue_append(cell_queue_t *queue, packed_cell_t *cell)
   ++queue->n;
 }
 
-/** Append command of type <b>command</b> in direction to <b>queue</b> for
- * CELL_STATS event. */
-static void
-cell_command_queue_append(cell_queue_t *queue, uint8_t command)
-{
-  insertion_command_queue_t *ic_queue = queue->insertion_commands;
-  if (!ic_pool)
-    ic_pool = mp_pool_new(sizeof(insertion_command_elem_t), 1024);
-  if (!ic_queue) {
-    ic_queue = tor_malloc_zero(sizeof(insertion_command_queue_t));
-    queue->insertion_commands = ic_queue;
-  }
-  if (ic_queue->last && ic_queue->last->command == command) {
-    ic_queue->last->counter++;
-  } else {
-    insertion_command_elem_t *elem = mp_pool_get(ic_pool);
-    elem->next = NULL;
-    elem->command = command;
-    elem->counter = 1;
-    if (ic_queue->last) {
-      ic_queue->last->next = elem;
-      ic_queue->last = elem;
-    } else {
-      ic_queue->first = ic_queue->last = elem;
-    }
-  }
-}
-
-/** Retrieve oldest command from <b>queue</b> and write it to
- * <b>command</b> for CELL_STATS event.  Return 0 for success, -1
- * otherwise. */
-static int
-cell_command_queue_pop(uint8_t *command, cell_queue_t *queue)
-{
-  int res = -1;
-  insertion_command_queue_t *ic_queue = queue->insertion_commands;
-  if (ic_queue && ic_queue->first) {
-    insertion_command_elem_t *ic_elem = ic_queue->first;
-    ic_elem->counter--;
-    if (ic_elem->counter < 1) {
-      ic_queue->first = ic_elem->next;
-      if (ic_elem == ic_queue->last)
-        ic_queue->last = NULL;
-      mp_pool_release(ic_elem);
-    }
-    *command = ic_elem->command;
-    res = 0;
-  }
-  return res;
-}
-
 /** Append a newly allocated copy of <b>cell</b> to the end of the
  * <b>exitward</b> (or app-ward) <b>queue</b> of <b>circ</b>.  If
  * <b>use_stats</b> is true, record statistics about the cell.
@@ -2215,52 +2148,12 @@ cell_queue_append_packed_copy(circuit_t *circ, cell_queue_t *queue,
 {
   struct timeval now;
   packed_cell_t *copy = packed_cell_copy(cell, wide_circ_ids);
+  (void)circ;
+  (void)exitward;
+  (void)use_stats;
   tor_gettimeofday_cached(&now);
   copy->inserted_time = (uint32_t)tv_to_msec(&now);
 
-  /* Remember the time when this cell was put in the queue. */
-  /*XXXX This may be obsoleted by inserted_time */
-  if ((get_options()->CellStatistics ||
-      get_options()->TestingEnableCellStatsEvent) && use_stats) {
-    uint32_t added;
-    insertion_time_queue_t *it_queue = queue->insertion_times;
-    if (!it_pool)
-      it_pool = mp_pool_new(sizeof(insertion_time_elem_t), 1024);
-
-#define SECONDS_IN_A_DAY 86400L
-    added = (uint32_t)(((now.tv_sec % SECONDS_IN_A_DAY) * 100L)
-            + ((uint32_t)now.tv_usec / (uint32_t)10000L));
-    if (!it_queue) {
-      it_queue = tor_malloc_zero(sizeof(insertion_time_queue_t));
-      queue->insertion_times = it_queue;
-    }
-    if (it_queue->last && it_queue->last->insertion_time == added) {
-      it_queue->last->counter++;
-    } else {
-      insertion_time_elem_t *elem = mp_pool_get(it_pool);
-      elem->next = NULL;
-      elem->insertion_time = added;
-      elem->counter = 1;
-      if (it_queue->last) {
-        it_queue->last->next = elem;
-        it_queue->last = elem;
-      } else {
-        it_queue->first = it_queue->last = elem;
-      }
-    }
-  }
-  /* Remember that we added a cell to the queue, and remember the cell
-   * command. */
-  if (get_options()->TestingEnableCellStatsEvent && circ) {
-    testing_cell_stats_entry_t *ent =
-                      tor_malloc_zero(sizeof(testing_cell_stats_entry_t));
-    ent->command = cell->command;
-    ent->exitward = exitward;
-    if (!circ->testing_cell_stats)
-      circ->testing_cell_stats = smartlist_new();
-    smartlist_add(circ->testing_cell_stats, ent);
-    cell_command_queue_append(queue, cell->command);
-  }
   cell_queue_append(queue, copy);
 }
 
@@ -2283,14 +2176,6 @@ cell_queue_clear(cell_queue_t *queue)
   }
   TOR_SIMPLEQ_INIT(&queue->head);
   queue->n = 0;
-  if (queue->insertion_times) {
-    while (queue->insertion_times->first) {
-      insertion_time_elem_t *elem = queue->insertion_times->first;
-      queue->insertion_times->first = elem->next;
-      mp_pool_release(elem);
-    }
-    tor_free(queue->insertion_times);
-  }
 }
 
 /** Extract and return the cell at the head of <b>queue</b>; return NULL if
@@ -2311,9 +2196,7 @@ cell_queue_pop(cell_queue_t *queue)
 size_t
 packed_cell_mem_cost(void)
 {
-  return sizeof(packed_cell_t) + MP_POOL_ITEM_OVERHEAD +
-    get_options()->CellStatistics ?
-    (sizeof(insertion_time_elem_t)+MP_POOL_ITEM_OVERHEAD) : 0;
+  return sizeof(packed_cell_t) + MP_POOL_ITEM_OVERHEAD;
 }
 
 /** Check whether we've got too much space used for cells.  If so,
@@ -2443,6 +2326,17 @@ set_streams_blocked_on_circ(circuit_t *circ, channel_t *chan,
   return n;
 }
 
+/** Extract the command from a packed cell. */
+static uint8_t
+packed_cell_get_command(const packed_cell_t *cell, int wide_circ_ids)
+{
+  if (wide_circ_ids) {
+    return get_uint8(cell->body+4);
+  } else {
+    return get_uint8(cell->body+2);
+  }
+}
+
 /** Pull as many cells as possible (but no more than <b>max</b>) from the
  * queue of the first active circuit on <b>chan</b>, and write them to
  * <b>chan</b>-&gt;outbuf.  Return the number of cells written.  Advance
@@ -2504,55 +2398,30 @@ channel_flush_from_first_active_circuit(channel_t *chan, int max)
     /* Calculate the exact time that this cell has spent in the queue. */
     if (get_options()->CellStatistics ||
         get_options()->TestingEnableCellStatsEvent) {
+      uint32_t msec_waiting;
       struct timeval tvnow;
-      uint32_t flushed;
-      uint32_t cell_waiting_time;
-      insertion_time_queue_t *it_queue = queue->insertion_times;
       tor_gettimeofday_cached(&tvnow);
-      flushed = (uint32_t)((tvnow.tv_sec % SECONDS_IN_A_DAY) * 100L +
-                 (uint32_t)tvnow.tv_usec / (uint32_t)10000L);
-      if (!it_queue || !it_queue->first) {
-        log_info(LD_GENERAL, "Cannot determine insertion time of cell. "
-                             "Looks like the CellStatistics option was "
-                             "recently enabled.");
-      } else {
-        insertion_time_elem_t *elem = it_queue->first;
-        cell_waiting_time =
-            (uint32_t)((flushed * 10L + SECONDS_IN_A_DAY * 1000L -
-                        elem->insertion_time * 10L) %
-                       (SECONDS_IN_A_DAY * 1000L));
-#undef SECONDS_IN_A_DAY
-        elem->counter--;
-        if (elem->counter < 1) {
-          it_queue->first = elem->next;
-          if (elem == it_queue->last)
-            it_queue->last = NULL;
-          mp_pool_release(elem);
-        }
-        if (get_options()->CellStatistics && !CIRCUIT_IS_ORIGIN(circ)) {
-          or_circ = TO_OR_CIRCUIT(circ);
-          or_circ->total_cell_waiting_time += cell_waiting_time;
-          or_circ->processed_cells++;
-        }
-        if (get_options()->TestingEnableCellStatsEvent) {
-          uint8_t command;
-          if (cell_command_queue_pop(&command, queue) < 0) {
-            log_info(LD_GENERAL, "Cannot determine command of cell. "
-                                 "Looks like the CELL_STATS event was "
-                                 "recently enabled.");
-          } else {
-            testing_cell_stats_entry_t *ent =
-                        tor_malloc_zero(sizeof(testing_cell_stats_entry_t));
-            ent->command = command;
-            ent->waiting_time = (unsigned int)cell_waiting_time / 10;
-            ent->removed = 1;
-            if (circ->n_chan == chan)
-              ent->exitward = 1;
-            if (!circ->testing_cell_stats)
-              circ->testing_cell_stats = smartlist_new();
-            smartlist_add(circ->testing_cell_stats, ent);
-          }
-        }
+      msec_waiting = ((uint32_t)tv_to_msec(&tvnow)) - cell->inserted_time;
+
+      if (get_options()->CellStatistics && !CIRCUIT_IS_ORIGIN(circ)) {
+        or_circ = TO_OR_CIRCUIT(circ);
+        or_circ->total_cell_waiting_time += msec_waiting;
+        or_circ->processed_cells++;
+      }
+
+      if (get_options()->TestingEnableCellStatsEvent) {
+        uint8_t command = packed_cell_get_command(cell, chan->wide_circ_ids);
+
+        testing_cell_stats_entry_t *ent =
+          tor_malloc_zero(sizeof(testing_cell_stats_entry_t));
+        ent->command = command;
+        ent->waiting_time = msec_waiting / 10;
+        ent->removed = 1;
+        if (circ->n_chan == chan)
+          ent->exitward = 1;
+        if (!circ->testing_cell_stats)
+          circ->testing_cell_stats = smartlist_new();
+        smartlist_add(circ->testing_cell_stats, ent);
       }
     }