Re: [ovs-dev] [PATCH ovn 3/3] northd: Incremental processing of VIF updates and deletions in 'lflow' node.

[Numan Siddique]

On Sat, Jul 1, 2023 at 7:33 AM Han Zhou  wrote:
>
> On Thu, Jun 29, 2023 at 10:54 PM Numan Siddique  wrote:
> >
> > On Sun, Jun 18, 2023 at 11:48 AM Han Zhou  wrote:
> > >
> > > This patch achieves zero recompute for VIF updates and deletions in
> > > common scenarios. The performance gain for these scenarios is similar to
> > > the patch "northd: Incremental processing of VIF additions in 'lflow'
> > > node."
> > >
> > > Signed-off-by: Han Zhou 
> > > ---
> > >  northd/northd.c | 231 +---
> > >  tests/ovn-northd.at |   4 +-
> > >  2 files changed, 154 insertions(+), 81 deletions(-)
> > >
> > > diff --git a/northd/northd.c b/northd/northd.c
> > > index aa0f853ce2db..c0c948fcea4b 100644
> > > --- a/northd/northd.c
> > > +++ b/northd/northd.c
> > > @@ -16303,6 +16303,113 @@ void build_lflows(struct ovsdb_idl_txn
> *ovnsb_txn,
> > >  hmap_destroy(_groups);
> > >  }
> > >
> > > +static void
> > > +sync_lsp_lflows_to_sb(struct ovsdb_idl_txn *ovnsb_txn,
> > > +  struct lflow_input *lflow_input,
> > > +  struct hmap *lflows,
> > > +  struct ovn_lflow *lflow)
> > > +{
> > > +size_t n_datapaths;
> > > +struct ovn_datapath **datapaths_array;
> > > +if (ovn_stage_to_datapath_type(lflow->stage) == DP_SWITCH) {
> > > +n_datapaths = ods_size(lflow_input->ls_datapaths);
> > > +datapaths_array = lflow_input->ls_datapaths->array;
> > > +} else {
> > > +n_datapaths = ods_size(lflow_input->lr_datapaths);
> > > +datapaths_array = lflow_input->lr_datapaths->array;
> > > +}
> > > +uint32_t n_ods = bitmap_count1(lflow->dpg_bitmap, n_datapaths);
> > > +ovs_assert(n_ods == 1);
> > > +/* There is only one datapath, so it should be moved out of the
> > > + * group to a single 'od'. */
> > > +size_t index = bitmap_scan(lflow->dpg_bitmap, true, 0,
> > > +   n_datapaths);
> > > +
> > > +bitmap_set0(lflow->dpg_bitmap, index);
> > > +lflow->od = datapaths_array[index];
> > > +
> > > +/* Logical flow should be re-hashed to allow lookups. */
> > > +uint32_t hash = hmap_node_hash(>hmap_node);
> > > +/* Remove from lflows. */
> > > +hmap_remove(lflows, >hmap_node);
> > > +hash = ovn_logical_flow_hash_datapath(>od->sb->header_.uuid,
> > > +  hash);
> > > +/* Add back. */
> > > +hmap_insert(lflows, >hmap_node, hash);
> > > +
> > > +/* Sync to SB. */
> > > +const struct sbrec_logical_flow *sbflow;
> > > +lflow->sb_uuid = uuid_random();
> > > +sbflow = sbrec_logical_flow_insert_persist_uuid(ovnsb_txn,
> > > +>sb_uuid);
> > > +const char *pipeline = ovn_stage_get_pipeline_name(lflow->stage);
> > > +uint8_t table = ovn_stage_get_table(lflow->stage);
> > > +sbrec_logical_flow_set_logical_datapath(sbflow, lflow->od->sb);
> > > +sbrec_logical_flow_set_logical_dp_group(sbflow, NULL);
> > > +sbrec_logical_flow_set_pipeline(sbflow, pipeline);
> > > +sbrec_logical_flow_set_table_id(sbflow, table);
> > > +sbrec_logical_flow_set_priority(sbflow, lflow->priority);
> > > +sbrec_logical_flow_set_match(sbflow, lflow->match);
> > > +sbrec_logical_flow_set_actions(sbflow, lflow->actions);
> > > +if (lflow->io_port) {
> > > +struct smap tags = SMAP_INITIALIZER();
> > > +smap_add(, "in_out_port", lflow->io_port);
> > > +sbrec_logical_flow_set_tags(sbflow, );
> > > +smap_destroy();
> > > +}
> > > +sbrec_logical_flow_set_controller_meter(sbflow, lflow->ctrl_meter);
> > > +/* Trim the source locator lflow->where, which looks something like
> > > + * "ovn/northd/northd.c:1234", down to just the part following the
> > > + * last slash, e.g. "northd.c:1234". */
> > > +const char *slash = strrchr(lflow->where, '/');
> > > +#if _WIN32
> > > +const char *backslash = strrchr(lflow->where, '\\');
> > > +if (!slash || backslash > slash) {
> > > +slash = backslash;
> > > +}
> > > +#endif
> > > +const char *where = slash ? slash + 1 : lflow->where;
> > > +
> > > +struct smap ids = SMAP_INITIALIZER();
> > > +smap_add(, "stage-name", ovn_stage_to_str(lflow->stage));
> > > +smap_add(, "source", where);
> > > +if (lflow->stage_hint) {
> > > +smap_add(, "stage-hint", lflow->stage_hint);
> > > +}
> > > +sbrec_logical_flow_set_external_ids(sbflow, );
> > > +smap_destroy();
> > > +}
> > > +
> > > +static bool
> > > +delete_lflow_for_lsp(struct ovn_port *op, bool is_update,
> > > + const struct sbrec_logical_flow_table
> *sb_lflow_table,
> > > + struct hmap *lflows)
> > > +{
> > > +struct lflow_ref_node *lfrn;
> > > +const char *operation = is_update ? "updated" : "deleted";
> > > +LIST_FOR_EACH_SAFE (lfrn, lflow_list_node, >lflows) {
> 

Re: [ovs-dev] [ovs-dev v12] ofproto-dpif-upcall: fix push_dp_ops

[0-day Robot]

Bleep bloop.  Greetings Peng He, 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:
ERROR: Author Peng He  needs to sign off.
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Peng He 
Lines checked: 204, 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] [ovs-dev v12] ofproto-dpif-upcall: fix push_dp_ops

[Peng He]

push_dp_ops only handles delete ops errors but ignores the modify
ops results. It's better to handle all the dp operation errors in
a consistent way.

This patch prevents the inconsistency by considering modify failure
in revalidators.

To note, we cannot perform two state transitions and change ukey_state
into UKEY_EVICTED directly here, because, if we do so, the
sweep will remove the ukey alone and leave dp flow alive. Later, the
dump will retrieve the dp flow and might even recover it. This will
contribute the stats of this dp flow twice.

v8->v9:   add testsuite and delete INCONSISTENT ukey at revalidate_sweep.
v9->v10:  change the commit message and refine the test case.
v10->v11: fix indentation and refine the test case.
v11->v12: fix the test suite.

Signed-off-by: Peng He 
---
 ofproto/ofproto-dpif-upcall.c | 50 +--
 tests/dpif-netdev.at  | 45 +++
 2 files changed, 81 insertions(+), 14 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 04b583f81..cde03abc6 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -53,6 +53,7 @@
 VLOG_DEFINE_THIS_MODULE(ofproto_dpif_upcall);
 
 COVERAGE_DEFINE(dumped_duplicate_flow);
+COVERAGE_DEFINE(dumped_inconsistent_flow);
 COVERAGE_DEFINE(dumped_new_flow);
 COVERAGE_DEFINE(handler_duplicate_upcall);
 COVERAGE_DEFINE(revalidate_missed_dp_flow);
@@ -258,6 +259,7 @@ enum ukey_state {
 UKEY_CREATED = 0,
 UKEY_VISIBLE,   /* Ukey is in umap, datapath flow install is queued. */
 UKEY_OPERATIONAL,   /* Ukey is in umap, datapath flow is installed. */
+UKEY_INCONSISTENT,  /* Ukey is in umap, datapath flow is inconsistent. */
 UKEY_EVICTING,  /* Ukey is in umap, datapath flow delete is queued. */
 UKEY_EVICTED,   /* Ukey is in umap, datapath flow is deleted. */
 UKEY_DELETED,   /* Ukey removed from umap, ukey free is deferred. */
@@ -1999,6 +2001,10 @@ transition_ukey_at(struct udpif_key *ukey, enum 
ukey_state dst,
  * UKEY_VISIBLE -> UKEY_EVICTED
  *  A handler attempts to install the flow, but the datapath rejects it.
  *  Consider that the datapath has already destroyed it.
+ * UKEY_OPERATIONAL -> UKEY_INCONSISTENT
+ *  A revalidator modifies the flow with error returns.
+ * UKEY_INCONSISTENT -> UKEY_EVICTING
+ *  A revalidator decides to evict the datapath flow.
  * UKEY_OPERATIONAL -> UKEY_EVICTING
  *  A revalidator decides to evict the datapath flow.
  * UKEY_EVICTING-> UKEY_EVICTED
@@ -2006,8 +2012,9 @@ transition_ukey_at(struct udpif_key *ukey, enum 
ukey_state dst,
  * UKEY_EVICTED -> UKEY_DELETED
  *  A revalidator has removed the ukey from the umap and is deleting it.
  */
-if (ukey->state == dst - 1 || (ukey->state == UKEY_VISIBLE &&
-   dst < UKEY_DELETED)) {
+if (ukey->state == dst - 1 ||
+   (ukey->state == UKEY_VISIBLE && dst < UKEY_DELETED) ||
+   (ukey->state == UKEY_OPERATIONAL && dst == UKEY_EVICTING)) {
 ukey->state = dst;
 } else {
 struct ds ds = DS_EMPTY_INITIALIZER;
@@ -2490,26 +2497,31 @@ push_dp_ops(struct udpif *udpif, struct ukey_op *ops, 
size_t n_ops)
 
 for (i = 0; i < n_ops; i++) {
 struct ukey_op *op = [i];
-struct dpif_flow_stats *push, *stats, push_buf;
-
-stats = op->dop.flow_del.stats;
-push = _buf;
-
-if (op->dop.type != DPIF_OP_FLOW_DEL) {
-/* Only deleted flows need their stats pushed. */
-continue;
-}
 
 if (op->dop.error) {
-/* flow_del error, 'stats' is unusable. */
 if (op->ukey) {
 ovs_mutex_lock(>ukey->mutex);
-transition_ukey(op->ukey, UKEY_EVICTED);
+if (op->dop.type == DPIF_OP_FLOW_DEL) {
+transition_ukey(op->ukey, UKEY_EVICTED);
+} else {
+/* Modification of the flow failed. */
+transition_ukey(op->ukey, UKEY_INCONSISTENT);
+}
 ovs_mutex_unlock(>ukey->mutex);
 }
 continue;
 }
 
+if (op->dop.type != DPIF_OP_FLOW_DEL) {
+/* Only deleted flows need their stats pushed. */
+continue;
+}
+
+struct dpif_flow_stats *push, *stats, push_buf;
+
+stats = op->dop.flow_del.stats;
+push = _buf;
+
 if (op->ukey) {
 ovs_mutex_lock(>ukey->mutex);
 transition_ukey(op->ukey, UKEY_EVICTED);
@@ -2857,6 +2869,15 @@ revalidate(struct revalidator *revalidator)
 continue;
 }
 
+if (ukey->state == UKEY_INCONSISTENT) {
+ukey->dump_seq = dump_seq;
+reval_op_init([n_ops++], UKEY_DELETE, udpif, ukey,
+  , _actions);
+ovs_mutex_unlock(>mutex);
+  

Re: [ovs-dev] [ovs-dev v12] ofproto-dpif-upcall: fix push_dp_ops

[0-day Robot]

Bleep bloop.  Greetings Peng He, 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 ofproto-dpif-upcall: fix push_dp_ops
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

Re: [ovs-dev] [ovs-dev v12] ofproto-dpif-upcall: fix push_dp_ops

[0-day Robot]

Bleep bloop.  Greetings Peng He, 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 ofproto-dpif-upcall: fix push_dp_ops
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] [ovs-dev v12] ofproto-dpif-upcall: fix push_dp_ops

[Peng He]

push_dp_ops only handles delete ops errors but ignores the modify
ops results. It's better to handle all the dp operation errors in
a consistent way.

This patch prevents the inconsistency by considering modify failure
in revalidators.

To note, we cannot perform two state transitions and change ukey_state
into UKEY_EVICTED directly here, because, if we do so, the
sweep will remove the ukey alone and leave dp flow alive. Later, the
dump will retrieve the dp flow and might even recover it. This will
contribute the stats of this dp flow twice.

v8->v9:   add testsuite and delete INCONSISTENT ukey at revalidate_sweep.
v9->v10:  change the commit message and refine the test case.
v10->v11: fix indentation and refine the test case.
v11->v12: fix the test suite.

Signed-off-by: Peng He 
---
 ofproto/ofproto-dpif-upcall.c | 50 +--
 tests/dpif-netdev.at  | 44 ++
 2 files changed, 80 insertions(+), 14 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index cd57fdbd9..c7df2d2f0 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -53,6 +53,7 @@
 VLOG_DEFINE_THIS_MODULE(ofproto_dpif_upcall);
 
 COVERAGE_DEFINE(dumped_duplicate_flow);
+COVERAGE_DEFINE(dumped_inconsistent_flow);
 COVERAGE_DEFINE(dumped_new_flow);
 COVERAGE_DEFINE(handler_duplicate_upcall);
 COVERAGE_DEFINE(revalidate_missed_dp_flow);
@@ -258,6 +259,7 @@ enum ukey_state {
 UKEY_CREATED = 0,
 UKEY_VISIBLE,   /* Ukey is in umap, datapath flow install is queued. */
 UKEY_OPERATIONAL,   /* Ukey is in umap, datapath flow is installed. */
+UKEY_INCONSISTENT,  /* Ukey is in umap, datapath flow is inconsistent. */
 UKEY_EVICTING,  /* Ukey is in umap, datapath flow delete is queued. */
 UKEY_EVICTED,   /* Ukey is in umap, datapath flow is deleted. */
 UKEY_DELETED,   /* Ukey removed from umap, ukey free is deferred. */
@@ -1999,6 +2001,10 @@ transition_ukey_at(struct udpif_key *ukey, enum 
ukey_state dst,
  * UKEY_VISIBLE -> UKEY_EVICTED
  *  A handler attempts to install the flow, but the datapath rejects it.
  *  Consider that the datapath has already destroyed it.
+ * UKEY_OPERATIONAL -> UKEY_INCONSISTENT
+ *  A revalidator modifies the flow with error returns.
+ * UKEY_INCONSISTENT -> UKEY_EVICTING
+ *  A revalidator decides to evict the datapath flow.
  * UKEY_OPERATIONAL -> UKEY_EVICTING
  *  A revalidator decides to evict the datapath flow.
  * UKEY_EVICTING-> UKEY_EVICTED
@@ -2006,8 +2012,9 @@ transition_ukey_at(struct udpif_key *ukey, enum 
ukey_state dst,
  * UKEY_EVICTED -> UKEY_DELETED
  *  A revalidator has removed the ukey from the umap and is deleting it.
  */
-if (ukey->state == dst - 1 || (ukey->state == UKEY_VISIBLE &&
-   dst < UKEY_DELETED)) {
+if (ukey->state == dst - 1 ||
+   (ukey->state == UKEY_VISIBLE && dst < UKEY_DELETED) ||
+   (ukey->state == UKEY_OPERATIONAL && dst == UKEY_EVICTING)) {
 ukey->state = dst;
 } else {
 struct ds ds = DS_EMPTY_INITIALIZER;
@@ -2472,26 +2479,31 @@ push_dp_ops(struct udpif *udpif, struct ukey_op *ops, 
size_t n_ops)
 
 for (i = 0; i < n_ops; i++) {
 struct ukey_op *op = [i];
-struct dpif_flow_stats *push, *stats, push_buf;
-
-stats = op->dop.flow_del.stats;
-push = _buf;
-
-if (op->dop.type != DPIF_OP_FLOW_DEL) {
-/* Only deleted flows need their stats pushed. */
-continue;
-}
 
 if (op->dop.error) {
-/* flow_del error, 'stats' is unusable. */
 if (op->ukey) {
 ovs_mutex_lock(>ukey->mutex);
-transition_ukey(op->ukey, UKEY_EVICTED);
+if (op->dop.type == DPIF_OP_FLOW_DEL) {
+transition_ukey(op->ukey, UKEY_EVICTED);
+} else {
+/* Modification of the flow failed. */
+transition_ukey(op->ukey, UKEY_INCONSISTENT);
+}
 ovs_mutex_unlock(>ukey->mutex);
 }
 continue;
 }
 
+if (op->dop.type != DPIF_OP_FLOW_DEL) {
+/* Only deleted flows need their stats pushed. */
+continue;
+}
+
+struct dpif_flow_stats *push, *stats, push_buf;
+
+stats = op->dop.flow_del.stats;
+push = _buf;
+
 if (op->ukey) {
 ovs_mutex_lock(>ukey->mutex);
 transition_ukey(op->ukey, UKEY_EVICTED);
@@ -2839,6 +2851,15 @@ revalidate(struct revalidator *revalidator)
 continue;
 }
 
+if (ukey->state == UKEY_INCONSISTENT) {
+ukey->dump_seq = dump_seq;
+reval_op_init([n_ops++], UKEY_DELETE, udpif, ukey,
+  , _actions);
+ovs_mutex_unlock(>mutex);
+   

Re: [ovs-dev] [ovs-dev v12] ofproto-dpif-upcall: fix push_dp_ops

[Peng He]

I miss one command in the test suite. will resend.

Peng He  于2023年7月1日周六 12:50写道：

> push_dp_ops only handles delete ops errors but ignores the modify
> ops results. It's better to handle all the dp operation errors in
> a consistent way.
>
> This patch prevents the inconsistency by considering modify failure
> in revalidators.
>
> To note, we cannot perform two state transitions and change ukey_state
> into UKEY_EVICTED directly here, because, if we do so, the
> sweep will remove the ukey alone and leave dp flow alive. Later, the
> dump will retrieve the dp flow and might even recover it. This will
> contribute the stats of this dp flow twice.
>
> v8->v9:   add testsuite and delete INCONSISTENT ukey at revalidate_sweep.
> v9->v10:  change the commit message and refine the test case.
> v10->v11: fix indentation and refine the test case.
> v11->v12: fix the test suite.
>
> Signed-off-by: Peng He 
> ---
>  ofproto/ofproto-dpif-upcall.c | 50 +--
>  tests/dpif-netdev.at  | 44 ++
>  2 files changed, 80 insertions(+), 14 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index cd57fdbd9..c7df2d2f0 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -53,6 +53,7 @@
>  VLOG_DEFINE_THIS_MODULE(ofproto_dpif_upcall);
>
>  COVERAGE_DEFINE(dumped_duplicate_flow);
> +COVERAGE_DEFINE(dumped_inconsistent_flow);
>  COVERAGE_DEFINE(dumped_new_flow);
>  COVERAGE_DEFINE(handler_duplicate_upcall);
>  COVERAGE_DEFINE(revalidate_missed_dp_flow);
> @@ -258,6 +259,7 @@ enum ukey_state {
>  UKEY_CREATED = 0,
>  UKEY_VISIBLE,   /* Ukey is in umap, datapath flow install is
> queued. */
>  UKEY_OPERATIONAL,   /* Ukey is in umap, datapath flow is installed. */
> +UKEY_INCONSISTENT,  /* Ukey is in umap, datapath flow is
> inconsistent. */
>  UKEY_EVICTING,  /* Ukey is in umap, datapath flow delete is
> queued. */
>  UKEY_EVICTED,   /* Ukey is in umap, datapath flow is deleted. */
>  UKEY_DELETED,   /* Ukey removed from umap, ukey free is deferred.
> */
> @@ -1999,6 +2001,10 @@ transition_ukey_at(struct udpif_key *ukey, enum
> ukey_state dst,
>   * UKEY_VISIBLE -> UKEY_EVICTED
>   *  A handler attempts to install the flow, but the datapath rejects
> it.
>   *  Consider that the datapath has already destroyed it.
> + * UKEY_OPERATIONAL -> UKEY_INCONSISTENT
> + *  A revalidator modifies the flow with error returns.
> + * UKEY_INCONSISTENT -> UKEY_EVICTING
> + *  A revalidator decides to evict the datapath flow.
>   * UKEY_OPERATIONAL -> UKEY_EVICTING
>   *  A revalidator decides to evict the datapath flow.
>   * UKEY_EVICTING-> UKEY_EVICTED
> @@ -2006,8 +2012,9 @@ transition_ukey_at(struct udpif_key *ukey, enum
> ukey_state dst,
>   * UKEY_EVICTED -> UKEY_DELETED
>   *  A revalidator has removed the ukey from the umap and is deleting
> it.
>   */
> -if (ukey->state == dst - 1 || (ukey->state == UKEY_VISIBLE &&
> -   dst < UKEY_DELETED)) {
> +if (ukey->state == dst - 1 ||
> +   (ukey->state == UKEY_VISIBLE && dst < UKEY_DELETED) ||
> +   (ukey->state == UKEY_OPERATIONAL && dst == UKEY_EVICTING)) {
>  ukey->state = dst;
>  } else {
>  struct ds ds = DS_EMPTY_INITIALIZER;
> @@ -2472,26 +2479,31 @@ push_dp_ops(struct udpif *udpif, struct ukey_op
> *ops, size_t n_ops)
>
>  for (i = 0; i < n_ops; i++) {
>  struct ukey_op *op = [i];
> -struct dpif_flow_stats *push, *stats, push_buf;
> -
> -stats = op->dop.flow_del.stats;
> -push = _buf;
> -
> -if (op->dop.type != DPIF_OP_FLOW_DEL) {
> -/* Only deleted flows need their stats pushed. */
> -continue;
> -}
>
>  if (op->dop.error) {
> -/* flow_del error, 'stats' is unusable. */
>  if (op->ukey) {
>  ovs_mutex_lock(>ukey->mutex);
> -transition_ukey(op->ukey, UKEY_EVICTED);
> +if (op->dop.type == DPIF_OP_FLOW_DEL) {
> +transition_ukey(op->ukey, UKEY_EVICTED);
> +} else {
> +/* Modification of the flow failed. */
> +transition_ukey(op->ukey, UKEY_INCONSISTENT);
> +}
>  ovs_mutex_unlock(>ukey->mutex);
>  }
>  continue;
>  }
>
> +if (op->dop.type != DPIF_OP_FLOW_DEL) {
> +/* Only deleted flows need their stats pushed. */
> +continue;
> +}
> +
> +struct dpif_flow_stats *push, *stats, push_buf;
> +
> +stats = op->dop.flow_del.stats;
> +push = _buf;
> +
>  if (op->ukey) {
>  ovs_mutex_lock(>ukey->mutex);
>  transition_ukey(op->ukey, UKEY_EVICTED);
> @@ -2839,6 +2851,15 @@ revalidate(struct revalidator 

[ovs-dev] [ovs-dev v12] ofproto-dpif-upcall: fix push_dp_ops

[Peng He]

push_dp_ops only handles delete ops errors but ignores the modify
ops results. It's better to handle all the dp operation errors in
a consistent way.

This patch prevents the inconsistency by considering modify failure
in revalidators.

To note, we cannot perform two state transitions and change ukey_state
into UKEY_EVICTED directly here, because, if we do so, the
sweep will remove the ukey alone and leave dp flow alive. Later, the
dump will retrieve the dp flow and might even recover it. This will
contribute the stats of this dp flow twice.

v8->v9:   add testsuite and delete INCONSISTENT ukey at revalidate_sweep.
v9->v10:  change the commit message and refine the test case.
v10->v11: fix indentation and refine the test case.
v11->v12: fix the test suite.

Signed-off-by: Peng He 
---
 ofproto/ofproto-dpif-upcall.c | 50 +--
 tests/dpif-netdev.at  | 44 ++
 2 files changed, 80 insertions(+), 14 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index cd57fdbd9..c7df2d2f0 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -53,6 +53,7 @@
 VLOG_DEFINE_THIS_MODULE(ofproto_dpif_upcall);
 
 COVERAGE_DEFINE(dumped_duplicate_flow);
+COVERAGE_DEFINE(dumped_inconsistent_flow);
 COVERAGE_DEFINE(dumped_new_flow);
 COVERAGE_DEFINE(handler_duplicate_upcall);
 COVERAGE_DEFINE(revalidate_missed_dp_flow);
@@ -258,6 +259,7 @@ enum ukey_state {
 UKEY_CREATED = 0,
 UKEY_VISIBLE,   /* Ukey is in umap, datapath flow install is queued. */
 UKEY_OPERATIONAL,   /* Ukey is in umap, datapath flow is installed. */
+UKEY_INCONSISTENT,  /* Ukey is in umap, datapath flow is inconsistent. */
 UKEY_EVICTING,  /* Ukey is in umap, datapath flow delete is queued. */
 UKEY_EVICTED,   /* Ukey is in umap, datapath flow is deleted. */
 UKEY_DELETED,   /* Ukey removed from umap, ukey free is deferred. */
@@ -1999,6 +2001,10 @@ transition_ukey_at(struct udpif_key *ukey, enum 
ukey_state dst,
  * UKEY_VISIBLE -> UKEY_EVICTED
  *  A handler attempts to install the flow, but the datapath rejects it.
  *  Consider that the datapath has already destroyed it.
+ * UKEY_OPERATIONAL -> UKEY_INCONSISTENT
+ *  A revalidator modifies the flow with error returns.
+ * UKEY_INCONSISTENT -> UKEY_EVICTING
+ *  A revalidator decides to evict the datapath flow.
  * UKEY_OPERATIONAL -> UKEY_EVICTING
  *  A revalidator decides to evict the datapath flow.
  * UKEY_EVICTING-> UKEY_EVICTED
@@ -2006,8 +2012,9 @@ transition_ukey_at(struct udpif_key *ukey, enum 
ukey_state dst,
  * UKEY_EVICTED -> UKEY_DELETED
  *  A revalidator has removed the ukey from the umap and is deleting it.
  */
-if (ukey->state == dst - 1 || (ukey->state == UKEY_VISIBLE &&
-   dst < UKEY_DELETED)) {
+if (ukey->state == dst - 1 ||
+   (ukey->state == UKEY_VISIBLE && dst < UKEY_DELETED) ||
+   (ukey->state == UKEY_OPERATIONAL && dst == UKEY_EVICTING)) {
 ukey->state = dst;
 } else {
 struct ds ds = DS_EMPTY_INITIALIZER;
@@ -2472,26 +2479,31 @@ push_dp_ops(struct udpif *udpif, struct ukey_op *ops, 
size_t n_ops)
 
 for (i = 0; i < n_ops; i++) {
 struct ukey_op *op = [i];
-struct dpif_flow_stats *push, *stats, push_buf;
-
-stats = op->dop.flow_del.stats;
-push = _buf;
-
-if (op->dop.type != DPIF_OP_FLOW_DEL) {
-/* Only deleted flows need their stats pushed. */
-continue;
-}
 
 if (op->dop.error) {
-/* flow_del error, 'stats' is unusable. */
 if (op->ukey) {
 ovs_mutex_lock(>ukey->mutex);
-transition_ukey(op->ukey, UKEY_EVICTED);
+if (op->dop.type == DPIF_OP_FLOW_DEL) {
+transition_ukey(op->ukey, UKEY_EVICTED);
+} else {
+/* Modification of the flow failed. */
+transition_ukey(op->ukey, UKEY_INCONSISTENT);
+}
 ovs_mutex_unlock(>ukey->mutex);
 }
 continue;
 }
 
+if (op->dop.type != DPIF_OP_FLOW_DEL) {
+/* Only deleted flows need their stats pushed. */
+continue;
+}
+
+struct dpif_flow_stats *push, *stats, push_buf;
+
+stats = op->dop.flow_del.stats;
+push = _buf;
+
 if (op->ukey) {
 ovs_mutex_lock(>ukey->mutex);
 transition_ukey(op->ukey, UKEY_EVICTED);
@@ -2839,6 +2851,15 @@ revalidate(struct revalidator *revalidator)
 continue;
 }
 
+if (ukey->state == UKEY_INCONSISTENT) {
+ukey->dump_seq = dump_seq;
+reval_op_init([n_ops++], UKEY_DELETE, udpif, ukey,
+  , _actions);
+ovs_mutex_unlock(>mutex);
+   

Re: [ovs-dev] [ovs-dev v4] dpif-netdev: fix dpif_netdev_flow_put

[0-day Robot]

Bleep bloop.  Greetings Peng He, 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:
ERROR: Author Peng He  needs to sign off.
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Peng He 
Lines checked: 201, 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

Re: [ovs-dev] [PATCH] ovsdb: raft: Support pre-vote mechanism to deal with disruptive server.

[Han Zhou]

On Fri, Jun 30, 2023 at 4:30 PM Ilya Maximets  wrote:
>
> On 6/25/23 19:35, Han Zhou wrote:
> > When a server becomes unstable due to overloading or intermittent
> > partitioning, it may miss some heartbeats and then starts election with
> > a new term, which would disrupt the otherwise healthy cluster formed by
> > the rest of the healthy nodes.  Such situation may exist for a long time
> > until the "flapping" server is shutdown or recovered completely, which
> > can severely impact the availability of the cluster. The pre-vote
> > mechanism introduced in the raft paper section 9.6 can prevent such
> > problems. This patch implements the pre-vote mechanism, in a way that is
> > backward compatible and safe for upgrades.
>
> Thanks, Han!  This is definitely interesting.  I don't think that this
> should be a solution for overloading though, overloading should be fixed
> by, well, reducing the load or increasing the election timer.  But some
> intermittent network issues is indeed a valid case.

Sorry that I wasn't clear about "overloading". I was referring to the case
when the machine is getting overloaded, not necessarily because of the
ovsdb-server process but because of other applications/VMs sharing the same
physical resource, which is likely what we have hit in the production.
You may argue it is an improper resource allocation, but from OVSDB point
of view, it is not an excuse for the whole RAFT cluster down. The pre-vote
can avoid such disaster, regardless of the factors of the problematic
server that we can't control.

>
> I looked through the code and I don't see any significant problems right
> away, but I would like to experiment with it a bit more next week, since
> the voting logic is fairly complex in general.
>
Appreciated!

> It's nice to see you found a use case for the stop-raft-rpc failure
> test. :)
>
> See some comments inline.
>
> Best regards, Ilya Maximets.
>
> >
> > Signed-off-by: Han Zhou 
> > ---
> >  ovsdb/raft-rpc.c   | 19 -
> >  ovsdb/raft-rpc.h   |  3 ++
> >  ovsdb/raft.c   | 88 ++
> >  tests/ovsdb-cluster.at | 42 
> >  4 files changed, 127 insertions(+), 25 deletions(-)
> >
> > diff --git a/ovsdb/raft-rpc.c b/ovsdb/raft-rpc.c
> > index dd14d81091fc..f750ba897046 100644
> > --- a/ovsdb/raft-rpc.c
> > +++ b/ovsdb/raft-rpc.c
> > @@ -283,6 +283,10 @@ raft_vote_request_to_jsonrpc(const struct
raft_vote_request *rq,
> >  json_object_put(args, "leadership_transfer",
> >  json_boolean_create(true));
> >  }
> > +if (rq->is_prevote) {
> > +json_object_put(args, "is_prevote",
> > +json_boolean_create(true));
> > +}
> >  }
> >
> >  static void
> > @@ -294,6 +298,8 @@ raft_vote_request_from_jsonrpc(struct ovsdb_parser
*p,
> >  rq->last_log_term = raft_parse_required_uint64(p, "last_log_term");
> >  rq->leadership_transfer
> >  = raft_parse_optional_boolean(p, "leadership_transfer") == 1;
> > +rq->is_prevote
> > += raft_parse_optional_boolean(p, "is_prevote") == 1;
> >  }
> >
> >  static void
> > @@ -305,6 +311,9 @@ raft_format_vote_request(const struct
raft_vote_request *rq, struct ds *s)
> >  if (rq->leadership_transfer) {
> >  ds_put_cstr(s, " leadership_transfer=true");
> >  }
> > +if (rq->is_prevote) {
> > +ds_put_cstr(s, " is_prevote=true");
> > +}
> >  }
> >
> >  /* raft_vote_reply. */
> > @@ -326,6 +335,9 @@ raft_vote_reply_to_jsonrpc(const struct
raft_vote_reply *rpy,
> >  {
> >  raft_put_uint64(args, "term", rpy->term);
> >  json_object_put_format(args, "vote", UUID_FMT,
UUID_ARGS(>vote));
> > +if (rpy->is_prevote) {
> > +json_object_put(args, "is_prevote", json_boolean_create(true));
> > +}
> >  }
> >
> >  static void
> > @@ -334,6 +346,7 @@ raft_vote_reply_from_jsonrpc(struct ovsdb_parser *p,
> >  {
> >  rpy->term = raft_parse_required_uint64(p, "term");
> >  rpy->vote = raft_parse_required_uuid(p, "vote");
> > +rpy->is_prevote = raft_parse_optional_boolean(p, "is_prevote") ==
1;
> >  }
> >
> >  static void
> > @@ -341,6 +354,9 @@ raft_format_vote_reply(const struct raft_vote_reply
*rpy, struct ds *s)
> >  {
> >  ds_put_format(s, " term=%"PRIu64, rpy->term);
> >  ds_put_format(s, " vote="SID_FMT, SID_ARGS(>vote));
> > +if (rpy->is_prevote) {
> > +ds_put_cstr(s, " is_prevote=true");
> > +}
> >  }
> >
> >  /* raft_add_server_request */
> > @@ -1008,7 +1024,8 @@ raft_rpc_get_vote(const union raft_rpc *rpc)
> >  return NULL;
> >
> >  case RAFT_RPC_VOTE_REPLY:
> > -return _vote_reply_cast(rpc)->vote;
> > +return raft_vote_reply_cast(rpc)->is_prevote ? NULL :
> > +_vote_reply_cast(rpc)->vote;
>
> It might be better to create a pointer variable here and access it
> instead of casting twice.
>
That was my first attempt, but I can't define a local variable under the

[ovs-dev] [ovs-dev v4] dpif-netdev: fix dpif_netdev_flow_put

[Peng He]

OVS allows overlapping megaflows, as long as the actions of these
megaflows are equal. However, the current implementation of action
modification relies on flow_lookup instead of ufid, this could result
in looking up a wrong megaflow and make the ukeys and megaflows inconsistent

Just like the test case in the patch, at first we have a rule with the
prefix:

10.1.2.0/24

and we will get a megaflow with prefixes 10.1.2.2/24 when a packet with IP
10.1.2.2 is received.

Then suppose we change the rule into 10.1.0.0/16. OVS prefers to keep the
10.1.2.2/24 megaflow and just changes its action instead of extending
the prefix into 10.1.2.2/16.

then suppose we have a 10.1.0.2 packet, since it misses the megaflow,
this time, we will have an overlapping megaflow with the right prefix:
10.1.0.2/16

now we have two megaflows:
10.1.2.2/24
10.1.0.2/16

last, suppose we have changed the ruleset again. The revalidator this
time still decides to change the actions of both megaflows instead of
deleting them.

The dpif_netdev_flow_put will search the megaflow to modify with unmasked
keys, however it might lookup the wrong megaflow as the key 10.1.2.2 matches
both 10.1.2.2/24 and 10.1.0.2/16!

This patch changes the megaflow lookup code in modification path into
relying the ufid to find the correct megaflow instead of key lookup.

Signed-off-by: Peng He 
---
 lib/dpif-netdev.c | 62 ++-
 tests/pmd.at  | 48 
 2 files changed, 82 insertions(+), 28 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 70b953ae6..b90ed1542 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -4198,36 +4198,43 @@ flow_put_on_pmd(struct dp_netdev_pmd_thread *pmd,
 memset(stats, 0, sizeof *stats);
 }
 
-ovs_mutex_lock(>flow_mutex);
-netdev_flow = dp_netdev_pmd_lookup_flow(pmd, key, NULL);
-if (!netdev_flow) {
-if (put->flags & DPIF_FP_CREATE) {
+if (put->flags & DPIF_FP_CREATE) {
+ovs_mutex_lock(>flow_mutex);
+netdev_flow = dp_netdev_pmd_lookup_flow(pmd, key, NULL);
+if (!netdev_flow) {
 dp_netdev_flow_add(pmd, match, ufid, put->actions,
put->actions_len, ODPP_NONE);
 } else {
-error = ENOENT;
+error = EEXIST;
 }
+ovs_mutex_unlock(>flow_mutex);
 } else {
+netdev_flow = dp_netdev_pmd_find_flow(pmd, ufid,
+  put->key, put->key_len);
+
 if (put->flags & DPIF_FP_MODIFY) {
-struct dp_netdev_actions *new_actions;
-struct dp_netdev_actions *old_actions;
+if (!netdev_flow) {
+error = ENOENT;
+} else {
+struct dp_netdev_actions *new_actions;
+struct dp_netdev_actions *old_actions;
 
-new_actions = dp_netdev_actions_create(put->actions,
-   put->actions_len);
+new_actions = dp_netdev_actions_create(put->actions,
+   put->actions_len);
 
-old_actions = dp_netdev_flow_get_actions(netdev_flow);
-ovsrcu_set(_flow->actions, new_actions);
+old_actions = dp_netdev_flow_get_actions(netdev_flow);
+ovsrcu_set(_flow->actions, new_actions);
 
-queue_netdev_flow_put(pmd, netdev_flow, match,
-  put->actions, put->actions_len,
-  DP_NETDEV_FLOW_OFFLOAD_OP_MOD);
-log_netdev_flow_change(netdev_flow, match, old_actions,
-   put->actions, put->actions_len);
+queue_netdev_flow_put(pmd, netdev_flow, match,
+  put->actions, put->actions_len,
+  DP_NETDEV_FLOW_OFFLOAD_OP_MOD);
+log_netdev_flow_change(netdev_flow, match, old_actions,
+   put->actions, put->actions_len);
 
-if (stats) {
-get_dpif_flow_status(pmd->dp, netdev_flow, stats, NULL);
-}
-if (put->flags & DPIF_FP_ZERO_STATS) {
+if (stats) {
+get_dpif_flow_status(pmd->dp, netdev_flow, stats, NULL);
+}
+if (put->flags & DPIF_FP_ZERO_STATS) {
 /* XXX: The userspace datapath uses thread local statistics
  * (for flows), which should be updated only by the owning
  * thread.  Since we cannot write on stats memory here,
@@ -4237,18 +4244,17 @@ flow_put_on_pmd(struct dp_netdev_pmd_thread *pmd,
  * - Should the need arise, this operation can be implemented
  *   by keeping a base value (to be update here) for each
  *   counter, and subtracting it before outputting the stats 

Re: [ovs-dev] [ovs-dev v3] dpif-netdev: fix dpif_netdev_flow_put

[Peng He]

Eelco Chaudron  于2023年6月23日周五 22:31写道：

>
>
> On 15 Jun 2023, at 4:51, Peng He wrote:
>
> > OVS allows overlapping megaflows, as long as the actions of these
> > megaflows are equal. However, the current implementation of action
> > modification relies on flow_lookup instead of ufid, this could result
> > in looking up a wrong megaflow and make the ukeys and megaflows
> inconsistent
> >
> > Just like the test case in the patch, at first we have a rule with the
> > prefix:
> >
> > 10.1.2.0/24
> >
> > and we will get a megaflow with prefixes 10.1.2.2/24 when a packet with
> IP
> > 10.1.2.2 is received.
> >
> > Then suppose we change the rule into 10.1.0.0/16. OVS prefers to keep
> the
> > 10.1.2.2/24 megaflow and just changes its action instead of extending
> > the prefix into 10.1.2.2/16.
> >
> > then suppose we have a 10.1.0.2 packet, since it misses the megaflow,
> > this time, we will have an overlapping megaflow with the right prefix:
> > 10.1.0.2/16
> >
> > now we have two megaflows:
> > 10.1.2.2/24
> > 10.1.0.2/16
> >
> > last, suppose we have changed the ruleset again. The revalidator this
> > time still decides to change the actions of both megaflows instead of
> > deleting them.
> >
> > The dpif_netdev_flow_put will search the megaflow to modify with unmasked
> > keys, however it might lookup the wrong megaflow as the key 10.1.2.2
> matches
> > both 10.1.2.2/24 and 10.1.0.2/16!
> >
> > This patch changes the megaflow lookup code in modification path into
> > relying the ufid to find the correct megaflow instead of key lookup.
> >
> > Signed-off-by: Peng He 
>
> Thanks for the fix, and in general it looks good, however I have one
> question about not covering all cases. See below.
>
> Cheers,
>
> Eelco
>
>
> > ---
> >  lib/dpif-netdev.c | 62 ++-
> >  tests/pmd.at  | 48 
> >  2 files changed, 82 insertions(+), 28 deletions(-)
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 70b953ae6..b90ed1542 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -4198,36 +4198,43 @@ flow_put_on_pmd(struct dp_netdev_pmd_thread *pmd,
> >  memset(stats, 0, sizeof *stats);
> >  }
> >
> > -ovs_mutex_lock(>flow_mutex);
> > -netdev_flow = dp_netdev_pmd_lookup_flow(pmd, key, NULL);
> > -if (!netdev_flow) {
> > -if (put->flags & DPIF_FP_CREATE) {
> > +if (put->flags & DPIF_FP_CREATE) {
>
> Should this not be:
>
> if (put->flags & DPIF_FP_CREATE && !(put->flags & DPIF_FP_MODIFY))
>
> I know this will break the fix, but I’m wondering what the possible
> combinations are.
> Quickly looking at the code the following ones seem to exist:
>
> DPIF_FP_CREATE  -> Create a flow, if one exists return EEXIST
> DPIF_FP_MODIFY  -> Modify a flow, if it does not exist return ENONT
> DPIF_FP_CREATE | DPIF_FP_MODIFY   -> If it exists modify it, if it does
> not exists create it.
>
> Could the last combination not potentially fail the lookup?
>
> Or are we assuming only standalone DPIF_FP_MODIFY’s are the problem? In
> theory, it could also be the combination.
>

sorry, I was wrong. Such a combination does exist in
the dpif_probe_feature, and it probes the datapath by trying to put flows:

 error = dpif_flow_put(dpif, DPIF_FP_CREATE | DPIF_FP_MODIFY |
DPIF_FP_PROBE,
   key->data, key->size, NULL, 0,
   nl_actions, nl_actions_size,
   ufid, NON_PMD_CORE_ID, NULL);

so we CANNOT change the code into:

if (put->flags & DPIF_FP_CREATE && !(put->flags & DPIF_FP_MODIFY))

as the issue the patch tries to fix only exist in MODIFY alone path.
If CREATE bit is set, we need to go through the CREATE path no matter if
MODIFY exist or not.



>
> > +ovs_mutex_lock(>flow_mutex);
> > +netdev_flow = dp_netdev_pmd_lookup_flow(pmd, key, NULL);
> > +if (!netdev_flow) {
> >  dp_netdev_flow_add(pmd, match, ufid, put->actions,
> > put->actions_len, ODPP_NONE);
> >  } else {
> > -error = ENOENT;
> > +error = EEXIST;
> >  }
> > +ovs_mutex_unlock(>flow_mutex);
> >  } else {
> > +netdev_flow = dp_netdev_pmd_find_flow(pmd, ufid,
> > +  put->key, put->key_len);
> > +
> >  if (put->flags & DPIF_FP_MODIFY) {
> > -struct dp_netdev_actions *new_actions;
> > -struct dp_netdev_actions *old_actions;
> > +if (!netdev_flow) {
> > +error = ENOENT;
> > +} else {
> > +struct dp_netdev_actions *new_actions;
> > +struct dp_netdev_actions *old_actions;
> >
> > -new_actions = dp_netdev_actions_create(put->actions,
> > -   put->actions_len);
> > +new_actions = dp_netdev_actions_create(put->actions,
> > +
>  

Re: [ovs-dev] [PATCH ovn 3/3] northd: Incremental processing of VIF updates and deletions in 'lflow' node.

[Han Zhou]

On Thu, Jun 29, 2023 at 10:54 PM Numan Siddique  wrote:
>
> On Sun, Jun 18, 2023 at 11:48 AM Han Zhou  wrote:
> >
> > This patch achieves zero recompute for VIF updates and deletions in
> > common scenarios. The performance gain for these scenarios is similar to
> > the patch "northd: Incremental processing of VIF additions in 'lflow'
> > node."
> >
> > Signed-off-by: Han Zhou 
> > ---
> >  northd/northd.c | 231 +---
> >  tests/ovn-northd.at |   4 +-
> >  2 files changed, 154 insertions(+), 81 deletions(-)
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index aa0f853ce2db..c0c948fcea4b 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -16303,6 +16303,113 @@ void build_lflows(struct ovsdb_idl_txn
*ovnsb_txn,
> >  hmap_destroy(_groups);
> >  }
> >
> > +static void
> > +sync_lsp_lflows_to_sb(struct ovsdb_idl_txn *ovnsb_txn,
> > +  struct lflow_input *lflow_input,
> > +  struct hmap *lflows,
> > +  struct ovn_lflow *lflow)
> > +{
> > +size_t n_datapaths;
> > +struct ovn_datapath **datapaths_array;
> > +if (ovn_stage_to_datapath_type(lflow->stage) == DP_SWITCH) {
> > +n_datapaths = ods_size(lflow_input->ls_datapaths);
> > +datapaths_array = lflow_input->ls_datapaths->array;
> > +} else {
> > +n_datapaths = ods_size(lflow_input->lr_datapaths);
> > +datapaths_array = lflow_input->lr_datapaths->array;
> > +}
> > +uint32_t n_ods = bitmap_count1(lflow->dpg_bitmap, n_datapaths);
> > +ovs_assert(n_ods == 1);
> > +/* There is only one datapath, so it should be moved out of the
> > + * group to a single 'od'. */
> > +size_t index = bitmap_scan(lflow->dpg_bitmap, true, 0,
> > +   n_datapaths);
> > +
> > +bitmap_set0(lflow->dpg_bitmap, index);
> > +lflow->od = datapaths_array[index];
> > +
> > +/* Logical flow should be re-hashed to allow lookups. */
> > +uint32_t hash = hmap_node_hash(>hmap_node);
> > +/* Remove from lflows. */
> > +hmap_remove(lflows, >hmap_node);
> > +hash = ovn_logical_flow_hash_datapath(>od->sb->header_.uuid,
> > +  hash);
> > +/* Add back. */
> > +hmap_insert(lflows, >hmap_node, hash);
> > +
> > +/* Sync to SB. */
> > +const struct sbrec_logical_flow *sbflow;
> > +lflow->sb_uuid = uuid_random();
> > +sbflow = sbrec_logical_flow_insert_persist_uuid(ovnsb_txn,
> > +>sb_uuid);
> > +const char *pipeline = ovn_stage_get_pipeline_name(lflow->stage);
> > +uint8_t table = ovn_stage_get_table(lflow->stage);
> > +sbrec_logical_flow_set_logical_datapath(sbflow, lflow->od->sb);
> > +sbrec_logical_flow_set_logical_dp_group(sbflow, NULL);
> > +sbrec_logical_flow_set_pipeline(sbflow, pipeline);
> > +sbrec_logical_flow_set_table_id(sbflow, table);
> > +sbrec_logical_flow_set_priority(sbflow, lflow->priority);
> > +sbrec_logical_flow_set_match(sbflow, lflow->match);
> > +sbrec_logical_flow_set_actions(sbflow, lflow->actions);
> > +if (lflow->io_port) {
> > +struct smap tags = SMAP_INITIALIZER();
> > +smap_add(, "in_out_port", lflow->io_port);
> > +sbrec_logical_flow_set_tags(sbflow, );
> > +smap_destroy();
> > +}
> > +sbrec_logical_flow_set_controller_meter(sbflow, lflow->ctrl_meter);
> > +/* Trim the source locator lflow->where, which looks something like
> > + * "ovn/northd/northd.c:1234", down to just the part following the
> > + * last slash, e.g. "northd.c:1234". */
> > +const char *slash = strrchr(lflow->where, '/');
> > +#if _WIN32
> > +const char *backslash = strrchr(lflow->where, '\\');
> > +if (!slash || backslash > slash) {
> > +slash = backslash;
> > +}
> > +#endif
> > +const char *where = slash ? slash + 1 : lflow->where;
> > +
> > +struct smap ids = SMAP_INITIALIZER();
> > +smap_add(, "stage-name", ovn_stage_to_str(lflow->stage));
> > +smap_add(, "source", where);
> > +if (lflow->stage_hint) {
> > +smap_add(, "stage-hint", lflow->stage_hint);
> > +}
> > +sbrec_logical_flow_set_external_ids(sbflow, );
> > +smap_destroy();
> > +}
> > +
> > +static bool
> > +delete_lflow_for_lsp(struct ovn_port *op, bool is_update,
> > + const struct sbrec_logical_flow_table
*sb_lflow_table,
> > + struct hmap *lflows)
> > +{
> > +struct lflow_ref_node *lfrn;
> > +const char *operation = is_update ? "updated" : "deleted";
> > +LIST_FOR_EACH_SAFE (lfrn, lflow_list_node, >lflows) {
> > +VLOG_DBG("Deleting SB lflow "UUID_FMT" for %s port %s",
> > + UUID_ARGS(>lflow->sb_uuid), operation, op->key);
> > +
> > +const struct sbrec_logical_flow *sblflow =
> > +

Re: [ovs-dev] [ovs-dev v11] ofproto-dpif-upcall: fix push_dp_ops

[Peng He]

Ilya Maximets  于2023年6月23日周五 20:40写道：

> On 6/23/23 14:20, Eelco Chaudron wrote:
> >
> >
> > On 9 Jun 2023, at 17:03, Peng He wrote:
> >
> >> push_dp_ops only handles delete ops errors but ignores the modify
> >> ops results. It's better to handle all the dp operation errors in
> >> a consistent way.
> >>
> >> This patch prevents the inconsistency by considering modify failure
> >> in revalidators.
> >>
> >> To note, we cannot perform two state transitions and change ukey_state
> >> into UKEY_EVICTED directly here, because, if we do so, the
> >> sweep will remove the ukey alone and leave dp flow alive. Later, the
> >> dump will retrieve the dp flow and might even recover it. This will
> >> contribute the stats of this dp flow twice.
> >>
> >> v8->v9:   add testsuite and delete INCONSISTENT ukey at
> revalidate_sweep.
> >> v9->v10:  change the commit message and refine the test case.
> >> v10->v11: fix indentation and refine the test case.
> >>
> >> Signed-off-by: Peng He 
> >
> > Thanks for making these changes, there is a merge conflict on the tests
> case and some small comments below.
> >
> > So maybe you can send a v12 and I’ll do a quick check and ACK it. What
> do you think?
> >
> > Cheers,
> >
> > Eelco
> >
> >
> >> ---
> >>  ofproto/ofproto-dpif-upcall.c | 51 +--
> >>  tests/dpif-netdev.at  | 44 ++
> >>  2 files changed, 81 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/ofproto/ofproto-dpif-upcall.c
> b/ofproto/ofproto-dpif-upcall.c
> >> index cd57fdbd9..c920c749c 100644
> >> --- a/ofproto/ofproto-dpif-upcall.c
> >> +++ b/ofproto/ofproto-dpif-upcall.c
> >> @@ -62,6 +62,7 @@ COVERAGE_DEFINE(upcall_flow_limit_hit);
> >>  COVERAGE_DEFINE(upcall_flow_limit_kill);
> >>  COVERAGE_DEFINE(upcall_ukey_contention);
> >>  COVERAGE_DEFINE(upcall_ukey_replace);
> >> +COVERAGE_DEFINE(dumped_inconsistent_flow);
> >
> > Can you add this in alphabetical order?
> >
> >>
> >>  /* A thread that reads upcalls from dpif, forwards each upcall's
> packet,
> >>   * and possibly sets up a kernel flow as a cache. */
> >> @@ -258,6 +259,7 @@ enum ukey_state {
> >>  UKEY_CREATED = 0,
> >>  UKEY_VISIBLE,   /* Ukey is in umap, datapath flow install is
> queued. */
> >>  UKEY_OPERATIONAL,   /* Ukey is in umap, datapath flow is
> installed. */
> >> +UKEY_INCONSISTENT,  /* Ukey is in umap, datapath flow is
> inconsistent. */
> >>  UKEY_EVICTING,  /* Ukey is in umap, datapath flow delete is
> queued. */
> >>  UKEY_EVICTED,   /* Ukey is in umap, datapath flow is deleted.
> */
> >>  UKEY_DELETED,   /* Ukey removed from umap, ukey free is
> deferred. */
> >> @@ -1999,6 +2001,10 @@ transition_ukey_at(struct udpif_key *ukey, enum
> ukey_state dst,
> >>   * UKEY_VISIBLE -> UKEY_EVICTED
> >>   *  A handler attempts to install the flow, but the datapath
> rejects it.
> >>   *  Consider that the datapath has already destroyed it.
> >> + * UKEY_OPERATIONAL -> UKEY_INCONSISTENT
> >> + *  A revalidator modifies the flow with error returns.
> >> + * UKEY_INCONSISTENT -> UKEY_EVICTING
> >> + *  A revalidator decides to evict the datapath flow.
> >>   * UKEY_OPERATIONAL -> UKEY_EVICTING
> >>   *  A revalidator decides to evict the datapath flow.
> >>   * UKEY_EVICTING-> UKEY_EVICTED
> >> @@ -2006,8 +2012,9 @@ transition_ukey_at(struct udpif_key *ukey, enum
> ukey_state dst,
> >>   * UKEY_EVICTED -> UKEY_DELETED
> >>   *  A revalidator has removed the ukey from the umap and is
> deleting it.
> >>   */
> >> -if (ukey->state == dst - 1 || (ukey->state == UKEY_VISIBLE &&
> >> -   dst < UKEY_DELETED)) {
> >> +if (ukey->state == dst - 1 ||
> >> +   (ukey->state == UKEY_VISIBLE && dst < UKEY_DELETED) ||
> >> +   (ukey->state == UKEY_OPERATIONAL && dst == UKEY_EVICTING)) {
> >>  ukey->state = dst;
> >>  } else {
> >>  struct ds ds = DS_EMPTY_INITIALIZER;
> >> @@ -2472,26 +2479,31 @@ push_dp_ops(struct udpif *udpif, struct ukey_op
> *ops, size_t n_ops)
> >>
> >>  for (i = 0; i < n_ops; i++) {
> >>  struct ukey_op *op = [i];
> >> -struct dpif_flow_stats *push, *stats, push_buf;
> >> -
> >> -stats = op->dop.flow_del.stats;
> >> -push = _buf;
> >> -
> >> -if (op->dop.type != DPIF_OP_FLOW_DEL) {
> >> -/* Only deleted flows need their stats pushed. */
> >> -continue;
> >> -}
> >>
> >>  if (op->dop.error) {
> >> -/* flow_del error, 'stats' is unusable. */
> >>  if (op->ukey) {
> >>  ovs_mutex_lock(>ukey->mutex);
> >> -transition_ukey(op->ukey, UKEY_EVICTED);
> >> +if (op->dop.type == DPIF_OP_FLOW_DEL) {
> >> +transition_ukey(op->ukey, UKEY_EVICTED);
> >> +} else {
> >> +/* Modification of the flow failed */
> >

Re: [ovs-dev] [PATCH ovn 1/3] northd.c: Generate and maintain SB lflow uuid in ovn_lflow.

[Han Zhou]

On Thu, Jun 29, 2023 at 10:06 PM Numan Siddique  wrote:
>
> On Fri, Jun 30, 2023 at 7:01 AM Han Zhou  wrote:
> >
> > On Thu, Jun 29, 2023 at 4:57 AM Dumitru Ceara  wrote:
> > >
> > > On 6/18/23 08:17, Han Zhou wrote:
> > > > For incremental processing, we need to maintain SB lflow uuids in
> > > > northd. For this reason, we generate the row uuid when creating the
> > > > Logical_Flow record in SB DB, rather than waiting for SB DB to
populate
> > > > back.
> > > >
> > > > Signed-off-by: Han Zhou 
> > > > ---
> > > >  northd/northd.c | 12 ++--
> > > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/northd/northd.c b/northd/northd.c
> > > > index a45c8b53ad4e..98f528f93cfc 100644
> > > > --- a/northd/northd.c
> > > > +++ b/northd/northd.c
> > > > @@ -5592,6 +5592,8 @@ struct ovn_lflow {
> > > >* 'dpg_bitmap'. */
> > > >  struct ovn_dp_group *dpg;/* Link to unique Sb datapath
group.
> > */
> > > >  const char *where;
> > > > +
> > > > +struct uuid sb_uuid;/* SB DB row uuid, specified by
> > northd. */
> > >
> > > Nit: can you please align this comment with the ones above?
> > >
> > > >  };
> > > >
> > > >  static void ovn_lflow_destroy(struct hmap *lflows, struct ovn_lflow
> > *lflow);
> > > > @@ -5649,6 +5651,7 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct
> > ovn_datapath *od,
> > > >  lflow->ctrl_meter = ctrl_meter;
> > > >  lflow->dpg = NULL;
> > > >  lflow->where = where;
> > > > +lflow->sb_uuid = UUID_ZERO;
> > > >  }
> > > >
> > > >  /* The lflow_hash_lock is a mutex array that protects updates to
the
> > shared
> > > > @@ -16020,6 +16023,7 @@ void build_lflows(struct ovsdb_idl_txn
> > *ovnsb_txn,
> > > >  size_t n_datapaths;
> > > >  bool is_switch;
> > > >
> > > > +lflow->sb_uuid = sbflow->header_.uuid;
> > > >  is_switch = ovn_stage_to_datapath_type(lflow->stage) ==
> > DP_SWITCH;
> > > >  if (is_switch) {
> > > >  n_datapaths = ods_size(input_data->ls_datapaths);
> > > > @@ -16095,7 +16099,9 @@ void build_lflows(struct ovsdb_idl_txn
> > *ovnsb_txn,
> > > >  dp_groups = _dp_groups;
> > > >  }
> > > >
> > > > -sbflow = sbrec_logical_flow_insert(ovnsb_txn);
> > > > +lflow->sb_uuid = uuid_random();
> > >
> > > If we ever decide to parallelize the lflow synchronization code this
> > > will hurt performance.  Until then we're fine I guess.
> >
> > Good point! I will add a comment to remind us before merging this.
>
> Interesting. Only after looking into the uuid_random() code can one
> figure that it acquires a lock.
>
> >
> > >
> > > > +sbflow = sbrec_logical_flow_insert_persist_uuid(ovnsb_txn,
> > > > +
> >  >sb_uuid);
> > > >  if (lflow->od) {
> > > >  sbrec_logical_flow_set_logical_datapath(sbflow,
> > lflow->od->sb);
> > > >  } else {
> > > > @@ -16305,7 +16311,9 @@ bool lflow_handle_northd_ls_changes(struct
> > ovsdb_idl_txn *ovnsb_txn,
> > > >
> > > >  /* Sync to SB. */
> > > >  const struct sbrec_logical_flow *sbflow;
> > > > -sbflow = sbrec_logical_flow_insert(ovnsb_txn);
> > > > +lflow->sb_uuid = uuid_random();
> > > > +sbflow =
sbrec_logical_flow_insert_persist_uuid(ovnsb_txn,
> > > > +
> >  >sb_uuid);
> > > >  const char *pipeline =
> > ovn_stage_get_pipeline_name(lflow->stage);
> > > >  uint8_t table = ovn_stage_get_table(lflow->stage);
> > > >  sbrec_logical_flow_set_logical_datapath(sbflow,
> > lflow->od->sb);
> > >
> > > With the small nit about the comment indentation addressed:
> > >
> > > Acked-by: Dumitru Ceara 
>
> Acked-by: Numan Siddique 
>
> Numan

Thanks Dumitru and Numan. I applied this patch to main.
>
> > >
> > ___
> > 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] Expose distributed gateway port information in NB DB

[Han Zhou]

On Fri, Jun 30, 2023 at 6:35 AM Lucas Martins  wrote:
>
>  Hi all,
>
> On Thu, Apr 27, 2023 at 7:08 PM Han Zhou  wrote:
> >
> >
> >
> > On Thu, Apr 27, 2023 at 10:15 AM Ihar Hrachyshka 
wrote:
> > >
> > > On Wed, Apr 19, 2023 at 5:17 AM Lucas Martins 
wrote:
> > > >
> > > > Hi Han, all
> > > >
> > > > On Mon, Apr 17, 2023 at 8:02 PM Han Zhou  wrote:
> > > > >
> > > > >
> > > > >
> > > > > On Mon, Apr 17, 2023 at 7:18 AM Lucas Martins 
wrote:
> > > > > >
> > > > > > Thanks all for the discussion and all the ideas here.
> > > > > >
> > > > > > After reading the emails, I think it boils down to two proposed
approaches:
> > > > > >
> > > > > > 1) CMS to continue to connect to the Southbound database if
they need
> > > > > > information about the physical location of the resources. That
would
> > > > > > avoid the inefficiency of having to copy data back-and-forth
from the
> > > > > > Northbound and Southbound database.
> > > > > >
> > > > > > I guess the downside of this approach is that CMS will have to
> > > > > > maintain a connection with both databases (which is already the
case
> > > > > > today).
> > > > > >
> > > > > > If we go with this approach, it would be good to have consensus
from
> > > > > > the core OVN team where some tables in the Southbound must be
kept
> > > > > > stable with backward compatibility in case of changes. Tables
such
> > > > > > Chassis, Chassis_Private and Port_Binding at least will need
that. I
> > > > > > guess that makes part of the Southbound database to not be
considered
> > > > > > OVN internal only.
> > > > >
> > > > > It doesn't look very clean to expose the SB DB to CMS, but in
practice I think it is not a real problem for doing so for keeping tables
backward compatible, because even without considering CMS, OVN itself needs
to keep the compatibility for upgrading.
> > > > > I am still open to the idea of keeping things in NB DB only, but
at least one issue not addressed so far in this discussion (even with the
status DB) is how to manage the orphan chassis if SB is not exposed to CMS.
It seems still more practical to me to let CMS connect to SB directly
instead of introducing a copy of Chassis table in NB and ovn-northd doing
the back-and-forth data sync. So I am not sure if it should be a goal to
remove all the SB access from CMS.
> > > > >
> > > >
> > > > Yeah the Chassis situation in OVN is a strange one, indeed. Given
that
> > > > ovn-controller is the one creating it, not the CMS, shouldn't it be
up
> > > > to OVN to actually manage that table ? Perhaps we need to introduce
> > > > some health check where ovn-controller could ping his own entry in
the
> > > > Chassis table from time to time to indicate that it's alive. And,
> > > > ovn-northd could clean up the orphan entries if they are not updated
> > > > in X amount of time.
> >
> > Thanks for the idea. In fact OVN has the "control-plane ping" mechanism
using the nb_cfg which has similar idea, but it is mostly used for testing
purposes and never recommended to be used in large scale production,
because this would add a lot of write operations from all ovn-controllers
to SB DB, which may increase the load significantly (depending on the
number of nodes and frequency of this ping) to the SB which is already a
bottle-neck.
> > There can also be false-positives when ovn-controller is busy and slow
(due to scenarios of recomputing within a large scale environment), which
would make the things worse.
> > On the other hand, an interval too long would also result in a cleanup
too late, and then a new chassis (sometimes because of reimaging of the
same node) would conflict with the old chassis.
> > In fact, the CMS is the one that has the best knowledge when a chassis
should be cleaned.
> >
> > >
> > > (Random thought) NB could have an API (ChassisDeprovisionRequest?)
> > > that would be used by CMS to request cleanup for a chassis by name.
> > > Northd could then update the object with the status of the request, or
> > > delete it once it's processed.
> > >
> >
> > This may work, but I'd avoid any imperative approach if possible. NB
(and also SB) should just maintain the desired states and let the
controllers/daemons to converge.
> > A "request" may be failed/timeout/invalid/out-of-date, which needs to
be transactional and will be relatively complex to handle.
> >
> > If the direct connection to SB is really the only concern, I think the
most viable approach is still to add a chassis table in NB just for CMS to
specify the valid chassis that should exist, and ovn-northd (probably with
a new thread) will take care of the cleanup. This is simple and scalable.
The only question is whether it's worth making this change.
> >
> > > >
> > > > Or piggybacking on the idea of the Status DB, maybe those health
> > > > checks could be done in that DB instead ?
> > > >
> > > > > On the other hand, giving another thought, it doesn't sound too
much extra cost for propagating the "hosting-chassis" information for
chassis-redirect 

Re: [ovs-dev] [PATCH 1/2] appveyor: Don't download OpenSSL.

[Ilya Maximets]

On 7/1/23 01:43, Ilya Maximets wrote:
> On 6/8/23 12:07, Alin Serdean wrote:
>> Ack, I will try to see if I can get GH actions.
>> It would also be better to have everything integrated in one place.
>>
>> I remember the discussion and the PR… I had some comments which never got 
>> addressed.
>>
>> Windows is pretty slow when starting processes ,especially if the builtin AV 
>> is not disabled for the directory you are building in. That is the main 
>> issue for the slow compile and test times.
>>
>> Using cmake/meson is helpful because unlike the automake tools, they will 
>> generate a visual studio solution and that in turn can be built by invoking 
>> a single msbuild process. Since meson does not have something builtin for 
>> testing, I was wondering if it would make more sense switching to 
>> cmake/ctest… 
> 
> We could just invoke autotest from meson as a workaround...
> I didn't work much with cmake, but personally I don't like it.
> 
> FWIW, appveyor started to fail not being able to find OpenSSL,
> even though it still claims that it's should be in the same
> location:
> 
> checking for openssl/ssl.h in C:/OpenSSL-Win64... no
> checking whether compiling and linking against OpenSSL works... no
> configure: error: Cannot find openssl (use --disable-ssl to configure without 
> SSL support)
> Command exited with code 1
> 
> That's annoying.

I opened an issue: https://github.com/appveyor/ci/issues/3883

> 
> Best regards, Ilya Maximets.
> 
>>
>> —
>> Alin
>>
>>> On 8 Jun 2023, at 11:43, Ilya Maximets  wrote:
>>>
>>> On 6/8/23 11:01, Alin Serdean wrote:

 Ack. I will try to see if I can address it by the end of the week.

 Thanks for clarifying the questions.

 Would GH actions be better than appveyor?
>>>
>>> Functionally, I don't think they are that much different.
>>> But GHA is better integrated into our CI, i.e. ovsrobot
>>> reports GHA status per patch on a patchwork, but it doesn't
>>> do the same for appveyor.  So, having GHA job is probably
>>> not a bad idea.  We discussed most of the pros and cons
>>> about 2 years ago here:
>>>  https://github.com/openvswitch/ovs-issues/issues/209
>>> The main problem is that windows build is slow in general.
>>> But that shouldn't be a blocker for CI.
>>>
>>> Best regards, Ilya Maximets.
>>>

 —
 Alin.

> On 2 Jun 2023, at 12:25, Ilya Maximets  wrote:
>
> On 6/1/23 13:50, Ilya Maximets wrote:
>>> On 5/31/23 23:05, Alin Serdean wrote:
>>>
>>> That makes sense.
>>>
>>> We can leverage the  following commit:
>>>
>>> https://patchwork.ozlabs.org/project/openvswitch/patch/20201013124655.1408-1-aserd...@cloudbasesolutions.com/
>>>  
>>> 
>>>
>>> But I still need to fix the permissions. I’ll try to find some time and 
>>> address it.
>>
>> Thanks.  I actually forgot about this patch.  Would be great if you can
>> send an updated version.  Would also be great to migrate to OpenSSL 3.0
>> instead of 1.1.1 to not migrate again in some not so distant future.
>> OpenSSL 3.0 should work fine, unless there are some other library changes
>> (not OpenSSL 3.1, because OpenSSL 3.1 is not an LTS release).
>
> I'll mark  the current patch as 'rejected' for now in favor of the
> future OpenSSL update with your suggested change.
>
>>
>>> I remember there were some discussions to modernize the build system to 
>>> meson or cmake. Was that effort fruitful in the end?
>>
>> The main issue with meson is that we still need automake/autotools for
>> our testsuite.  We might invoke autotest from meson, but it sounds a
>> bit strange to do that.  I agree that it is still beneficial in some
>> cases to use meson, e.g. for a windows build, so might make sense to
>> migrate anyway, but an attempt from 2021 didn't receive any follow ups.
>>
>> There was also a PR to add Windows build to GitHub Actions, but it
>> didn't move since your request to Sign-off the changes.
>>
>> Best regards, Ilya Maximets.
>>
>>>
>>> Alin.
>>>
 On 31 May 2023, at 22:41, Ilya Maximets  wrote:

 On 5/31/23 22:36, Alin Serdean wrote:
>
> It would be best to change the link with the latest version of 
> OpenSSL. That will ensure there are no mishaps .

 I think the problem here is that slproweb.com only provides
 OpenSSL 1.1.1+ right now and our build system doesn't work
 with that version.  And I don't have enough experience with
 windows build in order to fix it...

 Best regards, Ilya Maximets.

>
> Alin.
>
>>
>> On 31 May 2023, at 21:23, Ilya Maximets  wrote:
>>
>> OpenSSL is already available in the exact location we 

Re: [ovs-dev] [PATCH] dpif-netdev: Lockless meters.

[Ilya Maximets]

On 6/30/23 08:40, Eelco Chaudron wrote:
> 
> 
> On 29 Jun 2023, at 17:54, Ilya Maximets wrote:
> 
>> On 6/29/23 17:43, Eelco Chaudron wrote:
>>>
>>>
>>> On 22 Jun 2023, at 0:32, Ilya Maximets wrote:
>>>
 Current implementation of meters in the userspace datapath takes
 the meter lock for every packet batch.  If more than one thread
 hits the flow with the same meter, they will lock each other.

 Replace the critical section with atomic operations to avoid
 interlocking.  Meters themselves are RCU-protected, so it's safe
 to access them without holding a lock.

 Implementation does the following:

  1. Tries to advance the 'used' timer of the meter with atomic
 compare+exchange if it's smaller than 'now'.
  2. If the timer change succeeds, atomically update band buckets.
  3. Atomically update packet statistics for a meter.
  4. Go over buckets and try to atomically subtract the amount of
 packets or bytes, recording the highest exceeded band.
  5. Atomically update band statistics and drop packets.

 Bucket manipulations are implemented with atomic compare+exchange
 operations with extra checks, because bucket size should never
 exceed the maximum and it should never go below zero.

 Packet statistics may be momentarily inconsistent, i.e., number
 of packets and the number of bytes may reflect different sets
 of packets.  But it should be eventually consistent.  And the
 difference at any given time should be in just few packets.

 For the sake of reduced code complexity PKTPS meter tries to push
 packets through the band one by one, even though they all have
 the same weight.  This is also more fair if more than one thread
 is passing packets through the same band at the same time.
 Trying to predict the number of packets that can pass may also
 cause extra atomic operations reducing the performance.

 This implementation shows similar performance to the previous one,
 but should scale better with more threads hiting the same meter.
>>>
>>> This works looks great!! Some small comments below. Did limited testing and 
>>> seems to work fine.
>>>
>>> Cheers,
>>>
>>> Eelco
>>>
 Signed-off-by: Ilya Maximets 
 ---

 @Lin Huang, if you can try this change on your setup, that
 would be great.

  NEWS  |   2 +
  lib/dpif-netdev.c | 250 +-
  2 files changed, 140 insertions(+), 112 deletions(-)


...

>>>
 +
  size_t j;
  DP_PACKET_BATCH_REFILL_FOR_EACH (j, cnt, packet, packets_) {
 -if (exceeded_band[j] >= 0) {
 +uint32_t m = exceeded_band[j];
 +
 +if (m != UINT32_MAX) {
  /* Meter drop packet. */
 -band = >bands[exceeded_band[j]];
 -band->packet_count += 1;
 -band->byte_count += dp_packet_size(packet);
 -COVERAGE_INC(datapath_drop_meter);
 +band = >bands[m];
 +band_packets[m]++;
 +band_bytes[m] += dp_packet_size(packet);
>>>
>>>
>>> This code now looks like this (the diff is a mess to comment on):
>>>
>>> if (m != UINT32_MAX) {
>>> /* Meter drop packet. */
>>> band = >bands[m];
>>>
>>> !   ^^^ This line can be removed as band is not used.
>>
>> True.  Can remove.
> 
> Thanks, now I see that even shows up with clang-analyze.
> 
> I guess this is the only real change needed. So if you roll it in during 
> commit time:
> 
> Acked-by: Eelco Chaudron 

Thanks Simon and Eelco for review!  And thanks Lin and Zhang for testing!

I removed the unused assignment and applied the change.

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

Re: [ovs-dev] [PATCH 1/2] appveyor: Don't download OpenSSL.

[Ilya Maximets]

On 6/8/23 12:07, Alin Serdean wrote:
> Ack, I will try to see if I can get GH actions.
> It would also be better to have everything integrated in one place.
> 
> I remember the discussion and the PR… I had some comments which never got 
> addressed.
> 
> Windows is pretty slow when starting processes ,especially if the builtin AV 
> is not disabled for the directory you are building in. That is the main issue 
> for the slow compile and test times.
> 
> Using cmake/meson is helpful because unlike the automake tools, they will 
> generate a visual studio solution and that in turn can be built by invoking a 
> single msbuild process. Since meson does not have something builtin for 
> testing, I was wondering if it would make more sense switching to 
> cmake/ctest… 

We could just invoke autotest from meson as a workaround...
I didn't work much with cmake, but personally I don't like it.

FWIW, appveyor started to fail not being able to find OpenSSL,
even though it still claims that it's should be in the same
location:

checking for openssl/ssl.h in C:/OpenSSL-Win64... no
checking whether compiling and linking against OpenSSL works... no
configure: error: Cannot find openssl (use --disable-ssl to configure without 
SSL support)
Command exited with code 1

That's annoying.

Best regards, Ilya Maximets.

> 
> —
> Alin
> 
>> On 8 Jun 2023, at 11:43, Ilya Maximets  wrote:
>>
>> On 6/8/23 11:01, Alin Serdean wrote:
>>>
>>> Ack. I will try to see if I can address it by the end of the week.
>>>
>>> Thanks for clarifying the questions.
>>>
>>> Would GH actions be better than appveyor?
>>
>> Functionally, I don't think they are that much different.
>> But GHA is better integrated into our CI, i.e. ovsrobot
>> reports GHA status per patch on a patchwork, but it doesn't
>> do the same for appveyor.  So, having GHA job is probably
>> not a bad idea.  We discussed most of the pros and cons
>> about 2 years ago here:
>>  https://github.com/openvswitch/ovs-issues/issues/209
>> The main problem is that windows build is slow in general.
>> But that shouldn't be a blocker for CI.
>>
>> Best regards, Ilya Maximets.
>>
>>>
>>> —
>>> Alin.
>>>
 On 2 Jun 2023, at 12:25, Ilya Maximets  wrote:

 On 6/1/23 13:50, Ilya Maximets wrote:
>> On 5/31/23 23:05, Alin Serdean wrote:
>>
>> That makes sense.
>>
>> We can leverage the  following commit:
>>
>> https://patchwork.ozlabs.org/project/openvswitch/patch/20201013124655.1408-1-aserd...@cloudbasesolutions.com/
>>  
>> 
>>
>> But I still need to fix the permissions. I’ll try to find some time and 
>> address it.
>
> Thanks.  I actually forgot about this patch.  Would be great if you can
> send an updated version.  Would also be great to migrate to OpenSSL 3.0
> instead of 1.1.1 to not migrate again in some not so distant future.
> OpenSSL 3.0 should work fine, unless there are some other library changes
> (not OpenSSL 3.1, because OpenSSL 3.1 is not an LTS release).

 I'll mark  the current patch as 'rejected' for now in favor of the
 future OpenSSL update with your suggested change.

>
>> I remember there were some discussions to modernize the build system to 
>> meson or cmake. Was that effort fruitful in the end?
>
> The main issue with meson is that we still need automake/autotools for
> our testsuite.  We might invoke autotest from meson, but it sounds a
> bit strange to do that.  I agree that it is still beneficial in some
> cases to use meson, e.g. for a windows build, so might make sense to
> migrate anyway, but an attempt from 2021 didn't receive any follow ups.
>
> There was also a PR to add Windows build to GitHub Actions, but it
> didn't move since your request to Sign-off the changes.
>
> Best regards, Ilya Maximets.
>
>>
>> Alin.
>>
>>> On 31 May 2023, at 22:41, Ilya Maximets  wrote:
>>>
>>> On 5/31/23 22:36, Alin Serdean wrote:

 It would be best to change the link with the latest version of 
 OpenSSL. That will ensure there are no mishaps .
>>>
>>> I think the problem here is that slproweb.com only provides
>>> OpenSSL 1.1.1+ right now and our build system doesn't work
>>> with that version.  And I don't have enough experience with
>>> windows build in order to fix it...
>>>
>>> Best regards, Ilya Maximets.
>>>

 Alin.

>
> On 31 May 2023, at 21:23, Ilya Maximets  wrote:
>
> OpenSSL is already available in the exact location we need it [1].
> Also, the download itself fails for a long time already, because
> the version we're trying to download is not available.
>
> [1] https://www.appveyor.com/docs/windows-images-software/#tools

Re: [ovs-dev] [PATCH] ovsdb: raft: Support pre-vote mechanism to deal with disruptive server.

[Ilya Maximets]

On 6/25/23 19:35, Han Zhou wrote:
> When a server becomes unstable due to overloading or intermittent
> partitioning, it may miss some heartbeats and then starts election with
> a new term, which would disrupt the otherwise healthy cluster formed by
> the rest of the healthy nodes.  Such situation may exist for a long time
> until the "flapping" server is shutdown or recovered completely, which
> can severely impact the availability of the cluster. The pre-vote
> mechanism introduced in the raft paper section 9.6 can prevent such
> problems. This patch implements the pre-vote mechanism, in a way that is
> backward compatible and safe for upgrades.

Thanks, Han!  This is definitely interesting.  I don't think that this
should be a solution for overloading though, overloading should be fixed
by, well, reducing the load or increasing the election timer.  But some
intermittent network issues is indeed a valid case.

I looked through the code and I don't see any significant problems right
away, but I would like to experiment with it a bit more next week, since
the voting logic is fairly complex in general.

It's nice to see you found a use case for the stop-raft-rpc failure
test. :)

See some comments inline.

Best regards, Ilya Maximets.

> 
> Signed-off-by: Han Zhou 
> ---
>  ovsdb/raft-rpc.c   | 19 -
>  ovsdb/raft-rpc.h   |  3 ++
>  ovsdb/raft.c   | 88 ++
>  tests/ovsdb-cluster.at | 42 
>  4 files changed, 127 insertions(+), 25 deletions(-)
> 
> diff --git a/ovsdb/raft-rpc.c b/ovsdb/raft-rpc.c
> index dd14d81091fc..f750ba897046 100644
> --- a/ovsdb/raft-rpc.c
> +++ b/ovsdb/raft-rpc.c
> @@ -283,6 +283,10 @@ raft_vote_request_to_jsonrpc(const struct 
> raft_vote_request *rq,
>  json_object_put(args, "leadership_transfer",
>  json_boolean_create(true));
>  }
> +if (rq->is_prevote) {
> +json_object_put(args, "is_prevote",
> +json_boolean_create(true));
> +}
>  }
>  
>  static void
> @@ -294,6 +298,8 @@ raft_vote_request_from_jsonrpc(struct ovsdb_parser *p,
>  rq->last_log_term = raft_parse_required_uint64(p, "last_log_term");
>  rq->leadership_transfer
>  = raft_parse_optional_boolean(p, "leadership_transfer") == 1;
> +rq->is_prevote
> += raft_parse_optional_boolean(p, "is_prevote") == 1;
>  }
>  
>  static void
> @@ -305,6 +311,9 @@ raft_format_vote_request(const struct raft_vote_request 
> *rq, struct ds *s)
>  if (rq->leadership_transfer) {
>  ds_put_cstr(s, " leadership_transfer=true");
>  }
> +if (rq->is_prevote) {
> +ds_put_cstr(s, " is_prevote=true");
> +}
>  }
>  
>  /* raft_vote_reply. */
> @@ -326,6 +335,9 @@ raft_vote_reply_to_jsonrpc(const struct raft_vote_reply 
> *rpy,
>  {
>  raft_put_uint64(args, "term", rpy->term);
>  json_object_put_format(args, "vote", UUID_FMT, UUID_ARGS(>vote));
> +if (rpy->is_prevote) {
> +json_object_put(args, "is_prevote", json_boolean_create(true));
> +}
>  }
>  
>  static void
> @@ -334,6 +346,7 @@ raft_vote_reply_from_jsonrpc(struct ovsdb_parser *p,
>  {
>  rpy->term = raft_parse_required_uint64(p, "term");
>  rpy->vote = raft_parse_required_uuid(p, "vote");
> +rpy->is_prevote = raft_parse_optional_boolean(p, "is_prevote") == 1;
>  }
>  
>  static void
> @@ -341,6 +354,9 @@ raft_format_vote_reply(const struct raft_vote_reply *rpy, 
> struct ds *s)
>  {
>  ds_put_format(s, " term=%"PRIu64, rpy->term);
>  ds_put_format(s, " vote="SID_FMT, SID_ARGS(>vote));
> +if (rpy->is_prevote) {
> +ds_put_cstr(s, " is_prevote=true");
> +}
>  }
>  
>  /* raft_add_server_request */
> @@ -1008,7 +1024,8 @@ raft_rpc_get_vote(const union raft_rpc *rpc)
>  return NULL;
>  
>  case RAFT_RPC_VOTE_REPLY:
> -return _vote_reply_cast(rpc)->vote;
> +return raft_vote_reply_cast(rpc)->is_prevote ? NULL :
> +_vote_reply_cast(rpc)->vote;

It might be better to create a pointer variable here and access it
instead of casting twice.

>  
>  default:
>  OVS_NOT_REACHED();
> diff --git a/ovsdb/raft-rpc.h b/ovsdb/raft-rpc.h
> index 221f24d00128..10f30618e3e0 100644
> --- a/ovsdb/raft-rpc.h
> +++ b/ovsdb/raft-rpc.h
> @@ -125,12 +125,15 @@ struct raft_vote_request {
>  uint64_t last_log_index; /* Index of candidate's last log entry. */
>  uint64_t last_log_term;  /* Term of candidate's last log entry. */
>  bool leadership_transfer;  /* True to override minimum election timeout. 
> */
> +bool is_prevote; /* True: pre-vote; False: real vote (default). 
> */
>  };
>  
>  struct raft_vote_reply {
>  struct raft_rpc_common common;
>  uint64_t term;  /* Current term, for candidate to update itself. 
> */
>  struct uuid vote;   /* Server ID of vote. */
> +bool is_prevote;/* Copy the is_prevote from the request, 
> primarily
> + 

Re: [ovs-dev] Scale testing OVN with ovn-heater for OpenStack use cases

[Terry Wilson]

On Fri, Jun 30, 2023 at 2:26 AM Frode Nordahl
 wrote:
>
> Hello all,
>
> On Tue, May 30, 2023 at 5:16 PM Felix Huettner
>  wrote:
> >
> > Hi Dumitru,
> >
> > On Fri, May 26, 2023 at 01:30:54PM +0200, Dumitru Ceara wrote:
> > > On 5/24/23 09:37, Felix Huettner wrote:
> > > > Hi everyone,
> > >
> > > Hi Felix,
> > >
> > > >
> > > > Ilya mentioned to me that you will want to bring openstack examples to
> > > > ovn-heater.
> > > >
> > >
> > > Yes, we're discussing that.
> > >
> > > > I wanted to ask how to best join this effort. It would be great for us
> > >
> > > Everyone is welcome to contribute! :)
> > >
> >
> > Great, we will try that out.
>
> Thank you for your interest in this effort, as promised I'm in the
> process of organizing a community meeting to get this going.
>
> Below are some proposals for dates and times, please respond with a
> prioritized list of what works best for you and we'll try to land on a
> single date/time for a first meeting:
> Wednesday July 5th 13:00 UTC
> Tuesday July 11th 13:30 UTC
> Wednesday July 12th 8:30 UTC

I'd be interested in attending. Of these, Tuesday July 11th @ 13:30
works best for me. 08:30 would be 03:30 for me, which is a little
early. I could do Wed July 5th as well. It's the day after the the
July 4 holiday here, and a lot of people are making it a long weekend.
So there will be the inevitable "catch up" associated with that, but
it is doable.

Terry (otherwiseguy)

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

Re: [ovs-dev] [PATCH 3/3] checkpatch: print subject field if spelling error

[Aaron Conole]

Chandan Somani  writes:

> This will help narrow down spelling errors that are in the
> commit subject
>
> Signed-off-by: Chandan Somani 
> ---
>  utilities/checkpatch.py | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 57f7baf46..c5575ba00 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -1024,6 +1024,10 @@ def ovs_checkpatch_file(filename):
>  result = ovs_checkpatch_parse(part.get_payload(decode=False), filename,
>mail.get('Author', mail['From']),
>mail['Commit'])
> +if spellcheck:

We might also want to check for "Subject" in mail.  As an example, if we
hook up the checkpatch tests to test for spelling as well, they don't
provide a subject.  We could find a hacky way to add one, or only run
the test if the subject line is present.

> +if check_spelling(mail["Subject"], False):
> +print("Subject: %s" % mail["Subject"])
> +
>  ovs_checkpatch_print_result()
>  return result

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

Re: [ovs-dev] [PATCH 2/3] checkpatch: add suggestions to the spell checker

[Aaron Conole]

Chandan Somani  writes:

> This wil lbe useful for correcting possible spelling
> mistakes with ease. Suggestions limited to 3 at first,
> but configurable in the future
>
> Signed-off-by: Chandan Somani 
> ---

This is a really useful feature!  Thanks!

Acked-by: Aaron Conole 

>  utilities/checkpatch.py | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index bc5f696bb..57f7baf46 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -412,6 +412,7 @@ def check_spelling(line, comment):
>  words = words.replace(':', ' ').split(' ')
>  
>  flagged_words = []
> +num_suggestions = 3

Just a question, is there a way this can be made configurable?  I can
imagine only wanting more / less.

>  for word in words:
>  skip = False
> @@ -442,6 +443,8 @@ def check_spelling(line, comment):
>  if len(flagged_words > 0):
>  for mistake in flagged_words:
>  print_warning("Possible misspelled word: \"%s\"" % mistake)
> +print("Did you mean: ",
> +  spell_check_dict.suggest(mistake)[:num_suggestions])
>  
>  return True

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

Re: [ovs-dev] [PATCH 1/3] checkpatch: reorganize flagged words using a list

[Aaron Conole]

Chandan Somani  writes:

> A list approach lets us single out each flagged word and
> provide more useful details, like spelling suggestions.
> This will be used in an upcoming patch.
>
> Signed-off-by: Chandan Somani 
> ---
>  utilities/checkpatch.py | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 0d30b71b5..bc5f696bb 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -411,6 +411,8 @@ def check_spelling(line, comment):
>  words = filter_comments(line, True) if comment else line
>  words = words.replace(':', ' ').split(' ')
>  
> +flagged_words = []
> +
>  for word in words:
>  skip = False
>  strword = re.subn(r'\W+', '', word)[0].replace(',', '')
> @@ -435,9 +437,13 @@ def check_spelling(line, comment):
>  skip = True
>  
>  if not skip:
> -print_warning("Check for spelling mistakes (e.g. \"%s\")"
> -  % strword)
> -return True
> +flagged_words.append(strword)
> +
> +if len(flagged_words > 0):

This statement doesn't look right.  I think you meant:

if len(flagged_words) > 0:

> +for mistake in flagged_words:
> +print_warning("Possible misspelled word: \"%s\"" % mistake)
> +
> +return True
>  
>  return False

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

Re: [ovs-dev] Scale testing OVN with ovn-heater for OpenStack use cases

[Ihar Hrachyshka]

On Fri, Jun 30, 2023 at 3:31 AM Frode Nordahl 
wrote:

> Hello all,
>
> On Tue, May 30, 2023 at 5:16 PM Felix Huettner
>  wrote:
> >
> > Hi Dumitru,
> >
> > On Fri, May 26, 2023 at 01:30:54PM +0200, Dumitru Ceara wrote:
> > > On 5/24/23 09:37, Felix Huettner wrote:
> > > > Hi everyone,
> > >
> > > Hi Felix,
> > >
> > > >
> > > > Ilya mentioned to me that you will want to bring openstack examples
> to
> > > > ovn-heater.
> > > >
> > >
> > > Yes, we're discussing that.
> > >
> > > > I wanted to ask how to best join this effort. It would be great for
> us
> > >
> > > Everyone is welcome to contribute! :)
> > >
> >
> > Great, we will try that out.
>
> Thank you for your interest in this effort, as promised I'm in the
> process of organizing a community meeting to get this going.
>
> Below are some proposals for dates and times, please respond with a
> prioritized list of what works best for you and we'll try to land on a
> single date/time for a first meeting:
> Wednesday July 5th 13:00 UTC
> Tuesday July 11th 13:30 UTC
>

I'm interested in the topic. I am in EST and I don't think 8:30 UTC would
work for anyone in US / Canada. If possible, I'd suggest to hold it ~13 -
13:30 as the two options above.

Thanks!


> Wednesday July 12th 8:30 UTC
>
> > > > to have a tool to scale test ovn. Also we have an ovn deployment with
> > > > currently 450 compute nodes where we can try to extract the typically
> > > > used northbound entries from.
> > > >
> > >
> > > This is great!  I'm not an OpenStack user so I'm probably not using the
> > > right terminology and I might be missing things but I think we're
> > > interested in:
> > > - how many tenant networks do you have in the cluster
> > > - for each tenant network:
> > >   - how many VMs (on average but maybe also min/max/etc)
> > >   - how many floating IPs (dnat-and-snat)
> > >   - how many compute nodes run VMs from this network
> > > - for each compute:
> > >   - how many VMs run on it (on average but also min/max/etc)
> > >
> >
> > Ok, so i'll try to give an overview below.
> > If you need any more detail just ask.
> >
> > General amount of nodes:
> > * ~500 Hypervisors
> > * 9 Nodes that are just serve as gateway chassis (connected to 4
> >   different external networks)
> >
> > Amount of tenant Networks (== logical routers): 2300 (only 1400 are
> > actually hosting VMs).
> >
> > Amount of logical Routers: 2010 (2000 of these have a chassisredirect
> > port that is hosted on one of the 9 nodes.
> >
> > * Amount of VMs:
> >* min: 1
> >* max: 300
> >* avg: 4.5
> >* 630 networks only have a single VM, 230 have two VMs and 190 have 3
> >  VMs.
> > * Amount of Hypervisors per network:
> >* min: 1
> >* max: 90
> >* avg: 3.6
> >* 650 networks only strech on a single hypervisor, 250 on two
> >  hypervisors, 180 on three hypervisors
> >
> > Regarding floating ips we have currently 1530. I'll give you the amount
> > per Logical router:
> > * min: 1
> > * max: 230
> > * avg: 2.7
> > * 430 routers have a single floating ip, 140 routers have 2 floating
> >   ips.
> >
> > Regarding VMs per Hypervisor we have:
> > * min: 1
> > * max: 80
> > * avg: 15
> >
> >
> > Other potentially interesting values:
> >
> > * Amount of Load_Balancer: 0
> > * Amount of Logical_Switch_Port type=virtual: 800
> > * Amount of Port_Group: 6000
> > * Amount of ACL: 35500
> >* All of these referce portgroups in `match` using `@` or `$`
> >* Referencing other portgroups using `$`: 8300
>
> Thanks alot for providing this information, and gauging the size of
> your deployment, your inputs would be very valuable in this effort.
> One of the main goals of ovn-heater is the ability to simulate
> workloads, because as developers, we seldom have access to actual 500-
> or 1000- node clusters. Having a participant in the project with
> actual hardware installation of such a size would give us the
> opportunity to help confirm the accuracy of our simulations.
>
> --
> Frode Nordahl
>
> > Regards
> > Felix
> >
> > > Mayba Frode and Daniel can expand on this.
> > >
> > > Regards,
> > > Dumitru
> > >
> > > > Thanks
> > > > Felix
> > > >
> > > > On Tue, May 16, 2023 at 05:38:25PM +0200, Daniel Alvarez Sanchez
> wrote:
> > > >> Hey Frode, Dumitru, thanks a lot for initiating this
> > > >>
> > > >> On Thu, May 11, 2023 at 6:05 PM Frode Nordahl <
> frode.nord...@canonical.com>
> > > >> wrote:
> > > >>
> > > >>> Hello, Dumitru, Daniel and team,
> > > >>>
> > > >>> On Thu, May 11, 2023 at 5:23 PM Dumitru Ceara 
> wrote:
> > > 
> > >  Hi Frode,
> > > 
> > >  During an internal discussion with RedHat OpenStack folks (Daniel
> in cc)
> > >  about scalability of OVN in OpenStack deployments I remembered
> that you
> > >  mentioned that you're thinking of enhancing ovn-heater in order
> to test
> > >  your types of deployments [0].  I assumed that those are also
> OpenStack
> > >  based deployments so I was wondering if there's an opportunity for

[ovs-dev] [PATCH v2 1/4] dpif: stub out unimplemented action OVS_ACTION_ATTR_DEC_TTL

[Eric Garver]

This is prep for adding a different OVS_ACTION_ATTR_ enum value. This
action, OVS_ACTION_ATTR_DEC_TTL, is not actually implemented. However,
to make -Werror happy we must add a case to all existing switches.

Signed-off-by: Eric Garver 
---
 include/linux/openvswitch.h  | 1 +
 lib/dpif-netdev.c| 1 +
 lib/dpif.c   | 1 +
 lib/odp-execute.c| 2 ++
 lib/odp-util.c   | 2 ++
 ofproto/ofproto-dpif-ipfix.c | 1 +
 ofproto/ofproto-dpif-sflow.c | 1 +
 7 files changed, 9 insertions(+)

diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
index e305c331516b..a265e05ad253 100644
--- a/include/linux/openvswitch.h
+++ b/include/linux/openvswitch.h
@@ -1085,6 +1085,7 @@ enum ovs_action_attr {
OVS_ACTION_ATTR_CLONE,/* Nested OVS_CLONE_ATTR_*.  */
OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
+   OVS_ACTION_ATTR_DEC_TTL,  /* Nested OVS_DEC_TTL_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 abe63412ebfc..565cde5f5010 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -9193,6 +9193,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch 
*packets_,
 case OVS_ACTION_ATTR_POP_NSH:
 case OVS_ACTION_ATTR_CT_CLEAR:
 case OVS_ACTION_ATTR_CHECK_PKT_LEN:
+case OVS_ACTION_ATTR_DEC_TTL:
 case OVS_ACTION_ATTR_DROP:
 case OVS_ACTION_ATTR_ADD_MPLS:
 case __OVS_ACTION_ATTR_MAX:
diff --git a/lib/dpif.c b/lib/dpif.c
index b1cbf39c48d6..d328bf288de0 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1281,6 +1281,7 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch 
*packets_,
 case OVS_ACTION_ATTR_CT_CLEAR:
 case OVS_ACTION_ATTR_UNSPEC:
 case OVS_ACTION_ATTR_CHECK_PKT_LEN:
+case OVS_ACTION_ATTR_DEC_TTL:
 case OVS_ACTION_ATTR_DROP:
 case OVS_ACTION_ATTR_ADD_MPLS:
 case __OVS_ACTION_ATTR_MAX:
diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index 37f0f717af61..86f1ccb39bf9 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -837,6 +837,7 @@ requires_datapath_assistance(const struct nlattr *a)
 return false;
 
 case OVS_ACTION_ATTR_UNSPEC:
+case OVS_ACTION_ATTR_DEC_TTL:
 case __OVS_ACTION_ATTR_MAX:
 OVS_NOT_REACHED();
 }
@@ -1223,6 +1224,7 @@ odp_execute_actions(void *dp, struct dp_packet_batch 
*batch, bool steal,
 case OVS_ACTION_ATTR_RECIRC:
 case OVS_ACTION_ATTR_CT:
 case OVS_ACTION_ATTR_UNSPEC:
+case OVS_ACTION_ATTR_DEC_TTL:
 case __OVS_ACTION_ATTR_MAX:
 /* The following actions are handled by the scalar implementation. */
 case OVS_ACTION_ATTR_POP_VLAN:
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 2ec889c417e5..e1ca2cd0e02b 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -146,6 +146,7 @@ odp_action_len(uint16_t type)
 case OVS_ACTION_ATTR_DROP: return sizeof(uint32_t);
 
 case OVS_ACTION_ATTR_UNSPEC:
+case OVS_ACTION_ATTR_DEC_TTL:
 case __OVS_ACTION_ATTR_MAX:
 return ATTR_LEN_INVALID;
 }
@@ -1287,6 +1288,7 @@ format_odp_action(struct ds *ds, const struct nlattr *a,
 ds_put_cstr(ds, "drop");
 break;
 case OVS_ACTION_ATTR_UNSPEC:
+case OVS_ACTION_ATTR_DEC_TTL:
 case __OVS_ACTION_ATTR_MAX:
 default:
 format_generic_odp_action(ds, a);
diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index e6c2968f7e90..c71880e18122 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -3133,6 +3133,7 @@ dpif_ipfix_read_actions(const struct flow *flow,
 case OVS_ACTION_ATTR_POP_NSH:
 case OVS_ACTION_ATTR_CHECK_PKT_LEN:
 case OVS_ACTION_ATTR_UNSPEC:
+case OVS_ACTION_ATTR_DEC_TTL:
 case OVS_ACTION_ATTR_DROP:
 case OVS_ACTION_ATTR_ADD_MPLS:
 case __OVS_ACTION_ATTR_MAX:
diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
index a405eb0563fe..f026cf0c815c 100644
--- a/ofproto/ofproto-dpif-sflow.c
+++ b/ofproto/ofproto-dpif-sflow.c
@@ -1228,6 +1228,7 @@ dpif_sflow_read_actions(const struct flow *flow,
 case OVS_ACTION_ATTR_UNSPEC:
 case OVS_ACTION_ATTR_CHECK_PKT_LEN:
 case OVS_ACTION_ATTR_DROP:
+case OVS_ACTION_ATTR_DEC_TTL:
 case OVS_ACTION_ATTR_ADD_MPLS:
 case __OVS_ACTION_ATTR_MAX:
 default:
-- 
2.39.0

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

[ovs-dev] [PATCH v2 4/4] tests: system-traffic: add coverage for drop action

[Eric Garver]

Signed-off-by: Eric Garver 
---
 tests/system-traffic.at | 30 ++
 1 file changed, 30 insertions(+)

diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 4c378e1d02b0..31c1ef46d561 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -2048,6 +2048,36 @@ masks-cache:size:256
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_SETUP([datapath - drop action])
+OVS_TRAFFIC_VSWITCHD_START()
+OVS_CHECK_DROP_ACTION()
+AT_KEYWORDS(drop_action)
+
+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_DATA([flows.txt], [dnl
+table=0, dl_type=0x806, actions=normal
+table=0, in_port=ovs-p0, actions=goto_table:1
+table=1, in_port=ovs-p0, actions=goto_table:2
+table=2, in_port=ovs-p0, actions=resubmit(,1)
+])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+
+dnl generate some traffic
+NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2], [1], [ignore])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows | grep "eth_type(0x0800)" | dnl
+  sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/;s/,eth(),/,/;' | dnl
+  strip_recirc | strip_stats | strip_used | sort], [0], [dnl
+recirc_id(),in_port(2),eth_type(0x0800),ipv4(frag=no), packets:0, 
bytes:0, used:0.0s, actions:drop
+])
+
+OVS_VSWITCHD_STOP(["/|WARN|/d"])
+AT_CLEANUP
+
 AT_SETUP([datapath - simulated flow action update])
 OVS_TRAFFIC_VSWITCHD_START()
 
-- 
2.39.0

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

[ovs-dev] [PATCH v2 3/4] system-common-macros: check for drop action in datapath

[Eric Garver]

Signed-off-by: Eric Garver 
---
 tests/system-common-macros.at | 4 
 1 file changed, 4 insertions(+)

diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index 0077a8609c02..afc0df00a329 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -359,3 +359,7 @@ m4_define([OVS_CHECK_IPROUTE_ENCAP],
 # OVS_CHECK_CT_CLEAR()
 m4_define([OVS_CHECK_CT_CLEAR],
 [AT_SKIP_IF([! grep -q "Datapath supports ct_clear action" 
ovs-vswitchd.log])])
+
+# OVS_CHECK_DROP_ACTION()
+m4_define([OVS_CHECK_DROP_ACTION],
+[AT_SKIP_IF([! grep -q "Datapath supports drop action" ovs-vswitchd.log])])
-- 
2.39.0

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

[ovs-dev] [PATCH v2 0/4] dpif: probe support for OVS_ACTION_ATTR_DROP

[Eric Garver]

Probe the datapath implementation for support of OVS_ACTION_ATTR_DROP.
Also add a new test case.

v2:
  - new patch (1) to fix build (switch cases)
  - fixed check-system-userspace

Eric Garver (4):
  dpif: stub out unimplemented action OVS_ACTION_ATTR_DEC_TTL
  dpif: probe support for OVS_ACTION_ATTR_DROP
  system-common-macros: check for drop action in datapath
  tests: system-traffic: add coverage for drop action

 include/linux/openvswitch.h   |  3 ++-
 lib/dpif-netdev.c |  1 +
 lib/dpif.c|  7 +--
 lib/dpif.h|  1 -
 lib/odp-execute.c |  2 ++
 lib/odp-util.c|  2 ++
 ofproto/ofproto-dpif-ipfix.c  |  1 +
 ofproto/ofproto-dpif-sflow.c  |  1 +
 ofproto/ofproto-dpif.c| 34 --
 tests/system-common-macros.at |  4 
 tests/system-traffic.at   | 30 ++
 11 files changed, 76 insertions(+), 10 deletions(-)

-- 
2.39.0

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

[ovs-dev] [PATCH v2 2/4] dpif: probe support for OVS_ACTION_ATTR_DROP

[Eric Garver]

Signed-off-by: Eric Garver 
---
 include/linux/openvswitch.h |  2 +-
 lib/dpif.c  |  6 --
 lib/dpif.h  |  1 -
 ofproto/ofproto-dpif.c  | 34 --
 4 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
index a265e05ad253..fce6456f47c8 100644
--- a/include/linux/openvswitch.h
+++ b/include/linux/openvswitch.h
@@ -1086,11 +1086,11 @@ enum ovs_action_attr {
OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_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. */
 
 #ifndef __KERNEL__
OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
OVS_ACTION_ATTR_TUNNEL_POP,/* u32 port number. */
-   OVS_ACTION_ATTR_DROP,  /* u32 xlate_error. */
OVS_ACTION_ATTR_LB_OUTPUT, /* u32 bond-id. */
 #endif
__OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
diff --git a/lib/dpif.c b/lib/dpif.c
index d328bf288de0..dc02ec912693 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1928,12 +1928,6 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif)
 return dpif_is_netdev(dpif);
 }
 
-bool
-dpif_supports_explicit_drop_action(const struct dpif *dpif)
-{
-return dpif_is_netdev(dpif);
-}
-
 bool
 dpif_supports_lb_output_action(const struct dpif *dpif)
 {
diff --git a/lib/dpif.h b/lib/dpif.h
index 129cbf6a1d5f..fc1719f88b4f 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -938,7 +938,6 @@ 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_supports_explicit_drop_action(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 fad7342b0b02..c490a3c1da48 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1375,6 +1375,37 @@ check_ct_timeout_policy(struct dpif_backer *backer)
 return !error;
 }
 
+/* Tests whether backer's datapath supports the OVS_ACTION_ATTR_DROP action. */
+static bool
+check_drop_action(struct dpif_backer *backer)
+{
+struct odputil_keybuf keybuf;
+uint8_t actbuf[NL_A_U32_SIZE];
+struct ofpbuf actions;
+struct ofpbuf key;
+struct flow flow;
+bool supported;
+
+struct odp_flow_key_parms odp_parms = {
+.flow = ,
+.probe = true,
+};
+
+memset(, 0, sizeof flow);
+ofpbuf_use_stack(, , sizeof keybuf);
+odp_flow_key_from_flow(_parms, );
+
+ofpbuf_use_stack(, , sizeof actbuf);
+nl_msg_put_u32(, OVS_ACTION_ATTR_DROP, XLATE_OK);
+
+supported = dpif_probe_feature(backer->dpif, "drop", , , NULL);
+
+VLOG_INFO("%s: Datapath %s drop action",
+  dpif_name(backer->dpif), (supported) ? "supports"
+   : "does not support");
+return supported;
+}
+
 /* Tests whether 'backer''s datapath supports the all-zero SNAT case. */
 static bool
 dpif_supports_ct_zero_snat(struct dpif_backer *backer)
@@ -1627,8 +1658,7 @@ check_support(struct dpif_backer *backer)
 backer->rt_support.max_hash_alg = check_max_dp_hash_alg(backer);
 backer->rt_support.check_pkt_len = check_check_pkt_len(backer);
 backer->rt_support.ct_timeout = check_ct_timeout_policy(backer);
-backer->rt_support.explicit_drop_action =
-dpif_supports_explicit_drop_action(backer->dpif);
+backer->rt_support.explicit_drop_action = check_drop_action(backer);
 backer->rt_support.lb_output_action =
 dpif_supports_lb_output_action(backer->dpif);
 backer->rt_support.ct_zero_snat = dpif_supports_ct_zero_snat(backer);
-- 
2.39.0

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

Re: [ovs-dev] [PATCH ovn] mirror.c: Fix ovn-controller crash when mirror port is deleted from ovs.

[Han Zhou]

On Fri, Jun 30, 2023 at 1:07 AM Dumitru Ceara  wrote:
>
> On 6/29/23 08:42, Han Zhou wrote:
> > If the ovs port used as output_port in mirror is deleted (either by
> > mistake or intentionally), ovn-controller would crash in the
> > check_and_update_interface_table() when trying to sync the mirror port
> > configuration. e.g.:
> >
> > 0  0x00449d6e in check_and_update_interface_table
(ovs_mirror=0xcc5110, ovs_mirror=0xcc5110, sb_mirror=0xcc64b0) at
../controller/mirror.c:351
> > 1  sync_ovn_mirror (ovs_mirror_ports=0x7ffcf79b0280, br_int=0xbf5020,
ovs_idl_txn=0xe13570, m=0xce86b0) at ../controller/mirror.c:351
> > 2  mirror_run (ovs_idl_txn=0xe13570, ovs_mirror_table=,
sb_mirror_table=, br_int=0xbf5020, local_bindings=) at ../controller/mirror.c:174
> > 3  0x00409e8e in main (argc=, argv=) at ../controller/ovn-controller.c:5357
> >
> > Similarly, it can also crash in delete_ovs_mirror() if the mirror port
is
> > deleted together with src/dst port (when should_delete_ovs_mirror
returns
> > true).
> >
> > Both cases are captured in the updated test case of this patch.
> >
> > This patch fixes the problem by verifying the existance of the mirror
port, and
> > creates it if needed.
> >
> > Fixes: ba8aa26e44cb ("OVN Remote Port Mirroring: controller changes to
create ovs mirrors")
> > Signed-off-by: Han Zhou 
> > ---
>
> Looks good to me, thanks!
>
> Acked-by: Dumitru Ceara 
>
> Regards,
> Dumitru
>

Thanks Dumitru. I applied to main and backported down to branch-22.12.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5] ofproto-dpif-xlate: Reduce stack usage in recursive xlate functions

[Mike Pattrick]

On Fri, Jun 30, 2023 at 12:33 PM Ilya Maximets  wrote:
>
> On 6/30/23 18:13, Ilya Maximets wrote:
> > On 6/30/23 00:45, Mike Pattrick wrote:
> >> On Thu, Jun 29, 2023 at 3:10 PM Ilya Maximets  wrote:
> >>>
> >>> On 6/9/23 17:05, Mike Pattrick wrote:
>  Several xlate actions used in recursive translation currently store a
>  large amount of information on the stack. This can result in handler
>  threads quickly running out of stack space despite before
>  xlate_resubmit_resource_check() is able to terminate translation. This
>  patch reduces stack usage by over 3kb from several translation actions.
> >>>
> >>> I'm assuming that this number is mostly about patch_port_output function,
> >>> right?  I see only ~30% stack usage reduction for the main recursive
> >>> do_xlate_actions function.  It went from 720 to 512 bytes.
> >>>
> >>> Or am I calculating it wrong?  I'm using -fstack-size compiler option.
> >>
> >> I was using GDB for this, finding the difference in the stack pointer
> >> between calls to the same function during recursion. The intention of
> >> this patch is to get to xlate_resubmit_resource_check() without
> >> smashing the stack, no individual functions stack usage is as
> >> important as the holistic stack usage while recursing.
> >>
> >> For example, without patch:
> >>
> >> (gdb) fram 1
> >> #1  0x0044dee3 in xlate_table_action (...) at
> >> ofproto/ofproto-dpif-xlate.c:4539
> >> 4539if (xlate_resubmit_resource_check(ctx)) {
> >> (gdb) p $rsp
> >> $4 = (void *) 0x7f888e7aa700
> >> (gdb) frame 8
> >> #8  xlate_table_action (...) at ofproto/ofproto-dpif-xlate.c:4584
> >> 4584xlate_recursively(ctx, rule, table_id <= old_table_id,
> >> (gdb) p $rsp
> >> $5 = (void *) 0x7f888e7aba70
> >> (gdb) p 0x7f888e7aba70-0x7f888e7aa700
> >> $6 = 4976
> >>
> >> With patch:
> >>
> >> #1  0x0044e3bc in xlate_table_action (...) at
> >> ofproto/ofproto-dpif-xlate.c:4552
> >> 4552if (xlate_resubmit_resource_check(ctx)) {
> >> (gdb) p $rsp
> >> $4 = (void *) 0x7f78e69a9860
> >> (gdb) frame 9
> >> #9  0x0044ec00 in xlate_table_action (...) at
> >> ofproto/ofproto-dpif-xlate.c:7212
> >> 7212xlate_table_action(ctx, 
> >> ctx->xin->flow.in_port.ofp_port,
> >> (gdb) p $rsp
> >> $5 = (void *) 0x7f78e69a9c60
> >> (gdb) p 0x7f78e69a9c60-0x7f78e69a9860
> >> $6 = 1024
> >>
> >>
> >>>
>  This patch also moves some trace function from do_xlate_actions into its
>  own function to reduce stack usage.
> >>>
> >>> This doesn't seem to work.  With GCC and -O2, i.e. the standard 
> >>> optimization
> >>> level, this new function is always inlined for me, and there is no real
> >>> difference in the produced code and the stack usage.
> >>
> >> When I compile with O2, I get the following code generation:
> >>
> >> Without patch:
> >> 0044cda0 :
> >> [...]
> >>   44cdad:   48 81 ec 78 02 00 00subrsp,0x278
> >>
> >> With patch:
> >> 0044cff0 :
> >> [...]
> >>   44cffd:   48 81 ec 98 01 00 00subrsp,0x198
> >>
> >> Do you get different results?
> >
> > If we're just looking at the first sub in the function, I get the same
> > number (0x278) on current master and 0x148 with your patch applied.
> >
> > Moving the port name map to heap reduces the value to 0x128.
> > The xlate_trace function is fully inlined according to objdump.>
> >>
> >>>
> >>> In order to achieve actual reduction of stack usage there we need to move
> >>> to dynamic allocation of the port name map.q
> >>>
> >>> There are few more functions that are fully or partially inlined into
> >>> do_xlate_actions with default compiler options:
> >>>
> >>>  - xlate_output_reg_action: Allocates a large mf_subvalue on stack.
> >>>  - xlate_sample_action: Allocates huge struct flow_tnl(!) on stack.
> >>>  - compose_conntrack_action: Allocates mf_subvalue.
> >>>  - xlate_check_pkt_larger: Allocates mf_subvalue as well.
> >>>
> >>> With these moved to heap, it should be possible to reduce the stack usage
> >>> by about 60% for do_xlate_actions function.
> >>
> >> These don't inline for me on O2. What GCC version are you using?
> >
> > If I do changes in the functions above, the number is dropped to 0xd8.
> >
> > I'm building with gcc (GCC) 13.1.1 20230511 (Red Hat 13.1.1-2) on F38.
> > CFLAGS="-g -O2".
>
> Did a round of tests with clang version 16.0.4 (Fedora 16.0.4-1.fc38)
> and the same CFLAGS.  It's not so aggressive with inlining, so it doesn't
> inline the xlate_trace.  However, that doesn't affect the initial stack
> allocation.
>
> Master: 0x258
> With patch: 0x258  - No difference with master

That isn't fantastic! I've confirmed the same behaviour with clang.

I'll send in a new version.

-M

>
> With extra changes in functions listed above: 0x128
>
> >
> > Best regards, Ilya Maximets.
>

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

Re: [ovs-dev] [PATCH v1] bridge ovs-vsctl Bridge IPFIX enable_input_sampling, enable_ouput_sampling fix unexpected values

[Sayali Naval (sanaval) via dev]

Thanks. I'll look through the resources and submit a new, correctly formatted 
patch soon.

From: Ilya Maximets 
Sent: Friday, June 30, 2023 3:39 AM
To: Sayali Naval (sanaval) ; d...@openvswitch.org 

Cc: i.maxim...@ovn.org 
Subject: Re: [ovs-dev] [PATCH v1] bridge ovs-vsctl Bridge IPFIX 
enable_input_sampling, enable_ouput_sampling fix unexpected values

On 6/21/23 20:43, Sayali Naval (sanaval) via dev wrote:
> A gentle reminder to take a look at this patch.

Hi.  Sorry for a late reply.  But this patch came in strangely formatted,
so it wasn't recognized as a patch by any of our systems.

The patch has extra spacing after every line that makes it not readable
by automated tools and hence not applicable.

Could you try re-sending it?  Maybe re-configure your or use a different
mail client for that, e.g. git send-email.

There are some hints in the contribution guide:
  
https://docs.openvswitch.org/en/latest/internals/contributing/submitting-patches/

And some hints on mail client configuration from a kernel guide:
  https://static.lwn.net/kerneldoc/process/email-clients.html


For example, here is how your patch looks like:
  https://mail.openvswitch.org/pipermail/ovs-dev/2023-May/405130.html

And here is an example on how the patch formatting should look like:
  https://mail.openvswitch.org/pipermail/ovs-dev/2023-June/405786.html

Best regards, Ilya Maximets.

>
> Thanks,
> Sayali Naval
> 
> From: Sayali Naval (sanaval)
> Sent: Wednesday, May 31, 2023 11:02 AM
> To: d...@openvswitch.org 
> Subject: [ovs-dev] [PATCH v1] bridge ovs-vsctl Bridge IPFIX 
> enable_input_sampling, enable_ouput_sampling fix unexpected values
>
>
> As per the Open vSwitch Manual 
> (http://www.openvswitch.org/support/dist-docs/ovs-vsctl.8.txt) the Bridge 
> IPFIX parameters can be passed as follows:
>
>
> ovs-vsctl -- set Bridge br0 ipfix=@i \
>
>   --  --id=@i  create  IPFIX targets=\"192.168.0.34:4739\" obs_do‐
>
>   main_id=123   obs_point_id=456   cache_active_timeout=60
>
>   cache_max_flows=13 \
>
>   other_config:enable-input-sampling=false \
>
>   other_config:enable-output-sampling=false
>
>
> where the default values are:
>
> enable_input_sampling: true
>
> enable_output_sampling: true
>
>
> But in the existing code 
> https://github.com/openvswitch/ovs/blob/master/vswitchd/bridge.c#L1563-L1567, 
> these 2 parameters take up unexpected values in some scenarios.
>
>
>
> be_opts.enable_input_sampling = !smap_get_bool(_cfg->other_config,
>
>   "enable-input-sampling", false);
>
>
> be_opts.enable_output_sampling = !smap_get_bool(_cfg->other_config,
>
>   "enable-output-sampling", 
> false);
>
>
> Here, the function smap_get_bool is being used with a negation.
>
>
> smap_get_bool is defined as below:
>
> (https://github.com/openvswitch/ovs/blob/master/lib/smap.c#L220-L232)
>
>
> /* Gets the value associated with 'key' in 'smap' and converts it to a 
> boolean.
>
>  * If 'key' is not in 'smap', or its value is neither "true" nor "false",
>
>  * returns 'def'. */
>
> bool
>
> smap_get_bool(const struct smap *smap, const char *key, bool def)
>
> {
>
> const char *value = smap_get_def(smap, key, "");
>
> if (def) {
>
> return strcasecmp("false", value) != 0;
>
> } else {
>
> return !strcasecmp("true", value);
>
> }
>
> }
>
>
> This returns expected values for the default case (since the above code will 
> negate “false” we get from smap_get bool function and return the value 
> “true”) but unexpected values for the case where the sampling value is passed 
> through the CLI.
>
> For example, if we pass "true" for other_config:enable-input-sampling in the 
> CLI, the above code will negate the “true” value we get from the smap_bool 
> function and return the value “false”. Same would be the case for 
> enable_output_sampling.
>
>
> Signed-off-by: Sayali Naval 
>
> ---
>
> [bridge-ipfix-fix 277b3baae] Fix values for enable_input_sampling & 
> enable_ouput_sampling
>
>  Date: Thu May 25 10:32:43 2023 -0700
>
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
>
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
>
> index f5dc59ad0..b972d55d0 100644
>
> --- a/vswitchd/bridge.c
>
> +++ b/vswitchd/bridge.c
>
> @@ -1560,11 +1560,11 @@ bridge_configure_ipfix(struct bridge *br)
>
>  be_opts.enable_tunnel_sampling = smap_get_bool(_cfg->other_config,
>
>   "enable-tunnel-sampling", true);
>
>
>
> -be_opts.enable_input_sampling = !smap_get_bool(_cfg->other_config,
>
> -  "enable-input-sampling", 
> false);
>
> +be_opts.enable_input_sampling = smap_get_bool(_cfg->other_config,
>
> +  "enable-input-sampling", true);
>
>
>

Re: [ovs-dev] [PATCH v2] ovs-vsctl: Exit with error if postdb checks report errors.

[Flavio Leitner]

On 6/30/23 12:31, Aaron Conole wrote:

Flavio Leitner  writes:


Today the exit code refers to the execution of the change
in the database. However, when not using parameter --no-wait
(default), the ovs-vsctl also checks if OVSDB transactions
are successfully recorded and reload by ovs-vswitchd. In this
case, an error message is printed if there is a problem during
the reload, like for example the one below:

  # ovs-vsctl add-port br0 gre0 -- \
 set interface gre0 type=ip6gre options:key=100 \
 options:remote_ip=2001::2
ovs-vsctl: Error detected while setting up 'gre0': could not \
add network device gre0 to ofproto (Address family not supported\
by protocol).  See ovs-vswitchd log for details.
ovs-vsctl: The default log directory is "/var/log/openvswitch".
  # echo $?
0

This patch changes to exit with specific error code '65'
(EX_DATAERR) if errors were reported during the reload.

This change may break existing scripts because ovs-vsctl will
start to fail when before it was succeeding. However, if an
error is printed, then it is likely that the change was not
functional anyway.

Reported-at: https://bugzilla.redhat.com/1731553
Signed-off-by: Flavio Leitner 
---

LGTM.  I did a quick double check for FreeBSD and Mac OS X, and the
error code is the same value as on linux systems.

I don't have a windows machine to test with and unfortunately the robot
doesn't build series_* branches on appveyor (maybe something to look
at).  We may need a workaround for windows - but I'll let Alin take a
look.

Acked-by: Aaron Conole 


Thanks for reviewing and testing it.

fbl







v2:
 Followed Aaron's suggestion to return EX_DATAERR.

  NEWS |  4 
  tests/ovs-vsctl.at   | 19 +--
  tests/ovs-vswitchd.at|  2 +-
  tests/tunnel.at  |  2 +-
  utilities/ovs-vsctl.8.in |  2 ++
  utilities/ovs-vsctl.c| 30 --
  6 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/NEWS b/NEWS
index 66d5a4ea3..cb148a09f 100644
--- a/NEWS
+++ b/NEWS
@@ -28,6 +28,10 @@ Post-v3.1.0
   * Added new options --[ovsdb-server|ovs-vswitchd]-umask=MODE to set umask
 value when starting OVS daemons.  E.g., use --ovsdb-server-umask=0002
 in order to create OVSDB sockets with access mode of 0770.
+   - ovs-vsctl:
+ * Exit with error code 65 (EX_DATAERR) if errors were reported while
+   checking if OVSDB transactions are successfully recorded and reload
+   by ovs-vswitchd.
 - QoS:
   * Added new configuration option 'jitter' for a linux-netem QoS type.
 - DPDK:
diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
index a368bff6e..b282798cc 100644
--- a/tests/ovs-vsctl.at
+++ b/tests/ovs-vsctl.at
@@ -1522,7 +1522,7 @@ cat >experr experr << EOF
+ovs-vsctl: Error detected while setting up 'gre0': gre0: bad ip6gre 'remote_ip'
+gre0: ip6gre type requires valid 'remote_ip' argument.  See ovs-vswitchd log 
for details.
+ovs-vsctl: The default log directory is "$OVS_RUNDIR".
+EOF
+AT_CHECK([ovs-vsctl add-port br0 gre0 -- set interface gre0 type=ip6gre 
options:key=100 options:remote_ip=192.168.0.300], [65], [], [experr])
+OVS_VSWITCHD_STOP(["/is not a valid IP address/d
+/netdev_vport|WARN|gre0: bad ip6gre 'remote_ip'/d
+/netdev|WARN|gre0: could not set configuration/d"])
+AT_CLEANUP
diff --git a/tests/ovs-vswitchd.at b/tests/ovs-vswitchd.at
index 977b2eba1..80a748355 100644
--- a/tests/ovs-vswitchd.at
+++ b/tests/ovs-vswitchd.at
@@ -222,7 +222,7 @@ cat >experr <  
  OVS_VSWITCHD_STOP(['/ignoring bridge with invalid name/d'])

diff --git a/tests/tunnel.at b/tests/tunnel.at
index ddeb66bc9..d281c9e6c 100644
--- a/tests/tunnel.at
+++ b/tests/tunnel.at
@@ -1009,7 +1009,7 @@ OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 
type=vxlan \
  options:remote_ip=flow ofport_request=1])
  
  AT_CHECK([ovs-vsctl add-port br0 p2 -- set Interface p2 type=vxlan \

-options:remote_ip=flow options:exts=gbp options:key=1 
ofport_request=2], [0],
+options:remote_ip=flow options:exts=gbp options:key=1 
ofport_request=2], [65],
[], [ignore])
  
  AT_CHECK([grep 'p2: could not set configuration (File exists)' ovs-vswitchd.log | sed "s/^.*\(p2:.*\)$/\1/"], [0],

diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
index 9e319aa1c..7c7e7bc29 100644
--- a/utilities/ovs-vsctl.8.in
+++ b/utilities/ovs-vsctl.8.in
@@ -892,6 +892,8 @@ Usage, syntax, or configuration file error.
  .IP "2"
  The \fIbridge\fR argument to \fBbr\-exists\fR specified the name of a
  bridge that does not exist.
+.IP "65"
+An error has been reported post OVSDB reload.
  .SH "SEE ALSO"
  .
  .BR ovsdb\-server (1),
diff --git a/utilities/ovs-vsctl.c 

Re: [ovs-dev] [PATCH v5] ofproto-dpif-xlate: Reduce stack usage in recursive xlate functions

[Ilya Maximets]

On 6/30/23 18:13, Ilya Maximets wrote:
> On 6/30/23 00:45, Mike Pattrick wrote:
>> On Thu, Jun 29, 2023 at 3:10 PM Ilya Maximets  wrote:
>>>
>>> On 6/9/23 17:05, Mike Pattrick wrote:
 Several xlate actions used in recursive translation currently store a
 large amount of information on the stack. This can result in handler
 threads quickly running out of stack space despite before
 xlate_resubmit_resource_check() is able to terminate translation. This
 patch reduces stack usage by over 3kb from several translation actions.
>>>
>>> I'm assuming that this number is mostly about patch_port_output function,
>>> right?  I see only ~30% stack usage reduction for the main recursive
>>> do_xlate_actions function.  It went from 720 to 512 bytes.
>>>
>>> Or am I calculating it wrong?  I'm using -fstack-size compiler option.
>>
>> I was using GDB for this, finding the difference in the stack pointer
>> between calls to the same function during recursion. The intention of
>> this patch is to get to xlate_resubmit_resource_check() without
>> smashing the stack, no individual functions stack usage is as
>> important as the holistic stack usage while recursing.
>>
>> For example, without patch:
>>
>> (gdb) fram 1
>> #1  0x0044dee3 in xlate_table_action (...) at
>> ofproto/ofproto-dpif-xlate.c:4539
>> 4539if (xlate_resubmit_resource_check(ctx)) {
>> (gdb) p $rsp
>> $4 = (void *) 0x7f888e7aa700
>> (gdb) frame 8
>> #8  xlate_table_action (...) at ofproto/ofproto-dpif-xlate.c:4584
>> 4584xlate_recursively(ctx, rule, table_id <= old_table_id,
>> (gdb) p $rsp
>> $5 = (void *) 0x7f888e7aba70
>> (gdb) p 0x7f888e7aba70-0x7f888e7aa700
>> $6 = 4976
>>
>> With patch:
>>
>> #1  0x0044e3bc in xlate_table_action (...) at
>> ofproto/ofproto-dpif-xlate.c:4552
>> 4552if (xlate_resubmit_resource_check(ctx)) {
>> (gdb) p $rsp
>> $4 = (void *) 0x7f78e69a9860
>> (gdb) frame 9
>> #9  0x0044ec00 in xlate_table_action (...) at
>> ofproto/ofproto-dpif-xlate.c:7212
>> 7212xlate_table_action(ctx, ctx->xin->flow.in_port.ofp_port,
>> (gdb) p $rsp
>> $5 = (void *) 0x7f78e69a9c60
>> (gdb) p 0x7f78e69a9c60-0x7f78e69a9860
>> $6 = 1024
>>
>>
>>>
 This patch also moves some trace function from do_xlate_actions into its
 own function to reduce stack usage.
>>>
>>> This doesn't seem to work.  With GCC and -O2, i.e. the standard optimization
>>> level, this new function is always inlined for me, and there is no real
>>> difference in the produced code and the stack usage.
>>
>> When I compile with O2, I get the following code generation:
>>
>> Without patch:
>> 0044cda0 :
>> [...]
>>   44cdad:   48 81 ec 78 02 00 00subrsp,0x278
>>
>> With patch:
>> 0044cff0 :
>> [...]
>>   44cffd:   48 81 ec 98 01 00 00subrsp,0x198
>>
>> Do you get different results?
> 
> If we're just looking at the first sub in the function, I get the same
> number (0x278) on current master and 0x148 with your patch applied.
> 
> Moving the port name map to heap reduces the value to 0x128.
> The xlate_trace function is fully inlined according to objdump.> 
>>
>>>
>>> In order to achieve actual reduction of stack usage there we need to move
>>> to dynamic allocation of the port name map.q
>>>
>>> There are few more functions that are fully or partially inlined into
>>> do_xlate_actions with default compiler options:
>>>
>>>  - xlate_output_reg_action: Allocates a large mf_subvalue on stack.
>>>  - xlate_sample_action: Allocates huge struct flow_tnl(!) on stack.
>>>  - compose_conntrack_action: Allocates mf_subvalue.
>>>  - xlate_check_pkt_larger: Allocates mf_subvalue as well.
>>>
>>> With these moved to heap, it should be possible to reduce the stack usage
>>> by about 60% for do_xlate_actions function.
>>
>> These don't inline for me on O2. What GCC version are you using?
> 
> If I do changes in the functions above, the number is dropped to 0xd8.
> 
> I'm building with gcc (GCC) 13.1.1 20230511 (Red Hat 13.1.1-2) on F38.
> CFLAGS="-g -O2".

Did a round of tests with clang version 16.0.4 (Fedora 16.0.4-1.fc38)
and the same CFLAGS.  It's not so aggressive with inlining, so it doesn't
inline the xlate_trace.  However, that doesn't affect the initial stack
allocation.

Master: 0x258
With patch: 0x258  - No difference with master

With extra changes in functions listed above: 0x128

> 
> Best regards, Ilya Maximets.

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

Re: [ovs-dev] [PATCH v5] ofproto-dpif-xlate: Reduce stack usage in recursive xlate functions

[Ilya Maximets]

On 6/30/23 00:45, Mike Pattrick wrote:
> On Thu, Jun 29, 2023 at 3:10 PM Ilya Maximets  wrote:
>>
>> On 6/9/23 17:05, Mike Pattrick wrote:
>>> Several xlate actions used in recursive translation currently store a
>>> large amount of information on the stack. This can result in handler
>>> threads quickly running out of stack space despite before
>>> xlate_resubmit_resource_check() is able to terminate translation. This
>>> patch reduces stack usage by over 3kb from several translation actions.
>>
>> I'm assuming that this number is mostly about patch_port_output function,
>> right?  I see only ~30% stack usage reduction for the main recursive
>> do_xlate_actions function.  It went from 720 to 512 bytes.
>>
>> Or am I calculating it wrong?  I'm using -fstack-size compiler option.
> 
> I was using GDB for this, finding the difference in the stack pointer
> between calls to the same function during recursion. The intention of
> this patch is to get to xlate_resubmit_resource_check() without
> smashing the stack, no individual functions stack usage is as
> important as the holistic stack usage while recursing.
> 
> For example, without patch:
> 
> (gdb) fram 1
> #1  0x0044dee3 in xlate_table_action (...) at
> ofproto/ofproto-dpif-xlate.c:4539
> 4539if (xlate_resubmit_resource_check(ctx)) {
> (gdb) p $rsp
> $4 = (void *) 0x7f888e7aa700
> (gdb) frame 8
> #8  xlate_table_action (...) at ofproto/ofproto-dpif-xlate.c:4584
> 4584xlate_recursively(ctx, rule, table_id <= old_table_id,
> (gdb) p $rsp
> $5 = (void *) 0x7f888e7aba70
> (gdb) p 0x7f888e7aba70-0x7f888e7aa700
> $6 = 4976
> 
> With patch:
> 
> #1  0x0044e3bc in xlate_table_action (...) at
> ofproto/ofproto-dpif-xlate.c:4552
> 4552if (xlate_resubmit_resource_check(ctx)) {
> (gdb) p $rsp
> $4 = (void *) 0x7f78e69a9860
> (gdb) frame 9
> #9  0x0044ec00 in xlate_table_action (...) at
> ofproto/ofproto-dpif-xlate.c:7212
> 7212xlate_table_action(ctx, ctx->xin->flow.in_port.ofp_port,
> (gdb) p $rsp
> $5 = (void *) 0x7f78e69a9c60
> (gdb) p 0x7f78e69a9c60-0x7f78e69a9860
> $6 = 1024
> 
> 
>>
>>> This patch also moves some trace function from do_xlate_actions into its
>>> own function to reduce stack usage.
>>
>> This doesn't seem to work.  With GCC and -O2, i.e. the standard optimization
>> level, this new function is always inlined for me, and there is no real
>> difference in the produced code and the stack usage.
> 
> When I compile with O2, I get the following code generation:
> 
> Without patch:
> 0044cda0 :
> [...]
>   44cdad:   48 81 ec 78 02 00 00subrsp,0x278
> 
> With patch:
> 0044cff0 :
> [...]
>   44cffd:   48 81 ec 98 01 00 00subrsp,0x198
> 
> Do you get different results?

If we're just looking at the first sub in the function, I get the same
number (0x278) on current master and 0x148 with your patch applied.

Moving the port name map to heap reduces the value to 0x128.
The xlate_trace function is fully inlined according to objdump.

> 
>>
>> In order to achieve actual reduction of stack usage there we need to move
>> to dynamic allocation of the port name map.q
>>
>> There are few more functions that are fully or partially inlined into
>> do_xlate_actions with default compiler options:
>>
>>  - xlate_output_reg_action: Allocates a large mf_subvalue on stack.
>>  - xlate_sample_action: Allocates huge struct flow_tnl(!) on stack.
>>  - compose_conntrack_action: Allocates mf_subvalue.
>>  - xlate_check_pkt_larger: Allocates mf_subvalue as well.
>>
>> With these moved to heap, it should be possible to reduce the stack usage
>> by about 60% for do_xlate_actions function.
> 
> These don't inline for me on O2. What GCC version are you using?

If I do changes in the functions above, the number is dropped to 0xd8.

I'm building with gcc (GCC) 13.1.1 20230511 (Red Hat 13.1.1-2) on F38.
CFLAGS="-g -O2".

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

Re: [ovs-dev] [PATCH v2] ovs-vsctl: Exit with error if postdb checks report errors.

[Aaron Conole]

Flavio Leitner  writes:

> Today the exit code refers to the execution of the change
> in the database. However, when not using parameter --no-wait
> (default), the ovs-vsctl also checks if OVSDB transactions
> are successfully recorded and reload by ovs-vswitchd. In this
> case, an error message is printed if there is a problem during
> the reload, like for example the one below:
>
>  # ovs-vsctl add-port br0 gre0 -- \
> set interface gre0 type=ip6gre options:key=100 \
> options:remote_ip=2001::2
> ovs-vsctl: Error detected while setting up 'gre0': could not \
> add network device gre0 to ofproto (Address family not supported\
> by protocol).  See ovs-vswitchd log for details.
> ovs-vsctl: The default log directory is "/var/log/openvswitch".
>  # echo $?
> 0
>
> This patch changes to exit with specific error code '65'
> (EX_DATAERR) if errors were reported during the reload.
>
> This change may break existing scripts because ovs-vsctl will
> start to fail when before it was succeeding. However, if an
> error is printed, then it is likely that the change was not
> functional anyway.
>
> Reported-at: https://bugzilla.redhat.com/1731553
> Signed-off-by: Flavio Leitner 
> ---

LGTM.  I did a quick double check for FreeBSD and Mac OS X, and the
error code is the same value as on linux systems.

I don't have a windows machine to test with and unfortunately the robot
doesn't build series_* branches on appveyor (maybe something to look
at).  We may need a workaround for windows - but I'll let Alin take a
look.

Acked-by: Aaron Conole 

> v2:
> Followed Aaron's suggestion to return EX_DATAERR.
>
>  NEWS |  4 
>  tests/ovs-vsctl.at   | 19 +--
>  tests/ovs-vswitchd.at|  2 +-
>  tests/tunnel.at  |  2 +-
>  utilities/ovs-vsctl.8.in |  2 ++
>  utilities/ovs-vsctl.c| 30 --
>  6 files changed, 49 insertions(+), 10 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 66d5a4ea3..cb148a09f 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -28,6 +28,10 @@ Post-v3.1.0
>   * Added new options --[ovsdb-server|ovs-vswitchd]-umask=MODE to set 
> umask
> value when starting OVS daemons.  E.g., use --ovsdb-server-umask=0002
> in order to create OVSDB sockets with access mode of 0770.
> +   - ovs-vsctl:
> + * Exit with error code 65 (EX_DATAERR) if errors were reported while
> +   checking if OVSDB transactions are successfully recorded and reload
> +   by ovs-vswitchd.
> - QoS:
>   * Added new configuration option 'jitter' for a linux-netem QoS type.
> - DPDK:
> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> index a368bff6e..b282798cc 100644
> --- a/tests/ovs-vsctl.at
> +++ b/tests/ovs-vsctl.at
> @@ -1522,7 +1522,7 @@ cat >experr <  ovs-vsctl: Error detected while setting up 'reserved_name'.  See 
> ovs-vswitchd log for details.
>  ovs-vsctl: The default log directory is "$OVS_RUNDIR".
>  EOF
> -AT_CHECK([ovs-vsctl add-port br0 reserved_name], [0], [], [experr])
> +AT_CHECK([ovs-vsctl add-port br0 reserved_name], [65], [], [experr])
>  # Prevent race.
>  OVS_WAIT_UNTIL([test `grep -- "|WARN|" ovs-vswitchd.log | wc -l` -ge 1])
>  # Detect the warning log message
> @@ -1560,7 +1560,7 @@ cat >experr <  ovs-vsctl: Error detected while setting up 'reserved_name'.  See 
> ovs-vswitchd log for details.
>  ovs-vsctl: The default log directory is "$OVS_RUNDIR".
>  EOF
> -AT_CHECK([ovs-vsctl add-port br0 reserved_name], [0], [], [experr])
> +AT_CHECK([ovs-vsctl add-port br0 reserved_name], [65], [], [experr])
>  # Prevent race.
>  OVS_WAIT_UNTIL([test `grep -- "|WARN|" ovs-vswitchd.log | wc -l` -ge 1])
>  # Detect the warning log message
> @@ -1737,3 +1737,18 @@ AT_CHECK([ovs-vsctl --no-wait --bare --columns 
> _uuid,name list bridge tst1], [0]
>  
>  OVS_VSCTL_CLEANUP
>  AT_CLEANUP
> +
> +AT_SETUP([ovs-vsctl -- return error if OVSDB reload issues are reported])
> +OVS_VSWITCHD_START
> +dnl check if ovs-vsctl returns error 65 if ovs-vswitchd fails to reload.
> +
> +cat >experr << EOF
> +ovs-vsctl: Error detected while setting up 'gre0': gre0: bad ip6gre 
> 'remote_ip'
> +gre0: ip6gre type requires valid 'remote_ip' argument.  See ovs-vswitchd log 
> for details.
> +ovs-vsctl: The default log directory is "$OVS_RUNDIR".
> +EOF
> +AT_CHECK([ovs-vsctl add-port br0 gre0 -- set interface gre0 type=ip6gre 
> options:key=100 options:remote_ip=192.168.0.300], [65], [], [experr])
> +OVS_VSWITCHD_STOP(["/is not a valid IP address/d
> +/netdev_vport|WARN|gre0: bad ip6gre 'remote_ip'/d
> +/netdev|WARN|gre0: could not set configuration/d"])
> +AT_CLEANUP
> diff --git a/tests/ovs-vswitchd.at b/tests/ovs-vswitchd.at
> index 977b2eba1..80a748355 100644
> --- a/tests/ovs-vswitchd.at
> +++ b/tests/ovs-vswitchd.at
> @@ -222,7 +222,7 @@ cat >experr <  ovs-vsctl: Error detected while setting up 'b/c'.  See ovs-vswitchd log for 
> details.
>  ovs-vsctl: The default log directory is "$OVS_RUNDIR".
>  EOF
> -AT_CHECK([ovs-vsctl 

Re: [ovs-dev] [PATCH v3] python: Add async DNS support

[Ilya Maximets]

On 6/30/23 16:54, Adrian Moreno wrote:
> 
> 
> On 6/30/23 14:35, Ilya Maximets wrote:
>> On 6/30/23 14:23, Adrian Moreno wrote:
>>>
>>>
>>> On 6/30/23 13:40, Ilya Maximets wrote:
 On 6/30/23 12:39, Adrian Moreno wrote:
>
>
> On 6/14/23 23:07, Terry Wilson wrote:
>> This adds a Python version of the async DNS support added in:
>>
>> 771680d96 DNS: Add basic support for asynchronous DNS resolving
>>
>> The above version uses the unbound C library, and this
>> implimentation uses the SWIG-wrapped Python version of that.
>>
>> In the event that the Python unbound library is not available,
>> a warning will be logged and the resolve() method will just
>> return None. For the case where inet_parse_active() is passed
>> an IP address, it will not try to resolve it, so existing
>> behavior should be preserved in the case that the unbound
>> library is unavailable.
>>
>> Intentional differences from the C version are as follows:
>>
>>  OVS_HOSTS_FILE environment variable can bet set to override
>>  the system 'hosts' file. This is primarily to allow testing to
>>  be done without requiring network connectivity.
>>
>>  Since resolution can still be done via hosts file lookup, DNS
>>  lookups are not disabled when resolv.conf cannot be loaded.
>>
>>  The Python socket_util module has fallen behind its C equivalent.
>>  The bare minimum change was done to inet_parse_active() to support
>>  sync/async dns, as there is no equivalent to
>>  parse_sockaddr_components(), inet_parse_passive(), etc. A TODO
>>  was added to bring socket_util.py up to equivalency to the C
>>  version.
>>
>> Signed-off-by: Terry Wilson 
>> ---
>> .github/workflows/build-and-test.yml|   4 +-
>> Documentation/intro/install/general.rst |   4 +-
>> Documentation/intro/install/rhel.rst|   2 +-
>> Documentation/intro/install/windows.rst |   2 +-
>> NEWS|   4 +-
>> debian/control.in   |   1 +
>> m4/openvswitch.m4   |   8 +-
>> python/TODO.rst |   7 +
>> python/automake.mk  |   2 +
>> python/ovs/dns_resolve.py   | 272 +++
>> python/ovs/socket_util.py   |  21 +-
>> python/ovs/stream.py|   2 +-
>> python/ovs/tests/test_dns_resolve.py| 280 
>> 
>> python/setup.py |   6 +-
>> rhel/openvswitch-fedora.spec.in |   2 +-
>> tests/vlog.at   |   2 +
>> 16 files changed, 601 insertions(+), 18 deletions(-)
>> create mode 100644 python/ovs/dns_resolve.py
>> create mode 100644 python/ovs/tests/test_dns_resolve.py
>>
>> diff --git a/.github/workflows/build-and-test.yml 
>> b/.github/workflows/build-and-test.yml
>> index f66ab43b0..47d239f10 100644
>> --- a/.github/workflows/build-and-test.yml
>> +++ b/.github/workflows/build-and-test.yml
>> @@ -183,10 +183,10 @@ jobs:
>>   run:  sudo apt update || true
>> - name: install common dependencies
>>   run:  sudo apt install -y ${{ env.dependencies }}
>> -- name: install libunbound libunwind
>> +- name: install libunbound libunwind python3-unbound
>>   # GitHub Actions doesn't have 32-bit versions of these 
>> libraries.
>>   if:   matrix.m32 == ''
>> -  run:  sudo apt install -y libunbound-dev libunwind-dev
>> +  run:  sudo apt install -y libunbound-dev libunwind-dev 
>> python3-unbound
>> - name: install 32-bit libraries
>>   if:   matrix.m32 != ''
>>   run:  sudo apt install -y gcc-multilib
>> diff --git a/Documentation/intro/install/general.rst 
>> b/Documentation/intro/install/general.rst
>> index 42b5682fd..19e360d47 100644
>> --- a/Documentation/intro/install/general.rst
>> +++ b/Documentation/intro/install/general.rst
>> @@ -90,7 +90,7 @@ need the following software:
>>   If libcap-ng is installed, then Open vSwitch will automatically 
>> build with
>>   support for it.
>> 
>> -- Python 3.4 or later.
>> +- Python 3.6 or later.
>> 
>> - Unbound library, from http://www.unbound.net, is optional but 
>> recommended if
>>   you want to enable ovs-vswitchd and other utilities to use DNS 
>> names when
>> @@ -208,7 +208,7 @@ simply install and run Open vSwitch you require the 
>> following software:
>>   from iproute2 (part of all major distributions and available at
>>   https://wiki.linuxfoundation.org/networking/iproute2).
>> 

Re: [ovs-dev] [PATCH ovn] ovn-northd.at: Fix the LSP incremental processing test case.

[Han Zhou]

On Fri, Jun 30, 2023 at 5:53 AM Dumitru Ceara  wrote:
>
> On 6/28/23 08:22, Han Zhou wrote:
> > The test case intended to ensure there are no more than 3 failures in 10
> > runs. However, it used "break" to exit the loop whenever a failure is
> > encountered, end up with at most 1 failure and so the final check for
> > the failure count will always pass. It should use "continue" instead.
> >
> > Fixes: 8c30ba13869e ("ovn-northd.at: Fix unstable LSP incremental
processing test.")
> > Signed-off-by: Han Zhou 
> > ---
>
> Hi Han,
>
> This specific change looks good to me:
>
> Acked-by: Dumitru Ceara 
>
> However, I think the "70% of the time" part of the test is still
> optimistic.  I ran the test in a loop 20 times and it failed after 10
> iterations or so.
>
> I changed it to 50% locally and it passed all 20 iterations.
>
> Do you agree to reducing it to 50%?
>
Agree. Thanks for your review and the extra testing. I changed it to 50%
and applied it to main.
(Hopefully an upcoming patch will make it 100% predicatible).

Regards,
Han

> Thanks,
> Dumitru
>
> >  tests/ovn-northd.at | 12 ++--
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index ada2d2a4aa5e..88fe93a4e51f 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -9522,34 +9522,34 @@ for i in $(seq 10); do
> >  ovs-vsctl add-port br-int lsp${i}-0 -- set interface lsp${i}-0
external_ids:iface-id=lsp${i}-0
> >  wait_for_ports_up
> >  check ovn-nbctl --wait=hv sync
> > -check_recompute_counter 5 5 || break
> > +check_recompute_counter 5 5 || continue
> >
> >  check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> >  check ovn-nbctl --wait=hv lsp-add ls$i lsp${i}-1 --
lsp-set-addresses lsp${i}-1 "aa:aa:aa:00:00:01 192.168.0.11"
> >  ovs-vsctl add-port br-int lsp${i}-1 -- set interface lsp${i}-1
external_ids:iface-id=lsp${i}-1
> >  wait_for_ports_up
> >  check ovn-nbctl --wait=hv sync
> > -check_recompute_counter 0 0 || break
> > +check_recompute_counter 0 0 || continue
> >
> >  check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> >  check ovn-nbctl --wait=hv lsp-add ls$i lsp${i}-2 --
lsp-set-addresses lsp${i}-2 "aa:aa:aa:00:00:02 192.168.0.12"
> >  ovs-vsctl add-port br-int lsp${i}-2 -- set interface lsp${i}-2
external_ids:iface-id=lsp${i}-2
> >  wait_for_ports_up
> >  check ovn-nbctl --wait=hv sync
> > -check_recompute_counter 0 0 || break
> > +check_recompute_counter 0 0 || continue
> >
> >  check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> >  check ovn-nbctl --wait=hv lsp-del lsp${i}-1
> > -check_recompute_counter 0 1 || break
> > +check_recompute_counter 0 1 || continue
> >
> >  check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> >  check ovn-nbctl --wait=hv lsp-set-addresses lsp${i}-2
"aa:aa:aa:00:00:88 192.168.0.88"
> > -check_recompute_counter 0 1 || break
> > +check_recompute_counter 0 1 || continue
> >
> >  # No change, no recompute
> >  check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> >  check ovn-nbctl --wait=sb sync
> > -check_recompute_counter 0 0 || break
> > +check_recompute_counter 0 0 || continue
> >
> >  done
> >  echo Test failed $fail_count in 10.
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3] python: Add async DNS support

[Adrian Moreno]

On 6/30/23 14:35, Ilya Maximets wrote:

On 6/30/23 14:23, Adrian Moreno wrote:



On 6/30/23 13:40, Ilya Maximets wrote:

On 6/30/23 12:39, Adrian Moreno wrote:



On 6/14/23 23:07, Terry Wilson wrote:

This adds a Python version of the async DNS support added in:

771680d96 DNS: Add basic support for asynchronous DNS resolving

The above version uses the unbound C library, and this
implimentation uses the SWIG-wrapped Python version of that.

In the event that the Python unbound library is not available,
a warning will be logged and the resolve() method will just
return None. For the case where inet_parse_active() is passed
an IP address, it will not try to resolve it, so existing
behavior should be preserved in the case that the unbound
library is unavailable.

Intentional differences from the C version are as follows:

 OVS_HOSTS_FILE environment variable can bet set to override
 the system 'hosts' file. This is primarily to allow testing to
 be done without requiring network connectivity.

 Since resolution can still be done via hosts file lookup, DNS
 lookups are not disabled when resolv.conf cannot be loaded.

 The Python socket_util module has fallen behind its C equivalent.
 The bare minimum change was done to inet_parse_active() to support
 sync/async dns, as there is no equivalent to
 parse_sockaddr_components(), inet_parse_passive(), etc. A TODO
 was added to bring socket_util.py up to equivalency to the C
 version.

Signed-off-by: Terry Wilson 
---
.github/workflows/build-and-test.yml|   4 +-
Documentation/intro/install/general.rst |   4 +-
Documentation/intro/install/rhel.rst|   2 +-
Documentation/intro/install/windows.rst |   2 +-
NEWS|   4 +-
debian/control.in   |   1 +
m4/openvswitch.m4   |   8 +-
python/TODO.rst |   7 +
python/automake.mk  |   2 +
python/ovs/dns_resolve.py   | 272 +++
python/ovs/socket_util.py   |  21 +-
python/ovs/stream.py|   2 +-
python/ovs/tests/test_dns_resolve.py| 280 
python/setup.py |   6 +-
rhel/openvswitch-fedora.spec.in |   2 +-
tests/vlog.at   |   2 +
16 files changed, 601 insertions(+), 18 deletions(-)
create mode 100644 python/ovs/dns_resolve.py
create mode 100644 python/ovs/tests/test_dns_resolve.py

diff --git a/.github/workflows/build-and-test.yml 
b/.github/workflows/build-and-test.yml
index f66ab43b0..47d239f10 100644
--- a/.github/workflows/build-and-test.yml
+++ b/.github/workflows/build-and-test.yml
@@ -183,10 +183,10 @@ jobs:
  run:  sudo apt update || true
- name: install common dependencies
  run:  sudo apt install -y ${{ env.dependencies }}
-- name: install libunbound libunwind
+- name: install libunbound libunwind python3-unbound
  # GitHub Actions doesn't have 32-bit versions of these libraries.
  if:   matrix.m32 == ''
-  run:  sudo apt install -y libunbound-dev libunwind-dev
+  run:  sudo apt install -y libunbound-dev libunwind-dev python3-unbound
- name: install 32-bit libraries
  if:   matrix.m32 != ''
  run:  sudo apt install -y gcc-multilib
diff --git a/Documentation/intro/install/general.rst 
b/Documentation/intro/install/general.rst
index 42b5682fd..19e360d47 100644
--- a/Documentation/intro/install/general.rst
+++ b/Documentation/intro/install/general.rst
@@ -90,7 +90,7 @@ need the following software:
  If libcap-ng is installed, then Open vSwitch will automatically build with
  support for it.

-- Python 3.4 or later.

+- Python 3.6 or later.

- Unbound library, from http://www.unbound.net, is optional but recommended if

  you want to enable ovs-vswitchd and other utilities to use DNS names when
@@ -208,7 +208,7 @@ simply install and run Open vSwitch you require the 
following software:
  from iproute2 (part of all major distributions and available at
  https://wiki.linuxfoundation.org/networking/iproute2).

-- Python 3.4 or later.

+- Python 3.6 or later.

On Linux you should ensure that ``/dev/urandom`` exists. To support TAP

devices, you must also ensure that ``/dev/net/tun`` exists.
diff --git a/Documentation/intro/install/rhel.rst 
b/Documentation/intro/install/rhel.rst
index d1fc42021..f2151d890 100644
--- a/Documentation/intro/install/rhel.rst
+++ b/Documentation/intro/install/rhel.rst
@@ -92,7 +92,7 @@ Once that is completed, remove the file ``/tmp/ovs.spec``.
If python3-sphinx package is not available in your version of RHEL, you can
install it via pip with 'pip install sphinx'.

-Open vSwitch requires python 3.4 or newer which is not available in older

+Open vSwitch requires python 3.6 or 

[ovs-dev] [PATCH v2] ovs-vsctl: Exit with error if postdb checks report errors.

[Flavio Leitner]

Today the exit code refers to the execution of the change
in the database. However, when not using parameter --no-wait
(default), the ovs-vsctl also checks if OVSDB transactions
are successfully recorded and reload by ovs-vswitchd. In this
case, an error message is printed if there is a problem during
the reload, like for example the one below:

 # ovs-vsctl add-port br0 gre0 -- \
set interface gre0 type=ip6gre options:key=100 \
options:remote_ip=2001::2
ovs-vsctl: Error detected while setting up 'gre0': could not \
add network device gre0 to ofproto (Address family not supported\
by protocol).  See ovs-vswitchd log for details.
ovs-vsctl: The default log directory is "/var/log/openvswitch".
 # echo $?
0

This patch changes to exit with specific error code '65'
(EX_DATAERR) if errors were reported during the reload.

This change may break existing scripts because ovs-vsctl will
start to fail when before it was succeeding. However, if an
error is printed, then it is likely that the change was not
functional anyway.

Reported-at: https://bugzilla.redhat.com/1731553
Signed-off-by: Flavio Leitner 
---

v2:
Followed Aaron's suggestion to return EX_DATAERR.

 NEWS |  4 
 tests/ovs-vsctl.at   | 19 +--
 tests/ovs-vswitchd.at|  2 +-
 tests/tunnel.at  |  2 +-
 utilities/ovs-vsctl.8.in |  2 ++
 utilities/ovs-vsctl.c| 30 --
 6 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/NEWS b/NEWS
index 66d5a4ea3..cb148a09f 100644
--- a/NEWS
+++ b/NEWS
@@ -28,6 +28,10 @@ Post-v3.1.0
  * Added new options --[ovsdb-server|ovs-vswitchd]-umask=MODE to set umask
value when starting OVS daemons.  E.g., use --ovsdb-server-umask=0002
in order to create OVSDB sockets with access mode of 0770.
+   - ovs-vsctl:
+ * Exit with error code 65 (EX_DATAERR) if errors were reported while
+   checking if OVSDB transactions are successfully recorded and reload
+   by ovs-vswitchd.
- QoS:
  * Added new configuration option 'jitter' for a linux-netem QoS type.
- DPDK:
diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
index a368bff6e..b282798cc 100644
--- a/tests/ovs-vsctl.at
+++ b/tests/ovs-vsctl.at
@@ -1522,7 +1522,7 @@ cat >experr experr <
 #include 
 #include 
+#include 
 #include 
 
 #include "db-ctl-base.h"
@@ -56,6 +57,9 @@
 
 VLOG_DEFINE_THIS_MODULE(vsctl);
 
+/* Post OVSDB reload error reported. */
+#define EXIT_POSTDB_ERROR EX_DATAERR
+
 struct vsctl_context;
 
 /* --db: The database server to contact. */
@@ -115,7 +119,7 @@ static void parse_options(int argc, char *argv[], struct 
shash *local_options);
 static void run_prerequisites(struct ctl_command[], size_t n_commands,
   struct ovsdb_idl *);
 static bool do_vsctl(const char *args, struct ctl_command *, size_t n,
- struct ovsdb_idl *);
+ struct ovsdb_idl *, bool *);
 
 /* post_db_reload_check frame work is to allow ovs-vsctl to do additional
  * checks after OVSDB transactions are successfully recorded and reload by
@@ -134,11 +138,13 @@ static bool do_vsctl(const char *args, struct ctl_command 
*, size_t n,
  * Current implementation only check for Post OVSDB reload failures on new
  * interface additions with 'add-br' and 'add-port' commands.
  *
+ * post_db_reload_check returns 'true' if a failure is reported.
+ *
  * post_db_reload_expect_iface()
  *
  * keep track of interfaces to be checked post OVSDB reload. */
 static void post_db_reload_check_init(void);
-static void post_db_reload_do_checks(const struct vsctl_context *);
+static bool post_db_reload_do_checks(const struct vsctl_context *);
 static void post_db_reload_expect_iface(const struct ovsrec_interface *);
 
 static struct uuid *neoteric_ifaces;
@@ -200,9 +206,15 @@ main(int argc, char *argv[])
 }
 
 if (seqno != ovsdb_idl_get_seqno(idl)) {
+bool postdb_err;
+
 seqno = ovsdb_idl_get_seqno(idl);
-if (do_vsctl(args, commands, n_commands, idl)) {
+if (do_vsctl(args, commands, n_commands, idl, _err)) {
 free(args);
+if (postdb_err) {
+

Re: [ovs-dev] Scale testing OVN with ovn-heater for OpenStack use cases

[Dumitru Ceara]

On 6/30/23 09:25, Frode Nordahl wrote:
> Hello all,
> 
> On Tue, May 30, 2023 at 5:16 PM Felix Huettner
>  wrote:
>>
>> Hi Dumitru,
>>
>> On Fri, May 26, 2023 at 01:30:54PM +0200, Dumitru Ceara wrote:
>>> On 5/24/23 09:37, Felix Huettner wrote:
 Hi everyone,
>>>
>>> Hi Felix,
>>>

 Ilya mentioned to me that you will want to bring openstack examples to
 ovn-heater.

>>>
>>> Yes, we're discussing that.
>>>
 I wanted to ask how to best join this effort. It would be great for us
>>>
>>> Everyone is welcome to contribute! :)
>>>
>>
>> Great, we will try that out.
> 
> Thank you for your interest in this effort, as promised I'm in the
> process of organizing a community meeting to get this going.
> 
> Below are some proposals for dates and times, please respond with a
> prioritized list of what works best for you and we'll try to land on a
> single date/time for a first meeting:
> Wednesday July 5th 13:00 UTC
> Tuesday July 11th 13:30 UTC
> Wednesday July 12th 8:30 UTC
> 

Wednesday July 12th 8:30 UTC
Wednesday July 5th 13:00 UTC
Tuesday July 11th 13:30 UTC (I only have a 30 min slot available here)

Thanks for organizing this, Frode!

 to have a tool to scale test ovn. Also we have an ovn deployment with
 currently 450 compute nodes where we can try to extract the typically
 used northbound entries from.

>>>
>>> This is great!  I'm not an OpenStack user so I'm probably not using the
>>> right terminology and I might be missing things but I think we're
>>> interested in:
>>> - how many tenant networks do you have in the cluster
>>> - for each tenant network:
>>>   - how many VMs (on average but maybe also min/max/etc)
>>>   - how many floating IPs (dnat-and-snat)
>>>   - how many compute nodes run VMs from this network
>>> - for each compute:
>>>   - how many VMs run on it (on average but also min/max/etc)
>>>
>>
>> Ok, so i'll try to give an overview below.
>> If you need any more detail just ask.
>>
>> General amount of nodes:
>> * ~500 Hypervisors
>> * 9 Nodes that are just serve as gateway chassis (connected to 4
>>   different external networks)
>>
>> Amount of tenant Networks (== logical routers): 2300 (only 1400 are
>> actually hosting VMs).
>>
>> Amount of logical Routers: 2010 (2000 of these have a chassisredirect
>> port that is hosted on one of the 9 nodes.
>>
>> * Amount of VMs:
>>* min: 1
>>* max: 300
>>* avg: 4.5
>>* 630 networks only have a single VM, 230 have two VMs and 190 have 3
>>  VMs.
>> * Amount of Hypervisors per network:
>>* min: 1
>>* max: 90
>>* avg: 3.6
>>* 650 networks only strech on a single hypervisor, 250 on two
>>  hypervisors, 180 on three hypervisors
>>
>> Regarding floating ips we have currently 1530. I'll give you the amount
>> per Logical router:
>> * min: 1
>> * max: 230
>> * avg: 2.7
>> * 430 routers have a single floating ip, 140 routers have 2 floating
>>   ips.
>>
>> Regarding VMs per Hypervisor we have:
>> * min: 1
>> * max: 80
>> * avg: 15
>>
>>
>> Other potentially interesting values:
>>
>> * Amount of Load_Balancer: 0
>> * Amount of Logical_Switch_Port type=virtual: 800
>> * Amount of Port_Group: 6000
>> * Amount of ACL: 35500
>>* All of these referce portgroups in `match` using `@` or `$`
>>* Referencing other portgroups using `$`: 8300
> 
> Thanks alot for providing this information, and gauging the size of
> your deployment, your inputs would be very valuable in this effort.
> One of the main goals of ovn-heater is the ability to simulate
> workloads, because as developers, we seldom have access to actual 500-
> or 1000- node clusters. Having a participant in the project with
> actual hardware installation of such a size would give us the
> opportunity to help confirm the accuracy of our simulations.
> 

+1

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

Re: [ovs-dev] ovs-vsctl Bridge IPFIX enable_input_sampling, enable_ouput_sampling and enable_tunnel_sampling returning unexpected values

[Thomas Bachman]

Hi,

It wasn't clear to me from the responses whether there was an action item -
is something needed from the submitte? )Or is the patch under consideration?

cheers,

-Thomas


On Wed, May 24, 2023 at 9:08 AM Adrian Moreno  wrote:

>
>
> On 5/23/23 22:26, Ilya Maximets wrote:
> > On 5/23/23 20:51, Sayali Naval (sanaval) via dev wrote:
> >> Does anyone have any insights on the below?
> >> 
> >> From: Sayali Naval (sanaval)
> >> Sent: Friday, May 12, 2023 11:10 AM
> >> To: d...@openvswitch.org 
> >> Subject: [ovs-dev] ovs-vsctl Bridge IPFIX enable_input_sampling,
> enable_ouput_sampling and enable_tunnel_sampling returning unexpected values
> >>
> >>
> >> As per the Open vSwitch Manual (
> http://www.openvswitch.org/support/dist-docs/ovs-vsctl.8.txt) the Bridge
> IPFIX parameters can be passed as follows:
> >>
> >> ovs-vsctl -- set Bridge br0 ipfix=@i \
> >>--  --id=@i  create  IPFIX targets=\"192.168.0.34:4739\"
> obs_do‐
> >>main_id=123   obs_point_id=456
>  cache_active_timeout=60
> >>cache_max_flows=13 \
> >>other_config:enable-input-sampling=false \
> >>other_config:enable-output-sampling=false \
> >>other_config:enable-tunnel-sampling=true
> >>
> >> where the default values are:
> >>
> >> enable_input_sampling: true
> >> enable_output_sampling: true
> >> enable_tunnel_sampling: false
> >>
>
> I think wording is very confusing. In ovs-vsctl(8), before the example you
> provide, it says:
> "
> Configure bridge br0 to send one IPFIX flow record per packet
> sample to UDP port 4739 on host 192.168.0.34, with Observation Domain
> ID 123 and Observation Point ID 456, a flow cache active timeout of 1
> minute (60 seconds), maximum flow cache size of 13 flows, and flows
> sampled on output port with tunnel info(sampling on input and output
> port is enabled by default if not disabled) :
> "
>
> It doesn't explicitly say what's the default value for
> "other_config:enable-tunnel-sampling". On the other hand, the
> ovs-vswitchd.conf.db(5) says:
>
> "
> other_config : enable-input-sampling: optional string, either true or false
>  By default, Open vSwitch samples and reports flows at bridge port
> input in
> IPFIX flow records. Set this column to false to disable input sampling.
>
> other_config : enable-output-sampling: optional string, either true or
> false
>  By default, Open vSwitch samples and reports flows at bridge port
> output in
> IPFIX flow records. Set this column to false to disable output sampling.
> "
>
> One might interpret this as 'you can only set this to "false", since
> "true" is
> the default value'. If it was possible to forbid the user to put "true" in
> the
> field, maybe the current implementation would be reasonable. But it's not.
>
> Now for the tunnel-sampling:
> "
> other_config : enable-tunnel-sampling: optional string, either true or
> false
>  Set to true to enable sampling and reporting tunnel header 7-tuples
> in
> IPFIX flow records. Tunnel sampling is enabled by default.
> "
>
> So the default value is true after all!
>
>
> >> But in the existing code
> https://github.com/openvswitch/ovs/blob/master/vswitchd/bridge.c#L1560-L1567,
> these 3 parameters take up unexpected values in some scenarios.
> >>
> >> Consider the following case for enable_input_sampling and
> enable_output_sampling:
> >>
> >>  be_opts.enable_input_sampling =
> !smap_get_bool(_cfg->other_config,
> >>"enable-input-sampling",
> false);
> >>
> >>  be_opts.enable_output_sampling =
> !smap_get_bool(_cfg->other_config,
> >>
> "enable-output-sampling", false);
> >>
> >> Here, the function smap_get_bool is being used with a negation.
> >>
> >> smap_get_bool is defined as below:
> >> (https://github.com/openvswitch/ovs/blob/master/lib/smap.c#L220-L232)
> >>
> >> /* Gets the value associated with 'key' in 'smap' and converts it to a
> boolean.
> >>   * If 'key' is not in 'smap', or its value is neither "true" nor
> "false",
> >>   * returns 'def'. */
> >> bool
> >> smap_get_bool(const struct smap *smap, const char *key, bool def)
> >> {
> >>  const char *value = smap_get_def(smap, key, "");
> >>
> >>  if (def) {
> >>  return strcasecmp("false", value) != 0;
> >>  } else {
> >>  return !strcasecmp("true", value);
> >>  }
> >> }
> >>
> >> This returns expected values for the default case (since the above code
> will negate “false” we get from smap_get bool function and return the value
> “true”) but unexpected values for the case where the sampling value is
> passed through the CLI.
> >>
> >> For example, if we pass "true" for other_config:enable-input-sampling
> in the CLI, the above code will negate the “true” value we get from the
> smap_bool function and return the value “false”. Same would be the case for
> enable_output_sampling.
> >>
> >> My proposed solution is as below:
> 

Re: [ovs-dev] [PATCH ovn] Expose distributed gateway port information in NB DB

[Lucas Martins]

 Hi all,

On Thu, Apr 27, 2023 at 7:08 PM Han Zhou  wrote:
>
>
>
> On Thu, Apr 27, 2023 at 10:15 AM Ihar Hrachyshka  wrote:
> >
> > On Wed, Apr 19, 2023 at 5:17 AM Lucas Martins  wrote:
> > >
> > > Hi Han, all
> > >
> > > On Mon, Apr 17, 2023 at 8:02 PM Han Zhou  wrote:
> > > >
> > > >
> > > >
> > > > On Mon, Apr 17, 2023 at 7:18 AM Lucas Martins  
> > > > wrote:
> > > > >
> > > > > Thanks all for the discussion and all the ideas here.
> > > > >
> > > > > After reading the emails, I think it boils down to two proposed 
> > > > > approaches:
> > > > >
> > > > > 1) CMS to continue to connect to the Southbound database if they need
> > > > > information about the physical location of the resources. That would
> > > > > avoid the inefficiency of having to copy data back-and-forth from the
> > > > > Northbound and Southbound database.
> > > > >
> > > > > I guess the downside of this approach is that CMS will have to
> > > > > maintain a connection with both databases (which is already the case
> > > > > today).
> > > > >
> > > > > If we go with this approach, it would be good to have consensus from
> > > > > the core OVN team where some tables in the Southbound must be kept
> > > > > stable with backward compatibility in case of changes. Tables such
> > > > > Chassis, Chassis_Private and Port_Binding at least will need that. I
> > > > > guess that makes part of the Southbound database to not be considered
> > > > > OVN internal only.
> > > >
> > > > It doesn't look very clean to expose the SB DB to CMS, but in practice 
> > > > I think it is not a real problem for doing so for keeping tables 
> > > > backward compatible, because even without considering CMS, OVN itself 
> > > > needs to keep the compatibility for upgrading.
> > > > I am still open to the idea of keeping things in NB DB only, but at 
> > > > least one issue not addressed so far in this discussion (even with the 
> > > > status DB) is how to manage the orphan chassis if SB is not exposed to 
> > > > CMS. It seems still more practical to me to let CMS connect to SB 
> > > > directly instead of introducing a copy of Chassis table in NB and 
> > > > ovn-northd doing the back-and-forth data sync. So I am not sure if it 
> > > > should be a goal to remove all the SB access from CMS.
> > > >
> > >
> > > Yeah the Chassis situation in OVN is a strange one, indeed. Given that
> > > ovn-controller is the one creating it, not the CMS, shouldn't it be up
> > > to OVN to actually manage that table ? Perhaps we need to introduce
> > > some health check where ovn-controller could ping his own entry in the
> > > Chassis table from time to time to indicate that it's alive. And,
> > > ovn-northd could clean up the orphan entries if they are not updated
> > > in X amount of time.
>
> Thanks for the idea. In fact OVN has the "control-plane ping" mechanism using 
> the nb_cfg which has similar idea, but it is mostly used for testing purposes 
> and never recommended to be used in large scale production, because this 
> would add a lot of write operations from all ovn-controllers to SB DB, which 
> may increase the load significantly (depending on the number of nodes and 
> frequency of this ping) to the SB which is already a bottle-neck.
> There can also be false-positives when ovn-controller is busy and slow (due 
> to scenarios of recomputing within a large scale environment), which would 
> make the things worse.
> On the other hand, an interval too long would also result in a cleanup too 
> late, and then a new chassis (sometimes because of reimaging of the same 
> node) would conflict with the old chassis.
> In fact, the CMS is the one that has the best knowledge when a chassis should 
> be cleaned.
>
> >
> > (Random thought) NB could have an API (ChassisDeprovisionRequest?)
> > that would be used by CMS to request cleanup for a chassis by name.
> > Northd could then update the object with the status of the request, or
> > delete it once it's processed.
> >
>
> This may work, but I'd avoid any imperative approach if possible. NB (and 
> also SB) should just maintain the desired states and let the 
> controllers/daemons to converge.
> A "request" may be failed/timeout/invalid/out-of-date, which needs to be 
> transactional and will be relatively complex to handle.
>
> If the direct connection to SB is really the only concern, I think the most 
> viable approach is still to add a chassis table in NB just for CMS to specify 
> the valid chassis that should exist, and ovn-northd (probably with a new 
> thread) will take care of the cleanup. This is simple and scalable. The only 
> question is whether it's worth making this change.
>
> > >
> > > Or piggybacking on the idea of the Status DB, maybe those health
> > > checks could be done in that DB instead ?
> > >
> > > > On the other hand, giving another thought, it doesn't sound too much 
> > > > extra cost for propagating the "hosting-chassis" information for 
> > > > chassis-redirect ports back to NB, considering 

Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action

[Simon Horman]

On Fri, Jun 30, 2023 at 08:29:58AM -0400, Eric Garver wrote:
> On Fri, Jun 30, 2023 at 11:47:04AM +0200, Simon Horman wrote:
> > On Thu, Jun 29, 2023 at 04:30:05PM -0400, Eric Garver wrote:
> > > This adds an explicit drop action. This is used by OVS to drop packets
> > > for which it cannot determine what to do. An explicit action in the
> > > kernel allows passing the reason _why_ the packet is being dropped. We
> > > can then use perf tracing to match on the drop reason.
> > > 
> > > e.g. trace all OVS dropped skbs
> > > 
> > >  # perf trace -e skb:kfree_skb --filter="reason >= 0x3"
> > >  [..]
> > >  106.023 ping/2465 skb:kfree_skb(skbaddr: 0xa0e8765f2000, \
> > >   location:0xc0d9b462, protocol: 2048, reason: 196610)
> > > 
> > > reason: 196610 --> 0x30002 (OVS_XLATE_RECURSION_TOO_DEEP)
> > > 
> > > Signed-off-by: Eric Garver 
> > 
> > ...
> > 
> > > --- a/net/openvswitch/actions.c
> > > +++ b/net/openvswitch/actions.c
> > > @@ -32,6 +32,7 @@
> > >  #include "vport.h"
> > >  #include "flow_netlink.h"
> > >  #include "openvswitch_trace.h"
> > > +#include "drop.h"
> > >  
> > >  struct deferred_action {
> > >   struct sk_buff *skb;
> > > @@ -1477,6 +1478,18 @@ static int do_execute_actions(struct datapath *dp, 
> > > struct sk_buff *skb,
> > >   return dec_ttl_exception_handler(dp, skb,
> > >key, a);
> > >   break;
> > > +
> > > + case OVS_ACTION_ATTR_DROP:
> > > + u32 reason = nla_get_u32(a);
> > > +
> > > + reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
> > > + SKB_DROP_REASON_SUBSYS_SHIFT;
> > > +
> > > + if (reason == OVS_XLATE_OK)
> > > + break;
> > > +
> > > + kfree_skb_reason(skb, reason);
> > > + return 0;
> > >   }
> > 
> > Hi Eric,
> > 
> > thanks for your patches. This is an interesting new feature.
> > 
> > unfortunately clang-16 doesn't seem to like this very much.
> > I think that it is due to the declaration of reason not
> > being enclosed in a block - { }.
> > 
> >   net/openvswitch/actions.c:1483:4: error: expected expression
> >   u32 reason = nla_get_u32(a);
> >   ^
> >   net/openvswitch/actions.c:1485:4: error: use of undeclared identifier 
> > 'reason'
> >   reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
> >   ^
> >   net/openvswitch/actions.c:1488:8: error: use of undeclared identifier 
> > 'reason'
> >   if (reason == OVS_XLATE_OK)
> >   ^
> >   net/openvswitch/actions.c:1491:26: error: use of undeclared identifier 
> > 'reason'
> >   kfree_skb_reason(skb, reason);
> > ^
> >   4 errors generated.
> > 
> > 
> > net-next is currently closed.
> > So please provide a v2 once it reposts, after 10th July.
> 
> oof. My bad. I'll fix the clang issue and post v2 in a couple weeks.

Thanks Eric,

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

Re: [ovs-dev] Scale testing OVN with ovn-heater for OpenStack use cases

[Robin Jarry]

Frode Nordahl, Jun 30, 2023 at 09:25:
> Below are some proposals for dates and times, please respond with a
> prioritized list of what works best for you and we'll try to land on a
> single date/time for a first meeting:
> Wednesday July 5th 13:00 UTC
> Tuesday July 11th 13:30 UTC
> Wednesday July 12th 8:30 UTC

Hi Frode,

thanks a lot for organizing this. I will be interested in joining as
well. All three time slots are suitable for me.

Cheers,

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

Re: [ovs-dev] [PATCH 0/3] dpif: probe support for OVS_ACTION_ATTR_DROP

[Ilya Maximets]

On 6/30/23 14:33, Eric Garver wrote:
> On Fri, Jun 30, 2023 at 08:57:15AM +0200, Eelco Chaudron wrote:
>>
>>
>> On 29 Jun 2023, at 22:30, Eric Garver wrote:
>>
>>> Probe the datapath implementation for support of OVS_ACTION_ATTR_DROP.
>>> Also add a new test case.
>>
>>
>> Hi Eric,
>>
>> Thanks for the patch, but I get quite some build failures on missing switch 
>> cases.
>> Can you take a look? If you build with the --enable-Werror configure option 
>> you will see them.
> 
> Yeah. I added OVS_ACTION_ATTR_DEC_TTL to the enum, but failed to add it
> to the switch cases. next-next is closed anyways (doh!) so I'll wait a
> couple weeks before I post v2 of this series.

Hi, Eric.

There is an unfortunate timing issue.  OVS will enter a soft freeze
on Monday (technically tomorrow, but it's Saturday, so.. ).

The patch set is simple enough and looks fine in general, except for
compilation issues, so we could still accept it for the current release
during a soft freeze, unless other maintainers don't think so.
But we'll need to get a working version much sooner than in 2 weeks.
If you can post it today or on Monday that would be ideal.

We will need to wait for kernel changes to be accepted before we can
update the UAPI headers in our repo, but we should have about
a week for that from the 10th of July when the net-next opens and
about 17th of July when OVS will branch for 3.2 release.

I will not guarantee that the change will actually be accepted, because
there might be delays on the kernel side or we may find some issues
uncovered during review, but just wanted to lay down the possible best
case scenario here.

Otherwise, we'll need to postpone this series til August.

Thoughts?

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

Re: [ovs-dev] [PATCH ovn] ovn-northd.at: Fix the LSP incremental processing test case.

[Dumitru Ceara]

On 6/28/23 08:22, Han Zhou wrote:
> The test case intended to ensure there are no more than 3 failures in 10
> runs. However, it used "break" to exit the loop whenever a failure is
> encountered, end up with at most 1 failure and so the final check for
> the failure count will always pass. It should use "continue" instead.
> 
> Fixes: 8c30ba13869e ("ovn-northd.at: Fix unstable LSP incremental processing 
> test.")
> Signed-off-by: Han Zhou 
> ---

Hi Han,

This specific change looks good to me:

Acked-by: Dumitru Ceara 

However, I think the "70% of the time" part of the test is still
optimistic.  I ran the test in a loop 20 times and it failed after 10
iterations or so.

I changed it to 50% locally and it passed all 20 iterations.

Do you agree to reducing it to 50%?

Thanks,
Dumitru

>  tests/ovn-northd.at | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index ada2d2a4aa5e..88fe93a4e51f 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -9522,34 +9522,34 @@ for i in $(seq 10); do
>  ovs-vsctl add-port br-int lsp${i}-0 -- set interface lsp${i}-0 
> external_ids:iface-id=lsp${i}-0
>  wait_for_ports_up
>  check ovn-nbctl --wait=hv sync
> -check_recompute_counter 5 5 || break
> +check_recompute_counter 5 5 || continue
>  
>  check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
>  check ovn-nbctl --wait=hv lsp-add ls$i lsp${i}-1 -- lsp-set-addresses 
> lsp${i}-1 "aa:aa:aa:00:00:01 192.168.0.11"
>  ovs-vsctl add-port br-int lsp${i}-1 -- set interface lsp${i}-1 
> external_ids:iface-id=lsp${i}-1
>  wait_for_ports_up
>  check ovn-nbctl --wait=hv sync
> -check_recompute_counter 0 0 || break
> +check_recompute_counter 0 0 || continue
>  
>  check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
>  check ovn-nbctl --wait=hv lsp-add ls$i lsp${i}-2 -- lsp-set-addresses 
> lsp${i}-2 "aa:aa:aa:00:00:02 192.168.0.12"
>  ovs-vsctl add-port br-int lsp${i}-2 -- set interface lsp${i}-2 
> external_ids:iface-id=lsp${i}-2
>  wait_for_ports_up
>  check ovn-nbctl --wait=hv sync
> -check_recompute_counter 0 0 || break
> +check_recompute_counter 0 0 || continue
>  
>  check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
>  check ovn-nbctl --wait=hv lsp-del lsp${i}-1
> -check_recompute_counter 0 1 || break
> +check_recompute_counter 0 1 || continue
>  
>  check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
>  check ovn-nbctl --wait=hv lsp-set-addresses lsp${i}-2 "aa:aa:aa:00:00:88 
> 192.168.0.88"
> -check_recompute_counter 0 1 || break
> +check_recompute_counter 0 1 || continue
>  
>  # No change, no recompute
>  check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
>  check ovn-nbctl --wait=sb sync
> -check_recompute_counter 0 0 || break
> +check_recompute_counter 0 0 || continue
>  
>  done
>  echo Test failed $fail_count in 10.

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

Re: [ovs-dev] [PATCH 0/3] dpif: probe support for OVS_ACTION_ATTR_DROP

[Eelco Chaudron]

On 30 Jun 2023, at 14:33, Eric Garver wrote:

> On Fri, Jun 30, 2023 at 08:57:15AM +0200, Eelco Chaudron wrote:
>>
>>
>> On 29 Jun 2023, at 22:30, Eric Garver wrote:
>>
>>> Probe the datapath implementation for support of OVS_ACTION_ATTR_DROP.
>>> Also add a new test case.
>>
>>
>> Hi Eric,
>>
>> Thanks for the patch, but I get quite some build failures on missing switch 
>> cases.
>> Can you take a look? If you build with the --enable-Werror configure option 
>> you will see them.
>
> Yeah. I added OVS_ACTION_ATTR_DEC_TTL to the enum, but failed to add it
> to the switch cases. next-next is closed anyways (doh!) so I'll wait a
> couple weeks before I post v2 of this series.

Thanks! Will wait for v2 and do the review.

//Eelco


> Thanks.
> Eric.

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

Re: [ovs-dev] [PATCH v3] python: Add async DNS support

[Ilya Maximets]

On 6/30/23 14:23, Adrian Moreno wrote:
> 
> 
> On 6/30/23 13:40, Ilya Maximets wrote:
>> On 6/30/23 12:39, Adrian Moreno wrote:
>>>
>>>
>>> On 6/14/23 23:07, Terry Wilson wrote:
 This adds a Python version of the async DNS support added in:

 771680d96 DNS: Add basic support for asynchronous DNS resolving

 The above version uses the unbound C library, and this
 implimentation uses the SWIG-wrapped Python version of that.

 In the event that the Python unbound library is not available,
 a warning will be logged and the resolve() method will just
 return None. For the case where inet_parse_active() is passed
 an IP address, it will not try to resolve it, so existing
 behavior should be preserved in the case that the unbound
 library is unavailable.

 Intentional differences from the C version are as follows:

 OVS_HOSTS_FILE environment variable can bet set to override
 the system 'hosts' file. This is primarily to allow testing to
 be done without requiring network connectivity.

 Since resolution can still be done via hosts file lookup, DNS
 lookups are not disabled when resolv.conf cannot be loaded.

 The Python socket_util module has fallen behind its C equivalent.
 The bare minimum change was done to inet_parse_active() to support
 sync/async dns, as there is no equivalent to
 parse_sockaddr_components(), inet_parse_passive(), etc. A TODO
 was added to bring socket_util.py up to equivalency to the C
 version.

 Signed-off-by: Terry Wilson 
 ---
.github/workflows/build-and-test.yml|   4 +-
Documentation/intro/install/general.rst |   4 +-
Documentation/intro/install/rhel.rst|   2 +-
Documentation/intro/install/windows.rst |   2 +-
NEWS|   4 +-
debian/control.in   |   1 +
m4/openvswitch.m4   |   8 +-
python/TODO.rst |   7 +
python/automake.mk  |   2 +
python/ovs/dns_resolve.py   | 272 +++
python/ovs/socket_util.py   |  21 +-
python/ovs/stream.py|   2 +-
python/ovs/tests/test_dns_resolve.py| 280 
python/setup.py |   6 +-
rhel/openvswitch-fedora.spec.in |   2 +-
tests/vlog.at   |   2 +
16 files changed, 601 insertions(+), 18 deletions(-)
create mode 100644 python/ovs/dns_resolve.py
create mode 100644 python/ovs/tests/test_dns_resolve.py

 diff --git a/.github/workflows/build-and-test.yml 
 b/.github/workflows/build-and-test.yml
 index f66ab43b0..47d239f10 100644
 --- a/.github/workflows/build-and-test.yml
 +++ b/.github/workflows/build-and-test.yml
 @@ -183,10 +183,10 @@ jobs:
  run:  sudo apt update || true
- name: install common dependencies
  run:  sudo apt install -y ${{ env.dependencies }}
 -- name: install libunbound libunwind
 +- name: install libunbound libunwind python3-unbound
  # GitHub Actions doesn't have 32-bit versions of these libraries.
  if:   matrix.m32 == ''
 -  run:  sudo apt install -y libunbound-dev libunwind-dev
 +  run:  sudo apt install -y libunbound-dev libunwind-dev 
 python3-unbound
- name: install 32-bit libraries
  if:   matrix.m32 != ''
  run:  sudo apt install -y gcc-multilib
 diff --git a/Documentation/intro/install/general.rst 
 b/Documentation/intro/install/general.rst
 index 42b5682fd..19e360d47 100644
 --- a/Documentation/intro/install/general.rst
 +++ b/Documentation/intro/install/general.rst
 @@ -90,7 +90,7 @@ need the following software:
  If libcap-ng is installed, then Open vSwitch will automatically build 
 with
  support for it.

 -- Python 3.4 or later.
 +- Python 3.6 or later.

- Unbound library, from http://www.unbound.net, is optional but 
 recommended if
  you want to enable ovs-vswitchd and other utilities to use DNS names 
 when
 @@ -208,7 +208,7 @@ simply install and run Open vSwitch you require the 
 following software:
  from iproute2 (part of all major distributions and available at
  https://wiki.linuxfoundation.org/networking/iproute2).

 -- Python 3.4 or later.
 +- Python 3.6 or later.

On Linux you should ensure that ``/dev/urandom`` exists. To support TAP
devices, you must also ensure that ``/dev/net/tun`` exists.
 diff --git a/Documentation/intro/install/rhel.rst 
 b/Documentation/intro/install/rhel.rst
 index d1fc42021..f2151d890 

Re: [ovs-dev] [PATCH 0/3] dpif: probe support for OVS_ACTION_ATTR_DROP

[Eric Garver]

On Fri, Jun 30, 2023 at 08:57:15AM +0200, Eelco Chaudron wrote:
> 
> 
> On 29 Jun 2023, at 22:30, Eric Garver wrote:
> 
> > Probe the datapath implementation for support of OVS_ACTION_ATTR_DROP.
> > Also add a new test case.
> 
> 
> Hi Eric,
> 
> Thanks for the patch, but I get quite some build failures on missing switch 
> cases.
> Can you take a look? If you build with the --enable-Werror configure option 
> you will see them.

Yeah. I added OVS_ACTION_ATTR_DEC_TTL to the enum, but failed to add it
to the switch cases. next-next is closed anyways (doh!) so I'll wait a
couple weeks before I post v2 of this series.

Thanks.
Eric.

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

Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action

[Eric Garver]

On Fri, Jun 30, 2023 at 11:47:04AM +0200, Simon Horman wrote:
> On Thu, Jun 29, 2023 at 04:30:05PM -0400, Eric Garver wrote:
> > This adds an explicit drop action. This is used by OVS to drop packets
> > for which it cannot determine what to do. An explicit action in the
> > kernel allows passing the reason _why_ the packet is being dropped. We
> > can then use perf tracing to match on the drop reason.
> > 
> > e.g. trace all OVS dropped skbs
> > 
> >  # perf trace -e skb:kfree_skb --filter="reason >= 0x3"
> >  [..]
> >  106.023 ping/2465 skb:kfree_skb(skbaddr: 0xa0e8765f2000, \
> >   location:0xc0d9b462, protocol: 2048, reason: 196610)
> > 
> > reason: 196610 --> 0x30002 (OVS_XLATE_RECURSION_TOO_DEEP)
> > 
> > Signed-off-by: Eric Garver 
> 
> ...
> 
> > --- a/net/openvswitch/actions.c
> > +++ b/net/openvswitch/actions.c
> > @@ -32,6 +32,7 @@
> >  #include "vport.h"
> >  #include "flow_netlink.h"
> >  #include "openvswitch_trace.h"
> > +#include "drop.h"
> >  
> >  struct deferred_action {
> > struct sk_buff *skb;
> > @@ -1477,6 +1478,18 @@ static int do_execute_actions(struct datapath *dp, 
> > struct sk_buff *skb,
> > return dec_ttl_exception_handler(dp, skb,
> >  key, a);
> > break;
> > +
> > +   case OVS_ACTION_ATTR_DROP:
> > +   u32 reason = nla_get_u32(a);
> > +
> > +   reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
> > +   SKB_DROP_REASON_SUBSYS_SHIFT;
> > +
> > +   if (reason == OVS_XLATE_OK)
> > +   break;
> > +
> > +   kfree_skb_reason(skb, reason);
> > +   return 0;
> > }
> 
> Hi Eric,
> 
> thanks for your patches. This is an interesting new feature.
> 
> unfortunately clang-16 doesn't seem to like this very much.
> I think that it is due to the declaration of reason not
> being enclosed in a block - { }.
> 
>   net/openvswitch/actions.c:1483:4: error: expected expression
>   u32 reason = nla_get_u32(a);
>   ^
>   net/openvswitch/actions.c:1485:4: error: use of undeclared identifier 
> 'reason'
>   reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
>   ^
>   net/openvswitch/actions.c:1488:8: error: use of undeclared identifier 
> 'reason'
>   if (reason == OVS_XLATE_OK)
>   ^
>   net/openvswitch/actions.c:1491:26: error: use of undeclared identifier 
> 'reason'
>   kfree_skb_reason(skb, reason);
> ^
>   4 errors generated.
> 
> 
> net-next is currently closed.
> So please provide a v2 once it reposts, after 10th July.

oof. My bad. I'll fix the clang issue and post v2 in a couple weeks.

Thanks.
Eric.

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

Re: [ovs-dev] [PATCH v3] python: Add async DNS support

[Adrian Moreno]

On 6/30/23 13:40, Ilya Maximets wrote:

On 6/30/23 12:39, Adrian Moreno wrote:



On 6/14/23 23:07, Terry Wilson wrote:

This adds a Python version of the async DNS support added in:

771680d96 DNS: Add basic support for asynchronous DNS resolving

The above version uses the unbound C library, and this
implimentation uses the SWIG-wrapped Python version of that.

In the event that the Python unbound library is not available,
a warning will be logged and the resolve() method will just
return None. For the case where inet_parse_active() is passed
an IP address, it will not try to resolve it, so existing
behavior should be preserved in the case that the unbound
library is unavailable.

Intentional differences from the C version are as follows:

OVS_HOSTS_FILE environment variable can bet set to override
the system 'hosts' file. This is primarily to allow testing to
be done without requiring network connectivity.

Since resolution can still be done via hosts file lookup, DNS
lookups are not disabled when resolv.conf cannot be loaded.

The Python socket_util module has fallen behind its C equivalent.
The bare minimum change was done to inet_parse_active() to support
sync/async dns, as there is no equivalent to
parse_sockaddr_components(), inet_parse_passive(), etc. A TODO
was added to bring socket_util.py up to equivalency to the C
version.

Signed-off-by: Terry Wilson 
---
   .github/workflows/build-and-test.yml|   4 +-
   Documentation/intro/install/general.rst |   4 +-
   Documentation/intro/install/rhel.rst|   2 +-
   Documentation/intro/install/windows.rst |   2 +-
   NEWS|   4 +-
   debian/control.in   |   1 +
   m4/openvswitch.m4   |   8 +-
   python/TODO.rst |   7 +
   python/automake.mk  |   2 +
   python/ovs/dns_resolve.py   | 272 +++
   python/ovs/socket_util.py   |  21 +-
   python/ovs/stream.py|   2 +-
   python/ovs/tests/test_dns_resolve.py| 280 
   python/setup.py |   6 +-
   rhel/openvswitch-fedora.spec.in |   2 +-
   tests/vlog.at   |   2 +
   16 files changed, 601 insertions(+), 18 deletions(-)
   create mode 100644 python/ovs/dns_resolve.py
   create mode 100644 python/ovs/tests/test_dns_resolve.py

diff --git a/.github/workflows/build-and-test.yml 
b/.github/workflows/build-and-test.yml
index f66ab43b0..47d239f10 100644
--- a/.github/workflows/build-and-test.yml
+++ b/.github/workflows/build-and-test.yml
@@ -183,10 +183,10 @@ jobs:
 run:  sudo apt update || true
   - name: install common dependencies
 run:  sudo apt install -y ${{ env.dependencies }}
-- name: install libunbound libunwind
+- name: install libunbound libunwind python3-unbound
 # GitHub Actions doesn't have 32-bit versions of these libraries.
 if:   matrix.m32 == ''
-  run:  sudo apt install -y libunbound-dev libunwind-dev
+  run:  sudo apt install -y libunbound-dev libunwind-dev python3-unbound
   - name: install 32-bit libraries
 if:   matrix.m32 != ''
 run:  sudo apt install -y gcc-multilib
diff --git a/Documentation/intro/install/general.rst 
b/Documentation/intro/install/general.rst
index 42b5682fd..19e360d47 100644
--- a/Documentation/intro/install/general.rst
+++ b/Documentation/intro/install/general.rst
@@ -90,7 +90,7 @@ need the following software:
 If libcap-ng is installed, then Open vSwitch will automatically build with
 support for it.
   
-- Python 3.4 or later.

+- Python 3.6 or later.
   
   - Unbound library, from http://www.unbound.net, is optional but recommended if

 you want to enable ovs-vswitchd and other utilities to use DNS names when
@@ -208,7 +208,7 @@ simply install and run Open vSwitch you require the 
following software:
 from iproute2 (part of all major distributions and available at
 https://wiki.linuxfoundation.org/networking/iproute2).
   
-- Python 3.4 or later.

+- Python 3.6 or later.
   
   On Linux you should ensure that ``/dev/urandom`` exists. To support TAP

   devices, you must also ensure that ``/dev/net/tun`` exists.
diff --git a/Documentation/intro/install/rhel.rst 
b/Documentation/intro/install/rhel.rst
index d1fc42021..f2151d890 100644
--- a/Documentation/intro/install/rhel.rst
+++ b/Documentation/intro/install/rhel.rst
@@ -92,7 +92,7 @@ Once that is completed, remove the file ``/tmp/ovs.spec``.
   If python3-sphinx package is not available in your version of RHEL, you can
   install it via pip with 'pip install sphinx'.
   
-Open vSwitch requires python 3.4 or newer which is not available in older

+Open vSwitch requires python 3.6 or newer which is not available in older
   distributions. In the case of RHEL 6.x and its derivatives, one option is
   to install python34 

Re: [ovs-dev] [PATCH v3] python: Add async DNS support

[Ilya Maximets]

On 6/30/23 12:39, Adrian Moreno wrote:
> 
> 
> On 6/14/23 23:07, Terry Wilson wrote:
>> This adds a Python version of the async DNS support added in:
>>
>> 771680d96 DNS: Add basic support for asynchronous DNS resolving
>>
>> The above version uses the unbound C library, and this
>> implimentation uses the SWIG-wrapped Python version of that.
>>
>> In the event that the Python unbound library is not available,
>> a warning will be logged and the resolve() method will just
>> return None. For the case where inet_parse_active() is passed
>> an IP address, it will not try to resolve it, so existing
>> behavior should be preserved in the case that the unbound
>> library is unavailable.
>>
>> Intentional differences from the C version are as follows:
>>
>>OVS_HOSTS_FILE environment variable can bet set to override
>>the system 'hosts' file. This is primarily to allow testing to
>>be done without requiring network connectivity.
>>
>>Since resolution can still be done via hosts file lookup, DNS
>>lookups are not disabled when resolv.conf cannot be loaded.
>>
>>The Python socket_util module has fallen behind its C equivalent.
>>The bare minimum change was done to inet_parse_active() to support
>>sync/async dns, as there is no equivalent to
>>parse_sockaddr_components(), inet_parse_passive(), etc. A TODO
>>was added to bring socket_util.py up to equivalency to the C
>>version.
>>
>> Signed-off-by: Terry Wilson 
>> ---
>>   .github/workflows/build-and-test.yml|   4 +-
>>   Documentation/intro/install/general.rst |   4 +-
>>   Documentation/intro/install/rhel.rst|   2 +-
>>   Documentation/intro/install/windows.rst |   2 +-
>>   NEWS|   4 +-
>>   debian/control.in   |   1 +
>>   m4/openvswitch.m4   |   8 +-
>>   python/TODO.rst |   7 +
>>   python/automake.mk  |   2 +
>>   python/ovs/dns_resolve.py   | 272 +++
>>   python/ovs/socket_util.py   |  21 +-
>>   python/ovs/stream.py|   2 +-
>>   python/ovs/tests/test_dns_resolve.py| 280 
>>   python/setup.py |   6 +-
>>   rhel/openvswitch-fedora.spec.in |   2 +-
>>   tests/vlog.at   |   2 +
>>   16 files changed, 601 insertions(+), 18 deletions(-)
>>   create mode 100644 python/ovs/dns_resolve.py
>>   create mode 100644 python/ovs/tests/test_dns_resolve.py
>>
>> diff --git a/.github/workflows/build-and-test.yml 
>> b/.github/workflows/build-and-test.yml
>> index f66ab43b0..47d239f10 100644
>> --- a/.github/workflows/build-and-test.yml
>> +++ b/.github/workflows/build-and-test.yml
>> @@ -183,10 +183,10 @@ jobs:
>> run:  sudo apt update || true
>>   - name: install common dependencies
>> run:  sudo apt install -y ${{ env.dependencies }}
>> -- name: install libunbound libunwind
>> +- name: install libunbound libunwind python3-unbound
>> # GitHub Actions doesn't have 32-bit versions of these libraries.
>> if:   matrix.m32 == ''
>> -  run:  sudo apt install -y libunbound-dev libunwind-dev
>> +  run:  sudo apt install -y libunbound-dev libunwind-dev python3-unbound
>>   - name: install 32-bit libraries
>> if:   matrix.m32 != ''
>> run:  sudo apt install -y gcc-multilib
>> diff --git a/Documentation/intro/install/general.rst 
>> b/Documentation/intro/install/general.rst
>> index 42b5682fd..19e360d47 100644
>> --- a/Documentation/intro/install/general.rst
>> +++ b/Documentation/intro/install/general.rst
>> @@ -90,7 +90,7 @@ need the following software:
>> If libcap-ng is installed, then Open vSwitch will automatically build 
>> with
>> support for it.
>>   
>> -- Python 3.4 or later.
>> +- Python 3.6 or later.
>>   
>>   - Unbound library, from http://www.unbound.net, is optional but 
>> recommended if
>> you want to enable ovs-vswitchd and other utilities to use DNS names when
>> @@ -208,7 +208,7 @@ simply install and run Open vSwitch you require the 
>> following software:
>> from iproute2 (part of all major distributions and available at
>> https://wiki.linuxfoundation.org/networking/iproute2).
>>   
>> -- Python 3.4 or later.
>> +- Python 3.6 or later.
>>   
>>   On Linux you should ensure that ``/dev/urandom`` exists. To support TAP
>>   devices, you must also ensure that ``/dev/net/tun`` exists.
>> diff --git a/Documentation/intro/install/rhel.rst 
>> b/Documentation/intro/install/rhel.rst
>> index d1fc42021..f2151d890 100644
>> --- a/Documentation/intro/install/rhel.rst
>> +++ b/Documentation/intro/install/rhel.rst
>> @@ -92,7 +92,7 @@ Once that is completed, remove the file ``/tmp/ovs.spec``.
>>   If python3-sphinx package is not available in your version of RHEL, you can
>>   install it via pip with 'pip install sphinx'.
>>   
>> -Open vSwitch requires python 3.4 or 

Re: [ovs-dev] [PATCH v12] netdev-dpdk: Add custom rx-steering configuration.

[Ilya Maximets]

On 6/30/23 10:00, Robin Jarry wrote:
> Hi Ilya,
> 
> Ilya Maximets, Jun 29, 2023 at 23:57:
>>> +if (flags && ovsrcu_get(void *, >hw_info.offload_data)) {
>>
>> We probbaly shouldn't use the offload_data outside of netdev-offload
>> modules.  Simply checking netdev_is_flow_api_enabled() should be
>> enough.  Since both features require rte_flow oflload it's unlikely
>> that rx-steering can work if flow api is enabled globally.  And
>> netdev-offload-dpdk doesn't really check device capabilities.
> 
> OK, I'll change that for v13.
> 
>>>  static int
>>>  netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
>>> char **errp)
>>
>> Aside from the set_config, we also need a get_config callback update.
>> get_config() should return everything that set_config() set.  i.e.
>> we should return the "rx-steering": "rss+lacp", if user configured it.
>> The output will be visible in the dpif/show.
> 
> Hmm, I had missed the get_config callback. I have added something for
> get status but I assume it is different.

Right.  'status' is anything the netdev provider wants to report in
a free form.  'config' is something that users can put into a database
to get a configuration equivalent to a current one.

get_config() in netdev-dpdk currently reports weird stuff and needs
fixing.  If you want an example, look at n_rxq_desc reporting, it is
more or less correctly done.

Having some extra information in the status is fine.

> 
>>> +memset(reta_conf, 0, sizeof(reta_conf));
>>> +for (uint16_t i = 0; i < info.reta_size; i++) {
>>> +uint16_t idx = i / RTE_ETH_RETA_GROUP_SIZE;
>>> +uint16_t shift = i % RTE_ETH_RETA_GROUP_SIZE;
>>
>> An empty line here.
>>
>>> +reta_conf[idx].mask |= 1ULL << shift;
>>> +reta_conf[idx].reta[shift] = i % rss_n_rxq;
>>> +}
>>
>> And here.
> 
> Ack.
> 
>>> +for (int i = 0; i < dev->rx_steer_flows_num; i++) {
>>> +set_error(, RTE_FLOW_ERROR_TYPE_NONE);
>>> +if (rte_flow_destroy(dev->port_id, dev->rx_steer_flows[i], 
>>> )) {
>>> +VLOG_DBG("%s: rx-steering: failed to destroy flow: %s",
>>
>> Maybe a WARN ?
> 
> Will change that.
> 
>> So, this function destroys all the flows, OK.
>> But we also need to restore the redirection table, right?  Or is it handled
>> somehow in the driver when number of queues changed?  From my experience,
>> drivers tend to not restore this kind of configuration even on device detach
>> and it gets preserved even across OVS restarts.
> 
> From what I saw during testing, the RETA is reset every time the device
> is reset. In the first versions of this patch, I did reset the table but
> I seem to remember some discussions with Kevin and/or David where we
> concluded that it was redundant. I will check again to make sure before
> sending a respin.

OK.  Thanks!
Was just checking that this part wasn't overlooked.  Maybe add a comment
to a function on why it doesn't restore the redirection table, so the
code readers don't have extra questions?

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

Re: [ovs-dev] [PATCH 0/7] Improve linux QoS for exotic and fast links

[Adrian Moreno]

On 6/30/23 12:34, Eelco Chaudron wrote:



On 5 Jun 2023, at 8:25, Adrian Moreno wrote:


On 6/5/23 08:22, Adrian Moreno wrote:


On 6/2/23 16:13, Adrian Moreno wrote:

There are currently two issues that limit our ability to configure QoS
on certain cards in linux:

1) Firstly, the maximum link speed (which is used as maximum rate in
some tc classes), is obtained from netdev feature bits.
This is quite problematic because netdev feature bits do not keep up
with the new feature bits being added to ethtool. Therefore, cards whose
speed is not supported by OpenFlow currently report an incorrect speed
and this can affect QoS configuration.

This series addresses this problem by adding a new netdev function that
gets the actual link speed so we don't rely on feature bits which are
difficult to keep in sync.

2) The tc layer uses 32bit Bps rates which means we cannot express rates
higher than ~34Gbps.

This series addresses this problem using 64-bit tc attributes when
needed.

---
v0 -> v1
- Fix style issues.
- Add a test that checks we can expose a 50Gbps speed using a tap
    interface.
- Add an implementation of get_speed for netdev-bsd.
- Expose both HAVE_TCA_HTB_RATE64 and HAVE_TCA_POLICE_PKTRATE64 as AC
    variables and skip tests if the running kernel does not support them.

Adrian Moreno (7):
    netdev: add netdev_get_speed() to nedev API
    netdev-linux: use speed as max rate in tc classes
    netdev-linux: use 64-bit rtab tables
    netdev-linux: use 64-bit rates in htb tc classes
    netdev-linux: remove tc_matchall_fill_police
    netdev-linux: refactor nl_msg_put_act_police
    netdev-linux: support 64-bit rates in tc policing

   acinclude.m4 |  20 
   include/openvswitch/netdev.h |   1 +
   lib/dpif.h   |   4 +-
   lib/netdev-bsd.c |  21 
   lib/netdev-dpdk.c    |  52 +
   lib/netdev-linux-private.h   |   1 +
   lib/netdev-linux.c   | 210 +--
   lib/netdev-linux.h   |   2 +-
   lib/netdev-provider.h    |   9 ++
   lib/netdev.c |  23 
   lib/tc.c |   2 +
   ofproto/ofproto-dpif-sflow.c |  11 +-
   ofproto/ofproto.c    |   6 +-
   tests/atlocal.in |   5 +
   tests/system-interface.at    |  30 +
   tests/system-traffic.at  |  49 
   vswitchd/bridge.c    |  30 +++--
   17 files changed, 373 insertions(+), 103 deletions(-)



Sorry, I forgot to add v1 to this series. I'll re-send with the right version.



Ok, I was just noticed only the cover letter is unversioned.


Technically this is v2, as without any version it’s v1 :)



Right, versioning got messed up. The cover letter is unversioned for instance. I 
can resend but I'll wait for your comments first.



I’ll start reviewing this version as is.



Thanks!


//Eelco




--
Adrián Moreno

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

Re: [ovs-dev] ovs-vsctl Bridge IPFIX enable_input_sampling, enable_ouput_sampling and enable_tunnel_sampling returning unexpected values

[Adrian Moreno]

On 6/30/23 12:46, Ilya Maximets wrote:

On 6/30/23 03:38, Thomas Bachman wrote:

Hi,

It wasn't clear to me from the responses whether there was an action item - is 
something needed from the submitte? )Or is the patch under consideration?


Hi.  I replied to the patch now.   The patch itself has extra
spacing between lines, so it wasn't recognized as a patch by
any of our automated systems (like patchwork or our CI robot)
and fell through the cracks in the end.  Sorry about that.
It needs to be re-sent with correct formatting.



The first part of the patch I think it make sense. The part regarding 
tunnel_sampling I think it doesn't, but we can discuss that on a formal patch.


--
Adrián Moreno


Best regards, Ilya Maximets.



cheers,

-Thomas


On Wed, May 24, 2023 at 9:08 AM Adrian Moreno mailto:amore...@redhat.com>> wrote:



 On 5/23/23 22:26, Ilya Maximets wrote:
 > On 5/23/23 20:51, Sayali Naval (sanaval) via dev wrote:
 >> Does anyone have any insights on the below?
 >> 
 >> From: Sayali Naval (sanaval)
 >> Sent: Friday, May 12, 2023 11:10 AM
 >> To: d...@openvswitch.org  mailto:d...@openvswitch.org>>
 >> Subject: [ovs-dev] ovs-vsctl Bridge IPFIX enable_input_sampling, 
enable_ouput_sampling and enable_tunnel_sampling returning unexpected values
 >>
 >>
 >> As per the Open vSwitch Manual 
(http://www.openvswitch.org/support/dist-docs/ovs-vsctl.8.txt 
) the Bridge IPFIX 
parameters can be passed as follows:
 >>
 >> ovs-vsctl -- set Bridge br0 ipfix=@i \
 >>                --  --id=@i  create  IPFIX targets=\"192.168.0.34:4739 
\" obs_do‐
 >>                main_id=123       obs_point_id=456       
cache_active_timeout=60
 >>                cache_max_flows=13 \
 >>                other_config:enable-input-sampling=false \
 >>                other_config:enable-output-sampling=false \
 >>                other_config:enable-tunnel-sampling=true
 >>
 >> where the default values are:
 >>
 >> enable_input_sampling: true
 >> enable_output_sampling: true
 >> enable_tunnel_sampling: false
 >>

 I think wording is very confusing. In ovs-vsctl(8), before the example you
 provide, it says:
 "
 Configure bridge br0 to send one IPFIX flow record per packet
 sample to UDP port 4739 on host 192.168.0.34, with Observation Domain
 ID 123 and Observation Point ID 456, a flow cache active timeout of 1
 minute (60 seconds), maximum flow cache size of 13 flows, and flows
 sampled on output port with tunnel info(sampling on input and output
 port is enabled by default if not disabled) :
 "

 It doesn't explicitly say what's the default value for
 "other_config:enable-tunnel-sampling". On the other hand, the
 ovs-vswitchd.conf.db(5) says:

 "
 other_config : enable-input-sampling: optional string, either true or false
      By default, Open vSwitch samples and reports flows at bridge port 
input in
 IPFIX flow records. Set this column to false to disable input sampling.

 other_config : enable-output-sampling: optional string, either true or 
false
      By default, Open vSwitch samples and reports flows at bridge port 
output in
 IPFIX flow records. Set this column to false to disable output sampling.
 "

 One might interpret this as 'you can only set this to "false", since 
"true" is
 the default value'. If it was possible to forbid the user to put "true" in 
the
 field, maybe the current implementation would be reasonable. But it's not.

 Now for the tunnel-sampling:
 "
 other_config : enable-tunnel-sampling: optional string, either true or 
false
      Set to true to enable sampling and reporting tunnel header 7-tuples in
 IPFIX flow records. Tunnel sampling is enabled by default.
 "

 So the default value is true after all!


 >> But in the existing code 
https://github.com/openvswitch/ovs/blob/master/vswitchd/bridge.c#L1560-L1567 
, these 
3 parameters take up unexpected values in some scenarios.
 >>
 >> Consider the following case for enable_input_sampling and 
enable_output_sampling:
 >>
 >>          be_opts.enable_input_sampling = 
!smap_get_bool(_cfg->other_config,
 >>                                                "enable-input-sampling", 
false);
 >>
 >>          be_opts.enable_output_sampling = 
!smap_get_bool(_cfg->other_config,
 >>                                                
"enable-output-sampling", false);
 >>
 >> Here, the function smap_get_bool is being used with a negation.
 >>
 >> smap_get_bool is defined as below:
 >> (https://github.com/openvswitch/ovs/blob/master/lib/smap.c#L220-L232 

Re: [ovs-dev] ovs-vsctl Bridge IPFIX enable_input_sampling, enable_ouput_sampling and enable_tunnel_sampling returning unexpected values

[Ilya Maximets]

On 6/30/23 03:38, Thomas Bachman wrote:
> Hi,
> 
> It wasn't clear to me from the responses whether there was an action item - 
> is something needed from the submitte? )Or is the patch under consideration?

Hi.  I replied to the patch now.   The patch itself has extra
spacing between lines, so it wasn't recognized as a patch by
any of our automated systems (like patchwork or our CI robot)
and fell through the cracks in the end.  Sorry about that.
It needs to be re-sent with correct formatting.

Best regards, Ilya Maximets.

> 
> cheers,
> 
> -Thomas
> 
> 
> On Wed, May 24, 2023 at 9:08 AM Adrian Moreno  > wrote:
> 
> 
> 
> On 5/23/23 22:26, Ilya Maximets wrote:
> > On 5/23/23 20:51, Sayali Naval (sanaval) via dev wrote:
> >> Does anyone have any insights on the below?
> >> 
> >> From: Sayali Naval (sanaval)
> >> Sent: Friday, May 12, 2023 11:10 AM
> >> To: d...@openvswitch.org  
> mailto:d...@openvswitch.org>>
> >> Subject: [ovs-dev] ovs-vsctl Bridge IPFIX enable_input_sampling, 
> enable_ouput_sampling and enable_tunnel_sampling returning unexpected values
> >>
> >>
> >> As per the Open vSwitch Manual 
> (http://www.openvswitch.org/support/dist-docs/ovs-vsctl.8.txt 
> ) the Bridge 
> IPFIX parameters can be passed as follows:
> >>
> >> ovs-vsctl -- set Bridge br0 ipfix=@i \
> >>                --  --id=@i  create  IPFIX targets=\"192.168.0.34:4739 
> \" obs_do‐
> >>                main_id=123       obs_point_id=456       
> cache_active_timeout=60
> >>                cache_max_flows=13 \
> >>                other_config:enable-input-sampling=false \
> >>                other_config:enable-output-sampling=false \
> >>                other_config:enable-tunnel-sampling=true
> >>
> >> where the default values are:
> >>
> >> enable_input_sampling: true
> >> enable_output_sampling: true
> >> enable_tunnel_sampling: false
> >>
> 
> I think wording is very confusing. In ovs-vsctl(8), before the example you
> provide, it says:
> "
> Configure bridge br0 to send one IPFIX flow record per packet
> sample to UDP port 4739 on host 192.168.0.34, with Observation Domain
> ID 123 and Observation Point ID 456, a flow cache active timeout of 1
> minute (60 seconds), maximum flow cache size of 13 flows, and flows
> sampled on output port with tunnel info(sampling on input and output
> port is enabled by default if not disabled) :
> "
> 
> It doesn't explicitly say what's the default value for
> "other_config:enable-tunnel-sampling". On the other hand, the
> ovs-vswitchd.conf.db(5) says:
> 
> "
> other_config : enable-input-sampling: optional string, either true or 
> false
>      By default, Open vSwitch samples and reports flows at bridge port 
> input in
> IPFIX flow records. Set this column to false to disable input sampling.
> 
> other_config : enable-output-sampling: optional string, either true or 
> false
>      By default, Open vSwitch samples and reports flows at bridge port 
> output in
> IPFIX flow records. Set this column to false to disable output sampling.
> "
> 
> One might interpret this as 'you can only set this to "false", since 
> "true" is
> the default value'. If it was possible to forbid the user to put "true" 
> in the
> field, maybe the current implementation would be reasonable. But it's not.
> 
> Now for the tunnel-sampling:
> "
> other_config : enable-tunnel-sampling: optional string, either true or 
> false
>      Set to true to enable sampling and reporting tunnel header 7-tuples 
> in
> IPFIX flow records. Tunnel sampling is enabled by default.
> "
> 
> So the default value is true after all!
> 
> 
> >> But in the existing code 
> https://github.com/openvswitch/ovs/blob/master/vswitchd/bridge.c#L1560-L1567 
> ,
>  these 3 parameters take up unexpected values in some scenarios.
> >>
> >> Consider the following case for enable_input_sampling and 
> enable_output_sampling:
> >>
> >>          be_opts.enable_input_sampling = 
> !smap_get_bool(_cfg->other_config,
> >>                                                
> "enable-input-sampling", false);
> >>
> >>          be_opts.enable_output_sampling = 
> !smap_get_bool(_cfg->other_config,
> >>                                                
> "enable-output-sampling", false);
> >>
> >> Here, the function smap_get_bool is being used with a negation.
> >>
> >> smap_get_bool is defined as below:
> >> (https://github.com/openvswitch/ovs/blob/master/lib/smap.c#L220-L232 
> 

Re: [ovs-dev] [PATCH v3] python: Add async DNS support

[Adrian Moreno]

On 6/14/23 23:07, Terry Wilson wrote:

This adds a Python version of the async DNS support added in:

771680d96 DNS: Add basic support for asynchronous DNS resolving

The above version uses the unbound C library, and this
implimentation uses the SWIG-wrapped Python version of that.

In the event that the Python unbound library is not available,
a warning will be logged and the resolve() method will just
return None. For the case where inet_parse_active() is passed
an IP address, it will not try to resolve it, so existing
behavior should be preserved in the case that the unbound
library is unavailable.

Intentional differences from the C version are as follows:

   OVS_HOSTS_FILE environment variable can bet set to override
   the system 'hosts' file. This is primarily to allow testing to
   be done without requiring network connectivity.

   Since resolution can still be done via hosts file lookup, DNS
   lookups are not disabled when resolv.conf cannot be loaded.

   The Python socket_util module has fallen behind its C equivalent.
   The bare minimum change was done to inet_parse_active() to support
   sync/async dns, as there is no equivalent to
   parse_sockaddr_components(), inet_parse_passive(), etc. A TODO
   was added to bring socket_util.py up to equivalency to the C
   version.

Signed-off-by: Terry Wilson 
---
  .github/workflows/build-and-test.yml|   4 +-
  Documentation/intro/install/general.rst |   4 +-
  Documentation/intro/install/rhel.rst|   2 +-
  Documentation/intro/install/windows.rst |   2 +-
  NEWS|   4 +-
  debian/control.in   |   1 +
  m4/openvswitch.m4   |   8 +-
  python/TODO.rst |   7 +
  python/automake.mk  |   2 +
  python/ovs/dns_resolve.py   | 272 +++
  python/ovs/socket_util.py   |  21 +-
  python/ovs/stream.py|   2 +-
  python/ovs/tests/test_dns_resolve.py| 280 
  python/setup.py |   6 +-
  rhel/openvswitch-fedora.spec.in |   2 +-
  tests/vlog.at   |   2 +
  16 files changed, 601 insertions(+), 18 deletions(-)
  create mode 100644 python/ovs/dns_resolve.py
  create mode 100644 python/ovs/tests/test_dns_resolve.py

diff --git a/.github/workflows/build-and-test.yml 
b/.github/workflows/build-and-test.yml
index f66ab43b0..47d239f10 100644
--- a/.github/workflows/build-and-test.yml
+++ b/.github/workflows/build-and-test.yml
@@ -183,10 +183,10 @@ jobs:
run:  sudo apt update || true
  - name: install common dependencies
run:  sudo apt install -y ${{ env.dependencies }}
-- name: install libunbound libunwind
+- name: install libunbound libunwind python3-unbound
# GitHub Actions doesn't have 32-bit versions of these libraries.
if:   matrix.m32 == ''
-  run:  sudo apt install -y libunbound-dev libunwind-dev
+  run:  sudo apt install -y libunbound-dev libunwind-dev python3-unbound
  - name: install 32-bit libraries
if:   matrix.m32 != ''
run:  sudo apt install -y gcc-multilib
diff --git a/Documentation/intro/install/general.rst 
b/Documentation/intro/install/general.rst
index 42b5682fd..19e360d47 100644
--- a/Documentation/intro/install/general.rst
+++ b/Documentation/intro/install/general.rst
@@ -90,7 +90,7 @@ need the following software:
If libcap-ng is installed, then Open vSwitch will automatically build with
support for it.
  
-- Python 3.4 or later.

+- Python 3.6 or later.
  
  - Unbound library, from http://www.unbound.net, is optional but recommended if

you want to enable ovs-vswitchd and other utilities to use DNS names when
@@ -208,7 +208,7 @@ simply install and run Open vSwitch you require the 
following software:
from iproute2 (part of all major distributions and available at
https://wiki.linuxfoundation.org/networking/iproute2).
  
-- Python 3.4 or later.

+- Python 3.6 or later.
  
  On Linux you should ensure that ``/dev/urandom`` exists. To support TAP

  devices, you must also ensure that ``/dev/net/tun`` exists.
diff --git a/Documentation/intro/install/rhel.rst 
b/Documentation/intro/install/rhel.rst
index d1fc42021..f2151d890 100644
--- a/Documentation/intro/install/rhel.rst
+++ b/Documentation/intro/install/rhel.rst
@@ -92,7 +92,7 @@ Once that is completed, remove the file ``/tmp/ovs.spec``.
  If python3-sphinx package is not available in your version of RHEL, you can
  install it via pip with 'pip install sphinx'.
  
-Open vSwitch requires python 3.4 or newer which is not available in older

+Open vSwitch requires python 3.6 or newer which is not available in older
  distributions. In the case of RHEL 6.x and its derivatives, one option is
  to install python34 from `EPEL`_.
  
diff --git a/Documentation/intro/install/windows.rst b/Documentation/intro/install/windows.rst

index 

Re: [ovs-dev] [PATCH v1] bridge ovs-vsctl Bridge IPFIX enable_input_sampling, enable_ouput_sampling fix unexpected values

[Ilya Maximets]

On 6/21/23 20:43, Sayali Naval (sanaval) via dev wrote:
> A gentle reminder to take a look at this patch.

Hi.  Sorry for a late reply.  But this patch came in strangely formatted,
so it wasn't recognized as a patch by any of our systems.

The patch has extra spacing after every line that makes it not readable
by automated tools and hence not applicable.

Could you try re-sending it?  Maybe re-configure your or use a different
mail client for that, e.g. git send-email.

There are some hints in the contribution guide:
  
https://docs.openvswitch.org/en/latest/internals/contributing/submitting-patches/

And some hints on mail client configuration from a kernel guide:
  https://static.lwn.net/kerneldoc/process/email-clients.html


For example, here is how your patch looks like:
  https://mail.openvswitch.org/pipermail/ovs-dev/2023-May/405130.html

And here is an example on how the patch formatting should look like:
  https://mail.openvswitch.org/pipermail/ovs-dev/2023-June/405786.html

Best regards, Ilya Maximets.

> 
> Thanks,
> Sayali Naval
> 
> From: Sayali Naval (sanaval)
> Sent: Wednesday, May 31, 2023 11:02 AM
> To: d...@openvswitch.org 
> Subject: [ovs-dev] [PATCH v1] bridge ovs-vsctl Bridge IPFIX 
> enable_input_sampling, enable_ouput_sampling fix unexpected values
> 
> 
> As per the Open vSwitch Manual 
> (http://www.openvswitch.org/support/dist-docs/ovs-vsctl.8.txt) the Bridge 
> IPFIX parameters can be passed as follows:
> 
> 
> ovs-vsctl -- set Bridge br0 ipfix=@i \
> 
>   --  --id=@i  create  IPFIX targets=\"192.168.0.34:4739\" obs_do‐
> 
>   main_id=123   obs_point_id=456   cache_active_timeout=60
> 
>   cache_max_flows=13 \
> 
>   other_config:enable-input-sampling=false \
> 
>   other_config:enable-output-sampling=false
> 
> 
> where the default values are:
> 
> enable_input_sampling: true
> 
> enable_output_sampling: true
> 
> 
> But in the existing code 
> https://github.com/openvswitch/ovs/blob/master/vswitchd/bridge.c#L1563-L1567, 
> these 2 parameters take up unexpected values in some scenarios.
> 
> 
> 
> be_opts.enable_input_sampling = !smap_get_bool(_cfg->other_config,
> 
>   "enable-input-sampling", false);
> 
> 
> be_opts.enable_output_sampling = !smap_get_bool(_cfg->other_config,
> 
>   "enable-output-sampling", 
> false);
> 
> 
> Here, the function smap_get_bool is being used with a negation.
> 
> 
> smap_get_bool is defined as below:
> 
> (https://github.com/openvswitch/ovs/blob/master/lib/smap.c#L220-L232)
> 
> 
> /* Gets the value associated with 'key' in 'smap' and converts it to a 
> boolean.
> 
>  * If 'key' is not in 'smap', or its value is neither "true" nor "false",
> 
>  * returns 'def'. */
> 
> bool
> 
> smap_get_bool(const struct smap *smap, const char *key, bool def)
> 
> {
> 
> const char *value = smap_get_def(smap, key, "");
> 
> if (def) {
> 
> return strcasecmp("false", value) != 0;
> 
> } else {
> 
> return !strcasecmp("true", value);
> 
> }
> 
> }
> 
> 
> This returns expected values for the default case (since the above code will 
> negate “false” we get from smap_get bool function and return the value 
> “true”) but unexpected values for the case where the sampling value is passed 
> through the CLI.
> 
> For example, if we pass "true" for other_config:enable-input-sampling in the 
> CLI, the above code will negate the “true” value we get from the smap_bool 
> function and return the value “false”. Same would be the case for 
> enable_output_sampling.
> 
> 
> Signed-off-by: Sayali Naval 
> 
> ---
> 
> [bridge-ipfix-fix 277b3baae] Fix values for enable_input_sampling & 
> enable_ouput_sampling
> 
>  Date: Thu May 25 10:32:43 2023 -0700
> 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> 
> index f5dc59ad0..b972d55d0 100644
> 
> --- a/vswitchd/bridge.c
> 
> +++ b/vswitchd/bridge.c
> 
> @@ -1560,11 +1560,11 @@ bridge_configure_ipfix(struct bridge *br)
> 
>  be_opts.enable_tunnel_sampling = smap_get_bool(_cfg->other_config,
> 
>   "enable-tunnel-sampling", true);
> 
> 
> 
> -be_opts.enable_input_sampling = !smap_get_bool(_cfg->other_config,
> 
> -  "enable-input-sampling", 
> false);
> 
> +be_opts.enable_input_sampling = smap_get_bool(_cfg->other_config,
> 
> +  "enable-input-sampling", true);
> 
> 
> 
> -be_opts.enable_output_sampling = 
> !smap_get_bool(_cfg->other_config,
> 
> -  "enable-output-sampling", 
> false);
> 
> +be_opts.enable_output_sampling = smap_get_bool(_cfg->other_config,
> 
> +  

Re: [ovs-dev] [PATCH 0/7] Improve linux QoS for exotic and fast links

[Eelco Chaudron]

On 5 Jun 2023, at 8:25, Adrian Moreno wrote:

> On 6/5/23 08:22, Adrian Moreno wrote:
>>
>> On 6/2/23 16:13, Adrian Moreno wrote:
>>> There are currently two issues that limit our ability to configure QoS
>>> on certain cards in linux:
>>>
>>> 1) Firstly, the maximum link speed (which is used as maximum rate in
>>> some tc classes), is obtained from netdev feature bits.
>>> This is quite problematic because netdev feature bits do not keep up
>>> with the new feature bits being added to ethtool. Therefore, cards whose
>>> speed is not supported by OpenFlow currently report an incorrect speed
>>> and this can affect QoS configuration.
>>>
>>> This series addresses this problem by adding a new netdev function that
>>> gets the actual link speed so we don't rely on feature bits which are
>>> difficult to keep in sync.
>>>
>>> 2) The tc layer uses 32bit Bps rates which means we cannot express rates
>>> higher than ~34Gbps.
>>>
>>> This series addresses this problem using 64-bit tc attributes when
>>> needed.
>>>
>>> ---
>>> v0 -> v1
>>> - Fix style issues.
>>> - Add a test that checks we can expose a 50Gbps speed using a tap
>>>    interface.
>>> - Add an implementation of get_speed for netdev-bsd.
>>> - Expose both HAVE_TCA_HTB_RATE64 and HAVE_TCA_POLICE_PKTRATE64 as AC
>>>    variables and skip tests if the running kernel does not support them.
>>>
>>> Adrian Moreno (7):
>>>    netdev: add netdev_get_speed() to nedev API
>>>    netdev-linux: use speed as max rate in tc classes
>>>    netdev-linux: use 64-bit rtab tables
>>>    netdev-linux: use 64-bit rates in htb tc classes
>>>    netdev-linux: remove tc_matchall_fill_police
>>>    netdev-linux: refactor nl_msg_put_act_police
>>>    netdev-linux: support 64-bit rates in tc policing
>>>
>>>   acinclude.m4 |  20 
>>>   include/openvswitch/netdev.h |   1 +
>>>   lib/dpif.h   |   4 +-
>>>   lib/netdev-bsd.c |  21 
>>>   lib/netdev-dpdk.c    |  52 +
>>>   lib/netdev-linux-private.h   |   1 +
>>>   lib/netdev-linux.c   | 210 +--
>>>   lib/netdev-linux.h   |   2 +-
>>>   lib/netdev-provider.h    |   9 ++
>>>   lib/netdev.c |  23 
>>>   lib/tc.c |   2 +
>>>   ofproto/ofproto-dpif-sflow.c |  11 +-
>>>   ofproto/ofproto.c    |   6 +-
>>>   tests/atlocal.in |   5 +
>>>   tests/system-interface.at    |  30 +
>>>   tests/system-traffic.at  |  49 
>>>   vswitchd/bridge.c    |  30 +++--
>>>   17 files changed, 373 insertions(+), 103 deletions(-)
>>>
>>
>> Sorry, I forgot to add v1 to this series. I'll re-send with the right 
>> version.
>>
>
> Ok, I was just noticed only the cover letter is unversioned.

Technically this is v2, as without any version it’s v1 :)

I’ll start reviewing this version as is.

//Eelco


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

Re: [ovs-dev] [PATCH net-next 2/2] net: openvswitch: add drop action

[Simon Horman]

On Thu, Jun 29, 2023 at 04:30:05PM -0400, Eric Garver wrote:
> This adds an explicit drop action. This is used by OVS to drop packets
> for which it cannot determine what to do. An explicit action in the
> kernel allows passing the reason _why_ the packet is being dropped. We
> can then use perf tracing to match on the drop reason.
> 
> e.g. trace all OVS dropped skbs
> 
>  # perf trace -e skb:kfree_skb --filter="reason >= 0x3"
>  [..]
>  106.023 ping/2465 skb:kfree_skb(skbaddr: 0xa0e8765f2000, \
>   location:0xc0d9b462, protocol: 2048, reason: 196610)
> 
> reason: 196610 --> 0x30002 (OVS_XLATE_RECURSION_TOO_DEEP)
> 
> Signed-off-by: Eric Garver 

...

> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -32,6 +32,7 @@
>  #include "vport.h"
>  #include "flow_netlink.h"
>  #include "openvswitch_trace.h"
> +#include "drop.h"
>  
>  struct deferred_action {
>   struct sk_buff *skb;
> @@ -1477,6 +1478,18 @@ static int do_execute_actions(struct datapath *dp, 
> struct sk_buff *skb,
>   return dec_ttl_exception_handler(dp, skb,
>key, a);
>   break;
> +
> + case OVS_ACTION_ATTR_DROP:
> + u32 reason = nla_get_u32(a);
> +
> + reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
> + SKB_DROP_REASON_SUBSYS_SHIFT;
> +
> + if (reason == OVS_XLATE_OK)
> + break;
> +
> + kfree_skb_reason(skb, reason);
> + return 0;
>   }

Hi Eric,

thanks for your patches. This is an interesting new feature.

unfortunately clang-16 doesn't seem to like this very much.
I think that it is due to the declaration of reason not
being enclosed in a block - { }.

  net/openvswitch/actions.c:1483:4: error: expected expression
  u32 reason = nla_get_u32(a);
  ^
  net/openvswitch/actions.c:1485:4: error: use of undeclared identifier 'reason'
  reason |= SKB_DROP_REASON_SUBSYS_OPENVSWITCH <<
  ^
  net/openvswitch/actions.c:1488:8: error: use of undeclared identifier 'reason'
  if (reason == OVS_XLATE_OK)
  ^
  net/openvswitch/actions.c:1491:26: error: use of undeclared identifier 
'reason'
  kfree_skb_reason(skb, reason);
^
  4 errors generated.


net-next is currently closed.
So please provide a v2 once it reposts, after 10th July.

See: 
https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#development-cycle

-- 
pw-bot: changes-requested



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

Re: [ovs-dev] [PATCH ovn] mirror.c: Fix ovn-controller crash when mirror port is deleted from ovs.

[Dumitru Ceara]

On 6/29/23 08:42, Han Zhou wrote:
> If the ovs port used as output_port in mirror is deleted (either by
> mistake or intentionally), ovn-controller would crash in the
> check_and_update_interface_table() when trying to sync the mirror port
> configuration. e.g.:
> 
> 0  0x00449d6e in check_and_update_interface_table 
> (ovs_mirror=0xcc5110, ovs_mirror=0xcc5110, sb_mirror=0xcc64b0) at 
> ../controller/mirror.c:351
> 1  sync_ovn_mirror (ovs_mirror_ports=0x7ffcf79b0280, br_int=0xbf5020, 
> ovs_idl_txn=0xe13570, m=0xce86b0) at ../controller/mirror.c:351
> 2  mirror_run (ovs_idl_txn=0xe13570, ovs_mirror_table=, 
> sb_mirror_table=, br_int=0xbf5020, local_bindings= out>) at ../controller/mirror.c:174
> 3  0x00409e8e in main (argc=, argv=) at 
> ../controller/ovn-controller.c:5357
> 
> Similarly, it can also crash in delete_ovs_mirror() if the mirror port is
> deleted together with src/dst port (when should_delete_ovs_mirror returns
> true).
> 
> Both cases are captured in the updated test case of this patch.
> 
> This patch fixes the problem by verifying the existance of the mirror port, 
> and
> creates it if needed.
> 
> Fixes: ba8aa26e44cb ("OVN Remote Port Mirroring: controller changes to create 
> ovs mirrors")
> Signed-off-by: Han Zhou 
> ---

Looks good to me, thanks!

Acked-by: Dumitru Ceara 

Regards,
Dumitru

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

Re: [ovs-dev] [PATCH v12] netdev-dpdk: Add custom rx-steering configuration.

[Robin Jarry]

Hi Ilya,

Ilya Maximets, Jun 29, 2023 at 23:57:
> > +if (flags && ovsrcu_get(void *, >hw_info.offload_data)) {
>
> We probbaly shouldn't use the offload_data outside of netdev-offload
> modules.  Simply checking netdev_is_flow_api_enabled() should be
> enough.  Since both features require rte_flow oflload it's unlikely
> that rx-steering can work if flow api is enabled globally.  And
> netdev-offload-dpdk doesn't really check device capabilities.

OK, I'll change that for v13.

> >  static int
> >  netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
> > char **errp)
>
> Aside from the set_config, we also need a get_config callback update.
> get_config() should return everything that set_config() set.  i.e.
> we should return the "rx-steering": "rss+lacp", if user configured it.
> The output will be visible in the dpif/show.

Hmm, I had missed the get_config callback. I have added something for
get status but I assume it is different.

> > +memset(reta_conf, 0, sizeof(reta_conf));
> > +for (uint16_t i = 0; i < info.reta_size; i++) {
> > +uint16_t idx = i / RTE_ETH_RETA_GROUP_SIZE;
> > +uint16_t shift = i % RTE_ETH_RETA_GROUP_SIZE;
>
> An empty line here.
>
> > +reta_conf[idx].mask |= 1ULL << shift;
> > +reta_conf[idx].reta[shift] = i % rss_n_rxq;
> > +}
>
> And here.

Ack.

> > +for (int i = 0; i < dev->rx_steer_flows_num; i++) {
> > +set_error(, RTE_FLOW_ERROR_TYPE_NONE);
> > +if (rte_flow_destroy(dev->port_id, dev->rx_steer_flows[i], 
> > )) {
> > +VLOG_DBG("%s: rx-steering: failed to destroy flow: %s",
>
> Maybe a WARN ?

Will change that.

> So, this function destroys all the flows, OK.
> But we also need to restore the redirection table, right?  Or is it handled
> somehow in the driver when number of queues changed?  From my experience,
> drivers tend to not restore this kind of configuration even on device detach
> and it gets preserved even across OVS restarts.

>From what I saw during testing, the RETA is reset every time the device
is reset. In the first versions of this patch, I did reset the table but
I seem to remember some discussions with Kevin and/or David where we
concluded that it was redundant. I will check again to make sure before
sending a respin.

Thanks for the review.

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

Re: [ovs-dev] Scale testing OVN with ovn-heater for OpenStack use cases

[Felix Huettner via dev]

On Fri, Jun 30, 2023 at 09:25:47AM +0200, Frode Nordahl wrote:
> Hello all,
>
> On Tue, May 30, 2023 at 5:16 PM Felix Huettner
>  wrote:
> >
> > Hi Dumitru,
> >
> > On Fri, May 26, 2023 at 01:30:54PM +0200, Dumitru Ceara wrote:
> > > On 5/24/23 09:37, Felix Huettner wrote:
> > > > Hi everyone,
> > >
> > > Hi Felix,
> > >
> > > >
> > > > Ilya mentioned to me that you will want to bring openstack examples to
> > > > ovn-heater.
> > > >
> > >
> > > Yes, we're discussing that.
> > >
> > > > I wanted to ask how to best join this effort. It would be great for us
> > >
> > > Everyone is welcome to contribute! :)
> > >
> >
> > Great, we will try that out.
>
> Thank you for your interest in this effort, as promised I'm in the
> process of organizing a community meeting to get this going.
>
> Below are some proposals for dates and times, please respond with a
> prioritized list of what works best for you and we'll try to land on a
> single date/time for a first meeting:
> Wednesday July 5th 13:00 UTC
> Tuesday July 11th 13:30 UTC
> Wednesday July 12th 8:30 UTC

Hi Frode,

both the slot at 11th and 12th fit for us without any issues.
5th is also doable but we would prefer the other two.

Thanks
Felix


>
> > > > to have a tool to scale test ovn. Also we have an ovn deployment with
> > > > currently 450 compute nodes where we can try to extract the typically
> > > > used northbound entries from.
> > > >
> > >
> > > This is great!  I'm not an OpenStack user so I'm probably not using the
> > > right terminology and I might be missing things but I think we're
> > > interested in:
> > > - how many tenant networks do you have in the cluster
> > > - for each tenant network:
> > >   - how many VMs (on average but maybe also min/max/etc)
> > >   - how many floating IPs (dnat-and-snat)
> > >   - how many compute nodes run VMs from this network
> > > - for each compute:
> > >   - how many VMs run on it (on average but also min/max/etc)
> > >
> >
> > Ok, so i'll try to give an overview below.
> > If you need any more detail just ask.
> >
> > General amount of nodes:
> > * ~500 Hypervisors
> > * 9 Nodes that are just serve as gateway chassis (connected to 4
> >   different external networks)
> >
> > Amount of tenant Networks (== logical routers): 2300 (only 1400 are
> > actually hosting VMs).
> >
> > Amount of logical Routers: 2010 (2000 of these have a chassisredirect
> > port that is hosted on one of the 9 nodes.
> >
> > * Amount of VMs:
> >* min: 1
> >* max: 300
> >* avg: 4.5
> >* 630 networks only have a single VM, 230 have two VMs and 190 have 3
> >  VMs.
> > * Amount of Hypervisors per network:
> >* min: 1
> >* max: 90
> >* avg: 3.6
> >* 650 networks only strech on a single hypervisor, 250 on two
> >  hypervisors, 180 on three hypervisors
> >
> > Regarding floating ips we have currently 1530. I'll give you the amount
> > per Logical router:
> > * min: 1
> > * max: 230
> > * avg: 2.7
> > * 430 routers have a single floating ip, 140 routers have 2 floating
> >   ips.
> >
> > Regarding VMs per Hypervisor we have:
> > * min: 1
> > * max: 80
> > * avg: 15
> >
> >
> > Other potentially interesting values:
> >
> > * Amount of Load_Balancer: 0
> > * Amount of Logical_Switch_Port type=virtual: 800
> > * Amount of Port_Group: 6000
> > * Amount of ACL: 35500
> >* All of these referce portgroups in `match` using `@` or `$`
> >* Referencing other portgroups using `$`: 8300
>
> Thanks alot for providing this information, and gauging the size of
> your deployment, your inputs would be very valuable in this effort.
> One of the main goals of ovn-heater is the ability to simulate
> workloads, because as developers, we seldom have access to actual 500-
> or 1000- node clusters. Having a participant in the project with
> actual hardware installation of such a size would give us the
> opportunity to help confirm the accuracy of our simulations.
>
> --
> Frode Nordahl
>
> > Regards
> > Felix
> >
> > > Mayba Frode and Daniel can expand on this.
> > >
> > > Regards,
> > > Dumitru
> > >
> > > > Thanks
> > > > Felix
> > > >
> > > > On Tue, May 16, 2023 at 05:38:25PM +0200, Daniel Alvarez Sanchez wrote:
> > > >> Hey Frode, Dumitru, thanks a lot for initiating this
> > > >>
> > > >> On Thu, May 11, 2023 at 6:05 PM Frode Nordahl 
> > > >> 
> > > >> wrote:
> > > >>
> > > >>> Hello, Dumitru, Daniel and team,
> > > >>>
> > > >>> On Thu, May 11, 2023 at 5:23 PM Dumitru Ceara  
> > > >>> wrote:
> > > 
> > >  Hi Frode,
> > > 
> > >  During an internal discussion with RedHat OpenStack folks (Daniel in 
> > >  cc)
> > >  about scalability of OVN in OpenStack deployments I remembered that 
> > >  you
> > >  mentioned that you're thinking of enhancing ovn-heater in order to 
> > >  test
> > >  your types of deployments [0].  I assumed that those are also 
> > >  OpenStack
> > >  based deployments so I was wondering if there's an opportunity for
> > >  

Re: [ovs-dev] Scale testing OVN with ovn-heater for OpenStack use cases

[Frode Nordahl]

Hello all,

On Tue, May 30, 2023 at 5:16 PM Felix Huettner
 wrote:
>
> Hi Dumitru,
>
> On Fri, May 26, 2023 at 01:30:54PM +0200, Dumitru Ceara wrote:
> > On 5/24/23 09:37, Felix Huettner wrote:
> > > Hi everyone,
> >
> > Hi Felix,
> >
> > >
> > > Ilya mentioned to me that you will want to bring openstack examples to
> > > ovn-heater.
> > >
> >
> > Yes, we're discussing that.
> >
> > > I wanted to ask how to best join this effort. It would be great for us
> >
> > Everyone is welcome to contribute! :)
> >
>
> Great, we will try that out.

Thank you for your interest in this effort, as promised I'm in the
process of organizing a community meeting to get this going.

Below are some proposals for dates and times, please respond with a
prioritized list of what works best for you and we'll try to land on a
single date/time for a first meeting:
Wednesday July 5th 13:00 UTC
Tuesday July 11th 13:30 UTC
Wednesday July 12th 8:30 UTC

> > > to have a tool to scale test ovn. Also we have an ovn deployment with
> > > currently 450 compute nodes where we can try to extract the typically
> > > used northbound entries from.
> > >
> >
> > This is great!  I'm not an OpenStack user so I'm probably not using the
> > right terminology and I might be missing things but I think we're
> > interested in:
> > - how many tenant networks do you have in the cluster
> > - for each tenant network:
> >   - how many VMs (on average but maybe also min/max/etc)
> >   - how many floating IPs (dnat-and-snat)
> >   - how many compute nodes run VMs from this network
> > - for each compute:
> >   - how many VMs run on it (on average but also min/max/etc)
> >
>
> Ok, so i'll try to give an overview below.
> If you need any more detail just ask.
>
> General amount of nodes:
> * ~500 Hypervisors
> * 9 Nodes that are just serve as gateway chassis (connected to 4
>   different external networks)
>
> Amount of tenant Networks (== logical routers): 2300 (only 1400 are
> actually hosting VMs).
>
> Amount of logical Routers: 2010 (2000 of these have a chassisredirect
> port that is hosted on one of the 9 nodes.
>
> * Amount of VMs:
>* min: 1
>* max: 300
>* avg: 4.5
>* 630 networks only have a single VM, 230 have two VMs and 190 have 3
>  VMs.
> * Amount of Hypervisors per network:
>* min: 1
>* max: 90
>* avg: 3.6
>* 650 networks only strech on a single hypervisor, 250 on two
>  hypervisors, 180 on three hypervisors
>
> Regarding floating ips we have currently 1530. I'll give you the amount
> per Logical router:
> * min: 1
> * max: 230
> * avg: 2.7
> * 430 routers have a single floating ip, 140 routers have 2 floating
>   ips.
>
> Regarding VMs per Hypervisor we have:
> * min: 1
> * max: 80
> * avg: 15
>
>
> Other potentially interesting values:
>
> * Amount of Load_Balancer: 0
> * Amount of Logical_Switch_Port type=virtual: 800
> * Amount of Port_Group: 6000
> * Amount of ACL: 35500
>* All of these referce portgroups in `match` using `@` or `$`
>* Referencing other portgroups using `$`: 8300

Thanks alot for providing this information, and gauging the size of
your deployment, your inputs would be very valuable in this effort.
One of the main goals of ovn-heater is the ability to simulate
workloads, because as developers, we seldom have access to actual 500-
or 1000- node clusters. Having a participant in the project with
actual hardware installation of such a size would give us the
opportunity to help confirm the accuracy of our simulations.

-- 
Frode Nordahl

> Regards
> Felix
>
> > Mayba Frode and Daniel can expand on this.
> >
> > Regards,
> > Dumitru
> >
> > > Thanks
> > > Felix
> > >
> > > On Tue, May 16, 2023 at 05:38:25PM +0200, Daniel Alvarez Sanchez wrote:
> > >> Hey Frode, Dumitru, thanks a lot for initiating this
> > >>
> > >> On Thu, May 11, 2023 at 6:05 PM Frode Nordahl 
> > >> 
> > >> wrote:
> > >>
> > >>> Hello, Dumitru, Daniel and team,
> > >>>
> > >>> On Thu, May 11, 2023 at 5:23 PM Dumitru Ceara  wrote:
> > 
> >  Hi Frode,
> > 
> >  During an internal discussion with RedHat OpenStack folks (Daniel in 
> >  cc)
> >  about scalability of OVN in OpenStack deployments I remembered that you
> >  mentioned that you're thinking of enhancing ovn-heater in order to test
> >  your types of deployments [0].  I assumed that those are also OpenStack
> >  based deployments so I was wondering if there's an opportunity for
> >  collaboration here.
> > >>>
> > >>> Thank you for reaching out to me on this topic. We do indeed have scale
> > >>> testing in the context of OpenStack on our roadmap and welcome the
> > >>> invitation to collaborate on this.
> > >>>
> > >>> As you know we did some preparation work together already as you refer
> > >>> to in [0], and we thank you for your help in upstreaming ovn-heater.
> > >>>
> > >>> We have yet to schedule exactly when in the cycle to work on this, I
> > >>> guess a next step could be to set up a meet to share some ideas 

Re: [ovs-dev] [PATCH 0/3] dpif: probe support for OVS_ACTION_ATTR_DROP

[Eelco Chaudron]

On 29 Jun 2023, at 22:30, Eric Garver wrote:

> Probe the datapath implementation for support of OVS_ACTION_ATTR_DROP.
> Also add a new test case.


Hi Eric,

Thanks for the patch, but I get quite some build failures on missing switch 
cases.
Can you take a look? If you build with the --enable-Werror configure option you 
will see them.

Thanks,

Eelco

> Eric Garver (3):
>   dpif: probe support for OVS_ACTION_ATTR_DROP
>   system-common-macros: check for drop action in datapath
>   tests: system-traffic: add coverage for drop action
>
>  include/linux/openvswitch.h   |  3 ++-
>  lib/dpif.c|  6 --
>  lib/dpif.h|  1 -
>  ofproto/ofproto-dpif.c| 34 --
>  tests/system-common-macros.at |  4 
>  tests/system-traffic.at   | 29 +
>  6 files changed, 67 insertions(+), 10 deletions(-)
>
> -- 
> 2.39.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] dpif-netdev: Lockless meters.

[Eelco Chaudron]

On 29 Jun 2023, at 17:54, Ilya Maximets wrote:

> On 6/29/23 17:43, Eelco Chaudron wrote:
>>
>>
>> On 22 Jun 2023, at 0:32, Ilya Maximets wrote:
>>
>>> Current implementation of meters in the userspace datapath takes
>>> the meter lock for every packet batch.  If more than one thread
>>> hits the flow with the same meter, they will lock each other.
>>>
>>> Replace the critical section with atomic operations to avoid
>>> interlocking.  Meters themselves are RCU-protected, so it's safe
>>> to access them without holding a lock.
>>>
>>> Implementation does the following:
>>>
>>>  1. Tries to advance the 'used' timer of the meter with atomic
>>> compare+exchange if it's smaller than 'now'.
>>>  2. If the timer change succeeds, atomically update band buckets.
>>>  3. Atomically update packet statistics for a meter.
>>>  4. Go over buckets and try to atomically subtract the amount of
>>> packets or bytes, recording the highest exceeded band.
>>>  5. Atomically update band statistics and drop packets.
>>>
>>> Bucket manipulations are implemented with atomic compare+exchange
>>> operations with extra checks, because bucket size should never
>>> exceed the maximum and it should never go below zero.
>>>
>>> Packet statistics may be momentarily inconsistent, i.e., number
>>> of packets and the number of bytes may reflect different sets
>>> of packets.  But it should be eventually consistent.  And the
>>> difference at any given time should be in just few packets.
>>>
>>> For the sake of reduced code complexity PKTPS meter tries to push
>>> packets through the band one by one, even though they all have
>>> the same weight.  This is also more fair if more than one thread
>>> is passing packets through the same band at the same time.
>>> Trying to predict the number of packets that can pass may also
>>> cause extra atomic operations reducing the performance.
>>>
>>> This implementation shows similar performance to the previous one,
>>> but should scale better with more threads hiting the same meter.
>>
>> This works looks great!! Some small comments below. Did limited testing and 
>> seems to work fine.
>>
>> Cheers,
>>
>> Eelco
>>
>>> Signed-off-by: Ilya Maximets 
>>> ---
>>>
>>> @Lin Huang, if you can try this change on your setup, that
>>> would be great.
>>>
>>>  NEWS  |   2 +
>>>  lib/dpif-netdev.c | 250 +-
>>>  2 files changed, 140 insertions(+), 112 deletions(-)
>>>
>>> diff --git a/NEWS b/NEWS
>>> index 66d5a4ea3..4a2b7dbca 100644
>>> --- a/NEWS
>>> +++ b/NEWS
>>> @@ -37,6 +37,8 @@ Post-v3.1.0
>>> - SRv6 Tunnel Protocol
>>>   * Added support for userspace datapath (only).
>>> - Userspace datapath:
>>> + * Implementation of OpenFlow meters is now lockless allowing for 
>>> better
>>> +   multi-thread scalability.
>>>   * IP and L4 checksum offload support is now enabled by default for
>>> interfaces that support it.  See the 'status' column in the 
>>> 'interface'
>>> table to check the status.
>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>>> index abe63412e..2fa556a62 100644
>>> --- a/lib/dpif-netdev.c
>>> +++ b/lib/dpif-netdev.c
>>> @@ -212,21 +212,21 @@ static void dpcls_remove(struct dpcls *, struct 
>>> dpcls_rule *);
>>>  struct dp_meter_band {
>>>  uint32_t rate;
>>>  uint32_t burst_size;
>>> -uint64_t bucket; /* In 1/1000 packets (for PKTPS), or in bits (for 
>>> KBPS) */
>>> -uint64_t packet_count;
>>> -uint64_t byte_count;
>>> +atomic_uint64_t bucket;  /* In 1/1000 packets for PKTPS,
>>> +  * or in bits for KBPS. */
>>> +atomic_uint64_t packet_count;
>>> +atomic_uint64_t byte_count;
>>>  };
>>>
>>>  struct dp_meter {
>>>  struct cmap_node node;
>>> -struct ovs_mutex lock;
>>>  uint32_t id;
>>>  uint16_t flags;
>>>  uint16_t n_bands;
>>>  uint32_t max_delta_t;
>>> -uint64_t used;
>>> -uint64_t packet_count;
>>> -uint64_t byte_count;
>>> +atomic_uint64_t used;  /* Time of a last use in milliseconds. */
>>> +atomic_uint64_t packet_count;
>>> +atomic_uint64_t byte_count;
>>>  struct dp_meter_band bands[];
>>>  };
>>>
>>> @@ -7165,22 +7165,56 @@ dpif_netdev_meter_get_features(const struct dpif * 
>>> dpif OVS_UNUSED,
>>>  features->max_color = 0;
>>>  }
>>>
>>> +/* Tries to atomically add 'n' to 'value' in terms of saturation 
>>> arithmetic,
>>> + * i.e., if the result will be larger than 'max_value', will store 
>>> 'max_value'
>>> + * instead. */
>>> +static void
>>> +atomic_sat_add(atomic_uint64_t *value, uint64_t n, uint64_t max_value)
>>> +{
>>> +uint64_t current, new_value;
>>> +
>>> +atomic_read_relaxed(value, );
>>> +do {
>>> +new_value = current + n;
>>> +new_value = MIN(new_value, max_value);
>>> +} while (!atomic_compare_exchange_weak_relaxed(value, ,
>>> +   new_value));