Claudio Jeker(cje...@diehard.n-r-g.com) on 2019.06.25 11:13:55 +0200:
> In bgpd there are a few objects that use reference counts to keep track on
> how many things point to them. Those are struct pt_entry, rde_aspath,
> rde_communities, and nexthop. The way this reference counting is done and
> especially the cleanup is a bit different everywhere. This diff is making
> them more similar by calling all functions _ref and _unref also the _unref
> function will remove the element once the last reference is dropped.
> There are still some internal differences but all in all for the callers
> this makes the code look a lot more similar.
>
> OK?
ok
> --
> :wq Claudio
>
> Index: rde.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> retrieving revision 1.474
> diff -u -p -r1.474 rde.c
> --- rde.c 25 Jun 2019 09:04:42 -0000 1.474
> +++ rde.c 25 Jun 2019 09:05:27 -0000
> @@ -1353,7 +1353,7 @@ rde_update_dispatch(struct imsg *imsg)
> }
>
> /* unlock the previously locked nexthop, it is no longer used */
> - (void)nexthop_put(state.nexthop);
> + nexthop_unref(state.nexthop);
> state.nexthop = NULL;
> if ((pos = rde_get_mp_nexthop(mpp, mplen, aid, &state)) == -1) {
> log_peer_warnx(&peer->conf, "bad nlri nexthop");
> @@ -1653,7 +1653,7 @@ bad_flags:
> op, len);
> return (-1);
> }
> - nexthop_put(state->nexthop); /* just to be sure */
> + nexthop_unref(state->nexthop); /* just to be sure */
> state->nexthop = nexthop_get(&nexthop);
> break;
> case ATTR_MED:
> @@ -2015,7 +2015,7 @@ rde_get_mp_nexthop(u_char *data, u_int16
> return (-1);
> }
>
> - nexthop_put(state->nexthop); /* just to be sure */
> + nexthop_unref(state->nexthop); /* just to be sure */
> state->nexthop = nexthop_get(&nexthop);
>
> /* ignore reserved (old SNPA) field as per RFC4760 */
> Index: rde.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
> retrieving revision 1.218
> diff -u -p -r1.218 rde.h
> --- rde.h 24 Jun 2019 06:39:49 -0000 1.218
> +++ rde.h 25 Jun 2019 09:05:27 -0000
> @@ -423,7 +423,7 @@ void communities_copy(struct rde_commun
> void communities_clean(struct rde_community *);
>
> static inline struct rde_community *
> -communities_get(struct rde_community *comm)
> +communities_ref(struct rde_community *comm)
> {
> if (comm->refcnt == 0)
> fatalx("%s: not-referenced community", __func__);
> @@ -433,12 +433,12 @@ communities_get(struct rde_community *co
> }
>
> static inline void
> -communities_put(struct rde_community *comm)
> +communities_unref(struct rde_community *comm)
> {
> if (comm == NULL)
> return;
> rdemem.comm_refs--;
> - if (--comm->refcnt == 1)
> + if (--comm->refcnt == 1) /* last ref is hold internally */
> communities_unlink(comm);
> }
>
> @@ -460,11 +460,15 @@ int rde_filter_equal(struct filter_hea
> void rde_filter_calc_skip_steps(struct filter_head *);
>
> /* rde_prefix.c */
> -static inline int
> -pt_empty(struct pt_entry *pt)
> -{
> - return (pt->refcnt == 0);
> -}
> +void pt_init(void);
> +void pt_shutdown(void);
> +void pt_getaddr(struct pt_entry *, struct bgpd_addr *);
> +struct pt_entry *pt_fill(struct bgpd_addr *, int);
> +struct pt_entry *pt_get(struct bgpd_addr *, int);
> +struct pt_entry *pt_add(struct bgpd_addr *, int);
> +void pt_remove(struct pt_entry *);
> +struct pt_entry *pt_lookup(struct bgpd_addr *);
> +int pt_prefix_cmp(const struct pt_entry *, const struct pt_entry *);
>
> static inline struct pt_entry *
> pt_ref(struct pt_entry *pt)
> @@ -475,25 +479,15 @@ pt_ref(struct pt_entry *pt)
> return pt;
> }
>
> -static inline int
> +static inline void
> pt_unref(struct pt_entry *pt)
> {
> if (pt->refcnt == 0)
> fatalx("pt_unref: underflow");
> - --pt->refcnt;
> - return pt_empty(pt);
> + if (--pt->refcnt == 0)
> + pt_remove(pt);
> }
>
> -void pt_init(void);
> -void pt_shutdown(void);
> -void pt_getaddr(struct pt_entry *, struct bgpd_addr *);
> -struct pt_entry *pt_fill(struct bgpd_addr *, int);
> -struct pt_entry *pt_get(struct bgpd_addr *, int);
> -struct pt_entry *pt_add(struct bgpd_addr *, int);
> -void pt_remove(struct pt_entry *);
> -struct pt_entry *pt_lookup(struct bgpd_addr *);
> -int pt_prefix_cmp(const struct pt_entry *, const struct pt_entry *);
> -
> /* rde_rib.c */
> extern u_int16_t rib_size;
> extern struct rib_desc *ribs;
> @@ -608,7 +602,7 @@ void nexthop_unlink(struct prefix *);
> void nexthop_update(struct kroute_nexthop *);
> struct nexthop *nexthop_get(struct bgpd_addr *);
> struct nexthop *nexthop_ref(struct nexthop *);
> -int nexthop_put(struct nexthop *);
> +int nexthop_unref(struct nexthop *);
> int nexthop_compare(struct nexthop *, struct nexthop *);
>
> /* rde_update.c */
> Index: rde_filter.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_filter.c,v
> retrieving revision 1.120
> diff -u -p -r1.120 rde_filter.c
> --- rde_filter.c 22 Jun 2019 05:44:05 -0000 1.120
> +++ rde_filter.c 25 Jun 2019 09:05:27 -0000
> @@ -444,7 +444,7 @@ rde_filterstate_clean(struct filterstate
> {
> path_clean(&state->aspath);
> communities_clean(&state->communities);
> - nexthop_put(state->nexthop);
> + nexthop_unref(state->nexthop);
> state->nexthop = NULL;
> }
>
> @@ -481,7 +481,7 @@ filterset_free(struct filter_set_head *s
> pftable_unref(s->action.id);
> else if (s->type == ACTION_SET_NEXTHOP &&
> bgpd_process == PROC_RDE)
> - nexthop_put(s->action.nh);
> + nexthop_unref(s->action.nh);
> free(s);
> }
> }
> Index: rde_prefix.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_prefix.c,v
> retrieving revision 1.38
> diff -u -p -r1.38 rde_prefix.c
> --- rde_prefix.c 15 Feb 2019 09:55:21 -0000 1.38
> +++ rde_prefix.c 25 Jun 2019 09:05:27 -0000
> @@ -188,7 +188,7 @@ pt_add(struct bgpd_addr *prefix, int pre
> void
> pt_remove(struct pt_entry *pte)
> {
> - if (!pt_empty(pte))
> + if (pte->refcnt != 0)
> fatalx("pt_remove: entry still holds references");
>
> if (RB_REMOVE(pt_tree, &pttable, pte) == NULL)
> Index: rde_rib.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v
> retrieving revision 1.195
> diff -u -p -r1.195 rde_rib.c
> --- rde_rib.c 22 Jun 2019 05:44:05 -0000 1.195
> +++ rde_rib.c 25 Jun 2019 09:05:27 -0000
> @@ -62,11 +62,11 @@ struct rib_context {
> };
> LIST_HEAD(, rib_context) rib_dumps = LIST_HEAD_INITIALIZER(rib_dumps);
>
> -static int prefix_add(struct bgpd_addr *, int, struct rib *,
> +static int prefix_add(struct bgpd_addr *, int, struct rib *,
> struct rde_peer *, struct rde_aspath *,
> struct rde_community *, struct nexthop *,
> u_int8_t, u_int8_t);
> -static int prefix_move(struct prefix *, struct rde_peer *,
> +static int prefix_move(struct prefix *, struct rde_peer *,
> struct rde_aspath *, struct rde_community *,
> struct nexthop *, u_int8_t, u_int8_t);
>
> @@ -326,8 +326,7 @@ rib_remove(struct rib_entry *re)
> /* entry is locked, don't free it. */
> return;
>
> - if (pt_unref(re->prefix))
> - pt_remove(re->prefix);
> + pt_unref(re->prefix);
>
> if (RB_REMOVE(rib_tree, rib_tree(re_rib(re)), re) == NULL)
> log_warnx("rib_remove: remove failed.");
> @@ -501,12 +500,6 @@ SIPHASH_KEY pathtablekey;
>
> #define PATH_HASH(x) &pathtable.path_hashtbl[x &
> pathtable.path_hashmask]
>
> -static inline int
> -path_empty(struct rde_aspath *asp)
> -{
> - return (asp == NULL || asp->refcnt <= 0);
> -}
> -
> static inline struct rde_aspath *
> path_ref(struct rde_aspath *asp)
> {
> @@ -521,10 +514,14 @@ path_ref(struct rde_aspath *asp)
> static inline void
> path_unref(struct rde_aspath *asp)
> {
> + if (asp == NULL)
> + return;
> if ((asp->flags & F_ATTR_LINKED) == 0)
> fatalx("%s: unlinked object", __func__);
> asp->refcnt--;
> rdemem.path_refs--;
> + if (asp->refcnt <= 0)
> + path_unlink(asp);
> }
>
> void
> @@ -765,8 +762,8 @@ path_unlink(struct rde_aspath *asp)
> return;
>
> /* make sure no reference is hold for this rde_aspath */
> - if (!path_empty(asp))
> - fatalx("%s: still has prefixes", __func__);
> + if (asp->refcnt != 0)
> + fatalx("%s: still holds references", __func__);
>
> LIST_REMOVE(asp, path_l);
> asp->flags &= ~F_ATTR_LINKED;
> @@ -938,7 +935,7 @@ prefix_move(struct prefix *p, struct rde
> np = prefix_alloc();
> /* add reference to new AS path and communities */
> np->aspath = path_ref(asp);
> - np->communities = communities_get(comm);
> + np->communities = communities_ref(comm);
> np->peer = peer;
> np->pt = p->pt; /* skip refcnt update since ref is moved */
> np->re = p->re;
> @@ -970,10 +967,9 @@ prefix_move(struct prefix *p, struct rde
>
> /* destroy all references to other objects and free the old prefix */
> nexthop_unlink(p);
> - nexthop_put(p->nexthop);
> - communities_put(p->communities);
> - if (oasp)
> - path_unref(oasp);
> + nexthop_unref(p->nexthop);
> + communities_unref(p->communities);
> + path_unref(oasp);
> p->communities = NULL;
> p->nexthop = NULL;
> p->aspath = NULL;
> @@ -982,10 +978,6 @@ prefix_move(struct prefix *p, struct rde
> p->pt = NULL;
> prefix_free(p);
>
> - /* destroy old path if empty */
> - if (path_empty(oasp))
> - path_unlink(oasp);
> -
> return (0);
> }
>
> @@ -1061,28 +1053,22 @@ prefix_withdraw(struct rib *rib, struct
> struct bgpd_addr *prefix, int prefixlen)
> {
> struct prefix *p;
> - struct rde_aspath *asp;
>
> p = prefix_get(rib, peer, prefix, prefixlen);
> if (p == NULL) /* Got a dummy withdrawn request. */
> return (0);
>
> /* unlink from aspath ...*/
> - asp = p->aspath;
> - if (asp != NULL) {
> - path_unref(asp);
> - p->aspath = NULL;
> - if (path_empty(asp))
> - path_unlink(asp);
> - }
> + path_unref(p->aspath);
> + p->aspath = NULL;
>
> /* ... communities ... */
> - communities_put(p->communities);
> + communities_unref(p->communities);
> p->communities = NULL;
>
> /* ... and nexthop but keep the re link */
> nexthop_unlink(p);
> - nexthop_put(p->nexthop);
> + nexthop_unref(p->nexthop);
> p->nexthop = NULL;
> p->nhflags = 0;
> /* re link still exists */
> @@ -1271,14 +1257,8 @@ prefix_updateall(struct prefix *p, enum
> void
> prefix_destroy(struct prefix *p)
> {
> - struct rde_aspath *asp;
> -
> - asp = prefix_aspath(p);
> prefix_unlink(p);
> prefix_free(p);
> -
> - if (path_empty(asp))
> - path_unlink(asp);
> }
>
> /*
> @@ -1290,7 +1270,7 @@ prefix_link(struct prefix *p, struct rib
> struct nexthop *nexthop, u_int8_t nhflags, u_int8_t vstate)
> {
> p->aspath = path_ref(asp);
> - p->communities = communities_get(comm);
> + p->communities = communities_ref(comm);
> p->peer = peer;
> p->pt = pt_ref(re->prefix);
> p->re = re;
> @@ -1321,12 +1301,10 @@ prefix_unlink(struct prefix *p)
>
> /* destroy all references to other objects */
> nexthop_unlink(p);
> - nexthop_put(p->nexthop);
> - communities_put(p->communities);
> - if (p->aspath)
> - path_unref(p->aspath);
> - if (pt_unref(p->pt))
> - pt_remove(p->pt);
> + nexthop_unref(p->nexthop);
> + communities_unref(p->communities);
> + path_unref(p->aspath);
> + pt_unref(p->pt);
> p->communities = NULL;
> p->nexthop = NULL;
> p->aspath = NULL;
> @@ -1418,7 +1396,7 @@ nexthop_shutdown(void)
> nh != NULL; nh = nnh) {
> nnh = LIST_NEXT(nh, nexthop_l);
> nh->state = NEXTHOP_UNREACH;
> - (void)nexthop_put(nh);
> + nexthop_unref(nh);
> }
> if (!LIST_EMPTY(&nexthoptable.nexthop_hashtbl[i])) {
> nh = LIST_FIRST(&nexthoptable.nexthop_hashtbl[i]);
> @@ -1481,7 +1459,7 @@ nexthop_update(struct kroute_nexthop *ms
> nh->oldstate = nh->state;
> if (nh->oldstate == NEXTHOP_LOOKUP)
> /* drop reference which was hold during the lookup */
> - if (nexthop_put(nh))
> + if (nexthop_unref(nh))
> return; /* nh lost last ref, no work left */
>
> if (nh->next_prefix) {
> @@ -1546,7 +1524,7 @@ nexthop_modify(struct nexthop *setnh, en
> */
> if (aid != setnh->exit_nexthop.aid)
> break;
> - nexthop_put(*nexthop);
> + nexthop_unref(*nexthop);
> *nexthop = nexthop_ref(setnh);
> *flags = 0;
> break;
> @@ -1617,11 +1595,10 @@ nexthop_ref(struct nexthop *nexthop)
> }
>
> int
> -nexthop_put(struct nexthop *nh)
> +nexthop_unref(struct nexthop *nh)
> {
> if (nh == NULL)
> return (0);
> -
> if (--nh->refcnt > 0)
> return (0);
>
>
