Re: [PATCH 1/2 v3] alpha: add a delay to inb_p, inb_w and inb_l

["Maciej W. Rozycki" <macro@wdc.com>]

On Fri, 22 May 2020, Mikulas Patocka wrote:

> >  Hmm, having barriers *afterwards* across all the MMIO accessors, even 
> > ones that do not have such a requirement according to memory-barriers.txt, 
> > does hurt performance unnecessarily however.  What I think has to be done 
> > is adding barriers beforehand, and then only add barriers afterwards where 
> > necessary.  Commit 92d7223a74 did a part of that, but did not consistently 
> > update all the remaining accessors.
> > 
> >  So I don't think reverting 92d7223a74 permanently is the right way to go, 
> > however it certainly makes sense to do that temporarily to get rid of the 
> > fatal regression, sort all the corner cases and then apply the resulting 
> > replacement fix.
> 
> See Documentation/memory-barriers.txt, the section "KERNEL I/O BARRIER 
> EFFECTS"
> 
> According to the specification, there must be a barrier before a write to 
> the MMIO space (paragraph 3) and after a read from MMIO space (parahraph 
> 4) - if this causes unnecessary slowdown, the driver should use 
> readX_relaxed or writeX_relaxed functions - the *_relaxed functions are 
> ordered with each other (see the paragraph "(*) readX_relaxed(), 
> writeX_relaxed()"), but they are not ordered with respect to memory 
> access.

 The specification doesn't require a barrier *after* a write however, 
which is what I have been concerned about as it may cost hundreds of 
cycles wasted.  I'm not concerned about a barrier after a read (and one 
beforehand is of course also required).

> The commit 92d7223a74 fixes that requirement (although there is no real 
> driver that was fixed by this), so I don't think it should be reverted. 
> The proper fix should be to add delays to the serial port and readltime 
> clock (or perhaps to all IO-port accesses).

 Adding artificial delays will only paper over the real problem I'm 
afraid.

> > I think ultimately we do want the barriers beforehand, just like the 
> > MIPS port has (and survives) in arch/mips/include/asm/io.h.  Observe 
> 
> If the MIPS port doesn't have MMIO barrier after read[bwl], then it is 
> violating the specification. Perhaps there is no existing driver that is 
> hurt by this violation, so this violation survived.

 It does have a barrier, see:

	/* prevent prefetching of coherent DMA data prematurely */	\
	if (!relax)							\
		rmb();							\

In the light of #5 however:

        5. A readX() by a CPU thread from the peripheral will complete before
           any subsequent delay() loop can begin execution on the same thread.

I think it may have to be replaced with a completion barrier however, and 
that will be tricky because you cannot just issue a second read to the 
resource accessed after the `rmb' to make the first read complete, as a 
MMIO read may have side effects (e.g. clearing an interrupt request).  So 
the read would have to happen to a different location.

 Some architectures have a hardware completion barrier instruction, such 
as the modern MIPS ISA, which makes life sweet and easy (as much as life 
can be sweet and easy with a weakly ordered bus model) as no dummy read is 
then required, but surely not all do (including older MIPS ISA revisions).

  Maciej