Re: [ovs-dev] [PATCH v2] OpenFlow: Add extn to set conntrack entries limit per zone.

[Naveen Yerramneni]

Hi Simon,

Thanks for the update. I will send v3 with nits addreesed.

Thanks,
Naveen 

> On 15-Mar-2023, at 2:24 PM, Simon Horman  wrote:
> 
> On Wed, Mar 15, 2023 at 08:27:40AM +, Naveen Yerramneni wrote:
>> Hi Simon,
>> 
>> Thanks, I will wait for your review.
>> 
>> Multiple patches were sent out due to my mistake, my apologies for that.
> 
> Understood, sorry for jumping the gun a bit there.
> 
>> Thanks,
>> Naveen
>> 
>> 
>>> On 14-Mar-2023, at 6:17 PM, Simon Horman  wrote:
>>> 
>>> On Tue, Mar 14, 2023 at 10:51:55AM +, Naveen Yerramneni wrote:
 Add OpenFlow extn to set conntrack entries limit per zone.
 
 Signed-off-by: Naveen Yerramneni 
>>> 
>>> Hi Naveen,
>>> 
>>> There were 4 postings of this patchset yesterday, and one so far today.
>>> Please consider allowing 24h between postings to give reviewers a chance to
>>> look at things.
>>> 
>>> Overall this patch looks nice to me, I reviewed it with reference to:
>>> 
>>> 2a7c4805a76d ("Add OpenFlow command to flush conntrack table entries.")
>>> 
>>> Some minor comments inline.
>>> 
>>> I will run some tests over it.
>>> And respond further if that shows up anything of interest.
> 
> I did run the automated workflow over this [1].
> Including check-offloads [2].
> And things seem clean to me.
> 
> So with the minor nits in my previous email addressed:
> 
> Reviewed-by: Simon Horman 
> 
> [1] 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_horms_ovs_actions_runs_4415889468=DwIBAg=s883GpUCOChKOHiocYtGcg=2PQjSDR7A28z1kXE1ptSm6X36oL_nCq1XxeEt7FkLmA=1ULPoISx8VbNNUUySPVpF590U5f5xd19I9VzrZ49p5qRiK4_DMTkx2kW7x9uY55Q=pAhfSRiPDj3ees57ibAJlo2tAut2yPd_kIKekNDMpa4=
>  
> [2] 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_pipermail_ovs-2Ddev_2023-2DMarch_402672.html=DwIBAg=s883GpUCOChKOHiocYtGcg=2PQjSDR7A28z1kXE1ptSm6X36oL_nCq1XxeEt7FkLmA=1ULPoISx8VbNNUUySPVpF590U5f5xd19I9VzrZ49p5qRiK4_DMTkx2kW7x9uY55Q=6ihLNNSY3oKIqfrhSJAWPXjH8G2KD-3NzQTcj9VdLr8=
>  
> 
>>> 
>>> ...
>>> 
 diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
 index f87e27a8c..6797cebac 100644
 --- a/ofproto/ofproto-dpif.c
 +++ b/ofproto/ofproto-dpif.c
 @@ -5631,6 +5631,27 @@ ct_del_zone_timeout_policy(const char 
 *datapath_type, uint16_t zone_id)
}
 }
 
 +static void
 +ct_set_zone_limit(const struct ofproto *ofproto_, const uint16_t zone_id,
 +  const uint32_t limit)
 +{
 +struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
 +struct ovs_list zone_limits = OVS_LIST_INITIALIZER(_limits);
 +
 +ct_dpif_push_zone_limit(_limits, zone_id, limit, 0);
 +int err = ct_dpif_set_limits(ofproto->backer->dpif, NULL, 
 _limits);
 +if (err) {
 +VLOG_ERR_RL(, "failed to set zone limit id=%"PRIu16", "
 +  "limit=%"PRIu32" (%s)", zone_id, limit,
 +  ovs_strerror(err));
 +} else {
 +VLOG_DBG("configured zone limit for zone=%"PRIu16", 
 limit=%"PRIu32"",
 +zone_id, limit);
 +}
 +  
 +ct_dpif_free_zone_limits(_limits); 
>>> 
>>> nit: the two lines line above have trailing whitespace.
>>> 
 +}
 +
>>> 
>>> ...

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

Re: [ovs-dev] [PATCH v2] OpenFlow: Add extn to set conntrack entries limit per zone.

[Simon Horman]

On Wed, Mar 15, 2023 at 08:27:40AM +, Naveen Yerramneni wrote:
> Hi Simon,
> 
> Thanks, I will wait for your review.
> 
> Multiple patches were sent out due to my mistake, my apologies for that.

Understood, sorry for jumping the gun a bit there.

> Thanks,
> Naveen
> 
> 
> > On 14-Mar-2023, at 6:17 PM, Simon Horman  wrote:
> > 
> > On Tue, Mar 14, 2023 at 10:51:55AM +, Naveen Yerramneni wrote:
> >> Add OpenFlow extn to set conntrack entries limit per zone.
> >> 
> >> Signed-off-by: Naveen Yerramneni 
> > 
> > Hi Naveen,
> > 
> > There were 4 postings of this patchset yesterday, and one so far today.
> > Please consider allowing 24h between postings to give reviewers a chance to
> > look at things.
> > 
> > Overall this patch looks nice to me, I reviewed it with reference to:
> > 
> > 2a7c4805a76d ("Add OpenFlow command to flush conntrack table entries.")
> > 
> > Some minor comments inline.
> > 
> > I will run some tests over it.
> > And respond further if that shows up anything of interest.

I did run the automated workflow over this [1].
Including check-offloads [2].
And things seem clean to me.

So with the minor nits in my previous email addressed:

Reviewed-by: Simon Horman 

[1] https://github.com/horms/ovs/actions/runs/4415889468
[2] https://mail.openvswitch.org/pipermail/ovs-dev/2023-March/402672.html

> > 
> > ...
> > 
> >> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> >> index f87e27a8c..6797cebac 100644
> >> --- a/ofproto/ofproto-dpif.c
> >> +++ b/ofproto/ofproto-dpif.c
> >> @@ -5631,6 +5631,27 @@ ct_del_zone_timeout_policy(const char 
> >> *datapath_type, uint16_t zone_id)
> >> }
> >> }
> >> 
> >> +static void
> >> +ct_set_zone_limit(const struct ofproto *ofproto_, const uint16_t zone_id,
> >> +  const uint32_t limit)
> >> +{
> >> +struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
> >> +struct ovs_list zone_limits = OVS_LIST_INITIALIZER(_limits);
> >> +
> >> +ct_dpif_push_zone_limit(_limits, zone_id, limit, 0);
> >> +int err = ct_dpif_set_limits(ofproto->backer->dpif, NULL, 
> >> _limits);
> >> +if (err) {
> >> +VLOG_ERR_RL(, "failed to set zone limit id=%"PRIu16", "
> >> +  "limit=%"PRIu32" (%s)", zone_id, limit,
> >> +  ovs_strerror(err));
> >> +} else {
> >> +VLOG_DBG("configured zone limit for zone=%"PRIu16", 
> >> limit=%"PRIu32"",
> >> +zone_id, limit);
> >> +}
> >> +  
> >> +ct_dpif_free_zone_limits(_limits); 
> > 
> > nit: the two lines line above have trailing whitespace.
> > 
> >> +}
> >> +
> > 
> > ...
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] OpenFlow: Add extn to set conntrack entries limit per zone.

[Naveen Yerramneni]

Hi Simon,

Thanks, I will wait for your review.

Multiple patches were sent out due to my mistake, my apologies for that.


Thanks,
Naveen


> On 14-Mar-2023, at 6:17 PM, Simon Horman  wrote:
> 
> On Tue, Mar 14, 2023 at 10:51:55AM +, Naveen Yerramneni wrote:
>> Add OpenFlow extn to set conntrack entries limit per zone.
>> 
>> Signed-off-by: Naveen Yerramneni 
> 
> Hi Naveen,
> 
> There were 4 postings of this patchset yesterday, and one so far today.
> Please consider allowing 24h between postings to give reviewers a chance to
> look at things.
> 
> Overall this patch looks nice to me, I reviewed it with reference to:
> 
> 2a7c4805a76d ("Add OpenFlow command to flush conntrack table entries.")
> 
> Some minor comments inline.
> 
> I will run some tests over it.
> And respond further if that shows up anything of interest.
> 
> ...
> 
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index f87e27a8c..6797cebac 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -5631,6 +5631,27 @@ ct_del_zone_timeout_policy(const char *datapath_type, 
>> uint16_t zone_id)
>> }
>> }
>> 
>> +static void
>> +ct_set_zone_limit(const struct ofproto *ofproto_, const uint16_t zone_id,
>> +  const uint32_t limit)
>> +{
>> +struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
>> +struct ovs_list zone_limits = OVS_LIST_INITIALIZER(_limits);
>> +
>> +ct_dpif_push_zone_limit(_limits, zone_id, limit, 0);
>> +int err = ct_dpif_set_limits(ofproto->backer->dpif, NULL, _limits);
>> +if (err) {
>> +VLOG_ERR_RL(, "failed to set zone limit id=%"PRIu16", "
>> +  "limit=%"PRIu32" (%s)", zone_id, limit,
>> +  ovs_strerror(err));
>> +} else {
>> +VLOG_DBG("configured zone limit for zone=%"PRIu16", 
>> limit=%"PRIu32"",
>> +zone_id, limit);
>> +}
>> +  
>> +ct_dpif_free_zone_limits(_limits); 
> 
> nit: the two lines line above have trailing whitespace.
> 
>> +}
>> +
> 
> ...

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

Re: [ovs-dev] [PATCH v2] OpenFlow: Add extn to set conntrack entries limit per zone.

[Simon Horman]

On Tue, Mar 14, 2023 at 10:51:55AM +, Naveen Yerramneni wrote:
> Add OpenFlow extn to set conntrack entries limit per zone.
> 
> Signed-off-by: Naveen Yerramneni 

Hi Naveen,

There were 4 postings of this patchset yesterday, and one so far today.
Please consider allowing 24h between postings to give reviewers a chance to
look at things.

Overall this patch looks nice to me, I reviewed it with reference to:

2a7c4805a76d ("Add OpenFlow command to flush conntrack table entries.")

Some minor comments inline.

I will run some tests over it.
And respond further if that shows up anything of interest.

...

> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index f87e27a8c..6797cebac 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -5631,6 +5631,27 @@ ct_del_zone_timeout_policy(const char *datapath_type, 
> uint16_t zone_id)
>  }
>  }
>  
> +static void
> +ct_set_zone_limit(const struct ofproto *ofproto_, const uint16_t zone_id,
> +  const uint32_t limit)
> +{
> +struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
> +struct ovs_list zone_limits = OVS_LIST_INITIALIZER(_limits);
> +
> +ct_dpif_push_zone_limit(_limits, zone_id, limit, 0);
> +int err = ct_dpif_set_limits(ofproto->backer->dpif, NULL, _limits);
> +if (err) {
> +VLOG_ERR_RL(, "failed to set zone limit id=%"PRIu16", "
> +  "limit=%"PRIu32" (%s)", zone_id, limit,
> +  ovs_strerror(err));
> +} else {
> +VLOG_DBG("configured zone limit for zone=%"PRIu16", limit=%"PRIu32"",
> +zone_id, limit);
> +}
> +  
> +ct_dpif_free_zone_limits(_limits); 

nit: the two lines line above have trailing whitespace.

> +}
> +

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

Re: [ovs-dev] [PATCH v2] OpenFlow: Add extn to set conntrack entries limit per zone.

[0-day Robot]

References:  <20230314105155.12571-1-naveen.yerramn...@nutanix.com>
 

Bleep bloop.  Greetings Naveen Yerramneni, I am a robot and I have tried out 
your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line has non-spaces leading whitespace
WARNING: Line has trailing whitespace
#157 FILE: ofproto/ofproto-dpif.c:5641:
  

WARNING: Line has trailing whitespace
#158 FILE: ofproto/ofproto-dpif.c:5642:
ct_dpif_free_zone_limits(_limits); 

Lines checked: 344, Warnings: 3, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2] OpenFlow: Add extn to set conntrack entries limit per zone.

[Naveen Yerramneni]

Add OpenFlow extn to set conntrack entries limit per zone.

Signed-off-by: Naveen Yerramneni 
---
Notes:
  v1 -> v2
  - Fix memory leak and added logs

 NEWS   |  2 ++
 include/openflow/nicira-ext.h  | 10 ++
 include/openvswitch/ofp-msgs.h |  4 
 lib/ofp-bundle.c   |  1 +
 lib/ofp-print.c| 11 +++
 lib/rconn.c|  1 +
 ofproto/ofproto-dpif.c | 22 ++
 ofproto/ofproto-provider.h |  4 
 ofproto/ofproto.c  | 25 +
 tests/ofp-print.at | 10 ++
 tests/ovs-ofctl.at | 12 
 utilities/ovs-ofctl.8.in   |  5 +
 utilities/ovs-ofctl.c  | 34 ++
 13 files changed, 141 insertions(+)

diff --git a/NEWS b/NEWS
index fe6055a27..f6ae60856 100644
--- a/NEWS
+++ b/NEWS
@@ -32,6 +32,8 @@ v3.1.0 - xx xxx 
- OpenFlow:
  * New OpenFlow extension NXT_CT_FLUSH to flush connections matching
the specified fields.
+ * New OpenFlow extension NXT_CT_SET_ZONE_LIMIT to set conntrack table
+   limit at zone level.
- ovs-ctl:
  * New option '--dump-hugepages' to include hugepages in core dumps. This
can assist with postmortem analysis involving DPDK, but may also produce
diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
index 768775898..0f93ea21c 100644
--- a/include/openflow/nicira-ext.h
+++ b/include/openflow/nicira-ext.h
@@ -1101,4 +1101,14 @@ struct nx_ct_flush {
 };
 OFP_ASSERT(sizeof(struct nx_ct_flush) == 8);
 
+/* NXT_CT_SET_ZONE_LIMIT.
+ *
+ * Sets connection tracking table zone limit. */
+struct nx_ct_zone_limit {
+uint8_t zero[2];   /* Must be zero. */
+ovs_be16 zone_id;  /* Connection tracking zone. */
+ovs_be32 limit;/* Drop limit. */
+};
+OFP_ASSERT(sizeof(struct nx_ct_zone_limit) == 8);
+
 #endif /* openflow/nicira-ext.h */
diff --git a/include/openvswitch/ofp-msgs.h b/include/openvswitch/ofp-msgs.h
index 708427fc0..a9518557e 100644
--- a/include/openvswitch/ofp-msgs.h
+++ b/include/openvswitch/ofp-msgs.h
@@ -518,6 +518,9 @@ enum ofpraw {
 /* NXT 1.0+ (32): struct nx_ct_flush, uint8_t[8][]. */
 OFPRAW_NXT_CT_FLUSH,
 
+/* NXT 1.0+ (35): struct nx_ct_zone_limit. */
+OFPRAW_NXT_CT_SET_ZONE_LIMIT,
+
 /* NXST 1.0+ (3): void. */
 OFPRAW_NXST_IPFIX_BRIDGE_REQUEST,
 
@@ -776,6 +779,7 @@ enum ofptype {
 OFPTYPE_IPFIX_FLOW_STATS_REPLY,   /* OFPRAW_NXST_IPFIX_FLOW_REPLY */
 OFPTYPE_CT_FLUSH_ZONE,/* OFPRAW_NXT_CT_FLUSH_ZONE. */
 OFPTYPE_CT_FLUSH, /* OFPRAW_NXT_CT_FLUSH. */
+OFPTYPE_CT_SET_ZONE_LIMIT,/* OFPRAW_NXT_CT_SET_ZONE_LIMIT. */
 
 /* Flow monitor extension. */
 OFPTYPE_FLOW_MONITOR_CANCEL,  /* OFPRAW_NXT_FLOW_MONITOR_CANCEL.
diff --git a/lib/ofp-bundle.c b/lib/ofp-bundle.c
index 941a8370e..3ed1f30d8 100644
--- a/lib/ofp-bundle.c
+++ b/lib/ofp-bundle.c
@@ -293,6 +293,7 @@ ofputil_is_bundlable(enum ofptype type)
 case OFPTYPE_IPFIX_FLOW_STATS_REPLY:
 case OFPTYPE_CT_FLUSH_ZONE:
 case OFPTYPE_CT_FLUSH:
+case OFPTYPE_CT_SET_ZONE_LIMIT:
 break;
 }
 
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 874079b84..8a64b72c0 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -967,6 +967,15 @@ ofp_print_nxt_ct_flush(struct ds *string, const struct 
ofp_header *oh)
 return 0;
 }
 
+static enum ofperr
+ofp_print_nxt_ct_set_zone_limit(struct ds *string,
+const struct nx_ct_zone_limit *nzl)
+{
+ds_put_format(string, " zone_id=%"PRIu16, ntohs(nzl->zone_id));
+ds_put_format(string, " limit=%"PRIu32, ntohl(nzl->limit));
+return 0;
+}
+
 static enum ofperr
 ofp_to_string__(const struct ofp_header *oh,
 const struct ofputil_port_map *port_map,
@@ -1204,6 +1213,8 @@ ofp_to_string__(const struct ofp_header *oh,
 return ofp_print_nxt_ct_flush_zone(string, ofpmsg_body(oh));
 case OFPTYPE_CT_FLUSH:
 return ofp_print_nxt_ct_flush(string, oh);
+case OFPTYPE_CT_SET_ZONE_LIMIT:
+return ofp_print_nxt_ct_set_zone_limit(string, ofpmsg_body(oh));
 }
 
 return 0;
diff --git a/lib/rconn.c b/lib/rconn.c
index 4afa21515..91c982d98 100644
--- a/lib/rconn.c
+++ b/lib/rconn.c
@@ -1427,6 +1427,7 @@ is_admitted_msg(const struct ofpbuf *b)
 case OFPTYPE_IPFIX_FLOW_STATS_REPLY:
 case OFPTYPE_CT_FLUSH_ZONE:
 case OFPTYPE_CT_FLUSH:
+case OFPTYPE_CT_SET_ZONE_LIMIT:
 default:
 return true;
 }
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index f87e27a8c..6797cebac 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5631,6 +5631,27 @@ ct_del_zone_timeout_policy(const char *datapath_type, 
uint16_t zone_id)
 }
 }
 
+static void
+ct_set_zone_limit(const struct ofproto *ofproto_, const uint16_t zone_id,
+  const uint32_t limit)
+{
+struct