On 10/18/23 09:56, Ales Musil wrote:
> 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 <amu...@redhat.com>
> ---
> 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 |  4 +++
>  ofproto/ofproto.c          | 12 ++++++++
>  ofproto/ofproto.h          |  2 ++
>  tests/system-traffic.at    | 54 +++++++++++++++++++++++++++++++++++
>  vswitchd/bridge.c          | 58 +++++++++++++++++++++++++++++++-------
>  7 files changed, 164 insertions(+), 10 deletions(-)
> 
> 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..33fb99280 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -1921,6 +1921,10 @@ 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. */

This function can use a behavior explanation, i.e. what happens if the limit
is NULL, or what value 'zone' zone should take for a default zone.

> +    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 25e8c3f75..445a9ffbd 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])
> +
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2 
> packet=50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000200080000
>  actions=resubmit(,0)"])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2 
> packet=50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000300080000
>  actions=resubmit(,0)"])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2 
> packet=50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000400080000
>  actions=resubmit(,0)"])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2 
> packet=50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000500080000
>  actions=resubmit(,0)"])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2 
> packet=50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000600080000
>  actions=resubmit(,0)"])

Maybe a loop instead of copying almost the same thing?
It can also probably be 
E.g.

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..4545556b5 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -157,6 +157,7 @@ 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. */

May add '-1 if not specified.' to the comment.

>      struct simap tp;            /* A map from timeout policy attribute to
>                                   * timeout value. */
>      struct hmap_node node;      /* Node in 'struct datapath' 'ct_zones'
> @@ -176,6 +177,7 @@ struct datapath {
>      unsigned int last_used;     /* The last idl_seqno that this 'datapath'
>                                   * used in OVSDB. This number is used for
>                                   * garbage collection. */
> +    int64_t ct_default_limit;   /* Default limit for CT zones. */

Should probably be ct_zone_default_limit.
Might have the same comment update.

>  };
>  
>  /* All bridges, indexed by name. */
> @@ -662,6 +664,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 +673,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 +717,7 @@ datapath_create(const char *type)
>  {
>      struct datapath *dp = xzalloc(sizeof *dp);
>      dp->type = xstrdup(type);
> +    dp->ct_default_limit = -1;
>      hmap_init(&dp->ct_zones);
>      hmap_insert(&all_datapaths, &dp->node, hash_string(type, 0));
>      smap_init(&dp->caps);
> @@ -722,6 +734,11 @@ datapath_destroy(struct datapath *dp)
>              ct_zone_remove_and_destroy(dp, ct_zone);
>          }
>  
> +        if (dp->ct_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 +760,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_default_limit != default_limit) {
> +        ofproto_ct_zone_limit_update(dp->type, OVS_ZONE_LIMIT_DEFAULT_ZONE,
> +                                     dp_cfg->ct_zone_default_limit);
> +        dp->ct_default_limit = default_limit;
> +    }
> +
>  }
>  
>  static void

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to