Linux-Serial Archive on lore.kernel.org
 help / color / Atom feed
From: "Maciej W. Rozycki" <macro@wdc.com>
To: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: "Maciej W. Rozycki" <macro@linux-mips.org>,
	Mikulas Patocka <mpatocka@redhat.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Richard Henderson <rth@twiddle.net>,
	Matt Turner <mattst88@gmail.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	alpha <linux-alpha@vger.kernel.org>,
	linux-serial@vger.kernel.org, linux-rtc@vger.kernel.org
Subject: Re: [PATCH 1/2 v3] alpha: add a delay to inb_p, inb_w and inb_l
Date: Wed, 13 May 2020 18:17:16 +0100 (BST)
Message-ID: <alpine.LFD.2.21.2005131802160.6492@redsun52.ssa.fujisawa.hgst.com> (raw)
In-Reply-To: <20200513144128.GA16995@mail.rc.ru>

On Wed, 13 May 2020, Ivan Kokshaysky wrote:

> >  Individual PCI port locations correspond to different MMIO locations, so 
> > yes, accesses to these can be reordered (merging won't happen due to the 
> > use of the sparse address space).
> 
> Correct, it's how Alpha write buffers work. According to 21064 hardware
> reference manual, these buffers are flushed when one of the following
> conditions is met:
> 
> 1) The write buffer contains at least two valid entries.
> 2) The write buffer contains one valid entry and at least 256 CPU cycles
>    have elapsed since the execution of the last write buffer-directed
>    instruction.
> 3) The write buffer contains an MB, STQ_C or STL_C instruction.
> 4) A load miss is pending to an address currently valid in the write
>    buffer that requires the write buffer to be flushed.
> 
> I'm certain that in these rtc/serial cases we've got readX arriving
> to device *before* preceeding writeX because of 2). That's why small
> delay (300-1400 ns, apparently depends on CPU frequency) seemingly
> "fixes" the problem. The 4) is not met because loads and stores are
> to different ports, and 3) has been broken by commit 92d7223a74.
> 
> So I believe that correct fix would be to revert 92d7223a74 and
> add wmb() before [io]writeX macros to meet memory-barriers.txt
> requirement. The "wmb" instruction is cheap enough and won't hurt
> IO performance too much.

 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.  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 that unlike the Alpha ISA the MIPS ISA does have nontrivial `rmb' 
aka the SYNC_RMB hardware instruction.

  Maciej

  parent reply index

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-06 11:21 [PATCH 1/2] alpha: add a delay between RTC port write and read Mikulas Patocka
2020-05-06 14:20 ` Arnd Bergmann
2020-05-06 17:12   ` [PATCH 1/2 v2] alpha: add a delay to inb_p, inb_w and inb_l Mikulas Patocka
2020-05-07  8:06     ` [PATCH 1/2 v3] " Mikulas Patocka
2020-05-07  8:20       ` Greg Kroah-Hartman
2020-05-07 10:53         ` Mikulas Patocka
2020-05-07 13:30       ` Arnd Bergmann
2020-05-07 14:09         ` Mikulas Patocka
2020-05-07 15:08           ` Arnd Bergmann
2020-05-07 15:45             ` Mikulas Patocka
2020-05-07 15:46             ` [PATCH v4] alpha: add a barrier after outb, outw and outl Mikulas Patocka
2020-05-07 19:12               ` Arnd Bergmann
2020-05-10  1:27                 ` Maciej W. Rozycki
2020-05-10  1:25             ` [PATCH 1/2 v3] alpha: add a delay to inb_p, inb_w and inb_l Maciej W. Rozycki
2020-05-10 18:50               ` Mikulas Patocka
2020-05-11 14:58                 ` Maciej W. Rozycki
2020-05-12 19:35                   ` Mikulas Patocka
2020-05-13 14:41                   ` Ivan Kokshaysky
2020-05-13 16:13                     ` Greg Kroah-Hartman
2020-05-13 17:17                     ` Maciej W. Rozycki [this message]
2020-05-22 13:03                       ` Mikulas Patocka
2020-05-22 13:37                         ` Maciej W. Rozycki
2020-05-22 13:26                     ` Mikulas Patocka
2020-05-22 20:00                       ` Mikulas Patocka
2020-05-23 10:26                         ` [PATCH v4] alpha: fix memory barriers so that they conform to the specification Mikulas Patocka
2020-05-23 15:10                           ` Ivan Kokshaysky
2020-05-23 15:34                             ` Mikulas Patocka
2020-05-23 15:37                               ` [PATCH v5] " Mikulas Patocka
2020-05-24 14:54                                 ` Maciej W. Rozycki
2020-05-25 13:56                                   ` Mikulas Patocka
2020-05-25 14:07                                     ` Arnd Bergmann
2020-05-25 14:45                                     ` Maciej W. Rozycki
2020-05-25 15:53                                       ` [PATCH v6] " Mikulas Patocka
2020-05-26 14:47                                         ` [PATCH v7] " Mikulas Patocka
2020-05-27  0:18                                           ` Maciej W. Rozycki
2020-06-08  6:58                                             ` Mikulas Patocka
2020-06-08 23:49                                               ` Matt Turner
2020-05-25 15:54                                       ` [PATCH v5] " Mikulas Patocka
2020-05-25 16:39                                         ` Maciej W. Rozycki
2020-05-26 14:48                                           ` Mikulas Patocka
2020-05-27  0:23                                             ` Maciej W. Rozycki
2020-05-23 16:44                               ` [PATCH v4] " Maciej W. Rozycki
2020-05-23 17:09                                 ` Mikulas Patocka
2020-05-23 19:27                                   ` Maciej W. Rozycki
2020-05-23 20:11                                     ` Mikulas Patocka

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.LFD.2.21.2005131802160.6492@redsun52.ssa.fujisawa.hgst.com \
    --to=macro@wdc.com \
    --cc=arnd@arndb.de \
    --cc=gregkh@linuxfoundation.org \
    --cc=ink@jurassic.park.msu.ru \
    --cc=linux-alpha@vger.kernel.org \
    --cc=linux-rtc@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=macro@linux-mips.org \
    --cc=mattst88@gmail.com \
    --cc=mpatocka@redhat.com \
    --cc=rth@twiddle.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Serial Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-serial/0 linux-serial/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-serial linux-serial/ https://lore.kernel.org/linux-serial \
		linux-serial@vger.kernel.org
	public-inbox-index linux-serial

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-serial


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git