Skip to content

Commit

Permalink
Go back to guessing instead of trying to try every bestParent change.…
Browse files Browse the repository at this point in the history 
… Try to make guesses cause fewer flappy routes. Track the time each link was last tested, use either the least recently tested link or the node's test time when deciding how old our info about a node is. Move tryExistingNode out of the conditional chain in the janitor, to prevent it from being blocked during bootstrapping (when we're most vulnerable to blackholes anyway). Misc code cleanup.
  • Loading branch information
@Arceliar
Arceliar committed Nov 14, 2014
1 parent c47ec81 commit f3ce288
Show file tree
Hide file tree
Showing 6 changed files with 67 additions and 115 deletions.
25 changes: 14 additions & 11 deletions dht/dhtcore/Janitor.c
View file
Expand Up @@ -212,7 +212,7 @@ static void dhtResponseCallback(struct RouterModule_Promise* promise,

struct Node_Link* link = NodeStore_linkForPath(janitor->nodeStore,
addresses->elems[i].path);
if (link && Node_getBestParent(link->child)) {
if (link) {
// We already know about this path and mill space is precious. Skip it.
continue;
}
Expand Down Expand Up @@ -242,9 +242,9 @@ static void peersResponseCallback(struct RouterModule_Promise* promise,
if (!Bits_memcmp(addresses->elems[i].key, from->key, 32)) { continue; }

struct Node_Link* nl = NodeStore_linkForPath(janitor->nodeStore, addresses->elems[i].path);
if (!nl || !Node_getBestParent(nl->child) || Bits_memcmp(nl->child->address.ip6.bytes,
addresses->elems[i].ip6.bytes,
Address_SEARCH_TARGET_SIZE))
if (!nl || Bits_memcmp(nl->child->address.ip6.bytes,
addresses->elems[i].ip6.bytes,
Address_SEARCH_TARGET_SIZE))
{
struct Node_Two* node = NodeStore_nodeForAddr(janitor->nodeStore,
addresses->elems[i].ip6.bytes);
Expand Down Expand Up @@ -480,14 +480,17 @@ static void getPeersMill(struct Janitor* janitor, struct Address* addr)
static bool tryExistingNode(struct Janitor* janitor)
{
struct Node_Two* worst = NULL;
uint64_t worstTime = 0;
struct Node_Two* node = NodeStore_getNextNode(janitor->nodeStore, NULL);
while (node) {
uint64_t nodeTime = NodeStore_timeSinceLastPing(janitor->nodeStore, node);
if (node == janitor->nodeStore->selfNode) {
// No reason to ping the selfNode.
} else if (node->address.path != UINT64_MAX &&
(!worst || node->timeOfLastPing < worst->timeOfLastPing))
(!worst || nodeTime > worstTime))
{
worst = node;
worstTime = nodeTime;
}
node = NodeStore_getNextNode(janitor->nodeStore, node);
}
Expand Down Expand Up @@ -611,17 +614,19 @@ static void maintanenceCycle(void* vcontext)
} else if (tryNodeMill(janitor)) {
// Try to find a new node.

} else if (Random_uint8(janitor->rand) % 2 && tryExistingNode(janitor))
{
// 50% of the time, try to ping an existing node or find a new one.

} else if (tryRandomLink(janitor)) {
// Ping a random link from a random node.

} else {
Log_debug(janitor->logger, "Could not find anything to do");
}

// Try to ping the existing node we have heard from least recently.
tryExistingNode(janitor);

// Look for better nodes for the dht.
keyspaceMaintenance(janitor);

// random search
Random_bytes(janitor->rand, addr.ip6.bytes, 16);
// Make this a valid address.
Expand All @@ -639,8 +644,6 @@ static void maintanenceCycle(void* vcontext)
checkPeers(janitor, n);
}

keyspaceMaintenance(janitor);

Log_debug(janitor->logger,
"Global Mean Response Time: %u nodes [%d] links [%d]",
RouterModule_globalMeanResponseTime(janitor->routerModule),
Expand Down
1 change: 0 additions & 1 deletion dht/dhtcore/Node.c
View file
Expand Up @@ -52,7 +52,6 @@ void Node_setParentReachAndPath(struct Node_Two* node,
node->bestParent_pvt = bestParent;
node->reach_pvt = reach;
node->address.path = path;
node->timeOfLastPing = 0;
}

bool Node_isOneHopLink(struct Node_Link* link)
Expand Down
13 changes: 5 additions & 8 deletions dht/dhtcore/Node.h
View file
Expand Up @@ -33,8 +33,8 @@ struct Node_Two
*/
uint32_t reach_pvt;

/** Time the node was last pinged, reset on path changes. */
uint64_t timeOfLastPing;
/** Time the node was last pinged, *not* reset on path changes. */
uint64_t timeLastPinged;

/** This is used to mark/sweep nodes in getWorstNode(), it's meaningless otherwise. */
int marked;
Expand Down Expand Up @@ -99,6 +99,9 @@ struct Node_Link
*/
uint32_t linkState;

/** The time this link was last seen carrying traffic. (Currently limited to ping traffic.) */
uint64_t timeLastSeen;

/** The parent of this peer, this is where the root of the RBTree is. */
struct Node_Two* parent;

Expand Down Expand Up @@ -126,12 +129,6 @@ struct Node_Link
/** The path which the incoming packet followed when this node was discovered. */
uint64_t discoveredPath;

/** The best path through this link at the time we last explicitly tested it. */
uint64_t linkPathWhenTested;

/** The best path to the child at the time we last explicitly tested this link. */
uint64_t childPathWhenTested;

Identity
};

Expand Down
127 changes: 44 additions & 83 deletions dht/dhtcore/NodeStore.c
View file
Expand Up @@ -399,6 +399,13 @@ static uint32_t guessReachOfChild(struct Node_Link* link)
r = guess;
}

// Try to reduce oscillation based on guesses.
struct Node_Link* bp = Node_getBestParent(link->child);
if (bp && bp != link) {
uint32_t bpGuess = guessReachOfChild(bp);
if (r > bpGuess) { r = bpGuess; }
}

Assert_true(r < Node_getReach(link->parent) && r != 0);
return r;
}
Expand Down Expand Up @@ -484,38 +491,6 @@ static void updateBestParent(struct Node_Link* newBestParent,
Assert_true(0);
}

// To prevent us from repeatedly testing the same link when paths haven't changed.
// FIXME(arceliar): This is probably not always the right way to decide.
static bool linkNeedsTesting(struct NodeStore_pvt* store, struct Node_Link* link)
{
struct Address addr = link->child->address;
addr.path = NodeStore_getRouteLabel(&store->pub,
link->parent->address.path,
link->cannonicalLabel);
bool addrIsOK = true;
if (addr.path == NodeStore_getRouteLabel_PARENT_NOT_FOUND) { addrIsOK = false; }
if (addr.path == NodeStore_getRouteLabel_CHILD_NOT_FOUND) { addrIsOK = false; }
if (addr.path == NodeStore_optimizePath_INVALID) { addrIsOK = false; }
return (addrIsOK && link->linkPathWhenTested != addr.path) ||
(link->childPathWhenTested != link->child->address.path);
}

// Queues the node at the end of a link to be pinged.
// If the link is better than the current best parent, this should cause it to switch.
static void tryLink(struct NodeStore_pvt* store, struct Node_Link* link)
{
struct Address addr = link->child->address;
addr.path = NodeStore_getRouteLabel(&store->pub,
link->parent->address.path,
link->cannonicalLabel);
if (addr.path == NodeStore_getRouteLabel_PARENT_NOT_FOUND) { return; }
if (addr.path == NodeStore_getRouteLabel_CHILD_NOT_FOUND) { return; }
if (addr.path == NodeStore_optimizePath_INVALID) { return; }
link->linkPathWhenTested = addr.path;
link->childPathWhenTested = link->child->address.path;
RumorMill_addNode(store->renumberMill, &addr);
}

static void handleGoodNews(struct Node_Two* node,
uint32_t newReach,
struct NodeStore_pvt* store)
Expand All @@ -541,8 +516,7 @@ static void handleGoodNews(struct Node_Two* node,
if (!childBestParent || Node_getReach(childBestParent->parent) < newReach) {
uint32_t nextReach = guessReachOfChild(link);
if (Node_getReach(child) > nextReach) { continue; }
if (!linkNeedsTesting(store, link)) { continue; }
tryLink(store, link);
updateBestParent(link, nextReach, store);
}
}
}
Expand All @@ -569,9 +543,7 @@ static void handleBadNewsTwo(struct Node_Link* link, struct NodeStore_pvt* store
struct Node_Link* rp = link->child->reversePeers;
struct Node_Link* best = Node_getBestParent(node);
while (rp) {
if (Node_getReach(rp->parent) >= Node_getReach(best->parent) &&
linkNeedsTesting(store, rp))
{
if (Node_getReach(rp->parent) >= Node_getReach(best->parent)) {
if (Node_getReach(rp->parent) > Node_getReach(best->parent)
|| rp->parent->address.path < best->parent->address.path)
{
Expand All @@ -587,7 +559,9 @@ static void handleBadNewsTwo(struct Node_Link* link, struct NodeStore_pvt* store
if (nextReach <= Node_getReach(node)) { return; }
Assert_true(Node_getReach(node) < Node_getReach(best->parent));

tryLink(store, best);
check(store);
updateBestParent(best, nextReach, store);
check(store);
}

/**
Expand Down Expand Up @@ -801,8 +775,7 @@ static struct Node_Link* linkNodes(struct Node_Two* parent,
link->parent = parent;
link->discoveredPath = discoveredPath;
link->linkState = 0;
link->linkPathWhenTested = discoveredPath;
link->childPathWhenTested = discoveredPath;
link->timeLastSeen = Time_currentTimeMilliseconds(store->eventBase);
Identity_set(link);

// reverse link
Expand Down Expand Up @@ -1486,7 +1459,7 @@ struct Node_Link* NodeStore_discoverNode(struct NodeStore* nodeStore,
child->alloc = alloc;
Bits_memcpyConst(&child->address, addr, sizeof(struct Address));
child->encodingScheme = EncodingScheme_clone(scheme, child->alloc);
child->timeOfLastPing = 0;
child->timeLastPinged = Time_currentTimeMilliseconds(store->eventBase);
Identity_set(child);
}

Expand Down Expand Up @@ -1766,6 +1739,7 @@ struct NodeStore* NodeStore_new(struct Address* myAddress,
out->pub.selfNode = selfNode;
struct Node_Link* selfLink = linkNodes(selfNode, selfNode, 1, 0xffffffffu, 0, 1, out);
Node_setParentReachAndPath(selfNode, selfLink, UINT32_MAX, 1);
selfNode->timeLastPinged = Time_currentTimeMilliseconds(out->eventBase);
out->selfLink = selfLink;
RB_INSERT(NodeRBTree, &out->nodeTree, selfNode);

Expand Down Expand Up @@ -2090,6 +2064,7 @@ static void updatePathReach(struct NodeStore_pvt* store, const uint64_t path, ui
{
struct Node_Link* link = store->selfLink;
uint64_t pathFrag = path;
uint64_t now = Time_currentTimeMilliseconds(store->eventBase);
for (;;) {
struct Node_Link* nextLink = NULL;
uint64_t nextPath = firstHopInPath(pathFrag, &nextLink, link, store);
Expand All @@ -2100,6 +2075,21 @@ static void updatePathReach(struct NodeStore_pvt* store, const uint64_t path, ui
// expecting behavior of nextLinkOnPath()
Assert_ifParanoid(nextLink->parent == link->child);

if (Node_getBestParent(nextLink->child) != nextLink) {
// If nextLink->child->bestParent is worse than nextLink, we should replace it.
uint64_t newPath = extendRoute(nextLink->parent->address.path,
nextLink->parent->encodingScheme,
nextLink->cannonicalLabel,
link->inverseLinkEncodingFormNumber);
if (newPath != extendRoute_INVALID &&
Node_getReach(nextLink->child) < newReach &&
Node_getReach(nextLink->parent) > newReach)
{
// This path apparently gives us a better route than our current bestParent.
updateBestParent(nextLink, newReach, store);
}
}

if (Node_getBestParent(nextLink->child) == nextLink) {
// This is a performance hack, if nextLink->child->bestParent->parent is this node
// we'll skip updating reach here since even if we did, it would be updated all over
Expand All @@ -2110,24 +2100,6 @@ static void updatePathReach(struct NodeStore_pvt* store, const uint64_t path, ui
// selfNode reach == UINT32_MAX so this case handles it.
} else if (!LabelSplicer_routesThrough(path, link->child->address.path)) {
// The path the packet came in on is not actually the best known path to the node.
// See if that's because it's better than the current bestParent.
if (link != Node_getBestParent(link->child)) {
bool pathIsOK = true;
uint64_t newPath = NodeStore_getRouteLabel(&store->pub,
link->parent->address.path,
link->cannonicalLabel);
if (newPath == NodeStore_getRouteLabel_PARENT_NOT_FOUND) { pathIsOK = false; }
if (newPath == NodeStore_getRouteLabel_CHILD_NOT_FOUND) { pathIsOK = false; }
if (newPath == NodeStore_optimizePath_INVALID) { pathIsOK = false; }

if (pathIsOK && Node_getReach(link->child) < newReach) {
// This path apparently gives us a better route than our current bestParent.
if (Node_getReach(link->parent) <= newReach) {
handleNews(link->parent, newReach+1, store);
}
updateBestParent(link, newReach, store);
}
}
} else {
handleNews(link->child, newReach, store);
}
Expand All @@ -2144,6 +2116,8 @@ static void updatePathReach(struct NodeStore_pvt* store, const uint64_t path, ui
update(link, 1, store);
}

nextLink->timeLastSeen = now;

pathFrag = nextPath;
link = nextLink;
}
Expand All @@ -2157,27 +2131,8 @@ static void updatePathReach(struct NodeStore_pvt* store, const uint64_t path, ui
handleNews(link->child, newReach, store);
uint32_t newLinkState = subReach(Node_getReach(link->parent), newReach);
update(link, newLinkState - link->linkState, store);

} else {
// This path doesn't lead through the node's current best parent.
// Check if it's better than the node's bestParent, and if so then update.
if (!link->parent) { return; }
if (!link->child) { return; }
if (newReach < Node_getReach(link->child)) { return; }
uint64_t newPath = NodeStore_getRouteLabel(&store->pub,
link->parent->address.path,
link->cannonicalLabel);
if (newPath == NodeStore_getRouteLabel_PARENT_NOT_FOUND) { return; }
if (newPath == NodeStore_getRouteLabel_CHILD_NOT_FOUND) { return; }
if (newPath == NodeStore_optimizePath_INVALID) { return; }
if (path != newPath && Node_getBestParent(link->child)) { return; }

// This path apparently gives us a better route than our current bestParent.
if (Node_getReach(link->parent) <= newReach) {
handleNews(link->parent, newReach+1, store);
}
updateBestParent(link, newReach, store);
}
link->child->timeLastPinged = Time_currentTimeMilliseconds(store->eventBase);
}

void NodeStore_pathResponse(struct NodeStore* nodeStore, uint64_t path, uint64_t milliseconds)
Expand All @@ -2188,9 +2143,8 @@ void NodeStore_pathResponse(struct NodeStore* nodeStore, uint64_t path, uint64_t
struct Node_Two* node = link->child;
uint32_t newReach;
if (node->address.path == path) {
// Use old reach value to calculate new reach--or not.
//newReach = calcNextReach(Node_getReach(node), milliseconds);
newReach = calcNextReach(0, milliseconds);
// Use old reach value to calculate new reach.
newReach = calcNextReach(Node_getReach(node), milliseconds);
}
else {
// Old reach value doesn't relate to this path, so we should do something different
Expand Down Expand Up @@ -2319,5 +2273,12 @@ uint16_t NodeStore_bucketForAddr(struct Address* source, struct Address* dest)
uint64_t NodeStore_timeSinceLastPing(struct NodeStore* nodeStore, struct Node_Two* node)
{
struct NodeStore_pvt* store = Identity_check((struct NodeStore_pvt*)nodeStore);
return Time_currentTimeMilliseconds(store->eventBase) - node->timeOfLastPing;
uint64_t now = Time_currentTimeMilliseconds(store->eventBase);
uint64_t lastSeen = node->timeLastPinged;
struct Node_Link* link = Node_getBestParent(node);
while (link && link != store->selfLink) {
lastSeen = (link->timeLastSeen < lastSeen) ? link->timeLastSeen : lastSeen;
link = Node_getBestParent(link->parent);
}
return now - lastSeen;
}
11 changes: 4 additions & 7 deletions dht/dhtcore/NodeStore_admin.c
View file
Expand Up @@ -60,13 +60,10 @@ static void dumpTable(Dict* args, void* vcontext, String* txid, struct Allocator
Dict_putInt(nodeDict, String_CONST("link"), Node_getReach(nn), requestAlloc);
Dict_putInt(nodeDict, String_CONST("version"), nn->address.protocolVersion, requestAlloc);

// Time we last pinged this node
if (!nn->timeOfLastPing) {
Dict_putString(nodeDict, String_CONST("time"), String_CONST("never"), requestAlloc);
} else {
uint64_t timeSinceLastPing = NodeStore_timeSinceLastPing(ctx->store, nn);
Dict_putInt(nodeDict, String_CONST("time"), timeSinceLastPing, requestAlloc);
}
Dict_putInt(nodeDict,
String_CONST("time"),
NodeStore_timeSinceLastPing(ctx->store, nn),
requestAlloc);

List_addDict(table, nodeDict, requestAlloc);
}
Expand Down
5 changes: 0 additions & 5 deletions dht/dhtcore/RouterModule.c
View file
Expand Up @@ -516,11 +516,6 @@ static void onResponseOrTimeout(String* data, uint32_t milliseconds, void* vping
return;
}

// Update ping time of the node if it came from the best path.
if (node->address.path == message->address->path) {
node->timeOfLastPing = Time_currentTimeMilliseconds(module->eventBase);
}

#ifdef Log_DEBUG
String* versionBin = Dict_getString(message->asDict, CJDHTConstants_VERSION);
if (versionBin && versionBin->len == 20) {
Expand Down

0 comments on commit f3ce288

Please sign in to comment.