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.web12.12741.1577977941146262520 for ; Thu, 02 Jan 2020 07:12:21 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=HKfDp6T5; 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=1577977940; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=6r98uM9DSeSugst5kqVy2maXbFVpSaJAXdz/PdOxK5s=; b=HKfDp6T5xaeN91XnZP55XemPyg+cnrf8kOZw+I8qLJZJJ1zf0sfWMr+WX2ZJC9Ai8CPEhm nCy7fIw9VIOItHpDNUIBX6N5AOILiGVTXFE2b3nUbHQlpMGlx8Djcnk6lrxmw4ALtBWDfs 9sWn3oz+Xh3cluHSk0PbWgW13KJ2Ee4= 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-296-kav4o6iWNleJ3m5ds5PAwg-1; Thu, 02 Jan 2020 10:12:16 -0500 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 mimecast-mx01.redhat.com (Postfix) with ESMTPS id A545E801A35; Thu, 2 Jan 2020 15:12:15 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-90.ams2.redhat.com [10.36.117.90]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7D4AF64452; Thu, 2 Jan 2020 15:12:14 +0000 (UTC) Subject: Re: [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Pre-allocate PROCEDURE_TOKEN buffer To: "Ni, Ray" , "Dong, Eric" , "devel@edk2.groups.io" References: <20191227074852.1332-1-eric.dong@intel.com> <734D49CCEBEEF84792F5B80ED585239D5C3D17A0@SHSMSX104.ccr.corp.intel.com> From: "Laszlo Ersek" Message-ID: <53138ceb-61db-6e1d-0743-1ce81525e0fb@redhat.com> Date: Thu, 2 Jan 2020 16:12: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: <734D49CCEBEEF84792F5B80ED585239D5C3D17A0@SHSMSX104.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-MC-Unique: kav4o6iWNleJ3m5ds5PAwg-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 01/02/20 04:30, Ni, Ray wrote: > Reviewed-by: Ray Ni Eric, please go ahead with Ray's R-b. Thanks Laszlo > >> -----Original Message----- >> From: Dong, Eric >> Sent: Friday, December 27, 2019 3:49 PM >> To: devel@edk2.groups.io >> Cc: Ni, Ray ; Laszlo Ersek >> Subject: [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Pre-allocate >> PROCEDURE_TOKEN buffer >> >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2388 >> >> Token is new introduced by MM MP Protocol. Current logic allocate Token >> every time when need to use it. The logic caused SMI latency raised to >> very high. Update logic to allocate Token buffer at driver's entry point. >> Later use the token from the allocated token buffer. Only when all the >> buffer have been used, then need to allocate new buffer. >> >> Former change (9caaa79dd7e078ebb4012dde3b3d3a5d451df609) missed >> PROCEDURE_TOKEN part, this change covers it. >> >> Cc: Ray Ni >> Cc: Laszlo Ersek >> Signed-off-by: Eric Dong >> --- >> >> v2 changes: >> >> Remove the not used variable. >> >> >> UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 190 ++++++++++++------- >> -- >> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 6 +- >> 2 files changed, 109 insertions(+), 87 deletions(-) >> >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c >> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c >> index 4808045f71..870250b0c5 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c >> @@ -429,38 +429,29 @@ ReleaseToken ( >> >> >> **/ >> >> VOID >> >> -FreeTokens ( >> >> +ResetTokens ( >> >> VOID >> >> ) >> >> { >> >> LIST_ENTRY *Link; >> >> PROCEDURE_TOKEN *ProcToken; >> >> - TOKEN_BUFFER *TokenBuf; >> >> >> >> - // >> >> - // Only free the token buffer recorded in the OldTOkenBufList >> >> - // upon exiting SMI. Current token buffer stays allocated so >> >> - // next SMI doesn't need to re-allocate. >> >> - // >> >> - gSmmCpuPrivate->UsedTokenNum = 0; >> >> - >> >> - Link = GetFirstNode (&gSmmCpuPrivate->OldTokenBufList); >> >> - while (!IsNull (&gSmmCpuPrivate->OldTokenBufList, Link)) { >> >> - TokenBuf = TOKEN_BUFFER_FROM_LINK (Link); >> >> - >> >> - Link = RemoveEntryList (&TokenBuf->Link); >> >> - >> >> - FreePool (TokenBuf->Buffer); >> >> - FreePool (TokenBuf); >> >> - } >> >> - >> >> - while (!IsListEmpty (&gSmmCpuPrivate->TokenList)) { >> >> - Link = GetFirstNode (&gSmmCpuPrivate->TokenList); >> >> + Link = GetFirstNode (&gSmmCpuPrivate->TokenList); >> >> + while (!IsNull (&gSmmCpuPrivate->TokenList, Link)) { >> >> ProcToken = PROCEDURE_TOKEN_FROM_LINK (Link); >> >> >> >> - RemoveEntryList (&ProcToken->Link); >> >> + ProcToken->RunningApCount = 0; >> >> + ProcToken->Used = FALSE; >> >> + >> >> + // >> >> + // Check the spinlock status and release it if not released yet. >> >> + // >> >> + if (!AcquireSpinLockOrFail(ProcToken->SpinLock)) { >> >> + DEBUG((DEBUG_ERROR, "Risk::SpinLock still not released!")); >> >> + } >> >> + ReleaseSpinLock (ProcToken->SpinLock); >> >> >> >> - FreePool (ProcToken); >> >> + Link = GetNextNode (&gSmmCpuPrivate->TokenList, Link); >> >> } >> >> } >> >> >> >> @@ -685,9 +676,9 @@ BSPHandler ( >> WaitForAllAPs (ApCount); >> >> >> >> // >> >> - // Clean the tokens buffer. >> >> + // Reset the tokens buffer. >> >> // >> >> - FreeTokens (); >> >> + ResetTokens (); >> >> >> >> // >> >> // Reset BspIndex to -1, meaning BSP has not been elected. >> >> @@ -1056,7 +1047,7 @@ IsTokenInUse ( >> while (!IsNull (&gSmmCpuPrivate->TokenList, Link)) { >> >> ProcToken = PROCEDURE_TOKEN_FROM_LINK (Link); >> >> >> >> - if (ProcToken->SpinLock == Token) { >> >> + if (ProcToken->Used && ProcToken->SpinLock == Token) { >> >> return TRUE; >> >> } >> >> >> >> @@ -1067,61 +1058,112 @@ IsTokenInUse ( >> } >> >> >> >> /** >> >> - create token and save it to the maintain list. >> >> - >> >> - @param RunningApCount Input running AP count. >> >> - >> >> - @retval return the spin lock used as token. >> >> + Allocate buffer for the SPIN_LOCK and PROCEDURE_TOKEN. >> >> >> >> **/ >> >> -PROCEDURE_TOKEN * >> >> -CreateToken ( >> >> - IN UINT32 RunningApCount >> >> +VOID >> >> +AllocateTokenBuffer ( >> >> + VOID >> >> ) >> >> { >> >> - PROCEDURE_TOKEN *ProcToken; >> >> - SPIN_LOCK *SpinLock; >> >> UINTN SpinLockSize; >> >> - TOKEN_BUFFER *TokenBuf; >> >> UINT32 TokenCountPerChunk; >> >> + UINTN ProcTokenSize; >> >> + UINTN Index; >> >> + PROCEDURE_TOKEN *ProcToken; >> >> + SPIN_LOCK *SpinLock; >> >> + UINT8 *SpinLockBuffer; >> >> + UINT8 *ProcTokenBuffer; >> >> >> >> SpinLockSize = GetSpinLockProperties (); >> >> + ProcTokenSize = sizeof (PROCEDURE_TOKEN); >> >> + >> >> TokenCountPerChunk = FixedPcdGet32 >> (PcdCpuSmmMpTokenCountPerChunk); >> >> + ASSERT (TokenCountPerChunk != 0); >> >> + if (TokenCountPerChunk == 0) { >> >> + DEBUG ((DEBUG_ERROR, "PcdCpuSmmMpTokenCountPerChunk should >> not be Zero!\n")); >> >> + CpuDeadLoop (); >> >> + } >> >> + DEBUG ((DEBUG_INFO, "CpuSmm: SpinLock Size = 0x%x, >> PcdCpuSmmMpTokenCountPerChunk = 0x%x\n", SpinLockSize, >> TokenCountPerChunk)); >> >> >> >> - if (gSmmCpuPrivate->UsedTokenNum == TokenCountPerChunk) { >> >> - DEBUG ((DEBUG_VERBOSE, "CpuSmm: No free token buffer, allocate new >> buffer!\n")); >> >> + // >> >> + // Separate the Spin_lock and Proc_token because the alignment requires >> by Spin_Lock. >> >> + // >> >> + SpinLockBuffer = AllocatePool (SpinLockSize * TokenCountPerChunk); >> >> + ASSERT (SpinLockBuffer != NULL); >> >> >> >> - // >> >> - // Record current token buffer for later free action usage. >> >> - // Current used token buffer not in this list. >> >> - // >> >> - TokenBuf = AllocatePool (sizeof (TOKEN_BUFFER)); >> >> - ASSERT (TokenBuf != NULL); >> >> - TokenBuf->Signature = TOKEN_BUFFER_SIGNATURE; >> >> - TokenBuf->Buffer = gSmmCpuPrivate->CurrentTokenBuf; >> >> + ProcTokenBuffer = AllocatePool (ProcTokenSize * TokenCountPerChunk); >> >> + ASSERT (ProcTokenBuffer != NULL); >> >> + >> >> + for (Index = 0; Index < TokenCountPerChunk; Index++) { >> >> + SpinLock = (SPIN_LOCK *)(SpinLockBuffer + SpinLockSize * Index); >> >> + InitializeSpinLock (SpinLock); >> >> + >> >> + ProcToken = (PROCEDURE_TOKEN *)(ProcTokenBuffer + ProcTokenSize * >> Index); >> >> + ProcToken->Signature = PROCEDURE_TOKEN_SIGNATURE; >> >> + ProcToken->SpinLock = SpinLock; >> >> + ProcToken->Used = FALSE; >> >> + ProcToken->RunningApCount = 0; >> >> + >> >> + InsertTailList (&gSmmCpuPrivate->TokenList, &ProcToken->Link); >> >> + } >> >> +} >> >> >> >> - InsertTailList (&gSmmCpuPrivate->OldTokenBufList, &TokenBuf->Link); >> >> +/** >> >> + Find first free token in the allocated token list. >> >> + >> >> + @retval return the first free PROCEDURE_TOKEN. >> >> + >> >> +**/ >> >> +PROCEDURE_TOKEN * >> >> +FindFirstFreeToken ( >> >> + VOID >> >> + ) >> >> +{ >> >> + LIST_ENTRY *Link; >> >> + PROCEDURE_TOKEN *ProcToken; >> >> >> >> - gSmmCpuPrivate->CurrentTokenBuf = AllocatePool (SpinLockSize * >> TokenCountPerChunk); >> >> - ASSERT (gSmmCpuPrivate->CurrentTokenBuf != NULL); >> >> - gSmmCpuPrivate->UsedTokenNum = 0; >> >> + Link = GetFirstNode (&gSmmCpuPrivate->TokenList); >> >> + while (!IsNull (&gSmmCpuPrivate->TokenList, Link)) { >> >> + ProcToken = PROCEDURE_TOKEN_FROM_LINK (Link); >> >> + >> >> + if (!ProcToken->Used) { >> >> + return ProcToken; >> >> + } >> >> + >> >> + Link = GetNextNode (&gSmmCpuPrivate->TokenList, Link); >> >> } >> >> >> >> - SpinLock = (SPIN_LOCK *)(gSmmCpuPrivate->CurrentTokenBuf + >> SpinLockSize * gSmmCpuPrivate->UsedTokenNum); >> >> - gSmmCpuPrivate->UsedTokenNum++; >> >> + return NULL; >> >> +} >> >> + >> >> +/** >> >> + Get the free token. >> >> + >> >> + If no free token, allocate new tokens then return the free one. >> >> + >> >> + @retval return the first free PROCEDURE_TOKEN. >> >> >> >> - InitializeSpinLock (SpinLock); >> >> - AcquireSpinLock (SpinLock); >> >> +**/ >> >> +PROCEDURE_TOKEN * >> >> +GetFreeToken ( >> >> + IN UINT32 RunningApsCount >> >> + ) >> >> +{ >> >> + PROCEDURE_TOKEN *NewToken; >> >> >> >> - ProcToken = AllocatePool (sizeof (PROCEDURE_TOKEN)); >> >> - ASSERT (ProcToken != NULL); >> >> - ProcToken->Signature = PROCEDURE_TOKEN_SIGNATURE; >> >> - ProcToken->SpinLock = SpinLock; >> >> - ProcToken->RunningApCount = RunningApCount; >> >> + NewToken = FindFirstFreeToken (); >> >> + if (NewToken == NULL) { >> >> + AllocateTokenBuffer (); >> >> + NewToken = FindFirstFreeToken (); >> >> + } >> >> + ASSERT (NewToken != NULL); >> >> >> >> - InsertTailList (&gSmmCpuPrivate->TokenList, &ProcToken->Link); >> >> + NewToken->Used = TRUE; >> >> + NewToken->RunningApCount = RunningApsCount; >> >> + AcquireSpinLock (NewToken->SpinLock); >> >> >> >> - return ProcToken; >> >> + return NewToken; >> >> } >> >> >> >> /** >> >> @@ -1231,7 +1273,7 @@ InternalSmmStartupThisAp ( >> mSmmMpSyncData->CpuData[CpuIndex].Procedure = Procedure; >> >> mSmmMpSyncData->CpuData[CpuIndex].Parameter = ProcArguments; >> >> if (Token != NULL) { >> >> - ProcToken= CreateToken (1); >> >> + ProcToken= GetFreeToken (1); >> >> mSmmMpSyncData->CpuData[CpuIndex].Token = ProcToken; >> >> *Token = (MM_COMPLETION)ProcToken->SpinLock; >> >> } >> >> @@ -1320,7 +1362,7 @@ InternalSmmStartupAllAPs ( >> } >> >> >> >> if (Token != NULL) { >> >> - ProcToken = CreateToken ((UINT32)mMaxNumberOfCpus); >> >> + ProcToken = GetFreeToken ((UINT32)mMaxNumberOfCpus); >> >> *Token = (MM_COMPLETION)ProcToken->SpinLock; >> >> } else { >> >> ProcToken = NULL; >> >> @@ -1732,28 +1774,12 @@ InitializeDataForMmMp ( >> VOID >> >> ) >> >> { >> >> - UINTN SpinLockSize; >> >> - UINT32 TokenCountPerChunk; >> >> - >> >> - SpinLockSize = GetSpinLockProperties (); >> >> - TokenCountPerChunk = FixedPcdGet32 >> (PcdCpuSmmMpTokenCountPerChunk); >> >> - ASSERT (TokenCountPerChunk != 0); >> >> - if (TokenCountPerChunk == 0) { >> >> - DEBUG ((DEBUG_ERROR, "PcdCpuSmmMpTokenCountPerChunk should >> not be Zero!\n")); >> >> - CpuDeadLoop (); >> >> - } >> >> - DEBUG ((DEBUG_INFO, "CpuSmm: SpinLock Size = 0x%x, >> PcdCpuSmmMpTokenCountPerChunk = 0x%x\n", SpinLockSize, >> TokenCountPerChunk)); >> >> - >> >> - gSmmCpuPrivate->CurrentTokenBuf = AllocatePool (SpinLockSize * >> TokenCountPerChunk); >> >> - ASSERT (gSmmCpuPrivate->CurrentTokenBuf != NULL); >> >> - >> >> - gSmmCpuPrivate->UsedTokenNum = 0; >> >> - >> >> gSmmCpuPrivate->ApWrapperFunc = AllocatePool (sizeof >> (PROCEDURE_WRAPPER) * gSmmCpuPrivate- >>> SmmCoreEntryContext.NumberOfCpus); >> >> ASSERT (gSmmCpuPrivate->ApWrapperFunc != NULL); >> >> >> >> InitializeListHead (&gSmmCpuPrivate->TokenList); >> >> - InitializeListHead (&gSmmCpuPrivate->OldTokenBufList); >> >> + >> >> + AllocateTokenBuffer (); >> >> } >> >> >> >> /** >> >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h >> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h >> index 5c98494e2c..33b3dd140e 100644 >> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h >> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h >> @@ -214,6 +214,7 @@ typedef struct { >> >> >> SPIN_LOCK *SpinLock; >> >> volatile UINT32 RunningApCount; >> >> + BOOLEAN Used; >> >> } PROCEDURE_TOKEN; >> >> >> >> #define PROCEDURE_TOKEN_FROM_LINK(a) CR (a, PROCEDURE_TOKEN, >> Link, PROCEDURE_TOKEN_SIGNATURE) >> >> @@ -254,11 +255,6 @@ typedef struct { >> >> >> PROCEDURE_WRAPPER *ApWrapperFunc; >> >> LIST_ENTRY TokenList; >> >> - >> >> - LIST_ENTRY OldTokenBufList; >> >> - >> >> - UINT8 *CurrentTokenBuf; >> >> - UINT32 UsedTokenNum; // Only record tokens used in >> CurrentTokenBuf. >> >> } SMM_CPU_PRIVATE_DATA; >> >> >> >> extern SMM_CPU_PRIVATE_DATA *gSmmCpuPrivate; >> >> -- >> 2.23.0.windows.1 >