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

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

[Parav Pandit]

The cited commit [1] describes that when using writel(), explcit wmb()
is not needed. However, it should have said that dma_wmb() is not
needed.

Hence update the example to be more accurate that matches the current
implementation and document section of dma_wmb()/dma_rmb().

[1] commit 5846581e3563 ("locking/memory-barriers.txt: Fix broken DMA vs. MMIO ordering example")
Signed-off-by: Parav Pandit <parav@nvidia.com>
---
 Documentation/memory-barriers.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index 832b5d36e279..cc3a15ac53b3 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -1927,10 +1927,10 @@ 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 dma_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.
 
      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] locking/memory-barriers.txt: Improve documentation for writel() usage

[Bagas Sanjaya]

On 9/15/22 12:01, Parav Pandit wrote:
> The cited commit [1] describes that when using writel(), explcit wmb()
> is not needed. However, it should have said that dma_wmb() is not
> needed.
> 
> Hence update the example to be more accurate that matches the current
> implementation and document section of dma_wmb()/dma_rmb().
> 
> [1] commit 5846581e3563 ("locking/memory-barriers.txt: Fix broken DMA vs. MMIO ordering example")

Just say the blamed commit without using numbered references.


-- 
An old man doll... just what I always wanted! - Clara

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

[Parav Pandit]

> From: Bagas Sanjaya <bagasdotme@gmail.com>
> Sent: Thursday, September 15, 2022 8:16 AM

> On 9/15/22 12:01, Parav Pandit wrote:
> > The cited commit [1] describes that when using writel(), explcit wmb()
> > is not needed. However, it should have said that dma_wmb() is not
> > needed.
> >
> > Hence update the example to be more accurate that matches the current
> > implementation and document section of dma_wmb()/dma_rmb().
> >
> > [1] commit 5846581e3563 ("locking/memory-barriers.txt: Fix broken DMA
> > vs. MMIO ordering example")
> 
> Just say the blamed commit without using numbered references.

Ok. Will send v1 with the change.

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

[Arnd Bergmann]

On Thu, Sep 15, 2022, at 7:01 AM, Parav Pandit wrote:
> The cited commit [1] describes that when using writel(), explcit wmb()
> is not needed. However, it should have said that dma_wmb() is not
> needed.

Are you sure? As I understand it, the dma_wmb() only serializes
a set of memory accesses, but does not serialized against an MMIO
access, which depending on the CPU architecture may require a
different type of barrier.

E.g. on arm, writel() uses __iowmb(), which like wmb() is defined
as "dsb(x); arm_heavy_mb();", while dma_wmb() is a "dmb(oshst)".

     Arnd

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

[Parav Pandit]

> From: Arnd Bergmann <arnd@arndb.de>
> Sent: Thursday, September 15, 2022 8:38 AM
> 
> On Thu, Sep 15, 2022, at 7:01 AM, Parav Pandit wrote:
> > The cited commit [1] describes that when using writel(), explcit wmb()
> > is not needed. However, it should have said that dma_wmb() is not
> > needed.
> 
> Are you sure? As I understand it, the dma_wmb() only serializes a set of
> memory accesses, but does not serialized against an MMIO access, which
> depending on the CPU architecture may require a different type of barrier.
> 
> E.g. on arm, writel() uses __iowmb(), which like wmb() is defined as "dsb(x);
> arm_heavy_mb();", while dma_wmb() is a "dmb(oshst)".

You are right, on arm heavy barrier dsb() is needed, while on arm64, dmb(oshst) is sufficient.

So more accurate documentation is to say that 
'when using writel() a prior IO barrier is not needed ...'

How about that?

It started with my cleanup efforts to two drivers [1] and [2] that had difficulty in using writel() on 32-bit system, and it ended up open coding writel() as wmb() + mlx5_write64().

I am cleaning up the repetitive pattern of, 
wmb();
mlx5_write64()

Before I fix drivers, I thought to improve the documentation that I can follow. :)

[1] https://elixir.bootlin.com/linux/latest/source/drivers/infiniband/hw/mlx5/wr.c#L1042
[2] https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h#L226

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

[Arnd Bergmann]

On Thu, Sep 15, 2022, at 4:18 PM, Parav Pandit wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>> Sent: Thursday, September 15, 2022 8:38 AM
>> 
>> On Thu, Sep 15, 2022, at 7:01 AM, Parav Pandit wrote:
>> > The cited commit [1] describes that when using writel(), explcit wmb()
>> > is not needed. However, it should have said that dma_wmb() is not
>> > needed.
>> 
>> Are you sure? As I understand it, the dma_wmb() only serializes a set of
>> memory accesses, but does not serialized against an MMIO access, which
>> depending on the CPU architecture may require a different type of barrier.
>> 
>> E.g. on arm, writel() uses __iowmb(), which like wmb() is defined as "dsb(x);
>> arm_heavy_mb();", while dma_wmb() is a "dmb(oshst)".
>
> You are right, on arm heavy barrier dsb() is needed, while on arm64, 
> dmb(oshst) is sufficient.
>
> So more accurate documentation is to say that 
> 'when using writel() a prior IO barrier is not needed ...'
>
> How about that?

That's probably fine, not sure if it's worth changing.

> It started with my cleanup efforts to two drivers [1] and [2] that had 
> difficulty in using writel() on 32-bit system, and it ended up open 
> coding writel() as wmb() + mlx5_write64().
>
> I am cleaning up the repetitive pattern of, 
> wmb();
> mlx5_write64()
>
> Before I fix drivers, I thought to improve the documentation that I can 
> follow. :)

Right, that is definitely a good idea.

I see that there is more going on with that function, at least
the loop in post_send_nop() probably just wants to use
__iowrite64_copy(), but that also has no barrier in it, while
changing mlx5_write64() to use iowrite64be() or similar would
of course add excessive barriers inside of the loop.

       Arnd

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

[Parav Pandit]

> From: Arnd Bergmann <arnd@arndb.de>
> Sent: Thursday, September 15, 2022 11:16 AM
> 
> On Thu, Sep 15, 2022, at 4:18 PM, Parav Pandit wrote:
> >> From: Arnd Bergmann <arnd@arndb.de>
> >> Sent: Thursday, September 15, 2022 8:38 AM
> >>
> >> On Thu, Sep 15, 2022, at 7:01 AM, Parav Pandit wrote:
> >> > The cited commit [1] describes that when using writel(), explcit
> >> > wmb() is not needed. However, it should have said that dma_wmb() is
> >> > not needed.
> >>
> >> Are you sure? As I understand it, the dma_wmb() only serializes a set
> >> of memory accesses, but does not serialized against an MMIO access,
> >> which depending on the CPU architecture may require a different type of
> barrier.
> >>
> >> E.g. on arm, writel() uses __iowmb(), which like wmb() is defined as
> >> "dsb(x); arm_heavy_mb();", while dma_wmb() is a "dmb(oshst)".
> >
> > You are right, on arm heavy barrier dsb() is needed, while on arm64,
> > dmb(oshst) is sufficient.
> >
> > So more accurate documentation is to say that 'when using writel() a
> > prior IO barrier is not needed ...'
> >
> > How about that?
> 
> That's probably fine, not sure if it's worth changing.
>
I think it is worth because current documentation, indirectly (or incorrectly) indicate that 
"writel() does wmb() internally, so those drivers, who has difficulty in using writel() can do, wmb() + raw write".
And I sort of see above pattern in two drivers, and it is not good.
It ends up doing dsb(st) on arm64, while needed barrier is only dmb(oshst).

So to fix those two drivers, it is better to first avoid wmb() documentation reference when referring to writel().

 > > It started with my cleanup efforts to two drivers [1] and [2] that had
> > difficulty in using writel() on 32-bit system, and it ended up open
> > coding writel() as wmb() + mlx5_write64().
> >
> > I am cleaning up the repetitive pattern of, wmb();
> > mlx5_write64()
> >
> > Before I fix drivers, I thought to improve the documentation that I
> > can follow. :)
> 
> Right, that is definitely a good idea.
> 
> I see that there is more going on with that function, at least the loop in
> post_send_nop() probably just wants to use __iowrite64_copy(), but that
> also has no barrier in it, while changing mlx5_write64() to use iowrite64be()
> or similar would of course add excessive barriers inside of the loop.

True. All other conversion seems possible.
For post_send_nop(), __iowmb() needs to be exposed, which is not available today and it is only one-off user,
I am inclined to keep post_send_nop()  as-is, but want to improve/correct rest of the callers in these two drivers.

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

[Arnd Bergmann]

On Thu, Sep 15, 2022, at 6:35 PM, Parav Pandit wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>> Sent: Thursday, September 15, 2022 11:16 AM
>> On Thu, Sep 15, 2022, at 4:18 PM, Parav Pandit wrote:
>> >
>> > So more accurate documentation is to say that 'when using writel() a
>> > prior IO barrier is not needed ...'
>> >
>> > How about that?
>> 
>> That's probably fine, not sure if it's worth changing.
>>
> I think it is worth because current documentation, indirectly (or 
> incorrectly) indicate that 
> "writel() does wmb() internally, so those drivers, who has difficulty 
> in using writel() can do, wmb() + raw write".

I don't think it's wrong from a barrier perspective though:
if a driver uses writel_relaxed(), then the only way to guarantee
ordering is to have a full wmb() before it.

> And I sort of see above pattern in two drivers, and it is not good.
> It ends up doing dsb(st) on arm64, while needed barrier is only 
> dmb(oshst).
>
> So to fix those two drivers, it is better to first avoid wmb() 
> documentation reference when referring to writel().

Yes, this suggestion is correct. On x86 and a few others, I think
it's even worse when wmb() is an expensive barrier, while writel()
is the same as writel_relaxed() and the barrier is implied by the
MMIO access.

It might help to spell this out and say that writel() is always
preferred over wmb()+writel_relaxed().

Site note: there are several other problems with wmb()+__raw_writel(),
which on many architectures does not guarantee any atomicity of
the access (a word store could get split into four byte stores),
breaks endianess assumptions and may still not provide the correct
barrier semantics.

>> I see that there is more going on with that function, at least the loop in
>> post_send_nop() probably just wants to use __iowrite64_copy(), but that
>> also has no barrier in it, while changing mlx5_write64() to use iowrite64be()
>> or similar would of course add excessive barriers inside of the loop.
>
> True. All other conversion seems possible.
> For post_send_nop(), __iowmb() needs to be exposed, which is not 
> available today and it is only one-off user,
> I am inclined to keep post_send_nop()  as-is, but want to 
> improve/correct rest of the callers in these two drivers.

__iowmb() is architecture-specific and does not have a well-defined
behavior. wmb() is probably the best choice for post_send_nop().
Alternatively, one could use __iowrite64_copy() for the first few
fields followed by a single writel64be for the last one.

If you think we need something better than that, maybe having
an iowrite64_copy() (without leading __) that includes a barrier
would work.

     Arnd

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

[Parav Pandit]

> From: Arnd Bergmann <arnd@arndb.de>
> Sent: Friday, September 16, 2022 12:09 AM

[..]
> >>
> > I think it is worth because current documentation, indirectly (or
> > incorrectly) indicate that
> > "writel() does wmb() internally, so those drivers, who has difficulty
> > in using writel() can do, wmb() + raw write".
> 
> I don't think it's wrong from a barrier perspective though:
> if a driver uses writel_relaxed(), then the only way to guarantee ordering is
> to have a full wmb() before it.
> 
Sorry for the late response.

Yes. Idea is to avoid wmb() whenever it is not necessary.

I will update the example description to reflect it.

> > And I sort of see above pattern in two drivers, and it is not good.
> > It ends up doing dsb(st) on arm64, while needed barrier is only
> > dmb(oshst).
> >
> > So to fix those two drivers, it is better to first avoid wmb()
> > documentation reference when referring to writel().
> 
> Yes, this suggestion is correct. On x86 and a few others, I think it's even
> worse when wmb() is an expensive barrier, while writel() is the same as
> writel_relaxed() and the barrier is implied by the MMIO access.
> 
> It might help to spell this out and say that writel() is always preferred over
> wmb()+writel_relaxed().
> 
True.

> Site note: there are several other problems with wmb()+__raw_writel(),
> which on many architectures does not guarantee any atomicity of the access
> (a word store could get split into four byte stores), breaks endianess
> assumptions and may still not provide the correct barrier semantics.
>
Hmm. So far didn't observe this on arm64, x86_64, ppc64 yet.
May be because the address is aligned to 8 bytes, we don't see the byte stores?
 
> >> I see that there is more going on with that function, at least the
> >> loop in
> >> post_send_nop() probably just wants to use __iowrite64_copy(), but
> >> that also has no barrier in it, while changing mlx5_write64() to use
> >> iowrite64be() or similar would of course add excessive barriers inside of
> the loop.
> >
> > True. All other conversion seems possible.
> > For post_send_nop(), __iowmb() needs to be exposed, which is not
> > available today and it is only one-off user, I am inclined to keep
> > post_send_nop()  as-is, but want to improve/correct rest of the
> > callers in these two drivers.
> 
> __iowmb() is architecture-specific and does not have a well-defined
> behavior. wmb() is probably the best choice for post_send_nop().
Yes.

> Alternatively, one could use __iowrite64_copy() for the first few fields
> followed by a single writel64be for the last one.
> 
__iowrite64_copy() () seems right fit for post_send_nop() compare t current code.

> If you think we need something better than that, maybe having an
> iowrite64_copy() (without leading __) that includes a barrier would work.

It is only one-off user, and not so critical path, so we can differ iowrite64_copy() for now.

mlx5_write64() variant to use writeX() and avoid wmb() post the documentation update is good start.

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

[Arnd Bergmann]

On Fri, Sep 23, 2022, at 5:55 PM, Parav Pandit wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>> Sent: Friday, September 16, 2022 12:09 AM

>> > And I sort of see above pattern in two drivers, and it is not good.
>> > It ends up doing dsb(st) on arm64, while needed barrier is only
>> > dmb(oshst).
>> >
>> > So to fix those two drivers, it is better to first avoid wmb()
>> > documentation reference when referring to writel().
>> 
>> Yes, this suggestion is correct. On x86 and a few others, I think it's even
>> worse when wmb() is an expensive barrier, while writel() is the same as
>> writel_relaxed() and the barrier is implied by the MMIO access.
>> 
>> It might help to spell this out and say that writel() is always preferred over
>> wmb()+writel_relaxed().
>> 
> True.
>
>> Site note: there are several other problems with wmb()+__raw_writel(),
>> which on many architectures does not guarantee any atomicity of the access
>> (a word store could get split into four byte stores), breaks endianess
>> assumptions and may still not provide the correct barrier semantics.
>>
> Hmm. So far didn't observe this on arm64, x86_64, ppc64 yet.
> May be because the address is aligned to 8 bytes, we don't see the byte stores?

It's complicated. On some architectures (but not the ones you list),
__raw_writel() is a pointer dereference, so the compiler is allowed
to use whichever instruction it wants. Depending on the CPU it
is building for, gcc can decide to split up those stores when it
sees a pointer that was assigned from pointer with lesser alignment
(which is undefined behavior in C). If the pointer is actually
unaligned but gcc does not see this, then powerpc and arm will
trigger an alignment exception for an MMIO location even on CPUs
that can work with unaligned data on normal RAM.

> mlx5_write64() variant to use writeX() and avoid wmb() post the 
> documentation update is good start.

Ok, fair enough, as long as the loop function is not timing
critical itself.

    Arnd