Re: [ros-dev] [ros-diffs] [dgorbachev] 43333: Fix GCC 4.1.3 warning.

[Alex Ionescu]

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(a)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=433…
>  ==============================================================================
>  --- 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);
>      }
>   }
>
>
>
>

[Dmitry Gorbachev]

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.

[Alex Ionescu]

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(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev 
Best regards,
Alex Ionescu

[Dmitry Gorbachev]

>   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.

[Jose Catena]

>   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.

[WaxDragon]

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(a)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(a)reactos.org http://w...