From 6e303ecd5c604b4dc1d09c5123adc3465e581079 Mon Sep 17 00:00:00 2001 From: Seunghun Lee Date: Tue, 14 Feb 2023 08:27:51 +0900 Subject: [PATCH] client: Remove struct wl_thread_data The wl_thread_data was introduced to prevent deadlock as explained by the following commit message. Since the problem described by the commit message seems to have disappeared, let's remove it and observe the results. If we decided to keep this code, we will need to add a reference count mechanism or something to prevent use after free and memory leaks. I believe this code will only add complexity and increase the likelihood of errors. Wayland specification says clearly that if a thread successfully calls wl_display_prepare_read_queue(), it must either call wl_display_read_events() when it's ready or cancel the read intention by calling wl_display_cancel_read(). So even if a deadlock occur again around this APIs, I believe we should still handle it as the application's responsibility. commit 409d548f6d3b43b8a4991619c3effeeed809f789 Author: Boram Park Date: Fri Apr 8 08:14:42 2016 +0900 client: use the thread reader_count to fix deadlock. Let's see the below scenario. 1. A thread polls several fds including the wayland's fd. 2. This thread probably calls wl_display_prepare_read() before polling fds. 3. This thread could be awake by other event source which isn't related with wayland fd. 4. After wake up, this thread could call wl_display_dispatch or wl_display_roundtrip for sync operation. Then, when this thread got a done event. this thread will fall in deadlock because this thread increases +2 reader_count in the sam thread. The read_event or cancel_read for the first prepare_read is not going to happen because this thread sleeps in pthread_cond_wait() line of read_event. This problem can be solved by using the reader_count per thread. The reader_count of thread will be increased/decreased whenever prepare_read, cancel_read and read_event are called. However, the original reader_count of display will be increased only once per thread. And, when cancel_read and read_event are called, it will be decreased to read event from fd and wake up other threads. And that, if the thread reader_count is still more than 0, it will be increased because it means the thread is still polling in somewhere. Change-Id: I2e881a6222e5ad380ace7a6d9571d5463cf701d9 Change-Id: Id77a4468fafaafe0fa79fbe186767adb8b09cde1 --- src/wayland-client.c | 342 +-------------------------------------------------- 1 file changed, 3 insertions(+), 339 deletions(-) diff --git a/src/wayland-client.c b/src/wayland-client.c index 2087aa0..91aba62 100644 --- a/src/wayland-client.c +++ b/src/wayland-client.c @@ -88,38 +88,6 @@ struct wl_event_queue { struct wl_display *display; }; -// TIZEN_ONLY(20190716) : wayland-client : force sync of display when threads are waiting for over WL_PTHREAD_COND_TIMEDWAIT_TIMEOUT -/* thread status */ -typedef enum -{ - WL_THREAD_STATE_INITIAL = 0, - WL_THREAD_STATE_PREPARE_READ = 1 << 0, - WL_THREAD_STATE_POLL_DONE = 1 << 1, - WL_THREAD_STATE_READ_EVENTS_BEGIN = 1 << 2, - WL_THREAD_STATE_READ_EVENTS_DONE = 1 << 3, - WL_THREAD_STATE_READ_EVENTS_CANCELD = 1 << 4, - WL_THREAD_STATE_WAIT_WAKEUP_BEGIN = 1 << 5, - WL_THREAD_STATE_WAIT_WAKEUP_DONE = 1 << 6, - WL_THREAD_STATE_WAIT_WAKEUP_TIMEOUT = 1 << 7, - WL_THREAD_STATE_WAIT_WAKEUP_ERROR = 1 << 8, - WL_THREAD_STATE_FORCE_DISPLAY_SYNC_BEGIN = 1 << 9, - WL_THREAD_STATE_FORCE_DISPLAY_SYNC_DONE = 1 << 10, - WL_THREAD_STATE_FORCE_DISPLAY_SYNC_ERROR = 1 << 11 -} thread_state; -// END - -struct wl_thread_data { - struct wl_list link; - int reader_count_in_thread; - int pid; - int tid; -// TIZEN_ONLY(20190716) : wayland-client : force sync of display when threads are waiting for over WL_PTHREAD_COND_TIMEDWAIT_TIMEOUT - thread_state state; - pthread_t thread_id; -// END - struct wl_display *display; -}; - struct wl_display { struct wl_proxy proxy; struct wl_connection *connection; @@ -151,14 +119,9 @@ struct wl_display { int reader_count; uint32_t read_serial; pthread_cond_t reader_cond; - - pthread_key_t thread_data_key; - struct wl_list threads; - pthread_mutex_t threads_mutex; // TIZEN_ONLY(20190716) : wayland-client : force sync of display when threads are waiting for over WL_PTHREAD_COND_TIMEDWAIT_TIMEOUT uint32_t force_sync_count; // END - uint32_t threads_count; /* last errno value */ int prev_errno; char *name; @@ -166,12 +129,6 @@ struct wl_display { /** \endcond */ -// TIZEN_ONLY(20190716) : wayland-client : force sync of display when threads are waiting for over WL_PTHREAD_COND_TIMEDWAIT_TIMEOUT -/* leave log about threads information and abort - when pthread_cond_timedwait() returns ETIMEDOUT */ -static void log_threads_reader_info(struct wl_display *display); -// END - /** * This helper function wakes up all threads that are * waiting for display->reader_cond (i. e. when reading is done, @@ -1186,86 +1143,6 @@ wl_proxy_marshal_array(struct wl_proxy *proxy, uint32_t opcode, } static void -destroy_thread_data(void *data) -{ - struct wl_display *display = NULL; - struct wl_thread_data *thread_data = data; - - display = thread_data->display; - assert(display); - - pthread_mutex_lock(&display->threads_mutex); - wl_list_remove(&thread_data->link); - display->threads_count--; - wl_log("Thread removed[%p pid:%d tid: %d] from display:%p, threads_cnt=%d\n", - thread_data, thread_data->pid, thread_data->tid, display, display->threads_count); - - if (thread_data->reader_count_in_thread > 0) - { - wl_log("######################################################################\n"); - wl_log("The thread's reader count is GREATER than zero. Must do cancellation !\n"); - wl_log("... [PID:%d][TID:%d] reader_count:%d, reader_count_in_thread:%d ...\n", - thread_data->pid, thread_data->tid, display->reader_count, thread_data->reader_count_in_thread); - wl_log("######################################################################\n"); - } - pthread_mutex_unlock(&display->threads_mutex); - - free(thread_data); -} - -static struct wl_thread_data* -get_thread_data(struct wl_display *display) -{ - struct wl_thread_data *thread_data; - struct wl_thread_data *th_data = NULL, *tmp = NULL; - - int pid = (int)getpid(); - int tid = (int)syscall(SYS_gettid); - - thread_data = pthread_getspecific(display->thread_data_key); - pthread_mutex_lock(&display->threads_mutex); - if (!thread_data && display->threads_count > 0) { - wl_list_for_each_safe(th_data, tmp, &display->threads, link) { - if (th_data && th_data->pid == pid && th_data->tid == tid) { - wl_log("[pid:%d tid:%d] Failed to pthread_getspecific. errno(%d, %m)\n", pid, tid, errno); - thread_data = th_data; - break; - } - } - } - pthread_mutex_unlock(&display->threads_mutex); - - if (!thread_data) { - int ret = 0; - thread_data = zalloc(sizeof *thread_data); - if (!thread_data) - return NULL; - - if ((ret = pthread_setspecific(display->thread_data_key, thread_data)) != 0) { - wl_log("[pid:%d tid:%d] Failed to pthread_setspecific. err = %d\n", pid, tid, ret); - } - - thread_data->reader_count_in_thread = 0; - thread_data->pid = pid; - thread_data->tid = tid; - thread_data->display = display; -// TIZEN_ONLY(20190716) : wayland-client : force sync of display when threads are waiting for over WL_PTHREAD_COND_TIMEDWAIT_TIMEOUT - thread_data->state = WL_THREAD_STATE_INITIAL; - thread_data->thread_id = pthread_self(); -// END - pthread_mutex_lock(&display->threads_mutex); - wl_list_init(&thread_data->link); - wl_list_insert(&display->threads, &thread_data->link); - display->threads_count++; - wl_log("Thread added[%p, pid:%d tid: %d] to display:%p, threads_cnt=%d, errno(%d, %m)\n", - thread_data, thread_data->pid, thread_data->tid, display, display->threads_count, errno); - pthread_mutex_unlock(&display->threads_mutex); - } - - return thread_data; -} - -static void display_handle_error(void *data, struct wl_display *display, void *object, uint32_t code, const char *message) @@ -1420,7 +1297,6 @@ WL_EXPORT struct wl_display * wl_display_connect_to_fd(int fd) { struct wl_display *display; - struct wl_thread_data *thread_data; const char *debug; debug = getenv("WAYLAND_DLOG"); @@ -1498,24 +1374,13 @@ wl_display_connect_to_fd(int fd) if (display->connection == NULL) goto err_connection; - display->threads_count = 0; - wl_list_init(&display->threads); - pthread_mutex_init(&display->threads_mutex, NULL); - if (pthread_key_create(&display->thread_data_key, destroy_thread_data) < 0) - goto err_connection; - - thread_data = get_thread_data(display); - if (!thread_data) - goto err_connection; - #ifdef WL_DEBUG_QUEUE if (debug_client) wl_dlog("display(%p) default_queue(%p) display_queue(%p) fd(%d)", display, &display->default_queue, &display->display_queue, display->fd); #endif - wl_log("display(%p) connected. (pid:%d, tid:%d, threads_cnt=%d, reader_cnt=%d)\n", - display, thread_data->pid, thread_data->tid, display->threads_count, display->reader_count); + wl_log("display(%p) connected.", display); pthread_mutex_unlock(&display->mutex); @@ -1525,7 +1390,6 @@ wl_display_connect_to_fd(int fd) // TIZEN_ONLY(20170410): Debug logs to identify the failure cause of wl_display_connect() wl_log("%s() failed. display(%p), fd(%d), errno(%d, %m)\n", __func__, display, fd, errno); // END - pthread_mutex_destroy(&display->threads_mutex); pthread_mutex_unlock(&display->mutex); pthread_mutex_destroy(&display->mutex); pthread_cond_destroy(&display->reader_cond); @@ -1612,38 +1476,19 @@ wl_display_connect(const char *name) WL_EXPORT void wl_display_disconnect(struct wl_display *display) { - int pid = -1; - int tid = -1; - - struct wl_thread_data *thread_data; - pthread_mutex_lock(&display->mutex); - thread_data = get_thread_data(display); - if (thread_data) - { - pid = thread_data->pid; - tid = thread_data->tid; - destroy_thread_data(thread_data); - thread_data = NULL; - } - - pthread_setspecific(display->thread_data_key, NULL); - pthread_key_delete(display->thread_data_key); - wl_connection_destroy(display->connection); wl_map_for_each(&display->objects, free_zombies, NULL); wl_map_release(&display->objects); wl_event_queue_release(&display->default_queue); wl_event_queue_release(&display->display_queue); - pthread_mutex_destroy(&display->threads_mutex); pthread_mutex_unlock(&display->mutex); pthread_mutex_destroy(&display->mutex); pthread_cond_destroy(&display->reader_cond); close(display->fd); - wl_log("display(%p) disconnected. (pid:%d, tid:%d, threads_cnt=%d, reader_cnt=%d\n", - display, pid, tid, display->threads_count, display->reader_count); + wl_log("display(%p) disconnected", display); if (display->name) free(display->name); @@ -2006,63 +1851,8 @@ dispatch_event(struct wl_display *display, struct wl_event_queue *queue) destroy_queued_closure(closure); } -static int -_check_reader_counts(struct wl_display *display) -{ - struct wl_thread_data *thread_data; - struct wl_thread_data *th_data, *th_data_next; - - int cnt = 0; - int threads_reader_counts = 0; - - thread_data = get_thread_data(display); - assert(thread_data); - - wl_log("[check_reader_counts] Number of threads=%d, reader_count=%d\n", display->threads_count, display->reader_count); - - pthread_mutex_lock(&display->threads_mutex); - wl_list_for_each_safe(th_data, th_data_next, &display->threads, link) { - wl_log("... thread[%d]:reader_count_in_thread=%d\n", cnt++, th_data->reader_count_in_thread); - - if (th_data->reader_count_in_thread > 0) - threads_reader_counts++; - } - pthread_mutex_unlock(&display->threads_mutex); - - if (threads_reader_counts < display->reader_count) - { - wl_log("[check_reader_counts] Abnormal reader_count !\n"); - wl_log("... reader_count=%d, total reader counts of threads=%d\n", display->reader_count, threads_reader_counts); - - return 1; - } - - return 0; -} - // TIZEN_ONLY(20190716) : wayland-client : force sync of display when threads are waiting for over WL_PTHREAD_COND_TIMEDWAIT_TIMEOUT static void -log_threads_reader_info(struct wl_display *display) -{ - struct wl_thread_data *thread_data; - struct wl_thread_data *th_data, *th_data_next; - int thread_cnt = 0; - - thread_data = get_thread_data(display); - assert(thread_data); - - wl_log("[thread info] current pid : %d, tid : %d\n", thread_data->pid, thread_data->tid); - wl_log("[thread info] display->reader_count : %d\n", display->reader_count); - - pthread_mutex_lock(&display->threads_mutex); - wl_list_for_each_safe(th_data, th_data_next, &display->threads, link) { - wl_log("[thread info] thread[%d][PID:%d][TID:%d][id:%lu] reader_count_in_thread:%d, state:%d\n", - thread_cnt++, th_data->pid, th_data->tid, th_data->thread_id, th_data->reader_count_in_thread, th_data->state); - } - pthread_mutex_unlock(&display->threads_mutex); -} - -static void force_sync_callback(void *data, struct wl_callback *callback, uint32_t serial) { wl_callback_destroy(callback); @@ -2114,7 +1904,6 @@ force_display_sync(struct wl_display *display) static int read_events(struct wl_display *display) { - struct wl_thread_data *thread_data; int total, rem, size; uint32_t serial; @@ -2123,49 +1912,18 @@ read_events(struct wl_display *display) struct timespec ts; int ret = 0, res = 0; // END - uint32_t _force_sync_count = 0; - - thread_data = get_thread_data(display); - assert(thread_data); -// TIZEN_ONLY(20190716) : wayland-client : force sync of display when threads are waiting for over WL_PTHREAD_COND_TIMEDWAIT_TIMEOUT - thread_data->state |= WL_THREAD_STATE_POLL_DONE; -// END - - thread_data->reader_count_in_thread--; display->reader_count--; - if (thread_data->reader_count_in_thread || display->reader_count < 0) { - wl_log("read_events[%p, pid:%d, tid:%d]: check reader count(T:%d, A:%d)", thread_data, thread_data->pid, thread_data->tid, - thread_data->reader_count_in_thread, display->reader_count); - -// TIZEN_ONLY(20190716) : wayland-client : force sync of display when threads are waiting for over WL_PTHREAD_COND_TIMEDWAIT_TIMEOUT - /* do wl_abort() when a thread specific reader count > 1 */ - log_threads_reader_info(display); - wl_abort("=== Invalid reader count (pid:%d, tid:%d, reader_count:%d, reader_count_in_thread:%d) ===\n", - thread_data->pid, thread_data->tid, display->reader_count, thread_data->reader_count_in_thread); -// END - } - if (display->reader_count == 0) { -// TIZEN_ONLY(20190716) : wayland-client : force sync of display when threads are waiting for over WL_PTHREAD_COND_TIMEDWAIT_TIMEOUT - thread_data->state |= WL_THREAD_STATE_READ_EVENTS_BEGIN; -// END total = wl_connection_read(display->connection); if (total < 0 && errno != EAGAIN && errno != EPIPE) wl_log("read failed: total(%d) errno(%d), display(%p)", total, errno, display); if (total == -1) { if (errno == EAGAIN) { -// TIZEN_ONLY(20190716) : wayland-client : force sync of display when threads are waiting for over WL_PTHREAD_COND_TIMEDWAIT_TIMEOUT - thread_data->state |= WL_THREAD_STATE_READ_EVENTS_DONE; -// END - /* we must wake up threads whenever * the reader_count dropped to 0 */ display_wakeup_threads(display); - if (thread_data->reader_count_in_thread > 0) - display->reader_count++; - return 0; } @@ -2173,10 +1931,6 @@ read_events(struct wl_display *display) display_fatal_error(display, errno); return -1; } else if (total == 0) { -// TIZEN_ONLY(20190716) : wayland-client : force sync of display when threads are waiting for over WL_PTHREAD_COND_TIMEDWAIT_TIMEOUT - thread_data->state |= WL_THREAD_STATE_READ_EVENTS_DONE; -// END - /* The compositor has closed the socket. This * should be considered an error so we'll fake * an errno */ @@ -2189,9 +1943,6 @@ read_events(struct wl_display *display) for (rem = total; rem >= 8; rem -= size) { size = queue_event(display, rem); if (size == -1) { -// TIZEN_ONLY(20190716) : wayland-client : force sync of display when threads are waiting for over WL_PTHREAD_COND_TIMEDWAIT_TIMEOUT - thread_data->state |= WL_THREAD_STATE_READ_EVENTS_DONE; -// END wl_log("queue_event failed\n"); display_fatal_error(display, errno); return -1; @@ -2201,9 +1952,6 @@ read_events(struct wl_display *display) } display_wakeup_threads(display); -// TIZEN_ONLY(20190716) : wayland-client : force sync of display when threads are waiting for over WL_PTHREAD_COND_TIMEDWAIT_TIMEOUT - thread_data->state |= WL_THREAD_STATE_READ_EVENTS_DONE; -// END } else { serial = display->read_serial; while (display->read_serial == serial) @@ -2214,55 +1962,25 @@ read_events(struct wl_display *display) ts.tv_nsec = now.tv_usec * 1000; ts.tv_sec = now.tv_sec + WL_PTHREAD_COND_TIMEDWAIT_TIMEOUT;// 10 seconds - thread_data->state |= WL_THREAD_STATE_WAIT_WAKEUP_BEGIN; - ret = pthread_cond_timedwait(&display->reader_cond, &display->mutex, &ts); - thread_data->state |= WL_THREAD_STATE_WAIT_WAKEUP_DONE; - if (ETIMEDOUT == ret) { - thread_data->state |= WL_THREAD_STATE_WAIT_WAKEUP_TIMEOUT; wl_log("=== Timeout on pthread_cond_timedwait. Start leaving data (timeout=%d)!===\n", WL_PTHREAD_COND_TIMEDWAIT_TIMEOUT); - - log_threads_reader_info(display); - wl_log("[read events] display->read_serial : %d, serial : %d\n", display->read_serial, serial); wl_log("=== Timeout on pthread_cond_timedwait. End of data leaving !===\n"); - /* After having waited for two times by calling pthread_cond_timedwait() and - * if the last reader (thread) doesn't wake the waiting threads up, it is considered to be an abnormal situation. - * Do wl_abort(). - */ - if (_force_sync_count > 1 && display->read_serial == serial) - { - if (_check_reader_counts(display)) - { - if (!errno) - { - wl_log("[WL_ERRNO_SET][FUNC:%s][LINE:%d] errno (%d -> %d), display:%p\n", __FUNCTION__, __LINE__, errno, display->last_error, display); - errno = display->last_error; - } - - wl_abort("Invalid reader count ! Abort !(reader_count=%d, errno=%d, last_error=%d, display:%p)\n", display->reader_count, errno, display->last_error, display); - } - } - - thread_data->state |= WL_THREAD_STATE_FORCE_DISPLAY_SYNC_BEGIN; wl_log("=== FORCE_DISPLAY_SYNC BEGIN ===\n"); res = force_display_sync(display); display->force_sync_count++; - _force_sync_count++; - thread_data->state |= WL_THREAD_STATE_FORCE_DISPLAY_SYNC_DONE; wl_log("=== FORCE_DISPLAY_SYNC DONE (res=%d, force_sync_count=%d) ===\n", res, display->force_sync_count); /* return if there is any error on doing display sync and leave display protocol error if exits */ if (res < 0) { - thread_data->state |= WL_THREAD_STATE_FORCE_DISPLAY_SYNC_ERROR; wl_log("=== FORCE_DISPLAY_SYNC ERROR (res=%d) ===\n", res); if (display->last_error) @@ -2279,7 +1997,6 @@ read_events(struct wl_display *display) } else if (ret) { - thread_data->state |= WL_THREAD_STATE_WAIT_WAKEUP_ERROR; wl_log("=== Error waiting pthread_cond_timedwait : continue, errno(%d) ===\n", ret); } } @@ -2291,46 +2008,15 @@ read_events(struct wl_display *display) } } - /* If reader_count_in_thread > 0, it means that this thread is still polling - * in somewhere. So inclease +1 for it. - */ - if (thread_data->reader_count_in_thread > 0) - display->reader_count++; - return 0; } static void cancel_read(struct wl_display *display) { - struct wl_thread_data *thread_data; - - thread_data = get_thread_data(display); - assert(thread_data); - - thread_data->reader_count_in_thread--; display->reader_count--; - if (thread_data->reader_count_in_thread || display->reader_count < 0) { - wl_log("Cancel_events[%p, pid:%d, tid:%d]: check reader count(T:%d, A:%d)\n", thread_data, thread_data->pid, thread_data->tid, - thread_data->reader_count_in_thread, display->reader_count); - -// TIZEN_ONLY(20190716) : wayland-client : force sync of display when threads are waiting for over WL_PTHREAD_COND_TIMEDWAIT_TIMEOUT - /* do wl_abort() when a thread specific reader count > 1 */ - log_threads_reader_info(display); - wl_abort("=== Invalid reader count (pid:%d, tid:%d, reader_count:%d, reader_count_in_thread:%d) ===\n", - thread_data->pid, thread_data->tid, display->reader_count, thread_data->reader_count_in_thread); -// END - } - if (display->reader_count == 0) display_wakeup_threads(display); - - if (thread_data->reader_count_in_thread > 0) - display->reader_count++; - -// TIZEN_ONLY(20190716) : wayland-client : force sync of display when threads are waiting for over WL_PTHREAD_COND_TIMEDWAIT_TIMEOUT - thread_data->state |= WL_THREAD_STATE_READ_EVENTS_CANCELD; -// END } /** Read events from display file descriptor @@ -2487,29 +2173,7 @@ wl_display_prepare_read_queue(struct wl_display *display, errno = EAGAIN; ret = -1; } else { - struct wl_thread_data *thread_data; - - thread_data = get_thread_data(display); - assert(thread_data); - - /* increase +1 per thread */ - if (thread_data->reader_count_in_thread == 0) - display->reader_count++; - else { - wl_log("Prepare_read[%d]: check reader count(T:%d, A:%d)\n", thread_data->tid, thread_data->reader_count_in_thread, display->reader_count); - -// TIZEN_ONLY(20190716) : wayland-client : force sync of display when threads are waiting for over WL_PTHREAD_COND_TIMEDWAIT_TIMEOUT - /* do wl_abort() when a thread specific reader count > 1 */ - log_threads_reader_info(display); - wl_abort("=== Invalid reader count (pid:%d, tid:%d, reader_count:%d, reader_count_in_thread:%d) ===\n", - thread_data->pid, thread_data->tid, display->reader_count, thread_data->reader_count_in_thread); -// END - } - -// TIZEN_ONLY(20190716) : wayland-client : force sync of display when threads are waiting for over WL_PTHREAD_COND_TIMEDWAIT_TIMEOUT - thread_data->state = WL_THREAD_STATE_PREPARE_READ; -// END - thread_data->reader_count_in_thread++; + display->reader_count++; ret = 0; } -- 2.7.4