[ovs-dev] [PATCH 4/4] datapath-windows: Add ECN support on STT decapsulation
Sairam Venugopal
vsairam at vmware.com
Fri Jun 3 00:11:13 UTC 2016
Hi Paul,
Please see the comments inline.
Thanks!
Sairam
On 5/17/16, 8:23 AM, "Paul Boca" <pboca at cloudbasesolutions.com> wrote:
>Signed-off-by: Paul-Daniel Boca <pboca at cloudbasesolutions.com>
>---
> datapath-windows/ovsext/NetProto.h | 10 ++++-
> datapath-windows/ovsext/PacketParser.h | 67
>++++++++++++++++++++++++++++++++++
> datapath-windows/ovsext/Stt.c | 45 +++++++++++++++++++++++
> datapath-windows/ovsext/Stt.h | 1 +
> 4 files changed, 122 insertions(+), 1 deletion(-)
>
>diff --git a/datapath-windows/ovsext/NetProto.h
>b/datapath-windows/ovsext/NetProto.h
>index f7527f8..6cf6d8e 100644
>--- a/datapath-windows/ovsext/NetProto.h
>+++ b/datapath-windows/ovsext/NetProto.h
>@@ -45,6 +45,8 @@ typedef struct EthHdr {
> #define ICMP_CSUM_OFFSET 2
> #define INET_CSUM_LENGTH (sizeof(UINT16))
>
>+#define PACKET_MAX_LENGTH 64*1024 // 64K
>+
> #define IP4_UNITS_TO_BYTES(x) ((x) << 2)
> #define IP4_BYTES_TO_UNITS(x) ((x) >> 2)
>
>@@ -245,7 +247,13 @@ typedef union _OVS_PACKET_HDR_INFO {
> typedef struct IPHdr {
> UINT8 ihl:4,
> version:4;
>- UINT8 tos;
>+ union {
>+ struct {
>+ UINT8 ecn:2,
>+ dscp:6;
>+ };
>+ UINT8 tos;
>+ };
> UINT16 tot_len;
> UINT16 id;
> UINT16 frag_off;
>diff --git a/datapath-windows/ovsext/PacketParser.h
>b/datapath-windows/ovsext/PacketParser.h
>index f1d7f28..a72b7dc 100644
>--- a/datapath-windows/ovsext/PacketParser.h
>+++ b/datapath-windows/ovsext/PacketParser.h
>@@ -99,6 +99,73 @@ OvsGetArp(const NET_BUFFER_LIST *packet,
> return OvsGetPacketBytes(packet, sizeof *storage, ofs, storage);
> }
>
>+/*
>+* Returns the start of NBL and computes the total length of Ethernet
>header
>+* Also returns the type of L3 header
>+*/
>+static const PVOID
Sai: This should return EthHdr * similar to other methods in PacketParser.h
Nit - name the function - OvsGetEth to keep it consistent with OvsGetTcp,
OvsGetIp, OvsGetArp
>+OvsGetEthHeader(const NET_BUFFER_LIST *packet,
>+ UINT16 *length,
>+ UINT16 *dlType,
>+ Eth_Header *storage)
>+{
>+ UINT8 offset = 0;
>+ PVOID vlanTagValue;
>+ PUINT8 buffStart = NULL;
>+
>+ const Eth_Header *eth = OvsGetPacketBytes(packet, ETH_MAX_HEADER_LEN,
>+ 0, storage);
Sai: You can use EthHdr from NetProto.h instead of Eth_Header. Looks like
Eth_Header gets used only in Flow.c
>+ if (eth == NULL) {
>+ return NULL;
>+ }
>+
>+ /* Keep a copy of packet start */
>+ buffStart = (PUINT8)eth;
>+
>+ /*
>+ * vlan_tci.
>+ */
>+ vlanTagValue = NET_BUFFER_LIST_INFO(packet,
>Ieee8021QNetBufferListInfo);
>+ if (!vlanTagValue) {
>+ if (eth->dix.typeNBO == ETH_TYPE_802_1PQ_NBO) {
>+ offset = sizeof(Eth_802_1pq_Tag);
>+ }
>+
>+ /*
>+ * XXX Please note after this point, src mac and dst mac should
>+ * not be accessed through eth
>+ */
Sai: You can get rid of this comment. Doesn¹t apply here.
>+ eth = (Eth_Header *)((UINT8 *)eth + offset);
>+ }
>+
Sai: Same with this one.
>+ /*
>+ * dl_type.
>+ *
>+ * XXX assume that at least the first
>+ * 12 bytes of received packets are mapped. This code has the
>stronger
>+ * assumption that at least the first 22 bytes of 'packet' is mapped
>(if my
>+ * arithmetic is right).
>+ */
>+ if (ETH_TYPENOT8023(eth->dix.typeNBO)) {
>+ *dlType = eth->dix.typeNBO;
>+ *length = ETH_HEADER_LEN_DIX + offset;
>+ } else if (OvsPacketLenNBL(packet) >= ETH_HEADER_LEN_802_3 &&
>+ eth->e802_3.llc.dsap == 0xaa &&
>+ eth->e802_3.llc.ssap == 0xaa &&
>+ eth->e802_3.llc.control == ETH_LLC_CONTROL_UFRAME &&
>+ eth->e802_3.snap.snapOrg[0] == 0x00 &&
>+ eth->e802_3.snap.snapOrg[1] == 0x00 &&
>+ eth->e802_3.snap.snapOrg[2] == 0x00) {
>+ *dlType = eth->e802_3.snap.snapType.typeNBO;
>+ *length = ETH_HEADER_LEN_802_3 + offset;
>+ } else {
>+ *dlType = htons(OVSWIN_DL_TYPE_NONE);
>+ *length = ETH_HEADER_LEN_DIX + offset;
>+ }
>+
Sai: can reutrn EthHdr * isntead of this cast.
>+ return (UINT8*)eth;
>+}
>+
> static const IPHdr *
> OvsGetIp(const NET_BUFFER_LIST *packet,
> UINT32 ofs,
>diff --git a/datapath-windows/ovsext/Stt.c b/datapath-windows/ovsext/Stt.c
>index 8a1b1a9..a9e1cac 100644
>--- a/datapath-windows/ovsext/Stt.c
>+++ b/datapath-windows/ovsext/Stt.c
>@@ -642,6 +642,9 @@ OvsSttReassemble(POVS_SWITCH_CONTEXT switchContext,
> NdisMoveMemory(&entry->ovsPktKey, &pktKey, sizeof
>(OVS_STT_PKT_KEY));
>
> entry->recvdLen = fragmentLength;
>+ if (ipHdr->ecn == IP_ECN_CE) {
>+ entry->ecn = IP_ECN_CE;
>+ }
>
> UINT64 currentTime;
> NdisGetCurrentSystemTime((LARGE_INTEGER *) ¤tTime);
>@@ -678,6 +681,9 @@ OvsSttReassemble(POVS_SWITCH_CONTEXT switchContext,
> if (segOffset == 0) {
> pktFragEntry->sttHdr = *sttHdr;
> }
>+ if (ipHdr->ecn == IP_ECN_CE) {
>+ pktFragEntry->ecn = IP_ECN_CE;
>+ }
>
> /* Copy the fragment data from Source to existing buffer */
> if (OvsGetPacketBytes(curNbl, fragmentLength, startOffset,
>@@ -689,6 +695,14 @@ OvsSttReassemble(POVS_SWITCH_CONTEXT switchContext,
>
> handle_error:
> if (lastPacket) {
>+ /* It is RECOMMENDED that if any segment of the received STT
>+ * frame has the CE (congestion experienced) bit set
>+ * in its IP header, then the CE bit SHOULD be set in the IP
>+ * header of the decapsulated STT frame.*/
>+ if (pktFragEntry->ecn == IP_ECN_CE) {
>+ ipHdr->ecn = IP_ECN_CE;
>+ }
>+
> /* Retrieve the original STT header */
> NdisMoveMemory(newSttHdr, &pktFragEntry->sttHdr, sizeof
>(SttHdr));
> targetPNbl = OvsAllocateNBLFromBuffer(switchContext,
>@@ -920,6 +934,37 @@ OvsDecapStt(POVS_SWITCH_CONTEXT switchContext,
> tunKey->ttl = ipHdr->ttl;
> tunKey->pad = 0;
>
>+ /* Handle ECN */
>+ if (0 != ipHdr->tos) {
>+ UINT16 length, dlType;
>+ Eth_Header storage;
>+ PUINT8 ethHdr = OvsGetEthHeader(*newNbl, &length, &dlType,
>&storage);
Sai: If you need the ethHdr only to access the IPHdr for ECN bits, can¹t
you access it via OvsExtractLayers?
Since you need to call the new OvsExtractLayers for applying offloads, you
can simply do it here and reuse it.
This way you can avoid the new function that sets dlType,length apart from
returning EthHdr. It would be cleaner if GetEth() just returned EthHdr
>+
>+ if (dlType == htons(ETH_TYPE_IPV4)) {
>+ IPHdr *innerIpHdr = (IPHdr*)(ethHdr + length);
>+
>+ /*
>+ * If CE is set for outer IP header, reset ECN of inner IP
>+ * header to CE, all other values are kept the same
>+ */
>+ if (ipHdr->ecn == IP_ECN_CE) {
>+ innerIpHdr->ecn |= IP_ECN_CE;
>+ }
>+ /* copy DSCP from outer header to inner header */
>+ innerIpHdr->dscp = ipHdr->dscp;
>+ /* fix IP checksum */
>+ innerIpHdr->check = IPChecksum((UINT8 *)innerIpHdr,
>+ innerIpHdr->ihl * 4, 0);
>+ } else if (dlType == htons(ETH_TYPE_IPV6)) {
>+ IPv6Hdr *innerIpv6Hdr = (IPv6Hdr*)(ethHdr + length);
>+ /* copy ECN and DSCN to inner header */
>+ innerIpv6Hdr->priority = ipHdr->ecn
>+ | ((innerIpv6Hdr->flow_lbl[0] & 0x3)
><< 2);
>+ innerIpv6Hdr->flow_lbl[0] = (innerIpv6Hdr->flow_lbl[0] & 0xF)
>+ | ((ipHdr->tos & 0xF) << 4);
>+ }
>+ }
>+
> /* Apply VLAN tag if present */
> if (ntohs(sttHdr->vlanTCI) & OVSWIN_VLAN_CFI) {
> NDIS_NET_BUFFER_LIST_8021Q_INFO vlanTag;
>diff --git a/datapath-windows/ovsext/Stt.h b/datapath-windows/ovsext/Stt.h
>index 8aea164..b9e8b88 100644
>--- a/datapath-windows/ovsext/Stt.h
>+++ b/datapath-windows/ovsext/Stt.h
>@@ -68,6 +68,7 @@ typedef struct _OVS_STT_PKT_ENTRY {
> UINT64 timeout;
> UINT32 recvdLen;
> UINT32 allocatedLen;
>+ UINT8 ecn;
> SttHdr sttHdr;
> PCHAR packetBuf;
> LIST_ENTRY link;
>--
>2.7.2.windows.1
>_______________________________________________
>dev mailing list
>dev at openvswitch.org
>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailma
>n_listinfo_dev&d=CwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=Dc
>ruz40PROJ40ROzSpxyQSLw6fcrOWpJgEcEmNR3JEQ&m=Mi-JxLLKNb2A_-R5uXH2WlnPdSYQTW
>RmVTuiSHvhFig&s=jWQLgUdiYUafyPCabvtXsEn-qfaO1mmnZZc8UaIbL9E&e=
More information about the dev
mailing list