Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751365AbdISIZT (ORCPT ); Tue, 19 Sep 2017 04:25:19 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:45708 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750747AbdISIZR (ORCPT ); Tue, 19 Sep 2017 04:25:17 -0400 Subject: Re: [PATCH v2 0/7] swait: Introduce and use swq_has_sleeper() To: Paolo Bonzini , Davidlohr Bueso , mingo@kernel.org, peterz@infradead.org Cc: npiggin@gmail.com, paulmck@linux.vnet.ibm.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, Cornelia Huck References: <20170913200824.28067-1-dave@stgolabs.net> From: Christian Borntraeger Date: Tue, 19 Sep 2017 10:25:11 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 x-cbid: 17091908-0008-0000-0000-000004983B6E X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17091908-0009-0000-0000-00001E296B76 Message-Id: <21d360c5-d98e-896b-ee3d-7fff203f3366@de.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-09-19_04:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000 definitions=main-1709190118 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1707 Lines: 45 On 09/15/2017 01:53 PM, Paolo Bonzini wrote: > On 13/09/2017 22:08, Davidlohr Bueso wrote: >> The following patches fix and/or justify (in baby steps) some of the >> callers. The main exception is s390, which I didn't follow how ->valid_wakeup >> can get hoisted as kvm_vcpu_block does not use that in the wait loop. > > valid_wakeup is just an optimization, so it's not a problem. > > There seems to be always an atomic_or or set_bit before > kvm_s390_vcpu_wakeup is called (except kvm_s390_idle_wakeup which has no > store at all and doesn't need any serialization). So my suggestion is > to add an smp__mb_after_atomic in kvm_s390_vcpu_wakeup; I'll let the > s390 guys do it. I will queue something like this diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c index a832ad0..44239b5 100644 --- a/arch/s390/kvm/interrupt.c +++ b/arch/s390/kvm/interrupt.c @@ -1074,6 +1074,12 @@ void kvm_s390_vcpu_wakeup(struct kvm_vcpu *vcpu) * in kvm_vcpu_block without having the waitqueue set (polling) */ vcpu->valid_wakeup = true; + /* + * This is mostly to document, that the read in swait_active could + * be moved before other stores, leading to subtle races. + * All current users do not store or use an atomic like update + */ + __smp_mb__after_atomic(); if (swait_active(&vcpu->wq)) { /* * The vcpu gave up the cpu voluntarily, mark it as a good but I am asking myself if it is "safer" to make this function use swq_has_sleepers in case we add in a distant future another user to kvm_s390_vcpu_wakeup that does use a normal store and everybody has already forgotten this?