ipc/android: Switch to using client push mutex to avoid cond var wait in server.

doc/ipc.md

@@ -134,17 +134,35 @@ We have the following design goals/constraints:
   - The mainloop is high rate (compositor rate) and new client connections are
     relatively infrequent.
 
-The IPC service creates a pipe as well as some state variables, a mutex, and a
-condition variable. When the JVM Service code has a new client, it calls
-`ipc_server_mainloop_add_fd()` to pass the fd in. It writes the FD number to the
-pipe, then waits on the condition variable to see either that number or the
-special "shutting down" sentinel value in the `last_accepted_fd` variable. If it
-sees the fd number, that indicates that the other side of the communication (the
-mainloop) has taken ownership of the fd and will handle closing it. If it sees
-the sentinel value, or has an error at some point, it assumes that ownership is
-retained and it should close the fd itself.
+The IPC service creates a pipe as well as some state variables, two mutexes, and a
+condition variable.
+
+When the JVM Service code has a new client, it calls
+`ipc_server_mainloop_add_fd()` to pass the FD in. It takes two mutexes, in
+order: `ipc_server_mainloop::client_push_mutex` and
+`ipc_server_mainloop::accept_mutex`. The purpose of
+`ipc_server_mainloop::client_push_mutex` is to allow only one client into the
+client-acceptance handshake at a time, so that no acknowledgement of client
+accept is lost. Once those two mutexes are locked,
+`ipc_server_mainloop_add_fd()` writes the FD number to the pipe. Then, it waits
+on the condition variable (releasing `accept_mutex`) to see either that FD
+number or the special "shutting down" sentinel value in the `last_accepted_fd`
+variable. If it sees the FD number, that indicates that the other side of the
+communication (the mainloop) has taken ownership of the FD and will handle
+closing it. If it sees the sentinel value, or has an error at some point, it
+assumes that ownership is retained and it should close the FD itself.
 
 The other side of the communication works as follows: epoll is used to check if
-there is new data waiting on the pipe. If so, the lock is taken, and an fd
-number is read from the pipe. A client thread is launched for that fd, then the
-`last_accepted_fd` variable is updated and the condition variable signalled.
+there is new data waiting on the pipe. If so, the
+`ipc_server_mainloop::accept_mutex` lock is taken, and an FD number is read from
+the pipe. A client thread is launched for that FD, then the `last_accepted_fd`
+variable is updated and the `ipc_server_mainloop::accept_cond` condition
+variable signalled.
+
+The initial plan required that the server also wait on
+`ipc_server_mainloop::accept_cond` for the `last_accepted_fd` to be reset back
+to `0` by the acknowledged client, thus preventing losing acknowledgements.
+However, it is undesirable for the clients to be able to block the
+compositor/server, so this wait was considered not acceptable. Instead, the
+`ipc_server_mainloop::client_push_mutex` is used so that at most one
+un-acknowledged client may have written to the pipe at any given time.

src/xrt/ipc/server/ipc_server.h

@@ -154,30 +154,81 @@ struct ipc_device
  * Contents are essentially implementation details, but are listed in full here so they may be included by value in the
  * main ipc_server struct.
  *
+ * @see ipc_design
+ *
  * @ingroup ipc_server
  */
 struct ipc_server_mainloop
 {
-#if defined(XRT_OS_ANDROID)
+#if defined(XRT_OS_ANDROID) || defined(XRT_DOXYGEN)
+	/*!
+	 * @name Android Mainloop Members
+	 * @{
+	 */
 	//! For waiting on various events in the main thread.
 	int epoll_fd;
 
 	//! File descriptor for the read end of our pipe for submitting new clients
 	int pipe_read;
 
-	//! File descriptor for the write end of our pipe for submitting new clients
+	/*!
+	 * File descriptor for the write end of our pipe for submitting new clients
+	 *
+	 * Must hold client_push_mutex while writing.
+	 */
 	int pipe_write;
 
-	//! The last client fd we accepted, to unblock the right client.
+	/*!
+	 * Mutex for being able to register oneself as a new client.
+	 *
+	 * Locked only by threads in `ipc_server_mainloop_add_fd()`.
+	 *
+	 * This must be locked first, and kept locked the entire time a client is attempting to register and wait for
+	 * confirmation. It ensures no acknowledgements of acceptance are lost and moves the overhead of ensuring this
+	 * to the client thread.
+	 */
+	pthread_mutex_t client_push_mutex;
+
+
+	/*!
+	 * The last client fd we accepted, to acknowledge client acceptance.
+	 *
+	 * Also used as a sentinel during shutdown.
+	 *
+	 * Must hold accept_mutex while writing.
+	 */
 	int last_accepted_fd;
 
-	//! Mutex for accepting clients
-	pthread_mutex_t accept_mutex;
-
-	//! Condition variable for accepting clients
+	/*!
+	 * Condition variable for accepting clients.
+	 *
+	 * Signalled when @ref last_accepted_fd is updated.
+	 *
+	 * Associated with @ref accept_mutex
+	 */
 	pthread_cond_t accept_cond;
 
-#elif defined(XRT_OS_LINUX)
+	/*!
+	 * Mutex for accepting clients.
+	 *
+	 * Locked by both clients and server: that is, by threads in `ipc_server_mainloop_add_fd()` and in the
+	 * server/compositor thread in an implementation function called from `ipc_server_mainloop_poll()`.
+	 *
+	 * Exists to operate in conjunction with @ref accept_cond - it exists to make sure that the client can be woken
+	 * when the server accepts it.
+	 */
+	pthread_mutex_t accept_mutex;
+
+
+	/*! @} */
+#define XRT_IPC_GOT_IMPL
+#endif
+
+#if (defined(XRT_OS_LINUX) && !defined(XRT_OS_ANDROID)) || defined(XRT_DOXYGEN)
+	/*!
+	 * @name Desktop Linux Mainloop Members
+	 * @{
+	 */
 
 	//! Socket that we accept connections on.
 	int listen_socket;
@@ -190,7 +241,13 @@ struct ipc_server_mainloop
 
 	//! The socket filename we bound to, if any.
 	char *socket_filename;
-#else
+
+	/*! @} */
+
+#define XRT_IPC_GOT_IMPL
+#endif
+
+#ifndef XRT_IPC_GOT_IMPL
 #error "Need port"
 #endif
 };

src/xrt/ipc/server/ipc_server_mainloop_android.c

@@ -86,31 +86,19 @@ static void
 handle_listen(struct ipc_server *vs, struct ipc_server_mainloop *ml)
 {
 	int newfd = 0;
+	pthread_mutex_lock(&ml->accept_mutex);
 	if (read(ml->pipe_read, &newfd, sizeof(newfd)) == sizeof(newfd)) {
-
-		pthread_mutex_lock(&ml->accept_mutex);
-		// Don't overwrite some client's notification: wait till this goes back to 0.
-		while (ml->last_accepted_fd != 0) {
-			int ret = pthread_cond_wait(&ml->accept_cond, &ml->accept_mutex);
-			if (ret < 0) {
-				U_LOG_E("pthread_cond_wait failed with '%i', rejecting client fd '%i' and failing.",
-				        ret, newfd);
-				close(newfd);
-				pthread_mutex_unlock(&ml->accept_mutex);
-				ipc_server_handle_failure(vs);
-				return;
-			}
-		}
-		// When we get here, everybody we've tried to notify has received their notification.
+		// client_push_mutex should prevent dropping acknowledgements
+		assert(ml->last_accepted_fd == 0);
 		// Release the thread that gave us this fd.
 		ml->last_accepted_fd = newfd;
 		pthread_cond_broadcast(&ml->accept_cond);
-		pthread_mutex_unlock(&ml->accept_mutex);
 	} else {
 		U_LOG_E("error on pipe read");
 		ipc_server_handle_failure(vs);
 		return;
 	}
+	pthread_mutex_unlock(&ml->accept_mutex);
 }
 
 #define NUM_POLL_EVENTS 8
@@ -190,6 +178,10 @@ ipc_server_mainloop_deinit(struct ipc_server_mainloop *ml)
 int
 ipc_server_mainloop_add_fd(struct ipc_server *vs, struct ipc_server_mainloop *ml, int newfd)
 {
+	// Take the client push lock here, serializing clients attempting to connect.
+	// This one won't be unlocked when waiting on the condition variable, ensuring we keep other clients out.
+	pthread_mutex_lock(&ml->client_push_mutex);
+
 	// Take the lock here, so we don't accidentally miss our fd being accepted.
 	pthread_mutex_lock(&ml->accept_mutex);
 
@@ -197,8 +189,7 @@ ipc_server_mainloop_add_fd(struct ipc_server *vs, struct ipc_server_mainloop *ml
 	int ret = write(ml->pipe_write, &newfd, sizeof(newfd));
 	if (ret < 0) {
 		U_LOG_E("write to pipe failed with '%i'.", ret);
-		pthread_mutex_unlock(&ml->accept_mutex);
-		return ret;
+		goto exit;
 	}
 
 	// Normal looping on the condition variable's condition.
@@ -206,12 +197,20 @@ ipc_server_mainloop_add_fd(struct ipc_server *vs, struct ipc_server_mainloop *ml
 		ret = pthread_cond_wait(&ml->accept_cond, &ml->accept_mutex);
 		if (ret < 0) {
 			U_LOG_E("pthread_cond_wait failed with '%i'.", ret);
-			pthread_mutex_unlock(&ml->accept_mutex);
-			return ret;
+			goto exit;
 		}
 	}
-	// OK, we have now been accepted. Zero out the last accepted fd to avoid confusing any other thread.
-	ml->last_accepted_fd = 0;
+	if (ml->last_accepted_fd == SHUTTING_DOWN) {
+		// we actually didn't hand off our client, we should error out.
+		U_LOG_W("server was shutting down.");
+		ret = -1;
+	} else {
+		// OK, we have now been accepted. Zero out the last accepted fd.
+		ml->last_accepted_fd = 0;
+		ret = 0;
+	}
+exit:
 	pthread_mutex_unlock(&ml->accept_mutex);
-	return 0;
+	pthread_mutex_unlock(&ml->client_push_mutex);
+	return ret;
 }

src/xrt/ipc/server/ipc_server_mainloop_android.h

@@ -17,6 +17,9 @@ extern "C" {
 
 /*!
  * Pass an fd for a new client to the mainloop.
+ *
+ * @see ipc_design
+ * @public @memberof ipc_server_mainloop
  */
 int
 ipc_server_mainloop_add_fd(struct ipc_server *vs, struct ipc_server_mainloop *ml, int newfd);