[PATCH v1] locking/memory-barriers.txt: Improve documentation for writel() usage

[PATCH v1] locking/memory-barriers.txt: Improve documentation for writel() usage

[Parav Pandit]

The cited commit describes that when using writel(), explcit wmb()
is not needed. wmb() is an expensive barrier. writel() uses the needed
I/O barrier instead of expensive wmb().

Hence update the example to be more accurate that matches the current
implementation.

commit 5846581e3563 ("locking/memory-barriers.txt: Fix broken DMA vs. MMIO ordering example")
Signed-off-by: Parav Pandit <parav@nvidia.com>

---
changelog:
v0->v1:
- Corrected to mention I/O barrier instead of dma_wmb().
- removed numbered references in commit log
- corrected typo 'explcit' to 'explicit' in commit log
---
 Documentation/memory-barriers.txt | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index 832b5d36e279..2d77a7411e52 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -1927,10 +1927,11 @@ There are some more advanced barrier functions:
      before we read the data from the descriptor, and the dma_wmb() allows
      us to guarantee the data is written to the descriptor before the device
      can see it now has ownership.  The dma_mb() implies both a dma_rmb() and
-     a dma_wmb().  Note that, when using writel(), a prior wmb() is not needed
-     to guarantee that the cache coherent memory writes have completed before
-     writing to the MMIO region.  The cheaper writel_relaxed() does not provide
-     this guarantee and must not be used here.
+     a dma_wmb().  Note that, when using writel(), a prior I/O barrier is not
+     needed to guarantee that the cache coherent memory writes have completed
+     before writing to the MMIO region.  The cheaper writel_relaxed() does not
+     provide this guarantee and must not be used here. Hence, writeX() is always
+     preferred.
 
      See the subsection "Kernel I/O barrier effects" for more information on
      relaxed I/O accessors and the Documentation/core-api/dma-api.rst file for
-- 
2.26.2

Re: [PATCH v1] locking/memory-barriers.txt: Improve documentation for writel() usage

[Joel Fernandes]

Hi,

> On Sep 29, 2022, at 10:04 PM, Parav Pandit <parav@nvidia.com> wrote:
> 
> The cited commit describes that when using writel(), explcit wmb()
> is not needed. wmb() is an expensive barrier. writel() uses the needed
> I/O barrier instead of expensive wmb().

Because you mentioned it in the commit message, Why not mention in the documentation text as well that writel() has the needed I/O barrier in it?

> 
> Hence update the example to be more accurate that matches the current
> implementation.

That would make it more accurate, since accuracy is your goal.

thanks,

 - Joel


> 
> commit 5846581e3563 ("locking/memory-barriers.txt: Fix broken DMA vs. MMIO ordering example")
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> 
> ---
> changelog:
> v0->v1:
> - Corrected to mention I/O barrier instead of dma_wmb().
> - removed numbered references in commit log
> - corrected typo 'explcit' to 'explicit' in commit log
> ---
> Documentation/memory-barriers.txt | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
> index 832b5d36e279..2d77a7411e52 100644
> --- a/Documentation/memory-barriers.txt
> +++ b/Documentation/memory-barriers.txt
> @@ -1927,10 +1927,11 @@ There are some more advanced barrier functions:
>      before we read the data from the descriptor, and the dma_wmb() allows
>      us to guarantee the data is written to the descriptor before the device
>      can see it now has ownership.  The dma_mb() implies both a dma_rmb() and
> -     a dma_wmb().  Note that, when using writel(), a prior wmb() is not needed
> -     to guarantee that the cache coherent memory writes have completed before
> -     writing to the MMIO region.  The cheaper writel_relaxed() does not provide
> -     this guarantee and must not be used here.
> +     a dma_wmb().  Note that, when using writel(), a prior I/O barrier is not
> +     needed to guarantee that the cache coherent memory writes have completed
> +     before writing to the MMIO region.  The cheaper writel_relaxed() does not
> +     provide this guarantee and must not be used here. Hence, writeX() is always
> +     preferred.
> 
>      See the subsection "Kernel I/O barrier effects" for more information on
>      relaxed I/O accessors and the Documentation/core-api/dma-api.rst file for
> -- 
> 2.26.2
> 

RE: [PATCH v1] locking/memory-barriers.txt: Improve documentation for writel() usage

[Parav Pandit]

Hi Joel,

> From: Joel Fernandes <joel@joelfernandes.org>
> Sent: Friday, September 30, 2022 7:41 AM
> 
> Hi,
> 
> > On Sep 29, 2022, at 10:04 PM, Parav Pandit <parav@nvidia.com> wrote:
> >
> > The cited commit describes that when using writel(), explcit wmb() is
> > not needed. wmb() is an expensive barrier. writel() uses the needed
> > I/O barrier instead of expensive wmb().
> 
> Because you mentioned it in the commit message, Why not mention in the
> documentation text as well that writel() has the needed I/O barrier in it?
>
It is already documented in [1].
Though [1] is doesn’t describe about using I/O barrier.

To keep [1] and above example in sync, and to address Akira's comment [2], it makes sense to drop I/O barrier from the above example and reword the line as below

"Note that, when using writel(), a prior barrier is not needed".
 
[1] https://elixir.bootlin.com/linux/latest/source/Documentation/memory-barriers.txt#L2559
[2] https://lore.kernel.org/lkml/5db465f3-698f-ebee-a668-1740a705ce9c@gmail.com/
> >
> > Hence update the example to be more accurate that matches the current
> > implementation.
> 
> That would make it more accurate, since accuracy is your goal.
> 
Yes. Above modified text looks more aligned to writeX() doc.
Will send v2.

Re: [PATCH v1] locking/memory-barriers.txt: Improve documentation for writel() usage

[Akira Yokosawa]

Hi, 

On Fri, 30 Sep 2022 05:03:55 +0300, Parav Pandit wrote:
> The cited commit describes that when using writel(), explcit wmb()
> is not needed. wmb() is an expensive barrier. writel() uses the needed
> I/O barrier instead of expensive wmb().
> 
> Hence update the example to be more accurate that matches the current
> implementation.
> 
> commit 5846581e3563 ("locking/memory-barriers.txt: Fix broken DMA vs. MMIO ordering example")
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> 
> ---
> changelog:
> v0->v1:
> - Corrected to mention I/O barrier instead of dma_wmb().
I don't think dma_wmb() and wmb() belong to "I/O barrier" as far as
memory-barriers.txt is concerned. They are listed in the "CPU MEMORY
BARRIERS" section. dma_wmb() belongs to "advanced barrier functions".

You see, writel() is one of the functions listed in the "KERNEL I/O
BARRIER EFFECTS" section.

Please be consistent with the word choice of the doc you are modifying,
so that any further confusion can be avoided in this infamously 
hard-to-follow document. :-)

Regards,
Akira

> - removed numbered references in commit log
> - corrected typo 'explcit' to 'explicit' in commit log
> ---
>  Documentation/memory-barriers.txt | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
[...]

RE: [PATCH v1] locking/memory-barriers.txt: Improve documentation for writel() usage

[Parav Pandit]

> From: Akira Yokosawa <akiyks@gmail.com>
> Sent: Friday, September 30, 2022 8:41 AM
> 
> Hi,
> 
> On Fri, 30 Sep 2022 05:03:55 +0300, Parav Pandit wrote:
> > The cited commit describes that when using writel(), explcit wmb() is
> > not needed. wmb() is an expensive barrier. writel() uses the needed
> > I/O barrier instead of expensive wmb().
> >
> > Hence update the example to be more accurate that matches the current
> > implementation.
> >
> > commit 5846581e3563 ("locking/memory-barriers.txt: Fix broken DMA vs.
> > MMIO ordering example")
> > Signed-off-by: Parav Pandit <parav@nvidia.com>
> >
> > ---
> > changelog:
> > v0->v1:
> > - Corrected to mention I/O barrier instead of dma_wmb().
> I don't think dma_wmb() and wmb() belong to "I/O barrier" as far as
> memory-barriers.txt is concerned. 

Well, in kernel code for ARM in [1] writel() issues _iowmb() I/O write memory barrier that maps to wmb().
But I agree that in describing the example of interest in this document, it is better to stay away from the detail description and let writeX() explain it.

[1] https://elixir.bootlin.com/linux/latest/source/arch/arm/include/asm/io.h#L290

> They are listed in the "CPU MEMORY
> BARRIERS" section. dma_wmb() belongs to "advanced barrier functions".
> 
> You see, writel() is one of the functions listed in the "KERNEL I/O BARRIER
> EFFECTS" section.
> 
> Please be consistent with the word choice of the doc you are modifying, so
> that any further confusion can be avoided in this infamously hard-to-follow
> document. :-)
>
I understood.
I will rephrase it as "Note that, when using writel(), a prior barrier is not needed"...