Re: [ovs-dev] [PATCH v3] utilities/ofctl: add-meters for save and restore

2023-02-24 Thread Adrian Moreno 

Hi Wan,

Sorry for the delay in the review.
I've started looking at this patch. Please see some initial comments below. I'll 
continue on Monday.


Thanks.

On 11/15/22 05:16, Wan Junjie wrote:

put dump-meters' result in one line so add-meters can handle.
save and restore meters when restart ovs.
bundle functions are not implemented in this patch.

Signed-off-by: Wan Junjie 
---
v3:
add '--oneline' option for dump-meter(s) command

v2:
fix failed testcases, as dump-meters format changes
---
  include/openvswitch/ofp-meter.h |   9 +-
  lib/ofp-meter.c | 103 -
  lib/ofp-print.c |   5 +-
  tests/dpif-netdev.at|   9 ++
  utilities/ovs-ofctl.8.in|  20 ++-
  utilities/ovs-ofctl.c   | 263 +---
  utilities/ovs-save  |   8 +
  7 files changed, 383 insertions(+), 34 deletions(-)

diff --git a/include/openvswitch/ofp-meter.h b/include/openvswitch/ofp-meter.h
index 6776eae87..0514b4ec4 100644
--- a/include/openvswitch/ofp-meter.h
+++ b/include/openvswitch/ofp-meter.h
@@ -61,7 +61,8 @@ int ofputil_decode_meter_config(struct ofpbuf *,
  struct ofputil_meter_config *,
  struct ofpbuf *bands);
  void ofputil_format_meter_config(struct ds *,
- const struct ofputil_meter_config *);
+ const struct ofputil_meter_config *,
+ int);
  
  struct ofputil_meter_mod {

  uint16_t command;
@@ -79,6 +80,12 @@ char *parse_ofp_meter_mod_str(struct ofputil_meter_mod *, 
const char *string,
  OVS_WARN_UNUSED_RESULT;
  void ofputil_format_meter_mod(struct ds *, const struct ofputil_meter_mod *);
  
+char *parse_ofp_meter_mod_file(const char *file_name,

+ int command,
+ struct ofputil_meter_mod **mms, size_t *n_mms,
+ enum ofputil_protocol *usable_protocols)
+OVS_WARN_UNUSED_RESULT;
+


Please align all the arguments one space after the "(". Also I think "int 
command" fits in the first line.




  struct ofputil_meter_stats {
  uint32_t meter_id;
  uint32_t flow_count;
diff --git a/lib/ofp-meter.c b/lib/ofp-meter.c
index 9ea40a0bf..b94cb6a05 100644
--- a/lib/ofp-meter.c
+++ b/lib/ofp-meter.c
@@ -15,6 +15,7 @@
   */
  
  #include 

+#include 
  #include "openvswitch/ofp-meter.h"
  #include "byte-order.h"
  #include "nx-match.h"
@@ -57,7 +58,7 @@ void
  ofputil_format_meter_band(struct ds *s, enum ofp13_meter_flags flags,
const struct ofputil_meter_band *mb)
  {
-ds_put_cstr(s, "\ntype=");
+ds_put_cstr(s, "type=");
  switch (mb->type) {
  case OFPMBT13_DROP:
  ds_put_cstr(s, "drop");
@@ -343,7 +344,7 @@ ofp_print_meter_flags(struct ds *s, enum ofp13_meter_flags 
flags)
  
  void

  ofputil_format_meter_config(struct ds *s,
-const struct ofputil_meter_config *mc)
+const struct ofputil_meter_config *mc, int oneline)
  {
  uint16_t i;
  
@@ -354,9 +355,12 @@ ofputil_format_meter_config(struct ds *s,
  
  ds_put_cstr(s, "bands=");

  for (i = 0; i < mc->n_bands; ++i) {
+ds_put_cstr(s, oneline > 0 ? " ": "\n");
  ofputil_format_meter_band(s, mc->flags, >bands[i]);
  }
-ds_put_char(s, '\n');
+if (oneline == 0) {
+ds_put_char(s, '\n');
+}
  }
  
  static enum ofperr

@@ -578,6 +582,24 @@ parse_ofp_meter_mod_str__(struct ofputil_meter_mod *mm, 
char *string,
  
  /* Meters require at least OF 1.3. */

  *usable_protocols = OFPUTIL_P_OF13_UP;
+if (command == -2) {
+size_t len;
+
+string += strspn(string, " \t\r\n");   /* Skip white space. */
+len = strcspn(string, ", \t\r\n"); /* Get length of the first token. */
+
+if (!strncmp(string, "add", len)) {
+command = OFPMC13_ADD;
+} else if (!strncmp(string, "delete", len)) {
+command = OFPMC13_DELETE;
+} else if (!strncmp(string, "modify", len)) {
+command = OFPMC13_MODIFY;
+} else {
+len = 0;
+command = OFPMC13_ADD;
+}
+string += len;
+}
  
  switch (command) {

  case -1:
@@ -606,6 +628,11 @@ parse_ofp_meter_mod_str__(struct ofputil_meter_mod *mm, 
char *string,
  mm->meter.n_bands = 0;
  mm->meter.bands = NULL;
  
+if (command == OFPMC13_DELETE && string[0] == '\0') {

+mm->meter.meter_id = OFPM13_ALL;
+return NULL;
+}
+
  if (fields & F_BANDS) {
  band_str = strstr(string, "band");
  if (!band_str) {
@@ -805,5 +832,73 @@ ofputil_format_meter_mod(struct ds *s, const struct 
ofputil_meter_mod *mm)
  ds_put_format(s, " cmd:%d ", mm->command);
  }
  
-ofputil_format_meter_config(s, >meter);

+ofputil_format_meter_config(s, >meter, 0);
+}
+
+/* If 'command' is

Re: [ovs-dev] [PATCH] ipfix: make template and stats interval configurable

2023-02-24 Thread Simon Horman 
On Fri, Feb 24, 2023 at 04:08:46PM +0100, Adrian Moreno wrote:
> 
> 
> On 2/24/23 14:09, Simon Horman wrote:
> > On Fri, Feb 24, 2023 at 12:05:45PM +0100, Adrian Moreno wrote:
> > > Please ignore this patch that was sent without the correct version.
> > 
> > Is the "v4" posting the correct revision?
> > 
> > https://mail.openvswitch.org/pipermail/ovs-dev/2023-February/402308.html
> > 
> 
> Yes, I resent it with the "v4" subject header.

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

Re: [ovs-dev] [PATCH] ipfix: make template and stats interval configurable

2023-02-24 Thread Adrian Moreno 



On 2/24/23 14:09, Simon Horman wrote:

On Fri, Feb 24, 2023 at 12:05:45PM +0100, Adrian Moreno wrote:

Please ignore this patch that was sent without the correct version.


Is the "v4" posting the correct revision?

https://mail.openvswitch.org/pipermail/ovs-dev/2023-February/402308.html



Yes, I resent it with the "v4" subject header.

--
Adrián Moreno

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

Re: [ovs-dev] [PATCH] ipfix: make template and stats interval configurable

2023-02-24 Thread Simon Horman 
On Fri, Feb 24, 2023 at 12:05:45PM +0100, Adrian Moreno wrote:
> Please ignore this patch that was sent without the correct version.

Is the "v4" posting the correct revision?

https://mail.openvswitch.org/pipermail/ovs-dev/2023-February/402308.html
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ofproto: Fix re-creation of tunnel backing interfaces on restart.

2023-02-24 Thread Simon Horman 
On Thu, Feb 23, 2023 at 05:39:27PM +0100, Ilya Maximets wrote:
> Tunnel OpenFlow ports do not exist in the datapath, instead there is a
> tunnel backing interface that serves all the tunnels of the same type.
> For example, if the geneve port 'my_tunnel' is added to OVS, it will
> create 'geneve_sys_6041' datapath port, if it doesn't already exist,
> and use this port as a tunnel output.
> 
> However, while creating/opening a new datapath after re-start,
> ovs-vswitchd only has a list of names of OpenFlow interfaces.  And it
> thinks that each datapath port, that is not on the list, is a stale
> port that needs to be removed.  This is obviously not correct for
> tunnel backing interfaces that can serve multiple tunnel ports and do
> not match OpenFlow port names.
> 
> This is causing removal and re-creation of all the tunnel backing
> interfaces in the datapath on OVS restart, causing disruption in
> existing connections.
> 
> It's hard to tell by only having a name of the interface if this
> interface is a tunnel backing interface, or someone just named a
> normal interface this way.  So, instead of trying to determine that,
> not removing any interfaces at all, while we don't know types of
> actual ports we need.
> 
> Assuming that all the ports that are currently not in the list of OF
> ports are tunnel backing ports.  Later, revalidation of tunnel backing
> ports in type_run() will determine which ports are still needed and
> which should be removed.
> 
> It's OK to add even a non-tunnel stale ports into tnl_backers, they
> will be cleaned up the same way as stale tunnel backers.
> 
> Reported-at: 
> https://mail.openvswitch.org/pipermail/ovs-discuss/2023-February/052215.html
> Signed-off-by: Ilya Maximets 

Thanks Ilya,

this looks good to me.
I have a few comments below, but those notwithstanding,

Reviewed-by: Simon Horman 

...

> @@ -729,8 +723,6 @@ open_dpif_backer(const char *type, struct dpif_backer 
> **backerp)
>  struct dpif_port_dump port_dump;
>  struct dpif_port port;
>  struct shash_node *node;
> -struct ovs_list garbage_list;
> -struct odp_garbage *garbage;
>  
>  struct sset names;
>  char *backer_name;
> @@ -792,25 +784,23 @@ open_dpif_backer(const char *type, struct dpif_backer 
> **backerp)
>  dpif_flow_flush(backer->dpif);
>  }
>  
> -/* Loop through the ports already on the datapath and remove any
> - * that we don't need anymore. */
> -ovs_list_init(_list);
> +/* Loop through the ports already on the datapath and find ones that are
> + * not on the initial OpenFlow ports list.  These are stale ports, that 
> we
> + * do not need anymore, or tunnel backing interfaces, that do not 
> generally
> + * match the name of OpenFlow tunnel ports, or both.  Adding all of them 
> to

nit: s/Adding/Add/

> + * the list of tunnel backers.  type_run() will garbage collect those 
> that
> + * are not active tunnel backing interfaces during revalidation. */

...

> diff --git a/tests/system-interface.at b/tests/system-interface.at
> index 784bada12..6cd8ae802 100644
> --- a/tests/system-interface.at
> +++ b/tests/system-interface.at
> @@ -63,3 +63,62 @@ AT_CHECK([
>  [stdout], [Device "br-p1" does not exist.]
>  )
>  AT_CLEANUP
> +
> +AT_SETUP([interface - datapath ports garbage collection])
> +OVS_CHECK_GENEVE()
> +OVS_TRAFFIC_VSWITCHD_START()
> +
> +dnl Not relevant for userspace datapath.
> +AT_SKIP_IF([! ovs-appctl dpctl/show | grep -q ovs-system])
> +
> +AT_CHECK([ovs-vsctl add-port br0 tunnel_port dnl
> +-- set Interface tunnel_port dnl
> +   type=geneve options:remote_ip=flow options:key=123])
> +
> +AT_CHECK([ip link add ovs-veth0 type veth peer name ovs-veth1])
> +on_exit 'ip link del ovs-veth0'
> +
> +AT_CHECK([ovs-vsctl add-port br0 ovs-veth0])
> +
> +OVS_WAIT_UNTIL([ip link show | grep ovs-system | grep -q genev])
> +
> +dnl Store the output of ip link for geneve port to compare ifindex later.
> +AT_CHECK([ip link show | grep ovs-system | grep genev > geneve.0])
> +
> +AT_CHECK([ovs-appctl dpctl/show | grep port], [0], [dnl
> +  port 0: ovs-system (internal)
> +  port 1: br0 (internal)
> +  port 2: genev_sys_6081 (geneve: packet_type=ptap)
> +  port 3: ovs-veth0
> +])
> +
> +OVS_APP_EXIT_AND_WAIT_BY_TARGET([ovs-vswitchd], [ovs-vswitchd.pid])
> +
> +dnl Check that geneve backing interface is still in the datapath.
> +AT_CHECK([ip link show | grep ovs-system | grep genev | diff -u - geneve.0])
> +
> +dnl Remove the veth port from the database while ovs-vswitchd is down.
> +AT_CHECK([ovs-vsctl --no-wait del-port ovs-veth0])
> +
> +dnl Check that it is still tied to the OVS datapath.
> +AT_CHECK([ip link show | grep ovs-system | grep -q ovs-veth0])
> +
> +dnl Bring ovs-vswitchd back up.
> +AT_CHECK([ovs-vswitchd --detach --no-chdir --pidfile --log-file -vdpif:dbg],
> + [0], [], [stderr])
> +
> +dnl Wait for the veth port to be removed from the datapath.
>

[ovs-dev] [PATCH v2] test: move check for tc ingress pps support to test script

2023-02-24 Thread Simon Horman 
Move check for tc ingress pps support to from aclocal to test script

This has several problems:

1. Stderror from failing commands is output when executing
   various make targets.

2. There are various failure conditions that lead
   to veth0 and veth1 being created by not cleaned up.

3. The check seems to execute for many make targets.
   And it attempts to temporarily modify system state.
   This seems inappropriate.

4. veth0 and veth1 seem far too generic and could easily
   conflict with other parts of the system.

All these problems are addressed by this patch.

Signed-off-by: Simon Horman 
Reviewed-by: Louis Peens 
---
v2
* As suggested by Ilya, use:
  - ovs_tc_pps rather than veth as prefix for interface names
  - on_exit for cleanup, rather than handling each case
  - dnl rather than '/' for multi-line commands
---
 tests/atlocal.in | 11 ---
 tests/system-offloads-traffic.at | 14 --
 2 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/tests/atlocal.in b/tests/atlocal.in
index e02248f6f829..859668586299 100644
--- a/tests/atlocal.in
+++ b/tests/atlocal.in
@@ -172,17 +172,6 @@ fi
 # Set HAVE_TC
 find_command tc
 
-# When HAVE_TC=yes, check if the current tc supports adding pps filter
-SUPPORT_TC_INGRESS_PPS="no"
-if test $HAVE_TC="yes"; then
-ip link add veth0 type veth peer name veth1
-tc qdisc add dev veth0 handle : ingress
-if tc filter add dev veth0 parent : u32 match u32 0 0 police pkts_rate 
100 pkts_burst 10; then
-SUPPORT_TC_INGRESS_PPS="yes"
-fi
-ip link del veth0
-fi
-
 # Set HAVE_TCPDUMP
 find_command tcpdump
 
diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at
index f2bf9c0639aa..1d21da5f196d 100644
--- a/tests/system-offloads-traffic.at
+++ b/tests/system-offloads-traffic.at
@@ -18,6 +18,16 @@ m4_define([OVS_CHECK_ACTIONS], [
[0], [$1])
 ])
 
+m4_define([CHECK_TC_INGRESS_PPS],
+[
+AT_SKIP_IF([test $HAVE_TC = "no"])
+AT_CHECK([ip link add ovs_tc_pps0 type veth peer name ovs_tc_pps1 || dnl
+  exit 77])
+on_exit 'ip link del ovs_tc_pps0'
+AT_CHECK([tc qdisc add dev ovs_tc_pps0 handle : ingress || exit 77])
+AT_CHECK([tc filter add dev ovs_tc_pps0 parent : u32 match dnl
+  u32 0 0 police pkts_rate 100 pkts_burst 10 || exit 77])
+])
 
 AT_SETUP([offloads - ping between two ports - offloads disabled])
 OVS_TRAFFIC_VSWITCHD_START()
@@ -132,7 +142,7 @@ AT_CLEANUP
 
 AT_SETUP([offloads - set ingress_policing_kpkts_rate and 
ingress_policing_kpkts_burst - offloads disabled])
 AT_KEYWORDS([ingress_policing_kpkts])
-AT_SKIP_IF([test $SUPPORT_TC_INGRESS_PPS = "no"])
+CHECK_TC_INGRESS_PPS()
 OVS_TRAFFIC_VSWITCHD_START()
 AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=false])
 AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
@@ -156,7 +166,7 @@ AT_CLEANUP
 
 AT_SETUP([offloads - set ingress_policing_kpkts_rate and 
ingress_policing_kpkts_burst - offloads enabled])
 AT_KEYWORDS([ingress_policing_kpkts])
-AT_SKIP_IF([test $SUPPORT_TC_INGRESS_PPS = "no"])
+CHECK_TC_INGRESS_PPS()
 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)
-- 
2.30.2

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

Re: [ovs-dev] [PATCH ovn] northd: add router broadcast option to logical switch

2023-02-24 Thread 0-day Robot 
Bleep bloop.  Greetings Felix Hüttner, 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 Felix Hüttner  needs to sign off.
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Felix Huettner 
WARNING: Line is 92 characters long (recommended limit is 79)
#69 FILE: northd/northd.c:8968:
if (!smap_get_bool(>nbs->other_config, 
"broadcast-arps-to-all-routers", true)) {

Lines checked: 247, Warnings: 2, 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 ovn] northd: fix comments on functions

2023-02-24 Thread 0-day Robot 
Bleep bloop.  Greetings Felix Hüttner, 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 Felix Hüttner  needs to sign off.
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Felix Huettner 
WARNING: Line is 80 characters long (recommended limit is 79)
#49 FILE: northd/northd.c:8339:
/* Ingress table 25/26: Destination lookup for unknown MACs (priority 0). */

Lines checked: 161, Warnings: 2, 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] ipfix: make template and stats interval configurable

2023-02-24 Thread Adrian Moreno 

Please ignore this patch that was sent without the correct version.

On 2/24/23 12:03, Adrián Moreno wrote:

From: Adrian Moreno 

Add options to the IPFIX table configure the interval to send statistics
and template information.

Reviewed-by: Simon Horman 
Signed-off-by: Adrian Moreno 

---
- v4:
- Bump vswitchd's schema version to 8.4.0.
- v3:
- Removed unit tests which generate errors in Intel CI. Will submit in
  independent series.
- v2:
   - Fixed a potential race condition in unit test.
- v1:
   - Added unit test.
---
  NEWS |  2 ++
  ofproto/ofproto-dpif-ipfix.c | 38 ++--
  ofproto/ofproto.h|  9 +
  vswitchd/bridge.c| 17 
  vswitchd/vswitch.ovsschema   | 14 +++--
  vswitchd/vswitch.xml | 20 +++
  6 files changed, 88 insertions(+), 12 deletions(-)

diff --git a/NEWS b/NEWS
index 85b349621..1794de046 100644
--- a/NEWS
+++ b/NEWS
@@ -56,6 +56,8 @@ v3.1.0 - 16 Feb 2023
   * Add new experimental PMD load based sleeping feature. PMD threads can
 request to sleep up to a user configured 'pmd-maxsleep' value under
 low load conditions.
+   - IPFIX template and statistics intervals can be configured through two new
+   options in the IPFIX table: 'template_interval' and 'stats_interval'.
  
  
  v3.0.0 - 15 Aug 2022

diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index f13478a88..e6c2968f7 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -140,6 +140,8 @@ struct dpif_ipfix_exporter {
  struct ovs_list cache_flow_start_timestamp_list;  /* 
ipfix_flow_cache_entry. */
  uint32_t cache_active_timeout;  /* In seconds. */
  uint32_t cache_max_flows;
+uint32_t stats_interval;
+uint32_t template_interval;
  char *virtual_obs_id;
  uint8_t virtual_obs_len;
  
@@ -174,11 +176,6 @@ struct dpif_ipfix {
  
  #define IPFIX_VERSION 0x000a
  
-/* When using UDP, IPFIX Template Records must be re-sent regularly.

- * The standard default interval is 10 minutes (600 seconds).
- * Cf. IETF RFC 5101 Section 10.3.6. */
-#define IPFIX_TEMPLATE_INTERVAL 600
-
  /* Cf. IETF RFC 5101 Section 3.1. */
  OVS_PACKED(
  struct ipfix_header {
@@ -637,6 +634,8 @@ ofproto_ipfix_bridge_exporter_options_equal(
  && a->sampling_rate == b->sampling_rate
  && a->cache_active_timeout == b->cache_active_timeout
  && a->cache_max_flows == b->cache_max_flows
+&& a->stats_interval == b->stats_interval
+&& a->template_interval == b->template_interval
  && a->enable_tunnel_sampling == b->enable_tunnel_sampling
  && a->enable_input_sampling == b->enable_input_sampling
  && a->enable_output_sampling == b->enable_output_sampling
@@ -674,6 +673,8 @@ ofproto_ipfix_flow_exporter_options_equal(
  return (a->collector_set_id == b->collector_set_id
  && a->cache_active_timeout == b->cache_active_timeout
  && a->cache_max_flows == b->cache_max_flows
+&& a->stats_interval == b->stats_interval
+&& a->template_interval == b->template_interval
  && a->enable_tunnel_sampling == b->enable_tunnel_sampling
  && sset_equals(>targets, >targets)
  && nullable_string_is_equal(a->virtual_obs_id, 
b->virtual_obs_id));
@@ -712,6 +713,9 @@ dpif_ipfix_exporter_init(struct dpif_ipfix_exporter 
*exporter)
  ovs_list_init(>cache_flow_start_timestamp_list);
  exporter->cache_active_timeout = 0;
  exporter->cache_max_flows = 0;
+exporter->stats_interval = OFPROTO_IPFIX_DEFAULT_TEMPLATE_INTERVAL;
+exporter->template_interval = OFPROTO_IPFIX_DEFAULT_TEMPLATE_INTERVAL;
+exporter->last_stats_sent_time = 0;
  exporter->virtual_obs_id = NULL;
  exporter->virtual_obs_len = 0;
  hmap_init(>domains);
@@ -734,6 +738,9 @@ dpif_ipfix_exporter_clear(struct dpif_ipfix_exporter 
*exporter)
  exporter->last_stats_sent_time = 0;
  exporter->cache_active_timeout = 0;
  exporter->cache_max_flows = 0;
+exporter->stats_interval = OFPROTO_IPFIX_DEFAULT_TEMPLATE_INTERVAL;
+exporter->template_interval = OFPROTO_IPFIX_DEFAULT_TEMPLATE_INTERVAL;
+exporter->last_stats_sent_time = 0;
  free(exporter->virtual_obs_id);
  exporter->virtual_obs_id = NULL;
  exporter->virtual_obs_len = 0;
@@ -761,6 +768,8 @@ dpif_ipfix_exporter_set_options(struct dpif_ipfix_exporter 
*exporter,
  const struct sset *targets,
  const uint32_t cache_active_timeout,
  const uint32_t cache_max_flows,
+const uint32_t stats_interval,
+const uint32_t template_interval,
  const char *virtual_obs_id) 
OVS_REQUIRES(mutex)
  {
  size_t

[ovs-dev] [PATCH v4] ipfix: make template and stats interval configurable

2023-02-24 Thread Adrián Moreno 
From: Adrian Moreno 

Add options to the IPFIX table configure the interval to send statistics
and template information.

Reviewed-by: Simon Horman 
Signed-off-by: Adrian Moreno 

---
- v4:
   - Bump vswitchd's schema version to 8.4.0.
- v3:
   - Removed unit tests which generate errors in Intel CI. Will submit in
 independent series.
- v2:
  - Fixed a potential race condition in unit test.
- v1:
  - Added unit test.
---
 NEWS |  2 ++
 ofproto/ofproto-dpif-ipfix.c | 38 ++--
 ofproto/ofproto.h|  9 +
 vswitchd/bridge.c| 17 
 vswitchd/vswitch.ovsschema   | 14 +++--
 vswitchd/vswitch.xml | 20 +++
 6 files changed, 88 insertions(+), 12 deletions(-)

diff --git a/NEWS b/NEWS
index 85b349621..1794de046 100644
--- a/NEWS
+++ b/NEWS
@@ -56,6 +56,8 @@ v3.1.0 - 16 Feb 2023
  * Add new experimental PMD load based sleeping feature. PMD threads can
request to sleep up to a user configured 'pmd-maxsleep' value under
low load conditions.
+   - IPFIX template and statistics intervals can be configured through two new
+   options in the IPFIX table: 'template_interval' and 'stats_interval'.
 
 
 v3.0.0 - 15 Aug 2022
diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index f13478a88..e6c2968f7 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -140,6 +140,8 @@ struct dpif_ipfix_exporter {
 struct ovs_list cache_flow_start_timestamp_list;  /* 
ipfix_flow_cache_entry. */
 uint32_t cache_active_timeout;  /* In seconds. */
 uint32_t cache_max_flows;
+uint32_t stats_interval;
+uint32_t template_interval;
 char *virtual_obs_id;
 uint8_t virtual_obs_len;
 
@@ -174,11 +176,6 @@ struct dpif_ipfix {
 
 #define IPFIX_VERSION 0x000a
 
-/* When using UDP, IPFIX Template Records must be re-sent regularly.
- * The standard default interval is 10 minutes (600 seconds).
- * Cf. IETF RFC 5101 Section 10.3.6. */
-#define IPFIX_TEMPLATE_INTERVAL 600
-
 /* Cf. IETF RFC 5101 Section 3.1. */
 OVS_PACKED(
 struct ipfix_header {
@@ -637,6 +634,8 @@ ofproto_ipfix_bridge_exporter_options_equal(
 && a->sampling_rate == b->sampling_rate
 && a->cache_active_timeout == b->cache_active_timeout
 && a->cache_max_flows == b->cache_max_flows
+&& a->stats_interval == b->stats_interval
+&& a->template_interval == b->template_interval
 && a->enable_tunnel_sampling == b->enable_tunnel_sampling
 && a->enable_input_sampling == b->enable_input_sampling
 && a->enable_output_sampling == b->enable_output_sampling
@@ -674,6 +673,8 @@ ofproto_ipfix_flow_exporter_options_equal(
 return (a->collector_set_id == b->collector_set_id
 && a->cache_active_timeout == b->cache_active_timeout
 && a->cache_max_flows == b->cache_max_flows
+&& a->stats_interval == b->stats_interval
+&& a->template_interval == b->template_interval
 && a->enable_tunnel_sampling == b->enable_tunnel_sampling
 && sset_equals(>targets, >targets)
 && nullable_string_is_equal(a->virtual_obs_id, b->virtual_obs_id));
@@ -712,6 +713,9 @@ dpif_ipfix_exporter_init(struct dpif_ipfix_exporter 
*exporter)
 ovs_list_init(>cache_flow_start_timestamp_list);
 exporter->cache_active_timeout = 0;
 exporter->cache_max_flows = 0;
+exporter->stats_interval = OFPROTO_IPFIX_DEFAULT_TEMPLATE_INTERVAL;
+exporter->template_interval = OFPROTO_IPFIX_DEFAULT_TEMPLATE_INTERVAL;
+exporter->last_stats_sent_time = 0;
 exporter->virtual_obs_id = NULL;
 exporter->virtual_obs_len = 0;
 hmap_init(>domains);
@@ -734,6 +738,9 @@ dpif_ipfix_exporter_clear(struct dpif_ipfix_exporter 
*exporter)
 exporter->last_stats_sent_time = 0;
 exporter->cache_active_timeout = 0;
 exporter->cache_max_flows = 0;
+exporter->stats_interval = OFPROTO_IPFIX_DEFAULT_TEMPLATE_INTERVAL;
+exporter->template_interval = OFPROTO_IPFIX_DEFAULT_TEMPLATE_INTERVAL;
+exporter->last_stats_sent_time = 0;
 free(exporter->virtual_obs_id);
 exporter->virtual_obs_id = NULL;
 exporter->virtual_obs_len = 0;
@@ -761,6 +768,8 @@ dpif_ipfix_exporter_set_options(struct dpif_ipfix_exporter 
*exporter,
 const struct sset *targets,
 const uint32_t cache_active_timeout,
 const uint32_t cache_max_flows,
+const uint32_t stats_interval,
+const uint32_t template_interval,
 const char *virtual_obs_id) OVS_REQUIRES(mutex)
 {
 size_t virtual_obs_len;
@@ -775,6 +784,8 @@ dpif_ipfix_exporter_set_options(struct dpif_ipfix_exporter 
*exporter,
 }
 exporter->cache_active_timeout = cache_active_timeout;

[ovs-dev] [PATCH ovn] northd: add router broadcast option to logical switch

2023-02-24 Thread Felix Hüttner via dev 
Assume the following setup:

++
| Logical Router |
| lr001  +-+
++ |
   |
++ |
| Logical Router | | ++ +--+
| lr002  +-+-+ Logical Switch +-+ Phyiscal Network |
++ | | ls-ext | |  |
   | ++ +--+
  ...  |
   |
++ |
| Logical Router | |
| lr300  +-+
++

If a arp request for the ip of lr001 on ls-ext is now received it is
only forwarded to that individual logical router.

If we however now receive a arp request for an ip not used by any of
lr001-lr300 we try to flood the arp request to all logical ports on ls-ext.
With around 300 routers this causes the arp request to be dropped after
some routers as we hit the 4096 resubmit limit.

In the most cases forwarding the arp requests to the logical routers is
pointless as we already know all of their ip addresses and they will
therefor not be able to answer the arp requests anyway.
Only if someone sends garps this is not the case. Then the request would
need to be flooded to all logical routers.

We can therefor not generally send these arp requests to MC_FLOOD_L2 as
this would break garps. As we can also not detect garps we need to leave
the solution to our users.

To do this we introduce the other_config `broadcast-arps-to-all-routers`
on logical switches (which is per default true). If set to false we add
a logical flow that forwards arp requests where we do not know a
specific target logical switch port to MC_FLOOD_L2, thereby bypassing
all logical routers.

Note that the testcase is quite flaky in the ci (as it takes verry long)
but runs well locally. I'm unsure how to best proceed there.

Signed-off-by: Felix Huettner 
---
 northd/northd.c |   7 +++
 northd/ovn-northd.8.xml |   7 +++
 ovn-nb.xml  |  12 +
 tests/ovn.at| 114 
 4 files changed, 140 insertions(+)

diff --git a/northd/northd.c b/northd/northd.c
index c366b545e..6aff04cc5 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -8964,6 +8964,13 @@ build_lswitch_destination_lookup_bmcast(struct 
ovn_datapath *od,
 }
 }

+
+if (!smap_get_bool(>nbs->other_config, 
"broadcast-arps-to-all-routers", true)) {
+ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 72,
+"eth.mcast && (arp.op == 1 || nd_ns)",
+"outport = \""MC_FLOOD_L2"\"; output;");
+}
+
 ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 70, "eth.mcast",
   "outport = \""MC_FLOOD"\"; output;");
 }
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 2eab2c4ae..033841383 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -1873,6 +1873,13 @@ output;
 non-router logical ports.
   

+  
+A priority-72 flow that outputs all ARP requests and ND packets with
+an Ethernet broadcast or multicast eth.dst to the
+MC_FLOOD_L2 multicast group if
+other_config:broadcast-arps-to-all-routers=true.
+  
+
   
 A priority-70 flow that outputs all packets with an Ethernet broadcast
 or multicast eth.dst to the MC_FLOOD
diff --git a/ovn-nb.xml b/ovn-nb.xml
index 8d56d0c6e..a41d5b11f 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -729,6 +729,18 @@
 localnet ports, fabric traffic that belongs to other tagged networks 
may
 be passed through such a port.
   
+
+  
+Determines whether arp requests and ipv6 neighbor solicitations should
+be send to all routers and other switchports (default) or if it should
+only be send to switchports where the ip/mac address is unknown.
+Setting this to false can significantly reduce the load if the logical
+switch can receive arp requests for ips it does not know about.
+However setting this to false also means that garps are no longer
+forwarded to all routers and therefor the mac bindings of the routers
+are no longer updated.
+  
 

 
diff --git a/tests/ovn.at b/tests/ovn.at
index dc5c5df3f..dfef5dacc 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -5048,6 +5048,120 @@ OVN_CLEANUP([hv1], [hv2], [hv3])
 AT_CLEANUP
 ])

+# 1 hypervisor, 1 logical switch with 1 logical port, 300 logical router
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([1 HVs, 1 LS, 2 lports/LS, 300 LR])
+AT_KEYWORDS([slowtest])
+ovn_start
+
+# Logical network:
+#
+# One logical switch ls1.
+# 300 logical routers lr001 - lr299. All connected to ls1
+# with one subnet:
+#lrp001 on ls1 for subnet 192.168.0.1/20
+#lrp002 on ls1 for subnet 192.168.0.2/20
+#...
+#lrp101 on ls1 for subnet 192.168.1.1/20
+#...
+#lrp299 on ls1 for subnet 192.168.2.99/20
+#
+# also 2 VIF on the first hypervisor.
+#

[ovs-dev] [PATCH] ipfix: make template and stats interval configurable

2023-02-24 Thread Adrián Moreno 
From: Adrian Moreno 

Add options to the IPFIX table configure the interval to send statistics
and template information.

Reviewed-by: Simon Horman 
Signed-off-by: Adrian Moreno 

---
- v4:
   - Bump vswitchd's schema version to 8.4.0.
- v3:
   - Removed unit tests which generate errors in Intel CI. Will submit in
 independent series.
- v2:
  - Fixed a potential race condition in unit test.
- v1:
  - Added unit test.
---
 NEWS |  2 ++
 ofproto/ofproto-dpif-ipfix.c | 38 ++--
 ofproto/ofproto.h|  9 +
 vswitchd/bridge.c| 17 
 vswitchd/vswitch.ovsschema   | 14 +++--
 vswitchd/vswitch.xml | 20 +++
 6 files changed, 88 insertions(+), 12 deletions(-)

diff --git a/NEWS b/NEWS
index 85b349621..1794de046 100644
--- a/NEWS
+++ b/NEWS
@@ -56,6 +56,8 @@ v3.1.0 - 16 Feb 2023
  * Add new experimental PMD load based sleeping feature. PMD threads can
request to sleep up to a user configured 'pmd-maxsleep' value under
low load conditions.
+   - IPFIX template and statistics intervals can be configured through two new
+   options in the IPFIX table: 'template_interval' and 'stats_interval'.
 
 
 v3.0.0 - 15 Aug 2022
diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index f13478a88..e6c2968f7 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -140,6 +140,8 @@ struct dpif_ipfix_exporter {
 struct ovs_list cache_flow_start_timestamp_list;  /* 
ipfix_flow_cache_entry. */
 uint32_t cache_active_timeout;  /* In seconds. */
 uint32_t cache_max_flows;
+uint32_t stats_interval;
+uint32_t template_interval;
 char *virtual_obs_id;
 uint8_t virtual_obs_len;
 
@@ -174,11 +176,6 @@ struct dpif_ipfix {
 
 #define IPFIX_VERSION 0x000a
 
-/* When using UDP, IPFIX Template Records must be re-sent regularly.
- * The standard default interval is 10 minutes (600 seconds).
- * Cf. IETF RFC 5101 Section 10.3.6. */
-#define IPFIX_TEMPLATE_INTERVAL 600
-
 /* Cf. IETF RFC 5101 Section 3.1. */
 OVS_PACKED(
 struct ipfix_header {
@@ -637,6 +634,8 @@ ofproto_ipfix_bridge_exporter_options_equal(
 && a->sampling_rate == b->sampling_rate
 && a->cache_active_timeout == b->cache_active_timeout
 && a->cache_max_flows == b->cache_max_flows
+&& a->stats_interval == b->stats_interval
+&& a->template_interval == b->template_interval
 && a->enable_tunnel_sampling == b->enable_tunnel_sampling
 && a->enable_input_sampling == b->enable_input_sampling
 && a->enable_output_sampling == b->enable_output_sampling
@@ -674,6 +673,8 @@ ofproto_ipfix_flow_exporter_options_equal(
 return (a->collector_set_id == b->collector_set_id
 && a->cache_active_timeout == b->cache_active_timeout
 && a->cache_max_flows == b->cache_max_flows
+&& a->stats_interval == b->stats_interval
+&& a->template_interval == b->template_interval
 && a->enable_tunnel_sampling == b->enable_tunnel_sampling
 && sset_equals(>targets, >targets)
 && nullable_string_is_equal(a->virtual_obs_id, b->virtual_obs_id));
@@ -712,6 +713,9 @@ dpif_ipfix_exporter_init(struct dpif_ipfix_exporter 
*exporter)
 ovs_list_init(>cache_flow_start_timestamp_list);
 exporter->cache_active_timeout = 0;
 exporter->cache_max_flows = 0;
+exporter->stats_interval = OFPROTO_IPFIX_DEFAULT_TEMPLATE_INTERVAL;
+exporter->template_interval = OFPROTO_IPFIX_DEFAULT_TEMPLATE_INTERVAL;
+exporter->last_stats_sent_time = 0;
 exporter->virtual_obs_id = NULL;
 exporter->virtual_obs_len = 0;
 hmap_init(>domains);
@@ -734,6 +738,9 @@ dpif_ipfix_exporter_clear(struct dpif_ipfix_exporter 
*exporter)
 exporter->last_stats_sent_time = 0;
 exporter->cache_active_timeout = 0;
 exporter->cache_max_flows = 0;
+exporter->stats_interval = OFPROTO_IPFIX_DEFAULT_TEMPLATE_INTERVAL;
+exporter->template_interval = OFPROTO_IPFIX_DEFAULT_TEMPLATE_INTERVAL;
+exporter->last_stats_sent_time = 0;
 free(exporter->virtual_obs_id);
 exporter->virtual_obs_id = NULL;
 exporter->virtual_obs_len = 0;
@@ -761,6 +768,8 @@ dpif_ipfix_exporter_set_options(struct dpif_ipfix_exporter 
*exporter,
 const struct sset *targets,
 const uint32_t cache_active_timeout,
 const uint32_t cache_max_flows,
+const uint32_t stats_interval,
+const uint32_t template_interval,
 const char *virtual_obs_id) OVS_REQUIRES(mutex)
 {
 size_t virtual_obs_len;
@@ -775,6 +784,8 @@ dpif_ipfix_exporter_set_options(struct dpif_ipfix_exporter 
*exporter,
 }
 exporter->cache_active_timeout = cache_active_timeout;

[ovs-dev] [PATCH ovn] northd: fix comments on functions

2023-02-24 Thread Felix Hüttner via dev 
the comments did refer to tables 1 below the actual table number.

Signed-off-by: Felix Huettner 
---
 northd/northd.c | 34 +-
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 770a5b50e..c366b545e 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -7856,7 +7856,7 @@ build_lrouter_groups(struct hmap *ports, struct ovs_list 
*lr_list)
 }

 /*
- * Ingress table 24: Flows that flood self originated ARP/RARP/ND packets in
+ * Ingress table 25: Flows that flood self originated ARP/RARP/ND packets in
  * the switching domain.
  */
 static void
@@ -7970,7 +7970,7 @@ lrouter_port_ipv6_reachable(const struct ovn_port *op,
 }

 /*
- * Ingress table 24: Flows that forward ARP/ND requests only to the routers
+ * Ingress table 25: Flows that forward ARP/ND requests only to the routers
  * that own the addresses. Other ARP/ND packets are still flooded in the
  * switching domain as regular broadcast.
  */
@@ -8007,7 +8007,7 @@ build_lswitch_rport_arp_req_flow(const char *ips,
 }

 /*
- * Ingress table 24: Flows that forward ARP/ND requests only to the routers
+ * Ingress table 25: Flows that forward ARP/ND requests only to the routers
  * that own the addresses.
  * Priorities:
  * - 80: self originated GARPs that need to follow regular processing.
@@ -8336,7 +8336,7 @@ build_lswitch_flows(const struct hmap *datapaths,

 struct ovn_datapath *od;

-/* Ingress table 25: Destination lookup for unknown MACs (priority 0). */
+/* Ingress table 25/26: Destination lookup for unknown MACs (priority 0). 
*/
 HMAP_FOR_EACH (od, key_node, datapaths) {
 if (!od->nbs) {
 continue;
@@ -8411,7 +8411,7 @@ build_lswitch_lflows_admission_control(struct 
ovn_datapath *od,
 }
 }

-/* Ingress table 18: ARP/ND responder, skip requests coming from localnet
+/* Ingress table 98: ARP/ND responder, skip requests coming from localnet
  * ports. (priority 100); see ovn-northd.8.xml for the rationale. */

 static void
@@ -8429,7 +8429,7 @@ build_lswitch_arp_nd_responder_skip_local(struct ovn_port 
*op,
 }
 }

-/* Ingress table 18: ARP/ND responder, reply for known IPs.
+/* Ingress table 19: ARP/ND responder, reply for known IPs.
  * (priority 50). */
 static void
 build_lswitch_arp_nd_responder_known_ips(struct ovn_port *op,
@@ -8689,7 +8689,7 @@ build_lswitch_arp_nd_responder_known_ips(struct ovn_port 
*op,
 }
 }

-/* Ingress table 18: ARP/ND responder, by default goto next.
+/* Ingress table 19: ARP/ND responder, by default goto next.
  * (priority 0)*/
 static void
 build_lswitch_arp_nd_responder_default(struct ovn_datapath *od,
@@ -8700,7 +8700,7 @@ build_lswitch_arp_nd_responder_default(struct 
ovn_datapath *od,
 }
 }

-/* Ingress table 18: ARP/ND responder for service monitor source ip.
+/* Ingress table 19: ARP/ND responder for service monitor source ip.
  * (priority 110)*/
 static void
 build_lswitch_arp_nd_service_monitor(struct ovn_northd_lb *lb,
@@ -8769,7 +8769,7 @@ build_lswitch_arp_nd_service_monitor(struct ovn_northd_lb 
*lb,
 }


-/* Logical switch ingress table 19 and 20: DHCP options and response
+/* Logical switch ingress table 20 and 21: DHCP options and response
  * priority 100 flows. */
 static void
 build_lswitch_dhcp_options_and_response(struct ovn_port *op,
@@ -8821,11 +8821,11 @@ build_lswitch_dhcp_options_and_response(struct ovn_port 
*op,
 }
 }

-/* Ingress table 19 and 20: DHCP options and response, by default goto
+/* Ingress table 20 and 21: DHCP options and response, by default goto
  * next. (priority 0).
- * Ingress table 21 and 22: DNS lookup and response, by default goto next.
+ * Ingress table 22 and 23: DNS lookup and response, by default goto next.
  * (priority 0).
- * Ingress table 23 - External port handling, by default goto next.
+ * Ingress table 24 - External port handling, by default goto next.
  * (priority 0). */
 static void
 build_lswitch_dhcp_and_dns_defaults(struct ovn_datapath *od,
@@ -8840,7 +8840,7 @@ build_lswitch_dhcp_and_dns_defaults(struct ovn_datapath 
*od,
 }
 }

-/* Logical switch ingress table 21 and 22: DNS lookup and response
+/* Logical switch ingress table 22 and 23: DNS lookup and response
 * priority 100 flows.
 */
 static void
@@ -8868,7 +8868,7 @@ build_lswitch_dns_lookup_and_response(struct ovn_datapath 
*od,
 }
 }

-/* Table 23: External port. Drop ARP request for router ips from
+/* Table 24: External port. Drop ARP request for router ips from
  * external ports  on chassis not binding those ports.
  * This makes the router pipeline to be run only on the chassis
  * binding the external ports. */
@@ -8885,7 +8885,7 @@ build_lswitch_external_port(struct ovn_port *op,
 }
 }

-/* Ingress table 24: Destination lookup, broadcast and multicast handling
+/* Ingress table 25: Destination lookup, broadcast and multicast handling
  * (priority 70 - 100). */
 static void
 build_lswitch_destination_lookup_bmcast(struct

[ovs-dev] [PATCH 0/1] usdt-scripts: use ebpf to track upcall_cost

2023-02-24 Thread Adrián Moreno 
I've been recently looking into upcall tracing inside OVS and its
current challenges. 

The current implementation is available in
utilities/usdt-scripts/upcall_cost.py. Essentially it collects all the
events and collelates them in userspace after the data collection has
ended.

I have been working on improving this implementation using ebpf to
perform the bulk of the correlation so that the script has less work to
do. As a result, not only the script is much faster but also is more
accurate since we now don't need to send packet bytes to userspace,
reducing the ring buffer congestion.

This is a summary of the improvements of this implementation:

- More accuracy:
- 6% reduction in event misses
- 57% increase in collected events
- 982% increase in number of analyzable event sets
- Less run time: 
- Total time of analysis drops from 27 minutes to 24 seconds.
- Support for fragments:
- We count the upcall cost of each fragment independently

* The numeric results correspond to a 20s iperf3 run with OVS
  to upcall 100% of traffic.


Adrian Moreno (1):
  usdt-scripts: use ebpf to track upcall_cost

 utilities/usdt-scripts/upcall_cost.py | 2657 +++--
 1 file changed, 1568 insertions(+), 1089 deletions(-)

-- 
2.39.2

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