zebra: Refactor nexthop group creation code to use allocated memory

Simplify the code for nexthop hash entry creation. I made nexthop
hash entry creation expect the nexthop group and depends to always
be allocated before lookup. Before, it was only allocated if it had
dependencies. I think it makes the code a bit more readable to go
ahead an allocate even for single nexthops as well.

Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>

diff --git a/zebra/rt_netlink.c b/zebra/rt_netlink.c
index 581dae710d119f0417aeae2056033268198dab95..6753a22eccef5cce7a1b1611d89541b929086a84 100644 (file)
--- a/zebra/rt_netlink.c
+++ b/zebra/rt_netlink.c
@@ -1895,9 +1895,6 @@ static int netlink_nexthop(int cmd, struct zebra_dplane_ctx *ctx)
        // TODO: Scope?
 
        if (!nhe->id) {
-               // TODO: When we start using this with the ctx's we might not
-               // need to do ID assignment ourselves and just let the kernel
-               // handle it.
                flog_err(
                        EC_ZEBRA_NHG_FIB_UPDATE,
                        "Failed trying to update a nexthop group in the kernel that does not have an ID");
@@ -1907,9 +1904,7 @@ static int netlink_nexthop(int cmd, struct zebra_dplane_ctx *ctx)
        addattr32(&req.n, sizeof(req), NHA_ID, nhe->id);
 
        if (cmd == RTM_NEWNEXTHOP) {
-               // TODO: IF not a group
-               const struct nexthop_group nhg = nhe->nhg;
-               const struct nexthop *nh = nhg.nexthop;
+               const struct nexthop *nh = nhe->nhg->nexthop;
 
                if (nhe->afi == AFI_IP)
                        req.nhm.nh_family = AF_INET;
@@ -1917,8 +1912,6 @@ static int netlink_nexthop(int cmd, struct zebra_dplane_ctx *ctx)
                        req.nhm.nh_family = AF_INET6;
 
                switch (nh->type) {
-                       // TODO: Need AF for just index also
-                       // just use dest?
                case NEXTHOP_TYPE_IPV4_IFINDEX:
                        addattr_l(&req.n, sizeof(req), NHA_GATEWAY,
                                  &nh->gate.ipv4, IPV4_MAX_BYTELEN);
@@ -2100,26 +2093,28 @@ enum zebra_dplane_result kernel_route_update(struct zebra_dplane_ctx *ctx)
  *
  * Return:     New nexthop
  */
-static struct nexthop netlink_nexthop_process_nh(struct rtattr **tb,
-                                                unsigned char family,
-                                                struct interface **ifp,
-                                                ns_id_t ns_id)
+static struct nexthop *netlink_nexthop_process_nh(struct rtattr **tb,
+                                                 unsigned char family,
+                                                 struct interface **ifp,
+                                                 ns_id_t ns_id)
 {
-       struct nexthop nh = {0};
+       struct nexthop *nh = NULL;
        void *gate = NULL;
+       enum nexthop_types_t type = 0;
        int if_index;
        size_t sz;
 
        if_index = *(int *)RTA_DATA(tb[NHA_OIF]);
 
+
        if (tb[NHA_GATEWAY]) {
                switch (family) {
                case AF_INET:
-                       nh.type = NEXTHOP_TYPE_IPV4_IFINDEX;
+                       type = NEXTHOP_TYPE_IPV4_IFINDEX;
                        sz = 4;
                        break;
                case AF_INET6:
-                       nh.type = NEXTHOP_TYPE_IPV6_IFINDEX;
+                       type = NEXTHOP_TYPE_IPV6_IFINDEX;
                        sz = 16;
                        break;
                default:
@@ -2127,26 +2122,35 @@ static struct nexthop netlink_nexthop_process_nh(struct rtattr **tb,
                                EC_ZEBRA_BAD_NHG_MESSAGE,
                                "Nexthop gateway with bad address family (%d) received from kernel",
                                family);
-                       return nh;
+                       return NULL;
                }
                gate = RTA_DATA(tb[NHA_GATEWAY]);
-               memcpy(&(nh.gate), gate, sz);
        } else {
-               nh.type = NEXTHOP_TYPE_IFINDEX;
+               type = NEXTHOP_TYPE_IFINDEX;
        }
 
-       nh.ifindex = if_index;
+       /* Allocate the new nexthop */
+       nh = nexthop_new();
 
-       *ifp = if_lookup_by_index_per_ns(zebra_ns_lookup(ns_id), nh.ifindex);
+       if (type)
+               nh->type = type;
+
+       if (gate)
+               memcpy(&(nh->gate), gate, sz);
+
+       if (if_index)
+               nh->ifindex = if_index;
+
+       *ifp = if_lookup_by_index_per_ns(zebra_ns_lookup(ns_id), nh->ifindex);
        if (ifp) {
-               nh.vrf_id = (*ifp)->vrf_id;
+               nh->vrf_id = (*ifp)->vrf_id;
        } else {
                flog_warn(
                        EC_ZEBRA_UNKNOWN_INTERFACE,
                        "%s: Unknown nexthop interface %u received, defaulting to VRF_DEFAULT",
-                       __PRETTY_FUNCTION__, nh.ifindex);
+                       __PRETTY_FUNCTION__, nh->ifindex);
 
-               nh.vrf_id = VRF_DEFAULT;
+               nh->vrf_id = VRF_DEFAULT;
        }
 
        if (tb[NHA_ENCAP] && tb[NHA_ENCAP_TYPE]) {
@@ -2159,7 +2163,7 @@ static struct nexthop netlink_nexthop_process_nh(struct rtattr **tb,
                }
 
                if (num_labels) {
-                       nexthop_add_labels(&nh, ZEBRA_LSP_STATIC, num_labels,
+                       nexthop_add_labels(nh, ZEBRA_LSP_STATIC, num_labels,
                                           labels);
                }
        }
@@ -2215,7 +2219,8 @@ static int netlink_nexthop_process_group(struct rtattr **tb,
                         * in the kernel.
                         */
 
-                       copy_nexthops(&nhg->nexthop, depend->nhg.nexthop, NULL);
+                       copy_nexthops(&nhg->nexthop, depend->nhg->nexthop,
+                                     NULL);
                } else {
                        flog_err(
                                EC_ZEBRA_NHG_SYNC,
@@ -2245,9 +2250,8 @@ int netlink_nexthop_change(struct nlmsghdr *h, ns_id_t ns_id, int startup)
        struct interface *ifp = NULL;
        struct nhmsg *nhm = NULL;
        /* struct for nexthop group abstraction  */
-       struct nexthop_group nhg = {0};
-       /* zebra's version of nexthops */
-       struct nexthop nh = {0};
+       struct nexthop_group *nhg = NULL;
+       struct nexthop *nh = NULL;
        /* If its a group, array of nexthops */
        struct list *nhg_depends = NULL;
        /* Count of nexthops in group array */
@@ -2296,12 +2300,14 @@ int netlink_nexthop_change(struct nlmsghdr *h, ns_id_t ns_id, int startup)
        nhe = zebra_nhg_lookup_id(id);
 
        if (h->nlmsg_type == RTM_NEWNEXTHOP) {
+               nhg = nexthop_group_new();
+
                if (tb[NHA_GROUP]) {
                        /**
                         * If this is a group message its only going to have
                         * an array of nexthop IDs associated with it
                         */
-                       dep_count = netlink_nexthop_process_group(tb, &nhg,
+                       dep_count = netlink_nexthop_process_group(tb, nhg,
                                                                  &nhg_depends);
                } else {
                        if (tb[NHA_BLACKHOLE]) {
@@ -2310,8 +2316,9 @@ int netlink_nexthop_change(struct nlmsghdr *h, ns_id_t ns_id, int startup)
                                 * traffic, it should not have an OIF, GATEWAY,
                                 * or ENCAP
                                 */
-                               nh.type = NEXTHOP_TYPE_BLACKHOLE;
-                               nh.bh_type = BLACKHOLE_UNSPEC;
+                               nh = nexthop_new();
+                               nh->type = NEXTHOP_TYPE_BLACKHOLE;
+                               nh->bh_type = BLACKHOLE_UNSPEC;
                        } else if (tb[NHA_OIF]) {
                                /**
                                 * This is a true new nexthop, so we need
@@ -2320,40 +2327,69 @@ int netlink_nexthop_change(struct nlmsghdr *h, ns_id_t ns_id, int startup)
                                nh = netlink_nexthop_process_nh(tb, family,
                                                                &ifp, ns_id);
                        }
-                       SET_FLAG(nh.flags, NEXTHOP_FLAG_ACTIVE);
-                       if (nhm->nh_flags & RTNH_F_ONLINK)
-                               SET_FLAG(nh.flags, NEXTHOP_FLAG_ONLINK);
+                       if (nh) {
+                               SET_FLAG(nh->flags, NEXTHOP_FLAG_ACTIVE);
+                               if (nhm->nh_flags & RTNH_F_ONLINK)
+                                       SET_FLAG(nh->flags,
+                                                NEXTHOP_FLAG_ONLINK);
 
-                       nexthop_group_add_sorted(&nhg, &nh);
+                               nexthop_group_add_sorted(nhg, nh);
+                       } else {
+                               flog_warn(
+                                       EC_ZEBRA_BAD_NHG_MESSAGE,
+                                       "Invalid Nexthop message received from the kernel with ID (%u)",
+                                       id);
+                               return -1;
+                       }
                }
 
-               if (!nhg.nexthop)
+               if (!nhg->nexthop) {
+                       /* Nothing to lookup */
+                       zebra_nhg_free_group_depends(nhg, nhg_depends);
                        return -1;
+               }
 
                if (nhe) {
                        /* This is a change to a group we already have
                         */
-                       nexthops_free(nhe->nhg.nexthop);
-                       if (dep_count) {
-                               list_delete(&nhe->nhg_depends);
+
+                       /* Free what's already there */
+                       zebra_nhg_free_members(nhe);
+
+                       /* Update with new info */
+                       nhe->nhg = nhg;
+                       if (dep_count)
                                nhe->nhg_depends = nhg_depends;
-                               /* Group is already allocated with depends */
-                               // TODO: Maybe better to just allocate both
-                               // rather than doing each differently?
-                               nhe->nhg = nhg;
-                       } else {
-                               nhe->nhg.nexthop = NULL;
-                               nexthop_group_copy(&nhe->nhg, &nhg);
-                       }
+
                } else {
                        /* This is a new nexthop group */
-                       nhe = zebra_nhg_find(&nhg, nh.vrf_id, afi, id,
+                       nhe = zebra_nhg_find(nhg, nhg->nexthop->vrf_id, afi, id,
                                             nhg_depends, dep_count);
-                       if (nhe) {
-                               nhe->is_kernel_nh = true;
-                       } else {
+                       if (!nhe) {
+                               flog_err(
+                                       EC_ZEBRA_TABLE_LOOKUP_FAILED,
+                                       "Zebra failed to find or create a nexthop hash entry for ID (%u) from the kernel",
+                                       id);
+                               zebra_nhg_free_group_depends(nhg, nhg_depends);
                                return -1;
                        }
+
+                       nhe->is_kernel_nh = true;
+                       if (id != nhe->id) {
+                               /* Duplicate but with different ID from
+                                * the kernel */
+
+                               /* The kernel allows duplicate nexthops
+                                * as long as they have different IDs.
+                                * We are ignoring those to prevent
+                                * syncing problems with the kernel
+                                * changes.
+                                */
+                               flog_warn(
+                                       EC_ZEBRA_DUPLICATE_NHG_MESSAGE,
+                                       "Nexthop Group from kernel with ID (%d) is a duplicate, ignoring",
+                                       id);
+                       }
                }
                if (ifp) {
                        /* Add the nhe to the interface's list

diff --git a/zebra/zebra_dplane.c b/zebra/zebra_dplane.c
index f20242bfc2c417cbc1dd5b8ed03862d5b8e3f134..0e806ff7f74ea4107a259660a8bb712e83c39b64 100644 (file)
--- a/zebra/zebra_dplane.c
+++ b/zebra/zebra_dplane.c
@@ -96,7 +96,6 @@ struct dplane_route_info {
        uint32_t zd_nexthop_mtu;
 
        /* Nexthop hash entry */
-       // TODO: Adjust the others as needed
        struct nhg_hash_entry zd_nhe;
 
        /* Nexthops */
@@ -471,7 +470,7 @@ static void dplane_ctx_free(struct zebra_dplane_ctx **pctx)
        case DPLANE_OP_NH_INSTALL:
        case DPLANE_OP_NH_UPDATE:
        case DPLANE_OP_NH_DELETE: {
-               nexthops_free((*pctx)->u.rinfo.zd_nhe.nhg.nexthop);
+               zebra_nhg_free_members(&(*pctx)->u.rinfo.zd_nhe);
                break;
        }
 
@@ -1495,8 +1494,7 @@ static int dplane_ctx_nexthop_init(struct zebra_dplane_ctx *ctx,
                                   enum dplane_op_e op,
                                   struct nhg_hash_entry *nhe)
 {
-       struct zebra_ns *zns;
-       struct zebra_vrf *zvrf;
+       struct zebra_ns *zns = NULL;
 
        int ret = EINVAL;
 
@@ -1507,13 +1505,23 @@ static int dplane_ctx_nexthop_init(struct zebra_dplane_ctx *ctx,
        ctx->zd_status = ZEBRA_DPLANE_REQUEST_SUCCESS;
 
        /* Copy over nhe info */
-       ctx->u.rinfo.zd_nhe = *nhe;
-       ctx->u.rinfo.zd_nhe.nhg.nexthop = NULL;
-       nexthop_group_copy(&(ctx->u.rinfo.zd_nhe.nhg), &nhe->nhg);
+       ctx->u.rinfo.zd_nhe.id = nhe->id;
+       ctx->u.rinfo.zd_nhe.vrf_id = nhe->vrf_id;
+       ctx->u.rinfo.zd_nhe.afi = nhe->afi;
+       ctx->u.rinfo.zd_nhe.refcnt = nhe->refcnt;
+       ctx->u.rinfo.zd_nhe.is_kernel_nh = nhe->is_kernel_nh;
+       ctx->u.rinfo.zd_nhe.dplane_ref = nhe->dplane_ref;
+       ctx->u.rinfo.zd_nhe.ifp = nhe->ifp;
+
+       ctx->u.rinfo.zd_nhe.nhg = nexthop_group_new();
+       nexthop_group_copy(ctx->u.rinfo.zd_nhe.nhg, nhe->nhg);
+
+       if (nhe->nhg_depends)
+               ctx->u.rinfo.zd_nhe.nhg_depends = list_dup(nhe->nhg_depends);
+
 
        /* Extract ns info - can't use pointers to 'core' structs */
-       zvrf = vrf_info_lookup(nhe->vrf_id);
-       zns = zvrf->zns;
+       zns = ((struct zebra_vrf *)vrf_info_lookup(nhe->vrf_id))->zns;
 
        // TODO: Might not need to mark this as an update, since
        // it probably won't require two messages

diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c
index 789e371da52aad445facf2e4ae6a2a241c796b21..522bf6a9a2538410800ba8c3c5f1a5dbf9f59d6a 100644 (file)
--- a/zebra/zebra_nhg.c
+++ b/zebra/zebra_nhg.c
@@ -161,17 +161,11 @@ static void *zebra_nhg_alloc(void *arg)
        pthread_mutex_unlock(&lock);
 
        nhe->nhg_depends = NULL;
-       nhe->nhg.nexthop = NULL;
 
-       if (copy->nhg_depends) {
+       if (copy->nhg_depends)
                nhe->nhg_depends = copy->nhg_depends;
-               /* These have already been allocated when
-                * building the dependency list
-                */
-               nhe->nhg = copy->nhg;
-       } else {
-               nexthop_group_copy(&nhe->nhg, &copy->nhg);
-       }
+
+       nhe->nhg = copy->nhg;
 
        nhe->vrf_id = copy->vrf_id;
        nhe->afi = copy->afi;
@@ -232,7 +226,7 @@ uint32_t zebra_nhg_hash_key(const void *arg)
 
        key = jhash_2words(nhe->vrf_id, nhe->afi, key);
 
-       key = jhash_1word(zebra_nhg_hash_key_nexthop_group(&nhe->nhg), key);
+       key = jhash_1word(zebra_nhg_hash_key_nexthop_group(nhe->nhg), key);
 
        return key;
 }
@@ -261,9 +255,9 @@ bool zebra_nhg_hash_equal(const void *arg1, const void *arg2)
         * Again we are not interested in looking at any recursively
         * resolved nexthops.  Top level only
         */
-       for (nh1 = nhe1->nhg.nexthop; nh1; nh1 = nh1->next) {
+       for (nh1 = nhe1->nhg->nexthop; nh1; nh1 = nh1->next) {
                uint32_t inner_nh_count = 0;
-               for (nh2 = nhe2->nhg.nexthop; nh2; nh2 = nh2->next) {
+               for (nh2 = nhe2->nhg->nexthop; nh2; nh2 = nh2->next) {
                        if (inner_nh_count == nh_count) {
                                break;
                        }
@@ -290,16 +284,16 @@ bool zebra_nhg_hash_id_equal(const void *arg1, const void *arg2)
 /**
  * zebra_nhg_find() - Find the zebra nhg in our table, or create it
  *
- * @nhg:       Nexthop group we lookup with
- * @vrf_id:    VRF id
- * @afi:       Address Family type
- * @id:                ID we lookup with, 0 means its from us and we need to give it
- *             an ID, otherwise its from the kernel as we use the ID it gave
- *             us.
- * @dep_info:  Array of nexthop dependency info (ID/weight)
- * @dep_count: Count for the number of nexthop dependencies
+ * @nhg:               Nexthop group we lookup with
+ * @vrf_id:            VRF id
+ * @afi:               Address Family type
+ * @id:                        ID we lookup with, 0 means its from us and we
+ *                     need to give it an ID, otherwise its from the
+ *                     kernel as we use the ID it gave us.
+ * @nhg_depends:       Nexthop dependencies
+ * @dep_count:         Count for the number of nexthop dependencies
  *
- * Return:     Hash entry found or created
+ * Return:             Hash entry found or created
  *
  * The nhg and n_grp are fundementally the same thing (a group of nexthops).
  * We are just using the nhg representation with routes and the n_grp
@@ -387,10 +381,7 @@ void zebra_nhg_free(void *arg)
 
        nhe = (struct nhg_hash_entry *)arg;
 
-       if (nhe->nhg_depends)
-               list_delete(&nhe->nhg_depends);
-
-       nexthops_free(nhe->nhg.nexthop);
+       zebra_nhg_free_members(nhe);
 
        XFREE(MTYPE_NHG, nhe);
 }

diff --git a/zebra/zebra_nhg.h b/zebra/zebra_nhg.h
index 287aaf275f7964c74185f6a732d553b4e5ef8948..634a416e1bef62b50515c01440cddc1bbf8ddba8 100644 (file)
--- a/zebra/zebra_nhg.h
+++ b/zebra/zebra_nhg.h
@@ -34,7 +34,7 @@ struct nhg_hash_entry {
        vrf_id_t vrf_id;
        bool is_kernel_nh;
 
-       struct nexthop_group nhg;
+       struct nexthop_group *nhg;
 
        /* If this is not a group, it
         * will be a single nexthop

diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c
index d71239abe9cef4c0931f2e2092fa270c17373f90..e882c9d6944129942e329409de6f5284a417eaec 100644 (file)
--- a/zebra/zebra_rib.c
+++ b/zebra/zebra_rib.c
@@ -2648,6 +2648,7 @@ int rib_add_multipath(afi_t afi, safi_t safi, struct prefix *p,
        /* Lookup table.  */
        table = zebra_vrf_table_with_table_id(afi, safi, re->vrf_id, re->table);
        if (!table) {
+               zebra_nhg_free_group_depends(re->ng, NULL);
                XFREE(MTYPE_RE, re);
                return 0;
        }
@@ -2657,27 +2658,13 @@ int rib_add_multipath(afi_t afi, safi_t safi, struct prefix *p,
        if (src_p)
                apply_mask_ipv6(src_p);
 
-       /* Using a kernel that supports nexthop ojects */
-       if (re->nhe_id) {
-               nhe = zebra_nhg_lookup_id(re->nhe_id);
-       } else {
-               nhe = zebra_nhg_find(re->ng, re->vrf_id, afi, 0, NULL, 0);
-               re->nhe_id = nhe->id;
-       }
+       nhe = zebra_nhg_find(re->ng, re->vrf_id, afi, re->nhe_id, NULL, 0);
 
        if (nhe) {
                // TODO: Add interface pointer
+               re->ng = nhe->nhg;
+               re->nhe_id = nhe->id;
                nhe->refcnt++;
-
-               /* Freeing the nexthop structs we were using
-                * for lookup since it will just point
-                * to the hash entry group now.
-                */
-               nexthops_free(re->ng->nexthop);
-               nexthop_group_delete(&re->ng);
-               /* Point to hash entry group */
-               re->ng = &nhe->nhg;
-
        } else {
                flog_err(
                        EC_ZEBRA_TABLE_LOOKUP_FAILED,

diff --git a/zebra/zebra_vty.c b/zebra/zebra_vty.c
index 7cf82118dfc55fd30c8ce83baac936ecf129d47b..c2a9d091d2deb99f4a9ca9c50a00b875675ac0bd 100644 (file)
--- a/zebra/zebra_vty.c
+++ b/zebra/zebra_vty.c
@@ -1138,7 +1138,7 @@ static void show_nexthop_group_cmd_helper(struct vty *vty,
                        vty_out(vty, "\n");
                }
 
-               for (ALL_NEXTHOPS(nhe->nhg, nhop)) {
+               for (ALL_NEXTHOPS_PTR(nhe->nhg, nhop)) {
                        vty_out(vty, "\t");
                        nexthop_group_write_nexthop(vty, nhop);
                }