Re: [GIT PULL] locking fixes

Re: [GIT PULL] locking fixes

[Christian Borntraeger]

Am 21.02.2015 um 02:51 schrieb Linus Torvalds:
> 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.

Sorry for the too late for rc1 answer, but I was traveling the last 4
days.

Hmm, some autocasting feels better, but I could not come up with a proper
solution that works for all cases (e.g. I tried "__auto_type __val = x"
or "typeof(x * 0)" to make this lvalue and rvalue, but all variants failed
in one or the other way).
Unless I can come up with a better solution your union patch is probably
the best way to go and rc1 seems to work.

> 
> 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).

I was getting several complaints from the linux-next buildbots about new
sparse warnings, compile warning and so on when doing this rework, e.g.
commit c5b19946eb76c675 ("kernel: Fix sparse warning for ACCESS_ONCE")
fixes two of those warnings.
So I am somewhat surprised that I never saw this as I am also following the
KVM list. turns out that arch/x86/kernel/kvm.c does not CC the kvm list
in get_maintainers.pl.

Maybe I should push something like that to Paolo/Marcelo.


--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5574,6 +5574,7 @@ S:	Supported
 F:	Documentation/*/kvm*.txt
 F:	Documentation/virtual/kvm/
 F:	arch/*/kvm/
+F:	arch/x86/kernel/kvm.c
 F:	arch/*/include/asm/kvm*
 F:	include/linux/kvm*
 F:	include/uapi/linux/kvm*

Christian




> 
>                             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