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
