Re: [ovs-dev] [PATCH ovn v4 3/4] controller: Prepare structure around CT zone limiting.

[Ales Musil]

On Thu, Jul 11, 2024 at 10:52 PM Dumitru Ceara  wrote:

> On 6/26/24 07:56, Ales Musil wrote:
> > In order to be able to store CT limits for specified zone, store the
> > zone inside separate struct instead of simap. This allows to add
> > the addition of limit without changing the whole infrastructure again.
> >
> > This is a preparation step for the CT zone limits.
> >
> > Signed-off-by: Ales Musil 
> > ---
>
> Hi Ales,
>

Hi Dumitru,

thank you for the review.


>
> > v4: Rebase on top of latest main.
> > Address comments from Lorenzo.
> > v3: Rebase on top of latest main.
> > v2: Fix NULL ptr deref.
> > ---
> >  controller/ct-zone.c| 169 +---
> >  controller/ct-zone.h|  13 ++-
> >  controller/ofctrl.c |   2 +-
> >  controller/ovn-controller.c |  15 ++--
> >  controller/physical.c   |  17 ++--
> >  controller/physical.h   |   2 +-
> >  6 files changed, 126 insertions(+), 92 deletions(-)
> >
> > diff --git a/controller/ct-zone.c b/controller/ct-zone.c
> > index e4f66a52a..6ed1e4108 100644
> > --- a/controller/ct-zone.c
> > +++ b/controller/ct-zone.c
> > @@ -26,12 +26,14 @@ ct_zone_restore(const struct
> sbrec_datapath_binding_table *dp_table,
> >  struct ct_zone_ctx *ctx, const char *name, int zone);
> >  static void ct_zone_add_pending(struct shash *pending_ct_zones,
> >  enum ct_zone_pending_state state,
> > -int zone, bool add, const char *name);
> > +struct ct_zone *zone, bool add,
> > +const char *name);
> >  static int ct_zone_get_snat(const struct sbrec_datapath_binding *dp);
> >  static bool ct_zone_assign_unused(struct ct_zone_ctx *ctx,
> >const char *zone_name, int
> *scan_start);
> > -static bool ct_zone_remove(struct ct_zone_ctx *ctx,
> > -   struct simap_node *ct_zone);
> > +static bool ct_zone_remove(struct ct_zone_ctx *ctx, const char *name);
> > +static void ct_zone_add(struct ct_zone_ctx *ctx, const char *name,
> > +uint16_t zone, bool set_pending);
> >
> >  void
> >  ct_zones_restore(struct ct_zone_ctx *ctx,
> > @@ -47,7 +49,8 @@ ct_zones_restore(struct ct_zone_ctx *ctx,
> >  struct ct_zone_pending_entry *ctpe = pending_node->data;
> >
> >  if (ctpe->add) {
> > -ct_zone_restore(dp_table, ctx, pending_node->name,
> ctpe->zone);
> > +ct_zone_restore(dp_table, ctx, pending_node->name,
> > +ctpe->ct_zone.zone);
> >  }
> >  }
> >
> > @@ -91,7 +94,6 @@ void
> >  ct_zones_update(const struct sset *local_lports,
> >  const struct hmap *local_datapaths, struct ct_zone_ctx
> *ctx)
> >  {
> > -struct simap_node *ct_zone;
> >  int scan_start = 1;
> >  const char *user;
> >  struct sset all_users = SSET_INITIALIZER(&all_users);
> > @@ -132,12 +134,14 @@ ct_zones_update(const struct sset *local_lports,
> >  }
> >
> >  /* Delete zones that do not exist in above sset. */
> > -SIMAP_FOR_EACH_SAFE (ct_zone, &ctx->current) {
> > -if (!sset_contains(&all_users, ct_zone->name)) {
> > -ct_zone_remove(ctx, ct_zone);
> > -} else if (!simap_find(&req_snat_zones, ct_zone->name)) {
> > -bitmap_set1(unreq_snat_zones_map, ct_zone->data);
> > -simap_put(&unreq_snat_zones, ct_zone->name, ct_zone->data);
> > +struct shash_node *node;
> > +SHASH_FOR_EACH_SAFE (node, &ctx->current) {
> > +struct ct_zone *ct_zone = node->data;
> > +if (!sset_contains(&all_users, node->name)) {
> > +ct_zone_remove(ctx, node->name);
> > +} else if (!simap_find(&req_snat_zones, node->name)) {
> > +bitmap_set1(unreq_snat_zones_map, ct_zone->zone);
> > +simap_put(&unreq_snat_zones, node->name, ct_zone->zone);
> >  }
> >  }
> >
> > @@ -152,7 +156,7 @@ ct_zones_update(const struct sset *local_lports,
> >  struct simap_node *unreq_node;
> >  SIMAP_FOR_EACH_SAFE (unreq_node, &unreq_snat_zones) {
> >  if (unreq_node->data == snat_req_node->data) {
> > -simap_find_and_delete(&ctx->current,
> unreq_node->name);
> > +ct_zone_remove(ctx, unreq_node->name);
> >  simap_delete(&unreq_snat_zones, unreq_node);
> >  }
> >  }
> > @@ -163,26 +167,12 @@ ct_zones_update(const struct sset *local_lports,
> >  bitmap_set0(unreq_snat_zones_map, snat_req_node->data);
> >  }
> >
> > -struct simap_node *node = simap_find(&ctx->current,
> > - snat_req_node->name);
> > -if (node) {
> > -if (node->data != snat_req_node->data) {
> > -/* Zone request has changed for this node. delete old
> entry and

[ovs-dev] [PATCH] learn: Support learn output with OFPP_TABLE port.

[LIU Yulong]

The ofport OFPP_TABLE(0xfff9) is defined as perform
actions in flow table. It will send the packet back
to table=0 to do reprocess. Add this port to learn
output action then users can leverage it to do some
flow table re-entry.

Signed-off-by: LIU Yulong 
---
 lib/learn.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/learn.c b/lib/learn.c
index a62add2fd..68d27307a 100644
--- a/lib/learn.c
+++ b/lib/learn.c
@@ -167,6 +167,7 @@ learn_execute(const struct ofpact_learn *learn, const 
struct flow *flow,
 if (ofp_to_u16(port) < ofp_to_u16(OFPP_MAX)
 || port == OFPP_IN_PORT
 || port == OFPP_FLOOD
+|| port == OFPP_TABLE
 || port == OFPP_LOCAL
 || port == OFPP_ALL) {
 ofpact_put_OUTPUT(ofpacts)->port = port;
-- 
2.27.0

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

[ovs-dev] [Patch ovn v5] Text respresntations for drop sampling.

[Jacob Tanenbaum]

Created a new column in the southbound database to hardcode a human readable
description for flows. This first use is describing why the flow is dropping 
packets.
The new column is called flow_desc and will create southbound database entries 
like this

_uuid   : 20f1897b-477e-47ae-a32c-c546d83ec097
actions : 
"sample(probability=65535,collector_set=123,obs_domain=1,obs_point=$cookie); /* 
drop */"
controller_meter: []
external_ids: {source="northd.c:8721", stage-name=ls_in_l2_unknown}
flow_desc   : "No L2 destination"
logical_datapath: []
logical_dp_group: ee3c3db5-98a2-4f34-8a84-409deae140a7
match   : "outport == \"none\""
pipeline: ingress
priority: 50
table_id: 27
tags: {}
hash: 0

future work includes entering more flow_desc for more flows and adding
flow_desc to the actions as a comment.

Signed-off-by: Jacob Tanenbaum 
Suggested-by: Dumitru Ceara 
Reported-at: https://issues.redhat.com/browse/FDP-307

---

v1: initial version
v2: correct commit message
make the flow_desc a char*
correct a typo in the ovn-sb.xml
correct the test
v3: rebase issue with NEWS file
v4: remove options:debug_drop_domain_id="1" from testing as we
do not depend on it
v5: introduce string wrapper
increment ovs-sb.ovsschema version
correct the testing
added descriptions to more dropped packets

diff --git a/NEWS b/NEWS
index 3e392ff08..0039b04be 100644
--- a/NEWS
+++ b/NEWS
@@ -38,6 +38,8 @@ Post v24.03.0
 ability to disable "VXLAN mode" to extend available tunnel IDs space for
 datapaths from 4095 to 16711680.  For more details see man ovn-nb(5) for
 mentioned option.
+  - Added a new column in the southbound database "flow_desc" to provide
+human readable context to flows.
 
 OVN v24.03.0 - 01 Mar 2024
 --
diff --git a/lib/ovn-util.h b/lib/ovn-util.h
index f75b821b6..6f5334d27 100644
--- a/lib/ovn-util.h
+++ b/lib/ovn-util.h
@@ -466,6 +466,30 @@ void sorted_array_apply_diff(const struct sorted_array *a1,
 bool add),
  const void *arg);
 
+/* A wrapper that holds strings */
+struct string_wrapper
+{
+char *str;
+bool owns_string;
+};
+
+static inline struct string_wrapper
+string_wrapper_create(char *str, bool take_ownership)
+{
+return (struct string_wrapper) {
+.str = str,
+.owns_string = take_ownership,
+};
+}
+
+static inline void
+string_wrapper_destroy(struct string_wrapper *s)
+{
+if (s->owns_string) {
+free(s->str);
+}
+}
+
 /* Utilities around properly handling exit command. */
 struct ovn_exit_args {
 struct unixctl_conn **conns;
diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c
index b2c60b5de..c66181801 100644
--- a/northd/lflow-mgr.c
+++ b/northd/lflow-mgr.c
@@ -25,6 +25,7 @@
 #include "debug.h"
 #include "lflow-mgr.h"
 #include "lib/ovn-parallel-hmap.h"
+#include "lib/ovn-util.h"
 
 VLOG_DEFINE_THIS_MODULE(lflow_mgr);
 
@@ -36,7 +37,7 @@ static void ovn_lflow_init(struct ovn_lflow *, struct 
ovn_datapath *od,
uint16_t priority, char *match,
char *actions, char *io_port,
char *ctrl_meter, char *stage_hint,
-   const char *where);
+   const char *where, struct string_wrapper flow_desc);
 static struct ovn_lflow *ovn_lflow_find(const struct hmap *lflows,
 enum ovn_stage stage,
 uint16_t priority, const char *match,
@@ -52,7 +53,7 @@ static struct ovn_lflow *do_ovn_lflow_add(
 const char *actions, const char *io_port,
 const char *ctrl_meter,
 const struct ovsdb_idl_row *stage_hint,
-const char *where);
+const char *where, struct string_wrapper flow_desc);
 
 
 static struct ovs_mutex *lflow_hash_lock(const struct hmap *lflow_table,
@@ -173,6 +174,7 @@ struct ovn_lflow {
   * 'dpg_bitmap'. */
 struct ovn_dp_group *dpg;/* Link to unique Sb datapath group. */
 const char *where;
+struct string_wrapper flow_desc;
 
 struct uuid sb_uuid; /* SB DB row uuid, specified by northd. */
 struct ovs_list referenced_by;  /* List of struct lflow_ref_node. */
@@ -659,7 +661,7 @@ lflow_table_add_lflow(struct lflow_table *lflow_table,
   const char *match, const char *actions,
   const char *io_port, const char *ctrl_meter,
   const struct ovsdb_idl_row *stage_hint,
-  const char *where,
+  const char *where, struct string_wrapper flow_desc,
   struct lflow_ref *lflow_ref)
 OVS_EXCLUDED(fake_hash_mutex)
 {
@@ -679,7 +681,7 @@ lflow_table_add_lflow(struct lflow_table *lflow_table,
 do_

Re: [ovs-dev] [PATCH] learn: Support learn output with OFPP_TABLE port

[0-day Robot]

Bleep bloop.  Greetings LIU Yulong, 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: The subject summary should end with a dot.
Subject: learn: Support learn output with OFPP_TABLE port
ERROR: Author LIU Yulong  needs to sign off.
Lines checked: 31, Warnings: 1, Errors: 1


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] learn: Support learn output with OFPP_TABLE port

[LIU Yulong]

The ofport OFPP_TABLE(0xfff9) is defined as perform
actions in flow table. It will send the packet back
to table=0 to do reprocess. Add this port to learn
output action then users can leverage it to do some
flow table re-entry.
---
 lib/learn.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/learn.c b/lib/learn.c
index b465a9954..63b06a7c8 100644
--- a/lib/learn.c
+++ b/lib/learn.c
@@ -171,6 +171,7 @@ learn_execute(const struct ofpact_learn *learn, const 
struct flow *flow,
 if (ofp_to_u16(port) < ofp_to_u16(OFPP_MAX)
 || port == OFPP_IN_PORT
 || port == OFPP_FLOOD
+|| port == OFPP_TABLE
 || port == OFPP_LOCAL
 || port == OFPP_ALL) {
 ofpact_put_OUTPUT(ofpacts)->port = port;
-- 
2.27.0

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

Re: [ovs-dev] [PATCH net-next v2] selftests: openvswitch: retry instead of sleep

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski :

On Wed, 10 Jul 2024 11:04:59 +0200 you wrote:
> There are a couple of places where the test script "sleep"s to wait for
> some external condition to be met.
> 
> This is error prone, specially in slow systems (identified in CI by
> "KSFT_MACHINE_SLOW=yes").
> 
> To fix this, add a "ovs_wait" function that tries to execute a command
> a few times until it succeeds. The timeout used is set to 5s for
> "normal" systems and doubled if a slow CI machine is detected.
> 
> [...]

Here is the summary with links:
  - [net-next,v2] selftests: openvswitch: retry instead of sleep
https://git.kernel.org/netdev/net-next/c/5e724cb688a2

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html


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

Re: [ovs-dev] [PATCH v2 13/13] ofp-actions: Load data from fields in sample action.

[0-day Robot]

Bleep bloop.  Greetings Adrian Moreno, 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.


Patch skipped due to previous failure.

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

Re: [ovs-dev] [PATCH v2 12/13] ofproto-dpif-xlate: Avoid allocating mf_subfield.

[0-day Robot]

Bleep bloop.  Greetings Adrian Moreno, 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.


Patch skipped due to previous failure.

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

Re: [ovs-dev] [PATCH v2 11/13] ofproto: xlate: Make sampled drops explicit.

[0-day Robot]

Bleep bloop.  Greetings Adrian Moreno, 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.


Patch skipped due to previous failure.

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

Re: [ovs-dev] [PATCH v2 10/13] tests: Test local sampling.

[0-day Robot]

Bleep bloop.  Greetings Adrian Moreno, 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.


Patch skipped due to previous failure.

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

Re: [ovs-dev] [PATCH v2 09/13] tests: Add test-psample testing utility.

[0-day Robot]

Bleep bloop.  Greetings Adrian Moreno, 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.


Patch skipped due to previous failure.

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

Re: [ovs-dev] [PATCH v2 08/13] ofproto-dpif-lsample: Show stats via unixctl.

[0-day Robot]

Bleep bloop.  Greetings Adrian Moreno, 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.


Patch skipped due to previous failure.

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

Re: [ovs-dev] [PATCH v2 06/13] ofproto-dpif-xlate: Use psample for local sample.

[0-day Robot]

Bleep bloop.  Greetings Adrian Moreno, 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.


Patch skipped due to previous failure.

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

Re: [ovs-dev] [PATCH v2 07/13] ofproto-dpif-xlate-cache: Add lsample to xcache.

[0-day Robot]

Bleep bloop.  Greetings Adrian Moreno, 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.


Patch skipped due to previous failure.

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

Re: [ovs-dev] [PATCH v2 05/13] vswitchd: Add local sampling to vswitchd schema.

[0-day Robot]

Bleep bloop.  Greetings Adrian Moreno, 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.


git-am:
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 vswitchd: Add local sampling to vswitchd schema.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


Patch skipped due to previous failure.

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 13/13] ofp-actions: Load data from fields in sample action.

[Adrian Moreno]

When sample action gets used as a way of sampling traffic with
controller-generated metadata (i.e: obs_domain_id and obs_point_id),
the controller will have to increase the number of flows to ensure each
part of the pipeline contains the right metadata.

As an example, if the controller decides to sample stateful traffic, it
could store the computed metadata for each connection in the conntrack
label. However, for established connections, a flow must be created for
each different ct_label value with a sample action that contains a
different hardcoded obs_domain and obs_point id.

This patch adds a new version of the NXAST_RAW_SAMPLE* action (number 4)
that supports specifying the observation point and domain using an
OpenFlow field reference, so now the controller can express:

 sample(...
obs_domain_id=NXM_NX_CT_LABEL[0..31],
obs_point_id=NXM_NX_CT_LABEL[32..63]
...
   )

Signed-off-by: Adrian Moreno 
---
 Documentation/ref/ovs-actions.7.rst |  12 +-
 NEWS|   2 +
 include/openvswitch/ofp-actions.h   |   8 +-
 lib/ofp-actions.c   | 245 +---
 ofproto/ofproto-dpif-xlate.c|  44 -
 python/ovs/flow/ofp.py  |   8 +-
 python/ovs/flow/ofp_act.py  |   6 +-
 tests/ofp-actions.at|   8 +
 tests/ofproto-dpif.at   |  44 +
 tests/ovs-ofctl.at  |  14 ++
 tests/system-traffic.at |  79 +
 11 files changed, 429 insertions(+), 41 deletions(-)

diff --git a/Documentation/ref/ovs-actions.7.rst 
b/Documentation/ref/ovs-actions.7.rst
index 80acd9070..01021dc0a 100644
--- a/Documentation/ref/ovs-actions.7.rst
+++ b/Documentation/ref/ovs-actions.7.rst
@@ -2201,13 +2201,17 @@ The following *argument* forms are accepted:
 The unsigned 32-bit integer identifier of the set of sample collectors to
 send sampled packets to.  Defaults to 0.
 
-  ``obs_domain_id=``\ *id*
+  ``obs_domain_id=``\ *value*
 When sending samples to IPFIX collectors, the unsigned 32-bit integer
-Observation Domain ID sent in every IPFIX flow record.  Defaults to 0.
+Observation Domain ID sent in every IPFIX flow record. The *value* may
+be specified as a 32-bit integer or a field or subfield in the syntax
+described under `Field Specifications`_ above. Defaults to 0.
 
-  ``obs_point_id=``\ *id*
+  ``obs_point_id=``\ *value*
 When sending samples to IPFIX collectors, the unsigned 32-bit integer
-Observation Point ID sent in every IPFIX flow record.  Defaults to 0.
+Observation Point ID sent in every IPFIX flow record. The *value* may
+be specified as a 32-bit integer or a field or subfield in the syntax
+described under `Field Specifications`_ above. Defaults to 0.
 
   ``sampling_port=``\ *port*
 Sample packets on *port*, which should be the ingress or egress port.  This
diff --git a/NEWS b/NEWS
index 096ff4d7d..7d7461c57 100644
--- a/NEWS
+++ b/NEWS
@@ -22,6 +22,8 @@ Post-v3.3.0
  support this feature by using the new datapath "psample" action.
- A new unixctl command 'lsample/show' shows packet and bytes statistics
  per local sample exporter.
+   - OpenFlow: a new version of the "sample" action is introduced that allows
+ the use of subfields in obs_point_id and obs_domain_id.
 
 
 v3.3.0 - 16 Feb 2024
diff --git a/include/openvswitch/ofp-actions.h 
b/include/openvswitch/ofp-actions.h
index 7b57e49ad..56dc2c147 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -1015,14 +1015,16 @@ enum nx_action_sample_direction {
 
 /* OFPACT_SAMPLE.
  *
- * Used for NXAST_SAMPLE, NXAST_SAMPLE2, and NXAST_SAMPLE3. */
+ * Used for NXAST_SAMPLE, NXAST_SAMPLE2, NXAST_SAMPLE3 and NXAST_SAMPLE4. */
 struct ofpact_sample {
 OFPACT_PADDED_MEMBERS(
 struct ofpact ofpact;
 uint16_t probability;   /* Always positive. */
 uint32_t collector_set_id;
-uint32_t obs_domain_id;
-uint32_t obs_point_id;
+uint32_t obs_domain_imm;
+struct mf_subfield obs_domain_src;
+uint32_t obs_point_imm;
+struct mf_subfield obs_point_src;
 ofp_port_t sampling_port;
 enum nx_action_sample_direction direction;
 );
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index da7b1dd31..6b030bf29 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -330,6 +330,8 @@ enum ofp_raw_action_type {
 NXAST_RAW_SAMPLE2,
 /* NX1.0+(41): struct nx_action_sample2. */
 NXAST_RAW_SAMPLE3,
+/* NX1.0+(51): struct nx_action_sample4. VLMFF */
+NXAST_RAW_SAMPLE4,
 
 /* NX1.0+(34): struct nx_action_conjunction. */
 NXAST_RAW_CONJUNCTION,
@@ -6188,6 +6190,34 @@ struct nx_action_sample2 {
  };
  OFP_ASSERT(sizeof(struct nx_action_sample2) == 32);
 
+/* Action structure for NXAST_SAMPLE4
+ *
+ * NXAST_SAMPLE4 was added in Open vSwitch 3.4.0.  Compared to NXAST_SAMPLE3,
+ * it adds support for using field speci

[ovs-dev] [PATCH v2 11/13] ofproto: xlate: Make sampled drops explicit.

[Adrian Moreno]

When the flow translation results in a datapath action list whose last
action is an "observational" action, i.e: one generated for IPFIX,
sFlow or local sampling applications, the packet is actually going to be
dropped (and observed).

In that case, add an explicit drop action so that drop statistics remain
accurate.

Combine the "optimizations" and other odp_actions "tweaks" into a single
function.

Signed-off-by: Adrian Moreno 
---
 ofproto/ofproto-dpif-xlate.c |  52 +++-
 ofproto/ofproto-dpif-xlate.h |   4 ++
 tests/drop-stats.at  | 113 +++
 tests/ofproto-dpif.at|  47 ++-
 4 files changed, 200 insertions(+), 16 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 0a0964348..15e89e6a8 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3415,6 +3415,7 @@ compose_sample_action(struct xlate_ctx *ctx,
 struct ofproto *ofproto = &ctx->xin->ofproto->up;
 uint32_t meter_id = ofproto->slowpath_meter_id;
 size_t cookie_offset = 0;
+size_t observe_offset;
 
 /* The meter action is only used to throttle userspace actions.
  * If they are not needed and the sampling rate is 100%, avoid generating
@@ -3432,6 +3433,7 @@ compose_sample_action(struct xlate_ctx *ctx,
 }
 
 if (args->psample) {
+observe_offset = ctx->odp_actions->size;
 odp_put_psample_action(ctx->odp_actions,
args->psample->group_id,
(void *) &args->psample->cookie,
@@ -3443,6 +3445,7 @@ compose_sample_action(struct xlate_ctx *ctx,
 nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_METER, meter_id);
 }
 
+observe_offset = ctx->odp_actions->size;
 odp_port_t odp_port = ofp_port_to_odp_port(
 ctx->xbridge, ctx->xin->flow.in_port.ofp_port);
 uint32_t pid = dpif_port_get_pid(ctx->xbridge->dpif, odp_port);
@@ -3457,6 +3460,9 @@ compose_sample_action(struct xlate_ctx *ctx,
 if (is_sample) {
 nl_msg_end_nested(ctx->odp_actions, actions_offset);
 nl_msg_end_nested(ctx->odp_actions, sample_offset);
+ctx->xout->last_observe_offset = sample_offset;
+} else {
+ctx->xout->last_observe_offset = observe_offset;
 }
 
 return cookie_offset;
@@ -8066,12 +8072,14 @@ xlate_wc_finish(struct xlate_ctx *ctx)
 }
 }
 
-/* This will optimize the odp actions generated. For now, it will remove
- * trailing clone actions that are unnecessary. */
+/* This will tweak the odp actions generated. For now, it will:
+ *  - Remove trailing clone actions that are unnecessary.
+ *  - Add an explicit drop action if the last action is observational or the
+ *action list is empty. */
 static void
-xlate_optimize_odp_actions(struct xlate_in *xin)
+xlate_tweak_odp_actions(struct xlate_ctx *ctx)
 {
-struct ofpbuf *actions = xin->odp_actions;
+struct ofpbuf *actions = ctx->xin->odp_actions;
 struct nlattr *last_action = NULL;
 struct nlattr *a;
 int left;
@@ -8085,9 +8093,16 @@ xlate_optimize_odp_actions(struct xlate_in *xin)
 last_action = a;
 }
 
+if (!last_action) {
+if (ovs_explicit_drop_action_supported(ctx->xbridge->ofproto)) {
+put_drop_action(actions, XLATE_OK);
+}
+return;
+}
+
 /* Remove the trailing clone() action, by directly embedding the nested
  * actions. */
-if (last_action && nl_attr_type(last_action) == OVS_ACTION_ATTR_CLONE) {
+if (nl_attr_type(last_action) == OVS_ACTION_ATTR_CLONE) {
 void *dest;
 
 nl_msg_reset_size(actions,
@@ -8096,6 +8111,16 @@ xlate_optimize_odp_actions(struct xlate_in *xin)
 
 dest = nl_msg_put_uninit(actions, nl_attr_get_size(last_action));
 memmove(dest, nl_attr_get(last_action), nl_attr_get_size(last_action));
+return;
+}
+
+/* If the last action of the list is an observability action, add an
+ * explicit drop action so that drop statistics remain reliable. */
+if (ctx->xout->last_observe_offset != UINT32_MAX &&
+(char *) last_action == (char *) actions->data +
+ ctx->xout->last_observe_offset &&
+ovs_explicit_drop_action_supported(ctx->xbridge->ofproto)) {
+put_drop_action(actions, XLATE_OK);
 }
 }
 
@@ -8113,6 +8138,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out 
*xout)
 *xout = (struct xlate_out) {
 .slow = 0,
 .recircs = RECIRC_REFS_EMPTY_INITIALIZER,
+.last_observe_offset = UINT32_MAX,
 };
 
 struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, &xcfgp);
@@ -8541,17 +8567,15 @@ exit:
 xout->slow = 0;
 if (xin->odp_actions) {
 ofpbuf_clear(xin->odp_actions);
+/* Make the drop explicit if the datapath supports it. */
+if (ovs_explicit_drop_action_supported(ctx.xbridge->ofproto)) {
+

[ovs-dev] [PATCH v2 06/13] ofproto-dpif-xlate: Use psample for local sample.

[Adrian Moreno]

Use the newly added psample action to implement OpenFlow sample() actions
with local sampling configuration if possible.

A bit of refactoring in compose_sample_actions arguments helps make it a
bit more readable.

Signed-off-by: Adrian Moreno 
---
 ofproto/ofproto-dpif-lsample.c |  16 +++
 ofproto/ofproto-dpif-lsample.h |   5 +
 ofproto/ofproto-dpif-xlate.c   | 238 ++---
 ofproto/ofproto-dpif-xlate.h   |   5 +-
 ofproto/ofproto-dpif.c |   2 +-
 tests/ofproto-dpif.at  | 159 ++
 6 files changed, 345 insertions(+), 80 deletions(-)

diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
index a2b2e8059..833ce923f 100644
--- a/ofproto/ofproto-dpif-lsample.c
+++ b/ofproto/ofproto-dpif-lsample.c
@@ -140,6 +140,22 @@ dpif_lsample_set_options(struct dpif_lsample *lsample,
 return changed;
 }
 
+/* Returns the group_id for a given collector_set_id, if it exists. */
+bool
+dpif_lsample_get_group_id(struct dpif_lsample *ps, uint32_t collector_set_id,
+  uint32_t *group_id)
+{
+struct lsample_exporter_node *node;
+bool found = false;
+
+node = dpif_lsample_find_exporter_node(ps, collector_set_id);
+if (node) {
+found = true;
+*group_id = node->exporter.options.group_id;
+}
+return found;
+}
+
 struct dpif_lsample *
 dpif_lsample_create(void)
 {
diff --git a/ofproto/ofproto-dpif-lsample.h b/ofproto/ofproto-dpif-lsample.h
index a491c137d..26517a645 100644
--- a/ofproto/ofproto-dpif-lsample.h
+++ b/ofproto/ofproto-dpif-lsample.h
@@ -18,6 +18,7 @@
 #define OFPROTO_DPIF_LSAMPLE_H 1
 
 #include 
+#include 
 #include 
 
 struct dpif_lsample;
@@ -32,4 +33,8 @@ bool dpif_lsample_set_options(struct dpif_lsample *,
   const struct ofproto_lsample_options *,
   size_t n_opts);
 
+bool dpif_lsample_get_group_id(struct dpif_lsample *,
+   uint32_t collector_set_id,
+   uint32_t *group_id);
+
 #endif /* OFPROTO_DPIF_LSAMPLE_H */
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 7c4950895..9f32982b0 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -45,6 +45,7 @@
 #include "nx-match.h"
 #include "odp-execute.h"
 #include "ofproto/ofproto-dpif-ipfix.h"
+#include "ofproto/ofproto-dpif-lsample.h"
 #include "ofproto/ofproto-dpif-mirror.h"
 #include "ofproto/ofproto-dpif-monitor.h"
 #include "ofproto/ofproto-dpif-sflow.h"
@@ -116,6 +117,7 @@ struct xbridge {
 struct mbridge *mbridge;  /* Mirroring. */
 struct dpif_sflow *sflow; /* SFlow handle, or null. */
 struct dpif_ipfix *ipfix; /* Ipfix handle, or null. */
+struct dpif_lsample *lsample; /* Local sample handle, or null. */
 struct netflow *netflow;  /* Netflow handle, or null. */
 struct stp *stp;  /* STP or null if disabled. */
 struct rstp *rstp;/* RSTP or null if disabled. */
@@ -686,6 +688,7 @@ static void xlate_xbridge_set(struct xbridge *, struct dpif 
*,
   const struct mbridge *,
   const struct dpif_sflow *,
   const struct dpif_ipfix *,
+  const struct dpif_lsample *,
   const struct netflow *,
   bool forward_bpdu, bool has_in_band,
   const struct dpif_backer_support *,
@@ -1069,6 +1072,7 @@ xlate_xbridge_set(struct xbridge *xbridge,
   const struct mbridge *mbridge,
   const struct dpif_sflow *sflow,
   const struct dpif_ipfix *ipfix,
+  const struct dpif_lsample *lsample,
   const struct netflow *netflow,
   bool forward_bpdu, bool has_in_band,
   const struct dpif_backer_support *support,
@@ -1099,6 +1103,11 @@ xlate_xbridge_set(struct xbridge *xbridge,
 xbridge->ipfix = dpif_ipfix_ref(ipfix);
 }
 
+if (xbridge->lsample != lsample) {
+dpif_lsample_unref(xbridge->lsample);
+xbridge->lsample = dpif_lsample_ref(lsample);
+}
+
 if (xbridge->stp != stp) {
 stp_unref(xbridge->stp);
 xbridge->stp = stp_ref(stp);
@@ -1213,9 +1222,10 @@ xlate_xbridge_copy(struct xbridge *xbridge)
 xlate_xbridge_set(new_xbridge,
   xbridge->dpif, xbridge->ml, xbridge->stp,
   xbridge->rstp, xbridge->ms, xbridge->mbridge,
-  xbridge->sflow, xbridge->ipfix, xbridge->netflow,
-  xbridge->forward_bpdu, xbridge->has_in_band,
-  &xbridge->support, xbridge->addr);
+  xbridge->sflow, xbridge->ipfix, xbridge->lsample,
+  xbridge->netflow, xbridge->forward_bpdu,
+  xbridge->has_in_band, &xbridge->suppor

[ovs-dev] [PATCH v2 12/13] ofproto-dpif-xlate: Avoid allocating mf_subfield.

[Adrian Moreno]

"enum mf_subfield" (a 128byte object) is dynamically allocated a few
times just to set it to an all-ones mask.

Avoid dynamically allocating them by creating a static all-ones mask
similar to what was done with "exact_match_mask".

Suggested-by: Eelco Chaudron 
Signed-off-by: Adrian Moreno 
---
 include/openvswitch/meta-flow.h |  3 +++
 lib/meta-flow.c |  2 ++
 ofproto/ofproto-dpif-xlate.c| 16 +---
 3 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
index 3b0220aaa..ca422e697 100644
--- a/include/openvswitch/meta-flow.h
+++ b/include/openvswitch/meta-flow.h
@@ -2233,6 +2233,9 @@ union mf_subvalue {
 };
 BUILD_ASSERT_DECL(sizeof(union mf_value) == sizeof (union mf_subvalue));
 
+/* A const mf_subvalue with all bits initialized to ones. */
+extern const union mf_subvalue exact_sub_match_mask ;
+
 bool mf_subvalue_intersect(const union mf_subvalue *a_value,
const union mf_subvalue *a_mask,
const union mf_subvalue *b_value,
diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index aa7cf1fcb..499be04b6 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -71,8 +71,10 @@ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 
5);
 #define MF_VALUE_EXACT_64 MF_VALUE_EXACT_32, MF_VALUE_EXACT_32
 #define MF_VALUE_EXACT_128 MF_VALUE_EXACT_64, MF_VALUE_EXACT_64
 #define MF_VALUE_EXACT_INITIALIZER { .tun_metadata = { MF_VALUE_EXACT_128 } }
+#define MF_SUBVALUE_EXACT_INITIALIZER { .u8 = { MF_VALUE_EXACT_128 } }
 
 const union mf_value exact_match_mask = MF_VALUE_EXACT_INITIALIZER;
+const union mf_subvalue exact_sub_match_mask = MF_SUBVALUE_EXACT_INITIALIZER;
 
 static void nxm_init(void);
 
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 15e89e6a8..843ea2f79 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5598,15 +5598,12 @@ xlate_output_reg_action(struct xlate_ctx *ctx,
 {
 uint64_t port = mf_get_subfield(&or->src, &ctx->xin->flow);
 if (port <= UINT16_MAX) {
-union mf_subvalue *value = xmalloc(sizeof *value);
-
 xlate_report(ctx, OFT_DETAIL, "output port is %"PRIu64, port);
-memset(value, 0xff, sizeof *value);
-mf_write_subfield_flow(&or->src, value, &ctx->wc->masks);
+mf_write_subfield_flow(&or->src, &exact_sub_match_mask,
+   &ctx->wc->masks);
 xlate_output_action(ctx, u16_to_ofp(port), or->max_len,
 false, is_last_action, false,
 group_bucket_action);
-free(value);
 } else {
 xlate_report(ctx, OFT_WARN, "output port %"PRIu64" is out of range",
  port);
@@ -6574,9 +6571,6 @@ compose_conntrack_action(struct xlate_ctx *ctx, struct 
ofpact_conntrack *ofc,
 {
 uint16_t zone;
 if (ofc->zone_src.field) {
-union mf_subvalue *value = xmalloc(sizeof *value);
-memset(value, 0xff, sizeof *value);
-
 zone = mf_get_subfield(&ofc->zone_src, &ctx->xin->flow);
 if (ctx->xin->frozen_state) {
 /* If the upcall is a resume of a recirculation, we only need to
@@ -6585,13 +6579,13 @@ compose_conntrack_action(struct xlate_ctx *ctx, struct 
ofpact_conntrack *ofc,
  * which will invalidate the megaflow with old the recirc_id.
  */
 if (!mf_is_frozen_metadata(ofc->zone_src.field)) {
-mf_write_subfield_flow(&ofc->zone_src, value,
+mf_write_subfield_flow(&ofc->zone_src, &exact_sub_match_mask,
&ctx->wc->masks);
 }
 } else {
-mf_write_subfield_flow(&ofc->zone_src, value, &ctx->wc->masks);
+mf_write_subfield_flow(&ofc->zone_src, &exact_sub_match_mask,
+   &ctx->wc->masks);
 }
-free(value);
 } else {
 zone = ofc->zone_imm;
 }
-- 
2.45.2

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

[ovs-dev] [PATCH v2 10/13] tests: Test local sampling.

[Adrian Moreno]

Test simultaneous IPFIX and local sampling including slow-path.

Signed-off-by: Adrian Moreno 
---
 tests/system-common-macros.at |   4 +
 tests/system-traffic.at   | 359 ++
 2 files changed, 363 insertions(+)

diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index 2a68cd664..e9be021f3 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -378,3 +378,7 @@ m4_define([OVS_CHECK_GITHUB_ACTION],
 # OVS_CHECK_DROP_ACTION()
 m4_define([OVS_CHECK_DROP_ACTION],
 [AT_SKIP_IF([! grep -q "Datapath supports explicit drop action" 
ovs-vswitchd.log])])
+
+# OVS_CHECK_PSAMPLE()
+m4_define([OVS_CHECK_PSAMPLE],
+[AT_SKIP_IF([! grep -q "Datapath supports psample action" 
ovs-vswitchd.log])])
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 3f1a15445..a83099304 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -9103,3 +9103,362 @@ OVS_WAIT_UNTIL([ovs-pcap p2.pcap | grep -q "m4_join([], 
[^],
 
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
+
+AT_BANNER([local-sampling])
+
+m4_define([SAMPLE_ACTION],
+
[sample(probability=65535,collector_set_id=$1,obs_domain_id=$2,obs_point_id=$3)]dnl
+)
+
+AT_SETUP([psample - sanity check])
+OVS_TRAFFIC_VSWITCHD_START()
+OVS_CHECK_PSAMPLE()
+
+ADD_NAMESPACES(at_ns0, at_ns1)
+
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
+ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
+
+AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
+-- create Flow_Sample_Collector_Set id=1 bridge=@br0 \
+   local_group_id=10 \
+-- create Flow_Sample_Collector_Set id=2 bridge=@br0 \
+   local_group_id=12],
+ [0], [ignore])
+
+AT_DATA([flows.txt], [dnl
+arp actions=NORMAL
+in_port=ovs-p0,ip actions=SAMPLE_ACTION(1, 2853183536, 2856341600),ovs-p1
+in_port=ovs-p1,ip actions=SAMPLE_ACTION(2, 3138396208, 3141554272),ovs-p0
+])
+
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+
+OVS_DAEMONIZE([ovstest test-psample > psample.out], [psample.pid])
+OVS_WAIT_UNTIL([grep -q "Listening for psample events" psample.out])
+
+NS_CHECK_EXEC([at_ns0], [ping -q -c 1 10.1.1.2 | FORMAT_PING], [0], [dnl
+1 packets transmitted, 1 received, 0% packet loss, time 0ms
+])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows -m --names], [0], [stdout])
+AT_CHECK([grep -q 'actions:psample(group=10,cookie=0xaa102030aa405060),ovs-p1' 
stdout])
+AT_CHECK([grep -q 'actions:psample(group=12,cookie=0xbb102030bb405060),ovs-p0' 
stdout])
+
+m4_define([SAMPLE1], [m4_join([ ],
+[group_id=0xa,prob=4294967295],
+[obs_domain=0xaa102030,obs_point=0xaa405060],
+[.*icmp.*nw_src=10.1.1.1,nw_dst=10.1.1.2])])
+
+m4_define([SAMPLE2], [m4_join([ ],
+[group_id=0xc,prob=4294967295],
+[obs_domain=0xbb102030,obs_point=0xbb405060],
+[.*icmp.*nw_src=10.1.1.2,nw_dst=10.1.1.1])])
+
+OVS_WAIT_UNTIL([grep -qE 'SAMPLE1' psample.out])
+OVS_WAIT_UNTIL([grep -qE 'SAMPLE2' psample.out])
+
+AT_CHECK([ovs-appctl lsample/show br0], [0], [dnl
+Local sample statistics for bridge "br0":
+Collector Set ID: 1:
+  Group ID : 10
+  Total packets: 1
+  Total bytes  : 98
+
+Collector Set ID: 2:
+  Group ID : 12
+  Total packets: 1
+  Total bytes  : 98
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([psample - sanity check IPv6])
+OVS_TRAFFIC_VSWITCHD_START()
+OVS_CHECK_PSAMPLE()
+
+ADD_NAMESPACES(at_ns0, at_ns1)
+
+ADD_VETH(p0, at_ns0, br0, "fc00::1/96")
+ADD_VETH(p1, at_ns1, br0, "fc00::2/96")
+
+AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
+-- create Flow_Sample_Collector_Set id=1 bridge=@br0 \
+   local_group_id=10 \
+-- create Flow_Sample_Collector_Set id=2 bridge=@br0 \
+   local_group_id=12],
+ [0], [ignore])
+
+AT_DATA([flows.txt], [dnl
+priority=100,in_port=ovs-p0,ip6,icmp6,icmpv6_type=128 actions=SAMPLE_ACTION(1, 
2853183536, 2856341600),ovs-p1
+priority=100,in_port=ovs-p1,ip6,icmp6,icmpv6_type=129 actions=SAMPLE_ACTION(2, 
3138396208, 3141554272),ovs-p0
+priority=0 actions=NORMAL
+])
+
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+
+OVS_DAEMONIZE([ovstest test-psample > psample.out], [psample.pid])
+OVS_WAIT_UNTIL([grep -q "Listening for psample events" psample.out])
+
+OVS_WAIT_UNTIL_EQUAL([ip netns exec at_ns0 ping6 -I fc00::1 -q -W 2 -c 1 
fc00::2 | FORMAT_PING], [dnl
+1 packets transmitted, 1 received, 0% packet loss, time 0ms])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows -m --names], [0], [stdout])
+AT_CHECK([grep -q 'actions:psample(group=10,cookie=0xaa102030aa405060),ovs-p1' 
stdout])
+AT_CHECK([grep -q 'actions:psample(group=12,cookie=0xbb102030bb405060),ovs-p0' 
stdout])
+
+m4_define([SAMPLE1], [m4_join([ ],
+[group_id=0xa,prob=4294967295],
+[obs_domain=0xaa102030,obs_point=0xaa405060],
+[.*icmp6.*ipv6_src=fc00::1,ipv6_dst=fc00::2])])
+m4_define([SAMPLE2], [m4_join([ ],
+[group_id=0xc,prob=4294967295],
+[obs_domain=0xbb102030,obs_p

[ovs-dev] [PATCH v2 04/13] ofproto: Add ofproto-dpif-lsample.

[Adrian Moreno]

Add a new resource in ofproto-dpif and the corresponding API in
ofproto_provider.h to represent and local sampling configuration.

Signed-off-by: Adrian Moreno 
---
 ofproto/automake.mk|   2 +
 ofproto/ofproto-dpif-lsample.c | 187 +
 ofproto/ofproto-dpif-lsample.h |  35 ++
 ofproto/ofproto-dpif.c |  38 +++
 ofproto/ofproto-dpif.h |   1 +
 ofproto/ofproto-provider.h |   9 ++
 ofproto/ofproto.c  |  12 +++
 ofproto/ofproto.h  |   8 ++
 8 files changed, 292 insertions(+)
 create mode 100644 ofproto/ofproto-dpif-lsample.c
 create mode 100644 ofproto/ofproto-dpif-lsample.h

diff --git a/ofproto/automake.mk b/ofproto/automake.mk
index 7c08b563b..cb1361b8a 100644
--- a/ofproto/automake.mk
+++ b/ofproto/automake.mk
@@ -30,6 +30,8 @@ ofproto_libofproto_la_SOURCES = \
ofproto/ofproto-dpif.h \
ofproto/ofproto-dpif-ipfix.c \
ofproto/ofproto-dpif-ipfix.h \
+   ofproto/ofproto-dpif-lsample.c \
+   ofproto/ofproto-dpif-lsample.h \
ofproto/ofproto-dpif-mirror.c \
ofproto/ofproto-dpif-mirror.h \
ofproto/ofproto-dpif-monitor.c \
diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
new file mode 100644
index 0..a2b2e8059
--- /dev/null
+++ b/ofproto/ofproto-dpif-lsample.c
@@ -0,0 +1,187 @@
+/*
+ * Copyright (c) 2024 Red Hat, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include 
+#include "ofproto-dpif-lsample.h"
+
+#include "cmap.h"
+#include "hash.h"
+#include "ofproto.h"
+#include "openvswitch/thread.h"
+
+/* Dpif local sampling.
+ *
+ * Thread safety: dpif_lsample allows lockless concurrent reads of local
+ * sampling exporters as long as the following restrictions are met:
+ *   1) While the last reference is being dropped, i.e: a thread is calling
+ *  "dpif_lsample_unref" on the last reference, other threads cannot call
+ *  "dpif_lsample_ref".
+ *   2) Threads do not quiese while holding references to internal
+ *  lsample_exporter objects.
+ */
+
+struct dpif_lsample {
+struct cmap exporters;  /* Contains lsample_exporter_node instances
+ * indexed by collector_set_id. */
+struct ovs_mutex mutex; /* Protects concurrent insertion/deletion
+ * of exporters. */
+
+struct ovs_refcount ref_cnt;/* Controls references to this instance. */
+};
+
+struct lsample_exporter {
+struct ofproto_lsample_options options;
+};
+
+struct lsample_exporter_node {
+struct cmap_node node;  /* In dpif_lsample->exporters. */
+struct lsample_exporter exporter;
+};
+
+static void
+dpif_lsample_delete_exporter(struct dpif_lsample *lsample,
+ struct lsample_exporter_node *node)
+{
+ovs_mutex_lock(&lsample->mutex);
+cmap_remove(&lsample->exporters, &node->node,
+hash_int(node->exporter.options.collector_set_id, 0));
+ovs_mutex_unlock(&lsample->mutex);
+
+ovsrcu_postpone(free, node);
+}
+
+/* Adds an exporter with the provided options which are copied. */
+static struct lsample_exporter_node *
+dpif_lsample_add_exporter(struct dpif_lsample *lsample,
+  const struct ofproto_lsample_options *options)
+{
+struct lsample_exporter_node *node;
+
+node = xzalloc(sizeof *node);
+node->exporter.options = *options;
+
+ovs_mutex_lock(&lsample->mutex);
+cmap_insert(&lsample->exporters, &node->node,
+hash_int(options->collector_set_id, 0));
+ovs_mutex_unlock(&lsample->mutex);
+
+return node;
+}
+
+static struct lsample_exporter_node *
+dpif_lsample_find_exporter_node(const struct dpif_lsample *lsample,
+const uint32_t collector_set_id)
+{
+struct lsample_exporter_node *node;
+
+CMAP_FOR_EACH_WITH_HASH (node, node,
+hash_int(collector_set_id, 0),
+&lsample->exporters) {
+if (node->exporter.options.collector_set_id == collector_set_id) {
+return node;
+}
+}
+return NULL;
+}
+
+/* Sets the lsample configuration and returns true if the configuration
+ * has changed. */
+bool
+dpif_lsample_set_options(struct dpif_lsample *lsample,
+ const struct ofproto_lsample_options *options,
+ size_t n_options)
+{
+const struct

[ovs-dev] [PATCH v2 08/13] ofproto-dpif-lsample: Show stats via unixctl.

[Adrian Moreno]

Add a command to dump statistics per exporter.

Signed-off-by: Adrian Moreno 
---
 NEWS   |   2 +
 ofproto/ofproto-dpif-lsample.c | 111 +
 ofproto/ofproto-dpif-lsample.h |   2 +
 ofproto/ofproto-dpif.c |   1 +
 4 files changed, 116 insertions(+)

diff --git a/NEWS b/NEWS
index 5f5871ef0..096ff4d7d 100644
--- a/NEWS
+++ b/NEWS
@@ -20,6 +20,8 @@ Post-v3.3.0
  allows samples to be emitted locally (instead of via IPFIX) in a
  datapath-specific manner.  The Linux kernel datapath is the first to
  support this feature by using the new datapath "psample" action.
+   - A new unixctl command 'lsample/show' shows packet and bytes statistics
+ per local sample exporter.
 
 
 v3.3.0 - 16 Feb 2024
diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
index f4bf49a7a..926f88c4e 100644
--- a/ofproto/ofproto-dpif-lsample.c
+++ b/ofproto/ofproto-dpif-lsample.c
@@ -21,7 +21,10 @@
 #include "dpif.h"
 #include "hash.h"
 #include "ofproto.h"
+#include "ofproto-dpif.h"
+#include "openvswitch/dynamic-string.h"
 #include "openvswitch/thread.h"
+#include "unixctl.h"
 
 /* Dpif local sampling.
  *
@@ -219,3 +222,111 @@ dpif_lsample_unref(struct dpif_lsample *lsample)
 dpif_lsample_destroy(lsample);
 }
 }
+
+static int
+comp_exporter_collector_id(const void *a_, const void *b_)
+{
+const struct lsample_exporter_node *a, *b;
+
+a = *(struct lsample_exporter_node **) a_;
+b = *(struct lsample_exporter_node **) b_;
+
+if (a->exporter.options.collector_set_id >
+b->exporter.options.collector_set_id) {
+return 1;
+}
+if (a->exporter.options.collector_set_id <
+b->exporter.options.collector_set_id) {
+return -1;
+}
+return 0;
+}
+
+static void
+lsample_exporter_list(struct dpif_lsample *lsample,
+  struct lsample_exporter_node ***list,
+  size_t *num_exporters)
+{
+struct lsample_exporter_node **exporter_list;
+struct lsample_exporter_node *node;
+size_t k = 0, n;
+
+n = cmap_count(&lsample->exporters);
+
+exporter_list = xcalloc(n, sizeof *exporter_list);
+
+CMAP_FOR_EACH (node, node, &lsample->exporters) {
+if (k >= n) {
+break;
+}
+exporter_list[k++] = node;
+}
+
+qsort(exporter_list, k, sizeof *exporter_list, comp_exporter_collector_id);
+
+*list = exporter_list;
+*num_exporters = k;
+}
+
+static void
+lsample_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
+ const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
+{
+struct lsample_exporter_node **node_list = NULL;
+struct ds ds = DS_EMPTY_INITIALIZER;
+const struct ofproto_dpif *ofproto;
+size_t i, num;
+
+ofproto = ofproto_dpif_lookup_by_name(argv[1]);
+if (!ofproto) {
+unixctl_command_reply_error(conn, "no such bridge");
+return;
+}
+
+if (!ofproto->lsample) {
+unixctl_command_reply_error(conn,
+"no local sampling exporters configured");
+return;
+}
+
+ds_put_format(&ds, "Local sample statistics for bridge \"%s\":\n",
+  argv[1]);
+
+lsample_exporter_list(ofproto->lsample, &node_list, &num);
+
+for (i = 0; i < num; i++) {
+uint64_t n_bytes;
+uint64_t n_packets;
+
+struct lsample_exporter_node *node = node_list[i];
+
+atomic_read_relaxed(&node->exporter.n_packets, &n_packets);
+atomic_read_relaxed(&node->exporter.n_bytes, &n_bytes);
+
+if (i) {
+ds_put_cstr(&ds, "\n");
+}
+
+ds_put_format(&ds, "Collector Set ID: %"PRIu32":\n",
+node->exporter.options.collector_set_id);
+ds_put_format(&ds, "  Group ID : %"PRIu32"\n",
+node->exporter.options.group_id);
+ds_put_format(&ds, "  Total packets: %"PRIu64"\n", n_packets);
+ds_put_format(&ds, "  Total bytes  : %"PRIu64"\n", n_bytes);
+}
+
+free(node_list);
+unixctl_command_reply(conn, ds_cstr(&ds));
+ds_destroy(&ds);
+}
+
+void dpif_lsample_init(void)
+{
+static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
+
+if (ovsthread_once_start(&once)) {
+unixctl_command_register("lsample/show", "bridge", 1, 1,
+ lsample_unixctl_show, NULL);
+ovsthread_once_done(&once);
+}
+}
diff --git a/ofproto/ofproto-dpif-lsample.h b/ofproto/ofproto-dpif-lsample.h
index dbf7237bd..6ac7580ac 100644
--- a/ofproto/ofproto-dpif-lsample.h
+++ b/ofproto/ofproto-dpif-lsample.h
@@ -25,6 +25,8 @@ struct dpif_lsample;
 struct ofproto_lsample_options;
 struct dpif_flow_stats;
 
+void dpif_lsample_init(void);
+
 struct dpif_lsample *dpif_lsample_create(void);
 
 struct dpif_lsample *dpif_lsample_ref(const struct dpif_lsample *);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 72638

[ovs-dev] [PATCH v2 09/13] tests: Add test-psample testing utility.

[Adrian Moreno]

This simple program reads from psample and prints the packets to stdout.

Signed-off-by: Adrian Moreno 
---
 include/linux/automake.mk |   1 +
 include/linux/psample.h   |  68 +
 tests/automake.mk |   3 +-
 tests/test-psample.c  | 288 ++
 4 files changed, 359 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/psample.h
 create mode 100644 tests/test-psample.c

diff --git a/include/linux/automake.mk b/include/linux/automake.mk
index cdae5eedc..ac306b53c 100644
--- a/include/linux/automake.mk
+++ b/include/linux/automake.mk
@@ -3,6 +3,7 @@ noinst_HEADERS += \
include/linux/netfilter/nf_conntrack_sctp.h \
include/linux/openvswitch.h \
include/linux/pkt_cls.h \
+   include/linux/psample.h \
include/linux/gen_stats.h \
include/linux/tc_act/tc_mpls.h \
include/linux/tc_act/tc_pedit.h \
diff --git a/include/linux/psample.h b/include/linux/psample.h
new file mode 100644
index 0..d5761b730
--- /dev/null
+++ b/include/linux/psample.h
@@ -0,0 +1,68 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef __LINUX_PSAMPLE_H
+#define __LINUX_PSAMPLE_H
+
+enum {
+   PSAMPLE_ATTR_IIFINDEX,
+   PSAMPLE_ATTR_OIFINDEX,
+   PSAMPLE_ATTR_ORIGSIZE,
+   PSAMPLE_ATTR_SAMPLE_GROUP,
+   PSAMPLE_ATTR_GROUP_SEQ,
+   PSAMPLE_ATTR_SAMPLE_RATE,
+   PSAMPLE_ATTR_DATA,
+   PSAMPLE_ATTR_GROUP_REFCOUNT,
+   PSAMPLE_ATTR_TUNNEL,
+
+   PSAMPLE_ATTR_PAD,
+   PSAMPLE_ATTR_OUT_TC,/* u16 */
+   PSAMPLE_ATTR_OUT_TC_OCC,/* u64, bytes */
+   PSAMPLE_ATTR_LATENCY,   /* u64, nanoseconds */
+   PSAMPLE_ATTR_TIMESTAMP, /* u64, nanoseconds */
+   PSAMPLE_ATTR_PROTO, /* u16 */
+   PSAMPLE_ATTR_USER_COOKIE,   /* binary, user provided data */
+   PSAMPLE_ATTR_SAMPLE_PROBABILITY,/* no argument, interpret rate in
+* PSAMPLE_ATTR_SAMPLE_RATE as a
+* probability scaled 0 - U32_MAX.
+*/
+
+   __PSAMPLE_ATTR_MAX
+};
+
+enum psample_command {
+   PSAMPLE_CMD_SAMPLE,
+   PSAMPLE_CMD_GET_GROUP,
+   PSAMPLE_CMD_NEW_GROUP,
+   PSAMPLE_CMD_DEL_GROUP,
+   PSAMPLE_CMD_SAMPLE_FILTER_SET,
+};
+
+enum psample_tunnel_key_attr {
+   PSAMPLE_TUNNEL_KEY_ATTR_ID, /* be64 Tunnel ID */
+   PSAMPLE_TUNNEL_KEY_ATTR_IPV4_SRC,   /* be32 src IP address. */
+   PSAMPLE_TUNNEL_KEY_ATTR_IPV4_DST,   /* be32 dst IP address. */
+   PSAMPLE_TUNNEL_KEY_ATTR_TOS,/* u8 Tunnel IP ToS. */
+   PSAMPLE_TUNNEL_KEY_ATTR_TTL,/* u8 Tunnel IP TTL. */
+   PSAMPLE_TUNNEL_KEY_ATTR_DONT_FRAGMENT,  /* No argument, set DF. */
+   PSAMPLE_TUNNEL_KEY_ATTR_CSUM,   /* No argument. CSUM 
packet. */
+   PSAMPLE_TUNNEL_KEY_ATTR_OAM,/* No argument. OAM frame.  
*/
+   PSAMPLE_TUNNEL_KEY_ATTR_GENEVE_OPTS,/* Array of Geneve options. 
*/
+   PSAMPLE_TUNNEL_KEY_ATTR_TP_SRC, /* be16 src Transport Port. 
*/
+   PSAMPLE_TUNNEL_KEY_ATTR_TP_DST, /* be16 dst Transport Port. 
*/
+   PSAMPLE_TUNNEL_KEY_ATTR_VXLAN_OPTS, /* Nested VXLAN opts* */
+   PSAMPLE_TUNNEL_KEY_ATTR_IPV6_SRC,   /* struct in6_addr src IPv6 
address. */
+   PSAMPLE_TUNNEL_KEY_ATTR_IPV6_DST,   /* struct in6_addr dst IPv6 
address. */
+   PSAMPLE_TUNNEL_KEY_ATTR_PAD,
+   PSAMPLE_TUNNEL_KEY_ATTR_ERSPAN_OPTS,/* struct erspan_metadata */
+   PSAMPLE_TUNNEL_KEY_ATTR_IPV4_INFO_BRIDGE,   /* No argument. 
IPV4_INFO_BRIDGE mode.*/
+   __PSAMPLE_TUNNEL_KEY_ATTR_MAX
+};
+
+/* Can be overridden at runtime by module option */
+#define PSAMPLE_ATTR_MAX (__PSAMPLE_ATTR_MAX - 1)
+
+#define PSAMPLE_NL_MCGRP_CONFIG_NAME "config"
+#define PSAMPLE_NL_MCGRP_SAMPLE_NAME "packets"
+#define PSAMPLE_GENL_NAME "psample"
+#define PSAMPLE_GENL_VERSION 1
+
+#endif
diff --git a/tests/automake.mk b/tests/automake.mk
index 04f48f2d8..edfc2cb33 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -499,7 +499,8 @@ endif
 if LINUX
 tests_ovstest_SOURCES += \
tests/test-netlink-conntrack.c \
-   tests/test-netlink-policy.c
+   tests/test-netlink-policy.c \
+   tests/test-psample.c
 endif
 
 tests_ovstest_LDADD = lib/libopenvswitch.la
diff --git a/tests/test-psample.c b/tests/test-psample.c
new file mode 100644
index 0..71b712190
--- /dev/null
+++ b/tests/test-psample.c
@@ -0,0 +1,288 @@
+/*
+ * Copyright (c) 2024 Red Hat, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, soft

[ovs-dev] [PATCH v2 07/13] ofproto-dpif-xlate-cache: Add lsample to xcache.

[Adrian Moreno]

Add a cache entry type for local sample objects.
Store both the dpif_lsample reference and the collector_set_id so we can
quickly find the particular exporter.

Using this mechanism, account for packet and byte statistics.

Signed-off-by: Adrian Moreno 
---
 ofproto/ofproto-dpif-lsample.c | 18 ++
 ofproto/ofproto-dpif-lsample.h |  4 
 ofproto/ofproto-dpif-xlate-cache.c | 11 ++-
 ofproto/ofproto-dpif-xlate-cache.h |  6 ++
 ofproto/ofproto-dpif-xlate.c   | 13 +
 ofproto/ofproto-dpif.c |  1 +
 6 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
index 833ce923f..f4bf49a7a 100644
--- a/ofproto/ofproto-dpif-lsample.c
+++ b/ofproto/ofproto-dpif-lsample.c
@@ -18,6 +18,7 @@
 #include "ofproto-dpif-lsample.h"
 
 #include "cmap.h"
+#include "dpif.h"
 #include "hash.h"
 #include "ofproto.h"
 #include "openvswitch/thread.h"
@@ -44,6 +45,8 @@ struct dpif_lsample {
 
 struct lsample_exporter {
 struct ofproto_lsample_options options;
+atomic_uint64_t n_packets;
+atomic_uint64_t n_bytes;
 };
 
 struct lsample_exporter_node {
@@ -156,6 +159,21 @@ dpif_lsample_get_group_id(struct dpif_lsample *ps, 
uint32_t collector_set_id,
 return found;
 }
 
+void
+dpif_lsample_credit_stats(struct dpif_lsample *lsample,
+  uint32_t collector_set_id,
+  const struct dpif_flow_stats *stats)
+{
+struct lsample_exporter_node *node;
+uint64_t orig;
+
+node = dpif_lsample_find_exporter_node(lsample, collector_set_id);
+if (node) {
+atomic_add_relaxed(&node->exporter.n_packets, stats->n_packets, &orig);
+atomic_add_relaxed(&node->exporter.n_bytes, stats->n_bytes, &orig);
+}
+}
+
 struct dpif_lsample *
 dpif_lsample_create(void)
 {
diff --git a/ofproto/ofproto-dpif-lsample.h b/ofproto/ofproto-dpif-lsample.h
index 26517a645..dbf7237bd 100644
--- a/ofproto/ofproto-dpif-lsample.h
+++ b/ofproto/ofproto-dpif-lsample.h
@@ -23,6 +23,7 @@
 
 struct dpif_lsample;
 struct ofproto_lsample_options;
+struct dpif_flow_stats;
 
 struct dpif_lsample *dpif_lsample_create(void);
 
@@ -37,4 +38,7 @@ bool dpif_lsample_get_group_id(struct dpif_lsample *,
uint32_t collector_set_id,
uint32_t *group_id);
 
+void dpif_lsample_credit_stats(struct dpif_lsample *,
+   uint32_t collector_set_id,
+   const struct dpif_flow_stats *);
 #endif /* OFPROTO_DPIF_LSAMPLE_H */
diff --git a/ofproto/ofproto-dpif-xlate-cache.c 
b/ofproto/ofproto-dpif-xlate-cache.c
index 2e1fcb3a6..73d79bfc7 100644
--- a/ofproto/ofproto-dpif-xlate-cache.c
+++ b/ofproto/ofproto-dpif-xlate-cache.c
@@ -35,9 +35,10 @@
 #include "learn.h"
 #include "mac-learning.h"
 #include "netdev-vport.h"
+#include "ofproto/ofproto-dpif.h"
+#include "ofproto/ofproto-dpif-lsample.h"
 #include "ofproto/ofproto-dpif-mirror.h"
 #include "ofproto/ofproto-dpif-xlate.h"
-#include "ofproto/ofproto-dpif.h"
 #include "ofproto/ofproto-provider.h"
 #include "openvswitch/dynamic-string.h"
 #include "openvswitch/vlog.h"
@@ -162,6 +163,11 @@ xlate_push_stats_entry(struct xc_entry *entry,
 }
 
 break;
+case XC_LSAMPLE:
+dpif_lsample_credit_stats(entry->lsample.lsample,
+  entry->lsample.collector_set_id,
+  stats);
+break;
 default:
 OVS_NOT_REACHED();
 }
@@ -245,6 +251,9 @@ xlate_cache_clear_entry(struct xc_entry *entry)
 break;
 case XC_TUNNEL_HEADER:
 break;
+case XC_LSAMPLE:
+dpif_lsample_unref(entry->lsample.lsample);
+break;
 default:
 OVS_NOT_REACHED();
 }
diff --git a/ofproto/ofproto-dpif-xlate-cache.h 
b/ofproto/ofproto-dpif-xlate-cache.h
index 0fc6d2ea6..df8115419 100644
--- a/ofproto/ofproto-dpif-xlate-cache.h
+++ b/ofproto/ofproto-dpif-xlate-cache.h
@@ -29,6 +29,7 @@
 struct bfd;
 struct bond;
 struct dpif_flow_stats;
+struct dpif_lsample;
 struct flow;
 struct group_dpif;
 struct mbridge;
@@ -53,6 +54,7 @@ enum xc_type {
 XC_GROUP,
 XC_TNL_NEIGH,
 XC_TUNNEL_HEADER,
+XC_LSAMPLE,
 };
 
 /* xlate_cache entries hold enough information to perform the side effects of
@@ -126,6 +128,10 @@ struct xc_entry {
 } operation;
 uint16_t hdr_size;
 } tunnel_hdr;
+struct {
+struct dpif_lsample *lsample;
+uint32_t collector_set_id;
+} lsample;
 };
 };
 
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 9f32982b0..0a0964348 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -6004,6 +6004,19 @@ xlate_sample_action(struct xlate_ctx *ctx,
 psample.cookie.lo = htonl(os->obs_point_id);
 
 compose_args.psample = &psample;
+
+if (ctx->xin->resubmit_stats

[ovs-dev] [PATCH v2 03/13] ofproto_dpif: Check for psample support.

[Adrian Moreno]

Only kernel datapath supports this action so add a function in dpif.c
that checks for that.

Signed-off-by: Adrian Moreno 
---
 lib/dpif.c |  7 +++
 lib/dpif.h |  1 +
 ofproto/ofproto-dpif.c | 46 ++
 ofproto/ofproto-dpif.h |  6 +-
 vswitchd/vswitch.xml   |  5 +
 5 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/lib/dpif.c b/lib/dpif.c
index 94db4630e..ab633fd27 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1953,6 +1953,13 @@ dpif_supports_lb_output_action(const struct dpif *dpif)
 return dpif_is_netdev(dpif);
 }
 
+bool
+dpif_may_support_psample(const struct dpif *dpif)
+{
+/* Userspace datapath does not support this action. */
+return !dpif_is_netdev(dpif);
+}
+
 /* Meters */
 void
 dpif_meter_get_features(const struct dpif *dpif,
diff --git a/lib/dpif.h b/lib/dpif.h
index a764e8a59..6bef7d5b3 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -941,6 +941,7 @@ int dpif_get_pmds_for_port(const struct dpif * dpif, 
odp_port_t port_no,
 char *dpif_get_dp_version(const struct dpif *);
 bool dpif_supports_tnl_push_pop(const struct dpif *);
 bool dpif_may_support_explicit_drop_action(const struct dpif *);
+bool dpif_may_support_psample(const struct dpif *);
 bool dpif_synced_dp_layers(struct dpif *);
 
 /* Log functions. */
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 1405d614e..41a5e989d 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -873,6 +873,12 @@ ovs_lb_output_action_supported(struct ofproto_dpif 
*ofproto)
 return ofproto->backer->rt_support.lb_output_action;
 }
 
+bool
+ovs_psample_supported(struct ofproto_dpif *ofproto)
+{
+return ofproto->backer->rt_support.psample;
+}
+
 /* Tests whether 'backer''s datapath supports recirculation.  Only newer
  * datapaths support OVS_KEY_ATTR_RECIRC_ID in keys.  We need to disable some
  * features on older datapaths that don't support this feature.
@@ -1609,6 +1615,44 @@ check_add_mpls(struct dpif_backer *backer)
 return supported;
 }
 
+/* Tests whether 'backer''s datapath supports the OVS_ACTION_ATTR_PSAMPLE
+ * action. */
+static bool
+check_psample(struct dpif_backer *backer)
+{
+uint8_t cookie[OVS_PSAMPLE_COOKIE_MAX_SIZE];
+struct odputil_keybuf keybuf;
+struct ofpbuf actions;
+struct ofpbuf key;
+bool supported;
+
+/* Intentionally bogus dl_type. */
+struct flow flow = {
+.dl_type = CONSTANT_HTONS(0x1234),
+};
+struct odp_flow_key_parms odp_parms = {
+.flow = &flow,
+.probe = true,
+};
+
+ofpbuf_use_stack(&key, &keybuf, sizeof keybuf);
+odp_flow_key_from_flow(&odp_parms, &key);
+ofpbuf_init(&actions, 32);
+
+/* Generate a random max-size cookie. */
+random_bytes(cookie, sizeof cookie);
+
+odp_put_psample_action(&actions, 10, cookie, sizeof cookie);
+
+supported = dpif_may_support_psample(backer->dpif) &&
+dpif_probe_feature(backer->dpif, "psample", &key, &actions, NULL);
+
+ofpbuf_uninit(&actions);
+VLOG_INFO("%s: Datapath %s psample action", dpif_name(backer->dpif),
+  supported ? "supports" : "does not support");
+return supported;
+}
+
 #define CHECK_FEATURE__(NAME, SUPPORT, FIELD, VALUE, ETHTYPE)   \
 static bool \
 check_##NAME(struct dpif_backer *backer)\
@@ -1698,6 +1742,7 @@ check_support(struct dpif_backer *backer)
 dpif_supports_lb_output_action(backer->dpif);
 backer->rt_support.ct_zero_snat = dpif_supports_ct_zero_snat(backer);
 backer->rt_support.add_mpls = check_add_mpls(backer);
+backer->rt_support.psample = check_psample(backer);
 
 /* Flow fields. */
 backer->rt_support.odp.ct_state = check_ct_state(backer);
@@ -5821,6 +5866,7 @@ get_datapath_cap(const char *datapath_type, struct smap 
*cap)
 smap_add(cap, "lb_output_action", s->lb_output_action ? "true" : "false");
 smap_add(cap, "ct_zero_snat", s->ct_zero_snat ? "true" : "false");
 smap_add(cap, "add_mpls", s->add_mpls ? "true" : "false");
+smap_add(cap, "psample", s->psample ? "true" : "false");
 
 /* The ct_tuple_flush is implemented on dpif level, so it is supported
  * for all backers. */
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index d33f73df8..ea18ccedf 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -213,7 +213,10 @@ struct group_dpif *group_dpif_lookup(struct ofproto_dpif *,
 DPIF_SUPPORT_FIELD(bool, ct_zero_snat, "Conntrack all-zero IP SNAT")\
 \
 /* True if the datapath supports add_mpls action. */\
-DPIF_SUPPORT_FIELD(bool, add_mpls, "MPLS Label add")
+DPIF_SUPPORT_FIELD(bool, add_mpls, "MPLS Label add")\
+  

[ovs-dev] [PATCH v2 05/13] vswitchd: Add local sampling to vswitchd schema.

[Adrian Moreno]

Add as new column in the Flow_Sample_Collector_Set table named
"local_group_id" which enables this feature.

Signed-off-by: Adrian Moreno 
---
 NEWS   |  4 ++
 vswitchd/bridge.c  | 78 +++---
 vswitchd/vswitch.ovsschema |  9 -
 vswitchd/vswitch.xml   | 41 ++--
 4 files changed, 121 insertions(+), 11 deletions(-)

diff --git a/NEWS b/NEWS
index e0359b759..5f5871ef0 100644
--- a/NEWS
+++ b/NEWS
@@ -16,6 +16,10 @@ Post-v3.3.0
per interface 'options:dpdk-lsc-interrupt' to 'false'.
- Python:
  * Added custom transaction support to the Idl via add_op().
+   - Local sampling is introduced.  It reuses the OpenFlow sample action and
+ allows samples to be emitted locally (instead of via IPFIX) in a
+ datapath-specific manner.  The Linux kernel datapath is the first to
+ support this feature by using the new datapath "psample" action.
 
 
 v3.3.0 - 16 Feb 2024
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 95a65fcdc..c5399d18c 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -288,6 +288,7 @@ static void bridge_configure_mac_table(struct bridge *);
 static void bridge_configure_mcast_snooping(struct bridge *);
 static void bridge_configure_sflow(struct bridge *, int *sflow_bridge_number);
 static void bridge_configure_ipfix(struct bridge *);
+static void bridge_configure_lsample(struct bridge *);
 static void bridge_configure_spanning_tree(struct bridge *);
 static void bridge_configure_tables(struct bridge *);
 static void bridge_configure_dp_desc(struct bridge *);
@@ -989,6 +990,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch 
*ovs_cfg)
 bridge_configure_netflow(br);
 bridge_configure_sflow(br, &sflow_bridge_number);
 bridge_configure_ipfix(br);
+bridge_configure_lsample(br);
 bridge_configure_spanning_tree(br);
 bridge_configure_tables(br);
 bridge_configure_dp_desc(br);
@@ -1537,10 +1539,11 @@ ovsrec_ipfix_is_valid(const struct ovsrec_ipfix *ipfix)
 return ipfix && ipfix->n_targets > 0;
 }
 
-/* Returns whether a Flow_Sample_Collector_Set row is valid. */
+/* Returns whether a Flow_Sample_Collector_Set row contains a valid IPFIX
+ * configuration. */
 static bool
-ovsrec_fscs_is_valid(const struct ovsrec_flow_sample_collector_set *fscs,
- const struct bridge *br)
+ovsrec_fscs_is_valid_ipfix(const struct ovsrec_flow_sample_collector_set *fscs,
+   const struct bridge *br)
 {
 return ovsrec_ipfix_is_valid(fscs->ipfix) && fscs->bridge == br->cfg;
 }
@@ -1558,7 +1561,7 @@ bridge_configure_ipfix(struct bridge *br)
 const char *virtual_obs_id;
 
 OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH(fe_cfg, idl) {
-if (ovsrec_fscs_is_valid(fe_cfg, br)) {
+if (ovsrec_fscs_is_valid_ipfix(fe_cfg, br)) {
 n_fe_opts++;
 }
 }
@@ -1621,7 +1624,7 @@ bridge_configure_ipfix(struct bridge *br)
 fe_opts = xcalloc(n_fe_opts, sizeof *fe_opts);
 opts = fe_opts;
 OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH(fe_cfg, idl) {
-if (ovsrec_fscs_is_valid(fe_cfg, br)) {
+if (ovsrec_fscs_is_valid_ipfix(fe_cfg, br)) {
 opts->collector_set_id = fe_cfg->id;
 sset_init(&opts->targets);
 sset_add_array(&opts->targets, fe_cfg->ipfix->targets,
@@ -1667,6 +1670,71 @@ bridge_configure_ipfix(struct bridge *br)
 }
 }
 
+/* Returns whether a Flow_Sample_Collector_Set row contains a valid local
+ * sampling configuration. */
+static bool
+ovsrec_fscs_is_valid_local(const struct ovsrec_flow_sample_collector_set *fscs,
+   const struct bridge *br)
+{
+return fscs->local_group_id && fscs->n_local_group_id == 1 &&
+   fscs->bridge == br->cfg;
+}
+
+/* Set local sample configuration on 'br'. */
+static void
+bridge_configure_lsample(struct bridge *br)
+{
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+const struct ovsrec_flow_sample_collector_set *fscs;
+struct ofproto_lsample_options *opts_array, *opts;
+size_t n_opts = 0;
+int ret;
+
+/* Iterate the Flow_Sample_Collector_Set table twice.
+ * First to get the number of valid configuration entries, then to process
+ * each of them and build an array of options. */
+OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH (fscs, idl) {
+if (ovsrec_fscs_is_valid_local(fscs, br)) {
+n_opts++;
+}
+}
+
+if (n_opts == 0) {
+ofproto_set_local_sample(br->ofproto, NULL, 0);
+return;
+}
+
+opts_array = xcalloc(n_opts, sizeof *opts_array);
+opts = opts_array;
+
+OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH (fscs, idl) {
+if (!ovsrec_fscs_is_valid_local(fscs, br)) {
+continue;
+}
+opts->collector_set_id = fscs->id;
+opts->group_id = *fscs->local_group_id;
+opts++

[ovs-dev] [PATCH v2 00/13] Introduce local sampling with NXAST_SAMPLE action.

[Adrian Moreno]

(Was: Add psample support to NXAST_SAMPLE action)

This is the userspace counterpart of the work done in the kernel
which has recently been merged in net-next [1]. There, a new
datapath action is added, called "psample".

>From the PoV of ovs-vswitchd, this new action is used to implement
"local sampling". Local sampling (or lsample for short) is configured
in a similar way as current per-flow IPFIX sampling, i.e: using the
Flow_Sample_Collector_Set table and the NXAST_SAMPLE action.

However, instead of sending the sample to an external IPFIX collector
though the network, the sample is emitted using the new action and
made available to locally running sample collector.

The specific way emit_sample sends the sample (and the way the local
collector shall collect it) is datapath-specific.
Currently, currently only the Linux kernel datapath implements it using
the psample netlink multicast group.

~~ Configuration ~~
Local sampling is configured via a new column in the
Flow_Sample_Collector_Set (FSCS) table called "local_group_id".
Configuring this value is orthogonal to also associating the FSCS
entry to an entry in the IPFIX table.

Once that entry in the OVSDB is configured, NXAST_SAMPLE actions coming
from the controller will be translated into the following odp action:

   sample(sample={P}%, actions(emit_sample(group={G},cookie={C})))

Where:
P: Is the sampling probability from NXAST_SAMPLE
G: Is the group id in the FSCS entry whose "id" matches the one in
the NXAST_SAMPLE.
C: Is a 64bit cookie result of concatenating the obs_domain and
obs_point from the NXAST_SAMPLE in network order, i.e:
"htonl(obs_domain) << 32 | htonl(obs_point)"
Notes:
- The parent sample action might be omitted if the probability is
  100% and there is no IPFIX sampling that requires the use of a
  meter.

~~ Dpif-lsample ~~
Internally, a new object called "dpif-lsample" is introduced to track
the configured local sampling exporters and track statistics based on
odp flow stats (using xcache).
It exposes the list of configured exporters and their statistics on a
new unixctl command called "lsample/show".

~~ Drop monitoring ~~
A common use-case for this action can be to sample drops. However,
adding sample actions to drops makes the existing drop statistics
disappear. In order to fix this, patch 11 make use of explicit
drop actions to ensure statistics still report drops even if sampled.

~~ Extended OpenFlow sample action ~~
Given the series aims at making sampling production ready, conntrack
integration must be considered. A common use-case for state-full
pipelines is to calculate the observation metadata at connection
establishment, store it in ct_label and then use it for packets of
established connections. However, this forces OVN to create a big number
of OFP Flows (one per distinct cookie). Patch 13 solves this by allowing
controllers to specify the obs_domain and point ids from another OFP
field.

~~ Testing ~~
The series includes an test utility program than can be executed by
running "tests/ovstest test-psample". This utility listens
to packets multicasted by the psample module and prints them (also
printing the obs_domain and obs_point ids).

~~ HW Offload ~~
tc offload is not being introduced in this series as existing sample
or userspace actions are not currently offloadable. Also some
improvements need to be implemented in tc for it to be feasible.

~~ DPDK datapath ~~
By naming the action "psample" it was intentionally restricted to the
Linux datapath only. A follow up task would be spawned to think of a
good way of implementing local-sampling in the userspace datapath.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=aae0b82b46cb5004bdf82a000c004d69a0885c33

---
* v1 -> v2
- Added a patch (12/13) that implements Eelco's suggestion of adding
  a global constant all-one "enum mf_subfield"
- Combined both drop-monitoring patches into one that is implemented by
  tweaking the odp actions list if it ends with a sample action.
- Added a test case that includes non-100% probability for which
  test-psample.c is tought to also print it.
- Support specifying smaller fields in obs_{point/domain}_id.
- Address other styling comments from Eelco and Ilya.

* rfc_v2 -> rfc_v3
- Added patches 11, 12 and 13 fixing drop monitoring and extending the
  OpenFlow action to support field-specifiers.


Adrian Moreno (13):
  ofproto-dpif: Allow forcing dp features.
  odp-util: Add support OVS_ACTION_ATTR_PSAMPLE.
  ofproto_dpif: Check for psample support.
  ofproto: Add ofproto-dpif-lsample.
  vswitchd: Add local sampling to vswitchd schema.
  ofproto-dpif-xlate: Use psample for local sample.
  ofproto-dpif-xlate-cache: Add lsample to xcache.
  ofproto-dpif-lsample: Show stats via unixctl.
  tests: Add test-psample testing utility.
  tests: Test local sampling.
  ofproto: xlate: Make sampled drops explicit.
  ofproto-dpif-xlate: Avoid allocating mf_subfield.
 

[ovs-dev] [PATCH v2 02/13] odp-util: Add support OVS_ACTION_ATTR_PSAMPLE.

[Adrian Moreno]

Add support for parsing and formatting the new action.

Also, flag OVS_ACTION_ATTR_SAMPLE as requiring datapath assistance if it
contains a nested OVS_ACTION_ATTR_PSAMPLE. The reason is that the
sampling rate from the parent "sample" is made available to the nested
"psample" by the kernel.

Signed-off-by: Adrian Moreno 
---
 include/linux/openvswitch.h  | 28 +++
 lib/dpif-netdev.c|  1 +
 lib/dpif.c   |  3 +-
 lib/odp-execute.c| 25 +-
 lib/odp-util.c   | 91 
 lib/odp-util.h   |  3 ++
 ofproto/ofproto-dpif-ipfix.c |  1 +
 ofproto/ofproto-dpif-sflow.c |  1 +
 python/ovs/flow/odp.py   |  8 
 tests/odp.at | 16 +++
 10 files changed, 175 insertions(+), 2 deletions(-)

diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
index d9fb991ef..ac4f67c6a 100644
--- a/include/linux/openvswitch.h
+++ b/include/linux/openvswitch.h
@@ -992,6 +992,31 @@ struct check_pkt_len_arg {
 };
 #endif
 
+#define OVS_PSAMPLE_COOKIE_MAX_SIZE 16
+/**
+ * enum ovs_pample_attr - Attributes for %OVS_ACTION_ATTR_PSAMPLE
+ * action.
+ *
+ * @OVS_PSAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
+ * sample.
+ * @OVS_PSAMPLE_ATTR_COOKIE: An optional variable-length binary cookie that
+ * contains user-defined metadata. The maximum length is
+ * OVS_PSAMPLE_COOKIE_MAX_SIZE bytes.
+ *
+ * Sends the packet to the psample multicast group with the specified group and
+ * cookie. It is possible to combine this action with the
+ * %OVS_ACTION_ATTR_TRUNC action to limit the size of the sample.
+ */
+enum ovs_psample_attr {
+OVS_PSAMPLE_ATTR_GROUP = 1,/* u32 number. */
+OVS_PSAMPLE_ATTR_COOKIE,   /* Optional, user specified cookie. */
+
+   /* private: */
+__OVS_PSAMPLE_ATTR_MAX
+};
+
+#define OVS_PSAMPLE_ATTR_MAX (__OVS_PSAMPLE_ATTR_MAX - 1)
+
 /**
  * enum ovs_action_attr - Action types.
  *
@@ -1056,6 +1081,8 @@ struct check_pkt_len_arg {
  * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
  * argument.
  * @OVS_ACTION_ATTR_DROP: Explicit drop action.
+ * @OVS_ACTION_ATTR_PSAMPLE: Send a sample of the packet to external observers
+ * via psample.
  */
 
 enum ovs_action_attr {
@@ -1087,6 +1114,7 @@ enum ovs_action_attr {
OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
OVS_ACTION_ATTR_DEC_TTL,  /* Nested OVS_DEC_TTL_ATTR_*. */
OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
+   OVS_ACTION_ATTR_PSAMPLE,  /* Nested OVS_PSAMPLE_ATTR_*. */
 
 #ifndef __KERNEL__
OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index c7f9e1490..f0594e5f5 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -9519,6 +9519,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch 
*packets_,
 case OVS_ACTION_ATTR_DROP:
 case OVS_ACTION_ATTR_ADD_MPLS:
 case OVS_ACTION_ATTR_DEC_TTL:
+case OVS_ACTION_ATTR_PSAMPLE:
 case __OVS_ACTION_ATTR_MAX:
 OVS_NOT_REACHED();
 }
diff --git a/lib/dpif.c b/lib/dpif.c
index 23eb18495..94db4630e 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1192,6 +1192,8 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch 
*packets_,
 case OVS_ACTION_ATTR_TUNNEL_PUSH:
 case OVS_ACTION_ATTR_TUNNEL_POP:
 case OVS_ACTION_ATTR_USERSPACE:
+case OVS_ACTION_ATTR_PSAMPLE:
+case OVS_ACTION_ATTR_SAMPLE:
 case OVS_ACTION_ATTR_RECIRC: {
 struct dpif_execute execute;
 struct ofpbuf execute_actions;
@@ -1278,7 +1280,6 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch 
*packets_,
 case OVS_ACTION_ATTR_POP_MPLS:
 case OVS_ACTION_ATTR_SET:
 case OVS_ACTION_ATTR_SET_MASKED:
-case OVS_ACTION_ATTR_SAMPLE:
 case OVS_ACTION_ATTR_TRUNC:
 case OVS_ACTION_ATTR_PUSH_ETH:
 case OVS_ACTION_ATTR_POP_ETH:
diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index 081e4d432..15577d539 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -818,13 +818,13 @@ requires_datapath_assistance(const struct nlattr *a)
 case OVS_ACTION_ATTR_RECIRC:
 case OVS_ACTION_ATTR_CT:
 case OVS_ACTION_ATTR_METER:
+case OVS_ACTION_ATTR_PSAMPLE:
 return true;
 
 case OVS_ACTION_ATTR_SET:
 case OVS_ACTION_ATTR_SET_MASKED:
 case OVS_ACTION_ATTR_PUSH_VLAN:
 case OVS_ACTION_ATTR_POP_VLAN:
-case OVS_ACTION_ATTR_SAMPLE:
 case OVS_ACTION_ATTR_HASH:
 case OVS_ACTION_ATTR_PUSH_MPLS:
 case OVS_ACTION_ATTR_POP_MPLS:
@@ -841,6 +841,28 @@ requires_datapath_assistance(const struct nlattr *a)
 case OVS_ACTION_ATTR_DROP:
 return false;
 
+case OVS_ACTION_ATTR_SAMPLE: {
+/* Nested "psample" actions rely on the datapath executing the
+ * parent "sample", storing the probability and making it available
+ * when the nested "psample" is run. */
+const struct n

[ovs-dev] [PATCH v2 01/13] ofproto-dpif: Allow forcing dp features.

[Adrian Moreno]

Datapath features can be set with dpif/set-dp-features unixctl command.
This command is not documented and therefore not supported in
production but only useful for unit tests.

A limitation was put in place originally to avoid enabling features at
runtime that were disabled at boot time to avoid breaking the datapath
in unexpected ways.

But, considering users should not use this command and it should only be
used for testing, we can assume whoever runs it knows what they are
doing. Therefore, the limitation should be bypass-able.

This patch adds a "--force" flag to the unixctl command to allow
bypassing the mentioned limitation.

Signed-off-by: Adrian Moreno 
---
 ofproto/ofproto-dpif.c | 32 
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index fcd7cd753..1405d614e 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -6432,7 +6432,8 @@ display_support_field(const char *name,
 static bool
 dpif_set_support(struct dpif_backer_support *rt_support,
  struct dpif_backer_support *bt_support,
- const char *name, const char *value, struct ds *ds)
+ const char *name, const char *value, bool force,
+ struct ds *ds)
 {
 struct shash all_fields = SHASH_INITIALIZER(&all_fields);
 struct dpif_support_field *field;
@@ -6484,8 +6485,13 @@ dpif_set_support(struct dpif_backer_support *rt_support,
 
 if (field->type == DPIF_SUPPORT_FIELD_bool) {
 if (!strcasecmp(value, "true")) {
-if (*(bool *)field->bt_ptr) {
-*(bool *)field->rt_ptr = true;
+if (*(bool *) field->bt_ptr || force) {
+if (force) {
+VLOG_WARN(
+"Enabling an unsupported feature is very dangerous"
+);
+}
+*(bool *) field->rt_ptr = true;
 changed = true;
 } else {
 ds_put_cstr(ds, "Can not enable features not supported by the 
datapth");
@@ -6720,10 +6726,19 @@ ofproto_unixctl_dpif_set_dp_features(struct 
unixctl_conn *conn,
  void *aux OVS_UNUSED)
 {
 struct ds ds = DS_EMPTY_INITIALIZER;
-const char *br = argv[1];
+struct ofproto_dpif *ofproto;
+bool changed, force = false;
 const char *name, *value;
-struct ofproto_dpif *ofproto = ofproto_dpif_lookup_by_name(br);
-bool changed;
+const char *br;
+
+if (argc > 2 && !strcmp(argv[1], "--force")) {
+force = true;
+argc--;
+argv++;
+}
+
+br = argv[1];
+ofproto = ofproto_dpif_lookup_by_name(br);
 
 if (!ofproto) {
 unixctl_command_reply_error(conn, "no such bridge");
@@ -6734,7 +6749,7 @@ ofproto_unixctl_dpif_set_dp_features(struct unixctl_conn 
*conn,
 value = argc > 3 ? argv[3] : NULL;
 changed = dpif_set_support(&ofproto->backer->rt_support,
&ofproto->backer->bt_support,
-   name, value, &ds);
+   name, value, force, &ds);
 if (changed) {
 xlate_set_support(ofproto, &ofproto->backer->rt_support);
 udpif_flush(ofproto->backer->udpif);
@@ -6777,7 +6792,8 @@ ofproto_unixctl_init(void)
 unixctl_command_register("dpif/dump-flows",
  "[-m] [--names | --no-names] bridge", 1, INT_MAX,
  ofproto_unixctl_dpif_dump_flows, NULL);
-unixctl_command_register("dpif/set-dp-features", "bridge", 1, 3 ,
+unixctl_command_register("dpif/set-dp-features",
+ "[--force] bridge [feature [value]]", 1, 4,
  ofproto_unixctl_dpif_set_dp_features, NULL);
 }
 
-- 
2.45.2

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

Re: [ovs-dev] [PATCH v4] ofproto: Add upcall/dump-ufid-rules command to map UFIDs to OpenFlow.

[Ilya Maximets]

On 7/11/24 13:29, Dumitru Ceara wrote:
> On 6/27/24 22:13, Ilya Maximets wrote:
>> On 6/18/24 21:41, Dumitru Ceara wrote:
>>> It improves the debugging experience if we can easily get a list of
>>> OpenFlow rules and groups that contribute to the creation of a datapath
>>> flow.
>>>
>>> The suggested workflow is:
>>> a. dump datapath flows (along with UUIDs), this also prints the core IDs
>>> (PMD IDs) when applicable.
>>>
>>>   $ ovs-appctl dpctl/dump-flows -m
>>>   flow-dump from pmd on cpu core: 7
>>>   ufid:7460db8f..., recirc_id(0), 
>>>
>>> b. dump related OpenFlow rules and groups:
>>>   $ ovs-appctl upcall/dump-ufid-rules ufid:7460db8f... pmd=7
>>>   cookie=0x12345678, table=0 
>>> priority=100,ip,in_port=2,nw_dst=10.0.0.2,actions=resubmit(,1)
>>>   cookie=0x0, table=1 priority=200,actions=group:1
>>>   group_id=1,bucket=bucket_id:0,actions=ct(commit,table=2,nat(dst=20.0.0.2))
>>>   cookie=0x0, table=2 actions=output:1
>>
>> Hi, Dumitru.  Thanks for the patch!
>>
>> This is definitely an interesting debug interface.  See some comments inline.
>>
> 
> Hi Ilya,
> 
> Thanks for the review!  I was trying to see if I can respin this before
> hard freeze but I'm a bit stuck with a couple of the comments below.
> Replied inline.
> 
>>
>> As an idea, should we call it ofproto/detrace ?
>>
> 
> Sure, I can rename it.
> 
>>>
>>> The new command only shows rules and groups attached to ukeys that are
>>> in states UKEY_VISIBLE or UKEY_OPERATIONAL.  That should be fine as all
>>> other ukeys should not be relevant for the use case presented above.
>>>
>>> For ukeys that don't have an xcache populated yet, the command goes
>>> ahead and populates one.  In theory this is creates a slight overhead as
>>> those ukeys might not need an xcache until they see traffic (and get
>>> revalidated) but in practice the overhead should be minimal.
>>
>> The flow dumped for the first time should always be revalidated,
>> even if it didn't see any more traffic.  See the should_revalidate()
>> function.  This means that there is only very narrow window between
>> the flow installation and the first revalidation, so the cache should
>> be availabel in all practical cases.
>>
>> Kepeing that in mind, I don't think that populating xcache from the
>> debug appctl is a good idea.  We're holding the mutex for a long time
>> while effectively revalidating the ukey from the appctl and more
>> importantly we're revalidating it with side effects allowed.  This
>> means we can learn some rules or update some other caches or even
>> emit some packets.  We should not do that from the debug appctl.
>>
>> I suggest to avoid this and just return early if the cache is not
>> yet available.  It should be available in most practical situations.
>>
> 
> Makes sense, it's probably fine.
> 
>>>
>>> This commit tries to mimic the output format of the ovs-ofctl
>>> dump-flows/dump-groups commands.  For groups it actually uses
>>> ofputil_group/_bucket functions for formatting.  For rules it uses
>>> flow_stats_ds() (the original function was exported and renamed to
>>> ofproto_rule_stats_ds()).
>>>
>>> Signed-off-by: Dumitru Ceara 
>>> ---
>>> V4:
>>> - Addressed Mike's comment:
>>>   - use predictable port IDs.
>>> V3:
>>> - Addressed Mike's comments:
>>>   - use ds_put_char()
>>>   - make the test less flaky
>>> V2:
>>> - Addressed Adrian's comments:
>>>   - check return value of populate_xcache()
>>>   - use flow_stats_ds() (renamed to ofproto_rule_stats_ds()) instead of
>>> custom printing
>>>   - move ukey->state check to caller
>>>   - handle case when group bucket is not available
>>>   - update test case to cover the point above
>>> ---
>>>  NEWS|   3 +
>>>  include/openvswitch/ofp-group.h |   7 ++
>>>  lib/ofp-group.c | 110 +++-
>>>  ofproto/ofproto-dpif-upcall.c   |  85 
>>>  ofproto/ofproto-dpif.c  |  24 +++
>>>  ofproto/ofproto-dpif.h  |   2 +
>>>  ofproto/ofproto-provider.h  |   1 +
>>>  ofproto/ofproto.c   |  85 
>>>  tests/ofproto-dpif.at   |  47 ++
>>>  tests/ofproto-macros.at |   9 ++-
>>>  10 files changed, 285 insertions(+), 88 deletions(-)
>>>
>>> diff --git a/NEWS b/NEWS
>>> index 5ae0108d552b..1bc085f97045 100644
>>> --- a/NEWS
>>> +++ b/NEWS
>>> @@ -9,6 +9,9 @@ Post-v3.3.0
>>>   https://github.com/openvswitch/ovs.git
>>> - DPDK:
>>>   * OVS validated with DPDK 23.11.1.
>>> +   - ovs-appctl:
>>> + * Added 'upcall/dump-ufid-rules' to output the set of OpenFlow rules 
>>> and
>>> +   groups that contributed to the creation of a specific datapath flow.
>>
>> I'd move this to the top, alpabetically.  I know that some entries
>> here are not in order, but it's better to keep them at least in
>> approximate order.
>>
> 
> Ack.
> 
>>>  
>>>  
>>>  v3.3.0 - 16 Feb 2024
>>> diff --git a/include/openvswitch/ofp-group.h 
>>> b/includ

Re: [ovs-dev] [PATCH ovn v2 3/8] northd: Assume all chassis support the "ovn-ct-lb-related" feature.

[Dumitru Ceara]

On 7/11/24 18:20, Dumitru Ceara wrote:
> This feature is supported in the last two LTS releases and the correct
> upgrade procedure mandates that we don't jump across LTS releases.  It's
> safe to remove the check in northd.
> 
> Signed-off-by: Dumitru Ceara 
> ---

For some reason ovs-vswitchd failed to start properly during one of the
system tests ran by ovsrobot:

https://github.com/ovsrobot/ovn/actions/runs/9895326654

2024-07-11T17:25:07Z|5|reconnect|INFO|unix:/workspace/ovn-tmp/tests/system-userspace-testsuite.dir/101/db.sock:
connected
2024-07-11T17:25:13Z|6|timeval|WARN|Unreasonably long 6175ms poll
interval (0ms user, 0ms system)
2024-07-11T17:25:13Z|7|timeval|WARN|faults: 23 minor, 0 major

It doesn't seem related to the patch to me, retesting.

Recheck-request: github-robot

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

Re: [ovs-dev] [PATCH v1 11/13] ofproto: xlate: Make flow-sampled drops explicit.

[Adrián Moreno]

[...]
> >> 2. This patch doesn't cover cases where sampling is not actually
> >>the last action, but further actions do not yield any datapath
> >>actions.  E.g. if you have a register resoration afterwards,
> >>which happens in automatically built pipelines like in OVN.
> >>Or if the last action after sampling is learn().  And things are
> >>getting more complicated if we take into account resubmits and
> >>processing in different bridges via patch ports.
> >
> > I agree this could be problematic. Maybe we should make sure the sample
> > is the last dp action and "fix it". A trick such as the one done for
> > sflow.
>
> These tricks are not accurate as well.  I beleive they do not track
> the information well across tunnel encaps at least.
>

I am currently testing a patch implements this for both per-bridge and
per-flow sampling. I attach the essense (i.e: not the tests) of it at
the end [1]. Can you expand on your concerns about tunnel encaps?

> >
> >>
> >> 3. If someone is sampling the drops specifically, they know where
> >>to find the packets, because they configured the collector.
> >>
> >
> > I think drop stats and samples are two different things. There are
> > typically extacted by different tools and systems.
> >
> > Besides, what about per-bridge sampling (next patch)?
> > The user enables sampling on a bridge without explicitly doing it
> > for drops and suddenly the drop statistics dissapear.
>
> I think 'user enables sampling on a bridge' counts as an explicit
> altering of the datapath pipeline.
>
> >
> >> 4. Packets are not actually dropped, they are delivered to userspace
> >>or psample.  It might make sense though to drop with a reson in
> >>case the upcall fails or psample fails to deliver to any entity
> >>and it is the last action in the datapath, but that's a kernel
> >>change to make.
> >>
> >
> > I don't want to get into another semantic discussion between consume,
> > free and drop or the dark corners of the OpenFlow protocol. For me it's
> > pretty clear that if the last action is to sample, the packet is
> > "dropped" in the sense that, from a switch' perspective, if it's not
> > forwarded/sent somewhere, it's dropped.
> >
> > I know you don't think the same :-).
> >
> > Would you then agree that the concept of dropping packets is very
> > unclear and OpenFlow does not make it easy (or even possible?) to
> > express a sampled drops and we should add an extension action to
> > explicitly drop packets?
>
> I agree that dropping packets is not clear in this case.
> I'm not a fan of OpenFlow drop action, it may cause extra logical issues.
> I'd go with a database knob instead.
>
> I'd say we can either drop these two patches for now and find a better
> solution in the next release cycle, or add an experimental global database
> knob that will control this behavior and will be disabled by default.
> Once we have better understanding how it behaves in a real world, we
> could switch the default or remove the feature if it causes issues or
> confusion.
>
> What do you think?
>

I guess this new know would be considered a new feature and therefore
not backportable to 3.4. So if this affects users (as is my suspicious),
we would have a non-fixable bug?

[1] Please consider this alternative to the two patches (it does not
include tests). FWIF, we could make this tweak configurable (default to
false) in the db.

---
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 0a0964348..da1ef0b49 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3415,6 +3415,7 @@ compose_sample_action(struct xlate_ctx *ctx,
 struct ofproto *ofproto = &ctx->xin->ofproto->up;
 uint32_t meter_id = ofproto->slowpath_meter_id;
 size_t cookie_offset = 0;
+size_t observe_offset;

 /* The meter action is only used to throttle userspace actions.
  * If they are not needed and the sampling rate is 100%, avoid generating
@@ -3432,6 +3433,7 @@ compose_sample_action(struct xlate_ctx *ctx,
 }

 if (args->psample) {
+observe_offset = ctx->odp_actions->size;
 odp_put_psample_action(ctx->odp_actions,
args->psample->group_id,
(void *) &args->psample->cookie,
@@ -3443,6 +3445,7 @@ compose_sample_action(struct xlate_ctx *ctx,
 nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_METER, meter_id);
 }

+observe_offset = ctx->odp_actions->size;
 odp_port_t odp_port = ofp_port_to_odp_port(
 ctx->xbridge, ctx->xin->flow.in_port.ofp_port);
 uint32_t pid = dpif_port_get_pid(ctx->xbridge->dpif, odp_port);
@@ -3457,6 +3460,9 @@ compose_sample_action(struct xlate_ctx *ctx,
 if (is_sample) {
 nl_msg_end_nested(ctx->odp_actions, actions_offset);
 nl_msg_end_nested(ctx->odp_actions, sample_offset);
+ctx->xout->last_observe_offset = sample_offset;
+} else

Re: [ovs-dev] [PATCH ovn v4 4/4] Controller, northd: Add support for CT zone limits.

[Dumitru Ceara]

On 6/26/24 07:56, Ales Musil wrote:
> Add support for limiting the CT zone usage per Ls, LR or LSP.
> When the limit is configured on logical switch it will also implicitly
> set limits for all ports in that logical switch. The port configuration
> can be overwritten individually and has priority over the whole logical
> switch configuration.
> 
> The value 0 means unlimited, when the value is not specified it is
> derived from OvS default CT limit specified for given OvS datapath.
> 
> Reported-at: https://bugzilla.redhat.com/2189924
> Signed-off-by: Ales Musil 
> ---
> v4: Rebase on top of latest main.
> Change naming of the ct_zone_limit_sync to avoid potential confusion as 
> suggested by Lorenzo.
> v3: Rebase on top of latest main.
> ---

Hi Ales,

Sorry for reviewing this so late in the cycle.  I think the patch looks
OK.  I mostly have some performance related concerns, it would be great
to get some scale testing numbers.  The other comment I had is minor.

>  NEWS|   3 +
>  controller/ct-zone.c| 175 
>  controller/ct-zone.h|  12 ++-
>  controller/ovn-controller.c |  25 +-
>  lib/ovn-util.c  |  17 
>  lib/ovn-util.h  |   3 +
>  northd/northd.c |   8 ++
>  ovn-nb.xml  |  29 ++
>  tests/ovn-controller.at |  99 
>  9 files changed, 349 insertions(+), 22 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index 01db77d57..f50d68a82 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -34,6 +34,9 @@ Post v24.03.0
>- NATs can now be given an arbitrary match condition and priority. This
>  allows for conditional NATs to be configured. See the ovn-nb(5) man
>  page for more information.
> +  - Add support for CT zone limit that can be specified per LR
> +(options:ct-zone-limit), LS (other_config:ct-zone-limit) or LSP
> +(options:ct-zone-limit).
>  
>  OVN v24.03.0 - 01 Mar 2024
>  --
> diff --git a/controller/ct-zone.c b/controller/ct-zone.c
> index 6ed1e4108..97f0de6c3 100644
> --- a/controller/ct-zone.c
> +++ b/controller/ct-zone.c
> @@ -34,6 +34,18 @@ static bool ct_zone_assign_unused(struct ct_zone_ctx *ctx,
>  static bool ct_zone_remove(struct ct_zone_ctx *ctx, const char *name);
>  static void ct_zone_add(struct ct_zone_ctx *ctx, const char *name,
>  uint16_t zone, bool set_pending);
> +static void
> +ct_zone_limits_update_per_dp(struct ct_zone_ctx *ctx,
> + const struct sbrec_datapath_binding *dp,
> + const char *name,
> + struct ovsdb_idl_index *pb_by_dp);
> +static void ct_zone_limit_update(struct ct_zone_ctx *ctx, const char *name,
> + int64_t limit);
> +static int64_t ct_zone_get_dp_limit(const struct sbrec_datapath_binding *dp);
> +static int64_t ct_zone_get_pb_limit(const struct sbrec_port_binding *pb);
> +static int64_t ct_zone_limit_normalize(int64_t limit);
> +static struct ovsrec_ct_zone *
> +ct_zone_find_ovsrec(const struct ovsrec_datapath *dp, uint16_t zone_id);
>  
>  void
>  ct_zones_restore(struct ct_zone_ctx *ctx,
> @@ -196,11 +208,14 @@ ct_zones_update(const struct sset *local_lports,
>  
>  void
>  ct_zones_commit(const struct ovsrec_bridge *br_int,
> +const struct ovsrec_datapath *ovs_dp,
> +struct ovsdb_idl_txn *ovs_idl_txn,
>  struct shash *pending_ct_zones)
>  {
>  struct shash_node *iter;
>  SHASH_FOR_EACH (iter, pending_ct_zones) {
>  struct ct_zone_pending_entry *ctzpe = iter->data;
> +struct ct_zone *ct_zone = &ctzpe->ct_zone;
>  
>  /* The transaction is open, so any pending entries in the
>   * CT_ZONE_DB_QUEUED must be sent and any in CT_ZONE_DB_QUEUED
> @@ -212,7 +227,7 @@ ct_zones_commit(const struct ovsrec_bridge *br_int,
>  
>  char *user_str = xasprintf("ct-zone-%s", iter->name);
>  if (ctzpe->add) {
> -char *zone_str = xasprintf("%"PRIu16, ctzpe->ct_zone.zone);
> +char *zone_str = xasprintf("%"PRIu16, ct_zone->zone);
>  struct smap_node *node =
>  smap_get_node(&br_int->external_ids, user_str);
>  if (!node || strcmp(node->value, zone_str)) {
> @@ -227,6 +242,19 @@ ct_zones_commit(const struct ovsrec_bridge *br_int,
>  }
>  free(user_str);
>  
> +struct ovsrec_ct_zone *ovs_zone =
> +ct_zone_find_ovsrec(ovs_dp, ct_zone->zone);

Does this create a measurable performance impact?  We're going to
potentially walk all already assigned CT zones every time a new port is
bound locally.  Should we cache the ovs_zone records and index them by
zone_id?  That probably also means that we might need to add incremental
processing support for ovsrec_ct_zone.

On the other hand, I tried a quick test of binding 500 ovs interfaces to
LSPs in a sandbox (o

Re: [ovs-dev] [PATCH ovn v4 3/4] controller: Prepare structure around CT zone limiting.

[Dumitru Ceara]

On 6/26/24 07:56, Ales Musil wrote:
> In order to be able to store CT limits for specified zone, store the
> zone inside separate struct instead of simap. This allows to add
> the addition of limit without changing the whole infrastructure again.
> 
> This is a preparation step for the CT zone limits.
> 
> Signed-off-by: Ales Musil 
> ---

Hi Ales,

> v4: Rebase on top of latest main.
> Address comments from Lorenzo.
> v3: Rebase on top of latest main.
> v2: Fix NULL ptr deref.
> ---
>  controller/ct-zone.c| 169 +---
>  controller/ct-zone.h|  13 ++-
>  controller/ofctrl.c |   2 +-
>  controller/ovn-controller.c |  15 ++--
>  controller/physical.c   |  17 ++--
>  controller/physical.h   |   2 +-
>  6 files changed, 126 insertions(+), 92 deletions(-)
> 
> diff --git a/controller/ct-zone.c b/controller/ct-zone.c
> index e4f66a52a..6ed1e4108 100644
> --- a/controller/ct-zone.c
> +++ b/controller/ct-zone.c
> @@ -26,12 +26,14 @@ ct_zone_restore(const struct sbrec_datapath_binding_table 
> *dp_table,
>  struct ct_zone_ctx *ctx, const char *name, int zone);
>  static void ct_zone_add_pending(struct shash *pending_ct_zones,
>  enum ct_zone_pending_state state,
> -int zone, bool add, const char *name);
> +struct ct_zone *zone, bool add,
> +const char *name);
>  static int ct_zone_get_snat(const struct sbrec_datapath_binding *dp);
>  static bool ct_zone_assign_unused(struct ct_zone_ctx *ctx,
>const char *zone_name, int *scan_start);
> -static bool ct_zone_remove(struct ct_zone_ctx *ctx,
> -   struct simap_node *ct_zone);
> +static bool ct_zone_remove(struct ct_zone_ctx *ctx, const char *name);
> +static void ct_zone_add(struct ct_zone_ctx *ctx, const char *name,
> +uint16_t zone, bool set_pending);
>  
>  void
>  ct_zones_restore(struct ct_zone_ctx *ctx,
> @@ -47,7 +49,8 @@ ct_zones_restore(struct ct_zone_ctx *ctx,
>  struct ct_zone_pending_entry *ctpe = pending_node->data;
>  
>  if (ctpe->add) {
> -ct_zone_restore(dp_table, ctx, pending_node->name, ctpe->zone);
> +ct_zone_restore(dp_table, ctx, pending_node->name,
> +ctpe->ct_zone.zone);
>  }
>  }
>  
> @@ -91,7 +94,6 @@ void
>  ct_zones_update(const struct sset *local_lports,
>  const struct hmap *local_datapaths, struct ct_zone_ctx *ctx)
>  {
> -struct simap_node *ct_zone;
>  int scan_start = 1;
>  const char *user;
>  struct sset all_users = SSET_INITIALIZER(&all_users);
> @@ -132,12 +134,14 @@ ct_zones_update(const struct sset *local_lports,
>  }
>  
>  /* Delete zones that do not exist in above sset. */
> -SIMAP_FOR_EACH_SAFE (ct_zone, &ctx->current) {
> -if (!sset_contains(&all_users, ct_zone->name)) {
> -ct_zone_remove(ctx, ct_zone);
> -} else if (!simap_find(&req_snat_zones, ct_zone->name)) {
> -bitmap_set1(unreq_snat_zones_map, ct_zone->data);
> -simap_put(&unreq_snat_zones, ct_zone->name, ct_zone->data);
> +struct shash_node *node;
> +SHASH_FOR_EACH_SAFE (node, &ctx->current) {
> +struct ct_zone *ct_zone = node->data;
> +if (!sset_contains(&all_users, node->name)) {
> +ct_zone_remove(ctx, node->name);
> +} else if (!simap_find(&req_snat_zones, node->name)) {
> +bitmap_set1(unreq_snat_zones_map, ct_zone->zone);
> +simap_put(&unreq_snat_zones, node->name, ct_zone->zone);
>  }
>  }
>  
> @@ -152,7 +156,7 @@ ct_zones_update(const struct sset *local_lports,
>  struct simap_node *unreq_node;
>  SIMAP_FOR_EACH_SAFE (unreq_node, &unreq_snat_zones) {
>  if (unreq_node->data == snat_req_node->data) {
> -simap_find_and_delete(&ctx->current, unreq_node->name);
> +ct_zone_remove(ctx, unreq_node->name);
>  simap_delete(&unreq_snat_zones, unreq_node);
>  }
>  }
> @@ -163,26 +167,12 @@ ct_zones_update(const struct sset *local_lports,
>  bitmap_set0(unreq_snat_zones_map, snat_req_node->data);
>  }
>  
> -struct simap_node *node = simap_find(&ctx->current,
> - snat_req_node->name);
> -if (node) {
> -if (node->data != snat_req_node->data) {
> -/* Zone request has changed for this node. delete old entry 
> and
> - * create new one*/
> -ct_zone_add_pending(&ctx->pending, CT_ZONE_OF_QUEUED,
> -snat_req_node->data, true,
> -snat_req_node->name);
> -bitmap_set0(ctx->bitmap, node->data);
>

Re: [ovs-dev] [PATCH v1 11/13] ofproto: xlate: Make flow-sampled drops explicit.

[Ilya Maximets]

On 7/11/24 00:19, Adrián Moreno wrote:
> On Wed, Jul 10, 2024 at 11:31:42PM GMT, Ilya Maximets wrote:
>> On 7/7/24 22:09, Adrian Moreno wrote:
>>> When an action set ends in a an OFP_SAMPLE action, it means the packet
>>> will be dropped and sampled. Make the drop explicit in this case so that
>>> drop statistics remain accurate.
>>>
>>> This could be done outside of the sample action, i.e: "sample(...),drop"
>>> but datapaths optimize sample actions that are found in the last
>>> position. So, taking into account that datapaths already report when the
>>> last sample probability fails, it is safe to put the drop inside the
>>> sample, i.e: "sample(...,drop)".
>>>
>>> Signed-off-by: Adrian Moreno 
>>> ---
>>>  ofproto/ofproto-dpif-xlate.c | 16 +++--
>>>  tests/drop-stats.at  | 65 
>>>  tests/ofproto-dpif.at| 44 
>>>  3 files changed, 123 insertions(+), 2 deletions(-)
>>
>> Hi, Adrian.  As we discussed before on the kernel list, I'm not sure
>> this is a right change to make for a fwe reasons:
>>
>> 1. We do not know the user's intentions.  We do not know if they
>>wanted to drop these packets or their goal was to sample them
>>and it just happened to be the last action in the list, because
>>they put it after the output action and not before.
>>
> 
> If there are datapath actions after output action you will probably get
> drops reported in both datapaths.

Not if it was a sampling action.

> 
>> 2. This patch doesn't cover cases where sampling is not actually
>>the last action, but further actions do not yield any datapath
>>actions.  E.g. if you have a register resoration afterwards,
>>which happens in automatically built pipelines like in OVN.
>>Or if the last action after sampling is learn().  And things are
>>getting more complicated if we take into account resubmits and
>>processing in different bridges via patch ports.
> 
> I agree this could be problematic. Maybe we should make sure the sample
> is the last dp action and "fix it". A trick such as the one done for
> sflow.

These tricks are not accurate as well.  I beleive they do not track
the information well across tunnel encaps at least.

> 
>>
>> 3. If someone is sampling the drops specifically, they know where
>>to find the packets, because they configured the collector.
>>
> 
> I think drop stats and samples are two different things. There are
> typically extacted by different tools and systems.
> 
> Besides, what about per-bridge sampling (next patch)?
> The user enables sampling on a bridge without explicitly doing it
> for drops and suddenly the drop statistics dissapear.

I think 'user enables sampling on a bridge' counts as an explicit
altering of the datapath pipeline.

> 
>> 4. Packets are not actually dropped, they are delivered to userspace
>>or psample.  It might make sense though to drop with a reson in
>>case the upcall fails or psample fails to deliver to any entity
>>and it is the last action in the datapath, but that's a kernel
>>change to make.
>>
> 
> I don't want to get into another semantic discussion between consume,
> free and drop or the dark corners of the OpenFlow protocol. For me it's
> pretty clear that if the last action is to sample, the packet is
> "dropped" in the sense that, from a switch' perspective, if it's not
> forwarded/sent somewhere, it's dropped.
> 
> I know you don't think the same :-).
> 
> Would you then agree that the concept of dropping packets is very
> unclear and OpenFlow does not make it easy (or even possible?) to
> express a sampled drops and we should add an extension action to
> explicitly drop packets?

I agree that dropping packets is not clear in this case.
I'm not a fan of OpenFlow drop action, it may cause extra logical issues.
I'd go with a database knob instead.

I'd say we can either drop these two patches for now and find a better
solution in the next release cycle, or add an experimental global database
knob that will control this behavior and will be disabled by default.
Once we have better understanding how it behaves in a real world, we
could switch the default or remove the feature if it causes issues or
confusion.

What do you think?

> 
>> 5. Drop reporting in either datapath implementation is not 100%
>>accurate anyway and requires users to know the kernel internals.
>>
> 
> Shouldn't we try to make them more accurate, not less?

We're not making them less accurate.

> 
>> Some of that also applies to the next patch in the set.
>>
> 
> I can take some of the critics for this patch but for me, the next one
> is bluntly fixing a bug: Per-bridge sflow/IPFIX should not break drop
> statistics.

It doesn't break the drop statistics if we see sampling as terminal
action.  It's not a drop from the datapath perspective.

> 
>> For the patch itself, you should also check that the action set is
>> actually empty (not the action list) after the sa

[ovs-dev] [PATCH] dpctl: Netdev leak causes stale VHU & reaching limit.

[Vipul Ashri via dev]

While running a test with a continous VM creation/deletion using an
orchestration script with-in cloud environment. In parallel we have
some monitoring script calling ovs-appctl dpctl/show stats commands
every minute.

During VHU port delete, one of netdev references were not reduced to
0 as show_dpif call has not given-up the reference back or doing bad
cleanup. This pending deference preventing VHU deletion sequence, this
is found to be one of corner case inside dpctl code which results in
leaking up netdev which ultimately results in stale VHU entry. After
fixing this problematic cleanup, issue is not seen.

Fixes: fceef2095222 ("dpctl: add ovs-appctl dpctl/* commands to talk to 
dpif-netdev")
Signed-off-by: Vipul Ashri 
---
 lib/dpctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/dpctl.c b/lib/dpctl.c
index a70df5342..81e9ca0e9 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -739,7 +739,6 @@ show_dpif(struct dpif *dpif, struct dpctl_params *dpctl_p)
 }
 error = netdev_get_stats(netdev, &s);
 if (!error) {
-netdev_close(netdev);
 print_stat(dpctl_p, "RX packets:", s.rx_packets);
 print_stat(dpctl_p, " errors:", s.rx_errors);
 print_stat(dpctl_p, " dropped:", s.rx_dropped);
@@ -770,6 +769,7 @@ show_dpif(struct dpif *dpif, struct dpctl_params *dpctl_p)
 dpctl_print(dpctl_p, ", could not retrieve stats (%s)",
 ovs_strerror(error));
 }
+netdev_close(netdev);
 }
 dpif_port_destroy(&dpif_port);
 }
-- 
2.34.1.windows.1

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

[ovs-dev] [PATCH] dpctl: Netdev leak causes stale VHU & reaching limit.

[Vipul Ashri via dev]

While running a test with a continous VM creation/deletion using an
orchestration script with-in cloud environment. In parallel we have
some monitoring script calling ovs-appctl dpctl/show stats commands
every minute.

During VHU port delete, one of netdev references were not reduced to
0 as show_dpif call has not given-up the reference back or doing bad
cleanup. This pending deference preventing VHU deletion sequence, this
is found to be one of corner case inside dpctl code which results in
leaking up netdev which ultimately results in stale VHU entry. After
fixing this problematic cleanup, issue is not seen.

Fixes: fceef2095222 ("dpctl: add ovs-appctl dpctl/* commands to talk to 
dpif-netdev")
Signed-off-by: Vipul Ashri 
---
 lib/dpctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/dpctl.c b/lib/dpctl.c
index a70df5342..81e9ca0e9 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -739,7 +739,6 @@ show_dpif(struct dpif *dpif, struct dpctl_params *dpctl_p)
 }
 error = netdev_get_stats(netdev, &s);
 if (!error) {
-netdev_close(netdev);
 print_stat(dpctl_p, "RX packets:", s.rx_packets);
 print_stat(dpctl_p, " errors:", s.rx_errors);
 print_stat(dpctl_p, " dropped:", s.rx_dropped);
@@ -770,6 +769,7 @@ show_dpif(struct dpif *dpif, struct dpctl_params *dpctl_p)
 dpctl_print(dpctl_p, ", could not retrieve stats (%s)",
 ovs_strerror(error));
 }
+netdev_close(netdev);
 }
 dpif_port_destroy(&dpif_port);
 }
-- 
2.34.1.windows.1

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

[ovs-dev] [PATCH ovn v2] northd: Fix logical router load-balancer nat rules when using DGP.

[Roberto Bartzen Acosta via dev]

This commit fixes the build_distr_lrouter_nat_flows_for_lb function to
include one NAT stateless flow entry for each DGP in use. Since we have
added support to create multiple gateway ports per logical router, it's
necessary to include in the LR nat rules pipeline a specific entry for each
attached DGP. Otherwise, the ingress traffic is only redirected when the
incoming LRP matches the chassis_resident field.

Considering that DNAT rules for DGPs were implemented with the need to
configure the DGP-related gateway-port column, the load-balancer NAT rule
configuration can use a similar idea. In this case, we don't know the LRP
responsible for the incoming traffic, and therefore we need to automatically
apply a stateless NAT rule to the load-balancer on all DGPs to allow inbound
traffic.

After applying this patch, the incoming and/or outgoing traffic can pass
through any chassis where the DGP resides without having problems with CT
state.

Fixes: 15348b7b806f ("ovn-northd: Multiple distributed gateway port support.")
Signed-off-by: Roberto Bartzen Acosta 
---
 northd/en-lr-stateful.c |  12 -
 northd/northd.c |  67 ++--
 tests/ovn-northd.at | 111 
 3 files changed, 163 insertions(+), 27 deletions(-)

diff --git a/northd/en-lr-stateful.c b/northd/en-lr-stateful.c
index baf1bd2f8..f09691af6 100644
--- a/northd/en-lr-stateful.c
+++ b/northd/en-lr-stateful.c
@@ -516,18 +516,6 @@ lr_stateful_record_create(struct lr_stateful_table *table,
 
 table->array[od->index] = lr_stateful_rec;
 
-/* Load balancers are not supported (yet) if a logical router has multiple
- * distributed gateway port.  Log a warning. */
-if (lr_stateful_rec->has_lb_vip && lr_has_multiple_gw_ports(od)) {
-static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
-VLOG_WARN_RL(&rl, "Load-balancers are configured on logical "
- "router %s, which has %"PRIuSIZE" distributed "
- "gateway ports. Load-balancer is not supported "
- "yet when there is more than one distributed "
- "gateway port on the router.",
- od->nbr->name, od->n_l3dgw_ports);
-}
-
 return lr_stateful_rec;
 }
 
diff --git a/northd/northd.c b/northd/northd.c
index 6898daa00..9d22698c9 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -11026,24 +11026,25 @@ static void
 build_distr_lrouter_nat_flows_for_lb(struct lrouter_nat_lb_flows_ctx *ctx,
  enum lrouter_nat_lb_flow_type type,
  struct ovn_datapath *od,
- struct lflow_ref *lflow_ref)
+ struct lflow_ref *lflow_ref,
+ struct ovn_port *dgp)
 {
-struct ovn_port *dgp = od->l3dgw_ports[0];
-
-const char *undnat_action;
+struct ds undnat_action = DS_EMPTY_INITIALIZER;
+struct ds dnat_action = DS_EMPTY_INITIALIZER;
+struct ds snat_action = DS_EMPTY_INITIALIZER;
 
 switch (type) {
 case LROUTER_NAT_LB_FLOW_FORCE_SNAT:
-undnat_action = "flags.force_snat_for_lb = 1; next;";
+ds_put_format(&undnat_action, "flags.force_snat_for_lb = 1; next;");
 break;
 case LROUTER_NAT_LB_FLOW_SKIP_SNAT:
-undnat_action = "flags.skip_snat_for_lb = 1; next;";
+ds_put_format(&undnat_action, "flags.skip_snat_for_lb = 1; next;");
 break;
 case LROUTER_NAT_LB_FLOW_NORMAL:
 case LROUTER_NAT_LB_FLOW_MAX:
-undnat_action = lrouter_use_common_zone(od)
-? "ct_dnat_in_czone;"
-: "ct_dnat;";
+ds_put_format(&undnat_action, "%s",
+  lrouter_use_common_zone(od) ? "ct_dnat_in_czone;"
+  : "ct_dnat;");
 break;
 }
 
@@ -11051,6 +11052,31 @@ build_distr_lrouter_nat_flows_for_lb(struct 
lrouter_nat_lb_flows_ctx *ctx,
 size_t new_match_len = ctx->new_match->length;
 size_t undnat_match_len = ctx->undnat_match->length;
 
+/* Change the logic to create LB NAT rules when we are using DGPs.
+ * 1. Remove the ct action from the lr_out_undenat NAT rule.
+ * 2. Add the LB backend IPs as a destination action of the lr_in_dnat
+ *NAT rule with cumulative effect because any backend dst IP used in
+ *the action list will redirect the packet to the ct_lb pipeline.
+ * 3. Add a new lr_out_snat NAT rule with the LB VIP as source IP
+ *action to perform the NAT stateless pipeline completely.
+ */
+if (od->n_l3dgw_ports > 1) {
+ds_clear(&undnat_action);
+ds_put_format(&undnat_action, "next;");
+
+for (size_t i = 0; i < ctx->lb_vip->n_backends; i++) {
+struct ovn_lb_backend *backend = &ctx->lb_vip->backends[i];
+bool ipv6 = !IN6_IS_ADDR_V4MAPPED(&backend->ip);
+ds_put_format(&dnat_actio

Re: [ovs-dev] [PATCH ovn v2 8/8] northd: Add ACL Sampling.

[0-day Robot]

Bleep bloop.  Greetings Dumitru Ceara, 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 is 96 characters long (recommended limit is 79)
#163 FILE: northd/northd.c:215:
 * ++--+ X |   
LB_L2_AFF_BACKEND_IP6   |

WARNING: Line is 96 characters long (recommended limit is 79)
#164 FILE: northd/northd.c:216:
 * | R1 | ORIG_DIP_IPV4 (>= IN_PRE_STATEFUL)   | R |(>= 
IN_LB_AFF_CHECK && |

WARNING: Line is 96 characters long (recommended limit is 79)
#165 FILE: northd/northd.c:217:
 * ++--+ E | <= 
IN_LB_AFF_LEARN)   |

WARNING: Line is 96 characters long (recommended limit is 79)
#169 FILE: northd/northd.c:220:
 * | R3 | OBS_POINT_ID_NEW |   |
   |

WARNING: Line is 96 characters long (recommended limit is 79)
#170 FILE: northd/northd.c:221:
 * ||   (>= ACL_EVAL* && <= ACL_ACTION*)   |   |
   |

WARNING: Comment with 'xxx' marker
#563 FILE: northd/northd.c:6840:
/* XXX: ACLs with action "pass" do not support sampling. */

WARNING: Line is 560 characters long (recommended limit is 79)
#1740 FILE: utilities/ovn-nbctl.8.xml:402:
[--type={switch | 
port-group}] [--log] 
[--meter=meter] 
[--severity=severity] 
[--name=name] [--label=label] 
[--sample-new=sample] 
[--sample-est=sample] [--may-exist] 
[--apply-after-lb] [--tier] acl-add 
entity direction priority match 
verdict

Lines checked: 1842, Warnings: 7, 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

Re: [ovs-dev] [PATCH v2 ovn] nbctl: Added local link ipv6 to nbctl show.

[Numan Siddique]

On Fri, Jul 5, 2024 at 12:41 PM Mj Ponsonby  wrote:
>
> From: MJ Ponsonby 
>
>
> This commit modifies a test to deal with the changed,
> response from the command
> This commit also modifies the function of ovn-nbctl show,
> to also return the ipv6 link local address as mentioned
>
> This information would be useful to have with the wider useage of
> advertising IPv4 Prefixes over IPv6 next hops [0]
>
> 0: https://datatracker.ietf.org/doc/html/rfc5549
>
> Reported-at: https://launchpad.net/bugs/2069804
> Signed-off-by: MJ Ponsonby 

Thanks.  Applied to main.

Numan

> ---
>  tests/ovn-nbctl.at|  1 +
>  utilities/ovn-nbctl.c | 10 ++
>  2 files changed, 11 insertions(+)
>
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index 19c83a4a5..de014e1f9 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -1670,6 +1670,7 @@ AT_CHECK([ovn-nbctl show lr0 | uuidfilt], [0], [dnl
>  router <0> (lr0)
>  port lrp0
>  mac: "00:00:00:01:02:03"
> +ipv6-lla: "fe80::200:ff:fe01:203"
>  networks: [["192.168.1.1/24"]]
>  ])
>
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 32ca4f750..04c123022 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -780,6 +780,16 @@ print_lr(const struct nbrec_logical_router *lr, struct 
> ds *s)
>  if (lrp->mac) {
>  ds_put_cstr(s, "mac: ");
>  ds_put_format(s, "\"%s\"\n", lrp->mac);
> +
> +/* Have the mac address in an array. */
> +struct eth_addr ea;
> +eth_addr_from_string(lrp->mac, &ea);
> +struct in6_addr lla;
> +in6_generate_lla(ea, &lla);
> +
> +ds_put_cstr(s, "ipv6-lla: \"");
> +ipv6_format_addr(&lla, s);
> +ds_put_cstr(s, "\"\n");
>  }
>  if (lrp->n_networks) {
>  ds_put_cstr(s, "networks: [");
> --
> 2.43.0
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v2 8/8] northd: Add ACL Sampling.

[Dumitru Ceara]

On 7/11/24 18:41, Ilya Maximets wrote:
> On 7/11/24 18:21, Dumitru Ceara wrote:
>> index e131a4c083..977d041d84 100644
>> --- a/ovn-nb.ovsschema
>> +++ b/ovn-nb.ovsschema
>> @@ -1,7 +1,7 @@
>>  {
>>  "name": "OVN_Northbound",
>>  "version": "7.4.0",
>> -"cksum": "1498303893 36355",
>> +"cksum": "83748782 38439",
> 
> Not a full review, but a note that you need to advance the schema
> version on changes, not just fix the checksum.
> 

I was convinced I did, but I think I messed it up during rebases.  I'll
fix it in v3, thanks for spotting this!

Regards,
Dumitru

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

Re: [ovs-dev] [PATCH ovn v2 8/8] northd: Add ACL Sampling.

[Ilya Maximets]

On 7/11/24 18:21, Dumitru Ceara wrote:
> index e131a4c083..977d041d84 100644
> --- a/ovn-nb.ovsschema
> +++ b/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
>  {
>  "name": "OVN_Northbound",
>  "version": "7.4.0",
> -"cksum": "1498303893 36355",
> +"cksum": "83748782 38439",

Not a full review, but a note that you need to advance the schema
version on changes, not just fix the checksum.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v2 8/8] northd: Add ACL Sampling.

[Dumitru Ceara]

From: Adrian Moreno 

Introduce a new table called Sample where per-flow IPFIX configuration
can be specified.
Also, reference rows from such table from the ACL table to enable the
configuration of ACL sampling. If enabled, northd will add a sample
action to each ACL related logical flow.

Packets that hit stateful ACLs are sampled in different ways depending
whether they are initiating a new session or are just forwarded on an
existing (already allowed) session.  Two new columns ("sample_new" and
"sample_est") are added to the ACL table to allow for potentially
different sampling rates for the two cases.

Note: If an ACL has both sampling enabled and a label associated to it
then the label value overrides the observation point ID defined in the
sample configuration.  This is a side effect of the implementation
(observation point IDs are stored in conntrack in the same part of the
ct_label where ACL labels are also stored).  The two features
(sampling and ACL labels) serve however similar purposes so it's not
expected that they're both enabled together.

When sampling is enabled on an ACL additional logical flows are created
for that ACL (one for stateless ACLs and 3 for stateful ACLs) in the ACL
action stage of the logical pipeline.  These additional flows match on a
combination of conntrack state values and observation point id values
(either against a logical register or against the stored ct_label state)
in order to determine whether the packets hitting the ACLs must be
sampled or not.  This comes with a slight increase in the number of
logical flows and in the number of OpenFlow rules.  The number of
additional flows _does not_ depend on the original ACL match or action.

New --sample-new and --sample-est optional arguments are added to the
'ovn-nbctl acl-add' command to allow configuring these new types of
sampling for ACLs.

An example workflow of configuring ACL samples is:
  # Create Sampling_App mappings for ACL traffic types:
  ovn-nbctl create Sampling_App name="acl-new-traffic-sampling" \
id="42"
  ovn-nbctl create sampling_app name="acl-est-traffic-sampling" \
id="43"
  # Create two sample collectors, one that samples all packets (c1)
  # and another one that samples with a probability of 10% (c2):
  c1=$(ovn-nbctl create Sample_Collector name=c1 \
   probability=65535 set_id=1)
  c2=$(ovn-nbctl create Sample_Collector name=c2 \
   probability=6553 set_id=2)
  # Create two sample configurations (for new and for established
  # traffic):
  s1=$(ovn-nbctl create sample collector="$c1 $c2" metadata=4301)
  s2=$(ovn-nbctl create sample collector="$c1 $c2" metadata=4302)
  # Create an ingress ACL to allow IP traffic:
  ovn-nbctl --sample-new=$s1 --sample-est=$s2 acl-add ls \
from-lport 1 "ip" allow-related

The config above will generate IPFIX samples with:
- 8 MSB of observation domain id set to 42 (Sampling_App
  "acl-new-traffic-sampling" config) and observation point id
  set to 4301 (Sample s1) for packets that create a new
  connection
- 8 MSB of observation domain id set to 43 (Sampling_app
  "acl-est-traffic-sampling" config) and observation point id
  set to 4302 (Sample s2) for packets that are part of an already
  existing connection

Note: in general, all generated IPFIX sample observation domain IDs are
built by ovn-controller in the following way:
The 8 MSB taken from the sample action's obs_domain_id and the last 24
LSB taken from the Southbound logical datapath tunnel_key (datapath ID).

Reported-at: https://issues.redhat.com/browse/FDP-305
Signed-off-by: Adrian Moreno 
Co-authored-by: Dumitru Ceara 
Signed-off-by: Dumitru Ceara 
---
V2:
- Addressed Adrian's comments:
  - fixed up observation domain id comment in commit log.
  - store the obs_domain_id in the ct_label as an 8 bit value (add a
test).
  - removed redundant check in build_acl_sample_label_action().
  - added missing space after ternary ":" operator.
  - documented limitation for sampling ACLs with action "pass".
  - documented sample_new behavior for stateless ACLs.
- Removed unused OVN_CT_SAMPLE_ID_SET_BIT and OVN_CT_SAMPLE_ID_SET.
---
 NEWS   |   3 +
 lib/logical-fields.c   |   6 +
 northd/northd.c| 519 +++--
 northd/ovn-northd.8.xml|  26 ++
 ovn-nb.ovsschema   |  44 ++-
 ovn-nb.xml |  63 +++
 tests/atlocal.in   |   6 +
 tests/ovn-macros.at|   4 +
 tests/ovn-nbctl.at |  20 +
 tests/ovn-northd.at| 240 ++--
 tests/ovn.at   |   9 +
 tests/system-common-macros.at  |  11 +
 tests/system-ovn.at| 149 +++
 utilities/containers/fedora/Dockerfile |   1 +
 utilities/containers/ubuntu/Dockerfile |   1 +
 utilities/ovn-nbctl.8.xml  |   

[ovs-dev] [PATCH ovn v2 6/8] northd: Add Sampling_App table.

[Dumitru Ceara]

This will represent a unified place to store IPFIX observation domain ID
configurations for sampling applications (currently only drop sampling
is supported as application but following commits will add more).

Signed-off-by: Dumitru Ceara 
---
 northd/automake.mk   |   2 +
 northd/en-lflow.c|   5 ++
 northd/en-sampling-app.c | 120 +++
 northd/en-sampling-app.h |  51 +
 northd/inc-proc-northd.c |  10 +++-
 northd/northd.h  |   1 +
 ovn-nb.ovsschema |  21 ++-
 ovn-nb.xml   |  17 ++
 tests/ovn-northd.at  |  17 ++
 9 files changed, 241 insertions(+), 3 deletions(-)
 create mode 100644 northd/en-sampling-app.c
 create mode 100644 northd/en-sampling-app.h

diff --git a/northd/automake.mk b/northd/automake.mk
index d491973a8b..6566ad2999 100644
--- a/northd/automake.mk
+++ b/northd/automake.mk
@@ -32,6 +32,8 @@ northd_ovn_northd_SOURCES = \
northd/en-lr-stateful.h \
northd/en-ls-stateful.c \
northd/en-ls-stateful.h \
+   northd/en-sampling-app.c \
+   northd/en-sampling-app.h \
northd/inc-proc-northd.c \
northd/inc-proc-northd.h \
northd/ipam.c \
diff --git a/northd/en-lflow.c b/northd/en-lflow.c
index c4b927fb8c..eb91f2a651 100644
--- a/northd/en-lflow.c
+++ b/northd/en-lflow.c
@@ -25,6 +25,7 @@
 #include "en-ls-stateful.h"
 #include "en-northd.h"
 #include "en-meters.h"
+#include "en-sampling-app.h"
 #include "lflow-mgr.h"
 
 #include "lib/inc-proc-eng.h"
@@ -86,6 +87,10 @@ lflow_get_input_data(struct engine_node *node,
 lflow_input->ovn_internal_version_changed =
 global_config->ovn_internal_version_changed;
 lflow_input->svc_monitor_mac = global_config->svc_monitor_mac;
+
+struct ed_type_sampling_app_data *sampling_app_data =
+engine_get_input_data("sampling_app", node);
+lflow_input->sampling_apps = &sampling_app_data->apps;
 }
 
 void en_lflow_run(struct engine_node *node, void *data)
diff --git a/northd/en-sampling-app.c b/northd/en-sampling-app.c
new file mode 100644
index 00..8d2d45a90f
--- /dev/null
+++ b/northd/en-sampling-app.c
@@ -0,0 +1,120 @@
+/*
+ * Copyright (c) 2024, Red Hat, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include 
+
+#include "openvswitch/vlog.h"
+
+#include "en-sampling-app.h"
+
+VLOG_DEFINE_THIS_MODULE(en_sampling_app);
+
+/* Static function declarations. */
+static void sampling_app_table_add(struct sampling_app_table *,
+   const struct nbrec_sampling_app *);
+static uint8_t sampling_app_table_get_id(const struct sampling_app_table *,
+ enum sampling_app_type);
+static void sampling_app_table_reset(struct sampling_app_table *);
+static enum sampling_app_type sampling_app_get_by_name(const char *app_name);
+
+void *
+en_sampling_app_init(struct engine_node *node OVS_UNUSED,
+ struct engine_arg *arg OVS_UNUSED)
+{
+struct ed_type_sampling_app_data *data = xzalloc(sizeof *data);
+sampling_app_table_reset(&data->apps);
+return data;
+}
+
+void
+en_sampling_app_cleanup(void *data OVS_UNUSED)
+{
+}
+
+void
+en_sampling_app_run(struct engine_node *node, void *data_)
+{
+const struct nbrec_sampling_app_table *nb_sampling_app_table =
+EN_OVSDB_GET(engine_get_input("NB_sampling_app", node));
+struct ed_type_sampling_app_data *data = data_;
+
+sampling_app_table_reset(&data->apps);
+
+const struct nbrec_sampling_app *sa;
+NBREC_SAMPLING_APP_TABLE_FOR_EACH (sa, nb_sampling_app_table) {
+sampling_app_table_add(&data->apps, sa);
+}
+
+engine_set_node_state(node, EN_UPDATED);
+}
+
+uint8_t
+sampling_app_get_id(const struct sampling_app_table *app_table,
+enum sampling_app_type app_type)
+{
+return sampling_app_table_get_id(app_table, app_type);
+}
+
+/* Static functions. */
+static
+void sampling_app_table_add(struct sampling_app_table *table,
+const struct nbrec_sampling_app *sa)
+{
+enum sampling_app_type app_type = sampling_app_get_by_name(sa->name);
+
+if (app_type == SAMPLING_APP_MAX) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+VLOG_WARN_RL(&rl, "Unexpected Sampling_App name: %s", sa->name);
+return;
+}
+table->app_ids[app_type] = sa->id;
+}
+
+static
+uint8_t sampling_app_table_get_id(const struct

[ovs-dev] [PATCH ovn v2 7/8] northd: Override NB_Global drop sampling id with Sampling_App config.

[Dumitru Ceara]

Signed-off-by: Dumitru Ceara 
---
 NEWS  |  3 +++
 northd/debug.c| 12 +++-
 northd/debug.h|  3 ++-
 northd/en-global-config.c | 31 +++
 northd/inc-proc-northd.c  |  1 +
 tests/ovn-northd.at   | 27 ++-
 6 files changed, 62 insertions(+), 15 deletions(-)

diff --git a/NEWS b/NEWS
index 3e392ff08b..fcf182bc02 100644
--- a/NEWS
+++ b/NEWS
@@ -38,6 +38,9 @@ Post v24.03.0
 ability to disable "VXLAN mode" to extend available tunnel IDs space for
 datapaths from 4095 to 16711680.  For more details see man ovn-nb(5) for
 mentioned option.
+  - The NB_Global.debug_drop_domain_id configured value is now overridden by
+the ID associated with the Sampling_App record created for drop sampling
+(Sampling_App.name configured to be "drop-sampling").
 
 OVN v24.03.0 - 01 Mar 2024
 --
diff --git a/northd/debug.c b/northd/debug.c
index 59da5d4f66..457993b7cf 100644
--- a/northd/debug.c
+++ b/northd/debug.c
@@ -3,6 +3,7 @@
 #include 
 
 #include "debug.h"
+#include "en-sampling-app.h"
 
 #include "openvswitch/dynamic-string.h"
 #include "openvswitch/vlog.h"
@@ -26,16 +27,17 @@ debug_enabled(void)
 }
 
 void
-init_debug_config(const struct nbrec_nb_global *nb)
+init_debug_config(const struct nbrec_nb_global *nb,
+  uint8_t drop_domain_id_override)
 {
-
 const struct smap *options = &nb->options;
 uint32_t collector_set_id = smap_get_uint(options,
   "debug_drop_collector_set",
   0);
-uint32_t observation_domain_id = smap_get_uint(options,
-   "debug_drop_domain_id",
-   0);
+uint32_t observation_domain_id =
+drop_domain_id_override != SAMPLING_APP_ID_NONE
+? drop_domain_id_override
+: smap_get_uint(options, "debug_drop_domain_id", 0);
 
 if (collector_set_id != config.collector_set_id ||
 observation_domain_id != config.observation_domain_id ||
diff --git a/northd/debug.h b/northd/debug.h
index c1a5e5aadb..a0b535253a 100644
--- a/northd/debug.h
+++ b/northd/debug.h
@@ -21,7 +21,8 @@
 #include "lib/ovn-nb-idl.h"
 #include "openvswitch/dynamic-string.h"
 
-void init_debug_config(const struct nbrec_nb_global *nb);
+void init_debug_config(const struct nbrec_nb_global *nb,
+   uint8_t drop_domain_id_override);
 void destroy_debug_config(void);
 
 const char *debug_drop_action(void);
diff --git a/northd/en-global-config.c b/northd/en-global-config.c
index 5b71ede1f2..c683c8fae5 100644
--- a/northd/en-global-config.c
+++ b/northd/en-global-config.c
@@ -24,6 +24,7 @@
 /* OVN includes */
 #include "debug.h"
 #include "en-global-config.h"
+#include "en-sampling-app.h"
 #include "include/ovn/features.h"
 #include "ipam.h"
 #include "lib/ovn-nb-idl.h"
@@ -42,8 +43,10 @@ static bool chassis_features_changed(const struct 
chassis_features *,
 static bool config_out_of_sync(const struct smap *config,
const struct smap *saved_config,
const char *key, bool must_be_present);
-static bool check_nb_options_out_of_sync(const struct nbrec_nb_global *,
- struct ed_type_global_config *);
+static bool check_nb_options_out_of_sync(
+const struct nbrec_nb_global *,
+struct ed_type_global_config *,
+const struct sampling_app_table *);
 static void update_sb_config_options_to_sbrec(struct ed_type_global_config *,
   const struct sbrec_sb_global *);
 
@@ -72,6 +75,9 @@ en_global_config_run(struct engine_node *node , void *data)
 EN_OVSDB_GET(engine_get_input("SB_sb_global", node));
 const struct sbrec_chassis_table *sbrec_chassis_table =
 EN_OVSDB_GET(engine_get_input("SB_chassis", node));
+const struct ed_type_sampling_app_data *sampling_app_data =
+engine_get_input_data("sampling_app", node);
+const struct sampling_app_table *sampling_apps = &sampling_app_data->apps;
 
 struct ed_type_global_config *config_data = data;
 
@@ -145,7 +151,8 @@ en_global_config_run(struct engine_node *node , void *data)
 build_chassis_features(sbrec_chassis_table, &config_data->features);
 }
 
-init_debug_config(nb);
+init_debug_config(nb, sampling_app_get_id(sampling_apps,
+  SAMPLING_APP_DROP_DEBUG));
 
 const struct sbrec_sb_global *sb =
 sbrec_sb_global_table_first(sb_global_table);
@@ -186,6 +193,9 @@ global_config_nb_global_handler(struct engine_node *node, 
void *data)
 EN_OVSDB_GET(engine_get_input("NB_nb_global", node));
 const struct sbrec_sb_global_table *sb_global_table =
 EN_OVSDB_GET(engine_get_input("SB_sb_global", node));
+const struct ed_type_sampl

[ovs-dev] [PATCH ovn v2 5/8] northd: Commit from-lport ACL label (and state) when LBs are used.

[Dumitru Ceara]

Quoting the ACL label section in the ovn.nb.5 man page:

  Associates an identifier with the ACL.
  The same value will be written to corresponding connection
  tracker entry. The value should be a valid 32-bit unsigned integer.
  This value can help in debugging from connection tracker side.
  For example, through this "label" we can backtrack to the ACL rule
  which is causing a "leaked" connection. Connection tracker entries are
  created only for allowed connections so the label is valid only
  for allow and allow-related actions.

The above states that the ACL label must always be stored in the
connection tracker entry label for allow-related ACLs (regardless of the
direction of the ACL).  However, since 74d82e296f80 ("northd: Support
the option to apply from-lport ACLs after load balancer."), the
connection is not re-committed in the ls_in_stateful stage (because it
already was committed as part of the load balancer DNAT).

Moreover, by not re-committing the connection after LB we also risk not
re-setting any potential ct_mark.blocked value the connection might
have.

This patch addresses the issue by always committing packets matched by
allow-related (or stateful in general) ACLs even if they were also
committed as part of the load balancing stage.

There's potentially a slight overhead when doing this (an additional
commit call into conntrack but _no_ recirculation).  This is however
acceptable as it is required for a correct packet processing pipeline
implementation.  Even without this fix, packets creating new
connections tha hit "--apply-after-lb" ACLs trigger a re-commit (for
storing the label and ct_mark.blocked).

A new test is added to ensure we don't break this functionality in the
future.

CC: Numan Siddique 
Fixes: 74d82e296f80 ("northd: Support the option to apply from-lport ACLs after 
load balancer.")
Signed-off-by: Dumitru Ceara 
---
 northd/northd.c |   8 +---
 tests/ovn-northd.at | 110 +++-
 tests/ovn.at|   4 +-
 3 files changed, 82 insertions(+), 40 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index fcf8f277ac..901b9e9cd1 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -7494,8 +7494,7 @@ build_lb_affinity_ls_flows(struct lflow_table *lflows,
 ipv6 ? REG_LB_L2_AFF_BACKEND_IP6 : REG_LB_AFF_BACKEND_IP4;
 
 /* Prepare common part of affinity LB and affinity learn action. */
-ds_put_format(&aff_action, REGBIT_CONNTRACK_COMMIT" = 0; %s = %s; ",
-  reg_vip, lb_vip->vip_str);
+ds_put_format(&aff_action, "%s = %s; ", reg_vip, lb_vip->vip_str);
 ds_put_cstr(&aff_action_learn, "commit_lb_aff(vip = \"");
 
 if (lb_vip->port_str) {
@@ -7635,11 +7634,6 @@ build_lb_rules(struct lflow_table *lflows, struct 
ovn_lb_datapaths *lb_dps,
 ds_clear(action);
 ds_clear(match);
 
-/* Make sure that we clear the REGBIT_CONNTRACK_COMMIT flag.  Otherwise
- * the load balanced packet will be committed again in
- * S_SWITCH_IN_STATEFUL. */
-ds_put_format(action, REGBIT_CONNTRACK_COMMIT" = 0; ");
-
 /* New connections in Ingress table. */
 const char *meter = NULL;
 bool reject = build_lb_vip_actions(lb, lb_vip, lb_vip_nb, action,
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 6802fac380..5acb13c519 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -1413,7 +1413,7 @@ check ovn-nbctl --wait=sb ls-lb-add sw0 lb1
 AT_CAPTURE_FILE([sbflows])
 OVS_WAIT_FOR_OUTPUT(
   [ovn-sbctl dump-flows sw0 | tee sbflows | grep 'priority=120.*backends' | 
ovn_strip_lflows], 0, [dnl
-  table=??(ls_in_lb   ), priority=120  , match=(ct.new && ip4.dst == 
10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; 
ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);)
+  table=??(ls_in_lb   ), priority=120  , match=(ct.new && ip4.dst == 
10.0.0.10 && tcp.dst == 80), 
action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);)
 ])
 
 # disabled LSPs should not be a backend of Load Balancer
@@ -1422,7 +1422,7 @@ check ovn-nbctl lsp-set-enabled sw0-p1 disabled
 AT_CAPTURE_FILE([sbflows])
 OVS_WAIT_FOR_OUTPUT(
   [ovn-sbctl dump-flows sw0 | tee sbflows | grep 'priority=120.*backends' | 
ovn_strip_lflows], 0, [dnl
-  table=??(ls_in_lb   ), priority=120  , match=(ct.new && ip4.dst == 
10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; 
ct_lb_mark(backends=20.0.0.3:80);)
+  table=??(ls_in_lb   ), priority=120  , match=(ct.new && ip4.dst == 
10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=20.0.0.3:80);)
 ])
 wait_row_count Service_Monitor 1
 
@@ -1431,7 +1431,7 @@ check ovn-nbctl lsp-set-enabled sw0-p1 enabled
 AT_CAPTURE_FILE([sbflows])
 OVS_WAIT_FOR_OUTPUT(
   [ovn-sbctl dump-flows sw0 | tee sbflows | grep 'priority=120.*backends' | 
ovn_strip_lflows], 0, [dnl
-  table=??(ls_in_lb   ), priority=120  , match=(ct.new && ip4.dst == 
10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; 
ct_lb_mark(backend

[ovs-dev] [PATCH ovn v2 4/8] tests: Fix unreliable "ACL and committing to conntrack" system test.

[Dumitru Ceara]

For the following logical topology:
VIF1 -- LS1 (stateful ACL) -- LR (no NAT) -- LS2 (stateful ACL) -- VIF2

The test was trying to determine whether a commit happened in the egress
pipeline of LS1 when forwarding packets through the logical patch port
towards LR.  There is unfortunately no reliable way of doing that by
just checking the datapath flows.

Instead, add a test that uses ovn-trace to ensure that the commit
doesn't happen.

This change is required because a follow up fix will add a missing
(re-)commit to the ingress pipeline of LS1 which was causing the
original test to incorrectly fail.

CC: Xavier Simonart 
Fixes: d17ece7f3706 ("northd: prevents sending packet to conntrack for router 
ports")
Signed-off-by: Dumitru Ceara 
---
 tests/ovn-northd.at | 63 +
 tests/system-ovn.at |  5 
 2 files changed, 63 insertions(+), 5 deletions(-)

diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index e345e6f591..6802fac380 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -10211,6 +10211,69 @@ acl_test to-lport "" pg ls_out_acl_eval
 AT_CLEANUP
 ])
 
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([ACL - ct state clear on router ports])
+AT_KEYWORDS([acl])
+
+ovn_start
+
+dnl Topology: vm1 -- s1 -- r1  -- s2 -- vm2
+dnl - LBs applied on s1 and s2 (all ACLs become stateful)
+dnl - allow ACLs on s1 and s2
+
+check ovn-nbctl  \
+  -- lr-add r1   \
+  -- lrp-add r1 r1_s1 00:de:ad:fe:00:01 173.0.1.1/24 \
+  -- lrp-add r1 r1_s2 00:de:ad:fe:00:02 173.0.2.1/24 \
+ \
+  -- ls-add s1   \
+  -- lsp-add s1 s1_r1\
+  -- lsp-set-type s1_r1 router   \
+  -- lsp-set-addresses s1_r1 router  \
+  -- lsp-set-options s1_r1 router-port=r1_s1 \
+ \
+  -- ls-add s2   \
+  -- lsp-add s2 s2_r1\
+  -- lsp-set-type s2_r1 router   \
+  -- lsp-set-addresses s2_r1 router  \
+  -- lsp-set-options s2_r1 router-port=r1_s2 \
+ \
+  -- lsp-add s1 vm1  \
+  -- lsp-set-addresses vm1 "00:de:ad:01:00:01 173.0.1.2" \
+ \
+  -- lsp-add s2 vm2  \
+  -- lsp-set-addresses vm2 "00:de:ad:01:00:02 173.0.2.2" \
+ \
+  -- lb-add lb1 30.0.0.1 173.0.2.2   \
+  -- lb-add lb2 173.0.2.250 173.0.1.3\
+  -- ls-lb-add s1 lb1\
+  -- ls-lb-add s2 lb2\
+ \
+  -- acl-add s1 from-lport 1001 "ip" allow   \
+  -- acl-add s1 to-lport 1002 "ip" allow \
+  -- acl-add s2 from-lport 1003 "ip" allow   \
+  -- acl-add s2 to-lport 1004 "ip" allow
+
+check ovn-nbctl --wait=sb sync
+
+dnl Simulate a packet going from vm1 -> router -> vm2.
+dnl It should not commit anything in the egress pipeline of S1 or in the
+dnl ingress pipeline of S2.
+flow="inport == \"vm1\" && eth.src == 00:de:ad:01:00:01 && eth.dst == 
00:de:ad:fe:00:01 && ip4.src == 173.0.1.2 && ip4.dst == 30.0.0.1 && ip.ttl==64"
+
+dnl Check that we only commit once for ACLs, in the egress ACL pipeline
+dnl (in S2, towards vm2).  The original problem this test is trying to
+dnl cover was that ct_state wasn't cleared when traversing from s1 -> r1
+dnl which caused two additional commits to happen:
+dnl - in the egress pipeline of S1, when sending the packet out on s1_r1
+dnl - in the ingress pipeline of S2, when processing the packet on s2_r1
+AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --ct new s1 "$flow" | grep -e 
ls_in_stateful -e ls_out_stateful -A 2 | grep commit], [0], [dnl
+ct_commit { ct_mark.blocked = 0; };
+])
+
+AT_CLEANUP
+])
+
 AT_SETUP([Localnet ports on LS with LB])
 ovn_start
 # In the past, traffic arriving on localnet ports has skipped conntrack.
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index c24ede7c50..ddb3d14e92 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -10632,11 +10632,6 @@ 
icmp,orig=(src=173.0.1.2,dst=173.0.2.2,id=,type=8,code=0),reply=(src=17
 
icmp,orig=(src=173.0.1.2,dst=30.0.0.1,id=,type=8,code=0),reply=(src=173.0.2.2,dst=173.0.1.2,id=,type=0,code=0),zone=,mark=2
 ])
 
-# Now check for multiple ct_commits
-ovs-appctl dpctl/dump-flows > dp_flows
-zone_id=$(ovn-appctl -t ovn-controller ct-zone-list | grep vm1 | cut -d ' ' 
-f2)
-AT_CHECK([test 1 = `cat dp_flows | grep "commit,zone=$zone_id" | wc -l`])
-
 

[ovs-dev] [PATCH ovn v2 3/8] northd: Assume all chassis support the "ovn-ct-lb-related" feature.

[Dumitru Ceara]

This feature is supported in the last two LTS releases and the correct
upgrade procedure mandates that we don't jump across LTS releases.  It's
safe to remove the check in northd.

Signed-off-by: Dumitru Ceara 
---
 northd/en-global-config.c |  14 
 northd/en-global-config.h |   1 -
 northd/inc-proc-northd.c  |   2 -
 northd/northd.c   |  38 ---
 tests/ovn-northd.at   | 130 --
 5 files changed, 14 insertions(+), 171 deletions(-)

diff --git a/northd/en-global-config.c b/northd/en-global-config.c
index 183b535dee..5b71ede1f2 100644
--- a/northd/en-global-config.c
+++ b/northd/en-global-config.c
@@ -367,7 +367,6 @@ northd_enable_all_features(struct ed_type_global_config 
*data)
 {
 data->features = (struct chassis_features) {
 .mac_binding_timestamp = true,
-.ct_lb_related = true,
 .fdb_timestamp = true,
 .ls_dpg_column = true,
 .ct_commit_nat_v2 = true,
@@ -398,15 +397,6 @@ build_chassis_features(const struct sbrec_chassis_table 
*sbrec_chassis_table,
 chassis_features->mac_binding_timestamp = false;
 }
 
-bool ct_lb_related =
-smap_get_bool(&chassis->other_config,
-  OVN_FEATURE_CT_LB_RELATED,
-  false);
-if (!ct_lb_related &&
-chassis_features->ct_lb_related) {
-chassis_features->ct_lb_related = false;
-}
-
 bool fdb_timestamp =
 smap_get_bool(&chassis->other_config,
   OVN_FEATURE_FDB_TIMESTAMP,
@@ -568,10 +558,6 @@ chassis_features_changed(const struct chassis_features 
*present,
 return true;
 }
 
-if (present->ct_lb_related != updated->ct_lb_related) {
-return true;
-}
-
 if (present->fdb_timestamp != updated->fdb_timestamp) {
 return true;
 }
diff --git a/northd/en-global-config.h b/northd/en-global-config.h
index c3cc881371..8a1c35fc8f 100644
--- a/northd/en-global-config.h
+++ b/northd/en-global-config.h
@@ -15,7 +15,6 @@ struct sbrec_sb_global;
 
 struct chassis_features {
 bool mac_binding_timestamp;
-bool ct_lb_related;
 bool fdb_timestamp;
 bool ls_dpg_column;
 bool ct_commit_nat_v2;
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index 180b2be3e9..522236ad2a 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -463,8 +463,6 @@ chassis_features_list(struct unixctl_conn *conn, int argc 
OVS_UNUSED,
 struct chassis_features *features = features_;
 struct ds ds = DS_EMPTY_INITIALIZER;
 
-ds_put_format(&ds, "ct_lb_related: %s\n",
-  features->ct_lb_related ? "true" : "false");
 ds_put_format(&ds, "mac_binding_timestamp: %s\n",
   features->mac_binding_timestamp ? "true" : "false");
 unixctl_command_reply(conn, ds_cstr(&ds));
diff --git a/northd/northd.c b/northd/northd.c
index 9cc6e6c14f..fcf8f277ac 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -6839,7 +6839,6 @@ build_acl_log_related_flows(const struct ovn_datapath *od,
 static void
 build_acls(const struct ls_stateful_record *ls_stateful_rec,
const struct ovn_datapath *od,
-   const struct chassis_features *features,
struct lflow_table *lflows,
const struct ls_port_group_table *ls_port_groups,
const struct shash *meter_groups,
@@ -6982,15 +6981,10 @@ build_acls(const struct ls_stateful_record 
*ls_stateful_rec,
  * related traffic such as an ICMP Port Unreachable through
  * that's generated from a non-listening UDP port.  */
 const char *ct_in_acl_action =
-features->ct_lb_related
-? REGBIT_ACL_HINT_ALLOW_REL" = 1; "
-  REGBIT_ACL_VERDICT_ALLOW" = 1; ct_commit_nat;"
-: REGBIT_ACL_HINT_ALLOW_REL" = 1; "
-  REGBIT_ACL_VERDICT_ALLOW" = 1; next;";
+REGBIT_ACL_HINT_ALLOW_REL" = 1; "
+REGBIT_ACL_VERDICT_ALLOW" = 1; ct_commit_nat;";
 const char *ct_out_acl_action =
-features->ct_lb_related
-? REGBIT_ACL_VERDICT_ALLOW" = 1; ct_commit_nat;"
-: REGBIT_ACL_VERDICT_ALLOW" = 1; next;";
+REGBIT_ACL_VERDICT_ALLOW" = 1; ct_commit_nat;";
 ds_clear(&match);
 ds_put_format(&match, "!ct.est && ct.rel && !ct.new%s "
   "&& ct_mark.blocked == 0",
@@ -15177,7 +15171,7 @@ build_lrouter_nat_defrag_and_lb(
  * a dynamically negotiated FTP data channel), but will allow
  * related traffic such as an ICMP Port Unreachable through
  * that's generated from a non-listening UDP port.  */
-if (lr_stateful_rec->has_lb_vip && features->ct_lb_related) {
+if (lr_stateful_rec->has_lb_vip) {
 ds_clear(match);
 
 ds_put_cstr(match, "ct.rel && !ct.est && !ct.new");
@@ -15197,18 +15191,16 @@ build_lrouter_nat_defrag_and_lb(
 ds_truncate(match, matc

[ovs-dev] [PATCH ovn v2 2/8] northd: Assume all chassis support the "ct-no-masked-label" feature.

[Dumitru Ceara]

This feature is supported in the last two LTS releases and the correct
upgrade procedure mandates that we don't jump across LTS releases.  It's
safe to remove the check in northd.

Also remove logical field definitions for ct_label.s that cannot
appear anymore in the southbound database.

Signed-off-by: Dumitru Ceara 
---
Changes in v2:
- fixed "Load balancer CT related backwards compatibility" test.
---
 controller/lflow.c  |  39 ++-
 controller/lflow.h  |   1 -
 controller/ovn-controller.c |  22 
 lib/logical-fields.c|  22 
 northd/en-global-config.c   |  23 
 northd/en-global-config.h   |   1 -
 northd/inc-proc-northd.c|   2 -
 northd/northd.c | 190 +++
 ovn-sb.xml  |  19 
 tests/ovn-controller.at |   8 +-
 tests/ovn-northd.at | 215 
 tests/ovn.at|   9 +-
 12 files changed, 95 insertions(+), 456 deletions(-)

diff --git a/controller/lflow.c b/controller/lflow.c
index b4c379044f..9f6564787f 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -100,7 +100,6 @@ consider_logical_flow(const struct sbrec_logical_flow 
*lflow,
 static void
 consider_lb_hairpin_flows(const struct ovn_controller_lb *lb,
   const struct hmap *local_datapaths,
-  bool use_ct_mark,
   struct ovn_desired_flow_table *flow_table);
 
 static void add_port_sec_flows(const struct shash *binding_lports,
@@ -1631,7 +1630,6 @@ static void
 add_lb_vip_hairpin_flows(const struct ovn_controller_lb *lb,
  struct ovn_lb_vip *lb_vip,
  struct ovn_lb_backend *lb_backend,
- bool use_ct_mark,
  struct ovn_desired_flow_table *flow_table)
 {
 uint64_t stub[1024 / 8];
@@ -1722,29 +1720,13 @@ add_lb_vip_hairpin_flows(const struct ovn_controller_lb 
*lb,
  * - packets must have ip.src == ip.dst at this point.
  * - the destination protocol and port must be of a valid backend that
  *   has the same IP as ip.dst.
- *
- * During upgrades logical flows might still use the old way of storing
- * ct.natted in ct_label.  For backwards compatibility, only use ct_mark
- * if ovn-northd notified ovn-controller to do that.
  */
-if (use_ct_mark) {
-uint32_t lb_ct_mark = OVN_CT_NATTED;
-match_set_ct_mark_masked(&hairpin_match, lb_ct_mark, lb_ct_mark);
+uint32_t lb_ct_mark = OVN_CT_NATTED;
+match_set_ct_mark_masked(&hairpin_match, lb_ct_mark, lb_ct_mark);
 
-ofctrl_add_flow(flow_table, OFTABLE_CHK_LB_HAIRPIN, 100,
-lb->slb->header_.uuid.parts[0], &hairpin_match,
-&ofpacts, &lb->slb->header_.uuid);
-} else {
-match_set_ct_mark_masked(&hairpin_match, 0, 0);
-ovs_u128 lb_ct_label = {
-.u64.lo = OVN_CT_NATTED,
-};
-match_set_ct_label_masked(&hairpin_match, lb_ct_label, lb_ct_label);
-
-ofctrl_add_flow(flow_table, OFTABLE_CHK_LB_HAIRPIN, 100,
-lb->slb->header_.uuid.parts[0], &hairpin_match,
-&ofpacts, &lb->slb->header_.uuid);
-}
+ofctrl_add_flow(flow_table, OFTABLE_CHK_LB_HAIRPIN, 100,
+lb->slb->header_.uuid.parts[0], &hairpin_match,
+&ofpacts, &lb->slb->header_.uuid);
 
 ofpbuf_uninit(&ofpacts);
 }
@@ -1967,7 +1949,6 @@ add_lb_ct_snat_hairpin_flows(const struct 
ovn_controller_lb *lb,
 static void
 consider_lb_hairpin_flows(const struct ovn_controller_lb *lb,
   const struct hmap *local_datapaths,
-  bool use_ct_mark,
   struct ovn_desired_flow_table *flow_table)
 {
 for (size_t i = 0; i < lb->n_vips; i++) {
@@ -1976,8 +1957,7 @@ consider_lb_hairpin_flows(const struct ovn_controller_lb 
*lb,
 for (size_t j = 0; j < lb_vip->n_backends; j++) {
 struct ovn_lb_backend *lb_backend = &lb_vip->backends[j];
 
-add_lb_vip_hairpin_flows(lb, lb_vip, lb_backend,
- use_ct_mark, flow_table);
+add_lb_vip_hairpin_flows(lb, lb_vip, lb_backend, flow_table);
 }
 }
 
@@ -1989,13 +1969,11 @@ consider_lb_hairpin_flows(const struct 
ovn_controller_lb *lb,
 static void
 add_lb_hairpin_flows(const struct hmap *local_lbs,
  const struct hmap *local_datapaths,
- bool use_ct_mark,
  struct ovn_desired_flow_table *flow_table)
 {
 const struct ovn_controller_lb *lb;
 HMAP_FOR_EACH (lb, hmap_node, local_lbs) {
-consider_lb_hairpin_flows(lb, local_datapaths,
-  use_ct_mark, flow_table);
+consider_lb_hairpin_flows(lb, local_datapaths, flow_table);
 }
 }
 
@@ -2155,7 +2133,6 @@ lflow_run(struct lflow_ctx_in *l_ctx

[ovs-dev] [PATCH ovn v2 1/8] northd: Assume all chassis support the "port-up-notif" feature.

[Dumitru Ceara]

This feature is supported in the last two LTS releases and the correct
upgrade procedure mandates that we don't jump across LTS releases.  It's
safe to remove the check in northd.

Signed-off-by: Dumitru Ceara 
---
 northd/northd.c |  3 +--
 tests/ovn-northd.at | 24 
 2 files changed, 1 insertion(+), 26 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 6898daa00d..1b5a7480e4 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -17903,8 +17903,7 @@ handle_port_binding_changes(struct ovsdb_idl_txn 
*ovnsb_txn,
 if (lsp_is_router(op->nbsp)) {
 up = true;
 } else if (sb->chassis) {
-up = smap_get_bool(&sb->chassis->other_config,
-   OVN_FEATURE_PORT_UP_NOTIF, false)
+up = !smap_get_bool(&sb->chassis->other_config, "is-remote", false)
  ? sb->n_up && sb->up[0]
  : true;
 }
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index a389d19886..7dc94e1f56 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -4168,30 +4168,6 @@ AT_CHECK([grep -qE 'duplicate logical.*port p1' 
northd/ovn-northd.log], [0])
 AT_CLEANUP
 ])
 
-OVN_FOR_EACH_NORTHD_NO_HV([
-AT_SETUP([Port_Binding.up backwards compatibility])
-ovn_start
-
-ovn-nbctl ls-add ls1
-ovn-nbctl --wait=sb lsp-add ls1 lsp1
-
-# Simulate the fact that lsp1 had been previously bound on hv1 by an
-# ovn-controller running an older version.
-ovn-sbctl \
---id=@e create encap chassis_name=hv1 ip="192.168.0.1" type="geneve" \
--- --id=@c create chassis name=hv1 encaps=@e \
--- set Port_Binding lsp1 chassis=@c
-
-wait_for_ports_up lsp1
-
-# Simulate the fact that hv1 is aware of Port_Binding.up, ovn-northd
-# should transition the port state to down.
-check ovn-sbctl set chassis hv1 other_config:port-up-notif=true
-wait_row_count nb:Logical_Switch_Port 1 up=false name=lsp1
-
-AT_CLEANUP
-])
-
 OVN_FOR_EACH_NORTHD_NO_HV_PARALLELIZATION([
 AT_SETUP([Load Balancers and lb_force_snat_ip for Gateway Routers])
 ovn_start
-- 
2.44.0

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

[ovs-dev] [PATCH ovn v2 0/8] Add ACL Sampling using per-flow IPFIX.

[Dumitru Ceara]

This series adds support for sampling packets processed by ACLs by using
per-flow IPFIX.  This new feature allows users to configure
(potentially) different sampling options for ACL matched traffic that
creates new connections or that is forwarded on existing connections.

This work is based on Adrian's original RFC:
https://patchwork.ozlabs.org/project/ovn/cover/20221018155936.1394396-1-amore...@redhat.com/

In order for the whole feature to work properly some pre-requisite work
is done:
- patches 1-3: simplify northd code assuming that all controllers are
  aware of features included in the previous LTS release (22.03) - the
  current LTS release is 24.03.
- patch 4: fixes an incorrect test that mistakenly fails when the bug
  fix in patch 5 is applied.
- patch 5: fixes a bug in the way ACLs with labels are processed when
  the switches also have load balancers configured

The feature itself is implemented by the last 3 patches:
- patch 6: adds support for users to configure different types of
  sampling applications (drop debug, acl-new-traffic,
  acl-established-traffic)
- patch 7: combines the already existing drop debug sampling
  configuration with the new sampling application configuration (giving
  priority to the latter)
- patch 8: adds sampling support to ACLs

Changes in V2:
- Addressed Adrian's comments on patch 8.
- Fixed unit test failure in patch 2.

Adrian Moreno (1):
  northd: Add ACL Sampling.

Dumitru Ceara (7):
  northd: Assume all chassis support the "port-up-notif" feature.
  northd: Assume all chassis support the "ct-no-masked-label" feature.
  northd: Assume all chassis support the "ovn-ct-lb-related" feature.
  tests: Fix unreliable "ACL and committing to conntrack" system test.
  northd: Commit from-lport ACL label (and state) when LBs are used.
  northd: Add Sampling_App table.
  northd: Override NB_Global drop sampling id with Sampling_App config.

 NEWS   |   6 +
 controller/lflow.c |  39 +-
 controller/lflow.h |   1 -
 controller/ovn-controller.c|  22 -
 lib/logical-fields.c   |  28 +-
 northd/automake.mk |   2 +
 northd/debug.c |  12 +-
 northd/debug.h |   3 +-
 northd/en-global-config.c  |  68 +--
 northd/en-global-config.h  |   2 -
 northd/en-lflow.c  |   5 +
 northd/en-sampling-app.c   | 120 
 northd/en-sampling-app.h   |  51 ++
 northd/inc-proc-northd.c   |  15 +-
 northd/northd.c| 750 ++--
 northd/northd.h|   1 +
 northd/ovn-northd.8.xml|  26 +
 ovn-nb.ovsschema   |  63 +-
 ovn-nb.xml |  80 +++
 ovn-sb.xml |  19 -
 tests/atlocal.in   |   6 +
 tests/ovn-controller.at|   8 +-
 tests/ovn-macros.at|   4 +
 tests/ovn-nbctl.at |  20 +
 tests/ovn-northd.at| 774 +
 tests/ovn.at   |  22 +-
 tests/system-common-macros.at  |  11 +
 tests/system-ovn.at| 154 -
 utilities/containers/fedora/Dockerfile |   1 +
 utilities/containers/ubuntu/Dockerfile |   1 +
 utilities/ovn-nbctl.8.xml  |   8 +-
 utilities/ovn-nbctl.c  |  43 +-
 32 files changed, 1617 insertions(+), 748 deletions(-)
 create mode 100644 northd/en-sampling-app.c
 create mode 100644 northd/en-sampling-app.h

-- 
2.44.0

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

Re: [ovs-dev] [PATCH net-next v2] selftests: openvswitch: retry instead of sleep

[Jakub Kicinski]

On Thu, 11 Jul 2024 09:41:08 + Adrián Moreno wrote:
> This patch is supposed to fix openvswitch selftests on "-dbg" machines.
> However, as Simon points out, all recent rounds are failing [1]. I don't
> see this patch being included in the batches and I was wondering why.
> 
> Also I see a (presumably unrelated) build error netdev/build_32bit.
> Is there anything I can do?

Hopefully fixed now, we'll see if it gets into the next run.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] northd: Detect if SB LB was removed and re-sync that row.

[Dumitru Ceara]

On 7/4/24 18:01, Numan Siddique wrote:
> On Wed, Jul 3, 2024 at 8:20 AM Ales Musil  wrote:
>>
>> When someone removes SB LB it won't be detected until the next
>> recompute of the "sync_to_sb_lb" node. This will cause traffic
>> disruptions in case of hairpin as the flows directly depend on
>> the SB LB entries. Add check to trigger recompute when we detect
>> that the row is missing in SB, but still present in NB.
>>
>> Reported-at: https://issues.redhat.com/browse/FDP-682
>> Signed-off-by: Ales Musil 
> 
> Thanks for the patch.
> 
> I'm not sure if we need to fix this.  Ideally a user is not supposed
> to destroy records in SB DB.
> And if the user does it so, then I'd assume the user should trigger a 
> recompute.
> 

I agree, it doesn't seem like a correct use of OVN if users mangle the
SB database directly.

> You'd see the same behavior if you delete a logical flow manually from
> SB DB.  ovn-northd doesn't fix this
> causing traffic disruptions until ovn-northd reconciles the SB DB.
> 
> I'm not against this fix.  This patch is straightforward and we
> already check for duplicate entries in SB LB
> in the sync_to_sb_lb_sb_load_balancer().
> 

The check for duplicates, on the other hand, needs to be kept because
the SB LB table doesn't have an index so a single insert operation from
northd might create duplicates in the SB (when running SB with raft).

>  But maybe we should discuss if we want to take a similar approach for
> other tables as well or not.
> 

In my opinion we shouldn't handle these cases.  Users shouldn't change
the southbound.

> Thanks
> Numan
> 

Regards,
Dumitru

>> ---
>>  northd/en-sync-sb.c | 38 +-
>>  tests/ovn-macros.at |  2 +-
>>  tests/ovn-northd.at | 38 ++
>>  3 files changed, 72 insertions(+), 6 deletions(-)
>>
>> diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
>> index 9bd8a1fc6..12dbd139e 100644
>> --- a/northd/en-sync-sb.c
>> +++ b/northd/en-sync-sb.c
>> @@ -20,6 +20,7 @@
>>
>>  /* OVS includes. */
>>  #include "lib/svec.h"
>> +#include "lib/uuidset.h"
>>  #include "openvswitch/util.h"
>>
>>  /* OVN includes. */
>> @@ -232,6 +233,7 @@ struct sb_lb_table {
>>  struct hmap entries; /* Stores struct sb_lb_record. */
>>  struct hmap ls_dp_groups;
>>  struct hmap lr_dp_groups;
>> +struct uuidset sb_entries;
>>  };
>>
>>  struct ed_type_sync_to_sb_lb_data {
>> @@ -347,16 +349,29 @@ sync_to_sb_lb_northd_handler(struct engine_node *node, 
>> void *data_)
>>  }
>>
>>  bool
>> -sync_to_sb_lb_sb_load_balancer(struct engine_node *node, void *data 
>> OVS_UNUSED)
>> +sync_to_sb_lb_sb_load_balancer(struct engine_node *node, void *data_)
>>  {
>>  const struct sbrec_load_balancer_table *sb_load_balancer_table =
>>  EN_OVSDB_GET(engine_get_input("SB_load_balancer", node));
>> +struct ed_type_sync_to_sb_lb_data *data = data_;
>>
>> -/* The only reason to handle SB.Load_Balancer updates is to detect
>> +/* The reasons to handle SB.Load_Balancer updates is to detect
>>   * spurious records being created in clustered databases due to
>> - * lack of indexing on the SB.Load_Balancer table.  All other changes
>> - * are valid and performed by northd, the only write-client for
>> - * this table. */
>> + * lack of indexing on the SB.Load_Balancer table.  The other reason 
>> might
>> + * be when someone removes the SB row while the NB row is still valid.
>> + * All other changes are valid and performed by northd. */
>> +
>> +const struct sbrec_load_balancer *sb_lb;
>> +SBREC_LOAD_BALANCER_TABLE_FOR_EACH_TRACKED (sb_lb,
>> +sb_load_balancer_table) {
>> +if (sbrec_load_balancer_is_deleted(sb_lb) &&
>> +uuidset_find(&data->sb_lbs.sb_entries, &sb_lb->header_.uuid)) {
>> +VLOG_WARN("A SB LB for \"%s\" is deleted but the NB LB entry "
>> +  "still exists.", sb_lb->name);
>> +return false;
>> +}
>> +}
>> +
>>  if (check_sb_lb_duplicates(sb_load_balancer_table)) {
>>  return false;
>>  }
>> @@ -626,6 +641,7 @@ sb_lb_table_init(struct sb_lb_table *sb_lbs)
>>  hmap_init(&sb_lbs->entries);
>>  ovn_dp_groups_init(&sb_lbs->ls_dp_groups);
>>  ovn_dp_groups_init(&sb_lbs->lr_dp_groups);
>> +uuidset_init(&sb_lbs->sb_entries);
>>  }
>>
>>  static void
>> @@ -638,6 +654,7 @@ sb_lb_table_clear(struct sb_lb_table *sb_lbs)
>>
>>  ovn_dp_groups_clear(&sb_lbs->ls_dp_groups);
>>  ovn_dp_groups_clear(&sb_lbs->lr_dp_groups);
>> +uuidset_clear(&sb_lbs->sb_entries);
>>  }
>>
>>  static void
>> @@ -647,6 +664,7 @@ sb_lb_table_destroy(struct sb_lb_table *sb_lbs)
>>  hmap_destroy(&sb_lbs->entries);
>>  ovn_dp_groups_destroy(&sb_lbs->ls_dp_groups);
>>  ovn_dp_groups_destroy(&sb_lbs->lr_dp_groups);
>> +uuidset_destroy(&sb_lbs->sb_entries);
>>  }
>>
>>  static struct sb_lb_record *
>> @@ -693,6 

Re: [ovs-dev] [PATCH v1 13/13] ofp-actions: Load data from fields in sample action.

[Ilya Maximets]

On 7/11/24 09:28, Adrián Moreno wrote:
> On Thu, Jul 11, 2024 at 12:32:06AM GMT, Ilya Maximets wrote:
>> On 7/10/24 23:38, Adrián Moreno wrote:
>>> On Wed, Jul 10, 2024 at 11:00:43PM GMT, Ilya Maximets wrote:
 On 7/7/24 22:09, Adrian Moreno wrote:
> When sample action gets used as a way of sampling traffic with
> controller-generated metadata (i.e: obs_domain_id and obs_point_id),
> the controller will have to increase the number of flows to ensure each
> part of the pipeline contains the right metadata.
>
> As an example, if the controller decides to sample stateful traffic, it
> could store the computed metadata for each connection in the conntrack
> label. However, for established connections, a flow must be created for
> each different ct_label value with a sample action that contains a
> different hardcoded obs_domain and obs_point id.
>
> This patch adds a new version of the NXAST_RAW_SAMPLE* action (number 4)
> that supports specifying the observation point and domain using an
> OpenFlow field reference, so now the controller can express:
>
>  sample(...
> obs_domain_id=NXM_NX_CT_LABEL[0..31],
> obs_point_id=NXM_NX_CT_LABEL[32..63]
> ...
>)
>
> Signed-off-by: Adrian Moreno 
> ---
>  include/openvswitch/ofp-actions.h |   8 +-
>  lib/ofp-actions.c | 249 +++---
>  ofproto/ofproto-dpif-xlate.c  |  55 ++-
>  python/ovs/flow/ofp.py|   8 +-
>  python/ovs/flow/ofp_act.py|   4 +-
>  tests/ofp-actions.at  |   5 +
>  tests/ofproto-dpif.at |  41 +
>  tests/system-traffic.at   |  74 +
>  8 files changed, 405 insertions(+), 39 deletions(-)

 Not a full review, it's a complicated change.  See a few comments below.

>
> diff --git a/include/openvswitch/ofp-actions.h 
> b/include/openvswitch/ofp-actions.h
> index 7b57e49ad..56dc2c147 100644
> --- a/include/openvswitch/ofp-actions.h
> +++ b/include/openvswitch/ofp-actions.h
> @@ -1015,14 +1015,16 @@ enum nx_action_sample_direction {
>
>  /* OFPACT_SAMPLE.
>   *
> - * Used for NXAST_SAMPLE, NXAST_SAMPLE2, and NXAST_SAMPLE3. */
> + * Used for NXAST_SAMPLE, NXAST_SAMPLE2, NXAST_SAMPLE3 and 
> NXAST_SAMPLE4. */
>  struct ofpact_sample {
>  OFPACT_PADDED_MEMBERS(
>  struct ofpact ofpact;
>  uint16_t probability;   /* Always positive. */
>  uint32_t collector_set_id;
> -uint32_t obs_domain_id;
> -uint32_t obs_point_id;
> +uint32_t obs_domain_imm;
> +struct mf_subfield obs_domain_src;
> +uint32_t obs_point_imm;
> +struct mf_subfield obs_point_src;
>  ofp_port_t sampling_port;
>  enum nx_action_sample_direction direction;
>  );
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index da7b1dd31..e329a7e3f 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -330,6 +330,8 @@ enum ofp_raw_action_type {
>  NXAST_RAW_SAMPLE2,
>  /* NX1.0+(41): struct nx_action_sample2. */
>  NXAST_RAW_SAMPLE3,
> +/* NX1.0+(51): struct nx_action_sample4, ... VLMFF */

 Why the dots are here?  The structure doesn't seem to have
 extra fields at the end.
>>>
>>> I missread the description then. I thought it was just about alignment.
>>>

> +NXAST_RAW_SAMPLE4,
>
>  /* NX1.0+(34): struct nx_action_conjunction. */
>  NXAST_RAW_CONJUNCTION,
> @@ -6188,6 +6190,34 @@ struct nx_action_sample2 {
>   };
>   OFP_ASSERT(sizeof(struct nx_action_sample2) == 32);
>
> +/* Action structure for NXAST_SAMPLE4
> + *
> + * NXAST_SAMPLE4 was added in Open vSwitch 3.4.0.  Compared to 
> NXAST_SAMPLE3,
> + * it adds support for using field specifiers for observation_domain_id 
> and
> + * observation_point_id. */
> +struct nx_action_sample4 {
> +ovs_be16 type;  /* OFPAT_VENDOR. */
> +ovs_be16 len;   /* Length is 32. */

 Is the length 32?  40, I suppose.
>>>
>>> Yep
>>>

> +ovs_be32 vendor;/* NX_VENDOR_ID. */
> +ovs_be16 subtype;   /* NXAST_SAMPLE. */

 NXAST_SAMPLE4 ?
>>>
>>> Ack.
>>>

> +ovs_be16 probability;   /* Fraction of packets to sample. */
> +ovs_be32 collector_set_id;  /* ID of collector set in OVSDB. */
> +ovs_be32 obs_domain_src;/* Source of the 
> observation_domain_id. */
> +union {
> +ovs_be16 obs_domain_ofs_nbits;  /* Range to use from source 
> field. */
> +ovs_be32 obs_domain_imm;/* Immediate value for domain 
> id. */
> +};
> +ovs_be32 obs_

Re: [ovs-dev] [External] Re: [PATCH ovn] northd: Fix issues for Forwarding_Group

[Ales Musil]

On Thu, Jul 11, 2024 at 5:41 AM Qiang Qiang45 Zhang 
wrote:

> Test cases have been added.
>
>
>
>
>
> *发件人:* Ales Musil 
> *发送时间:* 2024年7月10日 15:02
> *收件人:* Qiang Qiang45 Zhang 
> *抄送:* d...@openvswitch.org
> *主题:* [External] Re: [ovs-dev] [PATCH ovn] northd: Fix issues for
> Forwarding_Group
>
>
>
>
>
>
>
> On Mon, Jul 8, 2024 at 5:01 PM Qiang Qiang45 Zhang via dev <
> ovs-dev@openvswitch.org> wrote:
>
> When a logical switch has 2 FWGs and each FWG has 3 ports,
> Logical-Flow only has one fwg_group() action.
> Submitted-at: northd: Fix issues for Forwarding_Group by ZhangQiang3 *
> Pull Request #250 * ovn-org/ovn (github.com)<
> https://github.com/ovn-org/ovn/pull/250>
>
> From 02186da234426bc361615eb6b5142c76f296202f Mon Sep 17 00:00:00 2001
> From: zhangqiang45 zhangqian...@lenovo.com
> Date: Mon, 8 Jul 2024 14:25:04 +0800
> Subject: [PATCH ovn] northd: Fix issues for Forwarding_Group The use of
> variables from the outer loop in the inner loop causes the outer loop to
> terminate prematurely. eg. LVS are three fwgs,
> fwg1(p1,p2,p3,p4),fwg2(p11,p22),fwg3(p31,p32),only fwg1 in logical_flows
>
> ---
>
>
>
> Hello,
>
> thank you for the fix. The commit message seems to be broken as it
> includes the email header for some reason. Also could you please add
> test to ovn-northd.at that ensures this won't happen in future?
>
>
>
> northd/northd.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 6898daa00..21ab0bb91 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -7929,6 +7929,9 @@ build_fwd_group_lflows(struct ovn_datapath *od,
> struct lflow_table *lflows,
>  continue;
>  }
>
> +ds_clear(&match);
> +ds_clear(&actions);
> +
>  /* ARP responder for the forwarding group's virtual IP */
>  ds_put_format(&match, "arp.tpa == %s && arp.op == 1",
>fwd_group->vip);
> @@ -7959,9 +7962,9 @@ build_fwd_group_lflows(struct ovn_datapath *od,
> struct lflow_table *lflows,
>  ds_put_cstr(&group_ports, "liveness=\"true\",");
>  }
>  ds_put_cstr(&group_ports, "childports=");
> -for (i = 0; i < (fwd_group->n_child_port - 1); ++i) {
> +for (int j = 0; j < (fwd_group->n_child_port - 1); ++j) {
>
>
>
> nit: Please use size_t as the "j" type.
>
>  ds_put_format(&group_ports, "\"%s\",",
> - fwd_group->child_port[i]);
> + fwd_group->child_port[j]);
>  }
>  ds_put_format(&group_ports, "\"%s\"",
>fwd_group->child_port[fwd_group->n_child_port - 1]);
> --
> 2.39.3
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
>
> Thanks,
>
> Ales
>
>
> --
>
> *Ales Musil *
>
> Senior Software Engineer - OVN Core
>
> Red Hat EMEA 
>
> amu...@redhat.com
>
> 
>
>
>

Hi,
the email format is super broken, the test looks fine, however the "j" in
the loop is still int instead of size_t.
Could you please change that and take a look at
https://docs.ovn.org/en/latest/internals/contributing/submitting-patches.html
how to properly send a patch email.

Thanks,
Ales

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.com

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

Re: [ovs-dev] [PATCH v4] ofproto: Add upcall/dump-ufid-rules command to map UFIDs to OpenFlow.

[Dumitru Ceara]

On 6/27/24 22:13, Ilya Maximets wrote:
> On 6/18/24 21:41, Dumitru Ceara wrote:
>> It improves the debugging experience if we can easily get a list of
>> OpenFlow rules and groups that contribute to the creation of a datapath
>> flow.
>>
>> The suggested workflow is:
>> a. dump datapath flows (along with UUIDs), this also prints the core IDs
>> (PMD IDs) when applicable.
>>
>>   $ ovs-appctl dpctl/dump-flows -m
>>   flow-dump from pmd on cpu core: 7
>>   ufid:7460db8f..., recirc_id(0), 
>>
>> b. dump related OpenFlow rules and groups:
>>   $ ovs-appctl upcall/dump-ufid-rules ufid:7460db8f... pmd=7
>>   cookie=0x12345678, table=0 
>> priority=100,ip,in_port=2,nw_dst=10.0.0.2,actions=resubmit(,1)
>>   cookie=0x0, table=1 priority=200,actions=group:1
>>   group_id=1,bucket=bucket_id:0,actions=ct(commit,table=2,nat(dst=20.0.0.2))
>>   cookie=0x0, table=2 actions=output:1
> 
> Hi, Dumitru.  Thanks for the patch!
> 
> This is definitely an interesting debug interface.  See some comments inline.
> 

Hi Ilya,

Thanks for the review!  I was trying to see if I can respin this before
hard freeze but I'm a bit stuck with a couple of the comments below.
Replied inline.

> 
> As an idea, should we call it ofproto/detrace ?
> 

Sure, I can rename it.

>>
>> The new command only shows rules and groups attached to ukeys that are
>> in states UKEY_VISIBLE or UKEY_OPERATIONAL.  That should be fine as all
>> other ukeys should not be relevant for the use case presented above.
>>
>> For ukeys that don't have an xcache populated yet, the command goes
>> ahead and populates one.  In theory this is creates a slight overhead as
>> those ukeys might not need an xcache until they see traffic (and get
>> revalidated) but in practice the overhead should be minimal.
> 
> The flow dumped for the first time should always be revalidated,
> even if it didn't see any more traffic.  See the should_revalidate()
> function.  This means that there is only very narrow window between
> the flow installation and the first revalidation, so the cache should
> be availabel in all practical cases.
> 
> Kepeing that in mind, I don't think that populating xcache from the
> debug appctl is a good idea.  We're holding the mutex for a long time
> while effectively revalidating the ukey from the appctl and more
> importantly we're revalidating it with side effects allowed.  This
> means we can learn some rules or update some other caches or even
> emit some packets.  We should not do that from the debug appctl.
> 
> I suggest to avoid this and just return early if the cache is not
> yet available.  It should be available in most practical situations.
> 

Makes sense, it's probably fine.

>>
>> This commit tries to mimic the output format of the ovs-ofctl
>> dump-flows/dump-groups commands.  For groups it actually uses
>> ofputil_group/_bucket functions for formatting.  For rules it uses
>> flow_stats_ds() (the original function was exported and renamed to
>> ofproto_rule_stats_ds()).
>>
>> Signed-off-by: Dumitru Ceara 
>> ---
>> V4:
>> - Addressed Mike's comment:
>>   - use predictable port IDs.
>> V3:
>> - Addressed Mike's comments:
>>   - use ds_put_char()
>>   - make the test less flaky
>> V2:
>> - Addressed Adrian's comments:
>>   - check return value of populate_xcache()
>>   - use flow_stats_ds() (renamed to ofproto_rule_stats_ds()) instead of
>> custom printing
>>   - move ukey->state check to caller
>>   - handle case when group bucket is not available
>>   - update test case to cover the point above
>> ---
>>  NEWS|   3 +
>>  include/openvswitch/ofp-group.h |   7 ++
>>  lib/ofp-group.c | 110 +++-
>>  ofproto/ofproto-dpif-upcall.c   |  85 
>>  ofproto/ofproto-dpif.c  |  24 +++
>>  ofproto/ofproto-dpif.h  |   2 +
>>  ofproto/ofproto-provider.h  |   1 +
>>  ofproto/ofproto.c   |  85 
>>  tests/ofproto-dpif.at   |  47 ++
>>  tests/ofproto-macros.at |   9 ++-
>>  10 files changed, 285 insertions(+), 88 deletions(-)
>>
>> diff --git a/NEWS b/NEWS
>> index 5ae0108d552b..1bc085f97045 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -9,6 +9,9 @@ Post-v3.3.0
>>   https://github.com/openvswitch/ovs.git
>> - DPDK:
>>   * OVS validated with DPDK 23.11.1.
>> +   - ovs-appctl:
>> + * Added 'upcall/dump-ufid-rules' to output the set of OpenFlow rules 
>> and
>> +   groups that contributed to the creation of a specific datapath flow.
> 
> I'd move this to the top, alpabetically.  I know that some entries
> here are not in order, but it's better to keep them at least in
> approximate order.
> 

Ack.

>>  
>>  
>>  v3.3.0 - 16 Feb 2024
>> diff --git a/include/openvswitch/ofp-group.h 
>> b/include/openvswitch/ofp-group.h
>> index cd7af0ebff9c..79fcb3a4c0d1 100644
>> --- a/include/openvswitch/ofp-group.h
>> +++ b/include/openvswitch/ofp-group.h
>> @@ -70,6 +70,11 @@ st

Re: [ovs-dev] [PATCH net-next v2] selftests: openvswitch: retry instead of sleep

[Ilya Maximets]

On 7/10/24 11:04, Adrian Moreno wrote:
> There are a couple of places where the test script "sleep"s to wait for
> some external condition to be met.
> 
> This is error prone, specially in slow systems (identified in CI by
> "KSFT_MACHINE_SLOW=yes").
> 
> To fix this, add a "ovs_wait" function that tries to execute a command
> a few times until it succeeds. The timeout used is set to 5s for
> "normal" systems and doubled if a slow CI machine is detected.
> 
> This should make the following work:
> 
> $ vng --build  \
> --config tools/testing/selftests/net/config \
> --config kernel/configs/debug.config
> 
> $ vng --run . --user root -- "make -C tools/testing/selftests/ \
> KSFT_MACHINE_SLOW=yes TARGETS=net/openvswitch run_tests"
> 
> Signed-off-by: Adrian Moreno 
> ---
>  .../selftests/net/openvswitch/openvswitch.sh  | 45 +++
>  .../selftests/net/openvswitch/ovs-dpctl.py|  1 +
>  2 files changed, 38 insertions(+), 8 deletions(-)

Seem like we don't have a signal from CI for some reason yet,
but I tested this locally and it seem to work fine.  Either
way it's a better way of doing things than sleep'n'hope.

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

Re: [ovs-dev] [PATCH v10 2/2] ofproto-dpif-mirror: Add support for pre-selection filter.

[Eelco Chaudron]

On 8 Jul 2024, at 20:27, Mike Pattrick wrote:

> Currently a bridge mirror will collect all packets and tools like
> ovs-tcpdump can apply additional filters after they have already been
> duplicated by vswitchd. This can result in inefficient collection.
>
> This patch adds support to apply pre-selection to bridge mirrors, which
> can limit which packets are mirrored based on flow metadata. This
> significantly improves overall vswitchd performance during mirroring if
> only a subset of traffic is required.
>
> Signed-off-by: Mike Pattrick 

Thanks Mike for making the changes, the patch looks good to me.

Ilya are you planning to review this also? Let me know, or else I can apply 
this before my PTO.

Cheers,

Eelco

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

Re: [ovs-dev] [Patch ovn] actions: Explicitly finish CT actions.

[Ales Musil]

On Thu, Jul 11, 2024 at 11:37 AM Martin Kalcok 
wrote:

> As discussed here [0], a couple of functions that encode
> CT-related actions were using older, manual, way of finishing
> the action.
>
> As amusil mentioned here [1], there's a shorter and more explicit
> way of doing it. This change replaces manual way with the more
> explicit aproach.
>
> [0] https://mail.openvswitch.org/pipermail/ovs-dev/2024-June/414667.html
> [1] https://mail.openvswitch.org/pipermail/ovs-dev/2024-April/413317.html
>
> Signed-off-by: Martin Kalcok 
> ---
>

Hi Martin,

thank you for the follow up. You have missed encode_CT_NEXT() and
add_lb_ct_snat_hairpin_vip_flow() in lflow.c I have also a couple of
comments down below.


 lib/actions.c | 36 +---
>  1 file changed, 13 insertions(+), 23 deletions(-)
>
> diff --git a/lib/actions.c b/lib/actions.c
> index e8cc0994d..51da6210f 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -761,7 +761,6 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on,
>  struct ofpbuf *ofpacts)
>  {
>  size_t ct_offset = ofpacts->size;
> -ofpbuf_pull(ofpacts, ct_offset);
>
>  struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
>  ct->flags = NX_CT_F_COMMIT;
> @@ -777,24 +776,19 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on,
>   */
>  if (ovs_feature_is_supported(OVS_CT_ZERO_SNAT_SUPPORT)) {
>  size_t nat_offset = ofpacts->size;
>



> -ofpbuf_pull(ofpacts, nat_offset);
>
>  struct ofpact_nat *nat = ofpact_put_NAT(ofpacts);
>  nat->flags = 0;
>  nat->range_af = AF_UNSPEC;
>  nat->flags |= NX_NAT_F_SRC;
> -ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset);
> -ct = ofpacts->header;
> +ct = ofpbuf_at_assert(ofpacts, nat_offset, sizeof *ct);
>

This is not needed, it can be removed along with the nat_offset.


>  }
>
> -size_t set_field_offset = ofpacts->size;
> -ofpbuf_pull(ofpacts, set_field_offset);
> -
>  ovnacts_encode(on->nested, on->nested_len, ep, ofpacts);
> -ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset);
> -ct = ofpacts->header;
> -ofpact_finish(ofpacts, &ct->ofpact);
> -ofpbuf_push_uninit(ofpacts, ct_offset);
> +
> +ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct);
> +ofpacts->header = ct;
> +ofpact_finish_CT(ofpacts, &ct);
>  }
>
>  static void
> @@ -1027,7 +1021,6 @@ encode_ct_nat(const struct ovnact_ct_nat *cn,
>enum mf_field_id zone_src, struct ofpbuf *ofpacts)
>  {
>  const size_t ct_offset = ofpacts->size;
> -ofpbuf_pull(ofpacts, ct_offset);
>
>  struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
>  ct->recirc_table = cn->ltable + first_ptable(ep, ep->pipeline);
> @@ -1040,7 +1033,6 @@ encode_ct_nat(const struct ovnact_ct_nat *cn,
>  struct ofpact_nat *nat;
>  size_t nat_offset;
>  nat_offset = ofpacts->size;
> -ofpbuf_pull(ofpacts, nat_offset);
>
>  nat = ofpact_put_NAT(ofpacts);
>  nat->range_af = cn->family;
> @@ -1081,13 +1073,13 @@ encode_ct_nat(const struct ovnact_ct_nat *cn,
>  }
>  }
>
> -ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset);
> -ct = ofpacts->header;
> +ct = ofpbuf_at_assert(ofpacts, nat_offset, sizeof *ct);
> +ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct);
>  if (cn->commit) {
>  ct->flags |= NX_CT_F_COMMIT;
>  }
> -ofpact_finish(ofpacts, &ct->ofpact);
> -ofpbuf_push_uninit(ofpacts, ct_offset);
> +ofpacts->header = ct;
> +ofpact_finish_CT(ofpacts, &ct);
>

This can be simplified just by moving the cn->commit if below
the other init of ct at the start of the function.


>  }
>
>  static void
> @@ -1383,7 +1375,6 @@ encode_ct_lb(const struct ovnact_ct_lb *cl,
>  /* ct_lb without any destinations means that this is an
> established
>   * connection and we just need to do a NAT. */
>  const size_t ct_offset = ofpacts->size;
> -ofpbuf_pull(ofpacts, ct_offset);
>
>  struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
>  struct ofpact_nat *nat;
> @@ -1397,16 +1388,15 @@ encode_ct_lb(const struct ovnact_ct_lb *cl,
>  ct->alg = 0;
>
>  nat_offset = ofpacts->size;
> -ofpbuf_pull(ofpacts, nat_offset);
>
>  nat = ofpact_put_NAT(ofpacts);
>  nat->flags = 0;
>  nat->range_af = AF_UNSPEC;
>
> -ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset);
> -ct = ofpacts->header;
> -ofpact_finish(ofpacts, &ct->ofpact);
> -ofpbuf_push_uninit(ofpacts, ct_offset);
> +ct = ofpbuf_at_assert(ofpacts, nat_offset, sizeof *ct);
>

Once again the nat_offset is not needed. The assert with ct_offset is
enough.

+ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct);
> +ofpacts->header = ct;
> +ofpact_finish_CT(ofpacts, &ct);
>  return;
>  }
>
> --
> 2.40.1
>
> _

Re: [ovs-dev] [PATCH net-next v2] selftests: openvswitch: retry instead of sleep

[Adrián Moreno]

On Wed, Jul 10, 2024 at 11:04:59AM GMT, Adrian Moreno wrote:
> There are a couple of places where the test script "sleep"s to wait for
> some external condition to be met.
>
> This is error prone, specially in slow systems (identified in CI by
> "KSFT_MACHINE_SLOW=yes").
>
> To fix this, add a "ovs_wait" function that tries to execute a command
> a few times until it succeeds. The timeout used is set to 5s for
> "normal" systems and doubled if a slow CI machine is detected.
>
> This should make the following work:
>
> $ vng --build  \
> --config tools/testing/selftests/net/config \
> --config kernel/configs/debug.config
>
> $ vng --run . --user root -- "make -C tools/testing/selftests/ \
> KSFT_MACHINE_SLOW=yes TARGETS=net/openvswitch run_tests"
>
> Signed-off-by: Adrian Moreno 
> ---
>  .../selftests/net/openvswitch/openvswitch.sh  | 45 +++
>  .../selftests/net/openvswitch/ovs-dpctl.py|  1 +
>  2 files changed, 38 insertions(+), 8 deletions(-)
>
> diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh 
> b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> index bc71dbc18b21..cc0bfae2bafa 100755
> --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> @@ -11,6 +11,11 @@ ksft_skip=4
>  PAUSE_ON_FAIL=no
>  VERBOSE=0
>  TRACING=0
> +WAIT_TIMEOUT=5
> +
> +if test "X$KSFT_MACHINE_SLOW" == "Xyes"; then
> + WAIT_TIMEOUT=10
> +fi
>
>  tests="
>   arp_pingeth-arp: Basic arp ping between 
> two NS
> @@ -29,6 +34,30 @@ info() {
>   [ $VERBOSE = 0 ] || echo $*
>  }
>
> +ovs_wait() {
> + info "waiting $WAIT_TIMEOUT s for: $@"
> +
> + if "$@" ; then
> + info "wait succeeded immediately"
> + return 0
> + fi
> +
> + # A quick re-check helps speed up small races in fast systems.
> + # However, fractional sleeps might not necessarily work.
> + local start=0
> + sleep 0.1 || { sleep 1; start=1; }
> +
> + for (( i=start; i + if "$@" ; then
> + info "wait succeeded after $i seconds"
> + return 0
> + fi
> + sleep 1
> + done
> + info "wait failed after $i seconds"
> + return 1
> +}
> +
>  ovs_base=`pwd`
>  sbxs=
>  sbx_add () {
> @@ -278,20 +307,19 @@ test_psample() {
>
>   # Record psample data.
>   ovs_spawn_daemon "test_psample" python3 $ovs_base/ovs-dpctl.py 
> psample-events
> + ovs_wait grep -q "listening for psample events" ${ovs_dir}/stdout
>
>   # Send a single ping.
> - sleep 1
>   ovs_sbx "test_psample" ip netns exec client ping -I c1 172.31.110.20 -c 
> 1 || return 1
> - sleep 1
>
>   # We should have received one userspace action upcall and 2 psample 
> packets.
> - grep -E "userspace action command" $ovs_dir/s0.out >/dev/null 2>&1 || 
> return 1
> + ovs_wait grep -q "userspace action command" $ovs_dir/s0.out || return 1
>
>   # client -> server samples should only contain the first 14 bytes of 
> the packet.
> - grep -E "rate:4294967295,group:1,cookie:c0ffee data:[0-9a-f]{28}$" \
> -  $ovs_dir/stdout >/dev/null 2>&1 || return 1
> - grep -E "rate:4294967295,group:2,cookie:eeff0c" \
> -  $ovs_dir/stdout >/dev/null 2>&1 || return 1
> + ovs_wait grep -qE "rate:4294967295,group:1,cookie:c0ffee 
> data:[0-9a-f]{28}$" \
> + $ovs_dir/stdout || return 1
> +
> + ovs_wait grep -q "rate:4294967295,group:2,cookie:eeff0c" 
> $ovs_dir/stdout || return 1
>
>   return 0
>  }
> @@ -711,7 +739,8 @@ test_upcall_interfaces() {
>   ovs_add_netns_and_veths "test_upcall_interfaces" ui0 upc left0 l0 \
>   172.31.110.1/24 -u || return 1
>
> - sleep 1
> + ovs_wait grep -q "listening on upcall packet handler" 
> ${ovs_dir}/left0.out
> +
>   info "sending arping"
>   ip netns exec upc arping -I l0 172.31.110.20 -c 1 \
>   >$ovs_dir/arping.stdout 2>$ovs_dir/arping.stderr
> diff --git a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py 
> b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> index 1e15b0818074..8a0396bfaf99 100644
> --- a/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> +++ b/tools/testing/selftests/net/openvswitch/ovs-dpctl.py
> @@ -2520,6 +2520,7 @@ class PsampleEvent(EventSocket):
>  marshal_class = psample_msg
>
>  def read_samples(self):
> +print("listening for psample events", flush=True)
>  while True:
>  try:
>  for msg in self.get():
> --
> 2.45.2
>


This patch is supposed to fix openvswitch selftests on "-dbg" machines.
However, as Simon points out, all recent rounds are failing [1]. I don't
see this patch being included in the batches and I was wondering why.

Also I see a (presumably unrelated) build error netdev/build_32bit.
Is there anything I can do?

[1]
https://netdev.bots.linux.dev/contest.htm

[ovs-dev] [Patch ovn] actions: Explicitly finish CT actions.

[Martin Kalcok]

As discussed here [0], a couple of functions that encode
CT-related actions were using older, manual, way of finishing
the action.

As amusil mentioned here [1], there's a shorter and more explicit
way of doing it. This change replaces manual way with the more
explicit aproach.

[0] https://mail.openvswitch.org/pipermail/ovs-dev/2024-June/414667.html
[1] https://mail.openvswitch.org/pipermail/ovs-dev/2024-April/413317.html

Signed-off-by: Martin Kalcok 
---
 lib/actions.c | 36 +---
 1 file changed, 13 insertions(+), 23 deletions(-)

diff --git a/lib/actions.c b/lib/actions.c
index e8cc0994d..51da6210f 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -761,7 +761,6 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on,
 struct ofpbuf *ofpacts)
 {
 size_t ct_offset = ofpacts->size;
-ofpbuf_pull(ofpacts, ct_offset);
 
 struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
 ct->flags = NX_CT_F_COMMIT;
@@ -777,24 +776,19 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on,
  */
 if (ovs_feature_is_supported(OVS_CT_ZERO_SNAT_SUPPORT)) {
 size_t nat_offset = ofpacts->size;
-ofpbuf_pull(ofpacts, nat_offset);
 
 struct ofpact_nat *nat = ofpact_put_NAT(ofpacts);
 nat->flags = 0;
 nat->range_af = AF_UNSPEC;
 nat->flags |= NX_NAT_F_SRC;
-ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset);
-ct = ofpacts->header;
+ct = ofpbuf_at_assert(ofpacts, nat_offset, sizeof *ct);
 }
 
-size_t set_field_offset = ofpacts->size;
-ofpbuf_pull(ofpacts, set_field_offset);
-
 ovnacts_encode(on->nested, on->nested_len, ep, ofpacts);
-ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset);
-ct = ofpacts->header;
-ofpact_finish(ofpacts, &ct->ofpact);
-ofpbuf_push_uninit(ofpacts, ct_offset);
+
+ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct);
+ofpacts->header = ct;
+ofpact_finish_CT(ofpacts, &ct);
 }
 
 static void
@@ -1027,7 +1021,6 @@ encode_ct_nat(const struct ovnact_ct_nat *cn,
   enum mf_field_id zone_src, struct ofpbuf *ofpacts)
 {
 const size_t ct_offset = ofpacts->size;
-ofpbuf_pull(ofpacts, ct_offset);
 
 struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
 ct->recirc_table = cn->ltable + first_ptable(ep, ep->pipeline);
@@ -1040,7 +1033,6 @@ encode_ct_nat(const struct ovnact_ct_nat *cn,
 struct ofpact_nat *nat;
 size_t nat_offset;
 nat_offset = ofpacts->size;
-ofpbuf_pull(ofpacts, nat_offset);
 
 nat = ofpact_put_NAT(ofpacts);
 nat->range_af = cn->family;
@@ -1081,13 +1073,13 @@ encode_ct_nat(const struct ovnact_ct_nat *cn,
 }
 }
 
-ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset);
-ct = ofpacts->header;
+ct = ofpbuf_at_assert(ofpacts, nat_offset, sizeof *ct);
+ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct);
 if (cn->commit) {
 ct->flags |= NX_CT_F_COMMIT;
 }
-ofpact_finish(ofpacts, &ct->ofpact);
-ofpbuf_push_uninit(ofpacts, ct_offset);
+ofpacts->header = ct;
+ofpact_finish_CT(ofpacts, &ct);
 }
 
 static void
@@ -1383,7 +1375,6 @@ encode_ct_lb(const struct ovnact_ct_lb *cl,
 /* ct_lb without any destinations means that this is an established
  * connection and we just need to do a NAT. */
 const size_t ct_offset = ofpacts->size;
-ofpbuf_pull(ofpacts, ct_offset);
 
 struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
 struct ofpact_nat *nat;
@@ -1397,16 +1388,15 @@ encode_ct_lb(const struct ovnact_ct_lb *cl,
 ct->alg = 0;
 
 nat_offset = ofpacts->size;
-ofpbuf_pull(ofpacts, nat_offset);
 
 nat = ofpact_put_NAT(ofpacts);
 nat->flags = 0;
 nat->range_af = AF_UNSPEC;
 
-ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset);
-ct = ofpacts->header;
-ofpact_finish(ofpacts, &ct->ofpact);
-ofpbuf_push_uninit(ofpacts, ct_offset);
+ct = ofpbuf_at_assert(ofpacts, nat_offset, sizeof *ct);
+ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct);
+ofpacts->header = ct;
+ofpact_finish_CT(ofpacts, &ct);
 return;
 }
 
-- 
2.40.1

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

[ovs-dev] [PATCH ovn] ci: Use compressed format for podman save.

[Ales Musil]

The default format "docker-archive" is just pure tar without any
compression. Use "oci-archive" instead which compresses the layers
and saves some space in the cache which translates to time save
during the upload/download.

Signed-off-by: Ales Musil 
---
 .cirrus.yml| 2 +-
 .github/workflows/ovn-fake-multinode-tests.yml | 2 +-
 .github/workflows/test.yml | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/.cirrus.yml b/.cirrus.yml
index 0a16d7b5f..9b35d4cfb 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -15,7 +15,7 @@ build_image_task:
   build_container_script:
 - cd utilities/containers
 - make ubuntu
-- podman save -o /tmp/image.tar ovn-org/ovn-tests:ubuntu
+- podman save -o /tmp/image.tar --format oci-archive 
ovn-org/ovn-tests:ubuntu
 
   upload_image_script:
 - curl -s -X POST -T /tmp/image.tar 
http://$CIRRUS_HTTP_CACHE_HOST/${CIRRUS_CHANGE_IN_REPO}
diff --git a/.github/workflows/ovn-fake-multinode-tests.yml 
b/.github/workflows/ovn-fake-multinode-tests.yml
index 5c9030a49..b83bfce36 100644
--- a/.github/workflows/ovn-fake-multinode-tests.yml
+++ b/.github/workflows/ovn-fake-multinode-tests.yml
@@ -60,7 +60,7 @@ jobs:
 sudo -E ./ovn_cluster.sh build
 mkdir -p /tmp/_output
 sudo podman tag ovn/ovn-multi-node:latest ovn/ovn-multi-node:${{ 
matrix.cfg.branch }}
-sudo podman save ovn/ovn-multi-node:${{ matrix.cfg.branch }} > 
/tmp/_output/ovn_${{ matrix.cfg.branch }}_image.tar
+sudo podman save --format oci-archive ovn/ovn-multi-node:${{ 
matrix.cfg.branch }} > /tmp/_output/ovn_${{ matrix.cfg.branch }}_image.tar
   working-directory: ovn-fake-multinode
 
 - uses: actions/upload-artifact@v4
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index befa0bfac..0342c3dcf 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -56,7 +56,7 @@ jobs:
 run: podman pull ghcr.io/ovn-org/ovn-tests:${{ env.IMAGE_DISTRO }}
 
   - name: Export image
-run: podman save -o /tmp/image.tar ovn-org/ovn-tests:${{ 
env.IMAGE_DISTRO }}
+run: podman save -o /tmp/image.tar --format oci-archive 
ovn-org/ovn-tests:${{ env.IMAGE_DISTRO }}
 
   - name: Cache image
 id: image_cache
-- 
2.45.2

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

Re: [ovs-dev] [PATCH v1 13/13] ofp-actions: Load data from fields in sample action.

[Adrián Moreno]

On Thu, Jul 11, 2024 at 12:32:06AM GMT, Ilya Maximets wrote:
> On 7/10/24 23:38, Adrián Moreno wrote:
> > On Wed, Jul 10, 2024 at 11:00:43PM GMT, Ilya Maximets wrote:
> >> On 7/7/24 22:09, Adrian Moreno wrote:
> >>> When sample action gets used as a way of sampling traffic with
> >>> controller-generated metadata (i.e: obs_domain_id and obs_point_id),
> >>> the controller will have to increase the number of flows to ensure each
> >>> part of the pipeline contains the right metadata.
> >>>
> >>> As an example, if the controller decides to sample stateful traffic, it
> >>> could store the computed metadata for each connection in the conntrack
> >>> label. However, for established connections, a flow must be created for
> >>> each different ct_label value with a sample action that contains a
> >>> different hardcoded obs_domain and obs_point id.
> >>>
> >>> This patch adds a new version of the NXAST_RAW_SAMPLE* action (number 4)
> >>> that supports specifying the observation point and domain using an
> >>> OpenFlow field reference, so now the controller can express:
> >>>
> >>>  sample(...
> >>> obs_domain_id=NXM_NX_CT_LABEL[0..31],
> >>> obs_point_id=NXM_NX_CT_LABEL[32..63]
> >>> ...
> >>>)
> >>>
> >>> Signed-off-by: Adrian Moreno 
> >>> ---
> >>>  include/openvswitch/ofp-actions.h |   8 +-
> >>>  lib/ofp-actions.c | 249 +++---
> >>>  ofproto/ofproto-dpif-xlate.c  |  55 ++-
> >>>  python/ovs/flow/ofp.py|   8 +-
> >>>  python/ovs/flow/ofp_act.py|   4 +-
> >>>  tests/ofp-actions.at  |   5 +
> >>>  tests/ofproto-dpif.at |  41 +
> >>>  tests/system-traffic.at   |  74 +
> >>>  8 files changed, 405 insertions(+), 39 deletions(-)
> >>
> >> Not a full review, it's a complicated change.  See a few comments below.
> >>
> >>>
> >>> diff --git a/include/openvswitch/ofp-actions.h 
> >>> b/include/openvswitch/ofp-actions.h
> >>> index 7b57e49ad..56dc2c147 100644
> >>> --- a/include/openvswitch/ofp-actions.h
> >>> +++ b/include/openvswitch/ofp-actions.h
> >>> @@ -1015,14 +1015,16 @@ enum nx_action_sample_direction {
> >>>
> >>>  /* OFPACT_SAMPLE.
> >>>   *
> >>> - * Used for NXAST_SAMPLE, NXAST_SAMPLE2, and NXAST_SAMPLE3. */
> >>> + * Used for NXAST_SAMPLE, NXAST_SAMPLE2, NXAST_SAMPLE3 and 
> >>> NXAST_SAMPLE4. */
> >>>  struct ofpact_sample {
> >>>  OFPACT_PADDED_MEMBERS(
> >>>  struct ofpact ofpact;
> >>>  uint16_t probability;   /* Always positive. */
> >>>  uint32_t collector_set_id;
> >>> -uint32_t obs_domain_id;
> >>> -uint32_t obs_point_id;
> >>> +uint32_t obs_domain_imm;
> >>> +struct mf_subfield obs_domain_src;
> >>> +uint32_t obs_point_imm;
> >>> +struct mf_subfield obs_point_src;
> >>>  ofp_port_t sampling_port;
> >>>  enum nx_action_sample_direction direction;
> >>>  );
> >>> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> >>> index da7b1dd31..e329a7e3f 100644
> >>> --- a/lib/ofp-actions.c
> >>> +++ b/lib/ofp-actions.c
> >>> @@ -330,6 +330,8 @@ enum ofp_raw_action_type {
> >>>  NXAST_RAW_SAMPLE2,
> >>>  /* NX1.0+(41): struct nx_action_sample2. */
> >>>  NXAST_RAW_SAMPLE3,
> >>> +/* NX1.0+(51): struct nx_action_sample4, ... VLMFF */
> >>
> >> Why the dots are here?  The structure doesn't seem to have
> >> extra fields at the end.
> >
> > I missread the description then. I thought it was just about alignment.
> >
> >>
> >>> +NXAST_RAW_SAMPLE4,
> >>>
> >>>  /* NX1.0+(34): struct nx_action_conjunction. */
> >>>  NXAST_RAW_CONJUNCTION,
> >>> @@ -6188,6 +6190,34 @@ struct nx_action_sample2 {
> >>>   };
> >>>   OFP_ASSERT(sizeof(struct nx_action_sample2) == 32);
> >>>
> >>> +/* Action structure for NXAST_SAMPLE4
> >>> + *
> >>> + * NXAST_SAMPLE4 was added in Open vSwitch 3.4.0.  Compared to 
> >>> NXAST_SAMPLE3,
> >>> + * it adds support for using field specifiers for observation_domain_id 
> >>> and
> >>> + * observation_point_id. */
> >>> +struct nx_action_sample4 {
> >>> +ovs_be16 type;  /* OFPAT_VENDOR. */
> >>> +ovs_be16 len;   /* Length is 32. */
> >>
> >> Is the length 32?  40, I suppose.
> >
> > Yep
> >
> >>
> >>> +ovs_be32 vendor;/* NX_VENDOR_ID. */
> >>> +ovs_be16 subtype;   /* NXAST_SAMPLE. */
> >>
> >> NXAST_SAMPLE4 ?
> >
> > Ack.
> >
> >>
> >>> +ovs_be16 probability;   /* Fraction of packets to sample. */
> >>> +ovs_be32 collector_set_id;  /* ID of collector set in OVSDB. */
> >>> +ovs_be32 obs_domain_src;/* Source of the 
> >>> observation_domain_id. */
> >>> +union {
> >>> +ovs_be16 obs_domain_ofs_nbits;  /* Range to use from source 
> >>> field. */
> >>> +ovs_be32 obs_domain_imm;/* Immediate value for domain 
> >>> id. */
> >>> +};
> >>> +ovs_be32 obs_point_src; /* Source of the 
> >>>