[ovs-dev] datapath-windows: Support attribute OVS_KEY_ATTR_TCP_FLAGS
diff mbox

Message ID 1442966471-14972-1-git-send-email-aserdean@cloudbasesolutions.com
State Changes Requested
Headers show

Commit Message

Alin Serdean Sept. 23, 2015, midnight UTC
This patch adds OVS_KEY_ATTR_TCP_FLAGS to our flow mechanism.

Also clean unecesarry parts of code.

Signed-off-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
---
This patch is intended for branch-2.4 as well
---
 datapath-windows/ovsext/DpInternal.h   |  9 +++++---
 datapath-windows/ovsext/Flow.c         | 40 ++++++++++++++++++++++++++--------
 datapath-windows/ovsext/PacketParser.h |  8 +++----
 3 files changed, 41 insertions(+), 16 deletions(-)

Comments

Nithin Raju Oct. 1, 2015, 8:55 p.m. UTC | #1
hi Alin,
The change looks good. However I have a few minor comments. Pls. take a look.

> On Sep 22, 2015, at 5:00 PM, Alin Serdean <aserdean@cloudbasesolutions.com> wrote:

> 

> This patch adds OVS_KEY_ATTR_TCP_FLAGS to our flow mechanism.

> 

> Also clean unecesarry parts of code.

> 

> Signed-off-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>

> ---

> This patch is intended for branch-2.4 as well

> ---

> datapath-windows/ovsext/DpInternal.h   |  9 +++++---

> datapath-windows/ovsext/Flow.c         | 40 ++++++++++++++++++++++++++--------

> datapath-windows/ovsext/PacketParser.h |  8 +++----

> 3 files changed, 41 insertions(+), 16 deletions(-)

> 

> diff --git a/datapath-windows/ovsext/DpInternal.h b/datapath-windows/ovsext/DpInternal.h

> index 3da9d6a..1cb3549 100644

> --- a/datapath-windows/ovsext/DpInternal.h

> +++ b/datapath-windows/ovsext/DpInternal.h

> @@ -71,6 +71,7 @@ typedef struct _OVS_VPORT_EXT_INFO {

> typedef struct L4Key {

>     ovs_be16 tpSrc;              /* TCP/UDP/SCTP source port. */

>     ovs_be16 tpDst;              /* TCP/UDP/SCTP destination port. */

> +    ovs_be16 flags;              /* TCP flags */

> } L4Key;

> 


Wouldn’t it be better to add a pad[2] to L4Key itself? You can avoid updating the other structures. The current assumptions in each of the structures that include L4Key is that L4Key is a value of 4x.

> typedef struct Ipkey {

> @@ -81,7 +82,8 @@ typedef struct Ipkey {

>     uint8_t nwTtl;               /* IP TTL/Hop Limit. */

>     uint8_t nwFrag;              /* FLOW_FRAG_* flags. */

>     L4Key   l4;

> -} IpKey;  /* Size of 16 byte. */

> +    uint8_t pad[6];

> +} IpKey;  /* Size of 24 byte. */

> 

> typedef struct ArpKey {

>     ovs_be32 nwSrc;              /* IPv4 source address. */

> @@ -101,7 +103,7 @@ typedef struct Ipv6Key {

>     uint8_t nwTtl;               /* IP TTL/Hop Limit. */

>     uint8_t nwFrag;              /* FLOW_FRAG_* flags. */

>     L4Key  l4;

> -    uint32_t pad;

> +    uint8_t pad[2];

> } Ipv6Key;  /* Size of 48 byte. */

> 

> typedef struct Icmp6Key {

> @@ -116,7 +118,8 @@ typedef struct Icmp6Key {

>     uint8_t arpSha[6];           /* ARP/ND source hardware address. */

>     uint8_t arpTha[6];           /* ARP/ND target hardware address. */

>     struct in6_addr ndTarget;    /* IPv6 neighbor discovery (ND) target. */

> -} Icmp6Key; /* Size of 72 byte. */

> +    uint8_t pad[6];

> +} Icmp6Key; /* Size of 80 byte. */

> 

> typedef struct L2Key {

>     uint32_t inPort;             /* Port number of input port. */

> diff --git a/datapath-windows/ovsext/Flow.c b/datapath-windows/ovsext/Flow.c

> index 32cb086..bc46105 100644

> --- a/datapath-windows/ovsext/Flow.c

> +++ b/datapath-windows/ovsext/Flow.c

> @@ -988,6 +988,13 @@ _MapFlowIpv4KeyToNlKey(PNL_BUFFER nlBuf, IpKey *ipv4FlowPutKey)

>                 rc = STATUS_UNSUCCESSFUL;

>                 goto done;

>             }

> +            UINT16 tcpFlags = ipv4FlowPutKey->l4.flags;

> +            if (!NlMsgPutTailUnspec(nlBuf, OVS_KEY_ATTR_TCP_FLAGS,

> +                (PCHAR)(&tcpFlags),

> +                sizeof(tcpFlags))) {

> +                rc = STATUS_UNSUCCESSFUL;

> +                goto done;

> +            }

>             break;

>         }

> @@ -1082,6 +1089,13 @@ _MapFlowIpv6KeyToNlKey(PNL_BUFFER nlBuf, Ipv6Key *ipv6FlowPutKey,

>                 rc = STATUS_UNSUCCESSFUL;

>                 goto done;

>             }

> +            UINT16 tcpFlags = ipv6FlowPutKey->l4.flags;

> +            if (!NlMsgPutTailUnspec(nlBuf, OVS_KEY_ATTR_TCP_FLAGS,

> +                (PCHAR)(&tcpFlags),

> +                sizeof(tcpFlags))) {

> +                rc = STATUS_UNSUCCESSFUL;

> +                goto done;

> +            }

>             break;

>         }

> 

> @@ -1357,6 +1371,12 @@ _MapKeyAttrToFlowPut(PNL_ATTR *keyAttrs,

>                 ipv4FlowPutKey->l4.tpDst = tcpKey->tcp_dst;

>             }

> 

> +            if (keyAttrs[OVS_KEY_ATTR_TCP_FLAGS]) {

> +                const UINT16 *flags;

> +                flags = NlAttrGet(keyAttrs[OVS_KEY_ATTR_TCP_FLAGS]);

> +                ipv4FlowPutKey->l4.flags = *flags;

> +            }

> +

>             if (keyAttrs[OVS_KEY_ATTR_UDP]) {

>                 const struct ovs_key_udp *udpKey;

>                 udpKey = NlAttrGet(keyAttrs[OVS_KEY_ATTR_UDP]);

> @@ -1401,6 +1421,12 @@ _MapKeyAttrToFlowPut(PNL_ATTR *keyAttrs,

>                 ipv6FlowPutKey->l4.tpDst = tcpKey->tcp_dst;

>             }

> 

> +            if (keyAttrs[OVS_KEY_ATTR_TCP_FLAGS]) {

> +                const UINT16 *flags;

> +                flags = NlAttrGet(keyAttrs[OVS_KEY_ATTR_TCP_FLAGS]);

> +                ipv6FlowPutKey->l4.flags = *flags;

> +            }

> +

>             if (keyAttrs[OVS_KEY_ATTR_UDP]) {

>                 const struct ovs_key_udp *udpKey;

>                 udpKey = NlAttrGet(keyAttrs[OVS_KEY_ATTR_UDP]);

> @@ -1444,7 +1470,6 @@ _MapKeyAttrToFlowPut(PNL_ATTR *keyAttrs,

>                 destKey->l2.keyLen += OVS_IPV6_KEY_SIZE;

>             }

> 

> -            ipv6FlowPutKey->pad = 0;

>         }

>         break;

>     }

> @@ -1465,9 +1490,6 @@ _MapKeyAttrToFlowPut(PNL_ATTR *keyAttrs,

>             /* Kernel datapath assumes 'arpFlowPutKey->nwProto' to be in host

>              * order. */

>             arpFlowPutKey->nwProto = (UINT8)ntohs((arpKey->arp_op));

> -            arpFlowPutKey->pad[0] = 0;

> -            arpFlowPutKey->pad[1] = 0;

> -            arpFlowPutKey->pad[2] = 0;

>             destKey->l2.keyLen += OVS_ARP_KEY_SIZE;

>             break;

>         }

> @@ -1522,7 +1544,6 @@ _MapTunAttrToFlowPut(PNL_ATTR *keyAttrs,

>                               (tunAttrs[OVS_TUNNEL_KEY_ATTR_TTL]);

>         }

> 

> -        destKey->tunKey.pad = 0;


We probably want to keep this, since the pad is part of a union. Setting it to 0, sets tunKey.dst_port, tunKey.flow_hash to 0. It is another issue why we have this pad, or if we using tunKey.dst_port. We can tackle this in another patch.


>         destKey->l2.offset = 0;

>     } else {

>         destKey->tunKey.attr[0] = 0;

> @@ -1635,7 +1656,7 @@ OvsFlowUsed(OvsFlow *flow,

>     flow->used = tickCount.QuadPart * ovsTimeIncrementPerTick;

>     flow->packetCount++;

>     flow->byteCount += OvsPacketLenNBL(packet);

> -    flow->tcpFlags |= OvsGetTcpFlags(packet, &flow->key, layers);

> +    flow->tcpFlags |= OvsGetTcpFlags(packet, (UINT16 *)&flow->tcpFlags, layers);

> }

> 

> 

> @@ -1796,10 +1817,12 @@ OvsExtractFlow(const NET_BUFFER_LIST *packet,

>             ipKey->nwTtl = nh->ttl;

>             ipKey->l4.tpSrc = 0;

>             ipKey->l4.tpDst = 0;

> +            ipKey->l4.flags = 0;

> 

>             if (!(nh->frag_off & htons(IP_OFFSET))) {

>                 if (ipKey->nwProto == SOCKET_IPPROTO_TCP) {

>                     OvsParseTcp(packet, &ipKey->l4, layers);

> +                    OvsGetTcpFlags(packet, &ipKey->l4.flags, layers);


minor: could call OvsGetTcpFlags() inside OvsParseTcp() itself.

>                 } else if (ipKey->nwProto == SOCKET_IPPROTO_UDP) {

>                     OvsParseUdp(packet, &ipKey->l4, layers);

>                 } else if (ipKey->nwProto == SOCKET_IPPROTO_ICMP) {

> @@ -1829,10 +1852,11 @@ OvsExtractFlow(const NET_BUFFER_LIST *packet,

>         layers->isIPv6 = 1;

>         flow->ipv6Key.l4.tpSrc = 0;

>         flow->ipv6Key.l4.tpDst = 0;

> -        flow->ipv6Key.pad = 0;

> +        flow->ipv6Key.l4.flags = 0;

> 

>         if (flow->ipv6Key.nwProto == SOCKET_IPPROTO_TCP) {

>             OvsParseTcp(packet, &(flow->ipv6Key.l4), layers);

> +            OvsGetTcpFlags(packet, &flow->ipv6Key.l4.flags, layers);

minor: could call OvsGetTcpFlags() inside OvsParseTcp() itself.

>         } else if (flow->ipv6Key.nwProto == SOCKET_IPPROTO_UDP) {

>             OvsParseUdp(packet, &(flow->ipv6Key.l4), layers);

>         } else if (flow->ipv6Key.nwProto == SOCKET_IPPROTO_ICMPV6) {

> @@ -1910,9 +1934,7 @@ AddFlow(OVS_DATAPATH *datapath, OvsFlow *flow)

>      */

>     KeMemoryBarrier();

> 

> -    //KeAcquireSpinLock(&FilterDeviceExtension->NblQueueLock, &oldIrql);

>     InsertTailList(head, &flow->ListEntry);

> -    //KeReleaseSpinLock(&FilterDeviceExtension->NblQueueLock, oldIrql);

> 

>     datapath->nFlows++;

> 

> diff --git a/datapath-windows/ovsext/PacketParser.h b/datapath-windows/ovsext/PacketParser.h

> index 55d110f..b0f7b1d 100644

> --- a/datapath-windows/ovsext/PacketParser.h

> +++ b/datapath-windows/ovsext/PacketParser.h

> @@ -75,14 +75,14 @@ OvsGetTcpCtl(const NET_BUFFER_LIST *packet, // IN

> 

> static UINT8

> OvsGetTcpFlags(const NET_BUFFER_LIST *packet,    // IN

> -               const OvsFlowKey *key,   // IN

> +               UINT16 *flags,   // IN


‘flags’ looks like an OUT to me.

>                const POVS_PACKET_HDR_INFO layers) // IN

> {

> -    UNREFERENCED_PARAMETER(key); // should be removed later

> -

>     if (layers->isTcp) {

> -        return TCP_FLAGS(OvsGetTcpCtl(packet, layers));

> +        *flags = OvsGetTcpCtl(packet, layers);

> +        return TCP_FLAGS(*flags);

>     } else {

> +        *flags = 0;

>         return 0;

>     }

> }


I am not sure if the code is correct. OvsGetTcpCtl() returns a UINT16 which includes the following fields:
Data Offset:4 + reserved:3 + ECN:3 + Control Bits: 6

What we want is only the Control Bits. Hence the TCP_FLAGS().

You probably want to do:
*flags = TCP_FLAGS(OvsGetTcpCtl(packet, layers));

I think it is best to call OvsGetTcpFlags() from within ParseTcp() itself.

thanks,
-- Nithin

Patch
diff mbox

diff --git a/datapath-windows/ovsext/DpInternal.h b/datapath-windows/ovsext/DpInternal.h
index 3da9d6a..1cb3549 100644
--- a/datapath-windows/ovsext/DpInternal.h
+++ b/datapath-windows/ovsext/DpInternal.h
@@ -71,6 +71,7 @@  typedef struct _OVS_VPORT_EXT_INFO {
 typedef struct L4Key {
     ovs_be16 tpSrc;              /* TCP/UDP/SCTP source port. */
     ovs_be16 tpDst;              /* TCP/UDP/SCTP destination port. */
+    ovs_be16 flags;              /* TCP flags */
 } L4Key;
 
 typedef struct Ipkey {
@@ -81,7 +82,8 @@  typedef struct Ipkey {
     uint8_t nwTtl;               /* IP TTL/Hop Limit. */
     uint8_t nwFrag;              /* FLOW_FRAG_* flags. */
     L4Key   l4;
-} IpKey;  /* Size of 16 byte. */
+    uint8_t pad[6];
+} IpKey;  /* Size of 24 byte. */
 
 typedef struct ArpKey {
     ovs_be32 nwSrc;              /* IPv4 source address. */
@@ -101,7 +103,7 @@  typedef struct Ipv6Key {
     uint8_t nwTtl;               /* IP TTL/Hop Limit. */
     uint8_t nwFrag;              /* FLOW_FRAG_* flags. */
     L4Key  l4;
-    uint32_t pad;
+    uint8_t pad[2];
 } Ipv6Key;  /* Size of 48 byte. */
 
 typedef struct Icmp6Key {
@@ -116,7 +118,8 @@  typedef struct Icmp6Key {
     uint8_t arpSha[6];           /* ARP/ND source hardware address. */
     uint8_t arpTha[6];           /* ARP/ND target hardware address. */
     struct in6_addr ndTarget;    /* IPv6 neighbor discovery (ND) target. */
-} Icmp6Key; /* Size of 72 byte. */
+    uint8_t pad[6];
+} Icmp6Key; /* Size of 80 byte. */
 
 typedef struct L2Key {
     uint32_t inPort;             /* Port number of input port. */
diff --git a/datapath-windows/ovsext/Flow.c b/datapath-windows/ovsext/Flow.c
index 32cb086..bc46105 100644
--- a/datapath-windows/ovsext/Flow.c
+++ b/datapath-windows/ovsext/Flow.c
@@ -988,6 +988,13 @@  _MapFlowIpv4KeyToNlKey(PNL_BUFFER nlBuf, IpKey *ipv4FlowPutKey)
                 rc = STATUS_UNSUCCESSFUL;
                 goto done;
             }
+            UINT16 tcpFlags = ipv4FlowPutKey->l4.flags;
+            if (!NlMsgPutTailUnspec(nlBuf, OVS_KEY_ATTR_TCP_FLAGS,
+                (PCHAR)(&tcpFlags),
+                sizeof(tcpFlags))) {
+                rc = STATUS_UNSUCCESSFUL;
+                goto done;
+            }
             break;
         }
 
@@ -1082,6 +1089,13 @@  _MapFlowIpv6KeyToNlKey(PNL_BUFFER nlBuf, Ipv6Key *ipv6FlowPutKey,
                 rc = STATUS_UNSUCCESSFUL;
                 goto done;
             }
+            UINT16 tcpFlags = ipv6FlowPutKey->l4.flags;
+            if (!NlMsgPutTailUnspec(nlBuf, OVS_KEY_ATTR_TCP_FLAGS,
+                (PCHAR)(&tcpFlags),
+                sizeof(tcpFlags))) {
+                rc = STATUS_UNSUCCESSFUL;
+                goto done;
+            }
             break;
         }
 
@@ -1357,6 +1371,12 @@  _MapKeyAttrToFlowPut(PNL_ATTR *keyAttrs,
                 ipv4FlowPutKey->l4.tpDst = tcpKey->tcp_dst;
             }
 
+            if (keyAttrs[OVS_KEY_ATTR_TCP_FLAGS]) {
+                const UINT16 *flags;
+                flags = NlAttrGet(keyAttrs[OVS_KEY_ATTR_TCP_FLAGS]);
+                ipv4FlowPutKey->l4.flags = *flags;
+            }
+
             if (keyAttrs[OVS_KEY_ATTR_UDP]) {
                 const struct ovs_key_udp *udpKey;
                 udpKey = NlAttrGet(keyAttrs[OVS_KEY_ATTR_UDP]);
@@ -1401,6 +1421,12 @@  _MapKeyAttrToFlowPut(PNL_ATTR *keyAttrs,
                 ipv6FlowPutKey->l4.tpDst = tcpKey->tcp_dst;
             }
 
+            if (keyAttrs[OVS_KEY_ATTR_TCP_FLAGS]) {
+                const UINT16 *flags;
+                flags = NlAttrGet(keyAttrs[OVS_KEY_ATTR_TCP_FLAGS]);
+                ipv6FlowPutKey->l4.flags = *flags;
+            }
+
             if (keyAttrs[OVS_KEY_ATTR_UDP]) {
                 const struct ovs_key_udp *udpKey;
                 udpKey = NlAttrGet(keyAttrs[OVS_KEY_ATTR_UDP]);
@@ -1444,7 +1470,6 @@  _MapKeyAttrToFlowPut(PNL_ATTR *keyAttrs,
                 destKey->l2.keyLen += OVS_IPV6_KEY_SIZE;
             }
 
-            ipv6FlowPutKey->pad = 0;
         }
         break;
     }
@@ -1465,9 +1490,6 @@  _MapKeyAttrToFlowPut(PNL_ATTR *keyAttrs,
             /* Kernel datapath assumes 'arpFlowPutKey->nwProto' to be in host
              * order. */
             arpFlowPutKey->nwProto = (UINT8)ntohs((arpKey->arp_op));
-            arpFlowPutKey->pad[0] = 0;
-            arpFlowPutKey->pad[1] = 0;
-            arpFlowPutKey->pad[2] = 0;
             destKey->l2.keyLen += OVS_ARP_KEY_SIZE;
             break;
         }
@@ -1522,7 +1544,6 @@  _MapTunAttrToFlowPut(PNL_ATTR *keyAttrs,
                               (tunAttrs[OVS_TUNNEL_KEY_ATTR_TTL]);
         }
 
-        destKey->tunKey.pad = 0;
         destKey->l2.offset = 0;
     } else {
         destKey->tunKey.attr[0] = 0;
@@ -1635,7 +1656,7 @@  OvsFlowUsed(OvsFlow *flow,
     flow->used = tickCount.QuadPart * ovsTimeIncrementPerTick;
     flow->packetCount++;
     flow->byteCount += OvsPacketLenNBL(packet);
-    flow->tcpFlags |= OvsGetTcpFlags(packet, &flow->key, layers);
+    flow->tcpFlags |= OvsGetTcpFlags(packet, (UINT16 *)&flow->tcpFlags, layers);
 }
 
 
@@ -1796,10 +1817,12 @@  OvsExtractFlow(const NET_BUFFER_LIST *packet,
             ipKey->nwTtl = nh->ttl;
             ipKey->l4.tpSrc = 0;
             ipKey->l4.tpDst = 0;
+            ipKey->l4.flags = 0;
 
             if (!(nh->frag_off & htons(IP_OFFSET))) {
                 if (ipKey->nwProto == SOCKET_IPPROTO_TCP) {
                     OvsParseTcp(packet, &ipKey->l4, layers);
+                    OvsGetTcpFlags(packet, &ipKey->l4.flags, layers);
                 } else if (ipKey->nwProto == SOCKET_IPPROTO_UDP) {
                     OvsParseUdp(packet, &ipKey->l4, layers);
                 } else if (ipKey->nwProto == SOCKET_IPPROTO_ICMP) {
@@ -1829,10 +1852,11 @@  OvsExtractFlow(const NET_BUFFER_LIST *packet,
         layers->isIPv6 = 1;
         flow->ipv6Key.l4.tpSrc = 0;
         flow->ipv6Key.l4.tpDst = 0;
-        flow->ipv6Key.pad = 0;
+        flow->ipv6Key.l4.flags = 0;
 
         if (flow->ipv6Key.nwProto == SOCKET_IPPROTO_TCP) {
             OvsParseTcp(packet, &(flow->ipv6Key.l4), layers);
+            OvsGetTcpFlags(packet, &flow->ipv6Key.l4.flags, layers);
         } else if (flow->ipv6Key.nwProto == SOCKET_IPPROTO_UDP) {
             OvsParseUdp(packet, &(flow->ipv6Key.l4), layers);
         } else if (flow->ipv6Key.nwProto == SOCKET_IPPROTO_ICMPV6) {
@@ -1910,9 +1934,7 @@  AddFlow(OVS_DATAPATH *datapath, OvsFlow *flow)
      */
     KeMemoryBarrier();
 
-    //KeAcquireSpinLock(&FilterDeviceExtension->NblQueueLock, &oldIrql);
     InsertTailList(head, &flow->ListEntry);
-    //KeReleaseSpinLock(&FilterDeviceExtension->NblQueueLock, oldIrql);
 
     datapath->nFlows++;
 
diff --git a/datapath-windows/ovsext/PacketParser.h b/datapath-windows/ovsext/PacketParser.h
index 55d110f..b0f7b1d 100644
--- a/datapath-windows/ovsext/PacketParser.h
+++ b/datapath-windows/ovsext/PacketParser.h
@@ -75,14 +75,14 @@  OvsGetTcpCtl(const NET_BUFFER_LIST *packet, // IN
 
 static UINT8
 OvsGetTcpFlags(const NET_BUFFER_LIST *packet,    // IN
-               const OvsFlowKey *key,   // IN
+               UINT16 *flags,   // IN
                const POVS_PACKET_HDR_INFO layers) // IN
 {
-    UNREFERENCED_PARAMETER(key); // should be removed later
-
     if (layers->isTcp) {
-        return TCP_FLAGS(OvsGetTcpCtl(packet, layers));
+        *flags = OvsGetTcpCtl(packet, layers);
+        return TCP_FLAGS(*flags);
     } else {
+        *flags = 0;
         return 0;
     }
 }