Re: [ovs-dev] [PATCH v3 ovn] northd: centralized reply lb traffic even if FIP is defined

[Lorenzo Bianconi]

> Hi Lorenzo,

Hi Mark,

thx for the review.

> 
> I tried running the system test from this patch against ovn main and it
> passes even without the changes in northd. I think this is because the
> alice1 and foo1 ports need to be bound on different hvs to properly test the
> changes in northd.

ack, you are right. I will drop the system-ovn test and I will add ovn.at one.

> 
> See below for other comments.
> 
> On 5/24/23 13:56, Lorenzo Bianconi wrote:
> > In the current codebase for distributed gw router port use-case,
> > it is not possible to add a load balancer that redirects the traffic
> > to a back-end if it is used as internal IP of a FIP NAT rule since
> > the reply traffic is never centralized. Fix the issue centralizing the
> > traffic if it is the reply packet for the load balancer.
> > 
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2023609
> > Signed-off-by: Lorenzo Bianconi 
> > ---
> > Changes since v2:
> > - rebase on top of ovn main branch
> > - fix spelling
> > Changes since v1:
> > - add new system-test and unit-test
> > ---
> >   northd/northd.c |  15 ++
> >   northd/ovn-northd.8.xml |  16 +++
> >   tests/ovn-northd.at |  48 +++
> >   tests/system-ovn.at | 100 
> >   4 files changed, 179 insertions(+)
> > 
> > diff --git a/northd/northd.c b/northd/northd.c
> > index a6eca916b..6317c36fd 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -10740,6 +10740,8 @@ build_distr_lrouter_nat_flows_for_lb(struct 
> > lrouter_nat_lb_flows_ctx *ctx,
> >struct ovn_datapath *od)
> >   {
> >   struct ovn_port *dgp = od->l3dgw_ports[0];
> > +struct ds gw_redir_match = DS_EMPTY_INITIALIZER;
> > +struct ds gw_redir_action = DS_EMPTY_INITIALIZER;
> >   const char *undnat_action;
> > @@ -10784,6 +10786,17 @@ build_distr_lrouter_nat_flows_for_lb(struct 
> > lrouter_nat_lb_flows_ctx *ctx,
> >   return;
> >   }
> > +/* We need to centralize the LB traffic to properly perform
> > + * the undnat stage.
> > + */
> > +ds_put_format(_redir_match, "%s) && outport == %s",
> > +  ds_cstr(ctx->undnat_match), dgp->json_key);
> > +ds_put_format(_redir_action, "outport = %s; next;",
> > +  dgp->cr_port->json_key);
> > +ovn_lflow_add_with_hint(ctx->lflows, od, S_ROUTER_IN_GW_REDIRECT,
> > +200, ds_cstr(_redir_match),
> > +ds_cstr(_redir_action), 
> > >lb->nlb->header_);
> > +
> >   ds_put_format(ctx->undnat_match, ") && (inport == %s || outport == 
> > %s)"
> > " && is_chassis_resident(%s)", dgp->json_key, 
> > dgp->json_key,
> > dgp->cr_port->json_key);
> > @@ -10791,6 +10804,8 @@ build_distr_lrouter_nat_flows_for_lb(struct 
> > lrouter_nat_lb_flows_ctx *ctx,
> >   ds_cstr(ctx->undnat_match), undnat_action,
> >   >lb->nlb->header_);
> >   ds_truncate(ctx->undnat_match, undnat_match_len);
> > +ds_destroy(_redir_match);
> > +ds_destroy(_redir_action);
> >   }
> >   static void
> > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> > index 540fe03bd..295f52b11 100644
> > --- a/northd/ovn-northd.8.xml
> > +++ b/northd/ovn-northd.8.xml
> > @@ -4578,6 +4578,22 @@ icmp6 {
> >   
> >   
> > +  
> > +For all the configured load balancing rules that include an IPv4
> > +address VIP, and a list of IPv4 backend addresses
> > +B0, B1 .. Bn defined for the
> > +VIP a priority-200 flow is added that matches 
> > +ip4  (ip4.src == B0 || ip4.src == 
> > B1
> > +|| ... || ip4.src == Bn) with an action 
> > +outport = CR; next; where CR is the
> > +chassisredirect port representing the instance of the
> > +logical router distributed gateway port on the gateway chassis.
> > +If the backend IPv4 address Bx is also configured with
> > +L4 port PORT of protocol P, then the match
> > +also includes P.src == PORT.
> > +Similar flows are addeded for IPv6.
> 
> s/addeded/added/

ack, I will fix it.

> 
> > +  
> > +
> > 
> >   For each NAT rule in the OVN Northbound database that can
> >   be handled in a distributed manner, a priority-100 logical
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index e3669bdf5..59f932c5f 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -9484,3 +9484,51 @@ AT_CHECK([ovn-sbctl lflow-list sw | grep 
> > ls_out_pre_lb | grep priority=110 | gre
> >   AT_CLEANUP
> >   ])
> > +
> > +OVN_FOR_EACH_NORTHD_NO_HV([
> > +AT_SETUP([check fip and lb flows])
> > +AT_KEYWORDS([fip-lb-flows])
> > +ovn_start
> > +
> > +ovn-nbctl lr-add R1
> > +ovn-nbctl lrp-add R1 R1-S0 02:ac:10:01:00:01 10.0.0.1/24 1000::a/64
> > +ovn-nbctl lrp-add R1 R1-PUB 

Re: [ovs-dev] [PATCH v2] MAINTAINERS.rst: Move several people to emeritus status

[Ansis]

On Fri, May 19, 2023 at 9:53 AM Russell Bryant  wrote:
>
> The following document discusses emeritus committer status:
>
> https://docs.openvswitch.org/en/latest/internals/committer-emeritus-status/
>
> There are several people who I would guess consider themselves
> emeritus committers but have not formally declared it. Those moved to
> emeritus status in this commit have either explicitly communicated
> their desire to move or have both not been active in the last year and
> have not yet replied to this patch.
>
> It is easy to re-add people in the future should any emeritus
> committer desire to become active again.
>
> Per our policies, a vote of the majority of current committers (or
> the list of maintainers prior to this change) is required to move a
> committer to emeritus status.
>
> Signed-off-by: Russell Bryant 
> CC: Alin Serdean 
> CC: Andy Zhou 
> CC: Ansis Atteka 
> CC: Daniele Di Proietto 
> CC: Gurucharan Shetty 
> CC: Ian Stokes 
> CC: Ilya Maximets 
> CC: Jarno Rajahalme 
> CC: Jesse Gross 
> CC: Justin Pettit 
> CC: Pravin B Shelar 
> CC: Simon Horman 
> CC: Thomas Graf 
> CC: William Tu 
> CC: YAMAMOTO Takashi 
> ---
>  MAINTAINERS.rst | 42 +-
>  1 file changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/MAINTAINERS.rst b/MAINTAINERS.rst
> index 1dc406170..85b8e6416 100644
> --- a/MAINTAINERS.rst
> +++ b/MAINTAINERS.rst
> @@ -41,40 +41,20 @@ This is the current list of active Open vSwitch 
> committers:
>
> * - Name
>   - Email
> -   * - Alex Wang
> - - ee07b...@gmail.com
> * - Alin Serdean
>   - aserd...@ovn.org
> -   * - Andy Zhou
> - - az...@ovn.org
> * - Ansis Atteka
> - - aatt...@nicira.com
> -   * - Daniele Di Proietto
> - - daniele.di.proie...@gmail.com
> -   * - Gurucharan Shetty
> - - g...@ovn.org
> + - ansisatt...@gmail.com
> * - Ian Stokes
>   - isto...@ovn.org
> * - Ilya Maximets
>   - i.maxim...@ovn.org
> -   * - Jarno Rajahalme
> - - ja...@ovn.org
> -   * - Jesse Gross
> - - je...@kernel.org
> -   * - Justin Pettit
> - - jpet...@ovn.org
> -   * - Pravin B Shelar
> - - pshe...@ovn.org
> * - Russell Bryant
>   - russ...@ovn.org
> * - Simon Horman
>   - ho...@ovn.org
> -   * - Thomas Graf
> - - tg...@noironetworks.com
> * - William Tu
>   - u9012...@gmail.com
> -   * - YAMAMOTO Takashi
> - - yamam...@midokura.com
>
>  The project also maintains a list of Emeritus Committers (or Maintainers).
>  More information about Emeritus Committers can be found here:
> @@ -85,12 +65,32 @@ More information about Emeritus Committers can be found 
> here:
>
> * - Name
>   - Email
> +   * - Alex Wang
> + - ee07b...@gmail.com
> +   * - Andy Zhou
> + - az...@ovn.org
> * - Ben Pfaff
>   - b...@ovn.org
> +   * - Daniele Di Proietto
> + - daniele.di.proie...@gmail.com
> * - Ethan J. Jackson
>   - e...@eecs.berkeley.edu
> +   * - Gurucharan Shetty
> + - g...@ovn.org
> +   * - Jarno Rajahalme
> + - ja...@ovn.org
> +   * - Jesse Gross
> + - je...@kernel.org
> * - Joe Stringer
>   - j...@ovn.org
> +   * - Justin Pettit
> + - jpet...@ovn.org
> +   * - Pravin B Shelar
> + - pshe...@ovn.org
> +   * - Thomas Graf
> + - tg...@tgraf.ch
> +   * - YAMAMOTO Takashi
> + - yamam...@midokura.com
>
Acked-by: Ansis Atteka 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Mike Pattrick]

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.

This patch also moves some trace function from do_xlate_actions into its
own function to reduce stack usage.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2104779
Signed-off-by: Mike Pattrick 

---

Since v1:
 - Refactored code into specialized functions and renamed variables for
 improved readability.
Since v2:
 - Removed inline keywords

Signed-off-by: Mike Pattrick 
---
 ofproto/ofproto-dpif-xlate.c | 223 ++-
 1 file changed, 141 insertions(+), 82 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 29f4daa63..468fb8e48 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -501,6 +501,90 @@ ctx_cancel_freeze(struct xlate_ctx *ctx)
 
 static void finish_freezing(struct xlate_ctx *ctx);
 
+/* These functions and structure are used to save stack space in actions that
+ * need to retain a large amount of xlate_ctx state. */
+struct xretained_state {
+union mf_subvalue new_stack[1024 / sizeof(union mf_subvalue)];
+uint64_t actset_stub[1024 / 8];
+struct ofpbuf old_stack;
+struct ofpbuf old_action_set;
+struct flow old_flow;
+struct flow old_base;
+struct flow_tnl flow_tnl;
+};
+
+/* The return of this function must be freed by xretain_state_restore(). */
+static struct xretained_state *
+xretain_state_save(struct xlate_ctx *ctx)
+{
+struct xretained_state *retained = xmalloc(sizeof *retained);
+
+retained->old_flow = ctx->xin->flow;
+retained->old_stack = ctx->stack;
+retained->old_action_set = ctx->action_set;
+ofpbuf_use_stub(>stack, retained->new_stack,
+sizeof retained->new_stack);
+ofpbuf_use_stub(>action_set, retained->actset_stub,
+sizeof retained->actset_stub);
+
+return retained;
+}
+
+static void
+xretain_tunnel_save(struct xlate_ctx *ctx, struct xretained_state *retained,
+const struct xport *out_dev)
+{
+struct flow *flow = >xin->flow;
+
+retained->flow_tnl = ctx->wc->masks.tunnel;
+memset(>wc->masks.tunnel, 0, sizeof ctx->wc->masks.tunnel);
+
+memset(>tunnel, 0, sizeof flow->tunnel);
+flow->tunnel.metadata.tab
+ = ofproto_get_tun_tab(_dev->xbridge->ofproto->up);
+ctx->wc->masks.tunnel.metadata.tab = flow->tunnel.metadata.tab;
+}
+
+static void
+xretain_base_save(const struct xlate_ctx *ctx,
+  struct xretained_state *retained)
+{
+retained->old_base = ctx->base_flow;
+}
+
+static void
+xretain_base_restore(struct xlate_ctx *ctx,
+ const struct xretained_state *retained)
+{
+ctx->base_flow = retained->old_base;
+}
+
+static void
+xretain_flow_restore(struct xlate_ctx *ctx,
+ const struct xretained_state *retained)
+{
+ctx->xin->flow = retained->old_flow;
+}
+
+static void
+xretain_tunnel_restore(struct xlate_ctx *ctx,
+   const struct xretained_state *retained)
+{
+ctx->wc->masks.tunnel = retained->flow_tnl;
+}
+
+static void
+xretain_state_restore(struct xlate_ctx *ctx, struct xretained_state *retained)
+{
+ctx->xin->flow = retained->old_flow;
+ofpbuf_uninit(>action_set);
+ctx->action_set = retained->old_action_set;
+ofpbuf_uninit(>stack);
+ctx->stack = retained->old_stack;
+
+free(retained);
+}
+
 /* A controller may use OFPP_NONE as the ingress port to indicate that
  * it did not arrive on a "real" port.  'ofpp_none_bundle' exists for
  * when an input bundle is needed for validation (e.g., mirroring or
@@ -3915,27 +3999,19 @@ static void
 patch_port_output(struct xlate_ctx *ctx, const struct xport *in_dev,
   struct xport *out_dev, bool is_last_action)
 {
-struct flow *flow = >xin->flow;
-struct flow old_flow = ctx->xin->flow;
-struct flow_tnl old_flow_tnl_wc = ctx->wc->masks.tunnel;
+struct ovs_list *old_trace = ctx->xin->trace;
 bool old_conntrack = ctx->conntracked;
+struct flow *flow = >xin->flow;
 bool old_was_mpls = ctx->was_mpls;
-ovs_version_t old_version = ctx->xin->tables_version;
-struct ofpbuf old_stack = ctx->stack;
-uint8_t new_stack[1024];
-struct ofpbuf old_action_set = ctx->action_set;
-struct ovs_list *old_trace = ctx->xin->trace;
-uint64_t actset_stub[1024 / 8];
+struct xretained_state *retained_state;
+
+retained_state = xretain_state_save(ctx);
+
+xretain_tunnel_save(ctx, retained_state, out_dev);
 
-ofpbuf_use_stub(>stack, new_stack, sizeof new_stack);
-ofpbuf_use_stub(>action_set, actset_stub, sizeof actset_stub);
+ovs_version_t old_version = 

Re: [ovs-dev] [PATCH v2] MAINTAINERS.rst: Move several people to emeritus status

[Simon Horman]

On Fri, May 19, 2023 at 10:52:51AM -0400, Russell Bryant wrote:
> The following document discusses emeritus committer status:
> 
> https://docs.openvswitch.org/en/latest/internals/committer-emeritus-status/
> 
> There are several people who I would guess consider themselves
> emeritus committers but have not formally declared it. Those moved to
> emeritus status in this commit have either explicitly communicated
> their desire to move or have both not been active in the last year and
> have not yet replied to this patch.
> 
> It is easy to re-add people in the future should any emeritus
> committer desire to become active again.
> 
> Per our policies, a vote of the majority of current committers (or
> the list of maintainers prior to this change) is required to move a
> committer to emeritus status.
> 
> Signed-off-by: Russell Bryant 
> CC: Alin Serdean 
> CC: Andy Zhou 
> CC: Ansis Atteka 
> CC: Daniele Di Proietto 
> CC: Gurucharan Shetty 
> CC: Ian Stokes 
> CC: Ilya Maximets 
> CC: Jarno Rajahalme 
> CC: Jesse Gross 
> CC: Justin Pettit 
> CC: Pravin B Shelar 
> CC: Simon Horman 
> CC: Thomas Graf 
> CC: William Tu 
> CC: YAMAMOTO Takashi 
> ---
>  MAINTAINERS.rst | 42 +-
>  1 file changed, 21 insertions(+), 21 deletions(-)

Acked-by: Simon Horman 

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

Re: [ovs-dev] [PATCH v2] tests: layer3-tunnels: Skip bareudp tests if not supported by kernel.

[Ilya Maximets]

On 5/25/23 15:07, Frode Nordahl wrote:
> The bareudp tests depend on specific kernel configuration to
> succeed.  Skip the test if the feature is not enabled in the
> running kernel.
> 
> Signed-off-by: Frode Nordahl 
> ---
>  tests/system-kmod-macros.at  | 10 ++
>  tests/system-layer3-tunnels.at   |  4 ++--
>  tests/system-userspace-macros.at |  8 
>  3 files changed, 20 insertions(+), 2 deletions(-)

Thanks!  Applied and backported down to 2.17.

Best regards, Ilya Maximets.

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

Re: [ovs-dev] [PATCH] userspace: modify the width of tpa and spa

[Ilya Maximets]

On 5/25/23 13:27, Simon Horman wrote:
> On Thu, May 25, 2023 at 05:54:37PM +0800, yangchang wrote:
>> Arp_spa and arp_tpa are IP addresses, their width should be 32 bits.
>>
>> Signed-off-by: yangchang 
> 
> Reviewed-by: Simon Horman 

The change looks good to me, but the 'userspace' part in the patch subject
doesn't make a lot of sense.  I replaced it with 'ovs-fields'.  With that,
applied and backported down to 2.17.

Thanks!

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

Re: [ovs-dev] OVS NDP proxy / nd_options_type

[Sesterhenn, Maximilian via dev]

Hello together,

I would like to follow up on this.

Meanwhile I was able to find the following discussion [1] in this mailing list 
some time ago which looks like is about the same topic.

Is this something that's currently not possible within OVS?
If so, are there plans to add this?

I had a look at how OVN is doing it, and it seems like OVN is sending this type 
of traffic to their controller for external processing.

Would be nice if we could answer NDP NS packets with OVS flows directly like we 
already can for ARP requests.


[1] https://www.mail-archive.com/ovs-dev@openvswitch.org/msg46880.html
[2] 
https://github.com/openvswitch/ovs/blob/8045c0f8de5192355ca438ed7eef77457c3c1625/ofproto/ofproto-dpif.c#L4695

Thanks in advance for your time!

Best regards
Maximilian

From: discuss  on behalf of Sesterhenn, 
Maximilian via discuss 
Sent: Monday, May 22, 2023 22:16
To: ovs-disc...@openvswitch.org 
Subject: [ovs-discuss] OVS NDP proxy / nd_options_type

ovs-disc...@openvswitch.org appears similar to someone who previously sent you 
email, but may not be that person. Learn why this could be a 
risk

OUTSIDE-EPG!


Hey there,

maybe someone from this list can help me.

I'm currently trying to implement a simple NDP proxy using OVS.

For that, I defined two basic flows that should do the trick:

cookie=0x3e6,priority=1100,icmp6,icmp_type=135,icmp_code=0,in_port=patch-provnet-f,dl_src=MAC
 actions=set_field:136->icmpv6_type,set_field:0->icmpv6_code,goto_table:10

cookie=0x3e6,priority=1000,table=10,icmp6,icmp_type=136,icmp_code=0,in_port=patch-provnet-f,dl_src=MAC
 actions=move:NXM_OF_ETH_SRC[]->NXM_OF_ETH_DST[], 
mod_dl_src:MAC,move:NXM_NX_IPV6_SRC[]->NXM_NX_IPV6_DST[],move:NXM_NX_ND_TARGET[]->NXM_NX_IPV6_SRC[],set_field:MAC->nd_tll,set_field:2->nd_options_type,in_port

However, I'm unable to install the second flow, it works without the 
nd_options_type field.
It errors out with the message "OFPT_ERROR (xid=0xa): OFPBAC_BAD_SET_ARGUMENT".
The problem is that without that field my NA messages are incorrect.

I digged further and realized that this is something that older version of the 
OVS do not support.
I think I found the feature on the release notes of OVS 2.12.0 [1].

However, I'm on a quite recent system so that should not be a problem.

Is someone with more experience able to tell me what the reason could be?

Versioning:
Rocky Linux 9.1
Linux 5.14.0-162.23.1.el9_1.x86_64

# ovs-ofctl --version
ovs-ofctl (Open vSwitch) 3.1.2
OpenFlow versions 0x1:0x6

# ovs-vsctl --version
ovs-vsctl (Open vSwitch) 3.1.2
DB Schema 8.3.1

# modinfo openvswitch
filename:   
/lib/modules/5.14.0-162.23.1.el9_1.x86_64/kernel/net/openvswitch/openvswitch.ko.xz
alias:  net-pf-16-proto-16-family-ovs_ct_limit
alias:  net-pf-16-proto-16-family-ovs_meter
alias:  net-pf-16-proto-16-family-ovs_packet
alias:  net-pf-16-proto-16-family-ovs_flow
alias:  net-pf-16-proto-16-family-ovs_vport
alias:  net-pf-16-proto-16-family-ovs_datapath
license:GPL
description:Open vSwitch switching datapath
rhelversion:9.1
srcversion: C1E5F3D9CD0C9A09006C69E
depends:nf_conntrack,nf_nat,nf_conncount,libcrc32c,nf_defrag_ipv6
retpoline:  Y
intree: Y
name:   openvswitch
vermagic:   5.14.0-162.23.1.el9_1.x86_64 SMP preempt mod_unload modversions

[1] 
https://mail.openvswitch.org/pipermail/ovs-announce/2019-September/000255.html

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

[ovs-dev] [PATCH ovn v3] controller: Ignore DNS queries with RRs

[Brian Haley]

DNS queries with optional records (RRs), for example, with
cookies for EDNS, are not supported by the OVN resolver.
Trying to reply will result in mangled responses that
clients do not understand - the ANSWER section will
contain an incorrect option.

Instead, just return early when one is present, which
will trigger a negative response and cause clients to
go to the upstream forwarder, hopefully resulting in a
successful query.

In our testing, the resolver only retries if the
response is correctly formatted, which now happens
with this change.

Closes issue #192
Signed-off-by: Brian Haley 
---
Changes since v2:
- Updated commit message to be more clear
---
Changes since v1:
- Added issue #192 to commit message
---
 controller/pinctrl.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index b5df8b1eb..b45b4c747 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -2864,6 +2864,13 @@ pinctrl_handle_dns_lookup(
 goto exit;
 }
 
+/* Check if there is an additional record present, which is unsupported */
+if (in_dns_header->arcount) {
+VLOG_DBG_RL(, "Received DNS query with additional records, which"
+" is unsupported");
+goto exit;
+}
+
 struct udp_header *in_udp = dp_packet_l4(pkt_in);
 size_t udp_len = ntohs(in_udp->udp_len);
 size_t l4_len = dp_packet_l4_size(pkt_in);
-- 
2.34.1

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

[ovs-dev] [PATCH ovn 9/9] backporting-patches: Add concrete policy for where backports go.

[Mark Michelson]

This is based on discussions that have happened both on the ovs-dev
mailing list as well as public IRC developer meetings.

Signed-off-by: Mark Michelson 
---
 .../contributing/backporting-patches.rst  | 28 +++
 1 file changed, 28 insertions(+)

diff --git a/Documentation/internals/contributing/backporting-patches.rst 
b/Documentation/internals/contributing/backporting-patches.rst
index d7e4522cf..5011d0f9c 100644
--- a/Documentation/internals/contributing/backporting-patches.rst
+++ b/Documentation/internals/contributing/backporting-patches.rst
@@ -73,6 +73,34 @@ not a trivial cherry-pick, then the maintainer may opt to 
submit the backport
 for the older branch on the mailing list for further review. This should be 
done
 in the same manner as described above.
 
+Supported Versions
+~~
+
+As documented in :doc:`release-policy`, standard term support versions receive
+regular releases for a year, and LTS versions receive regular releases for two
+years, plus an additional year of critical and security fixes.
+
+To make things easy, maintainers should simply backport all bugfixes to the
+previous four branches before main. This is guaranteed to get the fix into all
+supported standard-support versions as well as the current LTS version. This
+will mean that maintainers will backport bugfixes to branches representing
+versions that are not currently supported.
+
+Critical and security fixes should be handled differently. Maintainers should
+determine what is the oldest LTS version that currently is supported for
+critical and security fixes. Maintainers should backport these fixes to all
+branches between main and that LTS version. This will mean that maintainers
+will backport critical and security fixes into branches representing versions
+that are not currently supported.
+
+The reason for backporting fixes into unsupported versions is twofold:
+
+- Backporting bugfixes into unsupported versions likely makes it easier to
+  backport critical and security fixes into older versions when necessary.
+- Backporting critical and security fixes into unsupported versions allows for
+  users that are not ready to upgrade to a supported version to continue using
+  the branch tip until they are ready to fully upgrade.
+
 Submission
 ~~
 
-- 
2.39.2

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

[ovs-dev] [PATCH ovn 8/9] backporting-patches: Fix inaccuracies in the document.

[Mark Michelson]

This document appears to be based on the OVS document, but things are
done slightly differently in OVN.

* OVN is 100% userspace, so there is no need to clarify "userspace"
  anywhere.
* The commit message example is not how we do things in OVN. Backport
  commit messages are typically identical to the original commit
  message, so there is nothing to specify.
* Linus is not involved in OVN's development :)

Signed-off-by: Mark Michelson 
---
 .../contributing/backporting-patches.rst  | 56 ---
 1 file changed, 11 insertions(+), 45 deletions(-)

diff --git a/Documentation/internals/contributing/backporting-patches.rst 
b/Documentation/internals/contributing/backporting-patches.rst
index 711a8e36b..d7e4522cf 100644
--- a/Documentation/internals/contributing/backporting-patches.rst
+++ b/Documentation/internals/contributing/backporting-patches.rst
@@ -42,23 +42,21 @@ within OVN, but is broadly applied in the following fashion:
   development branch.
 - Maintainers backport changes from a development branch to release branches.
 
-With regards to OVN user space code and code that does not comprise
-the Linux datapath and compat code, the development branch is `main` in the
-OVN repository. Patches are applied first to this branch, then to the
-most recent `branch-X.Y`, then earlier `branch-X.Z`, and so on. The most common
-kind of patch in this category is a bugfix which affects main and other
-branches.
+The development branch is `main` in the OVN repository. Patches are applied
+first to this branch, then to the most recent `branch-X.Y`, then earlier
+`branch-X.Z`, and so on. The most common kind of patch in this category is
+a bugfix which affects main and other branches.
 
-Changes to userspace components

+Backport Policy
+---
 
 Patches which are fixing bugs should be considered for backporting from
 `main` to release branches. OVN contributors submit their patches
-targeted to the `main` branch, using the ``Fixes`` tag described in
+targeted to the `main` branch, using the ``Fixes`` tag desribed in
 :doc:`submitting-patches`. The maintainer first applies the patch to `main`,
-then backports the patch to each older affected tree, as far back as it goes or
-at least to all currently supported branches. This is usually each branch back
-to the most recent LTS release branch.
+then backports the patch to each older affected tree, as far back as it goes
+or at least to all currently supported branches. This is usually each branch
+back to the most recent LTS release branch.
 
 If the fix only affects a particular branch and not `main`, contributors
 should submit the change with the target branch listed in the subject line of
@@ -79,39 +77,7 @@ Submission
 ~~
 
 Once the patches are all assembled and working on the OVN tree, they
-need to be formatted again using ``git format-patch``. The common format for
-commit messages for Linux backport patches is as follows:
-
-::
-
-datapath: Remove incorrect WARN_ONCE().
-
-Upstream commit:
-commit c6b2aafffc6934be72d96855c9a1d88970597fbc
-Author: Jarno Rajahalme 
-Date:   Mon Aug 1 19:08:29 2016 -0700
-
-openvswitch: Remove incorrect WARN_ONCE().
-
-ovs_ct_find_existing() issues a warning if an existing conntrack entry
-classified as IP_CT_NEW is found, with the premise that this should
-not happen.  However, a newly confirmed, non-expected conntrack entry
-remains IP_CT_NEW as long as no reply direction traffic is seen.  This
-has resulted into somewhat confusing kernel log messages.  This patch
-removes this check and warning.
-
-Fixes: 289f2253 ("openvswitch: Find existing conntrack entry after 
upcall.")
-Suggested-by: Joe Stringer 
-Signed-off-by: Jarno Rajahalme 
-Acked-by: Joe Stringer 
-
-Signed-off-by: Jarno Rajahalme 
-
-The upstream commit SHA should be the one that appears in Linus' tree so that
-reviewers can compare the backported patch with the one upstream.  Note that
-the subject line for the backported patch replaces the original patch's
-``openvswitch`` prefix with ``datapath``. Patches which only affect the
-``datapath/linux/compat`` directory should be prefixed with ``compat``.
+need to be formatted again using ``git format-patch``.
 
 The contents of a backport should be equivalent to the changes made by the
 original patch; explain any variations from the original patch in the commit
-- 
2.39.2

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

[ovs-dev] [PATCH ovn 7/9] backporting-patches: Change "master" to "main".

[Mark Michelson]

OVN hasn't used the name "master" for quite a while, and it looks like
we missed this document when the change to "main" was made.

Signed-off-by: Mark Michelson 
---
 .../internals/contributing/backporting-patches.rst   | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/internals/contributing/backporting-patches.rst 
b/Documentation/internals/contributing/backporting-patches.rst
index fbc200bfc..711a8e36b 100644
--- a/Documentation/internals/contributing/backporting-patches.rst
+++ b/Documentation/internals/contributing/backporting-patches.rst
@@ -43,24 +43,24 @@ within OVN, but is broadly applied in the following fashion:
 - Maintainers backport changes from a development branch to release branches.
 
 With regards to OVN user space code and code that does not comprise
-the Linux datapath and compat code, the development branch is `master` in the
+the Linux datapath and compat code, the development branch is `main` in the
 OVN repository. Patches are applied first to this branch, then to the
 most recent `branch-X.Y`, then earlier `branch-X.Z`, and so on. The most common
-kind of patch in this category is a bugfix which affects master and other
+kind of patch in this category is a bugfix which affects main and other
 branches.
 
 Changes to userspace components
 ---
 
 Patches which are fixing bugs should be considered for backporting from
-`master` to release branches. OVN contributors submit their patches
-targeted to the `master` branch, using the ``Fixes`` tag described in
-:doc:`submitting-patches`. The maintainer first applies the patch to `master`,
+`main` to release branches. OVN contributors submit their patches
+targeted to the `main` branch, using the ``Fixes`` tag described in
+:doc:`submitting-patches`. The maintainer first applies the patch to `main`,
 then backports the patch to each older affected tree, as far back as it goes or
 at least to all currently supported branches. This is usually each branch back
 to the most recent LTS release branch.
 
-If the fix only affects a particular branch and not `master`, contributors
+If the fix only affects a particular branch and not `main`, contributors
 should submit the change with the target branch listed in the subject line of
 the patch. Contributors should list all versions that the bug affects. The
 ``git format-patch`` argument ``--subject-prefix`` may be used when posting the
-- 
2.39.2

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

[ovs-dev] [PATCH ovn 5/9] release-process: Add section for standard-term support releases.

[Mark Michelson]

Signed-off-by: Mark Michelson 
---
 Documentation/internals/release-process.rst | 16 
 1 file changed, 16 insertions(+)

diff --git a/Documentation/internals/release-process.rst 
b/Documentation/internals/release-process.rst
index 7c6a72a41..1f4b90143 100644
--- a/Documentation/internals/release-process.rst
+++ b/Documentation/internals/release-process.rst
@@ -99,6 +99,22 @@ one year of critical bug fixes and security fixes.
 The current LTS version is documented on the `Long Term Support Releases`__
 page of `ovn.org`__.
 
+Standard-term Support Versions
+--
+
+Versions of OVN that are not designated as LTS versions are standard-term
+support releases.
+
+Standard term support versions will have releases made regularly for one year.
+There are no additional windows for which releases will be made for critical
+or security fixes.
+
+The branches for standard term support versions may receive bug fixes and
+critical and security fixes after the OVN team has stopped creating regular
+releases. This is strictly to facilitate easy backporting of fixes to LTS
+versions that are still receiving regular releases. Even though the branch may
+receive additional fixes, no releases will be made with these fixes.
+
 Release Numbering
 -
 
-- 
2.39.2

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

[ovs-dev] [PATCH ovn 6/9] release-policy: Document when versions receive new releases.

[Mark Michelson]

The OVN team has had no concrete policy regarding when releases are
made for a given version. In the past, new releases have been made only
when something critical has been found, meaning that most bug fixes
applied to a given OVN version never end up in a release.

This change proposes a monthly release per supported OVN version.

Signed-off-by: Mark Michelson 
---
 Documentation/internals/release-process.rst | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/internals/release-process.rst 
b/Documentation/internals/release-process.rst
index 1f4b90143..0287795dc 100644
--- a/Documentation/internals/release-process.rst
+++ b/Documentation/internals/release-process.rst
@@ -152,6 +152,12 @@ approximate:
 | T + 6 | Sep 1, Mar 1, ...   | Release version x.y.0|
 +---+-+--+
 
+After the .0 release is created for a given OVN version, that version will have
+releases made every month until its support lifetime has concluded. If for some
+reason, no changes occur to a supported OVN version during a month, then no
+release will be made that month. Therefore, there is no guarantee about the
+exact number of releases to be expected for any given OVN version.
+
 Release Calendar
 
 
-- 
2.39.2

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

[ovs-dev] [PATCH ovn 4/9] release-process: State release policy for LTS versions.

[Mark Michelson]

The definition of "regular releases" is being kept purposely vague in
this commit because a later commit will define what the interval is for
these releases.

Signed-off-by: Mark Michelson 
---
 Documentation/internals/release-process.rst | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/internals/release-process.rst 
b/Documentation/internals/release-process.rst
index 9a178e6b9..7c6a72a41 100644
--- a/Documentation/internals/release-process.rst
+++ b/Documentation/internals/release-process.rst
@@ -84,12 +84,13 @@ The OVN project will periodically designate a version as 
"long-term support" or
 LTS for short. An LTS version has the distinction of being maintained for
 longer than a standard version.
 
-LTS versions will receive bug fixes until the point that another LTS is
-released. At that point, the old LTS will receive an additional year of
+LTS versions will have releases made regularly until the point that another
+LTS is released. At that point, the old LTS will receive an additional year of
 critical and security fixes. Critical fixes are those that are required to
 ensure basic operation (e.g. memory leak fixes, crash fixes). Security fixes
 are those that address concerns about exploitable flaws in OVN and that have a
-corresponding CVE report.
+corresponding CVE report. Whenever such a fix is applied, a new release will
+be made for the LTS version.
 
 LTS versions are scheduled to be created once every two years. This means
 that any given LTS will receive bug fix support for two years, followed by
-- 
2.39.2

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

[ovs-dev] [PATCH ovn 1/9] release-process: Use more accurate example releases.

[Mark Michelson]

We don't use a four-digit major version, just the final two digits of
the year.

Signed-off-by: Mark Michelson 
---
 Documentation/internals/release-process.rst | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/internals/release-process.rst 
b/Documentation/internals/release-process.rst
index 848c2db84..d9aabf8d1 100644
--- a/Documentation/internals/release-process.rst
+++ b/Documentation/internals/release-process.rst
@@ -50,7 +50,7 @@ Scheduling`_ for the timing of each stage:
Please propose and discuss exceptions on ovs-dev.
  
 2. Fork a release branch from main, named for the expected release number,
-   e.g. "branch-2019.10" for the branch that will yield OVN 2019.10.x.
+   e.g. "branch-25.09" for the branch that will yield OVN 25.09.x.
 
Release branches are intended for testing and stabilization.  At this stage
and in later stages, they should receive only bug fixes, not new features.
@@ -65,14 +65,14 @@ Scheduling`_ for the timing of each stage:
and risk and discussed on ovs-dev before creating the branch.
 
 3. When committers come to rough consensus that the release is ready, they
-   release the .0 release on its branch, e.g. 2019.10.0 for branch-2019.10.  To
+   release the .0 release on its branch, e.g. 25.09.0 for branch-25.09.  To
make the actual release, a committer pushes a signed tag named, e.g.
-   v2019.10.0, to the OVN repository, makes a release tarball available on
+   v25.09.0, to the OVN repository, makes a release tarball available on
openvswitch.org, and posts a release announcement to ovs-announce.
 
 4. As bug fixes accumulate, or after important bugs or vulnerabilities are
-   fixed, committers may make additional releases from a branch: 2019.10.1,
-   2019.10.2, and so on.  The process is the same for these additional release
+   fixed, committers may make additional releases from a branch: 25.09.1,
+   25.09.2, and so on.  The process is the same for these additional release
as for a .0 release.
 
 .. _long-term-support:
-- 
2.39.2

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

[ovs-dev] [PATCH ovn 3/9] release-policy: Change "release" to "version" in LTS section.

[Mark Michelson]

Upcoming changes to the documentation will add documentation for
standard term support versions and will document a policy for when
releases are made for these versions. Our current use of "release" in
the documentation is interchangeable with "version" but upcoming changes
will differentiate these more clearly.

Signed-off-by: Mark Michelson 
---
 Documentation/internals/release-process.rst | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/internals/release-process.rst 
b/Documentation/internals/release-process.rst
index dd19744f9..9a178e6b9 100644
--- a/Documentation/internals/release-process.rst
+++ b/Documentation/internals/release-process.rst
@@ -77,21 +77,21 @@ Scheduling`_ for the timing of each stage:
 
 .. _long-term-support:
 
-Long-term Support Releases
+Long-term Support Versions
 --
 
-The OVN project will periodically designate a release as "long-term support" or
-LTS for short. An LTS release has the distinction of being maintained for
-longer than a standard release.
+The OVN project will periodically designate a version as "long-term support" or
+LTS for short. An LTS version has the distinction of being maintained for
+longer than a standard version.
 
-LTS releases will receive bug fixes until the point that another LTS is
+LTS versions will receive bug fixes until the point that another LTS is
 released. At that point, the old LTS will receive an additional year of
 critical and security fixes. Critical fixes are those that are required to
 ensure basic operation (e.g. memory leak fixes, crash fixes). Security fixes
 are those that address concerns about exploitable flaws in OVN and that have a
 corresponding CVE report.
 
-LTS releases are scheduled to be released once every two years. This means
+LTS versions are scheduled to be created once every two years. This means
 that any given LTS will receive bug fix support for two years, followed by
 one year of critical bug fixes and security fixes.
 
-- 
2.39.2

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

[ovs-dev] [PATCH ovn 2/9] release-process: Switch to two releases per year.

[Mark Michelson]

Based on discussions that have occurred on the development mailing list
and public IRC meetings, we have decided to reduce OVN to having two
releases per year instead of four.

When OVN initially spun off from OVS, the amount of feature requests and
performance improvements were happening at a rapid pace. Releasing every
three months made it possible to get these changes released rapidly.

OVN has reached a point now where the number of new features has
decreased. And the new features being requested are more complex and
could use a longer lead time to be implemented.

Therefore, this change makes OVN's release policy change to having two
releases per year instead of four.

Given that we now have a longer time between releases, this change also
gives a longer time for soft freeze and testing the branches before
release.

Signed-off-by: Mark Michelson 
---
 Documentation/internals/release-process.rst | 23 -
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/Documentation/internals/release-process.rst 
b/Documentation/internals/release-process.rst
index d9aabf8d1..dd19744f9 100644
--- a/Documentation/internals/release-process.rst
+++ b/Documentation/internals/release-process.rst
@@ -120,19 +120,19 @@ with an unspecified date.
 Release Scheduling
 --
 
-OVN makes releases at the following three-month cadence.  All dates are
+OVN makes releases at the following six-month cadence.  All dates are
 approximate:
 
 +---+-+--+
 | Time (months) | Example Dates   | Stage|
 +---+-+--+
-| T | Dec 1, Mar 1, ...   | Begin x.y release cycle  |
+| T | Mar 1, Sep 1, ...   | Begin x.y release cycle  |
 +---+-+--+
-| T + 2 | Feb 1, May 1, ...   | "Soft freeze" main for x.y release   |
+| T + 4 | Jul 1, Jan 1, ...   | "Soft freeze" main for x.y release   |
 +---+-+--+
-| T + 2.5   | Feb 15, May 15, ... | Fork branch-x.y from main|
+| T + 5 | Aug 1, Feb 1, ...   | Fork branch-x.y from main|
 +---+-+--+
-| T + 3 | Mar 1, Jun 1, ...   | Release version x.y.0|
+| T + 6 | Sep 1, Mar 1, ...   | Release version x.y.0|
 +---+-+--+
 
 Release Calendar
@@ -140,7 +140,8 @@ Release Calendar
 
 The 2023 timetable is shown below. Note that these dates are not set in stone.
 If extenuating circumstances arise, a release may be delayed from its target
-date.
+date. Also note that the release policy changed partway through 2023, which is
+why the release dates and timetables do not line up with the example above.
 
 +-+-+-+-+
 | Release | Soft Freeze | Branch Creation | Release |
@@ -151,21 +152,15 @@ date.
 +-+-+-+-+
 | 23.09.0 | Aug 4   | Aug 18  | Sep 1   |
 +-+-+-+-+
-| 23.12.0 | Nov 3   | Nov 17  | Dec 1   |
-+-+-+-+-+
 
 Below is the 2024 timetable
 
 +-+-+-+-+
 | Release | Soft Freeze | Branch Creation | Release |
 +-+-+-+-+
-| 24.03.0 | Feb 2   | Feb 16  | Mar 1   |
-+-+-+-+-+
-| 24.06.0 | May 10  | May 24  | Jun 7   |
-+-+-+-+-+
-| 24.09.0 | Aug 9   | Aug 23  | Sep 6   |
+| 24.03.0 | Jan 5   | Feb 2   | Mar 1   |
 +-+-+-+-+
-| 24.12.0 | Nov 8   | Nov 22  | Dec 6   |
+| 24.09.0 | Jul 5   | Aug 2   | Sep 6   |
 +-+-+-+-+
 
 Contact
-- 
2.39.2

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

[ovs-dev] [PATCH ovn 0/9] OVN Release Policy Updates

[Mark Michelson]

The current state of development in OVN has resulted in many discussions
happening about how things should change. The main points of the
discussions have been:

* Recent releases have been much more bare than releases made two or
  three years ago.
* Supporting as many versions of OVN as we have is a maintenance burden.
* It is not well-defined what the lifetime of non-LTS OVN versions is.
* We rarely, if ever, make sub-releases of released OVN versions, so it
  sometimes feels like backporting bugfixes is pointless.
* It is not always clear as a maintainer which branches should receive
  backports of fixes.

This set of changes aims to address these issues, while also correcting
some existing errors in the documentation.

The reason for the large number of patches is to allow for easy
discussion about each aspect of the change. This can allow for us to
reach conclusions for certain aspects and get them integrated into the
documentation while allowing for other aspects to continue to be fleshed
out.

Mark Michelson (9):
  release-process: Use more accurate example releases.
  release-process: Switch to two releases per year.
  release-policy: Change "release" to "version" in LTS section.
  release-process: State release policy for LTS versions.
  release-process: Add section for standard-term support releases.
  release-policy: Document when versions receive new releases.
  backporting-patches: Change "master" to "main".
  backporting-patches: Fix inaccuracies in the document.
  backporting-patches: Add concrete policy for where backports go.

 .../contributing/backporting-patches.rst  | 86 +--
 Documentation/internals/release-process.rst   | 72 ++--
 2 files changed, 85 insertions(+), 73 deletions(-)

-- 
2.39.2

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

Re: [ovs-dev] [PATCH v12] ofproto-dpif-upcall: Don't set statistics to 0 when they jump back

[Ilya Maximets]

On 5/25/23 15:37, Balazs Nemeth wrote:
> The only way that stats->{n_packets,n_bytes} would decrease is due to an
> overflow, or if there are bugs in how statistics are handled. In the
> past, there were multiple issues that caused a jump backward. A
> workaround was in place to set the statistics to 0 in that case. When
> this happened while the revalidator was under heavy load, the workaround
> had an unintended side effect where should_revalidate returned false
> causing the flow to be removed because the metric it calculated was
> based on a bogus value. Since many of those bugs have now been
> identified and resolved, there is no need to set the statistics to 0. In
> addition, the (unlikely) overflow still needs to be handled
> appropriately. If an unexpected jump does happen, just log it as a
> warning.
> 
> Signed-off-by: Balazs Nemeth 
> ---
>  ofproto/ofproto-dpif-upcall.c | 29 +++--
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index cd57fdbd9..4c34d695e 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -2339,6 +2339,26 @@ exit:
>  return result;
>  }
>  
> +static void
> +log_unexpected_stats_jump(struct udpif_key *ukey,
> +  const struct dpif_flow_stats *stats)

We need to add a thread-safety annotation here to make clang happy:

OVS_REQUIRES(ukey->mutex)

> +{
> +static struct vlog_rate_limit rll = VLOG_RATE_LIMIT_INIT(1, 5);
> +struct ds ds = DS_EMPTY_INITIALIZER;
> +struct ofpbuf *actions;
> +
> +odp_format_ufid(>ufid, );
> +ds_put_cstr(, " ");
> +odp_flow_key_format(ukey->key, ukey->key_len, );
> +ds_put_cstr(, ", actions:");
> +actions = ovsrcu_get(struct ofpbuf *, >actions);
> +format_odp_actions(, actions->data, actions->size, NULL);
> +VLOG_WARN_RL(, "Unexpected jump in packet stats from %"PRIu64
> +" to %"PRIu64" when handling ukey %s",
> +stats->n_packets, ukey->stats.n_packets, ds_cstr());

And stats should be in the opposite order.  The jump is from 
ukey->stats.n_packets
to stats->n_packets, not the other way around.  Sorry I missed that before.

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

Re: [ovs-dev] [PATCH v5 0/3] Support flowlabel calculation in SRv6 tunnels

[Ilya Maximets]

On 5/23/23 05:58, Nobuhiro MIKI wrote:
> This patchset adds a feature to support the calculation of flowlabel in
> SRv6 tunnels.
> 
> v5:
> * Write netdev_srv6_flowlabel enum to ipv6 flowlabel in header build.
> * Fix docs.
> * Add tests for the same flow.
> v4:
> * Set flowlabel on header push.
> * Fix docs.
> * Use RSS hash.
> v3:
> * Fix building error (sparse).
> v2:
> * Fix building error.
> 
> Nobuhiro MIKI (3):
>   netdev-native-tnl: Add ipv6_label param in netdev_tnl_push_ip_header.
>   netdev-native-tnl: Add ipv6_label param in netdev_tnl_ip_build_header.
>   userspace: Add new option srv6_flowlabel in SRv6 tunnel.
> 
>  lib/netdev-native-tnl.c   | 63 +++--
>  lib/netdev-native-tnl.h   |  6 +--
>  lib/netdev-vport.c|  8 
>  lib/netdev.h  | 12 +
>  lib/packets.c |  2 +-
>  lib/packets.h |  2 +
>  tests/tunnel-push-pop-ipv6.at | 86 +++
>  vswitchd/vswitch.xml  | 26 +++
>  8 files changed, 186 insertions(+), 19 deletions(-)
> 

LGTM, Thanks!  I applied the set.

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

Re: [ovs-dev] [PATCH 3/3] netdev-vport: RCU-fy tunnel config.

[Ilya Maximets]

On 5/25/23 13:51, Simon Horman wrote:
> On Sat, May 20, 2023 at 02:31:20AM +0200, Ilya Maximets wrote:
>> Tunnel config can be accessed by multiple threads at the same time and
>> it is supposed to be protected by the netdev_vport mutex.  However,
>> many functions are getting direct access to it via netdev API without
>> taking the mutex, creating a potential for various race conditions.
>>
>> Fix that by protecting the tunnel config with RCU.  The whole structure
>> is replaced on configuration changes.  Individual fields are never
>> updated and the structure itself is constant.  This way it can be safely
>> used by different threads within RCU grace period.
>>
>> Signed-off-by: Ilya Maximets 
> 
> Reviewed-by: Simon Horman 
> 

Thanks!  I applied the set.  Also backported down to 2.17
as these are mainly bug fixes.

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

Re: [ovs-dev] [PATCH ovn v2] controller: Ignore DNS queries with RRs

[Brian Haley]

Sorry for the top post, but I was wondering if there was a way to 
re-trigger the bot testing action on a patch? Somehow the testing on the 
v2 one failed even though v1 passed [0]. Since the only change was in 
the commit message seems it could just be a flaky test? Unless I'm 
missing something.


Thanks,

-Brian

[0] 
https://patchwork.ozlabs.org/project/ovn/patch/2023052223.363328-1-haleyb@gmail.com/


On 5/22/23 4:13 PM, Brian Haley wrote:

DNS queries with optional records (RRs), for example, with
cookies for EDNS, are not supported by the OVN resolver.
Trying to reply sometimes results in mangled responses
that clients do not understand.

Instead, just return early when one is present, which
should trigger a negative response and cause clients to
go to the upstream forwarder, hopefully resulting in a
successful query.

Closes issue #192
Signed-off-by: Brian Haley 
---
Changes since v1:
- Added issue #192 to commit message
---
  controller/pinctrl.c | 7 +++
  1 file changed, 7 insertions(+)

diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index b5df8b1eb..b45b4c747 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -2864,6 +2864,13 @@ pinctrl_handle_dns_lookup(
  goto exit;
  }
  
+/* Check if there is an additional record present, which is unsupported */

+if (in_dns_header->arcount) {
+VLOG_DBG_RL(, "Received DNS query with additional records, which"
+" is unsupported");
+goto exit;
+}
+
  struct udp_header *in_udp = dp_packet_l4(pkt_in);
  size_t udp_len = ntohs(in_udp->udp_len);
  size_t l4_len = dp_packet_l4_size(pkt_in);

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

[ovs-dev] [PATCH ovn] ovn-controller.c: Fix assertion failure during address set update.

[Han Zhou]

When an address set is deleted and a new one is created immediately with
the same name, the OVSDB deletion and creation notifications can come to
ovn-controller within the same message, and the order of the deletion
and addition in the IDL is undefined. In this case, if the deletion
happens to be processed after the addition, it will result in problems.

If the content of the new address set is the same as the old one, the
addition will be handled as no change to an existing address set and
ignored, but later when handling the deletion, the related flows will be
deleted, while we actually expect no change to the flows.

If the content of the new address set is different from the old one, the
addition will be handled as an update to the old address set, and the
delta will be tracked in the I-P engine node data's "updated" member.
Later when handling the deletion, the address set will be deleted from
the engine node data. So at this point the tracked change in "updated"
doesn't have a related address set object in the data, so later when the
lflow node handles the addr_sets changes, the assertion fails as shown
in below example backtrace:

0  0x7ffbe5fcabc5 in raise () from /lib64/libc.so.6
1  0x7ffbe5fb38a4 in abort () from /lib64/libc.so.6
2  0x004f579e in ovs_abort_valist (err_no=, 
format=, args=) at ../lib/util.c:444
3  0x004fd001 in vlog_abort_valist (module_=, 
message=0x5e6fd0 "%s: assertion %s failed in %s()", 
args=args@entry=0x7ffed304caa8) at ../lib/vlog.c:1249
4  0x004fd097 in vlog_abort (module=, message=) at ../lib/vlog.c:1263
5  0x004f54e1 in ovs_assert_failure (where=, 
function=, condition=) at ../lib/util.c:86
6  0x0041941d in as_update_can_be_handled (l_ctx_in=0x7ffed304cda0, 
l_ctx_in=0x7ffed304cda0, as_diff=0x25520f0, as_name=0x2550a00 "as1") at 
../controller/lflow.c:766
7  lflow_handle_addr_set_update (as_name=0x2550a00 "as1", as_diff=0x25520f0, 
l_ctx_in=0x7ffed304cda0, l_ctx_out=0x7ffed304cd50, changed=0x7ffed304cd4f) at 
../controller/lflow.c:869
8  0x00438376 in lflow_output_addr_sets_handler (node=0x7ffed3054420, 
data=) at ../controller/ovn-controller.c:2648
9  0x0045708e in engine_compute (recompute_allowed=, 
node=) at ../lib/inc-proc-eng.c:417
10 engine_run_node (recompute_allowed=true, node=0x7ffed3054420) at 
../lib/inc-proc-eng.c:479
11 engine_run (recompute_allowed=true) at ../lib/inc-proc-eng.c:504
12 0x0040939b in main (argc=, argv=) at 
../controller/ovn-controller.c:3757

This patch fixes it by handling deletions before additions/updates,
similar to how we handle other changes such as lflow and port-groups.

Fixes: c0fe8dca767f ("ovn-controller: Tracking SB address set updates.")
Signed-off-by: Han Zhou 
---
 controller/ovn-controller.c |  6 -
 tests/ovn-controller.at | 51 +
 2 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 1151d3664478..2706dc628d6d 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -2015,7 +2015,11 @@ addr_sets_update(const struct sbrec_address_set_table 
*address_set_table,
 if (sbrec_address_set_is_deleted(as)) {
 expr_const_sets_remove(addr_sets, as->name);
 sset_add(deleted, as->name);
-} else {
+}
+}
+
+SBREC_ADDRESS_SET_TABLE_FOR_EACH_TRACKED (as, address_set_table) {
+if (!sbrec_address_set_is_deleted(as)) {
 struct expr_constant_set *cs_old = shash_find_data(addr_sets,
as->name);
 if (!cs_old) {
diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index 64d6a9336613..a95834e69eca 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -2065,6 +2065,57 @@ AT_CHECK([echo $(($reprocess_count_new - 
$reprocess_count_old))], [0], [2
 OVN_CLEANUP([hv1])
 AT_CLEANUP
 
+AT_SETUP([ovn-controller - address set del-and-add])
+
+ovn_start
+
+net_add n1
+sim_add hv1
+as hv1
+check ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+check ovs-vsctl -- add-port br-int hv1-vif1 -- \
+set interface hv1-vif1 external-ids:iface-id=ls1-lp1
+
+check ovn-nbctl ls-add ls1
+
+check ovn-nbctl lsp-add ls1 ls1-lp1 \
+-- lsp-set-addresses ls1-lp1 "f0:00:00:00:00:01"
+
+wait_for_ports_up
+ovn-appctl -t ovn-controller vlog/set file:dbg
+
+ovn-nbctl create address_set name=as1 addresses=8.8.8.8
+check ovn-nbctl acl-add ls1 to-lport 100 'outport == "ls1-lp1" && ip4.src == 
$as1' drop
+check ovn-nbctl --wait=hv sync
+AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep -c "priority=1100"], 
[0], [1
+])
+
+# pause ovn-northd
+check as northd ovn-appctl -t ovn-northd pause
+check as northd-backup ovn-appctl -t ovn-northd pause
+
+# Simulate a SB address set "del and add" notification to ovn-controller in the
+# same IDL iteration. The flows programmed by ovn-controller 

Re: [ovs-dev] [PATCH v4] ovs-ofctl:'--bundle' option can be used with OpenFlow 1.3

[Mike Pattrick]

On Fri, Nov 4, 2022 at 2:24 AM yangchang  wrote:
>
> From the commit 25070e045e, bundle option can be used with OpenFlow 1.3
>
> Signed-off-by: yangchang 

Re-adding an explicit ack. From a quick search, it looks like bundles
weren't included in the original OF1.3 spec, but were included in
EXT-230.

The support exists in ofp-bundle.c for OFP13_VERSION, though, the
error message is a little confusing: "bundles need OpenFlow 1.3 or
later ('-O OpenFlow14')"

Acked-by: Mike Pattrick 

Cheers,
MKP

> ---
>  utilities/ovs-ofctl.8.in | 10 +-
>  utilities/ovs-save   |  6 +++---
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
> index 10a6a64de..953609bfd 100644
> --- a/utilities/ovs-ofctl.8.in
> +++ b/utilities/ovs-ofctl.8.in
> @@ -1315,7 +1315,7 @@ well as cookie values and table IDs if they are zero.
>  Do not execute read/write commands.
>  .
>  .IP "\fB\-\-bundle\fR"
> -Execute flow mods as an OpenFlow 1.4 atomic bundle transaction.
> +Execute flow mods as an OpenFlow 1.3 atomic bundle transaction.
>  .RS
>  .IP \(bu
>  Within a bundle, all flow mods are processed in the order they appear
> @@ -1327,15 +1327,15 @@ the transaction, or after all the flow mods in the 
> bundle have been
>  successfully applied.
>  .IP \(bu
>  The beginning and the end of the flow table modification commands in a
> -bundle are delimited with OpenFlow 1.4 bundle control messages, which
> +bundle are delimited with OpenFlow 1.3 bundle control messages, which
>  makes it possible to stream the included commands without explicit
>  OpenFlow barriers, which are otherwise used after each flow table
>  modification command.  This may make large modifications execute
>  faster as a bundle.
>  .IP \(bu
> -Bundles require OpenFlow 1.4 or higher.  An explicit \fB-O
> -OpenFlow14\fR option is not needed, but you may need to enable
> -OpenFlow 1.4 support for OVS by setting the OVSDB \fIprotocols\fR
> +Bundles require OpenFlow 1.3 or higher.  An explicit \fB-O
> +OpenFlow13\fR option is not needed, but you may need to enable
> +OpenFlow 1.3 support for OVS by setting the OVSDB \fIprotocols\fR
>  column in the \fIbridge\fR table.
>  .RE
>  .
> diff --git a/utilities/ovs-save b/utilities/ovs-save
> index 67092ecf7..2efd82c78 100755
> --- a/utilities/ovs-save
> +++ b/utilities/ovs-save
> @@ -102,7 +102,7 @@ save_interfaces () {
>  get_highest_ofp_version() {
>  ovs-vsctl get bridge "$1" protocols | \
>  sed 's/[][]//g' | sed 's/\ //g' | \
> -awk -F ',' '{ print (NF>0)? $(NF) : "OpenFlow14" }'
> +awk -F ',' '{ print (NF>0)? $(NF) : "OpenFlow13" }'
>  }
>
>  save_flows () {
> @@ -133,8 +133,8 @@ save_flows () {
>   cnt++;printf "{class="$1",type="$2",len="$3"}->"$4}'
>  echo "'"
>
> -# If possible use OpenFlow 1.4 atomic bundle txn for flows and groups
> -[ ${ofp_version#OpenFlow} -ge 14 ] && bundle=" --bundle" || bundle=""
> +# If possible use OpenFlow 1.3 atomic bundle txn for flows and groups
> +[ ${ofp_version#OpenFlow} -ge 13 ] && bundle=" --bundle" || bundle=""
>
>  echo "ovs-ofctl -O $ofp_version add-groups ${bridge} \
>\"$workdir/$bridge.groups.dump\" ${bundle}"
> --
> 2.27.0.windows.1
>
>
> yangch...@chinatelecom.cn
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

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

Re: [ovs-dev] [PATCH ovn v2] lflow-cache: Introduce cache for lflow actions

[Dan Williams]

On Thu, 2023-05-25 at 08:38 -0500, Dan Williams wrote:
> On Wed, 2023-05-24 at 11:12 +0200, Ihtisham ul Haq via dev wrote:
> > Using cache improves performance of recomputation of lflows(by
> > about 30%)
> > 
> > Exising lflow cache for `matches` and `expressions` is adopted
> > to include `actions` as well.
> > 
> > Co-authored-by: Felix Huettner 
> > Signed-off-by: Felix Huettner 
> > Signed-off-by: Ihtisham ul Haq 
> > ---
> >  controller/lflow-cache.c  | 12 --
> >  controller/lflow-cache.h  |  8 +--
> >  controller/lflow.c    | 41 +-
> >  controller/test-lflow-cache.c | 42 +++
> >  tests/ovn-lflow-cache.at  | 41 ++
> >  5 files changed, 115 insertions(+), 29 deletions(-)
> > 
> > diff --git a/controller/lflow-cache.c b/controller/lflow-cache.c
> > index f723eab8a..a648d77af 100644
> > --- a/controller/lflow-cache.c
> > +++ b/controller/lflow-cache.c
> > @@ -25,7 +25,9 @@
> >  #include "lflow-cache.h"
> >  #include "lib/uuid.h"
> >  #include "memory-trim.h"
> > +#include "openvswitch/ofpbuf.h"
> >  #include "openvswitch/vlog.h"
> > +#include "ovn/actions.h"
> >  #include "ovn/expr.h"
> > 
> >  VLOG_DEFINE_THIS_MODULE(lflow_cache);
> > @@ -209,7 +211,8 @@ lflow_cache_get_stats(const struct lflow_cache *lc, 
> > struct ds *output)
> > 
> >  void
> >  lflow_cache_add_expr(struct lflow_cache *lc, const struct uuid *lflow_uuid,
> > - struct expr *expr, size_t expr_sz)
> > + struct expr *expr, size_t expr_sz,
> > + struct ofpbuf *actions)
> >  {
> >  struct lflow_cache_value *lcv =
> >  lflow_cache_add__(lc, lflow_uuid, LCACHE_T_EXPR, expr_sz);
> > @@ -220,12 +223,14 @@ lflow_cache_add_expr(struct lflow_cache *lc, const 
> > struct uuid *lflow_uuid,
> >  }
> >  COVERAGE_INC(lflow_cache_add_expr);
> >  lcv->expr = expr;
> > +    lcv->actions = actions;
> 
> Since the function consumes the action, you need to free & uninit the
> actions in the !lcv case above this chunk, like happens for 'expr'.
> 
> >  }
> > 
> >  void
> >  lflow_cache_add_matches(struct lflow_cache *lc, const struct uuid 
> > *lflow_uuid,
> >  uint32_t conj_id_ofs, uint32_t n_conjs,
> > -    struct hmap *matches, size_t matches_sz)
> > +    struct hmap *matches, size_t matches_sz,
> > +    struct ofpbuf *actions)
> >  {
> >  struct lflow_cache_value *lcv =
> >  lflow_cache_add__(lc, lflow_uuid, LCACHE_T_MATCHES, matches_sz);
> > @@ -239,6 +244,7 @@ lflow_cache_add_matches(struct lflow_cache *lc, const 
> > struct uuid *lflow_uuid,
> >  lcv->expr_matches = matches;
> >  lcv->n_conjs = n_conjs;
> >  lcv->conj_id_ofs = conj_id_ofs;
> > +    lcv->actions = actions;
> 
> Similarly, since the function consumes the action, you need to free &
> uninit the actions in the !lcv case above this chunk, like happens for
> 'matches'.
> 
> >  }
> > 
> >  struct lflow_cache_value *
> > @@ -380,11 +386,13 @@ lflow_cache_delete__(struct lflow_cache *lc, struct 
> > lflow_cache_entry *lce)
> >  case LCACHE_T_EXPR:
> >  COVERAGE_INC(lflow_cache_free_expr);
> >  expr_destroy(lce->value.expr);
> > +    ovnacts_free((*lce->value.actions).data, 
> > (*lce->value.actions).size);
> 
> ovnacts_free(lce->value.actions->data, lce->value.actions->size);
> 
> seems a bit more canonical (eg, actions->data instead of dereferencing 
> actions and using '.')
> 
> >  break;
> >  case LCACHE_T_MATCHES:
> >  COVERAGE_INC(lflow_cache_free_matches);
> >  expr_matches_destroy(lce->value.expr_matches);
> >  free(lce->value.expr_matches);
> > +    ovnacts_free((*lce->value.actions).data, 
> > (*lce->value.actions).size);
> 
> Same here.
> 
> >  break;
> >  }
> > 
> > diff --git a/controller/lflow-cache.h b/controller/lflow-cache.h
> > index 300a56077..9d542beec 100644
> > --- a/controller/lflow-cache.h
> > +++ b/controller/lflow-cache.h
> > @@ -50,6 +50,8 @@ struct lflow_cache_value {
> >  uint32_t n_conjs;
> >  uint32_t conj_id_ofs;
> > 
> > +    struct ofpbuf *actions;
> > +
> >  union {
> >  struct hmap *expr_matches;
> >  struct expr *expr;
> > @@ -66,11 +68,13 @@ bool lflow_cache_is_enabled(const struct lflow_cache *);
> >  void lflow_cache_get_stats(const struct lflow_cache *, struct ds *output);
> > 
> >  void lflow_cache_add_expr(struct lflow_cache *, const struct uuid 
> > *lflow_uuid,
> > -  struct expr *expr, size_t expr_sz);
> > +  struct expr *expr, size_t expr_sz,
> > +  struct ofpbuf *actions);
> >  void lflow_cache_add_matches(struct lflow_cache *,
> >   const struct uuid *lflow_uuid,
> >   uint32_t conj_id_ofs, uint32_t 

Re: [ovs-dev] [PATCH ovn v2] lflow-cache: Introduce cache for lflow actions

[Dan Williams]

On Wed, 2023-05-24 at 11:12 +0200, Ihtisham ul Haq via dev wrote:
> Using cache improves performance of recomputation of lflows(by
> about 30%)
> 
> Exising lflow cache for `matches` and `expressions` is adopted
> to include `actions` as well.
> 
> Co-authored-by: Felix Huettner 
> Signed-off-by: Felix Huettner 
> Signed-off-by: Ihtisham ul Haq 
> ---
>  controller/lflow-cache.c  | 12 --
>  controller/lflow-cache.h  |  8 +--
>  controller/lflow.c    | 41 +-
>  controller/test-lflow-cache.c | 42 +++
>  tests/ovn-lflow-cache.at  | 41 ++
>  5 files changed, 115 insertions(+), 29 deletions(-)
> 
> diff --git a/controller/lflow-cache.c b/controller/lflow-cache.c
> index f723eab8a..a648d77af 100644
> --- a/controller/lflow-cache.c
> +++ b/controller/lflow-cache.c
> @@ -25,7 +25,9 @@
>  #include "lflow-cache.h"
>  #include "lib/uuid.h"
>  #include "memory-trim.h"
> +#include "openvswitch/ofpbuf.h"
>  #include "openvswitch/vlog.h"
> +#include "ovn/actions.h"
>  #include "ovn/expr.h"
> 
>  VLOG_DEFINE_THIS_MODULE(lflow_cache);
> @@ -209,7 +211,8 @@ lflow_cache_get_stats(const struct lflow_cache *lc, 
> struct ds *output)
> 
>  void
>  lflow_cache_add_expr(struct lflow_cache *lc, const struct uuid *lflow_uuid,
> - struct expr *expr, size_t expr_sz)
> + struct expr *expr, size_t expr_sz,
> + struct ofpbuf *actions)
>  {
>  struct lflow_cache_value *lcv =
>  lflow_cache_add__(lc, lflow_uuid, LCACHE_T_EXPR, expr_sz);
> @@ -220,12 +223,14 @@ lflow_cache_add_expr(struct lflow_cache *lc, const 
> struct uuid *lflow_uuid,
>  }
>  COVERAGE_INC(lflow_cache_add_expr);
>  lcv->expr = expr;
> +    lcv->actions = actions;

Since the function consumes the action, you need to free & uninit the
actions in the !lcv case above this chunk, like happens for 'expr'.

>  }
> 
>  void
>  lflow_cache_add_matches(struct lflow_cache *lc, const struct uuid 
> *lflow_uuid,
>  uint32_t conj_id_ofs, uint32_t n_conjs,
> -    struct hmap *matches, size_t matches_sz)
> +    struct hmap *matches, size_t matches_sz,
> +    struct ofpbuf *actions)
>  {
>  struct lflow_cache_value *lcv =
>  lflow_cache_add__(lc, lflow_uuid, LCACHE_T_MATCHES, matches_sz);
> @@ -239,6 +244,7 @@ lflow_cache_add_matches(struct lflow_cache *lc, const 
> struct uuid *lflow_uuid,
>  lcv->expr_matches = matches;
>  lcv->n_conjs = n_conjs;
>  lcv->conj_id_ofs = conj_id_ofs;
> +    lcv->actions = actions;

Similarly, since the function consumes the action, you need to free &
uninit the actions in the !lcv case above this chunk, like happens for
'matches'.

>  }
> 
>  struct lflow_cache_value *
> @@ -380,11 +386,13 @@ lflow_cache_delete__(struct lflow_cache *lc, struct 
> lflow_cache_entry *lce)
>  case LCACHE_T_EXPR:
>  COVERAGE_INC(lflow_cache_free_expr);
>  expr_destroy(lce->value.expr);
> +    ovnacts_free((*lce->value.actions).data, (*lce->value.actions).size);

ovnacts_free(lce->value.actions->data, lce->value.actions->size);

seems a bit more canonical (eg, actions->data instead of dereferencing actions 
and using '.')

>  break;
>  case LCACHE_T_MATCHES:
>  COVERAGE_INC(lflow_cache_free_matches);
>  expr_matches_destroy(lce->value.expr_matches);
>  free(lce->value.expr_matches);
> +    ovnacts_free((*lce->value.actions).data, (*lce->value.actions).size);

Same here.

>  break;
>  }
> 
> diff --git a/controller/lflow-cache.h b/controller/lflow-cache.h
> index 300a56077..9d542beec 100644
> --- a/controller/lflow-cache.h
> +++ b/controller/lflow-cache.h
> @@ -50,6 +50,8 @@ struct lflow_cache_value {
>  uint32_t n_conjs;
>  uint32_t conj_id_ofs;
> 
> +    struct ofpbuf *actions;
> +
>  union {
>  struct hmap *expr_matches;
>  struct expr *expr;
> @@ -66,11 +68,13 @@ bool lflow_cache_is_enabled(const struct lflow_cache *);
>  void lflow_cache_get_stats(const struct lflow_cache *, struct ds *output);
> 
>  void lflow_cache_add_expr(struct lflow_cache *, const struct uuid 
> *lflow_uuid,
> -  struct expr *expr, size_t expr_sz);
> +  struct expr *expr, size_t expr_sz,
> +  struct ofpbuf *actions);
>  void lflow_cache_add_matches(struct lflow_cache *,
>   const struct uuid *lflow_uuid,
>   uint32_t conj_id_ofs, uint32_t n_conjs,
> - struct hmap *matches, size_t matches_sz);
> + struct hmap *matches, size_t matches_sz,
> + struct ofpbuf *actions);
> 
>  struct lflow_cache_value *lflow_cache_get(struct lflow_cache *,
> 

[ovs-dev] [PATCH v12] ofproto-dpif-upcall: Don't set statistics to 0 when they jump back

[Balazs Nemeth]

The only way that stats->{n_packets,n_bytes} would decrease is due to an
overflow, or if there are bugs in how statistics are handled. In the
past, there were multiple issues that caused a jump backward. A
workaround was in place to set the statistics to 0 in that case. When
this happened while the revalidator was under heavy load, the workaround
had an unintended side effect where should_revalidate returned false
causing the flow to be removed because the metric it calculated was
based on a bogus value. Since many of those bugs have now been
identified and resolved, there is no need to set the statistics to 0. In
addition, the (unlikely) overflow still needs to be handled
appropriately. If an unexpected jump does happen, just log it as a
warning.

Signed-off-by: Balazs Nemeth 
---
 ofproto/ofproto-dpif-upcall.c | 29 +++--
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index cd57fdbd9..4c34d695e 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -2339,6 +2339,26 @@ exit:
 return result;
 }
 
+static void
+log_unexpected_stats_jump(struct udpif_key *ukey,
+  const struct dpif_flow_stats *stats)
+{
+static struct vlog_rate_limit rll = VLOG_RATE_LIMIT_INIT(1, 5);
+struct ds ds = DS_EMPTY_INITIALIZER;
+struct ofpbuf *actions;
+
+odp_format_ufid(>ufid, );
+ds_put_cstr(, " ");
+odp_flow_key_format(ukey->key, ukey->key_len, );
+ds_put_cstr(, ", actions:");
+actions = ovsrcu_get(struct ofpbuf *, >actions);
+format_odp_actions(, actions->data, actions->size, NULL);
+VLOG_WARN_RL(, "Unexpected jump in packet stats from %"PRIu64
+" to %"PRIu64" when handling ukey %s",
+stats->n_packets, ukey->stats.n_packets, ds_cstr());
+ds_destroy();
+}
+
 /* Verifies that the datapath actions of 'ukey' are still correct, and pushes
  * 'stats' for it.
  *
@@ -2372,18 +2392,15 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key 
*ukey,
 
 push.used = stats->used;
 push.tcp_flags = stats->tcp_flags;
-push.n_packets = (stats->n_packets > ukey->stats.n_packets
-  ? stats->n_packets - ukey->stats.n_packets
-  : 0);
-push.n_bytes = (stats->n_bytes > ukey->stats.n_bytes
-? stats->n_bytes - ukey->stats.n_bytes
-: 0);
+push.n_packets = stats->n_packets - ukey->stats.n_packets;
+push.n_bytes = stats->n_bytes - ukey->stats.n_bytes;
 
 if (stats->n_packets < ukey->stats.n_packets &&
 ukey->stats.n_packets < UINT64_THREE_QUARTERS) {
 /* Report cases where the packet counter is lower than the previous
  * instance, but exclude the potential wrapping of an uint64_t. */
 COVERAGE_INC(ukey_invalid_stat_reset);
+log_unexpected_stats_jump(ukey, stats);
 }
 
 if (need_revalidate) {
-- 
2.40.1

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

Re: [ovs-dev] [PATCH ovn v2] lflow-cache: Introduce cache for lflow actions

[Simon Horman]

On Wed, May 24, 2023 at 11:12:28AM +0200, Ihtisham ul Haq via dev wrote:
> Using cache improves performance of recomputation of lflows(by
> about 30%)
> 
> Exising lflow cache for `matches` and `expressions` is adopted
> to include `actions` as well.
> 
> Co-authored-by: Felix Huettner 
> Signed-off-by: Felix Huettner 
> Signed-off-by: Ihtisham ul Haq 

...

> diff --git a/controller/lflow.c b/controller/lflow.c
> index 0b071138d..5ca676ee8 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -1078,14 +1078,23 @@ consider_logical_flow__(const struct 
> sbrec_logical_flow *lflow,
>  struct sset template_vars_ref = SSET_INITIALIZER(_vars_ref);
>  struct expr *prereqs = NULL;
> 
> -if (!lflow_parse_actions(lflow, l_ctx_in, _vars_ref,
> - , )) {
> -ovnacts_free(ovnacts.data, ovnacts.size);
> -ofpbuf_uninit();
> -store_lflow_template_refs(l_ctx_out->lflow_deps_mgr,
> -  _vars_ref, lflow);
> -sset_destroy(_vars_ref);
> -return;
> +struct lflow_cache_value *lcv =
> +lflow_cache_get(l_ctx_out->lflow_cache, >header_.uuid);
> +enum lflow_cache_type lcv_type =
> +lcv ? lcv->type : LCACHE_T_NONE;
> +
> +if (lcv_type != LCACHE_T_NONE) {
> +ovnacts = *ofpbuf_clone(lcv->actions);

Hi,

This is being flagged as a memory leak by LeakSanitiser.

Link: 
https://github.com/ovsrobot/ovn/actions/runs/5066318567/jobs/9095972982#step:6:4817

> +} else {
> +if (!lflow_parse_actions(lflow, l_ctx_in, _vars_ref,
> + , )) {
> +ovnacts_free(ovnacts.data, ovnacts.size);
> +ofpbuf_uninit();
> +store_lflow_template_refs(l_ctx_out->lflow_deps_mgr,
> +_vars_ref, lflow);
> +sset_destroy(_vars_ref);
> +return;
> +}
>  }
> 
>  struct lookup_port_aux aux = {
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] stream-ssl: Disable alerts on unexpected EOF.

[Simon Horman]

On Wed, May 17, 2023 at 06:51:04PM +0200, Ilya Maximets wrote:
> OpenSSL 3.0 enabled alerts for unexpected EOF by default.  It supposed
> to alert the application whenever the connection terminated without
> a proper close_notify.  And that should allow applications to take
> actions to protect themselves from potential TLS truncation attack.
> This is how it looks like in the log:
> 
>  |stream_ssl|WARN|SSL_read: error:0A000126:SSL routines::unexpected eof while 
> reading
>  |jsonrpc|WARN|ssl:127.0.0.1:34288: receive error: Input/output error
>  |reconnect|WARN|ssl:127.0.0.1:34288: connection dropped (Input/output error)
> 
> The problem is that clients based on OVS libraries do not wait for
> the proper termination if it didn't happen right away.  It means that
> chances to have alerts on the server side for every single disconnection
> are very high.
> 
> None of the high level protocols supported by OVS daemons can carry
> state between re-connections, e.g., there are no session cookies or
> anything like that.  So, the TLS truncation attack is no applicable.
> 
> Disable the alert to avoid unnecessary warnings in the log.
> 
> Signed-off-by: Ilya Maximets 

Reviewed-by: Simon Horman 

Are there any plans to enhance the client-side behaviour?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] tests: Check ovsdb-server logs in OVSDB tests.

[Simon Horman]

On Wed, May 17, 2023 at 06:51:05PM +0200, Ilya Maximets wrote:
> Many OVSDB tests are not checking the server log for warnings or
> errors.  Some are not even using the log file.  It's mostly OK as we're
> usually checking the user-visible behavior.  But it would also be nice
> to detect some internal warnings if there are some.
> 
> Moving the OVSDB_SERVER_SHUTDOWN macro to the common place, adding
> the call to check_logs into it and making OVSDB tests use this macro.
> 
> Signed-off-by: Ilya Maximets 

Reviewed-by: Simon Horman 

As an aside.
Some of the lines in the test suite are excessively long.

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

[ovs-dev] [PATCH v2] tests: layer3-tunnels: Skip bareudp tests if not supported by kernel.

[Frode Nordahl]

The bareudp tests depend on specific kernel configuration to
succeed.  Skip the test if the feature is not enabled in the
running kernel.

Signed-off-by: Frode Nordahl 
---
 tests/system-kmod-macros.at  | 10 ++
 tests/system-layer3-tunnels.at   |  4 ++--
 tests/system-userspace-macros.at |  8 
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at
index fb15a5a7c..712925ded 100644
--- a/tests/system-kmod-macros.at
+++ b/tests/system-kmod-macros.at
@@ -237,3 +237,13 @@ m4_define([CHECK_L3L4_CONNTRACK_REASM])
 #
 # The kernel module tests do not use TC offload.
 m4_define([CHECK_NO_TC_OFFLOAD])
+
+# OVS_CHECK_BAREUDP()
+#
+# The feature needs to be enabled in the kernel configuration (CONFIG_BAREUDP)
+# to work.
+m4_define([OVS_CHECK_BAREUDP],
+[
+AT_SKIP_IF([! ip link add dev ovs_bareudp0 type bareudp dstport 6635 
ethertype mpls_uc 2>&1 >/dev/null])
+AT_CHECK([ip link del dev ovs_bareudp0])
+])
diff --git a/tests/system-layer3-tunnels.at b/tests/system-layer3-tunnels.at
index c37852b21..81123f730 100644
--- a/tests/system-layer3-tunnels.at
+++ b/tests/system-layer3-tunnels.at
@@ -154,7 +154,7 @@ OVS_VSWITCHD_STOP
 AT_CLEANUP
 
 AT_SETUP([layer3 - ping over MPLS Bareudp])
-OVS_CHECK_MIN_KERNEL(5, 7)
+OVS_CHECK_BAREUDP()
 OVS_TRAFFIC_VSWITCHD_START([_ADD_BR([br1])])
 ADD_NAMESPACES(at_ns0, at_ns1)
 
@@ -202,7 +202,7 @@ OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
 
 AT_SETUP([layer3 - ping over Bareudp])
-OVS_CHECK_MIN_KERNEL(5, 7)
+OVS_CHECK_BAREUDP()
 OVS_TRAFFIC_VSWITCHD_START([_ADD_BR([br1])])
 ADD_NAMESPACES(at_ns0, at_ns1)
 
diff --git a/tests/system-userspace-macros.at b/tests/system-userspace-macros.at
index 482079386..c1855cbc5 100644
--- a/tests/system-userspace-macros.at
+++ b/tests/system-userspace-macros.at
@@ -336,3 +336,11 @@ m4_define([CHECK_L3L4_CONNTRACK_REASM],
 #
 # Userspace tests do not use TC offload.
 m4_define([CHECK_NO_TC_OFFLOAD])
+
+# OVS_CHECK_BAREUDP()
+#
+# The userspace datapath does not support bareudp tunnels.
+m4_define([OVS_CHECK_BAREUDP],
+[
+AT_SKIP_IF([:])
+])
-- 
2.39.2

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

Re: [ovs-dev] [PATCH] ovsdb: condition: Process condition changes incrementally.

[Simon Horman]

On Thu, May 18, 2023 at 02:14:25PM +0200, Ilya Maximets wrote:
> In most cases, after the condition change request, the new condition
> is the same as old one plus minus a few clauses.  Today, ovsdb-server
> will evaluate every database row against all the old clauses and then
> against all the new clauses in order to tell if an update should be
> generated.
> 
> For example, every time a new port is added, ovn-controller adds two
> new clauses to conditions for a Port_Binding table.  And this condition
> may grow significantly in size making addition of every new port
> heavier on the server side.
> 
> The difference between conditions is not larger and, likely,
> significantly smaller than old and new conditions combined.  And if
> the row doesn't match clauses that are different between old and new
> conditions, that row should not be part of the update. It either
> matches both old and new, or it doesn't match either of them.
> 
> If the row matches some clauses in the difference, then we need to
> perform a full match against old and new in order to tell if it
> should be added/removed/modified.  This is necessary because different
> clauses may select same rows.
> 
> Let's generate the condition difference and use it to avoid evaluation
> of all the clauses for rows not affected by the condition change.
> 
> Testing shows 70% reduction in total CPU time in ovn-heater's 120-node
> density-light test with conditional monitoring.  Average CPU usage
> during the test phase went down from frequent 100% spikes to just 6-8%.
> 
> Note: This will not help with new connections, or re-connections,
> or new monitor requests after database conversion.  ovsdb-server will
> still evaluate every database row against every clause in the condition
> in these cases.  So, it's still important to not have too many clauses
> in conditions for large tables.
> 
> Signed-off-by: Ilya Maximets 
> ---
>  ovsdb/condition.c | 57 +
>  ovsdb/condition.h |  9 ++
>  ovsdb/monitor.c   | 71 ---
>  3 files changed, 115 insertions(+), 22 deletions(-)
> 
> diff --git a/ovsdb/condition.c b/ovsdb/condition.c
> index d0016fa7f..c2d5363f5 100644
> --- a/ovsdb/condition.c
> +++ b/ovsdb/condition.c
> @@ -497,6 +497,63 @@ ovsdb_condition_cmp_3way(const struct ovsdb_condition *a,
>  return 0;
>  }
>  
> +

nit: one blank line seems enough?

> +/* Given conditions 'a' and 'b', composes a new condition 'diff' that 
> contains
> + * clauses that are present in one of the conditions, but not in the other.
> + *
> + * If some data doesn't match the resulted 'diff' condition, that means one 
> of:
> + *  1. The data matches both 'a' and 'b'.
> + *  2. The data does not match either 'a' or 'b'.
> + *
> + * However, that is not true if one of the original conditions is a trivial
> + * True or False.  In this case the function will currently just return an
> + * empty (True) condition. */
> +void
> +ovsdb_condition_diff(struct ovsdb_condition *diff,
> + const struct ovsdb_condition *a,
> + const struct ovsdb_condition *b)
> +{
> +size_t i, j;
> +int cmp;
> +
> +ovsdb_condition_init(diff);
> +
> +if (ovsdb_condition_is_trivial(a) || ovsdb_condition_is_trivial(b)) {
> +return;
> +}
> +
> +diff->clauses = xzalloc((a->n_clauses + b->n_clauses)
> +* sizeof *diff->clauses);

nit: maybe xcalloc()

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

Re: [ovs-dev] [PATCH 3/3] netdev-vport: RCU-fy tunnel config.

[Simon Horman]

On Sat, May 20, 2023 at 02:31:20AM +0200, Ilya Maximets wrote:
> Tunnel config can be accessed by multiple threads at the same time and
> it is supposed to be protected by the netdev_vport mutex.  However,
> many functions are getting direct access to it via netdev API without
> taking the mutex, creating a potential for various race conditions.
> 
> Fix that by protecting the tunnel config with RCU.  The whole structure
> is replaced on configuration changes.  Individual fields are never
> updated and the structure itself is constant.  This way it can be safely
> used by different threads within RCU grace period.
> 
> Signed-off-by: Ilya Maximets 

Reviewed-by: Simon Horman 

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

Re: [ovs-dev] [PATCH 2/3] smap: Make argument of smap_add_ipv6 constant.

[Simon Horman]

On Sat, May 20, 2023 at 02:31:19AM +0200, Ilya Maximets wrote:
> The address is not getting modified inside.
> 
> Signed-off-by: Ilya Maximets 

Reviewed-by: Simon Horman 

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

Re: [ovs-dev] [PATCH 1/3] netdev-vport: Fix unsafe handling of GRE sequence number.

[Simon Horman]

On Sat, May 20, 2023 at 02:31:18AM +0200, Ilya Maximets wrote:
> GRE sequence number is maintained as part of the tunnel config.
> This triggers tunnel reconfiguration every time set_tunnel_config()
> is called, because memset over tunnel config will never be equal to
> the new config constructed from database options.
> 
> And sequence number incremented non-atomically without holding a
> mutex on tunnel push, that may lead to corruption if multiple
> threads are sending packets to the same tunnel.
> 
> Fix that by moving sequence number to the netdev_vport structure
> instead and using an atomic counter.
> 
> Fixes: 04975308 ("userspace: add gre sequence number support.")
> Fixes: 7dc18ae96d33 ("userspace: add erspan tunnel support.")
> Fixes: 3c6d05a02e0f ("userspace: Add GTP-U support.")
> Signed-off-by: Ilya Maximets 

Reviewed-by: Simon Horman 

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

Re: [ovs-dev] [PATCH] tests: layer3-tunnels: Skip bareudp tests if not supported by kernel.

[Ilya Maximets]

On 5/25/23 09:19, Simon Horman wrote:
> On Wed, May 24, 2023 at 09:23:59PM +0200, Ilya Maximets wrote:
>> On 5/24/23 15:39, Frode Nordahl wrote:
>>> The bareudp tests depend on specific kernel configuration to
>>> succeed.  Skip the test if the feature is not enabled in the
>>> running kernel.
>>>
>>> Signed-off-by: Frode Nordahl 
>>> ---
>>>  tests/system-kmod-macros.at  | 10 ++
>>>  tests/system-layer3-tunnels.at   |  2 ++
>>>  tests/system-userspace-macros.at |  8 
>>>  3 files changed, 20 insertions(+)
>>>
>>> diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at
>>> index fb15a5a7c..55e7821ce 100644
>>> --- a/tests/system-kmod-macros.at
>>> +++ b/tests/system-kmod-macros.at
>>> @@ -237,3 +237,13 @@ m4_define([CHECK_L3L4_CONNTRACK_REASM])
>>>  #
>>>  # The kernel module tests do not use TC offload.
>>>  m4_define([CHECK_NO_TC_OFFLOAD])
>>> +
>>> +# OVS_CHECK_BAREUDP()
>>> +#
>>> +# The feature needs to be enabled in the kernel configuration 
>>> (CONFIG_BAREUDP)
>>> +# to work.
>>> +m4_define([OVS_CHECK_BAREUDP],
>>> +[
>>> +AT_SKIP_IF([! ip link add dev bareudp0 type bareudp dstport 6635 
>>> ethertype mpls_uc 2>&1 >/dev/null])
>>> +AT_CHECK([ip link del dev bareudp0])
>>
>> Maybe call it ovs_bareudp0 or something like that?  Just to decrease chances
>> for random collisions.
>>
>>> +])
>>> diff --git a/tests/system-layer3-tunnels.at b/tests/system-layer3-tunnels.at
>>> index c37852b21..5546bc879 100644
>>> --- a/tests/system-layer3-tunnels.at
>>> +++ b/tests/system-layer3-tunnels.at
>>> @@ -155,6 +155,7 @@ AT_CLEANUP
>>>  
>>>  AT_SETUP([layer3 - ping over MPLS Bareudp])
>>>  OVS_CHECK_MIN_KERNEL(5, 7)
>>> +OVS_CHECK_BAREUDP()
>>
>> This new check supersedes the kernel version check.  Should we remove it?
>> The original idea was that we can create bareudp tunnels even if the ip 
>> utility
>> didn't support them at a time.  But, I suppose, enough time passed since then
>> and the ip utility available in distributions caught up, so we can just check
>> it instead.
> 
> I'd say that kernel checks are unreliable in the presence of backports.
> Whereas tool checks suffer the problem you describe.
> At this point I'd lean towards a tool-based check.
> As you say, some time has passed by now.

Yes, I agree.  We may remove the kernel version check though
as it is not necessary in the presence of a tool-based check.

> 
> FWIIW, I believe we've always relied on a tool based check for PPS rate
> limiting checks, which are similar. Perhaps unwisely. But in practice the
> problems I'm aware of there is with the implementation of the check
> (hopefully fixed by now) not the tool vs kernel issue.
> 
>>>  OVS_TRAFFIC_VSWITCHD_START([_ADD_BR([br1])])
>>>  ADD_NAMESPACES(at_ns0, at_ns1)
>>>  
>>> @@ -203,6 +204,7 @@ AT_CLEANUP
>>>  
>>>  AT_SETUP([layer3 - ping over Bareudp])
>>>  OVS_CHECK_MIN_KERNEL(5, 7)
>>
>> Same here.
>>
>>> +OVS_CHECK_BAREUDP()
>>>  OVS_TRAFFIC_VSWITCHD_START([_ADD_BR([br1])])
>>>  ADD_NAMESPACES(at_ns0, at_ns1)
>>>  
>>> diff --git a/tests/system-userspace-macros.at 
>>> b/tests/system-userspace-macros.at
>>> index 482079386..1cb67d6f6 100644
>>> --- a/tests/system-userspace-macros.at
>>> +++ b/tests/system-userspace-macros.at
>>> @@ -336,3 +336,11 @@ m4_define([CHECK_L3L4_CONNTRACK_REASM],
>>>  #
>>>  # Userspace tests do not use TC offload.
>>>  m4_define([CHECK_NO_TC_OFFLOAD])
>>> +
>>> +# OVS_CHECK_BAREUDP()
>>> +#
>>> +# The userspace skips all tests that check kernel configuration.
>>
>> This should probably say that userspace datapath doesn't support bareudp 
>> tunnels
>> instead.
>>
>>> +m4_define([OVS_CHECK_BAREUDP],
>>> +[
>>> +AT_SKIP_IF([:])
>>> +])
>>

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

Re: [ovs-dev] [PATCH ovn] controller: Handle OpenFlow errors.

[Ales Musil]

On Thu, May 25, 2023 at 10:37 AM Dumitru Ceara  wrote:

> Whenever an OpenFlow error is returned by OvS, trigger a reconnect of
> the OpenFlow (rconn) connection.  This will clear any installed OpenFlow
> rules/groups. To ensure consistency, trigger a full I-P recompute too.
>
> An example of scenario that can result in an OpenFlow error returned by
> OvS follows (describing two main loop iterations in ovn-controller):
>   - Iteration I:
> a. get updates from SB
> b. process these updates and generate "desired" openflows (lets assume
> this generates quite a lot of desired openflow modifications)
> c.1. add bundle-open msg to rconn
> c.2. add openflow mod msgs to rconn (only some of these make it
> through,
> the rest gets queued, the rconn is backlogged at this point).
> c.3. add bundle-commit msg to rconn (this gets queued)
>
>   - Iteration II:
> a. get updates from SB (rconn is still backlogged)
> b. process the updates and generate "desired" openflows (lets assume
> this takes 10+ seconds for the specific SB updates)
>
> At some point, while step II.b was being executed OvS declared the bundle
> operation (started at I.c.1) timeout.  We now act on this error by
> reconnecting which in turn triggers a flush of the rconn backlog and
> gives more chance to the next full recompute to succeed in installing
> all flows.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2134880
> Reported-by: François Rigault 
> CC: Ilya Maximets 
> Signed-off-by: Dumitru Ceara 
>

Hi Dumitru,

sorry for being nitpicky, please see my comment below.


> ---
>  controller/ofctrl.c | 17 ++---
>  controller/ofctrl.h |  2 +-
>  controller/ovn-controller.c | 10 --
>  3 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index b1ba1c743a..1da23bc27e 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -766,13 +766,18 @@ ofctrl_get_mf_field_id(void)
>
>  /* Runs the OpenFlow state machine against 'br_int', which is local to the
>   * hypervisor on which we are running.  Attempts to negotiate a Geneve
> option
> - * field for class OVN_GENEVE_CLASS, type OVN_GENEVE_TYPE. */
> -void
> + * field for class OVN_GENEVE_CLASS, type OVN_GENEVE_TYPE.
> + *
> + * Returns 'true' if an OpenFlow reconnect happened; 'false' otherwise.
> + */
> +bool
>  ofctrl_run(const struct ovsrec_bridge *br_int,
> const struct ovsrec_open_vswitch_table *ovs_table,
> struct shash *pending_ct_zones)
>  {
>  char *target = xasprintf("unix:%s/%s.mgmt", ovs_rundir(),
> br_int->name);
> +bool reconnected = false;
> +
>  if (strcmp(target, rconn_get_target(swconn))) {
>  VLOG_INFO("%s: connecting to switch", target);
>  rconn_connect(swconn, target, target);
> @@ -782,10 +787,12 @@ ofctrl_run(const struct ovsrec_bridge *br_int,
>  rconn_run(swconn);
>
>  if (!rconn_is_connected(swconn)) {
> -return;
> +goto done;
>

We don't have to introduce the "done" label. AFIACT you can just return
the "reconnected" right here with the same effect.


>  }
> +
>  if (seqno != rconn_get_connection_seqno(swconn)) {
>  seqno = rconn_get_connection_seqno(swconn);
> +reconnected = true;
>  state = S_NEW;
>
>  /* Reset the state of any outstanding ct flushes to resend them.
> */
> @@ -855,6 +862,9 @@ ofctrl_run(const struct ovsrec_bridge *br_int,
>   * point, so ensure that we come back again without waiting. */
>  poll_immediate_wake();
>  }
> +
> +done:
> +return reconnected;
>  }
>
>  void
> @@ -909,6 +919,7 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype
> type)
>  } else if (type == OFPTYPE_ERROR) {
>  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(30, 300);
>  log_openflow_rl(, VLL_INFO, oh, "OpenFlow error");
> +rconn_reconnect(swconn);
>  } else {
>  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(30, 300);
>  log_openflow_rl(, VLL_DBG, oh, "OpenFlow packet ignored");
> diff --git a/controller/ofctrl.h b/controller/ofctrl.h
> index f5751e3ee4..105f9370be 100644
> --- a/controller/ofctrl.h
> +++ b/controller/ofctrl.h
> @@ -51,7 +51,7 @@ struct ovn_desired_flow_table {
>  void ofctrl_init(struct ovn_extend_table *group_table,
>   struct ovn_extend_table *meter_table,
>   int inactivity_probe_interval);
> -void ofctrl_run(const struct ovsrec_bridge *br_int,
> +bool ofctrl_run(const struct ovsrec_bridge *br_int,
>  const struct ovsrec_open_vswitch_table *,
>  struct shash *pending_ct_zones);
>  enum mf_field_id ofctrl_get_mf_field_id(void);
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 1151d36644..b301c50157 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -5075,8 +5075,14 @@ 

Re: [ovs-dev] [PATCH ovn] controller: Handle OpenFlow errors.

[Simon Horman]

On Thu, May 25, 2023 at 10:37:23AM +0200, Dumitru Ceara wrote:
> Whenever an OpenFlow error is returned by OvS, trigger a reconnect of
> the OpenFlow (rconn) connection.  This will clear any installed OpenFlow
> rules/groups. To ensure consistency, trigger a full I-P recompute too.
> 
> An example of scenario that can result in an OpenFlow error returned by
> OvS follows (describing two main loop iterations in ovn-controller):
>   - Iteration I:
> a. get updates from SB
> b. process these updates and generate "desired" openflows (lets assume
> this generates quite a lot of desired openflow modifications)
> c.1. add bundle-open msg to rconn
> c.2. add openflow mod msgs to rconn (only some of these make it through,
> the rest gets queued, the rconn is backlogged at this point).
> c.3. add bundle-commit msg to rconn (this gets queued)
> 
>   - Iteration II:
> a. get updates from SB (rconn is still backlogged)
> b. process the updates and generate "desired" openflows (lets assume
> this takes 10+ seconds for the specific SB updates)
> 
> At some point, while step II.b was being executed OvS declared the bundle
> operation (started at I.c.1) timeout.  We now act on this error by
> reconnecting which in turn triggers a flush of the rconn backlog and
> gives more chance to the next full recompute to succeed in installing
> all flows.
> 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2134880
> Reported-by: François Rigault 
> CC: Ilya Maximets 
> Signed-off-by: Dumitru Ceara 

Reviewed-by: Simon Horman 

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

Re: [ovs-dev] [PATCH] userspace: modify the width of tpa and spa

[Simon Horman]

On Thu, May 25, 2023 at 05:54:37PM +0800, yangchang wrote:
> Arp_spa and arp_tpa are IP addresses, their width should be 32 bits.
> 
> Signed-off-by: yangchang 

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

Re: [ovs-dev] [PATCH v11] ofproto-dpif-upcall: Don't set statistics to 0 when they jump back

[Simon Horman]

On Thu, May 25, 2023 at 10:58:36AM +0200, Balazs Nemeth wrote:
> The only way that stats->{n_packets,n_bytes} would decrease is due to an
> overflow, or if there are bugs in how statistics are handled. In the
> past, there were multiple issues that caused a jump backward. A
> workaround was in place to set the statistics to 0 in that case. When
> this happened while the revalidator was under heavy load, the workaround
> had an unintended side effect where should_revalidate returned false
> causing the flow to be removed because the metric it calculated was
> based on a bogus value. Since many of those bugs have now been
> identified and resolved, there is no need to set the statistics to 0. In
> addition, the (unlikely) overflow still needs to be handled
> appropriately. If an unexpected jump does happen, just log it as a
> warning.
> 
> Signed-off-by: Balazs Nemeth 
> ---
>  ofproto/ofproto-dpif-upcall.c | 29 +++--
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index cd57fdbd9..4c34d695e 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -2339,6 +2339,26 @@ exit:
>  return result;
>  }
>  
> +static void
> +log_unexpected_stats_jump(struct udpif_key *ukey,
> +  const struct dpif_flow_stats *stats)
> +{
> +static struct vlog_rate_limit rll = VLOG_RATE_LIMIT_INIT(1, 5);
> +struct ds ds = DS_EMPTY_INITIALIZER;
> +struct ofpbuf *actions;
> +
> +odp_format_ufid(>ufid, );
> +ds_put_cstr(, " ");
> +odp_flow_key_format(ukey->key, ukey->key_len, );
> +ds_put_cstr(, ", actions:");
> +actions = ovsrcu_get(struct ofpbuf *, >actions);
> +format_odp_actions(, actions->data, actions->size, NULL);
> +VLOG_WARN_RL(, "Unexpected jump in packet stats from %"PRIu64
> +" to %"PRIu64" when handling ukey %s",
> +stats->n_packets, ukey->stats.n_packets, ds_cstr());

Hi Balazs,

Clang seems a bit upset about this bit:

  ../../ofproto/ofproto-dpif-upcall.c:2358:37: error: reading variable 'stats' 
requires holding any mutex [-Werror,-Wthread-safety-analysis]
 stats->n_packets, ukey->stats.n_packets, ds_cstr());
 ^
Link: 
https://github.com/ovsrobot/ovs/actions/runs/5078235117/jobs/9122656937#step:11:2833
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v4 2/2] netdev-dpdk: Check rx/tx descriptor sizes for device.

[Kevin Traynor]

On 24/05/2023 19:15, Ilya Maximets wrote:

On 5/16/23 13:59, Kevin Traynor wrote:

By default OVS configures 2048 descriptors for tx and rx queues
on DPDK devices. It also allows the user to configure those values.

If the values used are not acceptable to the device then queue setup
would fail.

The device exposes it's max/min/alignment requirements and OVS
applies some limits also. Use these to ensure an acceptable value
is used for the number of descriptors on a device tx/rx.

If the default or user value is not acceptable, adjust to a suitable
value and log.

Reported-at: https://bugzilla.redhat.com/2119876
Signed-off-by: Kevin Traynor 
---
  lib/netdev-dpdk.c | 61 ++-
  1 file changed, 50 insertions(+), 11 deletions(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 2d9afc493..e5fc1aa30 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1911,8 +1911,30 @@ dpdk_set_rxq_config(struct netdev_dpdk *dev, const 
struct smap *args)
  static void
  dpdk_process_queue_size(struct netdev *netdev, const struct smap *args,
-const char *flag, int default_size, int *new_size)
+struct rte_eth_dev_info *info, bool is_rx)
  {
-int queue_size = smap_get_int(args, flag, default_size);
+struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
+struct rte_eth_desc_lim *lim;
+int default_size, queue_size, cur_size, new_requested_size;
+int *cur_requested_size;
+bool reconfig = false;
  
+if (is_rx) {

+default_size = NIC_PORT_DEFAULT_RXQ_SIZE;
+new_requested_size = smap_get_int(args, "n_rxq_desc", default_size);
+cur_size = dev->rxq_size;
+cur_requested_size = >requested_rxq_size;
+lim = info ? >rx_desc_lim : NULL;
+


The empty line here seems unnecessary.



yep, I think I removed an assignment at some stage and forgot to remove 
the blank line.



+} else {
+default_size = NIC_PORT_DEFAULT_TXQ_SIZE;
+new_requested_size = smap_get_int(args, "n_txq_desc", default_size);
+cur_size = dev->txq_size;
+cur_requested_size = >requested_txq_size;
+lim = info ? >tx_desc_lim : NULL;
+}
+
+queue_size = new_requested_size;
+
+/* Check for OVS limits. */
  if (queue_size <= 0 || queue_size > NIC_PORT_MAX_Q_SIZE
  || !is_pow2(queue_size)) {
@@ -1920,7 +1942,24 @@ dpdk_process_queue_size(struct netdev *netdev, const 
struct smap *args,
  }
  
-if (queue_size != *new_size) {

-*new_size = queue_size;
+if (lim) {
+/* Check for device limits. */
+if (lim->nb_align) {
+queue_size = ROUND_UP(queue_size, lim->nb_align);
+}
+queue_size = MIN(queue_size, lim->nb_max);
+queue_size = MAX(queue_size, lim->nb_min);
+}
+
+*cur_requested_size = queue_size;
+
+if (cur_size != queue_size) {
  netdev_request_reconfigure(netdev);
+reconfig = true;
+}
+if (new_requested_size != queue_size) {
+VLOG(reconfig ? VLL_INFO : VLL_DBG,
+ "Interface %s cannot set %s descriptor size to %d. "
+ "Adjusted to %d.", dev->up.name, is_rx ? "rx": "tx" ,
+ new_requested_size, queue_size);


This message is a bit misleading.  It says that it can't set the
descriptor size.  But it's not the descriptor size, it's a number
of descriptors.  Or a descriptor ring size, but that's too much
of the unnecessary terminology.
And we should use netdev_get_name instead of accessing the name
directly.

How about this:

 VLOG(reconfig ? VLL_INFO : VLL_DBG,
  "%s: Unable to set the number of %s descriptors to %d. "
  "Adjusted to %d.", netdev_get_name(netdev),
  is_rx ? "rx": "tx", new_requested_size, queue_size);

?



That reads better, thanks.


  }
  }
@@ -1938,7 +1977,9 @@ netdev_dpdk_set_config(struct netdev *netdev, const 
struct smap *args,
  {RTE_ETH_FC_RX_PAUSE, RTE_ETH_FC_FULL}
  };
+struct rte_eth_dev_info info;
  const char *new_devargs;
  const char *vf_mac;
  int err = 0;
+int ret;
  
  ovs_mutex_lock(_mutex);

@@ -1947,11 +1988,4 @@ netdev_dpdk_set_config(struct netdev *netdev, const 
struct smap *args,
  dpdk_set_rxq_config(dev, args);
  
-dpdk_process_queue_size(netdev, args, "n_rxq_desc",

-NIC_PORT_DEFAULT_RXQ_SIZE,
->requested_rxq_size);
-dpdk_process_queue_size(netdev, args, "n_txq_desc",
-NIC_PORT_DEFAULT_TXQ_SIZE,
->requested_txq_size);
-
  new_devargs = smap_get(args, "dpdk-devargs");
  
@@ -2073,4 +2107,9 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,

  }
  
+ret = rte_eth_dev_info_get(dev->port_id, );

+
+dpdk_process_queue_size(netdev, args, !ret ?  : NULL, true);
+dpdk_process_queue_size(netdev, args, !ret ?  : 

[ovs-dev] [PATCH] userspace: modify the width of tpa and spa

[yangchang]

Arp_spa and arp_tpa are IP addresses, their width should be 32 bits.

Signed-off-by: yangchang 
---
 lib/meta-flow.xml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/meta-flow.xml b/lib/meta-flow.xml
index a1a2036..bdd12f6 100644
--- a/lib/meta-flow.xml
+++ b/lib/meta-flow.xml
@@ -4312,9 +4312,9 @@ r r c c c.
 
 
 
-
+
 
-
+
   
 

--
1.8.3.1



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

Re: [ovs-dev] [PATCH v10] netdev-dpdk: add control plane protection support

[Robin Jarry]

Hey Ilya,

Ilya Maximets, May 24, 2023 at 17:05:
> I had a '+' because rss and lacp are two different entities and I looked
> at it as a mode of operation. i.e. RSS plus special handling for LACP.
> RSS looks strange in a comma-separated list, IMO.

For now, there is only LACP but if other protocols are added (e.g. BFD),
wouldn't it be weird to have them separated as well?

options:rx-steering=rss+lacp+bfd

Since lacp and bfd will most likely be put in the additional rxq, it
would make sense to identify them as a group.

I also have other reserves about specifying rss here after thinking some
more about it:

- rss shouldn't be disabled anyway, this forces users to always specify
  it. This is not great from a usability point of view.

- When there is a single rxq configured by the user, there is no RSS
  happening per-se since all other traffic will be put in a single
  queue. The additional rxq being reserved for lacp and/or other special
  traffic.

What do you think about removing "rss" altogether from the items?

options:rx-steering=lacp,bfd,...

Cheers.

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

[ovs-dev] [PATCH v11] ofproto-dpif-upcall: Don't set statistics to 0 when they jump back

[Balazs Nemeth]

The only way that stats->{n_packets,n_bytes} would decrease is due to an
overflow, or if there are bugs in how statistics are handled. In the
past, there were multiple issues that caused a jump backward. A
workaround was in place to set the statistics to 0 in that case. When
this happened while the revalidator was under heavy load, the workaround
had an unintended side effect where should_revalidate returned false
causing the flow to be removed because the metric it calculated was
based on a bogus value. Since many of those bugs have now been
identified and resolved, there is no need to set the statistics to 0. In
addition, the (unlikely) overflow still needs to be handled
appropriately. If an unexpected jump does happen, just log it as a
warning.

Signed-off-by: Balazs Nemeth 
---
 ofproto/ofproto-dpif-upcall.c | 29 +++--
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index cd57fdbd9..4c34d695e 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -2339,6 +2339,26 @@ exit:
 return result;
 }
 
+static void
+log_unexpected_stats_jump(struct udpif_key *ukey,
+  const struct dpif_flow_stats *stats)
+{
+static struct vlog_rate_limit rll = VLOG_RATE_LIMIT_INIT(1, 5);
+struct ds ds = DS_EMPTY_INITIALIZER;
+struct ofpbuf *actions;
+
+odp_format_ufid(>ufid, );
+ds_put_cstr(, " ");
+odp_flow_key_format(ukey->key, ukey->key_len, );
+ds_put_cstr(, ", actions:");
+actions = ovsrcu_get(struct ofpbuf *, >actions);
+format_odp_actions(, actions->data, actions->size, NULL);
+VLOG_WARN_RL(, "Unexpected jump in packet stats from %"PRIu64
+" to %"PRIu64" when handling ukey %s",
+stats->n_packets, ukey->stats.n_packets, ds_cstr());
+ds_destroy();
+}
+
 /* Verifies that the datapath actions of 'ukey' are still correct, and pushes
  * 'stats' for it.
  *
@@ -2372,18 +2392,15 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key 
*ukey,
 
 push.used = stats->used;
 push.tcp_flags = stats->tcp_flags;
-push.n_packets = (stats->n_packets > ukey->stats.n_packets
-  ? stats->n_packets - ukey->stats.n_packets
-  : 0);
-push.n_bytes = (stats->n_bytes > ukey->stats.n_bytes
-? stats->n_bytes - ukey->stats.n_bytes
-: 0);
+push.n_packets = stats->n_packets - ukey->stats.n_packets;
+push.n_bytes = stats->n_bytes - ukey->stats.n_bytes;
 
 if (stats->n_packets < ukey->stats.n_packets &&
 ukey->stats.n_packets < UINT64_THREE_QUARTERS) {
 /* Report cases where the packet counter is lower than the previous
  * instance, but exclude the potential wrapping of an uint64_t. */
 COVERAGE_INC(ukey_invalid_stat_reset);
+log_unexpected_stats_jump(ukey, stats);
 }
 
 if (need_revalidate) {
-- 
2.40.1

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

Re: [ovs-dev] [PATCH ovn v7 0/5] Implement MTU Path Discovery for multichassis ports

[Ales Musil]

On Tue, May 23, 2023 at 11:56 PM Ihar Hrachyshka 
wrote:

> This series fixes a non-optimal behavior with some multichassis ports.
>
> Specifically,
>
> - when a multichassis port belongs to a switch that also has a localnet
>   port,
> - because ingress and egress traffic for the port is funnelled through
>   tunnels to guarantee delivery of packets to all chassis that host the
>   port,
> - because tunnels add overhead to each funnelled packet,
> - leaving less space for actual packets,
>
> ... the effective MTU of the path for multichassis ports is reduced by
> the size that takes the tunnel to wrap a packet around. (This depends on
> the type of tunnel, also on IP version of the tunnel.)
>
> When this happens, the port and its peers are not notified about the
> reduced MTU for the path to the port, effectively blackholing some of
> the larger packets.
>
> This patch series makes OVN detect these scenarios and handle them as
> follows:
>
> - for all multichassis ports, 4 flows are added - for inport and
>   outport matches - to detect "too-big" IP packets;
> - for all "too-big" packets, 2 flows are added to produce a ICMP
>   Fragmentation Needed (for IPv4) or Too Big (for IPv6) packet and
>   return it to the offending sender.
>
> The proposed implementation is isolated in ovn-controller. An
> alternative would be to implement flows producing ICMP errors via the
> existing Logical_Flow action icmp4_error, as is done for router ports.
> In this case, the 4 flows that detect "too-big" packets would still have
> to reside in ovn-controller because of limitations of the current OVN
> port model, namely lack of `mtu` as an attribute of OVN logical port.
>
> Caveats: this works for IP traffic only. The party receiving the ICMP
> error should handle it by temporarily lowering the MTU used to reach the
> port. Behavior may depend on protocol implementation of the offending
> peer as well as its networking stack configuration.
>
> This patch series was successfully tested with Linux kernel 6.0.8.
>
> This patch series is designed to simplify backporting process. It is
> split in logical pieces to simplify cherry-picking. I consider the
> original behavior described at the start of the cover letter a bug and
> worth a backport.
>
> ---
> v1: - initial version.
> v2: - more system test cases adjusted to new table numbers for egress
>   pipeline;
> - switch from xxd to coreutils (tr, head) tools to avoid a new
>   dependency;
> - adjusted a comment in the new test case to avoid "migrator"
>   terminology that makes no sense outside live migration context -
>   which is not the only potential use case for multichassis ports;
> - guard the new test case with HAVE_SCAPY;
> - nit: rewrapped some lines in patch 6/7 to avoid unnecessary line
>   changes;
> - added a NEWS entry for patch 6/7 that implements the core of the
>   feature;
> - rebased to latest main.
> v3: - expose and reuse lib/actions.c:encode_[start|finish]_controller_op
>   functions;
> - reuse ETH_HEADER_LEN, IP_HEADER_LEN, and IPV6_HEADER_LEN from
>   ovs:lib/packets.h;
> - add a comment for REGBIT_PKT_LARGER that mentions that the
>   register is also used in OFTABLE_OUTPUT_LARGE_PKT_DETECT table;
> - squash the patch that makes if-status-mgr track changes to
>   interface mtu into the patch that introduces the mtu field in the
>   manager;
> - doc: don't describe path discovery flows in the patch that
>   introduces new tables (only in the patch that actually implements
>   the flows);
> - nit: use MAX to shave off several lines of code;
> - nit: remove redundant if_status_mgr_iface_get_mtu declaration from
>   a header file;
> - rebased to latest main;
> - updated acks based on latest review comments.
> v4: - fix compilation issue in the middle of the series due to previous
>   commit rearrangement.
> v5: - introduce a new if-status-mgr input to pflow_output, then process
>   OVS interface updates inside if-status-mgr handlers.
> - if-status-mgr: remove set_mtu public function, instead other
>   modules should call to if_status_mgr_iface_update. This allows the
>   callers to not care which particular fields if-status-mgr would
>   like to persist from the OVS record, keeping concerns separated.
> v6: - update more tests in ovn-controller.at to reflect new table
>   numbers.
> v7: - remove ovs submodule bump that creeped in by mistake.
> ---
>
> Ihar Hrachyshka (5):
>   Track ip version of tunnel in chassis_tunnel struct
>   Track interface MTU in if-status-mgr
>   if-status: track interfaces for additional chassis
>   Add new egress tables to accommodate for too-big packets handling
>   Implement MTU Path Discovery for multichassis ports
>
>  NEWS|   6 +
>  controller/binding.c|  48 +--
>  controller/binding.h|   4 +
>  controller/if-status.c  |  48 ++-
>  

Re: [ovs-dev] [PATCH v10] netdev-dpdk: add control plane protection support

[Kevin Traynor]

On 24/05/2023 16:05, Robin Jarry wrote:

Kevin Traynor, May 24, 2023 at 16:54:

The "rss" item should be mandatory anyway. There should be no way to
disable it. I assume that it is what Ilya meant with these "+" symbols.
That would leave us with these examples:

# lacp+bfd on separate queue, rss on other queues

  options:rx-steering=rss,lacp,bfd

# same

  options:rx-steering=lacp,bfd,rss


^ It looks a little odd to me that some values are for single queues and
some are for the rest of the queues, with no way to distinguish.

So maybe Ilya had placed significance in the order with his suggestion ?
i.e. [default]+[single queue proto]+[other single queue proto]+...

I don't want to bike shed too much on it, i guess with good docs, it can
be clear anyway.


# only regular rss, same as no config at all

  options:rx-steering=rss

# invalid configurations

  options:rx-steering=lacp



  options:rx-steering=

(^ This will be ok and use the default rss)


In that case, maybe we should exclude "rss" from the available items to
avoid confusion and only require users to specify the protocols that
need to be put in a separate queue. The remaining queues being rss by
default.

Would that be ok?



Ilya explained the reasoning behind his idea in other mail and it seems 
fine, so for the sake of progress, I'd say to go with with his idea if 
you don't object to it.


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

[ovs-dev] [PATCH ovn] controller: Handle OpenFlow errors.

[Dumitru Ceara]

Whenever an OpenFlow error is returned by OvS, trigger a reconnect of
the OpenFlow (rconn) connection.  This will clear any installed OpenFlow
rules/groups. To ensure consistency, trigger a full I-P recompute too.

An example of scenario that can result in an OpenFlow error returned by
OvS follows (describing two main loop iterations in ovn-controller):
  - Iteration I:
a. get updates from SB
b. process these updates and generate "desired" openflows (lets assume
this generates quite a lot of desired openflow modifications)
c.1. add bundle-open msg to rconn
c.2. add openflow mod msgs to rconn (only some of these make it through,
the rest gets queued, the rconn is backlogged at this point).
c.3. add bundle-commit msg to rconn (this gets queued)

  - Iteration II:
a. get updates from SB (rconn is still backlogged)
b. process the updates and generate "desired" openflows (lets assume
this takes 10+ seconds for the specific SB updates)

At some point, while step II.b was being executed OvS declared the bundle
operation (started at I.c.1) timeout.  We now act on this error by
reconnecting which in turn triggers a flush of the rconn backlog and
gives more chance to the next full recompute to succeed in installing
all flows.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2134880
Reported-by: François Rigault 
CC: Ilya Maximets 
Signed-off-by: Dumitru Ceara 
---
 controller/ofctrl.c | 17 ++---
 controller/ofctrl.h |  2 +-
 controller/ovn-controller.c | 10 --
 3 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index b1ba1c743a..1da23bc27e 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -766,13 +766,18 @@ ofctrl_get_mf_field_id(void)
 
 /* Runs the OpenFlow state machine against 'br_int', which is local to the
  * hypervisor on which we are running.  Attempts to negotiate a Geneve option
- * field for class OVN_GENEVE_CLASS, type OVN_GENEVE_TYPE. */
-void
+ * field for class OVN_GENEVE_CLASS, type OVN_GENEVE_TYPE.
+ *
+ * Returns 'true' if an OpenFlow reconnect happened; 'false' otherwise.
+ */
+bool
 ofctrl_run(const struct ovsrec_bridge *br_int,
const struct ovsrec_open_vswitch_table *ovs_table,
struct shash *pending_ct_zones)
 {
 char *target = xasprintf("unix:%s/%s.mgmt", ovs_rundir(), br_int->name);
+bool reconnected = false;
+
 if (strcmp(target, rconn_get_target(swconn))) {
 VLOG_INFO("%s: connecting to switch", target);
 rconn_connect(swconn, target, target);
@@ -782,10 +787,12 @@ ofctrl_run(const struct ovsrec_bridge *br_int,
 rconn_run(swconn);
 
 if (!rconn_is_connected(swconn)) {
-return;
+goto done;
 }
+
 if (seqno != rconn_get_connection_seqno(swconn)) {
 seqno = rconn_get_connection_seqno(swconn);
+reconnected = true;
 state = S_NEW;
 
 /* Reset the state of any outstanding ct flushes to resend them. */
@@ -855,6 +862,9 @@ ofctrl_run(const struct ovsrec_bridge *br_int,
  * point, so ensure that we come back again without waiting. */
 poll_immediate_wake();
 }
+
+done:
+return reconnected;
 }
 
 void
@@ -909,6 +919,7 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype type)
 } else if (type == OFPTYPE_ERROR) {
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(30, 300);
 log_openflow_rl(, VLL_INFO, oh, "OpenFlow error");
+rconn_reconnect(swconn);
 } else {
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(30, 300);
 log_openflow_rl(, VLL_DBG, oh, "OpenFlow packet ignored");
diff --git a/controller/ofctrl.h b/controller/ofctrl.h
index f5751e3ee4..105f9370be 100644
--- a/controller/ofctrl.h
+++ b/controller/ofctrl.h
@@ -51,7 +51,7 @@ struct ovn_desired_flow_table {
 void ofctrl_init(struct ovn_extend_table *group_table,
  struct ovn_extend_table *meter_table,
  int inactivity_probe_interval);
-void ofctrl_run(const struct ovsrec_bridge *br_int,
+bool ofctrl_run(const struct ovsrec_bridge *br_int,
 const struct ovsrec_open_vswitch_table *,
 struct shash *pending_ct_zones);
 enum mf_field_id ofctrl_get_mf_field_id(void);
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 1151d36644..b301c50157 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -5075,8 +5075,14 @@ main(int argc, char *argv[])
 
 if (br_int) {
 ct_zones_data = engine_get_data(_ct_zones);
-if (ct_zones_data) {
-ofctrl_run(br_int, ovs_table, _zones_data->pending);
+if (ct_zones_data && ofctrl_run(br_int, ovs_table,
+_zones_data->pending)) {
+static struct vlog_rate_limit rl
+= VLOG_RATE_LIMIT_INIT(1, 

Re: [ovs-dev] [PATCH] tests: layer3-tunnels: Skip bareudp tests if not supported by kernel.

[Simon Horman]

On Wed, May 24, 2023 at 09:23:59PM +0200, Ilya Maximets wrote:
> On 5/24/23 15:39, Frode Nordahl wrote:
> > The bareudp tests depend on specific kernel configuration to
> > succeed.  Skip the test if the feature is not enabled in the
> > running kernel.
> > 
> > Signed-off-by: Frode Nordahl 
> > ---
> >  tests/system-kmod-macros.at  | 10 ++
> >  tests/system-layer3-tunnels.at   |  2 ++
> >  tests/system-userspace-macros.at |  8 
> >  3 files changed, 20 insertions(+)
> > 
> > diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at
> > index fb15a5a7c..55e7821ce 100644
> > --- a/tests/system-kmod-macros.at
> > +++ b/tests/system-kmod-macros.at
> > @@ -237,3 +237,13 @@ m4_define([CHECK_L3L4_CONNTRACK_REASM])
> >  #
> >  # The kernel module tests do not use TC offload.
> >  m4_define([CHECK_NO_TC_OFFLOAD])
> > +
> > +# OVS_CHECK_BAREUDP()
> > +#
> > +# The feature needs to be enabled in the kernel configuration 
> > (CONFIG_BAREUDP)
> > +# to work.
> > +m4_define([OVS_CHECK_BAREUDP],
> > +[
> > +AT_SKIP_IF([! ip link add dev bareudp0 type bareudp dstport 6635 
> > ethertype mpls_uc 2>&1 >/dev/null])
> > +AT_CHECK([ip link del dev bareudp0])
> 
> Maybe call it ovs_bareudp0 or something like that?  Just to decrease chances
> for random collisions.
> 
> > +])
> > diff --git a/tests/system-layer3-tunnels.at b/tests/system-layer3-tunnels.at
> > index c37852b21..5546bc879 100644
> > --- a/tests/system-layer3-tunnels.at
> > +++ b/tests/system-layer3-tunnels.at
> > @@ -155,6 +155,7 @@ AT_CLEANUP
> >  
> >  AT_SETUP([layer3 - ping over MPLS Bareudp])
> >  OVS_CHECK_MIN_KERNEL(5, 7)
> > +OVS_CHECK_BAREUDP()
> 
> This new check supersedes the kernel version check.  Should we remove it?
> The original idea was that we can create bareudp tunnels even if the ip 
> utility
> didn't support them at a time.  But, I suppose, enough time passed since then
> and the ip utility available in distributions caught up, so we can just check
> it instead.

I'd say that kernel checks are unreliable in the presence of backports.
Whereas tool checks suffer the problem you describe.
At this point I'd lean towards a tool-based check.
As you say, some time has passed by now.

FWIIW, I believe we've always relied on a tool based check for PPS rate
limiting checks, which are similar. Perhaps unwisely. But in practice the
problems I'm aware of there is with the implementation of the check
(hopefully fixed by now) not the tool vs kernel issue.

> >  OVS_TRAFFIC_VSWITCHD_START([_ADD_BR([br1])])
> >  ADD_NAMESPACES(at_ns0, at_ns1)
> >  
> > @@ -203,6 +204,7 @@ AT_CLEANUP
> >  
> >  AT_SETUP([layer3 - ping over Bareudp])
> >  OVS_CHECK_MIN_KERNEL(5, 7)
> 
> Same here.
> 
> > +OVS_CHECK_BAREUDP()
> >  OVS_TRAFFIC_VSWITCHD_START([_ADD_BR([br1])])
> >  ADD_NAMESPACES(at_ns0, at_ns1)
> >  
> > diff --git a/tests/system-userspace-macros.at 
> > b/tests/system-userspace-macros.at
> > index 482079386..1cb67d6f6 100644
> > --- a/tests/system-userspace-macros.at
> > +++ b/tests/system-userspace-macros.at
> > @@ -336,3 +336,11 @@ m4_define([CHECK_L3L4_CONNTRACK_REASM],
> >  #
> >  # Userspace tests do not use TC offload.
> >  m4_define([CHECK_NO_TC_OFFLOAD])
> > +
> > +# OVS_CHECK_BAREUDP()
> > +#
> > +# The userspace skips all tests that check kernel configuration.
> 
> This should probably say that userspace datapath doesn't support bareudp 
> tunnels
> instead.
> 
> > +m4_define([OVS_CHECK_BAREUDP],
> > +[
> > +AT_SKIP_IF([:])
> > +])
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v4] backtrace: Extend the backtrace functionality

[Ales Musil]

On Wed, May 24, 2023 at 8:31 PM Ilya Maximets  wrote:

> On 5/19/23 09:36, Ales Musil wrote:
> > Use the backtrace functions that is provided by libc,
> > this allows us to get backtrace that is independent of
> > the current memory map of the process. Which in turn can
> > be used for debugging/tracing purpose. The backtrace
> > is not 100% accurate due to various optimizations, most
> > notably "-fomit-frame-pointer" and LTO. This might result
> > that the line in source file doesn't correspond to the
> > real line. However, it should be able to pinpoint at least
> > the function where the backtrace was called.
> >
> > The usage for SIGSEGV is determined during compilation
> > based on available libraries. Libunwind has higher priority
> > if both methods are available to keep the compatibility with
> > current behavior.
> >
> > The backtrace is not marked as signal safe however the backtrace
> > manual page gives more detailed explanation why it might be the
> > case [0]. Load the "libgcc" or equivalent in advance within the
> > "fatal_signal_init" which should ensure that subsequent calls
> > to backtrace* do not call malloc and are signal safe.
> >
> > The typical backtrace will look similar to the one below:
> > /lib64/libopenvswitch-3.1.so.0(backtrace_capture+0x1e) [0x7fc5db298dfe]
> > /lib64/libopenvswitch-3.1.so.0(log_backtrace_at+0x57) [0x7fc5db2999e7]
> > /lib64/libovsdb-3.1.so.0(ovsdb_txn_complete+0x7b) [0x7fc5db56247b]
> > /lib64/libovsdb-3.1.so.0(ovsdb_txn_propose_commit_block+0x8d)
> [0x7fc5db563a8d]
> > ovsdb-server(+0xa661) [0x562cfce2e661]
> > ovsdb-server(+0x7e39) [0x562cfce2be39]
> > /lib64/libc.so.6(+0x27b4a) [0x7fc5db048b4a]
> > /lib64/libc.so.6(__libc_start_main+0x8b) [0x7fc5db048c0b]
> > ovsdb-server(+0x8c35) [0x562cfce2cc35]
> >
> > backtrace.h elaborates on how to effectively get the line
> > information associated with the addressed presented in the
> > backtrace.
> >
> > [0]
> > backtrace() and backtrace_symbols_fd() don't call malloc()
> > explicitly, but they are part of libgcc, which gets loaded
> > dynamically when first used.  Dynamic loading usually triggers
> > a call to malloc(3).  If you need certain calls to these two
> > functions to not allocate memory (in signal handlers, for
> > example), you need to make sure libgcc is loaded beforehand
> >
> > Reported-at: https://bugzilla.redhat.com/2177760
> > Signed-off-by: Ales Musil 
> > ---
> > v2: Extend the current use of libunwind rather than replacing it.
> > v3: Allow vlog_fd to be also 0.
> > Return the backtrace log from monitor in updated form.
> > Return use of the "vlog_direct_write_to_log_file_unsafe".
> > v4: Rebase on top of current master.
> > Address comments from Ilya:
> > Address the coding style issues.
> > Make sure that the backrace received by monitor doesn't
> > have more than maximum allowed frames.
> > Change the backtrace_format to accept delimiter.
> > Remove unnecessary checks in the tests.
> > ---
>
> Hi, Ales.  See a few comments below.
>

Hi Ilya,

sorry I have missed those in previous email, not sure why.
It should be addressed in v5.


> >  include/openvswitch/vlog.h |   3 +
> >  lib/backtrace.c| 116 +
> >  lib/backtrace.h|  57 +++---
> >  lib/fatal-signal.c |  52 +++--
> >  lib/ovsdb-error.c  |  15 ++---
> >  lib/vlog.c |   7 +++
> >  m4/openvswitch.m4  |   9 ++-
> >  tests/atlocal.in   |   2 +
> >  tests/daemon.at|  45 ++
> >  9 files changed, 228 insertions(+), 78 deletions(-)
> >
> > diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h
> > index e53ce6d81..d3c369bf2 100644
> > --- a/include/openvswitch/vlog.h
> > +++ b/include/openvswitch/vlog.h
> > @@ -148,6 +148,9 @@ void vlog_set_syslog_target(const char *target);
> >  /* Write directly to log file. */
> >  void vlog_direct_write_to_log_file_unsafe(const char *s);
> >
> > +/* Return the current vlog file descriptor. */
> > +int vlog_fd(void);
>
> I'll repeat my comment from v3 here:
> """
> It's unfortunate we need to export this kind of function.  We should
> give it a better name at least.  Maybe vlog_get_log_file_fd_unsafe() ?
> vlog module doesn't have a file descriptor, file does.  And fd is
> supposed to be protected by a mutex that we're clearly not holding
> while using this function, so 'unsafe'.
> """
>
> > +
> >  /* Initialization. */
> >  void vlog_init(void);
> >  void vlog_enable_async(void);
> > diff --git a/lib/backtrace.c b/lib/backtrace.c
> > index 2853d5ff1..4cac5e14c 100644
> > --- a/lib/backtrace.c
> > +++ b/lib/backtrace.c
> > @@ -32,12 +32,27 @@ VLOG_DEFINE_THIS_MODULE(backtrace);
> >  void
> >  backtrace_capture(struct backtrace *b)
> >  {
> > -void *frames[BACKTRACE_MAX_FRAMES];
> > -int i;
> > +b->n_frames = backtrace(b->frames, BACKTRACE_MAX_FRAMES);
> > +}
> > +
> > +void
> > 

[ovs-dev] [PATCH v5] backtrace: Extend the backtrace functionality

[Ales Musil]

Use the backtrace functions that is provided by libc,
this allows us to get backtrace that is independent of
the current memory map of the process. Which in turn can
be used for debugging/tracing purpose. The backtrace
is not 100% accurate due to various optimizations, most
notably "-fomit-frame-pointer" and LTO. This might result
that the line in source file doesn't correspond to the
real line. However, it should be able to pinpoint at least
the function where the backtrace was called.

The usage for SIGSEGV is determined during compilation
based on available libraries. Libunwind has higher priority
if both methods are available to keep the compatibility with
current behavior.

The backtrace is not marked as signal safe however the backtrace
manual page gives more detailed explanation why it might be the
case [0]. Load the "libgcc" or equivalent in advance within the
"fatal_signal_init" which should ensure that subsequent calls
to backtrace* do not call malloc and are signal safe.

The typical backtrace will look similar to the one below:
/lib64/libopenvswitch-3.1.so.0(backtrace_capture+0x1e) [0x7fc5db298dfe]
/lib64/libopenvswitch-3.1.so.0(log_backtrace_at+0x57) [0x7fc5db2999e7]
/lib64/libovsdb-3.1.so.0(ovsdb_txn_complete+0x7b) [0x7fc5db56247b]
/lib64/libovsdb-3.1.so.0(ovsdb_txn_propose_commit_block+0x8d) [0x7fc5db563a8d]
ovsdb-server(+0xa661) [0x562cfce2e661]
ovsdb-server(+0x7e39) [0x562cfce2be39]
/lib64/libc.so.6(+0x27b4a) [0x7fc5db048b4a]
/lib64/libc.so.6(__libc_start_main+0x8b) [0x7fc5db048c0b]
ovsdb-server(+0x8c35) [0x562cfce2cc35]

backtrace.h elaborates on how to effectively get the line
information associated with the addressed presented in the
backtrace.

[0]
backtrace() and backtrace_symbols_fd() don't call malloc()
explicitly, but they are part of libgcc, which gets loaded
dynamically when first used.  Dynamic loading usually triggers
a call to malloc(3).  If you need certain calls to these two
functions to not allocate memory (in signal handlers, for
example), you need to make sure libgcc is loaded beforehand

Reported-at: https://bugzilla.redhat.com/2177760
Signed-off-by: Ales Musil 
---
v2: Extend the current use of libunwind rather than replacing it.
v3: Allow vlog_fd to be also 0.
Return the backtrace log from monitor in updated form.
Return use of the "vlog_direct_write_to_log_file_unsafe".
v4: Rebase on top of current master.
Address comments from Ilya:
Address the coding style issues.
Make sure that the backrace received by monitor doesn't
have more than maximum allowed frames.
Change the backtrace_format to accept delimiter.
Remove unnecessary checks in the tests.
v5: Rebase on top of current master.
Address missed comment from v3:
Reaname the vlog_fd to vlog_get_log_file_fd_unsafe to reflect that
this is really unsafe.
Return the ovsdb backtrace to correct position in log and
keep the previous format.
---
 include/openvswitch/vlog.h |   3 +
 lib/backtrace.c| 116 +
 lib/backtrace.h|  57 +++---
 lib/fatal-signal.c |  52 +++--
 lib/ovsdb-error.c  |  12 +---
 lib/vlog.c |   7 +++
 m4/openvswitch.m4  |   9 ++-
 tests/atlocal.in   |   2 +
 tests/daemon.at|  45 ++
 9 files changed, 227 insertions(+), 76 deletions(-)

diff --git a/include/openvswitch/vlog.h b/include/openvswitch/vlog.h
index e53ce6d81..c9202a23e 100644
--- a/include/openvswitch/vlog.h
+++ b/include/openvswitch/vlog.h
@@ -148,6 +148,9 @@ void vlog_set_syslog_target(const char *target);
 /* Write directly to log file. */
 void vlog_direct_write_to_log_file_unsafe(const char *s);
 
+/* Return the current vlog file descriptor. */
+int  vlog_get_log_file_fd_unsafe(void);
+
 /* Initialization. */
 void vlog_init(void);
 void vlog_enable_async(void);
diff --git a/lib/backtrace.c b/lib/backtrace.c
index 2853d5ff1..4cac5e14c 100644
--- a/lib/backtrace.c
+++ b/lib/backtrace.c
@@ -32,12 +32,27 @@ VLOG_DEFINE_THIS_MODULE(backtrace);
 void
 backtrace_capture(struct backtrace *b)
 {
-void *frames[BACKTRACE_MAX_FRAMES];
-int i;
+b->n_frames = backtrace(b->frames, BACKTRACE_MAX_FRAMES);
+}
+
+void
+backtrace_format(const struct backtrace *bt, struct ds *ds,
+ const char *delimiter)
+{
+if (bt->n_frames) {
+char **symbols = backtrace_symbols(bt->frames, bt->n_frames);
+
+if (!symbols) {
+return;
+}
 
-b->n_frames = backtrace(frames, BACKTRACE_MAX_FRAMES);
-for (i = 0; i < b->n_frames; i++) {
-b->frames[i] = (uintptr_t) frames[i];
+for (int i = 0; i < bt->n_frames - 1; i++) {
+ds_put_format(ds, "%s%s", symbols[i], delimiter);
+}
+
+ds_put_format(ds, "%s", symbols[bt->n_frames - 1]);
+
+free(symbols);
 }
 }
 
@@ -47,23 +62,14 @@ backtrace_capture(struct backtrace *backtrace)
 {
 backtrace->n_frames = 0;
 }

Re: [ovs-dev] [PATCH ovn] northd: Fix address set incremental processing

[Ales Musil]

On Wed, May 24, 2023 at 1:11 PM Ilya Maximets  wrote:

> On 5/23/23 10:09, Ales Musil wrote:
> > The incremental processing is broken for addresses
> > that have mask which could "erase" portion of the address
> > itself e.g. 10.16.0.47/4, after applying the mask with token
> > parser the address becomes 0.0.0.0/4, which is fine for
> > logical flows. However, for the deletion/addition to database
> > we need the original string representation which cannot be
> > built from the parsed token.
> >
> > Use svec instead for the comparison which allows us to keep the
> > original strings.
> >
> > The change to svec shouldn't have any significant performance
> > impact, see the numbers below show. The setup was created by
> > the scale script from Han [0].
> >
> > Numbers with expr:
> > Time spent on processing nb_cfg 1:
> > ovn-northd delay before processing: 21ms
> > ovn-northd completion:  544ms
> > ovn-controller(s) completion:   544ms
> > Time spent on processing nb_cfg 2:
> > ovn-northd delay before processing: 17ms
> > ovn-northd completion:  537ms
> > ovn-controller(s) completion:   535ms
> > Time spent on processing nb_cfg 3:
> > ovn-northd delay before processing: 20ms
> > ovn-northd completion:  552ms
> > ovn-controller(s) completion:   550ms
> > Time spent on processing nb_cfg 4:
> > ovn-northd delay before processing: 16ms
> > ovn-northd completion:  529ms
> > ovn-controller(s) completion:   528ms
> >
> > Numbers with svec:
> > Time spent on processing nb_cfg 1:
> > ovn-northd delay before processing: 19ms
> > ovn-northd completion:  548ms
> > ovn-controller(s) completion:   548ms
> > Time spent on processing nb_cfg 2:
> > ovn-northd delay before processing: 19ms
> > ovn-northd completion:  537ms
> > ovn-controller(s) completion:   537ms
> > Time spent on processing nb_cfg 3:
> > ovn-northd delay before processing: 15ms
> > ovn-northd completion:  522ms
> > ovn-controller(s) completion:   521ms
> > Time spent on processing nb_cfg 4:
> > ovn-northd delay before processing: 17ms
> > ovn-northd completion:  522ms
> > ovn-controller(s) completion:   520ms
> >
> > [0]
> https://github.com/hzhou8/ovn-test-script/blob/e1149318201309db6d96ae8d5a33fcfbbe1872a3/test_ovn_controller_scale.sh
> >
> > Fixes: 0d5e0a6ced49 ("northd: Add I-P for syncing SB address sets.")
> > Reported-at: https://bugzilla.redhat.com/2196885
>
> The issue was originally reported on the mailing list.  Please,
> provide the link as well.  And it'd also be nice to credit the
> original reporter.
>

Added in v2.


>
> > Signed-off-by: Ales Musil 
> > ---
> >  northd/en-sync-sb.c | 66 +
> >  1 file changed, 30 insertions(+), 36 deletions(-)
> >
> > diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
> > index 758f00bfd..4bd77b168 100644
> > --- a/northd/en-sync-sb.c
> > +++ b/northd/en-sync-sb.c
> > @@ -22,7 +22,6 @@
> >  #include "openvswitch/util.h"
> >
> >  #include "en-sync-sb.h"
> > -#include "include/ovn/expr.h"
> >  #include "lib/inc-proc-eng.h"
> >  #include "lib/lb.h"
> >  #include "lib/ovn-nb-idl.h"
> > @@ -325,45 +324,40 @@ static void
> >  update_sb_addr_set(const char **nb_addresses, size_t n_addresses,
> > const struct sbrec_address_set *sb_as)
> >  {
> > -struct expr_constant_set *cs_nb_as =
> > -expr_constant_set_create_integers(
> > -(const char *const *) nb_addresses, n_addresses);
> > -struct expr_constant_set *cs_sb_as =
> > -expr_constant_set_create_integers(
> > -(const char *const *) sb_as->addresses, sb_as->n_addresses);
> > -
> > -struct expr_constant_set *addr_added = NULL;
> > -struct expr_constant_set *addr_deleted = NULL;
> > -expr_constant_set_integers_diff(cs_sb_as, cs_nb_as, _added,
> > -_deleted);
> > -
> > -struct ds ds = DS_EMPTY_INITIALIZER;
> > -if (addr_added && addr_added->n_values) {
> > -for (size_t i = 0; i < addr_added->n_values; i++) {
> > -ds_clear();
> > -expr_constant_format(_added->values[i],
> EXPR_C_INTEGER, );
> > -sbrec_address_set_update_addresses_addvalue(sb_as,
> ds_cstr());
> > -}
> > +size_t i;
> > +char *ip;
> > +
> > +struct svec svec_nb_as = SVEC_EMPTY_INITIALIZER;
> > +struct svec svec_sb_as = SVEC_EMPTY_INITIALIZER;
> > +
> > +for (i = 0; i < n_addresses; i++) {
> > +svec_add(_nb_as, nb_addresses[i]);
> >  }
> >
> > -if (addr_deleted && addr_deleted->n_values) {
> > -for (size_t i = 0; i < 

[ovs-dev] [PATCH ovn v2] northd: Fix address set incremental processing

[Ales Musil]

The incremental processing is broken for addresses
that have mask which could "erase" portion of the address
itself e.g. 10.16.0.47/4, after applying the mask with token
parser the address becomes 0.0.0.0/4, which is fine for
logical flows. However, for the deletion/addition to database
we need the original string representation which cannot be
built from the parsed token.

Use strcmp over already sorted lists which should give
us the needed diff. The time complexity didn't change,
but the performance was slightly improved, see the numbers
below. The setup was created by the scale script from Han [0].

Numbers with expr:
Time spent on processing nb_cfg 1:
ovn-northd delay before processing: 21ms
ovn-northd completion:  544ms
ovn-controller(s) completion:   544ms
Time spent on processing nb_cfg 2:
ovn-northd delay before processing: 17ms
ovn-northd completion:  537ms
ovn-controller(s) completion:   535ms
Time spent on processing nb_cfg 3:
ovn-northd delay before processing: 20ms
ovn-northd completion:  552ms
ovn-controller(s) completion:   550ms
Time spent on processing nb_cfg 4:
ovn-northd delay before processing: 16ms
ovn-northd completion:  529ms
ovn-controller(s) completion:   528ms

Numbers with the strcmp:
Time spent on processing nb_cfg 1:
ovn-northd delay before processing: 24ms
ovn-northd completion:  521ms
ovn-controller(s) completion:   521ms
Time spent on processing nb_cfg 2:
ovn-northd delay before processing: 17ms
ovn-northd completion:  500ms
ovn-controller(s) completion:   500ms
Time spent on processing nb_cfg 3:
ovn-northd delay before processing: 17ms
ovn-northd completion:  496ms
ovn-controller(s) completion:   497ms
Time spent on processing nb_cfg 4:
ovn-northd delay before processing: 18ms
ovn-northd completion:  502ms
ovn-controller(s) completion:   500ms

[0] 
https://github.com/hzhou8/ovn-test-script/blob/e1149318201309db6d96ae8d5a33fcfbbe1872a3/test_ovn_controller_scale.sh

Fixes: 0d5e0a6ced49 ("northd: Add I-P for syncing SB address sets.")
Reported-at: https://bugzilla.redhat.com/2196885
Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-discuss/2023-May/052426.html
Reported-By: 张祖建 
Signed-off-by: Ales Musil 
---
v2: Address comments from Ilya:
Use "custom" algorithm to prevent a ton of allocations with svec.
Add original reporter.
---
 northd/en-sync-sb.c | 183 +++-
 1 file changed, 112 insertions(+), 71 deletions(-)

diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
index 758f00bfd..87402d6fb 100644
--- a/northd/en-sync-sb.c
+++ b/northd/en-sync-sb.c
@@ -22,7 +22,6 @@
 #include "openvswitch/util.h"
 
 #include "en-sync-sb.h"
-#include "include/ovn/expr.h"
 #include "lib/inc-proc-eng.h"
 #include "lib/lb.h"
 #include "lib/ovn-nb-idl.h"
@@ -34,8 +33,15 @@
 
 VLOG_DEFINE_THIS_MODULE(en_sync_to_sb);
 
+/* This is just a type wrapper to enforce that it has to be sorted. */
+struct sorted_addresses {
+const char **arr;
+size_t n;
+};
+
+
 static void sync_addr_set(struct ovsdb_idl_txn *ovnsb_txn, const char *name,
-  const char **addrs, size_t n_addrs,
+  struct sorted_addresses *addresses,
   struct shash *sb_address_sets);
 static void sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn,
const struct nbrec_address_set_table *,
@@ -44,11 +50,17 @@ static void sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn,
const struct ovn_datapaths *lr_datapaths);
 static const struct sbrec_address_set *sb_address_set_lookup_by_name(
 struct ovsdb_idl_index *, const char *name);
-static void update_sb_addr_set(const char **nb_addresses, size_t n_addresses,
+static void update_sb_addr_set(struct sorted_addresses *,
const struct sbrec_address_set *);
 static void build_port_group_address_set(const struct nbrec_port_group *,
  struct svec *ipv4_addrs,
  struct svec *ipv6_addrs);
+static struct sorted_addresses
+sorted_addresses_from_nbrec(const struct nbrec_address_set *nb_as);
+static struct sorted_addresses
+sorted_addresses_from_svec(struct svec *addresses);
+static struct sorted_addresses
+sorted_addresses_from_sset(struct sset* addresses);
 
 void *
 en_sync_to_sb_init(struct engine_node *node OVS_UNUSED,
@@ -133,8 +145,9 @@ sync_to_sb_addr_set_nb_address_set_handler(struct 
engine_node *node,
 if (!sb_addr_set) {
 return false;
 }
-