On Tue, Mar 27, 2018 at 2:35 PM, Benjamin Herrenschmidt
[off-list ref] wrote:
On Tue, 2018-03-27 at 10:46 -0400, Sinan Kaya wrote:
quoted
combined buffers.
Alex:
"Don't bother. I can tell you right now that for x86 you have to have a
wmb() before the writel().
No, this isn't the semantics of writel. You shouldn't need it unless
something changed and we need to revisit our complete understanding of
*all* MMIO accessor semantics.
The issue seems to be that there have been two different ways of
dealing with this. There has historically been a number of different
drivers that have been carrying this wmb() workaround since something
like 2002. I get that the semantics for writel might have changed
since then, but those of us who already have the wmb() in our drivers
will be very wary of anyone wanting to go through and remove them
since writel is supposed to be "good enough". I would much rather err
on the side of caution here.
I view the wmb() + writel_relaxed() as more of a driver owning and
handling this itself. Besides in the Intel Ethernet driver case it is
better performance as our wmb() placement for us also provides a
secondary barrier so we don't need to add a separate smp_wmb() to deal
with a potential race we have with the Tx cleanup.
At least for UC space, it has always been accepted (and enforced) that
writel would not require any other barrier to order vs. previous stores
to memory.
So the one thing I would question here is if this is UC vs UC or if
this extends to other types as well? So for x86 we could find
references to Write Combining being flushed by a write to UC memory,
however I have yet to find a clear explanation of what a write to UC
does to WB. My personal inclination would be to err on the side of
caution. I just don't want us going through and removing the wmb()
calls because it "should" work. I would want to know for certain it
will work.
- Alex
On 3/27/2018 5:54 PM, Alexander Duyck wrote:
I view the wmb() + writel_relaxed() as more of a driver owning and
handling this itself. Besides in the Intel Ethernet driver case it is
better performance as our wmb() placement for us also provides a
secondary barrier so we don't need to add a separate smp_wmb() to deal
with a potential race we have with the Tx cleanup.
Thanks for the reminder.
I forgot about the double barrier optimization. wmb() + writel_relaxed()
seems to be the best option for Intel network drivers at this moment.
Otherwise, we'll have to remove wmb() and throw in smp barriers there
like you mentioned.
I'll leave the changes in the Intel drivers alone.
--
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.
On Tue, 2018-03-27 at 14:54 -0700, Alexander Duyck wrote:
On Tue, Mar 27, 2018 at 2:35 PM, Benjamin Herrenschmidt
[off-list ref] wrote:
quoted
On Tue, 2018-03-27 at 10:46 -0400, Sinan Kaya wrote:
quoted
combined buffers.
Alex:
"Don't bother. I can tell you right now that for x86 you have to have a
wmb() before the writel().
No, this isn't the semantics of writel. You shouldn't need it unless
something changed and we need to revisit our complete understanding of
*all* MMIO accessor semantics.
The issue seems to be that there have been two different ways of
dealing with this. There has historically been a number of different
drivers that have been carrying this wmb() workaround since something
like 2002. I get that the semantics for writel might have changed
since then, but those of us who already have the wmb() in our drivers
will be very wary of anyone wanting to go through and remove them
since writel is supposed to be "good enough". I would much rather err
on the side of caution here.
I view the wmb() + writel_relaxed() as more of a driver owning and
handling this itself. Besides in the Intel Ethernet driver case it is
better performance as our wmb() placement for us also provides a
secondary barrier so we don't need to add a separate smp_wmb() to deal
with a potential race we have with the Tx cleanup.
quoted
At least for UC space, it has always been accepted (and enforced) that
writel would not require any other barrier to order vs. previous stores
to memory.
So the one thing I would question here is if this is UC vs UC or if
this extends to other types as well? So for x86 we could find
references to Write Combining being flushed by a write to UC memory,
however I have yet to find a clear explanation of what a write to UC
does to WB.
Well, this is the standard write memory + trigger DMA case, the one
specific case for which Linus was adamant we don't need another barrier
back then ...
My personal inclination would be to err on the side of
caution.
Which means writel_relaxed is now pointless ?
We need clear semantics here. In this case the "side of caution" means
we are randomly doing things not understanding what really happens and
that makes me *more* nervous.
I just don't want us going through and removing the wmb()
calls because it "should" work. I would want to know for certain it
will work.
We need to know for certain anyway. Otherwise, all the drivers that do
not have wmb's are potentially broken.
So I dont agree with the status quo.
We need to establish precisely what x86 does, decide what we want the
semantic of writel to be, and implement things accordingly.
Ben.