subject:"\[ovs\-dev\] \[PATCH\] vswitchd, ofproto\-dpif\: Add support for CT limit"

Re: [ovs-dev] [PATCH] vswitchd, ofproto-dpif: Add support for CT limit

2023-09-18 Thread Ilya Maximets

On 9/11/23 08:36, Ales Musil wrote:
> 
> 
> On Sat, Sep 9, 2023 at 12:33 AM Ilya Maximets  > wrote:
> 
> On 8/10/23 14:48, Ales Musil wrote:
> > Add support for setting CT zone limit via ovs-vswitchd
> > database CT_Zone entry. The limit is propagated
> > into corresponding datapath.
> 
> 
> Hi Ilya,
>  
> 
> 
> Thanks for tha patch!  I didn't try it, but see some comments inline.
> 
> 
> thank you for the review.
>  
> 
> 
> > In order to keep
> > backward compatibility the dpctl/ct-set-limits command
> > can overwrite the database settings.
> 
> This doesn't seem like a good option.  If anything, the behavior
> should be the opposite, i.e. if the value is set in the database,
> it should overwrite whatever user have set before via appctl.
> We can treat a zero as a 'whatever currently is set' value, and
> all other values set in the database has to be enforced.  Or the
> column in the database may be optional, in this case we may treat
> an empty column as not configured and zero as unlimited.
> 
> 
> In this version 0 is default system behavior I guess for backward 
> compatibility it would make sense to leave the setting untouched for 0.

What happens if you set the column to some value and then set it
back to zero?

>  
> 
> 
> The dpctl/ct-set-limits may warn or fail in case of conflicting data.
> We also need a NEWS record about the new functionality.
> 
> 
> Fail would be better IMO if we go with the database value being enforced 
> unless 0. WDYT?

Failure should be fine.

Best regards, Ilya Maximets.

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

Re: [ovs-dev] [PATCH] vswitchd, ofproto-dpif: Add support for CT limit

2023-09-10 Thread Ales Musil

On Sat, Sep 9, 2023 at 12:33 AM Ilya Maximets  wrote:

> On 8/10/23 14:48, Ales Musil wrote:
> > Add support for setting CT zone limit via ovs-vswitchd
> > database CT_Zone entry. The limit is propagated
> > into corresponding datapath.
>

Hi Ilya,


>
> Thanks for tha patch!  I didn't try it, but see some comments inline.
>

thank you for the review.


>
> > In order to keep
> > backward compatibility the dpctl/ct-set-limits command
> > can overwrite the database settings.
>
> This doesn't seem like a good option.  If anything, the behavior
> should be the opposite, i.e. if the value is set in the database,
> it should overwrite whatever user have set before via appctl.
> We can treat a zero as a 'whatever currently is set' value, and
> all other values set in the database has to be enforced.  Or the
> column in the database may be optional, in this case we may treat
> an empty column as not configured and zero as unlimited.
>

In this version 0 is default system behavior I guess for backward
compatibility it would make sense to leave the setting untouched for 0.


>
> The dpctl/ct-set-limits may warn or fail in case of conflicting data.
> We also need a NEWS record about the new functionality.
>

Fail would be better IMO if we go with the database value being enforced
unless 0. WDYT?


>
> >
> > Signed-off-by: Ales Musil 
> > ---
> >  ofproto/ofproto-dpif.c | 42 +++
> >  ofproto/ofproto-dpif.h |  5 
> >  ofproto/ofproto-provider.h |  8 ++
> >  ofproto/ofproto.c  | 23 +
> >  ofproto/ofproto.h  |  3 +++
> >  tests/system-traffic.at| 51 +-
> >  vswitchd/bridge.c  | 10 
> >  vswitchd/vswitch.ovsschema |  8 --
> >  vswitchd/vswitch.xml   |  5 
> >  9 files changed, 152 insertions(+), 3 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index e22ca757a..0c2818a5a 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -5522,6 +5522,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 +5547,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 +5629,42 @@ ct_del_zone_timeout_policy(const char
> *datapath_type, uint16_t zone_id)
> >  }
> >  }
> >
> > +static void
> > +ct_zone_limit_queue_update(const char *datapath_type, uint16_t zone_id,
> > +   uint32_t limit)
> > +{
> > +struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
> > + datapath_type);
> > +if (!backer) {
> > +return;
> > +}
> > +
> > +struct ovs_list *queue = limit ?
> > +&backer->ct_zone_limits_to_add :
> &backer->ct_zone_limits_to_del;
> > +
> > +ct_dpif_push_zone_limit(queue, zone_id, limit, 0);
> > +}
> > +
> > +static void
> > +ct_zone_limits_commit(const char *datapath_type)
> > +{
> > +struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
> > + datapath_type);
> > +if (!backer) {
> > +return;
> > +}
> > +
> > +if (!ovs_list_is_empty(&backer->ct_zone_limits_to_add)) {
> > +ct_dpif_set_limits(backer->dpif, NULL,
> &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 +6954,6 @@ const struct ofproto_class ofproto_dpif_class = {
> >  ct_flush,   /* ct_flush */
> >  ct_set_zone_timeout_policy,
> >  ct_del_zone_timeout_policy,
> > +ct_zone_limit_queue_update,
> > +ct_zone_limits_commit
>
> Please, keep the trailing comma.
>
> >  };
> > 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;
> > +
> > +st

Re: [ovs-dev] [PATCH] vswitchd, ofproto-dpif: Add support for CT limit

2023-09-08 Thread Ilya Maximets

On 8/10/23 14:48, Ales Musil wrote:
> Add support for setting CT zone limit via ovs-vswitchd
> database CT_Zone entry. The limit is propagated
> into corresponding datapath.

Thanks for tha patch!  I didn't try it, but see some comments inline.

> In order to keep
> backward compatibility the dpctl/ct-set-limits command
> can overwrite the database settings.

This doesn't seem like a good option.  If anything, the behavior
should be the opposite, i.e. if the value is set in the database,
it should overwrite whatever user have set before via appctl.
We can treat a zero as a 'whatever currently is set' value, and
all other values set in the database has to be enforced.  Or the
column in the database may be optional, in this case we may treat
an empty column as not configured and zero as unlimited.

The dpctl/ct-set-limits may warn or fail in case of conflicting data.
We also need a NEWS record about the new functionality.

> 
> Signed-off-by: Ales Musil 
> ---
>  ofproto/ofproto-dpif.c | 42 +++
>  ofproto/ofproto-dpif.h |  5 
>  ofproto/ofproto-provider.h |  8 ++
>  ofproto/ofproto.c  | 23 +
>  ofproto/ofproto.h  |  3 +++
>  tests/system-traffic.at| 51 +-
>  vswitchd/bridge.c  | 10 
>  vswitchd/vswitch.ovsschema |  8 --
>  vswitchd/vswitch.xml   |  5 
>  9 files changed, 152 insertions(+), 3 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index e22ca757a..0c2818a5a 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -5522,6 +5522,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 +5547,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 +5629,42 @@ ct_del_zone_timeout_policy(const char *datapath_type, 
> uint16_t zone_id)
>  }
>  }
>  
> +static void
> +ct_zone_limit_queue_update(const char *datapath_type, uint16_t zone_id,
> +   uint32_t limit)
> +{
> +struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
> + datapath_type);
> +if (!backer) {
> +return;
> +}
> +
> +struct ovs_list *queue = limit ?
> +&backer->ct_zone_limits_to_add : &backer->ct_zone_limits_to_del;
> +
> +ct_dpif_push_zone_limit(queue, zone_id, limit, 0);
> +}
> +
> +static void
> +ct_zone_limits_commit(const char *datapath_type)
> +{
> +struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
> + datapath_type);
> +if (!backer) {
> +return;
> +}
> +
> +if (!ovs_list_is_empty(&backer->ct_zone_limits_to_add)) {
> +ct_dpif_set_limits(backer->dpif, NULL, 
> &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 +6954,6 @@ const struct ofproto_class ofproto_dpif_class = {
>  ct_flush,   /* ct_flush */
>  ct_set_zone_timeout_policy,
>  ct_del_zone_timeout_policy,
> +ct_zone_limit_queue_update,
> +ct_zone_limits_commit

Please, keep the trailing comma.

>  };
> 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 143ded690..99e4017c3 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -1

Re: [ovs-dev] [PATCH] vswitchd, ofproto-dpif: Add support for CT limit

2023-08-16 Thread Simon Horman

On Thu, Aug 10, 2023 at 02:48:27PM +0200, Ales Musil wrote:
> Add support for setting CT zone limit via ovs-vswitchd
> database CT_Zone entry. The limit is propagated
> into corresponding datapath. In order to keep
> backward compatibility the dpctl/ct-set-limits command
> can overwrite the database settings.
> 
> Signed-off-by: Ales Musil 

Acked-by: Simon Horman 

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

[ovs-dev] [PATCH] vswitchd, ofproto-dpif: Add support for CT limit

2023-08-10 Thread Ales Musil

Add support for setting CT zone limit via ovs-vswitchd
database CT_Zone entry. The limit is propagated
into corresponding datapath. In order to keep
backward compatibility the dpctl/ct-set-limits command
can overwrite the database settings.

Signed-off-by: Ales Musil 
---
 ofproto/ofproto-dpif.c | 42 +++
 ofproto/ofproto-dpif.h |  5 
 ofproto/ofproto-provider.h |  8 ++
 ofproto/ofproto.c  | 23 +
 ofproto/ofproto.h  |  3 +++
 tests/system-traffic.at| 51 +-
 vswitchd/bridge.c  | 10 
 vswitchd/vswitch.ovsschema |  8 --
 vswitchd/vswitch.xml   |  5 
 9 files changed, 152 insertions(+), 3 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index e22ca757a..0c2818a5a 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5522,6 +5522,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 +5547,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 +5629,42 @@ ct_del_zone_timeout_policy(const char *datapath_type, 
uint16_t zone_id)
 }
 }
 
+static void
+ct_zone_limit_queue_update(const char *datapath_type, uint16_t zone_id,
+   uint32_t limit)
+{
+struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
+ datapath_type);
+if (!backer) {
+return;
+}
+
+struct ovs_list *queue = limit ?
+&backer->ct_zone_limits_to_add : &backer->ct_zone_limits_to_del;
+
+ct_dpif_push_zone_limit(queue, zone_id, limit, 0);
+}
+
+static void
+ct_zone_limits_commit(const char *datapath_type)
+{
+struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
+ datapath_type);
+if (!backer) {
+return;
+}
+
+if (!ovs_list_is_empty(&backer->ct_zone_limits_to_add)) {
+ct_dpif_set_limits(backer->dpif, NULL, &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 +6954,6 @@ const struct ofproto_class ofproto_dpif_class = {
 ct_flush,   /* ct_flush */
 ct_set_zone_timeout_policy,
 ct_del_zone_timeout_policy,
+ct_zone_limit_queue_update,
+ct_zone_limits_commit
 };
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 143ded690..99e4017c3 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);
+
+/* Queues the CT zone limit update. In order for this change to take
+ * effect it needs to be commited. */
+void (*ct_zone_limit_queue_update)(const char *dp_type, uint16_t zone,
+   uint32_t limit);
+
+/* Commits the queued CT zone limit updates to datapath. */
+void (*ct_zone_limits_commit)(const char *dp_type);
 };
 
 extern const struct ofproto_class ofproto_dpif_class;
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index dbf4958bc..c293d97ac 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1026,6 +1026,29 @@ ofproto_ct_del_zone_timeout_policy(const char 
*datapath_type, uint16_t zone_id)
 
 }
 
+void
+ofproto_ct_zone_limit_queue_update(const char

Re: [ovs-dev] [PATCH] vswitchd, ofproto-dpif: Add support for CT limit

Re: [ovs-dev] [PATCH] vswitchd, ofproto-dpif: Add support for CT limit

Re: [ovs-dev] [PATCH] vswitchd, ofproto-dpif: Add support for CT limit

Re: [ovs-dev] [PATCH] vswitchd, ofproto-dpif: Add support for CT limit

[ovs-dev] [PATCH] vswitchd, ofproto-dpif: Add support for CT limit

5 matches

Site Navigation

Mail list logo

Footer information