*: introduce new rb-tree to optimize interface lookup by ifindex

Performance tests showed that, when running on a system with a large
number of interfaces, some daemons would spend a considerable amount
of time in the if_lookup_by_index() function. Introduce a new rb-tree
to solve this problem.

With this change, we need to use the if_set_index() function whenever
we want to change the ifindex of an interface. This is necessary to
ensure that the 'ifaces_by_index' rb-tree is updated accordingly. The
return value of all insert/remove operations in the interface rb-trees
is checked to ensure that an error is logged if a corruption is
detected.

Signed-off-by: Renato Westphal <renato@opensourcerouting.org>

diff --git a/babeld/babel_interface.c b/babeld/babel_interface.c
index 49c848d49e69f24398617927d030926db1efcfde..5bc48e8a258fc2fdc5bb99d92e81cb8357879ffd 100644 (file)
--- a/babeld/babel_interface.c
+++ b/babeld/babel_interface.c
@@ -138,7 +138,7 @@ babel_interface_delete (int cmd, struct zclient *client, zebra_size_t length, vr
 
     /* To support pseudo interface do not free interface structure.  */
     /* if_delete(ifp); */
-    ifp->ifindex = IFINDEX_INTERNAL;
+    if_set_index(ifp, IFINDEX_INTERNAL);
 
     return 0;
 }

diff --git a/bgpd/bgp_zebra.c b/bgpd/bgp_zebra.c
index ae8f2f26d4cbc823c320a4d78569a59a439da71d..aff88694ef967bc0938cd36ed804a1fa937419a4 100644 (file)
--- a/bgpd/bgp_zebra.c
+++ b/bgpd/bgp_zebra.c
@@ -238,7 +238,7 @@ static int bgp_interface_delete(int command, struct zclient *zclient,
 
        bgp_update_interface_nbrs(bgp, ifp, NULL);
 
-       ifp->ifindex = IFINDEX_INTERNAL;
+       if_set_index(ifp, IFINDEX_INTERNAL);
        return 0;
 }
 

diff --git a/eigrpd/eigrp_zebra.c b/eigrpd/eigrp_zebra.c
index b70c55104c0ea08111fd208f763f3f9930407b96..28d2f298119a712331ba570f3b642694eb8fcb75 100644 (file)
--- a/eigrpd/eigrp_zebra.c
+++ b/eigrpd/eigrp_zebra.c
@@ -192,6 +192,7 @@ static int eigrp_interface_delete(int command, struct zclient *zclient,
                eigrp_if_free(ifp->info,
                              INTERFACE_DOWN_BY_ZEBRA);
 
+       if_set_index(ifp, IFINDEX_INTERNAL);
        return 0;
 }
 

diff --git a/isisd/isis_zebra.c b/isisd/isis_zebra.c
index 7109bfb110b8ff62344ec0aa5161ee3e730c6ec5..387f99938e07646d14c05f5260dd3490b9c1b814 100644 (file)
--- a/isisd/isis_zebra.c
+++ b/isisd/isis_zebra.c
@@ -128,7 +128,7 @@ static int isis_zebra_if_del(int command, struct zclient *zclient,
           in case there is configuration info attached to it. */
        if_delete_retain(ifp);
 
-       ifp->ifindex = IFINDEX_INTERNAL;
+       if_set_index(ifp, IFINDEX_INTERNAL);
 
        return 0;
 }

diff --git a/ldpd/ldp_zebra.c b/ldpd/ldp_zebra.c
index 4b3f5b0f99ca6fa5137f81b356cdcdcd534f482a..f6dfe96dc4e5a28b0cb7fdf649bb726fa5b21b9c 100644 (file)
--- a/ldpd/ldp_zebra.c
+++ b/ldpd/ldp_zebra.c
@@ -288,7 +288,7 @@ ldp_interface_delete(int command, struct zclient *zclient, zebra_size_t length,
 
        /* To support pseudo interface do not free interface structure.  */
        /* if_delete(ifp); */
-       ifp->ifindex = IFINDEX_INTERNAL;
+       if_set_index(ifp, IFINDEX_INTERNAL);
 
        ifp2kif(ifp, &kif);
        main_imsg_compose_both(IMSG_IFSTATUS, &kif, sizeof(kif));

diff --git a/lib/if.c b/lib/if.c
index c40daf7ef5975c0865cc514940ae2e9167b34d2d..612cc2081e23432cf0b4d2426fdd5d617db287c4 100644 (file)
--- a/lib/if.c
+++ b/lib/if.c
@@ -41,7 +41,10 @@ DEFINE_MTYPE(LIB, CONNECTED_LABEL, "Connected interface label")
 DEFINE_MTYPE_STATIC(LIB, IF_LINK_PARAMS, "Informational Link Parameters")
 
 static int if_cmp_func(const struct interface *, const struct interface *);
+static int if_cmp_index_func(const struct interface *ifp1,
+                            const struct interface *ifp2);
 RB_GENERATE(if_name_head, interface, name_entry, if_cmp_func);
+RB_GENERATE(if_index_head, interface, index_entry, if_cmp_index_func);
 
 DEFINE_QOBJ_TYPE(interface)
 
@@ -118,6 +121,12 @@ static int if_cmp_func(const struct interface *ifp1,
        return if_cmp_name_func((char *)ifp1->name, (char *)ifp2->name);
 }
 
+static int if_cmp_index_func(const struct interface *ifp1,
+                            const struct interface *ifp2)
+{
+       return ifp1->ifindex - ifp2->ifindex;
+}
+
 /* Create new interface structure. */
 struct interface *if_create(const char *name, vrf_id_t vrf_id)
 {
@@ -130,11 +139,7 @@ struct interface *if_create(const char *name, vrf_id_t vrf_id)
        assert(name);
        strlcpy(ifp->name, name, sizeof(ifp->name));
        ifp->vrf_id = vrf_id;
-       if (RB_INSERT(if_name_head, &vrf->ifaces_by_name, ifp))
-               zlog_err(
-                       "if_create(%s): corruption detected -- interface with this "
-                       "name exists already in VRF %u!",
-                       ifp->name, vrf_id);
+       IFNAME_RB_INSERT(vrf, ifp);
        ifp->connected = list_new();
        ifp->connected->del = (void (*)(void *))connected_free;
 
@@ -156,16 +161,18 @@ void if_update_to_new_vrf(struct interface *ifp, vrf_id_t vrf_id)
 
        /* remove interface from old master vrf list */
        vrf = vrf_lookup_by_id(ifp->vrf_id);
-       if (vrf)
-               RB_REMOVE(if_name_head, &vrf->ifaces_by_name, ifp);
+       if (vrf) {
+               IFNAME_RB_REMOVE(vrf, ifp);
+               if (ifp->ifindex != IFINDEX_INTERNAL)
+                       IFINDEX_RB_REMOVE(vrf, ifp);
+       }
 
        ifp->vrf_id = vrf_id;
        vrf = vrf_get(ifp->vrf_id, NULL);
-       if (RB_INSERT(if_name_head, &vrf->ifaces_by_name, ifp))
-               zlog_err(
-                       "%s(%s): corruption detected -- interface with this "
-                       "name exists already in VRF %u!",
-                       __func__, ifp->name, vrf_id);
+
+       IFNAME_RB_INSERT(vrf, ifp);
+       if (ifp->ifindex != IFINDEX_INTERNAL)
+               IFINDEX_RB_INSERT(vrf, ifp);
 }
 
 
@@ -187,7 +194,9 @@ void if_delete(struct interface *ifp)
 {
        struct vrf *vrf = vrf_lookup_by_id(ifp->vrf_id);
 
-       RB_REMOVE(if_name_head, &vrf->ifaces_by_name, ifp);
+       IFNAME_RB_REMOVE(vrf, ifp);
+       if (ifp->ifindex != IFINDEX_INTERNAL)
+               IFINDEX_RB_REMOVE(vrf, ifp);
 
        if_delete_retain(ifp);
 
@@ -203,13 +212,10 @@ void if_delete(struct interface *ifp)
 struct interface *if_lookup_by_index(ifindex_t ifindex, vrf_id_t vrf_id)
 {
        struct vrf *vrf = vrf_lookup_by_id(vrf_id);
-       struct interface *ifp;
-
-       RB_FOREACH (ifp, if_name_head, &vrf->ifaces_by_name)
-               if (ifp->ifindex == ifindex)
-                       return ifp;
+       struct interface if_tmp;
 
-       return NULL;
+       if_tmp.ifindex = ifindex;
+       return RB_FIND(if_index_head, &vrf->ifaces_by_index, &if_tmp);
 }
 
 const char *ifindex2ifname(ifindex_t ifindex, vrf_id_t vrf_id)
@@ -378,6 +384,22 @@ struct interface *if_get_by_name(const char *name, vrf_id_t vrf_id, int vty)
        return if_create(name, vrf_id);
 }
 
+void if_set_index(struct interface *ifp, ifindex_t ifindex)
+{
+       struct vrf *vrf = vrf_lookup_by_id(ifp->vrf_id);
+
+       if (ifp->ifindex == ifindex)
+               return;
+
+       if (ifp->ifindex != IFINDEX_INTERNAL)
+               IFINDEX_RB_REMOVE(vrf, ifp)
+
+       ifp->ifindex = ifindex;
+
+       if (ifp->ifindex != IFINDEX_INTERNAL)
+               IFINDEX_RB_INSERT(vrf, ifp)
+}
+
 /* Does interface up ? */
 int if_is_up(struct interface *ifp)
 {

diff --git a/lib/if.h b/lib/if.h
index d4ae8fc4296c89e49c5cea9b8a3c5b8ba32eb267..3e2824b6c952dc6cd22d8b0becbe388df9fc235e 100644 (file)
--- a/lib/if.h
+++ b/lib/if.h
@@ -201,7 +201,7 @@ struct if_link_params {
 
 /* Interface structure */
 struct interface {
-       RB_ENTRY(interface) name_entry;
+       RB_ENTRY(interface) name_entry, index_entry;
 
        /* Interface name.  This should probably never be changed after the
           interface is created, because the configuration info for this
@@ -214,7 +214,12 @@ struct interface {
        char name[INTERFACE_NAMSIZ];
 
        /* Interface index (should be IFINDEX_INTERNAL for non-kernel or
-          deleted interfaces). */
+          deleted interfaces).
+          WARNING: the ifindex needs to be changed using the if_set_index()
+          function. Failure to respect this will cause corruption in the data
+          structure used to store the interfaces and if_lookup_by_index() will
+          not work as expected.
+        */
        ifindex_t ifindex;
 #define IFINDEX_INTERNAL       0
 
@@ -285,8 +290,38 @@ struct interface {
 };
 RB_HEAD(if_name_head, interface);
 RB_PROTOTYPE(if_name_head, interface, name_entry, if_cmp_func);
+RB_HEAD(if_index_head, interface);
+RB_PROTOTYPE(if_index_head, interface, index_entry, if_cmp_func);
 DECLARE_QOBJ_TYPE(interface)
 
+#define IFNAME_RB_INSERT(vrf, ifp)                                             \
+       if (RB_INSERT(if_name_head, &vrf->ifaces_by_name, (ifp)))              \
+               zlog_err(                                                      \
+                       "%s(%s): corruption detected -- interface with this "  \
+                       "name exists already in VRF %u!",                      \
+                       __func__, (ifp)->name, (ifp)->vrf_id);
+
+#define IFNAME_RB_REMOVE(vrf, ifp)                                             \
+       if (RB_REMOVE(if_name_head, &vrf->ifaces_by_name, (ifp)) == NULL)      \
+               zlog_err(                                                      \
+                       "%s(%s): corruption detected -- interface with this "  \
+                       "name doesn't exist in VRF %u!",                       \
+                       __func__, (ifp)->name, (ifp)->vrf_id);
+
+#define IFINDEX_RB_INSERT(vrf, ifp)                                            \
+       if (RB_INSERT(if_index_head, &vrf->ifaces_by_index, (ifp)))            \
+               zlog_err(                                                      \
+                       "%s(%u): corruption detected -- interface with this "  \
+                       "ifindex exists already in VRF %u!",                   \
+                       __func__, (ifp)->ifindex, (ifp)->vrf_id);
+
+#define IFINDEX_RB_REMOVE(vrf, ifp)                                            \
+       if (RB_REMOVE(if_index_head, &vrf->ifaces_by_index, (ifp)) == NULL)    \
+               zlog_err(                                                      \
+                       "%s(%u): corruption detected -- interface with this "  \
+                       "ifindex doesn't exist in VRF %u!",                    \
+                       __func__, (ifp)->ifindex, (ifp)->vrf_id);
+
 /* called from the library code whenever interfaces are created/deleted
  * note: interfaces may not be fully realized at that point; also they
  * may not exist in the system (ifindex = IFINDEX_INTERNAL)
@@ -426,6 +461,7 @@ extern struct interface *if_lookup_by_name_all_vrf(const char *ifname);
 extern struct interface *if_lookup_by_name(const char *ifname, vrf_id_t vrf_id);
 extern struct interface *if_get_by_name(const char *ifname, vrf_id_t vrf_id,
                                        int vty);
+extern void if_set_index(struct interface *ifp, ifindex_t ifindex);
 
 /* Delete the interface, but do not free the structure, and leave it in the
    interface list.  It is often advisable to leave the pseudo interface

diff --git a/lib/vrf.c b/lib/vrf.c
index e303e0e1c8e7b43d2ba81787e3bc391033ec1586..056f778a3e41fad559d19309a6012df5c9b7c00f 100644 (file)
--- a/lib/vrf.c
+++ b/lib/vrf.c
@@ -110,6 +110,7 @@ struct vrf *vrf_get(vrf_id_t vrf_id, const char *name)
                vrf = XCALLOC(MTYPE_VRF, sizeof(struct vrf));
                vrf->vrf_id = VRF_UNKNOWN;
                RB_INIT(if_name_head, &vrf->ifaces_by_name);
+               RB_INIT(if_index_head, &vrf->ifaces_by_index);
                QOBJ_REG(vrf, vrf);
                new = 1;
 

diff --git a/lib/vrf.h b/lib/vrf.h
index 5309100bd5ef812c4e509dbb168739dd05618f1e..8bfdc87689f1baf7d5c30b7baacfcc9c5249a9f0 100644 (file)
--- a/lib/vrf.h
+++ b/lib/vrf.h
@@ -79,6 +79,7 @@ struct vrf {
 
        /* Interfaces belonging to this VRF */
        struct if_name_head ifaces_by_name;
+       struct if_index_head ifaces_by_index;
 
        /* User data */
        void *info;

diff --git a/lib/zclient.c b/lib/zclient.c
index 1fc88ee6aad530a3532347470a646c1f365236ff..ad5c30584c122671e1032c4275b61ef8b480d1f7 100644 (file)
--- a/lib/zclient.c
+++ b/lib/zclient.c
@@ -1331,7 +1331,7 @@ void zebra_interface_if_set_value(struct stream *s, struct interface *ifp)
        u_char link_params_status = 0;
 
        /* Read interface's index. */
-       ifp->ifindex = stream_getl(s);
+       if_set_index(ifp, stream_getl(s));
        ifp->status = stream_getc(s);
 
        /* Read interface's value. */

diff --git a/nhrpd/nhrp_interface.c b/nhrpd/nhrp_interface.c
index a46962c91af37a41e073e3e91d2eab1fe344ad49..67e3f41b3d68225f108bbadd47306d3a209636cb 100644 (file)
--- a/nhrpd/nhrp_interface.c
+++ b/nhrpd/nhrp_interface.c
@@ -299,7 +299,7 @@ int nhrp_interface_delete(int cmd, struct zclient *client,
                return 0;
 
        debugf(NHRP_DEBUG_IF, "if-delete: %s", ifp->name);
-       ifp->ifindex = IFINDEX_INTERNAL;
+       if_set_index(ifp, ifp->ifindex);
        nhrp_interface_update(ifp);
        /* if_delete(ifp); */
        return 0;

diff --git a/ospf6d/ospf6_zebra.c b/ospf6d/ospf6_zebra.c
index 14dc6aa30b6adc9cf8ca8f26638fffb9d1a68e66..b032bd7a791c7429d07706fd67d18a3cfd4d1ee2 100644 (file)
--- a/ospf6d/ospf6_zebra.c
+++ b/ospf6d/ospf6_zebra.c
@@ -126,7 +126,7 @@ static int ospf6_zebra_if_del(int command, struct zclient *zclient,
   ospf6_interface_if_del (ifp);
 #endif /*0*/
 
-       ifp->ifindex = IFINDEX_INTERNAL;
+       if_set_index(ifp, IFINDEX_INTERNAL);
        return 0;
 }
 

diff --git a/ospfd/ospf_zebra.c b/ospfd/ospf_zebra.c
index a171e5f046f2ca64fc00d45b2192c48dfe5f2b90..3f94e6b4e2df41192a48ff2e892aeded80ce27d1 100644 (file)
--- a/ospfd/ospf_zebra.c
+++ b/ospfd/ospf_zebra.c
@@ -166,7 +166,7 @@ static int ospf_interface_delete(int command, struct zclient *zclient,
                if (rn->info)
                        ospf_if_free((struct ospf_interface *)rn->info);
 
-       ifp->ifindex = IFINDEX_INTERNAL;
+       if_set_index(ifp, IFINDEX_INTERNAL);
        return 0;
 }
 

diff --git a/ripd/rip_interface.c b/ripd/rip_interface.c
index 6fba0512e584fdb84a5658cc86d373f1daea69fa..21950c9f2bd01c37fb7ae2d444549af0e6ea50bf 100644 (file)
--- a/ripd/rip_interface.c
+++ b/ripd/rip_interface.c
@@ -471,7 +471,7 @@ int rip_interface_delete(int command, struct zclient *zclient,
 
        /* To support pseudo interface do not free interface structure.  */
        /* if_delete(ifp); */
-       ifp->ifindex = IFINDEX_INTERNAL;
+       if_set_index(ifp, IFINDEX_INTERNAL);
 
        return 0;
 }

diff --git a/ripngd/ripng_interface.c b/ripngd/ripng_interface.c
index dbee3ce69ff8a0b10e3d65ed6cc6688b55c70365..7203c906e19f783972026ebe2b8a3066e6df400a 100644 (file)
--- a/ripngd/ripng_interface.c
+++ b/ripngd/ripng_interface.c
@@ -299,7 +299,7 @@ int ripng_interface_delete(int command, struct zclient *zclient,
 
        /* To support pseudo interface do not free interface structure.  */
        /* if_delete(ifp); */
-       ifp->ifindex = IFINDEX_INTERNAL;
+       if_set_index(ifp, IFINDEX_INTERNAL);
 
        return 0;
 }

diff --git a/zebra/if_ioctl.c b/zebra/if_ioctl.c
index 54dbb555db379c13c24542d39f7b478ca95d5aa1..33e53551bf06ec6adaea343295e33aae34a69991 100644 (file)
--- a/zebra/if_ioctl.c
+++ b/zebra/if_ioctl.c
@@ -131,7 +131,7 @@ end:
 /* Get interface's index by ioctl. */
 static int if_get_index(struct interface *ifp)
 {
-       ifp->ifindex = if_nametoindex(ifp->name);
+       if_set_index(ifp, if_nametoindex(ifp->name));
        return ifp->ifindex;
 }
 

diff --git a/zebra/if_ioctl_solaris.c b/zebra/if_ioctl_solaris.c
index 145dc0ac50da067cb241f6396bc2532dd251d7a0..94738664b344a52cbd5d5cc76f0fef934f518abc 100644 (file)
--- a/zebra/if_ioctl_solaris.c
+++ b/zebra/if_ioctl_solaris.c
@@ -227,9 +227,9 @@ static int if_get_index(struct interface *ifp)
 
 /* OK we got interface index. */
 #ifdef ifr_ifindex
-       ifp->ifindex = lifreq.lifr_ifindex;
+       if_set_index(ifp, lifreq.lifr_ifindex);
 #else
-       ifp->ifindex = lifreq.lifr_index;
+       if_set_index(ifp, lifreq.lifr_index);
 #endif
        return ifp->ifindex;
 }

diff --git a/zebra/if_netlink.c b/zebra/if_netlink.c
index 0b97903ce0e5f0e78a244e11e340e726068cd673..a2235904c60668cfc9f7220417e36b0e6fc6e1aa 100644 (file)
--- a/zebra/if_netlink.c
+++ b/zebra/if_netlink.c
@@ -93,7 +93,7 @@ static void set_ifindex(struct interface *ifp, ifindex_t ifi_index,
                        if_delete_update(oifp);
                }
        }
-       ifp->ifindex = ifi_index;
+       if_set_index(ifp, ifi_index);
 }
 
 /* Utility function to parse hardware link-layer address and update ifp */

diff --git a/zebra/interface.c b/zebra/interface.c
index 1b11357a89ac324fdc977fd7485380f8ba1a63c9..fe724196421ed2672180edd5013595fa95ded994 100644 (file)
--- a/zebra/interface.c
+++ b/zebra/interface.c
@@ -685,7 +685,7 @@ void if_delete_update(struct interface *ifp)
           while processing the deletion.  Each client daemon is responsible
           for setting ifindex to IFINDEX_INTERNAL after processing the
           interface deletion message. */
-       ifp->ifindex = IFINDEX_INTERNAL;
+       if_set_index(ifp, IFINDEX_INTERNAL);
        ifp->node = NULL;
 
        /* if the ifp is in a vrf, move it to default so vrf can be deleted if

diff --git a/zebra/kernel_socket.c b/zebra/kernel_socket.c
index cbfc371199dc91b7c6c86bd58ddaecea44620589..89c933f90ff53f8d40e953bccfda264b81eb2d63 100644 (file)
--- a/zebra/kernel_socket.c
+++ b/zebra/kernel_socket.c
@@ -324,7 +324,7 @@ static int ifan_read(struct if_announcemsghdr *ifan)
 
                /* Create Interface */
                ifp = if_get_by_name(ifan->ifan_name, VRF_DEFAULT, 0);
-               ifp->ifindex = ifan->ifan_index;
+               if_set_index(ifp, ifan->ifan_index);
 
                if_get_metric(ifp);
                if_add_update(ifp);
@@ -528,7 +528,7 @@ int ifm_read(struct if_msghdr *ifm)
                 * Fill in newly created interface structure, or larval
                 * structure with ifindex IFINDEX_INTERNAL.
                 */
-               ifp->ifindex = ifm->ifm_index;
+               if_set_index(ifp, ifm->ifm_index);
 
 #ifdef HAVE_BSD_IFI_LINK_STATE /* translate BSD kernel msg for link-state */
                bsd_linkdetect_translate(ifm);