From 6c12af2c67e09f93ae37bbd7aace782bfb81cd78 Mon Sep 17 00:00:00 2001 From: Mathieu Desnoyers Date: Thu, 21 Sep 2023 06:29:15 +0100 Subject: [PATCH] Extract callbacks and enabled state to non-packed structure Atomic load/store should be performed on non-packed structure. Signed-off-by: Mathieu Desnoyers --- include/side/trace.h | 29 ++++++++++++++++++++--------- src/side.c | 42 +++++++++++++++++++++--------------------- 2 files changed, 41 insertions(+), 30 deletions(-) diff --git a/include/side/trace.h b/include/side/trace.h index f7bf901..8e0604d 100644 --- a/include/side/trace.h +++ b/include/side/trace.h @@ -650,19 +650,27 @@ struct side_tracer_dynamic_struct_visitor_ctx { void *priv; /* Private tracer context. */ } SIDE_PACKED; +/* + * This structure is _not_ packed to allow atomic operations on its + * fields. + */ +struct side_event_state { + uintptr_t enabled; + const struct side_callback *callbacks; + uint32_t nr_callbacks; +}; + struct side_event_description { - uintptr_t *enabled; + struct side_event_state *state; const char *provider_name; const char *event_name; const struct side_event_field *fields; const struct side_attr *attr; - const struct side_callback *callbacks; uint64_t flags; uint32_t version; uint32_t loglevel; /* enum side_loglevel */ uint32_t nr_fields; uint32_t nr_attr; - uint32_t nr_callbacks; } SIDE_PACKED; /* Event and type attributes */ @@ -1756,7 +1764,7 @@ struct side_event_description { #define side_arg_list(...) __VA_ARGS__ #define side_event_cond(_identifier) \ - if (side_unlikely(__atomic_load_n(&side_event_enable__##_identifier, \ + if (side_unlikely(__atomic_load_n(&side_event_state__##_identifier.enabled, \ __ATOMIC_RELAXED))) #define side_event_call(_identifier, _sav) \ @@ -1795,21 +1803,24 @@ struct side_event_description { side_event_call_variadic(_identifier, SIDE_PARAM(_sav), SIDE_PARAM(_var), SIDE_PARAM_SELECT_ARG1(_, ##_attr, side_attr_list())) #define _side_define_event(_linkage, _identifier, _provider, _event, _loglevel, _fields, _flags, _attr...) \ - _linkage uintptr_t side_event_enable__##_identifier __attribute__((section("side_event_enable"))); \ + _linkage struct side_event_state __attribute__((section("side_event_state"))) \ + side_event_state__##_identifier = { \ + .enabled = 0, \ + .callbacks = &side_empty_callback, \ + .nr_callbacks = 0, \ + }; \ _linkage struct side_event_description __attribute__((section("side_event_description"))) \ _identifier = { \ - .enabled = &(side_event_enable__##_identifier), \ + .state = &(side_event_state__##_identifier), \ .provider_name = _provider, \ .event_name = _event, \ .fields = _fields, \ .attr = SIDE_PARAM_SELECT_ARG1(_, ##_attr, side_attr_list()), \ - .callbacks = &side_empty_callback, \ .flags = (_flags), \ .version = 0, \ .loglevel = _loglevel, \ .nr_fields = SIDE_ARRAY_SIZE(SIDE_PARAM(_fields)), \ .nr_attr = SIDE_ARRAY_SIZE(SIDE_PARAM_SELECT_ARG1(_, ##_attr, side_attr_list())), \ - .nr_callbacks = 0, \ }; \ static const struct side_event_description *side_event_ptr__##_identifier \ __attribute__((section("side_event_description_ptr"), used)) = &(_identifier); @@ -1839,7 +1850,7 @@ struct side_event_description { _loglevel, SIDE_PARAM(_fields), SIDE_EVENT_FLAG_VARIADIC, SIDE_PARAM_SELECT_ARG1(_, ##_attr, side_attr_list())) #define side_declare_event(_identifier) \ - extern uintptr_t side_event_enable_##_identifier; \ + extern struct side_event_state side_event_state_##_identifier; \ extern struct side_event_description _identifier #ifdef __cplusplus diff --git a/src/side.c b/src/side.c index 3b46f0e..3c0f1b2 100644 --- a/src/side.c +++ b/src/side.c @@ -76,12 +76,12 @@ void side_call(const struct side_event_description *desc, const struct side_arg_ printf("ERROR: unexpected variadic event description\n"); abort(); } - enabled = __atomic_load_n(desc->enabled, __ATOMIC_RELAXED); + enabled = __atomic_load_n(&desc->state->enabled, __ATOMIC_RELAXED); if (side_unlikely(enabled & SIDE_EVENT_ENABLED_KERNEL_USER_EVENT_MASK)) { // TODO: call kernel write. } side_rcu_read_begin(&rcu_gp, &rcu_read_state); - for (side_cb = side_rcu_dereference(desc->callbacks); side_cb->u.call != NULL; side_cb++) + for (side_cb = side_rcu_dereference(desc->state->callbacks); side_cb->u.call != NULL; side_cb++) side_cb->u.call(desc, side_arg_vec, side_cb->priv); side_rcu_read_end(&rcu_gp, &rcu_read_state); } @@ -102,12 +102,12 @@ void side_call_variadic(const struct side_event_description *desc, printf("ERROR: unexpected non-variadic event description\n"); abort(); } - enabled = __atomic_load_n(desc->enabled, __ATOMIC_RELAXED); + enabled = __atomic_load_n(&desc->state->enabled, __ATOMIC_RELAXED); if (side_unlikely(enabled & SIDE_EVENT_ENABLED_KERNEL_USER_EVENT_MASK)) { // TODO: call kernel write. } side_rcu_read_begin(&rcu_gp, &rcu_read_state); - for (side_cb = side_rcu_dereference(desc->callbacks); side_cb->u.call_variadic != NULL; side_cb++) + for (side_cb = side_rcu_dereference(desc->state->callbacks); side_cb->u.call_variadic != NULL; side_cb++) side_cb->u.call_variadic(desc, side_arg_vec, var_struct, side_cb->priv); side_rcu_read_end(&rcu_gp, &rcu_read_state); } @@ -119,7 +119,7 @@ const struct side_callback *side_tracer_callback_lookup( { const struct side_callback *cb; - for (cb = desc->callbacks; cb->u.call != NULL; cb++) { + for (cb = desc->state->callbacks; cb->u.call != NULL; cb++) { if ((void *) cb->u.call == call && cb->priv == priv) return cb; } @@ -141,7 +141,7 @@ int _side_tracer_callback_register(struct side_event_description *desc, if (!initialized) side_init(); pthread_mutex_lock(&side_lock); - old_nr_cb = desc->nr_callbacks; + old_nr_cb = desc->state->nr_callbacks; if (old_nr_cb == UINT32_MAX) { ret = SIDE_ERROR_INVAL; goto unlock; @@ -151,7 +151,7 @@ int _side_tracer_callback_register(struct side_event_description *desc, ret = SIDE_ERROR_EXIST; goto unlock; } - old_cb = (struct side_callback *) desc->callbacks; + old_cb = (struct side_callback *) desc->state->callbacks; /* old_nr_cb + 1 (new cb) + 1 (NULL) */ new_cb = (struct side_callback *) calloc(old_nr_cb + 2, sizeof(struct side_callback)); if (!new_cb) { @@ -166,14 +166,14 @@ int _side_tracer_callback_register(struct side_event_description *desc, new_cb[old_nr_cb].u.call = (side_tracer_callback_func) call; new_cb[old_nr_cb].priv = priv; - side_rcu_assign_pointer(desc->callbacks, new_cb); + side_rcu_assign_pointer(desc->state->callbacks, new_cb); side_rcu_wait_grace_period(&rcu_gp); if (old_nr_cb) free(old_cb); - desc->nr_callbacks++; + desc->state->nr_callbacks++; /* Increment concurrently with kernel setting the top bits. */ if (!old_nr_cb) - (void) __atomic_add_fetch(desc->enabled, 1, __ATOMIC_RELAXED); + (void) __atomic_add_fetch(&desc->state->enabled, 1, __ATOMIC_RELAXED); unlock: pthread_mutex_unlock(&side_lock); return ret; @@ -218,12 +218,12 @@ static int _side_tracer_callback_unregister(struct side_event_description *desc, ret = SIDE_ERROR_NOENT; goto unlock; } - old_nr_cb = desc->nr_callbacks; - old_cb = (struct side_callback *) desc->callbacks; + old_nr_cb = desc->state->nr_callbacks; + old_cb = (struct side_callback *) desc->state->callbacks; if (old_nr_cb == 1) { new_cb = (struct side_callback *) &side_empty_callback; } else { - pos_idx = cb_pos - desc->callbacks; + pos_idx = cb_pos - desc->state->callbacks; /* Remove entry at pos_idx. */ /* old_nr_cb - 1 (removed cb) + 1 (NULL) */ new_cb = (struct side_callback *) calloc(old_nr_cb, sizeof(struct side_callback)); @@ -234,13 +234,13 @@ static int _side_tracer_callback_unregister(struct side_event_description *desc, memcpy(new_cb, old_cb, pos_idx); memcpy(&new_cb[pos_idx], &old_cb[pos_idx + 1], old_nr_cb - pos_idx - 1); } - side_rcu_assign_pointer(desc->callbacks, new_cb); + side_rcu_assign_pointer(desc->state->callbacks, new_cb); side_rcu_wait_grace_period(&rcu_gp); free(old_cb); - desc->nr_callbacks--; + desc->state->nr_callbacks--; /* Decrement concurrently with kernel setting the top bits. */ if (old_nr_cb == 1) - (void) __atomic_add_fetch(desc->enabled, -1, __ATOMIC_RELAXED); + (void) __atomic_add_fetch(&desc->state->enabled, -1, __ATOMIC_RELAXED); unlock: pthread_mutex_unlock(&side_lock); return ret; @@ -294,20 +294,20 @@ struct side_events_register_handle *side_events_register(struct side_event_descr static void side_event_remove_callbacks(struct side_event_description *desc) { - uint32_t nr_cb = desc->nr_callbacks; + uint32_t nr_cb = desc->state->nr_callbacks; struct side_callback *old_cb; if (!nr_cb) return; - old_cb = (struct side_callback *) desc->callbacks; - (void) __atomic_add_fetch(desc->enabled, -1, __ATOMIC_RELAXED); + old_cb = (struct side_callback *) desc->state->callbacks; + (void) __atomic_add_fetch(&desc->state->enabled, -1, __ATOMIC_RELAXED); /* * Setting the state back to 0 cb and empty callbacks out of * caution. This should not matter because instrumentation is * unreachable. */ - desc->nr_callbacks = 0; - side_rcu_assign_pointer(desc->callbacks, &side_empty_callback); + desc->state->nr_callbacks = 0; + side_rcu_assign_pointer(desc->state->callbacks, &side_empty_callback); /* * No need to wait for grace period because instrumentation is * unreachable. -- 2.34.1