diff --git a/src/openlcb/AliasAllocator.cxx b/src/openlcb/AliasAllocator.cxx index 17f386545..526e5c345 100644 --- a/src/openlcb/AliasAllocator.cxx +++ b/src/openlcb/AliasAllocator.cxx @@ -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(); } @@ -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); } diff --git a/src/openlcb/AliasAllocator.cxxtest b/src/openlcb/AliasAllocator.cxxtest index faef26b0d..67a284dcd 100644 --- a/src/openlcb/AliasAllocator.cxxtest +++ b/src/openlcb/AliasAllocator.cxxtest @@ -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 { @@ -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))); diff --git a/src/openlcb/AliasCache.cxx b/src/openlcb/AliasCache.cxx index 32400b2e8..cb4585603 100644 --- a/src/openlcb/AliasCache.cxx +++ b/src/openlcb/AliasCache.cxx @@ -33,14 +33,192 @@ #include "openlcb/AliasCache.hxx" +#include + #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 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() { @@ -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()) { @@ -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 @@ -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) @@ -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 } } diff --git a/src/openlcb/AliasCache.cxxtest b/src/openlcb/AliasCache.cxxtest index 6a648a242..f2f0ca637 100644 --- a/src/openlcb/AliasCache.cxxtest +++ b/src/openlcb/AliasCache.cxxtest @@ -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 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) { diff --git a/src/openlcb/AliasCache.hxx b/src/openlcb/AliasCache.hxx index fdbafbe57..69fe1a5d8 100644 --- a/src/openlcb/AliasCache.hxx +++ b/src/openlcb/AliasCache.hxx @@ -109,8 +109,6 @@ public: clear(); } - /** This NodeID will be used for reserved but unused local aliases. */ - static const NodeID RESERVED_ALIAS_NODE_ID; /// Sentinel entry for empty lists. static constexpr uint16_t NONE_ENTRY = 0xFFFFu; diff --git a/src/openlcb/CanDefs.hxx b/src/openlcb/CanDefs.hxx index 08749f49a..9c230e3db 100644 --- a/src/openlcb/CanDefs.hxx +++ b/src/openlcb/CanDefs.hxx @@ -136,6 +136,18 @@ struct CanDefs { NOT_LAST_FRAME = 0x10, }; + /// Constants used in the LocalAliasCache for reserved but not used + /// aliases. + enum ReservedAliasNodeId + { + /// To mark a reserved alias in the local alias cache, we use this as a + /// node ID and add the alias to the lowest 12 bits. Since this value + /// starts with a zero MSB byte, it is not a valid node ID. + RESERVED_ALIAS_NODE_BITS = 0xF000, + /// Mask for the reserved aliases. + RESERVED_ALIAS_NODE_MASK = 0xFFFFFFFFF000 + }; + /** Get the source field value of the CAN ID. * @param can_id identifier to act upon * @return source field @@ -363,6 +375,23 @@ struct CanDefs { frame.can_dlc = 0; } + /** Computes a reserved alias node ID for the local alias cache map. + * @param alias the alias to reserve + * @return Node ID to use in the alias map as a key. + */ + static NodeID get_reserved_alias_node_id(NodeAlias alias) + { + return RESERVED_ALIAS_NODE_BITS | alias; + } + + /** Tests if a node ID is a reserved alias Node ID. + * @param id node id to test + * @return true if this is a reserved alias node ID. */ + static bool is_reserved_alias_node_id(NodeID id) + { + return (id & RESERVED_ALIAS_NODE_MASK) == RESERVED_ALIAS_NODE_BITS; + } + private: /** This class should not be instantiated. */ CanDefs(); diff --git a/src/openlcb/IfCan.cxx b/src/openlcb/IfCan.cxx index 121c8ddd3..ad68b5e04 100644 --- a/src/openlcb/IfCan.cxx +++ b/src/openlcb/IfCan.cxx @@ -718,7 +718,7 @@ void IfCan::delete_local_node(Node *node) { if (alias) { // The node had a local alias. localAliases_.remove(alias); - localAliases_.add(AliasCache::RESERVED_ALIAS_NODE_ID, alias); + localAliases_.add(CanDefs::get_reserved_alias_node_id(alias), alias); // Sends AMR & returns alias to pool. aliasAllocator_->return_alias(node->node_id(), alias); } diff --git a/src/openlcb/IfCan.cxxtest b/src/openlcb/IfCan.cxxtest index fc1d5af65..42e66c3bc 100644 --- a/src/openlcb/IfCan.cxxtest +++ b/src/openlcb/IfCan.cxxtest @@ -1,5 +1,6 @@ #include "utils/async_if_test_helper.hxx" +#include "openlcb/CanDefs.hxx" #include "openlcb/WriteHelper.hxx" namespace openlcb @@ -417,13 +418,13 @@ TEST_F(AsyncMessageCanTests, ReservedAliasReclaimed) usleep(250000); wait(); // Here we have the next reserved alias. - RX(EXPECT_EQ(AliasCache::RESERVED_ALIAS_NODE_ID, + RX(EXPECT_EQ(CanDefs::get_reserved_alias_node_id(0x44C), ifCan_->local_aliases()->lookup(NodeAlias(0x44C)))); // A CID packet gets replied to. send_packet_and_expect_response(":X1478944CN;", ":X1070044CN;"); // We still have it in the cache. - RX(EXPECT_EQ(AliasCache::RESERVED_ALIAS_NODE_ID, + RX(EXPECT_EQ(CanDefs::get_reserved_alias_node_id(0x44C), ifCan_->local_aliases()->lookup(NodeAlias(0x44C)))); // We kick it out with a regular frame. send_packet(":X1800044CN;"); @@ -449,7 +450,7 @@ TEST_F(AsyncMessageCanTests, ReservedAliasReclaimed) RX(EXPECT_EQ( TEST_NODE_ID + 1, ifCan_->local_aliases()->lookup(NodeAlias(0x44D)))); usleep(250000); - RX(EXPECT_EQ(AliasCache::RESERVED_ALIAS_NODE_ID, + RX(EXPECT_EQ(CanDefs::get_reserved_alias_node_id(0x6AA), ifCan_->local_aliases()->lookup(NodeAlias(0x6AA)))); } diff --git a/src/openlcb/IfCanImpl.hxx b/src/openlcb/IfCanImpl.hxx index bd3129ca0..ccb8282c6 100644 --- a/src/openlcb/IfCanImpl.hxx +++ b/src/openlcb/IfCanImpl.hxx @@ -137,8 +137,8 @@ private: nmsg()->src.id); // Checks that there was no conflict on this alias. - if (if_can()->local_aliases()->lookup(alias) != - AliasCache::RESERVED_ALIAS_NODE_ID) + if (!CanDefs::is_reserved_alias_node_id( + if_can()->local_aliases()->lookup(alias))) { LOG(INFO, "Alias has seen conflict: %03X", alias); // Problem. Let's take another alias.