Skip to content

Commit

Permalink
fw: fix and simplify enumeration logic in Forwarder::onNewNextHop()
Browse files Browse the repository at this point in the history 
Also remove Strategy::enableNewNextHopTrigger(). Strategies can already
opt-in by overriding afterNewNextHop(), there is no need for a separate
enablement flag.

Refs: #4931
Change-Id: Ifcbdbc7e954bd20d9969fd99882a2a9aebbc8fcc
  • Loading branch information
@Pesa
Pesa committed Feb 17, 2021
1 parent 264af77 commit b21bed8
Show file tree
Hide file tree
Showing 5 changed files with 107 additions and 225 deletions.
48 changes: 16 additions & 32 deletions daemon/fw/forwarder.cpp
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
/*
* Copyright (c) 2014-2020, Regents of the University of California,
* Copyright (c) 2014-2021, Regents of the University of California,
* Arizona Board of Regents,
* Colorado State University,
* University Pierre & Marie Curie, Sorbonne University,
Expand Down Expand Up @@ -76,8 +76,8 @@ Forwarder::Forwarder(FaceTable& faceTable)
cleanupOnFaceRemoval(m_nameTree, m_fib, m_pit, face);
});

m_fib.afterNewNextHop.connect([&] (const Name& prefix, const fib::NextHop& nextHop) {
this->startProcessNewNextHop(prefix, nextHop);
m_fib.afterNewNextHop.connect([this] (const Name& prefix, const fib::NextHop& nextHop) {
this->onNewNextHop(prefix, nextHop);
});

m_strategyChoice.setDefaultStrategy(getDefaultStrategyName());
Expand Down Expand Up @@ -213,7 +213,7 @@ Forwarder::onContentStoreMiss(const FaceEndpoint& ingress,

// dispatch to strategy: after incoming Interest
this->dispatchToStrategy(*pitEntry,
[&] (fw::Strategy& strategy) {
[&] (auto& strategy) {
strategy.afterReceiveInterest(FaceEndpoint(ingress.face, 0), interest, pitEntry);
});
}
Expand All @@ -237,7 +237,7 @@ Forwarder::onContentStoreHit(const FaceEndpoint& ingress, const shared_ptr<pit::

// dispatch to strategy: after Content Store hit
this->dispatchToStrategy(*pitEntry,
[&] (fw::Strategy& strategy) { strategy.afterContentStoreHit(pitEntry, ingress, data); });
[&] (auto& strategy) { strategy.afterContentStoreHit(pitEntry, ingress, data); });
}

pit::OutRecord*
Expand Down Expand Up @@ -325,7 +325,7 @@ Forwarder::onIncomingData(const FaceEndpoint& ingress, const Data& data)

// trigger strategy: after receive Data
this->dispatchToStrategy(*pitEntry,
[&] (fw::Strategy& strategy) { strategy.afterReceiveData(pitEntry, ingress, data); });
[&] (auto& strategy) { strategy.afterReceiveData(pitEntry, ingress, data); });

// mark PIT satisfied
pitEntry->isSatisfied = true;
Expand Down Expand Up @@ -358,7 +358,7 @@ Forwarder::onIncomingData(const FaceEndpoint& ingress, const Data& data)

// invoke PIT satisfy callback
this->dispatchToStrategy(*pitEntry,
[&] (fw::Strategy& strategy) { strategy.beforeSatisfyInterest(pitEntry, ingress, data); });
[&] (auto& strategy) { strategy.beforeSatisfyInterest(pitEntry, ingress, data); });

// mark PIT satisfied
pitEntry->isSatisfied = true;
Expand Down Expand Up @@ -481,7 +481,7 @@ Forwarder::onIncomingNack(const FaceEndpoint& ingress, const lp::Nack& nack)

// trigger strategy: after receive NACK
this->dispatchToStrategy(*pitEntry,
[&] (fw::Strategy& strategy) { strategy.afterReceiveNack(ingress, nack, pitEntry); });
[&] (auto& strategy) { strategy.afterReceiveNack(ingress, nack, pitEntry); });
}

bool
Expand Down Expand Up @@ -542,37 +542,21 @@ Forwarder::onNewNextHop(const Name& prefix, const fib::NextHop& nextHop)
{
const auto affectedEntries = this->getNameTree().partialEnumerate(prefix,
[&] (const name_tree::Entry& nte) -> std::pair<bool, bool> {
const fib::Entry* fibEntry = nte.getFibEntry();
const fw::Strategy* strategy = nullptr;
if (nte.getStrategyChoiceEntry() != nullptr) {
strategy = &nte.getStrategyChoiceEntry()->getStrategy();
}
// current nte has buffered Interests but no fibEntry (except for the root nte) and the strategy
// enables new nexthop behavior, we enumerate the current nte and keep visiting its children.
if (nte.getName().size() == 0 ||
(strategy != nullptr && strategy->wantNewNextHopTrigger() &&
fibEntry == nullptr && nte.hasPitEntries())) {
return {true, true};
}
// we don't need the current nte (no pitEntry or strategy doesn't support new nexthop), but
// if the current nte has no fibEntry, it's still possible that its children are affected by
// the new nexthop.
else if (fibEntry == nullptr) {
return {false, true};
}
// if the current nte has a fibEntry, we ignore the current nte and don't visit its
// children because they are already covered by the current nte's fibEntry.
else {
// we ignore an NTE and skip visiting its descendants if that NTE has an
// associated FIB entry (1st condition), since in that case the new nexthop
// won't affect any PIT entries anywhere in that subtree, *unless* this is
// the initial NTE from which the enumeration started (2nd condition), which
// must always be considered
if (nte.getFibEntry() != nullptr && nte.getName().size() > prefix.size()) {
return {false, false};
}
return {nte.hasPitEntries(), true};
});

for (const auto& nte : affectedEntries) {
for (const auto& pitEntry : nte.getPitEntries()) {
this->dispatchToStrategy(*pitEntry,
[&] (fw::Strategy& strategy) {
strategy.afterNewNextHop(nextHop, pitEntry);
});
[&] (auto& strategy) { strategy.afterNewNextHop(nextHop, pitEntry); });
}
}
}
Expand Down
10 changes: 0 additions & 10 deletions daemon/fw/forwarder.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,16 +107,6 @@ class Forwarder
this->onIncomingNack(ingress, nack);
}

/** \brief start new nexthop processing
* \param prefix the prefix of the FibEntry containing the new nexthop
* \param nextHop the new NextHop
*/
void
startProcessNewNextHop(const Name& prefix, const fib::NextHop& nextHop)
{
this->onNewNextHop(prefix, nextHop);
}

NameTree&
getNameTree()
{
Expand Down
19 changes: 0 additions & 19 deletions daemon/fw/strategy.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,14 +114,6 @@ class Strategy : noncopyable
return m_name;
}

/** \return Whether the afterNewNextHop trigger should be invoked for this strategy.
*/
bool
wantNewNextHopTrigger() const
{
return m_wantNewNextHopTrigger;
}

public: // triggers
/** \brief Trigger after Interest is received
*
Expand Down Expand Up @@ -388,15 +380,6 @@ class Strategy : noncopyable
m_name = name;
}

NFD_PUBLIC_WITH_TESTS_ELSE_PROTECTED: // setter
/** \brief Set whether the afterNewNextHop trigger should be invoked for this strategy
*/
void
enableNewNextHopTrigger(bool enabled)
{
m_wantNewNextHopTrigger = enabled;
}

private: // registry
typedef std::function<unique_ptr<Strategy>(Forwarder& forwarder, const Name& strategyName)> CreateFunc;
typedef std::map<Name, CreateFunc> Registry; // indexed by strategy name
Expand All @@ -421,8 +404,6 @@ class Strategy : noncopyable
Forwarder& m_forwarder;

MeasurementsAccessor m_measurements;

bool m_wantNewNextHopTrigger = false;
};

} // namespace fw
Expand Down
92 changes: 91 additions & 1 deletion tests/daemon/fw/forwarder.t.cpp
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
/*
* Copyright (c) 2014-2020, Regents of the University of California,
* Copyright (c) 2014-2021, Regents of the University of California,
* Arizona Board of Regents,
* Colorado State University,
* University Pierre & Marie Curie, Sorbonne University,
Expand Down Expand Up @@ -698,6 +698,96 @@ BOOST_AUTO_TEST_CASE(UnsolicitedData)
BOOST_CHECK_EQUAL(forwarder.getCounters().nUnsolicitedData, 1);
}

BOOST_AUTO_TEST_CASE(NewNextHop)
{
auto face1 = addFace();
auto face2 = addFace();
auto face3 = addFace();
auto face4 = addFace();

auto& strategy = choose<DummyStrategy>(forwarder, "/A", DummyStrategy::getStrategyName());

Fib& fib = forwarder.getFib();
Pit& pit = forwarder.getPit();

// fib: "/", "/A/B", "/A/B/C/D/E"
fib::Entry* entry = fib.insert("/").first;
fib.addOrUpdateNextHop(*entry, *face1, 100);
entry = fib.insert("/A/B").first;
fib.addOrUpdateNextHop(*entry, *face2, 0);
entry = fib.insert("/A/B/C/D/E").first;
fib.addOrUpdateNextHop(*entry, *face3, 0);

// pit: "/A", "/A/B/C", "/A/B/Z"
auto interest1 = makeInterest("/A");
auto pit1 = pit.insert(*interest1).first;
pit1->insertOrUpdateInRecord(*face3, *interest1);
auto interest2 = makeInterest("/A/B/C");
auto pit2 = pit.insert(*interest2).first;
pit2->insertOrUpdateInRecord(*face3, *interest2);
auto interest3 = makeInterest("/A/B/Z");
auto pit3 = pit.insert(*interest3).first;
pit3->insertOrUpdateInRecord(*face3, *interest3);

// new nexthop for "/"
entry = fib.insert("/").first;
fib.addOrUpdateNextHop(*entry, *face2, 50);

// /A --> triggered
// /A/B/C --> not triggered
// /A/B/Z --> not triggered
BOOST_TEST_REQUIRE(strategy.afterNewNextHopCalls.size() == 1);
BOOST_TEST(strategy.afterNewNextHopCalls[0] == "/A");
strategy.afterNewNextHopCalls.clear();

// new nexthop for "/A"
entry = fib.insert("/A").first;
fib.addOrUpdateNextHop(*entry, *face4, 50);

// /A --> triggered
// /A/B/C --> not triggered
// /A/B/Z --> not triggered
BOOST_TEST_REQUIRE(strategy.afterNewNextHopCalls.size() == 1);
BOOST_TEST(strategy.afterNewNextHopCalls[0] == "/A");
strategy.afterNewNextHopCalls.clear();

// new nexthop for "/A/B"
entry = fib.insert("/A/B").first;
fib.addOrUpdateNextHop(*entry, *face4, 0);

// /A --> not triggered
// /A/B/C --> triggered
// /A/B/Z --> triggered
BOOST_TEST_REQUIRE(strategy.afterNewNextHopCalls.size() == 2);
BOOST_TEST(strategy.afterNewNextHopCalls[0] == "/A/B/C");
BOOST_TEST(strategy.afterNewNextHopCalls[1] == "/A/B/Z");
strategy.afterNewNextHopCalls.clear();

// new nexthop for "/A/B/C/D"
entry = fib.insert("/A/B/C/D").first;
fib.addOrUpdateNextHop(*entry, *face1, 0);

// nothing triggered
BOOST_TEST(strategy.afterNewNextHopCalls.size() == 0);

// create a second pit entry for /A
auto interest4 = makeInterest("/A");
interest4->setMustBeFresh(true);
auto pit4 = pit.insert(*interest4).first;
pit4->insertOrUpdateInRecord(*face3, *interest4);

// new nexthop for "/A"
entry = fib.insert("/A").first;
fib.addOrUpdateNextHop(*entry, *face1, 0);

// /A --> triggered twice
// /A/B/C --> not triggered
// /A/B/Z --> not triggered
BOOST_TEST_REQUIRE(strategy.afterNewNextHopCalls.size() == 2);
BOOST_TEST(strategy.afterNewNextHopCalls[0] == "/A");
BOOST_TEST(strategy.afterNewNextHopCalls[1] == "/A");
}

BOOST_AUTO_TEST_SUITE_END() // TestForwarder
BOOST_AUTO_TEST_SUITE_END() // Fw

Expand Down
Loading

0 comments on commit b21bed8

Please sign in to comment.