ASPA: Fix downstream check for two-point apex

The ASPA algorithm is quite complex if one wants to execute it fast.
Most notably, the performance-critical part is looking up the ASPA
records, and we are trying to reduce that to minimum.

Yet, in that effort, we missed the fact that in the downstream
algorithm, the down-ramp and up-ramp may touch, i.e. their top ends
have a lateral peering.

The original idea was to find the point where the down-ramp is
impossible to be extended, and from there on, the algorithm is basically
just the upstream algorithm. But it isn't, most notably with the lateral
peering scenario it is much more complex than this.

This issue was discovered by several people, and got a fix submitted by
Evann DREUMONT. That fix was correct but replaced the algorithm too
deeply. We don't want to do such large changes (including semantics)
inside the stable versions, and we have some more plans with all of this
considering performance, as soon as more ASPA records emerge.

This patch therefore simply removes the force_upstream shortcut from
where the down ramp is terminated, fixes the downstream code so that
it works without that shortcut, and explicitly allows the two-apex
downstream scenario.

Original-By: Evann DREUMONT <evann@grifon.fr>

diff --git a/nest/rt-table.c b/nest/rt-table.c
index ed364d3510c40a951cce172d103e70f78385b8fe..1d50e5d05ed3c2cd2cd69f968c61bb9cda06dc8a 100644 (file)
--- a/nest/rt-table.c
+++ b/nest/rt-table.c
@@ -432,44 +432,40 @@ end_of_aspa:;
     else if (!found)
     {
       /* Extend max-downstream (min-downstream is stopped by unknown) */
-      max_down = ap+1;
+      if (max_down == ap)
+       max_down = ap+1;
 
       /* Move min-upstream (can't include unknown) */
       min_up = ap;
     }
 
-    /* ASPA exists and downstream may be extended */
-    else if (down)
-    {
-      /* Extending max-downstream always */
-      max_down = ap+1;
-
-      /* Extending min-downstream unless unknown seen */
-      if (min_down == ap)
-       min_down = ap+1;
-
-      /* Downstream only */
-      if (!up)
-       min_up = max_up = ap;
-    }
-
-    /* No extension for downstream, force upstream only from now */
+    /* ASPA exists */
     else
     {
-      force_upstream = 1;
+      /* Downstream may be extended by this AS */
+      if (down)
+      {
+       /* Extending max-downstream unless invalid seen */
+       if (max_down == ap)
+         max_down = ap+1;
+
+       /* Extending min-downstream unless unknown seen */
+       if (min_down == ap)
+         min_down = ap+1;
+      }
 
-      /* Not even upstream, move the ending here */
+      /* Move upstream end unless extensible */
       if (!up)
        min_up = max_up = ap;
     }
   }
 
   /* Is the path surely valid? */
-  if (min_up <= min_down)
+  if (min_up <= min_down + !force_upstream)
     return ASPA_VALID;
 
   /* Is the path maybe valid? */
-  if (max_up <= max_down)
+  if (max_up <= max_down + !force_upstream)
     return ASPA_UNKNOWN;
 
   /* Now there is surely a valley there. */