From: Willy Tarreau Date: Sat, 9 Jul 2022 21:55:43 +0000 (+0200) Subject: MAJOR: poller: only touch/inspect the update_mask under tgid protection X-Git-Tag: v2.7-dev2~5 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1f947cb39ed92036131d71ea06404429208c4be3;p=thirdparty%2Fhaproxy.git MAJOR: poller: only touch/inspect the update_mask under tgid protection With thread groups and group-local masks, the update_mask cannot be touched nor even checked if it may change below us. In order to avoid this, we have to grab a reference to the FD's tgid before checking the update mask. The operations are cheap enough so that we don't notice it in performance tests. This is expected because the risk of meeting a reassigned FD during an update remains very low. It's worth noting that the tgid cannot be trusted during startup nor during soft-stop since that may come from anywhere at the moment. Since soft-stop runs under thread isolation we use that hint to decide whether or not to check that the FD's tgid matches the current one. The modification is applied to the 3 thread-aware pollers, i.e. epoll, kqueue, and evports. Also one poll_drop counter was missing for shared updates, though it might be hard to trigger it. With this change applied, thread groups are usable in benchmarks. --- diff --git a/src/ev_epoll.c b/src/ev_epoll.c index 7be659bc1f..884c03837f 100644 --- a/src/ev_epoll.c +++ b/src/ev_epoll.c @@ -169,16 +169,24 @@ static void _do_poll(struct poller *p, int exp, int wake) for (updt_idx = 0; updt_idx < fd_nbupdt; updt_idx++) { fd = fd_updt[updt_idx]; - _HA_ATOMIC_AND(&fdtab[fd].update_mask, ~ti->ltid_bit); - if (!fdtab[fd].owner) { + if (!fd_grab_tgid(fd, tgid)) { + /* was reassigned */ activity[tid].poll_drop_fd++; continue; } - _update_fd(fd); + _HA_ATOMIC_AND(&fdtab[fd].update_mask, ~ti->ltid_bit); + + if (fdtab[fd].owner) + _update_fd(fd); + else + activity[tid].poll_drop_fd++; + + fd_drop_tgid(fd); } fd_nbupdt = 0; - /* Scan the global update list */ + + /* Scan the shared update list */ for (old_fd = fd = update_list[tgid - 1].first; fd != -1; fd = fdtab[fd].update.next) { if (fd == -2) { fd = old_fd; @@ -188,13 +196,24 @@ static void _do_poll(struct poller *p, int exp, int wake) fd = -fd -4; if (fd == -1) break; - if (fdtab[fd].update_mask & ti->ltid_bit) - done_update_polling(fd); - else + + if (!fd_grab_tgid(fd, tgid)) { + /* was reassigned */ + activity[tid].poll_drop_fd++; continue; - if (!fdtab[fd].owner) + } + + if (!(fdtab[fd].update_mask & ti->ltid_bit)) continue; - _update_fd(fd); + + done_update_polling(fd); + + if (fdtab[fd].owner) + _update_fd(fd); + else + activity[tid].poll_drop_fd++; + + fd_drop_tgid(fd); } thread_idle_now(); diff --git a/src/ev_evports.c b/src/ev_evports.c index 79854702a6..19d572c663 100644 --- a/src/ev_evports.c +++ b/src/ev_evports.c @@ -126,16 +126,24 @@ static void _do_poll(struct poller *p, int exp, int wake) for (i = 0; i < fd_nbupdt; i++) { fd = fd_updt[i]; - _HA_ATOMIC_AND(&fdtab[fd].update_mask, ~ti->ltid_bit); - if (fdtab[fd].owner == NULL) { + if (!fd_grab_tgid(fd, tgid)) { + /* was reassigned */ activity[tid].poll_drop_fd++; continue; } - _update_fd(fd); + _HA_ATOMIC_AND(&fdtab[fd].update_mask, ~ti->ltid_bit); + + if (fdtab[fd].owner) + _update_fd(fd); + else + activity[tid].poll_drop_fd++; + + fd_drop_tgid(fd); } fd_nbupdt = 0; - /* Scan the global update list */ + + /* Scan the shared update list */ for (old_fd = fd = update_list[tgid - 1].first; fd != -1; fd = fdtab[fd].update.next) { if (fd == -2) { fd = old_fd; @@ -145,13 +153,24 @@ static void _do_poll(struct poller *p, int exp, int wake) fd = -fd -4; if (fd == -1) break; - if (fdtab[fd].update_mask & ti->ltid_bit) - done_update_polling(fd); - else + + if (!fd_grab_tgid(fd, tgid)) { + /* was reassigned */ + activity[tid].poll_drop_fd++; continue; - if (!fdtab[fd].owner) + } + + if (!(fdtab[fd].update_mask & ti->ltid_bit)) continue; - _update_fd(fd); + + done_update_polling(fd); + + if (fdtab[fd].owner) + _update_fd(fd); + else + activity[tid].poll_drop_fd++; + + fd_drop_tgid(fd); } thread_idle_now(); diff --git a/src/ev_kqueue.c b/src/ev_kqueue.c index 53bda1fe2c..3d555ec7ef 100644 --- a/src/ev_kqueue.c +++ b/src/ev_kqueue.c @@ -102,12 +102,20 @@ static void _do_poll(struct poller *p, int exp, int wake) for (updt_idx = 0; updt_idx < fd_nbupdt; updt_idx++) { fd = fd_updt[updt_idx]; - _HA_ATOMIC_AND(&fdtab[fd].update_mask, ~ti->ltid_bit); - if (!fdtab[fd].owner) { + if (!fd_grab_tgid(fd, tgid)) { + /* was reassigned */ activity[tid].poll_drop_fd++; continue; } - changes = _update_fd(fd, changes); + + _HA_ATOMIC_AND(&fdtab[fd].update_mask, ~ti->ltid_bit); + + if (fdtab[fd].owner) + changes = _update_fd(fd, changes); + else + activity[tid].poll_drop_fd++; + + fd_drop_tgid(fd); } /* Scan the global update list */ for (old_fd = fd = update_list[tgid - 1].first; fd != -1; fd = fdtab[fd].update.next) { @@ -119,13 +127,24 @@ static void _do_poll(struct poller *p, int exp, int wake) fd = -fd -4; if (fd == -1) break; - if (fdtab[fd].update_mask & ti->ltid_bit) - done_update_polling(fd); - else + + if (!fd_grab_tgid(fd, tgid)) { + /* was reassigned */ + activity[tid].poll_drop_fd++; continue; - if (!fdtab[fd].owner) + } + + if (!(fdtab[fd].update_mask & ti->ltid_bit)) continue; - changes = _update_fd(fd, changes); + + done_update_polling(fd); + + if (fdtab[fd].owner) + changes = _update_fd(fd, changes); + else + activity[tid].poll_drop_fd++; + + fd_drop_tgid(fd); } thread_idle_now();