From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 711C62034C09F for ; Wed, 25 Oct 2017 08:04:02 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D7D443D95F; Wed, 25 Oct 2017 15:07:46 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com D7D443D95F Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-213.rdu2.redhat.com [10.10.120.213]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1E8A66017B; Wed, 25 Oct 2017 15:07:44 +0000 (UTC) To: "Dong, Eric" , "edk2-devel@lists.01.org" Cc: "Ni, Ruiyu" , Paolo Bonzini , Jeff Fan , "Brian J. Johnson" References: <1508743358-3640-1-git-send-email-eric.dong@intel.com> <1508743358-3640-3-git-send-email-eric.dong@intel.com> <4fe39a52-0cd3-de2e-84f2-7363823a1b60@redhat.com> <1329f926-4c33-aaca-108a-7d15ae0503bc@redhat.com> From: Laszlo Ersek Message-ID: <94c27429-4b7d-6c21-c1e5-e07f3e9a5556@redhat.com> Date: Wed, 25 Oct 2017 17:07:44 +0200 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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Wed, 25 Oct 2017 15:07:47 +0000 (UTC) Subject: Re: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Oct 2017 15:04:02 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Eric, On 10/25/17 07:42, Dong, Eric wrote: > Hi Laszlo, > > I think I have clear about your raised issues for Ovmf platform. In this case, I think your platform not suit for this code change. How about I do below change based on the new code: > > - if (CpuMpData->CpuCount == 0) { > TimedWaitForApFinish ( > CpuMpData, > PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1, > PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds) > ); > - } > > After this change, for Ovmf can still set > PcdCpuApInitTimeOutInMicroSeconds to MAX_UINT32 to keep old solution. > For the real platform, it can set a small value to follow the new > solution. For this new change, it just wait one more > PcdCpuApInitTimeOutInMicroSeconds timeout, should not have > performance impact (This time may also been cost in later while > (CpuMpData->MpCpuExchangeInfo->NumApsExecuting != 0) loop) or have > litter performance impact (not cover by the later loop). The suggested change will work for OVMF, thanks! (Just please don't forget to un-indent the TimedWaitForApFinish() call that will become unconditional again.) --o-- Do you think Brian's concern should be investigated as well (perhaps in a separate Bugzilla entry)? Because, I believe, the scheduling pattern that I described earlier could be possible on physical platforms as well, at least in theory: >> (2) After at least one AP has started running, the logic expects >> "NumApsExecuting" to monotonically grow for a while, and then to >> monotonically decrease, back to zero. For example, if we have 1 BSP and >> 7 APs, the BSP logic more or less expects the following values in >> "NumApsExecuting": >> >> 1; 2; 3; 4; 5; 6; 7; >> 6; 5; 4; 3; 2; 1; 0 >> >> >> While this may be a valid expectation for physical processors (which actually >> run in parallel, in the physical world), in a virtual machine, it is not guaranteed. >> Dependent on hypervisor scheduling artifacts, it is possible that, say, three >> APs start up *and finish* before the remaining four APs start up *at all*. The >> INIT-SIPI-SIPI marks all APs for execution / scheduling in the hypervisor, yes, >> but the actual code execution may commence a lot later. For example, the >> BSP may witness the following series of values in "NumApsExecuting": >> >> 1; 2; 3; >> 2; 1; 0; >> 1; 2; 3; 4; >> 3; 2; 1; 0 >> >> and the BSP could think that there are 3 APs only, when it sees the first 0 >> value. I don't know if such a pattern is "likely", "unlikely", or "extremely unlikely" on physical platforms. I think the pattern is "theoretically possible" at least. I suggest that we go ahead with the small patch for the OVMF use case first, and then open a new BZ if Brian thinks there's a real safety problem with the code. ... I should note that, with the small patch that's going to fix the OVMF use case, the previous behavior will be restored for *all* platforms that continue using their previous PCD values. The cumulative effect of commit 0594ec417c89 and the upcoming patch will be that a platform may significantly decrease "PcdCpuApInitTimeOutInMicroSeconds", if it chooses to, and then the "NumApsExecuting" loop will take the role of the preexisting TimedWaitForApFinish() call. If a platform doesn't want that, then it should keep its "PcdCpuApInitTimeOutInMicroSeconds" as high as before, and then any implementation details in the "NumApsExecuting" loop will be irrelevant. Thanks! Laszlo