Nithin, thanks for your review. Please see my answers inline.
-----Original Message-----
From: Nithin Raju [mailto:nit...@vmware.com]
Sent: Thursday, 14 May, 2015 18:12
To: Sorin Vinturis
Cc: dev@openvswitch.org
Subject: Re: [ovs-dev] [PATCH 3/5] datapath-windows: Multiple NBLs support for
ingress data path
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);
}
}
SV: Shouldn't the initial NBL with multiple NBs be completed alfter all child
NBLs with single NB are completed?
That is the reason for decrementing the reference count before calling
OvsPartialCopyToMultipleNBLs().
> +
> + /* 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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
