diff mbox series

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

Message ID 20231129072334.91442-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
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Ales Musil Nov. 29, 2023, 7:23 a.m. 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>
---
v7: Rebase on top of current master.
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 54e057d43..bfae28d96 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;
 }
@@ -5532,6 +5534,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);
 }
 
@@ -5555,6 +5559,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
@@ -5635,6 +5641,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)
 {
@@ -6925,4 +6963,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 1fe22ab41..4709200bc 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -285,6 +285,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 7c26adda7..9dc7e48cd 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