[PATCH] powerpc/powernv: Fix concurrency issue with npu->mmio_atsd_usage

[PATCH] powerpc/powernv: Fix concurrency issue with npu->mmio_atsd_usage

[Reza Arbab]

We've encountered a performance issue when multiple processors stress
{get,put}_mmio_atsd_reg(). These functions contend for mmio_atsd_usage,
an unsigned long used as a bitmask.

The accesses to mmio_atsd_usage are done using test_and_set_bit_lock()
and clear_bit_unlock(). As implemented, both of these will require a
(successful) stwcx to that same cache line.

What we end up with is thread A, attempting to unlock, being slowed by
other threads repeatedly attempting to lock. A's stwcx instructions fail
and retry because the memory reservation is lost every time a different
thread beats it to the punch.

There may be a long-term way to fix this at a larger scale, but for now
resolve the immediate problem by gating our call to
test_and_set_bit_lock() with one to test_bit(), which is obviously
implemented without using a store.

Signed-off-by: Reza Arbab <arbab@linux.ibm.com>
---
 arch/powerpc/platforms/powernv/npu-dma.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
index 8cdf91f..c773465 100644
--- a/arch/powerpc/platforms/powernv/npu-dma.c
+++ b/arch/powerpc/platforms/powernv/npu-dma.c
@@ -437,8 +437,9 @@ static int get_mmio_atsd_reg(struct npu *npu)
 	int i;
 
 	for (i = 0; i < npu->mmio_atsd_count; i++) {
-		if (!test_and_set_bit_lock(i, &npu->mmio_atsd_usage))
-			return i;
+		if (!test_bit(i, &npu->mmio_atsd_usage))
+			if (!test_and_set_bit_lock(i, &npu->mmio_atsd_usage))
+				return i;
 	}
 
 	return -ENOSPC;
-- 
1.8.3.1

Re: [PATCH] powerpc/powernv: Fix concurrency issue with npu->mmio_atsd_usage

[Alistair Popple]

> There may be a long-term way to fix this at a larger scale, but for now
> resolve the immediate problem by gating our call to
> test_and_set_bit_lock() with one to test_bit(), which is obviously
> implemented without using a store.

I am less sure of this now but am continuing to investigate. However this patch
looks good.

Acked-by: Alistair Popple <alistair@popple.id.au>

> Signed-off-by: Reza Arbab <arbab@linux.ibm.com>
> ---
>  arch/powerpc/platforms/powernv/npu-dma.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
> index 8cdf91f..c773465 100644
> --- a/arch/powerpc/platforms/powernv/npu-dma.c
> +++ b/arch/powerpc/platforms/powernv/npu-dma.c
> @@ -437,8 +437,9 @@ static int get_mmio_atsd_reg(struct npu *npu)
>  	int i;
>  
>  	for (i = 0; i < npu->mmio_atsd_count; i++) {
> -		if (!test_and_set_bit_lock(i, &npu->mmio_atsd_usage))
> -			return i;
> +		if (!test_bit(i, &npu->mmio_atsd_usage))
> +			if (!test_and_set_bit_lock(i, &npu->mmio_atsd_usage))
> +				return i;
>  	}
>  
>  	return -ENOSPC;
> 

Re: powerpc/powernv: Fix concurrency issue with npu->mmio_atsd_usage

[Michael Ellerman]

On Fri, 2018-08-03 at 04:03:36 UTC, Reza Arbab wrote:
> We've encountered a performance issue when multiple processors stress
> {get,put}_mmio_atsd_reg(). These functions contend for mmio_atsd_usage,
> an unsigned long used as a bitmask.
> 
> The accesses to mmio_atsd_usage are done using test_and_set_bit_lock()
> and clear_bit_unlock(). As implemented, both of these will require a
> (successful) stwcx to that same cache line.
> 
> What we end up with is thread A, attempting to unlock, being slowed by
> other threads repeatedly attempting to lock. A's stwcx instructions fail
> and retry because the memory reservation is lost every time a different
> thread beats it to the punch.
> 
> There may be a long-term way to fix this at a larger scale, but for now
> resolve the immediate problem by gating our call to
> test_and_set_bit_lock() with one to test_bit(), which is obviously
> implemented without using a store.
> 
> Signed-off-by: Reza Arbab <arbab@linux.ibm.com>
> Acked-by: Alistair Popple <alistair@popple.id.au>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/9eab9901b015f489199105c470de1f

cheers