Linux-Serial Archive on lore.kernel.org
 help / color / Atom feed
From: "Maciej W. Rozycki" <macro@wdc.com>
To: Mikulas Patocka <mpatocka@redhat.com>
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>,
	"Maciej W. Rozycki" <macro@linux-mips.org>,
	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 v5] alpha: fix memory barriers so that they conform to the specification
Date: Mon, 25 May 2020 15:45:31 +0100 (BST)
Message-ID: <alpine.LFD.2.21.2005251500420.21168@redsun52.ssa.fujisawa.hgst.com> (raw)
In-Reply-To: <alpine.LRH.2.02.2005250944210.26265@file01.intranet.prod.int.rdu2.redhat.com>

On Mon, 25 May 2020, Mikulas Patocka wrote:

> >  Please observe that changing the `*_relaxed' entry points from merely 
> > aliasing the corresponding `__raw_*' handlers to more elaborate code 
> > sequences (though indeed slightly only, but still) makes the situation 
> > analogous to one we have with the ordinary MMIO accessor entry points.  
> > Those regular entry points have been made `extern inline' and wrapped 
> > into:
> > 
> > #if IO_CONCAT(__IO_PREFIX,trivial_rw_bw) == 1
> > 
> > and:
> > 
> > #if IO_CONCAT(__IO_PREFIX,trivial_rw_lq) == 1
> > 
> > respectively, with corresponding out-of-line entry points available, so 
> > that there is no extra inline code produced where the call to the relevant 
> > MMIO accessor is going to end up with an actual function call, as this 
> > would not help performance in any way and would expand code unnecessarily 
> > at all call sites.
> > 
> >  Therefore I suggest that your new `static inline' functions follow the 
> > pattern, perhaps by grouping them with the corresponding ordinary accessor 
> > functions in arch/alpha/include/asm/io.h within the relevant existing 
> > #ifdef, and then by making them `extern inline' and providing out-of-line 
> > implementations in arch/alpha/kernel/io.c, with the individual symbols 
> > exported.  Within arch/alpha/kernel/io.c the compiler will still inline 
> > code as it sees fit as it already does, e.g. `__raw_readq' might get 
> > inlined in `readq' if it turns out cheaper than arranging for an actual 
> > call, including all the stack frame preparation for `ra' preservation; 
> > it's less likely with say `writeq' which probably always ends with a tail 
> > call to `__raw_writeq' as no stack frame is required in that case.
> > 
> >  That for the read accessors.
> 
> I think that making the read*_relaxed functions extern inline just causes 
> source code bloat with no practical gain - if we make them extern inline, 
> we would need two implementations (one in the include file, the other in 
> the C file) - and it is not good practice to duplicate code.

 We do that already with the existing accessors, and while I agree the 
duplication is a bit unfortunate and could be solved with some macro 
hackery so that there is a single version in arch/alpha/include/asm/io.h 
that gets pasted to produce out-of-line copies in arch/alpha/kernel/io.c.  
That would be good having across all the accessors, but that is not 
relevant to the regression discussed here and therefore I do not request 
that you make such a clean-up as a part of this series.

> The functions __raw_read* are already extern inline, so the compiler will 
> inline/noinline them depending on the macros trivial_io_bw and 
> trivial_io_lq - so we can just call them from read*_relaxed without 
> repeating the extern inline pattern.

 The whole point of this peculiar arrangement for cooked accessors is to 
avoid having barriers inserted inline around out-of-line calls to raw 
accessors, and therefore I maintain that we need to have the arrangement 
applied consistently across all the cooked accessors.  Since you're 
creating new distinct cooked accessors (by making their names no longer 
alias to existing cooked accessors), they need to follow the pattern.

 NB this arrangement was added back in 2.6.9-rc3, with:

ChangeSet@1.1939.5.8, 2004-09-22 22:40:06-07:00, rth@kanga.twiddle.home
  [ALPHA] Implement new ioread interface.

I believe.  I don't know if any further details or discussion around that 
can be chased, but Richard might be able to chime in.

  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
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 [this message]
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.2005251500420.21168@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