Re: [RFC] Add EFI lock when creating new gauge record

["Gao, Liming" <liming.gao@intel.com>]

Heyi:
  I agree your fix. Its impact is acceptable. 

Thanks
Liming
>-----Original Message-----
>From: Heyi Guo [mailto:heyi.guo@linaro.org]
>Sent: Saturday, November 25, 2017 5:10 PM
>To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
>Cc: Carsey, Jaben <jaben.carsey@intel.com>; Daryl McDaniel <edk2-
>lists@mc2research.org>; Gao, Liming <liming.gao@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-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 <jaben.carsey@intel.com>; Daryl McDaniel <edk2-
>lists@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