RE: [ros-diffs] [CVS reactos] fix broken work item impl.

[Hartmut Birr]

Hi Gunnar,

for synchronisation between an unlimited count of objects (or 256) in a wait
queue and more than one waiters which removes this objects, there _must_ be
used a semaphore (or equivalent code). Please revert your changes.

- Hartmut

-----Original Message-----
From: [e-mail] [mailto:[e-mail]]
On Behalf Of [e-mail]
Sent: Tuesday, November 16, 2004 12:15 AM
To: [e-mail]
Subject: [ros-diffs] [CVS reactos] fix broken work item impl.

Commit in reactos/ntoskrnl/ex on MAIN	
work.c <> 	 +16	 -18	 1.20
http://cvs.reactos.com/cgi-bin/cvsweb/reactos/ntoskrnl/ex/work.c?rev=1.20&c
ontent-type=text/x-cvsweb-markup  -
http://cvs.reactos.com/cgi-bin/cvsweb/reactos/ntoskrnl/ex/work.c.diff?r1=te
xt&tr1=1.20&r2=text&tr2=1.21&f=h > 1.21
http://cvs.reactos.com/cgi-bin/cvsweb/reactos/ntoskrnl/ex/work.c?rev=1.21&c
ontent-type=text/x-cvsweb-markup 	
fix broken work item impl.
  _____

reactos http://cvs.reactos.com/cgi-bin/cvsweb/reactos /ntoskrnl
http://cvs.reactos.com/cgi-bin/cvsweb/reactos/ntoskrnl /ex
http://cvs.reactos.com/cgi-bin/cvsweb/reactos/ntoskrnl/ex

work.c 1.20
http://cvs.reactos.com/cgi-bin/cvsweb/reactos/ntoskrnl/ex/work.c?rev=1.20&c
ontent-type=text/x-cvsweb-markup  -
http://cvs.reactos.com/cgi-bin/cvsweb/reactos/ntoskrnl/ex/work.c.diff?r1=te
xt&tr1=1.20&r2=text&tr2=1.21&f=h > 1.21
http://cvs.reactos.com/cgi-bin/cvsweb/reactos/ntoskrnl/ex/work.c?rev=1.21&c
ontent-type=text/x-cvsweb-markup 
diff -u -r1.20 -r1.21

--- work.c	22 Oct 2004 20:18:37 -0000	1.20

+++ work.c	15 Nov 2004 23:14:36 -0000	1.21

@@ -1,4 +1,4 @@

-/* $Id: work.c,v 1.20 2004/10/22 20:18:37 ekohl Exp $
+/* $Id: work.c,v 1.21 2004/11/15 23:14:36 gdalsnes Exp $
  *

* COPYRIGHT:          See COPYING in the top level directory

* PROJECT:            ReactOS kernel
@@ -36,7 +36,7 @@

/*

* PURPOSE: Worker threads with nothing to do wait on this event

*/
-   KSEMAPHORE Sem;
+   KEVENT Event;

/*

* PURPOSE: Thread associated with work queue
@@ -84,7 +84,7 @@

}

else

{
-	     KeWaitForSingleObject((PVOID)&queue->Sem,
+	     KeWaitForSingleObject((PVOID)&queue->Event,
    			   Executive,

KernelMode,

FALSE,
@@ -102,9 +102,10 @@

InitializeListHead(&WorkQueue->Head);

KeInitializeSpinLock(&WorkQueue->Lock);
-   KeInitializeSemaphore(&WorkQueue->Sem,

-			 0,

-			 256);
+   KeInitializeEvent(&WorkQueue->Event,

+                     SynchronizationEvent,

+                     FALSE);

+
    for (i=0; i<NUMBER_OF_WORKER_THREADS; i++)

{

PsCreateSystemThread(&WorkQueue->Thread[i],
@@ -164,30 +165,27 @@

ExInterlockedInsertTailList(&EiNormalWorkQueue.Head,

&WorkItem->List,

&EiNormalWorkQueue.Lock);
-	    KeReleaseSemaphore(&EiNormalWorkQueue.Sem,

-			   IO_NO_INCREMENT,

-			   1,

-			   FALSE);
+       KeSetEvent(&EiNormalWorkQueue.Event,

+                  IO_NO_INCREMENT,

+                  FALSE);
    break;

case CriticalWorkQueue:

ExInterlockedInsertTailList(&EiCriticalWorkQueue.Head,

&WorkItem->List,

&EiCriticalWorkQueue.Lock);
-        KeReleaseSemaphore(&EiCriticalWorkQueue.Sem,

-	       	  	   IO_NO_INCREMENT,

-	       		   1,

-	       		   FALSE);
+        KeSetEvent(&EiCriticalWorkQueue.Event,

+                   IO_NO_INCREMENT,

+                   FALSE);
        break;

case HyperCriticalWorkQueue:

ExInterlockedInsertTailList(&EiHyperCriticalWorkQueue.Head,

&WorkItem->List,

&EiHyperCriticalWorkQueue.Lock);
-	    KeReleaseSemaphore(&EiHyperCriticalWorkQueue.Sem,

-			   IO_NO_INCREMENT,

-			   1,

-			   FALSE);
+       KeSetEvent(&EiHyperCriticalWorkQueue.Event,

+                  IO_NO_INCREMENT,

+                  FALSE);
     	break;

#ifdef __USE_W32API
email" href="http://www.badgers-in-foil.co.uk/projects/cvsspam/%22%3ECVSspam
0.2.8

[Gunnar Dalsnes]

I don't understand why you think my changes are not correct?

The previous impl. crashed when compiling ros-on-ros, due to semaphore limit
overflow and increasing the limit would only be a dirty hack. The semaphore
was not the right tool for this job.

Gunnar

-----Original Message-----
From: [e-mail] [mailto:[e-mail]] On
Behalf Of Hartmut Birr
Sent: Tuesday, November 16, 2004 6:39 PM
To: ReactOS Development List
Subject: [ros-dev] RE: [ros-diffs] [CVS reactos] fix broken work item impl.

Hi Gunnar,

for synchronisation between an unlimited count of objects (or 256) in a wait
queue and more than one waiters which removes this objects, there _must_ be
used a semaphore (or equivalent code). Please revert your changes.

[Hartmut Birr]

Hi,

you have changed the semaphore to a synchronisation event. If all worker
threads have a job and there are put more than one new job in the queue,
only one of the jobs is delivered to the workers. The first empty worker
removes one job. The event is never signaled again. All other jobs are never
delivered to the workers. Compiling ros on ros isn't the real problem. I use
three pc for testing. On two pc I can compile ros on ros without any
problems. The third pc is a smp machine. On my third pc, the problem is
somewhere in the smp code (and in many of my not commited changes).

- Hartmut

-----Original Message-----
From: [e-mail] [mailto:[e-mail]] On
Behalf Of Gunnar Dalsnes
Sent: Tuesday, November 16, 2004 9:30 PM
To: 'ReactOS Development List'
Subject: RE: [ros-dev] RE: [ros-diffs] [CVS reactos] fix broken work item
impl.

I don't understand why you think my changes are not correct?

The previous impl. crashed when compiling ros-on-ros, due to semaphore limit
overflow and increasing the limit would only be a dirty hack. The semaphore
was not the right tool for this job.

Gunnar

-----Original Message-----
From: [e-mail] [mailto:[e-mail]] On
Behalf Of Hartmut Birr
Sent: Tuesday, November 16, 2004 6:39 PM
To: ReactOS Development List
Subject: [ros-dev] RE: [ros-diffs] [CVS reactos] fix broken work item impl.

Hi Gunnar,

for synchronisation between an unlimited count of objects (or 256) in a wait
queue and more than one waiters which removes this objects, there _must_ be
used a semaphore (or equivalent code). Please revert your changes.

[Gunnar Dalsnes]

> you have changed the semaphore to a synchronisation event. If all worker

threads have a job and there are
> put more than one new job in the queue, only one of the jobs is delivered

to the workers. The first empty
> worker removes one job. The event is never signaled again. All other jobs

are never delivered to the
> workers.

I don't understand how you are thinking.

SyncronizationEvent = AutoResetEvent. It shouldn't be possible for a work
item not to be delivered. If any worker threads are allready running, the
ExInterlockedRemoveHeadList loop will execute the work item. If any thread
is waiting on the event, it will be woken. We/I could examine a more
detailed scenario if you want.
> Compiling ros on ros isn't the real problem. I use three pc for testing.

On two pc I can compile
> ros on ros without any problems. The third pc is a smp machine. On my

third pc, the problem is somewhere in
> the smp code (and in many of my not commited changes).

The new tcpip.sys generates lots of work items, and while compiling
ros-on-ros it bsod'd for me with STATUS_SEMAPHORE_LIMIT_EXCEEDED exception.
If you look at the old code, this can easily happend if the worker threads
are busy looping and dequeing items, without ever entering the wait.

[Alex Ionescu]

Hi,

I'm currently not able to look into this, but I'm afraid that you're 
both right and wrong.
Worker Threads/Queues are nothing more then an executive version of the 
kernel KQUEUE. I don't know why are using special structures and 
code...all that ExQueueWorkItem should do is call KeInsertQueue with the 
right parameters/queue name...I don't understand the point of using 
Events and Semaphores... KeRemoveQueue implements all the waiting 
necessary...a worker thread, in the entrypoint, simply loops by calling 
that function, which does the actual waiting (Even on ROS we have 
this!). You're both right that EVENTs and SEMAPHOREs are wrong...

I see it as utterly pointless to be working on a parallel implementation 
which seems to be broken no matter how you do it...just use KQUEUES like 
it should be done, and I believe this will be solved.

Best regards,
Alex Ionescu

Gunnar Dalsnes wrote:
> > you have changed the semaphore to a synchronisation event. If all worker
>
> threads have a job and there are
> > put more than one new job in the queue, only one of the jobs is delivered
>
> to the workers. The first empty
> > worker removes one job. The event is never signaled again. All other jobs
>
> are never delivered to the
> > workers.
>
> I don't understand how you are thinking.
>
> SyncronizationEvent = AutoResetEvent. It shouldn't be possible for a work
> item not to be delivered. If any worker threads are allready running, the
> ExInterlockedRemoveHeadList loop will execute the work item. If any thread
> is waiting on the event, it will be woken. We/I could examine a more
> detailed scenario if you want.
> > Compiling ros on ros isn't the real problem. I use three pc for testing.
>
> On two pc I can compile
> > ros on ros without any problems. The third pc is a smp machine. On my
>
> third pc, the problem is somewhere in
> > the smp code (and in many of my not commited changes).
>
> The new tcpip.sys generates lots of work items, and while compiling
> ros-on-ros it bsod'd for me with STATUS_SEMAPHORE_LIMIT_EXCEEDED exception.
> If you look at the old code, this can easily happend if the worker threads
> are busy looping and dequeing items, without ever entering the wait.
>
> Ros-dev mailing list
> [e-mail]
> http://reactos.com:8080/mailman/listinfo/ros-dev

[Steven Edwards]

I would like to have a discussion about project structure on
ros-general after reading this thread. If everyone will check there I
will be posting later.

Thanks
Steven

__________________________________ 
Do you Yahoo!? 
The all-new My Yahoo! - Get yours free! 
http://my.yahoo.com