On 8-Oct-09, at 9:32 AM, Dmitry Gorbachev wrote:
Hi,
This code has multiple synchronization issues, such as spinning on a test-for-spin-lock, which is non-atomic... might as well get rid of the spinlock.
That synchronization code is borrowed from KdpSerialDebugPrint. Please tell what are issues, I will fix then in both places.
You (the code) is spinning non-atomically on a spinlock at passive then raising the irql to dispatch to "acquire" it. This makes no sense whatsoever. Another code can race you to this place.
Why not just use a normal spinlock?
Also, you should probably be using a worker thread
I have read that "because the pool of system worker threads is a limited resource, WorkItem and WorkItemEx routines can be used only for operations that take a short period of time. If one of these routines runs for too long (if it contains an indefinite loop, for example), the system can deadlock. Therefore, if a driver requires long periods of delayed processing, it should instead call PsCreateSystemThread to create its own system thread." So I am not sure, whether it is better to use a worker thread or a dedicated thread.
Logging operations take a short amount of time and are spurious, in this case, a worker thread would work better, it seems.
instead of consuming system resources for a dedicated thread and mucking with priorities.
The thread is created when booting into "ReactOS (Log file)".
Why are you using this bad/broken API?
What is wrong with that API?
This is what GCC generates:
lock addl %eax, _KdpFreeBytes
I am actually impressed -- it must've picked up MSVC's ability to understand that with the (VOID), it should not generate the full exchange sequence.
Why are you doing an exchange without even checking the result?
It is not needed.
Just use InterlockedAdd
Unfortunately, there is no such function. :)
Uhh, MSDN says there is, and it should be an intrnsic in intrin_x86.h, please check again.
(but you're not accessing KdpFreeBytes safely elsewhere, so again, you're just wasting time doing bad synchronziation).
Otherwise, if addition will be interrupted in between, some output can become lost. This is worse then a bug above, which can cause repeating. On a non-multiprocessor system, there seems to be no other problems.
You're doing store math:
KdpFreeBytes -= num; KdpFreeBytes = KdpBufferSize;
on the variable without an interlock, so those operations will not be safe w.r.t to your interlock.
You're also doing load math:
num = KdpFreeBytes;
on the variable without a fence, so this operation will not be safe w.r.t to your interlock (only MSVC generates fences around volatile variables, and even then they're not sufficient on IA64/Alpha systems).
Ros-dev mailing list Ros-dev@reactos.org http://www.reactos.org/mailman/listinfo/ros-dev
Best regards, Alex Ionescu