From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752341AbbBWIfn (ORCPT ); Mon, 23 Feb 2015 03:35:43 -0500 Received: from e06smtp15.uk.ibm.com ([195.75.94.111]:43722 "EHLO e06smtp15.uk.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751358AbbBWIfk (ORCPT ); Mon, 23 Feb 2015 03:35:40 -0500 Message-ID: <54EAE656.9000504@de.ibm.com> Date: Mon, 23 Feb 2015 09:35:34 +0100 From: Christian Borntraeger User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Linus Torvalds , Ingo Molnar CC: Linux Kernel Mailing List , Peter Zijlstra , Thomas Gleixner , Andrew Morton , linux-next , KVM list Subject: Re: [GIT PULL] locking fixes References: <20150220133756.GA30528@gmail.com> In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15022308-0021-0000-0000-000002FAFA4C Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > wrote: >> How does this work for you at all? >> >> On Fri, Feb 20, 2015 at 5:37 AM, Ingo Molnar 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