Hi Nithin, Thank you for reviewing this patch. Please see my answers inline.
-Sorin -----Original Message----- From: Nithin Raju [mailto:nit...@vmware.com] Sent: Tuesday, 22 March, 2016 02:31 To: Sorin Vinturis Cc: dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH v6 2/6] datapath-windows: Added recirculation support. Hi Sorin/Alin, I looked at the patch and had a few comments. Pls. find them inlined. In general, I was a little confused about how we use the recirc ID and the hash. We can discuss more during the IRC meeting. Thanks, -- Nithin -----Original Message----- From: dev <dev-boun...@openvswitch.org> on behalf of Sorin Vinturis <svintu...@cloudbasesolutions.com> Date: Friday, March 18, 2016 at 7:58 AM To: "dev@openvswitch.org" <dev@openvswitch.org> Subject: [ovs-dev] [PATCH v6 2/6] datapath-windows: Added recirculation support. >Recirculation support for the OVS extension. > >Tested using PING and iperf with Driver Verifier enabled. > >Signed-off-by: Sorin Vinturis <svintu...@cloudbasesolutions.com> >Co-authored-by: Alin Gabriel Serdean <aserd...@cloudbasesolutions.com> >Reported-by: Sorin Vinturis <svintu...@cloudbasesolutions.com> >Reported-at: >https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvsw >itc >h_ovs-2Dissues_issues_104&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihV >MNt >Xt-uEs&r=pNHQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=OpMpvAAJDApIpx1r >ELD >Zz_PzL7S9hfkeHD2h-6_KTCg&s=oZxN8oCzUixc4GlkvE6n4m0RN2gf2XiL0OI-sVgXdus& >e= >--- >v2: Initialize flow key before using it. >v3: Synchronized access to deferred actions queue. >v4: Address comments. >v5: update flow key and layers in the recirc action, queue/execute packet > per source port >v6: Added percpu deferred action queues. >--- >--- > datapath-windows/automake.mk | 3 + > datapath-windows/ovsext/Actions.c | 234 >+++++++++++++++++++++++++++--- > datapath-windows/ovsext/Actions.h | 56 +++++++ > datapath-windows/ovsext/Datapath.c | 3 + > datapath-windows/ovsext/DpInternal.h | 2 +- > datapath-windows/ovsext/Flow.c | 78 ++++++++-- > datapath-windows/ovsext/Flow.h | 5 +- > datapath-windows/ovsext/Netlink/Netlink.h | 11 ++ > datapath-windows/ovsext/PacketIO.c | 16 +- > datapath-windows/ovsext/PacketIO.h | 10 -- > datapath-windows/ovsext/Recirc.c | 224 >++++++++++++++++++++++++++++ > datapath-windows/ovsext/Recirc.h | 90 ++++++++++++ > datapath-windows/ovsext/Tunnel.c | 15 +- > datapath-windows/ovsext/User.c | 13 +- > datapath-windows/ovsext/Util.h | 1 + > datapath-windows/ovsext/ovsext.vcxproj | 3 + > 16 files changed, 696 insertions(+), 68 deletions(-) create mode > 100644 datapath-windows/ovsext/Actions.h create mode 100644 > datapath-windows/ovsext/Recirc.c create mode 100644 > datapath-windows/ovsext/Recirc.h > >diff --git a/datapath-windows/automake.mk >b/datapath-windows/automake.mk index f29f548..04fc97f 100644 >--- a/datapath-windows/automake.mk >+++ b/datapath-windows/automake.mk >@@ -9,6 +9,7 @@ EXTRA_DIST += \ > datapath-windows/misc/uninstall.cmd \ > datapath-windows/ovsext.sln \ > datapath-windows/ovsext/Actions.c \ >+ datapath-windows/ovsext/Actions.h \ > datapath-windows/ovsext/Atomic.h \ > datapath-windows/ovsext/BufferMgmt.c \ > datapath-windows/ovsext/BufferMgmt.h \ @@ -45,6 +46,8 @@ EXTRA_DIST >+= \ > datapath-windows/ovsext/PacketIO.h \ > datapath-windows/ovsext/PacketParser.c \ > datapath-windows/ovsext/PacketParser.h \ >+ datapath-windows/ovsext/Recirc.c \ >+ datapath-windows/ovsext/Recirc.h \ > datapath-windows/ovsext/Stt.c \ > datapath-windows/ovsext/Stt.h \ > datapath-windows/ovsext/Switch.c \ >diff --git a/datapath-windows/ovsext/Actions.c >b/datapath-windows/ovsext/Actions.c >index 5a04541..333d487 100644 >--- a/datapath-windows/ovsext/Actions.c >+++ b/datapath-windows/ovsext/Actions.c >@@ -16,6 +16,7 @@ > > #include "precomp.h" > >+#include "Actions.h" > #include "Debug.h" > #include "Event.h" > #include "Flow.h" >@@ -24,6 +25,7 @@ > #include "NetProto.h" > #include "Offload.h" > #include "PacketIO.h" >+#include "Recirc.h" > #include "Stt.h" > #include "Switch.h" > #include "User.h" >@@ -35,6 +37,8 @@ > #endif > #define OVS_DBG_MOD OVS_DBG_ACTION > >+#define OVS_DEST_PORTS_ARRAY_MIN_SIZE 2 >+ > typedef struct _OVS_ACTION_STATS { > UINT64 rxGre; > UINT64 txGre; >@@ -66,7 +70,6 @@ OVS_ACTION_STATS ovsActionStats; > * exercised while adding new members to the structure - only add ones >that get > * used across multiple stages in the pipeline/get used in multiple >functions. > */ >-#define OVS_DEST_PORTS_ARRAY_MIN_SIZE 2 typedef struct >OvsForwardingContext { > POVS_SWITCH_CONTEXT switchContext; > /* The NBL currently used in the pipeline. */ @@ -99,7 +102,7 @@ >typedef struct OvsForwardingContext { > */ > OvsIPv4TunnelKey tunKey; > >- /* >+ /* > * Tunneling - Tx: > * To store the output port, when it is a tunneled port. We don't >foresee > * multiple tunneled ports as outport for any given NBL. >@@ -117,7 +120,6 @@ typedef struct OvsForwardingContext { > OVS_PACKET_HDR_INFO layers; > } OvsForwardingContext; > >- > /* > * >----------------------------------------------------------------------- >--- > * OvsInitForwardingCtx -- >@@ -564,10 +566,10 @@ OvsCompleteNBLForwardingCtx(OvsForwardingContext >*ovsFwdCtx, > static __inline NDIS_STATUS > OvsDoFlowLookupOutput(OvsForwardingContext *ovsFwdCtx) { >- OvsFlowKey key; >- OvsFlow *flow; >- UINT64 hash; >- NDIS_STATUS status; Unnecessary initialization. SV: I think that we should initialize all local variables before using them to avoid reading garbage when they are passed down the stack. While testing this patch there was a bug caused by the uninitialization of the flow key variable. In this particular case the recircId, member of the flow key, contained garbage, instead of zero, which was being reported to the userspace. >+ OvsFlowKey key = { 0 }; >+ OvsFlow *flow = NULL; >+ UINT64 hash = 0; >+ NDIS_STATUS status = NDIS_STATUS_SUCCESS; > POVS_VPORT_ENTRY vport = > OvsFindVportByPortNo(ovsFwdCtx->switchContext, >ovsFwdCtx->srcVportNo); > if (vport == NULL || vport->ovsState != OVS_STATE_CONNECTED) { @@ >-595,11 +597,13 @@ OvsDoFlowLookupOutput(OvsForwardingContext >*ovsFwdCtx) > if (flow) { > OvsFlowUsed(flow, ovsFwdCtx->curNbl, &ovsFwdCtx->layers); > ovsFwdCtx->switchContext->datapath.hits++; >- status = OvsActionsExecute(ovsFwdCtx->switchContext, >- ovsFwdCtx->completionList, >ovsFwdCtx->curNbl, >- ovsFwdCtx->srcVportNo, >ovsFwdCtx->sendFlags, >- &key, &hash, &ovsFwdCtx->layers, >- flow->actions, flow->actionsLen); >+ status = OvsDoExecuteActions(ovsFwdCtx->switchContext, >+ ovsFwdCtx->completionList, >+ ovsFwdCtx->curNbl, >+ ovsFwdCtx->srcVportNo, >+ ovsFwdCtx->sendFlags, >+ &key, &hash, &ovsFwdCtx->layers, >+ flow->actions, flow->actionsLen); > ovsFwdCtx->curNbl = NULL; > } else { > LIST_ENTRY missedPackets; >@@ -1520,8 +1524,58 @@ OvsExecuteSetAction(OvsForwardingContext >*ovsFwdCtx, > > /* > * >----------------------------------------------------------------------- >--- >- * OvsActionsExecute -- >- * Interpret and execute the specified 'actions' on the specifed >packet >+ * OvsActionExecuteRecirc -- >+ * The function adds a deferred action to allow the current packet, >nbl, >+ * to re-enter datapath packet processing. >+ * >----------------------------------------------------------------------- >--- >+ */ Nit: OvsActionExecuteRecirc => OvsExecuteRecirc would probably be simpler and clearer name. Eg. OvsExecuteSetAction(): it executes the set action. SV: I will rename the function to OvsExecuteRecirc. >+NDIS_STATUS >+OvsActionExecuteRecirc(OvsFlowKey *key, >+ UINT64 *hashAction, >+ const PNL_ATTR actions, >+ int rem, >+ OvsForwardingContext ovsFwdCtx) There does not seem to be a good reason to pass ovsFwdCtx as a structure and not a pointer. >+{ >+ POVS_DEFERRED_ACTION deferredAction = NULL; >+ PNET_BUFFER_LIST newNbl = NULL; >+ UINT64 hash = 0; Nit: initialization not necessary. >+ >+ if (!NlAttrIsLast(actions, rem)) { >+ /* >+ * Recirc action is the not the last action of the action >+ list, >so we >+ * need to clone the packet. >+ */ >+ newNbl = OvsPartialCopyNBL(ovsFwdCtx.switchContext, >ovsFwdCtx.curNbl, >+ 0, 0, TRUE /*copy NBL info*/); >+ /* >+ * Skip the recirc action when out of memory, but continue on >with the >+ * rest of the action list. >+ */ >+ if (newNbl == NULL) { >+ ovsActionStats.noCopiedNbl++; >+ return NDIS_STATUS_SUCCESS; >+ } >+ ovsFwdCtx.curNbl = newNbl; If you pass ŒovsFwdCtx' by pointer, you¹ll have to make sure that ovsFwdCxt->curNbl does not get updated. >+ } >+ >+ hash = hashAction ? *hashAction : OvsHashFlow(key); >+ >+ deferredAction = OvsAddDeferredActions(ovsFwdCtx.curNbl, key, >+ hash, >NULL); >+ if (deferredAction) { >+ deferredAction->key.recircId = NlAttrGetU32(actions); >+ } else { >+ if (newNbl) { As stat for this: ovsActionsStats.deferredActionsFull++? SV: OK. >+ OvsCompleteNBL(ovsFwdCtx.switchContext, newNbl, TRUE); >+ } >+ } >+ >+ return NDIS_STATUS_SUCCESS; >+} >+ >+/* >+ * >----------------------------------------------------------------------- >--- >+ * OvsDoExecuteActions -- >+ * Interpret and execute the specified 'actions' on the specified >packet > * 'curNbl'. The expectation is that if the packet needs to be >dropped > * (completed) for some reason, it is added to 'completionList' so >that the > * caller can complete the packet. If 'completionList' is NULL, the >NBL is >@@ -1537,16 +1591,16 @@ OvsExecuteSetAction(OvsForwardingContext >*ovsFwdCtx, > * >----------------------------------------------------------------------- >--- > */ > NDIS_STATUS >-OvsActionsExecute(POVS_SWITCH_CONTEXT switchContext, >- OvsCompletionList *completionList, >- PNET_BUFFER_LIST curNbl, >- UINT32 portNo, >- ULONG sendFlags, >- OvsFlowKey *key, >- UINT64 *hash, >- OVS_PACKET_HDR_INFO *layers, >- const PNL_ATTR actions, >- INT actionsLen) >+OvsDoExecuteActions(POVS_SWITCH_CONTEXT switchContext, >+ OvsCompletionList *completionList, >+ PNET_BUFFER_LIST curNbl, >+ UINT32 portNo, >+ ULONG sendFlags, >+ OvsFlowKey *key, >+ UINT64 *hash, >+ OVS_PACKET_HDR_INFO *layers, >+ const PNL_ATTR actions, >+ INT actionsLen) > { > PNL_ATTR a; > INT rem; >@@ -1695,6 +1749,31 @@ OvsActionsExecute(POVS_SWITCH_CONTEXT >switchContext, > break; > } > >+ case OVS_ACTION_ATTR_RECIRC: >+ { >+ if (ovsFwdCtx.destPortsSizeOut > 0 || >+ ovsFwdCtx.tunnelTxNic >!= NULL >+ || ovsFwdCtx.tunnelRxNic != NULL) { >+ status = OvsOutputBeforeSetAction(&ovsFwdCtx); >+ if (status != NDIS_STATUS_SUCCESS) { >+ dropReason = L"OVS-adding destination failed"; >+ goto dropit; >+ } >+ } >+ >+ status = OvsActionExecuteRecirc(key, hash, >+ (const PNL_ATTR)a, rem, >+ ovsFwdCtx); Nit: OvsActionExecuteRecirc => OvsExecuteRecirc or OvsProcessRecirc() or OvsQueueRecirc() Pass ŒovsFwdCtx¹ by pointer. >+ if (status != NDIS_STATUS_SUCCESS) { >+ dropReason = L"OVS-recirculation action failed"; >+ goto dropit; >+ } OvsActionExecuteRecirc() is not really returning failure under any conditions. Do we want to update that to be able to handle errors here? SV: I think that failure to execute the recirculation action should not impact the execution of the other actions. This is consistent with the Linux behavior. >+ >+ if (NlAttrIsLast(a, rem)) { >+ goto exit; >+ } Checking for last attribute and bailing out is not necessary. If it is the last attribute, the outer while loop will break out anwyay. In fact, with this code, you¹ll end up causing an NBL leak if OvsActionExecuteRecirc() did not enqueue the packet. SV: Actually, if recirculation is the last attribute the function should return the status and allow the nbl to be processed, and eventually completed, in OvsProcessDeferredActions. Otherwise, the while loop will break and the nbl will be completed twice resulting in a BSOD. >+ break; >+ } >+ > case OVS_ACTION_ATTR_USERSPACE: > { > PNL_ATTR userdataAttr; >@@ -1781,5 +1860,112 @@ dropit: > OvsCompleteNBLForwardingCtx(&ovsFwdCtx, dropReason); > } > >+exit: >+ return status; >+} >+ >+NDIS_STATUS Nit: pls add a header. At least a namesake :) SV: Sure. >+OvsActionsExecute(POVS_SWITCH_CONTEXT switchContext, >+ OvsCompletionList *completionList, >+ PNET_BUFFER_LIST curNbl, >+ UINT32 portNo, >+ ULONG sendFlags, >+ OvsFlowKey *key, >+ UINT64 *hash, >+ OVS_PACKET_HDR_INFO *layers, >+ const PNL_ATTR actions, >+ INT actionsLen) >+{ >+ NDIS_STATUS status = STATUS_SUCCESS; >+ >+ status = OvsDoExecuteActions(switchContext, completionList, curNbl, >+ portNo, sendFlags, key, hash, layers, >+ actions, actionsLen); >+ >+ if (status == STATUS_SUCCESS) { >+ status = OvsProcessDeferredActions(switchContext, completionList, >+ portNo, sendFlags, layers); >+ } >+ >+ return status; >+} >+ >+NDIS_STATUS >+OvsDoRecircFlowLookupOutput(POVS_SWITCH_CONTEXT switchContext, >+ OvsCompletionList *completionList, >+ PNET_BUFFER_LIST curNbl, >+ OvsFlowKey *key, >+ UINT64 *hash, Why do we need Œkey¹ and Œhash¹? These two can be local variables within OvsDoRecircFlowLookupOutput() >+ UINT32 srcPortNo, >+ OVS_PACKET_HDR_INFO *layers) Nit: pls add a header. At least a namesake :) Also, the function could just be renamed to OvsDoRecirc(). SV: OK. >+{ >+ NDIS_STATUS status = NDIS_STATUS_SUCCESS; >+ OvsFlow *flow = NULL; >+ OvsForwardingContext ovsFwdCtx = { 0 }; >+ ASSERT(layers); >+ >+ OvsInitForwardingCtx(&ovsFwdCtx, switchContext, curNbl, >+ srcPortNo, 0, >+ >NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(curNbl), >+ completionList, layers, TRUE); XXX: Pls. check for failure and call OvsCompleteNBLForwardingCtx(). SV: OvsInitForwardingCtx() is always returning NDIS_STATUS_SUCCESS. I am not really sure if we need a ŒovsFwdCtx¹ here. The whole idea behind ŒovsFwdCtx¹ was to have a ³context² structure that corresponds to a packet, and pass it around from one function to another. Since we are not passing ŒovsFwdCtx¹ to any other function, I am not sure it is needed. SV: Here the 'ovsFwdCtx' context is needed only for OvsCompleteNBLForwardingCtx(). > >+ status = OvsExtractFlow(ovsFwdCtx.curNbl, ovsFwdCtx.srcVportNo, key, >+ &ovsFwdCtx.layers, NULL); >+ if (status != NDIS_STATUS_SUCCESS) { >+ OvsCompleteNBLForwardingCtx(&ovsFwdCtx, >+ L"OVS-Dropped due to extract flow failure"); >+ status = NDIS_STATUS_FAILURE; >+ ovsActionStats.failedFlowMiss++; => ovsActionStats.failedFlowExtract++; The recircID is not being populated in the key as part of OvsExtractFlow(). My understand is that the flows would look something like this: key-foo, actions:bar,recirc(0x10) recirc_id(0x10),key_foo actions:2 We are not passing down the recirc id from the first flow to the second flow. SV: recircID is already present in the key, when this function is called. OvsExtractFlow() does not touch the recircID. >+ } >+ >+ flow = OvsLookupFlowRecirc(&ovsFwdCtx.switchContext->datapath, >+ key, hash); >+ if (flow) { >+ OvsFlowUsed(flow, ovsFwdCtx.curNbl, &ovsFwdCtx.layers); >+ ovsFwdCtx.switchContext->datapath.hits++; >+ status = OvsActionsExecute(ovsFwdCtx.switchContext, >+ ovsFwdCtx.completionList, >ovsFwdCtx.curNbl, >+ ovsFwdCtx.srcVportNo, >ovsFwdCtx.sendFlags, >+ key, hash, &ovsFwdCtx.layers, >+ flow->actions, flow->actionsLen); Where is the check for recursion? SV: In OvsDeferredActionsQueuePush(). Why not call OvsDoActionsExecute() directly? SV: You are right. OvsDoActionsExecute() should be called here. >+ ovsFwdCtx.curNbl = NULL; >+ } else { >+ POVS_VPORT_ENTRY vport = NULL; >+ LIST_ENTRY missedPackets; >+ UINT32 num = 0; >+ >+ ovsFwdCtx.switchContext->datapath.misses++; >+ InitializeListHead(&missedPackets); >+ vport = OvsFindVportByPortNo(switchContext, srcPortNo); >+ if (vport == NULL || vport->ovsState != OVS_STATE_CONNECTED) { >+ OvsCompleteNBLForwardingCtx(&ovsFwdCtx, >+ L"OVS-Dropped due to port removal"); >+ ovsActionStats.noVport++; >+ return NDIS_STATUS_SUCCESS; >+ } >+ status = OvsCreateAndAddPackets(NULL, 0, OVS_PACKET_CMD_MISS, >+ vport, key, ovsFwdCtx.curNbl, >+ srcPortNo == >+ >switchContext->virtualExternalPortId, >+ &ovsFwdCtx.layers, >+ ovsFwdCtx.switchContext, >+ &missedPackets, &num); >+ if (num) { >+ OvsQueuePackets(&missedPackets, num); >+ } >+ if (status == NDIS_STATUS_SUCCESS) { >+ /* Complete the packet since it was copied to user buffer. */ >+ OvsCompleteNBLForwardingCtx(&ovsFwdCtx, >+ L"OVS-Dropped since packet was copied to userspace"); >+ ovsActionStats.flowMiss++; >+ status = NDIS_STATUS_SUCCESS; >+ } else { >+ OvsCompleteNBLForwardingCtx(&ovsFwdCtx, >+ L"OVS-Dropped due to failure to queue to userspace"); >+ status = NDIS_STATUS_FAILURE; >+ ovsActionStats.failedFlowMiss++; >+ } >+ } >+ > return status; > } >diff --git a/datapath-windows/ovsext/Actions.h >b/datapath-windows/ovsext/Actions.h >new file mode 100644 >index 0000000..69a1f69 >--- /dev/null >+++ b/datapath-windows/ovsext/Actions.h >@@ -0,0 +1,56 @@ >+/* >+ * Copyright (c) 2016 Cloudbase Solutions Srl >+ * >+ * Licensed under the Apache License, Version 2.0 (the "License"); >+ * you may not use this file except in compliance with the License. >+ * You may obtain a copy of the License at: >+ * >+ * >https://urldefense.proofpoint.com/v2/url?u=http-3A__www.apache.org_lice >nse >s_LICENSE-2D2.0&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs& >r=p >NHQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=OpMpvAAJDApIpx1rELDZz_PzL7 >S9h fkeHD2h-6_KTCg&s=S30nm7dlEorfpeMZh2t0GL8LoSf_KXweQSeVsbNOCtg&e= >+ * >+ * Unless required by applicable law or agreed to in writing, software >+ * distributed under the License is distributed on an "AS IS" BASIS, >+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or >implied. >+ * See the License for the specific language governing permissions and >+ * limitations under the License. >+ */ >+ >+#ifndef __ACTIONS_H_ >+#define __ACTIONS_H_ 1 >+ >+#include "Switch.h" >+#include "PacketIO.h" >+ >+NDIS_STATUS >+OvsActionsExecute(POVS_SWITCH_CONTEXT switchContext, >+ OvsCompletionList *completionList, >+ PNET_BUFFER_LIST curNbl, >+ UINT32 srcVportNo, >+ ULONG sendFlags, >+ OvsFlowKey *key, >+ UINT64 *hash, >+ OVS_PACKET_HDR_INFO *layers, >+ const PNL_ATTR actions, >+ int actionsLen); There¹s already a declaration of OvsActionsExecute() in PacketIO.h. Maybe we want to clean that up. SV: There is no declaration of OvsActionsExecute() in PacketIO.h or in any other header. >+ >+NDIS_STATUS >+OvsDoExecuteActions(POVS_SWITCH_CONTEXT switchContext, >+ OvsCompletionList *completionList, >+ PNET_BUFFER_LIST curNbl, >+ UINT32 srcVportNo, >+ ULONG sendFlags, >+ OvsFlowKey *key, >+ UINT64 *hash, >+ OVS_PACKET_HDR_INFO *layers, >+ const PNL_ATTR actions, >+ int actionsLen); >+ >+NDIS_STATUS >+OvsDoRecircFlowLookupOutput(POVS_SWITCH_CONTEXT switchContext, >+ OvsCompletionList *completionList, >+ PNET_BUFFER_LIST curNbl, >+ OvsFlowKey *key, >+ UINT64 *hash, >+ UINT32 srcPortNo, >+ OVS_PACKET_HDR_INFO *layers); >+ >+#endif /* __ACTIONS_H_ */ >diff --git a/datapath-windows/ovsext/Datapath.c >b/datapath-windows/ovsext/Datapath.c >index a9a218d..876d6bf 100644 >--- a/datapath-windows/ovsext/Datapath.c >+++ b/datapath-windows/ovsext/Datapath.c >@@ -30,6 +30,7 @@ > #include "Event.h" > #include "User.h" > #include "PacketIO.h" >+#include "Recirc.h" > #include "NetProto.h" > #include "Flow.h" > #include "User.h" >@@ -383,6 +384,7 @@ OvsInit() > { > gOvsCtrlLock = &ovsCtrlLockObj; > NdisAllocateSpinLock(gOvsCtrlLock); >+ OvsDeferredActionsQueueAlloc(); > OvsInitEventQueue(); > } > >@@ -394,6 +396,7 @@ OvsCleanup() > NdisFreeSpinLock(gOvsCtrlLock); > gOvsCtrlLock = NULL; > } >+ OvsDeferredActionsQueueFree(); > } > > VOID >diff --git a/datapath-windows/ovsext/DpInternal.h >b/datapath-windows/ovsext/DpInternal.h >index c094f32..845c132 100644 >--- a/datapath-windows/ovsext/DpInternal.h >+++ b/datapath-windows/ovsext/DpInternal.h >@@ -20,7 +20,6 @@ > #include <netioapi.h> > #define IFNAMSIZ IF_NAMESIZE > #include "../ovsext/Netlink/Netlink.h" >-#include "Mpls.h" > > #define OVS_DP_NUMBER ((uint32_t) 0) > >@@ -166,6 +165,7 @@ typedef __declspec(align(8)) struct OvsFlowKey { > Icmp6Key icmp6Key; /* size 72 */ > MplsKey mplsKey; /* size 8 */ > }; >+ UINT32 recircId; /* Recirculation ID. */ Where is the keylen bring incremented to make sure that recircId is part of the hash calculation? SV: The keyLen is not incremented and it should. I will make the necessary changes. > } OvsFlowKey; > > #define OVS_WIN_TUNNEL_KEY_SIZE (sizeof (OvsIPv4TunnelKey)) diff --git >a/datapath-windows/ovsext/Flow.c b/datapath-windows/ovsext/Flow.c index >5eec513..2ceeb94 100644 >--- a/datapath-windows/ovsext/Flow.c >+++ b/datapath-windows/ovsext/Flow.c >@@ -110,9 +110,7 @@ const NL_POLICY nlFlowPolicy[] = { > [OVS_FLOW_ATTR_PROBE] = {.type = NL_A_FLAG, .optional = TRUE} }; > >-/* For Parsing nested OVS_FLOW_ATTR_KEY attributes. >- * Some of the attributes like OVS_KEY_ATTR_RECIRC_ID >- * are not supported yet. */ >+/* For Parsing nested OVS_FLOW_ATTR_KEY attributes. */ > > const NL_POLICY nlFlowKeyPolicy[] = { > [OVS_KEY_ATTR_ENCAP] = {.type = NL_A_VAR_LEN, .optional = TRUE}, >@@ -252,7 +250,7 @@ OvsFlowNlCmdHandler(POVS_USER_PARAMS_CONTEXT >usrParamsCtx, > UINT32 *replyLen) > { > NTSTATUS rc = STATUS_SUCCESS; >- BOOLEAN ok; >+ BOOLEAN ok = FALSE; > POVS_MESSAGE msgIn = (POVS_MESSAGE)usrParamsCtx->inputBuffer; > POVS_MESSAGE msgOut = (POVS_MESSAGE)usrParamsCtx->outputBuffer; > PNL_MSG_HDR nlMsgHdr = &(msgIn->nlMsg); @@ -260,11 +258,11 @@ >OvsFlowNlCmdHandler(POVS_USER_PARAMS_CONTEXT >usrParamsCtx, > POVS_HDR ovsHdr = &(msgIn->ovsHdr); > PNL_ATTR flowAttrs[__OVS_FLOW_ATTR_MAX]; > UINT32 attrOffset = NLMSG_HDRLEN + GENL_HDRLEN + OVS_HDRLEN; >- OvsFlowPut mappedFlow; >- OvsFlowStats stats; >- struct ovs_flow_stats replyStats; >+ OvsFlowPut mappedFlow = { 0 }; >+ OvsFlowStats stats = { 0 }; >+ struct ovs_flow_stats replyStats = { 0 }; > NL_ERROR nlError = NL_ERROR_SUCCESS; >- NL_BUFFER nlBuf; >+ NL_BUFFER nlBuf = { 0 }; > > RtlZeroMemory(&mappedFlow, sizeof(OvsFlowPut)); > RtlZeroMemory(&stats, sizeof(stats)); @@ -594,9 +592,9 @@ >_FlowNlDumpCmdHandler(POVS_USER_PARAMS_CONTEXT >usrParamsCtx, > UINT32 hdrOffset = 0; > > /* Get Next */ >- OvsFlowDumpOutput dumpOutput; >- OvsFlowDumpInput dumpInput; >- NL_BUFFER nlBuf; >+ OvsFlowDumpOutput dumpOutput = { 0 }; >+ OvsFlowDumpInput dumpInput = { 0 }; >+ NL_BUFFER nlBuf = { 0 }; Unnecessary initialization. SV: Correct. The variables are initialized in the function's body. > > NlBufInit(&nlBuf, usrParamsCtx->outputBuffer, > usrParamsCtx->outputLength); @@ -826,6 +824,12 @@ >MapFlowKeyToNlKey(PNL_BUFFER nlBuf, > goto error_nested_start; > } > >+ if (!NlMsgPutTailU32(nlBuf, OVS_KEY_ATTR_RECIRC_ID, >+ flowKey->recircId)) { >+ rc = STATUS_UNSUCCESSFUL; >+ goto done; >+ } >+ > /* Ethernet header */ > RtlCopyMemory(&(ethKey.eth_src), flowKey->l2.dlSrc, ETH_ADDR_LEN); > RtlCopyMemory(&(ethKey.eth_dst), flowKey->l2.dlDst, ETH_ADDR_LEN); >@@ -1348,6 +1352,10 @@ _MapKeyAttrToFlowPut(PNL_ATTR *keyAttrs, { > _MapTunAttrToFlowPut(keyAttrs, tunnelAttrs, destKey); > >+ if (keyAttrs[OVS_KEY_ATTR_RECIRC_ID]) { >+ destKey->recircId = >NlAttrGetU32(keyAttrs[OVS_KEY_ATTR_RECIRC_ID]); >+ } >+ > /* ===== L2 headers ===== */ > destKey->l2.inPort = NlAttrGetU32(keyAttrs[OVS_KEY_ATTR_IN_PORT]); > >@@ -2072,6 +2080,9 @@ OvsLookupFlow(OVS_DATAPATH *datapath, > > if (!hashValid) { > *hash = OvsJhashBytes(start, size, 0); >+ if (key->recircId) { >+ *hash = OvsJhashWords((UINT32*)hash, 1, key->recircId); >+ } > } > > head = &datapath->flowTable[HASH_BUCKET(*hash)]; >@@ -2090,6 +2101,47 @@ OvsLookupFlow(OVS_DATAPATH *datapath, > return NULL; > } > >+/* >+ * >----------------------------------------------------------------------- >--- >-- >+ * OvsLookupFlowRecirc -- >+ * >+ * Find flow from flow table based on flow key and recircId, if >available. >+ * Caller should either hold portset handle or should >+ * have a flowRef in datapath or Acquired datapath. >+ * >+ * Results: >+ * Flow pointer if lookup successful. >+ * NULL if not exists. >+ * >----------------------------------------------------------------------- >--- >-- >+ */ >+OvsFlow * >+OvsLookupFlowRecirc(OVS_DATAPATH *datapath, >+ const OvsFlowKey *key, >+ UINT64 *hash) >+{ >+ OvsFlow* flow = NULL; >+ >+ if (!hash || !(*hash)) { >+ flow = OvsLookupFlow(datapath, key, hash, FALSE); >+ } else { >+ /* >+ * Pre and post recirculation flows usually have the same hash >+ * value. To avoid hash collisions, rehash the 'hash' with >+ * 'recircId'. >+ */ >+ if (key->recircId) { >+ *hash = OvsJhashWords((UINT32*)hash, 1, key->recircId); >+ } >+ >+ flow = OvsLookupFlow(datapath, key, hash, FALSE); Last argument to OvsLookupFlow() is ŒhashValid¹. If it is FALSE, hash gets recalculated. What is the point of passing in hash? SV: I will verify the hash calculation. >+ if (flow) { >+ flow->hash = *hash; Why do we keep updating the hash? I see that ŒrecircId¹ has been added to OvsFlowKey. Where are we updating the key->l2.keyLen to indicate that recircId should also be added to the hash during lookup? Also, do we need this extra variable Œhash¹? Looks like it¹s value is moot. >+ } >+ } >+ >+ return flow; >+} >+ > > /* > * >----------------------------------------------------------------------- >--- >-- >@@ -2219,7 +2271,7 @@ ReportFlowInfo(OvsFlow *flow, > if (getFlags & FLOW_GET_KEY) { > // always copy the tunnel key part > RtlCopyMemory(&info->key, &flow->key, >- flow->key.l2.keyLen + flow->key.l2.offset); >+ flow->key.l2.keyLen + flow->key.l2.offset); > } > > if (getFlags & FLOW_GET_STATS) { >@@ -2239,6 +2291,8 @@ ReportFlowInfo(OvsFlow *flow, > } > } > >+ info->key.recircId = flow->key.recircId; >+ > return status; > } > >diff --git a/datapath-windows/ovsext/Flow.h >b/datapath-windows/ovsext/Flow.h index 74b9dfb..78bf7cc 100644 >--- a/datapath-windows/ovsext/Flow.h >+++ b/datapath-windows/ovsext/Flow.h >@@ -54,8 +54,11 @@ NDIS_STATUS OvsAllocateFlowTable(OVS_DATAPATH >*datapath, NDIS_STATUS OvsExtractFlow(const NET_BUFFER_LIST *pkt, >UINT32 inPort, > OvsFlowKey *flow, POVS_PACKET_HDR_INFO layers, > OvsIPv4TunnelKey *tunKey); -OvsFlow >*OvsLookupFlow(OVS_DATAPATH *datapath, const OvsFlowKey *key, >+OvsFlow* OvsLookupFlow(OVS_DATAPATH *datapath, const OvsFlowKey *key, > UINT64 *hash, BOOLEAN hashValid); >+OvsFlow* OvsLookupFlowRecirc(OVS_DATAPATH *datapath, >+ const OvsFlowKey *key, >+ UINT64 *hash); > UINT64 OvsHashFlow(const OvsFlowKey *key); VOID OvsFlowUsed(OvsFlow >*flow, const NET_BUFFER_LIST *pkt, > const POVS_PACKET_HDR_INFO layers); diff --git >a/datapath-windows/ovsext/Netlink/Netlink.h >b/datapath-windows/ovsext/Netlink/Netlink.h >index 99665fb..8f6a5be 100644 >--- a/datapath-windows/ovsext/Netlink/Netlink.h >+++ b/datapath-windows/ovsext/Netlink/Netlink.h >@@ -173,6 +173,17 @@ static __inline NlAttrTotalSize(UINT32 payloadSize) > return NLA_ALIGN(NlAttrSize(payloadSize)); > } > >+/* >+ * >----------------------------------------------------------------------- >--- >- >+ * Returns true if the last attribute is reached. >+ * >----------------------------------------------------------------------- >--- >- >+ */ >+BOOLEAN >+static __inline NlAttrIsLast(const PNL_ATTR nla, int rem) { >+ return nla->nlaLen == rem; >+} >+ > /* Netlink attribute validation */ > BOOLEAN NlAttrValidate(const PNL_ATTR, const PNL_POLICY); > >diff --git a/datapath-windows/ovsext/PacketIO.c >b/datapath-windows/ovsext/PacketIO.c >index cfbae34..a0ddc3d 100644 >--- a/datapath-windows/ovsext/PacketIO.c >+++ b/datapath-windows/ovsext/PacketIO.c >@@ -20,6 +20,8 @@ > */ > > #include "precomp.h" >+ >+#include "Actions.h" > #include "Switch.h" > #include "Vport.h" > #include "NetProto.h" >@@ -234,14 +236,14 @@ OvsStartNBLIngress(POVS_SWITCH_CONTEXT >switchContext, > OvsInitCompletionList(&completionList, switchContext, >sendCompleteFlags); > > for (curNbl = netBufferLists; curNbl != NULL; curNbl = nextNbl) { >- POVS_VPORT_ENTRY vport; >- UINT32 portNo; >+ POVS_VPORT_ENTRY vport = NULL; >+ UINT32 portNo = 0; > OVS_DATAPATH *datapath = &switchContext->datapath; >- OVS_PACKET_HDR_INFO layers; >- OvsFlowKey key; >- UINT64 hash; >- PNET_BUFFER curNb; >- POVS_BUFFER_CONTEXT ctx; >+ OVS_PACKET_HDR_INFO layers = { 0 }; >+ OvsFlowKey key = { 0 }; >+ UINT64 hash = 0; >+ PNET_BUFFER curNb = NULL; >+ POVS_BUFFER_CONTEXT ctx = NULL; Unnecessary initialization. SV: The initialization of the above variables is mandatory to avoid reading garbage when OvsActionsExecute() is executed. > > nextNbl = curNbl->Next; > curNbl->Next = NULL; >diff --git a/datapath-windows/ovsext/PacketIO.h >b/datapath-windows/ovsext/PacketIO.h >index 7247869..a7c1f76 100644 >--- a/datapath-windows/ovsext/PacketIO.h >+++ b/datapath-windows/ovsext/PacketIO.h >@@ -48,14 +48,4 @@ VOID OvsSendNBLIngress(POVS_SWITCH_CONTEXT >switchContext, > PNET_BUFFER_LIST netBufferLists, > ULONG sendFlags); > >-NDIS_STATUS OvsActionsExecute(POVS_SWITCH_CONTEXT switchContext, >- OvsCompletionList *completionList, >- PNET_BUFFER_LIST curNbl, UINT32 srcVportNo, >- ULONG sendFlags, OvsFlowKey *key, UINT64 >*hash, >- OVS_PACKET_HDR_INFO *layers, >- const PNL_ATTR actions, int actionsLen); >- >-VOID OvsLookupFlowOutput(POVS_SWITCH_CONTEXT switchContext, >- VOID *compList, PNET_BUFFER_LIST curNbl); >- > #endif /* __PACKETIO_H_ */ >diff --git a/datapath-windows/ovsext/Recirc.c >b/datapath-windows/ovsext/Recirc.c >new file mode 100644 >index 0000000..56b6200 >--- /dev/null >+++ b/datapath-windows/ovsext/Recirc.c >@@ -0,0 +1,224 @@ >+/* >+ * Copyright (c) 2016 Cloudbase Solutions Srl >+ * >+ * Licensed under the Apache License, Version 2.0 (the "License"); >+ * you may not use this file except in compliance with the License. >+ * You may obtain a copy of the License at: >+ * >+ * >https://urldefense.proofpoint.com/v2/url?u=http-3A__www.apache.org_lice >nse >s_LICENSE-2D2.0&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs& >r=p >NHQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=OpMpvAAJDApIpx1rELDZz_PzL7 >S9h fkeHD2h-6_KTCg&s=S30nm7dlEorfpeMZh2t0GL8LoSf_KXweQSeVsbNOCtg&e= >+ * >+ * Unless required by applicable law or agreed to in writing, software >+ * distributed under the License is distributed on an "AS IS" BASIS, >+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or >implied. >+ * See the License for the specific language governing permissions and >+ * limitations under the License. >+ */ >+ >+#include "Recirc.h" >+#include "Flow.h" >+#include "Jhash.h" >+ >+static POVS_DEFERRED_ACTION_QUEUE ovsDeferredActionQueues = NULL; >+ >+BOOLEAN >+OvsDeferredActionsQueueAlloc() >+{ >+ ovsDeferredActionQueues = >+ OvsAllocateMemoryPerCpu(sizeof(OVS_DEFERRED_ACTION_QUEUE), >+ OVS_RECIRC_POOL_TAG); >+ if (!ovsDeferredActionQueues) { >+ return FALSE; >+ } >+ return TRUE; >+} >+ >+VOID >+OvsDeferredActionsQueueFree() >+{ >+ OvsFreeMemoryWithTag(ovsDeferredActionQueues, >+ OVS_RECIRC_POOL_TAG); >+ ovsDeferredActionQueues = NULL; >+} >+ >+POVS_DEFERRED_ACTION_QUEUE >+OvsDeferredActionsQueueGet() >+{ >+ POVS_DEFERRED_ACTION_QUEUE queue = NULL; >+ ULONG index = 0; >+ KIRQL oldIrql = KeGetCurrentIrql(); >+ >+ if (oldIrql < DISPATCH_LEVEL) { >+ KeRaiseIrqlToDpcLevel(); >+ } >+ >+ index = KeGetCurrentProcessorNumber(); >+ queue = &ovsDeferredActionQueues[index]; >+ >+ if (oldIrql < DISPATCH_LEVEL) { >+ KeLowerIrql(oldIrql); >+ } >+ >+ return queue; >+} >+ >+/* >+ * >----------------------------------------------------------------------- >--- >+ * OvsDeferredActionsQueueInit -- >+ * The function resets the queue to be ready for the next packet. >+ * >----------------------------------------------------------------------- >--- >+ */ >+static >+VOID >+OvsDeferredActionsQueueInit(POVS_DEFERRED_ACTION_QUEUE queue) { >+ queue->head = 0; >+ queue->tail = 0; >+} >+ >+/* >+ * >----------------------------------------------------------------------- >--- >+ * OvsDeferredActionsQueueIsEmpty -- >+ * The function verifies if the queue is empty. >+ * >----------------------------------------------------------------------- >--- >+ */ >+static >+BOOLEAN >+OvsDeferredActionsQueueIsEmpty(const POVS_DEFERRED_ACTION_QUEUE queue) >+{ >+ return (queue->head == queue->tail); } >+ >+/* >+ * >----------------------------------------------------------------------- >--- >+ * OvsDeferredActionsQueuePop -- >+ * The function pops the next queue element. >+ * >----------------------------------------------------------------------- >--- >+ */ >+static >+POVS_DEFERRED_ACTION >+OvsDeferredActionsQueuePop(POVS_DEFERRED_ACTION_QUEUE queue) { >+ POVS_DEFERRED_ACTION deferredAction = NULL; >+ KIRQL oldIrql = KeGetCurrentIrql(); >+ >+ if (oldIrql < DISPATCH_LEVEL) { >+ KeRaiseIrqlToDpcLevel(); >+ } >+ >+ if (OvsDeferredActionsQueueIsEmpty(queue)) { >+ /* Reset the queue for the next packet. */ >+ OvsDeferredActionsQueueInit(queue); >+ } else { >+ deferredAction = &queue->queue[queue->tail++]; >+ } >+ >+ if (oldIrql < DISPATCH_LEVEL) { >+ KeLowerIrql(oldIrql); >+ } >+ >+ return deferredAction; >+} >+ >+/* >+ * >----------------------------------------------------------------------- >--- >+ * OvsDeferredActionsQueuePush -- >+ * The function pushes the current element in the deferred actions >queue. >+ * >----------------------------------------------------------------------- >--- >+ */ >+static >+POVS_DEFERRED_ACTION >+OvsDeferredActionsQueuePush(POVS_DEFERRED_ACTION_QUEUE queue) { >+ POVS_DEFERRED_ACTION deferredAction = NULL; >+ KIRQL oldIrql = KeGetCurrentIrql(); >+ >+ if (oldIrql < DISPATCH_LEVEL) { >+ KeRaiseIrqlToDpcLevel(); >+ } >+ >+ if (queue->head < DEFERRED_ACTION_QUEUE_SIZE) { >+ deferredAction = &queue->queue[queue->head++]; >+ } >+ >+ if (oldIrql < DISPATCH_LEVEL) { >+ KeLowerIrql(oldIrql); >+ } >+ >+ return deferredAction; >+} >+ >+/* >+ * >----------------------------------------------------------------------- >--- >+ * OvsAddDeferredActions -- >+ * This function returns the new queue entry if the queue is not >already >+ * full. >+ * >+ * Note: >+ * The function is thread-safe. >+ * >----------------------------------------------------------------------- >--- >+ */ >+POVS_DEFERRED_ACTION >+OvsAddDeferredActions(PNET_BUFFER_LIST packet, >+ OvsFlowKey *key, >+ UINT64 hash, >+ const PNL_ATTR actions) { >+ POVS_DEFERRED_ACTION_QUEUE queue = OvsDeferredActionsQueueGet(); >+ POVS_DEFERRED_ACTION deferredAction = NULL; >+ >+ deferredAction = OvsDeferredActionsQueuePush(queue); >+ if (deferredAction) { >+ deferredAction->packet = packet; >+ deferredAction->actions = actions; >+ deferredAction->key = *key; >+ deferredAction->hash = hash; >+ } >+ >+ return deferredAction; >+} >+ >+/* >+ * >----------------------------------------------------------------------- >--- >+ * OvsProcessDeferredActions -- >+ * This function processes all deferred actions contained in the >queue. >+ * >+ * Note: >+ * The function is thread-safe. >+ * >----------------------------------------------------------------------- >--- >+ */ >+NDIS_STATUS >+OvsProcessDeferredActions(POVS_SWITCH_CONTEXT switchContext, >+ OvsCompletionList *completionList, >+ UINT32 portNo, >+ ULONG sendFlags, >+ OVS_PACKET_HDR_INFO *layers) { >+ NDIS_STATUS status = NDIS_STATUS_SUCCESS; >+ POVS_DEFERRED_ACTION_QUEUE queue = OvsDeferredActionsQueueGet(); >+ POVS_DEFERRED_ACTION deferredAction = NULL; >+ >+ /* Process all deferred actions. */ >+ while ((deferredAction = OvsDeferredActionsQueuePop(queue)) != >+ NULL) >{ >+ if (deferredAction->actions) { >+ status = OvsDoExecuteActions(switchContext, >+ completionList, >+ deferredAction->packet, >+ portNo, >+ sendFlags, >+ &deferredAction->key, >+ &deferredAction->hash, >+ layers, >+ deferredAction->actions, >+ >NlAttrGetSize(deferredAction->actions)); >+ } else { >+ status = OvsDoRecircFlowLookupOutput(switchContext, >+ completionList, >+ deferredAction->packet, >+ &deferredAction->key, >+ &deferredAction->hash, >+ portNo, >+ layers); >+ } >+ } >+ >+ return status; >+} >diff --git a/datapath-windows/ovsext/Recirc.h >b/datapath-windows/ovsext/Recirc.h >new file mode 100644 >index 0000000..8e6d125 >--- /dev/null >+++ b/datapath-windows/ovsext/Recirc.h >@@ -0,0 +1,90 @@ >+/* >+ * Copyright (c) 2016 Cloudbase Solutions Srl >+ * >+ * Licensed under the Apache License, Version 2.0 (the "License"); >+ * you may not use this file except in compliance with the License. >+ * You may obtain a copy of the License at: >+ * >+ * >https://urldefense.proofpoint.com/v2/url?u=http-3A__www.apache.org_lice >nse >s_LICENSE-2D2.0&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs& >r=p >NHQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=OpMpvAAJDApIpx1rELDZz_PzL7 >S9h fkeHD2h-6_KTCg&s=S30nm7dlEorfpeMZh2t0GL8LoSf_KXweQSeVsbNOCtg&e= >+ * >+ * Unless required by applicable law or agreed to in writing, software >+ * distributed under the License is distributed on an "AS IS" BASIS, >+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or >implied. >+ * See the License for the specific language governing permissions and >+ * limitations under the License. >+ */ >+ >+#ifndef __RECIRC_H_ >+#define __RECIRC_H_ 1 >+ >+#include "Actions.h" >+ >+#define DEFERRED_ACTION_QUEUE_SIZE 10 >+ >+typedef struct _OVS_DEFERRED_ACTION { >+ PNET_BUFFER_LIST packet; Nit: packet => nbl in alignment with the other data structures across the codebase. SV: Sure, I will rename it. >+ PNL_ATTR actions; >+ OvsFlowKey key; I don¹t get the idea of using a hash. Maybe we can discuss this during the IRC meeting. >+ UINT64 hash; >+} OVS_DEFERRED_ACTION, *POVS_DEFERRED_ACTION; >+ >+/* >+ * >----------------------------------------------------------------------- >--- >+ * '_OVS_DEFERRED_ACTION_QUEUE' structure is responsible for keeping >track of >+ * all deferred actions. The maximum number of deferred actions should >not >+ * exceed 'DEFERRED_ACTION_QUEUE_SIZE'. >+ * >----------------------------------------------------------------------- >--- >+ */ >+typedef struct _OVS_DEFERRED_ACTION_QUEUE { >+ UINT32 head; >+ UINT32 tail; >+ OVS_DEFERRED_ACTION queue[DEFERRED_ACTION_QUEUE_SIZE]; >+} OVS_DEFERRED_ACTION_QUEUE, *POVS_DEFERRED_ACTION_QUEUE; >+ >+/* >+ * >----------------------------------------------------------------------- >--- >+ * OvsProcessDeferredActions -- >+ * This function processes all deferred actions contained in the >queue >+ * corresponding to the current CPU. >+ * >----------------------------------------------------------------------- >--- >+ */ >+NDIS_STATUS >+OvsProcessDeferredActions(POVS_SWITCH_CONTEXT switchContext, >+ OvsCompletionList *completionList, >+ UINT32 portNo, >+ ULONG sendFlags, >+ OVS_PACKET_HDR_INFO *layers); >+ >+/* >+ * >----------------------------------------------------------------------- >--- >+ * OvsAddDeferredActions -- >+ * This function adds the deferred action to the current CPU queue >and >+ * returns the new queue entry if the queue is not already full. >+ * >----------------------------------------------------------------------- >--- >+ */ >+POVS_DEFERRED_ACTION >+OvsAddDeferredActions(PNET_BUFFER_LIST packet, >+ OvsFlowKey *key, >+ UINT64 hash, >+ const PNL_ATTR actions); >+ >+/* >+ * >----------------------------------------------------------------------- >--- >+ * OvsDeferredActionsQueueAlloc -- >+ * This function allocates an array of deferred action queues, one >for >+ * each CPU core. >+ * >----------------------------------------------------------------------- >--- >+ */ >+BOOLEAN >+OvsDeferredActionsQueueAlloc(); >+ >+/* >+ * >----------------------------------------------------------------------- >--- >+ * OvsDeferredActionsQueueFree -- >+ * This function frees the array of deferred action queues. >+ * >----------------------------------------------------------------------- >--- >+ */ >+VOID >+OvsDeferredActionsQueueFree(); >+ >+#endif /* __RECIRC_H_ */ >diff --git a/datapath-windows/ovsext/Tunnel.c >b/datapath-windows/ovsext/Tunnel.c >index eea4a84..e957aaf 100644 >--- a/datapath-windows/ovsext/Tunnel.c >+++ b/datapath-windows/ovsext/Tunnel.c >@@ -39,6 +39,7 @@ > #include "PacketIO.h" > #include "NetProto.h" > #include "Flow.h" >+#include "Actions.h" > > extern POVS_SWITCH_CONTEXT gOvsSwitchContext; > >@@ -258,13 +259,13 @@ OvsInjectPacketThroughActions(PNET_BUFFER_LIST pNbl, > sendCompleteFlags); > > { >- POVS_VPORT_ENTRY vport; >- UINT32 portNo; >- OVS_PACKET_HDR_INFO layers; >- OvsFlowKey key; >- UINT64 hash; >- PNET_BUFFER curNb; >- OvsFlow *flow; >+ POVS_VPORT_ENTRY vport = NULL; >+ UINT32 portNo = 0; >+ OVS_PACKET_HDR_INFO layers = { 0 }; >+ OvsFlowKey key = { 0 }; >+ UINT64 hash = 0; >+ PNET_BUFFER curNb = NULL; >+ OvsFlow *flow = NULL; Unnecessary initialization. SV: It is necessary. Please see previous explanation on this topic. > > fwdDetail = NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(pNbl); > >diff --git a/datapath-windows/ovsext/User.c >b/datapath-windows/ovsext/User.c index e97f2b2..cadffda 100644 >--- a/datapath-windows/ovsext/User.c >+++ b/datapath-windows/ovsext/User.c >@@ -22,6 +22,7 @@ > > #include "precomp.h" > >+#include "Actions.h" > #include "Datapath.h" > #include "Debug.h" > #include "Event.h" >@@ -388,14 +389,14 @@ NTSTATUS > OvsExecuteDpIoctl(OvsPacketExecute *execute) { > NTSTATUS status = STATUS_SUCCESS; >- NTSTATUS ndisStatus; >+ NTSTATUS ndisStatus = STATUS_SUCCESS; > LOCK_STATE_EX lockState; >- PNET_BUFFER_LIST pNbl; >- PNL_ATTR actions; >+ PNET_BUFFER_LIST pNbl = NULL; >+ PNL_ATTR actions = NULL; > PNDIS_SWITCH_FORWARDING_DETAIL_NET_BUFFER_LIST_INFO fwdDetail; >- OvsFlowKey key; >- OVS_PACKET_HDR_INFO layers; >- POVS_VPORT_ENTRY vport; >+ OvsFlowKey key = { 0 }; >+ OVS_PACKET_HDR_INFO layers = { 0 }; >+ POVS_VPORT_ENTRY vport = NULL; Unnecessary initialization. SV: It is necessary. Please see previous explanation on this topic. > > if (execute->packetLen == 0) { > status = STATUS_INVALID_PARAMETER; diff --git >a/datapath-windows/ovsext/Util.h b/datapath-windows/ovsext/Util.h index >b2ec798..038754d 100644 >--- a/datapath-windows/ovsext/Util.h >+++ b/datapath-windows/ovsext/Util.h >@@ -36,6 +36,7 @@ > #define OVS_STT_POOL_TAG 'RSVO' > #define OVS_GRE_POOL_TAG 'GSVO' > #define OVS_TUNFLT_POOL_TAG 'WSVO' >+#define OVS_RECIRC_POOL_TAG 'CSVO' > > VOID *OvsAllocateMemory(size_t size); > VOID *OvsAllocateMemoryWithTag(size_t size, ULONG tag); diff --git >a/datapath-windows/ovsext/ovsext.vcxproj >b/datapath-windows/ovsext/ovsext.vcxproj >index e3aea97..af718f7 100644 >--- a/datapath-windows/ovsext/ovsext.vcxproj >+++ b/datapath-windows/ovsext/ovsext.vcxproj >@@ -71,6 +71,7 @@ > </ImportGroup> > <ItemGroup Label="WrappedTaskItems"> > <ClInclude Include="..\include\OvsDpInterfaceExt.h" /> >+ <ClInclude Include="Actions.h" /> > <ClInclude Include="Atomic.h" /> > <ClInclude Include="BufferMgmt.h" /> > <ClInclude Include="Datapath.h" /> @@ -93,6 +94,7 @@ > <ClInclude Include="PacketIO.h" /> > <ClInclude Include="PacketParser.h" /> > <ClInclude Include="precomp.h" /> >+ <ClInclude Include="Recirc.h" /> > <ClInclude Include="resource.h" /> > <ClInclude Include="Stt.h" /> > <ClInclude Include="Switch.h" /> >@@ -193,6 +195,7 @@ > <PreCompiledHeader>Create</PreCompiledHeader> > ><PreCompiledHeaderOutputFile>$(IntDir)\precomp.h.pch</PreCompiledHeader >Out >putFile> > </ClCompile> >+ <ClCompile Include="Recirc.c" /> > <ClCompile Include="Stt.c" /> > <ClCompile Include="Switch.c" /> > <ClCompile Include="Tunnel.c" /> >-- >1.9.0.msysgit.0 > >_______________________________________________ >dev mailing list >dev@openvswitch.org >https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mai >lma >n_listinfo_dev&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r >=pN >HQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=OpMpvAAJDApIpx1rELDZz_PzL7S >9hf keHD2h-6_KTCg&s=B0JRbghGT_5kotOu2XQ_yegCP9Fj-DzzcfQ0dmmqKtU&e= _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
