| Message ID | 1442966471-14972-1-git-send-email-aserdean@cloudbasesolutions.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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
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; } }
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(-)