powerpc/opal: Fix EBUSY bug in acquiring tokens
PATCH 1/1 powerpc/opal: Fix EBUSY bug in acquiring tokens
The current code checks the completion map to look for the first token that is complete. In some cases, a completion can come in but the token can still be on lease to the caller processing the completion. If this completed but unreleased token is the first token found in the bitmap by another tasks trying to acquire a token, then the __test_and_set_bit call will fail since the token will still be on lease. The acquisition will then fail with an EBUSY.
This patch reorganizes the acquisition code to look at the opal_async_token_map for an unleased token. If the token has no lease it must have no outstanding completions so we should never see an EBUSY, unless we have leased out too many tokens. Since opal_async_get_token_inrerruptible is protected by a semaphore, we will practically never see EBUSY anymore.
--- arch/powerpc/platforms/powernv/opal-async.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
arch/powerpc/platforms/powernv/opal-async.c +3 -3
@@ -39,18 +39,18 @@ int __opal_async_get_token(void)int token;spin_lock_irqsave (&opal_async_comp_lock, flags); - token =find_first_bit (opal_async_complete_map, opal_max_async_tokens); + token =find_first_zero_bit (opal_async_token_map, opal_max_async_tokens);if (token >= opal_max_async_tokens) { token = -EBUSY ; goto out; } -if (__test_and_set_bit (token, opal_async_token_map)) { +if (!__test_and_clear_bit (token, opal_async_complete_map)) { token = -EBUSY ; goto out; } -__clear_bit (token, opal_async_complete_map); +__set_bit (token, opal_async_token_map); out:spin_unlock_irqrestore (&opal_async_comp_lock, flags);
Signature · 1 line
2.14.1.821.g8fa685d3b7-goog
Michael Ellerman
reviewerQuoted 15 lines · William A. Kennington III
The current code checks the completion map to look for the first token that is complete. In some cases, a completion can come in but the token can still be on lease to the caller processing the completion. If this completed but unreleased token is the first token found in the bitmap by another tasks trying to acquire a token, then the
__test_and_set_bitcall will fail since the token will still be on lease. The acquisition will then fail with an EBUSY.This patch reorganizes the acquisition code to look at the
opal_async_token_mapfor an unleased token. If the token has no lease it must have no outstanding completions so we should never see an EBUSY, unless we have leased out too many tokens. Sinceopal_async_get_token_inrerruptibleis protected by a semaphore, we will practically never see EBUSY anymore.
Quoted 4 lines
Signed-off-by William A. Kennington III <[email protected]>--- arch/powerpc/platforms/powernv/opal-async.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
I think this is superseeded by Cyrils rework (which he's finally posted):
http://patchwork.ozlabs.org/patch/833630/
If not please let us know.
cheers
Quoted 28 lines · On Nov 4, 2017, at 2:14 AM, Michael Ellerman <[email protected]…
Quoted 15 lines · "William A. Kennington III" <[email protected] <mailto:[email protected]>> writes:
The current code checks the completion map to look for the first token that is complete. In some cases, a completion can come in but the token can still be on lease to the caller processing the completion. If this completed but unreleased token is the first token found in the bitmap by another tasks trying to acquire a token, then the
__test_and_set_bitcall will fail since the token will still be on lease. The acquisition will then fail with an EBUSY.This patch reorganizes the acquisition code to look at the
opal_async_token_mapfor an unleased token. If the token has no lease it must have no outstanding completions so we should never see an EBUSY, unless we have leased out too many tokens. Sinceopal_async_get_token_inrerruptibleis protected by a semaphore, we will practically never see EBUSY anymore.Quoted 4 lines
Signed-off-by William A. Kennington III <[email protected] <mailto:[email protected]>>---
arch/powerpc/platforms/powernv/opal-async.c| 6 +++---1 file changed, 3 insertions(+), 3 deletions(-)I think this is superseeded by Cyrils rework (which he's finally posted):
Quoted 6 lines
http://patchwork.ozlabs.org/patch/833630/ <http://patchwork.ozlabs.org/patch/833630/>
If not please let us know.
cheers
Yeah, I think Cyril’s rework fixes this. I wasn’t sure how long it would take for master to receive his changes so I figured we could use something in the interim to fix the locking failures. If his changes will be mailed into the next merge window then we should have the issue fixed in master. I understand that rework probably won’t make it into stable kernels? If not then we should probably send this along to stable kernel maintainers.
- William
Michael Ellerman
reviewerQuoted 1 line · William Kennington
Quoted 1 line
On Nov 4, 2017, at 2:14 AM, Michael Ellerman <[email protected] <mailto=
Quoted 2 lines · :[email protected]>> wrote:
Quoted 2 lines
=20
"William A. Kennington III" <[email protected] <mailto:[email protected]>> wri=
tes:
Quoted 17 lines
Quoted 17 lines
=20
Quoted 16 lines
The current code checks the completion map to look for the first token that is complete. In some cases, a completion can come in but the token can still be on lease to the caller processing the completion. If this completed but unreleased token is the first token found in the bitmap by another tasks trying to acquire a token, then the
__test_and_set_bitcall will fail since the token will still be on lease. The acquisition will then fail with an EBUSY.
=20
This patch reorganizes the acquisition code to look at theopal_async_token_mapfor an unleased token. If the token has no lease it must have no outstanding completions so we should never see an EBUSY, unless we have leased out too many tokens. Sinceopal_async_get_token_inrerruptibleis protected by a semaphore, we will practically never see EBUSY anymore.
=20
ogle.com>>
Quoted 8 lines
Quoted 8 lines
Quoted 3 lines
---
arch/powerpc/platforms/powernv/opal-async.c| 6 +++---1 file changed, 3 insertions(+), 3 deletions(-)=20
I think this is superseeded by Cyrils rework (which he's finally posted):
=20
http://patchwork.ozlabs.org/patch/833630/ <http://patchwork.ozlabs.org/=
patch/833630/>
Quoted 4 lines
Quoted 2 lines
=20
If not please let us know.Yeah, I think Cyril=E2=80=99s rework fixes this. I wasn=E2=80=99t sure ho=
w long it
Quoted 4 lines
would take for master to receive his changes so I figured we could use something in the interim to fix the locking failures. If his changes will be mailed into the next merge window then we should have the issue fixed in master. I understand that rework probably won=E2=80=99t ma=
ke it
Quoted 2 lines
into stable kernels? If not then we should probably send this along to stable kernel maintainers.
OK. I didn't realise the bug was sufficiently bad to need a backport to stable.
To make a backport easier I've merged this patch first, and then Cyril's on top of it (which essentially deletes this patch).
I assume you've tested this patch at least somewhat? :)
cheers
On = Nov 4, 2017, at 2:14 AM, Michael Ellerman <[email protected]> = wrote:"William A. Kennington III" = <[email protected]> writes:The current code checks the = completion map to look for the first tokenthat is = complete. In some cases, a completion can come in but the tokencan still be on lease to the caller processing the = completion. If thiscompleted but unreleased token is the = first token found in the bitmap byanother tasks trying to = acquire a token, then the __test_and_set_bitcall will = fail since the token will still be on lease. The acquisitionwill then fail with an EBUSY.This= patch reorganizes the acquisition code to look at theopal_async_token_map for an unleased token. If the token has = no lease itmust have no outstanding completions so we = should never see an EBUSY,unless we have leased out too = many tokens. Sinceopal_async_get_token_inrerruptible is = protected by a semaphore, we willpractically never see = EBUSY anymore.Signed-off-by: William A. = Kennington III <[email protected]>---arch/powerpc/platforms/powernv/opal-async.c | 6 +++---1 file changed, 3 insertions(+), 3 deletions(-)I think this is superseeded by Cyrils = rework (which he's finallyposted): http://patchwork.ozlabs.org/patch/833630/If not please let us know.cheersYeah, I think Cyril=E2=80=99s rework fixes = this. I wasn=E2=80=99t sure how long it would take for master to receive = his changes so I figured we could use something in the interim to fix = the locking failures. If his changes will be mailed into the next merge = window then we should have the issue fixed in master. I understand that = rework probably won=E2=80=99t make it into stable kernels? If not then = we should probably send this along to stable kernel = maintainers.- = William=
Michael Ellerman
AppliedQuoted 8 lines · William A. Kennington III
The current code checks the completion map to look for the first token that is complete. In some cases, a completion can come in but the token can still be on lease to the caller processing the completion. If this completed but unreleased token is the first token found in the bitmap by another tasks trying to acquire a token, then the
__test_and_set_bitcall will fail since the token will still be on lease. The acquisition will then fail with an EBUSY.
Quoted 8 lines
This patch reorganizes the acquisition code to look at the
opal_async_token_mapfor an unleased token. If the token has no lease it must have no outstanding completions so we should never see an EBUSY, unless we have leased out too many tokens. Sinceopal_async_get_token_inrerruptibleis protected by a semaphore, we will practically never see EBUSY anymore.Signed-off-by William A. Kennington III <[email protected]>
Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/71e24d7731a2903b1ae2bba2b2971c
cheers