The cited commit describes that when using writel(), explcit wmb()
is not needed. wmb() is an expensive barrier. writel() uses the needed
platform specific 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 <[email protected]>
---
changelog:
v3->v4:
- further trimmed the documentation for redundant description
v2->v3:
- removed redundant description for writeX()
- updated text for alignment and smaller change lines
- updated commit log with blank line before signed-off-by line
v1->v2:
- Further improved description of writel() example
- changed commit subject from 'usage' to 'example'
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 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index 832b5d36e279..4d24d39f5e42 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -1927,7 +1927,7 @@ 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
+ a dma_wmb(). Note that, when using writel(), a prior 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.
--
2.26.2
On Mon, Oct 10, 2022, at 12:13 PM, 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
> platform specific 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 <[email protected]>
I have no objections, though I still don't see a real need to change
the wording here.
Acked-by: Arnd Bergmann <[email protected]>
On 2022/10/18 5:55, Arnd Bergmann wrote:
> On Mon, Oct 10, 2022, at 12:13 PM, 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
>> platform specific 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 <[email protected]>
>
> I have no objections, though I still don't see a real need to change
> the wording here.
Parav, I think you need a full rewrite of the Changelog as the change
has become a simple substitution of s/wmb()/barrier/.
In second thought, I'm not sure such a substitution is really safe to
make.
"a barrier" can mean "any barrier", which can include a full barrier
in theory.
So I'd rather make the substituted text read something like:
Note that, when using writel(), a prior wmb() or weaker is not
needed to guarantee that the cache coherent memory writes have
completed before writing to the MMIO region.
In my opinion, "or weaker" is redundant for careful readers who are
well aware of context of this example, but won't do no harm.
Thoughts?
Thanks, Akira
>
> Acked-by: Arnd Bergmann <[email protected]>
On Tue, Oct 18, 2022, at 3:37 AM, Akira Yokosawa wrote:
> On 2022/10/18 5:55, Arnd Bergmann wrote:
>> On Mon, Oct 10, 2022, at 12:13 PM, Parav Pandit wrote:
>
> "a barrier" can mean "any barrier", which can include a full barrier
> in theory.
>
> So I'd rather make the substituted text read something like:
>
> Note that, when using writel(), a prior wmb() or weaker is not
> needed to guarantee that the cache coherent memory writes have
> completed before writing to the MMIO region.
>
> In my opinion, "or weaker" is redundant for careful readers who are
> well aware of context of this example, but won't do no harm.
I think that would be more confusing than either of the other variants.
Anything weaker than a full "wmb()" probably makes the driver calling
the writel() non-portable, so that is both vague and incorrect.
The current version works because it specifically mentions the correct
barrier to use, while Parav's version works because it doesn't
make any attempt to name the specific barrier and just states that
adding one is a bad idea regardless.
Arnd
On Tue, 18 Oct 2022 08:44:09 +0200, Arnd Bergmann wrote:
> On Tue, Oct 18, 2022, at 3:37 AM, Akira Yokosawa wrote:
>> On 2022/10/18 5:55, Arnd Bergmann wrote:
>>> On Mon, Oct 10, 2022, at 12:13 PM, Parav Pandit wrote:
>>
>> "a barrier" can mean "any barrier", which can include a full barrier
>> in theory.
>>
>> So I'd rather make the substituted text read something like:
>>
>> Note that, when using writel(), a prior wmb() or weaker is not
>> needed to guarantee that the cache coherent memory writes have
>> completed before writing to the MMIO region.
>>
>> In my opinion, "or weaker" is redundant for careful readers who are
>> well aware of context of this example, but won't do no harm.
>
> I think that would be more confusing than either of the other variants.
>
> Anything weaker than a full "wmb()" probably makes the driver calling
> the writel() non-portable, so that is both vague and incorrect.
Do you mean there is a writel() implementation somewhere in the kernel
which doesn't guarantee an implicit wmb() before MMIO write?
Or do you mean my version is confusing because it can imply a weaker
write barrier is sufficient before writel_relaxed()?
I'm confused...
Thanks, Akira
>
> The current version works because it specifically mentions the correct
> barrier to use, while Parav's version works because it doesn't
> make any attempt to name the specific barrier and just states that
> adding one is a bad idea regardless.
>
> Arnd
On Tue, Oct 18, 2022, at 9:40 AM, Akira Yokosawa wrote:
> On Tue, 18 Oct 2022 08:44:09 +0200, Arnd Bergmann wrote:
>>
>> Anything weaker than a full "wmb()" probably makes the driver calling
>> the writel() non-portable, so that is both vague and incorrect.
>
> Do you mean there is a writel() implementation somewhere in the kernel
> which doesn't guarantee an implicit wmb() before MMIO write?
There are lots of those, but that's not what I meant. E.g. on x86,
writel() does not imply a full wmb() but still guarantees serialization
between DMA and the register access.
> Or do you mean my version is confusing because it can imply a weaker
> write barrier is sufficient before writel_relaxed()?
That's what I meant, yes. On a lot of architectures, it is sufficient
to have something weaker than wmb() before writel_relaxed(), especially
on anything that defines writel_relaxed() to be the same as writel(),
any barrier would technically work. On arm32, using __iowmb() would be
sufficient, and this can be less than a full wmb() but again it's
obviously not portable. These details should not be needed in the
documentation.
Arnd
On Tue, 18 Oct 2022 09:49:34 +0200, Arnd Bergmann wrote:
> On Tue, Oct 18, 2022, at 9:40 AM, Akira Yokosawa wrote:
>> On Tue, 18 Oct 2022 08:44:09 +0200, Arnd Bergmann wrote:
>>>
>>> Anything weaker than a full "wmb()" probably makes the driver calling
>>> the writel() non-portable, so that is both vague and incorrect.
>>
>> Do you mean there is a writel() implementation somewhere in the kernel
>> which doesn't guarantee an implicit wmb() before MMIO write?
>
> There are lots of those, but that's not what I meant. E.g. on x86,
> writel() does not imply a full wmb() but still guarantees serialization
> between DMA and the register access.
>
>> Or do you mean my version is confusing because it can imply a weaker
>> write barrier is sufficient before writel_relaxed()?
>
> That's what I meant, yes. On a lot of architectures, it is sufficient
> to have something weaker than wmb() before writel_relaxed(), especially
> on anything that defines writel_relaxed() to be the same as writel(),
> any barrier would technically work. On arm32, using __iowmb() would be
> sufficient, and this can be less than a full wmb() but again it's
> obviously not portable. These details should not be needed in the
> documentation.
Thanks for the clarification.
I think I was confused by the current wording.
I might be wrong, but I guess Parav's motivation of this change was
to prevent this kind of confusion from the first place.
Parav, may I suggest a reworked changelog? :
The cited commit describes that when using writel(), explcit wmb()
is not needed. However, wmb() can be an expensive barrier depending
on platforms. Arch-specific writel() can use a platform-specific
weaker barrier needed for the guarantee mentioned in section "KERNEL
I/O BARRIER EFFECTS".
Current wording of:
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.
is confusing because it can be interpreted that writel() always has
a barrier equivalent to the heavy-weight wmb(), which is not the case.
Hence stop mentioning wmb() and just call "a prior barrier" in the
notice.
commit 5846581e3563 ("locking/memory-barriers.txt: Fix broken DMA vs. MMIO ordering example")
Am I still missing something?
Thanks, Akira
>
> Arnd
On Mon, Oct 17, 2022 at 10:55:00PM +0200, Arnd Bergmann wrote:
> On Mon, Oct 10, 2022, at 12:13 PM, 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
> > platform specific 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 <[email protected]>
>
> I have no objections, though I still don't see a real need to change
> the wording here.
FWIW, I also don't think this change is necessary. If anything, I'd say
we'd be better off _removing_ the text about writel from this section and
extending the reference to the "KERNEL I/O BARRIER EFFECTS" section,
as you could make similar comments about e.g. readb() and subsequent
barriers.
For example, something like the diff below.
Will
--->8
diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index 06f80e3785c5..93d9a90b7cfa 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -1910,7 +1910,8 @@ There are some more advanced barrier functions:
These are for use with consistent memory to guarantee the ordering
of writes or reads of shared memory accessible to both the CPU and a
- DMA capable device.
+ DMA capable device. See Documentation/core-api/dma-api.rst file for more
+ information about consistent memory.
For example, consider a device driver that shares memory with a device
and uses a descriptor status value to indicate if the descriptor belongs
@@ -1935,18 +1936,15 @@ There are some more advanced barrier functions:
writel(DESC_NOTIFY, doorbell);
}
- The dma_rmb() allows us guarantee the device has released ownership
+ The dma_rmb() allows us to guarantee that the device has released ownership
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.
-
- 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
- more information on consistent memory.
+ can see it now has ownership. dma_mb() implies both a dma_rmb() and
+ a dma_wmb().
+
+ Note that the dma_*() barriers do not provide any ordering guarantees for
+ accesses to MMIO regions. See the later "KERNEL I/O BARRIER EFFECTS"
+ subsection for more information about I/O accessors and MMIO ordering.
(*) pmem_wmb();
On Tue, Oct 18, 2022 at 11:05:55AM +0100, Will Deacon wrote:
> On Mon, Oct 17, 2022 at 10:55:00PM +0200, Arnd Bergmann wrote:
> > On Mon, Oct 10, 2022, at 12:13 PM, 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
> > > platform specific 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 <[email protected]>
> >
> > I have no objections, though I still don't see a real need to change
> > the wording here.
>
> FWIW, I also don't think this change is necessary. If anything, I'd say
> we'd be better off _removing_ the text about writel from this section and
> extending the reference to the "KERNEL I/O BARRIER EFFECTS" section,
> as you could make similar comments about e.g. readb() and subsequent
> barriers.
>
> For example, something like the diff below.
I do like this change, but we might be dealing with two different groups
of readers. Will and Arnd implemented significant parts of the current
MMIO/DMA ordering infrastructure. It is thus quite possible that wording
which suffices to remind them of how things work might or might not help
someone new to Linux who is trying to figure out what is required to make
their driver work.
The traditional resolution of this sort of thing is to provide the
documentation to a newbie and take any resulting confusion seriously.
Parav, thoughts?
Thanx, Paul
> Will
>
> --->8
>
> diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
> index 06f80e3785c5..93d9a90b7cfa 100644
> --- a/Documentation/memory-barriers.txt
> +++ b/Documentation/memory-barriers.txt
> @@ -1910,7 +1910,8 @@ There are some more advanced barrier functions:
>
> These are for use with consistent memory to guarantee the ordering
> of writes or reads of shared memory accessible to both the CPU and a
> - DMA capable device.
> + DMA capable device. See Documentation/core-api/dma-api.rst file for more
> + information about consistent memory.
>
> For example, consider a device driver that shares memory with a device
> and uses a descriptor status value to indicate if the descriptor belongs
> @@ -1935,18 +1936,15 @@ There are some more advanced barrier functions:
> writel(DESC_NOTIFY, doorbell);
> }
>
> - The dma_rmb() allows us guarantee the device has released ownership
> + The dma_rmb() allows us to guarantee that the device has released ownership
> 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.
> -
> - 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
> - more information on consistent memory.
> + can see it now has ownership. dma_mb() implies both a dma_rmb() and
> + a dma_wmb().
> +
> + Note that the dma_*() barriers do not provide any ordering guarantees for
> + accesses to MMIO regions. See the later "KERNEL I/O BARRIER EFFECTS"
> + subsection for more information about I/O accessors and MMIO ordering.
>
> (*) pmem_wmb();
>
>
Hi Paul, Will,
> From: Paul E. McKenney <[email protected]>
> Sent: Tuesday, October 18, 2022 1:49 PM
>
> On Tue, Oct 18, 2022 at 11:05:55AM +0100, Will Deacon wrote:
> > On Mon, Oct 17, 2022 at 10:55:00PM +0200, Arnd Bergmann wrote:
> > > On Mon, Oct 10, 2022, at 12:13 PM, 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 platform specific 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 <[email protected]>
> > >
> > > I have no objections, though I still don't see a real need to change
> > > the wording here.
> >
> > FWIW, I also don't think this change is necessary. If anything, I'd
> > say we'd be better off _removing_ the text about writel from this
> > section and extending the reference to the "KERNEL I/O BARRIER
> > EFFECTS" section, as you could make similar comments about e.g.
> > readb() and subsequent barriers.
> >
> > For example, something like the diff below.
>
> I do like this change, but we might be dealing with two different groups of
> readers. Will and Arnd implemented significant parts of the current
> MMIO/DMA ordering infrastructure. It is thus quite possible that wording
> which suffices to remind them of how things work might or might not help
> someone new to Linux who is trying to figure out what is required to make
> their driver work.
>
> The traditional resolution of this sort of thing is to provide the
> documentation to a newbie and take any resulting confusion seriously.
>
> Parav, thoughts?
I am ok with the change from Will that removes the writel() description.
However, it removes useful short description from the example of "why" writel() is used.
This is useful for newbie and experienced developers both.
So how about below additional change on top of Will's change?
This also aligns to rest of the short C comments in this example pseudo code.
If ok, I will take Will's and mine below change to v5.
index 4d24d39f5e42..5939c5e09570 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -1919,7 +1919,9 @@ There are some more advanced barrier functions:
/* assign ownership */
desc->status = DEVICE_OWN;
- /* notify device of new descriptors */
+ /* Make descriptor status visible to the device followed by
+ * notify device of new descriptors
+ */
writel(DESC_NOTIFY, doorbell);
On Tue, Oct 18, 2022 at 08:33:08PM +0000, Parav Pandit wrote:
> Hi Paul, Will,
>
> > From: Paul E. McKenney <[email protected]>
> > Sent: Tuesday, October 18, 2022 1:49 PM
> >
> > On Tue, Oct 18, 2022 at 11:05:55AM +0100, Will Deacon wrote:
> > > On Mon, Oct 17, 2022 at 10:55:00PM +0200, Arnd Bergmann wrote:
> > > > On Mon, Oct 10, 2022, at 12:13 PM, 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 platform specific 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 <[email protected]>
> > > >
> > > > I have no objections, though I still don't see a real need to change
> > > > the wording here.
> > >
> > > FWIW, I also don't think this change is necessary. If anything, I'd
> > > say we'd be better off _removing_ the text about writel from this
> > > section and extending the reference to the "KERNEL I/O BARRIER
> > > EFFECTS" section, as you could make similar comments about e.g.
> > > readb() and subsequent barriers.
> > >
> > > For example, something like the diff below.
> >
> > I do like this change, but we might be dealing with two different groups of
> > readers. Will and Arnd implemented significant parts of the current
> > MMIO/DMA ordering infrastructure. It is thus quite possible that wording
> > which suffices to remind them of how things work might or might not help
> > someone new to Linux who is trying to figure out what is required to make
> > their driver work.
> >
> > The traditional resolution of this sort of thing is to provide the
> > documentation to a newbie and take any resulting confusion seriously.
> >
> > Parav, thoughts?
>
> I am ok with the change from Will that removes the writel() description.
> However, it removes useful short description from the example of "why" writel() is used.
> This is useful for newbie and experienced developers both.
>
> So how about below additional change on top of Will's change?
> This also aligns to rest of the short C comments in this example pseudo code.
>
> If ok, I will take Will's and mine below change to v5.
>
> index 4d24d39f5e42..5939c5e09570 100644
> --- a/Documentation/memory-barriers.txt
> +++ b/Documentation/memory-barriers.txt
> @@ -1919,7 +1919,9 @@ There are some more advanced barrier functions:
> /* assign ownership */
> desc->status = DEVICE_OWN;
>
> - /* notify device of new descriptors */
> + /* Make descriptor status visible to the device followed by
> + * notify device of new descriptors
> + */
> writel(DESC_NOTIFY, doorbell);
Hearing no objections, please proceed.
Thanx, Paul
> From: Paul E. McKenney <[email protected]>
> Sent: Thursday, October 27, 2022 2:15 PM
>
> On Tue, Oct 18, 2022 at 08:33:08PM +0000, Parav Pandit wrote:
> > Hi Paul, Will,
> >
> > > From: Paul E. McKenney <[email protected]>
> > > Sent: Tuesday, October 18, 2022 1:49 PM
> > >
> > > On Tue, Oct 18, 2022 at 11:05:55AM +0100, Will Deacon wrote:
> > > > On Mon, Oct 17, 2022 at 10:55:00PM +0200, Arnd Bergmann wrote:
> > > > > On Mon, Oct 10, 2022, at 12:13 PM, 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 platform specific 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 <[email protected]>
> > > > >
> > > > > I have no objections, though I still don't see a real need to
> > > > > change the wording here.
> > > >
> > > > FWIW, I also don't think this change is necessary. If anything,
> > > > I'd say we'd be better off _removing_ the text about writel from
> > > > this section and extending the reference to the "KERNEL I/O
> > > > BARRIER EFFECTS" section, as you could make similar comments about
> e.g.
> > > > readb() and subsequent barriers.
> > > >
> > > > For example, something like the diff below.
> > >
> > > I do like this change, but we might be dealing with two different
> > > groups of readers. Will and Arnd implemented significant parts of
> > > the current MMIO/DMA ordering infrastructure. It is thus quite
> > > possible that wording which suffices to remind them of how things
> > > work might or might not help someone new to Linux who is trying to
> > > figure out what is required to make their driver work.
> > >
> > > The traditional resolution of this sort of thing is to provide the
> > > documentation to a newbie and take any resulting confusion seriously.
> > >
> > > Parav, thoughts?
> >
> > I am ok with the change from Will that removes the writel() description.
> > However, it removes useful short description from the example of "why"
> writel() is used.
> > This is useful for newbie and experienced developers both.
> >
> > So how about below additional change on top of Will's change?
> > This also aligns to rest of the short C comments in this example pseudo
> code.
> >
> > If ok, I will take Will's and mine below change to v5.
> >
> > index 4d24d39f5e42..5939c5e09570 100644
> > --- a/Documentation/memory-barriers.txt
> > +++ b/Documentation/memory-barriers.txt
> > @@ -1919,7 +1919,9 @@ There are some more advanced barrier functions:
> > /* assign ownership */
> > desc->status = DEVICE_OWN;
> >
> > - /* notify device of new descriptors */
> > + /* Make descriptor status visible to the device followed by
> > + * notify device of new descriptors
> > + */
> > writel(DESC_NOTIFY, doorbell);
>
> Hearing no objections, please proceed.
>
> Thanx, Paul
Thanks Paul, I will respin v5 shortly.