booting ros is broken (was: fix broken work item impl.)

[Hartmut Birr]

Hi,

I've added the 'work item fix' to my source tree for testing. I'm not able
to boot reactos. Ros hangs somewhere in ndis:

...
(ldr/loader.c:319) Could not open module file:
\SystemRoot\system32\drivers\DLKRTS.SYS
(io/pnpmgr.c:1521) Initialization of service DLKRTS failed (Status c0000001)
DriverBase for \SystemRoot\system32\drivers\pci.sys: 9cd89000
Peripheral Component Interconnect Bus Driver
(io/create.c:90) Parent is a Directory which is neither a file type nor a
device type
(io/pnpmgr.c:1521) Initialization of service Ne2000 failed (Status c0000001)
DriverBase for scsiport.sys: 9cd93000
DriverBase for atapi.sys: 9cd9f000
DriverBase for aic78u2.sys: 9cdc9000
DriverBase for class2.sys: 9ce6b000
DriverBase for disk.sys: 9ce75000
DriverBase for vfatfs.sys: 9ce7e000
DriverBase for bootvid.sys: 9ce93000
DriverBase for ndis.sys: 9ceb4000
(ndis/main.c:52)(DriverEntry) Called.
(ke/main.c:710) CommandLine: multi(0)disk(0)rdisk(0)partition(3)\ReactOS

Without the fix, booting continues after that point with:

DriverBase for \SystemRoot\system32\drivers\DLKRTS.SYS: 9cf09000
(ndis/miniport.c:1090)(NdisInitializeWrapper) Called.
(ndis/miniport.c:1631)(NdisMRegisterMiniport) Called.
(ndis/miniport.c:1577)(NdisIAddDevice) LinkageKey:
Class{4D36E972-E325-11CE-BFC1-08002BE10318}\0002\Linkage.
(ndis/miniport.c:1374)(NdisIStartAdapter) creating device \Device\DLKRTS1
(ndis/miniport.c:1394)(NdisIStartAdapter) opened device reg key
(ndis/miniport.c:1396)(NdisIStartAdapter) acquiring miniport block lock
...

- Hartmut

[Gunnar Dalsnes]

Ok. So my fix wasn't the right fix either. I will revert my changes.

Still, the old impl. is broken and bsod for me:-( I guess a temporary dirty
hack could be to increase the semaphore limit? Any objections?

Alex's suggestion to use kernel queues is the right fix i guess, but this is
somewhere in the future.

Gunnar
> -----Original Message-----
> From: [e-mail] 
> [mailto:[e-mail]] On Behalf Of Hartmut Birr
> Sent: Wednesday, November 17, 2004 8:33 PM
> To: ReactOS Development List
> Subject: [ros-dev] booting ros is broken (was: fix broken 
> work item impl.)
>
> Hi,
>
> I've added the 'work item fix' to my source tree for testing. 
> I'm not able
> to boot reactos. Ros hangs somewhere in ndis:
>
> ...
> (ldr/loader.c:319) Could not open module file:
> \SystemRoot\system32\drivers\DLKRTS.SYS
> (io/pnpmgr.c:1521) Initialization of service DLKRTS failed 
> (Status c0000001)
> DriverBase for \SystemRoot\system32\drivers\pci.sys: 9cd89000
> Peripheral Component Interconnect Bus Driver
> (io/create.c:90) Parent is a Directory which is neither a 
> file type nor a
> device type
> (io/pnpmgr.c:1521) Initialization of service Ne2000 failed 
> (Status c0000001)
> DriverBase for scsiport.sys: 9cd93000
> DriverBase for atapi.sys: 9cd9f000
> DriverBase for aic78u2.sys: 9cdc9000
> DriverBase for class2.sys: 9ce6b000
> DriverBase for disk.sys: 9ce75000
> DriverBase for vfatfs.sys: 9ce7e000
> DriverBase for bootvid.sys: 9ce93000
> DriverBase for ndis.sys: 9ceb4000
> (ndis/main.c:52)(DriverEntry) Called.
> (ke/main.c:710) CommandLine: 
> multi(0)disk(0)rdisk(0)partition(3)\ReactOS
>
> Without the fix, booting continues after that point with:
>
> DriverBase for \SystemRoot\system32\drivers\DLKRTS.SYS: 9cf09000
> (ndis/miniport.c:1090)(NdisInitializeWrapper) Called.
> (ndis/miniport.c:1631)(NdisMRegisterMiniport) Called.
> (ndis/miniport.c:1577)(NdisIAddDevice) LinkageKey:
> Class{4D36E972-E325-11CE-BFC1-08002BE10318}\0002\Linkage.
> (ndis/miniport.c:1374)(NdisIStartAdapter) creating device 
> \Device\DLKRTS1
> (ndis/miniport.c:1394)(NdisIStartAdapter) opened device reg key
> (ndis/miniport.c:1396)(NdisIStartAdapter) acquiring miniport 
> block lock
> ...
> Hartmut
>
> Ros-dev mailing list
> [e-mail]
> http://reactos.com:8080/mailman/listinfo/ros-dev

[Alex Ionescu]

Gunnar Dalsnes wrote:
> Ok. So my fix wasn't the right fix either. I will revert my changes.
>
> Still, the old impl. is broken and bsod for me:-( I guess a temporary dirty
> hack could be to increase the semaphore limit? Any objections?

I agree, it's better then wasting time in trying to fix into more detail 
code which shouldn't behave that way.
> Alex's suggestion to use kernel queues is the right fix i guess, but this is
> somewhere in the future.

I've added it to my timeline. As soon as I'm done testing the new DPC 
rules I'll check out KQUEUES ;)
Seems to be easy since your current code really seems solid.

Best regards,
Alex Ionescu
> Ros-dev mailing list
> [e-mail]
> http://reactos.com:8080/mailman/listinfo/ros-dev

[Hartmut Birr]

> -----Original Message-----
> From: [e-mail] 
> [mailto:[e-mail]] On Behalf Of Gunnar Dalsnes
> Sent: Thursday, November 18, 2004 12:05 AM
> To: 'ReactOS Development List'
> Subject: RE: [ros-dev] booting ros is broken (was: fix broken 
> work item impl.)
>
> Ok. So my fix wasn't the right fix either. I will revert my changes.
>
> Still, the old impl. is broken and bsod for me:-( I guess a 
> temporary dirty
> hack could be to increase the semaphore limit? Any objections?

If 256 work items are not enough, something else is wrong. You should change
the worker code a little bit, that the caller of each request is stored in
the work item. If the number of items reach a limit (10..20), the code
should print the items and callers. This will not work with non ros drivers
if the caller is stored in WORK_QUEUE_ITEM.

- Hartmut

[Filip Navara]

Attached is an experimental implementation of work queues using KQUEUEs. 
It depends on the KQUEUE patch I sent...

Regards,
Filip

Index: ntoskrnl/ex/work.c
===================================================================
RCS file: /CVS/ReactOS/reactos/ntoskrnl/ex/work.c,v
retrieving revision 1.22
diff -u -r1.22 work.c
--- ntoskrnl/ex/work.c	17 Nov 2004 23:55:36 -0000	1.22
+++ ntoskrnl/ex/work.c	21 Nov 2004 13:26:34 -0000
@@ -24,19 +24,9 @@
 typedef struct _WORK_QUEUE
 {
    /*
-    * PURPOSE: Head of the list of waiting work items
+    * PURPOSE: The actual queue for the work items
     */
-   LIST_ENTRY Head;
-   
-   /*
-    * PURPOSE: Sychronize access to the work queue
-    */
-   KSPIN_LOCK Lock;
-   
-   /*
-    * PURPOSE: Worker threads with nothing to do wait on this event
-    */
-   KSEMAPHORE Sem;
+   KQUEUE WorkerQueue;

/*
     * PURPOSE: Thread associated with work queue
@@ -75,22 +65,12 @@

for(;;)
      {
-	current = ExInterlockedRemoveHeadList(&queue->Head,
-					      &queue->Lock);
+	current = KeRemoveQueue(&queue->WorkerQueue, KernelMode, NULL);
    if (current!=NULL)
      {
         item = CONTAINING_RECORD(current,WORK_QUEUE_ITEM,List);
         item->WorkerRoutine(item->Parameter);
      }
-	else
-	  {
-	     KeWaitForSingleObject((PVOID)&queue->Sem,
-				   Executive,
-				   KernelMode,
-				   FALSE,
-				   NULL);
-	     DPRINT("Woke from wait\n");
-	  }
      }
 }

@@ -100,11 +80,7 @@
    ULONG i;
    PETHREAD Thread;

-   InitializeListHead(&WorkQueue->Head);
-   KeInitializeSpinLock(&WorkQueue->Lock);
-   KeInitializeSemaphore(&WorkQueue->Sem,
-			 0,
-			 256);
+   KeInitializeQueue(&WorkQueue->WorkerQueue, NUMBER_OF_WORKER_THREADS);
    for (i=0; i<NUMBER_OF_WORKER_THREADS; i++)
      {
    PsCreateSystemThread(&WorkQueue->Thread[i],
@@ -154,47 +130,26 @@
     ASSERT(WorkItem!=NULL);
     ASSERT_IRQL(DISPATCH_LEVEL);

-   /*
-    * Insert the item in the appropiate queue and wake up any thread
-    * waiting for something to do
-    */
+    /*
+     * Insert the item in the appropiate queue and wake up any thread
+     * waiting for something to do
+     */
     switch(QueueType)
     {
-    case DelayedWorkQueue:
-	    ExInterlockedInsertTailList(&EiNormalWorkQueue.Head,
-				    &WorkItem->List,
-				    &EiNormalWorkQueue.Lock);
-	    KeReleaseSemaphore(&EiNormalWorkQueue.Sem,
-			   IO_NO_INCREMENT,
-			   1,
-			   FALSE);
-	break;
+        case DelayedWorkQueue:
+           KeInsertQueue(&EiNormalWorkQueue.WorkerQueue,
+                         &WorkItem->List);
+           break;
    
-    case CriticalWorkQueue:
-        ExInterlockedInsertTailList(&EiCriticalWorkQueue.Head,
-				    &WorkItem->List,
-				    &EiCriticalWorkQueue.Lock);
-        KeReleaseSemaphore(&EiCriticalWorkQueue.Sem,
-	       	  	   IO_NO_INCREMENT,
-	       		   1,
-	       		   FALSE);
-	    break;
-
-    case HyperCriticalWorkQueue:
-	    ExInterlockedInsertTailList(&EiHyperCriticalWorkQueue.Head,
-				    &WorkItem->List,
-				    &EiHyperCriticalWorkQueue.Lock);
-	    KeReleaseSemaphore(&EiHyperCriticalWorkQueue.Sem,
-			   IO_NO_INCREMENT,
-			   1,
-			   FALSE);
-    	break;
-
-#ifdef __USE_W32API
-	case MaximumWorkQueue:
-	   // Unimplemented
-	   break;
-#endif
+        case CriticalWorkQueue:
+           KeInsertQueue(&EiCriticalWorkQueue.WorkerQueue,
+                         &WorkItem->List);
+           break;
+
+        case HyperCriticalWorkQueue:
+           KeInsertQueue(&EiHyperCriticalWorkQueue.WorkerQueue,
+                         &WorkItem->List);
+           break;
     }
 }