diff mbox series

[ovs-dev,v6,4/6] vswitchd, ofproto-dpif: Propagate the CT limit from database.

Message ID 20231102120021.89725-5-amusil@redhat.com
State Changes Requested
Delegated to: Ilya Maximets
Headers show
Series Expose CT limit via DB | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Ales Musil Nov. 2, 2023, noon UTC
Propagate the CT limit that is present in the DB into
datapath. The limit is currently only propagated on change
and can be overwritten by the dpctl commands.

Signed-off-by: Ales Musil <amusil@redhat.com>
---
v6: Rebase on top of current master.
    Address comments from Ilya:
    - Update the comments and names.
    - Use loop in the system-test.
v5: Rebase on top of current master.
    Address comments from Ilya:
    - Make sure the zones are always removed.
    - Fix style related problems.
    - Make sure the limit is initialized to -1.
v4: Rebase on top of current master.
    Make sure that the values from DB are propagated only if set. That applies to both limit and policies.
---
 ofproto/ofproto-dpif.c     | 39 ++++++++++++++++++++
 ofproto/ofproto-dpif.h     |  5 +++
 ofproto/ofproto-provider.h |  8 ++++
 ofproto/ofproto.c          | 12 ++++++
 ofproto/ofproto.h          |  2 +
 tests/system-traffic.at    | 54 +++++++++++++++++++++++++++
 vswitchd/bridge.c          | 75 +++++++++++++++++++++++++++++---------
 7 files changed, 177 insertions(+), 18 deletions(-)
diff mbox series

Patch

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index ba5706f6a..6a931a806 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -220,6 +220,7 @@  static void ofproto_unixctl_init(void);
 static void ct_zone_config_init(struct dpif_backer *backer);
 static void ct_zone_config_uninit(struct dpif_backer *backer);
 static void ct_zone_timeout_policy_sweep(struct dpif_backer *backer);
+static void ct_zone_limits_commit(struct dpif_backer *backer);
 
 static inline struct ofproto_dpif *
 ofproto_dpif_cast(const struct ofproto *ofproto)
@@ -513,6 +514,7 @@  type_run(const char *type)
 
     process_dpif_port_changes(backer);
     ct_zone_timeout_policy_sweep(backer);
+    ct_zone_limits_commit(backer);
 
     return 0;
 }
@@ -5522,6 +5524,8 @@  ct_zone_config_init(struct dpif_backer *backer)
     cmap_init(&backer->ct_zones);
     hmap_init(&backer->ct_tps);
     ovs_list_init(&backer->ct_tp_kill_list);
+    ovs_list_init(&backer->ct_zone_limits_to_add);
+    ovs_list_init(&backer->ct_zone_limits_to_del);
     clear_existing_ct_timeout_policies(backer);
 }
 
@@ -5545,6 +5549,8 @@  ct_zone_config_uninit(struct dpif_backer *backer)
     id_pool_destroy(backer->tp_ids);
     cmap_destroy(&backer->ct_zones);
     hmap_destroy(&backer->ct_tps);
+    ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_add);
+    ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_del);
 }
 
 static void
@@ -5625,6 +5631,38 @@  ct_del_zone_timeout_policy(const char *datapath_type, uint16_t zone_id)
     }
 }
 
+static void
+ct_zone_limit_update(const char *datapath_type, int32_t zone_id,
+                     int64_t *limit)
+{
+    struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
+                                                 datapath_type);
+    if (!backer) {
+        return;
+    }
+
+    if (limit) {
+        ct_dpif_push_zone_limit(&backer->ct_zone_limits_to_add, zone_id,
+                                *limit, 0);
+    } else {
+        ct_dpif_push_zone_limit(&backer->ct_zone_limits_to_del, zone_id, 0, 0);
+    }
+}
+
+static void
+ct_zone_limits_commit(struct dpif_backer *backer)
+{
+    if (!ovs_list_is_empty(&backer->ct_zone_limits_to_add)) {
+        ct_dpif_set_limits(backer->dpif, &backer->ct_zone_limits_to_add);
+        ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_add);
+    }
+
+    if (!ovs_list_is_empty(&backer->ct_zone_limits_to_del)) {
+        ct_dpif_del_limits(backer->dpif, &backer->ct_zone_limits_to_del);
+        ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_del);
+    }
+}
+
 static void
 get_datapath_cap(const char *datapath_type, struct smap *cap)
 {
@@ -6914,4 +6952,5 @@  const struct ofproto_class ofproto_dpif_class = {
     ct_flush,                   /* ct_flush */
     ct_set_zone_timeout_policy,
     ct_del_zone_timeout_policy,
+    ct_zone_limit_update,
 };
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index d8e0cd37a..b863dd6fc 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -284,6 +284,11 @@  struct dpif_backer {
                                                 feature than 'bt_support'. */
 
     struct atomic_count tnl_count;
+
+    struct ovs_list ct_zone_limits_to_add;  /* CT zone limits queued for
+                                             * addition into datapath. */
+    struct ovs_list ct_zone_limits_to_del;  /* CT zone limt queued for
+                                             * deletion from datapath. */
 };
 
 /* All existing ofproto_backer instances, indexed by ofproto->up.type. */
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 9f7b8b6e8..face0b574 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1921,6 +1921,14 @@  struct ofproto_class {
     /* Deletes the timeout policy associated with 'zone' in datapath type
      * 'dp_type'. */
     void (*ct_del_zone_timeout_policy)(const char *dp_type, uint16_t zone);
+
+    /* Updates the CT zone limit for specified zone. Setting 'zone' to
+     * 'OVS_ZONE_LIMIT_DEFAULT_ZONE' represents the default zone.
+     * 'NULL' passed as 'limit' indicates that the limit should be removed for
+     * the specified zone. The caller must ensure that the 'limit' value is
+     * within proper range (0 - UINT32_MAX). */
+    void (*ct_zone_limit_update)(const char *dp_type, int32_t zone,
+                                 int64_t *limit);
 };
 
 extern const struct ofproto_class ofproto_dpif_class;
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index e78c80d11..649add089 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1026,6 +1026,18 @@  ofproto_ct_del_zone_timeout_policy(const char *datapath_type, uint16_t zone_id)
 
 }
 
+void
+ofproto_ct_zone_limit_update(const char *datapath_type, int32_t zone_id,
+                             int64_t *limit)
+{
+    datapath_type = ofproto_normalize_type(datapath_type);
+    const struct ofproto_class *class = ofproto_class_find__(datapath_type);
+
+    if (class && class->ct_zone_limit_update) {
+        class->ct_zone_limit_update(datapath_type, zone_id, limit);
+    }
+}
+
 
 /* Spanning Tree Protocol (STP) configuration. */
 
diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index 8efdb20a0..7ce6a65e1 100644
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -384,6 +384,8 @@  void ofproto_ct_set_zone_timeout_policy(const char *datapath_type,
                                         struct simap *timeout_policy);
 void ofproto_ct_del_zone_timeout_policy(const char *datapath_type,
                                         uint16_t zone);
+void ofproto_ct_zone_limit_update(const char *datapath_type, int32_t zone_id,
+                                  int64_t *limit);
 void ofproto_get_datapath_cap(const char *datapath_type,
                               struct smap *dp_cap);
 
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index b6c8d7faf..b13384eff 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -5221,6 +5221,60 @@  default limit=0
 zone=4,limit=0,count=0
 ])
 
+dnl Test limit set via database.
+VSCTL_ADD_DATAPATH_TABLE()
+
+AT_CHECK([ovs-appctl dpctl/flush-conntrack zone=0])
+AT_CHECK([ovs-appctl dpctl/flush-conntrack zone=3])
+
+AT_CHECK([ovs-appctl dpctl/ct-set-limits default=10])
+AT_CHECK([ovs-appctl dpctl/ct-del-limits zone=3])
+AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
+default limit=10
+zone=0,limit=5,count=0
+])
+
+AT_CHECK([ovs-vsctl set-zone-limit $DP_TYPE zone=0 limit=3])
+AT_CHECK([ovs-vsctl set-zone-limit $DP_TYPE zone=3 limit=3])
+
+OVS_WAIT_UNTIL_EQUAL([ovs-appctl dpctl/ct-get-limits], [dnl
+default limit=10
+zone=0,limit=3,count=0
+zone=3,limit=3,count=0])
+
+for i in 2 3 4 5 6; do
+  packet="50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000${i}00080000"
+  AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 \
+              "in_port=2 packet=${packet} actions=resubmit(,0)"])
+done
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "orig=.src=10\.1\.1\.3," | sort ], [0], [dnl
+udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=2),reply=(src=10.1.1.4,dst=10.1.1.3,sport=2,dport=1),zone=3
+udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=3),reply=(src=10.1.1.4,dst=10.1.1.3,sport=3,dport=1),zone=3
+udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=4),reply=(src=10.1.1.4,dst=10.1.1.3,sport=4,dport=1),zone=3
+])
+
+AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
+default limit=10
+zone=0,limit=3,count=0
+zone=3,limit=3,count=3
+])
+
+AT_CHECK([ovs-vsctl del-zone-limit $DP_TYPE zone=3])
+OVS_WAIT_UNTIL_EQUAL([ovs-appctl dpctl/ct-get-limits], [dnl
+default limit=10
+zone=0,limit=3,count=0])
+
+AT_CHECK([ovs-vsctl set-zone-limit $DP_TYPE default limit=5])
+OVS_WAIT_UNTIL_EQUAL([ovs-appctl dpctl/ct-get-limits], [dnl
+default limit=5
+zone=0,limit=3,count=0])
+
+AT_CHECK([ovs-vsctl del-zone-limit $DP_TYPE default])
+OVS_WAIT_UNTIL_EQUAL([ovs-appctl dpctl/ct-get-limits], [dnl
+default limit=0
+zone=0,limit=3,count=0])
+
 OVS_TRAFFIC_VSWITCHD_STOP(["dnl
 /could not create datapath/d
 /(Cannot allocate memory) on packet/d"])
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index e9110c1d8..5be38b890 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -157,6 +157,8 @@  struct aa_mapping {
 /* Internal representation of conntrack zone configuration table in OVSDB. */
 struct ct_zone {
     uint16_t zone_id;
+    int64_t limit;              /* Limit of allowed entries. '-1' if not
+                                 * specified. */
     struct simap tp;            /* A map from timeout policy attribute to
                                  * timeout value. */
     struct hmap_node node;      /* Node in 'struct datapath' 'ct_zones'
@@ -168,14 +170,15 @@  struct ct_zone {
 
 /* Internal representation of datapath configuration table in OVSDB. */
 struct datapath {
-    char *type;                 /* Datapath type. */
-    struct hmap ct_zones;       /* Map of 'struct ct_zone' elements, indexed
-                                 * by 'zone'. */
-    struct hmap_node node;      /* Node in 'all_datapaths' hmap. */
-    struct smap caps;           /* Capabilities. */
-    unsigned int last_used;     /* The last idl_seqno that this 'datapath'
-                                 * used in OVSDB. This number is used for
-                                 * garbage collection. */
+    char *type;                     /* Datapath type. */
+    struct hmap ct_zones;           /* Map of 'struct ct_zone' elements,
+                                     * indexed by 'zone'. */
+    struct hmap_node node;          /* Node in 'all_datapaths' hmap. */
+    struct smap caps;               /* Capabilities. */
+    unsigned int last_used;         /* The last idl_seqno that this 'datapath'
+                                     * used in OVSDB. This number is used for
+                                     * garbage collection. */
+    int64_t ct_zone_default_limit;  /* Default CT limit for all zones. */
 };
 
 /* All bridges, indexed by name. */
@@ -662,6 +665,7 @@  ct_zone_alloc(uint16_t zone_id, struct ovsrec_ct_timeout_policy *tp_cfg)
     struct ct_zone *ct_zone = xzalloc(sizeof *ct_zone);
 
     ct_zone->zone_id = zone_id;
+    ct_zone->limit = -1;
     simap_init(&ct_zone->tp);
     get_timeout_policy_from_ovsrec(&ct_zone->tp, tp_cfg);
     return ct_zone;
@@ -670,6 +674,14 @@  ct_zone_alloc(uint16_t zone_id, struct ovsrec_ct_timeout_policy *tp_cfg)
 static void
 ct_zone_remove_and_destroy(struct datapath *dp, struct ct_zone *ct_zone)
 {
+    if (!simap_is_empty(&ct_zone->tp)) {
+        ofproto_ct_del_zone_timeout_policy(dp->type, ct_zone->zone_id);
+    }
+
+    if (ct_zone->limit > -1) {
+        ofproto_ct_zone_limit_update(dp->type, ct_zone->zone_id, NULL);
+    }
+
     hmap_remove(&dp->ct_zones, &ct_zone->node);
     simap_destroy(&ct_zone->tp);
     free(ct_zone);
@@ -706,6 +718,7 @@  datapath_create(const char *type)
 {
     struct datapath *dp = xzalloc(sizeof *dp);
     dp->type = xstrdup(type);
+    dp->ct_zone_default_limit = -1;
     hmap_init(&dp->ct_zones);
     hmap_insert(&all_datapaths, &dp->node, hash_string(type, 0));
     smap_init(&dp->caps);
@@ -722,6 +735,11 @@  datapath_destroy(struct datapath *dp)
             ct_zone_remove_and_destroy(dp, ct_zone);
         }
 
+        if (dp->ct_zone_default_limit > -1) {
+            ofproto_ct_zone_limit_update(dp->type, OVS_ZONE_LIMIT_DEFAULT_ZONE,
+                                         NULL);
+        }
+
         hmap_remove(&all_datapaths, &dp->node);
         hmap_destroy(&dp->ct_zones);
         free(dp->type);
@@ -743,29 +761,50 @@  ct_zones_reconfigure(struct datapath *dp, struct ovsrec_datapath *dp_cfg)
         struct ovsrec_ct_timeout_policy *tp_cfg = zone_cfg->timeout_policy;
 
         ct_zone = ct_zone_lookup(&dp->ct_zones, zone_id);
-        if (ct_zone) {
-            struct simap new_tp = SIMAP_INITIALIZER(&new_tp);
-            get_timeout_policy_from_ovsrec(&new_tp, tp_cfg);
-            if (update_timeout_policy(&ct_zone->tp, &new_tp)) {
+        if (!ct_zone) {
+            ct_zone = ct_zone_alloc(zone_id, tp_cfg);
+            hmap_insert(&dp->ct_zones, &ct_zone->node, hash_int(zone_id, 0));
+        }
+
+        struct simap new_tp = SIMAP_INITIALIZER(&new_tp);
+        get_timeout_policy_from_ovsrec(&new_tp, tp_cfg);
+
+        if (update_timeout_policy(&ct_zone->tp, &new_tp)) {
+            if (simap_count(&ct_zone->tp)) {
                 ofproto_ct_set_zone_timeout_policy(dp->type, ct_zone->zone_id,
                                                    &ct_zone->tp);
+            } else {
+                ofproto_ct_del_zone_timeout_policy(dp->type, ct_zone->zone_id);
             }
-        } else {
-            ct_zone = ct_zone_alloc(zone_id, tp_cfg);
-            hmap_insert(&dp->ct_zones, &ct_zone->node, hash_int(zone_id, 0));
-            ofproto_ct_set_zone_timeout_policy(dp->type, ct_zone->zone_id,
-                                               &ct_zone->tp);
         }
+
+        int64_t desired_limit = zone_cfg->limit ? *zone_cfg->limit : -1;
+        if (ct_zone->limit != desired_limit) {
+            ofproto_ct_zone_limit_update(dp->type, zone_id, zone_cfg->limit);
+            ct_zone->limit = desired_limit;
+        }
+
         ct_zone->last_used = idl_seqno;
     }
 
     /* Purge 'ct_zone's no longer found in the database. */
     HMAP_FOR_EACH_SAFE (ct_zone, node, &dp->ct_zones) {
         if (ct_zone->last_used != idl_seqno) {
-            ofproto_ct_del_zone_timeout_policy(dp->type, ct_zone->zone_id);
             ct_zone_remove_and_destroy(dp, ct_zone);
         }
     }
+
+    /* Reconfigure default CT zone limit if needed. */
+    int64_t default_limit = dp_cfg->ct_zone_default_limit
+                            ? *dp_cfg->ct_zone_default_limit
+                            : -1;
+
+    if (dp->ct_zone_default_limit != default_limit) {
+        ofproto_ct_zone_limit_update(dp->type, OVS_ZONE_LIMIT_DEFAULT_ZONE,
+                                     dp_cfg->ct_zone_default_limit);
+        dp->ct_zone_default_limit = default_limit;
+    }
+
 }
 
 static void