Pavlin Radoslavov wrote:
Ben Greear <[EMAIL PROTECTED]> wrote:
Pavlin Radoslavov wrote:
In your implementation you need to be very careful that you capture
all places inside IfTree that are related to the new pif_index to
vif mapping.
I was thinking on the way home: Maybe just map if-index to if-name. If
the mapping
lookup fails, do a long slow linear lookup and if the object is found,
add it to the if-index -> map.
If it succeeds, then lookup the vif by way of the existing hash,
double-check the if-index is
correct (if not, do a slow lookup).
I think this adds lots of complexity.
Just a simple if-index to vif-name-pointer should be sufficient.
Off the top of my head, you need to consider the following places
inside IfTree that will affect the mapping:
Maybe I'm too paranoid..but any code can get an if, and then muck with it's
vifs. The mapping is external to the if and vif objects, so it would be
hard
to make sure no one can ever screw up a listing.
I presume we are talking of a map that is internal to IfTree.
Any addition/removal of a vif should use the add_vif() and
remove_vif() methods so those methods need to take care of updating
the internal map as well.
Indeed, to be on the safe side, the IfTreeInterface::vifs()
method that returns a non-const reference to the vifs map should
be removed. This obviously requires refactoring in other parts
of the FEA. For the sake of moving things forward we can ignore it
for now, but a warning comment should be added to the VifMap& vifs()
method.
Also, an ifname can change while the ifindex remains the same, or
a new interface with the same name but a different ifindex can be
created.
The ifname and the vifname cannot change, because they are the
unique ID of an interface/vif. If they change, this will be
translated into delete/add sequence to the the IfTree and that
should take care of the ifindex update.
The ifindex is also unique per interface/vif so there shouldn't be
more than one interface/vif with the same ifindex (modulo ifindex of
0 which is invalid index).
I need to re-read your email when I'm fresh....but in the meantime, here
is a partial diff from
my tree with the hashing changes. It compiles, but not tested yet. We
can remove the fallbacks
to the linear searches if/when we are sure all the boundary cases are
resolved. In the meantime,
I don't think it will really hurt anything, and there is enough logging
to clue us in should we
still need to work on the hash...
Comments welcome.
Thanks,
Ben
--
Ben Greear <[EMAIL PROTECTED]>
Candela Technologies Inc http://www.candelatech.com
Index: iftree.cc
===================================================================
RCS file: /cvs/xorp/fea/iftree.cc,v
retrieving revision 1.56
diff -u -r1.56 iftree.cc
--- iftree.cc 4 Jan 2008 03:15:46 -0000 1.56
+++ iftree.cc 5 Mar 2008 07:49:50 -0000
@@ -58,6 +58,8 @@
IfTree::clear()
{
_interfaces.clear();
+ _ifindexes.clear();
+ _vifindexes.clear();
}
int
@@ -102,9 +104,33 @@
{
IfTree::IfMap::iterator iter;
+ // First, check out ifindex hash.
+ IfTree::IfIndexMap::iterator ifiter = _ifindexes.find(pif_index);
+ if (ifiter != _ifindexes.end()) {
+ iter = _interfaces.find(ifiter->second);
+ if (iter != _interfaces.end()) {
+ if (iter->second.pif_index() == pif_index) {
+ return (&iter->second);
+ }
+ else {
+ // Hmmm, mismatch in our hash.
+ XLOG_ERROR("mismatch in ifindex hash, pif_index: %i mapped-name: %s actual pif_index: %i\n",
+ pif_index, ifiter->second.c_str(), iter->second.pif_index());
+ // Delete the old hash that was wrong.
+ _ifindexes.erase(ifiter);
+ _ifindexes[pif_index] = iter->second.ifname();
+ }
+ }
+ }
+
+ XLOG_ERROR("WARNING: Finding interface by pif_index using linear search, pif_index: %i!\n",
+ pif_index);
for (iter = _interfaces.begin(); iter != _interfaces.end(); ++iter) {
- if (iter->second.pif_index() == pif_index)
+ if (iter->second.pif_index() == pif_index) {
+ // Add to our hash to speed future lookups.
+ _ifindexes[pif_index] = iter->second.ifname();
return (&iter->second);
+ }
}
return (NULL);
@@ -115,9 +141,33 @@
{
IfTree::IfMap::const_iterator iter;
+ // First, check out ifindex hash.
+ IfTree::IfIndexMap::iterator ifiter = _ifindexes.find(pif_index);
+ if (ifiter != _ifindexes.end()) {
+ iter = _interfaces.find(ifiter->second);
+ if (iter != _interfaces.end()) {
+ if (iter->second.pif_index() == pif_index) {
+ return (&iter->second);
+ }
+ else {
+ // Hmmm, mismatch in our hash.
+ XLOG_ERROR("mismatch in ifindex hash, pif_index: %i mapped-name: %s actual pif_index: %i\n",
+ pif_index, ifiter->second.c_str(), iter->second.pif_index());
+ // Delete the old hash that was wrong.
+ _ifindexes.erase(ifiter);
+ _ifindexes[pif_index] = iter->second.ifname();
+ }
+ }
+ }
+
+ XLOG_ERROR("WARNING: Finding interface by pif_index (const) using linear search, pif_index: %i!\n",
+ pif_index);
for (iter = _interfaces.begin(); iter != _interfaces.end(); ++iter) {
- if (iter->second.pif_index() == pif_index)
+ if (iter->second.pif_index() == pif_index) {
+ // Add to our hash to speed future lookups.
+ _ifindexes[pif_index] = iter->second.ifname();
return (&iter->second);
+ }
}
return (NULL);
@@ -149,14 +199,37 @@
IfTree::find_vif(uint32_t pif_index)
{
IfTree::IfMap::iterator iter;
+ // First, check out ifindex hash.
+ IfTree::IfIndexMap::iterator ifiter = _vifindexes.find(pif_index);
+ if (ifiter != _vifindexes.end()) {
+ iter = _interfaces.find(ifiter->second);
+ if (iter != _interfaces.end()) {
+ IfTreeVif* vifp = iter->second.find_vif(pif_index);
+ if (vifp) {
+ return vifp;
+ }
+ else {
+ // Hmmm, mismatch in our hash.
+ XLOG_ERROR("mismatch in vifindex hash, pif_index: %i mapped-name: %s\n",
+ pif_index, ifiter->second.c_str());
+ // Delete the old hash that was wrong.
+ _vifindexes.erase(ifiter);
+ }
+ }
+ }
+ XLOG_ERROR("WARNING: Finding vif by pif_index using linear search, pif_index: %i!\n",
+ pif_index);
//
// XXX: Find the first vif that matches the physical index
//
for (iter = _interfaces.begin(); iter != _interfaces.end(); ++iter) {
IfTreeVif* vifp = iter->second.find_vif(pif_index);
- if (vifp != NULL)
+ if (vifp != NULL) {
+ // Correct hash for next time
+ _vifindexes[pif_index] = iter->second.ifname();
return (vifp);
+ }
}
return (NULL);
@@ -166,14 +239,37 @@
IfTree::find_vif(uint32_t pif_index) const
{
IfTree::IfMap::const_iterator iter;
+ // First, check out ifindex hash.
+ IfTree::IfIndexMap::iterator ifiter = _vifindexes.find(pif_index);
+ if (ifiter != _vifindexes.end()) {
+ iter = _interfaces.find(ifiter->second);
+ if (iter != _interfaces.end()) {
+ const IfTreeVif* vifp = iter->second.find_vif(pif_index);
+ if (vifp) {
+ return vifp;
+ }
+ else {
+ // Hmmm, mismatch in our hash.
+ XLOG_ERROR("mismatch in vifindex hash, pif_index: %i mapped-name: %s\n",
+ pif_index, ifiter->second.c_str());
+ // Delete the old hash that was wrong.
+ _vifindexes.erase(ifiter);
+ }
+ }
+ }
+ XLOG_ERROR("WARNING: Finding vif by pif_index (const) using linear search, pif_index: %i!\n",
+ pif_index);
//
// XXX: Find the first vif that matches the physical index
//
for (iter = _interfaces.begin(); iter != _interfaces.end(); ++iter) {
const IfTreeVif* vifp = iter->second.find_vif(pif_index);
- if (vifp != NULL)
+ if (vifp != NULL) {
+ // Correct hash for next time
+ _vifindexes[pif_index] = iter->second.ifname();
return (vifp);
+ }
}
return (NULL);
@@ -227,6 +323,7 @@
return (vifp->find_addr(addr));
}
+
bool
IfTree::find_interface_vif_by_addr(const IPvX& addr,
const IfTreeInterface*& ifp,
@@ -499,6 +596,15 @@
XLOG_ASSERT(this_vifp != NULL);
IfTreeVif& this_vif = *this_vifp;
this_vif.copy_state(other_vif);
+ // Update the vif-index hash
+ // I'm not sure there is a 1-1 mapping for vif indexes and if-indexes in all
+ // cases. If nothing else, the linear search will still find the right answer,
+ // even if it is not efficient. Here's hoping it is at least usually a 1-1 mapping.
+ // --Ben
+ IfIndexMap::iterator ifiter = _vifindexes.find(this_vif.pif_index());
+ if (ifiter == _vifindexes.end()) {
+ _vifindexes[this_vif.pif_index()] = other_iface.ifname();
+ }
//
// Add the IPv4 addresses
@@ -557,6 +663,12 @@
// If interface is marked as deleted, delete it.
//
if (ii->second.is_marked(DELETED)) {
+ _ifindexes.erase(ii->second.pif_index());
+ IfTreeInterface::VifMap::const_iterator ov = ii->second.vifs().begin();
+ while (ov != ii->second.vifs().end()) {
+ _vifindexes.erase(ov->second.pif_index());
+ ov++;
+ }
_interfaces.erase(ii++);
continue;
}
@@ -967,6 +1079,12 @@
old_ifp = old_iftree.find_interface(ifname);
if (old_ifp == NULL) {
// Remove this item from the local tree
+ _ifindexes.erase(old_ifp->pif_index());
+ IfTreeInterface::VifMap::const_iterator ov = old_ifp->vifs().begin();
+ while (ov != old_ifp->vifs().end()) {
+ _vifindexes.erase(ov->second.pif_index());
+ ov++;
+ }
_interfaces.erase(ii++);
continue;
}
@@ -987,6 +1105,7 @@
old_vifp = old_ifp->find_vif(vifname);
if (old_vifp == NULL) {
// Remove this item from the local tree
+ _vifindexes.erase(vi->second.pif_index());
fi.vifs().erase(vi++);
continue;
}
Index: iftree.hh
===================================================================
RCS file: /cvs/xorp/fea/iftree.hh,v
retrieving revision 1.59
diff -u -r1.59 iftree.hh
--- iftree.hh 4 Jan 2008 03:15:46 -0000 1.59
+++ iftree.hh 5 Mar 2008 07:49:51 -0000
@@ -111,6 +111,10 @@
class IfTree : public IfTreeItem {
public:
typedef map<const string, IfTreeInterface> IfMap;
+ /** Map ifindex to ifname (NOT VIF name, since we cannot directly look up
+ * a VIF from a vifname.
+ */
+ typedef map<const int, string> IfIndexMap;
/**
* Remove all interface state from the interface tree.
@@ -375,8 +379,20 @@
*/
string str() const;
+ void updateVifCache(int pif_index, const string& ifname) {
+ _vifindexes[pif_index] = ifname;
+ }
+
+ void updateIfCache(int if_index, const string& ifname) {
+ _ifindexes[if_index] = ifname;
+ }
+
+
protected:
IfMap _interfaces;
+ // These exist only to speed up searches.
+ mutable IfIndexMap _ifindexes; // mapping of ifindex to if-name.
+ mutable IfIndexMap _vifindexes; // mapping of vif's pif_index to if-name
};
Index: data_plane/ifconfig/ifconfig_parse_netlink_socket.cc
===================================================================
RCS file: /cvs/xorp/fea/data_plane/ifconfig/ifconfig_parse_netlink_socket.cc,v
retrieving revision 1.17
diff -u -r1.17 ifconfig_parse_netlink_socket.cc
--- data_plane/ifconfig/ifconfig_parse_netlink_socket.cc 21 Feb 2008 02:02:33 -0000 1.17
+++ data_plane/ifconfig/ifconfig_parse_netlink_socket.cc 5 Mar 2008 07:49:51 -0000
@@ -310,8 +310,10 @@
//
// Set the physical interface index for the interface
//
- if (is_newlink || (if_index != ifp->pif_index()))
+ if (is_newlink || (if_index != ifp->pif_index())) {
ifp->set_pif_index(if_index);
+ iftree.updateIfCache(if_index, if_name);
+ }
//
// Get the MAC address
@@ -383,9 +385,11 @@
//
// Set the physical interface index for the vif
//
- if (is_newlink || (if_index != vifp->pif_index()))
+ if (is_newlink || (if_index != vifp->pif_index())) {
vifp->set_pif_index(if_index);
-
+ iftree.updateVifCache(if_index, if_name);
+ }
+
//
// Set the vif flags
//
@@ -603,7 +607,8 @@
//
return;
}
- XLOG_FATAL("Could not find vif with index %u in IfTree", if_index);
+ XLOG_ERROR("Could not find vif with index %u in IfTree", if_index);
+ return;
}
debug_msg("Address event on interface %s vif %s with interface index %u\n",
vifp->ifname().c_str(), vifp->vifname().c_str(),
_______________________________________________
Xorp-hackers mailing list
[email protected]
http://mailman.ICSI.Berkeley.EDU/mailman/listinfo/xorp-hackers
