[ovs-dev] [PATCH 3/5] datapath-windows: Multiple NBLs support for ingress data path

[Nithin Raju]

hi Sorin,
I looked at this patch. I had a few comments.

> +static NTSTATUS
> +OvsCreateNewNBLsFromMultipleNBs(POVS_SWITCH_CONTEXT switchContext,
> +                                PNET_BUFFER_LIST *curNbl,
> +                                PNET_BUFFER_LIST *nextNbl)
> +{
> +    NTSTATUS status = STATUS_SUCCESS;
> +    PNET_BUFFER_LIST newNbls = NULL;
> +    PNET_BUFFER_LIST lastNbl = NULL;
> +    PNET_BUFFER_LIST nbl = NULL;
> +    POVS_BUFFER_CONTEXT bufContext = NULL;
> +    BOOLEAN error = TRUE;
> +
> +    do {
> +        /* Decrement buffer context reference count. */
> +        bufContext = (POVS_BUFFER_CONTEXT)
> +            NET_BUFFER_LIST_CONTEXT_DATA_START(*curNbl);
> +        InterlockedDecrement((volatile LONG*)&bufContext->refCount);

I am wondering why we need to decrement the ‘bufContext->refCount’ here. We set it to 1 in OvsInitExternalNBLContext(). OvsPartialCopyToMultipleNBLs() would increment the value by “number of NBLs created”. When you call OvsCompleteNBL() on each of the new NBLs created, they decrement the refCount on the parent. Pls. see the following code in OvsCompleteNBL(). There’s no need to explicitly decrement the refCount.

     if (parent != NULL) {                                                     
         ctx = (POVS_BUFFER_CONTEXT)NET_BUFFER_LIST_CONTEXT_DATA_START(parent);
         ASSERT(ctx && ctx->magic == OVS_CTX_MAGIC);                           
         value = InterlockedDecrement((LONG volatile *)&ctx->refCount);        
         if (value == 0) {                                                     
             return OvsCompleteNBL(context, parent, FALSE);                    
         }                                                                     
     }                                                                         

> +
> +        /* Create new NBLs from curNbl with multiple net buffers. */
> +        newNbls = OvsPartialCopyToMultipleNBLs(switchContext,
> +                                               *curNbl, 0, 0, TRUE);
> +        if (NULL == newNbls) {
> +            OVS_LOG_ERROR("Failed to allocate NBLs with single NB.");
> +            status = NDIS_STATUS_RESOURCES;
> +            break;
> +        }
> +
> +        nbl = newNbls;
> +        while (nbl) {
> +            lastNbl = nbl;
> +            nbl = NET_BUFFER_LIST_NEXT_NBL(nbl);
> +        }

This can potentially be optimized by having OvsPartialCopyToMultipleNBLs() return a pointer to the last NBL also.

thanks,
-- Nithin