From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:400e:c01::230; helo=mail-pl0-x230.google.com; envelope-from=heyi.guo@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-pl0-x230.google.com (mail-pl0-x230.google.com [IPv6:2607:f8b0:400e:c01::230]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B037021A10992 for ; Sun, 26 Nov 2017 19:09:53 -0800 (PST) Received: by mail-pl0-x230.google.com with SMTP id f6so7641222pln.12 for ; Sun, 26 Nov 2017 19:14:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding:content-language; bh=uUXFLqxVBW1tsqpJnYMXJrB12uaDD4w5r+zS3ZqxKCQ=; b=g/nI9KSXU2smvBdS0SOBvQcHJgtpdwjTOTeTTA0KE4uqz7Wy3BnKPYS1oxf+jiJ/yq 5m5M9HCxSlm1zKqMeQe4cOQe/Y+yT8qiBXp0IKQwMzw12suQqVC5B7DQZnojNaUqiRY4 cGLljW0MTWfP2T9az7V6VPiP4/qfVskbxlqlM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding :content-language; bh=uUXFLqxVBW1tsqpJnYMXJrB12uaDD4w5r+zS3ZqxKCQ=; b=k1U/VsZom9QJZoajwC2EXCZZIgThHTBbxlCUt1fG4naBl+outtOAD040j0pWTkZWhh d17S4q09+6ZSUJj8F+5MXWN1DveTdCs924edBhN38RbPlI3Nd3WGGotzN0mi/GF/9YPE Fge26lumtxu513JOHw+ZIj4+Pc0Mfd2aqCQTUnjKKCe87Yl+IugbG/AYJhiXwNGcYFiG kjkU1Ao1NE5Oi95cD5WmeZHldKueBBVVJ2uMlZpegOKUK4TA4+yz7XqGyulK1IM7B2fV zrlsTrdPEIGEQ/qtfD66xMWO3ZhwPdPowjWmBBQQ+kFl2wUReIanZu/Ujiu2irAYVt8v 6tig== X-Gm-Message-State: AJaThX4F4fGSVPR3Vq6nrua2SbX6DGKb8IpI1fJ8QjGtR8Ru8FcNnX0U 4u7VFEiC5NEOJWN9kT/keGMNeQ== X-Google-Smtp-Source: AGs4zMbOlMenJSlt+NUbSSRnzQJnGMKfnuPSjP/zywqqhDwhCRgLPW0VR3ti2vPWZMCJmnQszf+XFA== X-Received: by 10.84.247.148 with SMTP id o20mr37267063pll.137.1511752454559; Sun, 26 Nov 2017 19:14:14 -0800 (PST) Received: from [10.152.199.178] ([104.237.91.79]) by smtp.gmail.com with ESMTPSA id x6sm41747568pfx.15.2017.11.26.19.14.12 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 26 Nov 2017 19:14:13 -0800 (PST) To: "Zeng, Star" , "edk2-devel@lists.01.org" , "Gao, Liming" Cc: "Carsey, Jaben" , Daryl McDaniel References: <6c7964be-bc2b-3ee4-e09e-c309fec7210f@linaro.org> <0C09AFA07DD0434D9E2A0C6AEB0483103B9BBCD9@shsmsx102.ccr.corp.intel.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B9BC210@shsmsx102.ccr.corp.intel.com> From: Heyi Guo Message-ID: <83f0b339-0eb3-b3a3-97e7-4e1be06273d7@linaro.org> Date: Mon, 27 Nov 2017 11:14:32 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103B9BC210@shsmsx102.ccr.corp.intel.com> Subject: Re: [RFC] Add EFI lock when creating new gauge record X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 27 Nov 2017 03:09:53 -0000 Content-Type: text/plain; charset=gbk; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Thanks; will do that. Regards, Gary (Heyi Guo) ÔÚ 11/27/2017 10:06 AM, Zeng, Star дµÀ: > I am ok with the approach. > > If no comment from others, I think you can post the patch for code review. > > Liming, do you have any comment? :) > > > Thanks, > Star > -----Original Message----- > From: Heyi Guo [mailto:heyi.guo@linaro.org] > Sent: Saturday, November 25, 2017 5:10 PM > To: Zeng, Star ; edk2-devel@lists.01.org > Cc: Carsey, Jaben ; Daryl McDaniel ; Gao, Liming > Subject: Re: [edk2] [RFC] Add EFI lock when creating new gauge record > > Hi Star, > > Sorry to make a mistake with the code package :) > > Please see my test results below. > > > ÔÚ 11/24/2017 5:41 PM, Zeng, Star дµÀ: >> Good problem statement. >> >> EndGaugeEx and GetGaugeEx also need to be updated as they are operating same memory buffer, otherwise the performance data may be messed up. >> >> Another idea is to just use a global variable, for example mRecording, to prevent reentrance, but that there may be perf data missing. >> >> How to realize " not impacted much " in " the performance measuring data was not impacted much by this patch "? >> That means there is a little performance impact? > I tested more than 50 times on my platform with the original version and the modified one, and calculated the average "Total Duration", which were 32567ms and 32600ms respectively. So the performance impact almost can be ignored. >> Could you re-measure with all StartGaugeEx, EndGaugeEx and GetGaugeEx add lock? > Good point. The above result was tested with all of them locked. > > I also attached my patch. > > Thanks and regards, > > Gary (Heyi Guo) >> >> Cc Liming. >> >> >> Thanks, >> Star >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >> Heyi Guo >> Sent: Friday, November 24, 2017 3:04 PM >> To: edk2-devel@lists.01.org >> Cc: Carsey, Jaben ; Daryl McDaniel >> >> Subject: [edk2] [RFC] Add EFI lock when creating new gauge record >> >> Hi folks, >> >> We got occasional system exceptions after enabling performance measuring feature in edk2. After debugging, we found there is potential memory overflow in DXE/DXE_CORE PerformanceLib when PERF_START is reentered, and reentrance is possible since we are supporting something like USB hot-plug, which is a timer event where gBS->ConnectController might be called and then PERF will be called in CoreConnectSingleController. >> However I can't reproduce the issue right now; please let me know if PERF reentrance is not theoretically possible in the latest edk2 code. >> >> When StartGaugeEx is being reentered, not only the gauge record might be overwritten, more serious situation will be caused if gauge data buffer reallocation procedure is interrupted, between line 180 and 187 in DxeCorePerformanceLib.c specifically. There, mMaxGaugeRecords will be doubled twice (denoted as 4X), but mGaugeData only points to a buffer of size 2X, which will probably cause the following 2X memory to be overflowed when gauge records are increased. >> >> My proposal is to add EFI lock with TPL notify in StartGaugeEx to avoid such situation. The test result seemed good on our platforms and the performance measuring data was not impacted much by this patch. >> >> Please let me know your comments. >> >> Thanks, >> >> Gary (Heyi Guo) >> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel