date:20211130

Re: [ovs-dev] [PATCH v3 1/4] system-dpdk: Refactor common logs matching.

2021-11-30 Thread Maxime Coquelin





On 11/30/21 16:00, David Marchand wrote:

Move EAL logs and commonly ignored logs to a common macro.
Remove obsolete ones (like i40e [1] and timer [2] logs).
Set log level for DPDK drivers to error only: the rationale is that we are
not testing DPDK drivers in system-dpdk.
Extend regex on hugepage logs since a check on hugepages availability is
already present on OVS side, and as a consequence, we don't care about
the warnings on availability for certain hugepage size.
Add logs checks for MFEX tests that were missing them.

1: https://git.dpdk.org/dpdk/commit/?id=a075ce2b3e8c
2: https://git.dpdk.org/dpdk/commit/?id=c1077933d45b

Signed-off-by: David Marchand 
---
Changes since v2:
- configured dpdk drivers log level to error,
- added OVS flow control warning to common list,

---
  tests/system-dpdk-macros.at |  3 ++
  tests/system-dpdk.at| 56 ++---
  2 files changed, 31 insertions(+), 28 deletions(-)



Reviewed-by: Maxime Coquelin 

Thanks,
Maxime

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

Re: [ovs-dev] [PATCH dpdk-latest] acinclude: Enable -Werror by default

2021-11-30 Thread Eli Britstein via dev




On 11/30/2021 2:06 PM, Ilya Maximets wrote:

External email: Use caution opening links or attachments


On 11/30/21 10:43, Eli Britstein wrote:

On 11/30/2021 12:31 AM, Ilya Maximets wrote:

External email: Use caution opening links or attachments


On 11/7/21 11:56, Eli Britstein via dev wrote:

Following dpdk commits [1]-[3], it is now possible to compile with
--enable-Werror. Change the default to on, with an option to disable
using --disable-Werror.

Notes:
1. To compile against 21.11-rc1, need to apply [4] and [5] patches.
2. There are still sparse errors, due to dpdk issue. [6] fixes it.

[1] a3f8d0587188 ("net: avoid cast-align warning in VLAN insert function")
[2] da0333c8790b ("mbuf: avoid cast-align warning in data offset macro")
[3] 6de430b7079e ("eal/x86: avoid cast-align warning in memcpy functions")
[4] https://patchwork.ozlabs.org/project/openvswitch/list/?series=268844
[5] https://patchwork.ozlabs.org/project/openvswitch/list/?series=261231
[6] 
https://patches.dpdk.org/project/dpdk/patch/20211028101428.15007-1-david.march...@redhat.com/

Signed-off-by: Eli Britstein 
Reviewed-by: Gaetan Rivet 
---
   .ci/linux-build.sh | 1 -
   .cirrus.yml| 2 +-
   acinclude.m4   | 4 ++--
   3 files changed, 3 insertions(+), 4 deletions(-)

Hi, Eli.  I'm not sure if I understand the reason behind this patch.

In linux x86 at least, we had warnings by dpdk. We discussed it in [7], and 
fixed some in OVS by [8].

The fixes in dpdk were merged (see above [1]-[3]), so I thought to have this 
option by default to prevent future warnings.

Hmm.  Maybe it's better to just remove -Wno-cast-align from CI scripts
instead?  Current version of a patch will not catch cast-align warnings
in our CI, since they are explicitly disabled.


Hi Ilya,

I cherry-picked my commit on top of the current dpdk-latest/dpdk 
branches, that already includes [4]-[6].


Also, I removed the -Wno-cast-align, 
https://github.com/elibritstein/OVS/commit/5663a73555ce3c79bf16d2519660bc4fc3e5d8f4, 
and pushed for CI.



CI for windows passed, though I do see warnings:

https://ci.appveyor.com/project/elibritstein/ovs/build/job/ejn39u0cr9f42l6c

https://ci.appveyor.com/project/elibritstein/ovs/build/job/emus2vv79ihcd4ng

GitHub actions passed:

https://github.com/elibritstein/OVS/actions/runs/1521686491


Should I squash the removal of -Wno-cast-align and post v2?

What do you suggest next?

Thanks,

Eli




We did encounter some issue on PPC, still pending to a proper resolution, [9].


[7] 
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fpipermail%2Fovs-dev%2F2021-July%2F384773.htmldata=04%7C01%7Celibr%40nvidia.com%7Cfbaf5bb4739649fbbebd08d9b3f9ea57%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637738708996798389%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=FA1FbBWhbCa6xaDDIYtWBVSJtLrKXd05SR4Mn4GltSM%3Dreserved=0

[8] 
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fpipermail%2Fovs-dev%2F2021-July%2F385482.htmldata=04%7C01%7Celibr%40nvidia.com%7Cfbaf5bb4739649fbbebd08d9b3f9ea57%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637738708996798389%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=YFtFa%2Fzspqp3cABpFb55Cykhg%2Fhpp9plfpSgLxE%2BZUQ%3Dreserved=0

[9] 
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fpipermail%2Fovs-dev%2F2021-November%2F389381.htmldata=04%7C01%7Celibr%40nvidia.com%7Cfbaf5bb4739649fbbebd08d9b3f9ea57%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637738708996798389%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=h%2Fl2gxScO%2BYHpEHUjXZjGxQBtLpmv9P4IXjrF4aCV3k%3Dreserved=0


But, in any case, I believe that it will break the Windows build, as
it currently produces a fair amount of warnings.

I admit I haven't tested windows. I tried to look into it, but could not find a 
free CI tool for that.

I tried to follow 
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.openvswitch.org%2Fen%2Flatest%2Fintro%2Finstall%2Fwindows%2Fdata=04%7C01%7Celibr%40nvidia.com%7Cfbaf5bb4739649fbbebd08d9b3f9ea57%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637738708996798389%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=4bjVibsMdat27ukfyWX9zUyEPW2%2FhKFHX9EDQXveYjs%3Dreserved=0,
 but could not understand what option to choose in 
https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.mingw.org%2Fwiki%2FGetting_Starteddata=04%7C01%7Celibr%40nvidia.com%7Cfbaf5bb4739649fbbebd08d9b3f9ea57%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637738708996798389%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=HlOnsopwilA0WyL3auvhGF5DZlrCadfCyOwLBl%2BJmsY%3Dreserved=0

Could you please advise?

We're using

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

2021-11-30 Thread 0-day Robot

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

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


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Rosemarie O'Riorden 
Lines checked: 79, Warnings: 1, Errors: 0


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

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

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

2021-11-30 Thread Mike Pattrick

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/

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1949849
Signed-off-by: Rosemarie O'Riorden 
Signed-off-by: Mike Pattrick 
---
 NEWS | 1 +
 acinclude.m4 | 6 ++
 lib/dpdk.c   | 7 +++
 3 files changed, 14 insertions(+)

diff --git a/NEWS b/NEWS
index dce1aeb2b..f3015c664 100644
--- a/NEWS
+++ b/NEWS
@@ -10,6 +10,7 @@ Post-v2.16.0
limiting behavior.
  * Add hardware offload support for matching IPv4/IPv6 frag types
(experimental).
+ * EAL argument --in-memory is applied by default if supported.
- Python:
  * For SSL support, the use of the pyOpenSSL library has been replaced
with the native 'ssl' module.
diff --git a/acinclude.m4 b/acinclude.m4
index 8ab690f47..700a0ce15 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -472,6 +472,12 @@ AC_DEFUN([OVS_CHECK_DPDK], [
   ], [[#include ]])
 ], [], [[#include ]])
 
+AC_CHECK_DECL([MAP_HUGE_SHIFT], [
+  AC_DEFINE([DPDK_IN_MEMORY_SUPPORTED], [1], [If MAP_HUGE_SHIFT is
+ defined, anonomous memory mapping is supported by the
+ kernel, and --in-memory can be used.])
+], [], [[#include ]])
+
 # DPDK uses dlopen to load plugins.
 OVS_FIND_DEPENDENCY([dlopen], [dl], [libdl])
 
diff --git a/lib/dpdk.c b/lib/dpdk.c
index b2ef31cd2..6cdd69bd2 100644
--- a/lib/dpdk.c
+++ b/lib/dpdk.c
@@ -405,6 +405,13 @@ dpdk_init__(const struct smap *ovs_other_config)
 svec_add(, ovs_get_program_name());
 construct_dpdk_args(ovs_other_config, );
 
+#ifdef DPDK_IN_MEMORY_SUPPORTED
+if (!args_contains(, "--in-memory") &&
+!args_contains(, "--legacy-mem")) {
+svec_add(, "--in-memory");
+}
+#endif
+
 if (args_contains(, "-c") || args_contains(, "-l")) {
 auto_determine = false;
 }
-- 
2.27.0

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

Re: [ovs-dev] [PATCH ovn] lflow-cache: Automatically trim cache when it becomes inactive.

2021-11-30 Thread Numan Siddique

On Tue, Nov 30, 2021 at 12:07 PM Dumitru Ceara  wrote:
>
> On 11/30/21 17:54, Numan Siddique wrote:
> > On Tue, Nov 23, 2021 at 2:49 PM Dumitru Ceara  wrote:
> >>
> >> By default perform memory trimming 30 seconds after the lflow cache
> >> utilization has dropped.  Otherwise, idle ovn-controller processes might
> >> wastefully fail to return unused memory to the system.  The timeout is
> >> configurable through the 'ovn-trim-timeout-ms' external_id.
> >>
> >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2024768
> >> Signed-off-by: Dumitru Ceara 
> >
> > Thanks for adding this support.  The patch and approach LGTM.
> >
> > I just have one question.  Please see below.
>
> Thanks for the review!
>
> >
> > Thanks
> > Numan
> >
> >> ---
> >>  NEWS|  3 ++
> >>  controller/chassis.c| 16 
> >>  controller/lflow-cache.c| 67 -
> >>  controller/lflow-cache.h|  5 ++-
> >>  controller/ovn-controller.8.xml |  8 
> >>  controller/ovn-controller.c |  9 -
> >>  controller/test-lflow-cache.c   | 13 +--
> >>  7 files changed, 114 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/NEWS b/NEWS
> >> index e27aaad06..9880a678e 100644
> >> --- a/NEWS
> >> +++ b/NEWS
> >> @@ -12,6 +12,9 @@ Post v21.09.0
> >>  running on SmartNIC control plane CPUs.  Please refer to
> >>  Documentation/topics/vif-plug-providers/vif-plug-providers.rst for 
> >> more
> >>  information.
> >> +  - ovn-controller: Add configuration knob, through OVS external-id
> >> +"ovn-trim-timeout-ms" to allow specifiying an lflow cache inactivity
> >> +timeout after which ovn-controller should trim memory.
> >>
> >>  OVN v21.09.0 - 01 Oct 2021
> >>  --
> >> diff --git a/controller/chassis.c b/controller/chassis.c
> >> index c11c10a34..8a1559653 100644
> >> --- a/controller/chassis.c
> >> +++ b/controller/chassis.c
> >> @@ -54,6 +54,7 @@ struct ovs_chassis_cfg {
> >>  const char *memlimit_lflow_cache;
> >>  const char *trim_limit_lflow_cache;
> >>  const char *trim_wmark_perc_lflow_cache;
> >> +const char *trim_timeout_ms;
> >>
> >>  /* Set of encap types parsed from the 'ovn-encap-type' external-id. */
> >>  struct sset encap_type_set;
> >> @@ -163,6 +164,12 @@ get_trim_wmark_perc_lflow_cache(const struct smap 
> >> *ext_ids)
> >>  return smap_get_def(ext_ids, "ovn-trim-wmark-perc-lflow-cache", "");
> >>  }
> >>
> >> +static const char *
> >> +get_trim_timeout(const struct smap *ext_ids)
> >> +{
> >> +return smap_get_def(ext_ids, "ovn-trim-timeout-ms", "");
> >> +}
> >> +
> >>  static const char *
> >>  get_encap_csum(const struct smap *ext_ids)
> >>  {
> >> @@ -292,6 +299,7 @@ chassis_parse_ovs_config(const struct 
> >> ovsrec_open_vswitch_table *ovs_table,
> >>  get_trim_limit_lflow_cache(>external_ids);
> >>  ovs_cfg->trim_wmark_perc_lflow_cache =
> >>  get_trim_wmark_perc_lflow_cache(>external_ids);
> >> +ovs_cfg->trim_timeout_ms = get_trim_timeout(>external_ids);
> >>
> >>  if (!chassis_parse_ovs_encap_type(encap_type, 
> >> _cfg->encap_type_set)) {
> >>  return false;
> >> @@ -336,6 +344,7 @@ chassis_build_other_config(const struct 
> >> ovs_chassis_cfg *ovs_cfg,
> >>   ovs_cfg->trim_limit_lflow_cache);
> >>  smap_replace(config, "ovn-trim-wmark-perc-lflow-cache",
> >>   ovs_cfg->trim_wmark_perc_lflow_cache);
> >> +smap_replace(config, "ovn-trim-timeout-ms", ovs_cfg->trim_timeout_ms);
> >>  smap_replace(config, "iface-types", 
> >> ds_cstr_ro(_cfg->iface_types));
> >>  smap_replace(config, "ovn-chassis-mac-mappings", 
> >> ovs_cfg->chassis_macs);
> >>  smap_replace(config, "is-interconn",
> >> @@ -415,6 +424,13 @@ chassis_other_config_changed(const struct 
> >> ovs_chassis_cfg *ovs_cfg,
> >>  return true;
> >>  }
> >>
> >> +const char *chassis_trim_timeout_ms =
> >> +get_trim_timeout(_rec->other_config);
> >> +
> >> +if (strcmp(ovs_cfg->trim_timeout_ms, chassis_trim_timeout_ms)) {
> >> +return true;
> >> +}
> >> +
> >>  const char *chassis_mac_mappings =
> >>  get_chassis_mac_mappings(_rec->other_config);
> >>  if (strcmp(ovs_cfg->chassis_macs, chassis_mac_mappings)) {
> >> diff --git a/controller/lflow-cache.c b/controller/lflow-cache.c
> >> index 6db85fc12..9c3db06e7 100644
> >> --- a/controller/lflow-cache.c
> >> +++ b/controller/lflow-cache.c
> >> @@ -24,8 +24,10 @@
> >>  #include "coverage.h"
> >>  #include "lflow-cache.h"
> >>  #include "lib/uuid.h"
> >> +#include "openvswitch/poll-loop.h"
> >>  #include "openvswitch/vlog.h"
> >>  #include "ovn/expr.h"
> >> +#include "timeval.h"
> >>
> >>  VLOG_DEFINE_THIS_MODULE(lflow_cache);
> >>
> >> @@ -59,7 +61,10 @@ struct lflow_cache {
> >>  uint64_t max_mem_usage;
> >>  uint32_t trim_limit;
> >>  uint32_t trim_wmark_perc;
> >> +uint32_t trim_timeout_ms;
>

Re: [ovs-dev] [PATCH ovn] lflow-cache: Automatically trim cache when it becomes inactive.

2021-11-30 Thread Dumitru Ceara

On 11/30/21 17:54, Numan Siddique wrote:
> On Tue, Nov 23, 2021 at 2:49 PM Dumitru Ceara  wrote:
>>
>> By default perform memory trimming 30 seconds after the lflow cache
>> utilization has dropped.  Otherwise, idle ovn-controller processes might
>> wastefully fail to return unused memory to the system.  The timeout is
>> configurable through the 'ovn-trim-timeout-ms' external_id.
>>
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2024768
>> Signed-off-by: Dumitru Ceara 
> 
> Thanks for adding this support.  The patch and approach LGTM.
> 
> I just have one question.  Please see below.

Thanks for the review!

> 
> Thanks
> Numan
> 
>> ---
>>  NEWS|  3 ++
>>  controller/chassis.c| 16 
>>  controller/lflow-cache.c| 67 -
>>  controller/lflow-cache.h|  5 ++-
>>  controller/ovn-controller.8.xml |  8 
>>  controller/ovn-controller.c |  9 -
>>  controller/test-lflow-cache.c   | 13 +--
>>  7 files changed, 114 insertions(+), 7 deletions(-)
>>
>> diff --git a/NEWS b/NEWS
>> index e27aaad06..9880a678e 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -12,6 +12,9 @@ Post v21.09.0
>>  running on SmartNIC control plane CPUs.  Please refer to
>>  Documentation/topics/vif-plug-providers/vif-plug-providers.rst for more
>>  information.
>> +  - ovn-controller: Add configuration knob, through OVS external-id
>> +"ovn-trim-timeout-ms" to allow specifiying an lflow cache inactivity
>> +timeout after which ovn-controller should trim memory.
>>
>>  OVN v21.09.0 - 01 Oct 2021
>>  --
>> diff --git a/controller/chassis.c b/controller/chassis.c
>> index c11c10a34..8a1559653 100644
>> --- a/controller/chassis.c
>> +++ b/controller/chassis.c
>> @@ -54,6 +54,7 @@ struct ovs_chassis_cfg {
>>  const char *memlimit_lflow_cache;
>>  const char *trim_limit_lflow_cache;
>>  const char *trim_wmark_perc_lflow_cache;
>> +const char *trim_timeout_ms;
>>
>>  /* Set of encap types parsed from the 'ovn-encap-type' external-id. */
>>  struct sset encap_type_set;
>> @@ -163,6 +164,12 @@ get_trim_wmark_perc_lflow_cache(const struct smap 
>> *ext_ids)
>>  return smap_get_def(ext_ids, "ovn-trim-wmark-perc-lflow-cache", "");
>>  }
>>
>> +static const char *
>> +get_trim_timeout(const struct smap *ext_ids)
>> +{
>> +return smap_get_def(ext_ids, "ovn-trim-timeout-ms", "");
>> +}
>> +
>>  static const char *
>>  get_encap_csum(const struct smap *ext_ids)
>>  {
>> @@ -292,6 +299,7 @@ chassis_parse_ovs_config(const struct 
>> ovsrec_open_vswitch_table *ovs_table,
>>  get_trim_limit_lflow_cache(>external_ids);
>>  ovs_cfg->trim_wmark_perc_lflow_cache =
>>  get_trim_wmark_perc_lflow_cache(>external_ids);
>> +ovs_cfg->trim_timeout_ms = get_trim_timeout(>external_ids);
>>
>>  if (!chassis_parse_ovs_encap_type(encap_type, 
>> _cfg->encap_type_set)) {
>>  return false;
>> @@ -336,6 +344,7 @@ chassis_build_other_config(const struct ovs_chassis_cfg 
>> *ovs_cfg,
>>   ovs_cfg->trim_limit_lflow_cache);
>>  smap_replace(config, "ovn-trim-wmark-perc-lflow-cache",
>>   ovs_cfg->trim_wmark_perc_lflow_cache);
>> +smap_replace(config, "ovn-trim-timeout-ms", ovs_cfg->trim_timeout_ms);
>>  smap_replace(config, "iface-types", ds_cstr_ro(_cfg->iface_types));
>>  smap_replace(config, "ovn-chassis-mac-mappings", ovs_cfg->chassis_macs);
>>  smap_replace(config, "is-interconn",
>> @@ -415,6 +424,13 @@ chassis_other_config_changed(const struct 
>> ovs_chassis_cfg *ovs_cfg,
>>  return true;
>>  }
>>
>> +const char *chassis_trim_timeout_ms =
>> +get_trim_timeout(_rec->other_config);
>> +
>> +if (strcmp(ovs_cfg->trim_timeout_ms, chassis_trim_timeout_ms)) {
>> +return true;
>> +}
>> +
>>  const char *chassis_mac_mappings =
>>  get_chassis_mac_mappings(_rec->other_config);
>>  if (strcmp(ovs_cfg->chassis_macs, chassis_mac_mappings)) {
>> diff --git a/controller/lflow-cache.c b/controller/lflow-cache.c
>> index 6db85fc12..9c3db06e7 100644
>> --- a/controller/lflow-cache.c
>> +++ b/controller/lflow-cache.c
>> @@ -24,8 +24,10 @@
>>  #include "coverage.h"
>>  #include "lflow-cache.h"
>>  #include "lib/uuid.h"
>> +#include "openvswitch/poll-loop.h"
>>  #include "openvswitch/vlog.h"
>>  #include "ovn/expr.h"
>> +#include "timeval.h"
>>
>>  VLOG_DEFINE_THIS_MODULE(lflow_cache);
>>
>> @@ -59,7 +61,10 @@ struct lflow_cache {
>>  uint64_t max_mem_usage;
>>  uint32_t trim_limit;
>>  uint32_t trim_wmark_perc;
>> +uint32_t trim_timeout_ms;
>>  uint64_t trim_count;
>> +long long int last_active_ms;
>> +bool recently_active;
>>  bool enabled;
>>  };
>>
>> @@ -79,6 +84,7 @@ static struct lflow_cache_value *lflow_cache_add__(
>>  static void lflow_cache_delete__(struct lflow_cache *lc,
>>   struct

Re: [ovs-dev] [PATCH ovn] lflow-cache: Automatically trim cache when it becomes inactive.

2021-11-30 Thread Numan Siddique

On Tue, Nov 23, 2021 at 2:49 PM Dumitru Ceara  wrote:
>
> By default perform memory trimming 30 seconds after the lflow cache
> utilization has dropped.  Otherwise, idle ovn-controller processes might
> wastefully fail to return unused memory to the system.  The timeout is
> configurable through the 'ovn-trim-timeout-ms' external_id.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2024768
> Signed-off-by: Dumitru Ceara 

Thanks for adding this support.  The patch and approach LGTM.

I just have one question.  Please see below.

Thanks
Numan

> ---
>  NEWS|  3 ++
>  controller/chassis.c| 16 
>  controller/lflow-cache.c| 67 -
>  controller/lflow-cache.h|  5 ++-
>  controller/ovn-controller.8.xml |  8 
>  controller/ovn-controller.c |  9 -
>  controller/test-lflow-cache.c   | 13 +--
>  7 files changed, 114 insertions(+), 7 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index e27aaad06..9880a678e 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -12,6 +12,9 @@ Post v21.09.0
>  running on SmartNIC control plane CPUs.  Please refer to
>  Documentation/topics/vif-plug-providers/vif-plug-providers.rst for more
>  information.
> +  - ovn-controller: Add configuration knob, through OVS external-id
> +"ovn-trim-timeout-ms" to allow specifiying an lflow cache inactivity
> +timeout after which ovn-controller should trim memory.
>
>  OVN v21.09.0 - 01 Oct 2021
>  --
> diff --git a/controller/chassis.c b/controller/chassis.c
> index c11c10a34..8a1559653 100644
> --- a/controller/chassis.c
> +++ b/controller/chassis.c
> @@ -54,6 +54,7 @@ struct ovs_chassis_cfg {
>  const char *memlimit_lflow_cache;
>  const char *trim_limit_lflow_cache;
>  const char *trim_wmark_perc_lflow_cache;
> +const char *trim_timeout_ms;
>
>  /* Set of encap types parsed from the 'ovn-encap-type' external-id. */
>  struct sset encap_type_set;
> @@ -163,6 +164,12 @@ get_trim_wmark_perc_lflow_cache(const struct smap 
> *ext_ids)
>  return smap_get_def(ext_ids, "ovn-trim-wmark-perc-lflow-cache", "");
>  }
>
> +static const char *
> +get_trim_timeout(const struct smap *ext_ids)
> +{
> +return smap_get_def(ext_ids, "ovn-trim-timeout-ms", "");
> +}
> +
>  static const char *
>  get_encap_csum(const struct smap *ext_ids)
>  {
> @@ -292,6 +299,7 @@ chassis_parse_ovs_config(const struct 
> ovsrec_open_vswitch_table *ovs_table,
>  get_trim_limit_lflow_cache(>external_ids);
>  ovs_cfg->trim_wmark_perc_lflow_cache =
>  get_trim_wmark_perc_lflow_cache(>external_ids);
> +ovs_cfg->trim_timeout_ms = get_trim_timeout(>external_ids);
>
>  if (!chassis_parse_ovs_encap_type(encap_type, _cfg->encap_type_set)) 
> {
>  return false;
> @@ -336,6 +344,7 @@ chassis_build_other_config(const struct ovs_chassis_cfg 
> *ovs_cfg,
>   ovs_cfg->trim_limit_lflow_cache);
>  smap_replace(config, "ovn-trim-wmark-perc-lflow-cache",
>   ovs_cfg->trim_wmark_perc_lflow_cache);
> +smap_replace(config, "ovn-trim-timeout-ms", ovs_cfg->trim_timeout_ms);
>  smap_replace(config, "iface-types", ds_cstr_ro(_cfg->iface_types));
>  smap_replace(config, "ovn-chassis-mac-mappings", ovs_cfg->chassis_macs);
>  smap_replace(config, "is-interconn",
> @@ -415,6 +424,13 @@ chassis_other_config_changed(const struct 
> ovs_chassis_cfg *ovs_cfg,
>  return true;
>  }
>
> +const char *chassis_trim_timeout_ms =
> +get_trim_timeout(_rec->other_config);
> +
> +if (strcmp(ovs_cfg->trim_timeout_ms, chassis_trim_timeout_ms)) {
> +return true;
> +}
> +
>  const char *chassis_mac_mappings =
>  get_chassis_mac_mappings(_rec->other_config);
>  if (strcmp(ovs_cfg->chassis_macs, chassis_mac_mappings)) {
> diff --git a/controller/lflow-cache.c b/controller/lflow-cache.c
> index 6db85fc12..9c3db06e7 100644
> --- a/controller/lflow-cache.c
> +++ b/controller/lflow-cache.c
> @@ -24,8 +24,10 @@
>  #include "coverage.h"
>  #include "lflow-cache.h"
>  #include "lib/uuid.h"
> +#include "openvswitch/poll-loop.h"
>  #include "openvswitch/vlog.h"
>  #include "ovn/expr.h"
> +#include "timeval.h"
>
>  VLOG_DEFINE_THIS_MODULE(lflow_cache);
>
> @@ -59,7 +61,10 @@ struct lflow_cache {
>  uint64_t max_mem_usage;
>  uint32_t trim_limit;
>  uint32_t trim_wmark_perc;
> +uint32_t trim_timeout_ms;
>  uint64_t trim_count;
> +long long int last_active_ms;
> +bool recently_active;
>  bool enabled;
>  };
>
> @@ -79,6 +84,7 @@ static struct lflow_cache_value *lflow_cache_add__(
>  static void lflow_cache_delete__(struct lflow_cache *lc,
>   struct lflow_cache_entry *lce);
>  static void lflow_cache_trim__(struct lflow_cache *lc, bool force);
> +static void lflow_cache_record_activity__(struct lflow_cache *lc);
>
>  struct lflow_cache *
>

Re: [ovs-dev] [PATCH v3] dpif-netdev: Call cpuid for x86 isa availability.

2021-11-30 Thread Van Haaren, Harry

General comments;
Thanks for reworking, indeed removing dependencies for CPU ISA checking is a 
good
improvement, and allows CPU ISA optimized implementations to run more often.

> -Original Message-
> From: David Marchand 
> Sent: Tuesday, November 30, 2021 2:51 PM
> To: d...@openvswitch.org
> Cc: i.maxim...@ovn.org; Stokes, Ian ; Amber, Kumar
> ; Van Haaren, Harry 
> Subject: [PATCH v3] dpif-netdev: Call cpuid for x86 isa availability.
> 
> DPIF AVX512 optimisations currently rely on DPDK availability while
> they can be used without DPDK.
> Besides, checking for availability of some isa only has to be done once
> and won't change while a OVS process runs.
> 
> Resolve isa availability in constructors by using a simplified query
> based on cpuid API that comes from the compiler.

Using constructors instead of an init() time call is interesting, but may not 
be what we
always want. For "vswitchd" it is a useful startup feature, however other 
binaries/tools
such as "ovs-vsctl" or "ovs-appctl" do not require CPUID-based ISA detection at 
all.
As per this patch, every launch of "ovs-vsctl" (or other tooling/binaries) will 
cause the
constructors to run.

I would like to add some VLOG_* info/logging to the CPU ISA detection, it may 
be useful
to understand the system if in future debug of CPU ISA implementations is 
required.
(This is how the constructor-running was identified, lots of printf() at 
tooling startup!)

> Note: this also fixes the check on BMI2 availability: DPDK had a bug
> for this isa, see https://git.dpdk.org/dpdk/commit/?id=aae3037ab1e0.

Yes, this has been resolved in DPDK (thanks to your linked patch), and will be 
consumed
into the next OVS 2.17 release.

> Suggested-by: Ilya Maximets 
> Signed-off-by: David Marchand 

Overall having OVS use its own CPUID checks is fine with me, I think some 
improvements
are possible with regards to debug/logging, and potentially revisit the 
constructor-function
vs cpu_isa_init() call, suggested improvements to implementation inline below.

Regards, -Harry


> Changes since v2:
> - fixed compilation with AVX512,
> 
> Changes since v1:
> - fixed minor checkpatch issue,
> 
> ---
>  lib/automake.mk|  2 +
>  lib/cpu.c  | 68 ++
>  lib/cpu.h  | 34 +
>  lib/dpdk-stub.c|  9 
>  lib/dpdk.c | 52 
>  lib/dpdk.h |  1 -
>  lib/dpif-netdev-avx512.c   |  5 +-
>  lib/dpif-netdev-extract-avx512.c   | 14 +++---
>  lib/dpif-netdev-lookup-avx512-gather.c |  7 +--
>  9 files changed, 118 insertions(+), 74 deletions(-)
>  create mode 100644 lib/cpu.c
>  create mode 100644 lib/cpu.h
> 
> diff --git a/lib/automake.mk b/lib/automake.mk
> index 46f869a336..5224e08564 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -38,6 +38,8 @@ lib_libopenvswitchavx512_la_CFLAGS = \
>   -fPIC \
>   $(AM_CFLAGS)
>  lib_libopenvswitchavx512_la_SOURCES = \
> + lib/cpu.c \
> + lib/cpu.h \
>   lib/dpif-netdev-lookup-avx512-gather.c \
>   lib/dpif-netdev-extract-avx512.c \
>   lib/dpif-netdev-avx512.c
> diff --git a/lib/cpu.c b/lib/cpu.c
> new file mode 100644
> index 00..ea1934d3ca
> --- /dev/null
> +++ b/lib/cpu.c
> @@ -0,0 +1,68 @@
> +/*
> + * Copyright (c) 2021, Red Hat, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include 
> +
> +#include "cpu.h"
> +#include "openvswitch/compiler.h"
> +
> +#ifdef __x86_64__
> +#include 
> +#include 
> +
> +enum x86_reg {
> +EAX,
> +EBX,
> +ECX,
> +EDX,
> +};
> +#define X86_LEAF_MASK 0x8000
> +#define X86_EXT_FEATURES_LEAF 0x0007
> +static bool x86_has_isa(uint32_t leaf, enum x86_reg reg, uint32_t bit)
> +{
> +uint32_t maxleaf = __get_cpuid_max(leaf & X86_LEAF_MASK, NULL);
> +uint32_t regs[4];
> +
> +if (maxleaf < leaf) {
> +return false;

This is a programming error, not a runtime error correct? We're asking for a
leaf that has not been supported in OVS. Presumably the programmer intended
to ask for a feature that OVS has support for. So a unique/identifiable error 
return
would be better than "false" which is currently ambiguous? (Currently the return
value "false" means both "not supported" and "programming error")

> +}
> +__cpuid_count(leaf, 0, regs[EAX],

[ovs-dev] [PATCH v4] checkpatch: Detect "trojan source" attack

2021-11-30 Thread Mike Pattrick

Recently there has been a lot of press about the "trojan source" attack,
where Unicode characters are used to obfuscate the true functionality of
code. This attack didn't effect OVS, but adding the check here will help
guard against it sneaking in later.

Signed-off-by: Mike Pattrick 
---
Changes in v2:
   - Now all unicode characters will result in an error.

Changes in v3:
   - Added a test to validate behavior

Changes in v4:
   - Simplified regex

---
 tests/checkpatch.at | 22 ++
 utilities/checkpatch.py | 11 +++
 2 files changed, 33 insertions(+)

diff --git a/tests/checkpatch.at b/tests/checkpatch.at
index 68c917af9..fadb625e9 100755
--- a/tests/checkpatch.at
+++ b/tests/checkpatch.at
@@ -424,3 +424,25 @@ try_checkpatch \
 "
 
 AT_CLEANUP
+
+AT_SETUP([checkpatch - Unicode code])
+try_checkpatch \
+   "COMMON_PATCH_HEADER
++ if (snowman == ☃️) {  /* Emoji
++ void НelloWorld() {  /* Homoglyph
++ ة /* ;C++/* BiDi
+" \
+"ERROR: Inappropriate non-ascii characters detected.
+#8 FILE: A.c:1:
+ if (snowman == ☃️) {  /* Emoji
+
+ERROR: Inappropriate non-ascii characters detected.
+#9 FILE: A.c:2:
+ void НelloWorld() {  /* Homoglyph
+
+ERROR: Inappropriate non-ascii characters detected.
+#10 FILE: A.c:3:
+ ة /* ;C++/* BiDi
+"
+
+AT_CLEANUP
diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index bf95358d5..84d5ded8c 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -181,6 +181,7 @@ __regex_added_doc_rst = re.compile(
 __regex_empty_return = re.compile(r'\s*return;')
 __regex_if_macros = re.compile(r'^ +(%s) \([\S]([\s\S]+[\S])*\) { +\\' %
__parenthesized_constructs)
+__regex_nonascii_characters = re.compile("[^\u-\u007f]")
 
 skip_leading_whitespace_check = False
 skip_trailing_whitespace_check = False
@@ -294,6 +295,11 @@ def pointer_whitespace_check(line):
 return __regex_ptr_declaration_missing_whitespace.search(line) is not None
 
 
+def nonascii_character_check(line):
+"""Return TRUE if inappropriate Unicode characters are detected """
+return __regex_nonascii_characters.search(line) is not None
+
+
 def cast_whitespace_check(line):
 """Return TRUE if there is no space between the '()' used in a cast and
the expression whose type is cast, i.e.: '(void *)foo'"""
@@ -565,6 +571,11 @@ checks = [
  'print':
  lambda: print_error("Inappropriate spacing in pointer declaration")},
 
+{'regex': r'(\.c|\.h)(\.in)?$', 'match_name': None,
+ 'check': lambda x: nonascii_character_check(x),
+ 'print':
+ lambda: print_error("Inappropriate non-ascii characters detected.")},
+
 {'regex': r'(\.c|\.h)(\.in)?$', 'match_name': None,
  'prereq': lambda x: not is_comment_line(x),
  'check': lambda x: cast_whitespace_check(x),
-- 
2.27.0

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

[ovs-dev] [PATCH v2] checkpatch: Correct line count in error messages

2021-11-30 Thread Mike Pattrick

As part of some previous checkpatch work, we discovered that checkpatch
isn't always reporting correct line numbers. As it turns out, Python's
splitlines function considers several characters to be new lines which
common text editors do not typically consider to be new lines. For
example, form feed characters, which this code base uses to cluster
functionality.

Signed-off-by: Mike Pattrick 
---
 utilities/checkpatch.py | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index bf95358d5..caf10537b 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -765,12 +765,16 @@ def ovs_checkpatch_parse(text, filename, author=None, 
committer=None):
 
 reset_counters()
 
-for line in text.splitlines():
+for line in text.split("\n"):
 if current_file != previous_file:
 previous_file = current_file
 
 lineno = lineno + 1
 total_line = total_line + 1
+
+if line == "\f":
+# Form feed
+continue
 if len(line) <= 0:
 continue
 
-- 
2.27.0

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

[ovs-dev] [RFC PATCH 1/1] dpdk: Update to use DPDK v21.11.

2021-11-30 Thread Ian Stokes

This commit adds support for DPDK v21.11, it includes the following
changes.

1. ci: Install python elftools for DPDK 21.02.
2. ci: Update meson requirement for DPDK 21.05.
3. netdev-dpdk: Fix build with 21.05.
4. ci: Compile DPDK in non developer mode.

   http://patchwork.ozlabs.org/project/openvswitch/list/?series=242480=*

5. netdev-dpdk: Remove access to DPDK internals.
6. netdev-dpdk: Remove unused attribute from rte_flow rule.
7. netdev-dpdk: Fix mbuf macros namespace with 21.11-rc1.
8. netdev-dpdk: Fix vhost namespace with 21.11-rc2.

   http://patchwork.ozlabs.org/project/openvswitch/list/?series=271040=*

For credit all authors of the original commits to 'dpdk-latest' with the above
changes have been added as co-authors for this commit.

Signed-off-by: David Marchand 
Co-authored-by: David Marchand 
Signed-off-by: Ian Stokes 
---
 .ci/linux-build.sh   |   6 +-
 .ci/linux-prepare.sh |   4 +-
 Documentation/faq/releases.rst   |   2 +-
 Documentation/intro/install/dpdk.rst |  16 ++---
 Documentation/topics/dpdk/phy.rst|   8 +--
 Documentation/topics/dpdk/vdev.rst   |   2 +-
 Documentation/topics/dpdk/vhost-user.rst |   2 +-
 Documentation/topics/testing.rst |   2 +-
 NEWS |   1 +
 lib/dp-packet.h  |  26 +++
 lib/netdev-dpdk.c| 115 ---
 11 files changed, 98 insertions(+), 86 deletions(-)

diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index 863f02388..ff6ae4b10 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -159,6 +159,10 @@ function install_dpdk()
 # Disable building DPDK unit tests. Not needed for OVS build or tests.
 DPDK_OPTS="$DPDK_OPTS -Dtests=false"
 
+# Disable DPDK developer mode, this results in less build checks and less
+# meson verbose outputs.
+DPDK_OPTS="$DPDK_OPTS -Ddeveloper_mode=disabled"
+
 # Install DPDK using prefix.
 DPDK_OPTS="$DPDK_OPTS --prefix=$(pwd)/build"
 
@@ -216,7 +220,7 @@ fi
 
 if [ "$DPDK" ] || [ "$DPDK_SHARED" ]; then
 if [ -z "$DPDK_VER" ]; then
-DPDK_VER="20.11.1"
+DPDK_VER="21.11"
 fi
 install_dpdk $DPDK_VER
 if [ "$CC" = "clang" ]; then
diff --git a/.ci/linux-prepare.sh b/.ci/linux-prepare.sh
index 360c0a68e..b3addf404 100755
--- a/.ci/linux-prepare.sh
+++ b/.ci/linux-prepare.sh
@@ -21,8 +21,8 @@ make -j4 HAVE_LLVM= HAVE_SQLITE= install
 cd ..
 
 pip3 install --disable-pip-version-check --user \
-flake8 hacking sphinx wheel setuptools
-pip3 install --user  'meson==0.47.1'
+flake8 hacking sphinx wheel setuptools pyelftools
+pip3 install --user  'meson==0.49.2'
 
 if [ "$M32" ]; then
 # Installing 32-bit libraries.
diff --git a/Documentation/faq/releases.rst b/Documentation/faq/releases.rst
index 64bc577e0..59d55202d 100644
--- a/Documentation/faq/releases.rst
+++ b/Documentation/faq/releases.rst
@@ -225,7 +225,7 @@ Q: Are all the DPDK releases that OVS versions work with 
maintained?
 The latest information about DPDK stable and LTS releases can be found
 at `DPDK stable`_.
 
-.. _DPDK stable: http://doc.dpdk.org/guides-20.11/contributing/stable.html
+.. _DPDK stable: http://doc.dpdk.org/guides-21.11/contributing/stable.html
 
 Q: I get an error like this when I configure Open vSwitch:
 
diff --git a/Documentation/intro/install/dpdk.rst 
b/Documentation/intro/install/dpdk.rst
index d554409fc..d9f44055d 100644
--- a/Documentation/intro/install/dpdk.rst
+++ b/Documentation/intro/install/dpdk.rst
@@ -42,7 +42,7 @@ Build requirements
 In addition to the requirements described in :doc:`general`, building Open
 vSwitch with DPDK will require the following:
 
-- DPDK 20.11.1
+- DPDK 21.11
 
 - A `DPDK supported NIC`_
 
@@ -59,8 +59,8 @@ vSwitch with DPDK will require the following:
 
 Detailed system requirements can be found at `DPDK requirements`_.
 
-.. _DPDK supported NIC: https://doc.dpdk.org/guides-20.11/nics/index.html
-.. _DPDK requirements: 
https://doc.dpdk.org/guides-20.11/linux_gsg/sys_reqs.html
+.. _DPDK supported NIC: https://doc.dpdk.org/guides-21.11/nics/index.html
+.. _DPDK requirements: 
https://doc.dpdk.org/guides-21.11/linux_gsg/sys_reqs.html
 
 .. _dpdk-install:
 
@@ -73,9 +73,9 @@ Install DPDK
 #. Download the `DPDK sources`_, extract the file and set ``DPDK_DIR``::
 
$ cd /usr/src/
-   $ wget https://fast.dpdk.org/rel/dpdk-20.11.1.tar.xz
-   $ tar xf dpdk-20.11.1.tar.xz
-   $ export DPDK_DIR=/usr/src/dpdk-stable-20.11.1
+   $ wget https://fast.dpdk.org/rel/dpdk-21.11.tar.xz
+   $ tar xf dpdk-21.11.tar.xz
+   $ export DPDK_DIR=/usr/src/dpdk-21.11
$ cd $DPDK_DIR
 
 #. Configure and install DPDK using Meson
@@ -121,7 +121,7 @@ Install DPDK
 
 .. _DPDK sources: http://dpdk.org/rel
 .. _DPDK documentation:
-   https://doc.dpdk.org/guides-20.11/linux_gsg/build_dpdk.html
+

Re: [ovs-dev] [PATCH] ovsdb-idl: Don't reparse orphaned rows.

2021-11-30 Thread Ilya Maximets

On 11/26/21 13:26, Dumitru Ceara wrote:
> Rows that refer to rows that were inserted in the current IDL run should
> only be reparsed if they don't get deleted (become orphan) in the current
> IDL run.
> 
> Fixes: 7b8aeadd60c8 ("ovsdb-idl: Re-parse backrefs of inserted rows only 
> once.")
> Reported-by: Ilya Maximets 
> Signed-off-by: Dumitru Ceara 
> ---
>  lib/ovsdb-idl.c|  7 ++-
>  tests/ovsdb-idl.at | 26 ++
>  2 files changed, 32 insertions(+), 1 deletion(-)

This one applied.  Thanks!

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

Re: [ovs-dev] [PATCH] ovsdb-idl: Get per-database memory usage statistics.

2021-11-30 Thread Ilya Maximets

On 11/30/21 16:04, Ilya Maximets wrote:
> On 11/30/21 10:47, Dumitru Ceara wrote:
>> Clients might be connected to multiple databases (e.g., ovn-controller
>> is connected to OVN_Southbound and Open_vSwitch databases) and the IDL
>> memory statistics are more useful if they're not aggregated.
>>
>> Signed-off-by: Dumitru Ceara 
>> ---
>>  lib/ovsdb-idl.c | 17 ++---
>>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> Applied.  Thanks!

Oops.  Sent to the wrong patch. :)
I didn't actually look at this one yet.  Sorry.

> 
> Best regards, Ilya Maximets.
> 

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

Re: [ovs-dev] [PATCH ovn v4] ovn-controller: Avoid infinite replying for TCP/ICMP connection reset messages

2021-11-30 Thread Numan Siddique

On Tue, Nov 30, 2021 at 4:23 AM Mohammad Heib  wrote:
>
> When the ovn controller receives an ip packet that targets a lport that has 
> ACL
> rule to reject ip packets, the controller will reply with TCP_RST or icmp4/6 
> unreachable packet
> to notify the sender that the destination is not available.
>
> In turn, the receiver host will receive the notification packet and handle it 
> as a normal IP packet
> and if the receiver host is part of the same logical-switch/port-group or has 
> IP reject ACL rule
> it will send TCP_RST or icmp4/6 unreachable packet replying to the TCP_RST or 
> icmp4/6 unreachable
> packet we received and here we will enter to an infinity loop of replying 
> about replying which
> will consume high CPU.
>
> To avoid such scenarios this patch proposes to drop/ignore TCP_RST or icmp4/6 
> unreachable packets
> that received on lport that has  IP reject ACL rules.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1934011
> Fixes: 64f8c9e9f ("actions: Add a new OVN action - reject {}.")
> Signed-off-by: Mohammad Heib 

Thanks.  I applied this patch to the main branch and I'm running some
tests to backport to other branches.

Numan

> ---
>  controller/pinctrl.c |  29 +++
>  tests/ovn.at | 113 +++
>  2 files changed, 142 insertions(+)
>
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index ae5320e09..0d443c150 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -1933,11 +1933,40 @@ pinctrl_handle_sctp_abort(struct rconn *swconn, const 
> struct flow *ip_flow,
>  dp_packet_uninit();
>  }
>
> +static bool
> +pinctrl_handle_reject_ignore_pkt(const struct flow *ip_flow,
> + struct dp_packet *pkt_in)
> +{
> +if (ip_flow->nw_proto == IPPROTO_TCP) {
> +struct tcp_header *th = dp_packet_l4(pkt_in);
> +if (!th || (TCP_FLAGS(th->tcp_ctl) & TCP_RST)) {
> +return true;
> +}
> +} else {
> +if (is_icmpv4(ip_flow, NULL)) {
> +struct icmp_header *ih = dp_packet_l4(pkt_in);
> +if (!ih || (ih->icmp_type == ICMP4_DST_UNREACH)) {
> +return true;
> +}
> +} else if (is_icmpv6(ip_flow, NULL)) {
> +struct icmp6_data_header *ih = dp_packet_l4(pkt_in);
> +if (!ih || (ih->icmp6_base.icmp6_type == ICMP6_DST_UNREACH)) {
> +return true;
> +}
> +}
> +}
> +return false;
> +}
> +
>  static void
>  pinctrl_handle_reject(struct rconn *swconn, const struct flow *ip_flow,
>struct dp_packet *pkt_in,
>const struct match *md, struct ofpbuf *userdata)
>  {
> +if (pinctrl_handle_reject_ignore_pkt(ip_flow, pkt_in)) {
> +return;
> +}
> +
>  if (ip_flow->nw_proto == IPPROTO_TCP) {
>  pinctrl_handle_tcp_reset(swconn, ip_flow, pkt_in, md, userdata, 
> true);
>  } else if (ip_flow->nw_proto == IPPROTO_SCTP) {
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 7a7ae0da6..a4ed03041 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -14223,6 +14223,119 @@ OVN_CLEANUP([hv1], [hv2], [hv3])
>  AT_CLEANUP
>  ])
>
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ACL Reject ping pong])
> +AT_KEYWORDS([ACL Reject ping pong])
> +ovn_start
> +
> +send_icmp6_packet() {
> +local inport=$1 hv=$2 eth_src=$3 eth_dst=$4 ipv6_src=$5 ipv6_dst=$6
> +
> +local ip6_hdr=60083aff${ipv6_src}${ipv6_dst}
> +local packet=${eth_dst}${eth_src}86dd${ip6_hdr}8000dcb662f1
> +
> +as hv$hv ovs-appctl netdev-dummy/receive hv${hv}-vif$inport $packet
> +}
> +
> +send_icmp_packet() {
> +local inport=$1 hv=$2 eth_src=$3 eth_dst=$4 ipv4_src=$5 ipv4_dst=$6 
> ip_chksum=$7 data=$8
> +shift 8
> +
> +local ip_ttl=ff
> +local ip_len=001c
> +local 
> packet=${eth_dst}${eth_src}08004500${ip_len}4000${ip_ttl}01${ip_chksum}${ipv4_src}${ipv4_dst}${data}
> +as hv$hv ovs-appctl netdev-dummy/receive hv${hv}-vif$inport $packet
> +}
> +
> +net_add n1
> +
> +sim_add hv1
> +as hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +ovs-vsctl -- add-port br-int hv1-vif1 -- \
> +set interface hv1-vif1 external-ids:iface-id=sw0-p1 \
> +options:tx_pcap=hv1/vif1-tx.pcap \
> +options:rxq_pcap=hv1/vif1-rx.pcap \
> +ofport-request=1
> +
> +sim_add hv2
> +as hv2
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.2
> +ovs-vsctl -- add-port br-int hv2-vif1 -- \
> +set interface hv2-vif1 external-ids:iface-id=sw0-p2 \
> +options:tx_pcap=hv2/vif1-tx.pcap \
> +options:rxq_pcap=hv2/vif1-rx.pcap \
> +ofport-request=1
> +
> +ovn-nbctl ls-add sw0
> +
> +check ovn-nbctl lsp-add sw0 sw0-p1
> +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3 1000::3"
> +check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3 
> 1000::3"
> +
> +check ovn-nbctl lsp-add sw0 sw0-p2
> +check ovn-nbctl

Re: [ovs-dev] [PATCH] stream-ssl: Avoid unnecessary memory copies on send.

2021-11-30 Thread Ilya Maximets

On 11/29/21 13:12, Dumitru Ceara wrote:
> On 11/22/21 00:46, Ilya Maximets wrote:
>> ssl_send() clones the data before sending, but if SSL_write() succeeds
>> at the first attempt, this is only a waste of CPU cycles.
>>
>> Trying to send the original buffer instead and only copying remaining
>> data if it's not possible to send it all right away.
>>
>> This should save a few cycles on every send.
>>
>> Note:
>> It's probably possible to avoid the copy even if we can't send
>> everything at once, but will, likely, require some major change
>> of the stream-sll module in order to take into account all the
>> corner cases related to SSL connection.  So, not trying to do that
>> for now.
>>
>> Signed-off-by: Ilya Maximets 
>> ---
> 
> Looks good to me; this passes OVS and OVN unit tests (when
> https://patchwork.ozlabs.org/project/openvswitch/list/?series=273948=*
> is also applied).
> 
> Acked-by: Dumitru Ceara 

Applied.  Thanks!

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

Re: [ovs-dev] [PATCH 0/2] ovsdb-data: Consolidate ovsdb atom and json strings.

2021-11-30 Thread Ilya Maximets

On 11/24/21 21:39, Mike Pattrick wrote:
> On Mon, 2021-11-22 at 01:09 +0100, Ilya Maximets wrote:
>> This patch-set is a continuation of work for deduplication of
>> ovsdb strings.  Previous patch (accepted):
>>   
>> https://patchwork.ozlabs.org/project/openvswitch/patch/20210923001353.4072533-1-i.maxim...@ovn.org/
>>
>> Ilya Maximets (2):
>>   json: Inline clone and destroy functions.
>>   ovsdb-data: Consolidate ovsdb atom and json strings.
>>
>>  include/openvswitch/json.h | 26 +--
>>  lib/json.c | 53 +++-
>> --
>>  lib/ovsdb-data.c   | 27 ---
>>  lib/ovsdb-data.h   | 25 ++
>>  ovsdb/ovsdb-idlc.in|  5 ++--
>>  ovsdb/ovsdb-server.c   |  7 ++---
>>  ovsdb/rbac.c   |  8 +++---
>>  python/ovs/db/data.py  | 10 ---
>>  tests/test-ovsdb.c |  9 ---
>>  9 files changed, 86 insertions(+), 84 deletions(-)
>>
> 
> Looks good to me!
> 
> Acked-by: Mike Pattrick  
> 

Thanks, Mike and Dumitru!  Applied.

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

Re: [ovs-dev] [PATCH 2/2] ovsdb-data: Consolidate ovsdb atom and json strings.

2021-11-30 Thread Ilya Maximets

On 11/29/21 16:47, Dumitru Ceara wrote:
> On 11/22/21 01:09, Ilya Maximets wrote:
>> ovsdb_atom_string and json_string are basically the same data structure
>> and ovsdb-server frequently needs to convert one to another.  We can
>> avoid that by using json_string from the beginning for all ovsdb
>> strings.  So, the conversion turns into simple json_clone(), i.e.
>> increment of a reference counter.  This change doesn't give any
>> significant performance boost, but it improves the code clarity and
>> may be useful for future development.
>>
>> Signed-off-by: Ilya Maximets 
>> ---
> 
> I'm assuming this also doesn't cause a visible performance hit because
> of extra, non-inlined, calls to json_string(), right?

Actually, I run a few more tests and this patch actually gives about 5-6%
performance boost to ovsdb-server in some cases.  I'll adjust the commit
message a bit before applying.

> Then:
> 
> Acked-by: Dumitru Ceara 

Thanks!

> 
> Thanks!
> 

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

Re: [ovs-dev] [PATCH] ovsdb-idl: Get per-database memory usage statistics.

2021-11-30 Thread Ilya Maximets

On 11/30/21 10:47, Dumitru Ceara wrote:
> Clients might be connected to multiple databases (e.g., ovn-controller
> is connected to OVN_Southbound and Open_vSwitch databases) and the IDL
> memory statistics are more useful if they're not aggregated.
> 
> Signed-off-by: Dumitru Ceara 
> ---
>  lib/ovsdb-idl.c | 17 ++---
>  1 file changed, 14 insertions(+), 3 deletions(-)

Applied.  Thanks!

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

[ovs-dev] [PATCH v3 3/4] system-dpdk: Test with mlx5 devices.

2021-11-30 Thread David Marchand

The DPDK unit test only runs if vfio or igb_uio kernel module is loaded:
on systems with only mlx5, this test is always skipped.

Besides, the test tries to grab the first device listed by dpdk-devbind.py,
regardless of the PCI device status regarding kmod binding.

Remove dependency on this DPDK script and use a minimal script that
reads PCI sysfs.

This script is not perfect, as one can imagine PCI devices bound to
vfio-pci for virtual machines.
Add a new environment variable DPDK_PCI_ADDR for testers to select the
PCI device of their liking.
For consistency and grep, the temporary file PCI_ADDR is renamed
to DPDK_PCI_ADDR.

Note: with mlx5 devices, there is now more OVS/DPDK warnings to waive.

Signed-off-by: David Marchand 
---
Changes since v2:
- sorted logs alphabetically,

---
 Documentation/topics/testing.rst |  1 -
 tests/automake.mk|  1 +
 tests/system-dpdk-find-device.py | 44 
 tests/system-dpdk-macros.at  | 10 ++--
 tests/system-dpdk.at |  4 ++-
 5 files changed, 50 insertions(+), 10 deletions(-)
 create mode 100755 tests/system-dpdk-find-device.py

diff --git a/Documentation/topics/testing.rst b/Documentation/topics/testing.rst
index 0ddcbd58ab..00f734a77a 100644
--- a/Documentation/topics/testing.rst
+++ b/Documentation/topics/testing.rst
@@ -343,7 +343,6 @@ To see a list of all the available tests, run::
 
 These tests support a `DPDK supported NIC`_. The tests operate on a wider set 
of
 environments, for instance, when a virtual port is used.
-They do require proper DPDK variables (``DPDK_DIR`` and ``DPDK_BUILD``).
 Moreover you need to have root privileges to load the required modules and to 
bind
 the NIC to the DPDK-compatible driver.
 
diff --git a/tests/automake.mk b/tests/automake.mk
index 3bc74a5aff..e7002634f2 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -184,6 +184,7 @@ SYSTEM_OFFLOADS_TESTSUITE_AT = \
 
 SYSTEM_DPDK_TESTSUITE_AT = \
tests/system-common-macros.at \
+   tests/system-dpdk-find-device.py \
tests/system-dpdk-macros.at \
tests/system-dpdk-testsuite.at \
tests/system-dpdk.at
diff --git a/tests/system-dpdk-find-device.py b/tests/system-dpdk-find-device.py
new file mode 100755
index 00..4253326e75
--- /dev/null
+++ b/tests/system-dpdk-find-device.py
@@ -0,0 +1,44 @@
+#!/usr/bin/env python3
+# Copyright (c) 2021 Red Hat, Inc.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at:
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+
+from pathlib import Path
+import os
+import sys
+
+# The tester might want to select a PCI device, if so, trust it.
+if 'DPDK_PCI_ADDR' in os.environ:
+print(os.environ['DPDK_PCI_ADDR'])
+sys.exit(0)
+
+mlx5_ib_available = Path('/sys/module/mlx5_ib').exists()
+
+for device in sorted(Path('/sys/bus/pci/devices').iterdir()):
+class_path = device / 'class'
+# Only consider Network class devices
+if class_path.read_text().strip() != '0x02':
+continue
+kmod_path = device / 'driver' / 'module'
+kmod_name = kmod_path.resolve().name
+# Devices of interest:
+# - device is bound to vfio_pci or igb_uio,
+# - device is bound to mlx5_core and mlx5 is loaded,
+if (kmod_name not in ['vfio_pci', 'igb_uio'] and
+(kmod_name not in ['mlx5_core'] or not mlx5_ib_available)):
+continue
+print(device.resolve().name)
+sys.exit(0)
+
+sys.exit(1)
diff --git a/tests/system-dpdk-macros.at b/tests/system-dpdk-macros.at
index 5a6b3cbff9..745740f36b 100644
--- a/tests/system-dpdk-macros.at
+++ b/tests/system-dpdk-macros.at
@@ -22,14 +22,8 @@ m4_define([OVS_DPDK_PRE_PHY_SKIP],
   [dnl Perform the precheck
OVS_DPDK_PRE_CHECK()
 
-   dnl Check if VFIO or UIO driver is loaded
-   AT_SKIP_IF([ ! (lsmod | grep -E "igb_uio|vfio") ], [], [stdout])
-
-   dnl Find PCI address candidate, skip if there is no DPDK-compatible NIC
-   AT_CHECK([$DPDK_DIR/usertools/dpdk-devbind.py -s | head -n +4 | tail -1], 
[], [stdout])
-   AT_CHECK([cat stdout | cut -d" " -s -f1 > PCI_ADDR])
-   AT_SKIP_IF([ ! test -s PCI_ADDR ])
-
+   dnl Check if a device is available for DPDK
+   AT_SKIP_IF([ ! $abs_top_srcdir/tests/system-dpdk-find-device.py > 
DPDK_PCI_ADDR])
 ])
 
 
diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
index 73c58030b1..76b60f3c3a 100644
--- a/tests/system-dpdk.at
+++ b/tests/system-dpdk.at
@@ -5,9 +5,11 @@ AT_BANNER([OVS-DPDK unit tests])
 
 m4_define([SYSTEM_DPDK_ALLOWED_LOGS],[
 \@does not exist. The Open vSwitch kernel

[ovs-dev] [PATCH v3 4/4] tests: Move MFEX tests to dpif-netdev.

2021-11-30 Thread David Marchand

The MFEX code and tests do not depend on DPDK anymore.
We can move the unit tests to dpif-netdev.

Signed-off-by: David Marchand 
---
Note: this patch depends on series
https://patchwork.ozlabs.org/project/openvswitch/list/?series=274452

---
 Documentation/topics/dpdk/bridge.rst |  22 +--
 tests/dpif-netdev.at | 167 +++
 tests/system-dpdk.at | 194 ---
 3 files changed, 178 insertions(+), 205 deletions(-)

diff --git a/Documentation/topics/dpdk/bridge.rst 
b/Documentation/topics/dpdk/bridge.rst
index 648ce203eb..c88658fa91 100644
--- a/Documentation/topics/dpdk/bridge.rst
+++ b/Documentation/topics/dpdk/bridge.rst
@@ -385,21 +385,21 @@ Unit Test Miniflow Extract
 ++
 
 Unit test can also be used to test the workflow mentioned above by running
-the following test-case in tests/system-dpdk.at ::
+the following test-case in tests/dpif-netdev.at ::
 
-make check-dpdk TESTSUITEFLAGS='-k MFEX'
-OVS-DPDK - MFEX Autovalidator
+make check TESTSUITEFLAGS='-k MFEX'
+dpif-netdev - MFEX Autovalidator
 
-The unit test uses mulitple traffic type to test the correctness of the
-implementaions.
+The unit test uses multiple traffic type to test the correctness of the
+implementations.
 
 The MFEX commands can also be tested for negative and positive cases to
 verify that the MFEX set command does not allow for incorrect parameters.
 A user can directly run the following configuration test case in
-tests/system-dpdk.at ::
+tests/dpif-netdev.at ::
 
-make check-dpdk TESTSUITEFLAGS='-k MFEX'
-OVS-DPDK - MFEX Configuration
+make check TESTSUITEFLAGS='-k MFEX'
+dpif-netdev - MFEX Configuration
 
 Running Fuzzy test with Autovalidator
 +
@@ -431,7 +431,7 @@ Unit Fuzzy test with Autovalidator
 +
 
 Unit test can also be used to test the workflow mentioned above by running
-the following test-case in tests/system-dpdk.at ::
+the following test-case in tests/dpif-netdev.at ::
 
-make check-dpdk TESTSUITEFLAGS='-k MFEX'
-OVS-DPDK - MFEX Autovalidator Fuzzy
+make check TESTSUITEFLAGS='-k MFEX'
+dpif-netdev - MFEX Autovalidator Fuzzy
diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
index 53eee185ad..fbb8fe9a71 100644
--- a/tests/dpif-netdev.at
+++ b/tests/dpif-netdev.at
@@ -635,3 +635,170 @@ OVS_WAIT_UNTIL([grep "flow: in_port is not an exact 
match" ovs-vswitchd.log])
 OVS_VSWITCHD_STOP(["/flow: in_port is not an exact match/d
 /failed to put/d"])
 AT_CLEANUP
+
+AT_SETUP([dpif-netdev - MFEX Autovalidator])
+AT_SKIP_IF([! $PYTHON3 -c "import scapy"], [], [])
+OVS_VSWITCHD_START(
+  [add-port br0 p1 \
+   -- set interface p1 type=dummy-pmd])
+
+AT_SKIP_IF([! ovs-appctl dpif-netdev/miniflow-parser-get | sed 1,4d | grep 
"True"], [], [dnl
+])
+
+on_exit "pkill -f -x -9 '$PYTHON3 $srcdir/genpkts.py -1'"
+($PYTHON3 $srcdir/genpkts.py -1 | while read pkt; do
+ ovs-appctl netdev-dummy/receive p1 "$pkt" || break
+ done) &
+
+AT_CHECK([ovs-appctl dpif-netdev/miniflow-parser-set autovalidator], [0], [dnl
+Miniflow extract implementation set to autovalidator.
+])
+
+OVS_WAIT_UNTIL([test `ovs-vsctl get interface p1 statistics:rx_packets` -ge 
1000])
+pkill -f -x -9 '$PYTHON3 $srcdir/genpkts.py -1'
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([dpif-netdev - MFEX Autovalidator Fuzzy])
+AT_SKIP_IF([! $PYTHON3 -c "import scapy"], [], [])
+OVS_VSWITCHD_START(
+  [add-port br0 p1 \
+   -- set interface p1 type=dummy-pmd])
+
+AT_SKIP_IF([! ovs-appctl dpif-netdev/miniflow-parser-get | sed 1,4d | grep 
"True"], [], [dnl
+])
+
+on_exit "pkill -f -x -9 '$PYTHON3 $srcdir/genpkts.py -1 fuzz'"
+($PYTHON3 $srcdir/genpkts.py -1 fuzz | while read pkt; do
+ ovs-appctl netdev-dummy/receive p1 "$pkt" || break
+ done) &
+
+AT_CHECK([ovs-appctl dpif-netdev/miniflow-parser-set autovalidator], [0], [dnl
+Miniflow extract implementation set to autovalidator.
+])
+
+OVS_WAIT_UNTIL([test `ovs-vsctl get interface p1 statistics:rx_packets` -ge 
1000])
+pkill -f -x -9 '$PYTHON3 $srcdir/genpkts.py -1 fuzz'
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([dpif-netdev - MFEX Configuration])
+AT_SKIP_IF([! $PYTHON3 -c "import scapy"], [], [])
+OVS_VSWITCHD_START(
+  [add-port br0 p1 \
+   -- set Open_vSwitch . other_config:pmd-cpu-mask=0xC \
+   -- set interface p1 type=dummy-pmd])
+
+on_exit "pkill -f -x -9 '$PYTHON3 $srcdir/genpkts.py -1'"
+($PYTHON3 $srcdir/genpkts.py -1 | while read pkt; do
+ ovs-appctl netdev-dummy/receive p1 "$pkt" || break
+ done) &
+
+AT_CHECK([ovs-appctl dpif-netdev/miniflow-parser-set scalar 1], [2],
+[], [dnl
+Error: unknown argument 1.
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+
+AT_CHECK([ovs-appctl dpif-netdev/miniflow-parser-set -pmd 6 study 300 xyz], 
[2],
+[], [dnl
+Error: invalid study_pkt_cnt value: xyz.
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+
+AT_CHECK([ovs-appctl

[ovs-dev] [PATCH v3 2/4] system-dpdk: Use dummy-pmd port for packet injection.

2021-11-30 Thread David Marchand

net_pcap is not always available in DPDK (like, in a dev
environment when you forgot to install the libpcap-devel).
On the other hand, OVS already has its own way to inject packets into a
bridge. Let's make use of it.

This solution is slower than net/pcap DPDK, so lower the number of
expected packets so that it runs in OVS_CTL_TIMEOUT seconds.

While at it, convert "known" packets from pcap to scapy so that the
injected packets can be updated without having to read/write a pcap file.

Note: this change also (avoids/) fixes a python exception in PcapWriter
with scapy 2.4.3 that comes from EPEL.

Suggested-by: Ilya Maximets 
Signed-off-by: David Marchand 
---
Changes since v2:
- updated documentation,
- cleaned tests/automake.mk,
- fixed shebang in python script,
- added missing check for scapy availability,

Changes since v1:
- renamed generator script,
- decreased packet count for fuzzy test,
- simplified wait expression for packet count,

---
 Documentation/topics/dpdk/bridge.rst |   7 ++--
 tests/automake.mk|   7 +---
 tests/genpkts.py |  56 +++
 tests/mfex_fuzzy.py  |  32 ---
 tests/pcap/mfex_test.pcap| Bin 416 -> 0 bytes
 tests/system-dpdk-macros.at  |   4 +-
 tests/system-dpdk.at |  33 +---
 7 files changed, 89 insertions(+), 50 deletions(-)
 create mode 100755 tests/genpkts.py
 delete mode 100755 tests/mfex_fuzzy.py
 delete mode 100644 tests/pcap/mfex_test.pcap

diff --git a/Documentation/topics/dpdk/bridge.rst 
b/Documentation/topics/dpdk/bridge.rst
index f645b9ade5..648ce203eb 100644
--- a/Documentation/topics/dpdk/bridge.rst
+++ b/Documentation/topics/dpdk/bridge.rst
@@ -408,7 +408,7 @@ Fuzzy tests can also be done on miniflow extract with the 
help of
 auto-validator and Scapy. The steps below describes the steps to
 reproduce the setup with IP being fuzzed to generate packets.
 
-Scapy is used to create fuzzy IP packets and save them into a PCAP ::
+Scapy is used to create fuzzy IP packets (see tests/genpkts.py) ::
 
 pkt = fuzz(Ether()/IP()/TCP())
 
@@ -418,9 +418,8 @@ Set the miniflow extract to autovalidator using ::
 
 OVS is configured to receive the generated packets ::
 
-$ ovs-vsctl add-port br0 pcap0 -- \
-set Interface pcap0 type=dpdk options:dpdk-devargs=net_pcap0
-"rx_pcap=fuzzy.pcap"
+$ ovs-vsctl add-port br0 p1 -- \
+set Interface p1 type=dummy-pmd
 
 With this workflow, the autovalidator will ensure that all MFEX
 implementations are classifying each packet in exactly the same way.
diff --git a/tests/automake.mk b/tests/automake.mk
index 43731d0973..3bc74a5aff 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -143,11 +143,6 @@ $(srcdir)/tests/fuzz-regression-list.at: tests/automake.mk
echo "TEST_FUZZ_REGRESSION([$$basename])"; \
done > $@.tmp && mv $@.tmp $@
 
-EXTRA_DIST += $(MFEX_AUTOVALIDATOR_TESTS)
-MFEX_AUTOVALIDATOR_TESTS = \
-   tests/pcap/mfex_test.pcap \
-   tests/mfex_fuzzy.py
-
 OVSDB_CLUSTER_TESTSUITE_AT = \
tests/ovsdb-cluster-testsuite.at \
tests/ovsdb-execution.at \
@@ -518,7 +513,7 @@ tests_test_type_props_SOURCES = tests/test-type-props.c
 CHECK_PYFILES = \
tests/appctl.py \
tests/flowgen.py \
-   tests/mfex_fuzzy.py \
+   tests/genpkts.py \
tests/ovsdb-monitor-sort.py \
tests/test-daemon.py \
tests/test-json.py \
diff --git a/tests/genpkts.py b/tests/genpkts.py
new file mode 100755
index 00..f64f786ccb
--- /dev/null
+++ b/tests/genpkts.py
@@ -0,0 +1,56 @@
+#!/usr/bin/env python3
+
+import sys
+
+from scapy.all import RandMAC, RandIP, RandIP6, RandShort, fuzz
+from scapy.all import IPv6, Dot1Q, IP, Ether, UDP, TCP
+
+if len(sys.argv) < 2:
+print('usage: {} packets_count [fuzz]'.format(sys.argv[0]))
+sys.exit(1)
+
+tmpl = []
+
+if len(sys.argv) == 2:
+eth = Ether(dst='ff:ff:ff:ff:ff:ff')
+vlan = eth / Dot1Q(vlan=1)
+p = eth / IP() / TCP(sport=20, dport=80, flags='SA', window=8192)
+tmpl += [p.build().hex()]
+p = eth / IP() / UDP(sport=53, dport=53)
+tmpl += [p.build().hex()]
+p = eth / IP() / TCP(sport=20, dport=80, flags='S', window=8192)
+tmpl += [p.build().hex()]
+p = eth / IP() / UDP(sport=53, dport=53)
+tmpl += [p.build().hex()]
+p = vlan / IP() / UDP(sport=53, dport=53)
+tmpl += [p.build().hex()]
+p = vlan / IP() / TCP(sport=20, dport=80, flags='S', window=8192)
+tmpl += [p.build().hex()]
+elif sys.argv[2] == 'fuzz':
+# Generate random protocol bases, use a fuzz() over the combined packet
+# for full fuzzing.
+eth = Ether(src=RandMAC(), dst=RandMAC())
+vlan = Dot1Q()
+ipv4 = IP(src=RandIP(), dst=RandIP())
+ipv6 = IPv6(src=RandIP6(), dst=RandIP6())
+udp = UDP(dport=RandShort(), sport=RandShort())
+tcp = TCP(dport=RandShort(), sport=RandShort())
+
+# IPv4 packets with fuzzing
+

Re: [ovs-dev] [PATCH] ofproto: Fix resource usage explosion due to removal of large number of flows.

2021-11-30 Thread Ilya Maximets

On 11/23/21 12:13, Vladislav Odintsov wrote:
> Thanks for the patch!
> 
> Tested-by: Vladislav Odintsov mailto:odiv...@gmail.com>>


Thanks!  Applied and backported down to 2.13.

Best regards, Ilya Maximets.

> 
> Regards,
> Vladislav Odintsov
> 
>> On 22 Nov 2021, at 18:23, Ilya Maximets > > wrote:
>>
>> While removing flows, removal itself is deferred, so classifier changes
>> performed already from the RCU thread.  This way every deferred removal
>> triggers classifier change and reallocation of a pvector.  Freeing of
>> old version of a pvector is postponed.  Since all this is happening
>> from an RCU thread, all these copies of the same pvector will be freed
>> only after the next grace period.
>>
>> Below is the example output of the 'valgrind --tool=massif' from an OVN
>> deployment, where copies of that pvector took 5 GB of memory while
>> processing a bundled flow removal:
>>
>> ---
>>   n    time(i) total(B)   useful-heap(B) extra-heap(B)
>> ---
>>  89 176,257,987,954    5,329,763,160    5,318,171,607    11,591,553
>> 99.78% (5,318,171,607B) (heap allocation functions) malloc/new/new[]
>> ->98.45% (5,247,008,392B) xmalloc__ (util.c:137)
>> |->98.17% (5,232,137,408B) pvector_impl_dup (pvector.c:48)
>> ||->98.16% (5,231,472,896B) pvector_remove (pvector.c:159)
>> |||->98.16% (5,231,472,800B) destroy_subtable (classifier.c:1558)
>> ->98.16% (5,231,472,800B) classifier_remove (classifier.c:792)
>>  ->98.16% (5,231,472,800B) classifier_remove_assert (classifier.c:832)
>>   ->98.16% (5,231,472,800B) remove_rule_rcu__ (ofproto.c:2978)
>>    ->98.16% (5,231,472,800B) remove_rule_rcu (ofproto.c:2990)
>>     ->98.16% (5,231,472,800B) ovsrcu_call_postponed (ovs-rcu.c:346)
>>  ->98.16% (5,231,472,800B) ovsrcu_postpone_thread (ovs-rcu.c:362)
>>   ->98.16% (5,231,472,800B) ovsthread_wrapper
>>    ->98.16% (5,231,472,800B) start_thread
>>     ->98.16% (5,231,472,800B) clone
>>
>> Collecting all the flows to be removed and postponing removal for
>> all of them together to avoid the problem.  This way all removals
>> will trigger only a single pvector re-allocation greatly reducing
>> the CPU and memory usage.
>>
>> Reported-by: Vladislav Odintsov > >
>> Reported-at: 
>> https://mail.openvswitch.org/pipermail/ovs-dev/2021-November/389538.html 
>> 
>> Signed-off-by: Ilya Maximets mailto:i.maxim...@ovn.org>>
>> ---
>> ofproto/ofproto-provider.h |  4 
>> ofproto/ofproto.c  | 31 +--
>> 2 files changed, 33 insertions(+), 2 deletions(-)
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v3 1/4] system-dpdk: Refactor common logs matching.

2021-11-30 Thread David Marchand

Move EAL logs and commonly ignored logs to a common macro.
Remove obsolete ones (like i40e [1] and timer [2] logs).
Set log level for DPDK drivers to error only: the rationale is that we are
not testing DPDK drivers in system-dpdk.
Extend regex on hugepage logs since a check on hugepages availability is
already present on OVS side, and as a consequence, we don't care about
the warnings on availability for certain hugepage size.
Add logs checks for MFEX tests that were missing them.

1: https://git.dpdk.org/dpdk/commit/?id=a075ce2b3e8c
2: https://git.dpdk.org/dpdk/commit/?id=c1077933d45b

Signed-off-by: David Marchand 
---
Changes since v2:
- configured dpdk drivers log level to error,
- added OVS flow control warning to common list,

---
 tests/system-dpdk-macros.at |  3 ++
 tests/system-dpdk.at| 56 ++---
 2 files changed, 31 insertions(+), 28 deletions(-)

diff --git a/tests/system-dpdk-macros.at b/tests/system-dpdk-macros.at
index c6708caaf0..ef0e84e939 100644
--- a/tests/system-dpdk-macros.at
+++ b/tests/system-dpdk-macros.at
@@ -58,6 +58,9 @@ m4_define([OVS_DPDK_START],
dnl Enable DPDK functionality
AT_CHECK([ovs-vsctl --no-wait set Open_vSwitch . 
other_config:dpdk-init=true])
 
+   dnl Change DPDK drivers log levels so that tests only catch errors
+   AT_CHECK([ovs-vsctl --no-wait set Open_vSwitch . 
other_config:dpdk-extra=--log-level=pmd.*:error])
+
dnl Start ovs-vswitchd.
AT_CHECK([ovs-vswitchd --detach --no-chdir --pidfile --log-file -vvconn 
-vofproto_dpif -vunixctl], [0], [stdout], [stderr])
AT_CAPTURE_FILE([ovs-vswitchd.log])
diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
index e0e750fde5..9c8f4bb15a 100644
--- a/tests/system-dpdk.at
+++ b/tests/system-dpdk.at
@@ -3,6 +3,13 @@ m4_define([CONFIGURE_VETH_OFFLOADS],
 
 AT_BANNER([OVS-DPDK unit tests])
 
+m4_define([SYSTEM_DPDK_ALLOWED_LOGS],[
+\@does not exist. The Open vSwitch kernel module is probably not loaded.@d
+\@EAL:   Invalid NUMA socket, default to 0@d
+\@EAL: No \(available\|free\) hugepages reported in hugepages-@d
+\@Failed to enable flow control@d
+])
+
 dnl --
 dnl Check if EAL init is successful
 AT_SETUP([OVS-DPDK - EAL init])
@@ -12,10 +19,7 @@ OVS_DPDK_START()
 AT_CHECK([grep "DPDK Enabled - initializing..." ovs-vswitchd.log], [], 
[stdout])
 AT_CHECK([grep "EAL" ovs-vswitchd.log], [], [stdout])
 AT_CHECK([grep "DPDK Enabled - initialized" ovs-vswitchd.log], [], [stdout])
-OVS_VSWITCHD_STOP(["/Global register is changed during/d
-/EAL:   Invalid NUMA socket, default to 0/d
-/EAL: WARNING: cpu flags constant_tsc=yes nonstop_tsc=no -> using unreliable 
clock cycles !/d
-/EAL: No free hugepages reported in hugepages-1048576kB/d"])
+OVS_VSWITCHD_STOP("[SYSTEM_DPDK_ALLOWED_LOGS]")
 AT_CLEANUP
 dnl --
 
@@ -37,12 +41,7 @@ sleep 2
 
 dnl Clean up
 AT_CHECK([ovs-vsctl del-port br10 phy0], [], [stdout], [stderr])
-OVS_VSWITCHD_STOP("/does not exist. The Open vSwitch kernel module is probably 
not loaded./d
-/Failed to enable flow control/d
-/Global register is changed during/d
-/EAL: WARNING: cpu flags constant_tsc=yes nonstop_tsc=no -> using unreliable 
clock cycles !/d
-/EAL: No free hugepages reported in hugepages-1048576kB/d
-")
+OVS_VSWITCHD_STOP("[SYSTEM_DPDK_ALLOWED_LOGS]")
 AT_CLEANUP
 dnl --
 
@@ -68,13 +67,9 @@ AT_CHECK([grep "VHOST_CONFIG: $OVS_RUNDIR/dpdkvhostclient0: 
reconnecting..." ovs
 
 dnl Clean up
 AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout], 
[stderr])
-OVS_VSWITCHD_STOP(["\@does not exist. The Open vSwitch kernel module is 
probably not loaded.@d
-\@Failed to enable flow control@d
+OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
 \@VHOST_CONFIG: failed to connect to $OVS_RUNDIR/dpdkvhostclient0: No such 
file or directory@d
-\@Global register is changed during@d
-\@EAL:   Invalid NUMA socket, default to 0@d
-\@EAL: WARNING: cpu flags constant_tsc=yes nonstop_tsc=no -> using unreliable 
clock cycles !@d
-\@EAL: No free hugepages reported in hugepages-1048576kB@d"])
+])")
 AT_CLEANUP
 dnl --
 
@@ -144,16 +139,12 @@ pkill -f -x -9 'tail -f /dev/null'
 
 dnl Clean up
 AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuser0], [], [stdout], [stderr])
-OVS_VSWITCHD_STOP(["\@does not exist. The Open vSwitch kernel module is 
probably not loaded.@d
-\@Failed to enable flow control@d
+OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
 \@VHOST_CONFIG: recvmsg failed@d
 \@VHOST_CONFIG: failed to connect to $OVS_RUNDIR/dpdkvhostuser0: No such file 
or directory@d
-\@Global register is changed during@d
 \@dpdkvhostuser ports are considered deprecated;  please migrate to 
dpdkvhostuserclient ports.@d
 \@failed to enumerate system

Re: [ovs-dev] [PATCH] ofproto: Fix resource usage explosion while processing bundled FLOW_MOD.

2021-11-30 Thread Ilya Maximets

On 11/22/21 09:07, Vladislav Odintsov wrote:
> Thanks for the patch Ilya!
> 
> I’ve tested this with my setup in OVN similar to described in message by link 
> at "Reported-at".
> ovs-vswitchd with this patch applied seems working fine. It processed flows 
> and though with high CPU load during ~3-5 seconds after start, it went to 
> normal CPU load about 2-4%.
> I’ve tested this with both: OVS v2.13.5 and master branch.
> 
> Tested-by: Vladislav Odintsov mailto:odiv...@gmail.com>>


Thanks!  Applied and backported down to 2.13.

Best regards, Ilya Maximets.

> 
> Regards,
> Vladislav Odintsov
> 
>> On 20 Nov 2021, at 02:07, Ilya Maximets > > wrote:
>>
>> While processing a bundle, OVS will add all new and modified rules
>> to classifiers.  Classifiers are using RCU-protected pvector to
>> store subtables.  Addition of a new subtable or removal of the old
>> one leads to re-allocation and memory copy of the pvector array.
>> Old version of that array is given to RCU thread to free it later.
>>
>> The problem is that bundle is processed under the mutex without
>> entering the quiescent state.  Therefore, memory can not be freed
>> until the whole bundle is processed.  So, if a few thousands of
>> flows added to the same table in a bundle, pvector will be re-allocated
>> while adding each of them.  So, we'll have a few thousands of copies
>> of the same array waiting to be freed.
>>
>> In case of OVN deployments, there could be hundreds of thousands of
>> flows in the same table leading to a fast consumption of a huge
>> amount of memory and wasting a lot of CPU cycles on allocations and
>> copies.  Below snippet of the 'valgrid --tool=massif' output shows
>> ovs-vswitchd consuming 3.5GB of RAM while processing a bundle with
>> 65K FLOW_MODs in the OVN deployment.  3.4GB of that memory are
>> copies of the same pvector.
>>
>> ---
>>   n    time(i) total(B)   useful-heap(B) extra-heap(B)
>> ---
>>  64 109,907,465,404    3,559,987,568    3,546,879,748    13,107,820
>> 99.63% (3,546,879,748B) (heap allocation functions) malloc/new/new[]
>> ->97.61% (3,474,750,333B) xmalloc__ (util.c:137)
>> |->97.61% (3,474,750,333B) xmalloc (util.c:172)
>> | ->96.38% (3,431,068,352B) pvector_impl_dup (pvector.c:48)
>> || ->96.38% (3,431,067,840B) pvector_insert (pvector.c:138)
>> || |->96.38% (3,431,067,840B) classifier_replace (classifier.c:664)
>> || | ->96.38% (3,431,067,840B) classifier_insert (classifier.c:695)
>> || |  ->96.38% (3,431,067,840B) replace_rule_start (ofproto.c:5563)
>> || |   ->96.38% (3,431,067,840B) add_flow_start (ofproto.c:5179)
>> || |    ->96.38% (3,431,067,840B) ofproto_flow_mod_start (ofproto.c:8017)
>> || | ->96.38% (3,431,067,744B) do_bundle_commit (ofproto.c:8168)
>> || | |->96.38% (3,431,067,744B) handle_bundle_control (ofproto.c:8309)
>> || | | ->96.38% (3,431,067,744B) handle_single_part_openflow 
>> (ofproto.c:8593)
>> || | |  ->96.38% (3,431,067,744B) handle_openflow (ofproto.c:8674)
>> || | |   ->96.38% (3,431,067,744B) ofconn_run (connmgr.c:1329)
>> || | |    ->96.38% (3,431,067,744B) connmgr_run (connmgr.c:356)
>> || | | ->96.38% (3,431,067,744B) ofproto_run (ofproto.c:1879)
>> || | |  ->96.38% (3,431,067,744B) bridge_run__ (bridge.c:3251)
>> || | |   ->96.38% (3,431,067,744B) bridge_run (bridge.c:3310)
>> || | |    ->96.38% (3,431,067,744B) main (ovs-vswitchd.c:127)
>>
>> Fixing that by postponing the publishing of classifier updates,
>> so each flow modification can work with the same version of pvector.
>>
>> Unfortunately, bundled PACKET_OUT messages requires all previous
>> changes to be published before processing, otherwise the packet
>> will use wrong version of OF tables.  Publishing all changes before
>> processing PACKET_OUT messages to avoid this issue.  Hopefully,
>> mixup of a big number of FLOW_MOD and PACKET_OUT messages is not
>> a common usecase.
>>
>> Reported-by: Vladislav Odintsov > >
>> Reported-at: 
>> https://mail.openvswitch.org/pipermail/ovs-dev/2021-November/389503.html 
>> 
>> Signed-off-by: Ilya Maximets mailto:i.maxim...@ovn.org>>
>> ---
>> ofproto/ofproto-provider.h |  1 +
>> ofproto/ofproto.c  | 47 ++
>> 2 files changed, 48 insertions(+)
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] tests/flowgen: Fix length field of 802.2 data link header.

2021-11-30 Thread Ilya Maximets

On 11/24/21 19:32, Paolo Valerio wrote:
> Hello Ilya,
> 
> Ilya Maximets  writes:
> 
>> Length in Data Link Header for these packets should not include
>> source and destination MACs or the length field itself.
>>
>> Therefore, it should be 14 bytes less, otherwise other network
>> tools like wireshark complains:
>>
>>   Expert Info (Error/Malformed):
>> Length field value goes past the end of the payload
>>
>> Additionally fixing the printing of the packet/flow configuration,
>> as it currently prints '%s=%s' strings without any real data.
>>
>> Signed-off-by: Ilya Maximets 
>> ---
> 
> The patch LGTM and works as expected.
> Do you think it makes sense to include a 'Fixes' tag?

I don't know perl enough to figure out if the issue existed
in the original perl script and I'm too lazy to actually test
that.  The issue exists on all supported branches and it's also
the issue with tests and not with the OVS itself, so the 'Fixes'
tag is not very important.

> 
> In any case:
> 
> Acked-by: Paolo Valerio 
> 

Thanks!  Applied and backported down to 2.13.

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

Re: [ovs-dev] [PATCH ovn] controller: Add IDL memory reports.

2021-11-30 Thread Numan Siddique

On Tue, Nov 30, 2021 at 4:50 AM Dumitru Ceara  wrote:
>
> On 11/29/21 23:21, Numan Siddique wrote:
> > On Tue, Nov 23, 2021 at 10:02 AM Dumitru Ceara  wrote:
> >>
> >> OVS commit 066741d9c5ca ("ovsdb-idl: Add memory report function.")
> >> enhanced the IDL to track memory usage.  Use it in ovn-controller for
> >> the Southbound DB and local OVS DB IDLs.
> >>
> >> Signed-off-by: Dumitru Ceara 
> >> ---
> >>  controller/ovn-controller.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> >> index 29c1a05b2..fa1ff13bd 100644
> >> --- a/controller/ovn-controller.c
> >> +++ b/controller/ovn-controller.c
> >> @@ -3444,6 +3444,8 @@ main(int argc, char *argv[])
> >>  ofctrl_get_memory_usage();
> >>  if_status_mgr_get_memory_usage(if_mgr, );
> >>  local_datapath_memory_usage();
> >> +ovsdb_idl_get_memory_usage(ovnsb_idl_loop.idl, );
> >> +ovsdb_idl_get_memory_usage(ovs_idl_loop.idl, );
> >
> > Thanks for the patch.  I've one question.
>
> Thanks for the review!
>
> >
> > The function ovsdb_idl_get_memory_usage() adds 2 items to the "usage"
> > simap - "idl-cells" and "idl-outstanding-txns" (if its value is
> > greater than zero).
> > After the ovsdb_idl_get_memory_usage() for OVS IDL is called, the
> > "idl-cells" value in usage will be the sum of SB IDL cells and OVS IDL
> > cells.
>
> Right.
>
> > That's intentional right ?  I think it makes sense.  Just wanted to
> > check with you.
>
> Yes, it's due to how ovsdb_idl_get_memory_usage() currently works.
>
> > Is there a value in reporting as - "sb-idl-cells=", ovs-idl-cells=.." 
> >  ?
>
> Good point, I think there is.  And I think we can do that by changing
> ovsdb_idl_get_memory_usage() to prefix the "usage" simap item keys with
> the DB name.
>
> I sent a patch for that in OVS (the OVN bits don't need to change):
> https://patchwork.ozlabs.org/project/openvswitch/patch/20211130094757.23210-1-dce...@redhat.com/

Thanks.   Since there would be no change in OVN.  I applied this
patch.  Probably we have to bump the ovs submodule
once your OVS patch is merged.

Numan

>
> Thanks,
> Dumitru
>
> ___
> 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 v3] dpif-netdev: Call cpuid for x86 isa availability.

2021-11-30 Thread David Marchand

DPIF AVX512 optimisations currently rely on DPDK availability while
they can be used without DPDK.
Besides, checking for availability of some isa only has to be done once
and won't change while a OVS process runs.

Resolve isa availability in constructors by using a simplified query
based on cpuid API that comes from the compiler.

Note: this also fixes the check on BMI2 availability: DPDK had a bug
for this isa, see https://git.dpdk.org/dpdk/commit/?id=aae3037ab1e0.

Suggested-by: Ilya Maximets 
Signed-off-by: David Marchand 
---
Changes since v2:
- fixed compilation with AVX512,

Changes since v1:
- fixed minor checkpatch issue,

---
 lib/automake.mk|  2 +
 lib/cpu.c  | 68 ++
 lib/cpu.h  | 34 +
 lib/dpdk-stub.c|  9 
 lib/dpdk.c | 52 
 lib/dpdk.h |  1 -
 lib/dpif-netdev-avx512.c   |  5 +-
 lib/dpif-netdev-extract-avx512.c   | 14 +++---
 lib/dpif-netdev-lookup-avx512-gather.c |  7 +--
 9 files changed, 118 insertions(+), 74 deletions(-)
 create mode 100644 lib/cpu.c
 create mode 100644 lib/cpu.h

diff --git a/lib/automake.mk b/lib/automake.mk
index 46f869a336..5224e08564 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -38,6 +38,8 @@ lib_libopenvswitchavx512_la_CFLAGS = \
-fPIC \
$(AM_CFLAGS)
 lib_libopenvswitchavx512_la_SOURCES = \
+   lib/cpu.c \
+   lib/cpu.h \
lib/dpif-netdev-lookup-avx512-gather.c \
lib/dpif-netdev-extract-avx512.c \
lib/dpif-netdev-avx512.c
diff --git a/lib/cpu.c b/lib/cpu.c
new file mode 100644
index 00..ea1934d3ca
--- /dev/null
+++ b/lib/cpu.c
@@ -0,0 +1,68 @@
+/*
+ * Copyright (c) 2021, Red Hat, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include 
+
+#include "cpu.h"
+#include "openvswitch/compiler.h"
+
+#ifdef __x86_64__
+#include 
+#include 
+
+enum x86_reg {
+EAX,
+EBX,
+ECX,
+EDX,
+};
+#define X86_LEAF_MASK 0x8000
+#define X86_EXT_FEATURES_LEAF 0x0007
+static bool x86_has_isa(uint32_t leaf, enum x86_reg reg, uint32_t bit)
+{
+uint32_t maxleaf = __get_cpuid_max(leaf & X86_LEAF_MASK, NULL);
+uint32_t regs[4];
+
+if (maxleaf < leaf) {
+return false;
+}
+__cpuid_count(leaf, 0, regs[EAX], regs[EBX], regs[ECX], regs[EDX]);
+return (regs[reg] & ((uint32_t) 1 << bit)) != 0;
+}
+
+static bool x86_isa[OVS_CPU_ISA_X86_LAST - OVS_CPU_ISA_X86_FIRST + 1];
+#define X86_ISA(leaf, reg, bit, name) \
+OVS_CONSTRUCTOR(cpu_isa_ ## name) { \
+x86_isa[name - OVS_CPU_ISA_X86_FIRST] = x86_has_isa(leaf, reg, bit); \
+}
+X86_ISA(X86_EXT_FEATURES_LEAF, EBX,  8, OVS_CPU_ISA_X86_BMI2)
+X86_ISA(X86_EXT_FEATURES_LEAF, EBX, 16, OVS_CPU_ISA_X86_AVX512F)
+X86_ISA(X86_EXT_FEATURES_LEAF, EBX, 30, OVS_CPU_ISA_X86_AVX512BW)
+X86_ISA(X86_EXT_FEATURES_LEAF, ECX,  1, OVS_CPU_ISA_X86_AVX512VBMI)
+X86_ISA(X86_EXT_FEATURES_LEAF, ECX, 14, OVS_CPU_ISA_X86_VPOPCNTDQ)
+#endif
+
+bool
+cpu_has_isa(enum ovs_cpu_isa isa OVS_UNUSED)
+{
+#ifdef __x86_64__
+if (isa >= OVS_CPU_ISA_X86_FIRST &&
+isa <= OVS_CPU_ISA_X86_LAST) {
+return x86_isa[isa - OVS_CPU_ISA_X86_FIRST];
+}
+#endif
+return false;
+}
diff --git a/lib/cpu.h b/lib/cpu.h
new file mode 100644
index 00..92897bb71c
--- /dev/null
+++ b/lib/cpu.h
@@ -0,0 +1,34 @@
+/*
+ * Copyright (c) 2021, Red Hat, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef CPU_H
+#define CPU_H 1
+
+#include 
+
+enum ovs_cpu_isa {
+OVS_CPU_ISA_X86_FIRST,
+OVS_CPU_ISA_X86_BMI2 = OVS_CPU_ISA_X86_FIRST,
+OVS_CPU_ISA_X86_AVX512F,
+OVS_CPU_ISA_X86_AVX512BW,
+OVS_CPU_ISA_X86_AVX512VBMI,
+OVS_CPU_ISA_X86_VPOPCNTDQ,
+OVS_CPU_ISA_X86_LAST = OVS_CPU_ISA_X86_VPOPCNTDQ,
+};
+
+bool cpu_has_isa(enum ovs_cpu_isa);
+
+#endif /* CPU_H */

Re: [ovs-dev] [PATCH ovn] controller/pinctrl: improve packet-in debuggability

2021-11-30 Thread Numan Siddique

On Tue, Nov 30, 2021 at 6:24 AM Mohammad Heib  wrote:
>
>
> On 11/29/21 10:39 PM, Numan Siddique wrote:
> > On Thu, Nov 18, 2021 at 5:55 AM Mohammad Heib  wrote:
> >> Improve packet-in debuggability within pinctrl module
> >> by printing basic details about each received packet-in
> >> message, those messages will be printed to the logs only
> >> when DBG log level is enabled.
> >>
> >> Also, add two coverage counters that will indicate the total
> >> packet-in messages that were received and the number of times
> >> that the pinctrl main thread was notified to handle a change
> >> in the local DBs, those counters can be used by the user as
> >> an indicator to enable the DBG logs level and see more details
> >> about the received packet-in in the logs.
> >>
> >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1821965
> >> Signed-off-by: Mohammad Heib 
> >> ---
> >>   controller/pinctrl.c | 66 
> >>   tests/ovn.at |  8 ++
> >>   2 files changed, 74 insertions(+)
> >>
> >> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> >> index ae5320e09..38bda2a89 100644
> >> --- a/controller/pinctrl.c
> >> +++ b/controller/pinctrl.c
> >> @@ -364,6 +364,8 @@ COVERAGE_DEFINE(pinctrl_drop_put_mac_binding);
> >>   COVERAGE_DEFINE(pinctrl_drop_buffered_packets_map);
> >>   COVERAGE_DEFINE(pinctrl_drop_controller_event);
> >>   COVERAGE_DEFINE(pinctrl_drop_put_vport_binding);
> >> +COVERAGE_DEFINE(pinctrl_notify_main_thread);
> >> +COVERAGE_DEFINE(pinctrl_total_pin_pkts);
> >>
> >>   struct empty_lb_backends_event {
> >>   struct hmap_node hmap_node;
> >> @@ -3073,6 +3075,7 @@ process_packet_in(struct rconn *swconn, const struct 
> >> ofp_header *msg)
> >>   {
> >>   static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> >>
> >> +struct ds pin_str = DS_EMPTY_INITIALIZER;
> >>   struct ofputil_packet_in pin;
> >>   struct ofpbuf continuation;
> >>   enum ofperr error = ofputil_decode_packet_in(msg, true, NULL, NULL, 
> >> ,
> >> @@ -3099,18 +3102,22 @@ process_packet_in(struct rconn *swconn, const 
> >> struct ofp_header *msg)
> >>   dp_packet_use_const(, pin.packet, pin.packet_len);
> >>   struct flow headers;
> >>   flow_extract(, );
> >> +ds_put_cstr(_str, "pinctrl received  packet-in | opcode=");
> >>
> >>   switch (ntohl(ah->opcode)) {
> >>   case ACTION_OPCODE_ARP:
> >> +ds_put_cstr(_str, "ARP");
> >>   pinctrl_handle_arp(swconn, , , _metadata,
> >>  );
> >>   break;
> >>   case ACTION_OPCODE_IGMP:
> >> +ds_put_cstr(_str, "IGMP");
> >>   pinctrl_ip_mcast_handle(swconn, , , 
> >> _metadata,
> >>   );
> >>   break;
> >>
> >>   case ACTION_OPCODE_PUT_ARP:
> >> +ds_put_cstr(_str, "PUT_ARP");
> >>   ovs_mutex_lock(_mutex);
> >>   pinctrl_handle_put_mac_binding(_metadata.flow, ,
> >>  true);
> >> @@ -3118,21 +3125,25 @@ process_packet_in(struct rconn *swconn, const 
> >> struct ofp_header *msg)
> >>   break;
> >>
> >>   case ACTION_OPCODE_PUT_DHCP_OPTS:
> >> +ds_put_cstr(_str, "PUT_DHCP_OPTS");
> >>   pinctrl_handle_put_dhcp_opts(swconn, , , ,
> >>, );
> >>   break;
> >>
> >>   case ACTION_OPCODE_ND_NA:
> >> +ds_put_cstr(_str, "ND_NA");
> >>   pinctrl_handle_nd_na(swconn, , _metadata, 
> >> ,
> >>false);
> >>   break;
> >>
> >>   case ACTION_OPCODE_ND_NA_ROUTER:
> >> +ds_put_cstr(_str, "ND_NA_ROUTER");
> >>   pinctrl_handle_nd_na(swconn, , _metadata, 
> >> ,
> >>true);
> >>   break;
> >>
> >>   case ACTION_OPCODE_PUT_ND:
> >> +ds_put_cstr(_str, "PUT_ND");
> >>   ovs_mutex_lock(_mutex);
> >>   pinctrl_handle_put_mac_binding(_metadata.flow, ,
> >>  false);
> >> @@ -3140,17 +3151,20 @@ process_packet_in(struct rconn *swconn, const 
> >> struct ofp_header *msg)
> >>   break;
> >>
> >>   case ACTION_OPCODE_PUT_FDB:
> >> +ds_put_cstr(_str, "PUT_FDB");
> >>   ovs_mutex_lock(_mutex);
> >>   pinctrl_handle_put_fdb(_metadata.flow, );
> >>   ovs_mutex_unlock(_mutex);
> >>   break;
> >>
> >>   case ACTION_OPCODE_PUT_DHCPV6_OPTS:
> >> +ds_put_cstr(_str, "PUT_DHCPV6_OPTS");
> >>   pinctrl_handle_put_dhcpv6_opts(swconn, , , ,
> >>  );
> >>   break;
> >>
> >>   case ACTION_OPCODE_DNS_LOOKUP:
> >> +ds_put_cstr(_str, "DNS_LOOKUP");
> >>   ovs_mutex_lock(_mutex);
> >>   pinctrl_handle_dns_lookup(swconn, , , ,
> >> );
> >> @@ -3158,63 +3172,83 @@ process_packet_in(struct rconn *swconn, const 
> >>

Re: [ovs-dev] [PATCH dpdk-latest] acinclude: Enable -Werror by default

2021-11-30 Thread Ilya Maximets

On 11/30/21 10:43, Eli Britstein wrote:
> 
> On 11/30/2021 12:31 AM, Ilya Maximets wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 11/7/21 11:56, Eli Britstein via dev wrote:
>>> Following dpdk commits [1]-[3], it is now possible to compile with
>>> --enable-Werror. Change the default to on, with an option to disable
>>> using --disable-Werror.
>>>
>>> Notes:
>>> 1. To compile against 21.11-rc1, need to apply [4] and [5] patches.
>>> 2. There are still sparse errors, due to dpdk issue. [6] fixes it.
>>>
>>> [1] a3f8d0587188 ("net: avoid cast-align warning in VLAN insert function")
>>> [2] da0333c8790b ("mbuf: avoid cast-align warning in data offset macro")
>>> [3] 6de430b7079e ("eal/x86: avoid cast-align warning in memcpy functions")
>>> [4] https://patchwork.ozlabs.org/project/openvswitch/list/?series=268844
>>> [5] https://patchwork.ozlabs.org/project/openvswitch/list/?series=261231
>>> [6] 
>>> https://patches.dpdk.org/project/dpdk/patch/20211028101428.15007-1-david.march...@redhat.com/
>>>
>>> Signed-off-by: Eli Britstein 
>>> Reviewed-by: Gaetan Rivet 
>>> ---
>>>   .ci/linux-build.sh | 1 -
>>>   .cirrus.yml    | 2 +-
>>>   acinclude.m4   | 4 ++--
>>>   3 files changed, 3 insertions(+), 4 deletions(-)
>> Hi, Eli.  I'm not sure if I understand the reason behind this patch.
> 
> In linux x86 at least, we had warnings by dpdk. We discussed it in [7], and 
> fixed some in OVS by [8].
> 
> The fixes in dpdk were merged (see above [1]-[3]), so I thought to have this 
> option by default to prevent future warnings.

Hmm.  Maybe it's better to just remove -Wno-cast-align from CI scripts
instead?  Current version of a patch will not catch cast-align warnings
in our CI, since they are explicitly disabled.

> 
> We did encounter some issue on PPC, still pending to a proper resolution, [9].
> 
> 
> [7] https://mail.openvswitch.org/pipermail/ovs-dev/2021-July/384773.html
> 
> [8] https://mail.openvswitch.org/pipermail/ovs-dev/2021-July/385482.html
> 
> [9] https://mail.openvswitch.org/pipermail/ovs-dev/2021-November/389381.html
> 
>> But, in any case, I believe that it will break the Windows build, as
>> it currently produces a fair amount of warnings.
> 
> I admit I haven't tested windows. I tried to look into it, but could not find 
> a free CI tool for that.
> 
> I tried to follow 
> https://docs.openvswitch.org/en/latest/intro/install/windows/, but could not 
> understand what option to choose in http://www.mingw.org/wiki/Getting_Started
> 
> Could you please advise?

We're using https://www.appveyor.com/ as a Windows CI.  It's free for
public projects.  Concurrency is limited to 1 job at a time, but that
should not be a big problem.  You should be able to set it up for
yourself.  appveyor.yml is included in the OVS repo.

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

Re: [ovs-dev] [PATCH ovn] controller/pinctrl: improve packet-in debuggability

2021-11-30 Thread Mohammad Heib




On 11/29/21 10:39 PM, Numan Siddique wrote:

On Thu, Nov 18, 2021 at 5:55 AM Mohammad Heib  wrote:

Improve packet-in debuggability within pinctrl module
by printing basic details about each received packet-in
message, those messages will be printed to the logs only
when DBG log level is enabled.

Also, add two coverage counters that will indicate the total
packet-in messages that were received and the number of times
that the pinctrl main thread was notified to handle a change
in the local DBs, those counters can be used by the user as
an indicator to enable the DBG logs level and see more details
about the received packet-in in the logs.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1821965
Signed-off-by: Mohammad Heib 
---
  controller/pinctrl.c | 66 
  tests/ovn.at |  8 ++
  2 files changed, 74 insertions(+)

diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index ae5320e09..38bda2a89 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -364,6 +364,8 @@ COVERAGE_DEFINE(pinctrl_drop_put_mac_binding);
  COVERAGE_DEFINE(pinctrl_drop_buffered_packets_map);
  COVERAGE_DEFINE(pinctrl_drop_controller_event);
  COVERAGE_DEFINE(pinctrl_drop_put_vport_binding);
+COVERAGE_DEFINE(pinctrl_notify_main_thread);
+COVERAGE_DEFINE(pinctrl_total_pin_pkts);

  struct empty_lb_backends_event {
  struct hmap_node hmap_node;
@@ -3073,6 +3075,7 @@ process_packet_in(struct rconn *swconn, const struct 
ofp_header *msg)
  {
  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);

+struct ds pin_str = DS_EMPTY_INITIALIZER;
  struct ofputil_packet_in pin;
  struct ofpbuf continuation;
  enum ofperr error = ofputil_decode_packet_in(msg, true, NULL, NULL, ,
@@ -3099,18 +3102,22 @@ process_packet_in(struct rconn *swconn, const struct 
ofp_header *msg)
  dp_packet_use_const(, pin.packet, pin.packet_len);
  struct flow headers;
  flow_extract(, );
+ds_put_cstr(_str, "pinctrl received  packet-in | opcode=");

  switch (ntohl(ah->opcode)) {
  case ACTION_OPCODE_ARP:
+ds_put_cstr(_str, "ARP");
  pinctrl_handle_arp(swconn, , , _metadata,
 );
  break;
  case ACTION_OPCODE_IGMP:
+ds_put_cstr(_str, "IGMP");
  pinctrl_ip_mcast_handle(swconn, , , _metadata,
  );
  break;

  case ACTION_OPCODE_PUT_ARP:
+ds_put_cstr(_str, "PUT_ARP");
  ovs_mutex_lock(_mutex);
  pinctrl_handle_put_mac_binding(_metadata.flow, ,
 true);
@@ -3118,21 +3125,25 @@ process_packet_in(struct rconn *swconn, const struct 
ofp_header *msg)
  break;

  case ACTION_OPCODE_PUT_DHCP_OPTS:
+ds_put_cstr(_str, "PUT_DHCP_OPTS");
  pinctrl_handle_put_dhcp_opts(swconn, , , ,
   , );
  break;

  case ACTION_OPCODE_ND_NA:
+ds_put_cstr(_str, "ND_NA");
  pinctrl_handle_nd_na(swconn, , _metadata, ,
   false);
  break;

  case ACTION_OPCODE_ND_NA_ROUTER:
+ds_put_cstr(_str, "ND_NA_ROUTER");
  pinctrl_handle_nd_na(swconn, , _metadata, ,
   true);
  break;

  case ACTION_OPCODE_PUT_ND:
+ds_put_cstr(_str, "PUT_ND");
  ovs_mutex_lock(_mutex);
  pinctrl_handle_put_mac_binding(_metadata.flow, ,
 false);
@@ -3140,17 +3151,20 @@ process_packet_in(struct rconn *swconn, const struct 
ofp_header *msg)
  break;

  case ACTION_OPCODE_PUT_FDB:
+ds_put_cstr(_str, "PUT_FDB");
  ovs_mutex_lock(_mutex);
  pinctrl_handle_put_fdb(_metadata.flow, );
  ovs_mutex_unlock(_mutex);
  break;

  case ACTION_OPCODE_PUT_DHCPV6_OPTS:
+ds_put_cstr(_str, "PUT_DHCPV6_OPTS");
  pinctrl_handle_put_dhcpv6_opts(swconn, , , ,
 );
  break;

  case ACTION_OPCODE_DNS_LOOKUP:
+ds_put_cstr(_str, "DNS_LOOKUP");
  ovs_mutex_lock(_mutex);
  pinctrl_handle_dns_lookup(swconn, , , ,
);
@@ -3158,63 +3172,83 @@ process_packet_in(struct rconn *swconn, const struct 
ofp_header *msg)
  break;

  case ACTION_OPCODE_LOG:
+ds_put_cstr(_str, "LOG");
  handle_acl_log(, );
  break;

  case ACTION_OPCODE_PUT_ND_RA_OPTS:
+ds_put_cstr(_str, "PUT_ND_RA_OPTS");
  pinctrl_handle_put_nd_ra_opts(swconn, , , ,
, );
  break;

  case ACTION_OPCODE_ND_NS:
+ds_put_cstr(_str, "ND_NS");
  pinctrl_handle_nd_ns(swconn, , , _metadata,
   );
  break;

  case ACTION_OPCODE_ICMP:
+ds_put_cstr(_str, "ICMP");
  pinctrl_handle_icmp(swconn, , ,

Re: [ovs-dev] [PATCH ovn] controller: Add IDL memory reports.

2021-11-30 Thread Dumitru Ceara

On 11/29/21 23:21, Numan Siddique wrote:
> On Tue, Nov 23, 2021 at 10:02 AM Dumitru Ceara  wrote:
>>
>> OVS commit 066741d9c5ca ("ovsdb-idl: Add memory report function.")
>> enhanced the IDL to track memory usage.  Use it in ovn-controller for
>> the Southbound DB and local OVS DB IDLs.
>>
>> Signed-off-by: Dumitru Ceara 
>> ---
>>  controller/ovn-controller.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>> index 29c1a05b2..fa1ff13bd 100644
>> --- a/controller/ovn-controller.c
>> +++ b/controller/ovn-controller.c
>> @@ -3444,6 +3444,8 @@ main(int argc, char *argv[])
>>  ofctrl_get_memory_usage();
>>  if_status_mgr_get_memory_usage(if_mgr, );
>>  local_datapath_memory_usage();
>> +ovsdb_idl_get_memory_usage(ovnsb_idl_loop.idl, );
>> +ovsdb_idl_get_memory_usage(ovs_idl_loop.idl, );
> 
> Thanks for the patch.  I've one question.

Thanks for the review!

> 
> The function ovsdb_idl_get_memory_usage() adds 2 items to the "usage"
> simap - "idl-cells" and "idl-outstanding-txns" (if its value is
> greater than zero).
> After the ovsdb_idl_get_memory_usage() for OVS IDL is called, the
> "idl-cells" value in usage will be the sum of SB IDL cells and OVS IDL
> cells.

Right.

> That's intentional right ?  I think it makes sense.  Just wanted to
> check with you.

Yes, it's due to how ovsdb_idl_get_memory_usage() currently works.

> Is there a value in reporting as - "sb-idl-cells=", ovs-idl-cells=.."  ?

Good point, I think there is.  And I think we can do that by changing
ovsdb_idl_get_memory_usage() to prefix the "usage" simap item keys with
the DB name.

I sent a patch for that in OVS (the OVN bits don't need to change):
https://patchwork.ozlabs.org/project/openvswitch/patch/20211130094757.23210-1-dce...@redhat.com/

Thanks,
Dumitru

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

[ovs-dev] [PATCH] ovsdb-idl: Get per-database memory usage statistics.

2021-11-30 Thread Dumitru Ceara

Clients might be connected to multiple databases (e.g., ovn-controller
is connected to OVN_Southbound and Open_vSwitch databases) and the IDL
memory statistics are more useful if they're not aggregated.

Signed-off-by: Dumitru Ceara 
---
 lib/ovsdb-idl.c | 17 ++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index a6323d2b8b9a..26b5aae36a92 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -498,9 +498,20 @@ ovsdb_idl_get_memory_usage(struct ovsdb_idl *idl, struct 
simap *usage)
 cells += n_rows * n_columns;
 }
 
-simap_increase(usage, "idl-cells", cells);
-simap_increase(usage, "idl-outstanding-txns",
-   hmap_count(>outstanding_txns));
+struct {
+const char *name;
+unsigned int val;
+} idl_mem_stats[] = {
+{"idl-outstanding-txns", hmap_count(>outstanding_txns)},
+{"idl-cells", cells},
+};
+
+for (size_t i = 0; i < ARRAY_SIZE(idl_mem_stats); i++) {
+char *stat_name = xasprintf("%s-%s", idl->class_->database,
+ idl_mem_stats[i].name);
+simap_increase(usage, stat_name, idl_mem_stats[i].val);
+free(stat_name);
+}
 }
 
 /* Returns a "sequence number" that represents the state of 'idl'.  When
-- 
2.27.0

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

Re: [ovs-dev] [PATCH dpdk-latest] acinclude: Enable -Werror by default

2021-11-30 Thread Eli Britstein via dev




On 11/30/2021 12:31 AM, Ilya Maximets wrote:

External email: Use caution opening links or attachments


On 11/7/21 11:56, Eli Britstein via dev wrote:

Following dpdk commits [1]-[3], it is now possible to compile with
--enable-Werror. Change the default to on, with an option to disable
using --disable-Werror.

Notes:
1. To compile against 21.11-rc1, need to apply [4] and [5] patches.
2. There are still sparse errors, due to dpdk issue. [6] fixes it.

[1] a3f8d0587188 ("net: avoid cast-align warning in VLAN insert function")
[2] da0333c8790b ("mbuf: avoid cast-align warning in data offset macro")
[3] 6de430b7079e ("eal/x86: avoid cast-align warning in memcpy functions")
[4] https://patchwork.ozlabs.org/project/openvswitch/list/?series=268844
[5] https://patchwork.ozlabs.org/project/openvswitch/list/?series=261231
[6] 
https://patches.dpdk.org/project/dpdk/patch/20211028101428.15007-1-david.march...@redhat.com/

Signed-off-by: Eli Britstein 
Reviewed-by: Gaetan Rivet 
---
  .ci/linux-build.sh | 1 -
  .cirrus.yml| 2 +-
  acinclude.m4   | 4 ++--
  3 files changed, 3 insertions(+), 4 deletions(-)

Hi, Eli.  I'm not sure if I understand the reason behind this patch.


In linux x86 at least, we had warnings by dpdk. We discussed it in [7], 
and fixed some in OVS by [8].


The fixes in dpdk were merged (see above [1]-[3]), so I thought to have 
this option by default to prevent future warnings.


We did encounter some issue on PPC, still pending to a proper 
resolution, [9].



[7] https://mail.openvswitch.org/pipermail/ovs-dev/2021-July/384773.html

[8] https://mail.openvswitch.org/pipermail/ovs-dev/2021-July/385482.html

[9] https://mail.openvswitch.org/pipermail/ovs-dev/2021-November/389381.html


But, in any case, I believe that it will break the Windows build, as
it currently produces a fair amount of warnings.


I admit I haven't tested windows. I tried to look into it, but could not 
find a free CI tool for that.


I tried to follow 
https://docs.openvswitch.org/en/latest/intro/install/windows/, but could 
not understand what option to choose in 
http://www.mingw.org/wiki/Getting_Started


Could you please advise?



Best regards, Ilya Maximets.

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

Re: [ovs-dev] [PATCH ovn v3] ovn-controller: Avoid infinite replying for TCP/ICMP connection reset messages

2021-11-30 Thread Mohammad Heib




On 11/29/21 8:45 PM, Numan Siddique wrote:

On Tue, Oct 12, 2021 at 8:18 AM  wrote:

From: Mohammad Heib 

When the ovn controller receives an ip packet that targets a lport that has ACL
rule to reject ip packets, the controller will reply with TCP_RST or icmp4/6 
unreachable packet
to notify the sender that the destination is not available.

In turn, the receiver host will receive the notification packet and handle it 
as a normal IP packet
and if the receiver host is part of the same logical-switch/port-group or has 
IP reject ACL rule
it will send TCP_RST or icmp4/6 unreachable packet replying to the TCP_RST or 
icmp4/6 unreachable
packet we received and here we will enter to an infinity loop of replying about 
replying which
will consume high CPU.

To avoid such scenarios this patch proposes to drop/ignore TCP_RST or icmp4/6 
unreachable packets
that received on lport that has  IP reject ACL rules.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1934011
Fixes: 64f8c9e9f ("actions: Add a new OVN action - reject {}.")
Signed-off-by: Mohammad Heib 


Hi Numan,

thank you for your review :).



Hi Mohammad,

Sorry for the very late response to this patch.

The patch LGTM.   I just have a few comments in the test.



---
  controller/pinctrl.c |  29 +++
  tests/ovn-northd.at  | 111 +++
  2 files changed, 140 insertions(+)

diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index cc3edaaf4..eff599d2b 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -1934,11 +1934,40 @@ pinctrl_handle_sctp_abort(struct rconn *swconn, const 
struct flow *ip_flow,
  dp_packet_uninit();
  }

+static bool
+pinctrl_handle_reject_ignore_pkt(const struct flow *ip_flow,
+ struct dp_packet *pkt_in)
+{
+if (ip_flow->nw_proto == IPPROTO_TCP) {
+struct tcp_header *th = dp_packet_l4(pkt_in);
+if (!th || (TCP_FLAGS(th->tcp_ctl) & TCP_RST)) {
+return true;
+}
+} else {
+if (is_icmpv4(ip_flow, NULL)) {
+struct icmp_header *ih = dp_packet_l4(pkt_in);
+if (!ih || (ih->icmp_type == ICMP4_DST_UNREACH)) {
+return true;
+}
+} else if (is_icmpv6(ip_flow, NULL)) {
+struct icmp6_data_header *ih = dp_packet_l4(pkt_in);
+if (!ih || (ih->icmp6_base.icmp6_type == ICMP6_DST_UNREACH)) {
+return true;
+}
+}
+}
+return false;
+}
+
  static void
  pinctrl_handle_reject(struct rconn *swconn, const struct flow *ip_flow,
struct dp_packet *pkt_in,
const struct match *md, struct ofpbuf *userdata)
  {
+if (pinctrl_handle_reject_ignore_pkt(ip_flow, pkt_in)) {
+return;
+}
+
  if (ip_flow->nw_proto == IPPROTO_TCP) {
  pinctrl_handle_tcp_reset(swconn, ip_flow, pkt_in, md, userdata, true);
  } else if (ip_flow->nw_proto == IPPROTO_SCTP) {
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 8b9049899..4e4ed7b5f 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -2062,6 +2062,117 @@ sw1flows3:  table=4 (ls_out_acl ), 
priority=2003 , match=((reg0[[9]] ==
  AT_CLEANUP
  ])


I think this test case should be added in ovn.at and not in
ovn-northd.at.  Please move it to ovn.at

yes, i was actually confused about where to add the test case but
definitely ovn.at is a more appropriate plac.




+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ACL Reject ping pong])
+AT_KEYWORDS([ACL Reject ping pong])
+ovn_start
+
+send_icmp6_packet() {
+local inport=$1 hv=$2 eth_src=$3 eth_dst=$4 ipv6_src=$5 ipv6_dst=$6
+
+local ip6_hdr=60083aff${ipv6_src}${ipv6_dst}
+local packet=${eth_dst}${eth_src}86dd${ip6_hdr}8000dcb662f1
+
+as hv$hv ovs-appctl netdev-dummy/receive hv${hv}-vif$inport $packet
+}
+
+send_icmp_packet() {
+local inport=$1 hv=$2 eth_src=$3 eth_dst=$4 ipv4_src=$5 ipv4_dst=$6 
ip_chksum=$7 data=$8
+shift 8
+
+local ip_ttl=ff
+local ip_len=001c
+local 
packet=${eth_dst}${eth_src}08004500${ip_len}4000${ip_ttl}01${ip_chksum}${ipv4_src}${ipv4_dst}${data}
+as hv$hv ovs-appctl netdev-dummy/receive hv${hv}-vif$inport $packet
+}
+
+net_add n1
+
+sim_add hv1
+as hv1
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+ovs-vsctl -- add-port br-int hv1-vif1 -- \
+set interface hv1-vif1 external-ids:iface-id=sw0-p1 \
+options:tx_pcap=hv1/vif1-tx.pcap \
+options:rxq_pcap=hv1/vif1-rx.pcap \
+ofport-request=1
+
+sim_add hv2
+as hv2
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.2
+ovs-vsctl -- add-port br-int hv2-vif1 -- \
+set interface hv2-vif1 external-ids:iface-id=sw0-p2 \
+options:tx_pcap=hv2/vif1-tx.pcap \
+options:rxq_pcap=hv2/vif1-rx.pcap \
+ofport-request=1
+
+ovn-nbctl ls-add sw0
+
+check ovn-nbctl lsp-add sw0 sw0-p1
+check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3

[ovs-dev] [PATCH ovn v4] ovn-controller: Avoid infinite replying for TCP/ICMP connection reset messages

2021-11-30 Thread Mohammad Heib

When the ovn controller receives an ip packet that targets a lport that has ACL
rule to reject ip packets, the controller will reply with TCP_RST or icmp4/6 
unreachable packet
to notify the sender that the destination is not available.

In turn, the receiver host will receive the notification packet and handle it 
as a normal IP packet
and if the receiver host is part of the same logical-switch/port-group or has 
IP reject ACL rule
it will send TCP_RST or icmp4/6 unreachable packet replying to the TCP_RST or 
icmp4/6 unreachable
packet we received and here we will enter to an infinity loop of replying about 
replying which
will consume high CPU.

To avoid such scenarios this patch proposes to drop/ignore TCP_RST or icmp4/6 
unreachable packets
that received on lport that has  IP reject ACL rules.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1934011
Fixes: 64f8c9e9f ("actions: Add a new OVN action - reject {}.")
Signed-off-by: Mohammad Heib 
---
 controller/pinctrl.c |  29 +++
 tests/ovn.at | 113 +++
 2 files changed, 142 insertions(+)

diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index ae5320e09..0d443c150 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -1933,11 +1933,40 @@ pinctrl_handle_sctp_abort(struct rconn *swconn, const 
struct flow *ip_flow,
 dp_packet_uninit();
 }
 
+static bool
+pinctrl_handle_reject_ignore_pkt(const struct flow *ip_flow,
+ struct dp_packet *pkt_in)
+{
+if (ip_flow->nw_proto == IPPROTO_TCP) {
+struct tcp_header *th = dp_packet_l4(pkt_in);
+if (!th || (TCP_FLAGS(th->tcp_ctl) & TCP_RST)) {
+return true;
+}
+} else {
+if (is_icmpv4(ip_flow, NULL)) {
+struct icmp_header *ih = dp_packet_l4(pkt_in);
+if (!ih || (ih->icmp_type == ICMP4_DST_UNREACH)) {
+return true;
+}
+} else if (is_icmpv6(ip_flow, NULL)) {
+struct icmp6_data_header *ih = dp_packet_l4(pkt_in);
+if (!ih || (ih->icmp6_base.icmp6_type == ICMP6_DST_UNREACH)) {
+return true;
+}
+}
+}
+return false;
+}
+
 static void
 pinctrl_handle_reject(struct rconn *swconn, const struct flow *ip_flow,
   struct dp_packet *pkt_in,
   const struct match *md, struct ofpbuf *userdata)
 {
+if (pinctrl_handle_reject_ignore_pkt(ip_flow, pkt_in)) {
+return;
+}
+
 if (ip_flow->nw_proto == IPPROTO_TCP) {
 pinctrl_handle_tcp_reset(swconn, ip_flow, pkt_in, md, userdata, true);
 } else if (ip_flow->nw_proto == IPPROTO_SCTP) {
diff --git a/tests/ovn.at b/tests/ovn.at
index 7a7ae0da6..a4ed03041 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -14223,6 +14223,119 @@ OVN_CLEANUP([hv1], [hv2], [hv3])
 AT_CLEANUP
 ])
 
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ACL Reject ping pong])
+AT_KEYWORDS([ACL Reject ping pong])
+ovn_start
+
+send_icmp6_packet() {
+local inport=$1 hv=$2 eth_src=$3 eth_dst=$4 ipv6_src=$5 ipv6_dst=$6
+
+local ip6_hdr=60083aff${ipv6_src}${ipv6_dst}
+local packet=${eth_dst}${eth_src}86dd${ip6_hdr}8000dcb662f1
+
+as hv$hv ovs-appctl netdev-dummy/receive hv${hv}-vif$inport $packet
+}
+
+send_icmp_packet() {
+local inport=$1 hv=$2 eth_src=$3 eth_dst=$4 ipv4_src=$5 ipv4_dst=$6 
ip_chksum=$7 data=$8
+shift 8
+
+local ip_ttl=ff
+local ip_len=001c
+local 
packet=${eth_dst}${eth_src}08004500${ip_len}4000${ip_ttl}01${ip_chksum}${ipv4_src}${ipv4_dst}${data}
+as hv$hv ovs-appctl netdev-dummy/receive hv${hv}-vif$inport $packet
+}
+
+net_add n1
+
+sim_add hv1
+as hv1
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+ovs-vsctl -- add-port br-int hv1-vif1 -- \
+set interface hv1-vif1 external-ids:iface-id=sw0-p1 \
+options:tx_pcap=hv1/vif1-tx.pcap \
+options:rxq_pcap=hv1/vif1-rx.pcap \
+ofport-request=1
+
+sim_add hv2
+as hv2
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.2
+ovs-vsctl -- add-port br-int hv2-vif1 -- \
+set interface hv2-vif1 external-ids:iface-id=sw0-p2 \
+options:tx_pcap=hv2/vif1-tx.pcap \
+options:rxq_pcap=hv2/vif1-rx.pcap \
+ofport-request=1
+
+ovn-nbctl ls-add sw0
+
+check ovn-nbctl lsp-add sw0 sw0-p1
+check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3 1000::3"
+check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3 
1000::3"
+
+check ovn-nbctl lsp-add sw0 sw0-p2
+check ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:04 10.0.0.4 1000::4"
+check ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4 
1000::4"
+
+check ovn-nbctl acl-add sw0 to-lport 1002 ip reject
+
+OVN_POPULATE_ARP
+
+wait_for_ports_up
+ovn-nbctl --wait=hv sync
+
+ovn-sbctl dump-flows sw0 > sw0-flows
+AT_CAPTURE_FILE([sw0-flows])
+
+AT_CHECK([grep -E 'ls_(in|out)_acl' sw0-flows |grep reject| sed 
's/table=../table=??/' | sort], [0],

Re: [ovs-dev] [PATCH v3 1/4] system-dpdk: Refactor common logs matching.

Re: [ovs-dev] [PATCH dpdk-latest] acinclude: Enable -Werror by default

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

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

Re: [ovs-dev] [PATCH ovn] lflow-cache: Automatically trim cache when it becomes inactive.

Re: [ovs-dev] [PATCH ovn] lflow-cache: Automatically trim cache when it becomes inactive.

Re: [ovs-dev] [PATCH ovn] lflow-cache: Automatically trim cache when it becomes inactive.

Re: [ovs-dev] [PATCH v3] dpif-netdev: Call cpuid for x86 isa availability.

[ovs-dev] [PATCH v4] checkpatch: Detect "trojan source" attack

[ovs-dev] [PATCH v2] checkpatch: Correct line count in error messages

[ovs-dev] [RFC PATCH 1/1] dpdk: Update to use DPDK v21.11.

Re: [ovs-dev] [PATCH] ovsdb-idl: Don't reparse orphaned rows.

Re: [ovs-dev] [PATCH] ovsdb-idl: Get per-database memory usage statistics.

Re: [ovs-dev] [PATCH ovn v4] ovn-controller: Avoid infinite replying for TCP/ICMP connection reset messages

Re: [ovs-dev] [PATCH] stream-ssl: Avoid unnecessary memory copies on send.

Re: [ovs-dev] [PATCH 0/2] ovsdb-data: Consolidate ovsdb atom and json strings.

Re: [ovs-dev] [PATCH 2/2] ovsdb-data: Consolidate ovsdb atom and json strings.

Re: [ovs-dev] [PATCH] ovsdb-idl: Get per-database memory usage statistics.

[ovs-dev] [PATCH v3 3/4] system-dpdk: Test with mlx5 devices.

[ovs-dev] [PATCH v3 4/4] tests: Move MFEX tests to dpif-netdev.

[ovs-dev] [PATCH v3 2/4] system-dpdk: Use dummy-pmd port for packet injection.

Re: [ovs-dev] [PATCH] ofproto: Fix resource usage explosion due to removal of large number of flows.

[ovs-dev] [PATCH v3 1/4] system-dpdk: Refactor common logs matching.

Re: [ovs-dev] [PATCH] ofproto: Fix resource usage explosion while processing bundled FLOW_MOD.

Re: [ovs-dev] [PATCH] tests/flowgen: Fix length field of 802.2 data link header.

Re: [ovs-dev] [PATCH ovn] controller: Add IDL memory reports.

[ovs-dev] [PATCH v3] dpif-netdev: Call cpuid for x86 isa availability.

Re: [ovs-dev] [PATCH ovn] controller/pinctrl: improve packet-in debuggability

Re: [ovs-dev] [PATCH dpdk-latest] acinclude: Enable -Werror by default

Re: [ovs-dev] [PATCH ovn] controller/pinctrl: improve packet-in debuggability

Re: [ovs-dev] [PATCH ovn] controller: Add IDL memory reports.

[ovs-dev] [PATCH] ovsdb-idl: Get per-database memory usage statistics.

Re: [ovs-dev] [PATCH dpdk-latest] acinclude: Enable -Werror by default

Re: [ovs-dev] [PATCH ovn v3] ovn-controller: Avoid infinite replying for TCP/ICMP connection reset messages

[ovs-dev] [PATCH ovn v4] ovn-controller: Avoid infinite replying for TCP/ICMP connection reset messages

35 matches

Site Navigation

Mail list logo

Footer information