From c06fdd9537c38a3c77512a4439fb6f292b2ccfb9 Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=A9mie=20Galarneau?= Date: Thu, 19 Sep 2019 13:22:17 -0400 Subject: [PATCH] relayd: move relay_session locking outside of make_viewer_streams MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit The locking period of the relay_session is slightly extended outside of make_viewer_streams(). This makes a follow-up change easier to review, but is also a clean-up cleaner as some of the session's attribute are accessed without holding the lock (`live_timer` and `id`) assuming (correctly, for the moment) that they don't change over the lifetime of the relayd_session. It also allows us to move the relayd_session locking logic from viewer_session_attach(). The signature of send_viewer_streams() is changed to accept the session's id rather than a pointer to the relay_session, therefore making the locking scope simpler to follow. Changing this makes follow-up changes easier to review. Signed-off-by: Jérémie Galarneau --- src/bin/lttng-relayd/live.c | 39 +++++++++++++++------------ src/bin/lttng-relayd/viewer-session.c | 5 ++-- 2 files changed, 24 insertions(+), 20 deletions(-) diff --git a/src/bin/lttng-relayd/live.c b/src/bin/lttng-relayd/live.c index 84fa14351..dc8238c94 100644 --- a/src/bin/lttng-relayd/live.c +++ b/src/bin/lttng-relayd/live.c @@ -197,7 +197,7 @@ end: */ static ssize_t send_viewer_streams(struct lttcomm_sock *sock, - struct relay_session *session, unsigned int ignore_sent_flag) + uint64_t session_id, unsigned int ignore_sent_flag) { ssize_t ret; struct lttng_viewer_stream send_stream; @@ -218,7 +218,7 @@ ssize_t send_viewer_streams(struct lttcomm_sock *sock, pthread_mutex_lock(&vstream->stream->lock); /* Ignore if not the same session. */ - if (vstream->stream->trace->session->id != session->id || + if (vstream->stream->trace->session->id != session_id || (!ignore_sent_flag && vstream->sent_flag)) { pthread_mutex_unlock(&vstream->stream->lock); viewer_stream_put(vstream); @@ -271,6 +271,9 @@ end_unlock: * viewer stream of the session, the number of unsent stream and the number of * stream created. Those counters can be NULL and thus will be ignored. * + * session must be locked to ensure that we see either none or all initial + * streams for a session, but no intermediate state.. + * * Return 0 on success or else a negative value. */ static @@ -283,12 +286,7 @@ int make_viewer_streams(struct relay_session *session, struct ctf_trace *ctf_trace; assert(session); - - /* - * Hold the session lock to ensure that we see either none or - * all initial streams for a session, but no intermediate state. - */ - pthread_mutex_lock(&session->lock); + ASSERT_LOCKED(session->lock); if (session->connection_closed) { *closed = true; @@ -376,7 +374,6 @@ int make_viewer_streams(struct relay_session *session, error_unlock: rcu_read_unlock(); - pthread_mutex_unlock(&session->lock); return ret; } @@ -946,8 +943,10 @@ int viewer_get_new_streams(struct relay_connection *conn) send_streams = 1; response.status = htobe32(LTTNG_VIEWER_NEW_STREAMS_OK); + pthread_mutex_lock(&session->lock); ret = make_viewer_streams(session, LTTNG_VIEWER_SEEK_LAST, &nb_total, &nb_unsent, &nb_created, &closed); + pthread_mutex_unlock(&session->lock); if (ret < 0) { goto end_put_session; } @@ -988,7 +987,7 @@ send_reply: * streams that were not sent from that point will be sent to * the viewer. */ - ret = send_viewer_streams(conn->sock, session, 0); + ret = send_viewer_streams(conn->sock, session_id, 0); if (ret < 0) { goto end_put_session; } @@ -1015,6 +1014,7 @@ int viewer_attach_session(struct relay_connection *conn) struct lttng_viewer_attach_session_response response; struct relay_session *session = NULL; bool closed = false; + uint64_t session_id; assert(conn); @@ -1026,6 +1026,7 @@ int viewer_attach_session(struct relay_connection *conn) goto error; } + session_id = be64toh(request.session_id); health_code_update(); memset(&response, 0, sizeof(response)); @@ -1036,16 +1037,15 @@ int viewer_attach_session(struct relay_connection *conn) goto send_reply; } - session = session_get_by_id(be64toh(request.session_id)); + session = session_get_by_id(session_id); if (!session) { - DBG("Relay session %" PRIu64 " not found", - (uint64_t) be64toh(request.session_id)); + DBG("Relay session %" PRIu64 " not found", session_id); response.status = htobe32(LTTNG_VIEWER_ATTACH_UNK); goto send_reply; } - DBG("Attach session ID %" PRIu64 " received", - (uint64_t) be64toh(request.session_id)); + DBG("Attach session ID %" PRIu64 " received", session_id); + pthread_mutex_lock(&session->lock); if (session->live_timer == 0) { DBG("Not live session"); response.status = htobe32(LTTNG_VIEWER_ATTACH_NOT_LIVE); @@ -1078,8 +1078,12 @@ int viewer_attach_session(struct relay_connection *conn) if (ret < 0) { goto end_put_session; } - response.streams_count = htobe32(nb_streams); + pthread_mutex_unlock(&session->lock); + session_put(session); + session = NULL; + + response.streams_count = htobe32(nb_streams); /* * If the session is closed when the viewer is attaching, it * means some of the streams may have been concurrently removed, @@ -1111,13 +1115,14 @@ send_reply: } /* Send stream and ignore the sent flag. */ - ret = send_viewer_streams(conn->sock, session, 1); + ret = send_viewer_streams(conn->sock, session_id, 1); if (ret < 0) { goto end_put_session; } end_put_session: if (session) { + pthread_mutex_unlock(&session->lock); session_put(session); } error: diff --git a/src/bin/lttng-relayd/viewer-session.c b/src/bin/lttng-relayd/viewer-session.c index a4b859726..5f9c7c576 100644 --- a/src/bin/lttng-relayd/viewer-session.c +++ b/src/bin/lttng-relayd/viewer-session.c @@ -47,12 +47,13 @@ int viewer_session_attach(struct relay_viewer_session *vsession, { int ret = 0; + ASSERT_LOCKED(session->lock); + /* Will not fail, as per the ownership guarantee. */ if (!session_get(session)) { ret = -1; goto end; } - pthread_mutex_lock(&session->lock); if (session->viewer_attached) { ret = -1; } else { @@ -69,8 +70,6 @@ int viewer_session_attach(struct relay_viewer_session *vsession, /* Put our local ref. */ session_put(session); } - /* Safe since we know the session exists. */ - pthread_mutex_unlock(&session->lock); end: return ret; } -- 2.34.1