[ovs-dev] [PATCH v3 ovn] controller: add datapath meter capability check

[Lorenzo Bianconi]

Dump datapath meter capabilities before configuring meters in
ovn-controller

Signed-off-by: Lorenzo Bianconi 
---
Changes since v2:
- move meter capability logic in lib/features.c

Changes since v1:
- move rconn in ovn-controller to avoid concurrency issues
---
 controller/ovn-controller.c |  4 ++
 include/ovn/features.h  |  7 +++
 lib/actions.c   |  3 ++
 lib/automake.mk |  1 +
 lib/features.c  | 89 +
 tests/ovn.at|  4 +-
 6 files changed, 106 insertions(+), 2 deletions(-)

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 0031a1035..8c6d4393b 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -3523,6 +3523,9 @@ main(int argc, char *argv[])
ovsrec_server_has_datapath_table(ovs_idl_loop.idl)
? _int_dp
: NULL);
+if (br_int) {
+ovs_feature_support_init(br_int);
+}
 
 /* Enable ACL matching for double tagged traffic. */
 if (ovs_idl_txn) {
@@ -3898,6 +3901,7 @@ loop_done:
 ovsdb_idl_loop_destroy(_idl_loop);
 ovsdb_idl_loop_destroy(_idl_loop);
 
+ovs_feature_support_deinit();
 free(ovs_remote);
 service_stop();
 
diff --git a/include/ovn/features.h b/include/ovn/features.h
index c35d59b14..4a6d45d88 100644
--- a/include/ovn/features.h
+++ b/include/ovn/features.h
@@ -23,17 +23,24 @@
 /* ovn-controller supported feature names. */
 #define OVN_FEATURE_PORT_UP_NOTIF "port-up-notif"
 
+struct ovsrec_bridge;
+struct rconn;
+
 /* OVS datapath supported features.  Based on availability OVN might generate
  * different types of openflows.
  */
 enum ovs_feature_support_bits {
 OVS_CT_ZERO_SNAT_SUPPORT_BIT,
+OVS_DP_METER_SUPPORT_BIT,
 };
 
 enum ovs_feature_value {
 OVS_CT_ZERO_SNAT_SUPPORT = (1 << OVS_CT_ZERO_SNAT_SUPPORT_BIT),
+OVS_DP_METER_SUPPORT = (1 << OVS_DP_METER_SUPPORT_BIT),
 };
 
+void ovs_feature_support_init(const struct ovsrec_bridge *br_int);
+void ovs_feature_support_deinit(void);
 bool ovs_feature_is_supported(enum ovs_feature_value feature);
 bool ovs_feature_support_update(const struct smap *ovs_capabilities);
 
diff --git a/lib/actions.c b/lib/actions.c
index c572e88ae..7cf6be308 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -87,6 +87,9 @@ encode_start_controller_op(enum action_opcode opcode, bool 
pause,
 oc->max_len = UINT16_MAX;
 oc->reason = OFPR_ACTION;
 oc->pause = pause;
+if (!ovs_feature_is_supported(OVS_DP_METER_SUPPORT)) {
+meter_id = NX_CTLR_NO_METER;
+}
 oc->meter_id = meter_id;
 
 struct action_header ah = { .opcode = htonl(opcode) };
diff --git a/lib/automake.mk b/lib/automake.mk
index ddfe33948..9f9f447d5 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -2,6 +2,7 @@ lib_LTLIBRARIES += lib/libovn.la
 lib_libovn_la_LDFLAGS = \
 $(OVS_LTINFO) \
 -Wl,--version-script=$(top_builddir)/lib/libovn.sym \
+$(OVS_LIBDIR)/libopenvswitch.la \
 $(AM_LDFLAGS)
 lib_libovn_la_SOURCES = \
lib/acl-log.c \
diff --git a/lib/features.c b/lib/features.c
index fddf4c450..bd7ba79ef 100644
--- a/lib/features.c
+++ b/lib/features.c
@@ -18,7 +18,14 @@
 #include 
 
 #include "lib/util.h"
+#include "lib/dirs.h"
+#include "socket-util.h"
+#include "lib/vswitch-idl.h"
 #include "openvswitch/vlog.h"
+#include "openvswitch/ofpbuf.h"
+#include "openvswitch/rconn.h"
+#include "openvswitch/ofp-msgs.h"
+#include "openvswitch/ofp-meter.h"
 #include "ovn/features.h"
 
 VLOG_DEFINE_THIS_MODULE(features);
@@ -40,11 +47,15 @@ static uint32_t supported_ovs_features;
 
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
 
+/* ovs-vswitchd connection. */
+static struct rconn *swconn;
+
 static bool
 ovs_feature_is_valid(enum ovs_feature_value feature)
 {
 switch (feature) {
 case OVS_CT_ZERO_SNAT_SUPPORT:
+case OVS_DP_METER_SUPPORT:
 return true;
 default:
 return false;
@@ -58,6 +69,80 @@ ovs_feature_is_supported(enum ovs_feature_value feature)
 return supported_ovs_features & feature;
 }
 
+static bool
+ovn_controller_get_ofp_capa(void)
+{
+if (!swconn) {
+return false;
+}
+
+rconn_run(swconn);
+if (!rconn_is_connected(swconn)) {
+return false;
+}
+
+bool ret = false;
+/* dump datapath meter capabilities. */
+struct ofpbuf *msg = ofpraw_alloc(OFPRAW_OFPST13_METER_FEATURES_REQUEST,
+  rconn_get_version(swconn), 0);
+rconn_send(swconn, msg, NULL);
+for (int i = 0; i < 10; i++) {
+msg = rconn_recv(swconn);
+if (!msg) {
+break;
+}
+
+const struct ofp_header *oh = msg->data;
+enum ofptype type;
+ofptype_decode(, oh);
+
+if (type == OFPTYPE_METER_FEATURES_STATS_REPLY) {
+struct ofputil_meter_features mf;
+

Re: [ovs-dev] [OVN Patch v6 4/4] northd: Restore parallel build with dp_groups

[Mark Michelson]

Hi Anton,

On a high level, this change results in some parts of the parallelized 
hashmap being unused. Should we keep the hashrow_locks structure and its 
APIs, for instance?


See below for more comments.

On 9/2/21 9:27 AM, anton.iva...@cambridgegreys.com wrote:

From: Anton Ivanov 

Restore parallel build with dp groups using rwlock instead
of per row locking as an underlying mechanism.

This provides improvement ~ 10% end-to-end on ovn-heater
under virutalization despite awakening some qemu gremlin
which makes qemu climb to silly CPU usage. The gain on
bare metal is likely to be higher.

Signed-off-by: Anton Ivanov 
---
  northd/ovn-northd.c | 215 
  1 file changed, 159 insertions(+), 56 deletions(-)

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 1f903882b..4537c55dd 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -59,6 +59,7 @@
  #include "unixctl.h"
  #include "util.h"
  #include "uuid.h"
+#include "ovs-thread.h"
  #include "openvswitch/vlog.h"
  
  VLOG_DEFINE_THIS_MODULE(ovn_northd);

@@ -4372,7 +4373,26 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct 
ovn_datapath *od,
  static bool use_logical_dp_groups = false;
  static bool use_parallel_build = true;
  
-static struct hashrow_locks lflow_locks;

+static struct ovs_rwlock flowtable_lock;


With the change from the custom hashrow_locks to using an ovs_rwlock, I 
think it's important to document what data this lock is intending to 
protect.


From what I can tell, this lock specifically is intended to protect 
access to the hmaps in the global lflow_state structure, and it's also 
intended to protect all ovn_datapaths' od_group hmaps. This is not 
something that is immediately obvious just from a global rwlock declaration.



+
+static void ovn_make_multi_lflow(struct ovn_lflow *old_lflow,
+  struct ovn_datapath *od,
+  struct lflow_state *lflow_map,
+  uint32_t hash)
+{
+hmapx_add(_lflow->od_group, od);
+hmap_remove(_map->single_od, _lflow->hmap_node);
+if (use_parallel_build) {
+hmap_insert_fast(_map->multiple_od, _lflow->hmap_node, hash);
+} else {
+hmap_insert(_map->multiple_od, _lflow->hmap_node, hash);
+}
+}
+
+#ifdef __clang__
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wthread-safety"
+#endif


The section above needs a comment to explain why -Wthread-safety is ignored.

  
  /* Adds a row with the specified contents to the Logical_Flow table.

   * Version to use when locking is required.
@@ -4388,55 +4408,127 @@ do_ovn_lflow_add(struct lflow_state *lflow_map, struct 
ovn_datapath *od,
  struct ovn_lflow *old_lflow;
  struct ovn_lflow *lflow;
  
+/* Fast Path.

+ * See if we can get away without writing - grab a rdlock and check
+ * if we can get away with as little work as possible.
+ */
+
  if (use_logical_dp_groups) {
-old_lflow = do_ovn_lflow_find(_map->single_od, NULL, stage,
-  priority, match,
+if (use_parallel_build) {
+ovs_rwlock_rdlock(_lock);
+}
+old_lflow = do_ovn_lflow_find(_map->single_od,
+  NULL, stage, priority, match,
actions, ctrl_meter, hash);
  if (old_lflow) {
-hmapx_add(_lflow->od_group, od);
-/* Found, different, od count went up. Move to multiple od. */
-if (hmapx_count(_lflow->od_group) > 1) {
-hmap_remove(_map->single_od, _lflow->hmap_node);
+if (!hmapx_contains(_lflow->od_group, od)) {
+/* od not in od_group, we need to add it and move to
+ * multiple. */
  if (use_parallel_build) {
-hmap_insert_fast(_map->multiple_od,
- _lflow->hmap_node, hash);
-} else {
-hmap_insert(_map->multiple_od,
-_lflow->hmap_node, hash);
+/* Upgrade the lock to write, we are likely to
+ * modify data. */
+ovs_rwlock_unlock(_lock);
+ovs_rwlock_wrlock(_lock);
+
+/* Check if someone got ahead of us and the flow is already
+ * in multiple. */
+if (!hmap_contains(_map->single_od,
+   _lflow->hmap_node)) {


The logic here is fine, but that comment paired with that if statement 
is strange. Either


a) Change the comment to say "Check if someone got ahead of us and the 
flow has been removed from single."


or

b) Change the if statement to "if 
(hmap_contains(_map->multiple_od, _lflow->hmap_node))"



+/* Someone did get ahead of us, add the od to the
+ * group. */
+ 

Re: [ovs-dev] [PATCH v2 2/2] stream-ssl: Update default protocols, enable TLSv1.3

[Flavio Leitner]

Hi Frode,

On Wed, Aug 25, 2021 at 01:05:14PM +0200, Frode Nordahl wrote:
> RFC 8996 [0] deprecates the use of TLSv1 and TLSv1.1 for security
> reasons.  Update our default list of protcols to be in compliance.
> 
> Also add TLSv1.3 to the default list of supported protocols.
> 
> 0: https://datatracker.ietf.org/doc/html/rfc8996
> Signed-off-by: Frode Nordahl 

This patch does two things:
  Deprecate TLSv1 and TLSv1.2
  Add support for TLSv1.3

Can we split them into separate logical patches?

Also, shouldn't we first warn the users about deprecating
TLSv1 and TLSv1.1 for a release period, and then remove from
the default list in the next release?  I mean, this is an user
visible change, so users might need some time to adapt.

What do you think?

fbl

> ---
>  NEWS  |  7 +++
>  lib/ssl-connect.man   |  6 --
>  lib/stream-ssl.c  | 35 +--
>  m4/openvswitch.m4 | 16 
>  tests/ovsdb-server.at |  6 ++
>  5 files changed, 58 insertions(+), 12 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index 1f2adf718..502e6693a 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -8,6 +8,13 @@ Post-v2.16.0
> by default.  'other_config:dpdk-socket-limit' can be set equal to
> the 'other_config:dpdk-socket-mem' to preserve the legacy memory
> limiting behavior.
> +   - Update default configuration for enabled TLS protocols:
> + * RFC 8996 deprecates the use of TLSv1 and TLSv1.1 for security reasons.
> + * Add TLSv1.3 to the default list of enabled protocols when the built 
> with
> +   OpenSSL v1.1.1 and newer.
> + * The new default is as such: TLSv1.2,TLSv1.3
> + * As a consequence we no longer support building Open vSwitch with 
> OpenSSL
> +   versions without TLSv1.2 support.
>  
>  
>  v2.16.0 - 16 Aug 2021
> diff --git a/lib/ssl-connect.man b/lib/ssl-connect.man
> index 6e54f77ef..0dd5a29be 100644
> --- a/lib/ssl-connect.man
> +++ b/lib/ssl-connect.man
> @@ -1,10 +1,12 @@
>  .IP "\fB\-\-ssl\-protocols=\fIprotocols\fR"
>  Specifies, in a comma- or space-delimited list, the SSL protocols
>  \fB\*(PN\fR will enable for SSL connections.  Supported
> -\fIprotocols\fR include \fBTLSv1\fR, \fBTLSv1.1\fR, and \fBTLSv1.2\fR.
> +\fIprotocols\fR include \fBTLSv1.2\fR and \fBTLSv1.3\fR depending on
> +which version of OpenSSL Open vSwitch is built with.
>  Regardless of order, the highest protocol supported by both sides will
>  be chosen when making the connection.  The default when this option is
> -omitted is \fBTLSv1,TLSv1.1,TLSv1.2\fR.
> +omitted is \fBTLSv1.2,TLSv1.3\fR if built with a version of OpenSSL that
> +supports \fBTLSv1.3\fR.
>  .
>  .IP "\fB\-\-ssl\-ciphers=\fIciphers\fR"
>  Specifies, in OpenSSL cipher string format, the ciphers \fB\*(PN\fR will 
> diff --git a/lib/stream-ssl.c b/lib/stream-ssl.c
> index 6b4cf6970..954067787 100644
> --- a/lib/stream-ssl.c
> +++ b/lib/stream-ssl.c
> @@ -162,7 +162,13 @@ struct ssl_config_file {
>  static struct ssl_config_file private_key;
>  static struct ssl_config_file certificate;
>  static struct ssl_config_file ca_cert;
> -static char *default_ssl_protocols = "TLSv1,TLSv1.1,TLSv1.2";
> +/* RFC 8996 deprecates the use of TLSv1 and TLSv1.1, users may still specify
> + * them in their configuration, but our defaults are in compliance. */
> +#if OPENSSL_VERSION_NUMBER < 0x10101000L
> +static char *default_ssl_protocols = "TLSv1.2";
> +#else
> +static char *default_ssl_protocols = "TLSv1.2,TLSv1.3";
> +#endif /* OPENSSL_VERSION_NUMBER < 0x10101000L */
>  static char *default_ssl_ciphers = "HIGH:!aNULL:!MD5";
>  static char *ssl_protocols = NULL;
>  static char *ssl_ciphers = NULL;
> @@ -1255,9 +1261,18 @@ stream_ssl_set_protocols(const char *arg)
>  return;
>  }
>  
> +/* TODO: Using SSL_CTX_set_options to enable individual protocol versions
> + * is deprecated as of OpenSSL v1.1.0.  Once we can drop support for 
> builds
> + * with OpenSSL pre v1.1.0 we should use SSL_CTX_set_min_proto_version 
> and
> + * SSL_CTX_set_max_proto_version instead. */
> +
>  /* Start with all the flags off and turn them on as requested. */
>  #ifndef SSL_OP_NO_SSL_MASK
> -/* For old OpenSSL without this macro, this is the correct value.  */
> +/* For old OpenSSL without this macro, this is the correct value.
> + *
> + * NOTE: We deliberately did not extend this compatibility macro to
> + * include SSL_OP_NO_TLSv1_3 because if you do have a version of OpenSSL
> + * with TLSv1.3 support this macro would be defined by OpenSSL. */
>  #define SSL_OP_NO_SSL_MASK (SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3 | \
>  SSL_OP_NO_TLSv1 | SSL_OP_NO_TLSv1_1 | \
>  SSL_OP_NO_TLSv1_2)
> @@ -1272,12 +1287,20 @@ stream_ssl_set_protocols(const char *arg)
>  goto exit;
>  }
>  while (word != NULL) {
> -long on_flag;
> -if (!strcasecmp(word, "TLSv1.2")){
> +long 

Re: [ovs-dev] [PATCH] datapath: handle DNAT tuple collision

[Paolo Valerio]

Dumitru Ceara  writes:

> Upstream commit:
> commit 8aa7b526dc0b5dbf40c1b834d76a667ad672a410
> Author: Dumitru Ceara 
> Date:   Wed Oct 7 17:48:03 2020 +0200
>
> openvswitch: handle DNAT tuple collision
>
> With multiple DNAT rules it's possible that after destination
> translation the resulting tuples collide.
>
> For example, two openvswitch flows:
> nw_dst=10.0.0.10,tp_dst=10, 
> actions=ct(commit,table=2,nat(dst=20.0.0.1:20))
> nw_dst=10.0.0.20,tp_dst=10, 
> actions=ct(commit,table=2,nat(dst=20.0.0.1:20))
>
> Assuming two TCP clients initiating the following connections:
> 10.0.0.10:5000->10.0.0.10:10
> 10.0.0.10:5000->10.0.0.20:10
>
> Both tuples would translate to 10.0.0.10:5000->20.0.0.1:20 causing
> nf_conntrack_confirm() to fail because of tuple collision.
>
> Netfilter handles this case by allocating a null binding for SNAT at
> egress by default.  Perform the same operation in openvswitch for DNAT
> if no explicit SNAT is requested by the user and allocate a null binding
> for SNAT for packets in the "original" direction.
>
> Reported-at: https://bugzilla.redhat.com/1877128
> Suggested-by: Florian Westphal 
> Fixes: 05752523e565 ("openvswitch: Interface with NAT.")
> Signed-off-by: Dumitru Ceara 
> Signed-off-by: Jakub Kicinski 
>
> Fixes: f8f97cdce9ad ("datapath: Interface with NAT.")
> Signed-off-by: Dumitru Ceara 
> ---

Acked-by: Paolo Valerio 

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

Re: [ovs-dev] [PATCH v2 1/2] stream-ssl: Fix handling of default ciphers/protocols

[Flavio Leitner]

Hi Frode,

Thanks for your patch.
Please see my comments below.

On Wed, Aug 25, 2021 at 01:05:13PM +0200, Frode Nordahl wrote:
> Contrary to what is stated in the documentation, when SSL
> ciphers or protocols options are omitted the default values
> will not be set.  The SSL library default settings will be used
> instead.
> 
> Fix handling of default ciphers and protocols so that we actually
> enforce what is listed as defaults.
> 
> Fixes: e18a1d086133 ("Add support for specifying SSL connection parameters to 
> ovsdb")
> Signed-off-by: Frode Nordahl 
> ---
>  lib/stream-ssl.c | 30 ++
>  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/stream-ssl.c b/lib/stream-ssl.c
> index 0ea3f2c08..6b4cf6970 100644
> --- a/lib/stream-ssl.c
> +++ b/lib/stream-ssl.c
> @@ -162,8 +162,10 @@ struct ssl_config_file {
>  static struct ssl_config_file private_key;
>  static struct ssl_config_file certificate;
>  static struct ssl_config_file ca_cert;
> -static char *ssl_protocols = "TLSv1,TLSv1.1,TLSv1.2";
> -static char *ssl_ciphers = "HIGH:!aNULL:!MD5";
> +static char *default_ssl_protocols = "TLSv1,TLSv1.1,TLSv1.2";
> +static char *default_ssl_ciphers = "HIGH:!aNULL:!MD5";
> +static char *ssl_protocols = NULL;
> +static char *ssl_ciphers = NULL;
>  
>  /* Ordinarily, the SSL client and server verify each other's certificates 
> using
>   * a CA certificate.  Setting this to false disables this behavior.  (This 
> is a
> @@ -1225,14 +1227,19 @@ stream_ssl_set_key_and_cert(const char 
> *private_key_file,
>  void
>  stream_ssl_set_ciphers(const char *arg)
>  {
> -if (ssl_init() || !arg || !strcmp(ssl_ciphers, arg)) {

The ssl_init() calls at least one time do_ssl_init() which then
calls SSL_CTX_set_cipher_list(ctx, "HIGH:!aNULL:!MD5").
Those are the defaults in the man-page and not from the library.

The do_ssl_init() also does:
   method = CONST_CAST(SSL_METHOD *, SSLv23_method());  

That should return SSLv3, TLSv1, TLSv1.1 and TLS1.2.

   ctx = SSL_CTX_new(method); 
   SSL_CTX_set_options(ctx, SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3);

And there it excludes those SSL v2 and v3.

Therefore, the default would be "TLSv1,TLSv1.1,TLSv1.2" which is
the same in the man-page.

Did I miss something?

Thanks
fbl



> +const char *input = arg ? arg : default_ssl_ciphers;
> +
> +if (ssl_init() || !input || (ssl_ciphers && !strcmp(ssl_ciphers, 
> input))) {
>  return;
>  }
> -if (SSL_CTX_set_cipher_list(ctx,arg) == 0) {
> +if (SSL_CTX_set_cipher_list(ctx, input) == 0) {
>  VLOG_ERR("SSL_CTX_set_cipher_list: %s",
>   ERR_error_string(ERR_get_error(), NULL));
>  }
> -ssl_ciphers = xstrdup(arg);
> +if (ssl_ciphers) {
> +free(ssl_ciphers);
> +}
> +ssl_ciphers = xstrdup(input);
>  }
>  
>  /* Set SSL protocols based on the string input. Aborts with an error message
> @@ -1240,7 +1247,11 @@ stream_ssl_set_ciphers(const char *arg)
>  void
>  stream_ssl_set_protocols(const char *arg)
>  {
> -if (ssl_init() || !arg || !strcmp(arg, ssl_protocols)){
> +const char *input = arg ? arg : default_ssl_protocols;
> +
> +if (ssl_init() || !input
> +|| (ssl_protocols && !strcmp(input, ssl_protocols)))
> +{
>  return;
>  }
>  
> @@ -1253,7 +1264,7 @@ stream_ssl_set_protocols(const char *arg)
>  #endif
>  long protocol_flags = SSL_OP_NO_SSL_MASK;
>  
> -char *s = xstrdup(arg);
> +char *s = xstrdup(input);
>  char *save_ptr = NULL;
>  char *word = strtok_r(s, " ,\t", _ptr);
>  if (word == NULL) {
> @@ -1281,7 +1292,10 @@ stream_ssl_set_protocols(const char *arg)
>  /* Set the actual options. */
>  SSL_CTX_set_options(ctx, protocol_flags);
>  
> -ssl_protocols = xstrdup(arg);
> +if (ssl_protocols) {
> +  free(ssl_protocols);
> +}
> +ssl_protocols = xstrdup(input);
>  
>  exit:
>  free(s);
> -- 
> 2.32.0
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

Re: [ovs-dev] [ovn] system@ovs-system: failed to query port patch-outside-localnet-to-br-int: Invalid argument

[Vladislav Odintsov]

Hi Numan,


Regards,
Vladislav Odintsov

> On 9 Sep 2021, at 20:36, Numan Siddique  wrote:
> 
> On Thu, Sep 9, 2021 at 10:43 AM Vladislav Odintsov  > wrote:
>> 
>> I’ve forgot to add:
>> there warnings appear while starting ovn-controller service.
>> 
>> Regards,
>> Vladislav Odintsov
>> 
>>> On 9 Sep 2021, at 17:41, Vladislav Odintsov  wrote:
>>> 
>>> Hi,
>>> 
>>> with ovn master code and OVS 2.16.0 with OOT kernel module I see error in 
>>> ovs-vswitchd:
>>> 
>>> 2021-09-09T14:39:33.276Z|96912|bridge|INFO|bridge internet: deleted 
>>> interface patch-outside-localnet-to-br-int on port 94
>>> 2021-09-09T14:39:33.276Z|96913|bridge|INFO|bridge br-int: deleted interface 
>>> patch-br-int-to-outside-localnet on port 152
>>> 2021-09-09T14:39:33.278Z|96914|dpif|WARN|system@ovs-system: failed to query 
>>> port patch-outside-localnet-to-br-int: Invalid argument
>>> 2021-09-09T14:39:33.278Z|96915|dpif|WARN|system@ovs-system: failed to query 
>>> port patch-br-int-to-outside-localnet: Invalid argument
>>> 2021-09-09T14:39:33.350Z|96918|bridge|INFO|bridge internet: added interface 
>>> patch-outside-localnet-to-br-int on port 95
>>> 2021-09-09T14:39:33.350Z|96919|bridge|INFO|bridge br-int: added interface 
>>> patch-br-int-to-outside-localnet on port 157
>>> 
>>> Though, I’m not seeing any negative impact, maybe somebody of developers 
>>> know the reason and the possible impact?
> 
> These patch ports are created and deleted by OVN.  Do you see it all
> the time you start/restart ovn-controller ?

Yes. Each time then I issue restart.
Dont you know which query failed and argument could be invalid…?

> 
> Perhaps ovn-controller is deleting these patch ports during start up
> (since it has not seen the localnet port binding yet )
> thinking these patch ports are not required  and it is recreating the
> patch ports once it sees the localnet port binding.
> 

Nope, this chassis has port-bindings:

ovn-sbctl find port-binding chassis=4c33e205-1745-4ee3-9efd-f778afd5a4d2 | grep 
_uuid | wc -l
5

> Thanks
> Numan
> 
> 
> 
>>> 
>>> Regards,
>>> Vladislav Odintsov
>>> 
>>> ___
>>> dev mailing list
>>> d...@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> 
>> ___
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> ___
> dev mailing list
> d...@openvswitch.org 
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev 
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ovn] system@ovs-system: failed to query port patch-outside-localnet-to-br-int: Invalid argument

[Numan Siddique]

On Thu, Sep 9, 2021 at 10:43 AM Vladislav Odintsov  wrote:
>
> I’ve forgot to add:
> there warnings appear while starting ovn-controller service.
>
> Regards,
> Vladislav Odintsov
>
> > On 9 Sep 2021, at 17:41, Vladislav Odintsov  wrote:
> >
> > Hi,
> >
> > with ovn master code and OVS 2.16.0 with OOT kernel module I see error in 
> > ovs-vswitchd:
> >
> > 2021-09-09T14:39:33.276Z|96912|bridge|INFO|bridge internet: deleted 
> > interface patch-outside-localnet-to-br-int on port 94
> > 2021-09-09T14:39:33.276Z|96913|bridge|INFO|bridge br-int: deleted interface 
> > patch-br-int-to-outside-localnet on port 152
> > 2021-09-09T14:39:33.278Z|96914|dpif|WARN|system@ovs-system: failed to query 
> > port patch-outside-localnet-to-br-int: Invalid argument
> > 2021-09-09T14:39:33.278Z|96915|dpif|WARN|system@ovs-system: failed to query 
> > port patch-br-int-to-outside-localnet: Invalid argument
> > 2021-09-09T14:39:33.350Z|96918|bridge|INFO|bridge internet: added interface 
> > patch-outside-localnet-to-br-int on port 95
> > 2021-09-09T14:39:33.350Z|96919|bridge|INFO|bridge br-int: added interface 
> > patch-br-int-to-outside-localnet on port 157
> >
> > Though, I’m not seeing any negative impact, maybe somebody of developers 
> > know the reason and the possible impact?

These patch ports are created and deleted by OVN.  Do you see it all
the time you start/restart ovn-controller ?

Perhaps ovn-controller is deleting these patch ports during start up
(since it has not seen the localnet port binding yet )
thinking these patch ports are not required  and it is recreating the
patch ports once it sees the localnet port binding.

Thanks
Numan



> >
> > Regards,
> > Vladislav Odintsov
> >
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn] ci: ovn-kubernetes: Log ovn-kubernetes commit SHA.

[Dumitru Ceara]

This makes it easier to debug test failures.

Signed-off-by: Dumitru Ceara 
---
 .ci/ovn-kubernetes/Dockerfile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.ci/ovn-kubernetes/Dockerfile b/.ci/ovn-kubernetes/Dockerfile
index 9dde47259..9cfc32f62 100644
--- a/.ci/ovn-kubernetes/Dockerfile
+++ b/.ci/ovn-kubernetes/Dockerfile
@@ -42,7 +42,7 @@ ARG OVNKUBE_COMMIT
 WORKDIR /root
 RUN git clone https://github.com/ovn-org/ovn-kubernetes.git
 WORKDIR /root/ovn-kubernetes/go-controller
-RUN git checkout ${OVNKUBE_COMMIT} && make
+RUN git checkout ${OVNKUBE_COMMIT} && git log -n 1 && make
 
 # Build the final image
 FROM fedora:33
-- 
2.27.0

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

[ovs-dev] [PATCH v2] dpif-netdev: Fix pmd thread comments to include SMC.

[Cian Ferriter]

These comments are relevant to SMC too.

Fixes: 60d8ccae135f ("dpif-netdev: Add SMC cache after EMC cache")
Signed-off-by: Cian Ferriter 
Acked-by: Kevin Traynor 
---
 lib/dpif-netdev-private-dfc.h| 3 ++-
 lib/dpif-netdev-private-thread.h | 8 
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/lib/dpif-netdev-private-dfc.h b/lib/dpif-netdev-private-dfc.h
index 92092ebec..3dfc91f0f 100644
--- a/lib/dpif-netdev-private-dfc.h
+++ b/lib/dpif-netdev-private-dfc.h
@@ -59,7 +59,8 @@ extern "C" {
  * Thread-safety
  * =
  *
- * Each pmd_thread has its own private exact match cache.
+ * Each pmd_thread has its own private exact match cache and signature match
+ * cache.
  * If dp_netdev_input is not called from a pmd thread, a mutex is used.
  */
 
diff --git a/lib/dpif-netdev-private-thread.h b/lib/dpif-netdev-private-thread.h
index a782d9678..ac4885538 100644
--- a/lib/dpif-netdev-private-thread.h
+++ b/lib/dpif-netdev-private-thread.h
@@ -78,10 +78,10 @@ struct dp_netdev_pmd_thread {
 struct ovs_refcount ref_cnt;/* Every reference must be refcount'ed. */
 struct cmap_node node;  /* In 'dp->poll_threads'. */
 
-/* Per thread exact-match cache.  Note, the instance for cpu core
- * NON_PMD_CORE_ID can be accessed by multiple threads, and thusly
- * need to be protected by 'non_pmd_mutex'.  Every other instance
- * will only be accessed by its own pmd thread. */
+/* Per thread exact match cache and signature match cache.  Note, the
+ * instance for cpu core NON_PMD_CORE_ID can be accessed by multiple
+ * threads, and thusly need to be protected by 'non_pmd_mutex'.  Every
+ * other instance will only be accessed by its own pmd thread. */
 OVS_ALIGNED_VAR(CACHE_LINE_SIZE) struct dfc_cache flow_cache;
 
 /* Flow-Table and classifiers
-- 
2.32.0

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

Re: [ovs-dev] [PATCH] dpif-netdev: Mention SMC in 2 pmd_thread comments

[Ferriter, Cian]

> -Original Message-
> From: Kevin Traynor 
> Sent: Thursday 9 September 2021 14:39
> To: Ferriter, Cian ; ovs-dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH] dpif-netdev: Mention SMC in 2 pmd_thread 
> comments
> 
> pmd_thread seems like it is referencing a struct so better to just use a
> natural description (also, style is usually to have summary in sentence
> case including ".")
> 
> How about:
> 
> dpif-netdev: Fix pmd thread comments to include SMC.
> 

Sounds better, I'll use this and send a v2.

> On 27/08/2021 16:52, Cian Ferriter wrote:
> > These comments are relevant to SMC too.
> >
> 
> Fixes: 60d8ccae135f ("dpif-netdev: Add SMC cache after EMC cache")
> 

I'll add the fixes tag.

> (though it's not worth backporting they have moved files)
> 
> Very minor comments, so i'll add ack:
> 
> Acked-by: Kevin Traynor 
> 

Thanks for the review Kevin, I'll add your Acked-by tag in the v2.

> > Signed-off-by: Cian Ferriter 
> > ---
> >  lib/dpif-netdev-private-dfc.h| 3 ++-
> >  lib/dpif-netdev-private-thread.h | 8 
> >  2 files changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/dpif-netdev-private-dfc.h b/lib/dpif-netdev-private-dfc.h
> > index 92092ebec..3dfc91f0f 100644
> > --- a/lib/dpif-netdev-private-dfc.h
> > +++ b/lib/dpif-netdev-private-dfc.h
> > @@ -59,7 +59,8 @@ extern "C" {
> >   * Thread-safety
> >   * =
> >   *
> > - * Each pmd_thread has its own private exact match cache.
> > + * Each pmd_thread has its own private exact match cache and signature 
> > match
> > + * cache.
> >   * If dp_netdev_input is not called from a pmd thread, a mutex is used.
> >   */
> >
> > diff --git a/lib/dpif-netdev-private-thread.h 
> > b/lib/dpif-netdev-private-thread.h
> > index a782d9678..ac4885538 100644
> > --- a/lib/dpif-netdev-private-thread.h
> > +++ b/lib/dpif-netdev-private-thread.h
> > @@ -78,10 +78,10 @@ struct dp_netdev_pmd_thread {
> >  struct ovs_refcount ref_cnt;/* Every reference must be 
> > refcount'ed. */
> >  struct cmap_node node;  /* In 'dp->poll_threads'. */
> >
> > -/* Per thread exact-match cache.  Note, the instance for cpu core
> > - * NON_PMD_CORE_ID can be accessed by multiple threads, and thusly
> > - * need to be protected by 'non_pmd_mutex'.  Every other instance
> > - * will only be accessed by its own pmd thread. */
> > +/* Per thread exact match cache and signature match cache.  Note, the
> > + * instance for cpu core NON_PMD_CORE_ID can be accessed by multiple
> > + * threads, and thusly need to be protected by 'non_pmd_mutex'.  Every
> > + * other instance will only be accessed by its own pmd thread. */
> >  OVS_ALIGNED_VAR(CACHE_LINE_SIZE) struct dfc_cache flow_cache;
> >
> >  /* Flow-Table and classifiers
> >

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

Re: [ovs-dev] [PATCH 2/2] docs: Recommend the use of dpdkvhostuserclient ports

[Kevin Traynor]

On 27/08/2021 16:15, Cian Ferriter wrote:
> dpdkvhostuser ports are deprecated, but are still being recommended to
> users through the documentation. Fix this.
> 
> Signed-off-by: Cian Ferriter 
> ---
>  Documentation/howto/dpdk.rst| 30 +++--
>  Documentation/howto/userspace-tunneling.rst |  3 ++-
>  Documentation/intro/install/afxdp.rst   |  5 ++--
>  3 files changed, 21 insertions(+), 17 deletions(-)
> 

Acked-by: Kevin Traynor 

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

Re: [ovs-dev] [PATCH 1/2] docs/install/afxdp: Fix wrapping in QEMU CMDs

[Kevin Traynor]

On 27/08/2021 16:15, Cian Ferriter wrote:
> Directly copying the CMD from either the .rst file or the docs on the
> web caused errors in the QEMU command because of how it was wrapped.
> 
> Signed-off-by: Cian Ferriter 
> 

Acked-by: Kevin Traynor 

In general, it would be nice to replace all the qemu command line
snippets with libvirt xml. As someone once told me, "normal people use
libvirt" :-)

> ---
> 
> The CMD could be wrapped, but without the indentation to look like this:
>   qemu-system-x86_64 -hda ubuntu1810.qcow \
>-m 4096 \
>-cpu host,+x2apic -enable-kvm \
>-chardev socket,id=char1,path=/usr/local/var/run/openvswitch/vhost-user-1 \
>-netdev type=vhost-user,id=mynet1,chardev=char1,vhostforce,queues=4 \
>-device virtio-net-pci,mac=00:00:00:00:00:01,\
> netdev=mynet1,mq=on,vectors=10 \
>-object memory-backend-file,id=mem,size=4096M,\
> mem-path=/dev/hugepages,share=on \
>-numa node,memdev=mem -mem-prealloc -smp 2
> 
> But I went with the below, which has lines that extend past 79 characters.
>   qemu-system-x86_64 -hda ubuntu1810.qcow \
>-m 4096 \
>-cpu host,+x2apic -enable-kvm \
>-chardev socket,id=char1,path=/usr/local/var/run/openvswitch/vhost-user-1 \
>-netdev type=vhost-user,id=mynet1,chardev=char1,vhostforce,queues=4 \
>-device 
> virtio-net-pci,mac=00:00:00:00:00:01,netdev=mynet1,mq=on,vectors=10 \
>-object 
> memory-backend-file,id=mem,size=4096M,mem-path=/dev/hugepages,share=on \
>-numa node,memdev=mem -mem-prealloc -smp 2
> ---
>  Documentation/intro/install/afxdp.rst | 12 
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/intro/install/afxdp.rst 
> b/Documentation/intro/install/afxdp.rst
> index f2643e0d4..47149cc73 100644
> --- a/Documentation/intro/install/afxdp.rst
> +++ b/Documentation/intro/install/afxdp.rst
> @@ -374,11 +374,9 @@ Start a VM with virtio and tap device::
>qemu-system-x86_64 -hda ubuntu1810.qcow \
>  -m 4096 \
>  -cpu host,+x2apic -enable-kvm \
> --device virtio-net-pci,mac=00:02:00:00:00:01,netdev=net0,mq=on,\
> -  vectors=10,mrg_rxbuf=on,rx_queue_size=1024 \
> +-device 
> virtio-net-pci,mac=00:02:00:00:00:01,netdev=net0,mq=on,vectors=10,mrg_rxbuf=on,rx_queue_size=1024
>  \
>  -netdev type=tap,id=net0,vhost=on,queues=8 \
> --object memory-backend-file,id=mem,size=4096M,\
> -  mem-path=/dev/hugepages,share=on \
> +-object 
> memory-backend-file,id=mem,size=4096M,mem-path=/dev/hugepages,share=on \
>  -numa node,memdev=mem -mem-prealloc -smp 2
>  
>  Create OpenFlow rules::
> @@ -415,10 +413,8 @@ Start VM using vhost-user mode::
> -cpu host,+x2apic -enable-kvm \
> -chardev socket,id=char1,path=/usr/local/var/run/openvswitch/vhost-user-1 
> \
> -netdev type=vhost-user,id=mynet1,chardev=char1,vhostforce,queues=4 \
> -   -device virtio-net-pci,mac=00:00:00:00:00:01,\
> -  netdev=mynet1,mq=on,vectors=10 \
> -   -object memory-backend-file,id=mem,size=4096M,\
> -  mem-path=/dev/hugepages,share=on \
> +   -device 
> virtio-net-pci,mac=00:00:00:00:00:01,netdev=mynet1,mq=on,vectors=10 \
> +   -object 
> memory-backend-file,id=mem,size=4096M,mem-path=/dev/hugepages,share=on \
> -numa node,memdev=mem -mem-prealloc -smp 2
>  
>  Setup the OpenFlow ruls::
> 

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

Re: [ovs-dev] [ovn] system@ovs-system: failed to query port patch-outside-localnet-to-br-int: Invalid argument

[Vladislav Odintsov]

I’ve forgot to add:
there warnings appear while starting ovn-controller service.

Regards,
Vladislav Odintsov

> On 9 Sep 2021, at 17:41, Vladislav Odintsov  wrote:
> 
> Hi,
> 
> with ovn master code and OVS 2.16.0 with OOT kernel module I see error in 
> ovs-vswitchd:
> 
> 2021-09-09T14:39:33.276Z|96912|bridge|INFO|bridge internet: deleted interface 
> patch-outside-localnet-to-br-int on port 94
> 2021-09-09T14:39:33.276Z|96913|bridge|INFO|bridge br-int: deleted interface 
> patch-br-int-to-outside-localnet on port 152
> 2021-09-09T14:39:33.278Z|96914|dpif|WARN|system@ovs-system: failed to query 
> port patch-outside-localnet-to-br-int: Invalid argument
> 2021-09-09T14:39:33.278Z|96915|dpif|WARN|system@ovs-system: failed to query 
> port patch-br-int-to-outside-localnet: Invalid argument
> 2021-09-09T14:39:33.350Z|96918|bridge|INFO|bridge internet: added interface 
> patch-outside-localnet-to-br-int on port 95
> 2021-09-09T14:39:33.350Z|96919|bridge|INFO|bridge br-int: added interface 
> patch-br-int-to-outside-localnet on port 157
> 
> Though, I’m not seeing any negative impact, maybe somebody of developers know 
> the reason and the possible impact?
> 
> Regards,
> Vladislav Odintsov
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[ovs-dev] [ovn] system@ovs-system: failed to query port patch-outside-localnet-to-br-int: Invalid argument

[Vladislav Odintsov]

Hi,

with ovn master code and OVS 2.16.0 with OOT kernel module I see error in 
ovs-vswitchd:

2021-09-09T14:39:33.276Z|96912|bridge|INFO|bridge internet: deleted interface 
patch-outside-localnet-to-br-int on port 94
2021-09-09T14:39:33.276Z|96913|bridge|INFO|bridge br-int: deleted interface 
patch-br-int-to-outside-localnet on port 152
2021-09-09T14:39:33.278Z|96914|dpif|WARN|system@ovs-system: failed to query 
port patch-outside-localnet-to-br-int: Invalid argument
2021-09-09T14:39:33.278Z|96915|dpif|WARN|system@ovs-system: failed to query 
port patch-br-int-to-outside-localnet: Invalid argument
2021-09-09T14:39:33.350Z|96918|bridge|INFO|bridge internet: added interface 
patch-outside-localnet-to-br-int on port 95
2021-09-09T14:39:33.350Z|96919|bridge|INFO|bridge br-int: added interface 
patch-br-int-to-outside-localnet on port 157

Though, I’m not seeing any negative impact, maybe somebody of developers know 
the reason and the possible impact?

Regards,
Vladislav Odintsov

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

Re: [ovs-dev] [PATCH v14 7/7] netdev-offload-tc: Add offload support for sFlow

[Eelco Chaudron]

On 15 Jul 2021, at 8:01, Chris Mi wrote:


Create a unique group ID to map the sFlow info when offloading sFlow
action to TC. When showing the offloaded datapath flows, translate the
group ID from TC sample action to sFlow info using the mapping.


See comments below and this completes my review of the v14. See the 
other email on the update of the flows going wrong.


I’ll do a more thorough review on v15 assuming the issue gets fixed, 
especially the sgid allocation/re-use/free.



Signed-off-by: Chris Mi 
Reviewed-by: Eli Britstein 
---
 NEWS|   1 +
 lib/netdev-offload-tc.c | 212 
+---

 lib/tc.c|  61 +++-
 lib/tc.h|  15 ++-
 4 files changed, 272 insertions(+), 17 deletions(-)

diff --git a/NEWS b/NEWS
index 6cdccc715..7c0361e18 100644
--- a/NEWS
+++ b/NEWS
@@ -50,6 +50,7 @@ Post-v2.15.0
- OVS now reports the datapath capability 'ct_zero_snat', which 
reflects

  whether the SNAT with all-zero IP address is supported.
  See ovs-vswitchd.conf.db(5) for details.
+   - Add sFlow offload support for kernel (netlink) datapath.


 v2.15.0 - 15 Feb 2021
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 2f16cf279..71e51394f 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -20,6 +20,7 @@
 #include 

 #include "dpif.h"
+#include "dpif-offload-provider.h"
 #include "hash.h"
 #include "openvswitch/hmap.h"
 #include "openvswitch/match.h"
@@ -1087,6 +1088,19 @@ parse_tc_flower_to_match(struct tc_flower 
*flower,

 action = flower->actions;
 for (i = 0; i < flower->action_count; i++, action++) {
 switch (action->type) {
+case TC_ACT_SAMPLE: {
+const struct sgid_node *node;
+
+node = sgid_find(action->sample.group_id);
+if (!node) {
+VLOG_ERR_RL(_rl, "%s: sgid node is NULL, 
sgid: %d",

+__func__, action->sample.group_id);


This should probably be a warning, as this could happen if the DP has 
not yet been synced? i.e. revalidator being in the process of cleanup 
and someone requesting dp content?



+return ENOENT;
+}
+nl_msg_put(buf, node->sflow.action,
+   node->sflow.action->nla_len);
+}
+break;
 case TC_ACT_VLAN_POP: {
 nl_msg_put_flag(buf, OVS_ACTION_ATTR_POP_VLAN);
 }
@@ -1825,6 +1839,156 @@ parse_match_ct_state_to_flower(struct 
tc_flower *flower, struct match *match)

 }
 }

+static int
+parse_userspace_attributes(const struct nlattr *actions,
+   struct dpif_sflow_attr *sflow_attr)
+{
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
+const struct nlattr *nla;
+unsigned int left;
+
+NL_NESTED_FOR_EACH_UNSAFE (nla, left, actions) {
+if (nl_attr_type(nla) == OVS_USERSPACE_ATTR_USERDATA) {
+struct user_action_cookie *cookie;
+
+cookie = CONST_CAST(struct user_action_cookie *, 
nl_attr_get(nla));

+if (cookie->type == USER_ACTION_COOKIE_SFLOW) {


Here I think the userdata type should be changed to "struct nlattr * " 
just like in the struct dpif_upcall.
This should probably be done in the patch introducing the 
dpif_sflow_attr, so it will also be aligned with the actions member.


+sflow_attr->userdata = CONST_CAST(void *, 
nl_attr_get(nla));

+sflow_attr->userdata_len = nl_attr_get_size(nla);
+return 0;
+}
+}
+}
+
+VLOG_DBG_RL(, "%s: cannot offload userspace action other than 
sFlow",

+__func__);
+return EOPNOTSUPP;
+}
+
+static int
+parse_sample_actions_attribute(const struct nlattr *actions,
+   struct dpif_sflow_attr *sflow_attr)
+{
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
+const struct nlattr *nla;
+unsigned int left;
+int err = EINVAL;
+
+NL_NESTED_FOR_EACH_UNSAFE (nla, left, actions) {
+if (nl_attr_type(nla) == OVS_ACTION_ATTR_USERSPACE) {
+err = parse_userspace_attributes(nla, sflow_attr);
+} else {
+/* We can't offload other nested actions */
+VLOG_DBG_RL(, "%s: can only offload "
+"OVS_ACTION_ATTR_USERSPACE attribute", 
__func__);

+return EINVAL;
+}
+}
+
+if (err) {
+VLOG_ERR_RL(_rl, "%s: no OVS_ACTION_ATTR_USERSPACE 
attribute",

+__func__);
+}
+return err;
+}
+
+static int
+parse_sample_action(struct tc_flower *flower, struct tc_action 
*tc_action,

+const struct nlattr *sample_action,
+const struct flow_tnl *tnl, uint32_t *group_id,
+const ovs_u128 *ufid)
+{
+static struct vlog_rate_limit rl = 

Re: [ovs-dev] [PATCH v14 0/7] Add offload support for sFlow

[Eelco Chaudron]

On 9 Sep 2021, at 4:35, Chris Mi wrote:

> On 9/8/2021 9:57 PM, Eelco Chaudron wrote:
>>
>> On 8 Sep 2021, at 13:52, Chris Mi wrote:
>>
>>> On 9/6/2021 5:47 PM, Eelco Chaudron wrote:
 On 6 Sep 2021, at 11:14, Chris Mi wrote:

> On 9/3/2021 8:54 PM, Eelco Chaudron wrote:
>> On 3 Sep 2021, at 14:02, Eelco Chaudron wrote:
>>
>>   On 15 Jul 2021, at 8:01, Chris Mi wrote:
>>
>>   This patch set adds offload support for sFlow.
>>
>>   Psample is a genetlink channel for packet sampling. TC action
>>   act_sample
>>   uses psample to send sampled packets to userspace.
>>
>>   When offloading sample action to TC, userspace creates a
>>   unique ID to
>>   map sFlow action and tunnel info and passes this ID to kernel
>>   instead
>>   of the sFlow info. psample will send this ID and sampled 
>> packet to
>>   userspace. Using the ID, userspace can recover the sFlow info
>>   and send
>>   sampled packet to the right sFlow monitoring host.
>>
>>   Hi Chris,
>>
>>   One thing missing from this patchset is a test case. I think we
>>   should add it, as I’m going over this manually every patch 
>> iteration.
>>
>>   I would add the following:
>>
>>*
>>
>>   Set the sample rate to 1, send 100 packets and make sure you
>>   receive all of them
>>
>> o Also, verify the output of “ovs-appctl dpctl/dump-flows
>>   system@ovs-system type=tc” is correct, i.e., matches the
>>   kernel output
>>*
>>
>>   Set the sample rate to 10, send 100 packets and make sure you
>>   receive 10.
>>
>> o Also, verify the output of “ovs-appctl dpctl/dump-flows
>>   system@ovs-system type=tc” is correct, i.e., matches the
>>   kernel output
>>
>>   Cheers,
>>
>>   Eelco
>>
>>   PS: I also had a problem where only one packet got sent to the
>>   collector, and then no more packets would arrive. Of course, when
>>   I added some debug code, it never happened, and when removing the
>>   debug code, it also worked fine :( Did you ever experience
>>   something like this? I will play a bit more when reviewing
>>   specific code, maybe it will happen again.
>>
>>
>> One additional thing I’m seeing is the following:
>>
>> $ ovs-appctl dpctl/dump-flows system@ovs-system type=tc
>> recirc_id(0),in_port(3),eth(src=52:54:00:88:51:38,dst=04:f4:bc:28:57:01),eth_type(0x0800),ipv4(frag=no),
>>  packets:7144, bytes:600096, used:7.430s, 
>> actions:sample(sample=10.0%,actions(userspace(pid=3925927229,sFlow(vid=0,pcp=0,output=2147483650),actions))),2
>>
>> Sometimes I get a rather large sFlow(output=) value in the sFlow output. 
>> Converting the above to hex, 0x8002, where I see this in 
>> fix_sflow_action() as being mentioned multiple output ports? This seems 
>> wrong to me as it should be 3 (as it always is in the none hw offload 
>> case). This odd value is also reported to the sFlow samples.
>>
>> Unfortunately, I do not have a reproducer for this.
>>
> Actually, in the very beginning of the development when I have a lot of 
> debug code, I also observed such odd port number sometimes.
> Such rule will disappear soon. So it is hard to test such corner case. 
> And it doesn't affect the functionality.
> So current code didn't deal with such corner case.
 I’m getting this almost every time I restart OVS and initiate a flow 
 (using a simple ping). This does not go away by itself, it stays until the 
 flow times out. So this should be investigated and fixed.

 My test setup is rather simple, just two ports, one is a VM one is a 
 external host, which I ping from the VM. All default config, so using the 
 NORMAL rule. This is my sFlow config:

ovs-vsctl -- --id=@sflow create sflow agent=lo \
 target="127.0.0.1" header=128 \
 sampling=10 polling=4 \
 -- set bridge ovs_pvp_br0 sflow=@sflow

>>> When writing the test, I can reproduce it. Not sure what changed. But it 
>>> happens even without offload. I don't know what need to be fixed.
>>> OVS generates the DP rule and installs it. If offload is enabled, 
>>> sFlow(vid=0,pcp=0,output=2147483650) (actually the whole action) is saved in
>>> dpif_sflow_attr->action when installing the DP rule. When receiving sampled 
>>> packets, tc or driver passes the gid to ovs daemon and
>>> the daemon will find the dpif_sflow_attr->action using the gid. TC didn't 
>>> change it. It is up to OVS sflow engine to process it.
>> Nice! I did not see it in the 

Re: [ovs-dev] [PATCH] netdev-linux: Fix a null pointer dereference in netdev_linux_notify_sock()

[Flavio Leitner]

On Thu, Sep 09, 2021 at 02:08:50PM +0200, David Marchand wrote:
> On Wed, Sep 8, 2021 at 1:53 PM Yunjian Wang  wrote:
> >
> > If nl_sock_join_mcgroup() returns an error, the 'sock' is freed
> > and set to NULL. So we should add NULL check of 'sock' before calling
> > nl_sock_listen_all_nsid().
> >
> > Fixes: cf114a7fce80 ("netlink linux: enable listening to all nsids")
> > Cc: Flavio Leitner 
> > Signed-off-by: Yunjian Wang 
> > ---
> >  lib/netdev-linux.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> > index 60dd13891..7fec5f5a6 100644
> > --- a/lib/netdev-linux.c
> > +++ b/lib/netdev-linux.c
> > @@ -636,7 +636,9 @@ netdev_linux_notify_sock(void)
> >  }
> >  }
> >  }
> > -nl_sock_listen_all_nsid(sock, true);
> > +if (sock) {
> > +nl_sock_listen_all_nsid(sock, true);
> > +}
> >  ovsthread_once_done();
> >  }
> >
> 
> Would it make sense to move this call before the loop on groups?
> Something like:

It does to me. The nl_sock_listen_all_nsid() only sets a flag in
the socket, so it should not matter whether it is done before or
after joining the mcgroups.

fbl


> 
> @@ -627,6 +627,7 @@ netdev_linux_notify_sock(void)
>  if (!error) {
>  size_t i;
> 
> +nl_sock_listen_all_nsid(sock, true);
>  for (i = 0; i < ARRAY_SIZE(mcgroups); i++) {
>  error = nl_sock_join_mcgroup(sock, mcgroups[i]);
>  if (error) {
> @@ -636,7 +637,6 @@ netdev_linux_notify_sock(void)
>  }
>  }
>  }
> -nl_sock_listen_all_nsid(sock, true);
>  ovsthread_once_done();
>  }
> 
> 
> -- 
> David Marchand
> 

-- 
fbl

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

Re: [ovs-dev] [PATCH] dpif-netdev: Mention SMC in 2 pmd_thread comments

[Kevin Traynor]

pmd_thread seems like it is referencing a struct so better to just use a
natural description (also, style is usually to have summary in sentence
case including ".")

How about:

dpif-netdev: Fix pmd thread comments to include SMC.

On 27/08/2021 16:52, Cian Ferriter wrote:
> These comments are relevant to SMC too.
> 

Fixes: 60d8ccae135f ("dpif-netdev: Add SMC cache after EMC cache")

(though it's not worth backporting they have moved files)

Very minor comments, so i'll add ack:

Acked-by: Kevin Traynor 

> Signed-off-by: Cian Ferriter 
> ---
>  lib/dpif-netdev-private-dfc.h| 3 ++-
>  lib/dpif-netdev-private-thread.h | 8 
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/dpif-netdev-private-dfc.h b/lib/dpif-netdev-private-dfc.h
> index 92092ebec..3dfc91f0f 100644
> --- a/lib/dpif-netdev-private-dfc.h
> +++ b/lib/dpif-netdev-private-dfc.h
> @@ -59,7 +59,8 @@ extern "C" {
>   * Thread-safety
>   * =
>   *
> - * Each pmd_thread has its own private exact match cache.
> + * Each pmd_thread has its own private exact match cache and signature match
> + * cache.
>   * If dp_netdev_input is not called from a pmd thread, a mutex is used.
>   */
>  
> diff --git a/lib/dpif-netdev-private-thread.h 
> b/lib/dpif-netdev-private-thread.h
> index a782d9678..ac4885538 100644
> --- a/lib/dpif-netdev-private-thread.h
> +++ b/lib/dpif-netdev-private-thread.h
> @@ -78,10 +78,10 @@ struct dp_netdev_pmd_thread {
>  struct ovs_refcount ref_cnt;/* Every reference must be refcount'ed. 
> */
>  struct cmap_node node;  /* In 'dp->poll_threads'. */
>  
> -/* Per thread exact-match cache.  Note, the instance for cpu core
> - * NON_PMD_CORE_ID can be accessed by multiple threads, and thusly
> - * need to be protected by 'non_pmd_mutex'.  Every other instance
> - * will only be accessed by its own pmd thread. */
> +/* Per thread exact match cache and signature match cache.  Note, the
> + * instance for cpu core NON_PMD_CORE_ID can be accessed by multiple
> + * threads, and thusly need to be protected by 'non_pmd_mutex'.  Every
> + * other instance will only be accessed by its own pmd thread. */
>  OVS_ALIGNED_VAR(CACHE_LINE_SIZE) struct dfc_cache flow_cache;
>  
>  /* Flow-Table and classifiers
> 

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

[ovs-dev] [PATCH v3 ovn] northd: reduce number of nd_na lb logical flows

[Lorenzo Bianconi]

As it has been already done for IPv4, collapse IPv6 Neighbour
Advertisment flows for load balancer using address list and
reduce the number of logical flows from N*M to N where N is
the number of logical router port and M is the number of
Virtual IPs.

https://bugzilla.redhat.com/show_bug.cgi?id=1970258
Signed-off-by: Lorenzo Bianconi 
---
Changes since v2:
- remove open code and run build_lrouter_nd_flow() instead

Changes since v1:
- rebase on top of ovn master
- add DDlog support
---
 northd/ovn-northd.c  | 25 ++---
 northd/ovn_northd.dl | 66 
 tests/ovn-northd.at  | 36 
 3 files changed, 75 insertions(+), 52 deletions(-)

diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index ee761cef0..fc623bcbe 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -9851,8 +9851,7 @@ build_lrouter_nd_flow(struct ovn_datapath *od, struct 
ovn_port *op,
 ds_put_format(,
   "%s { "
 "eth.src = %s; "
-"ip6.src = %s; "
-"nd.target = %s; "
+"ip6.src = nd.target; "
 "nd.tll = %s; "
 "outport = inport; "
 "flags.loopback = 1; "
@@ -9860,8 +9859,6 @@ build_lrouter_nd_flow(struct ovn_datapath *od, struct 
ovn_port *op,
   "};",
   action,
   eth_addr,
-  ip_address,
-  ip_address,
   eth_addr);
 ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_IP_INPUT, priority,
   ds_cstr(), ds_cstr(), NULL,
@@ -11899,7 +11896,6 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op,
>nbrp->header_, lflows);
 }
 
-const char *ip_address;
 if (sset_count(>od->lb_ips_v4)) {
 ds_clear(match);
 if (is_l3dgw_port(op)) {
@@ -11920,17 +11916,26 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op,
 ds_destroy(_balancer_ips_v4);
 }
 
-SSET_FOR_EACH (ip_address, >od->lb_ips_v6) {
+if (sset_count(>od->lb_ips_v6)) {
 ds_clear(match);
+ds_clear(actions);
+
+struct ds load_balancer_ips_v6 = DS_EMPTY_INITIALIZER;
+
+ds_put_cstr(_balancer_ips_v6, "{ ");
+ds_put_and_free_cstr(_balancer_ips_v6,
+ sset_join(>od->lb_ips_v6, ", ", " }"));
+
 if (is_l3dgw_port(op)) {
 ds_put_format(match, "is_chassis_resident(%s)",
   op->cr_port->json_key);
 }
-
 build_lrouter_nd_flow(op->od, op, "nd_na",
-  ip_address, NULL, REG_INPORT_ETH_ADDR,
-  match, false, 90, NULL,
-  lflows, meter_groups);
+  ds_cstr(_balancer_ips_v6), NULL,
+  REG_INPORT_ETH_ADDR, match, false, 90,
+  NULL, lflows, meter_groups);
+
+ds_destroy(_balancer_ips_v6);
 }
 
 if (!op->od->is_gw_router && !op->od->n_l3dgw_ports) {
diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index ff92c989c..d91f8111f 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -5537,6 +5537,44 @@ LogicalRouterNdFlow(router, lrp, "nd_na", ipv6, true, 
mac, extra_match, drop, pr
 LogicalRouterArpNdFlow(router, nat@NAT{.external_ip = IPv6{ipv6}}, lrp,
mac, extra_match, drop, priority).
 
+relation LogicalRouterNdFlowLB(
+lr: Intern,
+lrp: Option>,
+ip: string,
+mac: string,
+extra_match: Option,
+stage_hint: bit<32>)
+Flow(.logical_datapath = lr._uuid,
+ .stage = s_ROUTER_IN_IP_INPUT(),
+ .priority = 90,
+ .__match = __match.intern(),
+ .actions = actions,
+ .stage_hint = stage_hint,
+ .io_port = None,
+ .controller_meter = lr.copp.get(cOPP_ND_NA())) :-
+LogicalRouterNdFlowLB(.lr = lr, .lrp = lrp, .ip = ip,
+  .mac = mac, .extra_match = extra_match,
+  .stage_hint = stage_hint),
+var __match = {
+var clauses = vec_with_capacity(4);
+match (lrp) {
+Some{p} -> clauses.push("inport == ${json_string_escape(p.name)}"),
+None -> ()
+};
+clauses.push("nd_ns && nd.target == ${ip}");
+clauses.append(extra_match.to_vec());
+clauses.join(" && ")
+},
+var actions =
+i"nd_na { "
+   "eth.src = ${mac}; "
+   "ip6.src = nd.target; "
+   "nd.tll = ${mac}; "
+   "outport = inport; "
+   "flags.loopback = 1; "
+   "output; "
+ "};".
+
 relation LogicalRouterArpFlow(
 lr: Intern,
 lrp: Option>,
@@ -5622,8 

Re: [ovs-dev] [PATCH] dpdk: Use --in-memory by default.

[David Marchand]

On Fri, Aug 6, 2021 at 2:44 PM Rosemarie O'Riorden  wrote:
>
> If anonymous memory mapping is supported by the kernel, it's better
> to run OVS entirely in memory rather than creating shared data
> structures. OVS doesn't work in multi-process mode, so there is no need
> to litter a filesystem and experience random crashes due to old memory
> chunks stored in re-opened files.
>
> When OVS is not running in memory and crashes, it never reaches the
> clean up scripts that delete the new files it has created, resulting in
> "dirty" memory. OVS will partially overwrite this memory on the next
> start-up, but will fail to run again because its filesystem is full of
> old memory.
>
> Here is an example of these crashes:
>   https://inbox.dpdk.org/dev/20200910162407.12669-1-david.march...@redhat.com/

Overall, I am fine with this change.


- In the commitlog, I would focus on the simplication aspect and that
we only use what we need.
Mention of this bug can be removed.
The reason is that this is not as black/white as described: reusing
dirty memory only happened in a special case with containers.
But this does not affect OVS (I don't think we run OVS in containers).


- As mentionned by Ilya, MAP_HUGE_SHIFT is most likely supported on
systems running current OVS.
Can we remove the permissions tweak on /dev/hugepages in
rhel/usr_lib_systemd_system_ovs-vswitchd.service.in ?
(Cc: Timothy who might have an opinion on this).


- Somehow related:
* as a followup, we can deprecate --dpdk-hugepage-dir,
* I have been playing with --in-memory and I noticed some bug with the
telemetry socket.
The last started dpdk process takes control of the socket regardless
of other processes, so this happens when running multiple dpdk
applications with none or same XDG_RUNTIME_DIR set in env.
This is more of a fyi: it should not be a problem for people running
ovs with a dedicated user, but it could be a problem for those vilains
that run all their applications as root (/me whistles).


>
> Reported at: https://bugzilla.redhat.com/show_bug.cgi?id=1949849
> Signed-off-by: Rosemarie O'Riorden 


Thanks,

-- 
David Marchand

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

Re: [ovs-dev] [PATCH v14 6/7] ofproto: Introduce API to process sFlow offload packet

[Eelco Chaudron]

Two more comments to this patch…


On 7 Sep 2021, at 10:01, Eelco Chaudron wrote:

> On 15 Jul 2021, at 8:01, Chris Mi wrote:
>
>> Process sFlow offload packet in handler thread if handler id is 0.
>>
>> Signed-off-by: Chris Mi 
>> Reviewed-by: Eli Britstein 
>
> Thanks for making these changes, as this implementation looks way cleaner 
> than it was before!
>
>> ---
>>  ofproto/ofproto-dpif-upcall.c | 57 +++
>>  1 file changed, 57 insertions(+)
>>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index ccf97266c..7e934614d 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -22,6 +22,7 @@
>>  #include "connmgr.h"
>>  #include "coverage.h"
>>  #include "cmap.h"
>> +#include "lib/dpif-offload-provider.h"
>>  #include "lib/dpif-provider.h"
>>  #include "dpif.h"
>>  #include "openvswitch/dynamic-string.h"
>> @@ -742,6 +743,51 @@ udpif_get_n_flows(struct udpif *udpif)
>>  return flow_count;
>>  }
>>
>> +static void
>> +process_offload_sflow(struct udpif *udpif, struct dpif_offload_sflow *sflow)
>> +{
>> +const struct dpif_sflow_attr *attr = sflow->attr;
>> +struct user_action_cookie *cookie;
>> +struct dpif_sflow *dpif_sflow;
>> +struct ofproto_dpif *ofproto;
>> +struct upcall upcall;
>> +uint32_t iifindex;
>> +struct flow flow;
>> +
>> +if (!attr) {
>> +VLOG_WARN_RL(, "%s: dpif_sflow_attr is NULL", __func__);
>
> As these are more user log messages, not debug, I would change it to 
> something like:
>
> VLOG_WARN_RL(, "sFlow upcall is missing its attribute");
>
>> +return;
>> +}
>> +
>> +cookie = attr->userdata;
>
> Just to be safe should we also check if cookie is not NULL + log message?

In addition we should also make sure att->userdata_len is at least equal to 
sizeof *cookie.

>> +ofproto = ofproto_dpif_lookup_by_uuid(>ofproto_uuid);
>> +if (!ofproto) {
>> +VLOG_WARN_RL(, "%s: could not find ofproto", __func__);
>
> Maybe here we could also print the UUID, so it makes it possible to debug for 
> which tc rule the upcall was.
> VLOG_WARN_RL(, "sFlow upcall can't find ofproto dpif for UUID 
> "UUID_FMT", ...)
>
>> +return;
>> +}
>> +
>> +dpif_sflow = ofproto->sflow;
>> +if (!sflow) {
>
> Guess this needs to be dpif_sflow
>
>> +VLOG_WARN_RL(, "%s: could not find dpif_sflow", __func__);
>
> Guess there is no find here, so it might be the same as the first LOG above:
> VLOG_WARN_RL(, "sFlow upcall is missing dpif information");
>
>
>> +return;
>> +}
>> +
>> +memset(, 0, sizeof flow);
>> +if (attr->tunnel) {
>> +memcpy(, attr->tunnel, sizeof flow.tunnel);
>> +}
>> +iifindex = sflow->iifindex;
>> +flow.in_port.odp_port = netdev_ifindex_to_odp_port(iifindex);
>> +memset(, 0, sizeof upcall);
>> +upcall.flow = 
>> +upcall.cookie = *cookie;
>> +upcall.packet = >packet;
>> +upcall.sflow = dpif_sflow;
>> +upcall.ufid = >attr->ufid;

Here you should use the attr value? >ufid

>> +upcall.type = SFLOW_UPCALL;
>> +process_upcall(udpif, , NULL, NULL);
>> +}
>> +
>>  /* The upcall handler thread tries to read a batch of UPCALL_MAX_BATCH
>>   * upcalls from dpif, processes the batch and installs corresponding flows
>>   * in dpif. */
>> @@ -756,8 +802,19 @@ udpif_upcall_handler(void *arg)
>>  poll_immediate_wake();
>>  } else {
>>  dpif_recv_wait(udpif->dpif, handler->handler_id);
>> +dpif_offload_sflow_recv_wait(udpif->dpif);
>
> Guess this only needs to be called from handler 0, same as below.
>
>>  latch_wait(>exit_latch);
>>  }
>> +/* Only handler id 0 thread process sFlow offload packet. */
>> +if (handler->handler_id == 0) {
>> +struct dpif_offload_sflow sflow;
>> +int err;
>> +
>> +err = dpif_offload_sflow_recv(udpif->dpif, );
>> +if (!err) {
>> +process_offload_sflow(udpif, );
>> +}
>> +}
>>  poll_block();
>>  }
>>
>> -- 
>> 2.21.0

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

Re: [ovs-dev] [PATCH v2 ovn] northd: reduce number of nd_na lb logical flows

[Lorenzo Bianconi]

> On 8/27/21 2:00 PM, Lorenzo Bianconi wrote:
> > > Hi Lorenzo,
> > > 
> > > This mostly looks good, but I have one question below:
> > > 
> > > On 8/17/21 1:51 PM, Lorenzo Bianconi wrote:
> > > > As it has been already done for IPv4, collapse IPv6 Neighbour
> > > > Advertisment flows for load balancer using address list and
> > > > reduce the number of logical flows from N*M to N where N is
> > > > the number of logical router port and M is the number of
> > > > Virtual IPs.
> > > > 
> > > > https://bugzilla.redhat.com/show_bug.cgi?id=1970258
> > > > Signed-off-by: Lorenzo Bianconi 
> > > > ---
> > > > Changes since v1:
> > > > - rebase on top of ovn master
> > > > - add DDlog support
> > > > ---
> > > >northd/ovn-northd.c  | 41 +++
> > > >northd/ovn_northd.dl | 66 
> > > > 
> > > >tests/ovn-northd.at  | 36 
> > > >3 files changed, 90 insertions(+), 53 deletions(-)
> > > > 
> > > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > > > index e80876af1..ee08281c3 100644
> > > > --- a/northd/ovn-northd.c
> > > > +++ b/northd/ovn-northd.c
> > > > @@ -9694,8 +9694,7 @@ build_lrouter_nd_flow(struct ovn_datapath *od, 
> > > > struct ovn_port *op,
> > > >ds_put_format(,
> > > >  "%s { "
> > > >"eth.src = %s; "
> > > > -"ip6.src = %s; "
> > > > -"nd.target = %s; "
> > > > +"ip6.src = nd.target; "
> > > >"nd.tll = %s; "
> > > >"outport = inport; "
> > > >"flags.loopback = 1; "
> > > > @@ -9703,8 +9702,6 @@ build_lrouter_nd_flow(struct ovn_datapath *od, 
> > > > struct ovn_port *op,
> > > >  "};",
> > > >  action,
> > > >  eth_addr,
> > > > -  ip_address,
> > > > -  ip_address,
> > > >  eth_addr);
> > > >ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_IP_INPUT, 
> > > > priority,
> > > >  ds_cstr(), 
> > > > ds_cstr(), NULL,
> > > > @@ -11742,7 +11739,6 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op,
> > > >   >nbrp->header_, lflows);
> > > >}
> > > > -const char *ip_address;
> > > >if (sset_count(>od->lb_ips_v4)) {
> > > >ds_clear(match);
> > > >if (is_l3dgw_port(op)) {
> > > > @@ -11763,17 +11759,40 @@ build_lrouter_ipv4_ip_input(struct ovn_port 
> > > > *op,
> > > >ds_destroy(_balancer_ips_v4);
> > > >}
> > > > -SSET_FOR_EACH (ip_address, >od->lb_ips_v6) {
> > > > +if (sset_count(>od->lb_ips_v6)) {
> > > >ds_clear(match);
> > > > +ds_clear(actions);
> > > > +
> > > > +struct ds load_balancer_ips_v6 = DS_EMPTY_INITIALIZER;
> > > > +
> > > > +ds_put_cstr(_balancer_ips_v6, "{ ");
> > > > +ds_put_and_free_cstr(_balancer_ips_v6,
> > > > + sset_join(>od->lb_ips_v6, ", ", " 
> > > > }"));
> > > > +
> > > > +ds_put_format(match, "inport == %s && nd_ns && nd.target 
> > > > == %s",
> > > > +  op->json_key, 
> > > > ds_cstr(_balancer_ips_v6));
> > > >if (is_l3dgw_port(op)) {
> > > > -ds_put_format(match, "is_chassis_resident(%s)",
> > > > +ds_put_format(match, " && is_chassis_resident(%s)",
> > > >  op->cr_port->json_key);
> > > >}
> > > > +ds_put_format(actions,
> > > > +  "nd_na { "
> > > > +"eth.src = %s; "
> > > > +"ip6.src = nd.target; "
> > > > +"nd.tll = %s; "
> > > > +"outport = inport; "
> > > > +"flags.loopback = 1; "
> > > > +"output; "
> > > > +  "};",
> > > > +  REG_INPORT_ETH_ADDR,
> > > > +  REG_INPORT_ETH_ADDR);
> > > > +ovn_lflow_add_with_hint__(lflows, op->od, 
> > > > S_ROUTER_IN_IP_INPUT, 90,
> > > > +  ds_cstr(match), 
> > > > ds_cstr(actions), NULL,
> > > > +  copp_meter_get(COPP_ND_NA,
> > > > + op->od->nbr->copp,
> > > > + meter_groups), 
> > > > NULL);
> > > 
> > > I don't understand why you replaced the call to build_lrouter_nd_flow() 
> > > with
> > > manual creation of the match and actions. As long as you pass
> > > ds_cstr(_balancer_ips_v6) as the 

Re: [ovs-dev] [PATCH] netdev-linux: Fix a null pointer dereference in netdev_linux_notify_sock()

[David Marchand]

On Wed, Sep 8, 2021 at 1:53 PM Yunjian Wang  wrote:
>
> If nl_sock_join_mcgroup() returns an error, the 'sock' is freed
> and set to NULL. So we should add NULL check of 'sock' before calling
> nl_sock_listen_all_nsid().
>
> Fixes: cf114a7fce80 ("netlink linux: enable listening to all nsids")
> Cc: Flavio Leitner 
> Signed-off-by: Yunjian Wang 
> ---
>  lib/netdev-linux.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 60dd13891..7fec5f5a6 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -636,7 +636,9 @@ netdev_linux_notify_sock(void)
>  }
>  }
>  }
> -nl_sock_listen_all_nsid(sock, true);
> +if (sock) {
> +nl_sock_listen_all_nsid(sock, true);
> +}
>  ovsthread_once_done();
>  }
>

Would it make sense to move this call before the loop on groups?
Something like:

@@ -627,6 +627,7 @@ netdev_linux_notify_sock(void)
 if (!error) {
 size_t i;

+nl_sock_listen_all_nsid(sock, true);
 for (i = 0; i < ARRAY_SIZE(mcgroups); i++) {
 error = nl_sock_join_mcgroup(sock, mcgroups[i]);
 if (error) {
@@ -636,7 +637,6 @@ netdev_linux_notify_sock(void)
 }
 }
 }
-nl_sock_listen_all_nsid(sock, true);
 ovsthread_once_done();
 }


-- 
David Marchand

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

Re: [ovs-dev] [PATCH] conntrack : dpif parameter is used, need to remove OVS_UNUSED.

[David Marchand]

For the title, my suggestion is:

dpif-netdev: Remove unnecessary OVS_UNUSED.

On Fri, Aug 27, 2021 at 12:30 PM lin huang  wrote:
>
> The dpif parameter of zone limits function is used.
> We need to remove OVS_UNUSED flag.

Strictly speaking, there is no *need* for removing the flag.
But I agree this flag here can be dropped: the compiler would only
care for this hint if the variable was unused (which is not the case
here).

I would add a Fixes: tag in the commitlog.

Fixes: a7f33fdbfb67 ("conntrack: Support zone limits.")

>
> Signed-off-by: linhuang 

Please double check the robot warnings on Author and SoB tag.

With those comments fixed, you can add:
Reviewed-by: David Marchand 


-- 
David Marchand

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

Re: [ovs-dev] [PATCH ovn] ovn-northd: Avoid verification of NB_Global.options if nothing changed.

[Mark Gray]

On 08/09/2021 13:18, Ilya Maximets wrote:
> northd constructs transaction for the Northbound database options even
> if they don't need to be changed.  This also includes verification of
> a current state.  IDL will eventually drop the transaction if it will
> not contain any other operations, but if there are some other
> operations, this useless update to the same value will be included
> along with the verification.  This causes transaction failures because
> NB_Global.options can be updated by CMS at the same time.
> 
> For example, ovn-kubernetes sets 'options:e2e_timestamp' for it's own
> purposes, and if this value will be updated while there is an in-flight
> transaction from the northd, transaction will fail the verification and
> northd will have to re-try it.
> 
> To avoid these issues, updating and verifying options only if needed.
> 
> Fixes: b07f1bc3d068 ("Add VXLAN support for non-VTEP datapath bindings")
> Signed-off-by: Ilya Maximets 
> ---

Acked-by: Mark D. Gray 



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

Re: [ovs-dev] [PATCH ovn] ovn-northd: Fix update of a mac prefix.

[Mark Gray]

On 08/09/2021 13:16, Ilya Maximets wrote:
> 'smap_add' doesn't check for duplicates.  This leads to having
> more than one entry with a 'mac_prefix' key in the 'options' smap.
> 
> 'ovsdb_datum_sort_unique' removes duplicates before sending the
> transaction.  It does that by sorting values and keeping the first one
> per key.  In normal case 'mac_prefix' transitions from nothing to
> random and it works fine.  However, northd also tries to change
> all-zero prefix to random one and this fails, because all-zero prefix
> goes first in a sorted map and a new random value gets dropped from
> the transaction.  This makes northd to update macs of all ports
> on every iteration with a new random mac prefix.
> 
> Fix that by replacing smap value and not relying on IDL for removing
> duplicates.

Thanks, this looks good to me and I tested it and it worked. I couldn't
find this behavior (ability to assign a random mac) documented anywhere
(for example in ovn-nb.5). It might be worth documenting it.

Acked-by: Mark D. Gray 

> 
> Fixes: 39242c106eff ("northd: refactor and split some IPAM functions")
> Signed-off-by: Ilya Maximets 
> ---
>  northd/ovn-northd.c |  2 +-
>  tests/ovn.at| 13 +
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index ee761cef0..eef752542 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -14212,7 +14212,7 @@ ovnnb_db_run(struct northd_context *ctx,
>  struct smap options;
>  smap_clone(, >options);
>  
> -smap_add(, "mac_prefix", mac_addr_prefix);
> +smap_replace(, "mac_prefix", mac_addr_prefix);
>  
>  if (!monitor_mac) {
>  eth_addr_random(_monitor_mac_ea);
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 5104a6895..30625ec37 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -8030,6 +8030,19 @@ mac_prefix=$(ovn-nbctl --wait=sb get NB_Global . 
> options:mac_prefix | tr -d \")
>  port_addr=$(ovn-nbctl get Logical-Switch-Port p91 dynamic_addresses | tr -d 
> \")
>  AT_CHECK([test "$port_addr" = "${mac_prefix}:00:00:09"], [0], [])
>  
> +# set mac_prefix to all-zeroes and check it is allocated in a random manner
> +ovn-nbctl --wait=hv set NB_Global . options:mac_prefix="00:00:00:00:00:00"
> +ovn-nbctl ls-add sw14
> +ovn-nbctl --wait=sb set Logical-Switch sw14 other_config:mac_only=true
> +ovn-nbctl --wait=sb lsp-add sw14 p141 -- lsp-set-addresses p141 dynamic
> +
> +mac_prefix=$(ovn-nbctl --wait=sb get NB_Global . options:mac_prefix | tr -d 
> \")
> +port_addr=$(ovn-nbctl get Logical-Switch-Port p141 dynamic_addresses | tr -d 
> \")
> +AT_CHECK([test "$mac_prefix" != "00:00:00:00:00:00"], [0], [])
> +AT_CHECK([test "$port_addr" = "${mac_prefix}:00:00:0a"], [0], [])
> +ovn-nbctl --wait=sb lsp-del sw14 p141
> +ovn-nbctl --wait=sb ls-del sw14
> +
>  ovn-nbctl --wait=hv set NB_Global . options:mac_prefix="00:11:22"
>  ovn-nbctl ls-add sw10
>  ovn-nbctl --wait=sb set Logical-Switch sw10 other_config:ipv6_prefix="ae01::"
> 

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

Re: [ovs-dev] [PATCH v2 ovn] controller: add datapath meter capability check

[Mark Gray]

On 08/09/2021 14:27, Lorenzo Bianconi wrote:
> Dump datapath meter capabilities before configuring meters in
> ovn-controller
> 
> Signed-off-by: Lorenzo Bianconi 
> ---
> Changes since v1:
> - move rconn in ovn-controller to avoid concurrency issues
> ---
>  controller/lflow.c  |  2 +-
>  controller/ovn-controller.c | 65 +
>  controller/ovn-controller.h |  2 ++
>  3 files changed, 68 insertions(+), 1 deletion(-)
> 
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 923d8f0a4..d4fda6114 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -648,7 +648,7 @@ add_matches_to_flow_table(const struct sbrec_logical_flow 
> *lflow,
>  .ct_snat_vip_ptable = OFTABLE_CT_SNAT_HAIRPIN,
>  .fdb_ptable = OFTABLE_GET_FDB,
>  .fdb_lookup_ptable = OFTABLE_LOOKUP_FDB,
> -.ctrl_meter_id = ctrl_meter_id,
> +.ctrl_meter_id = dp_meter_capa > 0 ? ctrl_meter_id : 
> NX_CTLR_NO_METER,
>  };

I would prefer to not share this as a global variable to reduce the
coupling. I think you can pass this down to this function through
lflow_ctx_in.

>  ovnacts_encode(ovnacts->data, ovnacts->size, , );
>  
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 0031a1035..5e77f5c2e 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -73,6 +73,9 @@
>  #include "stopwatch.h"
>  #include "lib/inc-proc-eng.h"
>  #include "hmapx.h"
> +#include "openvswitch/rconn.h"
> +#include "openvswitch/ofp-msgs.h"
> +#include "openvswitch/ofp-meter.h"
>  
>  VLOG_DEFINE_THIS_MODULE(main);
>  
> @@ -117,6 +120,9 @@ static const char *ssl_private_key_file;
>  static const char *ssl_certificate_file;
>  static const char *ssl_ca_cert_file;
>  
> +/* datapath meter capabilities. */
> +int dp_meter_capa = -1;

Is there a reason this is an 'int'. I can't see and I think you are just
trying to show if the functionality is available or not? maybe a bool
would be better.

> +
>  /* By default don't set an upper bound for the lflow cache and enable auto
>   * trimming above 10K logical flows when reducing cache size by 50%.
>   */
> @@ -3065,6 +3071,58 @@ check_northd_version(struct ovsdb_idl *ovs_idl, struct 
> ovsdb_idl *ovnsb_idl,
>  return true;
>  }
>  
> +static void
> +ovn_controller_connect_vswitch(struct rconn *swconn,
> +   const struct ovsrec_bridge *br_int)
> +{
> +if (!rconn_is_connected(swconn)) {
> +char *target = xasprintf("unix:%s/%s.mgmt", ovs_rundir(),
> + br_int->name);
> +if (strcmp(target, rconn_get_target(swconn))) {
> +VLOG_INFO("%s: connecting to switch", target);
> +rconn_connect(swconn, target, target);
> +}
> +free(target);
> +}
> +}
> +
> +static void
> +ovn_controller_get_meter_capa(struct rconn *swconn)
> +{
> +if (dp_meter_capa >= 0) {
> +return;
> +}
> +
> +rconn_run(swconn);
> +if (!rconn_is_connected(swconn)) {
> +return;
> +}
> +
> +/* dump datapath meter capabilities. */
> +struct ofpbuf *msg = ofpraw_alloc(OFPRAW_OFPST13_METER_FEATURES_REQUEST,
> +  rconn_get_version(swconn), 0);
> +rconn_send(swconn, msg, NULL);
> +for (int i = 0; i < 10; i++) {
> +msg = rconn_recv(swconn);
> +if (!msg) {
> +break;
> +}
> +
> +const struct ofp_header *oh = msg->data;
> +enum ofptype type;
> +ofptype_decode(, oh);
> +
> +if (type == OFPTYPE_METER_FEATURES_STATS_REPLY) {
> +struct ofputil_meter_features mf;
> +
> +ofputil_decode_meter_features(oh, );
> +dp_meter_capa = mf.max_meters > 0;
> +engine_set_force_recompute(true);
> +}
> +ofpbuf_delete(msg);
> +}
> +}
> +
>  int
>  main(int argc, char *argv[])
>  {
> @@ -3445,6 +3503,8 @@ main(int argc, char *argv[])
>  char *ovn_version = ovn_get_internal_version();
>  VLOG_INFO("OVN internal version is : [%s]", ovn_version);
>  
> +struct rconn *swconn = rconn_create(5, 0, DSCP_DEFAULT,
> +1 << OFP15_VERSION);
>  /* Main loop. */
>  exiting = false;
>  restart = false;
> @@ -3523,6 +3583,10 @@ main(int argc, char *argv[])
> ovsrec_server_has_datapath_table(ovs_idl_loop.idl)
> ? _int_dp
> : NULL);
> +if (br_int) {
> +ovn_controller_connect_vswitch(swconn, br_int);
> +}
> +ovn_controller_get_meter_capa(swconn);

This reminds me a lot of what ovs_feature_is_supported() in
features.c/.h is trying to do. I think you should integrate your change
into there somehow rather than creating a once-off interface for dp-meters.

Currently, features.c is only checking the capabilities by reading the
bridge capabilities table in OVS but I