From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752985AbbG3N5M (ORCPT ); Thu, 30 Jul 2015 09:57:12 -0400 Received: from e06smtp16.uk.ibm.com ([195.75.94.112]:33101 "EHLO e06smtp16.uk.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752696AbbG3N5K (ORCPT ); Thu, 30 Jul 2015 09:57:10 -0400 X-Helo: d06dlp03.portsmouth.uk.ibm.com X-MailFrom: borntraeger@de.ibm.com X-RcptTo: linux-kernel@vger.kernel.org Message-ID: <55BA2D2E.7080901@de.ibm.com> Date: Thu, 30 Jul 2015 15:57:02 +0200 From: Christian Borntraeger User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Paolo Bonzini , linux-kernel@vger.kernel.org, kvm@vger.kernel.org CC: mtosatti@redhat.com, srutherford@google.com, rkrcmar@redhat.com Subject: Re: [PATCH 2/4] KVM: document memory barriers for kvm->vcpus/kvm->online_vcpus References: <1438176538-26191-1-git-send-email-pbonzini@redhat.com> <1438176538-26191-3-git-send-email-pbonzini@redhat.com> <55BA0D1D.7090002@de.ibm.com> <55BA1C94.80200@redhat.com> In-Reply-To: <55BA1C94.80200@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15073013-0025-0000-0000-0000063A1268 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Am 30.07.2015 um 14:46 schrieb Paolo Bonzini: > > > On 30/07/2015 13:40, Christian Borntraeger wrote: >>>> + /* Pairs with smp_wmb() in kvm_vm_ioctl_create_vcpu, in case >>>> + * the caller has read kvm->online_vcpus before (as is the case >>>> + * for kvm_for_each_vcpu, for example). >>>> + */ >>>> smp_rmb(); >> Hmmm, wouldnt something like smp_mb__after_atomic >> >>>> return kvm->vcpus[i]; >>>> } >>>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c >>>> index 8dc4828f623f..093b3d10b411 100644 >>>> --- a/virt/kvm/kvm_main.c >>>> +++ b/virt/kvm/kvm_main.c >>>> @@ -2206,6 +2206,8 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id) >>>> } >>>> >>>> kvm->vcpus[atomic_read(&kvm->online_vcpus)] = vcpu; >> and smp_mb__before_atomic >> >> be the better function? >> >> >>>> + >>>> + /* Pairs with smp_rmb() in kvm_get_vcpu. */ >>>> smp_wmb(); >>>> atomic_inc(&kvm->online_vcpus); > > The difference between smp_rmb/smp_wmb on one side, and smp_mb on the > other side, is that smp_mb protects previous stores from subsequent loads. > > smp_mb__before_atomic would be overkill for atomic_inc, where there's no > race involving the read of online_vcpus (there's only one writer and > it's protected by kvm->lock). We only care about ordering the > kvm->vcpus store with the kvm->online_vcpus store. Ok, makes sense, if we only want to order these two stores. But then the comment >>>> + /* Pairs with smp_wmb() in kvm_vm_ioctl_create_vcpu, in case >>>> + * the caller has read kvm->online_vcpus before (as is the case >>>> + * for kvm_for_each_vcpu, for example). >>>> + */ is somewhat distracting because of "read" and "before". So something like /* Pairs with smp_wmb() in kvm_vm_ioctl_create_vcpu, to serialize the setting of kvm->vcpus and setting kvm->online_vcpus.... might be better.