From 5a795ad5d4097a8328927eac242b7b7b6ca548ff Mon Sep 17 00:00:00 2001 From: Jacob Hageman Date: Wed, 21 Oct 2020 10:02:00 -0400 Subject: [PATCH] Fix #929, Add message map has implementation - Message map size based on used routes - Oversized (4x) to limit collisions while retaining resonable size related to routing table (still smaller) - ~10% single collisions seen for full routing table with realistic message ID use - Oversizing means map can never fill, simplifies logic - Observed approximately 10%-20% performance hit, trade against memory use (can now use full 32 bit MsgId space) - Hash intended for 32 bit, if CFE_SB_MsgId_Atom_t size changes may require modification to hash - Also added full coverage unit tests --- modules/sbr/src/cfe_sbr_map_hash.c | 163 ++++++++++++++++++ .../test_cfe_sbr_map_hash.c | 118 +++++++++++++ 2 files changed, 281 insertions(+) create mode 100644 modules/sbr/src/cfe_sbr_map_hash.c create mode 100644 modules/sbr/unit-test-coverage/test_cfe_sbr_map_hash.c diff --git a/modules/sbr/src/cfe_sbr_map_hash.c b/modules/sbr/src/cfe_sbr_map_hash.c new file mode 100644 index 000000000..dd9b8bd5c --- /dev/null +++ b/modules/sbr/src/cfe_sbr_map_hash.c @@ -0,0 +1,163 @@ +/* +** GSC-18128-1, "Core Flight Executive Version 6.7" +** +** Copyright (c) 2006-2019 United States Government as represented by +** the Administrator of the National Aeronautics and Space Administration. +** All Rights Reserved. +** +** Licensed under the Apache License, Version 2.0 (the "License"); +** you may not use this file except in compliance with the License. +** You may obtain a copy of the License at +** +** http://www.apache.org/licenses/LICENSE-2.0 +** +** Unless required by applicable law or agreed to in writing, software +** distributed under the License is distributed on an "AS IS" BASIS, +** WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +** See the License for the specific language governing permissions and +** limitations under the License. +*/ + +/****************************************************************************** + * Hash routing map implementation + * + * Notes: + * These functions manipulate/access global variables and need + * to be protected by the SB Shared data lock. + * + */ + +/* + * Include Files + */ + +#include "common_types.h" +#include "private/cfe_sbr.h" +#include "cfe_sbr_priv.h" +#include +#include + +/* + * Macro Definitions + */ + +/** + * \brief Message map size + * + * For hash mapping, map size is a multiple of maximum number of routes. + * The multiple impacts the number of collisions when the routes fill up. + * 4 was initially chosen to provide for plenty of holes in the map, while + * still remaining much smaller than the routing table. Note the + * multiple must be a factor of 2 to use the efficient shift logic, and + * can't be bigger than what can be indexed by CFE_SB_MsgId_Atom_t + */ +#define CFE_SBR_MSG_MAP_SIZE (4 * CFE_PLATFORM_SB_MAX_MSG_IDS) + +/* Verify power of two */ +#if ((CFE_SBR_MSG_MAP_SIZE & (CFE_SBR_MSG_MAP_SIZE - 1)) != 0) +#error CFE_SBR_MSG_MAP_SIZE must be a power of 2 for hash algorithm to work +#endif + +/** \brief Hash algorithm magic number + * + * Ref: + * https://stackoverflow.com/questions/664014/what-integer-hash-function-are-good-that-accepts-an-integer-hash-key/12996028#12996028 + */ +#define CFE_SBR_HASH_MAGIC (0x45d9f3b) + +/****************************************************************************** + * Shared data + */ + +/** \brief Message map shared data */ +CFE_SBR_RouteId_t CFE_SBR_MSGMAP[CFE_SBR_MSG_MAP_SIZE]; + +/****************************************************************************** + * Internal helper function to hash the message id + * + * Note: algorithm designed for a 32 bit int, changing the size of + * CFE_SB_MsgId_Atom_t may require an update to this impelementation + */ +CFE_SB_MsgId_Atom_t CFE_SBR_MsgIdHash(CFE_SB_MsgId_t MsgId) +{ + CFE_SB_MsgId_Atom_t hash; + + hash = CFE_SB_MsgIdToValue(MsgId); + + hash = ((hash >> 16) ^ hash) * CFE_SBR_HASH_MAGIC; + hash = ((hash >> 16) ^ hash) * CFE_SBR_HASH_MAGIC; + hash = (hash >> 16) ^ hash; + + /* Reduce to fit in map */ + hash &= CFE_SBR_MSG_MAP_SIZE - 1; + + return hash; +} + +/****************************************************************************** + * Interface function - see header for description + */ +void CFE_SBR_Init_Map(void) +{ + /* Clear the shared data */ + memset(&CFE_SBR_MSGMAP, 0, sizeof(CFE_SBR_MSGMAP)); +} + +/****************************************************************************** + * Interface function - see header for description + */ +uint32 CFE_SBR_SetRouteId(CFE_SB_MsgId_t MsgId, CFE_SBR_RouteId_t RouteId) +{ + CFE_SB_MsgId_Atom_t hash; + uint32 collisions = 0; + + if (CFE_SB_IsValidMsgId(MsgId)) + { + hash = CFE_SBR_MsgIdHash(MsgId); + + /* + * Increment from original hash to find the next open slot. + * Since map is larger than possible routes this will + * never deadlock + */ + while (CFE_SBR_IsValidRouteId(CFE_SBR_MSGMAP[hash])) + { + /* Increment or loop to start of array */ + hash = (hash + 1) & (CFE_SBR_MSG_MAP_SIZE - 1); + collisions++; + } + + CFE_SBR_MSGMAP[hash] = RouteId; + } + + return collisions; +} + +/****************************************************************************** + * Interface function - see API for description + */ +CFE_SBR_RouteId_t CFE_SBR_GetRouteId(CFE_SB_MsgId_t MsgId) +{ + CFE_SB_MsgId_Atom_t hash; + CFE_SBR_RouteId_t routeid = CFE_SBR_INVALID_ROUTE_ID; + + if (CFE_SB_IsValidMsgId(MsgId)) + { + hash = CFE_SBR_MsgIdHash(MsgId); + routeid = CFE_SBR_MSGMAP[hash]; + + /* + * Increment from original hash to find matching route. + * Since map is larger than possible routes this will + * never deadlock + */ + while (CFE_SBR_IsValidRouteId(routeid) && !CFE_SB_MsgId_Equal(CFE_SBR_GetMsgId(routeid), MsgId)) + { + /* Increment or loop to start of array */ + hash = (hash + 1) & (CFE_SBR_MSG_MAP_SIZE - 1); + routeid = CFE_SBR_MSGMAP[hash]; + } + } + + return routeid; +} diff --git a/modules/sbr/unit-test-coverage/test_cfe_sbr_map_hash.c b/modules/sbr/unit-test-coverage/test_cfe_sbr_map_hash.c new file mode 100644 index 000000000..6e93d257b --- /dev/null +++ b/modules/sbr/unit-test-coverage/test_cfe_sbr_map_hash.c @@ -0,0 +1,118 @@ +/* +** GSC-18128-1, "Core Flight Executive Version 6.7" +** +** Copyright (c) 2006-2019 United States Government as represented by +** the Administrator of the National Aeronautics and Space Administration. +** All Rights Reserved. +** +** Licensed under the Apache License, Version 2.0 (the "License"); +** you may not use this file except in compliance with the License. +** You may obtain a copy of the License at +** +** http://www.apache.org/licenses/LICENSE-2.0 +** +** Unless required by applicable law or agreed to in writing, software +** distributed under the License is distributed on an "AS IS" BASIS, +** WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +** See the License for the specific language governing permissions and +** limitations under the License. +*/ + +/* + * Test SBR direct message map implementation + */ + +/* + * Includes + */ +#include "utassert.h" +#include "ut_support.h" +#include "private/cfe_sbr.h" +#include "cfe_sbr_priv.h" + +/* + * Defines + */ + +/* Unhash magic number */ +#define CFE_SBR_UNHASH_MAGIC (0x119de1f3) + +/****************************************************************************** + * Local helper to unhash + */ +CFE_SB_MsgId_t Test_SBR_Unhash(CFE_SB_MsgId_Atom_t Hash) +{ + + Hash = ((Hash >> 16) ^ Hash) * CFE_SBR_UNHASH_MAGIC; + Hash = ((Hash >> 16) ^ Hash) * CFE_SBR_UNHASH_MAGIC; + Hash = (Hash >> 16) ^ Hash; + + return CFE_SB_ValueToMsgId(Hash); +} + +void Test_SBR_Map_Hash(void) +{ + + CFE_SB_MsgId_Atom_t msgidx; + CFE_SBR_RouteId_t routeid[3]; + CFE_SB_MsgId_t msgid[3]; + uint32 count; + uint32 collisions; + + UtPrintf("Invalid msg checks"); + ASSERT_EQ(CFE_SBR_SetRouteId(CFE_SB_ValueToMsgId(0), CFE_SBR_ValueToRouteId(0)), 0); + ASSERT_EQ(CFE_SBR_IsValidRouteId(CFE_SBR_GetRouteId(CFE_SB_ValueToMsgId(0))), false); + + UtPrintf("Initialize routing and map"); + CFE_SBR_Init(); + + /* Force valid msgid responses */ + UT_SetForceFail(UT_KEY(CFE_SB_IsValidMsgId), true); + + UtPrintf("Check that all entries are set invalid"); + count = 0; + for (msgidx = 0; msgidx <= CFE_PLATFORM_SB_HIGHEST_VALID_MSGID; msgidx++) + { + if (!CFE_SBR_IsValidRouteId(CFE_SBR_GetRouteId(CFE_SB_ValueToMsgId(msgidx)))) + { + count++; + } + } + ASSERT_EQ(count, CFE_PLATFORM_SB_HIGHEST_VALID_MSGID + 1); + + /* Note AddRoute required for hash logic to work since it depends on MsgId in routing table */ + UtPrintf("Add routes and check with a rollover and a skip"); + msgid[0] = CFE_SB_ValueToMsgId(0); + msgid[1] = Test_SBR_Unhash(0xFFFFFFFF); + msgid[2] = Test_SBR_Unhash(0x7FFFFFFF); + routeid[0] = CFE_SBR_AddRoute(msgid[0], &collisions); + ASSERT_EQ(collisions, 0); + routeid[1] = CFE_SBR_AddRoute(msgid[1], &collisions); + ASSERT_EQ(collisions, 0); + routeid[2] = CFE_SBR_AddRoute(msgid[2], &collisions); + ASSERT_EQ(collisions, 2); + + ASSERT_EQ(CFE_SBR_RouteIdToValue(CFE_SBR_GetRouteId(msgid[0])), CFE_SBR_RouteIdToValue(routeid[0])); + ASSERT_EQ(CFE_SBR_RouteIdToValue(CFE_SBR_GetRouteId(msgid[1])), CFE_SBR_RouteIdToValue(routeid[1])); + ASSERT_EQ(CFE_SBR_RouteIdToValue(CFE_SBR_GetRouteId(msgid[2])), CFE_SBR_RouteIdToValue(routeid[2])); + + /* Performance check, 0xFFFFFF on 3.2GHz linux box is around 8-9 seconds */ + count = 0; + for (msgidx = 0; msgidx <= 0xFFFF; msgidx++) + { + if (CFE_SBR_IsValidRouteId(CFE_SBR_GetRouteId(CFE_SB_ValueToMsgId(msgidx)))) + { + count++; + } + } + UtPrintf("Valid route id's encountered in performance loop: %u", count); +} + +/* Main unit test routine */ +void UtTest_Setup(void) +{ + UT_Init("map_hash"); + UtPrintf("Software Bus Routing hash map coverage test..."); + + UT_ADD_TEST(Test_SBR_Map_Hash); +}