[ovs-dev] [PATCH v2 13/26] ovn-northd-ddlog: Intern the `Router` table.

Ben Pfaff blp at ovn.org
Thu Apr 1 23:20:55 UTC 2021


From: Leonid Ryzhyk <lryzhyk at vmware.com>

This is the first in a series of commits that will replace the use of
the DDlog's `Ref<>` type with `Intern<>` throughout the OVN code base.
`Ref` and `Intern` are the two forms of smart pointers supported by
DDlog at the moment.  `Ref` is a reference counted pointer.  Copying
a `Ref<>` simply increments its reference count.  `Intern<>` is an
interned object reference.  It guarantees that there exists exactly
one copy of each unique interned value.  Interned objects are slightly
more expensive to create, but they have several important advantages:
(1) they save memory by deduplicating identical values, (2) they allow
by-pointer comparisons, and (3) they avoid unnecessary recomputations
in some scenarios. See DDlog docs [1], [2] for more detail.

In this commit we change the type of records in the `Router` table from
`Ref<Router>` to `Intern<Router>`.  This reduces the amount of churn
and speeds up northd significantly in scenarios where the set of router
ports changes frequently, which triggers updates to
`nb::Logical_Router`, which in turn updates corresponding records
in the `Router` table.  Interning guarantees that these updates are
no-ops and do not trigger any other rules.

[1] https://github.com/vmware/differential-datalog/blob/master/doc/tutorial/tutorial.md#reference-type-ref
[2] https://github.com/vmware/differential-datalog/blob/master/doc/tutorial/tutorial.md#interned-values-intern-istring

Signed-off-by: Leonid Ryzhyk <lryzhyk at vmware.com>
Signed-off-by: Ben Pfaff <blp at ovn.org>
---
 northd/lrouter.dl    | 21 ++++++++++++---------
 northd/multicast.dl  |  6 +++---
 northd/ovn_northd.dl | 31 +++++++++++++++----------------
 3 files changed, 30 insertions(+), 28 deletions(-)

diff --git a/northd/lrouter.dl b/northd/lrouter.dl
index 574926b73b67..b2b429af3c96 100644
--- a/northd/lrouter.dl
+++ b/northd/lrouter.dl
@@ -430,7 +430,7 @@ LogicalRouterLBs(lr, vec_empty()) :-
 
 function chassis_redirect_name(port_name: string): string = "cr-${port_name}"
 
-relation &Router(
+typedef Router = Router {
     /* Fields copied from nb::Logical_Router. */
     _uuid:              uuid,
     name:               string,
@@ -452,9 +452,12 @@ relation &Router(
     mcast_cfg:          Ref<McastRouterCfg>,
     learn_from_arp_request: bool,
     force_lb_snat: bool,
-)
+}
+
+relation Router[Intern<Router>]
 
-&Router(._uuid         =    lr._uuid,
+Router[Router{
+        ._uuid         =    lr._uuid,
         .name          =    lr.name,
         .static_routes =    lr.static_routes,
         .policies      =    lr.policies,
@@ -476,7 +479,7 @@ relation &Router(
         .lbs        = lbs,
         .mcast_cfg  = mcast_cfg,
         .learn_from_arp_request = learn_from_arp_request,
-        .force_lb_snat = force_lb_snat) :-
+        .force_lb_snat = force_lb_snat}.intern()] :-
     lr in nb::Logical_Router(),
     lr.is_enabled(),
     LogicalRouterRedirectPort(lr._uuid, l3dgw_port),
@@ -488,7 +491,7 @@ relation &Router(
     var force_lb_snat = lb_force_snat_router_ip(lr.options).
 
 /* RouterLB: many-to-many relation between logical routers and nb::LB */
-relation RouterLB(router: Ref<Router>, lb: Ref<nb::Load_Balancer>)
+relation RouterLB(router: Intern<Router>, lb: Ref<nb::Load_Balancer>)
 
 RouterLB(router, lb) :-
     router in &Router(.lbs = lbs),
@@ -496,7 +499,7 @@ RouterLB(router, lb) :-
 
 /* Load balancer VIPs associated with routers */
 relation RouterLBVIP(
-    router: Ref<Router>,
+    router: Intern<Router>,
     lb: Ref<nb::Load_Balancer>,
     vip: string,
     backends: string)
@@ -576,7 +579,7 @@ relation &RouterPort(
     lrp:              nb::Logical_Router_Port,
     json_name:        string,
     networks:         lport_addresses,
-    router:           Ref<Router>,
+    router:           Intern<Router>,
     is_redirect:      bool,
     peer:             RouterPeer,
     mcast_cfg:        Ref<McastPortCfg>,
@@ -711,7 +714,7 @@ function find_lrp_member_ip(networks: lport_addresses, ip: v46_ip): Option<v46_i
 
 /* Step 1: compute router-route pairs */
 relation RouterStaticRoute_(
-    router      : Ref<Router>,
+    router      : Intern<Router>,
     key         : route_key,
     nexthop     : v46_ip,
     output_port : Option<string>,
@@ -735,7 +738,7 @@ typedef route_dst = RouteDst {
 }
 
 relation RouterStaticRoute(
-    router      : Ref<Router>,
+    router      : Intern<Router>,
     key         : route_key,
     dsts        : Set<route_dst>)
 
diff --git a/northd/multicast.dl b/northd/multicast.dl
index 990203bffe25..9b0fa80738d7 100644
--- a/northd/multicast.dl
+++ b/northd/multicast.dl
@@ -160,7 +160,7 @@ SwitchMcastFloodReportPorts(switch, set_empty()) :-
 /* Mapping between Router and the set of port uuids on which to
  * flood IP multicast reports statically.
  */
-relation RouterMcastFloodPorts(sw: Ref<Router>, ports: Set<uuid>)
+relation RouterMcastFloodPorts(sw: Intern<Router>, ports: Set<uuid>)
 
 RouterMcastFloodPorts(router, flood_ports) :-
     &RouterPort(
@@ -213,7 +213,7 @@ IgmpSwitchMulticastGroup(address, switch, ports) :-
  */
 relation IgmpRouterGroupPort(
     address: string,
-    router : Ref<Router>,
+    router : Intern<Router>,
     port   : uuid
 )
 
@@ -236,7 +236,7 @@ IgmpRouterGroupPort(address, rtr_port.router, rtr_port.lrp._uuid) :-
  */
 relation IgmpRouterMulticastGroup(
     address: string,
-    router : Ref<Router>,
+    router : Intern<Router>,
     ports  : Set<uuid>
 )
 
diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index bb8be08dc55e..80d8598bd7dc 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -289,7 +289,7 @@ OutProxy_Port_Binding(._uuid              = lrp._uuid,
     }.
 /*
 */
-function get_router_load_balancer_ips(router: Router) :
+function get_router_load_balancer_ips(router: Intern<Router>) :
     (Set<string>, Set<string>) =
 {
     var all_ips_v4 = set_empty();
@@ -319,8 +319,7 @@ function get_router_load_balancer_ips(router: Router) :
 function get_nat_addresses(rport: RouterPort): Set<string> =
 {
     var addresses = set_empty();
-    var router = deref(rport.router);
-    var has_redirect = router.l3dgw_port.is_some();
+    var has_redirect = rport.router.l3dgw_port.is_some();
     match (eth_addr_from_string(rport.lrp.mac)) {
         None -> addresses,
         Some{mac} -> {
@@ -328,7 +327,7 @@ function get_nat_addresses(rport: RouterPort): Set<string> =
             var central_ip_address = false;
 
             /* Get NAT IP addresses. */
-            for (nat in router.nats) {
+            for (nat in rport.router.nats) {
                 /* Determine whether this NAT rule satisfies the conditions for
                  * distributed NAT processing. */
                 if (has_redirect and nat.nat.__type == "dnat_and_snat" and
@@ -371,7 +370,7 @@ function get_nat_addresses(rport: RouterPort): Set<string> =
             };
 
             /* A set to hold all load-balancer vips. */
-            (var all_ips_v4, var all_ips_v6) = get_router_load_balancer_ips(router);
+            (var all_ips_v4, var all_ips_v6) = get_router_load_balancer_ips(rport.router);
 
             for (ip_address in set_union(all_ips_v4, all_ips_v6)) {
                 c_addresses = c_addresses ++ " ${ip_address}";
@@ -382,7 +381,7 @@ function get_nat_addresses(rport: RouterPort): Set<string> =
                 /* Gratuitous ARP for centralized NAT rules on distributed gateway
                  * ports should be restricted to the gateway chassis. */
                 if (has_redirect) {
-                    c_addresses = c_addresses ++ " is_chassis_resident(${router.redirect_port_name})"
+                    c_addresses = c_addresses ++ " is_chassis_resident(${rport.router.redirect_port_name})"
                 } else ();
 
                 addresses.insert(c_addresses)
@@ -4083,7 +4082,7 @@ function get_arp_forward_ips(rp: Ref<RouterPort>): (Set<string>, Set<string>) =
     var all_ips_v6 = set_empty();
 
     (var lb_ips_v4, var lb_ips_v6)
-        = get_router_load_balancer_ips(deref(rp.router));
+        = get_router_load_balancer_ips(rp.router);
     for (a in lb_ips_v4) {
         /* Check if the ovn port has a network configured on which we could
          * expect ARP requests for the LB VIP.
@@ -4852,7 +4851,7 @@ LogicalRouterNatArpNdFlow(router, nat) :-
     (var ip, var nats) = snat_ip,
     Some{var nat} = nats.nth(0).
 
-relation LogicalRouterNatArpNdFlow(router: Ref<Router>, nat: NAT)
+relation LogicalRouterNatArpNdFlow(router: Intern<Router>, nat: NAT)
 LogicalRouterArpNdFlow(router, nat, None, rEG_INPORT_ETH_ADDR(), None, false, 90) :-
     LogicalRouterNatArpNdFlow(router, nat).
 
@@ -4882,7 +4881,7 @@ LogicalRouterPortNatArpNdFlow(router, nat, l3dgw_port) :-
 /* Respond to ARP/NS requests on the chassis that binds the gw
  * port. Drop the ARP/NS requests on other chassis.
  */
-relation LogicalRouterPortNatArpNdFlow(router: Ref<Router>, nat: NAT, lrp: nb::Logical_Router_Port)
+relation LogicalRouterPortNatArpNdFlow(router: Intern<Router>, nat: NAT, lrp: nb::Logical_Router_Port)
 LogicalRouterArpNdFlow(router, nat, Some{lrp}, mac, Some{extra_match}, false, 92),
 LogicalRouterArpNdFlow(router, nat, Some{lrp}, mac, None, true, 91) :-
     LogicalRouterPortNatArpNdFlow(router, nat, lrp),
@@ -4913,7 +4912,7 @@ LogicalRouterArpNdFlow(router, nat, Some{lrp}, mac, None, true, 91) :-
 
 /* Now divide the ARP/ND flows into ARP and ND. */
 relation LogicalRouterArpNdFlow(
-    router: Ref<Router>,
+    router: Intern<Router>,
     nat: NAT,
     lrp: Option<nb::Logical_Router_Port>,
     mac: string,
@@ -4930,7 +4929,7 @@ LogicalRouterNdFlow(router, lrp, "nd_na", ipv6, true, mac, extra_match, drop, pr
                            mac, extra_match, drop, priority).
 
 relation LogicalRouterArpFlow(
-    lr: Ref<Router>,
+    lr: Intern<Router>,
     lrp: Option<nb::Logical_Router_Port>,
     ip: in_addr,
     mac: string,
@@ -4973,7 +4972,7 @@ Flow(.logical_datapath = lr._uuid,
     }.
 
 relation LogicalRouterNdFlow(
-    lr: Ref<Router>,
+    lr: Intern<Router>,
     lrp: Option<nb::Logical_Router_Port>,
     action: string,
     ip: in6_addr,
@@ -5393,7 +5392,7 @@ function lrouter_nat_is_stateless(nat: NAT): bool = {
  * and action says "next" instead of ct*.
  */
 function lrouter_nat_add_ext_ip_match(
-    router: Ref<Router>,
+    router: Intern<Router>,
     nat: NAT,
     __match: string,
     ipX: string,
@@ -6433,7 +6432,7 @@ function numbered_vec(set: Set<'A>) : Vec<(bit<16>, 'A)> = {
 
 relation EcmpGroup(
     group_id: bit<16>,
-    router: Ref<Router>,
+    router: Intern<Router>,
     key: route_key,
     dsts: Set<route_dst>,
     route_match: string,        // This is build_route_match(key).0
@@ -6490,7 +6489,7 @@ Flow(.logical_datapath = router._uuid,
  * an ECMP route need to go through conntrack.
  */
 relation EcmpSymmetricReply(
-    router: Ref<Router>,
+    router: Intern<Router>,
     dst: route_dst,
     route_match: string,
     tunkey: integer)
@@ -6712,7 +6711,7 @@ function all_same_addr_family(addrs: Set<string>): bool {
 }
 
 relation EcmpReroutePolicy(
-    r: Ref<Router>,
+    r: Intern<Router>,
     policy: nb::Logical_Router_Policy,
     ecmp_group_id: usize
 )
-- 
2.29.2



More information about the dev mailing list