From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2B386C10F13 for ; Thu, 11 Apr 2019 20:30:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id E12A82073F for ; Thu, 11 Apr 2019 20:30:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726748AbfDKUav (ORCPT ); Thu, 11 Apr 2019 16:30:51 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:47660 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726538AbfDKUav (ORCPT ); Thu, 11 Apr 2019 16:30:51 -0400 Received: from pps.filterd (m0098394.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.27/8.16.0.27) with SMTP id x3BKU0Xi002710 for ; Thu, 11 Apr 2019 16:30:50 -0400 Received: from e32.co.us.ibm.com (e32.co.us.ibm.com [32.97.110.150]) by mx0a-001b2d01.pphosted.com with ESMTP id 2rt9v50h3u-1 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=NOT) for ; Thu, 11 Apr 2019 16:30:50 -0400 Received: from localhost by e32.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 11 Apr 2019 21:30:49 +0100 Received: from b03cxnp08026.gho.boulder.ibm.com (9.17.130.18) by e32.co.us.ibm.com (192.168.1.132) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; (version=TLSv1/SSLv3 cipher=AES256-GCM-SHA384 bits=256/256) Thu, 11 Apr 2019 21:30:47 +0100 Received: from b03ledav002.gho.boulder.ibm.com (b03ledav002.gho.boulder.ibm.com [9.17.130.233]) by b03cxnp08026.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id x3BKUjvf62718088 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 11 Apr 2019 20:30:46 GMT Received: from b03ledav002.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id C0E02136061; Thu, 11 Apr 2019 20:30:45 +0000 (GMT) Received: from b03ledav002.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 089E3136060; Thu, 11 Apr 2019 20:30:44 +0000 (GMT) Received: from [9.85.168.241] (unknown [9.85.168.241]) by b03ledav002.gho.boulder.ibm.com (Postfix) with ESMTP; Thu, 11 Apr 2019 20:30:44 +0000 (GMT) Subject: Re: [RFC v2 2/3] vfio-ccw: Prevent quiesce function going into an infinite loop To: Cornelia Huck Cc: kvm@vger.kernel.org, linux-s390@vger.kernel.org, farman@linux.ibm.com, pasic@linux.ibm.com, pmorel@linux.ibm.com References: <2c17cf29fbce8fc1cfbf60cfd04559d00c8eeac0.1554756534.git.alifm@linux.ibm.com> <20190411182434.07d5f685.cohuck@redhat.com> From: Farhan Ali Date: Thu, 11 Apr 2019 16:30:44 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20190411182434.07d5f685.cohuck@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 x-cbid: 19041120-0004-0000-0000-000014FB91C1 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00010910; HX=3.00000242; KW=3.00000007; PH=3.00000004; SC=3.00000284; SDB=6.01187686; UDB=6.00622139; IPR=6.00968450; MB=3.00026399; MTD=3.00000008; XFM=3.00000015; UTC=2019-04-11 20:30:48 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 19041120-0005-0000-0000-00008B3965EE Message-Id: X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-04-11_13:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 mlxscore=0 impostorscore=0 mlxlogscore=819 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1810050000 definitions=main-1904110134 Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On 04/11/2019 12:24 PM, Cornelia Huck wrote: > On Mon, 8 Apr 2019 17:05:32 -0400 > Farhan Ali wrote: > >> The quiesce function calls cio_cancel_halt_clear() and if we >> get an -EBUSY we go into a loop where we: >> - wait for any interrupts >> - flush all I/O in the workqueue >> - retry cio_cancel_halt_clear >> >> During the period where we are waiting for interrupts or >> flushing all I/O, the channel subsystem could have completed >> a halt/clear action and turned off the corresponding activity >> control bits in the subchannel status word. This means the next >> time we call cio_cancel_halt_clear(), we will again start by >> calling cancel subchannel and so we can be stuck between calling >> cancel and halt forever. >> >> Rather than calling cio_cancel_halt_clear() immediately after >> waiting, let's try to disable the subchannel. If we succeed in >> disabling the subchannel then we know nothing else can happen >> with the device. >> >> Suggested-by: Eric Farman >> Signed-off-by: Farhan Ali >> --- >> drivers/s390/cio/vfio_ccw_drv.c | 27 ++++++++++++--------------- >> 1 file changed, 12 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c >> index 5aca475..4405f2a 100644 >> --- a/drivers/s390/cio/vfio_ccw_drv.c >> +++ b/drivers/s390/cio/vfio_ccw_drv.c >> @@ -43,26 +43,23 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch) >> if (ret != -EBUSY) >> goto out_unlock; >> >> + iretry = 255; >> do { >> - iretry = 255; >> >> ret = cio_cancel_halt_clear(sch, &iretry); >> - while (ret == -EBUSY) { >> - /* >> - * Flush all I/O and wait for >> - * cancel/halt/clear completion. >> - */ >> - private->completion = &completion; >> - spin_unlock_irq(sch->lock); >> - >> + /* >> + * Flush all I/O and wait for >> + * cancel/halt/clear completion. >> + */ >> + private->completion = &completion; >> + spin_unlock_irq(sch->lock); >> + >> + if (ret == -EBUSY) > > I don't think you need to do the unlock/lock and change > private->completion if you don't actually wait, no? If we don't end up waiting, then changing private->completion would not be needed. But we would still need to release the spinlock due to [1]. > > Looking at the possible return codes: > * -ENODEV -> device is not operational anyway, in theory you should even > not need to bother with disabling the subchannel > * -EIO -> we've run out of retries, and the subchannel still is not > idle; I'm not sure if we could do anything here, as disable is > unlikely to work, either We could break out of the loop early for these cases. My thinking was I wanted to depend on the result of trying to disable, because ultimately that's what we want. I can add the cases to break out of the loop early. > * -EBUSY -> we expect an interrupt (or a timeout), the loop looks fine > for that > * 0 -> the one thing that might happen is that we get an unsolicited > interrupt between the successful cancel_halt_clear and the disable; > not even giving up the lock here might even be better here? I didn't think of this case, but if cancel_halt_clear succeeds with 0 then we should wait, no? > > I think this loop will probably work as it is after this patch, but > giving up the lock when not really needed makes me a bit queasy... what > do others think? > >> wait_for_completion_timeout(&completion, 3*HZ); >> >> - private->completion = NULL; >> - flush_workqueue(vfio_ccw_work_q); >> - spin_lock_irq(sch->lock); >> - ret = cio_cancel_halt_clear(sch, &iretry); >> - }; >> - >> + private->completion = NULL; [1] flush_workqueue can go to sleep so we would still need to release spinlock and reacquire it again to try disabling the subchannel. >> + flush_workqueue(vfio_ccw_work_q); >> + spin_lock_irq(sch->lock); >> ret = cio_disable_subchannel(sch); >> } while (ret == -EBUSY); >> out_unlock: > >