Re: [GIT PULL] locking fixes

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 2114 bytes --]

So here's my try at fixing READ_ONCE() so that it is happy with 'const' sources.

It is entirely untested. Comments/testing?

Christian, I guess I could just have forced a cast instead of the
union. I'd like you to take a look at this, because right now it's
holding up me pulling from Ingo.

And Ingo, I think you need to add some kind of test for "horrible new
warnings". I think your pull request *worked*, but the tens of lines
of new warnings it generates is unacceptable, and will just cause me
to undo the pull if I notice in time (like I did this time).

                            Linus

On Fri, Feb 20, 2015 at 4:03 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> How does this work for you at all?
>
> On Fri, Feb 20, 2015 at 5:37 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
>> index 94f643484300..e354cc6446ab 100644
>> --- a/arch/x86/kernel/kvm.c
>> +++ b/arch/x86/kernel/kvm.c
>> @@ -803,8 +808,8 @@ static void kvm_unlock_kick(struct arch_spinlock *lock, __ticket_t ticket)
>>         add_stats(RELEASED_SLOW, 1);
>>         for_each_cpu(cpu, &waiting_cpus) {
>>                 const struct kvm_lock_waiting *w = &per_cpu(klock_waiting, cpu);
>> -               if (ACCESS_ONCE(w->lock) == lock &&
>> -                   ACCESS_ONCE(w->want) == ticket) {
>> +               if (READ_ONCE(w->lock) == lock &&
>> +                   READ_ONCE(w->want) == ticket) {
>>                         add_stats(RELEASED_SLOW_KICKED, 1);
>>                         kvm_kick_cpu(cpu);
>>                         break;
>
> I get horrible compile warnings from this, because of how 'w' is a
> pointer to a 'const' structure, which then causes things like
>
>     include/linux/compiler.h:262:39: warning: passing argument 1 of
> ‘__read_once_size’ discards ‘const’ qualifier from pointer target type
>       ({ typeof(x) __val; __read_once_size(&x, &__val, sizeof(__val)); __val; })
>
> which is fairly hard to avoid (looks like it might need a union)
>
>                        Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 1509 bytes --]

commit 1cb20581b0a32673fa3d056829ca89db4fa0de09
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Fri Feb 20 15:46:31 2015 -0800

    kernel: make READ_ONCE() valid on const pointers
    
    There is certainly nothing wrong with using READ_ONCE() on a const
    pointer, but the helper function __read_once_size() would cause warnings
    because it would drop the 'const' qualifier.
    
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 include/linux/compiler.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index d1ec10a940ff..312cf710e26a 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -202,7 +202,7 @@ static __always_inline void data_access_exceeds_word_size(void)
 {
 }
 
-static __always_inline void __read_once_size(volatile void *p, void *res, int size)
+static __always_inline void __read_once_size(const volatile void *p, void *res, int size)
 {
 	switch (size) {
 	case 1: *(__u8 *)res = *(volatile __u8 *)p; break;
@@ -259,7 +259,7 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
  */
 
 #define READ_ONCE(x) \
-	({ typeof(x) __val; __read_once_size(&x, &__val, sizeof(__val)); __val; })
+	({ union { typeof(x) __val; char __c[1]; } __u; __read_once_size(&(x), __u.__c, sizeof((x))); __u.__val; })
 
 #define WRITE_ONCE(x, val) \
 	({ typeof(x) __val; __val = val; __write_once_size(&x, &__val, sizeof(__val)); __val; })