Skip to content

Commit

Permalink
Fixes consistency issues in the AliasCache. (#433)
Browse files Browse the repository at this point in the history 
Fixes consistency issues in the AliasCache.
- Makes sure that when the AliasCache adds a new mapping for (id, alias)
  then the id also gets checked in idMap, not just the alias in aliasMap.
  This avoids the situation where two different alias might be registered
  for the same ID.
- Runs the Alias Cache consistency test after each modification
  when running under GTEST.
- Removes RESERVED_ALIAS_NODE_ID because that created exactly this
  situation.
- Adds helper functions to generate a reserved alias node ID that is
  different for each alias, by using a range of invalid Node IDs.
- Updates existing code and tests that were depending on
  RESERVED_ALIAS_NODE_ID.
- Adds missing braces to the consistency check function.

===

* Fixes consistency issues in the AliasCache.

- Makes sure that when the AliasCache adds a new mapping for (id, alias)
  then the id also gets checked in idMap, not just the alias in aliasMap.
  This avoids the situation where two different alias might be registered
  for the same ID.
- Runs the Alias Cache consistency test after each modification
  when running under GTEST.
- Removes RESERVED_ALIAS_NODE_ID because that created exactly this
  situation.
- Adds helper functions to generate a reserved alias node ID that is
  different for each alias, by using a range of invalid Node IDs.
- Updates existing code and tests that were depending on
  RESERVED_ALIAS_NODE_ID.

* Fix whitespace

* Adds missing braces.
  • Loading branch information
@balazsracz
balazsracz authored Sep 17, 2020
1 parent e1b0c2d commit 1060956
Show file tree
Hide file tree
Showing 9 changed files with 249 additions and 112 deletions.
8 changes: 5 additions & 3 deletions src/openlcb/AliasAllocator.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,9 @@ StateFlowBase::Action AliasAllocator::send_rid_frame()
pending_alias()->state = AliasInfo::STATE_RESERVED;
if_can()->frame_dispatcher()->unregister_handler(
&conflictHandler_, pending_alias()->alias, ~0x1FFFF000U);
if_can()->local_aliases()->add(AliasCache::RESERVED_ALIAS_NODE_ID,
pending_alias()->alias);
if_can()->local_aliases()->add(
CanDefs::get_reserved_alias_node_id(pending_alias()->alias),
pending_alias()->alias);
reserved_alias_pool_.insert(transfer_message());
return release_and_exit();
}
Expand Down Expand Up @@ -259,7 +260,8 @@ void AliasAllocator::TEST_add_allocated_alias(NodeAlias alias, bool repeat)
a->data()->do_not_reallocate();
}
if_can()->local_aliases()->add(
AliasCache::RESERVED_ALIAS_NODE_ID, a->data()->alias);
CanDefs::get_reserved_alias_node_id(a->data()->alias),
a->data()->alias);
reserved_aliases()->insert(a);
}

Expand Down
3 changes: 2 additions & 1 deletion src/openlcb/AliasAllocator.cxxtest
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include "utils/async_if_test_helper.hxx"
#include "openlcb/AliasAllocator.hxx"
#include "openlcb/AliasCache.hxx"
#include "openlcb/CanDefs.hxx"

namespace openlcb
{
Expand Down Expand Up @@ -156,7 +157,7 @@ TEST_F(AsyncAliasAllocatorTest, AllocationConflict)
EXPECT_EQ(AliasInfo::STATE_RESERVED, b_->data()->state);
run_x([this]() {
// This one should be marked as reserved.
EXPECT_EQ(AliasCache::RESERVED_ALIAS_NODE_ID,
EXPECT_EQ(CanDefs::get_reserved_alias_node_id(0xAA5),
ifCan_->local_aliases()->lookup(NodeAlias(0xAA5)));
// This one should be unknown.
EXPECT_EQ(0U, ifCan_->local_aliases()->lookup(NodeAlias(0x555)));
Expand Down
208 changes: 206 additions & 2 deletions src/openlcb/AliasCache.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,192 @@

#include "openlcb/AliasCache.hxx"

#include <set>

#include "os/OS.hxx"

#ifdef GTEST
#define TEST_CONSISTENCY
#endif

namespace openlcb
{

#define CONSTANT 0x1B0CA37ABA9 /**< constant for random number generation */

const NodeID AliasCache::RESERVED_ALIAS_NODE_ID = 1;
#if defined(TEST_CONSISTENCY)
extern volatile int consistency_result;
volatile int consistency_result = 0;

int AliasCache::check_consistency()
{
if (idMap.size() != aliasMap.size())
{
return 1;
}
if (aliasMap.size() == entries)
{
if (!freeList.empty())
{
return 2;
}
}
else
{
if (freeList.empty())
{
return 3;
}
}
if (aliasMap.size() == 0 && (!oldest.empty() || !newest.empty()))
{
return 4;
}
std::set<void *> free_entries;
for (PoolIdx p = freeList; !p.empty(); p = p.deref(this)->older_)
{
Metadata *m = p.deref(this);
if (free_entries.count(m))
{
return 5; // duplicate entry on freelist
}
free_entries.insert(m);
}
if (free_entries.size() + aliasMap.size() != entries)
{
return 6; // lost some metadata entries
}
for (auto kv : aliasMap)
{
if (free_entries.count(kv.deref(this)))
{
return 19;
}
}
for (auto kv : idMap)
{
if (free_entries.count(kv.deref(this)))
{
return 20;
}
}
if (aliasMap.size() == 0)
{
if (!oldest.empty())
{
return 7;
}
if (!newest.empty())
{
return 8;
}
}
else
{
if (oldest.empty())
{
return 9;
}
if (newest.empty())
{
return 10;
}
if (free_entries.count(oldest.deref(this)))
{
return 11; // oldest is free
}
if (free_entries.count(newest.deref(this)))
{
return 12; // newest is free
}
}
if (aliasMap.size() == 0)
{
return 0;
}
// Check linking.
{
PoolIdx prev = oldest;
unsigned count = 1;
if (!prev.deref(this)->older_.empty())
{
return 13;
}
while (!prev.deref(this)->newer_.empty())
{
auto next = prev.deref(this)->newer_;
++count;
if (free_entries.count(next.deref(this)))
{
return 21;
}
if (next.deref(this)->older_.idx_ != prev.idx_)
{
return 14;
}
prev = next;
}
if (prev.idx_ != newest.idx_)
{
return 18;
}
if (count != aliasMap.size())
{
return 27;
}
}
{
PoolIdx next = newest;
if (!next.deref(this)->newer_.empty())
{
return 15;
}
while (!next.deref(this)->older_.empty())
{
auto prev = next.deref(this)->older_;
if (free_entries.count(prev.deref(this)))
{
return 22;
}
if (prev.deref(this)->newer_.idx_ != next.idx_)
{
return 16;
}
next = prev;
}
if (next.idx_ != oldest.idx_)
{
return 17;
}
}
for (unsigned i = 0; i < entries; ++i)
{
if (free_entries.count(pool + i))
{
continue;
}
auto *e = pool + i;
if (idMap.find(e->get_node_id()) == idMap.end())
{
return 23;
}
if (idMap.find(e->get_node_id())->idx_ != i)
{
return 24;
}
if (aliasMap.find(e->alias_) == aliasMap.end())
{
return 25;
}
if (aliasMap.find(e->alias_)->idx_ != i)
{
return 26;
}
}
return 0;
}

#endif

void AliasCache::clear()
{
Expand Down Expand Up @@ -82,6 +260,19 @@ void AliasCache::add(NodeID id, NodeAlias alias)
(*removeCallback)(insert->get_node_id(), insert->alias_, context);
}
}
auto nit = idMap.find(id);
if (nit != idMap.end())
{
/* we already have a mapping for this id, so lets remove it */
insert = nit->deref(this);
remove(insert->alias_);

if (removeCallback)
{
/* tell the interface layer that we removed this mapping */
(*removeCallback)(insert->get_node_id(), insert->alias_, context);
}
}

if (!freeList.empty())
{
Expand Down Expand Up @@ -141,6 +332,11 @@ void AliasCache::add(NodeID id, NodeAlias alias)
}

newest = n;

#if defined(TEST_CONSISTENCY)
consistency_result = check_consistency();
HASSERT(0 == consistency_result);
#endif
}

/** Remove an alias from an alias cache. This method does not call the
Expand Down Expand Up @@ -178,7 +374,11 @@ void AliasCache::remove(NodeAlias alias)
metadata->older_ = freeList;
freeList.idx_ = metadata - pool;
}


#if defined(TEST_CONSISTENCY)
consistency_result = check_consistency();
HASSERT(0 == consistency_result);
#endif
}

bool AliasCache::retrieve(unsigned entry, NodeID* node, NodeAlias* alias)
Expand Down Expand Up @@ -296,6 +496,10 @@ void AliasCache::touch(Metadata* metadata)
newest.deref(this)->newer_.idx_ = metadata - pool;
newest.idx_ = metadata - pool;
}
#if defined(TEST_CONSISTENCY)
consistency_result = check_consistency();
HASSERT(0 == consistency_result);
#endif
}

}
98 changes: 0 additions & 98 deletions src/openlcb/AliasCache.cxxtest
View file
Original file line number Diff line number Diff line change
Expand Up @@ -463,104 +463,6 @@ protected:
AliasCache c_{get_id(0x33), 10};
};

namespace openlcb {
int AliasCache::check_consistency() {
if (idMap.size() != aliasMap.size()) return 1;
if (aliasMap.size() == entries) {
if (!freeList.empty()) return 2;
} else {
if (freeList.empty()) return 3;
}
if (aliasMap.size() == 0 && (!oldest.empty() || !newest.empty()))
{
return 4;
}
std::set<void*> free_entries;
for (PoolIdx p = freeList; !p.empty(); p = p.deref(this)->older_)
{
Metadata *m = p.deref(this);
if (free_entries.count(m)) {
return 5; // duplicate entry on freelist
}
free_entries.insert(m);
}
if (free_entries.size() + aliasMap.size() != entries) {
return 6; // lost some metadata entries
}
for (auto kv : aliasMap) {
if (free_entries.count(kv.deref(this)))
{
return 19;
}
}
for (auto kv : idMap) {
if (free_entries.count(kv.deref(this)))
{
return 20;
}
}
if (aliasMap.size() == 0) {
if (!oldest.empty()) return 7;
if (!newest.empty()) return 8;
} else {
if (oldest.empty()) return 9;
if (newest.empty()) return 10;
}
if (free_entries.count(oldest.deref(this)))
{
return 11; // oldest is free
}
if (free_entries.count(newest.deref(this)))
{
return 12; // newest is free
}
if (aliasMap.size() == 0) return 0;
// Check linking.
{
PoolIdx prev = oldest;
unsigned count = 1;
if (!prev.deref(this)->older_.empty()) return 13;
while (!prev.deref(this)->newer_.empty())
{
auto next = prev.deref(this)->newer_;
++count;
if (free_entries.count(next.deref(this)))
{
return 21;
}
if (next.deref(this)->older_.idx_ != prev.idx_) return 14;
prev = next;
}
if (prev.idx_ != newest.idx_) return 18;
if (count != aliasMap.size()) return 27;
}
{
PoolIdx next = newest;
if (!next.deref(this)->newer_.empty()) return 15;
while (!next.deref(this)->older_.empty())
{
auto prev = next.deref(this)->older_;
if (free_entries.count(prev.deref(this)))
{
return 22;
}
if (prev.deref(this)->newer_.idx_ != next.idx_) return 16;
next = prev;
}
if (next.idx_ != oldest.idx_) return 17;
}
for (unsigned i = 0; i < entries; ++i) {
if (free_entries.count(pool+i)) continue;
auto* e = pool+i;
if (idMap.find(e->get_node_id()) == idMap.end()) return 23;
if (idMap.find(e->get_node_id())->idx_ != i) return 24;
if (aliasMap.find(e->alias_) == aliasMap.end()) return 25;
if (aliasMap.find(e->alias_)->idx_ != i) return 26;
}
return 0;
}

}

TEST_F(AliasStressTest, stress_test)
{
Expand Down
Loading

0 comments on commit 1060956

Please sign in to comment.