Skip to contentSkip to conversation

epoll: Fix eventpoll read-lock not writer-fair in PREEMPT_RT

RFC PATCH In review Frederic Weisbecker 3 participants 4 messages Aug 25, 2021 – Aug 27, 2021 VFS

lore
Share & tools
lore

Frederic Weisbecker

author
#0 lore raw

RFC PATCH 1/1 epoll: Fix eventpoll read-lock not writer-fair in PREEMPT_RT

Hi,

Ok the patch is gross but at least this lets me start a discussion about the issue.

---
From d9d66d650b3dac8947a34464dd2e0b546a8c6b63 Mon Sep 17 00:00:00 2001
From: Frederic Weisbecker <[email protected]>
Date: Wed, 25 Aug 2021 14:24:54 +0200
Subject: [RFC PATCH -RT] epoll: Fix eventpoll read-lock not writer-fair in PREEMPT_RT

The eventpoll lock has been converted to an rwlock some time ago with:

a218cc491420 (epoll: use rwlock in order to reduce ep_poll
callback() contention)

Unfortunately this can result in scenarios where a high priority caller of epoll_wait() need to wait for the completion of lower priority wakers.

The typical scenario is:

1) epoll_wait() waits and sleeps for new events in the ep_poll() loop.

2) new events arrive in ep_poll_callback(), the waiter is awaken while ep->lock is read-acquired.

3) The high priority waiter preempts the waker but it can't acquire the write lock in epoll_wait() so it blocks waiting for the low prio waker without priority inheritance.

I guess making readlock writer fair is still not the plan so all I can propose is to make that rwlock build-conditional.

Signed-off-by Frederic Weisbecker <[email protected]>
---
 fs/eventpoll.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
fs/eventpoll.c +8 -2
index 1e596e1d0bba..c1fb4b01ea4f 100644
@@ -1133,7 +1133,10 @@ static int ep_poll_callback(wait_queue_entry_t *wait, unsigned mode, int sync, v
 	unsigned long flags;
 	int ewake = 0;
 
-	read_lock_irqsave(&ep->lock, flags);
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+		read_lock_irqsave(&ep->lock, flags);
+	else
+		write_lock_irqsave(&ep->lock, flags);
 
 	ep_set_busy_poll_napi_id(epi);
 

@@ -1197,7 +1200,10 @@ static int ep_poll_callback(wait_queue_entry_t *wait, unsigned mode, int sync, v
 		pwake++;
 
 out_unlock:
-	read_unlock_irqrestore(&ep->lock, flags);
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+		read_unlock_irqrestore(&ep->lock, flags);
+	else
+		write_unlock_irqrestore(&ep->lock, flags);
 
 	/* We have to call this outside the lock */
 	if (pwake)
Signature · 1 line
2.25.1
#1 lore raw
Quoted 26 lines · Frederic Weisbecker

Hi,

Ok the patch is gross but at least this lets me start a discussion about the issue.

---
From d9d66d650b3dac8947a34464dd2e0b546a8c6b63 Mon Sep 17 00:00:00 2001
From: Frederic Weisbecker <[email protected]>
Date: Wed, 25 Aug 2021 14:24:54 +0200
Subject: [RFC PATCH -RT] epoll: Fix eventpoll read-lock not writer-fair in PREEMPT_RT

The eventpoll lock has been converted to an rwlock some time ago with:

a218cc491420 (epoll: use rwlock in order to reduce ep_poll
callback() contention)

Unfortunately this can result in scenarios where a high priority caller of epoll_wait() need to wait for the completion of lower priority wakers.

The typical scenario is:

1) epoll_wait() waits and sleeps for new events in the ep_poll() loop.

2) new events arrive in ep_poll_callback(), the waiter is awaken while ep->lock is read-acquired.

Quoted 6 lines

3) The high priority waiter preempts the waker but it can't acquire the write lock in epoll_wait() so it blocks waiting for the low prio waker without priority inheritance.

I guess making readlock writer fair is still not the plan so all I can propose is to make that rwlock build-conditional.

It is writer fair in a sense that once a writer attempts to acquire the lock no new reader are allowed in.
What you want is that the writer pi-boosts each reader which is what is not done (multi reader boost). Long ago there was an attempt to make this happen (I think with rwsem) but it turned out to be problematic. There was a workaround by only allowing one reader and doing PI as usual.
This was then dropped because multi-reader became a must have thing for other reasons and in the meantime the lack of pi-boosting wasn't that *problematic* anymore. The problematic user converted in the meantime to RCU having the reading side lockless and the writer had a regular lock.

Quoted 1 line
Signed-off-by Frederic Weisbecker <[email protected]>

Sebastian

#2 lore raw
Quoted 34 lines · Sebastian Andrzej Siewior
Quoted 26 lines · Frederic Weisbecker

Hi,

Ok the patch is gross but at least this lets me start a discussion about the issue.

---
From d9d66d650b3dac8947a34464dd2e0b546a8c6b63 Mon Sep 17 00:00:00 2001
From: Frederic Weisbecker <[email protected]>
Date: Wed, 25 Aug 2021 14:24:54 +0200
Subject: [RFC PATCH -RT] epoll: Fix eventpoll read-lock not writer-fair in PREEMPT_RT

The eventpoll lock has been converted to an rwlock some time ago with:

a218cc491420 (epoll: use rwlock in order to reduce ep_poll
callback() contention)

Unfortunately this can result in scenarios where a high priority caller of epoll_wait() need to wait for the completion of lower priority wakers.

The typical scenario is:

1) epoll_wait() waits and sleeps for new events in the ep_poll() loop.

2) new events arrive in ep_poll_callback(), the waiter is awaken while ep->lock is read-acquired.

Quoted 6 lines

3) The high priority waiter preempts the waker but it can't acquire the write lock in epoll_wait() so it blocks waiting for the low prio waker without priority inheritance.

I guess making readlock writer fair is still not the plan so all I can propose is to make that rwlock build-conditional.

Quoted 8 lines

It is writer fair in a sense that once a writer attempts to acquire the lock no new reader are allowed in.

What you want is that the writer pi-boosts each reader which is what is not done (multi reader boost). Long ago there was an attempt to make this happen (I think with rwsem) but it turned out to be problematic. There was a workaround by only allowing one reader and doing PI as usual.

This patch is essentially forcing that exact workaround for eventpoll.

John Ogness

#3 lore raw
Quoted 2 lines · John Ogness
Quoted 1 line · Sebastian Andrzej Siewior

On 2021-08-25 15:27:54 [+0200], Frederic Weisbecker wrote:

…

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

I guess making readlock writer fair is still not the plan so all I can propose is to make that rwlock build-conditional.

…

Quoted 10 lines
Quoted 8 lines

It is writer fair in a sense that once a writer attempts to acquire the lock no new reader are allowed in.

What you want is that the writer pi-boosts each reader which is what is not done (multi reader boost). Long ago there was an attempt to make this happen (I think with rwsem) but it turned out to be problematic. There was a workaround by only allowing one reader and doing PI as usual.

This patch is essentially forcing that exact workaround for eventpoll.

Frederic ended the mail with "readlock is not writer fair" so I explained that it is and he means something else and this is not not coming. I also suggested between the lines that he might try to move the reader side to RCU.

Quoted 1 line

John Ogness

Sebastian