Why are you using this bad/broken API?
Why are you doing an exchange without even checking the result?
Just use InterlockedAdd (but you're not accessing KdpFreeBytes safely elsewhere, so again, you're just wasting time doing bad synchronziation).
Best regards, Alex Ionescu
On Wed, Oct 7, 2009 at 4:04 PM, dgorbachev@svn.reactos.org wrote:
Author: dgorbachev Date: Wed Oct 7 22:04:17 2009 New Revision: 43333
URL: http://svn.reactos.org/svn/reactos?rev=43333&view=rev Log: Fix GCC 4.1.3 warning.
Modified: trunk/reactos/ntoskrnl/kd/kdio.c
Modified: trunk/reactos/ntoskrnl/kd/kdio.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/kd/kdio.c?rev=4333... ============================================================================== --- trunk/reactos/ntoskrnl/kd/kdio.c [iso-8859-1] (original) +++ trunk/reactos/ntoskrnl/kd/kdio.c [iso-8859-1] Wed Oct 7 22:04:17 2009 @@ -68,7 +68,7 @@ KdpDebugBuffer, end, NULL, NULL); }
- InterlockedExchangeAddUL(&KdpFreeBytes, num);
- (VOID)InterlockedExchangeAddUL(&KdpFreeBytes, num);
} }
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.
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.
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
Why are you doing an exchange without even checking the result?
It is not needed.
Just use InterlockedAdd
Unfortunately, there is no such function. :)
(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.
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
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?
I can guess why such a construct is used, but better let Aleksey explain it.
Logging operations take a short amount of time and are spurious, in this case, a worker thread would work better, it seems.
Not sure what is spurious.
Tried a worker thread, but with intensive logging, the system become less responsive. Task Manager shows high CPU usage (> 50%) for System process. A dedicated thread way was more smooth.
Uhh, MSDN says there is, and it should be an intrnsic in intrin_x86.h, please check again.
I also thought that it is in intrin_x86.h, but no such luck.
MSDN: "This function is supported only on Itanium Processor Family (IPF)."
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).
Yes, it is all not safe against SMP, but on UP I see only one (small) bug.
Logging operations take a short amount of time and are spurious, in this case, a worker thread would work better, it seems.
If a task is to be repeated, I think it is better a system task: while it is waiting for an event it does not consume any cpu, and wake-up upon event and back to wait implies less overhead than creating and destroying a worker thread every time. If the task is very spurious and not time critical, it does not matter much anyway. I hope this makes sense.
Jose Catena DIGIWAVES S.L.
I think some of you have never debugged the network stack or enabled PnP spew during boot. Spurious,indeed!
WD
On Oct 8, 2009 4:48 PM, "Jose Catena" jc1@diwaves.com wrote:
Logging operations take a short amount of time and are spurious, in > this
case, a worker thread... If a task is to be repeated, I think it is better a system task: while it is waiting for an event it does not consume any cpu, and wake-up upon event and back to wait implies less overhead than creating and destroying a worker thread every time. If the task is very spurious and not time critical, it does not matter much anyway. I hope this makes sense.
Jose Catena DIGIWAVES S.L.
_______________________________________________ Ros-dev mailing list Ros-dev@reactos.org http://w...
-
Alex Ionescu -
Dmitry Gorbachev -
Jose Catena -
WaxDragon