Windows System Software -- Consulting, Training, Development -- Unique Expertise, Guaranteed Results

Before Posting...
Please check out the Community Guidelines in the Announcements and Administration Category.

Search the collection before remove or is a collection the right choice?

OneKneeToeOneKneeToe Member Posts: 16

Hello Community:

Thank you again for taking the time to look at my question.

In my driver, as read requests arrive, i add the request objects to a collection. As interrupts are processed, read requests are removed from the collection, data copied and request completed.

Now I have added the ability for a read request to be cancelled. At the time of adding the request to the collection, I mark it cancelable. When I remove a request from the collection, I unmark it. I have an EvtRequestCancel that will remove the specific request object from the collection - WdfCollectionRemove(...). I also use a spinlock when accessing the Collection.

Here is where I am stuck - WdfCollectionRemove has no OUT or return value. There is no way for me to know that the request was successfully removed so I can complete it as cancelled.

What is the right approach?

  • Should I acquire the spinlock and for-loop search the collection?
  • Should I even be using a collection in this scenario?

Thank you again!

Regards,
Juan

Comments

  • Tim_RobertsTim_Roberts Member - All Emails Posts: 13,158

    Why are you using a WDFCOLLECTION instead of a manual WDFQUEUE, which was designed for this task and will handle the cancellation for you automatically? There are a large number of very ugly race conditions possible in cancel handling. That is one of the huge benefits of the WDFQUEUE.

    Tim Roberts, [email protected]
    Providenza & Boekelheide, Inc.

  • Peter_Viscarola_(OSR)Peter_Viscarola_(OSR) Administrator Posts: 7,505

    Not only that, but WDFCOLLECTION has a significant number of disadvantages. Most people aren’t aware of this. For one, WDFCOLLECTION allocates on every insert (so you have to handle insert failures), and deallocates on every remove (so, let’s just say it’s not a highly performant implementation).

    I don’t understand why they implemented WDFCOLLECTION this way. You would think they just put a list head in every WDFOBJECT... but, apparently not.

    Peter

    Peter Viscarola
    OSR
    @OSRDrivers

  • Doron_HolanDoron_Holan Member - All Emails Posts: 10,448
    I implemented WDFCOLLECTION. The trade off I chose was that I didn’t want each wdf object to carry the cost of /potentially/ being added to a collection, so the cost is incurred when added. Furthermore, an object can be added to more than one collection, so now if an object had the list entry you need to track added state and list iteration behaves differently when added to the second list as the underlying linked structure would be different. In the end, WDFCOLLECTION was a meant to be a simple, albeit inefficient, utility object AND if a dev wants something better, they are free to implement it on their own.
    d
  • Peter_Viscarola_(OSR)Peter_Viscarola_(OSR) Administrator Posts: 7,505

    Hi Doron,

    The trade off I chose was that I didn’t want each wdf object to carry the cost of /potentially/ being added to a collection

    The cost being storage for a LIST_ENTRY... in the overall structure that’s already being allocated...vs the cost of having to do an allocation on every insert?

    Suffice it to say that is not the choice I would have made.

    I’m willing to bet, a lot, that failure on insert is most often a failure mode that’s not handled by driver code. I bet 90% of the time the Object that was being inserted just gets dropped on the floor by the dev with a “this will never happen” comment.

    an object can be added to more than one collection

    I didn’t know that!

    now if an object had the list entry you need to track added state

    Yeah, I would have chosen to not allow an object to be in multiple collections at the same time. I wonder how often that’s used?

    if a dev wants something better, they are free to implement it on their own.

    Sure. But... It’s actually harder than it sounds to do that, without owning the definition of the Object. The code gets awkward and ugly. You put the LIST_ENTRY in the Object’s general context? Then your making a list that’s object context type specific. What do you pass into Insert? A pointer to the context? Or a pointer to the LIST_ENTRY IN THE CONTEXT? Imagine the fun of walking the list and checking for some attribute of the Object in that pattern? You could create a unique context when the Object is instantiated just for this purpose... But, again, it’s ugly during Object creation and I don’t think that it ends much better.

    Peter Viscarola
    OSR
    @OSRDrivers

  • OneKneeToeOneKneeToe Member Posts: 16
    edited November 20

    Hello Tim:

    Thank you for the reply.

    The way I understand things, I am using a queue to handle the pended IO file reads. The read request may not be serviceable since HW may not have provided data yet. So I store the request objects into a collection - looked like an easy / lightweight option.

    I stumbled upon SynchronizationScope and WdfSynchronizationScopeQueue. My understanding here is that this would ensure that the DPC and the EvtRequestCancel do not run at the same time. But will this be enough?

    • If the driver gets a call to cancel Request_A, but before EvtRequestCancel can run, the interrupts DPC runs.
    • The DPC unmarks Request_A, copies data and completes it.
    • What happens to the call to cancel Reqeust_A? Request_A is no longer a valid object.
    • If the EvntReqeustCancel routine runs, it will complete Request_A, resulting in a BSOD I imagine.
  • Peter_Viscarola_(OSR)Peter_Viscarola_(OSR) Administrator Posts: 7,505

    So I store the request objects into a collection

    And, as Mr. Roberts said, this is precisely the purpose for which WDFQUEUEs with Manual Dispatching were created. While your Request is on the Queue, if it gets canceled, it will be removed. If you haven't started the Request on your device yet, that 100% eliminates the race condition on cancel.

    looked like an easy / lightweight option

    But it's not. Did you read the rest of the thread you initiated?

    But will this be enough?

    Well, it's "enough"... if you write your cancel logic correctly. The example you cite is exactly the issue. Cancel handling can be very difficult. It's one of the very few areas that WDF did not sufficiently address or provide us with a simplifying abstraction.

    By the way SyncScopeQueue will only serialize with your DpcForIsr if (a) You don't have a PASSIVE_LEVEL ExecutionLevel constraint, and (b) you set AutomaticSerialzation to TRUE when in your WDF_INTERRUPT_CONFIG structure (when you specify your DpcForIsr). IIRC, the default is in fact TRUE.

    You really have to think-through cancellation to handle it correctly. I suggest you start by asking yourself: "When I put a Request on my hardware, what's the maximum amount of time my hardware can take to complete it?" If the answer is on the order of a few seconds worst-case, then there's no need for you to cancel in-progress requests at all.

    Cancel logic has always been, and remains, the most common place to find errors in driver code (at least in the code that I get to review). Few people seem to understand the difference between Cancel and Cleanup, and fewer still seem to understand what they need to do at cancel. It seems, Mr. OneKneeToe, you are well on your way to understanding the issue.

    Peter

    Peter Viscarola
    OSR
    @OSRDrivers

  • OneKneeToeOneKneeToe Member Posts: 16

    Hello Peter:

    Thanks again for getting involved in one of my questions - I had not seen your responses as I was writing mine this morning - they (Doron, Tim's follow-up and yours) must have posted as I was writing.

    I think the Pros are telling me that I'm doing it wrong ;).

    Thank you Tim and Peter; I will replace the collection with a queue. :smiley:

    Juan

  • OneKneeToeOneKneeToe Member Posts: 16

    Just a quick update to close-off this thread; I was successful in replacing the use of collections with a queue.

    • No real change in logic, other than removing the EvtCancel function; Although I could have kept it, but I had no need to clean-up before completing the request.
    • I eliminated all the "(Un)MarkCancelable" calls:
    1. cancellable requests cannot be forwarded to a queue.
    2. While the request is in the queue, the framework handles cancellation.
    3. In my case, once the driver starts to process the request, i.e. request removed from queue, it cannot be canceled.

    Thanks again Tim, Doron and Peter.

    Queue creation

    WDF_IO_QUEUE_CONFIG queueConfig;
    WDF_IO_QUEUE_CONFIG_INIT( &queueConfig, WdfIoQueueDispatchManual );
    queueConfig.PowerManaged = WdfFalse;
    queueConfig.AllowZeroLengthRequests = FALSE;
    queueConfig.DefaultQueue = FALSE;
    
    WDF_OBJECT_ATTRIBUTES queueAttributes;
    WDF_OBJECT_ATTRIBUTES_INIT( &queueAttributes );
    queueAttributes.SynchronizationScope = WdfSynchronizationScopeQueue;
    
    NTSTATUS status{ WdfIoQueueCreate( wdfDevice, &queueConfig, &queueAttributes, &myWdfPendedRequests ) };
    if( !NT_SUCCESS( status ) )
    {
       TraceEvents( TRACE_LEVEL_ERROR, DBG_REQUESTHANDLER, "Queue creation failed. status=[%!STATUS!]", status);
    }
    return status;
    

    enqueueReadRequest

    NTSTATUS status{ validateRequestParameters( request ) };
    if( NT_SUCCESS( status ) )
    {
       status = WdfRequestForwardToIoQueue( request, myWdfPendedRequests );
       if( NT_SUCCESS( status ) )
       {
          KeSetEvent( &( EvtNewReadRequest ), priority, FALSE );
       }
       else
       {
          TraceEvents( TRACE_LEVEL_ERROR, DBG_REQUESTHANDLER,
                                  "Failed to add request to queue. status=[%!STATUS!]", status);
       }        
    }
    return status;
    

    getRequest

    WDFREQUEST request{ NULL };
    NTSTATUS status { WdfIoQueueRetrieveNextRequest( myWdfPendedRequests, &request ) };
    if( STATUS_NO_MORE_ENTRIES == status )
    {
       TraceEvents( TRACE_LEVEL_INFORMATION, DBG_REQUESTHANDLER, "Queue is empty." );
       request = NULL;
    }
    else if( !NT_SUCCESS( status ) )
    {
       TraceEvents( TRACE_LEVEL_ERROR, DBG_REQUESTHANDLER,
                               "Failed to retrieve next request from queue. status=[%STATUS!].", status );
       request = NULL;
    }
    return request;
    

    processRequest

    WDFREQUEST request { getRequest() };
    if( NULL != request )
    {
       WDFMEMORY readFileMemory;
       NTSTATUS status { WdfRequestRetrieveOutputMemory( request, &readFileMemory ) };
       if( NT_SUCCESS( status ) )
       {
          status = WdfMemoryCopyFromBuffer( readFileMemory, 0, &data, numBytes );
          if( !NT_SUCCESS( status ) )
          {
             TraceEvents( TRACE_LEVEL_ERROR, DBG_REQUESTHANDLER,
                                     "WdfMemoryCopyFromBuffer() failed, status=[%!STATUS!]", status );
             status = STATUS_UNSUCCESSFUL;
             numBytes = 0;
          }
       }
       else
       {
          TraceEvents( TRACE_LEVEL_ERROR, DBG_REQUESTHANDLER,
                                  "Call to WdfRequestRetrieveOutputMemory() failed, status=[%!STATUS!]", status );
          status = STATUS_UNSUCCESSFUL;
          numBytes = 0;
       }
       completeRequest( request, status, numBytes );
    } // if not null
    
Sign In or Register to comment.

Howdy, Stranger!

It looks like you're new here. If you want to get involved, click one of these buttons!

Upcoming OSR Seminars
Writing WDF Drivers 21 Oct 2019 OSR Seminar Space & ONLINE
Internals & Software Drivers 18 Nov 2019 Dulles, VA
Kernel Debugging 30 Mar 2020 OSR Seminar Space
Developing Minifilters 27 Apr 2020 OSR Seminar Space & ONLINE