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 > [email protected] > http://www.reactos.org/mailman/listinfo/ros-dev Best regards, Alex Ionescu _______________________________________________ Ros-dev mailing list [email protected] http://www.reactos.org/mailman/listinfo/ros-dev
