From 84213d6c64e93e147c344855162e0e23d64da772 Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=A9mie=20Galarneau?= Date: Thu, 13 Jun 2024 21:52:18 +0000 Subject: [PATCH] sessiond: ust_app: use RAII to manage ust_app_session locking MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Like ltt_session, use the non_copyable_reference util to implement weak locked reference helpers that are used instead of directly manipulating the ust_app_session's pthread mutex. Change-Id: I2c362f2b5944b3d8e089beabc07987b797bb5d36 Signed-off-by: Jérémie Galarneau --- src/bin/lttng-sessiond/ust-app.cpp | 207 ++++++++++++----------------- src/bin/lttng-sessiond/ust-app.hpp | 77 +++++++---- 2 files changed, 131 insertions(+), 153 deletions(-) diff --git a/src/bin/lttng-sessiond/ust-app.cpp b/src/bin/lttng-sessiond/ust-app.cpp index 1e54f3a40..0e40506ff 100644 --- a/src/bin/lttng-sessiond/ust-app.cpp +++ b/src/bin/lttng-sessiond/ust-app.cpp @@ -907,7 +907,7 @@ static void delete_ust_app_session_rcu(struct rcu_head *head) lttng::utils::container_of(head, &ust_app_session::rcu_head); lttng_ht_destroy(ua_sess->channels); - free(ua_sess); + delete ua_sess; } /* @@ -925,7 +925,8 @@ static void delete_ust_app_session(int sock, struct ust_app_session *ua_sess, st LTTNG_ASSERT(ua_sess); ASSERT_RCU_READ_LOCKED(); - pthread_mutex_lock(&ua_sess->lock); + /* Locked for the duration of the function. */ + auto locked_ua_sess = ua_sess->lock(); LTTNG_ASSERT(!ua_sess->deleted); ua_sess->deleted = true; @@ -1004,10 +1005,7 @@ static void delete_ust_app_session(int sock, struct ust_app_session *ua_sess, st LTTNG_ASSERT(!ret); } - pthread_mutex_unlock(&ua_sess->lock); - consumer_output_put(ua_sess->consumer); - call_rcu(&ua_sess->rcu_head, delete_ust_app_session_rcu); } @@ -1168,7 +1166,7 @@ static struct ust_app_session *alloc_ust_app_session() struct ust_app_session *ua_sess; /* Init most of the default value by allocating and zeroing */ - ua_sess = zmalloc(); + ua_sess = new ust_app_session; if (ua_sess == nullptr) { PERROR("malloc"); goto error_free; @@ -1177,7 +1175,6 @@ static struct ust_app_session *alloc_ust_app_session() ua_sess->handle = -1; ua_sess->channels = lttng_ht_new(0, LTTNG_HT_TYPE_STRING); ua_sess->metadata_attr.type = LTTNG_UST_ABI_CHAN_METADATA; - pthread_mutex_init(&ua_sess->lock, nullptr); return ua_sess; @@ -4347,7 +4344,7 @@ static void ust_app_unregister(ust_app& app) * Add session to list for teardown. This is safe since at this point we * are the only one using this list. */ - lttng::pthread::lock_guard ust_app_session_lock(ua_sess->lock); + auto locked_ua_sess = ua_sess->lock(); if (ua_sess->deleted) { continue; @@ -5029,7 +5026,6 @@ static int ust_app_channel_create(struct ltt_ust_session *usess, struct ust_app_channel *ua_chan = nullptr; LTTNG_ASSERT(ua_sess); - ASSERT_LOCKED(ua_sess->lock); if (!strncmp(uchan->name, DEFAULT_METADATA_NAME, sizeof(uchan->name))) { copy_channel_attr_to_ustctl(&ua_sess->metadata_attr, &uchan->attr); @@ -5133,10 +5129,8 @@ int ust_app_enable_event_glb(struct ltt_ust_session *usess, continue; } - pthread_mutex_lock(&ua_sess->lock); - + auto locked_ua_sess = ua_sess->lock(); if (ua_sess->deleted) { - pthread_mutex_unlock(&ua_sess->lock); continue; } @@ -5149,7 +5143,6 @@ int ust_app_enable_event_glb(struct ltt_ust_session *usess, * an application exit. */ if (!ua_chan_node) { - pthread_mutex_unlock(&ua_sess->lock); continue; } @@ -5168,16 +5161,13 @@ int ust_app_enable_event_glb(struct ltt_ust_session *usess, "Skipping app", uevent->attr.name, app->pid); - goto next_app; + continue; } ret = enable_ust_app_event(ua_event, app); if (ret < 0) { - pthread_mutex_unlock(&ua_sess->lock); goto error; } - next_app: - pthread_mutex_unlock(&ua_sess->lock); } } error: @@ -5223,10 +5213,9 @@ int ust_app_create_event_glb(struct ltt_ust_session *usess, continue; } - pthread_mutex_lock(&ua_sess->lock); + auto locked_ua_sess = ua_sess->lock(); - if (ua_sess->deleted) { - pthread_mutex_unlock(&ua_sess->lock); + if (locked_ua_sess->deleted) { continue; } @@ -5239,7 +5228,6 @@ int ust_app_create_event_glb(struct ltt_ust_session *usess, ua_chan = lttng::utils::container_of(ua_chan_node, &ust_app_channel::node); ret = create_ust_app_event(ua_chan, uevent, app); - pthread_mutex_unlock(&ua_sess->lock); if (ret < 0) { if (ret != -LTTNG_UST_ERR_EXIST) { /* Possible value at this point: -ENOMEM. If so, we stop! */ @@ -5271,37 +5259,29 @@ static int ust_app_start_trace(struct ltt_ust_session *usess, struct ust_app *ap DBG("Starting tracing for ust app pid %d", app->pid); lttng::urcu::read_lock_guard read_lock; + const auto update_health_code_on_exit = + lttng::make_scope_exit([]() noexcept { health_code_update(); }); if (!app->compatible) { - goto end; + return 0; } ua_sess = lookup_session_by_app(usess, app); if (ua_sess == nullptr) { /* The session is in teardown process. Ignore and continue. */ - goto end; + return 0; } - pthread_mutex_lock(&ua_sess->lock); + auto locked_ua_sess = ua_sess->lock(); - if (ua_sess->deleted) { - pthread_mutex_unlock(&ua_sess->lock); - goto end; - } - - if (ua_sess->enabled) { - pthread_mutex_unlock(&ua_sess->lock); - goto end; + if (locked_ua_sess->deleted) { + return 0; } - /* Upon restart, we skip the setup, already done */ - if (ua_sess->started) { - goto skip_setup; + if (locked_ua_sess->enabled) { + return 0; } - health_code_update(); - -skip_setup: /* This starts the UST tracing */ pthread_mutex_lock(&app->sock_lock); ret = lttng_ust_ctl_start_session(app->sock, ua_sess->handle); @@ -5311,14 +5291,12 @@ skip_setup: DBG3("UST app start session failed. Application is dead: pid = %d, sock = %d", app->pid, app->sock); - pthread_mutex_unlock(&ua_sess->lock); - goto end; + return 0; } else if (ret == -EAGAIN) { WARN("UST app start session failed. Communication time out: pid = %d, sock = %d", app->pid, app->sock); - pthread_mutex_unlock(&ua_sess->lock); - goto end; + return 0; } else { ERR("UST app start session failed with ret %d: pid = %d, sock = %d", @@ -5326,15 +5304,14 @@ skip_setup: app->pid, app->sock); } - goto error_unlock; + + return -1; } /* Indicate that the session has been started once */ ua_sess->started = true; ua_sess->enabled = true; - pthread_mutex_unlock(&ua_sess->lock); - health_code_update(); /* Quiescent wait after starting trace */ @@ -5358,14 +5335,7 @@ skip_setup: } } -end: - health_code_update(); return 0; - -error_unlock: - pthread_mutex_unlock(&ua_sess->lock); - health_code_update(); - return -1; } /* @@ -5379,21 +5349,22 @@ static int ust_app_stop_trace(struct ltt_ust_session *usess, struct ust_app *app DBG("Stopping tracing for ust app pid %d", app->pid); lttng::urcu::read_lock_guard read_lock; + const auto update_health_code_on_exit = + lttng::make_scope_exit([]() noexcept { health_code_update(); }); if (!app->compatible) { - goto end_no_session; + return 0; } ua_sess = lookup_session_by_app(usess, app); if (ua_sess == nullptr) { - goto end_no_session; + return 0; } - pthread_mutex_lock(&ua_sess->lock); + auto locked_ua_sess = ua_sess->lock(); if (ua_sess->deleted) { - pthread_mutex_unlock(&ua_sess->lock); - goto end_no_session; + return 0; } /* @@ -5403,7 +5374,7 @@ static int ust_app_stop_trace(struct ltt_ust_session *usess, struct ust_app *app * indicate that this is a stop error. */ if (!ua_sess->started) { - goto error_rcu_unlock; + return -1; } health_code_update(); @@ -5417,12 +5388,12 @@ static int ust_app_stop_trace(struct ltt_ust_session *usess, struct ust_app *app DBG3("UST app stop session failed. Application is dead: pid = %d, sock = %d", app->pid, app->sock); - goto end_unlock; + return 0; } else if (ret == -EAGAIN) { WARN("UST app stop session failed. Communication time out: pid = %d, sock = %d", app->pid, app->sock); - goto end_unlock; + return 0; } else { ERR("UST app stop session failed with ret %d: pid = %d, sock = %d", @@ -5430,7 +5401,8 @@ static int ust_app_stop_trace(struct ltt_ust_session *usess, struct ust_app *app app->pid, app->sock); } - goto error_rcu_unlock; + + return -1; } health_code_update(); @@ -5469,16 +5441,7 @@ static int ust_app_stop_trace(struct ltt_ust_session *usess, struct ust_app *app (void) push_metadata(locked_registry, ua_sess->consumer); } -end_unlock: - pthread_mutex_unlock(&ua_sess->lock); -end_no_session: - health_code_update(); return 0; - -error_rcu_unlock: - pthread_mutex_unlock(&ua_sess->lock); - health_code_update(); - return -1; } static int ust_app_flush_app_session(ust_app& app, ust_app_session& ua_sess) @@ -5488,16 +5451,18 @@ static int ust_app_flush_app_session(ust_app& app, ust_app_session& ua_sess) struct ust_app_channel *ua_chan; struct consumer_socket *socket; + const auto update_health_code_on_exit = + lttng::make_scope_exit([]() noexcept { health_code_update(); }); + DBG("Flushing app session buffers for ust app pid %d", app.pid); if (!app.compatible) { - goto end_not_compatible; + return 0; } - pthread_mutex_lock(&ua_sess.lock); - - if (ua_sess.deleted) { - goto end_deleted; + const auto locked_ua_sess = ua_sess.lock(); + if (locked_ua_sess->deleted) { + return 0; } health_code_update(); @@ -5529,13 +5494,6 @@ static int ust_app_flush_app_session(ust_app& app, ust_app_session& ua_sess) break; } - health_code_update(); - -end_deleted: - pthread_mutex_unlock(&ua_sess.lock); - -end_not_compatible: - health_code_update(); return retval; } @@ -5628,15 +5586,16 @@ static int ust_app_clear_quiescent_app_session(struct ust_app *app, struct ust_a DBG("Clearing stream quiescent state for ust app pid %d", app->pid); lttng::urcu::read_lock_guard read_lock; + const auto update_health_code_on_exit = + lttng::make_scope_exit([]() noexcept { health_code_update(); }); if (!app->compatible) { - goto end_not_compatible; + return 0; } - pthread_mutex_lock(&ua_sess->lock); - - if (ua_sess->deleted) { - goto end_unlock; + const auto locked_ua_sess = ua_sess->lock(); + if (locked_ua_sess->deleted) { + return 0; } health_code_update(); @@ -5644,8 +5603,7 @@ static int ust_app_clear_quiescent_app_session(struct ust_app *app, struct ust_a socket = consumer_find_socket_by_bitness(app->abi.bits_per_long, ua_sess->consumer); if (!socket) { ERR("Failed to find consumer (%" PRIu32 ") socket", app->abi.bits_per_long); - ret = -1; - goto end_unlock; + return -1; } /* Clear quiescent state. */ @@ -5668,13 +5626,6 @@ static int ust_app_clear_quiescent_app_session(struct ust_app *app, struct ust_a break; } - health_code_update(); - -end_unlock: - pthread_mutex_unlock(&ua_sess->lock); - -end_not_compatible: - health_code_update(); return ret; } @@ -6187,14 +6138,14 @@ static void ust_app_synchronize(struct ltt_ust_session *usess, struct ust_app *a ret = find_or_create_ust_app_session(usess, app, &ua_sess, nullptr); if (ret < 0) { /* Tracer is probably gone or ENOMEM. */ - goto end; + return; } LTTNG_ASSERT(ua_sess); - pthread_mutex_lock(&ua_sess->lock); - if (ua_sess->deleted) { - goto deleted_session; + const auto locked_ua_sess = ua_sess->lock(); + if (locked_ua_sess->deleted) { + return; } { @@ -6218,11 +6169,6 @@ static void ust_app_synchronize(struct ltt_ust_session *usess, struct ust_app *a usess->id); } } - -deleted_session: - pthread_mutex_unlock(&ua_sess->lock); -end: - return; } static void ust_app_global_destroy(struct ltt_ust_session *usess, struct ust_app *app) @@ -6354,10 +6300,8 @@ int ust_app_add_ctx_channel_glb(struct ltt_ust_session *usess, continue; } - pthread_mutex_lock(&ua_sess->lock); - - if (ua_sess->deleted) { - pthread_mutex_unlock(&ua_sess->lock); + const auto locked_ua_sess = ua_sess->lock(); + if (locked_ua_sess->deleted) { continue; } @@ -6365,15 +6309,13 @@ int ust_app_add_ctx_channel_glb(struct ltt_ust_session *usess, lttng_ht_lookup(ua_sess->channels, (void *) uchan->name, &uiter); ua_chan_node = lttng_ht_iter_get_node(&uiter); if (ua_chan_node == nullptr) { - goto next_app; + continue; } ua_chan = caa_container_of(ua_chan_node, struct ust_app_channel, node); ret = create_ust_app_channel_context(ua_chan, &uctx->ctx, app); if (ret < 0) { - goto next_app; + continue; } - next_app: - pthread_mutex_unlock(&ua_sess->lock); } } @@ -7498,28 +7440,23 @@ static int ust_app_regenerate_statedump(struct ltt_ust_session *usess, struct us DBG("Regenerating the metadata for ust app pid %d", app->pid); lttng::urcu::read_lock_guard read_lock; + const auto update_health_code_on_exit = + lttng::make_scope_exit([]() noexcept { health_code_update(); }); ua_sess = lookup_session_by_app(usess, app); if (ua_sess == nullptr) { /* The session is in teardown process. Ignore and continue. */ - goto end; + return 0; } - pthread_mutex_lock(&ua_sess->lock); - - if (ua_sess->deleted) { - goto end_unlock; + const auto locked_ua_sess = ua_sess->lock(); + if (locked_ua_sess->deleted) { + return 0; } pthread_mutex_lock(&app->sock_lock); ret = lttng_ust_ctl_regenerate_statedump(app->sock, ua_sess->handle); pthread_mutex_unlock(&app->sock_lock); - -end_unlock: - pthread_mutex_unlock(&ua_sess->lock); - -end: - health_code_update(); return ret; } @@ -8128,3 +8065,25 @@ void ust_app_put(struct ust_app *app) urcu_ref_put(&app->ref, ust_app_release); } + +ust_app_session::const_locked_weak_ref ust_app_session::lock() const noexcept +{ + pthread_mutex_lock(&_lock); + return ust_app_session::const_locked_weak_ref(*this); +} + +ust_app_session::locked_weak_ref ust_app_session::lock() noexcept +{ + pthread_mutex_lock(&_lock); + return ust_app_session::locked_weak_ref(*this); +} + +void ust_app_session::_const_session_unlock(const ust_app_session *session) +{ + pthread_mutex_unlock(&session->_lock); +} + +void ust_app_session::_session_unlock(ust_app_session *session) +{ + pthread_mutex_unlock(&session->_lock); +} diff --git a/src/bin/lttng-sessiond/ust-app.hpp b/src/bin/lttng-sessiond/ust-app.hpp index f292753c0..d1551a335 100644 --- a/src/bin/lttng-sessiond/ust-app.hpp +++ b/src/bin/lttng-sessiond/ust-app.hpp @@ -17,6 +17,7 @@ #include #include +#include #include #include @@ -190,59 +191,77 @@ struct ust_app_channel { }; struct ust_app_session { - /* - * Lock protecting this session's ust app interaction. Held - * across command send/recv to/from app. Never nests within the - * session registry lock. - */ - pthread_mutex_t lock; - - bool enabled; +private: + static void _session_unlock(ust_app_session *session); + static void _const_session_unlock(const ust_app_session *session); + +public: + using locked_weak_ref = lttng::non_copyable_reference< + ust_app_session, + lttng::memory::create_deleter_class::deleter>; + using const_locked_weak_ref = lttng::non_copyable_reference< + const ust_app_session, + lttng::memory::create_deleter_class::deleter>; + + ust_app_session::const_locked_weak_ref lock() const noexcept; + ust_app_session::locked_weak_ref lock() noexcept; + + bool enabled = false; /* started: has the session been in started state at any time ? */ - bool started; /* allows detection of start vs restart. */ - int handle; /* used has unique identifier for app session */ + bool started = false; /* allows detection of start vs restart. */ + int handle = 0; /* used has unique identifier for app session */ - bool deleted; /* Session deleted flag. Check with lock held. */ + bool deleted = false; /* Session deleted flag. Check with lock held. */ /* * Tracing session ID. Multiple ust app session can have the same tracing * session id making this value NOT unique to the object. */ - uint64_t tracing_id; - uint64_t id; /* Unique session identifier */ - struct lttng_ht *channels; /* Registered channels */ - struct lttng_ht_node_u64 node; + uint64_t tracing_id = 0; + uint64_t id = 0; /* Unique session identifier */ + struct lttng_ht *channels = nullptr; /* Registered channels */ + struct lttng_ht_node_u64 node = {}; /* * Node indexed by UST session object descriptor (handle). Stored in the * ust_sessions_objd hash table in the ust_app object. */ - struct lttng_ht_node_ulong ust_objd_node; + struct lttng_ht_node_ulong ust_objd_node = {}; /* Starts with 'ust'; no leading slash. */ - char path[PATH_MAX]; + char path[PATH_MAX] = {}; /* UID/GID of the application owning the session */ - struct lttng_credentials real_credentials; + struct lttng_credentials real_credentials = {}; /* Effective UID and GID. Same as the tracing session. */ - struct lttng_credentials effective_credentials; - struct cds_list_head teardown_node; + struct lttng_credentials effective_credentials = {}; + struct cds_list_head teardown_node = {}; /* * Once at least *one* session is created onto the application, the * corresponding consumer is set so we can use it on unregistration. */ - struct consumer_output *consumer; - enum lttng_buffer_type buffer_type; + struct consumer_output *consumer = nullptr; + enum lttng_buffer_type buffer_type = LTTNG_BUFFER_PER_PID; /* ABI of the session. Same value as the application. */ - uint32_t bits_per_long; + uint32_t bits_per_long = 0; /* For delayed reclaim */ - struct rcu_head rcu_head; + struct rcu_head rcu_head = {}; /* If the channel's streams have to be outputed or not. */ - unsigned int output_traces; - unsigned int live_timer_interval; /* usec */ + unsigned int output_traces = 0; + unsigned int live_timer_interval = 0; /* usec */ /* Metadata channel attributes. */ - struct lttng_ust_ctl_consumer_channel_attr metadata_attr; + struct lttng_ust_ctl_consumer_channel_attr metadata_attr = {}; - char root_shm_path[PATH_MAX]; - char shm_path[PATH_MAX]; + char root_shm_path[PATH_MAX] = {}; + char shm_path[PATH_MAX] = {}; + +private: + /* + * Lock protecting this session's ust app interaction. Held + * across command send/recv to/from app. Never nests within the + * session registry lock. + */ + mutable pthread_mutex_t _lock = PTHREAD_MUTEX_INITIALIZER; }; /* -- 2.34.1