From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web09.182.1614279743373837984 for ; Thu, 25 Feb 2021 11:02:23 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=WJG7JiCQ; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1614279742; 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=q2etLdvOT7IdXpa1KGGl9NW6ixH6i0IkCGfSmgBmRSY=; b=WJG7JiCQqeyWNpCYlSp3EkiQmZ6mivFg8My+uxRenewvgejOkjIhxXPUkoq/7Tyt7QHcd8 vSTEV26jeCmS/SYZW4eQHhh85Ev8GbFP7Ip7CkRMjD3kMcpSYt8feTUi7a1Rz+NOtp9XH2 TpIBxJfZCfyngkYFni1uG0+cW6a5nRI= 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-458-6CSKHDQTPjWb_zPOx0vhbw-1; Thu, 25 Feb 2021 14:02:11 -0500 X-MC-Unique: 6CSKHDQTPjWb_zPOx0vhbw-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 91F611020C28; Thu, 25 Feb 2021 19:02:09 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-188.ams2.redhat.com [10.36.115.188]) by smtp.corp.redhat.com (Postfix) with ESMTP id 24E0F68D22; Thu, 25 Feb 2021 19:02:07 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v3 1/4] UefiCpuPkg/MpInitLib: Use XADD to avoid lock acquire/release To: "Ni, Ray" , "devel@edk2.groups.io" Cc: "Dong, Eric" , "Kumar, Rahul1" , "Kinney, Michael D" References: <20210209141634.1999-1-ray.ni@intel.com> <20210209141634.1999-2-ray.ni@intel.com> From: "Laszlo Ersek" Message-ID: <52eabc56-e8cf-2bdb-333c-62ecd5152ca5@redhat.com> Date: Thu, 25 Feb 2021 20:02:06 +0100 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 02/25/21 05:04, Ni, Ray wrote: > Laszlo, > Do you think that Mike's R-b to the first patch can be an ack to the V3 patch set? No, I don't think so. If an R-b is given in response to a specific patch (not the cover letter), and the reviewer doesn't explicitly say "series Reviewed-by" or "for the entire series:", then the R-b applies only to the specific patch. Now, a different question is whether you want or need Mike's R-b for *all* of the patches. That's up to you and Mike to decide. > Can you please review and provide comments? Yes, I'll comment soon. Thanks Laszlo >> -----Original Message----- >> From: Kinney, Michael D >> Sent: Wednesday, February 24, 2021 2:12 AM >> To: devel@edk2.groups.io; Ni, Ray ; Kinney, Michael D >> >> Cc: Dong, Eric ; Laszlo Ersek ; >> Kumar, Rahul1 >> Subject: RE: [edk2-devel] [PATCH v3 1/4] UefiCpuPkg/MpInitLib: Use XADD >> to avoid lock acquire/release >> >> Reviewed-by: Michael D Kinney >> >>> -----Original Message----- >>> From: devel@edk2.groups.io On Behalf Of Ni, >> Ray >>> Sent: Tuesday, February 9, 2021 6:17 AM >>> To: devel@edk2.groups.io >>> Cc: Dong, Eric ; Laszlo Ersek ; >> Kumar, Rahul1 >>> Subject: [edk2-devel] [PATCH v3 1/4] UefiCpuPkg/MpInitLib: Use XADD to >> avoid lock acquire/release >>> >>> When AP firstly wakes up, MpFuncs.nasm contains below logic to assign >>> an unique ApIndex to each AP according to who comes first: >>> ---ASM--- >>> TestLock: >>> xchg [edi], eax >>> cmp eax, NotVacantFlag >>> jz TestLock >>> >>> mov ecx, esi >>> add ecx, ApIndexLocation >>> inc dword [ecx] >>> mov ebx, [ecx] >>> >>> Releaselock: >>> mov eax, VacantFlag >>> xchg [edi], eax >>> ---ASM END--- >>> >>> "lock inc" cannot be used to increase ApIndex because not only the >>> global ApIndex should be increased, but also the result should be >>> stored to a local general purpose register EBX. >>> >>> This patch learns from the NASM implementation of >>> InternalSyncIncrement() to use "XADD" instruction which can increase >>> the global ApIndex and store the original ApIndex to EBX in one >>> instruction. >>> >>> With this patch, OVMF when running in a 255 threads QEMU spends about >>> one second to wakeup all APs. Original implementation needs more than >>> 10 seconds. >>> >>> Signed-off-by: Ray Ni >>> Cc: Eric Dong >>> Cc: Laszlo Ersek >>> Cc: Rahul Kumar >>> --- >>> .../Library/MpInitLib/Ia32/MpFuncs.nasm | 20 ++++++------------- >>> UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 18 ++++++----------- >>> 2 files changed, 12 insertions(+), 26 deletions(-) >>> >>> diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm >> b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm >>> index 7e81d24aa6..2eaddc93bc 100644 >>> --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm >>> +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm >>> @@ -1,5 +1,5 @@ >>> ;------------------------------------------------------------------------------ ; >>> >>> -; Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.
>>> >>> +; Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.
>>> >>> ; SPDX-License-Identifier: BSD-2-Clause-Patent >>> >>> ; >>> >>> ; Module Name: >>> >>> @@ -125,19 +125,11 @@ SkipEnableExecuteDisable: >>> add edi, LockLocation >>> >>> mov eax, NotVacantFlag >>> >>> >>> >>> -TestLock: >>> >>> - xchg [edi], eax >>> >>> - cmp eax, NotVacantFlag >>> >>> - jz TestLock >>> >>> - >>> >>> - mov ecx, esi >>> >>> - add ecx, ApIndexLocation >>> >>> - inc dword [ecx] >>> >>> - mov ebx, [ecx] >>> >>> - >>> >>> -Releaselock: >>> >>> - mov eax, VacantFlag >>> >>> - xchg [edi], eax >>> >>> + mov edi, esi >>> >>> + add edi, ApIndexLocation >>> >>> + mov ebx, 1 >>> >>> + lock xadd dword [edi], ebx ; EBX = ApIndex++ >>> >>> + inc ebx ; EBX is CpuNumber >>> >>> >>> >>> mov edi, esi >>> >>> add edi, StackSizeLocation >>> >>> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm >> b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm >>> index aecfd07bc0..5b588f2dcb 100644 >>> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm >>> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm >>> @@ -1,5 +1,5 @@ >>> ;------------------------------------------------------------------------------ ; >>> >>> -; Copyright (c) 2015 - 2019, Intel Corporation. All rights reserved.
>>> >>> +; Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.
>>> >>> ; SPDX-License-Identifier: BSD-2-Clause-Patent >>> >>> ; >>> >>> ; Module Name: >>> >>> @@ -161,18 +161,12 @@ LongModeStart: >>> add edi, LockLocation >>> >>> mov rax, NotVacantFlag >>> >>> >>> >>> -TestLock: >>> >>> - xchg qword [edi], rax >>> >>> - cmp rax, NotVacantFlag >>> >>> - jz TestLock >>> >>> - >>> >>> - lea ecx, [esi + ApIndexLocation] >>> >>> - inc dword [ecx] >>> >>> - mov ebx, [ecx] >>> >>> + mov edi, esi >>> >>> + add edi, ApIndexLocation >>> >>> + mov ebx, 1 >>> >>> + lock xadd dword [edi], ebx ; EBX = ApIndex++ >>> >>> + inc ebx ; EBX is CpuNumber >>> >>> >>> >>> -Releaselock: >>> >>> - mov rax, VacantFlag >>> >>> - xchg qword [edi], rax >>> >>> ; program stack >>> >>> mov edi, esi >>> >>> add edi, StackSizeLocation >>> >>> -- >>> 2.27.0.windows.1 >>> >>> >>> >>> -=-=-=-=-=-= >>> Groups.io Links: You receive all messages sent to this group. >>> View/Reply Online (#71517): >> https://edk2.groups.io/g/devel/message/71517 >>> Mute This Topic: https://groups.io/mt/80504936/1643496 >>> Group Owner: devel+owner@edk2.groups.io >>> Unsubscribe: https://edk2.groups.io/g/devel/unsub >> [michael.d.kinney@intel.com] >>> -=-=-=-=-=-= >>> >