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

["Zeng, Star" <star.zeng@intel.com>]

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?

Could you re-measure with all StartGaugeEx, EndGaugeEx and GetGaugeEx add lock?


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