From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web10.496.1584127116730980626 for ; Fri, 13 Mar 2020 12:18:36 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=R6OynQ7h; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1584127115; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Y/+M5mbx/XN6RS/PbNdWueGBxG77LprzMdLO82VUfHY=; b=R6OynQ7h/MeH36tyfOS7BuGRa9TqHEP9gxB+ejHMXpyZ23/45kKYq/3Lvu7o0DguWieqLD G+L40PXlQ33WwPbYSx+edJgWYzaLqSR+ut3AS2/jI5LWA84PJe/oW+pHiRsvwof9kufyYD GBjwxY68+kfkIHmA0eBoWFmZLE/cY3o= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-45-prWGW3T7Md-8qc--Zns_CA-1; Fri, 13 Mar 2020 15:18:32 -0400 X-MC-Unique: prWGW3T7Md-8qc--Zns_CA-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 8487F107B76B; Fri, 13 Mar 2020 19:18:30 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-172.ams2.redhat.com [10.36.117.172]) by smtp.corp.redhat.com (Postfix) with ESMTP id D01B7100164D; Fri, 13 Mar 2020 19:17:21 +0000 (UTC) Subject: Re: [RFC][PATCH v1] UefiCpuPkg/MpInitLib DXE: Reduce AP status check interval To: Hao A Wu , devel@edk2.groups.io, rfc@edk2.groups.io Cc: Eric Dong , Ray Ni , Michael D Kinney , Star Zeng References: <20200313132229.7628-1-hao.a.wu@intel.com> From: "Laszlo Ersek" Message-ID: <6bf39348-1fcb-fd14-bc16-dd026ed200ad@redhat.com> Date: Fri, 13 Mar 2020 20:17:02 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200313132229.7628-1-hao.a.wu@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Hi Hao, On 03/13/20 14:22, Hao A Wu wrote: > This commit will reduce the interval of the AP status check event from 100 > milliseconds to 10 milliseconds. > > (I searched the history of the 100ms interval, it seems no comment or log > message was mentioned for the choice of this value. Looks like the value > is selected by experience.) > > The purpose is to reduce the response time when the BSP calls below > EFI_MP_SERVICES_PROTOCOL services in a non-blocking manner: > > * StartupAllAPs() > * StartupThisAP() > > Reducing the check interval will benefit the performance for the case when > the BSP uses WaitForEvent() or uses CheckEvent() in a loop to wait for > AP(s) to complete the task, especially when the task can be finished > considerably fast on AP(s). > > An example is within function CpuFeaturesInitialize() under > UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c, > where BSP will perform the same task with APs and requires all the > processors to finish the task before BSP proceeds to its next task. > > Impact: > A. The impact is minimal when there is no non-blocking calls of the > StartupAllAPs/StartupThisAp MP services, because the check function > CheckAndUpdateApsStatus() will return directly when there is no > registered wait event (i.e. no non-blocking request). > > B. There will be a performance tradeoff when BSP continues to proceed > other tasks after submitting a non-blocking StartupAllAPs/StartupThisAP > request. If the AP status check takes a good portion of the shortened > interval, BSP will have less time slice working on its own task before > all the APs complete their tasks. > > My investigation for Impact B is that it is a rare scenario in the edk2 > code base. > > Unitests: > A. OS boot successfully. > B. System (with 24 threads) boot time reduced. Almost all the saved time > comes from the above-mentioned case in CpuFeaturesInitialize(). > > Cc: Eric Dong > Cc: Ray Ni > Cc: Laszlo Ersek > Cc: Michael D Kinney > Cc: Star Zeng > Signed-off-by: Hao A Wu > --- > UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > index a987c32109..9ba886e8ed 100644 > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > @@ -15,7 +15,7 @@ > > #include > > -#define AP_CHECK_INTERVAL (EFI_TIMER_PERIOD_MILLISECONDS (100)) > +#define AP_CHECK_INTERVAL (EFI_TIMER_PERIOD_MILLISECONDS (10)) > #define AP_SAFE_STACK_SIZE 128 > > CPU_MP_DATA *mCpuMpData = NULL; > The use case is valid, IMO. And the commit message is helpful. But I really think this constant should be PCD. Here's why I think a platform might want to control it: - The best (finest) possible resolution for timer events is platform dependent, IIUC. The duration of the "idle tick" is platform-specific. And, it likely makes no sense to set AP_CHECK_INTERVAL to a duration that's around, or under, what the arch timer resolution allows for. - In the other direction, CheckAndUpdateApsStatus() contains a loop that counts up to CpuMpData->CpuCount. In a very large system (hundreds or maybe thousands of APs) this function may have non-negligible cost. I suggest introducing a PCD for this (measured in msecs) and using 100 msecs as the default. Thanks Laszlo