Heyi: I agree your fix. Its impact is acceptable. Thanks Liming >-----Original Message----- >From: Heyi Guo [mailto:heyi....@linaro.org] >Sent: Saturday, November 25, 2017 5:10 PM >To: Zeng, Star <star.z...@intel.com>; edk2-devel@lists.01.org >Cc: Carsey, Jaben <jaben.car...@intel.com>; Daryl McDaniel <edk2- >li...@mc2research.org>; Gao, Liming <liming....@intel.com> >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-boun...@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 <jaben.car...@intel.com>; Daryl McDaniel <edk2- >li...@mc2research.org> >> 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
_______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
