Fix: misaligned urcu reader accesses

Running the LTTng-tools tests (test_valid_filter, for example) under
address sanitizer results in the following warning:

  /usr/include/lttng/urcu/static/urcu-ust.h:155:6: runtime error: member access within misaligned address 0x7fc45db3a020 for type 'struct lttng_ust_urcu_reader', which requires 128 byte alignment
  0x7fc45db3a020: note: pointer points here
   c4 7f 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00
                ^

While the node member of lttng_ust_urcu_reader has an "aligned"
attribute of CAA_CACHE_LINE_SIZE, the compiler can't ensure the
alignment of members for dynamically allocated instances.

The `data` pointer is changed from char* to struct
lttng_ust_urcu_reader*, allowing the compiler to enforce the expected
alignment constraints.

Since `data` was addressed in bytes, the code using this field is
adapted to use element counts. As the chunks are only used to allocate
reader instances (and not other types), it makes the code a bit easier
to read.

Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Change-Id: Ic826cf94444681bea3a192d3a9f4262a0961e948

diff --git a/src/lib/lttng-ust-common/lttng-ust-urcu.c b/src/lib/lttng-ust-common/lttng-ust-urcu.c
index 0b2d6fc05a9f5c44c720f3f3e57c5901124d10ba..8d2a23b0d479cffdb2e40f8f9c4731363143deed 100644 (file)
--- a/src/lib/lttng-ust-common/lttng-ust-urcu.c
+++ b/src/lib/lttng-ust-common/lttng-ust-urcu.c
@@ -65,10 +65,7 @@ void *mremap_wrapper(void *old_address __attribute__((unused)),
 
 /* Sleep delay in ms */
 #define RCU_SLEEP_DELAY_MS     10
-#define INIT_NR_THREADS                8
-#define ARENA_INIT_ALLOC               \
-       sizeof(struct registry_chunk)   \
-       + INIT_NR_THREADS * sizeof(struct lttng_ust_urcu_reader)
+#define INIT_READER_COUNT      8
 
 /*
  * Active attempts to check for reader Q.S. before calling sleep().
@@ -137,10 +134,10 @@ DEFINE_URCU_TLS(struct lttng_ust_urcu_reader *, lttng_ust_urcu_reader);
 static CDS_LIST_HEAD(registry);
 
 struct registry_chunk {
-       size_t data_len;                /* data length */
-       size_t used;                    /* amount of data used */
+       size_t capacity;                /* capacity of this chunk (in elements) */
+       size_t used;                    /* count of elements used */
        struct cds_list_head node;      /* chunk_list node */
-       char data[];
+       struct lttng_ust_urcu_reader readers[];
 };
 
 struct registry_arena {
@@ -190,6 +187,13 @@ static void smp_mb_master(void)
        }
 }
 
+/* Get the size of a chunk's allocation from its capacity (an element count). */
+static size_t chunk_allocation_size(size_t capacity)
+{
+       return (capacity * sizeof(struct lttng_ust_urcu_reader)) +
+               sizeof(struct registry_chunk);
+}
+
 /*
  * Always called with rcu_registry lock held. Releases this lock between
  * iterations and grabs it again. Holds the lock when it returns.
@@ -358,24 +362,20 @@ static
 void expand_arena(struct registry_arena *arena)
 {
        struct registry_chunk *new_chunk, *last_chunk;
-       size_t old_chunk_len, new_chunk_len;
+       size_t old_chunk_size_bytes, new_chunk_size_bytes, new_capacity;
 
        /* No chunk. */
        if (cds_list_empty(&arena->chunk_list)) {
-               assert(ARENA_INIT_ALLOC >=
-                       sizeof(struct registry_chunk)
-                       + sizeof(struct lttng_ust_urcu_reader));
-               new_chunk_len = ARENA_INIT_ALLOC;
+               new_chunk_size_bytes = chunk_allocation_size(INIT_READER_COUNT);
                new_chunk = (struct registry_chunk *) mmap(NULL,
-                       new_chunk_len,
+                       new_chunk_size_bytes,
                        PROT_READ | PROT_WRITE,
                        MAP_ANONYMOUS | MAP_PRIVATE,
                        -1, 0);
                if (new_chunk == MAP_FAILED)
                        abort();
-               memset(new_chunk, 0, new_chunk_len);
-               new_chunk->data_len =
-                       new_chunk_len - sizeof(struct registry_chunk);
+               memset(new_chunk, 0, new_chunk_size_bytes);
+               new_chunk->capacity = INIT_READER_COUNT;
                cds_list_add_tail(&new_chunk->node, &arena->chunk_list);
                return;         /* We're done. */
        }
@@ -383,34 +383,32 @@ void expand_arena(struct registry_arena *arena)
        /* Try expanding last chunk. */
        last_chunk = cds_list_entry(arena->chunk_list.prev,
                struct registry_chunk, node);
-       old_chunk_len =
-               last_chunk->data_len + sizeof(struct registry_chunk);
-       new_chunk_len = old_chunk_len << 1;
+       old_chunk_size_bytes = chunk_allocation_size(last_chunk->capacity);
+       new_capacity = last_chunk->capacity << 1;
+       new_chunk_size_bytes = chunk_allocation_size(new_capacity);
 
-       /* Don't allow memory mapping to move, just expand. */
-       new_chunk = mremap_wrapper(last_chunk, old_chunk_len,
-               new_chunk_len, 0);
+        /* Don't allow memory mapping to move, just expand. */
+       new_chunk = mremap_wrapper(last_chunk, old_chunk_size_bytes,
+               new_chunk_size_bytes, 0);
        if (new_chunk != MAP_FAILED) {
                /* Should not have moved. */
                assert(new_chunk == last_chunk);
-               memset((char *) last_chunk + old_chunk_len, 0,
-                       new_chunk_len - old_chunk_len);
-               last_chunk->data_len =
-                       new_chunk_len - sizeof(struct registry_chunk);
+               memset((char *) last_chunk + old_chunk_size_bytes, 0,
+                       new_chunk_size_bytes - old_chunk_size_bytes);
+               last_chunk->capacity = new_capacity;
                return;         /* We're done. */
        }
 
        /* Remap did not succeed, we need to add a new chunk. */
        new_chunk = (struct registry_chunk *) mmap(NULL,
-               new_chunk_len,
+               new_chunk_size_bytes,
                PROT_READ | PROT_WRITE,
                MAP_ANONYMOUS | MAP_PRIVATE,
                -1, 0);
        if (new_chunk == MAP_FAILED)
                abort();
-       memset(new_chunk, 0, new_chunk_len);
-       new_chunk->data_len =
-               new_chunk_len - sizeof(struct registry_chunk);
+       memset(new_chunk, 0, new_chunk_size_bytes);
+       new_chunk->capacity = new_capacity;
        cds_list_add_tail(&new_chunk->node, &arena->chunk_list);
 }
 
@@ -418,22 +416,23 @@ static
 struct lttng_ust_urcu_reader *arena_alloc(struct registry_arena *arena)
 {
        struct registry_chunk *chunk;
-       struct lttng_ust_urcu_reader *rcu_reader_reg;
        int expand_done = 0;    /* Only allow to expand once per alloc */
-       size_t len = sizeof(struct lttng_ust_urcu_reader);
 
 retry:
        cds_list_for_each_entry(chunk, &arena->chunk_list, node) {
-               if (chunk->data_len - chunk->used < len)
+               size_t spot_idx;
+
+               /* Skip fully used chunks. */
+               if (chunk->used == chunk->capacity) {
                        continue;
-               /* Find spot */
-               for (rcu_reader_reg = (struct lttng_ust_urcu_reader *) &chunk->data[0];
-                               rcu_reader_reg < (struct lttng_ust_urcu_reader *) &chunk->data[chunk->data_len];
-                               rcu_reader_reg++) {
-                       if (!rcu_reader_reg->alloc) {
-                               rcu_reader_reg->alloc = 1;
-                               chunk->used += len;
-                               return rcu_reader_reg;
+               }
+
+               /* Find a spot. */
+               for (spot_idx = 0; spot_idx < chunk->capacity; spot_idx++) {
+                       if (!chunk->readers[spot_idx].alloc) {
+                               chunk->readers[spot_idx].alloc = 1;
+                               chunk->used++;
+                               return &chunk->readers[spot_idx];
                        }
                }
        }
@@ -481,7 +480,7 @@ void cleanup_thread(struct registry_chunk *chunk,
        cds_list_del(&rcu_reader_reg->node);
        rcu_reader_reg->tid = 0;
        rcu_reader_reg->alloc = 0;
-       chunk->used -= sizeof(struct lttng_ust_urcu_reader);
+       chunk->used--;
 }
 
 static
@@ -490,9 +489,9 @@ struct registry_chunk *find_chunk(struct lttng_ust_urcu_reader *rcu_reader_reg)
        struct registry_chunk *chunk;
 
        cds_list_for_each_entry(chunk, &registry_arena.chunk_list, node) {
-               if (rcu_reader_reg < (struct lttng_ust_urcu_reader *) &chunk->data[0])
+               if (rcu_reader_reg < (struct lttng_ust_urcu_reader *) &chunk->readers[0])
                        continue;
-               if (rcu_reader_reg >= (struct lttng_ust_urcu_reader *) &chunk->data[chunk->data_len])
+               if (rcu_reader_reg >= (struct lttng_ust_urcu_reader *) &chunk->readers[chunk->capacity])
                        continue;
                return chunk;
        }
@@ -641,8 +640,7 @@ void lttng_ust_urcu_exit(void)
 
                cds_list_for_each_entry_safe(chunk, tmp,
                                &registry_arena.chunk_list, node) {
-                       munmap((void *) chunk, chunk->data_len
-                                       + sizeof(struct registry_chunk));
+                       munmap((void *) chunk, chunk_allocation_size(chunk->capacity));
                }
                CDS_INIT_LIST_HEAD(&registry_arena.chunk_list);
                ret = pthread_key_delete(lttng_ust_urcu_key);
@@ -692,17 +690,18 @@ static
 void lttng_ust_urcu_prune_registry(void)
 {
        struct registry_chunk *chunk;
-       struct lttng_ust_urcu_reader *rcu_reader_reg;
 
        cds_list_for_each_entry(chunk, &registry_arena.chunk_list, node) {
-               for (rcu_reader_reg = (struct lttng_ust_urcu_reader *) &chunk->data[0];
-                               rcu_reader_reg < (struct lttng_ust_urcu_reader *) &chunk->data[chunk->data_len];
-                               rcu_reader_reg++) {
-                       if (!rcu_reader_reg->alloc)
+               size_t spot_idx;
+
+               for (spot_idx = 0; spot_idx < chunk->capacity; spot_idx++) {
+                       struct lttng_ust_urcu_reader *reader = &chunk->readers[spot_idx];
+
+                       if (!reader->alloc)
                                continue;
-                       if (rcu_reader_reg->tid == pthread_self())
+                       if (reader->tid == pthread_self())
                                continue;
-                       cleanup_thread(chunk, rcu_reader_reg);
+                       cleanup_thread(chunk, reader);
                }
        }
 }