epoll: ep_autoremove_wake_function should use list_del_init_careful
Subsystems (2)
PATCH RESEND 1/1 epoll: ep_autoremove_wake_function should use list_del_init_careful
autoremove_wake_function uses list_del_init_careful, so should epoll's more aggressive variant. It only doesn't because it was copied from an older wait.c rather than the most recent.
--- fs/eventpoll.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/eventpoll.c +1 -1
@@ -1756,11 +1756,11 @@ static struct timespec64 *ep_timeout_to_timespec(struct timespec64 *to, long ms)static int ep_autoremove_wake_function (struct wait_queue_entry *wq_entry,unsigned int mode,int sync,void *key) {int ret =default_wake_function (wq_entry, mode, sync, key); -list_del_init (&wq_entry->entry); +list_del_init_careful (&wq_entry->entry);return ret; }/** * ep_poll - Retrieves ready events, and delivers them to the caller-supplied
Signature · 1 line
2.39.0.rc0.267.gcb52ba06e7-goog
Quoted 16 lines · Benjamin Segall
autoremove_wake_functionuseslist_del_init_careful, so should epoll's more aggressive variant. It only doesn't because it was copied from an olderwait.crather than the most recent.Fixes a16ceb139610 ("epoll: autoremove wakers even more aggressively")Signed-off-by Ben Segall <[email protected]>--- fs/eventpoll.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)fs/eventpoll.c +0
-0
fs/eventpoll.c static struct timespec64 *ep_timeout_to_timespec(struct timespec64 *to, long ms) · line 1763
Quoted 9 lines
fs/eventpoll.c +1
-1@@ -1756,11 +1756,11 @@ static struct timespec64 *ep_timeout_to_timespec(struct timespec64 *to, long ms)static int ep_autoremove_wake_function (struct wait_queue_entry *wq_entry,unsigned int mode,int sync,void *key) {int ret =default_wake_function (wq_entry, mode, sync, key); -list_del_init (&wq_entry->entry); +list_del_init_careful (&wq_entry->entry);return ret; }
Can you please provide a more detailed explanation about why list_del_init_careful() is needed here?
- Eric
Christian Brauner
maintainerQuoted 22 lines · Eric Biggers
Quoted 21 lines · Benjamin Segall
autoremove_wake_functionuseslist_del_init_careful, so should epoll's more aggressive variant. It only doesn't because it was copied from an olderwait.crather than the most recent.Fixes a16ceb139610 ("epoll: autoremove wakers even more aggressively")Signed-off-by Ben Segall <[email protected]>--- fs/eventpoll.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)fs/eventpoll.c +0
-0@@ -1756,11 +1756,11 @@ static struct timespec64 *ep_timeout_to_timespec(struct timespec64 *to, long ms)static int ep_autoremove_wake_function (struct wait_queue_entry *wq_entry,unsigned int mode,int sync,void *key) {int ret =default_wake_function (wq_entry, mode, sync, key);
Quoted 7 lines
fs/eventpoll.c static struct timespec64 *ep_timeout_to_timespec(struct timespec64 *to, long ms) · line 1763
Quoted 4 lines
fs/eventpoll.c +1
-1-list_del_init (&wq_entry->entry); +list_del_init_careful (&wq_entry->entry);return ret; }Can you please provide a more detailed explanation about why
list_del_init_careful()is needed here?
Yeah, this needs more explanation... Next time someone looks at this code and there's a *_careful() added they'll want to know why.
Quoted 31 lines · Christian Brauner
Quoted 22 lines · Eric Biggers
Quoted 21 lines · Benjamin Segall
autoremove_wake_functionuseslist_del_init_careful, so should epoll's more aggressive variant. It only doesn't because it was copied from an olderwait.crather than the most recent.Fixes a16ceb139610 ("epoll: autoremove wakers even more aggressively")Signed-off-by Ben Segall <[email protected]>--- fs/eventpoll.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)fs/eventpoll.c +0
-0@@ -1756,11 +1756,11 @@ static struct timespec64 *ep_timeout_to_timespec(struct timespec64 *to, long ms)static int ep_autoremove_wake_function (struct wait_queue_entry *wq_entry,unsigned int mode,int sync,void *key) {int ret =default_wake_function (wq_entry, mode, sync, key);Quoted 7 lines
fs/eventpoll.c static struct timespec64 *ep_timeout_to_timespec(struct timespec64 *to, long ms) · line 1763
Quoted 4 lines
fs/eventpoll.c +1
-1-list_del_init (&wq_entry->entry); +list_del_init_careful (&wq_entry->entry);return ret; }Can you please provide a more detailed explanation about why
list_del_init_careful()is needed here?
Quoted 2 lines
Yeah, this needs more explanation... Next time someone looks at this code and there's a
*_careful()added they'll want to know why.
So the general reason is the same as with autoremove_wake_function, it pairs with the list_entry_careful in ep_poll (which is epoll's modified copy of finish_wait).
I think the original actual _problem_ was a -stable issue that was fixed instead by doing additional backports, so this may just avoid potential extra loops and avoid potential compiler shenanigans from the data race.
Quoted 6 lines · On Wed, 31 May 2023 15:15:41 -0700 Benjamin Segall <[email protected]> wrote:
Quoted 5 lines
Quoted 2 lines
Can you please provide a more detailed explanation about why
list_del_init_careful()is needed here?Yeah, this needs more explanation... Next time someone looks at this code and there's a
*_careful()added they'll want to know why.
Quoted 7 lines
So the general reason is the same as with
autoremove_wake_function, it pairs with thelist_entry_carefulinep_poll(which is epoll's modified copy offinish_wait).I think the original actual _problem_ was a -stable issue that was fixed instead by doing additional backports, so this may just avoid potential extra loops and avoid potential compiler shenanigans from the data race.
The point is that the foo_careful() callsites should be commented, please.
PATCH v2 1/1 epoll: ep_autoremove_wake_function should use list_del_init_careful
autoremove_wake_function uses list_del_init_careful, so should epoll's more aggressive variant. It only doesn't because it was copied from an older wait.c rather than the most recent.
--- fs/eventpoll.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
fs/eventpoll.c +5 -1
@@ -1803,11 +1803,15 @@ static struct timespec64 *ep_timeout_to_timespec(struct timespec64 *to, long ms)static int ep_autoremove_wake_function (struct wait_queue_entry *wq_entry,unsigned int mode,int sync,void *key) {int ret =default_wake_function (wq_entry, mode, sync, key); -list_del_init (&wq_entry->entry); +/* +* Pairs with list_empty_careful in ep_poll, and ensures future loop +* iterations see the cause of this wakeup. +*/ +list_del_init_careful (&wq_entry->entry);return ret; }/** * ep_poll - Retrieves ready events, and delivers them to the caller-supplied
Signature · 1 line
2.41.0.rc0.172.g3f132b7071-goog