* arch/riscv doesn't support xchg() on bool
@ 2019-10-21 20:40 Eric Biggers
2019-10-22 1:23 ` Paul Walmsley
0 siblings, 1 reply; 3+ messages in thread
From: Eric Biggers @ 2019-10-21 20:40 UTC (permalink / raw)
To: linux-riscv
Hi,
The kbuild test robot reported a build error on RISC-V in this patch:
https://patchwork.kernel.org/patch/11182389/
... because of the line:
if (!xchg(&mode->logged_impl_name, true)) {
where logged_impl_name is a 'bool'. The problem is that unlike most (or all?)
other kernel architectures, arch/riscv/ doesn't support xchg() on bytes.
See: arch/riscv/include/asm/cmpxchg.h
Is there any chance this could be implemented, to avoid this
architecture-specific quirk? Note, there's at least one other
place in the kernel that also uses xchg() on a bool.
- Eric
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: arch/riscv doesn't support xchg() on bool 2019-10-21 20:40 arch/riscv doesn't support xchg() on bool Eric Biggers @ 2019-10-22 1:23 ` Paul Walmsley 2019-10-22 1:45 ` Eric Biggers 0 siblings, 1 reply; 3+ messages in thread From: Paul Walmsley @ 2019-10-22 1:23 UTC (permalink / raw) To: Eric Biggers; +Cc: linux-riscv, linux-fscrypt, linux-kernel Hi Eric, On Mon, 21 Oct 2019, Eric Biggers wrote: > The kbuild test robot reported a build error on RISC-V in this patch: > > https://patchwork.kernel.org/patch/11182389/ > > ... because of the line: > > if (!xchg(&mode->logged_impl_name, true)) { > > where logged_impl_name is a 'bool'. The problem is that unlike most (or > all?) other kernel architectures, arch/riscv/ doesn't support xchg() on > bytes. When I looked at this in August, it looked like several Linux other architectures - SPARC, Microblaze, C-SKY, and Hexagon - also didn't support xchg() on anything other than 32-bit types: https://lore.kernel.org/lkml/alpine.DEB.2.21.9999.1908161931110.32497@viisi.sifive.com/ Examples: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/sparc/include/asm/cmpxchg_32.h#n18 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/sparc/include/asm/cmpxchg_32.h#n41 > Is there any chance this could be implemented, to avoid this > architecture-specific quirk? It is certainly possible. I wonder whether it is wise. Several of the other architectures implement a software workaround for this operation, and I guess you're advocating that we do the same. We could copy one these implementations. However, the workarounds balloon into quite a lot of code. Here is an example from MIPS: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/kernel/cmpxchg.c#n10 I could be wrong, but I think this expansion would be pretty surprising for most users of xchg(). I suspect most xchg() users are looking for something performant, and would be better served by simply using a variable with a 32-bit type. In the case of your patch, it appears that struct fscrypt_mode.logged_impl_name is only used in the patched function. It looks like it could be promoted into a u32 without much difficulty. Would you be willing to consider that approach of solving the problem? Then the code would be able to take advantage of the fast hardware implementation that's available on many architectures (including RISC-V). > Note, there's at least one other place in the kernel that also uses > xchg() on a bool. Given the nasty compatibility code, I wonder if we'd be better served by removing most of this compatibility code across the kernel, and just requiring callers to use a 32-bit type? For most callers that I've seen, this doesn't seem to be much of an issue; and it would avoid the nasty code involved in software emulations of xchg(). - Paul _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: arch/riscv doesn't support xchg() on bool 2019-10-22 1:23 ` Paul Walmsley @ 2019-10-22 1:45 ` Eric Biggers 0 siblings, 0 replies; 3+ messages in thread From: Eric Biggers @ 2019-10-22 1:45 UTC (permalink / raw) To: Paul Walmsley; +Cc: linux-riscv, linux-fscrypt, linux-kernel Hi Paul, On Mon, Oct 21, 2019 at 06:23:11PM -0700, Paul Walmsley wrote: > Hi Eric, > > On Mon, 21 Oct 2019, Eric Biggers wrote: > > > The kbuild test robot reported a build error on RISC-V in this patch: > > > > https://patchwork.kernel.org/patch/11182389/ > > > > ... because of the line: > > > > if (!xchg(&mode->logged_impl_name, true)) { > > > > where logged_impl_name is a 'bool'. The problem is that unlike most (or > > all?) other kernel architectures, arch/riscv/ doesn't support xchg() on > > bytes. > > When I looked at this in August, it looked like several Linux other > architectures - SPARC, Microblaze, C-SKY, and Hexagon - also didn't > support xchg() on anything other than 32-bit types: > > https://lore.kernel.org/lkml/alpine.DEB.2.21.9999.1908161931110.32497@viisi.sifive.com/ > > Examples: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/sparc/include/asm/cmpxchg_32.h#n18 > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/sparc/include/asm/cmpxchg_32.h#n41 > > > Is there any chance this could be implemented, to avoid this > > architecture-specific quirk? > > It is certainly possible. I wonder whether it is wise. Several of the > other architectures implement a software workaround for this operation, > and I guess you're advocating that we do the same. We could copy one > these implementations. However, the workarounds balloon into quite a lot > of code. Here is an example from MIPS: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/kernel/cmpxchg.c#n10 > > I could be wrong, but I think this expansion would be pretty surprising > for most users of xchg(). I suspect most xchg() users are looking for > something performant, and would be better served by simply using a > variable with a 32-bit type. > > In the case of your patch, it appears that struct > fscrypt_mode.logged_impl_name is only used in the patched function. It > looks like it could be promoted into a u32 without much difficulty. > Would you be willing to consider that approach of solving the problem? > Then the code would be able to take advantage of the fast hardware > implementation that's available on many architectures (including RISC-V). Yes, I already sent a new version of the patch, which changes the variable to an int: https://patchwork.kernel.org/patch/11203003/. I was wondering more about how to stop other people from running into this. > > > Note, there's at least one other place in the kernel that also uses > > xchg() on a bool. > > Given the nasty compatibility code, I wonder if we'd be better served by > removing most of this compatibility code across the kernel, and just > requiring callers to use a 32-bit type? For most callers that I've seen, > this doesn't seem to be much of an issue; and it would avoid the nasty > code involved in software emulations of xchg(). > It's possible that's the better approach; someone would need to go through all the xchg() users and check whether any truly need the 8 or 16-bit support. My main concern was just the annoyance of code that only fails to compile on certain architectures. It should either be one way or the other everywhere. - Eric _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-10-22 1:45 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-10-21 20:40 arch/riscv doesn't support xchg() on bool Eric Biggers 2019-10-22 1:23 ` Paul Walmsley 2019-10-22 1:45 ` Eric Biggers
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).