Fix leak and race condition with EventPipeBufferList

[sywhang]

EventPipeBufferList has a potential leak under EventPipeBufferManager::DeAllocateBuffers(), and a potential race condition where WriteEvent may set the flag to indicate a buffer is in use when another thread is trying to free the buffers after checking that they aren't in use. So we should probably protect that flag with a lock to prevent this.

Some background info (comment from Noah):
dotnet/coreclr#21817 (comment)

[noahfalk]

Just fyi - its true we want to be multi-thread safe, but as this code is intended to be high performance we also need to be careful what types of synchronization we use. The flag was probably created to avoid having a lock, so adding a lock around it might be counter-productive : )
At one end of the range we've got options like Crst (a critical section) that are easy to code but slower, and at the other end of the range we've got lock-free algorithms that are fast and exceedingly error prone. There are also some options in the middle. We should probably measure to see what performance looks like right now and potentially chat with @vancem to see if he has any feedback on how much overhead is acceptable in the name of simpler/safer coding.

[vancem]

To answer @noahfalk's question about perf. The key thing that we need is scalability. We do anticipate that logging will be done on a big multi-core machine (e.g. 32+ processors), and they all might be logging 1000s of events every second (maybe more, but 1000s is very reasonable). You can imagine under such a situation that a lock that is take on every event being logged is problematic, so we want to avoid that.

Lock free algorithms are very error prone as @noahfalk indicates so we would like to at least minimize their use.

THe heart of the Eventpipe design is we wnat to decouple threads as much as possible so that things scale. Thus each thread has its own buffer that events are logged to. This part should remain lock free to keep the scaling. All other operations (getting new buffers, transfering them to some file writer etc) can be under a lock. That seems to be the design we want.

[vancem]

Note that I wanted to mention the existance of memoyrbarrierprocesswide (see
https://docs.microsoft.com/en-us/dotnet/api/system.threading.interlocked.memorybarrierprocesswide?view=netcore-2.2&viewFallbackFrom=netframework-4.7.2. there is a unmanaged equivalent) when interacting with lock-free stuff).

[vancem]

As mentioned at the meeting we had, having a per-thread spin lock protecting the per-thread buffer which is taken when an event is logged or when a flush/shutdown is forced is reasonable.

[tommcdon]

@cshung can Noah's fix be merged for this issue?

[tommcdon]

@cshung ping

[tommcdon]

@cshung refreshing ping

[noahfalk]

@cshung - I think we are done now right?

[cshung]

@noahfalk