Re: [ovs-dev] [PATCH ovn v3 2/2] expr: Avoid crash if all sub-expressions crushed down to 'true'.

2023-03-30 Thread Han Zhou
On Wed, Mar 29, 2023 at 9:05 AM Ilya Maximets  wrote:
>
> expr_sort() can potentially return NULL if all sub-expressions got
> crushed into 'true'.  This didn't happen before, but can happen
> with more aggressive optimizations in crush_or() in the future.
>
> Signed-off-by: Ilya Maximets 
> ---
>  lib/expr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/expr.c b/lib/expr.c
> index 8fd07b2d5..69c387140 100644
> --- a/lib/expr.c
> +++ b/lib/expr.c
> @@ -2988,7 +2988,7 @@ expr_sort(struct expr *expr)
>  }
>  free(subs);
>
> -return expr;
> +return expr ? expr : expr_create_boolean(true);
>  }
>
>  static struct expr *expr_normalize_or(struct expr *expr);
> --
> 2.39.2
>

Acked-by: Han Zhou 

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


Re: [ovs-dev] [PATCH ovn v3 1/2] expr: Remove supersets from OR expressions.

2023-03-30 Thread Han Zhou
On Wed, Mar 29, 2023 at 9:05 AM Ilya Maximets  wrote:
>
> While crushing OR expressions, OVN removes exact replicas of sub
> expressions.  However, there could be many CMP expressions that are
> supersets of each other.  These are most likely to be created as a
> result of cross-product while expanding brackets in the AND expression
> in crush_and_numeric(), i.e. while converting
> "x && (a0 || a1) && (b0 || b1)" into "xa0b0 || xa0b1 || xa1b0 || xa1b1".
>
> In addition to removal of exact duplicates introducing scan and removal
> of supersets of other existing sub-expressions to reduce the amount of
> generated flows.  This adds extra computations, but should save time
> later, since less flows will be generated.
>
> Example:
>
>   "ip4.src == 172.168.0.0/16 && ip4.src!={172.168.13.0/24, 172.168.15.0/24
}"
>
>   Processing of this expression yields 42 flows:
>
>   $ ./tests/ovstest test-ovn expr-to-flows <<< "$expr"
>
>   ip,nw_src=172.168.0.0/255.255.1.0
>   ip,nw_src=172.168.0.0/255.255.10.0
>   ip,nw_src=172.168.0.0/255.255.12.0
>   ip,nw_src=172.168.0.0/255.255.3.0
>   ip,nw_src=172.168.0.0/255.255.4.0
>   ip,nw_src=172.168.0.0/255.255.5.0
>   ip,nw_src=172.168.0.0/255.255.6.0
>   ip,nw_src=172.168.0.0/255.255.8.0
>   ip,nw_src=172.168.0.0/255.255.9.0
>   ip,nw_src=172.168.128.0/17
>   <... 32 more flows ...>
>
>   We can see that many flows above do overlap, e.g. 255.255.3.0
>   mask is a superset of 255.255.1.0.  Everything that matches
>   255.255.3.0, will match 255.255.1.0 as well (the value is the same).
>
>   By removing all the unnecessary supersets, the set of flows can
>   be reduced from 42 down to 7:
>
>   ip,nw_src=172.168.0.0/255.255.1.0
>   ip,nw_src=172.168.0.0/255.255.4.0
>   ip,nw_src=172.168.0.0/255.255.8.0
>   ip,nw_src=172.168.128.0/17
>   ip,nw_src=172.168.16.0/255.255.16.0
>   ip,nw_src=172.168.32.0/255.255.32.0
>   ip,nw_src=172.168.64.0/255.255.64.0
>
> This change should be particularly useful for expressions with
> inequality checks, like the one above.  Such expressions are
> frequent among ACL rules.
>
>   "ip4.src != {172.168.13.0/24, 172.168.14.0/24, 172.168.15.0/24}"
>
>   Brefore:
>   $ ./tests/ovstest test-ovn expr-to-flows <<< "$expr" | wc -l
>   2894
>
>   After:
>   $ ./tests/ovstest test-ovn expr-to-flows <<< "$expr" | wc -l
>   23
>
> Superset lookups are performed only if there are expressions with
> more bits in the mask than in the current one.  So, there is no extra
> cost for equality checks on normal address sets, like port group sets,
> where all the IPs are exect matches or have the same prefix length
> otherwise.
>
> Use of bitmaps instead of subvalue functions significantly speeds up
> processing since most of the subvalue space is an all-zero empty space.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2177197
> Reported-by: Nadia Pinaeva 
> Signed-off-by: Ilya Maximets 
> ---
>  include/ovn/expr.h |   1 +
>  lib/expr.c | 238 ++---
>  tests/ovn.at   |  12 +++
>  3 files changed, 196 insertions(+), 55 deletions(-)
>
> diff --git a/include/ovn/expr.h b/include/ovn/expr.h
> index 80d95ff67..c48f82398 100644
> --- a/include/ovn/expr.h
> +++ b/include/ovn/expr.h
> @@ -384,6 +384,7 @@ struct expr {
>  struct {
>  union mf_subvalue value;
>  union mf_subvalue mask;
> +size_t mask_n_bits;
>  };
>  };
>  } cmp;
> diff --git a/lib/expr.c b/lib/expr.c
> index 0a6f7e574..8fd07b2d5 100644
> --- a/lib/expr.c
> +++ b/lib/expr.c
> @@ -15,21 +15,23 @@
>   */
>
>  #include 
> +#include "bitmap.h"
>  #include "byte-order.h"
> -#include "openvswitch/json.h"
> +#include "hmapx.h"
>  #include "nx-match.h"
>  #include "openvswitch/dynamic-string.h"
> +#include "openvswitch/json.h"
>  #include "openvswitch/match.h"
>  #include "openvswitch/ofp-actions.h"
> -#include "openvswitch/vlog.h"
>  #include "openvswitch/shash.h"
> +#include "openvswitch/vlog.h"
> +#include "ovn-util.h"
>  #include "ovn/expr.h"
>  #include "ovn/lex.h"
>  #include "ovn/logical-fields.h"
>  #include "simap.h"
>  #include "sset.h"
>  #include "util.h"
> -#include "ovn-util.h"
>
>  VLOG_DEFINE_THIS_MODULE(expr);
>
> @@ -2520,6 +2522,183 @@ crush_and_string(struct expr *expr, const struct
expr_symbol *symbol)
>  return expr_fix(expr);
>  }
>
> +static int
> +compare_cmps_3way(const struct expr *a, const struct expr *b)
> +{
> +ovs_assert(a->cmp.symbol == b->cmp.symbol);
> +if (!a->cmp.symbol->width) {
> +return strcmp(a->cmp.string, b->cmp.string);
> +} else if (a->cmp.mask_n_bits != b->cmp.mask_n_bits) {
> +return a->cmp.mask_n_bits < b->cmp.mask_n_bits ? -1 : 1;
> +} else {
> +int d = memcmp(>cmp.value, >cmp.value, sizeof
a->cmp.value);
> +if (!d) {
> +d = memcmp(>cmp.mask, >cmp.mask, sizeof a->cmp.mask);
> +}
> +return d;
> +}
> +}
> +
> +static 

Re: [ovs-dev] [PATCH v2] conntrack-tp: Fix clang warning.

2023-03-30 Thread 0-day Robot
Bleep bloop.  Greetings lin huang, I am a robot and I have tried out your patch.
Thanks for your contribution.

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


checkpatch:
ERROR: Author Lin Huang  needs to sign off.
Lines checked: 37, Warnings: 0, Errors: 1


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

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


Re: [ovs-dev] [PATCH dpdk-latest v3 0/6] Add support for DPDK meter HW offload

2023-03-30 Thread Nole Zhang
Hi,
Thanks for your reply.
Our original idea is imitating OVS-TC, meter action and flow is 
independence. First through
the proxy port create the meter, then flow use meter with meter id. So I only 
add the proxy port,
doesn't revise the flow with proxy port. So you think this idea cant be 
accepted?
As your said, when the flow first time use the meter/proxy port, we 
offload the meter to the HW, 
when the last flow with the meter/proxy port will be delete, we delete the 
meter from the HW. 
 About your idea, I have some issue about modify  the flow, because 
the flow cant get the meter
status, so we can't through flow to modify the meter in the HW.  We have two 
way to solve it, one is modify
the meter, we delete all the flow with meter from the HW, the key is through 
meter id lookup the all flow with
the meter id from the HW.  Another is update the meter through the proxy port 
in the HW. Do you think which
is better?
B.R.
Peng

> -Original Message-
> From: Eli Britstein 
> Sent: Thursday, March 30, 2023 8:34 PM
> To: Simon Horman ; d...@openvswitch.org
> Cc: Ilya Maximets ; Chaoyong He
> ; Nole Zhang ;
> Kevin Liu ; oss-drivers 
> Subject: RE: [PATCH dpdk-latest v3 0/6] Add support for DPDK meter HW
> offload
> 
> Hi
> 
> The usage of the proxy port is wrong.
> All transfer offload should be migrated to use it. There was [1-4], but it was
> not completed.
> 
> Meters should be created only on the proxy ports. As all offloads are moved
> to use proxy ports, this is how they are shared.
> Since we don't know the proxy port when meter is added, it can be lazy
> created upon the first flow that uses this meter/proxy-port (referenced
> counted), and destroy upon the last flow destroy using it.
> As there might be multiple "proxy" ports, there might be multiple HW meter
> objects associated with the same SW one. This should be managed.
> 
> [1]
> http://patchwork.ozlabs.org/project/openvswitch/list/?series=302525
> =*
> [2]
> http://patchwork.ozlabs.org/project/openvswitch/list/?series=310413
> =*
> [3]
> http://patchwork.ozlabs.org/project/openvswitch/list/?series=310415
> =*
> [4]
> http://patchwork.ozlabs.org/project/openvswitch/list/?series=342885
> =*
> 
> Thanks,
> Eli
> 
> >-Original Message-
> >From: Simon Horman 
> >Sent: Thursday, 30 March 2023 14:21
> >To: d...@openvswitch.org
> >Cc: Ilya Maximets ; Eli Britstein
> >; Chaoyong He ; Peng
> Zhang
> >; Jin Liu ; oss-
> >driv...@corigine.com
> >Subject: [PATCH dpdk-latest v3 0/6] Add support for DPDK meter HW
> >offload
> >
> >External email: Use caution opening links or attachments
> >
> >
> >Hi,
> >
> >this series adds support for DPDK meter HW offload.
> >
> >Changes between v2 and v3.
> >* Use common API for DPDK and non-DPDK meter offloads
> >* Make use of netdev_ports_traverse to offload the meter
> >* Add dpdk-latest to subject prefix
> >
> >Changes between v1 and v2:
> >* Add the prox mechanism: add the meter by proxy id
> >* Change the offload interface from netdev-dpdk to the netdev-offload
> >* Changed base to dpdk-latest branch
> >
> >Peng Zhang (6):
> >  netdev-offload-dpdk: use flow transfer proxy
> >  netdev-offload: Let meter offload API can be used with DPDK
> >  dpif-netdev: Offloading meter with DPDK
> >  netdev-offload-dpdk: Implement meter offload API for DPDK
> >  netdev-dpdk: add meter algorithms
> >  netdev-dpdk-offload: Add support for meter action
> >
> > Documentation/howto/dpdk.rst  |   5 +-
> > lib/dpif-netdev.c |  22 ++-
> > lib/netdev-dpdk.c | 306 +-
> > lib/netdev-dpdk.h |  43 +
> > lib/netdev-offload-dpdk.c |  97 +++
> > lib/netdev-offload-provider.h |  21 ++-
> > lib/netdev-offload-tc.c   |   9 +-
> > lib/netdev-offload.c  | 135 ++-
> > lib/netdev-offload.h  |   9 +
> > 9 files changed, 633 insertions(+), 14 deletions(-)
> >
> >--
> >2.30.2

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


[ovs-dev] [PATCH v2] conntrack-tp: Fix clang warning.

2023-03-30 Thread miterv
From: Lin Huang 

Declaration of 'struct conn' will not be visible outside of this function.
Declaration of 'struct conntrack' will not be visible outside of this function.
Declaration of 'struct timeout_policy' will not be visible outside of this 
function.
---
 lib/conntrack-tp.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/lib/conntrack-tp.h b/lib/conntrack-tp.h
index 4d411d19f..7ece2eae2 100644
--- a/lib/conntrack-tp.h
+++ b/lib/conntrack-tp.h
@@ -17,8 +17,15 @@
 #ifndef CONNTRACK_TP_H
 #define CONNTRACK_TP_H 1
 
+#include 
+
 #define CT_DPIF_NETDEV_TP_MIN 30
+
 enum ct_timeout;
+struct conn;
+struct conntrack;
+struct timeout_policy;
+
 void timeout_policy_init(struct conntrack *ct);
 int timeout_policy_update(struct conntrack *ct, struct timeout_policy *tp);
 int timeout_policy_delete(struct conntrack *ct, uint32_t tp_id);
-- 
2.32.0.windows.2

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


Re: [ovs-dev] [PATCH] ovsdb: add unixctl cmd to show memory-trim-on-compaction setting

2023-03-30 Thread Vladislav Odintsov

> On 21 Mar 2023, at 17:01, Ilya Maximets  wrote:
> 
> On 3/20/23 13:51, Vladislav Odintsov wrote:
>> No, my usecase with 2.17 is that I just want to check agains a running 
>> process wether memory compaction is enabled or not without searching 
>> specific line in logs, which in addition can be rotated.
> 
> OK, but can you just set it without checking?  I mean, it will be the
> same number of interactions, or less if the status needs changing.

I just wanted to have a way to get this value automatically.
Actually, I did this for my local build and afther I’ve just upgraded to 3.1.
If you mean community doesn’t need this - I’m okay with that ;)

> 
>> 
>>> On 20 Mar 2023, at 15:27, Ilya Maximets  wrote:
>>> 
>>> On 3/20/23 13:20, Vladislav Odintsov wrote:
 Hi Ilya,
 
 Ah, I didn’t see that its default was changed in newer versions...
 We run 2.17 where it’s off by default and I decided to submit a patch 
 which is useful for that version.
 Anyway my opinion is if user has an ability to change the setting, it 
 should be possible to get its actual value. At least in versions were it’s 
 off by default.
>>> 
>>> We also have this:
>>> https://patchwork.ozlabs.org/project/openvswitch/patch/397efb8df22b13c19c326a3eecb48ada93c83420.ca...@redhat.com/
>>> 
>>> This one we can actually treat as a bug fix and backport down to 2.17.
>>> In case the logging is what is bothering you.  I know that ovn-k8s,
>>> for example, is calling this appctl frequently littering the log.
>>> The change can fix at least that.
>>> 
>>> Best regards, Ilya Maximets.
>>> 
 
> On 20 Mar 2023, at 15:07, Ilya Maximets  wrote:
> 
> On 3/20/23 10:24, Vladislav Odintsov wrote:
>> In commit [1] there was added support to enable memory trimming on OVSDB 
>> tlog
>> compation.  However there was no option to get current setting value 
>> except
>> log parsing.  This patch adds new unixctl command
>> 'ovsdb-server/show-memory-trim-on-compaction' to print current setting 
>> value.
>> 
>> 1: 
>> https://github.com/openvswitch/ovs/commit/f38f98a2c0dd7fcaf20fbe11d1e67a9b2afc0b2a
>> 
>> Signed-off-by: Vladislav Odintsov 
>> ---
>> NEWS |  4 
>> ovsdb/ovsdb-server.c | 28 
>> 2 files changed, 32 insertions(+)
>> 
>> diff --git a/NEWS b/NEWS
>> index 72b9024e6..8515f4aaa 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -17,6 +17,10 @@ Post-v3.1.0
>>in order to create OVSDB sockets with access mode of 0770.
>>- QoS:
>>  * Added new configuration option 'jitter' for a linux-netem QoS 
>> type.
>> +  - OVSDB:
>> + * New unixctl command 
>> 'ovsdb-server/show-memory-trim-on-compaction'.
>> +   This command shows current value for memory trimming setting for
>> +   OVSDB server.
> 
> Hi, Vladislav.  The memory trimming is enabled by default since 3.0.
> And I'm not aware of any cases where it is beneficial to disable it.
> The current appctl call was mostly kept for backward compatibility
> and should be deprecated and removed in one of the future releases.
> So, I'm not sure if we actually need to add a new knob for status
> checking, unless you actually want to disable the trimming.
> 
> What do you think?
> 
> Best regards, Ilya Maximets.
 
 
 Regards,
 Vladislav Odintsov
 
>>> 
>> 
>> 
>> Regards,
>> Vladislav Odintsov
>> 
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Regards,
Vladislav Odintsov

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


Re: [ovs-dev] [PATCH v4] db-ctl-base: Partially revert b8bf410a5

2023-03-30 Thread Ilya Maximets
On 3/30/23 10:10, Dumitru Ceara wrote:
> On 3/29/23 22:26, dalva...@redhat.com wrote:
>> From: Daniel Alvarez Sanchez 
>>
>> The commit b8bf410a5 [0] broke the `ovs-vsctl add` command
>> which now overwrites the value if it existed already.
>>
>> This patch reverts the code around the `cmd_add` function
>> to restore the previous behavior. It also adds testing coverage
>> for this functionality.
>>
>> [0] 
>> https://github.com/openvswitch/ovs/commit/b8bf410a5c94173da02279b369d75875c4035959
> 
> Hi Daniel,
> 
> Thanks for the fix!  Sorry for missing this at review last time.
> 
>>
>> Fixes: b8bf410a5 ("db-ctl-base: Use partial map/set updates for last add/set 
>> commands")
> 
> The "Fixes" tag can be corrected by Ilya at apply time, I guess.
> 
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2182767
>> Signed-off-by: Daniel Alvarez Sanchez 
> 
> Looks good to me, thanks!
> 
> Acked-by: Dumitru Ceara 

Thanks!  I fixed the tag and applied the change.
Also backported to 3.1.

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


Re: [ovs-dev] [PATCH 1/1] vswitch.xml: Add description of SRv6 tunnel and related options.

2023-03-30 Thread Ilya Maximets
On 3/30/23 09:29, Nobuhiro MIKI wrote:
> The description of SRv6 was missing in vswitch.xml, which is
> used to generate the man page, so this patch adds it.
> 
> Fixes: 03fc1ad78521 ("userspace: Add SRv6 tunnel support.")
> Signed-off-by: Nobuhiro MIKI 
> ---
>  vswitchd/vswitch.xml | 39 ---
>  1 file changed, 32 insertions(+), 7 deletions(-)
> 
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 88e2c94e2f0e..19b79ddb5d48 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -2845,6 +2845,16 @@
>  
>
>  
> +  SRv6

I converted this to lowercase because 'type' is case-sensitive
(capital 'B' in 'Bareudp' is a mistake..) and applied the patch.
Thanks!

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


Re: [ovs-dev] [PATCH ovn] controller: Clear tunnels from old integration bridge

2023-03-30 Thread Ihar Hrachyshka
On Thu, Mar 30, 2023 at 3:53 AM Ales Musil  wrote:
>
> After integration bridge change the tunnels would
> stay on the old bridge preventing new tunnels creation
> and disrupting traffic. Detect the bridge change
> and clear the tunnels from the old integration bridge.
>
> Reported-at: https://bugzilla.redhat.com/2173635
> Signed-off-by: Ales Musil 
> ---
>  controller/encaps.c | 36 +++-
>  controller/encaps.h |  4 +-
>  controller/ovn-controller.c | 26 +++-
>  tests/ovn.at| 83 +
>  4 files changed, 145 insertions(+), 4 deletions(-)
>
> diff --git a/controller/encaps.c b/controller/encaps.c
> index 2662eaf98..c66743d3b 100644
> --- a/controller/encaps.c
> +++ b/controller/encaps.c
> @@ -386,6 +386,20 @@ chassis_tzones_overlap(const struct sset 
> *transport_zones,
>  return false;
>  }
>
> +static void
> +clear_old_tunnels(const struct ovsrec_bridge *old_br_int)
> +{
> +for (size_t i = 0; i < old_br_int->n_ports; i++) {
> +const struct ovsrec_port *port = old_br_int->ports[i];
> +const char *id = smap_get(>external_ids, "ovn-chassis-id");
> +if (id) {
> +VLOG_DBG("Clearing old tunnel port \"%s\" (%s) from bridge "
> + "\"%s\".", port->name, id, old_br_int->name);
> +ovsrec_bridge_update_ports_delvalue(old_br_int, port);
> +}
> +}
> +}
> +
>  void
>  encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
> const struct ovsrec_bridge *br_int,
> @@ -393,12 +407,32 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
> const struct sbrec_chassis *this_chassis,
> const struct sbrec_sb_global *sbg,
> const struct ovsrec_open_vswitch_table *ovs_table,
> -   const struct sset *transport_zones)
> +   const struct sset *transport_zones,
> +   const struct ovsrec_bridge_table *bridge_table,
> +   const char *br_int_name)
>  {
>  if (!ovs_idl_txn || !br_int) {
>  return;
>  }
>
> +if (!br_int_name) {
> +/* The controller has just started, we need to look through all
> + * bridges for old tunnel ports. */
> +const struct ovsrec_bridge *br;
> +OVSREC_BRIDGE_TABLE_FOR_EACH (br, bridge_table) {
> +if (!strcmp(br->name, br_int->name)) {
> +continue;
> +}
> +clear_old_tunnels(br);
> +}
> +} else if (strcmp(br_int_name, br_int->name)) {
> +/* The integration bridge was changed, clear tunnel ports from
> + * the old one. */
> +const struct ovsrec_bridge *old_br_int =
> +get_bridge(bridge_table, br_int_name);
> +clear_old_tunnels(old_br_int);

When two ovn-controllers are running on the same host, won't this
remove tunnels that belong to the other ovn-controller (which are part
of the other controller' bridge)? I think you should only remove
tunnels that match the names as constructed by tunnel_create_name()
[the names should include the so-called unique "chassis index"].


> +}
> +
>  const struct sbrec_chassis *chassis_rec;
>
>  struct tunnel_ctx tc = {
> diff --git a/controller/encaps.h b/controller/encaps.h
> index 867c6f28c..cf38dac1a 100644
> --- a/controller/encaps.h
> +++ b/controller/encaps.h
> @@ -35,7 +35,9 @@ void encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
>  const struct sbrec_chassis *,
>  const struct sbrec_sb_global *,
>  const struct ovsrec_open_vswitch_table *,
> -const struct sset *transport_zones);
> +const struct sset *transport_zones,
> +const struct ovsrec_bridge_table *bridge_table,
> +const char *br_int_name);
>
>  bool encaps_cleanup(struct ovsdb_idl_txn *ovs_idl_txn,
>  const struct ovsrec_bridge *br_int);
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 76be2426e..242d93823 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -536,6 +536,21 @@ process_br_int(struct ovsdb_idl_txn *ovs_idl_txn,
>  *br_int_ = br_int;
>  }
>
> +static void
> +consider_br_int_change(const struct ovsrec_bridge *br_int, char 
> **current_name)
> +{
> +ovs_assert(current_name);
> +
> +if (!*current_name) {
> +*current_name = xstrdup(br_int->name);
> +}
> +
> +if (strcmp(*current_name, br_int->name)) {
> +free(*current_name);
> +*current_name = xstrdup(br_int->name);
> +}
> +}
> +
>  static void
>  update_ssl_config(const struct ovsrec_ssl_table *ssl_table)
>  {
> @@ -4918,6 +4933,8 @@ main(int argc, char *argv[])
>  char *ovn_version = ovn_get_internal_version();
>  VLOG_INFO("OVN internal version is : [%s]", ovn_version);
>
> +char *current_br_int_name = NULL;
> +
>  /* Main loop. */
>  exiting = false;
>  restart = false;
> @@ -5070,7 +5087,9 @@ 

[ovs-dev] [ovn] ha-chassis-group false positive failover

2023-03-30 Thread Vladislav Odintsov
Hi all,

I’m facing a false positive failover in the event of openflow connection got 
dropped after inactivity probe timeout due to high load of ovn-controller 
handling huge amount of ovsdb updates.
I wonder wether its a bug or I have to tune any specifics. See ovn-controller 
logs:

2023-03-30T07:57:24.469Z|58440|inc_proc_eng|INFO|node: logical_flow_output, 
recompute (failed handler for input port_groups) took 35774ms
2023-03-30T07:57:28.521Z|58441|lflow_cache|INFO|Detected cache inactivity (last 
active 40006 ms ago): trimming cache
2023-03-30T07:57:28.528Z|58442|timeval|WARN|Unreasonably long 40084ms poll 
interval (32947ms user, 6968ms system)
2023-03-30T07:57:28.528Z|58443|timeval|WARN|faults: 43468 minor, 0 major
…..
2023-03-30T07:57:48.784Z|58496|reconnect|ERR|ssl::6642: no response to 
inactivity probe after 60.2 seconds, disconnecting
2023-03-30T07:57:48.784Z|58497|reconnect|INFO|ssl::6642: connection dropped
2023-03-30T07:57:48.905Z|58498|main|INFO|OVNSB commit failed, force recompute 
next time.
2023-03-30T07:57:49.786Z|58499|reconnect|INFO|ssl::6642: connecting...
2023-03-30T07:57:49.924Z|58500|reconnect|INFO|ssl::6642: connected
2023-03-30T07:57:29.781Z|58501|poll_loop|INFO|wakeup due to 0-ms timeout at 
lib/stream-ssl.c:838 (74% CPU usage)
2023-03-30T07:57:29.836Z|58502|poll_loop|INFO|wakeup due to 0-ms timeout at 
lib/stream-ssl.c:838 (74% CPU usage)
….
2023-03-30T07:57:05.976Z|58545|poll_loop|INFO|wakeup due to 0-ms timeout at 
lib/stream-ssl.c:838 (101% CPU usage)
2023-03-30T07:57:07.002Z|58546|memory|INFO|peak resident set size grew 54% in 
last 682257.2 seconds, from 4435696 kB to 6824820 kB
2023-03-30T07:57:07.002Z|58547|memory|INFO|idl-cells-OVN_Southbound:11491359 
idl-cells-Open_vSwitch:14416 lflow-cache-entries-cache-expr:530298 
lflow-cache-entries-cache-matches:31770 lflow-cache-size-KB:769553 
local_datapath_usage-KB:630
 ofctrl_desired_flow_usage-KB:577052 ofctrl_installed_flow_usage-KB:409538 
ofctrl_sb_flow_ref_usage-KB:273707
…
2023-03-30T07:57:31.667Z|58552|rconn|WARN|unix:/run/openvswitch/br-int.mgmt: 
connection dropped (Broken pipe)
2023-03-30T07:57:31.739Z|58553|binding|INFO|Releasing lport cr-xxx from this 
chassis (sb_readonly=0)
2023-03-30T07:57:31.739Z|58554|if_status|WARN|Dropped 1 log messages in last 
1254725 seconds (most recently, 1254725 seconds ago) due to excessive rate
2023-03-30T07:57:31.739Z|58555|if_status|WARN|Trying to release unknown 
interface cr-xxx
2023-03-30T07:57:31.743Z|58556|binding|INFO|Releasing lport cr-yyy from this 
chassis (sb_readonly=0)
2023-03-30T07:57:31.743Z|58557|binding|INFO|Releasing lport cr-zzz from this 
chassis (sb_readonly=0)
…

After some time these ports are claimed back:

2023-03-30T07:57:49.320Z|59137|rconn|INFO|unix:/run/openvswitch/br-int.mgmt: 
connecting...
2023-03-30T07:57:49.320Z|59138|rconn|INFO|unix:/run/openvswitch/br-int.mgmt: 
connected
2023-03-30T07:57:49.523Z|59139|poll_loop|INFO|wakeup due to 0-ms timeout at 
controller/ovn-controller.c:4296 (99% CPU usage)
2023-03-30T07:57:49.574Z|59140|poll_loop|INFO|wakeup due to 0-ms timeout at 
controller/ovn-controller.c:4286 (99% CPU usage)
2023-03-30T07:58:01.451Z|59141|poll_loop|INFO|wakeup due to [POLLIN] on fd 22 
(:34736<->:6642) at lib/stream-ssl.c:836 (99% CPU usage)
2023-03-30T07:58:02.449Z|59142|binding|INFO|Claiming lport cr-xxx for this 
chassis.
2023-03-30T07:58:02.449Z|59143|binding|INFO|cr-xxx: Claiming xx:xx:xx:xx:xx 
x.x.x.x/xx
2023-03-30T07:58:02.453Z|59144|binding|INFO|Claiming lport cr-yyy for this 
chassis.
2023-03-30T07:58:02.453Z|59145|binding|INFO|cr-yyy: Claiming yy:yy:yy:yy:yy:yy 
y.y.y.y/yy
…

Previously I’ve met with similar problem but the openflow probe interval was 30 
seconds and I set it to 60 and then to 120 seconds.
But it seems that OVS was configured on default inactivity probe timeout for 
openflow - 60 seconds. So, the error above occured. In OVS logs it looks like 
next:

2023-03-30T07:57:28.632Z|61003|rconn|ERR|br-int<->unix#8: no response to 
inactivity probe after 60 seconds, disconnecting

This node is a dedicated l3gateway which has is-interconn=true setting 
(interconnection case) and has quite big openflow table around 2.7M flows.

So, I’ve got some questions for OVN and OVS:
1. I couldn’t find in ovn-controller code where the logic of handling openflow 
connection drop. It is a bug when after openflow connection drop the claimed 
cr-ports are being released? I guess, yes. Can you please point me to the 
corrent place of code to start investigating?
2. I see almost the same openflow amount on each l3gateway with interconnection 
role. Even l3gateways which have no allocated cr-port-bindings have 2M+ 
openflow flows. Is it normal operation or it’s a place for possible 
optimisation? I mean it seems that excess flows are installed.
3. How to handle long OVS OF updates, which block inactivity probe for OF 
connection and then got dropped? From ovn-controller it’s possible to configure 
inactivity 

Re: [ovs-dev] [PATCH] seq: Make read of the current value atomic

2023-03-30 Thread Jon Kohler


> On Mar 30, 2023, at 11:57 AM, Jon Kohler  wrote:
> 
> 
> 
>> On Mar 28, 2023, at 1:32 PM, Mike Pattrick  wrote:
>> 
>> On Mon, Mar 27, 2023 at 7:25 AM Eelco Chaudron  wrote:
>>> 
>>> Make the read of the current seq->value atomic, i.e., not needing to
>>> acquire the global mutex when reading it. On 64-bit systems, this
>>> incurs no overhead, and it will avoid the mutex and potentially
>>> a system call.
>>> 
>>> For incrementing the value followed by waking up the threads, we are
>>> still taking the mutex, so the current behavior is not changing. The
>>> seq_read() behavior is already defined as, "Returns seq's current
>>> sequence number (which could change immediately)". So the change
>>> should not impact the current behavior.
>>> 
>>> Signed-off-by: Eelco Chaudron 
>>> ---
>>> lib/ovs-rcu.c |2 +-
>>> lib/seq.c |   37 -
>>> lib/seq.h |1 -
>>> 3 files changed, 17 insertions(+), 23 deletions(-)
>>> 
>>> diff --git a/lib/ovs-rcu.c b/lib/ovs-rcu.c
>>> index 946aa04d1..9e07d9bab 100644
>>> --- a/lib/ovs-rcu.c
>>> +++ b/lib/ovs-rcu.c
>>> @@ -170,7 +170,7 @@ ovsrcu_try_quiesce(void)
>>>ovs_assert(!single_threaded());
>>>perthread = ovsrcu_perthread_get();
>>>if (!seq_try_lock()) {
>>> -perthread->seqno = seq_read_protected(global_seqno);
>>> +perthread->seqno = seq_read(global_seqno);
>>>if (perthread->cbset) {
>>>ovsrcu_flush_cbset__(perthread, true);
>>>}
>>> diff --git a/lib/seq.c b/lib/seq.c
>>> index 99e5bf8bd..22646f3f8 100644
>>> --- a/lib/seq.c
>>> +++ b/lib/seq.c
>>> @@ -32,7 +32,7 @@ COVERAGE_DEFINE(seq_change);
>>> 
>>> /* A sequence number object. */
>>> struct seq {
>>> -uint64_t value OVS_GUARDED;
>>> +atomic_uint64_t value;
>>>struct hmap waiters OVS_GUARDED; /* Contains 'struct seq_waiter's. */
>>> };
>>> 
>>> @@ -72,6 +72,7 @@ static void seq_wake_waiters(struct seq *) 
>>> OVS_REQUIRES(seq_mutex);
>>> struct seq * OVS_EXCLUDED(seq_mutex)
>>> seq_create(void)
>>> {
>>> +uint64_t seq_value;
>>>struct seq *seq;
>>> 
>>>seq_init();
>>> @@ -81,7 +82,8 @@ seq_create(void)
>>>COVERAGE_INC(seq_change);
>>> 
>>>ovs_mutex_lock(_mutex);
>> 
>> I think it's also possible to get rid of the mutex here. Operations
>> like modifying a bridge or interface can result in calling seq_create
>> dozens of times, which could potentially lead to contention in cases
>> when a lot of interfaces are constantly added or removed. I'm mainly
>> thinking about kubernetes here.
>> 
>> Other than that, I found this patch to reduce locking on seq_mutex
>> pretty significantly with a userspace workload:
>> 
>> Before: 468727.2/sec
>> After: 229265.8/sec
>> 
>> The rest of this patch looks good to me, but what do you think about:
>> 
>> diff --git a/lib/seq.c b/lib/seq.c
>> index 22646f3f8..09fdccce5 100644
>> --- a/lib/seq.c
>> +++ b/lib/seq.c
>> @@ -57,7 +57,7 @@ struct seq_thread {
>> 
>> static struct ovs_mutex seq_mutex = OVS_MUTEX_INITIALIZER;
>> 
>> -static uint64_t seq_next OVS_GUARDED_BY(seq_mutex) = 1;
>> +static atomic_uint64_t seq_next = 1;
>> 
>> static pthread_key_t seq_thread_key;
>> 
>> @@ -81,11 +81,9 @@ seq_create(void)
>> 
>>COVERAGE_INC(seq_change);
>> 
>> -ovs_mutex_lock(_mutex);
>> -seq_value = seq_next++;
>> +seq_value = atomic_count_inc64(_next);
>>atomic_store_relaxed(>value, seq_value);
>>hmap_init(>waiters);
>> -ovs_mutex_unlock(_mutex);
>> 
>>return seq;
>> }
>> @@ -128,7 +126,7 @@ void
>> seq_change_protected(struct seq *seq)
>>OVS_REQUIRES(seq_mutex)
>> {
>> -uint64_t seq_value = seq_next++;
>> +uint64_t seq_value = atomic_count_inc64(_next);
>> 
>>COVERAGE_INC(seq_change);
>> 
> 
> Mike - Seems like a good idea at first blush?
> 
> Eelco, 
> This patch has perfect timing, as we’re currently chasing down some
> overhead on large systems, e.g. quad socket or newer chips with many
> dozens/hundreds of processors per system. In these systems, we see
> a lot of overhead from the revalidator threads coming online and
> constantly competing for the mutex in seq_read(), seq_woke(), and
> seq_change(). This leads to ovs-vswitchd being on-CPU constantly at
> somewhere between 20-80% of one core as per top. This time comes from
> spending a ton of time contending for the lock, then just futex'ing in
> and out of sleep as a result.
> 
> Along these same lines of thinking, would it be possible to get rid of
> this mutex in seq_change() too? Perhaps we could have some atomic val
> that indicated if there was a waiter, and if so, then take the mutex
> to deal with the wakeup, but if not, don't take the mutex at all?
> 
> Thats just trying to further optimize, even with this patch there is
> gains by easing contention on the mutex from all of the read calls, so
> great work!
> 
> While I'm on the thought: The next point of contention specifically in
> the revalidator case that I'm tracking is under nl_dump_next() where
> 

Re: [ovs-dev] [PATCH v12 0/5] userspace: Add SRv6 tunnel support.

2023-03-30 Thread Ilya Maximets
On 3/30/23 04:23, Nobuhiro MIKI wrote:
> On 2023/03/30 6:17, Ilya Maximets wrote:
>> On 3/29/23 07:51, Nobuhiro MIKI wrote:
>>> v12:
>>> * Fix nw_proto after encap.
>>> * Fix to check nw_proto in system-traffic.at.
>>> * Remove OVS_UNUSED annotation.
>>> v11:
>>> * Fix comments.
>>> * Clean up conditional statements.
>>>   ("!rt_hdr || rt_hdr->type != IPV6_SRCRT_TYPE_4").
>>> * Remove variables from function prototype.
>>> * Define IPPROTO_IPIP for sparse.
>>> v10:
>>> * Clean up tnl_type_to_nw_proto().
>>> * Support frag_hdr=NULL and/or rt_hdr=NULL in parse_ipv6_ext_hdrs().
>>> * Clean up srv6_build_header() to use netdev_tnl_ip_build_header().
>>> * Fix to avoid using sizeof() style.
>>> * Add validation that checks segs[0] == params->flow->tunnel.ipv6_dst.
>>> * Add more tests for tests/odp.at.
>>> * Enhance validation for external input to prevent over-writing
>>> v9:
>>> * Fix compile warnings
>>> v8:
>>> * Split the patch into multiple patches.
>>> * Fix docs and NEWS to point to version 3.2.
>>> * Move tests from tests/system-userspace-traffic.at
>>>   to tests/system-traffic.at.
>>> v7:
>>> * fix flake8 error
>>> v6:
>>> * add tests that show interoperability between OVS and native kernel's
>>>   implementation in tests/system-userspace-traffic.at.
>>> * fix the documentation.
>>> * add validation in routing header by parse_ipv6_ext_hdrs.
>>> * add parsing implementation and test in tests/odp.at,
>>>   python/ovs/flow/odp.py and python/ovs/tests/test_odp.py.
>>> * fix coding style.
>>> * add build-time assertion on the structure size.
>>> v5:
>>> * rebased on latest master
>>> v4:
>>> * fix alignment on cast
>>> v3:
>>> * fix alignment on cast
>>> v2:
>>> * fix pointer arithmetic
>>>
>>> Nobuhiro MIKI (5):
>>>   tests: Define new ADD_VETH_NS macro.
>>>   tnl-ports: Support multiple nw_protos.
>>>   flow: Support rt_hdr in parse_ipv6_ext_hdrs().
>>>   userspace: Add SRv6 tunnel support.
>>>   odp: Add SRv6 tunnel actions.
>>>
>>>  Documentation/faq/configuration.rst |  21 +
>>>  Documentation/faq/releases.rst  |   1 +
>>>  NEWS|   2 +
>>>  include/linux/openvswitch.h |   1 +
>>>  include/sparse/netinet/in.h |   1 +
>>>  lib/conntrack.c |   4 +-
>>>  lib/dpif-netlink-rtnl.c |   5 ++
>>>  lib/dpif-netlink.c  |   5 ++
>>>  lib/flow.c  |  48 +++---
>>>  lib/flow.h  |   3 +-
>>>  lib/ipf.c   |  15 ++--
>>>  lib/netdev-native-tnl.c | 130 
>>>  lib/netdev-native-tnl.h |  10 +++
>>>  lib/netdev-vport.c  |  53 
>>>  lib/netdev.h|   4 +
>>>  lib/odp-util.c  |  70 +++
>>>  lib/packets.h   |  20 +
>>>  lib/tnl-ports.c |  85 +++---
>>>  ofproto/ofproto-dpif-xlate.c|   4 +
>>>  python/ovs/flow/odp.py  |   8 ++
>>>  python/ovs/tests/test_odp.py|  16 
>>>  tests/odp.at|  12 ++-
>>>  tests/system-common-macros.at   |  16 
>>>  tests/system-kmod-macros.at |   8 ++
>>>  tests/system-traffic.at | 124 ++
>>>  tests/system-userspace-macros.at|   6 ++
>>>  tests/tunnel-push-pop-ipv6.at   |  23 +
>>>  tests/tunnel.at |  56 
>>>  28 files changed, 695 insertions(+), 56 deletions(-)
>>>
>>
>> I added back the definition of IPPROTO_IPIP as it turned out that
>> MSVC doesn't have it too.  With that and a minor tweak of the NEWS
>> entry, I applied the set.
>>
> 
> I think it was a complicated patch set.
> Thanks so much for your kind cooperation!

Thanks for your contribution!
Was a good learning exercise for me, as I learned a few
things about SRv6. :)

> 
>> I noticed though that we're missing an update for the vswitch.xml
>> file.  I didn't think that should hold the whole set, but could
>> you please prepare an update as a separate patch?  You may follow
>> the changes in ebe0e518b048 ("tunnel: Bareudp Tunnel Support.") as
>> an example.
>>
> 
> Of course. I'll prepare a separate patch to update vswitch.xml.

Thanks!

> 
> Best Regards,
> Nobuhiro MIKI

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


Re: [ovs-dev] [PATCH] conntrack-tp: Fix clang warning.

2023-03-30 Thread Ilya Maximets
On 3/30/23 16:16, mit...@outlook.com wrote:
> From: Lin Huang 
> 
> Declaration of 'struct conn' will not be visible outside of this function.
> Declaration of 'struct conntrack' will not be visible outside of this 
> function.
> Declaration of 'struct timeout_policy' will not be visible outside of this 
> function.
> 
> Signed-off-by: Lin Huang 
> ---
>  lib/conntrack-tp.h | 5 +
>  1 file changed, 5 insertions(+)
> 

I can't reproduce the warning on a current master, but the change
looks correct to me.  Would be nice to add some empty lines to
the file though to visually split macros, structs and functions.

> diff --git a/lib/conntrack-tp.h b/lib/conntrack-tp.h
> index 4d411d19f..2227f6fa5 100644
> --- a/lib/conntrack-tp.h
> +++ b/lib/conntrack-tp.h
> @@ -17,8 +17,13 @@
>  #ifndef CONNTRACK_TP_H
>  #define CONNTRACK_TP_H 1
>  
> +#include 
> +
>  #define CT_DPIF_NETDEV_TP_MIN 30

An empty line here.

>  enum ct_timeout;
> +struct conn;
> +struct conntrack;
> +struct timeout_policy;

And here.

>  void timeout_policy_init(struct conntrack *ct);
>  int timeout_policy_update(struct conntrack *ct, struct timeout_policy *tp);
>  int timeout_policy_delete(struct conntrack *ct, uint32_t tp_id);

If you can add these empty lines and send v2, that would be great.
Thanks!

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


Re: [ovs-dev] [PATCH] util: fix an issue that thread name cannot be set

2023-03-30 Thread Ilya Maximets
On 3/30/23 04:17, Songtao Zhan wrote:
> 
> To: d...@openvswitch.org,
> i.maxim...@ovn.org
> 
> The name of the current thread consists of a name with a maximum
> length of 16 bytes and a thread ID. The final name may be longer
> than 16 bytes. If the name is longer than 16 bytes, the thread
> name will fail to be set
> 
> Signed-off-by: Songtao Zhan 

Looks reasonable to me, thanks!  See some minor comments below.

Best regards, Ilya Maximets.

> ---
>  lib/util.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lib/util.c b/lib/util.c
> index 96a71550d..023829694 100644
> --- a/lib/util.c
> +++ b/lib/util.c
> @@ -645,6 +645,10 @@ set_subprogram_name(const char *subprogram_name)
>  free(subprogram_name_set(pname));
> 
>  #if HAVE_GLIBC_PTHREAD_SETNAME_NP
> +/* The maximum thead name including '\0' supported by the system is 16 */

I'd remove 'by the system' from the sentence above.
And the sentence should end with a period.

> +if (strlen(pname) > 15) {
> +pname[15] = '\0' ;

No need for the extra space before ';'.

> +}
>  pthread_setname_np(pthread_self(), pname);
>  #elif HAVE_NETBSD_PTHREAD_SETNAME_NP
>  pthread_setname_np(pthread_self(), "%s", pname);

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


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

2023-03-30 Thread Ilya Maximets
On 3/30/23 11:45, Simon Horman wrote:
> On Fri, Mar 17, 2023 at 09:47:36PM +0100, Eelco Chaudron wrote:
>>
>>
>> Send from my phone
>>
>>> Op 17 mrt. 2023 om 21:11 heeft Marcelo Ricardo Leitner 
>>>  het volgende geschreven:
>>>
>>> On Thu, Mar 16, 2023 at 09:51:34AM +0100, Eelco Chaudron wrote:


> On 22 Dec 2022, at 13:32, Eelco Chaudron wrote:
>
>> On 22 Dec 2022, at 10:26, Balazs Nemeth wrote:
>
>> The only way that stats->{n_packets,n_bytes} would decrease is due to an
> 
> nit: s/way/ways/
> 
>> overflow, or if there are bugs in how statistics are handled. In the
>> past, there were multiple bugs 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.
> 
> 1. Perhaps it would be prudent to log/count if/when this occurs

+1
We do have a coverage counter that will indicate the case where stats
jump back.  However, if we're certain that this should never happen,
we should, probably, emit a warning or even an error log as well, so
users are aware that something went wrong.

> 2. I take it that the overflow handling would be follow-up work,
>is that correct?

The unsigned arithmetic should take case of overflowed counters,
because the result of subtraction will still give a correct difference
between the old and a new value, even if it overflowed and the new
value is smaller.  Unless, of course, it overflowed more than once.

> 
>> Signed-off-by: Balazs Nemeth 
>> ---
>>
>> Depends on:
>> - netdev-offload-tc: Preserve tc statistics when flow gets modified.
>> - tc: Add support for TCA_STATS_PKT64
>> - ofproto-dpif-upcall: Wait for valid hw flow stats before applying 
>> min-revalidate-pps
>> - ofproto-dpif-upcall: Use last known stats ukey stats on revalidate 
>> missed dp flows.
>
> Did a lot of tests with this patch applied, but as mentioned it does need 
> all four patches mentioned above applied first.
>
> Acked-by: Eelco Chaudron 

 Now that all the dependent patches have been applied and backported to 
 2.17, can we have another look at this patch?
>>>
>>> Not sure what you mean here, Eelco. You mean, just a patch review or
>>> you mean something else, like evaluating if the patch is still needed
>>> at all?
>>
>> Guess I was not clear enough :) what I meant was for the maintainers to
>> have another look at this patch and if no further objections apply it.
> 
> There seems to now be minor fuzz when applying this patch (locally).
> Perhaps a rebase is in order?

Yeah, the coverage counter got introduced in a close proximity.

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


Re: [ovs-dev] [RFC ovn 0/3] rework OVN QoS implementation

2023-03-30 Thread Rodolfo Alonso Hernandez
Hello:

In OpenStack Neutron we are only using the "qos_min_rate" setting in the
LSP (for max rate and DSCP we use QoS registers), only for egress traffic.
The way it is supposed to work is the TC rules should be applied on the
egress interface. In the case of VLAN or flat networks (not tunneled), the
physical bridge will have an interface; this interface should receive the
TC rules.

When testing the current code (not the new patch series) using OpenStack,
we deployed the environment with two physical bridges connected to two
physical networks. Then we created two ports, one per network. In OpenStack
Neutron we are only using the "qos_min_rate" setting. *Each time a QoS was
defined for a LSP, both physical bridges external interfaces were receiving
the TC configuration, instead of only the physical interface of the
network.*

NOTE: in Neutron we are only defining the "qos_min_rate", as commented
before. Because OVN makes a direct translation between the OVN parameters
and the TC rules, we also need to define the "qos_max_rate", same as when
executing a TC command ("rate" cannot be defined without "ceil"). But this
is something we need to fix in Neutron.

About the new series, it is difficult for Neutron to set the "qos_ovs_port"
in the LSP. Neutron is acting as a CMS and can't read the local OVS
database of a particular node. We can have access to OVN NB and SB, but
currently it is not possible to read each node's local OVS database. If
this is going to be the implementation, we'll need to refactor how "min-bw"
feature works, using some agents deployed in the nodes to retrieve this
info (NOTE: we have agents in the nodes but not for this purpose; in any
case, that can be considered).

Regards.

On Wed, Mar 29, 2023 at 11:31 PM Lorenzo Bianconi <
lorenzo.bianc...@redhat.com> wrote:

> > Hi Lorenzo et al.
>
> Hi Ihar,
>
> thx for reviewing the series.
>
> >
> > I took a look at the series and have a number of concerns. Note that I
> > looked at the series in context of OpenStack and the way Neutron uses
> > qos_ options. Some points below are not directly relevant to the
> > series but I think they add important context.
> >
> > ---
> >
> > First, I will agree with the direction of the patch - it is beneficial
> > to switch from TC to using OVS queues to configure LSP level qos_*
> > settings.
> >
> > But, see below.
> >
> > 1. The 2/3 patch explicitly claims that the qos_min_rate option is for
> > localnet ports only: "QoS is only supported on localnet port for the
> > moment". Actually, it's vice versa: the option is documented for VIF
> > (regular) ports
> > only:
> https://github.com/ovn-org/ovn/blob/1bec9e3ddd8500793b52e11c3dc1f41ef1f48591/ovn-nb.xml#L1096
> > To compare, the localnet section of the document doesn't list this
> > option:
> https://github.com/ovn-org/ovn/blob/1bec9e3ddd8500793b52e11c3dc1f41ef1f48591/ovn-nb.xml#L1034
> >
> > This assumption about the application of the options to localnet
> > ports, and not to other port types, seems to creep into the 1/3 patch
> > of the series too.
> >
> > I tried to understand where the misunderstanding comes from. Perhaps
> > it's because the "egress qos" system test was added to cover a
> > supposed I-P regression where qos is not applied to localnet ports.
> > Then later, when adding qos_min_rate option support, I patched the
> > existing test case, without realizing that it sets qos rules to
> > localnet port, not regular VIF port (as per NB documentation). We
> > should probably fix the test case (or add a new one for regular ports
> > if we decide that localnet qos is a valid scenario).
> >
> > Or perhaps it's because we call consider_localnet_lport with qos_map
> > passed, so it also configures qos queues for these ports.
> >
> > Regardless, it's clear that regular (VIF) ports should support qos_
> > options, so any changes to disable qos for them is invalid. Actually,
> > OpenStack Neutron already uses qos_min_rate option to configure its
> > minimal bandwidth guarantees, and these guarantees are set for regular
> > ports. I don't know if localnet ports should also support qos rules
> > (if so, documentation should be updated to include the options in
> > localnet section).
>
> ack, I will fix the VIF port use-case in RFC v2. It was not clear to me
> OpenStack just applies QoS on logical switch ports, thx for pointing this
> out.
>
> >
> > 2. AFAIU there's some bug in qos code where policies that are not set
> > for a port are being applied, when there are multiple localnet ports.
> > (At least that's what I figured from a discussion with Rodolfo;
> > Rodolfo, could you please describe the exact scenario that you
> > experienced?) But it's not related to issues with qos_ options set to
> > localnet ports (because Neutron doesn't set them for localnets.)
> >
> > 3. Side note: while reviewing the series, I noticed that
> > port_has_qos_params function doesn't check if qos_min_rate option is
> > set. This is not a problem of your series, but 

Re: [ovs-dev] [RFC ovn 0/3] rework OVN QoS implementation

2023-03-30 Thread Ihar Hrachyshka
On Thu, Mar 30, 2023 at 6:44 AM Rodolfo Alonso Hernandez
 wrote:
>
> Hello:
>
> In OpenStack Neutron we are only using the "qos_min_rate" setting in the LSP 
> (for max rate and DSCP we use QoS registers), only for egress traffic. The 
> way it is supposed to work is the TC rules should be applied on the egress 
> interface. In the case of VLAN or flat networks (not tunneled), the physical 
> bridge will have an interface; this interface should receive the TC rules.
>
> When testing the current code (not the new patch series) using OpenStack, we 
> deployed the environment with two physical bridges connected to two physical 
> networks. Then we created two ports, one per network. In OpenStack Neutron we 
> are only using the "qos_min_rate" setting. Each time a QoS was defined for a 
> LSP, both physical bridges external interfaces were receiving the TC 
> configuration, instead of only the physical interface of the network.
>

I think this is just the way it's implemented: every egress iface gets
the same queues configured. But these queues are tied to ports
(through unique qdisc_queue_id). It's just that, in general, some
queues on some egress interfaces won't be utilized (depending on the
logical topology).

Is it a *real* problem? Do we expect e.g. performance issues because
of unnecessary queues defined?

> NOTE: in Neutron we are only defining the "qos_min_rate", as commented 
> before. Because OVN makes a direct translation between the OVN parameters and 
> the TC rules, we also need to define the "qos_max_rate", same as when 
> executing a TC command ("rate" cannot be defined without "ceil"). But this is 
> something we need to fix in Neutron.
>
> About the new series, it is difficult for Neutron to set the "qos_ovs_port" 
> in the LSP. Neutron is acting as a CMS and can't read the local OVS database 
> of a particular node. We can have access to OVN NB and SB, but currently it 
> is not possible to read each node's local OVS database. If this is going to 
> be the implementation, we'll need to refactor how "min-bw" feature works, 
> using some agents deployed in the nodes to retrieve this info (NOTE: we have 
> agents in the nodes but not for this purpose; in any case, that can be 
> considered).
>
> Regards.
>
> On Wed, Mar 29, 2023 at 11:31 PM Lorenzo Bianconi 
>  wrote:
>>
>> > Hi Lorenzo et al.
>>
>> Hi Ihar,
>>
>> thx for reviewing the series.
>>
>> >
>> > I took a look at the series and have a number of concerns. Note that I
>> > looked at the series in context of OpenStack and the way Neutron uses
>> > qos_ options. Some points below are not directly relevant to the
>> > series but I think they add important context.
>> >
>> > ---
>> >
>> > First, I will agree with the direction of the patch - it is beneficial
>> > to switch from TC to using OVS queues to configure LSP level qos_*
>> > settings.
>> >
>> > But, see below.
>> >
>> > 1. The 2/3 patch explicitly claims that the qos_min_rate option is for
>> > localnet ports only: "QoS is only supported on localnet port for the
>> > moment". Actually, it's vice versa: the option is documented for VIF
>> > (regular) ports
>> > only:https://github.com/ovn-org/ovn/blob/1bec9e3ddd8500793b52e11c3dc1f41ef1f48591/ovn-nb.xml#L1096
>> > To compare, the localnet section of the document doesn't list this
>> > option: 
>> > https://github.com/ovn-org/ovn/blob/1bec9e3ddd8500793b52e11c3dc1f41ef1f48591/ovn-nb.xml#L1034
>> >
>> > This assumption about the application of the options to localnet
>> > ports, and not to other port types, seems to creep into the 1/3 patch
>> > of the series too.
>> >
>> > I tried to understand where the misunderstanding comes from. Perhaps
>> > it's because the "egress qos" system test was added to cover a
>> > supposed I-P regression where qos is not applied to localnet ports.
>> > Then later, when adding qos_min_rate option support, I patched the
>> > existing test case, without realizing that it sets qos rules to
>> > localnet port, not regular VIF port (as per NB documentation). We
>> > should probably fix the test case (or add a new one for regular ports
>> > if we decide that localnet qos is a valid scenario).
>> >
>> > Or perhaps it's because we call consider_localnet_lport with qos_map
>> > passed, so it also configures qos queues for these ports.
>> >
>> > Regardless, it's clear that regular (VIF) ports should support qos_
>> > options, so any changes to disable qos for them is invalid. Actually,
>> > OpenStack Neutron already uses qos_min_rate option to configure its
>> > minimal bandwidth guarantees, and these guarantees are set for regular
>> > ports. I don't know if localnet ports should also support qos rules
>> > (if so, documentation should be updated to include the options in
>> > localnet section).
>>
>> ack, I will fix the VIF port use-case in RFC v2. It was not clear to me
>> OpenStack just applies QoS on logical switch ports, thx for pointing this 
>> out.
>>
>> >
>> > 2. AFAIU there's some bug in qos code where 

Re: [ovs-dev] [PATCH] seq: Make read of the current value atomic

2023-03-30 Thread Jon Kohler


> On Mar 28, 2023, at 1:32 PM, Mike Pattrick  wrote:
> 
> On Mon, Mar 27, 2023 at 7:25 AM Eelco Chaudron  wrote:
>> 
>> Make the read of the current seq->value atomic, i.e., not needing to
>> acquire the global mutex when reading it. On 64-bit systems, this
>> incurs no overhead, and it will avoid the mutex and potentially
>> a system call.
>> 
>> For incrementing the value followed by waking up the threads, we are
>> still taking the mutex, so the current behavior is not changing. The
>> seq_read() behavior is already defined as, "Returns seq's current
>> sequence number (which could change immediately)". So the change
>> should not impact the current behavior.
>> 
>> Signed-off-by: Eelco Chaudron 
>> ---
>> lib/ovs-rcu.c |2 +-
>> lib/seq.c |   37 -
>> lib/seq.h |1 -
>> 3 files changed, 17 insertions(+), 23 deletions(-)
>> 
>> diff --git a/lib/ovs-rcu.c b/lib/ovs-rcu.c
>> index 946aa04d1..9e07d9bab 100644
>> --- a/lib/ovs-rcu.c
>> +++ b/lib/ovs-rcu.c
>> @@ -170,7 +170,7 @@ ovsrcu_try_quiesce(void)
>> ovs_assert(!single_threaded());
>> perthread = ovsrcu_perthread_get();
>> if (!seq_try_lock()) {
>> -perthread->seqno = seq_read_protected(global_seqno);
>> +perthread->seqno = seq_read(global_seqno);
>> if (perthread->cbset) {
>> ovsrcu_flush_cbset__(perthread, true);
>> }
>> diff --git a/lib/seq.c b/lib/seq.c
>> index 99e5bf8bd..22646f3f8 100644
>> --- a/lib/seq.c
>> +++ b/lib/seq.c
>> @@ -32,7 +32,7 @@ COVERAGE_DEFINE(seq_change);
>> 
>> /* A sequence number object. */
>> struct seq {
>> -uint64_t value OVS_GUARDED;
>> +atomic_uint64_t value;
>> struct hmap waiters OVS_GUARDED; /* Contains 'struct seq_waiter's. */
>> };
>> 
>> @@ -72,6 +72,7 @@ static void seq_wake_waiters(struct seq *) 
>> OVS_REQUIRES(seq_mutex);
>> struct seq * OVS_EXCLUDED(seq_mutex)
>> seq_create(void)
>> {
>> +uint64_t seq_value;
>> struct seq *seq;
>> 
>> seq_init();
>> @@ -81,7 +82,8 @@ seq_create(void)
>> COVERAGE_INC(seq_change);
>> 
>> ovs_mutex_lock(_mutex);
> 
> I think it's also possible to get rid of the mutex here. Operations
> like modifying a bridge or interface can result in calling seq_create
> dozens of times, which could potentially lead to contention in cases
> when a lot of interfaces are constantly added or removed. I'm mainly
> thinking about kubernetes here.
> 
> Other than that, I found this patch to reduce locking on seq_mutex
> pretty significantly with a userspace workload:
> 
> Before: 468727.2/sec
> After: 229265.8/sec
> 
> The rest of this patch looks good to me, but what do you think about:
> 
> diff --git a/lib/seq.c b/lib/seq.c
> index 22646f3f8..09fdccce5 100644
> --- a/lib/seq.c
> +++ b/lib/seq.c
> @@ -57,7 +57,7 @@ struct seq_thread {
> 
> static struct ovs_mutex seq_mutex = OVS_MUTEX_INITIALIZER;
> 
> -static uint64_t seq_next OVS_GUARDED_BY(seq_mutex) = 1;
> +static atomic_uint64_t seq_next = 1;
> 
> static pthread_key_t seq_thread_key;
> 
> @@ -81,11 +81,9 @@ seq_create(void)
> 
> COVERAGE_INC(seq_change);
> 
> -ovs_mutex_lock(_mutex);
> -seq_value = seq_next++;
> +seq_value = atomic_count_inc64(_next);
> atomic_store_relaxed(>value, seq_value);
> hmap_init(>waiters);
> -ovs_mutex_unlock(_mutex);
> 
> return seq;
> }
> @@ -128,7 +126,7 @@ void
> seq_change_protected(struct seq *seq)
> OVS_REQUIRES(seq_mutex)
> {
> -uint64_t seq_value = seq_next++;
> +uint64_t seq_value = atomic_count_inc64(_next);
> 
> COVERAGE_INC(seq_change);
> 

Mike - Seems like a good idea at first blush?

Eelco, 
This patch has perfect timing, as we’re currently chasing down some
overhead on large systems, e.g. quad socket or newer chips with many
dozens/hundreds of processors per system. In these systems, we see
a lot of overhead from the revalidator threads coming online and
constantly competing for the mutex in seq_read(), seq_woke(), and
seq_change(). This leads to ovs-vswitchd being on-CPU constantly at
somewhere between 20-80% of one core as per top. This time comes from
spending a ton of time contending for the lock, then just futex'ing in
and out of sleep as a result.

Along these same lines of thinking, would it be possible to get rid of
this mutex in seq_change() too? Perhaps we could have some atomic val
that indicated if there was a waiter, and if so, then take the mutex
to deal with the wakeup, but if not, don't take the mutex at all?

Thats just trying to further optimize, even with this patch there is
gains by easing contention on the mutex from all of the read calls, so
great work!

While I'm on the thought: The next point of contention specifically in
the revalidator case that I'm tracking is under nl_dump_next() where
we contend for dump->mutex just to do a short lived update to 
dump->status. Perhaps that can be a spinlock? I'll start a separate 
thread after I try that out.

Jon

> 

[ovs-dev] [PATCH v6] lib, ovsdb, ovs-vsctl, vtep-ctl: Fix multiple Coverity defects

2023-03-30 Thread James Raphael Tiovalen
This commit addresses several high and medium-impact Coverity defects by
fixing several possible null-pointer dereferences and potentially
uninitialized variables.

There were cases when crashes were encountered when some null pointers
were dereferenced. Null pointer checks and alternative code paths have
been added to prevent unwanted crashes. For impossible conditions,
non-null assertions are added. Additionally, some struct variables were
not initialized. Zero-initializations have been added to prevent
potential data leaks or undefined behavior.

Some variables would always be initialized by either the code flow or
the compiler and some pointers would never be null. Thus, some Coverity
reports might be false positives. That said, it is still considered best
practice to perform null pointer checks and initialize variables upfront
just in case to ensure the overall resilience and security of OVS, as
long as they do not impact performance-critical code. As a bonus, it
would also make static analyzer tools, such as Coverity, happy.

Unit tests have been successfully executed via `make check` and they
successfully passed.

Signed-off-by: James Raphael Tiovalen 
---
v6:
- Convert some explicit null pointer checks to assertions since they are
checking for impossible conditions.
- Use `nullable_memset()` and `nullable_memcpy()` instead of manually
performing null checks for `memset()` and `memcpy()`.

v5:
Improve commit message to better describe the intent of this patch.

v4:
- Fix some apply-robot checkpatch errors and warnings.
- Fix github-robot build failure: linux clang test asan.

v3:
Fix some apply-robot checkpatch errors and warnings.

v2:
Fix some apply-robot checkpatch errors and warnings.
---
 lib/dp-packet.c | 22 ---
 lib/dpctl.c | 16 +-
 lib/json.c  | 22 ---
 lib/latch-unix.c|  2 +-
 lib/memory.c| 12 ++-
 lib/netdev-native-tnl.c | 17 +--
 lib/odp-execute.c   |  4 
 lib/pcap-file.c |  4 +++-
 lib/rtnetlink.c |  5 +
 lib/sflow_agent.c   |  6 ++
 lib/shash.c |  5 -
 lib/sset.c  |  2 ++
 ovsdb/condition.c   | 10 -
 ovsdb/file.c| 21 ++
 ovsdb/jsonrpc-server.c  |  6 +-
 ovsdb/monitor.c | 36 +++
 ovsdb/ovsdb-client.c| 46 +++-
 ovsdb/ovsdb-server.c| 17 +--
 ovsdb/ovsdb-util.c  |  9 
 ovsdb/ovsdb.c   |  8 ++-
 ovsdb/query.c   |  2 ++
 ovsdb/replication.c | 15 +++--
 ovsdb/row.c |  4 +++-
 ovsdb/table.c   |  2 +-
 ovsdb/transaction.c |  2 ++
 utilities/ovs-vsctl.c   | 47 -
 vtep/vtep-ctl.c | 10 ++---
 27 files changed, 259 insertions(+), 93 deletions(-)

diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index ae8ab5800..f9c58a72f 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -171,6 +171,7 @@ dp_packet_new_with_headroom(size_t size, size_t headroom)
 struct dp_packet *
 dp_packet_clone(const struct dp_packet *buffer)
 {
+ovs_assert(buffer != NULL);
 return dp_packet_clone_with_headroom(buffer, 0);
 }
 
@@ -183,9 +184,12 @@ dp_packet_clone_with_headroom(const struct dp_packet 
*buffer, size_t headroom)
 struct dp_packet *new_buffer;
 uint32_t mark;
 
-new_buffer = dp_packet_clone_data_with_headroom(dp_packet_data(buffer),
- dp_packet_size(buffer),
- headroom);
+const void *data_dp = dp_packet_data(buffer);
+ovs_assert(data_dp != NULL);
+
+new_buffer = dp_packet_clone_data_with_headroom(data_dp,
+dp_packet_size(buffer),
+headroom);
 /* Copy the following fields into the returned buffer: l2_pad_size,
  * l2_5_ofs, l3_ofs, l4_ofs, cutlen, packet_type and md. */
 memcpy(_buffer->l2_pad_size, >l2_pad_size,
@@ -323,7 +327,9 @@ dp_packet_shift(struct dp_packet *b, int delta)
 
 if (delta != 0) {
 char *dst = (char *) dp_packet_data(b) + delta;
-memmove(dst, dp_packet_data(b), dp_packet_size(b));
+const void *data_dp = dp_packet_data(b);
+ovs_assert(data_dp != NULL);
+memmove(dst, data_dp, dp_packet_size(b));
 dp_packet_set_data(b, dst);
 }
 }
@@ -348,7 +354,7 @@ void *
 dp_packet_put_zeros(struct dp_packet *b, size_t size)
 {
 void *dst = dp_packet_put_uninit(b, size);
-memset(dst, 0, size);
+nullable_memset(dst, 0, size);
 return dst;
 }
 
@@ -359,7 +365,7 @@ void *
 dp_packet_put(struct dp_packet *b, const void *p, size_t size)
 {
 void *dst = dp_packet_put_uninit(b, size);
-memcpy(dst, p, size);
+nullable_memcpy(dst, p, size);
 return dst;
 }
 
@@ -431,7 

[ovs-dev] [PATCH] conntrack-tp: Fix clang warning.

2023-03-30 Thread miterv
From: Lin Huang 

Declaration of 'struct conn' will not be visible outside of this function.
Declaration of 'struct conntrack' will not be visible outside of this function.
Declaration of 'struct timeout_policy' will not be visible outside of this 
function.

Signed-off-by: Lin Huang 
---
 lib/conntrack-tp.h | 5 +
 1 file changed, 5 insertions(+)

diff --git a/lib/conntrack-tp.h b/lib/conntrack-tp.h
index 4d411d19f..2227f6fa5 100644
--- a/lib/conntrack-tp.h
+++ b/lib/conntrack-tp.h
@@ -17,8 +17,13 @@
 #ifndef CONNTRACK_TP_H
 #define CONNTRACK_TP_H 1
 
+#include 
+
 #define CT_DPIF_NETDEV_TP_MIN 30
 enum ct_timeout;
+struct conn;
+struct conntrack;
+struct timeout_policy;
 void timeout_policy_init(struct conntrack *ct);
 int timeout_policy_update(struct conntrack *ct, struct timeout_policy *tp);
 int timeout_policy_delete(struct conntrack *ct, uint32_t tp_id);
-- 
2.37.1.windows.1

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


Re: [ovs-dev] [PATCH ovn v2 branch-22.12 2/2] northd: Drop packets for LBs with no backends

2023-03-30 Thread 0-day Robot
Bleep bloop.  Greetings Ales Musil, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

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


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Dumitru Ceara 
Lines checked: 105, Warnings: 1, Errors: 0


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

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


[ovs-dev] [PATCH ovn v2 branch-22.12 2/2] northd: Drop packets for LBs with no backends

2023-03-30 Thread Ales Musil
When the LB is configured without any backend
and doesn't report event or reject the packet
just simply drop the packet.

Reported-at: https://bugzilla.redhat.com/2177173
Signed-off-by: Ales Musil 
Signed-off-by: Dumitru Ceara 
(cherry picked from commit 75b0bcbb019a8caa2cfebffeff1ecf155fcd1fbc)
---
This version applies cleanly down to 21.12.
---
 northd/northd.c |  4 
 tests/ovn-northd.at | 50 +
 2 files changed, 54 insertions(+)

diff --git a/northd/northd.c b/northd/northd.c
index 89bd9cbb3..080852528 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3912,6 +3912,10 @@ build_lb_vip_actions(struct ovn_lb_vip *lb_vip,
 }
 } else if (lb_vip->empty_backend_rej && !lb_vip->n_backends) {
 reject = true;
+} else if (!lb_vip->empty_backend_rej && !lb_vip->n_backends) {
+ds_clear(action);
+ds_put_cstr(action, debug_drop_action());
+skip_hash_fields = true;
 } else {
 ds_put_format(action, "%s(backends=%s);", ct_lb_action,
   lb_vip_nb->backend_ips);
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 5a4c6a31f..5fea88f6f 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -4253,6 +4253,15 @@ AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], 
[dnl
   table=7 (ls_out_stateful), priority=100  , match=(reg0[[1]] == 1 && 
reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = 
reg3; }; next;)
 ])
 
+# LB with event=false and reject=false
+AT_CHECK([ovn-nbctl create load_balancer name=lb1 options:reject=false 
options:event=false vips:\"10.0.0.20\"=\"\" protocol=tcp], [0], [ignore])
+check ovn-nbctl --wait=sb ls-lb-add sw0 lb1
+
+AT_CHECK([ovn-sbctl dump-flows sw0 | grep "ls_in_lb " | sort ], [0], [dnl
+  table=12(ls_in_lb   ), priority=0, match=(1), action=(next;)
+  table=12(ls_in_lb   ), priority=110  , match=(ct.new && ip4.dst == 
10.0.0.20), action=(drop;)
+])
+
 AT_CLEANUP
 ])
 
@@ -5629,6 +5638,47 @@ AT_CHECK([grep "lr_out_snat" lr0flows | sed 
's/table=./table=?/' | sort], [0], [
   table=? (lr_out_snat), priority=120  , match=(nd_ns), action=(next;)
 ])
 
+# LB with event=false and reject=false
+check ovn-nbctl lr-lb-del lr0
+check ovn-nbctl remove logical_router lr0 options lb_force_snat_ip
+AT_CHECK([ovn-nbctl create load_balancer name=lb6 options:reject=false 
options:event=false vips:\"172.168.10.30\"=\"\" protocol=tcp], [0], [ignore])
+check ovn-nbctl --wait=sb lr-lb-add lr0 lb6
+
+AT_CHECK([ovn-sbctl dump-flows lr0 | grep "lr_in_dnat" | sort], [0], [dnl
+  table=7 (lr_in_dnat ), priority=0, match=(1), action=(next;)
+  table=7 (lr_in_dnat ), priority=110  , match=(ct.est && !ct.rel && 
ip4 && reg0 == 172.168.10.30 && ct_mark.natted == 1), action=(next;)
+  table=7 (lr_in_dnat ), priority=110  , match=(ct.new && !ct.rel && 
ip4 && reg0 == 172.168.10.30), action=(drop;)
+  table=7 (lr_in_dnat ), priority=50   , match=(ct.rel && !ct.est && 
!ct.new), action=(ct_commit_nat;)
+  table=7 (lr_in_dnat ), priority=70   , match=(ct.rel && !ct.est && 
!ct.new && ct_mark.force_snat == 1), action=(flags.force_snat_for_lb = 1; 
ct_commit_nat;)
+  table=7 (lr_in_dnat ), priority=70   , match=(ct.rel && !ct.est && 
!ct.new && ct_mark.skip_snat == 1), action=(flags.skip_snat_for_lb = 1; 
ct_commit_nat;)
+])
+
+# LB with event=false, reject=false and skip_snat
+check ovn-nbctl --wait=sb set load_balancer lb6 options:skip_snat=true
+
+AT_CHECK([ovn-sbctl dump-flows lr0 | grep "lr_in_dnat" | sort], [0], [dnl
+  table=7 (lr_in_dnat ), priority=0, match=(1), action=(next;)
+  table=7 (lr_in_dnat ), priority=110  , match=(ct.est && !ct.rel && 
ip4 && reg0 == 172.168.10.30 && ct_mark.natted == 1), 
action=(flags.skip_snat_for_lb = 1; next;)
+  table=7 (lr_in_dnat ), priority=110  , match=(ct.new && !ct.rel && 
ip4 && reg0 == 172.168.10.30), action=(flags.skip_snat_for_lb = 1; drop;)
+  table=7 (lr_in_dnat ), priority=50   , match=(ct.rel && !ct.est && 
!ct.new), action=(ct_commit_nat;)
+  table=7 (lr_in_dnat ), priority=70   , match=(ct.rel && !ct.est && 
!ct.new && ct_mark.force_snat == 1), action=(flags.force_snat_for_lb = 1; 
ct_commit_nat;)
+  table=7 (lr_in_dnat ), priority=70   , match=(ct.rel && !ct.est && 
!ct.new && ct_mark.skip_snat == 1), action=(flags.skip_snat_for_lb = 1; 
ct_commit_nat;)
+])
+
+check ovn-nbctl remove load_balancer lb6 options skip_snat
+
+# LB with event=false, reject=false and force_snat
+check ovn-nbctl --wait=sb set logical_router lr0 
options:lb_force_snat_ip="router_ip"
+
+AT_CHECK([ovn-sbctl dump-flows lr0 | grep "lr_in_dnat" | sort], [0], [dnl
+  table=7 (lr_in_dnat ), priority=0, match=(1), action=(next;)
+  table=7 (lr_in_dnat ), priority=110  , match=(ct.est && !ct.rel && 
ip4 && reg0 == 172.168.10.30 && ct_mark.natted == 1), 

[ovs-dev] [PATCH ovn v2 branch-22.12 1/2] northd: Make the enclose handling less error prone

2023-03-30 Thread Ales Musil
The enclose handling was working for all cases
except the drop. Make sure that the enclose is
properly defined for that case and make sure
that it is used properly.

Fixes: cf205ca0e52c ("northd: Fix missig "); " from LB flows")
Signed-off-by: Ales Musil 
---
 northd/northd.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 3165f4bc2..89bd9cbb3 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -10525,11 +10525,13 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip 
*lb_vip,
lb->selection_fields, false,
features->ct_no_masked_label);
 bool drop = !!strncmp(ds_cstr(action), "ct_lb", strlen("ct_lb"));
+const char *enclose = drop ? "" : ");";
 if (!drop) {
 /* Remove the trailing ");". */
 ds_truncate(action, action->length - 2);
 }
 
+
 /* Higher priority rules are added for load-balancing in DNAT
  * table.  For every match (on a VIP[:port]), we add two flows.
  * One flow is for specific matching on ct.new with an action
@@ -10548,8 +10550,8 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip 
*lb_vip,
 const char *skip_snat = features->ct_lb_related && !drop
 ? "; skip_snat"
 : "";
-skip_snat_new_action = xasprintf("flags.skip_snat_for_lb = 1; %s%s);",
- ds_cstr(action), skip_snat);
+skip_snat_new_action = xasprintf("flags.skip_snat_for_lb = 1; %s%s%s",
+ ds_cstr(action), skip_snat, enclose);
 skip_snat_est_action = xasprintf("flags.skip_snat_for_lb = 1; "
  "next;");
 }
@@ -10685,8 +10687,8 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip 
*lb_vip,
 const char *force_snat = features->ct_lb_related && !drop
  ? "; force_snat"
  : "";
-char *new_actions = xasprintf("flags.force_snat_for_lb = 1; %s%s);",
-  ds_cstr(action), force_snat);
+char *new_actions = xasprintf("flags.force_snat_for_lb = 1; %s%s%s",
+  ds_cstr(action), force_snat, enclose);
 build_gw_lrouter_nat_flows_for_lb(lb, gw_router_force_snat,
 n_gw_router_force_snat, reject, new_match,
 new_actions, est_match,
@@ -10701,9 +10703,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip 
*lb_vip,
lb_aff_force_snat_router,
n_lb_aff_force_snat_router);
 
-if (!drop) {
-ds_put_cstr(action, ");");
-}
+ds_put_cstr(action, enclose);
 
 build_gw_lrouter_nat_flows_for_lb(lb, gw_router, n_gw_router,
 reject, new_match, ds_cstr(action), est_match,
-- 
2.39.2

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


Re: [ovs-dev] [PATCH ovn v3 2/2] Add fake multinode system tests.

2023-03-30 Thread Dumitru Ceara
On 3/29/23 16:02, num...@ovn.org wrote:
> From: Numan Siddique 
> 
> This patch adds a simple system test using ovn-fake-multinode
> setup.  The tests can be run as - 'make check-multinode'
> 
> Before running these tests, user should deploy fake_multinode setup
> by running 'ovn_cluster.sh start'.
> 
> This test suite is also triggered for the newly added fake multinode CI
> job.
> 
> The fake multinode system tests suite can be enhanced further for new
> features and to cover multi node scenarios.
> 
> Signed-off-by: Numan Siddique 
> ---

I have a few minor comments, feel free to add my ack when you apply the
patch if you address them:

Acked-by: Dumitru Ceara 

Thanks,
Dumitru

>  .../workflows/ovn-fake-multinode-tests.yml|  70 ++-
>  tests/automake.mk |  27 ++-
>  tests/multinode-macros.at | 190 ++
>  tests/multinode-testsuite.at  |  27 +++
>  tests/multinode.at|  74 +++
>  5 files changed, 384 insertions(+), 4 deletions(-)
>  create mode 100644 tests/multinode-macros.at
>  create mode 100644 tests/multinode-testsuite.at
>  create mode 100644 tests/multinode.at
> 
> diff --git a/.github/workflows/ovn-fake-multinode-tests.yml 
> b/.github/workflows/ovn-fake-multinode-tests.yml
> index fa768f235c..ffc84f9d3f 100644
> --- a/.github/workflows/ovn-fake-multinode-tests.yml
> +++ b/.github/workflows/ovn-fake-multinode-tests.yml
> @@ -88,6 +88,13 @@ jobs:
>CENTRAL_IMAGE: ${{ matrix.cfg.central_image }}
># Disable SSL for now. Revisit this if required.
>ENABLE_SSL: no
> +  CC: gcc
> +  OPTS: "--disable-ssl"
> +  dependencies: |
> +automake libtool gcc bc libjemalloc2 libjemalloc-dev\
> +libssl-dev llvm-dev libelf-dev libnuma-dev libpcap-dev  \
> +selinux-policy-dev ncat python3-scapy isc-dhcp-server \
> +podman openvswitch-switch libunbound-dev libunwind-dev
># https://github.com/actions/runner-images/issues/6282
>XDG_RUNTIME_DIR: ''
>  
> @@ -101,6 +108,8 @@ jobs:
>  - { central_image: "ovn/ovn-multi-node:22.03" }
>  
>  steps:
> +- name: install required dependencies
> +  run:  sudo apt install -y ${{ env.dependencies }}
>  
>  - name: Free up disk space
>run: sudo eatmydata apt-get remove --auto-remove -y aspnetcore-* 
> dotnet-* libmono-* mono-* msbuild php-* php7* ghc-* zulu-*
> @@ -125,10 +134,8 @@ jobs:
>  path: 'ovn-fake-multinode'
>  ref: 'v0.1'
>  
> -- name: Install dependencies
> +- name: Start openvswitch
>run: |
> -sudo apt update
> -sudo apt-get install -y podman openvswitch-switch
>  sudo systemctl start openvswitch-switch
>  sudo ovs-vsctl show
>  
> @@ -145,6 +152,63 @@ jobs:
>  sudo ./.ci/test_basic.sh
>working-directory: ovn-fake-multinode
>  
> +- name: update PATH
> +  run:  |
> +echo "$HOME/bin">> $GITHUB_PATH
> +echo "$HOME/.local/bin" >> $GITHUB_PATH
> +
> +- name: set up python
> +  uses: actions/setup-python@v4
> +  with:
> +python-version: '3.x'
> +
> +- name: Check out ovn
> +  uses: actions/checkout@v3
> +  with:
> +path: 'ovn'
> +submodules: recursive
> +
> +- name: Build OVN and trigger fake-multinode system tests
> +  run: |
> +set -x
> +pwd
> +cd ovn
> +./.ci/linux-prepare.sh
> +./.ci/linux-build.sh
> +sudo make check-multinode  || :
> +sudo podman exec -it ovn-central ovn-nbctl show || :
> +sudo podman exec -it ovn-central ovn-sbctl show || :
> +sudo podman exec -it ovn-chassis-1 ovs-vsctl show || :
> +sudo podman exec -it ovn-chassis-1 ip netns || :
> +sudo podman exec -it ovn-chassis-1 cat 
> /var/log/ovn/ovn-controller.log || :
> +sudo cat tests/multinode-testsuite.dir/1/multinode-testsuite.log || :
> +
> +- name: copy logs on failure
> +  if: failure() || cancelled()
> +  run: |
> +# upload-artifact@v3 throws exceptions if it tries to upload socket
> +# files and we could have some socket files in testsuite.dir.
> +# Also, upload-artifact@v3 doesn't work well enough with wildcards.
> +# So, we're just archiving everything here to avoid any issues.
> +pwd
> +ls -l

These two lines can be removed I guess.

> +mkdir logs
> +ls -l ovn/
> +ls -l ovn/tests/

Same here.

> +
> +cp ovn/config.log ./logs/
> +# multinode tests are run as root, need to adjust permissions.
> +sudo chmod -R +r ovn/tests/multinode-testsuite.dir.* || true
> +cp -r ovn/tests/multinode-testsuite.dir.* ./logs/ || true
> +tar -czvf logs.tgz logs/
> +
> +- name: upload logs on failure
> +  if: failure() || cancelled()
> +  uses: actions/upload-artifact@v3
> +  with:
> +   

Re: [ovs-dev] [PATCH ovn v3 1/2] CI: Add a couple of periodic jobs using ovn-fake-multinode.

2023-03-30 Thread Dumitru Ceara
On 3/29/23 16:01, num...@ovn.org wrote:
> From: Numan Siddique 
> 
> This patch adds a couple of jobs using ovn-fake-multinode.
> It first builds 2 ovn-fake-multinode container images
>   - one with OVN 22.03
>   - one with present main.
> 
> The first job deploys ovn-fake-multinode with the main
> OVN and runs simple tests provided by ovn-fake-multinode [1].
> 
> The second job deploys ovn-fake-multinode setup with the
> central image using OVN 22.03 and chassis image using main OVN.
> This job tests the scenario
>   - ovn-northd and OVN dbs are running the most recent LTS.
>   - ovn-controller is running the latest commit from the branch.
> 
> The workflow is right now scheduled to trigger on midnight everyday.
> Once we cache the built image or reduce the overall run time of
> this workflow we can enable for every push.
> 
> [1] - 
> https://github.com/ovn-org/ovn-fake-multinode/blob/main/.ci/test_basic.sh
> 
> Signed-off-by: Numan Siddique 
> ---

I have a few minor comments, feel free to add my ack when you apply the
patch if you address them:

Acked-by: Dumitru Ceara 

Thanks,
Dumitru

>  .../workflows/ovn-fake-multinode-tests.yml| 151 ++
>  Makefile.am   |   1 +
>  2 files changed, 152 insertions(+)
>  create mode 100644 .github/workflows/ovn-fake-multinode-tests.yml
> 
> diff --git a/.github/workflows/ovn-fake-multinode-tests.yml 
> b/.github/workflows/ovn-fake-multinode-tests.yml
> new file mode 100644
> index 00..fa768f235c
> --- /dev/null
> +++ b/.github/workflows/ovn-fake-multinode-tests.yml
> @@ -0,0 +1,151 @@
> +name: System tests using ovn-fake-multinode
> +
> +on:

It might be good also add:

  workflow_dispatch:

So we can run the workflow on demand as well.

> +  schedule:
> +# Run everyday at midnight
> +- cron: '0 0 * * *'
> +
> +concurrency:
> +  group: ${{ github.workflow }}-${{ github.event.pull_request.number || 
> github.run_id }}
> +  cancel-in-progress: true
> +
> +jobs:
> +  build:
> +env:
> +  RUNC_CMD: podman
> +  OS_IMAGE: "fedora:37"
> +  # https://github.com/actions/runner-images/issues/6282
> +  XDG_RUNTIME_DIR: ''
> +
> +name: Build ovn-fake-multinode image
> +runs-on: ubuntu-20.04
> +steps:
> +- name: Check out ovn-fake-multi-node
> +  uses: actions/checkout@v3
> +  with:
> +repository: 'ovn-org/ovn-fake-multinode'
> +path: 'ovn-fake-multinode'
> +ref: 'v0.1'
> +
> +- name: Check out ovn
> +  uses: actions/checkout@v3
> +  with:
> +path: 'ovn-fake-multinode/ovn'
> +submodules: recursive
> +
> +- name: Check out ovs master
> +  uses: actions/checkout@v3
> +  with:
> +path: 'ovn-fake-multinode/ovs'
> +repository: 'openvswitch/ovs'
> +ref: 'master'

This works fine because ovn-fake-multinode builds OVN against the
version checked out in the submodule.  But it might not be obvious.
Shall we add a comment before these two steps?

> +
> +- name: Install dependencies
> +  run: |
> +sudo apt update
> +sudo apt-get install -y podman
> +
> +- name: Build ovn-fake-multi-node main image
> +  run: |
> +set -x
> +sudo -E ./ovn_cluster.sh build
> +mkdir -p /tmp/_output
> +sudo podman save ovn/ovn-multi-node:latest > 
> /tmp/_output/ovn_main_image.tar
> +  working-directory: ovn-fake-multinode
> +
> +- name: Checkout ovn branch-22.03
> +  uses: actions/checkout@v3
> +  with:
> +path: 'ovn-fake-multinode/ovn'
> +submodules: recursive
> +ref: 'branch-22.03'
> +
> +- name: Build ovn-fake-multi-node 22.03 image
> +  run: |
> +set -x
> +sudo -E ./ovn_cluster.sh build
> +mkdir -p /tmp/_output
> +sudo podman tag ovn/ovn-multi-node:latest ovn/ovn-multi-node:22.03
> +sudo podman save ovn/ovn-multi-node:22.03 > 
> /tmp/_output/ovn_22_03_image.tar
> +  working-directory: ovn-fake-multinode
> +
> +- uses: actions/upload-artifact@v3
> +  with:
> +name: test-main-image
> +path: /tmp/_output/ovn_main_image.tar
> +
> +- uses: actions/upload-artifact@v3
> +  with:
> +name: test-22-03-image
> +path: /tmp/_output/ovn_22_03_image.tar
> +
> +  multinode-tests:
> +runs-on: ubuntu-20.04
> +timeout-minutes: 15
> +env:
> +  RUNC_CMD: podman
> +  OS_IMAGE: "fedora:37"
> +  CENTRAL_IMAGE: ${{ matrix.cfg.central_image }}
> +  # Disable SSL for now. Revisit this if required.
> +  ENABLE_SSL: no
> +  # https://github.com/actions/runner-images/issues/6282
> +  XDG_RUNTIME_DIR: ''
> +
> +name: multinode tests ${{ join(matrix.cfg.*, ' ') }}
> +needs: [build]
> +strategy:
> +  fail-fast: false
> +  matrix:
> +cfg:
> +- { central_image: "ovn/ovn-multi-node:latest" }
> +- { central_image: "ovn/ovn-multi-node:22.03" }
> +
> +steps:
> +
> + 

Re: [ovs-dev] [PATCH dpdk-latest v3 0/6] Add support for DPDK meter HW offload

2023-03-30 Thread Eli Britstein via dev
Hi

The usage of the proxy port is wrong.
All transfer offload should be migrated to use it. There was [1-4], but it was 
not completed.

Meters should be created only on the proxy ports. As all offloads are moved to 
use proxy ports, this is how they are shared.
Since we don't know the proxy port when meter is added, it can be lazy created 
upon the first flow that uses this meter/proxy-port (referenced counted), and 
destroy upon the last flow destroy using it.
As there might be multiple "proxy" ports, there might be multiple HW meter 
objects associated with the same SW one. This should be managed.

[1] http://patchwork.ozlabs.org/project/openvswitch/list/?series=302525=*
[2] http://patchwork.ozlabs.org/project/openvswitch/list/?series=310413=*
[3] http://patchwork.ozlabs.org/project/openvswitch/list/?series=310415=*
[4] http://patchwork.ozlabs.org/project/openvswitch/list/?series=342885=*

Thanks,
Eli

>-Original Message-
>From: Simon Horman 
>Sent: Thursday, 30 March 2023 14:21
>To: d...@openvswitch.org
>Cc: Ilya Maximets ; Eli Britstein ;
>Chaoyong He ; Peng Zhang
>; Jin Liu ; oss-
>driv...@corigine.com
>Subject: [PATCH dpdk-latest v3 0/6] Add support for DPDK meter HW offload
>
>External email: Use caution opening links or attachments
>
>
>Hi,
>
>this series adds support for DPDK meter HW offload.
>
>Changes between v2 and v3.
>* Use common API for DPDK and non-DPDK meter offloads
>* Make use of netdev_ports_traverse to offload the meter
>* Add dpdk-latest to subject prefix
>
>Changes between v1 and v2:
>* Add the prox mechanism: add the meter by proxy id
>* Change the offload interface from netdev-dpdk to the netdev-offload
>* Changed base to dpdk-latest branch
>
>Peng Zhang (6):
>  netdev-offload-dpdk: use flow transfer proxy
>  netdev-offload: Let meter offload API can be used with DPDK
>  dpif-netdev: Offloading meter with DPDK
>  netdev-offload-dpdk: Implement meter offload API for DPDK
>  netdev-dpdk: add meter algorithms
>  netdev-dpdk-offload: Add support for meter action
>
> Documentation/howto/dpdk.rst  |   5 +-
> lib/dpif-netdev.c |  22 ++-
> lib/netdev-dpdk.c | 306 +-
> lib/netdev-dpdk.h |  43 +
> lib/netdev-offload-dpdk.c |  97 +++
> lib/netdev-offload-provider.h |  21 ++-
> lib/netdev-offload-tc.c   |   9 +-
> lib/netdev-offload.c  | 135 ++-
> lib/netdev-offload.h  |   9 +
> 9 files changed, 633 insertions(+), 14 deletions(-)
>
>--
>2.30.2

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


Re: [ovs-dev] [PATCH ovn] system-tests: Reduce flakiness of netcat UDP clients

2023-03-30 Thread Dumitru Ceara
On 3/29/23 07:42, Ales Musil wrote:
> When we send a packet through UDP without other side
> having open port, it usually replies with ICMP message.
> netcat upon getting that error fails with:
> "Ncat: Connection refused.". However, there is a race
> because this message might arrive after the netcat client
> is already done, so it will exit without any error.
> 
> To prevent that error ignore the return code, stdout and stderr
> for those calls. It is fine to ignore because all those changed
> instances are checking packet count after the calls so if the
> netcat fails for other reasons it will be detected by the packet
> count check.
> 
> Signed-off-by: Ales Musil 
> ---

Thanks, Ales!  I applied this to the main branch and backported it to
23.03 and 22.12.

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


Re: [ovs-dev] [PATCH ovn branch-22.12] lb: Allow IPv6 template LBs to use explicit backends.

2023-03-30 Thread Dumitru Ceara
On 3/29/23 11:17, Ales Musil wrote:
> On Tue, Mar 28, 2023 at 4:31 PM Dumitru Ceara  wrote:
> 
>> This was working fine for IPv4 but partially by accident because IPv4
>> addresses don't contain ':'.  Fix it for the general case by trying to
>> parse explicit backends if parsing templates fails.
>>
>> Also add unit and system tests for all supported cases.
>>
>> Fixes: b45853fba816 ("lb: Support using templates.")
>> Signed-off-by: Dumitru Ceara 
>> Acked-by: Ales Musil 
>> (cherry picked from commit 7310a9859a6a5a9ad4faff1e5620e5cfa142eb7d)
>> ---
>> NOTE: this is a custom backport because it picks up the tests added by
>> 086744a645a7 ("northd: Use LB port_str in northd.").  It doesn't pick up
>> the rest of the changes from that commit because they're not applicable
>> on branch-22.12.
>> ---
>>  lib/lb.c  |  51 ++---
>>  lib/lb.h  |   6 +-
>>  tests/ovn-nbctl.at|  26 +
>>  tests/system-ovn.at   | 126 --
>>  utilities/ovn-nbctl.c |   2 +-
>>  5 files changed, 182 insertions(+), 29 deletions(-)
>>
>> diff --git a/lib/lb.c b/lib/lb.c
>> index c13d07b99c..b74e4d90c1 100644
>> --- a/lib/lb.c
>> +++ b/lib/lb.c
>> @@ -36,6 +36,7 @@ static const char *lb_neighbor_responder_mode_names[] = {
>>  static struct nbrec_load_balancer_health_check *
>>  ovn_lb_get_health_check(const struct nbrec_load_balancer *nbrec_lb,
>>  const char *vip_port_str, bool template);
>> +static void ovn_lb_backends_clear(struct ovn_lb_vip *vip);
>>
>>  struct ovn_lb_ip_set *
>>  ovn_lb_ip_set_create(void)
>> @@ -236,6 +237,8 @@ ovn_lb_backends_init_template(struct ovn_lb_vip
>> *lb_vip, const char *value_)
>>  ds_put_format(, "%s: should be a template of the form:
>> "
>>
>>  "'^backendip_variable1[:^port_variable1|:port]', ",
>>atom);
>> +free(backend_port);
>> +free(backend_ip);
>>  }
>>  free(atom);
>>  }
>> @@ -283,8 +286,27 @@ ovn_lb_vip_init_template(struct ovn_lb_vip *lb_vip,
>> const char *lb_key_,
>>   lb_key_);
>>  }
>>
>> +/* Backends can either be templates or explicit IPs and ports. */
>>  lb_vip->address_family = address_family;
>> -return ovn_lb_backends_init_template(lb_vip, lb_value);
>> +lb_vip->template_backends = true;
>> +char *template_error = ovn_lb_backends_init_template(lb_vip,
>> lb_value);
>> +
>> +if (template_error) {
>> +lb_vip->template_backends = false;
>> +ovn_lb_backends_clear(lb_vip);
>> +
>> +char *explicit_error = ovn_lb_backends_init_explicit(lb_vip,
>> lb_value);
>> +if (explicit_error) {
>> +char *error =
>> +xasprintf("invalid backend: template (%s) OR explicit
>> (%s)",
>> +  template_error, explicit_error);
>> +free(explicit_error);
>> +free(template_error);
>> +return error;
>> +}
>> +free(template_error);
>> +}
>> +return NULL;
>>  }
>>
>>  /* Returns NULL on success, an error string on failure.  The caller is
>> @@ -302,15 +324,29 @@ ovn_lb_vip_init(struct ovn_lb_vip *lb_vip, const
>> char *lb_key,
>> address_family);
>>  }
>>
>> -void
>> -ovn_lb_vip_destroy(struct ovn_lb_vip *vip)
>> +static void
>> +ovn_lb_backends_destroy(struct ovn_lb_vip *vip)
>>  {
>> -free(vip->vip_str);
>> -free(vip->port_str);
>>  for (size_t i = 0; i < vip->n_backends; i++) {
>>  free(vip->backends[i].ip_str);
>>  free(vip->backends[i].port_str);
>>  }
>> +}
>> +
>> +static void
>> +ovn_lb_backends_clear(struct ovn_lb_vip *vip)
>> +{
>> +ovn_lb_backends_destroy(vip);
>> +vip->backends = NULL;
>> +vip->n_backends = 0;
>> +}
>> +
>> +void
>> +ovn_lb_vip_destroy(struct ovn_lb_vip *vip)
>> +{
>> +free(vip->vip_str);
>> +free(vip->port_str);
>> +ovn_lb_backends_destroy(vip);
>>  free(vip->backends);
>>  }
>>
>> @@ -355,11 +391,10 @@ ovn_lb_vip_format(const struct ovn_lb_vip *vip,
>> struct ds *s, bool template)
>>  }
>>
>>  void
>> -ovn_lb_vip_backends_format(const struct ovn_lb_vip *vip, struct ds *s,
>> -   bool template)
>> +ovn_lb_vip_backends_format(const struct ovn_lb_vip *vip, struct ds *s)
>>  {
>>  bool needs_brackets = vip->address_family == AF_INET6 && vip->port_str
>> -  && !template;
>> +  && !vip->template_backends;
>>  for (size_t i = 0; i < vip->n_backends; i++) {
>>  struct ovn_lb_backend *backend = >backends[i];
>>
>> diff --git a/lib/lb.h b/lib/lb.h
>> index 55becc1bf7..e5a3875609 100644
>> --- a/lib/lb.h
>> +++ b/lib/lb.h
>> @@ -98,6 +98,9 @@ struct ovn_lb_vip {
>>*/
>>  struct ovn_lb_backend *backends;
>>  size_t n_backends;
>> +bool template_backends; /* True if the backends are templates. 

Re: [ovs-dev] [PATCH dpdk-latest v3 6/6] netdev-dpdk-offload: Add support for meter action

2023-03-30 Thread 0-day Robot
References:  <20230330112057.14242-7-simon.hor...@corigine.com>
 

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

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


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Simon Horman 
Lines checked: 50, Warnings: 1, Errors: 0


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

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


Re: [ovs-dev] [PATCH dpdk-latest v3 5/6] netdev-dpdk: add meter algorithms

2023-03-30 Thread 0-day Robot
References:  <20230330112057.14242-6-simon.hor...@corigine.com>
 

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

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


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Simon Horman 
Lines checked: 99, Warnings: 1, Errors: 0


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

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


Re: [ovs-dev] [PATCH dpdk-latest v3 4/6] netdev-offload-dpdk: Implement meter offload API for DPDK

2023-03-30 Thread 0-day Robot
References:  <20230330112057.14242-5-simon.hor...@corigine.com>
 

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

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


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Simon Horman 
Lines checked: 406, Warnings: 1, Errors: 0


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

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


Re: [ovs-dev] [PATCH ovn branch-22.12] northd: Drop packets for LBs with no backends

2023-03-30 Thread Dumitru Ceara
On 3/29/23 08:46, Ales Musil wrote:
> When the LB is configured without any backend
> and doesn't report event or reject the packet
> just simply drop the packet.
> 
> Reported-at: https://bugzilla.redhat.com/2177173
> Signed-off-by: Ales Musil 
> Signed-off-by: Dumitru Ceara 
> (cherry picked from commit 75b0bcbb019a8caa2cfebffeff1ecf155fcd1fbc)
> ---
> This version applies cleanly down to 21.12.
> ---

Thanks for the backport, Ales!  But, as 0-day bot also points out, this
fails CI:

https://mail.openvswitch.org/pipermail/ovs-build/2023-March/029516.html
https://github.com/ovsrobot/ovn/actions/runs/4551145516/jobs/8024882262

## --- ##
## ovn 22.12.1 test suite. ##
## --- ##
994. ovn-northd.at:5066: testing ovn -- LR NAT flows -- ovn-northd -- 
parallelization=yes ...
[...]
+++ /home/dceara/git-repos/ovn/tests/testsuite.dir/at-groups/994/stdout 
2023-03-30 13:31:49.436472934 +0200
@@ -1,6 +1,6 @@
   table=7 (lr_in_dnat ), priority=0, match=(1), action=(next;)
   table=7 (lr_in_dnat ), priority=110  , match=(ct.est && !ct.rel && 
ip4 && reg0 == 172.168.10.30 && ct_mark.natted == 1), 
action=(flags.skip_snat_for_lb = 1; next;)
-  table=7 (lr_in_dnat ), priority=110  , match=(ct.new && !ct.rel && 
ip4 && reg0 == 172.168.10.30), action=(flags.skip_snat_for_lb = 1; drop;)
+  table=7 (lr_in_dnat ), priority=110  , match=(ct.new && !ct.rel && 
ip4 && reg0 == 172.168.10.30), action=(flags.skip_snat_for_lb = 1; drop;);)
[...]

Thanks,
Dumitru

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


Re: [ovs-dev] [PATCH dpdk-latest v3 3/6] dpif-netdev: Offloading meter with DPDK

2023-03-30 Thread 0-day Robot
References:  <20230330112057.14242-4-simon.hor...@corigine.com>
 

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

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


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Simon Horman 
Lines checked: 70, Warnings: 1, Errors: 0


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

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


Re: [ovs-dev] [PATCH dpdk-latest v3 2/6] netdev-offload: Let meter offload API can be used with DPDK

2023-03-30 Thread 0-day Robot
References:  <20230330112057.14242-3-simon.hor...@corigine.com>
 

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

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


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Simon Horman 
Lines checked: 313, Warnings: 1, Errors: 0


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

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


Re: [ovs-dev] [PATCH dpdk-latest v3 1/6] netdev-offload-dpdk: use flow transfer proxy

2023-03-30 Thread 0-day Robot
References:  <20230330112057.14242-2-simon.hor...@corigine.com>
 

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

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


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Simon Horman 
Lines checked: 148, Warnings: 1, Errors: 0


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

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


[ovs-dev] [PATCH dpdk-latest v3 2/6] netdev-offload: Let meter offload API can be used with DPDK

2023-03-30 Thread Simon Horman
From: Peng Zhang 

Changing meter API so it can offload meter to HW in different
datapath. And the corresponding functions to call the DPDK
meter callbacks from all the registered flow API providers.
The interfaces are like those related to DPDK meter in dpif_class,
in order to pass necessary info to HW.

Signed-off-by: Peng Zhang 
Signed-off-by: Jin Liu 
Co-authored-by: Jin Liu 
Signed-off-by: Simon Horman 
---
 Documentation/howto/dpdk.rst  |   5 +-
 lib/netdev-offload-provider.h |  21 +-
 lib/netdev-offload-tc.c   |   9 ++-
 lib/netdev-offload.c  | 135 +-
 lib/netdev-offload.h  |   9 +++
 5 files changed, 168 insertions(+), 11 deletions(-)

diff --git a/Documentation/howto/dpdk.rst b/Documentation/howto/dpdk.rst
index 04609b20bd21..02fc568770ee 100644
--- a/Documentation/howto/dpdk.rst
+++ b/Documentation/howto/dpdk.rst
@@ -401,10 +401,11 @@ Supported actions for hardware offload are:
 - Modification of IPv6 (set_field:->ipv6_src/ipv6_dst/mod_nw_ttl).
 - Clone/output (tnl_push and output) for encapsulating over a tunnel.
 - Tunnel pop, for packets received on physical ports.
+- Meter.
 
 .. note::
-  Tunnel offloads are experimental APIs in DPDK. In order to enable it,
-  compile with -DALLOW_EXPERIMENTAL_API.
+  Tunnel offloads and Meter offloads are experimental APIs in DPDK. To enable
+  these features, compile with -DALLOW_EXPERIMENTAL_API.
 
 Multiprocess
 
diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h
index 9108856d18d1..b02531a0f813 100644
--- a/lib/netdev-offload-provider.h
+++ b/lib/netdev-offload-provider.h
@@ -98,27 +98,42 @@ struct netdev_flow_api {
  * and the configuration in 'config'. On failure, a non-zero error code is
  * returned.
  *
+ * If the datapath is netdev, the api needs the param struct netdev *,
+ * If the datapath is system, the param struct netdev* is unused and
+ * should be NULL.
+ *
  * The meter id specified through 'config->meter_id' is ignored. */
-int (*meter_set)(ofproto_meter_id meter_id,
+int (*meter_set)(struct netdev *,
+ ofproto_meter_id meter_id,
  struct ofputil_meter_config *config);
 
 /* Queries HW for meter stats with the given 'meter_id'. Store the stats
  * of dropped packets to band 0. On failure, a non-zero error code is
  * returned.
  *
+ * If the datapath is netdev, the api needs the param struct netdev *,
+ * If the datapath is system, the param struct netdev* is unused and
+ * should be NULL.
+ *
  * Note that the 'stats' structure is already initialized, and only the
  * available statistics should be incremented, not replaced. Those fields
  * are packet_in_count, byte_in_count and band[]->byte_count and
  * band[]->packet_count. */
-int (*meter_get)(ofproto_meter_id meter_id,
+int (*meter_get)(struct netdev *,
+ ofproto_meter_id meter_id,
  struct ofputil_meter_stats *stats);
 
 /* Removes meter 'meter_id' from HW. Store the stats of dropped packets to
  * band 0. On failure, a non-zero error code is returned.
  *
+ * If the datapath is netdev, the api needs the param struct netdev *,
+ * If the datapath is system, the param struct netdev* is unused and
+ * should be NULL.
+ *
  * 'stats' may be passed in as NULL if no stats are needed, See the above
  * function for additional details on the 'stats' usage. */
-int (*meter_del)(ofproto_meter_id meter_id,
+int (*meter_del)(struct netdev *,
+ ofproto_meter_id meter_id,
  struct ofputil_meter_stats *stats);
 
 /* Initializies the netdev flow api.
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index ce7f8ad97306..6a965f4d366b 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -2892,7 +2892,8 @@ meter_free_police_index(uint32_t police_index)
 }
 
 static int
-meter_tc_set_policer(ofproto_meter_id meter_id,
+meter_tc_set_policer(struct netdev *netdev OVS_UNUSED,
+ ofproto_meter_id meter_id,
  struct ofputil_meter_config *config)
 {
 uint32_t police_index;
@@ -2946,7 +2947,8 @@ meter_tc_set_policer(ofproto_meter_id meter_id,
 }
 
 static int
-meter_tc_get_policer(ofproto_meter_id meter_id,
+meter_tc_get_policer(struct netdev *netdev OVS_UNUSED,
+ ofproto_meter_id meter_id,
  struct ofputil_meter_stats *stats)
 {
 uint32_t police_index;
@@ -2965,7 +2967,8 @@ meter_tc_get_policer(ofproto_meter_id meter_id,
 }
 
 static int
-meter_tc_del_policer(ofproto_meter_id meter_id,
+meter_tc_del_policer(struct netdev *netdev OVS_UNUSED,
+ ofproto_meter_id meter_id,
  struct ofputil_meter_stats *stats)
 {
 uint32_t police_index;
diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c
index 

[ovs-dev] [PATCH dpdk-latest v3 6/6] netdev-dpdk-offload: Add support for meter action

2023-03-30 Thread Simon Horman
From: Peng Zhang 

Add support of DPDK meter action logic.

Signed-off-by: Peng Zhang 
Signed-off-by: Jin Liu 
Co-authored-by: Jin Liu 
Signed-off-by: Simon Horman 
---
 lib/netdev-offload-dpdk.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index 144b406459dc..651120392a0f 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -2114,6 +2114,16 @@ parse_clone_actions(struct netdev *netdev,
 return 0;
 }
 
+static void OVS_UNUSED
+parse_meter_action(struct flow_actions *actions, uint32_t meter_id)
+{
+struct rte_flow_action_meter *rte_meter;
+
+rte_meter = xzalloc(sizeof *rte_meter);
+rte_meter->mtr_id = meter_id;
+add_flow_action(actions, RTE_FLOW_ACTION_TYPE_METER, rte_meter);
+}
+
 static void
 add_jump_action(struct flow_actions *actions, uint32_t group)
 {
@@ -2220,6 +2230,9 @@ parse_flow_actions(struct netdev *netdev,
 if (add_tnl_pop_action(netdev, actions, nla)) {
 return -1;
 }
+}  else if (nl_attr_type(nla) == OVS_ACTION_ATTR_METER) {
+uint32_t meter_id =  nl_attr_get_u32(nla);
+parse_meter_action(actions, meter_id);
 #endif
 } else {
 VLOG_DBG_RL(, "Unsupported action type %d", nl_attr_type(nla));
-- 
2.30.2

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


[ovs-dev] [PATCH dpdk-latest v3 4/6] netdev-offload-dpdk: Implement meter offload API for DPDK

2023-03-30 Thread Simon Horman
From: Peng Zhang 

For dpif-netdev, meters are mapped by DPDK meter with one-to-one
relationship. Implement meter offload API to set/get/del the DPDK
meter with proxy port id.

Signed-off-by: Peng Zhang 
Signed-off-by: Jin Liu 
Co-authored-by: Jin Liu 
Signed-off-by: Simon Horman 
---
 lib/netdev-dpdk.c | 202 ++
 lib/netdev-dpdk.h |  41 
 lib/netdev-offload-dpdk.c |  84 
 3 files changed, 327 insertions(+)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index d6f2f0517da6..cc2d0762226f 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -37,6 +37,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -5330,8 +5331,209 @@ netdev_dpdk_rte_flow_query_count(struct netdev *netdev,
 return ret;
 }
 
+static int OVS_UNUSED
+netdev_dpdk_meter_profile_init(struct rte_mtr_meter_profile *profile,
+   struct rte_mtr_capabilities *cap,
+   const uint64_t rate,
+   const uint64_t burst,
+   const int flag)
+{
+if (!cap->meter_srtcm_rfc2697_n_max) {
+return EOPNOTSUPP;
+}
+
+profile->alg = RTE_MTR_SRTCM_RFC2697;
+profile->packet_mode = flag;
+profile->srtcm_rfc2697.cir = rate;
+profile->srtcm_rfc2697.cbs = burst;
+profile->srtcm_rfc2697.ebs = burst;
+
+return 0;
+}
+
 #ifdef ALLOW_EXPERIMENTAL_API
 
+static int
+netdev_dpdk_rte_mtr_meter_add(struct rte_mtr_meter_profile *profile,
+  struct netdev *netdev,
+  uint32_t meter_id,
+  const uint32_t rate,
+  const uint32_t burst,
+  const int flag,
+  struct rte_mtr_error *error)
+{
+struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
+uint32_t meter_profile_id = meter_id;
+uint32_t meter_policy_id = meter_id;
+struct rte_mtr_capabilities cap;
+struct rte_mtr_stats mtr_stats;
+struct rte_mtr_params params;
+uint64_t stats_mask = 0;
+int prox_port_id;
+int clear = 0;
+int mod;
+int ret;
+
+memset(_stats, 0, sizeof(struct rte_mtr_stats));
+memset(, 0, sizeof(cap));
+
+ovs_mutex_lock(>mutex);
+
+prox_port_id = dev->flow_transfer_proxy_port_id;
+ret = rte_mtr_capabilities_get(prox_port_id, , error);
+if (ret) {
+goto out;
+}
+
+ret = netdev_dpdk_meter_profile_init(profile, , rate, burst, flag);
+if (ret) {
+goto out;
+}
+
+/* If can get the meter stats, the meter is offload in the HW.
+ * So the operate is mod, just update the meter_profile.
+ *
+ * If can't get the meter stats, the meter is not offload in the HW.
+ * So the operate is add, need create the profile, policy, mtr. */
+mod = rte_mtr_stats_read(prox_port_id, meter_id, _stats, _mask,
+ clear, error);
+ret = rte_mtr_meter_profile_add(prox_port_id, meter_profile_id, profile,
+error);
+if (!mod || ret) {
+goto out;
+}
+
+rte_mtr_policy_drop_red(policy);
+ret = rte_mtr_meter_policy_add(prox_port_id, meter_policy_id, ,
+   error);
+
+if (ret) {
+goto out;
+}
+
+memset(, 0 , sizeof(struct rte_mtr_params));
+params.meter_profile_id = meter_profile_id;
+params.meter_policy_id = meter_policy_id;
+params.stats_mask = cap.stats_mask;
+params.meter_enable = 1;
+
+ret = rte_mtr_create(prox_port_id, meter_id, , 1, error);
+out:
+ovs_mutex_unlock(>mutex);
+return ret;
+}
+
+int
+netdev_dpdk_meter_create(struct netdev *netdev,
+ const uint32_t meter_profile_id,
+ const uint64_t rate,
+ const uint64_t burst,
+ const int flag)
+{
+struct rte_mtr_meter_profile profile;
+struct rte_mtr_error error;
+int ret;
+
+memset(, 0 , sizeof(struct rte_mtr_meter_profile));
+memset(, 0 , sizeof(struct rte_mtr_error));
+
+ret = netdev_dpdk_rte_mtr_meter_add(, netdev, meter_profile_id,
+rate, burst, flag, );
+if (!ret) {
+if (!VLOG_DROP_DBG()) {
+VLOG_DBG("%s: rte_meter_id %d  port_id %d mtr create ",
+ netdev_get_name(netdev), meter_profile_id,
+ netdev_dpdk_get_prox_port_id(netdev));
+}
+} else {
+VLOG_DBG("%s: rte_mtr creation failed: %d (%s).",
+ netdev_get_name(netdev), error.type, error.message);
+}
+return ret;
+}
+
+int
+netdev_dpdk_meter_del(struct netdev *netdev,
+  const uint32_t meter_id,
+  const uint32_t meter_profile_id,
+  const uint32_t meter_policy_id)
+{
+struct netdev_dpdk *dev = 

[ovs-dev] [PATCH dpdk-latest v3 5/6] netdev-dpdk: add meter algorithms

2023-03-30 Thread Simon Horman
From: Peng Zhang 

Add the meter algorithms. DPDK meter support three algorithms,
and OVS also can support these algorithms.

Signed-off-by: Peng Zhang 
Signed-off-by: Jin Liu 
Co-authored-by: Jin Liu 
Signed-off-by: Simon Horman 
---
 lib/netdev-dpdk.c | 58 +--
 1 file changed, 51 insertions(+), 7 deletions(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index cc2d0762226f..2ce95aed2455 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -5331,6 +5331,50 @@ netdev_dpdk_rte_flow_query_count(struct netdev *netdev,
 return ret;
 }
 
+/* RTE_MTR_TRTCM_RFC2697 meter profile */
+static void
+netdev_dpdk_meter_profile_rfc2697_init(struct rte_mtr_meter_profile *profile,
+   const uint64_t rate,
+   const uint64_t burst,
+   const int flag)
+{
+   profile->alg = RTE_MTR_SRTCM_RFC2697;
+   profile->packet_mode = flag;
+   profile->srtcm_rfc2697.cir = rate;
+   profile->srtcm_rfc2697.cbs = burst;
+   profile->srtcm_rfc2697.ebs = burst;
+}
+
+/* RTE_MTR_TRTCM_RFC2698 meter profile */
+static void
+netdev_dpdk_meter_profile_rfc2698_init(struct rte_mtr_meter_profile *profile,
+   const uint64_t rate,
+   const uint64_t burst,
+   const int flag)
+{
+   profile->alg = RTE_MTR_TRTCM_RFC2698;
+   profile->packet_mode = flag;
+   profile->trtcm_rfc2698.cir = rate;
+   profile->trtcm_rfc2698.cbs = burst;
+   profile->trtcm_rfc2698.pir = rate;
+   profile->trtcm_rfc2698.pbs = burst;
+}
+
+/* RTE_MTR_TRTCM_RFC2698 meter profile */
+static void
+netdev_dpdk_meter_profile_rfc4115_init(struct rte_mtr_meter_profile *profile,
+   const uint64_t rate,
+   const uint64_t burst,
+   const int flag)
+{
+   profile->alg = RTE_MTR_TRTCM_RFC4115;
+   profile->packet_mode = flag;
+   profile->trtcm_rfc4115.cir = rate;
+   profile->trtcm_rfc4115.cbs = burst;
+   profile->trtcm_rfc4115.eir = rate;
+   profile->trtcm_rfc4115.ebs = burst;
+}
+
 static int OVS_UNUSED
 netdev_dpdk_meter_profile_init(struct rte_mtr_meter_profile *profile,
struct rte_mtr_capabilities *cap,
@@ -5338,16 +5382,16 @@ netdev_dpdk_meter_profile_init(struct 
rte_mtr_meter_profile *profile,
const uint64_t burst,
const int flag)
 {
-if (!cap->meter_srtcm_rfc2697_n_max) {
+if (cap->meter_srtcm_rfc2697_n_max) {
+netdev_dpdk_meter_profile_rfc2697_init(profile, rate, burst, flag);
+} else if (cap->meter_trtcm_rfc2698_n_max) {
+netdev_dpdk_meter_profile_rfc2698_init(profile, rate, burst, flag);
+} else if (cap->meter_trtcm_rfc4115_n_max) {
+netdev_dpdk_meter_profile_rfc4115_init(profile, rate, burst, flag);
+} else {
 return EOPNOTSUPP;
 }
 
-profile->alg = RTE_MTR_SRTCM_RFC2697;
-profile->packet_mode = flag;
-profile->srtcm_rfc2697.cir = rate;
-profile->srtcm_rfc2697.cbs = burst;
-profile->srtcm_rfc2697.ebs = burst;
-
 return 0;
 }
 
-- 
2.30.2

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


[ovs-dev] [PATCH dpdk-latest v3 3/6] dpif-netdev: Offloading meter with DPDK

2023-03-30 Thread Simon Horman
From: Peng Zhang 

OVS-DPDK meters are created in advance and OpenFlow rules refer to them by
their unique ID. A new API is used to offload them. By calling the API,
meters are created and try to be offload by port in the bridge with the
proxy port id.

Signed-off-by: Peng Zhang 
Signed-off-by: Jin Liu 
Co-authored-by: Jin Liu 
Signed-off-by: Simon Horman 
---
 lib/dpif-netdev.c | 22 +-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 2c08a71c8db2..e8d0ca6606de 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -7277,6 +7277,11 @@ dpif_netdev_meter_set(struct dpif *dpif, 
ofproto_meter_id meter_id,
 
 ovs_mutex_unlock(>meters_lock);
 
+if (netdev_is_flow_api_enabled()) {
+dpdk_meter_offload_set(dpif_normalize_type(dpif_type(dpif)),
+   meter_id, config);
+}
+
 return 0;
 }
 
@@ -7313,8 +7318,18 @@ dpif_netdev_meter_get(const struct dpif *dpif,
 
 ovs_mutex_unlock(>lock);
 stats->n_bands = i;
-}
 
+if (netdev_is_flow_api_enabled()) {
+dpdk_meter_offload_get(dpif_normalize_type(dpif_type(dpif)),
+   meter_id_, stats);
+
+/* nit: Meter offload currently only supports one band */
+if (meter->n_bands) {
+stats->bands[0].packet_count = stats->packet_in_count;
+stats->bands[0].byte_count = stats->byte_in_count;
+}
+}
+}
 return 0;
 }
 
@@ -7330,6 +7345,11 @@ dpif_netdev_meter_del(struct dpif *dpif,
 if (!error) {
 uint32_t meter_id = meter_id_.uint32;
 
+if (netdev_is_flow_api_enabled()) {
+dpdk_meter_offload_del(dpif_normalize_type(dpif_type(dpif)),
+   meter_id_, stats);
+}
+
 ovs_mutex_lock(>meters_lock);
 dp_meter_detach_free(>meters, meter_id);
 ovs_mutex_unlock(>meters_lock);
-- 
2.30.2

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


[ovs-dev] [PATCH dpdk-latest v3 1/6] netdev-offload-dpdk: use flow transfer proxy

2023-03-30 Thread Simon Horman
From: Peng Zhang 

Manage "transfer" flows via the corresponding mechanism.
Doing so requires that the traffic source be specified
explicitly, via the corresponding pattern item.

Signed-off-by: Peng Zhang 
Signed-off-by: Jin Liu 
Co-authored-by: Jin Liu 
Signed-off-by: Simon Horman 
---
 lib/netdev-dpdk.c | 60 +--
 lib/netdev-dpdk.h |  2 ++
 2 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index fff57f78279a..d6f2f0517da6 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -437,6 +437,7 @@ enum dpdk_hw_ol_features {
 struct netdev_dpdk {
 PADDED_MEMBERS_CACHELINE_MARKER(CACHE_LINE_SIZE, cacheline0,
 dpdk_port_t port_id;
+dpdk_port_t flow_transfer_proxy_port_id;
 
 /* If true, device was attached by rte_eth_dev_attach(). */
 bool attached;
@@ -1155,6 +1156,24 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
 uint32_t rx_chksm_offload_capa = RTE_ETH_RX_OFFLOAD_UDP_CKSUM |
  RTE_ETH_RX_OFFLOAD_TCP_CKSUM |
  RTE_ETH_RX_OFFLOAD_IPV4_CKSUM;
+int ret;
+
+/* Managing "transfer" flows requires that the user communicate them
+ * via a port which has the privilege to control the embedded switch.
+ * For some vendors, all ports in a given switching domain have
+ * this privilege. For other vendors, it's only one port.
+ *
+ * Get the proxy port ID and remember it for later use.
+ */
+ret = rte_flow_pick_transfer_proxy(dev->port_id,
+   >flow_transfer_proxy_port_id,
+   NULL);
+if (ret != 0) {
+/* The PMD does not indicate the proxy port.
+ * Assume the proxy is unneeded.
+ */
+dev->flow_transfer_proxy_port_id = dev->port_id;
+}
 
 rte_eth_dev_info_get(dev->port_id, );
 
@@ -3769,6 +3788,7 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc 
OVS_UNUSED,
 struct ds used_interfaces = DS_EMPTY_INITIALIZER;
 struct rte_eth_dev_info dev_info;
 dpdk_port_t sibling_port_id;
+struct netdev_dpdk *dev;
 dpdk_port_t port_id;
 bool used = false;
 char *response;
@@ -3786,8 +3806,6 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc 
OVS_UNUSED,
   argv[1]);
 
 RTE_ETH_FOREACH_DEV_SIBLING (sibling_port_id, port_id) {
-struct netdev_dpdk *dev;
-
 LIST_FOR_EACH (dev, list_node, _list) {
 if (dev->port_id != sibling_port_id) {
 continue;
@@ -3807,6 +3825,17 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc 
OVS_UNUSED,
 }
 ds_destroy(_interfaces);
 
+/* The device being detached may happen to be a flow proxy port
+ * for another device (still attached). Update the flow proxy port id,
+ * indicate that the device being detached no longer needs a flow proxy.
+ */
+LIST_FOR_EACH (dev, list_node, _list) {
+if (dev->port_id == port_id) {
+dev->flow_transfer_proxy_port_id = port_id;
+break;
+}
+}
+
 rte_eth_dev_info_get(port_id, _info);
 rte_eth_dev_close(port_id);
 if (rte_dev_remove(dev_info.device) < 0) {
@@ -3817,6 +3846,16 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc 
OVS_UNUSED,
 response = xasprintf("All devices shared with device '%s' "
  "have been detached", argv[1]);
 
+/* The device being detached may happen to be a flow proxy port.
+ * After the flow proxy port was detached, the related ports
+ * will reconfigure the device and update the proxy_port_id.
+ */
+LIST_FOR_EACH (dev, list_node, _list) {
+ if (dev->flow_transfer_proxy_port_id == port_id) {
+netdev_request_reconfigure(>up);
+}
+}
+
 ovs_mutex_unlock(_mutex);
 unixctl_command_reply(conn, response);
 free(response);
@@ -5192,6 +5231,23 @@ out:
 return ret;
 }
 
+int
+netdev_dpdk_get_prox_port_id(struct netdev *netdev)
+{
+struct netdev_dpdk *dev;
+int ret = -1;
+
+if (!is_dpdk_class(netdev->netdev_class)) {
+return ret;
+}
+
+dev = netdev_dpdk_cast(netdev);
+ovs_mutex_lock(>mutex);
+ret = dev->flow_transfer_proxy_port_id;
+ovs_mutex_unlock(>mutex);
+return ret;
+}
+
 bool
 netdev_dpdk_flow_api_supported(struct netdev *netdev)
 {
diff --git a/lib/netdev-dpdk.h b/lib/netdev-dpdk.h
index 5cd95d00f5a5..277db156fe06 100644
--- a/lib/netdev-dpdk.h
+++ b/lib/netdev-dpdk.h
@@ -51,6 +51,8 @@ netdev_dpdk_rte_flow_query_count(struct netdev *netdev,
  struct rte_flow_error *error);
 int
 netdev_dpdk_get_port_id(struct netdev *netdev);
+int
+netdev_dpdk_get_prox_port_id(struct netdev *netdev);
 
 #ifdef ALLOW_EXPERIMENTAL_API
 
-- 
2.30.2

___
dev mailing list
d...@openvswitch.org

[ovs-dev] [PATCH dpdk-latest v3 0/6] Add support for DPDK meter HW offload

2023-03-30 Thread Simon Horman
Hi,

this series adds support for DPDK meter HW offload.

Changes between v2 and v3.
* Use common API for DPDK and non-DPDK meter offloads
* Make use of netdev_ports_traverse to offload the meter
* Add dpdk-latest to subject prefix

Changes between v1 and v2:
* Add the prox mechanism: add the meter by proxy id
* Change the offload interface from netdev-dpdk to the netdev-offload
* Changed base to dpdk-latest branch

Peng Zhang (6):
  netdev-offload-dpdk: use flow transfer proxy
  netdev-offload: Let meter offload API can be used with DPDK
  dpif-netdev: Offloading meter with DPDK
  netdev-offload-dpdk: Implement meter offload API for DPDK
  netdev-dpdk: add meter algorithms
  netdev-dpdk-offload: Add support for meter action

 Documentation/howto/dpdk.rst  |   5 +-
 lib/dpif-netdev.c |  22 ++-
 lib/netdev-dpdk.c | 306 +-
 lib/netdev-dpdk.h |  43 +
 lib/netdev-offload-dpdk.c |  97 +++
 lib/netdev-offload-provider.h |  21 ++-
 lib/netdev-offload-tc.c   |   9 +-
 lib/netdev-offload.c  | 135 ++-
 lib/netdev-offload.h  |   9 +
 9 files changed, 633 insertions(+), 14 deletions(-)

-- 
2.30.2

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


Re: [ovs-dev] [RFC ovn 0/2] ACL Sampling using per-flow IPFIX

2023-03-30 Thread Dumitru Ceara
On 3/21/23 10:48, Adrian Moreno wrote:
> 
> 
> On 3/17/23 20:59, Numan Siddique wrote:
>> On Tue, Oct 18, 2022 at 12:00 PM Adrian Moreno 
>> wrote:
>>>
>>> Based on the introduction of the OVN "sample" action (still WIP) [1],
>>> the proposal of this RFC is to use per-flow IPFIX sampling to increase
>>> visibility on ACLs.
>>>
>>> The idea of ACL sampling is very similar to the already existing ACL
>>> logging whith the following key differences:
>>>
>>> - Using IPFIX sampling collects header information of the actual packet
>>>    that was dropped / accepted by the ACL. This information is key to
>>>    debug an issue or understand the traffic profile that traverses the
>>>    ACLs.
>>>
>>> - With ACL logging, the information goes to the ovn-controller,
>>>    adding pressure to it. Using IPFIX sampling can offload the
>>>    ovn-controller by sending samples to external IPFIX collectors.
>>>
>>> - Using the sample action, we don't need to rely on a meter to limit the
>>>    amount of data we process since we have the sampling
>>> rate/probability.
>>>
>>> - Using IPFIX as standard format makes the solution interoperable so
>>>    it's possible to combine with other IPFIX sources to build
>>>    comprehensive observability tools.
>>>
>>> This RFC includes a prototype implementation based on the creation of a
>>> new NBDB table "Sample" and a reference to it from the ACL table. This
>>> would allow the use of per-flow IPFIX sampling to add visibility to
>>> other areas of OVN as the needs arise.
>>>
>>> [1]
>>> https://patchwork.ozlabs.org/project/ovn/patch/20221017131403.563877-2-amore...@redhat.com/
>>>
>>>
>>> Adrian Moreno (2):
>>>    northd: add ACL Sampling
>>>    ovn-nbctl: add sample to acl-add
>>
>> Hi Adrian,
>>
>> Do you plan to submit formal patches ?  Or you're expecting any
>> feedback on this series before submitting formally ?
>>
>> If so,  I can take a look at the rfc patches.
>>
> 
> Hi Numan,
> 
> I am planning to do some performance benchmarking and add it to the
> formal patch but I would love to get some general feedback on the topic.
> Whether the approach seems sane (adding sample actions in ACL lflows),
> whether the general NBDB API is going in the proper direction or if
> there is some pitfall I'm ignoring. Of course I'm not asking for a full
> review but a general go/no-go would be nice.

Hi Adrian,

I'm not sure if Numan has other comments.  FWIW I had a look at the two
patches in the series and I think the direction is ok.  I did share some
comments on patch 1/2.

Thanks for working on this!

Regards,
Dumitru

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


Re: [ovs-dev] [RFC ovn 1/2] northd: add ACL Sampling

2023-03-30 Thread Dumitru Ceara
On 10/18/22 17:59, Adrian Moreno wrote:
> Introduce a new table called Sample where per-flow IPFIX configuration
> can be specified.
> Also, reference rows from such table from the ACL table to enable the
> configuration of ACL sampling. If enabled, northd will add a sample
> action to each ACL-related logical flow.
> 
> Signed-off-by: Adrian Moreno 
> ---
>  northd/northd.c  | 31 ++-
>  ovn-nb.ovsschema | 23 ++-
>  ovn-nb.xml   | 31 +++
>  3 files changed, 83 insertions(+), 2 deletions(-)
> 
> diff --git a/northd/northd.c b/northd/northd.c
> index 61d474840..3e09e3a0f 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -6194,6 +6194,27 @@ build_acl_log(struct ds *actions, const struct 
> nbrec_acl *acl,
>  ds_put_cstr(actions, "); ");
>  }
>  
> +static void
> +build_acl_sample(struct ds *actions, const struct nbrec_acl *acl)
> +{
> +if (!acl->sample) {
> +return;
> +}
> +ds_put_format(actions, "sample(probability=%"PRId16","
> +   "collector_set=%d,"
> +   "obs_domain=%hd,",
> +(uint16_t) acl->sample->probability,
> +(uint32_t) acl->sample->collector_set_id,
> +(uint8_t) acl->sample->obs_domain_id);
> +
> +if (acl->sample->obs_point_id) {
> +ds_put_format(actions, "obs_point=%"PRId32");",
> +  (uint32_t) *acl->sample->obs_point_id);
> +} else {
> +ds_put_cstr(actions, "obs_point=$cookie);");
> +}
> +}
> +
>  static void
>  build_reject_acl_rules(struct ovn_datapath *od, struct hmap *lflows,
> enum ovn_stage stage, struct nbrec_acl *acl,
> @@ -6260,6 +6281,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath 
> *od,
>  if (!strcmp(acl->action, "allow-stateless")) {
>  ds_clear(actions);
>  build_acl_log(actions, acl, meter_groups);
> +build_acl_sample(actions, acl);
>  ds_put_cstr(actions, "next;");
>  ovn_lflow_add_with_hint(lflows, od, stage,
>  acl->priority + OVN_ACL_PRI_OFFSET,
> @@ -6275,6 +6297,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath 
> *od,
>  if (!has_stateful) {
>  ds_clear(actions);
>  build_acl_log(actions, acl, meter_groups);
> +build_acl_sample(actions, acl);
>  ds_put_cstr(actions, "next;");
>  ovn_lflow_add_with_hint(lflows, od, stage,
>  acl->priority + OVN_ACL_PRI_OFFSET,
> @@ -6304,6 +6327,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath 
> *od,
>REG_LABEL" = %"PRId64"; ", acl->label);
>  }
>  build_acl_log(actions, acl, meter_groups);
> +build_acl_sample(actions, acl);
>  ds_put_cstr(actions, "next;");
>  ovn_lflow_add_with_hint(lflows, od, stage,
>  acl->priority + OVN_ACL_PRI_OFFSET,
> @@ -6329,6 +6353,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath 
> *od,
>REG_LABEL" = %"PRId64"; ", acl->label);
>  }
>  build_acl_log(actions, acl, meter_groups);
> +build_acl_sample(actions, acl);
>  ds_put_cstr(actions, "next;");
>  ovn_lflow_add_with_hint(lflows, od, stage,
>  acl->priority + OVN_ACL_PRI_OFFSET,
> @@ -6349,7 +6374,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath 
> *od,
>   */
>  bool log_related = smap_get_bool(>options, "log-related",
>   false);
> -if (acl->log && acl->label && log_related) {
> +if ((acl->log || acl->sample) && acl->label && log_related) {
>  /* Related/reply flows need to be set on the opposite 
> pipeline
>   * from where the ACL itself is set.
>   */
> @@ -6365,6 +6390,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath 
> *od,
>use_ct_inv_match ? " && !ct.inv" : "",
>ct_blocked_match, acl->label);
>  build_acl_log(actions, acl, meter_groups);
> +build_acl_sample(actions, acl);
>  ds_put_cstr(actions, "next;");
>  ovn_lflow_add_with_hint(lflows, od, log_related_stage,
>  UINT16_MAX - 2,
> @@ -6402,6 +6428,7 @@ consider_acl(struct hmap *lflows, struct ovn_datapath 
> *od,
>  } else {
>  ds_put_format(match, " && (%s)", acl->match);
>  build_acl_log(actions, acl, meter_groups);
> +build_acl_sample(actions, acl);
>  ds_put_cstr(actions, debug_implicit_drop_action());
> 

Re: [ovs-dev] [PATCH ovn v3 2/2] Add fake multinode system tests.

2023-03-30 Thread Simon Horman
On Wed, Mar 29, 2023 at 10:02:21AM -0400, num...@ovn.org wrote:
> From: Numan Siddique 
> 
> This patch adds a simple system test using ovn-fake-multinode
> setup.  The tests can be run as - 'make check-multinode'
> 
> Before running these tests, user should deploy fake_multinode setup
> by running 'ovn_cluster.sh start'.
> 
> This test suite is also triggered for the newly added fake multinode CI
> job.
> 
> The fake multinode system tests suite can be enhanced further for new
> features and to cover multi node scenarios.
> 
> Signed-off-by: Numan Siddique 

Reviewed-by: Simon Horman 

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


Re: [ovs-dev] [PATCH ovn v3 1/2] CI: Add a couple of periodic jobs using ovn-fake-multinode.

2023-03-30 Thread Simon Horman
On Wed, Mar 29, 2023 at 10:01:57AM -0400, num...@ovn.org wrote:
> From: Numan Siddique 
> 
> This patch adds a couple of jobs using ovn-fake-multinode.
> It first builds 2 ovn-fake-multinode container images
>   - one with OVN 22.03
>   - one with present main.
> 
> The first job deploys ovn-fake-multinode with the main
> OVN and runs simple tests provided by ovn-fake-multinode [1].
> 
> The second job deploys ovn-fake-multinode setup with the
> central image using OVN 22.03 and chassis image using main OVN.
> This job tests the scenario
>   - ovn-northd and OVN dbs are running the most recent LTS.
>   - ovn-controller is running the latest commit from the branch.
> 
> The workflow is right now scheduled to trigger on midnight everyday.
> Once we cache the built image or reduce the overall run time of
> this workflow we can enable for every push.
> 
> [1] - 
> https://github.com/ovn-org/ovn-fake-multinode/blob/main/.ci/test_basic.sh
> 
> Signed-off-by: Numan Siddique 

Reviewed-by: Simon Horman 

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


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

2023-03-30 Thread Simon Horman
On Fri, Mar 17, 2023 at 09:47:36PM +0100, Eelco Chaudron wrote:
> 
> 
> Send from my phone
> 
> > Op 17 mrt. 2023 om 21:11 heeft Marcelo Ricardo Leitner 
> >  het volgende geschreven:
> > 
> > On Thu, Mar 16, 2023 at 09:51:34AM +0100, Eelco Chaudron wrote:
> >> 
> >> 
> >>> On 22 Dec 2022, at 13:32, Eelco Chaudron wrote:
> >>> 
>  On 22 Dec 2022, at 10:26, Balazs Nemeth wrote:
> >>> 
>  The only way that stats->{n_packets,n_bytes} would decrease is due to an

nit: s/way/ways/

>  overflow, or if there are bugs in how statistics are handled. In the
>  past, there were multiple bugs 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.

1. Perhaps it would be prudent to log/count if/when this occurs
2. I take it that the overflow handling would be follow-up work,
   is that correct?

>  Signed-off-by: Balazs Nemeth 
>  ---
>  
>  Depends on:
>  - netdev-offload-tc: Preserve tc statistics when flow gets modified.
>  - tc: Add support for TCA_STATS_PKT64
>  - ofproto-dpif-upcall: Wait for valid hw flow stats before applying 
>  min-revalidate-pps
>  - ofproto-dpif-upcall: Use last known stats ukey stats on revalidate 
>  missed dp flows.
> >>> 
> >>> Did a lot of tests with this patch applied, but as mentioned it does need 
> >>> all four patches mentioned above applied first.
> >>> 
> >>> Acked-by: Eelco Chaudron 
> >> 
> >> Now that all the dependent patches have been applied and backported to 
> >> 2.17, can we have another look at this patch?
> > 
> > Not sure what you mean here, Eelco. You mean, just a patch review or
> > you mean something else, like evaluating if the patch is still needed
> > at all?
> 
> Guess I was not clear enough :) what I meant was for the maintainers to
> have another look at this patch and if no further objections apply it.

There seems to now be minor fuzz when applying this patch (locally).
Perhaps a rebase is in order?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


[ovs-dev] [PATCH v10] netdev-offload-tc: del ufid mapping if device not exist.

2023-03-30 Thread Faicker Mo
The device may be deleted and added with ifindex changed.
The tc rules on the device will be deleted if the device is deleted.
The func tc_del_filter will fail when flow del. The mapping of
ufid to tc will not be deleted.
The traffic will trigger the same flow(with same ufid) to put to tc
on the new device. Duplicated ufid mapping will be added.
If the hashmap is expanded, the old mapping entry will be the first entry,
and now the dp flow can't be deleted.

Signed-off-by: Faicker Mo 
---
v2:
- Add tc offload test case
v3:
- No change
v4:
- No change
v5:
- No change
v6:
- No change
v7:
- Minor fix for test case and rebased
v8:
- Shorten the time of the test case
v9:
- Remove sleep in the test case
v10:
- Improve the success rate of the test case
---
 lib/netdev-offload-tc.c  |  3 +-
 tests/system-offloads-traffic.at | 55 
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 4721f0160..c9662081f 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -276,8 +276,9 @@ del_filter_and_ufid_mapping(struct tcf_id *id, const 
ovs_u128 *ufid,
 }
 
 err = tc_del_flower_filter(id);
-if (!err) {
+if (!err || err == ENODEV) {
 del_ufid_tc_mapping(ufid);
+return 0;
 }
 return err;
 }
diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at
index eb331d6ce..da18597cd 100644
--- a/tests/system-offloads-traffic.at
+++ b/tests/system-offloads-traffic.at
@@ -750,3 +750,58 @@ AT_CHECK([ovs-appctl coverage/read-counter 
ukey_invalid_stat_reset], [0], [dnl
 
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
+
+AT_SETUP([offloads - delete ufid mapping if device not exist - offloads 
enabled])
+OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch . 
other_config:hw-offload=true])
+
+AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
+
+ADD_NAMESPACES(at_ns0, at_ns1, at_ns2)
+
+dnl Disable IPv6 to skip unexpected flow
+AT_CHECK([sysctl -w net.ipv6.conf.br0.disable_ipv6=1], [0], [ignore])
+NS_CHECK_EXEC([at_ns0], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], 
[ignore])
+NS_CHECK_EXEC([at_ns1], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], 
[ignore])
+NS_CHECK_EXEC([at_ns2], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], 
[ignore])
+
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "aa:1a:54:e9:c5:56")
+ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
+
+NS_CHECK_EXEC([at_ns0], [ping -q -c 2 -i 0.2 10.1.1.2 | FORMAT_PING], [0], [dnl
+2 packets transmitted, 2 received, 0% packet loss, time 0ms
+])
+
+dnl Delete and add interface ovs-p0/p0
+AT_CHECK([ip link del dev ovs-p0])
+AT_CHECK([ip link add p0 type veth peer name ovs-p0 || return 77])
+AT_CHECK([ip link set p0 netns at_ns0])
+AT_CHECK([ip link set dev ovs-p0 up])
+NS_CHECK_EXEC([at_ns0], [ip addr add dev p0 "10.1.1.1/24"])
+NS_CHECK_EXEC([at_ns0], [ip link set dev p0 up])
+NS_CHECK_EXEC([at_ns0], [ip link set dev p0 address "aa:1a:54:e9:c5:56"])
+
+AT_CHECK([ovs-appctl revalidator/purge], [0])
+
+dnl Generate flows to trigger the hmap expand once
+ADD_VETH(p2, at_ns2, br0, "10.1.1.3/24")
+NS_CHECK_EXEC([at_ns0], [ping -q -c 2 -i 0.2 10.1.1.2 | FORMAT_PING], [0], [dnl
+2 packets transmitted, 2 received, 0% packet loss, time 0ms
+])
+NS_CHECK_EXEC([at_ns0], [ping -q -c 2 -i 0.2 10.1.1.3 | FORMAT_PING], [0], [dnl
+2 packets transmitted, 2 received, 0% packet loss, time 0ms
+])
+
+AT_CHECK([ovs-appctl revalidator/purge], [0])
+dnl Fix purge fail occasionally
+AT_CHECK([ovs-appctl revalidator/purge], [0])
+
+AT_CHECK([test $(ovs-appctl dpctl/dump-flows | grep -c "eth_type(0x0800)") -eq 
0], [0], [ignore])
+
+OVS_TRAFFIC_VSWITCHD_STOP(["/could not open network device ovs-p0/d
+/on nonexistent port/d
+/failed to flow_get/d
+/Failed to acquire udpif_key/d
+/No such device/d
+/failed to offload flow/d
+"])
+AT_CLEANUP
-- 
2.39.1



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


[ovs-dev] [PATCH] ci: Separate DPDK from OVS build.

2023-03-30 Thread David Marchand
Let's separate DPDK compilation from the rest of OVS build:
- this avoids multiple jobs building DPDK in parallel, which especially
  affects builds in the dpdk-latest branch,
- we separate concerns about DPDK build requirements from OVS build
  requirements, like python dependencies,
- building DPDK does not depend on how we will link OVS against it, so we
  can use a single cache entry regardless of DPDK_SHARED option,

Signed-off-by: David Marchand 
---
 .ci/dpdk-build.sh| 68 
 .ci/dpdk-prepare.sh  | 11 +
 .ci/linux-build.sh   | 64 ++
 .ci/linux-prepare.sh |  3 +-
 .github/workflows/build-and-test.yml | 66 +--
 Makefile.am  |  2 +
 6 files changed, 150 insertions(+), 64 deletions(-)
 create mode 100755 .ci/dpdk-build.sh
 create mode 100755 .ci/dpdk-prepare.sh

diff --git a/.ci/dpdk-build.sh b/.ci/dpdk-build.sh
new file mode 100755
index 00..71da4a2dfe
--- /dev/null
+++ b/.ci/dpdk-build.sh
@@ -0,0 +1,68 @@
+#!/bin/bash
+
+set -o errexit
+set -x
+
+function build_dpdk()
+{
+local VERSION_FILE="dpdk-dir/cached-version"
+local DPDK_VER=$1
+local DPDK_OPTS=""
+
+if [ "${DPDK_VER##refs/*/}" != "${DPDK_VER}" ]; then
+# Avoid using cache for git tree build.
+rm -rf dpdk-dir
+
+DPDK_GIT=${DPDK_GIT:-https://dpdk.org/git/dpdk}
+git clone --single-branch $DPDK_GIT dpdk-dir -b "${DPDK_VER##refs/*/}"
+pushd dpdk-dir
+git log -1 --oneline
+else
+if [ -f "${VERSION_FILE}" ]; then
+VER=$(cat ${VERSION_FILE})
+if [ "${VER}" = "${DPDK_VER}" ]; then
+echo "Found already built DPDK ${VER} build in $(pwd)/dpdk-dir"
+return
+fi
+fi
+# No cache or version mismatch.
+rm -rf dpdk-dir
+wget https://fast.dpdk.org/rel/dpdk-$1.tar.xz
+tar xvf dpdk-$1.tar.xz > /dev/null
+DIR_NAME=$(tar -tf dpdk-$1.tar.xz | head -1 | cut -f1 -d"/")
+mv ${DIR_NAME} dpdk-dir
+pushd dpdk-dir
+fi
+
+# Switching to 'default' machine to make dpdk-dir cache usable on
+# different CPUs. We can't be sure that all CI machines are exactly same.
+DPDK_OPTS="$DPDK_OPTS -Dmachine=default"
+
+# Disable building DPDK unit tests. Not needed for OVS build or tests.
+DPDK_OPTS="$DPDK_OPTS -Dtests=false"
+
+# Disable DPDK developer mode, this results in less build checks and less
+# meson verbose outputs.
+DPDK_OPTS="$DPDK_OPTS -Ddeveloper_mode=disabled"
+
+# OVS compilation and "normal" unit tests (run in the CI) do not depend on
+# any DPDK driver being present.
+# We can disable all drivers to save compilation time.
+DPDK_OPTS="$DPDK_OPTS -Ddisable_drivers=*/*"
+
+# Install DPDK using prefix.
+DPDK_OPTS="$DPDK_OPTS --prefix=$(pwd)/build"
+
+meson $DPDK_OPTS build
+ninja -C build
+ninja -C build install
+
+echo "Installed DPDK source in $(pwd)"
+popd
+echo "${DPDK_VER}" > ${VERSION_FILE}
+}
+
+if [ -z "$DPDK_VER" ]; then
+DPDK_VER="22.11.1"
+fi
+build_dpdk $DPDK_VER
diff --git a/.ci/dpdk-prepare.sh b/.ci/dpdk-prepare.sh
new file mode 100755
index 00..f7e6215dda
--- /dev/null
+++ b/.ci/dpdk-prepare.sh
@@ -0,0 +1,11 @@
+#!/bin/bash
+
+set -ev
+
+# Installing wheel separately because it may be needed to build some
+# of the packages during dependency backtracking and pip >= 22.0 will
+# abort backtracking on build failures:
+# https://github.com/pypa/pip/issues/10655
+pip3 install --disable-pip-version-check --user wheel
+pip3 install --disable-pip-version-check --user pyelftools
+pip3 install --user  'meson==0.53.2'
diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index 10021fddb2..99850a9434 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -9,9 +9,7 @@ EXTRA_OPTS="--enable-Werror"
 
 function install_dpdk()
 {
-local DPDK_VER=$1
 local VERSION_FILE="dpdk-dir/cached-version"
-local DPDK_OPTS=""
 local DPDK_LIB=$(pwd)/dpdk-dir/build/lib/x86_64-linux-gnu
 
 if [ "$DPDK_SHARED" ]; then
@@ -24,63 +22,14 @@ function install_dpdk()
 # Export the following path for pkg-config to find the .pc file.
 export PKG_CONFIG_PATH=$DPDK_LIB/pkgconfig/:$PKG_CONFIG_PATH
 
-if [ "${DPDK_VER##refs/*/}" != "${DPDK_VER}" ]; then
-# Avoid using cache for git tree build.
-rm -rf dpdk-dir
-
-DPDK_GIT=${DPDK_GIT:-https://dpdk.org/git/dpdk}
-git clone --single-branch $DPDK_GIT dpdk-dir -b "${DPDK_VER##refs/*/}"
-pushd dpdk-dir
-git log -1 --oneline
-else
-if [ -f "${VERSION_FILE}" ]; then
-VER=$(cat ${VERSION_FILE})
-if [ "${VER}" = "${DPDK_VER}" ]; then
-# Update the library paths.
-sudo ldconfig
-echo "Found cached DPDK ${VER} build in 

[ovs-dev] [PATCH v2] learning-switch: Fix coredump of OpenFlow15 learning-switch

2023-03-30 Thread Faicker Mo
The OpenFlow15 Packet-Out message contains the match instead of the in_port.
The flow.tunnel.metadata.tab is not inited but used in the loop of 
tun_metadata_to_nx_match.

The coredump gdb backtrace is:
 #0  memcpy_from_metadata (dst=dst@entry=0x7ffcfac2f060, 
src=src@entry=0x7ffcfac30880, loc=loc@entry=0x10) at lib/tun-metadata.c:467
 #1  0x004506e8 in metadata_loc_from_match_read (match=0x7ffcfac30598, 
is_masked=, mask=0x7ffcfac30838, idx=0, map=0x0) at 
lib/tun-metadata.c:865
 #2  metadata_loc_from_match_read (is_masked=, 
mask=0x7ffcfac30838, idx=0, match=0x7ffcfac30598, map=0x0) at 
lib/tun-metadata.c:854
 #3  tun_metadata_to_nx_match (b=b@entry=0x892260, oxm=oxm@entry=OFP15_VERSION, 
match=match@entry=0x7ffcfac30598) at lib/tun-metadata.c:888
 #4  0x0047c1f8 in nx_put_raw (b=b@entry=0x892260, 
oxm=oxm@entry=OFP15_VERSION, match=match@entry=0x7ffcfac30598, 
cookie=, cookie@entry=0,
 cookie_mask=, cookie_mask@entry=0) at lib/nx-match.c:1186
 #5  0x0047d693 in oxm_put_match (b=b@entry=0x892260, 
match=match@entry=0x7ffcfac30598, version=version@entry=OFP15_VERSION) at 
lib/nx-match.c:1343
 #6  0x0043194e in ofputil_encode_packet_out 
(po=po@entry=0x7ffcfac30580, protocol=) at lib/ofp-packet.c:1226
 #7  0x0040a4fe in process_packet_in (sw=sw@entry=0x891d70, 
oh=) at lib/learning-switch.c:619
 #8  0x0040acdc in lswitch_process_packet (msg=0x892210, sw=0x891d70) 
at lib/learning-switch.c:374
 #9  lswitch_run (sw=0x891d70) at lib/learning-switch.c:324
 #10 0x00406f26 in main (argc=, argv=) at 
utilities/ovs-testcontroller.c:180

Fix that by zeroing out the po variable.

Fixes: 577bfa9f6879 ("ofp-util: Add OpenFlow 1.5 packet-out support")
Signed-off-by: Faicker Mo 
---
v2:
- changed the init and add test case
---
 lib/learning-switch.c   |  2 +-
 tests/system-traffic.at | 21 +
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/lib/learning-switch.c b/lib/learning-switch.c
index 8102475ca..275c30e26 100644
--- a/lib/learning-switch.c
+++ b/lib/learning-switch.c
@@ -524,7 +524,7 @@ process_packet_in(struct lswitch *sw, const struct 
ofp_header *oh)
 uint64_t ofpacts_stub[64 / 8];
 struct ofpbuf ofpacts;
 
-struct ofputil_packet_out po;
+struct ofputil_packet_out po = {0};
 enum ofperr error;
 
 struct dp_packet pkt;
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 2558f3b24..7de26840c 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -7845,3 +7845,24 @@ OVS_WAIT_UNTIL([cat p2.pcap | grep -E "0x0050: * 
* *5002 *2000 *b85e *00
 
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
+
+AT_BANNER([learning-switch])
+AT_SETUP([learning-switch - OpenFlow15])
+dnl Start ovs-testcontroller
+AT_CHECK([ovs-testcontroller --no-chdir --detach punix:controller --pidfile -v 
ptcp:], [0], [ignore])
+OVS_WAIT_UNTIL([test -e controller])
+dnl Setup ovs
+OVS_TRAFFIC_VSWITCHD_START([-- set bridge br0 protocols=OpenFlow15 -- 
set-controller br0 tcp:127.0.0.1:6653])
+
+ADD_NAMESPACES(at_ns0,  at_ns1)
+
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
+ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
+
+NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2 | FORMAT_PING], 
[0], [dnl
+10 packets transmitted, 10 received, 0% packet loss, time 0ms
+])
+kill `cat ovs-testcontroller.pid`
+OVS_WAIT_UNTIL([! test -e controller])
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
-- 
2.39.1



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


Re: [ovs-dev] [PATCH v2 2/6] netdev-offload: Add DPDK meter offload API

2023-03-30 Thread Simon Horman
On Wed, Mar 22, 2023 at 09:35:29PM +0100, Ilya Maximets wrote:
> On 3/9/23 14:02, Simon Horman wrote:
> > From: Peng Zhang 

...

> > diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h
> > index 9108856d18d1..7ecbb8d026f1 100644
> > --- a/lib/netdev-offload-provider.h
> > +++ b/lib/netdev-offload-provider.h
> > @@ -102,6 +102,16 @@ struct netdev_flow_api {
> >  int (*meter_set)(ofproto_meter_id meter_id,
> >   struct ofputil_meter_config *config);
> >  
> > +/* Offloads or modifies the offloaded meter on the netdev with the 
> > given
> > + * 'meter_id' and the configuration in 'config'. On failure, a non-zero
> > + * error code is returned.
> > + *
> > + * The meter id specified through 'config->meter_id' is converted as an
> > + * internal meter id. */
> > +int (*dpdk_meter_set)(struct netdev *,
> > +  ofproto_meter_id meter_id,
> > +  struct ofputil_meter_config *);
> 
> Hi.
> 
> This looks strange.  We do already have the meter API here.
> There is no need to create a separate set of APIs for each
> offload provider.  This breaks the abstraction.
> 
> If there is something missing in existing meter_* callbacks,
> modify them, so they can be suitable for both implementations.
> If you need to iterate over all the ports, we have a special
> netdev_ports_traverse() function that can be used from the
> netdev-offload-dpdk.

Thanks, we are working on revising this accordingly.

> Also, In my reply for v1 I mentioned that this patch set
> should be sent for dpdk-latest branch, not master.  i.e.,
> it should have 'dpdk-latest' in the subject prefix.

Sorry about that. This series is based on dpdk-latest.
But I omitted dpdk-latest from the subject prefix.

> CC: Ian.
> 
> dpdk-latest branch may need a rebase though.  Ian, may I ask
> you to rebase it on the latest master?  Or I can do that
> myself, if necessary.
> 
> Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


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

2023-03-30 Thread Naveen Yerramneni
Add OpenFlow extn to set conntrack entries limit per zone.
This extn will be used in future to set the zone level limit for
drop zones used by OVN.

Signed-off-by: Naveen Yerramneni 
Reviewed-by: Simon Horman 
---
Notes:
  v1 -> v2
  - Fix memory leak and added logs
  v2 -> v3
  - Addressed nits
  v3 -> v4
  - Updated change description

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

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

Re: [ovs-dev] [PATCH v4] db-ctl-base: Partially revert b8bf410a5

2023-03-30 Thread Dumitru Ceara
On 3/29/23 22:26, dalva...@redhat.com wrote:
> From: Daniel Alvarez Sanchez 
> 
> The commit b8bf410a5 [0] broke the `ovs-vsctl add` command
> which now overwrites the value if it existed already.
> 
> This patch reverts the code around the `cmd_add` function
> to restore the previous behavior. It also adds testing coverage
> for this functionality.
> 
> [0] 
> https://github.com/openvswitch/ovs/commit/b8bf410a5c94173da02279b369d75875c4035959

Hi Daniel,

Thanks for the fix!  Sorry for missing this at review last time.

> 
> Fixes: b8bf410a5 ("db-ctl-base: Use partial map/set updates for last add/set 
> commands")

The "Fixes" tag can be corrected by Ilya at apply time, I guess.

> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2182767
> Signed-off-by: Daniel Alvarez Sanchez 

Looks good to me, thanks!

Acked-by: Dumitru Ceara 

Regards,
Dumitru

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


[ovs-dev] [PATCH ovn] controller: Clear tunnels from old integration bridge

2023-03-30 Thread Ales Musil
After integration bridge change the tunnels would
stay on the old bridge preventing new tunnels creation
and disrupting traffic. Detect the bridge change
and clear the tunnels from the old integration bridge.

Reported-at: https://bugzilla.redhat.com/2173635
Signed-off-by: Ales Musil 
---
 controller/encaps.c | 36 +++-
 controller/encaps.h |  4 +-
 controller/ovn-controller.c | 26 +++-
 tests/ovn.at| 83 +
 4 files changed, 145 insertions(+), 4 deletions(-)

diff --git a/controller/encaps.c b/controller/encaps.c
index 2662eaf98..c66743d3b 100644
--- a/controller/encaps.c
+++ b/controller/encaps.c
@@ -386,6 +386,20 @@ chassis_tzones_overlap(const struct sset *transport_zones,
 return false;
 }
 
+static void
+clear_old_tunnels(const struct ovsrec_bridge *old_br_int)
+{
+for (size_t i = 0; i < old_br_int->n_ports; i++) {
+const struct ovsrec_port *port = old_br_int->ports[i];
+const char *id = smap_get(>external_ids, "ovn-chassis-id");
+if (id) {
+VLOG_DBG("Clearing old tunnel port \"%s\" (%s) from bridge "
+ "\"%s\".", port->name, id, old_br_int->name);
+ovsrec_bridge_update_ports_delvalue(old_br_int, port);
+}
+}
+}
+
 void
 encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
const struct ovsrec_bridge *br_int,
@@ -393,12 +407,32 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
const struct sbrec_chassis *this_chassis,
const struct sbrec_sb_global *sbg,
const struct ovsrec_open_vswitch_table *ovs_table,
-   const struct sset *transport_zones)
+   const struct sset *transport_zones,
+   const struct ovsrec_bridge_table *bridge_table,
+   const char *br_int_name)
 {
 if (!ovs_idl_txn || !br_int) {
 return;
 }
 
+if (!br_int_name) {
+/* The controller has just started, we need to look through all
+ * bridges for old tunnel ports. */
+const struct ovsrec_bridge *br;
+OVSREC_BRIDGE_TABLE_FOR_EACH (br, bridge_table) {
+if (!strcmp(br->name, br_int->name)) {
+continue;
+}
+clear_old_tunnels(br);
+}
+} else if (strcmp(br_int_name, br_int->name)) {
+/* The integration bridge was changed, clear tunnel ports from
+ * the old one. */
+const struct ovsrec_bridge *old_br_int =
+get_bridge(bridge_table, br_int_name);
+clear_old_tunnels(old_br_int);
+}
+
 const struct sbrec_chassis *chassis_rec;
 
 struct tunnel_ctx tc = {
diff --git a/controller/encaps.h b/controller/encaps.h
index 867c6f28c..cf38dac1a 100644
--- a/controller/encaps.h
+++ b/controller/encaps.h
@@ -35,7 +35,9 @@ void encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
 const struct sbrec_chassis *,
 const struct sbrec_sb_global *,
 const struct ovsrec_open_vswitch_table *,
-const struct sset *transport_zones);
+const struct sset *transport_zones,
+const struct ovsrec_bridge_table *bridge_table,
+const char *br_int_name);
 
 bool encaps_cleanup(struct ovsdb_idl_txn *ovs_idl_txn,
 const struct ovsrec_bridge *br_int);
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 76be2426e..242d93823 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -536,6 +536,21 @@ process_br_int(struct ovsdb_idl_txn *ovs_idl_txn,
 *br_int_ = br_int;
 }
 
+static void
+consider_br_int_change(const struct ovsrec_bridge *br_int, char **current_name)
+{
+ovs_assert(current_name);
+
+if (!*current_name) {
+*current_name = xstrdup(br_int->name);
+}
+
+if (strcmp(*current_name, br_int->name)) {
+free(*current_name);
+*current_name = xstrdup(br_int->name);
+}
+}
+
 static void
 update_ssl_config(const struct ovsrec_ssl_table *ssl_table)
 {
@@ -4918,6 +4933,8 @@ main(int argc, char *argv[])
 char *ovn_version = ovn_get_internal_version();
 VLOG_INFO("OVN internal version is : [%s]", ovn_version);
 
+char *current_br_int_name = NULL;
+
 /* Main loop. */
 exiting = false;
 restart = false;
@@ -5070,7 +5087,9 @@ main(int argc, char *argv[])
chassis,
sbrec_sb_global_first(ovnsb_idl_loop.idl),
ovs_table,
-   _zones);
+   _zones,
+   bridge_table,
+   current_br_int_name);
 
 stopwatch_start(CONTROLLER_LOOP_STOPWATCH_NAME,
 time_msec());
@@ -5257,7 +5276,10 @@ main(int argc, char *argv[])
 stopwatch_stop(IF_STATUS_MGR_RUN_STOPWATCH_NAME,
  

[ovs-dev] [PATCH 1/1] vswitch.xml: Add description of SRv6 tunnel and related options.

2023-03-30 Thread Nobuhiro MIKI
The description of SRv6 was missing in vswitch.xml, which is
used to generate the man page, so this patch adds it.

Fixes: 03fc1ad78521 ("userspace: Add SRv6 tunnel support.")
Signed-off-by: Nobuhiro MIKI 
---
 vswitchd/vswitch.xml | 39 ---
 1 file changed, 32 insertions(+), 7 deletions(-)

diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 88e2c94e2f0e..19b79ddb5d48 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -2845,6 +2845,16 @@
 
   
 
+  SRv6
+  
+
+Segment Routing IPv6 (SRv6) tunnel encapsulates L3 traffic as
+"IPv6 in IPv6" or "IPv4 in IPv6" with Segment Routing Header (SRH)
+defined in RFC 8754. The segment list in SRH can be set using a
+SRv6 specific option.
+
+  
+
 
   
 
@@ -2853,8 +2863,8 @@
   
 These options apply to interfaces with  of
 geneve, bareudp, gre,
-ip6gre, vxlan, lisp and
-stt.
+ip6gre, vxlan, lisp,
+stt and srv6.
   
 
   
@@ -2867,7 +2877,8 @@
 considered more specific than  if
 a port defines one and another port defines the other.
  is not applicable for bareudp
-tunnels. Hence it is not considered while identifying a bareudp tunnel.
+and srv6 tunnels. Hence it is not considered while identifying
+bareudp or srv6 tunnels.
   
 
   
@@ -2935,8 +2946,9 @@
 
   
 
-  Optional, not applicable for bareudp. The key that
-  received packets must contain, one of:
+  Optional, not applicable for bareudp and
+  srv6. The key that received packets must contain,
+  one of:
 
 
 
@@ -2965,8 +2977,9 @@
 
   
   
-Optional, not applicable for bareudp. The key to be set
-on outgoing packets, one of:
+Optional, not applicable for bareudp and
+srv6. The key to be set on outgoing packets,
+one of:
   
 
 
@@ -3264,6 +3277,18 @@
   
 
 
+
+  
+
+   Specifies the segment list in Segment Routing Header (SRH).
+   It consists of a comma-separated list of segments represented
+   in IPv6 format, e.g. "fc00:100::1,fc00:200::1,fc00:300::1".
+   Note that the first segment must be the same as
+   .
+
+  
+
+
 
   
 These options apply only to patch ports, that is, interfaces
-- 
2.31.1

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