On Wed, Mar 02, 2022 at 01:25:42PM +0100, Theo Buehler wrote:
> On Wed, Mar 02, 2022 at 01:07:09PM +0100, Claudio Jeker wrote:
> > On Wed, Mar 02, 2022 at 01:03:04PM +0100, Claudio Jeker wrote:
> > > This diff changes prefix_adjout_withdraw() to take a prefix pointer
> > > as argument. So instead of doing the lookup in the withdraw function the
> > > caller may need to do it.
> > >
> > > With this one call to up_generate_updates() can be replaced with a direct
> > > call to prefix_adjout_withdraw(). rde_up_flush_upcall() tries to withdraw
> > > every prefix in the Adj-RIB-Out of a peer. The indirection via
> > > up_generate_updates() makes little sense here.
> >
> > Forgot a hunk from the much bigger diff I wrestle with.
> > There is no need to pass the peer to rde_up_flush_upcall. This also fixes
> > a minor bug with rde_softreconfig_in_done() which expects arg to be
> > NULL.
>
> I'm ok with this, though I have one question.
>
> > @@ -1252,21 +1252,19 @@ prefix_adjout_update(struct rde_peer *pe
> > * the prefix in the RIB linked to the peer withdraw list.
> > */
> > int
> > -prefix_adjout_withdraw(struct rde_peer *peer, struct bgpd_addr *prefix,
> > - int prefixlen)
> > +prefix_adjout_withdraw(struct prefix *p)
> > {
> > - struct prefix *p;
> > -
> > - p = prefix_adjout_get(peer, 0, prefix, prefixlen);
> > - if (p == NULL) /* Got a dummy withdrawn request. */
> > - return (0);
> > + struct rde_peer *peer = prefix_peer(p);
> >
> > if ((p->flags & PREFIX_FLAG_ADJOUT) == 0)
> > fatalx("%s: prefix without PREFIX_FLAG_ADJOUT hit", __func__);
> >
> > - /* already a withdraw, error */
> > - if (p->flags & PREFIX_FLAG_WITHDRAW)
> > - log_warnx("%s: prefix already withdrawed", __func__);
> > + /* already a withdraw, shortcut */
> > + if (p->flags & PREFIX_FLAG_WITHDRAW) {
> > + p->lastchange = getmonotime();
> > + p->flags &= ~PREFIX_FLAG_STALE;
> > + return (0);
> > + }
>
> I'm a bit confused by this part. You changed this to an error a few days
> ago, now you change it back to a shortcut. Why?
>
I forgot to mention that. So during reconfigure rde_up_flush_upcall()
walks over the full adj_rib_out tree. This includes pending withdraws and
we need to properly handle them. I decided it is best to revert this back
to the way it was.
The current code is actually incorrect. The RB_INSERT() at the end of
prefix_adjout_withdraw() would try re-insert the prefix that is already in
the tree.
Looking at this again, I realized that the accounting is not quite right.
When prefix_adjout_withdraw is called on a PREFIX_FLAG_DEAD prefix the
prefix_out_cnt is lowered but it should not. Again this is a very uncommon
case but it is wrong none the less.
up_wcnt and up_nlricnt need to be lowered where the corresponding RB_REMOVE()
calls are. The prefix_out_cnt() needs to be lowered when prefix_unlink()
is called. A similar change should be done for prefix_adjout_update() but
I will do that as a next step.
--
:wq Claudio
Index: rde.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
retrieving revision 1.538
diff -u -p -r1.538 rde.c
--- rde.c 28 Feb 2022 12:52:38 -0000 1.538
+++ rde.c 2 Mar 2022 13:32:38 -0000
@@ -3050,9 +3050,7 @@ rde_generate_updates(struct rib *rib, st
static void
rde_up_flush_upcall(struct prefix *p, void *ptr)
{
- struct rde_peer *peer = ptr;
-
- up_generate_updates(out_rules, peer, NULL, p);
+ prefix_adjout_withdraw(p);
}
u_char queue_buf[4096];
@@ -3444,7 +3442,7 @@ rde_reload_done(void)
if (peer->reconf_rib) {
if (prefix_dump_new(peer, AID_UNSPEC,
- RDE_RUNNER_ROUNDS, peer, rde_up_flush_upcall,
+ RDE_RUNNER_ROUNDS, NULL, rde_up_flush_upcall,
rde_softreconfig_in_done, NULL) == -1)
fatal("%s: prefix_dump_new", __func__);
log_peer_info(&peer->conf, "flushing Adj-RIB-Out");
Index: rde.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
retrieving revision 1.244
diff -u -p -r1.244 rde.h
--- rde.h 25 Feb 2022 11:36:54 -0000 1.244
+++ rde.h 2 Mar 2022 13:32:12 -0000
@@ -596,8 +596,7 @@ int prefix_withdraw(struct rib *, stru
void prefix_add_eor(struct rde_peer *, uint8_t);
int prefix_adjout_update(struct rde_peer *, struct filterstate *,
struct bgpd_addr *, int, uint8_t);
-int prefix_adjout_withdraw(struct rde_peer *, struct bgpd_addr *,
- int);
+void prefix_adjout_withdraw(struct prefix *);
void prefix_adjout_destroy(struct prefix *p);
void prefix_adjout_dump(struct rde_peer *, void *,
void (*)(struct prefix *, void *));
Index: rde_rib.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v
retrieving revision 1.230
diff -u -p -r1.230 rde_rib.c
--- rde_rib.c 1 Mar 2022 09:39:36 -0000 1.230
+++ rde_rib.c 2 Mar 2022 13:39:35 -0000
@@ -1251,37 +1251,40 @@ prefix_adjout_update(struct rde_peer *pe
* Withdraw a prefix from the Adj-RIB-Out, this unlinks the aspath but leaves
* the prefix in the RIB linked to the peer withdraw list.
*/
-int
-prefix_adjout_withdraw(struct rde_peer *peer, struct bgpd_addr *prefix,
- int prefixlen)
+void
+prefix_adjout_withdraw(struct prefix *p)
{
- struct prefix *p;
-
- p = prefix_adjout_get(peer, 0, prefix, prefixlen);
- if (p == NULL) /* Got a dummy withdrawn request. */
- return (0);
+ struct rde_peer *peer = prefix_peer(p);
if ((p->flags & PREFIX_FLAG_ADJOUT) == 0)
fatalx("%s: prefix without PREFIX_FLAG_ADJOUT hit", __func__);
- /* already a withdraw, error */
- if (p->flags & PREFIX_FLAG_WITHDRAW)
- log_warnx("%s: prefix already withdrawed", __func__);
+ /* already a withdraw, shortcut */
+ if (p->flags & PREFIX_FLAG_WITHDRAW) {
+ p->lastchange = getmonotime();
+ p->flags &= ~PREFIX_FLAG_STALE;
+ return;
+ }
/* pending update just got withdrawn */
- if (p->flags & PREFIX_FLAG_UPDATE)
+ if (p->flags & PREFIX_FLAG_UPDATE) {
RB_REMOVE(prefix_tree, &peer->updates[p->pt->aid], p);
+ peer->up_nlricnt--;
+ }
/* unlink prefix if it was linked (not a withdraw or dead) */
- if ((p->flags & (PREFIX_FLAG_WITHDRAW | PREFIX_FLAG_DEAD)) == 0)
+ if ((p->flags & (PREFIX_FLAG_WITHDRAW | PREFIX_FLAG_DEAD)) == 0) {
prefix_unlink(p);
+ peer->prefix_out_cnt--;
+ }
/* nothing needs to be done for PREFIX_FLAG_DEAD and STALE */
p->flags &= ~PREFIX_FLAG_MASK;
p->lastchange = getmonotime();
p->flags |= PREFIX_FLAG_WITHDRAW;
- if (RB_INSERT(prefix_tree, &peer->withdraws[prefix->aid], p) != NULL)
+ if (RB_INSERT(prefix_tree, &peer->withdraws[p->pt->aid], p) != NULL)
fatalx("%s: RB tree invariant violated", __func__);
- return (1);
+
+ peer->up_wcnt++;
}
void
@@ -1298,13 +1301,19 @@ prefix_adjout_destroy(struct prefix *p)
return;
}
- if (p->flags & PREFIX_FLAG_WITHDRAW)
+ if (p->flags & PREFIX_FLAG_WITHDRAW) {
RB_REMOVE(prefix_tree, &peer->withdraws[p->pt->aid], p);
- if (p->flags & PREFIX_FLAG_UPDATE)
+ peer->up_wcnt--;
+ }
+ if (p->flags & PREFIX_FLAG_UPDATE) {
RB_REMOVE(prefix_tree, &peer->updates[p->pt->aid], p);
+ peer->up_nlricnt--;
+ }
/* unlink prefix if it was linked (not a withdraw or dead) */
- if ((p->flags & (PREFIX_FLAG_WITHDRAW | PREFIX_FLAG_DEAD)) == 0)
+ if ((p->flags & (PREFIX_FLAG_WITHDRAW | PREFIX_FLAG_DEAD)) == 0) {
prefix_unlink(p);
+ peer->prefix_out_cnt--;
+ }
/* nothing needs to be done for PREFIX_FLAG_DEAD and STALE */
p->flags &= ~PREFIX_FLAG_MASK;
Index: rde_update.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_update.c,v
retrieving revision 1.134
diff -u -p -r1.134 rde_update.c
--- rde_update.c 1 Mar 2022 09:53:42 -0000 1.134
+++ rde_update.c 2 Mar 2022 13:35:16 -0000
@@ -102,6 +102,7 @@ up_generate_updates(struct filter_head *
{
struct filterstate state;
struct bgpd_addr addr;
+ struct prefix *p;
int need_withdraw;
uint8_t prefixlen;
@@ -119,10 +120,8 @@ up_generate_updates(struct filter_head *
again:
if (new == NULL) {
/* withdraw prefix */
- if (prefix_adjout_withdraw(peer, &addr, prefixlen) == 1) {
- peer->prefix_out_cnt--;
- peer->up_wcnt++;
- }
+ if ((p = prefix_adjout_get(peer, 0, &addr, prefixlen)) != NULL)
+ prefix_adjout_withdraw(p);
} else {
need_withdraw = 0;
/*
@@ -655,7 +654,6 @@ up_dump_prefix(u_char *buf, int len, str
if (withdraw) {
/* prefix no longer needed, remove it */
prefix_adjout_destroy(p);
- peer->up_wcnt--;
peer->prefix_sent_withdraw++;
} else {
/* prefix still in Adj-RIB-Out, keep it */
