Thanks a lot for all the hard work Anand!
The patches are in good state overall with some small nits 😊. In the comments over the patches I said I will add an incremental for them. I attached a diff to hopefully get you going. Disregarding the changes to ` OvsApplySWChecksumOnNB` which I will send on a patch tomorrow, I will copy the rest of things over here and go over them and added [AS] in front and bold it: diff --git a/datapath-windows/ovsext/Actions.c b/datapath-windows/ovsext/Actions.c index a68679c..42f0459 100644 --- a/datapath-windows/ovsext/Actions.c +++ b/datapath-windows/ovsext/Actions.c @@ -676,6 +676,32 @@ OvsTunnelPortTx(OvsForwardingContext *ovsFwdCtx) } OVS_FWD_INFO switchFwdInfo = { 0 }; + POVS_BUFFER_CONTEXT ctx = (POVS_BUFFER_CONTEXT)NET_BUFFER_LIST_CONTEXT_DATA_START(ovsFwdCtx->curNbl); + if (ctx->mru != 0) { + /* Fragment nbl based on mru. If it returns NULL then the original + * reassembled NBL is sent out to the VIF which will be dropped if + * the packet size is more than VIF MTU. + */ + POVS_VPORT_ENTRY tx = ovsFwdCtx->tunnelTxNic; + PNET_BUFFER_LIST fragNbl = OvsFragmentNBL(ovsFwdCtx->switchContext, + ovsFwdCtx->curNbl, + &(ovsFwdCtx->layers), + ctx->mru, 0, TRUE); + if (fragNbl != NULL) { + OvsCompleteNBLForwardingCtx(ovsFwdCtx, + L"Dropped since fragmenting NBL"); + status = OvsInitForwardingCtx(ovsFwdCtx, + ovsFwdCtx->switchContext, + fragNbl, + ovsFwdCtx->srcVportNo, + ovsFwdCtx->sendFlags, + NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(fragNbl), + ovsFwdCtx->completionList, + &ovsFwdCtx->layers, FALSE); + ovsFwdCtx->tunnelTxNic = tx; + } + } + [AS] When outputting a packet over a tunnel we need to make sure that the packet was fragmented prior of it being sent out via a type of tunnel. Otherwise the inner packet packet + headroom will be over the MTU of the port on which it will be outputted. /* Apply the encapsulation. The encapsulation will not consume the NBL. */ switch(ovsFwdCtx->tunnelTxNic->ovsType) { case OVS_VPORT_TYPE_GRE: @@ -890,14 +916,17 @@ OvsOutputForwardingCtx(OvsForwardingContext *ovsFwdCtx) */ if (ovsFwdCtx->tunnelTxNic != NULL || ovsFwdCtx->tunnelRxNic != NULL) { nb = NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx->curNbl); + POVS_BUFFER_CONTEXT srcCtx = (POVS_BUFFER_CONTEXT)NET_BUFFER_LIST_CONTEXT_DATA_START(ovsFwdCtx->curNbl); newNbl = OvsPartialCopyNBL(ovsFwdCtx->switchContext, ovsFwdCtx->curNbl, 0, 0, TRUE /*copy NBL info*/); + POVS_BUFFER_CONTEXT dstCtx = (POVS_BUFFER_CONTEXT)NET_BUFFER_LIST_CONTEXT_DATA_START(newNbl); if (newNbl == NULL) { status = NDIS_STATUS_RESOURCES; ovsActionStats.noCopiedNbl++; dropReason = L"Dropped due to failure to create NBL copy."; goto dropit; } + dstCtx->mru = srcCtx->mru; [AS] Just to copy over the mru after the PartialCopy } /* It does not seem like we'll get here unless 'portsToUpdate' > 0. */ @@ -2058,11 +2087,21 @@ OvsDoExecuteActions(POVS_SWITCH_CONTEXT switchContext, goto dropit; } } - + PNET_BUFFER_LIST bla = ovsFwdCtx.curNbl; status = OvsExecuteConntrackAction(switchContext, &ovsFwdCtx.curNbl, ovsFwdCtx.completionList, ovsFwdCtx.fwdDetail->SourcePortId, layers, key, (const PNL_ATTR)a); + if (bla != ovsFwdCtx.curNbl) { + OvsInitForwardingCtx(&ovsFwdCtx, + ovsFwdCtx.switchContext, + ovsFwdCtx.curNbl, + ovsFwdCtx.srcVportNo, + ovsFwdCtx.sendFlags, + NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(ovsFwdCtx.curNbl), + ovsFwdCtx.completionList, + &ovsFwdCtx.layers, FALSE); + } [AS] This is needed because in ` OvsIpv4Reassemble` you complete the original nbl and create a new one and assign it over to the curNbl of the ovsFwdCtx. A side effect of completing the nbl will be that it will remove the ` destinationPorts`. This means that if you are not the last action and someone will add a port after it, ` OvsAddPorts` will be called and it will BSOD when: ` fwdPort = NDIS_SWITCH_PORT_DESTINATION_AT_ARRAY_INDEX(ovsFwdCtx->destinationPorts, ovsFwdCtx->destPortsSizeOut);` that is why I suggested passing over the fwdCtx and reiniting the fwdCtx after completing the original packet. Using OvsInitForwardingCtx will add the destination ports of the new nbl. if (status != NDIS_STATUS_SUCCESS) { /* Pending NBLs are consumed by Defragmentation. */ if (status != NDIS_STATUS_PENDING) { diff --git a/datapath-windows/ovsext/BufferMgmt.c b/datapath-windows/ovsext/BufferMgmt.c index 0011c10..c5f88cb 100644 --- a/datapath-windows/ovsext/BufferMgmt.c +++ b/datapath-windows/ovsext/BufferMgmt.c @@ -1411,6 +1411,7 @@ OvsFragmentNBL(PVOID ovsContext, dstCtx->refCount = 1; dstCtx->magic = OVS_CTX_MAGIC; dstCtx->dataOffsetDelta = hdrSize + headRoom; + dstCtx->mru = 0; [AS] I added this for consistency because I did not see the mru being set in this case. InterlockedIncrement((LONG volatile *)&srcCtx->refCount); #ifdef DBG > -----Original Message----- > From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev- > boun...@openvswitch.org] On Behalf Of Anand Kumar > Sent: Friday, March 24, 2017 10:51 PM > To: d...@openvswitch.org > Subject: [ovs-dev] [PATCH v6 0/5] datapath-windows: Add support for Ipv4 > fragments > > Add support for maintaining and tracking IPv4 fragments. > This patch series adds a new file IpFragment.c and IpFragment.h which > includes Ipv4 fragment related API's. > > --- > v5->v6: Rebase > v4->v5: > - Modified Patch 3 to retain MRU in _OVS_BUFFER_CONTEXT instead of > using it in ovsForwardingContext with minor changes in rest of the > patches. > v3->v4: > - Rebase > - Acquire read lock for read operations. > v2->v3: > - using spinlock instead of RW lock. > - updated log messages, summary, fixed alignment issues. > v1->v2: > - Patch 4 updated to make it compile for release mode. > --- > Anand Kumar (5): > datapath-windows: Added a new file to support Ipv4 fragments. > datapath-windows: Added Ipv4 fragments support in Conntrack > datapath-windows: Retain MRU value in the _OVS_BUFFER_CONTEXT. > datapath-windows: Updated OvsTcpSegmentNBL to handle IP fragments. > datapath-windows: Fragment NBL based on MRU size > > datapath-windows/automake.mk | 2 + > datapath-windows/ovsext/Actions.c | 40 ++- > datapath-windows/ovsext/BufferMgmt.c | 195 +++++++++---- > datapath-windows/ovsext/BufferMgmt.h | 11 +- > datapath-windows/ovsext/Conntrack.c | 35 ++- > datapath-windows/ovsext/Conntrack.h | 6 +- > datapath-windows/ovsext/Debug.h | 3 +- > datapath-windows/ovsext/DpInternal.h | 2 +- > datapath-windows/ovsext/Geneve.c | 2 +- > datapath-windows/ovsext/Gre.c | 2 +- > datapath-windows/ovsext/IpFragment.c | 502 > +++++++++++++++++++++++++++++++++ > datapath-windows/ovsext/IpFragment.h | 73 +++++ > datapath-windows/ovsext/Stt.c | 2 +- > datapath-windows/ovsext/Switch.c | 9 + > datapath-windows/ovsext/User.c | 22 +- > datapath-windows/ovsext/Vxlan.c | 2 +- > datapath-windows/ovsext/ovsext.vcxproj | 2 + > 17 files changed, 831 insertions(+), 79 deletions(-) create mode 100644 > datapath-windows/ovsext/IpFragment.c > create mode 100644 datapath-windows/ovsext/IpFragment.h > > -- > 2.9.3.windows.1 > > _______________________________________________ > dev mailing list > d...@openvswitch.org<mailto:d...@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev