2017-04-07 19:06:37

by Sinan Kaya

[permalink] [raw]
Subject: [PATCH V2] scsi: mpt3sas: remove redundant wmb

Due to relaxed ordering requirements on multiple architectures,
drivers are required to use wmb/rmb/mb combinations when they
need to guarantee observability between the memory and the HW.

The mpt3sas driver is already using wmb() for this purpose.
However, it issues a writel following wmb(). writel() function
on arm/arm64 arhictectures have an embedded wmb() call inside.

This results in unnecessary performance loss and code duplication.

writel already guarantees ordering for both cpu and bus. we don't need
additional wmb()

Signed-off-by: Sinan Kaya <[email protected]>
---
drivers/scsi/mpt3sas/mpt3sas_base.c | 2 --
1 file changed, 2 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 5b7aec5..18039bb 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -1025,7 +1025,6 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
0 : ioc->reply_free_host_index + 1;
ioc->reply_free[ioc->reply_free_host_index] =
cpu_to_le32(reply);
- wmb();
writel(ioc->reply_free_host_index,
&ioc->chip->ReplyFreeHostIndex);
}
@@ -1074,7 +1073,6 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
return IRQ_NONE;
}

- wmb();
if (ioc->is_warpdrive) {
writel(reply_q->reply_post_host_index,
ioc->reply_post_host_index[msix_index]);
--
1.9.1


2017-04-20 02:29:08

by Martin K. Petersen

[permalink] [raw]
Subject: Re: [PATCH V2] scsi: mpt3sas: remove redundant wmb

Sinan Kaya <[email protected]> writes:

> Due to relaxed ordering requirements on multiple architectures,
> drivers are required to use wmb/rmb/mb combinations when they need to
> guarantee observability between the memory and the HW.
>
> The mpt3sas driver is already using wmb() for this purpose. However,
> it issues a writel following wmb(). writel() function on arm/arm64
> arhictectures have an embedded wmb() call inside.
>
> This results in unnecessary performance loss and code duplication.
>
> writel already guarantees ordering for both cpu and bus. we don't need
> additional wmb()

Broadcom folks, please review!

--
Martin K. Petersen Oracle Linux Engineering

2017-04-21 07:56:33

by Sreekanth Reddy

[permalink] [raw]
Subject: Re: [PATCH V2] scsi: mpt3sas: remove redundant wmb

On Thu, Apr 20, 2017 at 7:58 AM, Martin K. Petersen
<[email protected]> wrote:
> Sinan Kaya <[email protected]> writes:
>
>> Due to relaxed ordering requirements on multiple architectures,
>> drivers are required to use wmb/rmb/mb combinations when they need to
>> guarantee observability between the memory and the HW.
>>
>> The mpt3sas driver is already using wmb() for this purpose. However,
>> it issues a writel following wmb(). writel() function on arm/arm64
>> arhictectures have an embedded wmb() call inside.

[Sreekanth] Whether same thing applicable for SPARC & POWER
architectures. If yes then we are fine with this patch changes.

>>
>> This results in unnecessary performance loss and code duplication.
>>
>> writel already guarantees ordering for both cpu and bus. we don't need
>> additional wmb()
>
> Broadcom folks, please review!
>
> --
> Martin K. Petersen Oracle Linux Engineering

2017-04-21 13:47:57

by Sinan Kaya

[permalink] [raw]
Subject: Re: [PATCH V2] scsi: mpt3sas: remove redundant wmb

On 4/21/2017 3:56 AM, Sreekanth Reddy wrote:
> [Sreekanth] Whether same thing applicable for SPARC & POWER
> architectures. If yes then we are fine with this patch changes.

This behavior is common for all architectures according to this document.

Who would be the best person to comment on SPARC and POWER architectures
in specific? James and I exchanged some comments on the first version.

James? can you comment on POWER behavior.

https://www.kernel.org/doc/Documentation/memory-barriers.txt

Inside of the Linux kernel, I/O should be done through the appropriate accessor
routines - such as inb() or writel() - which know how to make such accesses
appropriately sequential.

"Whilst this, for the most part, renders the explicit
use of memory barriers unnecessary",

there are a couple of situations where they might be needed:

(1) On some systems, I/O stores are not strongly ordered across all CPUs, and
so for _all_ general drivers locks should be used and mmiowb() must be
issued prior to unlocking the critical section.

(2) If the accessor functions are used to refer to an I/O memory window with
relaxed memory access properties, then _mandatory_ memory barriers are
required to enforce ordering.

--
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

2017-04-21 19:02:46

by Brian King

[permalink] [raw]
Subject: Re: [PATCH V2] scsi: mpt3sas: remove redundant wmb

On 04/21/2017 02:56 AM, Sreekanth Reddy wrote:
> On Thu, Apr 20, 2017 at 7:58 AM, Martin K. Petersen
> <[email protected]> wrote:
>> Sinan Kaya <[email protected]> writes:
>>
>>> Due to relaxed ordering requirements on multiple architectures,
>>> drivers are required to use wmb/rmb/mb combinations when they need to
>>> guarantee observability between the memory and the HW.
>>>
>>> The mpt3sas driver is already using wmb() for this purpose. However,
>>> it issues a writel following wmb(). writel() function on arm/arm64
>>> arhictectures have an embedded wmb() call inside.
>
> [Sreekanth] Whether same thing applicable for SPARC & POWER
> architectures. If yes then we are fine with this patch changes.

This is also true for Power.

Reviewed-by: Brian King <[email protected]>

-Brian

--
Brian King
Power Linux I/O
IBM Linux Technology Center

2017-04-24 22:33:29

by Martin K. Petersen

[permalink] [raw]
Subject: Re: [PATCH V2] scsi: mpt3sas: remove redundant wmb


Sinan,

> Due to relaxed ordering requirements on multiple architectures,
> drivers are required to use wmb/rmb/mb combinations when they need to
> guarantee observability between the memory and the HW.

Applied to 4.12/scsi-queue, thanks!

--
Martin K. Petersen Oracle Linux Engineering