Re: bgpd communities rewrite
On Fri, Jun 07, 2019 at 11:59:10AM +0200, Claudio Jeker wrote:
> On Tue, May 28, 2019 at 03:53:11PM +0200, Claudio Jeker wrote:
> > On Tue, May 14, 2019 at 12:37:25AM +0200, Claudio Jeker wrote:
> > > This diff changes the way communites are stored and modified in bgpd.
> > > The current implementation was showing that community_*_delete() consumed
> > > a lot of CPU time because of the way rde_attr was used.
> > > Communities are extensivly used by filters (e.g. arouteserver) and
> > > therefor they are stored in an own special datastructure that allows fast
> > > changes especially during filtering. The communities are now a special
> > > part of the filterstate and because of this set and delete of a community
> > > is much simpler.
> > >
> > > This seems to make my test setup a fair bit snappier and quicker at
> > > processing UPDATEs.
> >
> > While the old diff got us to warp speed this diff goes to warp 5 or 6.
> > It optimizes the filter functions community_match, community_delete and
> > community_set to be as quick as possible since those functions are in the
> > hot path. As a side-effect the filter specific community struct got removed
> > and everything uses the unified one (which is where I wanted to end up).
> >
> > Please test (especially the ext. community cases since extended
> > communities are just painful)
>
> I would like to get this into the tree in the next days since there are a
> few other big changes in the works and juggling that outside of the tree
> is getting complicated.
Here an updated diff that actually applies cleanly after the last few bgpd
commits.
--
:wq Claudio
Index: usr.sbin/bgpctl/bgpctl.c
===
RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
retrieving revision 1.238
diff -u -p -r1.238 bgpctl.c
--- usr.sbin/bgpctl/bgpctl.c23 May 2019 14:12:06 - 1.238
+++ usr.sbin/bgpctl/bgpctl.c28 May 2019 13:24:30 -
@@ -81,6 +81,7 @@ intshow_rib_detail_msg(struct imsg *,
voidshow_rib_brief(struct ctl_show_rib *, u_char *);
voidshow_rib_detail(struct ctl_show_rib *, u_char *, int, int);
voidshow_attr(void *, u_int16_t, int);
+voidshow_communities(u_char *, size_t, int);
voidshow_community(u_char *, u_int16_t);
voidshow_large_community(u_char *, u_int16_t);
voidshow_ext_community(u_char *, u_int16_t);
@@ -282,7 +283,7 @@ main(int argc, char *argv[])
}
if (res->as.type != AS_UNDEF)
ribreq.as = res->as;
- if (res->community.type != COMMUNITY_TYPE_NONE)
+ if (res->community.flags != 0)
ribreq.community = res->community;
ribreq.neighbor = neighbor;
strlcpy(ribreq.rib, res->rib, sizeof(ribreq.rib));
@@ -1252,6 +1253,12 @@ show_rib_detail_msg(struct imsg *imsg, i
asdata += sizeof(struct ctl_show_rib);
show_rib_detail(, asdata, nodescr, flag0);
break;
+ case IMSG_CTL_SHOW_RIB_COMMUNITIES:
+ ilen = imsg->hdr.len - IMSG_HEADER_SIZE;
+ if (ilen % sizeof(struct community))
+ errx(1, "bad IMSG_CTL_SHOW_RIB_COMMUNITIES received");
+ show_communities(imsg->data, ilen, flag0);
+ break;
case IMSG_CTL_SHOW_RIB_ATTR:
ilen = imsg->hdr.len - IMSG_HEADER_SIZE;
if (ilen < 3)
@@ -1632,6 +1639,154 @@ show_attr(void *b, u_int16_t len, int fl
printf("%c", EOL0(flag0));
}
+static void
+print_community(u_int16_t a, u_int16_t v)
+{
+ if (a == COMMUNITY_WELLKNOWN)
+ switch (v) {
+ case COMMUNITY_GRACEFUL_SHUTDOWN:
+ printf("GRACEFUL_SHUTDOWN");
+ break;
+ case COMMUNITY_NO_EXPORT:
+ printf("NO_EXPORT");
+ break;
+ case COMMUNITY_NO_ADVERTISE:
+ printf("NO_ADVERTISE");
+ break;
+ case COMMUNITY_NO_EXPSUBCONFED:
+ printf("NO_EXPORT_SUBCONFED");
+ break;
+ case COMMUNITY_NO_PEER:
+ printf("NO_PEER");
+ break;
+ case COMMUNITY_BLACKHOLE:
+ printf("BLACKHOLE");
+ break;
+ default:
+ printf("%hu:%hu", a, v);
+ break;
+ }
+ else
+ printf("%hu:%hu", a, v);
+}
+
+static void
+print_ext_community(u_int8_t *data)
+{
+ u_int64_t ext;
+ struct in_addr ip;
+ u_int32_t as4, u32;
+ u_int16_t as2, u16;
+ u_int8_ttype, subtype;
+
+ type = data[0];
+ subtype = data[1];
+
+ printf("%s ",
Re: bgpd communities rewrite
On Tue, May 28, 2019 at 03:53:11PM +0200, Claudio Jeker wrote:
> On Tue, May 14, 2019 at 12:37:25AM +0200, Claudio Jeker wrote:
> > This diff changes the way communites are stored and modified in bgpd.
> > The current implementation was showing that community_*_delete() consumed
> > a lot of CPU time because of the way rde_attr was used.
> > Communities are extensivly used by filters (e.g. arouteserver) and
> > therefor they are stored in an own special datastructure that allows fast
> > changes especially during filtering. The communities are now a special
> > part of the filterstate and because of this set and delete of a community
> > is much simpler.
> >
> > This seems to make my test setup a fair bit snappier and quicker at
> > processing UPDATEs.
>
> While the old diff got us to warp speed this diff goes to warp 5 or 6.
> It optimizes the filter functions community_match, community_delete and
> community_set to be as quick as possible since those functions are in the
> hot path. As a side-effect the filter specific community struct got removed
> and everything uses the unified one (which is where I wanted to end up).
>
> Please test (especially the ext. community cases since extended
> communities are just painful)
I would like to get this into the tree in the next days since there are a
few other big changes in the works and juggling that outside of the tree
is getting complicated.
Thanks for any help
--
:wq Claudio
Index: usr.sbin/bgpctl/bgpctl.c
===
RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
retrieving revision 1.238
diff -u -p -r1.238 bgpctl.c
--- usr.sbin/bgpctl/bgpctl.c23 May 2019 14:12:06 - 1.238
+++ usr.sbin/bgpctl/bgpctl.c28 May 2019 13:24:30 -
@@ -81,6 +81,7 @@ intshow_rib_detail_msg(struct imsg *,
voidshow_rib_brief(struct ctl_show_rib *, u_char *);
voidshow_rib_detail(struct ctl_show_rib *, u_char *, int, int);
voidshow_attr(void *, u_int16_t, int);
+voidshow_communities(u_char *, size_t, int);
voidshow_community(u_char *, u_int16_t);
voidshow_large_community(u_char *, u_int16_t);
voidshow_ext_community(u_char *, u_int16_t);
@@ -282,7 +283,7 @@ main(int argc, char *argv[])
}
if (res->as.type != AS_UNDEF)
ribreq.as = res->as;
- if (res->community.type != COMMUNITY_TYPE_NONE)
+ if (res->community.flags != 0)
ribreq.community = res->community;
ribreq.neighbor = neighbor;
strlcpy(ribreq.rib, res->rib, sizeof(ribreq.rib));
@@ -1252,6 +1253,12 @@ show_rib_detail_msg(struct imsg *imsg, i
asdata += sizeof(struct ctl_show_rib);
show_rib_detail(, asdata, nodescr, flag0);
break;
+ case IMSG_CTL_SHOW_RIB_COMMUNITIES:
+ ilen = imsg->hdr.len - IMSG_HEADER_SIZE;
+ if (ilen % sizeof(struct community))
+ errx(1, "bad IMSG_CTL_SHOW_RIB_COMMUNITIES received");
+ show_communities(imsg->data, ilen, flag0);
+ break;
case IMSG_CTL_SHOW_RIB_ATTR:
ilen = imsg->hdr.len - IMSG_HEADER_SIZE;
if (ilen < 3)
@@ -1632,6 +1639,154 @@ show_attr(void *b, u_int16_t len, int fl
printf("%c", EOL0(flag0));
}
+static void
+print_community(u_int16_t a, u_int16_t v)
+{
+ if (a == COMMUNITY_WELLKNOWN)
+ switch (v) {
+ case COMMUNITY_GRACEFUL_SHUTDOWN:
+ printf("GRACEFUL_SHUTDOWN");
+ break;
+ case COMMUNITY_NO_EXPORT:
+ printf("NO_EXPORT");
+ break;
+ case COMMUNITY_NO_ADVERTISE:
+ printf("NO_ADVERTISE");
+ break;
+ case COMMUNITY_NO_EXPSUBCONFED:
+ printf("NO_EXPORT_SUBCONFED");
+ break;
+ case COMMUNITY_NO_PEER:
+ printf("NO_PEER");
+ break;
+ case COMMUNITY_BLACKHOLE:
+ printf("BLACKHOLE");
+ break;
+ default:
+ printf("%hu:%hu", a, v);
+ break;
+ }
+ else
+ printf("%hu:%hu", a, v);
+}
+
+static void
+print_ext_community(u_int8_t *data)
+{
+ u_int64_t ext;
+ struct in_addr ip;
+ u_int32_t as4, u32;
+ u_int16_t as2, u16;
+ u_int8_ttype, subtype;
+
+ type = data[0];
+ subtype = data[1];
+
+ printf("%s ", log_ext_subtype(type, subtype));
+
+ switch (type) {
+ case EXT_COMMUNITY_TRANS_TWO_AS:
+ memcpy(, data + 2, sizeof(as2));
+ memcpy(, data + 4,
Re: bgpd communities rewrite
On Tue, May 14, 2019 at 12:37:25AM +0200, Claudio Jeker wrote:
> This diff changes the way communites are stored and modified in bgpd.
> The current implementation was showing that community_*_delete() consumed
> a lot of CPU time because of the way rde_attr was used.
> Communities are extensivly used by filters (e.g. arouteserver) and
> therefor they are stored in an own special datastructure that allows fast
> changes especially during filtering. The communities are now a special
> part of the filterstate and because of this set and delete of a community
> is much simpler.
>
> This seems to make my test setup a fair bit snappier and quicker at
> processing UPDATEs.
While the old diff got us to warp speed this diff goes to warp 5 or 6.
It optimizes the filter functions community_match, community_delete and
community_set to be as quick as possible since those functions are in the
hot path. As a side-effect the filter specific community struct got removed
and everything uses the unified one (which is where I wanted to end up).
Please test (especially the ext. community cases since extended
communities are just painful)
--
:wq Claudio
Index: usr.sbin/bgpctl/bgpctl.c
===
RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
retrieving revision 1.238
diff -u -p -r1.238 bgpctl.c
--- usr.sbin/bgpctl/bgpctl.c23 May 2019 14:12:06 - 1.238
+++ usr.sbin/bgpctl/bgpctl.c28 May 2019 13:24:30 -
@@ -81,6 +81,7 @@ intshow_rib_detail_msg(struct imsg *,
voidshow_rib_brief(struct ctl_show_rib *, u_char *);
voidshow_rib_detail(struct ctl_show_rib *, u_char *, int, int);
voidshow_attr(void *, u_int16_t, int);
+voidshow_communities(u_char *, size_t, int);
voidshow_community(u_char *, u_int16_t);
voidshow_large_community(u_char *, u_int16_t);
voidshow_ext_community(u_char *, u_int16_t);
@@ -282,7 +283,7 @@ main(int argc, char *argv[])
}
if (res->as.type != AS_UNDEF)
ribreq.as = res->as;
- if (res->community.type != COMMUNITY_TYPE_NONE)
+ if (res->community.flags != 0)
ribreq.community = res->community;
ribreq.neighbor = neighbor;
strlcpy(ribreq.rib, res->rib, sizeof(ribreq.rib));
@@ -1252,6 +1253,12 @@ show_rib_detail_msg(struct imsg *imsg, i
asdata += sizeof(struct ctl_show_rib);
show_rib_detail(, asdata, nodescr, flag0);
break;
+ case IMSG_CTL_SHOW_RIB_COMMUNITIES:
+ ilen = imsg->hdr.len - IMSG_HEADER_SIZE;
+ if (ilen % sizeof(struct community))
+ errx(1, "bad IMSG_CTL_SHOW_RIB_COMMUNITIES received");
+ show_communities(imsg->data, ilen, flag0);
+ break;
case IMSG_CTL_SHOW_RIB_ATTR:
ilen = imsg->hdr.len - IMSG_HEADER_SIZE;
if (ilen < 3)
@@ -1632,6 +1639,154 @@ show_attr(void *b, u_int16_t len, int fl
printf("%c", EOL0(flag0));
}
+static void
+print_community(u_int16_t a, u_int16_t v)
+{
+ if (a == COMMUNITY_WELLKNOWN)
+ switch (v) {
+ case COMMUNITY_GRACEFUL_SHUTDOWN:
+ printf("GRACEFUL_SHUTDOWN");
+ break;
+ case COMMUNITY_NO_EXPORT:
+ printf("NO_EXPORT");
+ break;
+ case COMMUNITY_NO_ADVERTISE:
+ printf("NO_ADVERTISE");
+ break;
+ case COMMUNITY_NO_EXPSUBCONFED:
+ printf("NO_EXPORT_SUBCONFED");
+ break;
+ case COMMUNITY_NO_PEER:
+ printf("NO_PEER");
+ break;
+ case COMMUNITY_BLACKHOLE:
+ printf("BLACKHOLE");
+ break;
+ default:
+ printf("%hu:%hu", a, v);
+ break;
+ }
+ else
+ printf("%hu:%hu", a, v);
+}
+
+static void
+print_ext_community(u_int8_t *data)
+{
+ u_int64_t ext;
+ struct in_addr ip;
+ u_int32_t as4, u32;
+ u_int16_t as2, u16;
+ u_int8_ttype, subtype;
+
+ type = data[0];
+ subtype = data[1];
+
+ printf("%s ", log_ext_subtype(type, subtype));
+
+ switch (type) {
+ case EXT_COMMUNITY_TRANS_TWO_AS:
+ memcpy(, data + 2, sizeof(as2));
+ memcpy(, data + 4, sizeof(u32));
+ printf("%s:%u", log_as(ntohs(as2)), ntohl(u32));
+ break;
+ case EXT_COMMUNITY_TRANS_IPV4:
+ memcpy(, data + 2, sizeof(ip));
+ memcpy(, data + 6, sizeof(u16));
+ printf("%s:%hu", inet_ntoa(ip), ntohs(u16));
+
bgpd communities rewrite
This diff changes the way communites are stored and modified in bgpd.
The current implementation was showing that community_*_delete() consumed
a lot of CPU time because of the way rde_attr was used.
Communities are extensivly used by filters (e.g. arouteserver) and
therefor they are stored in an own special datastructure that allows fast
changes especially during filtering. The communities are now a special
part of the filterstate and because of this set and delete of a community
is much simpler.
This seems to make my test setup a fair bit snappier and quicker at
processing UPDATEs.
--
:wq Claudio
Index: usr.sbin/bgpctl/bgpctl.c
===
RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
retrieving revision 1.236
diff -u -p -r1.236 bgpctl.c
--- usr.sbin/bgpctl/bgpctl.c3 May 2019 01:48:42 - 1.236
+++ usr.sbin/bgpctl/bgpctl.c12 May 2019 15:25:13 -
@@ -81,6 +81,7 @@ intshow_rib_detail_msg(struct imsg *,
voidshow_rib_brief(struct ctl_show_rib *, u_char *);
voidshow_rib_detail(struct ctl_show_rib *, u_char *, int, int);
voidshow_attr(void *, u_int16_t, int);
+voidshow_communities(u_char *, size_t, int);
voidshow_community(u_char *, u_int16_t);
voidshow_large_community(u_char *, u_int16_t);
voidshow_ext_community(u_char *, u_int16_t);
@@ -1248,6 +1249,12 @@ show_rib_detail_msg(struct imsg *imsg, i
asdata += sizeof(struct ctl_show_rib);
show_rib_detail(, asdata, nodescr, flag0);
break;
+ case IMSG_CTL_SHOW_RIB_COMMUNITIES:
+ ilen = imsg->hdr.len - IMSG_HEADER_SIZE;
+ if (ilen % sizeof(struct community))
+ errx(1, "bad IMSG_CTL_SHOW_RIB_COMMUNITIES received");
+ show_communities(imsg->data, ilen, flag0);
+ break;
case IMSG_CTL_SHOW_RIB_ATTR:
ilen = imsg->hdr.len - IMSG_HEADER_SIZE;
if (ilen < 3)
@@ -1628,6 +1635,138 @@ show_attr(void *b, u_int16_t len, int fl
printf("%c", EOL0(flag0));
}
+static void
+print_community(u_int16_t a, u_int16_t v)
+{
+ if (a == COMMUNITY_WELLKNOWN)
+ switch (v) {
+ case COMMUNITY_GRACEFUL_SHUTDOWN:
+ printf("GRACEFUL_SHUTDOWN");
+ break;
+ case COMMUNITY_NO_EXPORT:
+ printf("NO_EXPORT");
+ break;
+ case COMMUNITY_NO_ADVERTISE:
+ printf("NO_ADVERTISE");
+ break;
+ case COMMUNITY_NO_EXPSUBCONFED:
+ printf("NO_EXPORT_SUBCONFED");
+ break;
+ case COMMUNITY_NO_PEER:
+ printf("NO_PEER");
+ break;
+ case COMMUNITY_BLACKHOLE:
+ printf("BLACKHOLE");
+ break;
+ default:
+ printf("%hu:%hu", a, v);
+ break;
+ }
+ else
+ printf("%hu:%hu", a, v);
+}
+
+static void
+print_ext_community(u_int8_t *data)
+{
+ u_int64_t ext;
+ struct in_addr ip;
+ u_int32_t as4, u32;
+ u_int16_t as2, u16;
+ u_int8_ttype, subtype;
+
+ type = data[0];
+ subtype = data[1];
+
+ printf("%s ", log_ext_subtype(type, subtype));
+
+ switch (type) {
+ case EXT_COMMUNITY_TRANS_TWO_AS:
+ memcpy(, data + 2, sizeof(as2));
+ memcpy(, data + 4, sizeof(u32));
+ printf("%s:%u", log_as(ntohs(as2)), ntohl(u32));
+ break;
+ case EXT_COMMUNITY_TRANS_IPV4:
+ memcpy(, data + 2, sizeof(ip));
+ memcpy(, data + 6, sizeof(u16));
+ printf("%s:%hu", inet_ntoa(ip), ntohs(u16));
+ break;
+ case EXT_COMMUNITY_TRANS_FOUR_AS:
+ memcpy(, data + 2, sizeof(as4));
+ memcpy(, data + 6, sizeof(u16));
+ printf("%s:%hu", log_as(ntohl(as4)), ntohs(u16));
+ break;
+ case EXT_COMMUNITY_TRANS_OPAQUE:
+ case EXT_COMMUNITY_TRANS_EVPN:
+ memcpy(, data, sizeof(ext));
+ ext = be64toh(ext) & 0xLL;
+ printf("0x%llx", (unsigned long long)ext);
+ break;
+ case EXT_COMMUNITY_NON_TRANS_OPAQUE:
+ memcpy(, data, sizeof(ext));
+ ext = be64toh(ext) & 0xLL;
+ switch (ext) {
+ case EXT_COMMUNITY_OVS_VALID:
+ printf("valid ");
+ break;
+ case EXT_COMMUNITY_OVS_NOTFOUND:
+ printf("not-found ");
+ break;
+ case EXT_COMMUNITY_OVS_INVALID:
+
