[ovs-dev] [PATCH] datapath-windows: Support attribute OVS_KEY_ATTR_TCP_FLAGS

Nithin Raju nithin at vmware.com
Thu Oct 1 20:55:01 UTC 2015


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 at 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 at 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


More information about the dev mailing list