From 6b19e40730c6d331b502171d5cf615f393afff6a Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 2 Nov 2023 12:02:43 -0400 Subject: [PATCH 1/2] lib: Create a helper function for io read operations Currently when io is ready inside of the event system the first FD received is always preferred as the ones that are handled first. This leads to results where events associated with these first FD's are always handled first. In anticipation of a change to make this more fair let's abstract the function handler. Signed-off-by: Donald Sharp --- lib/event.c | 104 +++++++++++++++++++++++++++------------------------- 1 file changed, 55 insertions(+), 49 deletions(-) diff --git a/lib/event.c b/lib/event.c index 4121c0a448ce..f5ebd0197351 100644 --- a/lib/event.c +++ b/lib/event.c @@ -1589,6 +1589,60 @@ static int thread_process_io_helper(struct event_loop *m, struct event *thread, return 1; } +static inline void thread_process_io_inner_loop(struct event_loop *m, + unsigned int num, + struct pollfd *pfds, nfds_t *i, + uint32_t *ready) +{ + /* no event for current fd? immediately continue */ + if (pfds[*i].revents == 0) + return; + + *ready = *ready + 1; + + /* + * Unless someone has called event_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 event_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. + * + * 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 | POLLERR)) { + thread_process_io_helper(m, m->read[pfds[*i].fd], POLLIN, + pfds[*i].revents, *i); + } + if (pfds[*i].revents & POLLOUT) + thread_process_io_helper(m, m->write[pfds[*i].fd], POLLOUT, + pfds[*i].revents, *i); + + /* + * if one of our file descriptors is garbage, remove the same + * from both pfds + update sizes and index + */ + if (pfds[*i].revents & POLLNVAL) { + memmove(m->handler.pfds + *i, m->handler.pfds + *i + 1, + (m->handler.pfdcount - *i - 1) * sizeof(struct pollfd)); + m->handler.pfdcount--; + m->handler.pfds[m->handler.pfdcount].fd = 0; + m->handler.pfds[m->handler.pfdcount].events = 0; + + memmove(pfds + *i, pfds + *i + 1, + (m->handler.copycount - *i - 1) * sizeof(struct pollfd)); + m->handler.copycount--; + m->handler.copy[m->handler.copycount].fd = 0; + m->handler.copy[m->handler.copycount].events = 0; + + *i = *i - 1; + } +} + /** * Process I/O events. * @@ -1604,55 +1658,7 @@ static void thread_process_io(struct event_loop *m, unsigned int num) struct pollfd *pfds = m->handler.copy; for (nfds_t i = 0; i < m->handler.copycount && ready < num; ++i) { - /* no event for current fd? immediately continue */ - if (pfds[i].revents == 0) - continue; - - ready++; - - /* - * Unless someone has called event_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 event_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. - * - * 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 | POLLERR)) { - thread_process_io_helper(m, m->read[pfds[i].fd], POLLIN, - pfds[i].revents, i); - } - if (pfds[i].revents & POLLOUT) - thread_process_io_helper(m, m->write[pfds[i].fd], - POLLOUT, pfds[i].revents, i); - - /* - * if one of our file descriptors is garbage, remove the same - * from both pfds + update sizes and index - */ - if (pfds[i].revents & POLLNVAL) { - memmove(m->handler.pfds + i, m->handler.pfds + i + 1, - (m->handler.pfdcount - i - 1) - * sizeof(struct pollfd)); - m->handler.pfdcount--; - m->handler.pfds[m->handler.pfdcount].fd = 0; - m->handler.pfds[m->handler.pfdcount].events = 0; - - memmove(pfds + i, pfds + i + 1, - (m->handler.copycount - i - 1) - * sizeof(struct pollfd)); - m->handler.copycount--; - m->handler.copy[m->handler.copycount].fd = 0; - m->handler.copy[m->handler.copycount].events = 0; - - i--; - } + thread_process_io_inner_loop(m, num, pfds, &i, &ready); } } From 57ea8ac8c34e347e541239019784167090785850 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Thu, 2 Nov 2023 16:46:27 -0400 Subject: [PATCH 2/2] lib: Modify event system to treat fd access more fairly Keep track of the last starting spot of where fd's were being handled for read operations. Modify the io read handler to cycle through the list of fd's that need to be handled such that fd's at the front do not take precedence for being handled all the time. Signed-off-by: Donald Sharp --- lib/event.c | 15 ++++++++++++--- lib/frrevent.h | 2 ++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/lib/event.c b/lib/event.c index f5ebd0197351..6e55310b6549 100644 --- a/lib/event.c +++ b/lib/event.c @@ -1649,6 +1649,10 @@ static inline void thread_process_io_inner_loop(struct event_loop *m, * Walks through file descriptor array looking for those pollfds whose .revents * field has something interesting. Deletes any invalid file descriptors. * + * Try to impart some impartiality to handling of io. The event + * system will cycle through the fd's available for io + * giving each one a chance to go first. + * * @param m the thread master * @param num the number of active file descriptors (return value of poll()) */ @@ -1656,10 +1660,15 @@ static void thread_process_io(struct event_loop *m, unsigned int num) { unsigned int ready = 0; struct pollfd *pfds = m->handler.copy; + nfds_t i, last_read = m->last_read % m->handler.copycount; - for (nfds_t i = 0; i < m->handler.copycount && ready < num; ++i) { - thread_process_io_inner_loop(m, num, pfds, &i, &ready); - } + for (i = last_read; i < m->handler.copycount && ready < num; ++i) + thread_process_io_inner_loop(m, num, pfds, &i, &ready); + + for (i = 0; i < last_read && ready < num; ++i) + thread_process_io_inner_loop(m, num, pfds, &i, &ready); + + m->last_read++; } /* Add all timers that have popped to the ready list. */ diff --git a/lib/frrevent.h b/lib/frrevent.h index 3f74df359bc2..616fe131af5c 100644 --- a/lib/frrevent.h +++ b/lib/frrevent.h @@ -91,6 +91,8 @@ struct event_loop { pthread_mutex_t mtx; pthread_t owner; + nfds_t last_read; + bool ready_run_loop; RUSAGE_T last_getrusage; };