From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-74.mimecast.com (us-smtp-delivery-74.mimecast.com [63.128.21.74]) by mx.groups.io with SMTP id smtpd.web11.50688.1584964849513503046 for ; Mon, 23 Mar 2020 05:00:49 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Z13xyYgk; spf=pass (domain: redhat.com, ip: 63.128.21.74, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1584964848; 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=EAO33DGKP8DBIh+drBhsMHCGvRNCo5bM55S80Ealjas=; b=Z13xyYgkrGpo+4g7pC9RGHvO0UPB2Xpd3a2nzDHVK5COC9gxtGfKnE+grZg2l4QmwVurfC wTB7DNycHdd520et3IgEt0hsLrYpN0Qe1lOZpo1J1DI0MRlcSAPnlCpkfywdZq6ahmeiib YHHrI60zou8X7jJBbmvYJVZjCH7iyD4= 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-308-dGyTU0rZOdyfm1KUujBsng-1; Mon, 23 Mar 2020 08:00:43 -0400 X-MC-Unique: dGyTU0rZOdyfm1KUujBsng-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 1CF75DBAC; Mon, 23 Mar 2020 12:00:42 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-32.ams2.redhat.com [10.36.112.32]) by smtp.corp.redhat.com (Postfix) with ESMTP id F09C1100EBA7; Mon, 23 Mar 2020 12:00:39 +0000 (UTC) Subject: Re: [edk2-devel] [RFC][PATCH v1] UefiCpuPkg/MpInitLib DXE: Reduce AP status check interval To: "Ni, Ray" , "devel@edk2.groups.io" , "Wu, Hao A" , "rfc@edk2.groups.io" Cc: "Dong, Eric" , "Kinney, Michael D" , "Zeng, Star" References: <20200313132229.7628-1-hao.a.wu@intel.com> <6bf39348-1fcb-fd14-bc16-dd026ed200ad@redhat.com> <734D49CCEBEEF84792F5B80ED585239D5C49B8BF@SHSMSX104.ccr.corp.intel.com> From: "Laszlo Ersek" Message-ID: Date: Mon, 23 Mar 2020 13:00:39 +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: <734D49CCEBEEF84792F5B80ED585239D5C49B8BF@SHSMSX104.ccr.corp.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 On 03/16/20 02:37, Ni, Ray wrote: >> 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. > > Laszlo, > Adding a PCD means platform integrators need to consider which value to set. > Most of the time, they may just use the default PCD value. > Then, why not we add the PCD later when a real case is met? The patch changes existent behavior; it is not for a newly introduced feature. Because most platforms are not in the edk2 tree, we don't know what platforms could be regressed by increasing the polling frequency tenfold. (And remember that the polling action has O(n) cost, where "n" is the logical processor count.) Under your suggestion, the expression "real case is met" amounts to "someone reports a regression" (possibly after the next stable tag, even). I don't think that's a good idea. In particular, the patch is motivated by RegisterCpuFeaturesLib -- the CpuFeaturesInitialize() function -- on some platform(s) that Hao uses. But there are platforms that don't use RegisterCpuFeaturesLib, and still use MpInitLib. Thanks, Laszlo