From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web10.5699.1575013161849510371 for ; Thu, 28 Nov 2019 23:39:27 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=KvPZkeNq; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1575013160; 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=oqPmXJvmo0aloLenZ6e6DlcBruceB++V8deaZAizGxQ=; b=KvPZkeNqQZNPQ/uIrco1BojCgVTbdpx1BurQsc1J+Y2R7K3++JBsuMcqUe60cDOjTEEPKl 8tzmQAsduixKCEqBomuPnm8n80ZUDBtNyiBV+n0vzs8iTbMyZPfRGNMuOPTJIvxmZSpJsU vSDwwiMQ6oH4yXw8n01Xk85kTH/NXgI= 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-300-YC8nywwPPoSp0aQxmQlYhA-1; Fri, 29 Nov 2019 02:39:17 -0500 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id A0FC210054E3; Fri, 29 Nov 2019 07:39:16 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-166.ams2.redhat.com [10.36.116.166]) by smtp.corp.redhat.com (Postfix) with ESMTP id BC79D19C4F; Fri, 29 Nov 2019 07:39:14 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate Token every time. To: "Dong, Eric" , "devel@edk2.groups.io" Cc: "Ni, Ray" , "Gao, Liming" References: <20191128061716.196-1-eric.dong@intel.com> <7ac1f712-5da3-e6b2-f24e-936feb81daa3@redhat.com> From: "Laszlo Ersek" Message-ID: <27cc2a77-2fcf-b242-f588-278a8d1e35ca@redhat.com> Date: Fri, 29 Nov 2019 08:39:13 +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: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-MC-Unique: YC8nywwPPoSp0aQxmQlYhA-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 11/29/19 04:02, Dong, Eric wrote: > Hi Laszlo, > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek > Sent: Thursday, November 28, 2019 9:57 PM > To: Dong, Eric ; devel@edk2.groups.io > Cc: Ni, Ray ; Gao, Liming > Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate Token every time. >> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 56 ++++++++++++++++++++-- >> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 16 +++++++ >> 2 files changed, 68 insertions(+), 4 deletions(-) > > commenting on the header file changes first: > [Eric] what's this sentence means? Follow above comments to update the comment message? Your patch email included the C source file changes first, and the header file changes second. I find that more difficult to reason about than the opposite order (header first, C source second). Therefore, I split your email in two parts, and moved the H file changes to the top. And, I commented on those H file changes first. >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c >> index d8d2b6f444..4632e5b0c2 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c >> @@ -492,6 +492,23 @@ FreeTokens ( >> { >> LIST_ENTRY *Link; >> PROCEDURE_TOKEN *ProcToken; >> + TOKEN_BUFFER *TokenBuf; >> + >> + // >> + // Not free the buffer, just clear the UsedTokenNum. In order to >> + // avoid later trig allcate action again when need to use token. >> + // >> + gSmmCpuPrivate->UsedTokenNum = 0; > > (6) Here we do not zero out the current token buffer, but in > CreateToken() and InitializeDataForMmMp(), we use AllocateZeroPool(). > > This is an inconsistency, we should call either ZeroMem() here (if > zeroing matters), or AllocatePool() in the other two places (if zeroing > does not matter). > [Eric] Not catch your meaning here? Why can't use "=0" here? In both CreateToken() and InitializeDataForMmMp(), we perform *three* actions: (a) ensure CurrentTokenBuf is allocated, (b) clear CurrentTokenBuf, (c) set UsedTokenNum to zero. In FreeTokens(), we perform *two* actions: (a) ensure CurrentTokenBuf is allocated (it needs no explicit action, but it is an invariant nonetheless), (c) set UsedTokenNum to zero. Step (b) is missing from FreeTokens(). That's inconsistent with CreateToken() and InitializeDataForMmMp(). The question is whether the following predicate is important or not: - "all unused tokens in the current token buffer must be all-bits-zero" If this predicate is important, then you should add step (b) to FreeTokens(): ZeroMem ( gSmmCpuPrivate->CurrentTokenBuf, SpinLockSize * MAX_PRE_RESERVE_TOKEN_COUNT ); If the predicate is not important, then you should replace the AllocateZeroPool() calls with AllocatePool(), in CreateToken() and InitializeDataForMmMp(). It is not consistent to clear CurrentTokenBuf in only *some* cases when UsedTokenNum is set to zero. >> @@ -1115,13 +1131,35 @@ CreateToken ( >> VOID >> ) >> { >> - PROCEDURE_TOKEN *ProcToken; >> + PROCEDURE_TOKEN *ProcToken; >> SPIN_LOCK *CpuToken; >> UINTN SpinLockSize; >> + TOKEN_BUFFER *TokenBuf; >> >> SpinLockSize = GetSpinLockProperties (); >> - CpuToken = AllocatePool (SpinLockSize); >> - ASSERT (CpuToken != NULL); >> + >> + if (gSmmCpuPrivate->UsedTokenNum == MAX_PRE_RESERVE_TOKEN_COUNT) { >> + DEBUG((DEBUG_INFO, "CpuSmm: No free token buffer, allocate new buffer!\n")); > > (7) This is an expected case, and not too much a corner case at that. > Furthermore, the DEBUG message is in a performance-sensitive path. > [Eric] this code is called by the caller. I don't think it's performance sensitive. What's your > rule for "performance-sensitive path" ? I add this debug message because I want to know > how often the pre allocate buffer is not enough. We can enlarge the buffer size to get > better performance. The patch is about making CreateToken() faster. It's done by allocating SMRAM less frequently (not on every call to CreateToken()). In some cases however, CreateToken() will still allocate SMRAM, and that's going to be a "spike" in latency, I expect. Adding a DEBUG_INFO to that code path makes the spike worse. It does not affect the throughput of CreateToken() much (the spike is amortized over MAX_PRE_RESERVE_TOKEN_COUNT calls), but it makes the distribution less even. I would use DEBUG_VERBOSE to avoid worsening the spike when a platform build does not ask for DEBUG_VERBOSE explicitly. If you disagree, I can live with DEBUG_INFO. Thanks Laszlo