Messages in this thread | | | From | Marco Elver <> | Date | Fri, 16 Jul 2021 15:02:21 +0200 | Subject | Re: [PATCH 5/5] locking/atomic: add generic arch_*() bitops |
| |
On Fri, 16 Jul 2021 at 14:21, Mark Rutland <mark.rutland@arm.com> wrote: [...] > > Why not just: > > > > #define test_bit arch_test_bit > > > > and similar for the ones that use plain accesses? > > > > And not include instrumented-non-atomic.h here nor do the > > __is_defined(*_uses_plain_access) change to the instrumented header, > > which seems to overcomplicate things as it effectively just aliases the > > non-arch_ name to the arch_ name if *_uses_plain_access is defined. > > I'd done that to still permit the compiler to out-of-line the non-arch > forms if it wanted to. That said, I see that for the atomics we forced > those to be __always_inline anyway, so maybe that's not a concern. > > I'm happy either way.
I'd prefer simplicity. In an optimized build, I think even if it decided to not inline, the perf and code size difference is a wash. Intuition tells me that inlining would even be preferred either way, but I've been wrong about that in the past.
I think originally we turned the atomics __always_inline because of uaccess regions. Because debugging tools do work in uaccess regions, arch_* aren't necessarily appropriate. So something like a test_bit() in an uaccess region might actually generate a warning. This might be a valid argument for __always_inline irrespective of optimization potential (if there is any).
Thanks, -- Marco
| |