From d279ef571ea459ed430ae46ec57ebf7e16cbcb54 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 12 Aug 2020 09:46:44 -0400 Subject: [PATCH 1/2] lib: Fixup comment indentations in thread.c Somewhere along the way the indentation for comments got all messed up. Let's make it follow our standards and also look right too. Signed-off-by: Donald Sharp --- lib/thread.c | 71 ++++++++++++++++++++++++++-------------------------- 1 file changed, 35 insertions(+), 36 deletions(-) diff --git a/lib/thread.c b/lib/thread.c index 1df4eee25c..48183cc9ad 100644 --- a/lib/thread.c +++ b/lib/thread.c @@ -726,17 +726,16 @@ static void thread_free(struct thread_master *master, struct thread *thread) static int fd_poll(struct thread_master *m, struct pollfd *pfds, nfds_t pfdsize, nfds_t count, const struct timeval *timer_wait) { - /* If timer_wait is null here, that means poll() should block - * indefinitely, - * unless the thread_master has overridden it by setting + /* + * If timer_wait is null here, that means poll() should block + * indefinitely, unless the thread_master has overridden it by setting * ->selectpoll_timeout. + * * If the value is positive, it specifies the maximum number of - * milliseconds - * to wait. If the timeout is -1, it specifies that we should never wait - * and - * always return immediately even if no event is detected. If the value - * is - * zero, the behavior is default. */ + * milliseconds to wait. If the timeout is -1, it specifies that + * we should never wait and always return immediately even if no + * event is detected. If the value is zero, the behavior is default. + */ int timeout = -1; /* number of file descriptors with events */ @@ -860,7 +859,7 @@ funcname_thread_add_timer_timeval(struct thread_master *m, frr_with_mutex(&m->mtx) { if (t_ptr && *t_ptr) - // thread is already scheduled; don't reschedule + /* thread is already scheduled; don't reschedule */ return NULL; thread = thread_get(m, type, func, arg, debugargpass); @@ -940,7 +939,7 @@ struct thread *funcname_thread_add_event(struct thread_master *m, frr_with_mutex(&m->mtx) { if (t_ptr && *t_ptr) - // thread is already scheduled; don't reschedule + /* thread is already scheduled; don't reschedule */ break; thread = thread_get(m, THREAD_EVENT, func, arg, debugargpass); @@ -1047,11 +1046,12 @@ static void do_thread_cancel(struct thread_master *master) struct cancel_req *cr; struct listnode *ln; for (ALL_LIST_ELEMENTS_RO(master->cancel_req, ln, cr)) { - /* If this is an event object cancellation, linear search - * through event - * list deleting any events which have the specified argument. - * We also - * need to check every thread in the ready queue. */ + /* + * If this is an event object cancellation, linear search + * through event list deleting any events which have the + * specified argument. We also need to check every thread + * in the ready queue. + */ if (cr->eventobj) { struct thread *t; @@ -1075,11 +1075,12 @@ static void do_thread_cancel(struct thread_master *master) continue; } - /* The pointer varies depending on whether the cancellation - * request was - * made asynchronously or not. If it was, we need to check - * whether the - * thread even exists anymore before cancelling it. */ + /* + * The pointer varies depending on whether the cancellation + * request was made asynchronously or not. If it was, we + * need to check whether the thread even exists anymore + * before cancelling it. + */ thread = (cr->thread) ? cr->thread : *cr->threadref; if (!thread) @@ -1304,17 +1305,16 @@ static void thread_process_io(struct thread_master *m, unsigned int num) ready++; - /* Unless someone has called thread_cancel from another pthread, - * the only - * thing that could have changed in m->handler.pfds while we - * were - * asleep is the .events field in a given pollfd. Barring - * thread_cancel() - * that value should be a superset of the values we have in our - * copy, so - * there's no need to update it. Similarily, barring deletion, - * the fd - * should still be a valid index into the master's pfds. */ + /* + * Unless someone has called thread_cancel from another + * pthread, the only thing that could have changed in + * m->handler.pfds while we were asleep is the .events + * field in a given pollfd. Barring thread_cancel() that + * value should be a superset of the values we have in our + * copy, so there's no need to update it. Similarily, + * barring deletion, the fd should still be a valid index + * into the master's pfds. + */ if (pfds[i].revents & (POLLIN | POLLHUP)) { thread_process_io_helper(m, m->read[pfds[i].fd], POLLIN, pfds[i].revents, i); @@ -1427,11 +1427,10 @@ struct thread *thread_fetch(struct thread_master *m, struct thread *fetch) * * - If there are events pending, set the poll() timeout to zero * - If there are no events pending, but there are timers - * pending, set the - * timeout to the smallest remaining time on any timer + * pending, set the timeout to the smallest remaining time on + * any timer. * - If there are neither timers nor events pending, but there - * are file - * descriptors pending, block indefinitely in poll() + * are file descriptors pending, block indefinitely in poll() * - If nothing is pending, it's time for the application to die * * In every case except the last, we need to hit poll() at least From d142453d6b322de258e123dcde28b2ee9813a289 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Wed, 12 Aug 2020 09:49:20 -0400 Subject: [PATCH 2/2] lib: Properly handle POLLERR from poll() There are situations where POLLERR will be returned. But since we were not handling it. Thread processing effectively is turned into an infinite loop, which is bad. Modify the code so that if we receive a POLLERR we turn it into a read event to be handled as an error from the handler function. This was discovered in pim: Thread statistics for pimd: Showing poll FD's for main -------------------------- Count: 14/1024 0 fd: 9 events: 1 revents: 0 mroute_read 1 fd: 12 events: 1 revents: 0 vty_accept 2 fd: 13 events: 1 revents: 0 vtysh_accept 3 fd: 11 events: 1 revents: 0 zclient_read 4 fd: 15 events: 1 revents: 0 mroute_read 5 fd: 16 events: 1 revents: 0 mroute_read 6 fd: 17 events: 1 revents: 0 pim_sock_read 7 fd: 19 events: 1 revents: 0 pim_sock_read 8 fd: 21 events: 1 revents: 0 pim_igmp_read 9 fd: 22 events: 1 revents: 0 pim_sock_read 10 fd: 23 events: 1 revents: 0 pim_sock_read 11 fd: 20 events: 1 revents: 0 vtysh_read 12 fd: 18 events: 1 revents: 0 pim_sock_read 13 fd: 24 events: 0 revents: 0 strace was showing this line over and over and over: poll([{fd=9, events=POLLIN}, {fd=12, events=POLLIN}, {fd=13, events=POLLIN}, {fd=11, events=POLLIN}, {fd=15, events=POLLIN}, {fd=16, events=POLLIN}, {fd=17, events=POLLIN}, {fd=19, events=POLLIN}, {fd=21, events=POLLIN}, {fd=22, events=POLLIN}, {fd=23, events=POLLIN}, {fd=20, events=POLLIN}, {fd=18, events=POLLIN}, {fd=6, events=POLLIN}], 14, 20) = 1 ([{fd=21, revents=POLLERR}]) Signed-off-by: Donald Sharp --- lib/thread.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/thread.c b/lib/thread.c index 48183cc9ad..19e4827283 100644 --- a/lib/thread.c +++ b/lib/thread.c @@ -1314,8 +1314,12 @@ static void thread_process_io(struct thread_master *m, unsigned int num) * copy, so there's no need to update it. Similarily, * barring deletion, the fd should still be a valid index * into the master's pfds. + * + * We are including POLLERR here to do a READ event + * this is because the read should fail and the + * read function should handle it appropriately */ - if (pfds[i].revents & (POLLIN | POLLHUP)) { + if (pfds[i].revents & (POLLIN | POLLHUP | POLLERR)) { thread_process_io_helper(m, m->read[pfds[i].fd], POLLIN, pfds[i].revents, i); }