diff mbox series

[ovs-dev] datapath-windows: Update OvsIPv4TunnelKey flags in geneve decap.

Message ID 20171018003159.4056-1-kumaranand@vmware.com
State Not Applicable
Headers show
Series [ovs-dev] datapath-windows: Update OvsIPv4TunnelKey flags in geneve decap. | expand

Commit Message

Anand Kumar Oct. 18, 2017, 12:31 a.m. UTC 
Set the geneve flags OVS_TNL_F_GENEVE_OPT and OVS_TNL_F_CRT_OPT
in OvsDecapGeneve, so that windows behavior is similiar to linux
https://github.com/openvswitch/ovs/blob/master/datapath/linux/compat/geneve.c#L242

Signed-off-by: Anand Kumar <kumaranand@vmware.com>
---
 datapath-windows/ovsext/Geneve.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Nithin Raju Oct. 18, 2017, 10:43 a.m. UTC | #1 
> On Oct 17, 2017, at 5:31 PM, Anand Kumar <kumaranand@vmware.com> wrote:
> 
> Set the geneve flags OVS_TNL_F_GENEVE_OPT and OVS_TNL_F_CRT_OPT
> in OvsDecapGeneve, so that windows behavior is similiar to linux
> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvswitch_ovs_blob_master_datapath_linux_compat_geneve.c-23L242&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=-xl6DPE_Y3uQD-mpZD7osBo2iL4s3jwdmSjTlGgjlsQ&m=cM6BjMekLlE1U4o08yWj5yBcHAbEGejEhoi3U90gn18&s=2k5d-dCk_oECkPfP55vafQ9Yf8NbapRCDkwD7w-vQXk&e=
> 
> Signed-off-by: Anand Kumar <kumaranand@vmware.com>

Thanks for the fix.

Acked-by: Nithin Raju <nithin@vmware.com>
Shashank Ram Oct. 18, 2017, 4:07 p.m. UTC | #2 
On 10/17/17, 5:31 PM, "ovs-dev-bounces@openvswitch.org on behalf of Anand Kumar" <ovs-dev-bounces@openvswitch.org on behalf of kumaranand@vmware.com> wrote:

    Set the geneve flags OVS_TNL_F_GENEVE_OPT and OVS_TNL_F_CRT_OPT
    in OvsDecapGeneve, so that windows behavior is similiar to linux
    https://github.com/openvswitch/ovs/blob/master/datapath/linux/compat/geneve.c#L242
    
    Signed-off-by: Anand Kumar <kumaranand@vmware.com>

    ---
     datapath-windows/ovsext/Geneve.c | 8 ++++----
     1 file changed, 4 insertions(+), 4 deletions(-)
    
    diff --git a/datapath-windows/ovsext/Geneve.c b/datapath-windows/ovsext/Geneve.c
    index 43374e2..77244b1 100644
    --- a/datapath-windows/ovsext/Geneve.c
    +++ b/datapath-windows/ovsext/Geneve.c
    @@ -324,10 +324,10 @@ NDIS_STATUS OvsDecapGeneve(POVS_SWITCH_CONTEXT switchContext,
             status = STATUS_NDIS_INVALID_PACKET;
             goto dropNbl;
         }
    -    tunKey->flags = OVS_TNL_F_KEY;
    -    if (geneveHdr->oam) {
    -        tunKey->flags |= OVS_TNL_F_OAM;
    -    }
    +    /* Update tunnelKey flags. */
    +    tunKey->flags = OVS_TNL_F_KEY | OVS_TNL_F_GENEVE_OPT |
    +                    (geneveHdr->oam ? OVS_TNL_F_OAM : 0) |
    +                    (geneveHdr->critical ? OVS_TNL_F_CRT_OPT : 0);

Shouldn’t OVS_TNL_F_GENEVE_OPT be set only when the options header is present?

         tunKey->tunnelId = GENEVE_VNI_TO_TUNNELID(geneveHdr->vni);
         tunKey->tunOptLen = (uint8)geneveHdr->optLen * 4;
         if (tunKey->tunOptLen > TUN_OPT_MAX_LEN ||
    -- 
    
    _______________________________________________
Anand Kumar Oct. 18, 2017, 8:06 p.m. UTC | #3 
Hi Shashank,

I have set the tunnel key flags OVS_TNL_F_GENEVE_OPT by default to match the linux behavior @https://github.com/openvswitch/ovs/blob/master/datapath/linux/compat/geneve.c#L242

Thanks,
Anand Kumar

On 10/18/17, 9:07 AM, "Shashank Ram" <rams@vmware.com> wrote:

    
    
    On 10/17/17, 5:31 PM, "ovs-dev-bounces@openvswitch.org on behalf of Anand Kumar" <ovs-dev-bounces@openvswitch.org on behalf of kumaranand@vmware.com> wrote:
    
        Set the geneve flags OVS_TNL_F_GENEVE_OPT and OVS_TNL_F_CRT_OPT
        in OvsDecapGeneve, so that windows behavior is similiar to linux
        https://github.com/openvswitch/ovs/blob/master/datapath/linux/compat/geneve.c#L242
        
        Signed-off-by: Anand Kumar <kumaranand@vmware.com>

        ---
         datapath-windows/ovsext/Geneve.c | 8 ++++----
         1 file changed, 4 insertions(+), 4 deletions(-)
        
        diff --git a/datapath-windows/ovsext/Geneve.c b/datapath-windows/ovsext/Geneve.c
        index 43374e2..77244b1 100644
        --- a/datapath-windows/ovsext/Geneve.c
        +++ b/datapath-windows/ovsext/Geneve.c
        @@ -324,10 +324,10 @@ NDIS_STATUS OvsDecapGeneve(POVS_SWITCH_CONTEXT switchContext,
                 status = STATUS_NDIS_INVALID_PACKET;
                 goto dropNbl;
             }
        -    tunKey->flags = OVS_TNL_F_KEY;
        -    if (geneveHdr->oam) {
        -        tunKey->flags |= OVS_TNL_F_OAM;
        -    }
        +    /* Update tunnelKey flags. */
        +    tunKey->flags = OVS_TNL_F_KEY | OVS_TNL_F_GENEVE_OPT |
        +                    (geneveHdr->oam ? OVS_TNL_F_OAM : 0) |
        +                    (geneveHdr->critical ? OVS_TNL_F_CRT_OPT : 0);
    
    Shouldn’t OVS_TNL_F_GENEVE_OPT be set only when the options header is present?
    
             tunKey->tunnelId = GENEVE_VNI_TO_TUNNELID(geneveHdr->vni);
             tunKey->tunOptLen = (uint8)geneveHdr->optLen * 4;
             if (tunKey->tunOptLen > TUN_OPT_MAX_LEN ||
        -- 
        
        _______________________________________________
Sairam Venugopal Oct. 18, 2017, 10:45 p.m. UTC | #4 
Thanks for fixing this. Should we check for geneveHdr->opts before setting OVS_TNL_F_GENEVE_OPT?

I know the current code maintains consistency with Linux. But we need to understand if this is intended.

Acked-by: Sairam Venugopal <vsairam@vmware.com>





On 10/17/17, 5:31 PM, "ovs-dev-bounces@openvswitch.org on behalf of Anand Kumar" <ovs-dev-bounces@openvswitch.org on behalf of kumaranand@vmware.com> wrote:

>Set the geneve flags OVS_TNL_F_GENEVE_OPT and OVS_TNL_F_CRT_OPT
>in OvsDecapGeneve, so that windows behavior is similiar to linux
>https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvswitch_ovs_blob_master_datapath_linux_compat_geneve.c-23L242&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=Z6vowHUOjP5ysP_g372c49Nqc1vEKqHKNBkR5Q5Z7uo&m=ntDbsrs3dZtqbGfAIQnQA7qzLK8SlLouMDV36u0JsGQ&s=TtHsQ-7YSUu8XJHpn4iiQkhaothJamn3dU7_FKuRptE&e=
>
>Signed-off-by: Anand Kumar <kumaranand@vmware.com>
>---
> datapath-windows/ovsext/Geneve.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/datapath-windows/ovsext/Geneve.c b/datapath-windows/ovsext/Geneve.c
>index 43374e2..77244b1 100644
>--- a/datapath-windows/ovsext/Geneve.c
>+++ b/datapath-windows/ovsext/Geneve.c
>@@ -324,10 +324,10 @@ NDIS_STATUS OvsDecapGeneve(POVS_SWITCH_CONTEXT switchContext,
>         status = STATUS_NDIS_INVALID_PACKET;
>         goto dropNbl;
>     }
>-    tunKey->flags = OVS_TNL_F_KEY;
>-    if (geneveHdr->oam) {
>-        tunKey->flags |= OVS_TNL_F_OAM;
>-    }
>+    /* Update tunnelKey flags. */
>+    tunKey->flags = OVS_TNL_F_KEY | OVS_TNL_F_GENEVE_OPT |
>+                    (geneveHdr->oam ? OVS_TNL_F_OAM : 0) |
>+                    (geneveHdr->critical ? OVS_TNL_F_CRT_OPT : 0);
>     tunKey->tunnelId = GENEVE_VNI_TO_TUNNELID(geneveHdr->vni);
>     tunKey->tunOptLen = (uint8)geneveHdr->optLen * 4;
>     if (tunKey->tunOptLen > TUN_OPT_MAX_LEN ||
>-- 
>2.9.3.windows.1
>
>_______________________________________________
>dev mailing list
>dev@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=ntDbsrs3dZtqbGfAIQnQA7qzLK8SlLouMDV36u0JsGQ&s=weAfeEKoncNrwUrh3c5va-Efy3yApbOLqPd2LZleZkM&e=
Sairam Venugopal Oct. 19, 2017, 3:13 p.m. UTC | #5 
Anand,

Looks like Linux does make a check to see if there are any metadata present before setting these flags - https://github.com/openvswitch/ovs/blob/master/datapath/linux/compat/geneve.c#L239

Can you verify this and update the patch? (un-ack in meantime??)

Thanks,
Sairam




On 10/18/17, 3:45 PM, "ovs-dev-bounces@openvswitch.org on behalf of Sairam Venugopal" <ovs-dev-bounces@openvswitch.org on behalf of vsairam@vmware.com> wrote:

>Thanks for fixing this. Should we check for geneveHdr->opts before setting OVS_TNL_F_GENEVE_OPT?
>
>I know the current code maintains consistency with Linux. But we need to understand if this is intended.
>
>Acked-by: Sairam Venugopal <vsairam@vmware.com>
>
>
>
>
>
>On 10/17/17, 5:31 PM, "ovs-dev-bounces@openvswitch.org on behalf of Anand Kumar" <ovs-dev-bounces@openvswitch.org on behalf of kumaranand@vmware.com> wrote:
>
>>Set the geneve flags OVS_TNL_F_GENEVE_OPT and OVS_TNL_F_CRT_OPT
>>in OvsDecapGeneve, so that windows behavior is similiar to linux
>>https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvswitch_ovs_blob_master_datapath_linux_compat_geneve.c-23L242&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=Z6vowHUOjP5ysP_g372c49Nqc1vEKqHKNBkR5Q5Z7uo&m=ntDbsrs3dZtqbGfAIQnQA7qzLK8SlLouMDV36u0JsGQ&s=TtHsQ-7YSUu8XJHpn4iiQkhaothJamn3dU7_FKuRptE&e=
>>
>>Signed-off-by: Anand Kumar <kumaranand@vmware.com>
>>---
>> datapath-windows/ovsext/Geneve.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>>diff --git a/datapath-windows/ovsext/Geneve.c b/datapath-windows/ovsext/Geneve.c
>>index 43374e2..77244b1 100644
>>--- a/datapath-windows/ovsext/Geneve.c
>>+++ b/datapath-windows/ovsext/Geneve.c
>>@@ -324,10 +324,10 @@ NDIS_STATUS OvsDecapGeneve(POVS_SWITCH_CONTEXT switchContext,
>>         status = STATUS_NDIS_INVALID_PACKET;
>>         goto dropNbl;
>>     }
>>-    tunKey->flags = OVS_TNL_F_KEY;
>>-    if (geneveHdr->oam) {
>>-        tunKey->flags |= OVS_TNL_F_OAM;
>>-    }
>>+    /* Update tunnelKey flags. */
>>+    tunKey->flags = OVS_TNL_F_KEY | OVS_TNL_F_GENEVE_OPT |
>>+                    (geneveHdr->oam ? OVS_TNL_F_OAM : 0) |
>>+                    (geneveHdr->critical ? OVS_TNL_F_CRT_OPT : 0);
>>     tunKey->tunnelId = GENEVE_VNI_TO_TUNNELID(geneveHdr->vni);
>>     tunKey->tunOptLen = (uint8)geneveHdr->optLen * 4;
>>     if (tunKey->tunOptLen > TUN_OPT_MAX_LEN ||
>>-- 
>>2.9.3.windows.1
>>
>>_______________________________________________
>>dev mailing list
>>dev@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=ntDbsrs3dZtqbGfAIQnQA7qzLK8SlLouMDV36u0JsGQ&s=weAfeEKoncNrwUrh3c5va-Efy3yApbOLqPd2LZleZkM&e=
>_______________________________________________
>dev mailing list
>dev@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=eMSUuuWxH9h1UeKSVIcTwgE8ktXUUIht3ybSd8WphY8&s=Y6LNJpoXU2BdIsXqef34Gk0al6E4EaHcbISPLhClhVc&e=
Anand Kumar Oct. 19, 2017, 5:29 p.m. UTC | #6 
Hi Sairam,

Sure, I will add the check to see if there are any geneve tunnel options and set OVS_TNL_F_GENEVE_OPT accordingly and send out v2 version of the patch.

Thanks,
Anand Kumar

On 10/19/17, 8:13 AM, "Sairam Venugopal" <vsairam@vmware.com> wrote:

    Anand,
    
    Looks like Linux does make a check to see if there are any metadata present before setting these flags - https://github.com/openvswitch/ovs/blob/master/datapath/linux/compat/geneve.c#L239
    
    Can you verify this and update the patch? (un-ack in meantime??)
    
    Thanks,
    Sairam
    
    
    
    
    On 10/18/17, 3:45 PM, "ovs-dev-bounces@openvswitch.org on behalf of Sairam Venugopal" <ovs-dev-bounces@openvswitch.org on behalf of vsairam@vmware.com> wrote:
    
    >Thanks for fixing this. Should we check for geneveHdr->opts before setting OVS_TNL_F_GENEVE_OPT?
    >
    >I know the current code maintains consistency with Linux. But we need to understand if this is intended.
    >
    >Acked-by: Sairam Venugopal <vsairam@vmware.com>
    >
    >
    >
    >
    >
    >On 10/17/17, 5:31 PM, "ovs-dev-bounces@openvswitch.org on behalf of Anand Kumar" <ovs-dev-bounces@openvswitch.org on behalf of kumaranand@vmware.com> wrote:
    >
    >>Set the geneve flags OVS_TNL_F_GENEVE_OPT and OVS_TNL_F_CRT_OPT
    >>in OvsDecapGeneve, so that windows behavior is similiar to linux
    >>https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvswitch_ovs_blob_master_datapath_linux_compat_geneve.c-23L242&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=Z6vowHUOjP5ysP_g372c49Nqc1vEKqHKNBkR5Q5Z7uo&m=ntDbsrs3dZtqbGfAIQnQA7qzLK8SlLouMDV36u0JsGQ&s=TtHsQ-7YSUu8XJHpn4iiQkhaothJamn3dU7_FKuRptE&e=
    >>
    >>Signed-off-by: Anand Kumar <kumaranand@vmware.com>
    >>---
    >> datapath-windows/ovsext/Geneve.c | 8 ++++----
    >> 1 file changed, 4 insertions(+), 4 deletions(-)
    >>
    >>diff --git a/datapath-windows/ovsext/Geneve.c b/datapath-windows/ovsext/Geneve.c
    >>index 43374e2..77244b1 100644
    >>--- a/datapath-windows/ovsext/Geneve.c
    >>+++ b/datapath-windows/ovsext/Geneve.c
    >>@@ -324,10 +324,10 @@ NDIS_STATUS OvsDecapGeneve(POVS_SWITCH_CONTEXT switchContext,
    >>         status = STATUS_NDIS_INVALID_PACKET;
    >>         goto dropNbl;
    >>     }
    >>-    tunKey->flags = OVS_TNL_F_KEY;
    >>-    if (geneveHdr->oam) {
    >>-        tunKey->flags |= OVS_TNL_F_OAM;
    >>-    }
    >>+    /* Update tunnelKey flags. */
    >>+    tunKey->flags = OVS_TNL_F_KEY | OVS_TNL_F_GENEVE_OPT |
    >>+                    (geneveHdr->oam ? OVS_TNL_F_OAM : 0) |
    >>+                    (geneveHdr->critical ? OVS_TNL_F_CRT_OPT : 0);
    >>     tunKey->tunnelId = GENEVE_VNI_TO_TUNNELID(geneveHdr->vni);
    >>     tunKey->tunOptLen = (uint8)geneveHdr->optLen * 4;
    >>     if (tunKey->tunOptLen > TUN_OPT_MAX_LEN ||
    >>-- 
    >>2.9.3.windows.1
    >>
    >>_______________________________________________
    >>dev mailing list
    >>dev@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=ntDbsrs3dZtqbGfAIQnQA7qzLK8SlLouMDV36u0JsGQ&s=weAfeEKoncNrwUrh3c5va-Efy3yApbOLqPd2LZleZkM&e=
    >_______________________________________________
    >dev mailing list
    >dev@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=eMSUuuWxH9h1UeKSVIcTwgE8ktXUUIht3ybSd8WphY8&s=Y6LNJpoXU2BdIsXqef34Gk0al6E4EaHcbISPLhClhVc&e=
diff mbox series

Patch

diff --git a/datapath-windows/ovsext/Geneve.c b/datapath-windows/ovsext/Geneve.c
index 43374e2..77244b1 100644
--- a/datapath-windows/ovsext/Geneve.c
+++ b/datapath-windows/ovsext/Geneve.c
@@ -324,10 +324,10 @@  NDIS_STATUS OvsDecapGeneve(POVS_SWITCH_CONTEXT switchContext,
         status = STATUS_NDIS_INVALID_PACKET;
         goto dropNbl;
     }
-    tunKey->flags = OVS_TNL_F_KEY;
-    if (geneveHdr->oam) {
-        tunKey->flags |= OVS_TNL_F_OAM;
-    }
+    /* Update tunnelKey flags. */
+    tunKey->flags = OVS_TNL_F_KEY | OVS_TNL_F_GENEVE_OPT |
+                    (geneveHdr->oam ? OVS_TNL_F_OAM : 0) |
+                    (geneveHdr->critical ? OVS_TNL_F_CRT_OPT : 0);
     tunKey->tunnelId = GENEVE_VNI_TO_TUNNELID(geneveHdr->vni);
     tunKey->tunOptLen = (uint8)geneveHdr->optLen * 4;
     if (tunKey->tunOptLen > TUN_OPT_MAX_LEN ||