Re: RFC on writel and writel_relaxed

[Jason Gunthorpe <jgg@ziepe.ca>]

On Tue, Mar 27, 2018 at 09:01:57AM +1100, Benjamin Herrenschmidt wrote:
> On Mon, 2018-03-26 at 17:46 -0400, Sinan Kaya wrote:
> > On 3/26/2018 5:30 PM, Arnd Bergmann wrote:
> > > > But that was never a requirement of writel(),
> > > > Documentation/memory-barriers.txt gives an explicit example demanding
> > > > the wmb() before writel() for ordering system memory against writel.
> > > 
> > > Indeed, but it's in an example for when to use dma_wmb(), not wmb().
> > > Adding Alexander Duyck to Cc, he added that section as part of
> > > 1077fa36f23e ("arch: Add lightweight memory barriers dma_rmb() and
> > > dma_wmb()"). Also adding the other people that were involved with that.
> > > 
> > 
> > ARM developers can get away with not including wmb() in their code and use
> > writel() to observe memory writes due to implicit barriers.
> > 
> > However, same code will not work on Intel.
> 
> Wrong. It will.
> 
> You do NOT need wmb between writes to memory and writel.
> 
> > writel() has a compiler barrier in it for x86.
> > wmb() has a sync operation in it for x86. 
> > 
> > Unless wmb() is called, PCIe device won't observe memory updates from the CPU.
> 
> This is completely wrong. They will. Intel provides the necessary
> ordering guarantees without an explicit wmb.
> 
> Otherwise almost all drivers out there are broken which I very much
> doubt :-)

But.. Sinan is right, you look anywhere in the driver tree and you
find stuff like this:

drivers/net/ethernet/intel/i40e/i40e_txrx.c

        /* Force memory writes to complete before letting h/w
         * know there are new descriptors to fetch.
         */
        wmb();


It is *systemic*

I even see patches adding wmb() based on actual observed memory
corruption during testing on Intel:

https://patchwork.kernel.org/patch/10177207/

So you think all of this is unnecessary and writel is totally strongly
ordered, even on multi-socket Intel?

Jason