Skip to content

bgpd: fix attr intern overhead #19547

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

bgpd: fix attr intern overhead #19547

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions bgpd/bgp_attr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1052,6 +1052,74 @@ static void attrhash_init(void)
hash_create(attrhash_key_make, attrhash_cmp, "BGP Attributes");
}

/*
* Increment the refcnt for intern'd attr and its intern'd subs
* If any intern'd sub-object in 'struct attr' is added/deleted, update this function
*/
void bgp_attr_ref(struct attr *attr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, have some reservations about this. this is a ton of exposure of internal details that will have to be maintained forever - just to be used in exactly one place in bgp_route.c (right?)
could we find a way to do this through the attr_intern() code path - like maybe an adaptation or variant of that to take a reuse context? that might allow all the maintenance of the attr struct internals to remain inside attr_intern and _unintern?

{
struct community *comm = bgp_attr_get_community(attr);
struct ecommunity *ecomm = bgp_attr_get_ecommunity(attr);
struct ecommunity *v6ecomm = bgp_attr_get_ipv6_ecommunity(attr);
struct lcommunity *lcomm = bgp_attr_get_lcommunity(attr);
struct cluster_list *cluster = bgp_attr_get_cluster(attr);
struct transit *transit = bgp_attr_get_transit(attr);
struct bgp_route_evpn *bre = bgp_attr_get_evpn_overlay(attr);
struct bgp_nhc *nhc = bgp_attr_get_nhc(attr);

if (!attr)
return;

/* Increment the attr itself */
attr->refcnt++;

/* Increment the attr subs */
if (attr->aspath)
attr->aspath->refcnt++;

#ifdef ENABLE_BGP_VNC
struct bgp_attr_encap_subtlv *vnc_subtlvs = bgp_attr_get_vnc_subtlvs(attr);
#endif

if (comm)
comm->refcnt++;

if (ecomm)
ecomm->refcnt++;

if (v6ecomm)
v6ecomm->refcnt++;

if (lcomm)
lcomm->refcnt++;

if (cluster)
cluster->refcnt++;

if (transit)
transit->refcnt++;

if (attr->encap_subtlvs)
attr->encap_subtlvs->refcnt++;

if (bre)
bre->refcnt++;

if (attr->srv6_l3vpn)
attr->srv6_l3vpn->refcnt++;

if (attr->srv6_vpn)
attr->srv6_vpn->refcnt++;

#ifdef ENABLE_BGP_VNC
if (vnc_subtlvs)
vnc_subtlvs->refcnt++;
#endif

if (nhc)
nhc->refcnt++;
}

/*
* special for hash_clean below
*/
Expand Down
7 changes: 6 additions & 1 deletion bgpd/bgp_attr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,11 @@ struct bgp_attr_srv6_l3vpn {
uint8_t transposition_offset;
};

/* BGP core attribute structure. */
/*
* BGP core attribute structure.
* If you add/remove a sub-object in this struct, update the bgp_attr_ref
* appropriately to keep in sync with attr lifecycle.
*/
struct attr {
/* AS Path structure */
struct aspath *aspath;
Expand Down Expand Up @@ -399,6 +403,7 @@ extern void bgp_dump_routes_attr(struct stream *s, struct bgp_path_info *bpi,
const struct prefix *p);
extern bool attrhash_cmp(const void *arg1, const void *arg2);
extern unsigned int attrhash_key_make(const void *p);
extern void bgp_attr_ref(struct attr *attr);
extern void attr_show_all(struct vty *vty, bool summary);
extern unsigned long int attr_count(void);
extern unsigned long int attr_unknown_count(void);
Expand Down
6 changes: 3 additions & 3 deletions bgpd/bgp_evpn.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4936,7 +4936,7 @@ static int process_type2_route(struct peer *peer, afi_t afi, safi_t safi,
if (attr)
bgp_update(peer, (struct prefix *)&p, addpath_id, attr, afi,
safi, ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, &prd,
&label[0], num_labels, 0, NULL);
&label[0], num_labels, 0, NULL, NULL);
else
bgp_withdraw(peer, (struct prefix *)&p, addpath_id, afi, safi,
ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, &prd, &label[0],
Expand Down Expand Up @@ -5026,7 +5026,7 @@ static int process_type3_route(struct peer *peer, afi_t afi, safi_t safi,
if (attr)
bgp_update(peer, (struct prefix *)&p, addpath_id, attr, afi,
safi, ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, &prd, NULL,
0, 0, NULL);
0, 0, NULL, NULL);
else
bgp_withdraw(peer, (struct prefix *)&p, addpath_id, afi, safi,
ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, &prd, NULL, 0);
Expand Down Expand Up @@ -5159,7 +5159,7 @@ static int process_type5_route(struct peer *peer, afi_t afi, safi_t safi,
if (attr && is_valid_update)
bgp_update(peer, (struct prefix *)&p, addpath_id, attr, afi,
safi, ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, &prd,
&label, 1, 0, evpn);
&label, 1, 0, evpn, NULL);
else {
if (!is_valid_update) {
char attr_str[BUFSIZ] = {0};
Expand Down
4 changes: 2 additions & 2 deletions bgpd/bgp_evpn_mh.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -768,7 +768,7 @@ int bgp_evpn_type4_route_process(struct peer *peer, afi_t afi, safi_t safi,
if (attr) {
bgp_update(peer, (struct prefix *)&p, addpath_id, attr, afi,
safi, ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, &prd, NULL,
0, 0, NULL);
0, 0, NULL, NULL);
} else {
bgp_withdraw(peer, (struct prefix *)&p, addpath_id, afi, safi,
ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, &prd, NULL, 0);
Expand Down Expand Up @@ -1235,7 +1235,7 @@ int bgp_evpn_type1_route_process(struct peer *peer, afi_t afi, safi_t safi,
/* Process the route. */
if (attr) {
bgp_update(peer, (struct prefix *)&p, addpath_id, attr, afi, safi, ZEBRA_ROUTE_BGP,
BGP_ROUTE_NORMAL, &prd, &label[0], 1, 0, NULL);
BGP_ROUTE_NORMAL, &prd, &label[0], 1, 0, NULL, NULL);
} else {
bgp_withdraw(peer, (struct prefix *)&p, addpath_id, afi, safi, ZEBRA_ROUTE_BGP,
BGP_ROUTE_NORMAL, &prd, &label[0], 1);
Expand Down
2 changes: 1 addition & 1 deletion bgpd/bgp_flowspec.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ int bgp_nlri_parse_flowspec(struct peer *peer, struct attr *attr,
if (!withdraw) {
bgp_update(peer, &p, 0, attr, afi, safi,
ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, NULL,
NULL, 0, 0, NULL);
NULL, 0, 0, NULL, NULL);
} else {
bgp_withdraw(peer, &p, 0, afi, safi, ZEBRA_ROUTE_BGP,
BGP_ROUTE_NORMAL, NULL, NULL, 0);
Expand Down
2 changes: 1 addition & 1 deletion bgpd/bgp_label.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,7 +576,7 @@ int bgp_nlri_parse_label(struct peer *peer, struct attr *attr,
if (attr) {
bgp_update(peer, &p, addpath_id, attr, packet->afi,
safi, ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL,
NULL, &label, 1, 0, NULL);
NULL, &label, 1, 0, NULL, NULL);
} else {
bgp_withdraw(peer, &p, addpath_id, packet->afi,
SAFI_UNICAST, ZEBRA_ROUTE_BGP,
Expand Down
2 changes: 1 addition & 1 deletion bgpd/bgp_mac.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ static void bgp_process_mac_rescan_table(struct bgp *bgp, struct peer *peer,
bgp_update(peer, p, pi->addpath_rx_id, pi->attr,
AFI_L2VPN, SAFI_EVPN, ZEBRA_ROUTE_BGP,
BGP_ROUTE_NORMAL, &prd, label_pnt, num_labels,
1, bgp_attr_get_evpn_overlay(pi->attr));
1, bgp_attr_get_evpn_overlay(pi->attr), NULL);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion bgpd/bgp_mplsvpn.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ int bgp_nlri_parse_vpn(struct peer *peer, struct attr *attr,
if (attr) {
bgp_update(peer, &p, addpath_id, attr, packet->afi,
SAFI_MPLS_VPN, ZEBRA_ROUTE_BGP,
BGP_ROUTE_NORMAL, &prd, &label, 1, 0, NULL);
BGP_ROUTE_NORMAL, &prd, &label, 1, 0, NULL, NULL);
} else {
bgp_withdraw(peer, &p, addpath_id, packet->afi,
SAFI_MPLS_VPN, ZEBRA_ROUTE_BGP,
Expand Down
43 changes: 38 additions & 5 deletions bgpd/bgp_route.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,17 @@ DEFINE_HOOK(bgp_route_update,
struct bgp_path_info *old_route, struct bgp_path_info *new_route),
(bgp, afi, safi, bn, old_route, new_route));

/* Invalidate cache if we're uninterning the cached pointer, then do the unintern. */
static inline void attr_unintern_safe_ctx(struct bgp_attr_reuse_ctx *reuse_ctx, struct attr **p)
{
if (reuse_ctx && reuse_ctx->valid && reuse_ctx->interned == *p) {
reuse_ctx->valid = false;
reuse_ctx->interned = NULL;
}
bgp_attr_unintern(p);
}


/* Extern from bgp_dump.c */
extern const char *bgp_origin_str[];
extern const char *bgp_origin_long_str[];
Expand Down Expand Up @@ -4977,7 +4988,7 @@ void bgp_update(struct peer *peer, const struct prefix *p, uint32_t addpath_id,
struct attr *attr, afi_t afi, safi_t safi, int type,
int sub_type, struct prefix_rd *prd, mpls_label_t *label,
uint8_t num_labels, int soft_reconfig,
struct bgp_route_evpn *evpn)
struct bgp_route_evpn *evpn, struct bgp_attr_reuse_ctx *reuse_ctx)
{
int ret;
struct bgp_dest *dest;
Expand Down Expand Up @@ -5322,7 +5333,28 @@ void bgp_update(struct peer *peer, const struct prefix *p, uint32_t addpath_id,
bgp_attr_set_ecommunity(&new_attr, new_ecomm);
}

attr_new = bgp_attr_intern(&new_attr);
/*
* Re-use intern'd attribute for same NLRI-section with multiple prefixes.
* Do intern: If the final 'new_attr' does not equal to the parsed attr.
* Re-use: If the final 'new_attr' eqauls to the parsed attr AND reuse_ctx referes
* to the same attr ptr, AFI/SAFI, then re-use the cached interned attribute.
*/
if (reuse_ctx && attrhash_cmp(&new_attr, attr) &&
reuse_ctx->valid && reuse_ctx->in_attr == attr &&
reuse_ctx->afi == afi && reuse_ctx->safi == safi) {
attr_new = reuse_ctx->interned;
bgp_attr_ref(attr_new);
} else {
attr_new = bgp_attr_intern(&new_attr);
/* Populate cache only for the unchanged-from-parsed case. */
if (reuse_ctx && attrhash_cmp(&new_attr, attr)) {
reuse_ctx->valid = true;
reuse_ctx->in_attr = attr;
reuse_ctx->afi = afi;
reuse_ctx->safi = safi;
reuse_ctx->interned = attr_new;
}
}

/* If the update is implicit withdraw. */
if (pi) {
Expand Down Expand Up @@ -5386,7 +5418,7 @@ void bgp_update(struct peer *peer, const struct prefix *p, uint32_t addpath_id,
}

bgp_dest_unlock_node(dest);
bgp_attr_unintern(&attr_new);
attr_unintern_safe_ctx(reuse_ctx, &attr_new);
if (p_evpn)
evpn_overlay_free(p_evpn);
return;
Expand Down Expand Up @@ -6068,7 +6100,7 @@ static void bgp_soft_reconfig_table_update(struct peer *peer,

bgp_update(peer, bgp_dest_get_prefix(dest), ain->addpath_rx_id,
ain->attr, afi, safi, ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, prd,
label_pnt, num_labels, 1, bre);
label_pnt, num_labels, 1, bre, NULL);
}

static void bgp_soft_reconfig_table(struct peer *peer, afi_t afi, safi_t safi,
Expand Down Expand Up @@ -7216,6 +7248,7 @@ bool bgp_addpath_encode_rx(struct peer *peer, afi_t afi, safi_t safi)
int bgp_nlri_parse_ip(struct peer *peer, struct attr *attr,
struct bgp_nlri *packet)
{
struct bgp_attr_reuse_ctx reuse_ctx = {0};
uint8_t *pnt;
uint8_t *lim;
struct prefix p;
Expand Down Expand Up @@ -7334,7 +7367,7 @@ int bgp_nlri_parse_ip(struct peer *peer, struct attr *attr,
if (attr)
bgp_update(peer, &p, addpath_id, attr, afi, safi,
ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, NULL,
NULL, 0, 0, NULL);
NULL, 0, 0, NULL, &reuse_ctx);
else
bgp_withdraw(peer, &p, addpath_id, afi, safi,
ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, NULL,
Expand Down
18 changes: 17 additions & 1 deletion bgpd/bgp_route.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,21 @@ struct bgp_aggregate {
struct route_map *suppress_map;
};

/* Context used to avoid repeated interning for multiple prefixes per NLRI-section */
struct bgp_attr_reuse_ctx {
bool valid;

/* parsed attr pointer this cache entry corresponds to (for equality check) */
const struct attr *in_attr;

/* AFI/SAFI of the NLRI section for which reuse applies */
afi_t afi;
safi_t safi;

/* interned attr(reference) to reuse during prefix parsing */
struct attr *interned;
};

#define BGP_NEXTHOP_AFI_FROM_NHLEN(nhlen) \
((nhlen) < IPV4_MAX_BYTELEN \
? 0 \
Expand Down Expand Up @@ -851,7 +866,8 @@ extern void bgp_update(struct peer *peer, const struct prefix *p,
safi_t safi, int type, int sub_type,
struct prefix_rd *prd, mpls_label_t *label,
uint8_t num_labels, int soft_reconfig,
struct bgp_route_evpn *evpn);
struct bgp_route_evpn *evpn,
struct bgp_attr_reuse_ctx *reuse_ctx);
extern void bgp_withdraw(struct peer *peer, const struct prefix *p,
uint32_t addpath_id, afi_t afi, safi_t safi, int type,
int sub_type, struct prefix_rd *prd,
Expand Down
2 changes: 1 addition & 1 deletion bgpd/bgp_rpki.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -718,7 +718,7 @@ static void revalidate_bgp_node(struct bgp_dest *bgp_dest, afi_t afi, safi_t saf

(void)bgp_update(ain->peer, bgp_dest_get_prefix(bgp_dest), ain->addpath_rx_id,
ain->attr, afi, safi, ZEBRA_ROUTE_BGP, BGP_ROUTE_NORMAL, NULL,
label, num_labels, 1, NULL);
label, num_labels, 1, NULL, NULL);
}
}

Expand Down
10 changes: 5 additions & 5 deletions bgpd/rfapi/vnc_export_bgp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ void vnc_direct_bgp_add_route_ce(struct bgp *bgp, struct agg_node *rn,
afi, SAFI_UNICAST, ZEBRA_ROUTE_VNC_DIRECT,
BGP_ROUTE_REDISTRIBUTE, NULL, /* RD not used for unicast */
NULL, 0, /* tag not used for unicast */
0, NULL); /* EVPN not used */
0, NULL, NULL); /* EVPN not used */
bgp_attr_unintern(&iattr);
}

Expand Down Expand Up @@ -1033,7 +1033,7 @@ void vnc_direct_bgp_add_nve(struct bgp *bgp, struct rfapi_descriptor *rfd)
/* RD not used for unicast */
NULL,
/* tag not used for unicast */
0, 0, NULL); /* EVPN not used */
0, 0, NULL, NULL); /* EVPN not used */

bgp_attr_unintern(&iattr);
}
Expand Down Expand Up @@ -1245,7 +1245,7 @@ static void vnc_direct_add_rn_group_rd(struct bgp *bgp,
afi, SAFI_UNICAST, ZEBRA_ROUTE_VNC_DIRECT,
BGP_ROUTE_REDISTRIBUTE, NULL, /* RD not used for unicast */
NULL, /* tag not used for unicast */
0, 0, NULL); /* EVPN not used */
0, 0, NULL, NULL); /* EVPN not used */

bgp_attr_unintern(&iattr);

Expand Down Expand Up @@ -1697,7 +1697,7 @@ void vnc_direct_bgp_rh_add_route(struct bgp *bgp, afi_t afi,
afi, SAFI_UNICAST, ZEBRA_ROUTE_VNC_DIRECT_RH,
BGP_ROUTE_REDISTRIBUTE, NULL, /* RD not used for unicast */
NULL, /* tag not used for unicast, EVPN neither */
0, 0, NULL); /* EVPN not used */
0, 0, NULL, NULL); /* EVPN not used */
bgp_attr_unintern(&iattr);
}

Expand Down Expand Up @@ -1936,7 +1936,7 @@ void vnc_direct_bgp_rh_vpn_enable(struct bgp *bgp, afi_t afi)
NULL,
/* tag not used for unicast,
or EVPN */
0, 0, NULL); /* EVPN not used */
0, 0, NULL, NULL); /* EVPN not used */

bgp_attr_unintern(&iattr);
}
Expand Down
Loading