[ovs-dev] [PATCH v2 5/5] datapath-windows: Fragment NBL based on MRU size

[Sairam Venugopal]

Hi Anand,

Thanks for the patch. Please find my comments inline. 

Thanks,
Sairam




On 1/12/17, 1:13 PM, "ovs-dev-bounces at openvswitch.org on behalf of Anand Kumar" <ovs-dev-bounces at openvswitch.org on behalf of kumaranand at vmware.com> wrote:

>MRU value is updated only for the Ipv4 fragments. If it is non zero,
>then fragment the NBL based on MRU value and send out the new NBL to
>the vnic.
>---
> datapath-windows/ovsext/Actions.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
>diff --git a/datapath-windows/ovsext/Actions.c b/datapath-windows/ovsext/Actions.c
>index 9cc79dc..3156fb6 100644
>--- a/datapath-windows/ovsext/Actions.c
>+++ b/datapath-windows/ovsext/Actions.c
>@@ -34,6 +34,7 @@
> #include "Vport.h"
> #include "Vxlan.h"
> #include "Geneve.h"
>+#include "IpFragment.h"
> 
> #ifdef OVS_DBG_MOD
> #undef OVS_DBG_MOD
>@@ -873,6 +874,7 @@ OvsOutputForwardingCtx(OvsForwardingContext *ovsFwdCtx)
>     NDIS_STATUS status = STATUS_SUCCESS;
>     POVS_SWITCH_CONTEXT switchContext = ovsFwdCtx->switchContext;
>     PCWSTR dropReason;
>+    PNET_BUFFER_LIST fragNbl = NULL;
> 
>     /*
>      * Handle the case where the some of the destination ports are tunneled
>@@ -918,6 +920,30 @@ OvsOutputForwardingCtx(OvsForwardingContext *ovsFwdCtx)
>             goto dropit;
>         }
> 
>+        if (ovsFwdCtx->mru != 0) {
>+            /* fragment nbl based on mru */
>+            fragNbl = OvsFragmentNBL(ovsFwdCtx->switchContext,ovsFwdCtx->curNbl, &(ovsFwdCtx->layers),
>+                                     ovsFwdCtx->mru, 0, TRUE);


Sai: What happens when fragNbl == NULL?

>+            if (fragNbl != NULL) {
>+                OvsCompleteNBLForwardingCtx(ovsFwdCtx,
>+                                            L"Dropped, failed to create fragments");
Sai: Isn’t the error message incorrect? You just created fragments. You are completing the original Nbl because you created the fragments.
Can you add some comments here about what is happening? Also, can’t you set the sendFlags in the OvsInitForwardingCtx?
>+                ovsFwdCtx->sendFlags |= NDIS_SEND_FLAGS_SWITCH_DESTINATION_GROUP;
>+                status = OvsInitForwardingCtx(ovsFwdCtx,
>+                                              ovsFwdCtx->switchContext,
>+                                              fragNbl,
>+                                              ovsFwdCtx->srcVportNo,
>+                                              ovsFwdCtx->sendFlags,
>+                                              NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(fragNbl),
>+                                              ovsFwdCtx->mru,
>+                                              ovsFwdCtx->completionList,
>+                                              &ovsFwdCtx->layers, FALSE);

Sai: OvsInitForwardingCtx currently always returns NDIS_STATUS_SUCCESS. Either get rid of this check or update the dropReason.
>+                if (status != NDIS_STATUS_SUCCESS) {
>+                    dropReason = L"Dropped due to resouces";
>+                    goto dropit;
>+                }
>+            }
>+        }
>+
>         OvsSendNBLIngress(ovsFwdCtx->switchContext, ovsFwdCtx->curNbl,
>                           ovsFwdCtx->sendFlags);
>         /* End this pipeline by resetting the corresponding context. */
>-- 
>2.9.3.windows.1
>
>_______________________________________________
>dev mailing list
>dev at openvswitch.org
>https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=Z6vowHUOjP5ysP_g372c49Nqc1vEKqHKNBkR5Q5Z7uo&m=x7HRyEFKPCD18rrJ_FpVr0GRAua8d7FJNeAE7hKCX9M&s=i_NgegHuIAZaHV4cT8RL1r1Hz-mdmFMGDNj9qO6zxd0&e=