From 00d36e836cc57a26d9315cfb693953fcaba9bedb Mon Sep 17 00:00:00 2001 From: Mathieu Lacage Date: Sat, 31 May 2008 10:46:23 -0700 Subject: [PATCH 1/6] add logging, cleanup AddAtEnd. --- src/common/packet-metadata.cc | 112 ++++++++++++++++++++++++---------- 1 file changed, 79 insertions(+), 33 deletions(-) diff --git a/src/common/packet-metadata.cc b/src/common/packet-metadata.cc index 3bc23f6ba..0c5345415 100644 --- a/src/common/packet-metadata.cc +++ b/src/common/packet-metadata.cc @@ -350,6 +350,7 @@ PacketMetadata::AppendValue (uint32_t value, uint8_t *buffer) void PacketMetadata::UpdateTail (uint16_t written) { + NS_LOG_FUNCTION (this << written); if (m_head == 0xffff) { NS_ASSERT (m_tail == 0xffff); @@ -376,6 +377,7 @@ PacketMetadata::UpdateTail (uint16_t written) void PacketMetadata::UpdateHead (uint16_t written) { + NS_LOG_FUNCTION (this << written); if (m_head == 0xffff) { NS_ASSERT (m_tail == 0xffff); @@ -401,6 +403,7 @@ PacketMetadata::UpdateHead (uint16_t written) uint16_t PacketMetadata::AddSmall (const struct PacketMetadata::SmallItem *item) { + NS_LOG_FUNCTION (this << item->next << item->prev << item->typeUid << item->size << item->chunkUid); NS_ASSERT (m_data != 0); NS_ASSERT (m_used != item->prev && m_used != item->next); uint32_t typeUidSize = GetUleb128Size (item->typeUid); @@ -431,6 +434,9 @@ PacketMetadata::AddBig (uint32_t next, uint32_t prev, const PacketMetadata::SmallItem *item, const PacketMetadata::ExtraItem *extraItem) { + NS_LOG_FUNCTION (this << next << prev << + item->next << item->prev << item->typeUid << item->size << item->chunkUid << + extraItem->fragmentStart << extraItem->fragmentEnd << extraItem->packetUid); NS_ASSERT (m_data != 0); uint32_t typeUid = ((item->typeUid & 0x1) == 0x1)?item->typeUid:item->typeUid+1; NS_ASSERT (m_used != prev && m_used != next); @@ -470,12 +476,32 @@ PacketMetadata::AddBig (uint32_t next, uint32_t prev, return n; } +/** + * \param item the item data to write + * \param extraItem the extra item data to write + * \param available the number of bytes which can + * be written without having to rewrite the buffer entirely. + * + * XXX: should rewrite the code below to avoid using + * TryToAppend calls. + */ void PacketMetadata::ReplaceTail (PacketMetadata::SmallItem *item, PacketMetadata::ExtraItem *extraItem, uint32_t available) { + NS_LOG_FUNCTION (this << + item->next << item->prev << item->typeUid << item->size << item->chunkUid << + extraItem->fragmentStart << extraItem->fragmentEnd << extraItem->packetUid << + available); + NS_ASSERT (m_data != 0); + if (m_tail + available == m_used && + m_used == m_data->m_dirtyEnd) + { + available = m_data->m_size - m_tail; + } + if (available >= 14 && m_data->m_count == 1) { @@ -518,11 +544,18 @@ PacketMetadata::ReplaceTail (PacketMetadata::SmallItem *item, *this = h; } +/** + * \param current the offset we should start reading the data from + * \param item pointer to where we should store the data to return to the caller + * \param extraItem pointer to where we should store the data to return to the caller + * \returns the number of bytes read. + */ uint32_t PacketMetadata::ReadItems (uint16_t current, struct PacketMetadata::SmallItem *item, struct PacketMetadata::ExtraItem *extraItem) const { + NS_LOG_FUNCTION (this << current); const uint8_t *buffer = &m_data->m_data[current]; item->next = buffer[0]; item->next |= (buffer[1]) << 8; @@ -644,12 +677,12 @@ PacketMetadata::AddHeader (const Header &header, uint32_t size) void PacketMetadata::DoAddHeader (uint32_t uid, uint32_t size) { + NS_LOG_FUNCTION (this << uid << size); if (!m_enable) { m_metadataSkipped = true; return; } - NS_LOG_FUNCTION ("uid=" << uid << "size=" << size << ""); struct PacketMetadata::SmallItem item; item.next = m_head; @@ -665,12 +698,12 @@ void PacketMetadata::RemoveHeader (const Header &header, uint32_t size) { uint32_t uid = header.GetInstanceTypeId ().GetUid () << 1; + NS_LOG_FUNCTION (this << uid << size); if (!m_enable) { m_metadataSkipped = true; return; } - NS_LOG_FUNCTION ("(uid=" << uid << ", size=" << size << ")"); struct PacketMetadata::SmallItem item; struct PacketMetadata::ExtraItem extraItem; uint32_t read = ReadItems (m_head, &item, &extraItem); @@ -703,12 +736,12 @@ void PacketMetadata::AddTrailer (const Trailer &trailer, uint32_t size) { uint32_t uid = trailer.GetInstanceTypeId ().GetUid () << 1; + NS_LOG_FUNCTION (this << uid << size); if (!m_enable) { m_metadataSkipped = true; return; } - NS_LOG_FUNCTION ("(uid=" << uid << ", size=" << size << ")"); struct PacketMetadata::SmallItem item; item.next = 0xffff; item.prev = m_tail; @@ -723,12 +756,12 @@ void PacketMetadata::RemoveTrailer (const Trailer &trailer, uint32_t size) { uint32_t uid = trailer.GetInstanceTypeId ().GetUid () << 1; + NS_LOG_FUNCTION (this << uid << size); if (!m_enable) { m_metadataSkipped = true; return; } - NS_LOG_FUNCTION ("(uid=" << uid << ", size=" << size << ")"); struct PacketMetadata::SmallItem item; struct PacketMetadata::ExtraItem extraItem; uint32_t read = ReadItems (m_tail, &item, &extraItem); @@ -760,6 +793,7 @@ PacketMetadata::RemoveTrailer (const Trailer &trailer, uint32_t size) void PacketMetadata::AddAtEnd (PacketMetadata const&o) { + NS_LOG_FUNCTION (this << &o); if (!m_enable) { m_metadataSkipped = true; @@ -767,46 +801,53 @@ PacketMetadata::AddAtEnd (PacketMetadata const&o) } if (m_tail == 0xffff) { + // We have no items so 'AddAtEnd' is + // equivalent to self-assignment. *this = o; return; } NS_ASSERT (m_head != 0xffff && m_tail != 0xffff); - uint16_t lastTail; - lastTail = m_tail; - struct PacketMetadata::SmallItem lastItem; - PacketMetadata::ExtraItem lastExtraItem; - uint32_t lastTailSize = ReadItems (m_tail, &lastItem, &lastExtraItem); - if (m_tail + lastTailSize == m_used && - m_used == m_data->m_dirtyEnd) + // We read the current tail because we are going to append + // after this item. + struct PacketMetadata::SmallItem tailItem; + PacketMetadata::ExtraItem tailExtraItem; + uint32_t tailSize = ReadItems (m_tail, &tailItem, &tailExtraItem); + + uint16_t current; + struct PacketMetadata::SmallItem item; + PacketMetadata::ExtraItem extraItem; + o.ReadItems (o.m_head, &item, &extraItem); + if (extraItem.packetUid == tailExtraItem.packetUid && + item.typeUid == tailItem.typeUid && + item.chunkUid == tailItem.chunkUid && + item.size == tailItem.size && + extraItem.fragmentStart == tailExtraItem.fragmentEnd) { - lastTailSize = m_data->m_size - m_tail; + /* If the previous tail came from the same header as + * the next item we want to append to our array, then, + * we merge them and attempt to reuse the previous tail's + * location. + */ + tailExtraItem.fragmentEnd = extraItem.fragmentEnd; + // XXX This call might be wrong. + ReplaceTail (&tailItem, &tailExtraItem, tailSize); + current = item.next; + } + else + { + current = o.m_head; } - uint16_t current = o.m_head; + /* Now that we have merged our current tail with the head of the + * next packet, we just append all items from the next packet + * to the current packet. + */ while (current != 0xffff) { - struct PacketMetadata::SmallItem item; - PacketMetadata::ExtraItem extraItem; o.ReadItems (current, &item, &extraItem); - if (extraItem.packetUid == lastExtraItem.packetUid && - item.typeUid == lastItem.typeUid && - item.chunkUid == lastItem.chunkUid && - item.size == lastItem.size && - extraItem.fragmentStart == lastExtraItem.fragmentEnd) - { - // replace previous tail. - lastExtraItem.fragmentEnd = extraItem.fragmentEnd; - NS_ASSERT (m_tail == lastTail); - // XXX This call might be wrong. - ReplaceTail (&lastItem, &lastExtraItem, lastTailSize); - } - else - { - // append the extra items. - uint16_t written = AddBig (0xffff, m_tail, &item, &extraItem); - UpdateTail (written); - } + uint16_t written = AddBig (0xffff, m_tail, &item, &extraItem); + UpdateTail (written); if (current == o.m_tail) { break; @@ -826,6 +867,7 @@ PacketMetadata::AddPaddingAtEnd (uint32_t end) void PacketMetadata::RemoveAtStart (uint32_t start) { + NS_LOG_FUNCTION (this << start); if (!m_enable) { m_metadataSkipped = true; @@ -891,6 +933,7 @@ PacketMetadata::RemoveAtStart (uint32_t start) void PacketMetadata::RemoveAtEnd (uint32_t end) { + NS_LOG_FUNCTION (this << end); if (!m_enable) { m_metadataSkipped = true; @@ -1069,6 +1112,7 @@ PacketMetadata::ItemIterator::Next (void) uint32_t PacketMetadata::GetSerializedSize (void) const { + NS_LOG_FUNCTION (this); uint32_t totalSize = 0; totalSize += 4; if (!m_enable) @@ -1105,6 +1149,7 @@ PacketMetadata::GetSerializedSize (void) const void PacketMetadata::Serialize (Buffer::Iterator i, uint32_t size) const { + NS_LOG_FUNCTION (this); uint32_t bytesWritten = 0; i.WriteU32 (size); bytesWritten += 4; @@ -1160,6 +1205,7 @@ PacketMetadata::Serialize (Buffer::Iterator i, uint32_t size) const uint32_t PacketMetadata::Deserialize (Buffer::Iterator i) { + NS_LOG_FUNCTION (this); struct PacketMetadata::SmallItem item; struct PacketMetadata::ExtraItem extraItem; uint32_t totalSize = i.ReadU32 (); From e20525cad9089e2033ca67b34ea5e5d628d9c653 Mon Sep 17 00:00:00 2001 From: Mathieu Lacage Date: Sat, 31 May 2008 10:47:24 -0700 Subject: [PATCH 2/6] add small comment --- src/common/packet-metadata.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/common/packet-metadata.cc b/src/common/packet-metadata.cc index 0c5345415..a9d1c041e 100644 --- a/src/common/packet-metadata.cc +++ b/src/common/packet-metadata.cc @@ -496,6 +496,11 @@ PacketMetadata::ReplaceTail (PacketMetadata::SmallItem *item, available); NS_ASSERT (m_data != 0); + /* If the tail we want to replace is located at the end of the data array, + * and if there is extra room at the end of this array, then, + * we can try to use that extra space to avoid falling in the slow + * path below. + */ if (m_tail + available == m_used && m_used == m_data->m_dirtyEnd) { From a5c312cb1ceb669fc3136965e363c37ba59efe0c Mon Sep 17 00:00:00 2001 From: Mathieu Lacage Date: Sat, 31 May 2008 10:57:49 -0700 Subject: [PATCH 3/6] some more cleanup --- src/common/packet-metadata.cc | 134 ++++++---------------------------- src/common/packet-metadata.h | 3 - 2 files changed, 22 insertions(+), 115 deletions(-) diff --git a/src/common/packet-metadata.cc b/src/common/packet-metadata.cc index a9d1c041e..7481f5aa3 100644 --- a/src/common/packet-metadata.cc +++ b/src/common/packet-metadata.cc @@ -186,101 +186,6 @@ PacketMetadata::Append32 (uint32_t value, uint8_t *buffer) buffer[2] = (value >> 16) & 0xff; buffer[3] = (value >> 24) & 0xff; } -bool -PacketMetadata::TryToAppend16 (uint16_t value, uint8_t **pBuffer, uint8_t *end) -{ - uint8_t *start = *pBuffer; - if (start + 1 < end) - { - start[0] = value & 0xff; - start[1] = value >> 8; - *pBuffer = start + 2; - return true; - } - return false; -} -bool -PacketMetadata::TryToAppend32 (uint32_t value, uint8_t **pBuffer, uint8_t *end) -{ - uint8_t *start = *pBuffer; - if (start + 3 < end) - { - start[0] = value & 0xff; - start[1] = (value >> 8) & 0xff; - start[2] = (value >> 16) & 0xff; - start[3] = (value >> 24) & 0xff; - *pBuffer = start + 4; - return true; - } - return false; -} -bool -PacketMetadata::TryToAppend (uint32_t value, uint8_t **pBuffer, uint8_t *end) -{ - uint8_t *start = *pBuffer; - if (value < 0x80 && start < end) - { - start[0] = value; - *pBuffer = start + 1; - return true; - } - if (value < 0x4000 && start + 1 < end) - { - uint8_t byte = value & (~0x80); - start[0] = 0x80 | byte; - value >>= 7; - start[1] = value; - *pBuffer = start + 2; - return true; - } - if (value < 0x200000 && start + 2 < end) - { - uint8_t byte = value & (~0x80); - start[0] = 0x80 | byte; - value >>= 7; - byte = value & (~0x80); - start[1] = 0x80 | byte; - value >>= 7; - byte = value & (~0x80); - start[2] = value; - *pBuffer = start + 3; - return true; - } - if (value < 0x10000000 && start + 3 < end) - { - uint8_t byte = value & (~0x80); - start[0] = 0x80 | byte; - value >>= 7; - byte = value & (~0x80); - start[1] = 0x80 | byte; - value >>= 7; - byte = value & (~0x80); - start[2] = 0x80 | byte; - value >>= 7; - start[3] = value; - *pBuffer = start + 4; - return true; - } - if (start + 4 < end) - { - uint8_t byte = value & (~0x80); - start[0] = 0x80 | byte; - value >>= 7; - byte = value & (~0x80); - start[1] = 0x80 | byte; - value >>= 7; - byte = value & (~0x80); - start[2] = 0x80 | byte; - value >>= 7; - byte = value & (~0x80); - start[3] = 0x80 | byte; - value >>= 7; - start[4] = value; - *pBuffer = start + 5; - return true; - } - return false; -} void PacketMetadata::AppendValueExtra (uint32_t value, uint8_t *buffer) @@ -481,9 +386,6 @@ PacketMetadata::AddBig (uint32_t next, uint32_t prev, * \param extraItem the extra item data to write * \param available the number of bytes which can * be written without having to rewrite the buffer entirely. - * - * XXX: should rewrite the code below to avoid using - * TryToAppend calls. */ void PacketMetadata::ReplaceTail (PacketMetadata::SmallItem *item, @@ -507,27 +409,35 @@ PacketMetadata::ReplaceTail (PacketMetadata::SmallItem *item, available = m_data->m_size - m_tail; } - if (available >= 14 && + uint32_t typeUid = ((item->typeUid & 0x1) == 0x1)?item->typeUid:item->typeUid+1; + uint32_t typeUidSize = GetUleb128Size (typeUid); + uint32_t sizeSize = GetUleb128Size (item->size); + uint32_t fragStartSize = GetUleb128Size (extraItem->fragmentStart); + uint32_t fragEndSize = GetUleb128Size (extraItem->fragmentEnd); + uint32_t n = 2 + 2 + typeUidSize + sizeSize + 2 + fragStartSize + fragEndSize + 4; + + if (available >= n && m_data->m_count == 1) { uint8_t *buffer = &m_data->m_data[m_tail]; - uint8_t *end = buffer + available; - Append16 (item->next, buffer); buffer += 2; Append16 (item->prev, buffer); buffer += 2; - if (TryToAppend (item->typeUid, &buffer, end) && - TryToAppend (item->size, &buffer, end) && - TryToAppend16 (item->chunkUid, &buffer, end) && - TryToAppend (extraItem->fragmentStart, &buffer, end) && - TryToAppend (extraItem->fragmentEnd, &buffer, end) && - TryToAppend32 (extraItem->packetUid, &buffer, end)) - { - m_used = buffer - &m_data->m_data[0]; - m_data->m_dirtyEnd = m_used; - return; - } + AppendValue (typeUid, buffer); + buffer += typeUidSize; + AppendValue (item->size, buffer); + buffer += sizeSize; + Append16 (item->chunkUid, buffer); + buffer += 2; + AppendValue (extraItem->fragmentStart, buffer); + buffer += fragStartSize; + AppendValue (extraItem->fragmentEnd, buffer); + buffer += fragEndSize; + Append32 (extraItem->packetUid, buffer); + m_used = buffer - &m_data->m_data[0]; + m_data->m_dirtyEnd = m_used; + return; } // create a copy of the packet. diff --git a/src/common/packet-metadata.h b/src/common/packet-metadata.h index b5a2096c0..6358cb7de 100644 --- a/src/common/packet-metadata.h +++ b/src/common/packet-metadata.h @@ -254,9 +254,6 @@ private: uint32_t ReadUleb128 (const uint8_t **pBuffer) const; inline void Append16 (uint16_t value, uint8_t *buffer); inline void Append32 (uint32_t value, uint8_t *buffer); - inline bool TryToAppend (uint32_t value, uint8_t **pBuffer, uint8_t *end); - inline bool TryToAppend32 (uint32_t value, uint8_t **pBuffer, uint8_t *end); - inline bool TryToAppend16 (uint16_t value, uint8_t **pBuffer, uint8_t *end); inline void AppendValue (uint32_t value, uint8_t *buffer); void AppendValueExtra (uint32_t value, uint8_t *buffer); inline void Reserve (uint32_t n); From f08ea71825e1046e4f0002c51ecee1d5422c926e Mon Sep 17 00:00:00 2001 From: Mathieu Lacage Date: Sat, 31 May 2008 11:00:01 -0700 Subject: [PATCH 4/6] add small comment --- src/common/packet-metadata.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/common/packet-metadata.cc b/src/common/packet-metadata.cc index 7481f5aa3..1c4ec5d59 100644 --- a/src/common/packet-metadata.cc +++ b/src/common/packet-metadata.cc @@ -439,6 +439,10 @@ PacketMetadata::ReplaceTail (PacketMetadata::SmallItem *item, m_data->m_dirtyEnd = m_used; return; } + + /* Below is the slow path which is hit if the new tail we want + * to append is bigger than the previous tail. + */ // create a copy of the packet. PacketMetadata h (m_packetUid, 0); From 92ebbd036dc6dcd65d5c887baec2c975ace05659 Mon Sep 17 00:00:00 2001 From: Mathieu Lacage Date: Sat, 31 May 2008 14:12:24 -0700 Subject: [PATCH 5/6] a testcase for bug 197 --- src/common/packet-metadata-test.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/common/packet-metadata-test.cc b/src/common/packet-metadata-test.cc index 7748542b2..1b4704ab6 100644 --- a/src/common/packet-metadata-test.cc +++ b/src/common/packet-metadata-test.cc @@ -746,7 +746,11 @@ PacketMetadataTest::RunTests (void) CHECK_HISTORY (p, 3, 1, 1000, 3); CHECK_HISTORY (p1, 3, 2, 1000, 3); - + p = Create (200); + ADD_HEADER (p, 24); + p1 = p->CreateFragment(0, 100); + p2 = p->CreateFragment(100, 100); + p1->AddAtEnd (p2); return result; } From add091ea239ab98e1863c408fd6a8a9bd0515848 Mon Sep 17 00:00:00 2001 From: Mathieu Lacage Date: Sat, 31 May 2008 14:12:40 -0700 Subject: [PATCH 6/6] bug197: fix infinite loop --- src/common/packet-metadata.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/common/packet-metadata.cc b/src/common/packet-metadata.cc index 1c4ec5d59..dbbabda23 100644 --- a/src/common/packet-metadata.cc +++ b/src/common/packet-metadata.cc @@ -455,6 +455,7 @@ PacketMetadata::ReplaceTail (PacketMetadata::SmallItem *item, uint16_t written = h.AddBig (0xffff, h.m_tail, &tmpItem, &tmpExtraItem); h.UpdateTail (written); + current = tmpItem.next; } // append new tail. uint16_t written = h.AddBig (0xffff, h.m_tail, item, extraItem);