From b97f4782e1e7bc6a67afb7b7cae9d9ddb875e198 Mon Sep 17 00:00:00 2001 From: mengensun Date: Fri, 18 Feb 2022 19:35:39 +0800 Subject: [PATCH] Revert "epoll: atomically remove wait entry on wake up" Revert "eventpoll: fix missing wakeup for ovflist in ep_poll_callback" Revert commit: 72417f224476469fee87e520e656eb197c8842d4 Revert commit: f51fcbb32f68f14d694db359bb101b116662deb1 thoese two commit make nginx slowly like this: before revert: -------------- ./wrk -t84 -c8400 -d10s -T5s -H "Connection: Close" --latency http://x.x.x.x/hello Running 10s test @ http://x.x.x.x/hello 84 threads and 8400 connections Thread Stats Avg Stdev Max +/- Stdev Latency 26.06ms 33.53ms 1.03s 92.66% Req/Sec 723.03 416.59 9.03k 72.25% Latency Distribution 50% 18.59ms 75% 31.94ms 90% 52.37ms 99% 141.92ms 592056 requests in 10.10s, 83.00MB read Socket errors: connect 0, read 253478, write 0, timeout 0 Requests/sec: 58619.13 Transfer/sec: 8.22MB after revert: ------------- ./wrk -t84 -c8400 -d10s -T5s -H "Connection: Close" --latency http://x.x.x.x/hello Running 10s test @ http://x.x.x.x/hello 84 threads and 8400 connections Thread Stats Avg Stdev Max +/- Stdev Latency 22.10ms 29.46ms 894.07ms 93.15% Req/Sec 0.90k 598.46 12.02k 74.86% Latency Distribution 50% 15.29ms 75% 27.79ms 90% 44.95ms 99% 111.73ms 736417 requests in 10.10s, 103.24MB read Socket errors: connect 0, read 327949, write 0, timeout 0 Requests/sec: 72921.54 Transfer/sec: 10.22MB see, after revert nginx bench's through high and time latency low Signed-off-by: mengensun Reviewed-by: mungerjiang Reviewed-by: kaixuxia --- fs/eventpoll.c | 61 +++++++++++++++++++++++--------------------------- 1 file changed, 28 insertions(+), 33 deletions(-) diff --git a/fs/eventpoll.c b/fs/eventpoll.c index 2e5779721..86f64f922 100644 --- a/fs/eventpoll.c +++ b/fs/eventpoll.c @@ -1174,10 +1174,6 @@ static inline bool chain_epi_lockless(struct epitem *epi) { struct eventpoll *ep = epi->ep; - /* Fast preliminary check */ - if (epi->next != EP_UNACTIVE_PTR) - return false; - /* Check that the same epi has not been just chained from another CPU */ if (cmpxchg(&epi->next, EP_UNACTIVE_PTR, NULL) != EP_UNACTIVE_PTR) return false; @@ -1244,12 +1240,16 @@ static int ep_poll_callback(wait_queue_entry_t *wait, unsigned mode, int sync, v * chained in ep->ovflist and requeued later on. */ if (READ_ONCE(ep->ovflist) != EP_UNACTIVE_PTR) { - if (chain_epi_lockless(epi)) - ep_pm_stay_awake_rcu(epi); - } else if (!ep_is_linked(epi)) { - /* In the usual case, add event to ready list. */ - if (list_add_tail_lockless(&epi->rdllink, &ep->rdllist)) + if (epi->next == EP_UNACTIVE_PTR && + chain_epi_lockless(epi)) ep_pm_stay_awake_rcu(epi); + goto out_unlock; + } + + /* If this file is already in the ready list we exit soon */ + if (!ep_is_linked(epi) && + list_add_tail_lockless(&epi->rdllink, &ep->rdllist)) { + ep_pm_stay_awake_rcu(epi); } /* @@ -1825,6 +1825,7 @@ static int ep_poll(struct eventpoll *ep, struct epoll_event __user *events, { int res = 0, eavail, timed_out = 0; u64 slack = 0; + bool waiter = false; wait_queue_entry_t wait; ktime_t expires, *to = NULL; @@ -1869,23 +1870,21 @@ fetch_events: */ ep_reset_busy_poll_napi_id(ep); - do { - /* - * Internally init_wait() uses autoremove_wake_function(), - * thus wait entry is removed from the wait queue on each - * wakeup. Why it is important? In case of several waiters - * each new wakeup will hit the next waiter, giving it the - * chance to harvest new event. Otherwise wakeup can be - * lost. This is also good performance-wise, because on - * normal wakeup path no need to call __remove_wait_queue() - * explicitly, thus ep->lock is not taken, which halts the - * event delivery. - */ - init_wait(&wait); + /* + * We don't have any available event to return to the caller. We need + * to sleep here, and we will be woken by ep_poll_callback() when events + * become available. + */ + if (!waiter) { + waiter = true; + init_waitqueue_entry(&wait, current); + write_lock_irq(&ep->lock); __add_wait_queue_exclusive(&ep->wq, &wait); write_unlock_irq(&ep->lock); + } + for (;;) { /* * We don't want to sleep if the ep_poll_callback() sends us * a wakeup in between. That's why we set the task state @@ -1915,20 +1914,10 @@ fetch_events: timed_out = 1; break; } - - /* We were woken up, thus go and try to harvest some events */ - eavail = 1; - - } while (0); + } __set_current_state(TASK_RUNNING); - if (!list_empty_careful(&wait.entry)) { - write_lock_irq(&ep->lock); - __remove_wait_queue(&ep->wq, &wait); - write_unlock_irq(&ep->lock); - } - send_events: /* * Try to transfer events to user space. In case we get 0 events and @@ -1939,6 +1928,12 @@ send_events: !(res = ep_send_events(ep, events, maxevents)) && !timed_out) goto fetch_events; + if (waiter) { + write_lock_irq(&ep->lock); + __remove_wait_queue(&ep->wq, &wait); + write_unlock_irq(&ep->lock); + } + return res; }