From: Hitoshi Mitake <h.mitake@gmail.com>
To: Ingo Molnar <mingo@elte.hu>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
Matthew Wilcox <matthew.r.wilcox@intel.com>,
Roland Dreier <roland@purestorage.com>,
Andrew Morton <akpm@linux-foundation.org>,
James Bottomley <James.Bottomley@parallels.com>,
linux-kernel@vger.kernel.org, linux-nvme@infradead.org,
hpa@linux.intel.com
Subject: Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq
Date: Tue, 24 Jan 2012 01:05:05 +0900 [thread overview]
Message-ID: <CAMO-S2iegkRWGcnDt0SZqJT3Y3JHQZcHfMY=THKMN7n4Ae0Fyw@mail.gmail.com> (raw)
In-Reply-To: <20120121165830.GA9216@elte.hu>
On Sun, Jan 22, 2012 at 01:58, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Hitoshi Mitake <h.mitake@gmail.com> wrote:
>
>> On Sat, Jan 21, 2012 at 17:28, Ingo Molnar <mingo@elte.hu> wrote:
>> >
>> > * Linus Torvalds <torvalds@linux-foundation.org> wrote:
>> >
>> >> On Thu, Jan 19, 2012 at 5:01 PM, Matthew Wilcox
>> >> <matthew.r.wilcox@intel.com> wrote:
>> >> > The only places that uses readq/writeq are in the initialisation
>> >> > path. Since they're not performance critical, always use readl/writel.
>> >>
>> >> The arch rules are that i fthe architecture has readq/writeq, they
>> >> will be #define'd (they may be inline functions, but there will be a
>> >>
>> >> #define readq readq
>> >>
>> >> to make it visible to the preprocessor as well).
>> >>
>> >> So if you don't need the atomicity guarantees of a "real" readq, you
>> >> can do this instead:
>> >>
>> >> #ifndef readq
>> >> static inline u64 readq(void __iomem *addr)
>> >> {
>> >> return readl(addr) | (((u64) readl(addr + 4)) << 32LL);
>> >> }
>> >> #endif
>> >>
>> >> and then use readq() as if it existed.
>> >>
>> >> And I do think we should expose this in some generic manner. Because
>> >> we currently don't, we already have that pattern copied in quite a few
>> >> drivers.
>> >>
>> >> Maybe <asm-generic/io-nonatomic.h> or something? Making it
>> >> clear that its not atomic, but avoiding the silly duplication
>> >> we do now..
>> >>
>> >> This whole mess was introduced in commit dbee8a0affd5 ("x86:
>> >> remove 32-bit versions of readq()/writeq()"), and it already
>> >> talked about the problems but didn't help with the drivers
>> >> that simply don't care.
>> >>
>> >> All the people in those threads were doing their
>> >> self-satisfied "writeq is broken", without much acknowledging
>> >> that 99% of users simply don't seem to care.
>> >>
>> >> "Occupy Writeq - We are the 99%"
>> >
>> > Agreed, and offering a generic facility for silly duplication
>> > was the motivation of the original commit by Hitoshi Mitake.
>> >
>> > This:
>> >
>> > | The presense of a writeq() implementation on 32-bit x86 that
>> > | splits the 64-bit write into two 32-bit writes turns out to
>> > | break the mpt2sas driver (and in general is risky for drivers
>> > | as was discussed in
>> > | <http://lkml.kernel.org/r/adaab6c1h7c.fsf@cisco.com>).
>> >
>> > is actually a mostly bogus statement and creates more problems
>> > than it solves.
>> >
>> > Hitoshi-san, would you be interested in re-adding the generic
>> > readq/writeq definitions in a slight variation to 2c5643b1c5, to
>> > a separate io-nonatomic.h file, so that drivers that want it can
>> > #include that file and be happy?
>>
>> It sounds nice. In the previous discussion, I suggested that
>> chaning the name of non-atomic readq/writeq to
>> readq_nonatomic/writeq_nonatomic. And James Bottomley
>> replied that it is fine but not really very useful:
>>
>> https://lkml.org/lkml/2011/5/19/13
>>
>> The idea of providing non-atomic readq/writeq in the new file
>> with the name which express non-atomicity clearly might be
>> able to satisfy both of safety and usefulness.
>>
>> It will reduce the duplication of the definition. In addition
>> readq/writeq users don't have to type the long symbols with
>> _nonatomic suffix and can know non-atomicity from the name
>> of header file.
>>
>> I'd like to hear opinions from James, Roland and folks who
>> dislike non-atomic readq/writeq.
>
> Drivers that want the definition add this line to their driver:
>
> #include <asm/io-nonatomic.h>
>
> and then readq()/writeq() does the obvious thing. No need for
> readq_nonatomic()/writeq_nonatomic() - that extra line declares
> things clearly enough and cannot be added accidentally.
>
I wrote the patch which adds the new file include/asm-generic/io-nonatomic.h.
io-nonatomic.h provides non-atomic version readq()/writeq().
The patch also removes some duplicated readq()/writeq() definitions
which are added in the commit: dbee8a0affd5e6eaa5d7c816c4bc233f6f110f50.
The commit was made by Roland Dreier and removed readq()/writeq()
from arch/x86/include/asm.h.
If I find no error after the building allyesconfig and allmodconfig test,
I'll send the patch for review. This may take a few hour.
BTW, I placed io-nonatomic.h under include/asm-generic/ because
non-atomic version readq()/writeq() is architecture dependent.
If you have comments on this point, I'd like to hear.
Thanks,
--
Hitoshi Mitake
h.mitake@gmail.com
next prev parent reply other threads:[~2012-01-23 16:05 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-01-20 1:01 [PATCH] NVMe: Fix compilation on architecturs without readq/writeq Matthew Wilcox
2012-01-20 1:21 ` Linus Torvalds
2012-01-20 17:43 ` Wilcox, Matthew R
2012-01-21 8:28 ` Ingo Molnar
2012-01-21 15:54 ` Hitoshi Mitake
2012-01-21 16:58 ` Ingo Molnar
2012-01-23 16:05 ` Hitoshi Mitake [this message]
2012-01-23 16:57 ` Linus Torvalds
2012-01-23 23:04 ` H. Peter Anvin
2012-01-29 8:02 ` Hitoshi Mitake
2012-01-31 3:03 ` Linus Torvalds
2012-01-31 3:05 ` Linus Torvalds
2012-02-04 15:25 ` Hitoshi Mitake
2012-01-31 11:58 ` Alan Cox
2012-01-31 12:09 ` Ingo Molnar
2012-01-31 12:18 ` Alan Cox
2012-01-31 12:23 ` Ingo Molnar
2012-02-01 23:35 ` Linus Torvalds
2012-02-02 1:05 ` James Bottomley
2012-02-02 1:15 ` Linus Torvalds
2012-02-02 15:05 ` H. Peter Anvin
2012-02-04 15:39 ` Hitoshi Mitake
2012-02-05 6:53 ` Geert Uytterhoeven
2012-02-05 7:01 ` Hitoshi Mitake
2012-02-04 15:34 ` Hitoshi Mitake
2012-02-07 2:48 ` Hitoshi Mitake
2012-02-04 15:24 ` Hitoshi Mitake
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='CAMO-S2iegkRWGcnDt0SZqJT3Y3JHQZcHfMY=THKMN7n4Ae0Fyw@mail.gmail.com' \
--to=h.mitake@gmail.com \
--cc=James.Bottomley@parallels.com \
--cc=akpm@linux-foundation.org \
--cc=hpa@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nvme@infradead.org \
--cc=matthew.r.wilcox@intel.com \
--cc=mingo@elte.hu \
--cc=roland@purestorage.com \
--cc=torvalds@linux-foundation.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).