Hi,

I am new at driver programming. I was wondering is the following code
is SMP safe or would I need spinlocks around the indicated statements.

//My device extension declaration
typedef struct _DEVICE_EXTENSION
{
PIRP pCurrIrp;
BOOL start;
KEVENT evIoctl;
} DEVICE_EXTENSION, *PDEVICE_EXTENSION;

//My Dispatch routine (called w/ PASSIVE_IRQL)
NTSTATUS MyDispatchRoutine(..., IN IRP pIrp)
{
... //myPdx points to my device extension
...
switch (controlCode)
{
case IOCTL_GET:
if (!myPdx->start) <====spinlocks around this needed?
{ //IoCompleteRequest with error and exit
}

if (myPdx->pCurrIrp) <====spinlocks around this needed?
{ //IoCompleteRequest with busy error and exit
}
........
InterlockedExchangePointer((PVOID*)&myPdx->pCurrIrp, pIrp);
//wake worker thread
KeSetEvent(&myPdx->evIoctl, IO_NO_INCREMENT, FALSE);
.......
break;
case IOCTL_START:
InterlockedExchange((LONG*)&(myPdx->start), TRUE );
....
....//IoCompleteRequest with ok
break;
.....
.....
}
}

//My executive worker thread which is signalled by the IOCTL above
// (called w/ PASSIVE_IRQL)
void MyWorkerThread(void)
{
...//myPdx points to my device extension
PIRP pIrp;
...
...//Acquire mutex (stored in dev. extnsion; not shown) to serialize
...// this thread execution vs. other instances of this thread
...
if (myPdx->bCaptureSound) <====spinlocks around this needed?
{
KeWaitForSingleObject(&myPdx->evIoctl, Executive, KernelMode,
FALSE, NULL);
KeClearEvent(&myPdx->evIoctl);
pIrp = myPdx->pCurrIrp; <====spinlocks around this needed?
........
InterlockedExchangePointer((PVOID *)&myPdx->pCurrIrp, NULL);
........//do other stuff
.......//release mutex acquired above for serialization
}

I have indicated where I am confused about the spinlock thing.

For writing to any variables in my device extension I use the
InterlockedXXX functions but during reading of those variables, do I
need spinlocks or some other protection? My understanding is that any
"regular" read statement (such as "pIrp = myPdx->pCurrIrp") comprises
of multiple machine instructions (ie. non-atomic) during which the
scheduler can swap out the process and another process can modify the
data resulting in corruption. Is this reasoning correct??

On a uniprocessor system I have seen that the worker thread execution
is interspersed with the dispatch routine. But since in the worker
thread I am working with the pointer to myPdx->pCurrIrp called pIrp,
the dispatch routine is free to modify myPdx->pCurrIrp. So things work
out fine w/ no lost IRPs. I wonder if things would change in an SMP
machine.

I know I asked a mouthfull (screenfull?) and I appreciate all tips.

Thanks & Regards,

James

Re: Is this code SMP safe? by Alexander

Alexander
Fri Jul 01 17:34:35 CDT 2005

For atomic checking that an IRP pointer is NULL and replacing it you should
use InterlockedCompareExchangePointer:

if (NULL == InterlockedCompareExchangePointer(((PVOID*)&myPdx->pCurrIrp,
pIrp, NULL))
{
// success
}

To atomically fetch IRP pointer and replace it with NULL, you should write:

pIrp = InterlockedExchangePointer((PVOID *)&myPdx->pCurrIrp, NULL);

You should not use ClearEvent. Use auto-reset event instead, otherwise you
may get a race condition.

<liquidl@email.com> wrote in message
news:1120244566.998824.219960@f14g2000cwb.googlegroups.com...
> Hi,
>
> I am new at driver programming. I was wondering is the following code
> is SMP safe or would I need spinlocks around the indicated statements.
>
> //My device extension declaration
> typedef struct _DEVICE_EXTENSION
> {
> PIRP pCurrIrp;
> BOOL start;
> KEVENT evIoctl;
> } DEVICE_EXTENSION, *PDEVICE_EXTENSION;
>
> //My Dispatch routine (called w/ PASSIVE_IRQL)
> NTSTATUS MyDispatchRoutine(..., IN IRP pIrp)
> {
> ... //myPdx points to my device extension
> ...
> switch (controlCode)
> {
> case IOCTL_GET:
> if (!myPdx->start) <====spinlocks around this needed?
> { //IoCompleteRequest with error and exit
> }
>
> if (myPdx->pCurrIrp) <====spinlocks around this needed?
> { //IoCompleteRequest with busy error and exit
> }
> ........
> InterlockedExchangePointer((PVOID*)&myPdx->pCurrIrp, pIrp);
> //wake worker thread
> KeSetEvent(&myPdx->evIoctl, IO_NO_INCREMENT, FALSE);
> .......
> break;
> case IOCTL_START:
> InterlockedExchange((LONG*)&(myPdx->start), TRUE );
> ....
> ....//IoCompleteRequest with ok
> break;
> .....
> .....
> }
> }
>
> //My executive worker thread which is signalled by the IOCTL above
> // (called w/ PASSIVE_IRQL)
> void MyWorkerThread(void)
> {
> ...//myPdx points to my device extension
> PIRP pIrp;
> ...
> ...//Acquire mutex (stored in dev. extnsion; not shown) to serialize
> ...// this thread execution vs. other instances of this thread
> ...
> if (myPdx->bCaptureSound) <====spinlocks around this needed?
> {
> KeWaitForSingleObject(&myPdx->evIoctl, Executive, KernelMode,
> FALSE, NULL);
> KeClearEvent(&myPdx->evIoctl);
> pIrp = myPdx->pCurrIrp; <====spinlocks around this needed?
> ........
> InterlockedExchangePointer((PVOID *)&myPdx->pCurrIrp, NULL);
> ........//do other stuff
> .......//release mutex acquired above for serialization
> }
>
> I have indicated where I am confused about the spinlock thing.
>
> For writing to any variables in my device extension I use the
> InterlockedXXX functions but during reading of those variables, do I
> need spinlocks or some other protection? My understanding is that any
> "regular" read statement (such as "pIrp = myPdx->pCurrIrp") comprises
> of multiple machine instructions (ie. non-atomic) during which the
> scheduler can swap out the process and another process can modify the
> data resulting in corruption. Is this reasoning correct??
>
> On a uniprocessor system I have seen that the worker thread execution
> is interspersed with the dispatch routine. But since in the worker
> thread I am working with the pointer to myPdx->pCurrIrp called pIrp,
> the dispatch routine is free to modify myPdx->pCurrIrp. So things work
> out fine w/ no lost IRPs. I wonder if things would change in an SMP
> machine.
>
> I know I asked a mouthfull (screenfull?) and I appreciate all tips.
>
> Thanks & Regards,
>
> James
>



Re: Is this code SMP safe? by liquidl

liquidl
Tue Jul 05 13:16:10 CDT 2005

So I do not need spinlocks around the marked statements in the code?

Also, in case of the test

case IOCTL_GET:
if (!myPdx->start)
{ //IoCompleteRequest with error and exit
}

I only need to read the value of the flag and not really change it.
The InterlockedXXX functions always set the value also. How do I
atomically read without setting?

Can you elaborate on why ClearEvent can cause a race condition?

Thanks


Re: Is this code SMP safe? by Alexander

Alexander
Wed Jul 06 00:35:30 CDT 2005

If you do KeWaitForSingleObject, followed by ClearEvent, another thread can
call SetEvent in between those calls, and you will lose that signal. With a
synchronization event (auto-reset) it won't happen.

If you want just to test a flag, you don't usually need any synchronization
functions. But in your code you may want to set some flag after you test it,
and such a test should usually be done as atomic "test and set" call.

<liquidl@email.com> wrote in message
news:1120587370.716341.58530@z14g2000cwz.googlegroups.com...
> So I do not need spinlocks around the marked statements in the code?
>
> Also, in case of the test
>
> case IOCTL_GET:
> if (!myPdx->start)
> { //IoCompleteRequest with error and exit
> }
>
> I only need to read the value of the flag and not really change it.
> The InterlockedXXX functions always set the value also. How do I
> atomically read without setting?
>
> Can you elaborate on why ClearEvent can cause a race condition?
>
> Thanks
>