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.web10.9399.1612537891348513123 for ; Fri, 05 Feb 2021 07:11:31 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=fZap7XUz; 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=1612537890; 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=ruaprTyGuE/wWlPcYtjaNkhS71YW8ibOYgUGVpmtjr4=; b=fZap7XUzd8bdpjQcTrJ5wZYlndFDiXlnTHzFrZc6aocBeaG5qUf7cJce8CAKAgr7UuchXY UEmSDC+owQMgXymu7csLFNNbSHoSqfQn+HMZTsSAkYY0qhb4/pBGE7jl5KZHGwIAHC8gkX HXUqVOkiKmjdkIfGhhuZBLP9ojmtgKE= 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-166-25ijKof2N4W9SBKBvnQaSg-1; Fri, 05 Feb 2021 10:11:28 -0500 X-MC-Unique: 25ijKof2N4W9SBKBvnQaSg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id B10A684E24F; Fri, 5 Feb 2021 15:11:27 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-245.ams2.redhat.com [10.36.113.245]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7C9855D72F; Fri, 5 Feb 2021 15:11:26 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 3/3] UefiCpuPkg/MpInitLib: Use XADD to avoid lock acquire/release To: devel@edk2.groups.io, ray.ni@intel.com Cc: Eric Dong , Rahul1 Kumar References: <20210205075810.981-1-ray.ni@intel.com> <20210205075810.981-4-ray.ni@intel.com> From: "Laszlo Ersek" Message-ID: <5e601122-8344-c9f4-9dcf-5e158215f842@redhat.com> Date: Fri, 5 Feb 2021 16:11:25 +0100 MIME-Version: 1.0 In-Reply-To: <20210205075810.981-4-ray.ni@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 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/05/21 08:58, Ni, Ray wrote: > When AP firstly wakes up, MpFuncs.nasm contains below logic to assign > an unique ApIndex to each AP according to who comes first: > ---NASM--- > mov edi, esi > add edi, MP_CPU_EXCHANGE_INFO_FIELD (Lock) > mov eax, NotVacantFlag > > TestLock: > xchg [edi], eax > cmp eax, NotVacantFlag > jz TestLock > > mov ecx, esi > add ecx, MP_CPU_EXCHANGE_INFO_FIELD (ApIndex) > inc dword [ecx] > mov ebx, [ecx] > > Releaselock: > mov eax, VacantFlag > xchg [edi], eax > ---NASM END--- > > "LOCK INC" cannot be used to increase MP_CPU_EXCHANGE_INFO.ApIndex > because not only the MP_CPU_EXCHANGE_INFO.ApIndex should be > increased, but also the result should be stored to a thread 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: Laszlo Ersek > Cc: Eric Dong > Cc: Rahul1 Kumar > --- > .../Library/MpInitLib/Ia32/MpFuncs.nasm | 20 ++++--------------- > UefiCpuPkg/Library/MpInitLib/MpEqu.inc | 4 ---- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 1 - > UefiCpuPkg/Library/MpInitLib/MpLib.h | 3 +-- > UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 18 ++++------------- > 5 files changed, 9 insertions(+), 37 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > index 2f1b102717..7bd2415670 100644 > --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > @@ -122,22 +122,10 @@ SkipEnableExecuteDisable: > > ; AP init > mov edi, esi > - add edi, MP_CPU_EXCHANGE_INFO_FIELD (Lock) > - mov eax, NotVacantFlag > - > -TestLock: > - xchg [edi], eax > - cmp eax, NotVacantFlag > - jz TestLock > - > - mov ecx, esi > - add ecx, MP_CPU_EXCHANGE_INFO_FIELD (ApIndex) > - inc dword [ecx] > - mov ebx, [ecx] > - > -Releaselock: > - mov eax, VacantFlag > - xchg [edi], eax > + add edi, MP_CPU_EXCHANGE_INFO_FIELD (ApIndex) > + mov ebx, 1 > + lock xadd dword [edi], ebx ; EBX = ApIndex++ > + inc ebx ; EBX is CpuNumber > > mov edi, esi > add edi, MP_CPU_EXCHANGE_INFO_FIELD (StackSize) > diff --git a/UefiCpuPkg/Library/MpInitLib/MpEqu.inc b/UefiCpuPkg/Library/MpInitLib/MpEqu.inc > index 46c2b5c116..2e9368a374 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpEqu.inc > +++ b/UefiCpuPkg/Library/MpInitLib/MpEqu.inc > @@ -13,9 +13,6 @@ > ;------------------------------------------------------------------------------- > %include "Nasm.inc" > > -VacantFlag equ 00h > -NotVacantFlag equ 0ffh > - > CPU_SWITCH_STATE_IDLE equ 0 > CPU_SWITCH_STATE_STORED equ 1 > CPU_SWITCH_STATE_LOADED equ 2 > @@ -72,7 +69,6 @@ endstruc > ; Equivalent NASM structure of MP_CPU_EXCHANGE_INFO > ; > struc MP_CPU_EXCHANGE_INFO > - .Lock: CTYPE_UINTN 1 > .StackStart: CTYPE_UINTN 1 > .StackSize: CTYPE_UINTN 1 > .CFunction: CTYPE_UINTN 1 > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index 2568986d8c..5040053dad 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -1006,7 +1006,6 @@ FillExchangeInfoData ( > IA32_CR4 Cr4; > > ExchangeInfo = CpuMpData->MpCpuExchangeInfo; > - ExchangeInfo->Lock = 0; > ExchangeInfo->StackStart = CpuMpData->Buffer; > ExchangeInfo->StackSize = CpuMpData->CpuApStackSize; > ExchangeInfo->BufferStart = CpuMpData->WakeupBuffer; > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h > index 02652eaae1..0bd60388b1 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h > @@ -1,7 +1,7 @@ > /** @file > Common header file for MP Initialize Library. > > - Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.
> + Copyright (c) 2016 - 2021, Intel Corporation. All rights reserved.
> Copyright (c) 2020, AMD Inc. All rights reserved.
> > SPDX-License-Identifier: BSD-2-Clause-Patent > @@ -190,7 +190,6 @@ typedef struct _CPU_MP_DATA CPU_MP_DATA; > // into this structure are used in assembly code in this module > // > typedef struct { > - UINTN Lock; > UINTN StackStart; > UINTN StackSize; > UINTN CFunction; > diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm > index bf7faaf60b..50df802d1f 100644 > --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm > +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm > @@ -158,21 +158,11 @@ LongModeStart: > > ; AP init > mov edi, esi > - add edi, MP_CPU_EXCHANGE_INFO_FIELD (Lock) > - mov rax, NotVacantFlag > + add edi, MP_CPU_EXCHANGE_INFO_FIELD (ApIndex) > + mov ebx, 1 > + lock xadd dword [edi], ebx ; EBX = ApIndex++ > + inc ebx ; EBX is CpuNumber > > -TestLock: > - xchg qword [edi], rax > - cmp rax, NotVacantFlag > - jz TestLock > - > - lea ecx, [esi + MP_CPU_EXCHANGE_INFO_FIELD (ApIndex)] > - inc dword [ecx] > - mov ebx, [ecx] > - > -Releaselock: > - mov rax, VacantFlag > - xchg qword [edi], rax > ; program stack > mov edi, esi > add edi, MP_CPU_EXCHANGE_INFO_FIELD (StackSize) > Reviewed-by: Laszlo Ersek