date:20220629

Re: [ovs-dev] [PATCH ovn v2 0/6] MAC binding aging mechanism

2022-06-29 Thread Ales Musil

On Thu, Jun 30, 2022 at 6:58 AM Han Zhou  wrote:

>
>
> On Mon, Jun 27, 2022 at 11:55 PM Ales Musil  wrote:
> >
> > Hi,
> >
> > so I did the suggested test. Setup was HIV1 - ext0 and vm0, HIV2 ext1
> and vm1
> >
> > The networks were connected as follow:
> > - vm0 and vm1 on the same switch
> > - logical router connected with the "internal" and "external" switch
> > - "external" switch connected to ext0 and ext1 through localnet
> >
> > So the traffic was flowing:
> > vmX -- LR -- localnet -- extX
> >
> > The iperf was running between vm0 - ext1 and vm1 - ext0.
> >
> > I have removed the MAC binding for ext0 multiple times to see if it
> affects the other traffic.
> > And it actually does not, which is great.
> >
> > iperf output from vm0 - ext1:
> > [ ID] Interval   Transfer Bitrate Retr  Cwnd
> > [  5]   0.00-1.00   sec  12.0 MBytes   101 Mbits/sec0290 KBytes
>
> > [  5]   1.00-2.00   sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]   2.00-3.00   sec  12.0 MBytes   101 Mbits/sec0290 KBytes
>
> > [  5]   3.00-4.00   sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]   4.00-5.00   sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]   5.00-6.00   sec  12.0 MBytes   101 Mbits/sec0290 KBytes
>
> > [  5]   6.00-7.00   sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]   7.00-8.00   sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]   8.00-9.00   sec  12.0 MBytes   101 Mbits/sec0290 KBytes
>
> > [  5]   9.00-10.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  10.00-11.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  11.00-12.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes
>
> > [  5]  12.00-13.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  13.00-14.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes
>
> > [  5]  14.00-15.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  15.00-16.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  16.00-17.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes
>
> > [  5]  17.00-18.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  18.00-19.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  19.00-20.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes
>
> > [  5]  20.00-21.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  21.00-22.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  22.00-23.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes
>
> > [  5]  23.00-24.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  24.00-25.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes
>
> > [  5]  25.00-26.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  26.00-27.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  27.00-28.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes
>
> > [  5]  28.00-29.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  29.00-30.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  30.00-31.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes
>
> > [  5]  31.00-32.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  32.00-33.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes
>
> > [  5]  33.00-34.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  34.00-35.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  35.00-36.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes
>
> > [  5]  36.00-37.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  37.00-38.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  38.00-39.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes
>
> > [  5]  39.00-40.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  40.00-41.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  41.00-42.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes
>
> > [  5]  42.00-43.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  43.00-44.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes
>
> > [  5]  44.00-45.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  45.00-46.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  46.00-47.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes
>
> > [  5]  47.00-48.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  48.00-49.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  49.00-50.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes
>
> > [  5]  50.00-51.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  51.00-52.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes
>
> > [  5]  52.00-53.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes
>
> > [  5]  53.00-54.00  sec  11.9 MByte

Re: [ovs-dev] [PATCH ovn v2 0/6] MAC binding aging mechanism

2022-06-29 Thread Han Zhou

On Mon, Jun 27, 2022 at 11:55 PM Ales Musil  wrote:
>
> Hi,
>
> so I did the suggested test. Setup was HIV1 - ext0 and vm0, HIV2 ext1 and
vm1
>
> The networks were connected as follow:
> - vm0 and vm1 on the same switch
> - logical router connected with the "internal" and "external" switch
> - "external" switch connected to ext0 and ext1 through localnet
>
> So the traffic was flowing:
> vmX -- LR -- localnet -- extX
>
> The iperf was running between vm0 - ext1 and vm1 - ext0.
>
> I have removed the MAC binding for ext0 multiple times to see if it
affects the other traffic.
> And it actually does not, which is great.
>
> iperf output from vm0 - ext1:
> [ ID] Interval   Transfer Bitrate Retr  Cwnd
> [  5]   0.00-1.00   sec  12.0 MBytes   101 Mbits/sec0290 KBytes

> [  5]   1.00-2.00   sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]   2.00-3.00   sec  12.0 MBytes   101 Mbits/sec0290 KBytes

> [  5]   3.00-4.00   sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]   4.00-5.00   sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]   5.00-6.00   sec  12.0 MBytes   101 Mbits/sec0290 KBytes

> [  5]   6.00-7.00   sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]   7.00-8.00   sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]   8.00-9.00   sec  12.0 MBytes   101 Mbits/sec0290 KBytes

> [  5]   9.00-10.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  10.00-11.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  11.00-12.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

> [  5]  12.00-13.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  13.00-14.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

> [  5]  14.00-15.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  15.00-16.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  16.00-17.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

> [  5]  17.00-18.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  18.00-19.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  19.00-20.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

> [  5]  20.00-21.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  21.00-22.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  22.00-23.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

> [  5]  23.00-24.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  24.00-25.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

> [  5]  25.00-26.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  26.00-27.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  27.00-28.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

> [  5]  28.00-29.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  29.00-30.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  30.00-31.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

> [  5]  31.00-32.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  32.00-33.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

> [  5]  33.00-34.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  34.00-35.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  35.00-36.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

> [  5]  36.00-37.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  37.00-38.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  38.00-39.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

> [  5]  39.00-40.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  40.00-41.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  41.00-42.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

> [  5]  42.00-43.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  43.00-44.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

> [  5]  44.00-45.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  45.00-46.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  46.00-47.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

> [  5]  47.00-48.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  48.00-49.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  49.00-50.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

> [  5]  50.00-51.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  51.00-52.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

> [  5]  52.00-53.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  53.00-54.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  54.00-55.00  sec  12.0 MBytes   101 Mbits/sec0290 KBytes

> [  5]  55.00-56.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

> [  5]  56.00-57.00  sec  11.9 MBytes  99.6 Mbits/sec0290 KBytes

[ovs-dev] Fwd: |fail| pw1650288 [PATCH ovn] Use _[add, del]value to manage additional_[chassis, encap]

2022-06-29 Thread Ihar Hrachyshka

Hi,

[sorry if this is the wrong venue.]

I've noticed this failure in my local environment and thought that's
some issue with my environment, but now I see it in the gate too.

You can find a link to the failure below, but in general it goes as:

something in a test calls a ovs/ovn CLI command with AT_CHECK macro.
The command succeeds. But it also produces a log warning like:

+2022-06-29T17:31:47Z|1|timeval|WARN|Unreasonably long 70035ms
poll interval (1ms user, 659ms system)
+2022-06-29T17:31:48Z|2|timeval|WARN|faults: 155 minor, 2214 major
+2022-06-29T17:31:48Z|3|timeval|WARN|disk: 484520 reads, 0 writes
+2022-06-29T17:31:48Z|4|timeval|WARN|context switches: 2282
voluntary, 2167 involuntary

And because AT_CHECK expects an empty output, this triggers a failure
for the whole test case.

Any ideas what could cause this? Perhaps there's a pending patch that
handles the issue that I couldn't find?

Just thought at least bringing attention to the failure is a good idea...

Ihar


-- Forwarded message -
From: 0-day Robot 
Date: Wed, Jun 29, 2022 at 3:00 PM
Subject: |fail| pw1650288 [ovs-dev] [PATCH ovn] Use _[add, del]value
to manage additional_[chassis, encap]
To: 
Cc: , 


From: ro...@bytheb.org

Test-Label: github-robot: Build and Test
Test-Status: fail
http://patchwork.ozlabs.org/api/patches/1650288/

_github build: failed_
Build URL: https://github.com/ovsrobot/ovn/actions/runs/2584602580
Build Logs:
---Summary of failed steps---
"linux clang test asan" failed at step build
--End summary of failed steps

---BEGIN LOGS

 [Begin job log] "linux clang test asan" at step build

| #define HAVE_LINUX_IF_ETHER_H 1
| #define HAVE_LINUX_NET_NAMESPACE_H 1
| #define HAVE_STDATOMIC_H 1
| #define HAVE_BITS_FLOATN_COMMON_H 1
| #define HAVE_BACKTRACE 1
| #define HAVE_LINUX_PERF_EVENT_H 1
| #define HAVE_THREAD_LOCAL 1
| #define HAVE_GCC4_ATOMICS 1
| #define ATOMIC_ALWAYS_LOCK_FREE_1B 1
| #define ATOMIC_ALWAYS_LOCK_FREE_2B 1
| #define ATOMIC_ALWAYS_LOCK_FREE_4B 1
| #define ATOMIC_ALWAYS_LOCK_FREE_8B 1
| #define HAVE_GLIBC_PTHREAD_SETNAME_NP 1
| #define HAVE_CXX11 1
| #define HAVE_ATOMIC 1
| #define HAVE_POSIX_MEMALIGN 1
| #define HAVE_UNBOUND 1
| #define HAVE_STDIO_H 1
| #define HAVE_STRING_H 1
| #define HAVE_PRAGMA_MESSAGE 1
|
| configure: exit 0

+ exit 1
##[error]Process completed with exit code 1.

 [End job log] "linux clang test asan" at step build

END LOGS-

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

Re: [ovs-dev] [PATCH ovn] tests: fixed flaky test localnet connectivity with multiple requested-chassis

2022-06-29 Thread Ihar Hrachyshka

Thank you.

Acked-by: Ihar Hrachyshka 

On Wed, Jun 22, 2022 at 12:21 PM Xavier Simonart  wrote:
>
> The test was failing randomly (on highly loaded systems) mainly because
> the MAC address of the migrator port in the main switch was migrating at
> unexpected times.
> In addition, a few undefined variables were fixed, and sleep was removed /
> replaced by ovs_wait actions.
>
> Signed-off-by: Xavier Simonart 
> ---
>  tests/ovn.at | 22 --
>  1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index bfaa41962..12d1f8667 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -14691,10 +14691,10 @@ wait_column "" Port_Binding 
> requested_additional_chassis logical_port=migrator
>  wait_for_ports_up
>
>  # advertise location of ports through localnet port
> -send_garp hv1 migrator 00ff  $migrator_spa $migrator_tpa
> -send_garp hv1 first 0001  $first_spa $first_tpa
> -send_garp hv2 second 0002  $second_spa $second_tpa
> -send_garp hv3 third 0003  $third_spa $third_tpa
> +send_garp hv1 migrator 00ff  $migrator_tpa $migrator_tpa
> +send_garp hv1 first 0001  $first_spa $first_spa
> +send_garp hv2 second 0002  $second_spa $second_spa
> +send_garp hv3 third 0003  $third_spa $third_spa
>  reset_env
>
>  # check that...
> @@ -14840,6 +14840,12 @@ echo $request >> hv3/third.expected
>
>  check_packets
>
> +# Wait for MAC address of migrator to be on hv1 related port in main switch.
> +# Hence the MAC will not migrate back unexpectedly later.
> +p1=$(as main ovs-ofctl show n1 | grep hv1_br-phys | awk '{print int($1)}')
> +p2=$(as main ovs-ofctl show n1 | grep hv2_br-phys | awk '{print int($1)}')
> +OVS_WAIT_UNTIL([test x`as main ovs-appctl fdb/show n1 | grep 
> 00:00:00:00:00:ff  | awk '{print $1}'` = x$p1])
> +
>  # Complete migration: destination is bound
>  check ovn-nbctl lsp-set-options migrator requested-chassis=hv2
>  wait_column "$hv2_uuid" Port_Binding chassis logical_port=migrator
> @@ -14849,12 +14855,16 @@ wait_column "" Port_Binding 
> requested_additional_chassis logical_port=migrator
>  wait_for_ports_up
>
>  check ovn-nbctl --wait=hv sync
> -sleep 1
> +OVS_WAIT_UNTIL([test `as hv2 ovs-vsctl get Interface migrator 
> external_ids:ovn-installed` = '"true"'])
>
>  # advertise new location of the port through localnet port
> -send_garp hv2 migrator 00ff  $migrator_spa $migrator_tpa
> +send_garp hv2 migrator 00ff  $migrator_tpa $migrator_tpa
> +
>  reset_env
>
> +# Wait for MAC address of migrator to be on hv2 port in main switch
> +OVS_WAIT_UNTIL([test x`as main ovs-appctl fdb/show n1 | grep 
> 00:00:00:00:00:ff  | awk '{print $1}'` = x$p2])
> +
>  # check that...
>  # unicast from Third doesn't arrive to hv1:Migrator
>  # unicast from Third arrives to hv2:Migrator
> --
> 2.31.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

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

Re: [ovs-dev] [RFC PATCH 0/6] Remove OVS kernel driver

2022-06-29 Thread Ilya Maximets

On 6/10/22 02:31, Frode Nordahl wrote:
> On Fri, Jun 3, 2022 at 4:16 PM Frode Nordahl
>  wrote:
>>
>> On Thu, Jun 2, 2022 at 6:58 PM Ilya Maximets  wrote:
>>>
>>> On 6/1/22 22:53, Gregory Rose wrote:


 On 5/31/2022 12:22 PM, Frode Nordahl wrote:
> On Tue, May 31, 2022 at 7:05 PM Ilya Maximets  wrote:
>>
>> On 5/31/22 17:36, Gregory Rose wrote:
>>>
>>>
>>> On 5/25/2022 6:47 AM, Flavio Leitner wrote:

 Hi Greg,


 On Mon, May 23, 2022 at 09:10:36PM +0200, Ilya Maximets wrote:
> On 5/19/22 20:04, Gregory Rose wrote:
>>
>>
>> On 4/15/2022 2:42 PM, Greg Rose wrote:
>>> It is time to remove support for the OVS kernel driver and push
>>> towards use of the upstream Linux openvswitch kernel driver
>>> in it's place [1].
>>>
>>> This patch series represents a first attempt but there are a few
>>> primary remaining issues that I have yet to address.
>>>
>>> A) Removal of debian packing support for the dkms kernel driver
>>>   module. The debian/rules are not well known to me - I've never
>>>   actually made any changes in that area and do not have a
>>>   well formed understanding of how debian packaging works.  I 
>>> wil
>>>   attempt to fix that up in upcoming patch series.
>>> B) Figuring out how the github workflow - I removed the tests I
>>>   could find that depend on the Linux kernel (i.e. they use
>>>   install_kernel() function.  Several other tests are  failing
>>>   that would not seem to depend on the Linux kernel.  I need to
>>>   read and understand that code better.
>>> C) There are many Linux specific source modules in the datapath that
>>>   will need eventual removal but some headers are still 
>>> required for
>>>   the userspace code (which seems counterintuitive but...)
>>>
>>> Reviews, suggestions, etc. are appreciated!
>>>
>>> 1.  
>>> https://mail.openvswitch.org/pipermail/ovs-dev/2022-April/393292.html
>>
>> I would like to suggest at this time that rather than removing the 
>> OVS
>> Linux kernel path that we "freeze" it at Linux 5.8. This will make it
>> easier for some consumers of OVS that are continuing to support the
>> Linux kernel datapath in old distributions.
>>
>> The ultimate goal of shifting toward DPDK and AFXDP datapaths is 
>> still
>> preserved but we are placing less burden on some consumers of OVS for
>> older Linux distributions.
>>
>> Perhaps in suggesting removal of the kernel datapath I was being a 
>> bit
>> overly aggressive.
>>
>> Thoughts? Concerns? Other suggestions?
>
> Hi.  I think we discussed that before.  Removal from the master branch
> doesn't mean that we will stop supporting the kernel module 
> immediately.
> It will remain in branch 2.17 which will become our new LTS series 
> soon.
> This branch will be supported until 2025.  And we also talked about
> possibility of extending the support just for a kernel module on that
> branch, if required.  It's not necassary to use the kernel module and
> OVS form the same branch, obviously.
>
> Removal from the master branch will just make it possible to remove
> the maintenance burden eventually, not right away.
>
> And FWIW, the goal is not to force everyone to use userspace datapath,
> but remove a maintenance burden and push users to use a better 
> supported
> version of a code.  Frankly, we're not doing a great job supporting 
> the
> out-of-tree module these days.  It's getting hard to backport bug 
> fixes.
> And will be even harder over time since the code drifts away from the
> version in the upstream kernel.  Mainly because we're not backporting
> new features for a few years already.
>
> Does that make sense?

 Any thoughts on this? The freeze time is approaching, so it would
 be great to know your plans for this patch set.

 Thanks,
 fbl

>>>
>>> Hi Flavio and Ilya,
>>>
>>> I'll go ahead with the plans as per previous agreements - having issues
>>> with removing the debian kernel module support since I have never
>>> worked on debian rules type make environments.  I seem to break
>>> something with every attempt but I will keep at it.
>>>
>>> What's my time frame before the freeze?
>>
>> The "soft-freeze" supposed to be on July 1st.  The branch creation
>> for a new release - mid July.  It would be gre

Re: [ovs-dev] [PATCH] dpif-netdev: Fix leak of AVX512 DPIF scratch pad.

2022-06-29 Thread Ilya Maximets

On 6/29/22 15:27, Amber, Kumar wrote:
> Hi Ilya,
> 
> 
>> -Original Message-
>> From: Ilya Maximets 
>> Sent: Wednesday, June 29, 2022 5:55 PM
>> To: Eelco Chaudron 
>> Cc: i.maxim...@ovn.org; ovs-dev@openvswitch.org; Flavio Leitner
>> ; Amber, Kumar 
>> Subject: Re: [ovs-dev] [PATCH] dpif-netdev: Fix leak of AVX512 DPIF scratch
>> pad.
>>
>> On 6/29/22 13:45, Eelco Chaudron wrote:
>>>
>>>
>>> On 29 Jun 2022, at 12:22, Ilya Maximets wrote:
>>>
 dp_netdev_input_outer_avx512 allocates a 16KB scratch pad per PMD
 thread, but it's never freed.  This may cause significant memory
 drain in dynamic environments.

   ==4068109==ERROR: LeakSanitizer: detected memory leaks

   Direct leak of 38656 byte(s) in 2 object(s) allocated from:
0 0xf069fd in posix_memalign (vswitchd/ovs-vswitchd+0xf069fd)
1 0x1d7ed14 in xmalloc_size_align lib/util.c:254:13
2 0x1d7ed14 in xmalloc_pagealign lib/util.c:352:12
3 0x2098254 in dp_netdev_input_outer_avx512 lib/dpif-netdev-
>> avx512.c:69:17
4 0x191591a in dp_netdev_process_rxq_port lib/dpif-netdev.c:5332:19
5 0x190a961 in pmd_thread_main lib/dpif-netdev.c:6963:17
6 0x1c4b69a in ovsthread_wrapper lib/ovs-thread.c:422:12
7 0x7fd5ea6f1179 in start_thread pthread_create.c

  SUMMARY: AddressSanitizer: 38656 byte(s) leaked in 2 allocation(s).

 Fixes: 9ac84a1a3698 ("dpif-avx512: Add ISA implementation of dpif.")
 Signed-off-by: Ilya Maximets 
 ---
>>>
>>> Good catch... Do you have some script to run all kinds of sanitizers?
>>
>> No.  For this one I just built with
>>   CFLAGS="-msse4.2 -O1 -fno-omit-frame-pointer -fno-common -g -
>> fsanitize=address -fsanitize=undefined" CC=clang
>>
>> And then tried to run with asan environment variables:
>>
>>   ASAN_OPTIONS='detect_leaks=1:abort_on_error=true:log_path=asan'
>>   UBSAN_OPTIONS='print_stacktrace=1:halt_on_error=true:log_path=ubsan'
>>
>> LeakSanitizer is part of the AddressSanitizer, you just need to ask to detect
>> leaks (we're disabling that by default in tests/atlocal.in).
>>
>> I didn't check, but I guess, you should be able to reproduce the issue by 
>> just
>> building with Asan and running:
>>
>>   ASAN_OPTIONS='detect_leaks=1:abort_on_error=true:log_path=asan' make
>> check-dpdk
>>
>>>
>>>
>>> Acked-by: Eelco Chaudron 
>>>
> Acked-by: Kumar Amber 
> 
> The changes look good to me, and Thanks again for fixing it.

Thanks, everyone!  Applied and backported down to 2.16.

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

Re: [ovs-dev] [PATCH v4] ofproto-dpif-xlate: No clone when tunnel push is last action

2022-06-29 Thread Ilya Maximets

On 6/29/22 18:28, Rosemarie O'Riorden wrote:
> Hi Ilya,
> 
> 
> Thank you for reviewing my patch and suggesting a fix. It looks great to
> me. You can do as you said, and apply the changes to my patch.

Thanks for checking!  Applied now.

Best regards, Ilya Maximets.

> 
> 
> Rosemarie O'Riorden
> 
> On 6/28/22 07:06, Ilya Maximets wrote:
> 
>> On 6/3/22 17:31, Rosemarie O'Riorden wrote:
>>> When OVS sees a tunnel push with a nested list next, it will not
>>> clone the packet, as a clone is not needed. However, a clone action will
>>> still be created with the tunnel push encapulated inside. There is no
>>> need to create the clone action in this case, as extra parsing will need
>>> to be performed, which is less efficient.
>>>
>>> Signed-off-by: Rosemarie O'Riorden 
>>> ---
>>> v2:
>>>  - Fixes tests that are affected by the change
>>> v3:
>>>  - Adds support for hardware offloading
>>> v4:
>>>  - Adds negative check in test
>>>  - Fixes logic in function native_tunnel_output
>>>  - Stylistic and organizational improvements
>>>
>>>  lib/netdev-offload-dpdk.c | 38 +-
>>>  lib/netlink.c |  7 +++
>>>  lib/netlink.h |  1 +
>>>  ofproto/ofproto-dpif-xlate.c  | 22 ++--
>>>  tests/nsh.at  |  6 +--
>>>  tests/packet-type-aware.at| 24 -
>>>  tests/tunnel-push-pop-ipv6.at | 20 +++
>>>  tests/tunnel-push-pop.at  | 98 ---
>>>  8 files changed, 152 insertions(+), 64 deletions(-)
>>>
>> 
>>
>>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>>> index 9ea21edc4..a908160a5 100644
>>> --- a/ofproto/ofproto-dpif-xlate.c
>>> +++ b/ofproto/ofproto-dpif-xlate.c
>>> @@ -3739,7 +3739,11 @@ native_tunnel_output(struct xlate_ctx *ctx, const 
>>> struct xport *xport,
>>>  size_t clone_ofs = 0;
>>>  size_t push_action_size;
>>>  
>>> -clone_ofs = nl_msg_start_nested(ctx->odp_actions, 
>>> OVS_ACTION_ATTR_CLONE);
>>> +if (!is_last_action) {
>>> +clone_ofs = nl_msg_start_nested(ctx->odp_actions,
>>> +OVS_ACTION_ATTR_CLONE);
>>> +}
>>> +size_t pre_patch_port_outp_size = ctx->odp_actions->size;
>> Hi, Rosemarie.  Thanks for addressing previous comments!
>> This version looks correct to me.  I also did some tests
>> with dummy offload and they seem to work fine.
>>
>> The only thing I think we can improve is the variable name
>> here.  I think, it will be a bit cleaner if we will rename
>> and re-use the same variable for the offset in both cases.
>> Maybe something like this:
>>
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index 0c92ed922..c645c3470 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -3736,14 +3736,12 @@ native_tunnel_output(struct xlate_ctx *ctx, const 
>> struct xport *xport,
>>s_ip, tnl_params.is_ipv6,
>>tnl_push_data.tnl_type);
>>  
>> -size_t clone_ofs = 0;
>> +size_t offset;
>>  size_t push_action_size;
>>  
>> -if (!is_last_action) {
>> -clone_ofs = nl_msg_start_nested(ctx->odp_actions,
>> -OVS_ACTION_ATTR_CLONE);
>> -}
>> -size_t pre_patch_port_outp_size = ctx->odp_actions->size;
>> +offset = is_last_action
>> + ? ctx->odp_actions->size
>> + : nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_CLONE);
>>  odp_put_tnl_push_action(ctx->odp_actions, &tnl_push_data);
>>  push_action_size = ctx->odp_actions->size;
>>  
>> ---
>>
>> What do you think?
>>
>> If that looks good to you, I can just apply the diff above and
>> rename the variable in other places while applying the change.
>>
>> Best regards, Ilya Maximets.
>>
> 

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

Re: [ovs-dev] [PATCH ovn] Use _[add, del]value to manage additional_[chassis, encap]

2022-06-29 Thread Mark Michelson


Wow, nice improvement, Ihar!

Acked-by: Mark Michelson 

On 6/29/22 12:47, Ihar Hrachyshka wrote:

Signed-off-by: Ihar Hrachyshka 
---
  controller/binding.c | 63 +---
  1 file changed, 6 insertions(+), 57 deletions(-)

diff --git a/controller/binding.c b/controller/binding.c
index 2279570f9..3beafdb0d 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -938,39 +938,17 @@ update_port_encap_if_needed(const struct 
sbrec_port_binding *pb,
  return true;
  }
  
-static void

-append_additional_encap(const struct sbrec_port_binding *pb,
-const struct sbrec_encap *encap)
-{
-struct sbrec_encap **additional_encap = xmalloc(
-(pb->n_additional_encap + 1) * (sizeof *additional_encap));
-if (pb->n_additional_encap) {
-memcpy(additional_encap, pb->additional_encap,
-   pb->n_additional_encap * (sizeof *additional_encap));
-}
-additional_encap[pb->n_additional_encap] = (struct sbrec_encap *) encap;
-sbrec_port_binding_set_additional_encap(
-pb, additional_encap, pb->n_additional_encap + 1);
-free(additional_encap);
-}
-
  static void
  remove_additional_encap_for_chassis(const struct sbrec_port_binding *pb,
  const struct sbrec_chassis *chassis_rec)
  {
-struct sbrec_encap **additional_encap = xmalloc(
-pb->n_additional_encap * (sizeof *additional_encap));
-
-size_t idx = 0;
  for (size_t i = 0; i < pb->n_additional_encap; i++) {
  if (!strcmp(pb->additional_encap[i]->chassis_name,
  chassis_rec->name)) {
-continue;
+sbrec_port_binding_update_additional_encap_delvalue(
+pb, pb->additional_encap[i]);
  }
-additional_encap[idx++] = pb->additional_encap[i];
  }
-sbrec_port_binding_set_additional_encap(pb, additional_encap, idx);
-free(additional_encap);
  }
  
  static bool

@@ -991,7 +969,7 @@ update_port_additional_encap_if_needed(
  if (sb_readonly) {
  return false;
  }
-append_additional_encap(pb, encap_rec);
+sbrec_port_binding_update_additional_encap_addvalue(pb, encap_rec);
  }
  return true;
  }
@@ -1008,40 +986,11 @@ is_additional_chassis(const struct sbrec_port_binding 
*pb,
  return false;
  }
  
-static void

-append_additional_chassis(const struct sbrec_port_binding *pb,
-  const struct sbrec_chassis *chassis_rec)
-{
-struct sbrec_chassis **additional_chassis = xmalloc(
-(pb->n_additional_chassis + 1) * (sizeof *additional_chassis));
-if (pb->n_additional_chassis) {
-memcpy(additional_chassis, pb->additional_chassis,
-   pb->n_additional_chassis * (sizeof *additional_chassis));
-}
-additional_chassis[pb->n_additional_chassis] = (
-(struct sbrec_chassis *) chassis_rec);
-sbrec_port_binding_set_additional_chassis(
-pb, additional_chassis, pb->n_additional_chassis + 1);
-free(additional_chassis);
-}
-
  static void
  remove_additional_chassis(const struct sbrec_port_binding *pb,
const struct sbrec_chassis *chassis_rec)
  {
-struct sbrec_chassis **additional_chassis = xmalloc(
-(pb->n_additional_chassis - 1) * (sizeof *additional_chassis));
-size_t idx = 0;
-for (size_t i = 0; i < pb->n_additional_chassis; i++) {
-if (pb->additional_chassis[i] == chassis_rec) {
-continue;
-}
-additional_chassis[idx++] = pb->additional_chassis[i];
-}
-sbrec_port_binding_set_additional_chassis(
-pb, additional_chassis, pb->n_additional_chassis - 1);
-free(additional_chassis);
-
+sbrec_port_binding_update_additional_chassis_delvalue(pb, chassis_rec);
  remove_additional_encap_for_chassis(pb, chassis_rec);
  }
  
@@ -1100,7 +1049,8 @@ claim_lport(const struct sbrec_port_binding *pb,

  VLOG_INFO("%s: Claiming %s", pb->logical_port, pb->mac[i]);
  }
  
-append_additional_chassis(pb, chassis_rec);

+sbrec_port_binding_update_additional_chassis_addvalue(pb,
+  chassis_rec);
  if (pb->chassis == chassis_rec) {
  sbrec_port_binding_set_chassis(pb, NULL);
  }
@@ -1901,7 +1851,6 @@ binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
  any_changes = true;
  }
  if (is_additional_chassis(binding_rec, chassis_rec)) {
-remove_additional_encap_for_chassis(binding_rec, chassis_rec);
  remove_additional_chassis(binding_rec, chassis_rec);
  any_changes = true;
  }



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

Re: [ovs-dev] [PATCH ovn] patch.c: Avoid patch interface deletion & recreation during restart.

2022-06-29 Thread Han Zhou

On Wed, Jun 29, 2022 at 1:37 AM Dumitru Ceara  wrote:
>
> On 6/28/22 19:18, Han Zhou wrote:
> > On Tue, Jun 28, 2022 at 8:20 AM Dumitru Ceara  wrote:
> >>
> >> On 6/28/22 02:49, Han Zhou wrote:
> >>> When ovn-controller is restarted, it may need multiple iterations of
> >>> main loop before completely download all related data from SB DB,
> >>> especially when ovn-monitor-all=false, so after restart, before it
sees
> >>> the related localnet ports from SB DB, it treats the related patch
> >>> ports on the chassis as not needed, and deleted them. Later when it
> >>> downloads thoses port-bindings it recreates them.  For a graceful
> >>> upgrade, we don't this to happen, because it would break the traffic.
> >>>
> >>> This is especially problematic at ovn-k8s setups because the external
> >>> side of the patch port is used to program some flows for external
> >>> network communication. When the patch ports are recreated, the OF port
> >>> number changes and those flows are broken. The CMS would detect and
fix
> >>> the flows but it would result in even longer downtime.
> >>>
> >>> This patch postpone the deletion of the patch ports, with the
assumption
> >>> that leaving the unused ports on the chassis for little longer is not
> >>> harmful.
> >>>
> >>> Signed-off-by: Han Zhou 
> >>> ---
> >>>  controller/patch.c  | 15 -
> >>>  tests/ovn-controller.at | 71
+
> >>>  2 files changed, 85 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/controller/patch.c b/controller/patch.c
> >>> index ed831302c..9faae5879 100644
> >>> --- a/controller/patch.c
> >>> +++ b/controller/patch.c
> >>> @@ -307,11 +307,24 @@ patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
> >>>
> >>>  /* Now 'existing_ports' only still contains patch ports that
exist
> > in the
> >>>   * database but shouldn't.  Delete them from the database. */
> >>> +
> >>> +/* Wait for some iterations before really deleting any patch
> > ports, because
> >>> + * with conditional monitoring it is possible that related SB
data
> > is not
> >>> + * completely downloaded yet after last restart of
ovn-controller.
> >>> + * Otherwise it may cause unncessary dataplane interruption
during
> >>> + * restart/upgrade. */
> >>> +static int delete_patch_port_delay = 10;
> >>
> >> Hi Han,
> >>
> >> It's possible that ovn-controller wakes up 10 times in a row
immediately
> >> due to local OVSDB changes and doesn't process any new SB updates in
> >> that time so 10 might not be enough in some cases.
> >>
> >
> > Thanks Dumitru for the review!
> > In theory I agree with you 10 times is not 100% guaranteeing SB update
> > completed if other things are triggering the wakeups. However, in
practice,
> > the purpose here is for the start/restart scenario. I think it is very
> > unlikely that local OVSDB is changing that frequently at the same time
when
> > ovn-controller is being restarted. We have some similar logic (not ideal
> > for sure) at vif-plug.c:vif_plug_run() for similar reasons.
> >
>
> Ah I didn't know we do that already for vif-plug.  Thanks for pointing
> it out!
>
> >> Does it make sense to wait a given amount of time instead?  E.g., a few
> >> seconds?  Can we make this configurable somehow?  Maybe an
> >> ovn-controller command line argument to override the default?
> >
> > Waiting for a given amount of time is also not ideal. It is possible
that
> > when ovn-controller starts the SB IDL is not connected (due to server
side
> > problems/ control plane network problems, etc.) so we don't know how
long
> > it should wait at all.
> >
> > I am ok with adding command line arguments to adjust, but I'd really
want
> > to avoid that unless it is proved to be necessary. I'd rather use a
bigger
> > hardcoded value to avoid another knob which is not easy to understand by
> > users - it should be something handled by the code itself.
> >
>
> I understand your point against additional knobs.  Maybe we don't need
> a new one.  What if we do a mixed approach?  We already have the
> ovn-controller startup timestamp so we could have a single (hardcoded)
> delay counter but also ensure that at least X seconds elapsed.  It might
> be a bit over-engineered but what do you think of the following
> incremental?
>

Thanks Dumitru. I am ok with adding a check for time passed in addition to
the iterations. The function daemon_started_recently() you added below
would have a problem because each caller of this function would trigger the
decrement of the controller_startup_delay, so if there are 10 callers it
would decrease to 0 with just one iteration. But the overall approach
demonstrated looks good. I will work on V2 with your idea incorporated.

In addition, it doesn't seem necessary to me for the vif_plug to reset the
counter when DB is reconnected, because I don't expect it to lose old data
in such cases - the monitor condition remains unchanged. So I think
dbs_connected_recently() is not really needed. I

Re: [ovs-dev] [PATCH ovn] tests: add multi-chassis keyword to relevant test cases

2022-06-29 Thread Mark Michelson


Hi Ihar, I acked and pushed this patch to main and branch-22.06.

On 6/24/22 01:14, Ihar Hrachyshka wrote:

Signed-off-by: Ihar Hrachyshka 
---
  tests/ovn.at | 7 +++
  1 file changed, 7 insertions(+)

diff --git a/tests/ovn.at b/tests/ovn.at
index bfaa41962..e2d1d993e 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -14012,6 +14012,7 @@ AT_CLEANUP
  
  OVN_FOR_EACH_NORTHD([

  AT_SETUP([options:multiple requested-chassis for logical port])
+AT_KEYWORDS([multi-chassis])
  ovn_start
  
  net_add n1

@@ -14101,6 +14102,7 @@ AT_CLEANUP
  
  OVN_FOR_EACH_NORTHD([

  AT_SETUP([options:multiple requested-chassis for logical port: change chassis 
role])
+AT_KEYWORDS([multi-chassis])
  ovn_start
  
  net_add n1

@@ -14150,6 +14152,7 @@ AT_CLEANUP
  
  OVN_FOR_EACH_NORTHD([

  AT_SETUP([options:multiple requested-chassis for logical port: unclaimed 
behavior])
+AT_KEYWORDS([multi-chassis])
  ovn_start
  
  net_add n1

@@ -14230,6 +14233,7 @@ AT_CLEANUP
  
  OVN_FOR_EACH_NORTHD([

  AT_SETUP([basic connectivity with multiple requested-chassis])
+AT_KEYWORDS([multi-chassis])
  ovn_start
  
  net_add n1

@@ -14564,6 +14568,7 @@ AT_CLEANUP
  
  OVN_FOR_EACH_NORTHD([

  AT_SETUP([localnet connectivity with multiple requested-chassis])
+AT_KEYWORDS([multi-chassis])
  ovn_start
  
  net_add n1

@@ -14923,6 +14928,7 @@ AT_CLEANUP
  
  OVN_FOR_EACH_NORTHD([

  AT_SETUP([options:activation-strategy for logical port])
+AT_KEYWORDS([multi-chassis])
  ovn_start
  
  net_add n1

@@ -15132,6 +15138,7 @@ AT_CLEANUP
  
  OVN_FOR_EACH_NORTHD([

  AT_SETUP([options:activation-strategy=rarp is not waiting for southbound db])
+AT_KEYWORDS([multi-chassis])
  # unskip when ovn-controller is able to process incremental updates to flow
  # table without ovsdb transaction available
  AT_SKIP_IF([true])



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

Re: [ovs-dev] [PATCH v2 ovn] northd: add condition for stateless nat drop flow in S_ROUTER_IN_GW_REDIRECT pipeline

2022-06-29 Thread Mark Michelson


I applied this patch to main, branch-22.06, branch-22.03, and branch-21.12.

On 6/27/22 16:30, Mark Michelson wrote:

Thanks for the update, Lorenzo.

Acked-by: Mark Michelson 

On 6/24/22 08:38, Lorenzo Bianconi wrote:
Match the drop flow for stateless dnat_and_snat flow in 
S_ROUTER_IN_GW_REDIRECT

stage just if allowed_ext_ips or exempted_ext_ips conditions do not
match since it breaks the hairpin scenario with stateless nat.
Fix the northd documentation.

Fixes: c0224a14f ("northd: fix stateless nat with allowed_ext_ips")
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2094980
Signed-off-by: Lorenzo Bianconi 
---
Change since v1:
- fix lflow for exempted_ext_ips in S_ROUTER_IN_GW_REDIRECT stage
---
  northd/northd.c | 47 +
  northd/ovn-northd.8.xml | 28 +++-
  tests/system-ovn.at | 15 +
  3 files changed, 72 insertions(+), 18 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 6997c280c..a9d3b5285 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -12072,6 +12072,7 @@ build_gateway_redirect_flows_for_lrouter(
  }
  for (size_t i = 0; i < od->n_l3dgw_ports; i++) {
  const struct ovsdb_idl_row *stage_hint = NULL;
+    bool add_def_flow = true;
  if (od->l3dgw_ports[i]->nbrp) {
  stage_hint = &od->l3dgw_ports[i]->nbrp->header_;
@@ -12090,22 +12091,42 @@ build_gateway_redirect_flows_for_lrouter(
  ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_GW_REDIRECT, 
50,

  ds_cstr(match), ds_cstr(actions),
  stage_hint);
-    }
+    for (int j = 0; j < od->n_nat_entries; j++) {
+    const struct ovn_nat *nat = &od->nat_entries[j];
-    for (int i = 0; i < od->n_nat_entries; i++) {
-    const struct ovn_nat *nat = &od->nat_entries[i];
+    if (!lrouter_nat_is_stateless(nat->nb) ||
+    strcmp(nat->nb->type, "dnat_and_snat") ||
+    (!nat->nb->allowed_ext_ips && 
!nat->nb->exempted_ext_ips)) {

+    continue;
+    }
-    if (!lrouter_nat_is_stateless(nat->nb) ||
-    strcmp(nat->nb->type, "dnat_and_snat")) {
-   continue;
-    }
+    struct ds match_ext = DS_EMPTY_INITIALIZER;
+    struct nbrec_address_set  *as = nat->nb->allowed_ext_ips
+    ? nat->nb->allowed_ext_ips : nat->nb->exempted_ext_ips;
+    ds_put_format(&match_ext, "%s && ip%s.src == $%s",
+  ds_cstr(match), nat_entry_is_v6(nat) ? "6" 
: "4",

+  as->name);
-    ds_clear(match);
-    ds_put_format(match, "ip && ip%s.dst == %s",
-  nat_entry_is_v6(nat) ? "6" : "4",
-  nat->nb->external_ip);
-    ovn_lflow_add(lflows, od, S_ROUTER_IN_GW_REDIRECT, 100,
-  ds_cstr(match), "drop;");
+    if (nat->nb->allowed_ext_ips) {
+    ovn_lflow_add_with_hint(lflows, od, 
S_ROUTER_IN_GW_REDIRECT,

+    75, ds_cstr(&match_ext),
+    ds_cstr(actions), stage_hint);
+    if (add_def_flow) {
+    ds_clear(&match_ext);
+    ds_put_format(&match_ext, "ip && ip%s.dst == %s",
+  nat_entry_is_v6(nat) ? "6" : "4",
+  nat->nb->external_ip);
+    ovn_lflow_add(lflows, od, 
S_ROUTER_IN_GW_REDIRECT, 70,

+  ds_cstr(&match_ext), "drop;");
+    add_def_flow = false;
+    }
+    } else if (nat->nb->exempted_ext_ips) {
+    ovn_lflow_add_with_hint(lflows, od, 
S_ROUTER_IN_GW_REDIRECT,
+    75, ds_cstr(&match_ext), 
"drop;",

+    stage_hint);
+    }
+    ds_destroy(&match_ext);
+    }
  }
  /* Packets are allowed by default. */
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 59c584710..06773eee4 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -3028,8 +3028,7 @@ icmp6 {
    ip && ip6.dst == B
    with an action ct_snat; . If the NAT rule is 
of type

    dnat_and_snat and has stateless=true in the
-  options, then the action would be ip4/6.dst=
-  (B).
+  options, then the action would be next;.
  
  
@@ -3069,7 +3068,7 @@ icmp6 {
    action ct_snat_in_czone; to unSNAT in the 
common

    zone.  If the NAT rule is of type dnat_and_snat and has
    stateless=true in the options, then the 
action

-  would be ip4/6.dst=(B).
+  would be next;.
  
  
@@ -4227,6 +4226,26 @@ icmp6 {
  external ip and D is NAT external mac.
    
+  
+

Re: [ovs-dev] [ovs-discuss] Commit 355fef6f2 seems to break connectivity in my setup

2022-06-29 Thread Marcelo Leitner

Hi,

On Tue, Jun 14, 2022 at 05:26:25PM +0300, Oz Shlomo wrote:
> Hi Ilya,
>
> On 6/14/2022 4:03 PM, Ilya Maximets wrote:
> > On 6/14/22 10:27, Oz Shlomo via dev wrote:
> > >
> > >
> > > On 6/8/2022 3:16 AM, Frode Nordahl wrote:
> > > > On Tue, Jun 7, 2022 at 12:16 AM Ilya Maximets  
> > > > wrote:
> > > > >
> > > > > On 5/31/22 23:48, Ilya Maximets wrote:
> > > > > > On 5/31/22 21:15, Frode Nordahl wrote:
> > > > > > > On Mon, May 30, 2022 at 5:25 PM Frode Nordahl
> > > > >
> > > > > 
> > > > >
> > > > > > > I've pushed the first part of the fix here:
> > > > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fpipermail%2Fovs-dev%2F2022-May%2F394450.html&data=05%7C01%7Cozsh%40nvidia.com%7C7ab5490f9c334e9d877f08da4e0652fc%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637908086346878848%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=pq0CABjao2UWojg6yZut7RL%2FZEeuRou0qUVZKNYP3rQ%3D&reserved=0
> > > > > >
> > > > > > Thanks!  I saw that and I tend to think that it is correct.
> > > > > > I'll try to test it and apply in the next couple of days.
> > > > > >
> > > > > > One question about the test above: which entity actually adds
> > > > > > the ct_state to the packet or at which moment that happens?
> > > > > > I see it, but I'm not sure I fully understand that.  Looks
> > > > > > like I'm missing smething obvious.
> > > > >
> > > > > I found what is going on there - kernel simply tracks everything
> > > > > if not told to do so, so ICMP packets create the ct entry and
> > > > > subsequent packets re-use it, so icmp replies have +trk set while
> > > > > entering OVS.
> > > >
> > > > Great, my hunch was that something along these lines was happening as
> > > > well, I have to admit the test case was found by locating something
> > > > closest to the real life use case and it proved to work as a good test
> > > > for this condition.
> > > >
> > > > > 
> > > > >
> > > > > Let's have some summary of the issues discovered here so far,
> > > > > including a few new issues:
> > > > >
> > > > > 1. ct states set externally are not tracked correctly by OVS.
> > > > >      Fix: 
> > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fpipermail%2Fovs-dev%2F2022-May%2F394450.html&data=05%7C01%7Cozsh%40nvidia.com%7C7ab5490f9c334e9d877f08da4e0652fc%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637908086346878848%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=pq0CABjao2UWojg6yZut7RL%2FZEeuRou0qUVZKNYP3rQ%3D&reserved=0
> > > > >      Status:  LGTM, will apply soon.
> > > > >      This fixes the problem originally reported by Liam, IIUC.  Right?
> > > >
> > > > Yes.
> > > >
> > > > > 2. Kernel ct() actions are trying to re-use the cached connection
> > > > >      after the tuple changes.
> > > > >      This ends up to be the OVN hairpin issue as reported here:
> > > > >    
> > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.launchpad.net%2Fubuntu%2F%2Bsource%2Fovn%2F%2Bbug%2F1967856&data=05%7C01%7Cozsh%40nvidia.com%7C7ab5490f9c334e9d877f08da4e0652fc%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637908086346878848%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=25B7VbtRFguupC7VoNjZK%2FWlasu%2BMSTUzJkszvEpDaQ%3D&reserved=0
> > > > >
> > > > >      Proposed Fix:
> > > > >    
> > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fnetdev%2F20220606221140.488984-1-i.maximets%40ovn.org%2FT%2F%23u&data=05%7C01%7Cozsh%40nvidia.com%7C7ab5490f9c334e9d877f08da4e0652fc%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637908086346878848%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=areRLYsAEbare7yo%2FxmIF9k2tMw2v8ZQkwHcR%2FEvV%2Bo%3D&reserved=0
> > > > >
> > > > >      Status: Needs review.
> > > >
> > > > I can confirm that the proposed fix resolves the OVN hairpin issue. It
> > > > also looks simple enough to be backportable all the way to where we
> > > > would need it (kernel 5.4.0). I'll have a look at giving this wider
> > > > exposure in an internal CI environment as a canary for any unintended
> > > > consequences if that would be helpful.
> > > >
> > >
> > > Sorry for jumping in late on this, as this patch was already accepted to 
> > > the kernel.
> > > However, I have a concern that this patch would break the tc datapath 
> > > when ovs hw offload is enabled.
> > >
> > > IIUC then this patch adds an implicit ovs_ct_clear call for 5-tuple 
> > > modify actions. However, this implicit action will not apply to flows 
> > > that use the tc datapath.
> > >
> > > Forde, can you verify that indeed this fix breaks the OVN hairpin use 
> > > case when hw offload is enabled.
> > Hi, Oz.  I don't think that this kernel fix

Re: [ovs-dev] [branch-2.16] dpif-netdev: Refactor AVX512 runtime checks.

2022-06-29 Thread Ilya Maximets

On 6/29/22 15:38, David Marchand wrote:
> [ original commit fe171e4f109f001f07b867756a261d898f0d2cfc ]
> 
> As described in the bugzilla below, cpu_has_isa code may be compiled
> with some AVX512 instructions in it, because cpu.c is built as part of
> the libopenvswitchavx512.
> This is a problem when this function (supposed to probe for AVX512
> instructions availability) is invoked from generic OVS code, on older
> CPUs that don't support them.
> 
> For the same reason, dpcls_subtable_avx512_gather_probe,
> dp_netdev_input_outer_avx512_probe, mfex_avx512_probe and
> mfex_avx512_vbmi_probe are potential runtime bombs and can't either be
> built as part of libopenvswitchavx512.
> 
> Move cpu.c to be part of the "normal" libopenvswitch.
> And move other helpers in generic OVS code.
> 
> Note:
> - dpcls_subtable_avx512_gather_probe is split in two, because it also
>   needs to do its own magic,
> - while moving those helpers, prefer direct calls to cpu_has_isa and
>   avoid cast to intermediate integer variables when a simple boolean
>   is enough,
> 
> Fixes: 352b6c7116cd ("dpif-lookup: add avx512 gather implementation.")
> Fixes: abb807e27dd4 ("dpif-netdev: Add command to switch dpif 
> implementation.")
> Fixes: 250ceddcc2d0 ("dpif-netdev/mfex: Add AVX512 based optimized miniflow 
> extract")
> Fixes: b366fa2f4947 ("dpif-netdev: Call cpuid for x86 isa availability.")
> Reported-at: https://bugzilla.redhat.com/2100393
> Reported-by: Ales Musil 
> Co-authored-by: Ales Musil 
> Signed-off-by: Ales Musil 
> Signed-off-by: David Marchand 
> ---

Thanks, David!
I applied all backports to appropriate branches.

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

Re: [ovs-dev] [PATCH v4] ovsdb idl: Add the support to specify the uuid for row insert.

2022-06-29 Thread 0-day Robot

Bleep bloop.  Greetings Numan Siddique, 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: Line is 118 characters long (recommended limit is 79)
#104 FILE: lib/db-ctl-base.man:206:
.IP "[\fB\-\-id=@\fIname\fR or \fB\-\-id=\fIuuid\fR] \fBcreate\fR \fItable 
column\fR[\fB:\fIkey\fR]\fB=\fIvalue\fR..."

Lines checked: 469, 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 v4] ovsdb idl: Add the support to specify the uuid for row insert.

2022-06-29 Thread numans

From: Numan Siddique 

ovsdb-server allows the OVSDB clients to specify the uuid for
the row inserts [1].  The C IDL client library is  missing this
feature.  This patch adds this support.

For each schema table, a new function is generated -
insert_persistent_uuid(txn, uuid) and the users
of IDL client library can make use of this function.

ovs-vsctl and other derivatives of ctl now supports the same
in the generic 'create' command with the option "--id=".

[1] - a529e3cd1f("ovsdb-server: Allow OVSDB clients to specify the UUID for 
inserted rows.:)

Signed-off-by: Numan Siddique 
Acked-by: Adrian Moreno 
Acked-by: Han Zhou 
---

v3 -> v4
---
  * Added an entry in python/TODO.rst.

v2 -> v3

  * Addressed review comments from Han
  - Added test case for --id ctl option

v1 -> v2
-
  * Addressed review comments from Adrian Moreno
  * Added the support in generic 'create' command to specify the uuid in
--id option.


 lib/db-ctl-base.c| 38 --
 lib/db-ctl-base.man  |  5 ++-
 lib/db-ctl-base.xml  |  4 ++
 lib/ovsdb-idl-provider.h |  1 +
 lib/ovsdb-idl.c  | 86 +---
 lib/ovsdb-idl.h  |  3 ++
 ovsdb/ovsdb-idlc.in  | 15 +++
 python/TODO.rst  |  2 +
 tests/ovs-vsctl.at   | 25 
 tests/ovsdb-idl.at   | 27 +
 tests/test-ovsdb.c   | 59 +++
 11 files changed, 229 insertions(+), 36 deletions(-)

diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
index 707456158..f39e090a0 100644
--- a/lib/db-ctl-base.c
+++ b/lib/db-ctl-base.c
@@ -1732,28 +1732,42 @@ cmd_create(struct ctl_context *ctx)
 const struct ovsdb_idl_row *row;
 const struct uuid *uuid = NULL;
 int i;
+struct uuid uuid_;
+bool persist_uuid = false;
 
 ctx->error = get_table(table_name, &table);
 if (ctx->error) {
 return;
 }
+
 if (id) {
-struct ovsdb_symbol *symbol = NULL;
+if (uuid_from_string(&uuid_, id)) {
+uuid = &uuid_;
+persist_uuid = true;
+} else {
+struct ovsdb_symbol *symbol = NULL;
 
-ctx->error = create_symbol(ctx->symtab, id, &symbol, NULL);
-if (ctx->error) {
-return;
-}
-if (table->is_root) {
-/* This table is in the root set, meaning that rows created in it
- * won't disappear even if they are unreferenced, so disable
- * warnings about that by pretending that there is a reference. */
-symbol->strong_ref = true;
+ctx->error = create_symbol(ctx->symtab, id, &symbol, NULL);
+if (ctx->error) {
+return;
+}
+if (table->is_root) {
+/* This table is in the root set, meaning that rows created in
+* it won't disappear even if they are unreferenced, so disable
+* warnings about that by pretending that there is a
+* reference. */
+symbol->strong_ref = true;
+}
+uuid = &symbol->uuid;
 }
-uuid = &symbol->uuid;
 }
 
-row = ovsdb_idl_txn_insert(ctx->txn, table, uuid);
+if (persist_uuid) {
+row = ovsdb_idl_txn_insert_persist_uuid(ctx->txn, table, uuid);
+} else {
+row = ovsdb_idl_txn_insert(ctx->txn, table, uuid);
+}
+
 for (i = 2; i < ctx->argc; i++) {
 ctx->error = set_column(table, row, ctx->argv[i], ctx->symtab);
 if (ctx->error) {
diff --git a/lib/db-ctl-base.man b/lib/db-ctl-base.man
index 9c786f298..4dc165f94 100644
--- a/lib/db-ctl-base.man
+++ b/lib/db-ctl-base.man
@@ -203,7 +203,7 @@ Without \fB\-\-if-exists\fR, it is an error if \fIrecord\fR 
does not
 exist.  With \fB\-\-if-exists\fR, this command does nothing if
 \fIrecord\fR does not exist.
 .
-.IP "[\fB\-\-id=@\fIname\fR] \fBcreate\fR \fItable 
column\fR[\fB:\fIkey\fR]\fB=\fIvalue\fR..."
+.IP "[\fB\-\-id=@\fIname\fR or \fB\-\-id=\fIuuid\fR] \fBcreate\fR \fItable 
column\fR[\fB:\fIkey\fR]\fB=\fIvalue\fR..."
 Creates a new record in \fItable\fR and sets the initial values of
 each \fIcolumn\fR.  Columns not explicitly set will receive their
 default values.  Outputs the UUID of the new row.
@@ -212,6 +212,9 @@ If \fB@\fIname\fR is specified, then the UUID for the new 
row may be
 referred to by that name elsewhere in the same \fB\*(PN\fR
 invocation in contexts where a UUID is expected.  Such references may
 precede or follow the \fBcreate\fR command.
+.IP
+If a valid \fIuuid\fR is specified, then it is used as the UUID
+of the new row.
 .
 .RS
 .IP "Caution (ovs-vsctl as example)"
diff --git a/lib/db-ctl-base.xml b/lib/db-ctl-base.xml
index f6efe98ea..4f63aa7f7 100644
--- a/lib/db-ctl-base.xml
+++ b/lib/db-ctl-base.xml
@@ -323,6 +323,10 @@
 invocation in contexts where a UUID is expected.  Such references may
 precede or follow the create command.
   
+  
+If a vali

[ovs-dev] [PATCH ovn] Use _[add, del]value to manage additional_[chassis, encap]

2022-06-29 Thread Ihar Hrachyshka

Signed-off-by: Ihar Hrachyshka 
---
 controller/binding.c | 63 +---
 1 file changed, 6 insertions(+), 57 deletions(-)

diff --git a/controller/binding.c b/controller/binding.c
index 2279570f9..3beafdb0d 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -938,39 +938,17 @@ update_port_encap_if_needed(const struct 
sbrec_port_binding *pb,
 return true;
 }
 
-static void
-append_additional_encap(const struct sbrec_port_binding *pb,
-const struct sbrec_encap *encap)
-{
-struct sbrec_encap **additional_encap = xmalloc(
-(pb->n_additional_encap + 1) * (sizeof *additional_encap));
-if (pb->n_additional_encap) {
-memcpy(additional_encap, pb->additional_encap,
-   pb->n_additional_encap * (sizeof *additional_encap));
-}
-additional_encap[pb->n_additional_encap] = (struct sbrec_encap *) encap;
-sbrec_port_binding_set_additional_encap(
-pb, additional_encap, pb->n_additional_encap + 1);
-free(additional_encap);
-}
-
 static void
 remove_additional_encap_for_chassis(const struct sbrec_port_binding *pb,
 const struct sbrec_chassis *chassis_rec)
 {
-struct sbrec_encap **additional_encap = xmalloc(
-pb->n_additional_encap * (sizeof *additional_encap));
-
-size_t idx = 0;
 for (size_t i = 0; i < pb->n_additional_encap; i++) {
 if (!strcmp(pb->additional_encap[i]->chassis_name,
 chassis_rec->name)) {
-continue;
+sbrec_port_binding_update_additional_encap_delvalue(
+pb, pb->additional_encap[i]);
 }
-additional_encap[idx++] = pb->additional_encap[i];
 }
-sbrec_port_binding_set_additional_encap(pb, additional_encap, idx);
-free(additional_encap);
 }
 
 static bool
@@ -991,7 +969,7 @@ update_port_additional_encap_if_needed(
 if (sb_readonly) {
 return false;
 }
-append_additional_encap(pb, encap_rec);
+sbrec_port_binding_update_additional_encap_addvalue(pb, encap_rec);
 }
 return true;
 }
@@ -1008,40 +986,11 @@ is_additional_chassis(const struct sbrec_port_binding 
*pb,
 return false;
 }
 
-static void
-append_additional_chassis(const struct sbrec_port_binding *pb,
-  const struct sbrec_chassis *chassis_rec)
-{
-struct sbrec_chassis **additional_chassis = xmalloc(
-(pb->n_additional_chassis + 1) * (sizeof *additional_chassis));
-if (pb->n_additional_chassis) {
-memcpy(additional_chassis, pb->additional_chassis,
-   pb->n_additional_chassis * (sizeof *additional_chassis));
-}
-additional_chassis[pb->n_additional_chassis] = (
-(struct sbrec_chassis *) chassis_rec);
-sbrec_port_binding_set_additional_chassis(
-pb, additional_chassis, pb->n_additional_chassis + 1);
-free(additional_chassis);
-}
-
 static void
 remove_additional_chassis(const struct sbrec_port_binding *pb,
   const struct sbrec_chassis *chassis_rec)
 {
-struct sbrec_chassis **additional_chassis = xmalloc(
-(pb->n_additional_chassis - 1) * (sizeof *additional_chassis));
-size_t idx = 0;
-for (size_t i = 0; i < pb->n_additional_chassis; i++) {
-if (pb->additional_chassis[i] == chassis_rec) {
-continue;
-}
-additional_chassis[idx++] = pb->additional_chassis[i];
-}
-sbrec_port_binding_set_additional_chassis(
-pb, additional_chassis, pb->n_additional_chassis - 1);
-free(additional_chassis);
-
+sbrec_port_binding_update_additional_chassis_delvalue(pb, chassis_rec);
 remove_additional_encap_for_chassis(pb, chassis_rec);
 }
 
@@ -1100,7 +1049,8 @@ claim_lport(const struct sbrec_port_binding *pb,
 VLOG_INFO("%s: Claiming %s", pb->logical_port, pb->mac[i]);
 }
 
-append_additional_chassis(pb, chassis_rec);
+sbrec_port_binding_update_additional_chassis_addvalue(pb,
+  chassis_rec);
 if (pb->chassis == chassis_rec) {
 sbrec_port_binding_set_chassis(pb, NULL);
 }
@@ -1901,7 +1851,6 @@ binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
 any_changes = true;
 }
 if (is_additional_chassis(binding_rec, chassis_rec)) {
-remove_additional_encap_for_chassis(binding_rec, chassis_rec);
 remove_additional_chassis(binding_rec, chassis_rec);
 any_changes = true;
 }
-- 
2.25.1

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

Re: [ovs-dev] [PATCH ovn] ovn-northd.at: Add OVN_FOR_EACH_NORTHD around LR NB Static_MAC_Binding table

2022-06-29 Thread Numan Siddique

On Wed, Jun 29, 2022 at 8:30 AM Dumitru Ceara  wrote:
>
> On 6/29/22 07:47, Ales Musil wrote:
> > This is continuation after discussion during review on
> > 80187a803 (ovn-northd: Add flow to use eth.src if nd.tll is 0
> > in put_nd() action.)
> >
> > Signed-off-by: Ales Musil 
> > ---
>
> Looks good to me, thanks!
>
> Acked-by: Dumitru Ceara 

Thanks.  Applied to the main branch.

Numan

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

Re: [ovs-dev] [PATCH v4] ofproto-dpif-xlate: No clone when tunnel push is last action

2022-06-29 Thread Rosemarie O'Riorden

Hi Ilya,


Thank you for reviewing my patch and suggesting a fix. It looks great to
me. You can do as you said, and apply the changes to my patch.


Rosemarie O'Riorden

On 6/28/22 07:06, Ilya Maximets wrote:

> On 6/3/22 17:31, Rosemarie O'Riorden wrote:
>> When OVS sees a tunnel push with a nested list next, it will not
>> clone the packet, as a clone is not needed. However, a clone action will
>> still be created with the tunnel push encapulated inside. There is no
>> need to create the clone action in this case, as extra parsing will need
>> to be performed, which is less efficient.
>>
>> Signed-off-by: Rosemarie O'Riorden 
>> ---
>> v2:
>>  - Fixes tests that are affected by the change
>> v3:
>>  - Adds support for hardware offloading
>> v4:
>>  - Adds negative check in test
>>  - Fixes logic in function native_tunnel_output
>>  - Stylistic and organizational improvements
>>
>>  lib/netdev-offload-dpdk.c | 38 +-
>>  lib/netlink.c |  7 +++
>>  lib/netlink.h |  1 +
>>  ofproto/ofproto-dpif-xlate.c  | 22 ++--
>>  tests/nsh.at  |  6 +--
>>  tests/packet-type-aware.at| 24 -
>>  tests/tunnel-push-pop-ipv6.at | 20 +++
>>  tests/tunnel-push-pop.at  | 98 ---
>>  8 files changed, 152 insertions(+), 64 deletions(-)
>>
> 
>
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index 9ea21edc4..a908160a5 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -3739,7 +3739,11 @@ native_tunnel_output(struct xlate_ctx *ctx, const 
>> struct xport *xport,
>>  size_t clone_ofs = 0;
>>  size_t push_action_size;
>>  
>> -clone_ofs = nl_msg_start_nested(ctx->odp_actions, 
>> OVS_ACTION_ATTR_CLONE);
>> +if (!is_last_action) {
>> +clone_ofs = nl_msg_start_nested(ctx->odp_actions,
>> +OVS_ACTION_ATTR_CLONE);
>> +}
>> +size_t pre_patch_port_outp_size = ctx->odp_actions->size;
> Hi, Rosemarie.  Thanks for addressing previous comments!
> This version looks correct to me.  I also did some tests
> with dummy offload and they seem to work fine.
>
> The only thing I think we can improve is the variable name
> here.  I think, it will be a bit cleaner if we will rename
> and re-use the same variable for the offset in both cases.
> Maybe something like this:
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 0c92ed922..c645c3470 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -3736,14 +3736,12 @@ native_tunnel_output(struct xlate_ctx *ctx, const 
> struct xport *xport,
>s_ip, tnl_params.is_ipv6,
>tnl_push_data.tnl_type);
>  
> -size_t clone_ofs = 0;
> +size_t offset;
>  size_t push_action_size;
>  
> -if (!is_last_action) {
> -clone_ofs = nl_msg_start_nested(ctx->odp_actions,
> -OVS_ACTION_ATTR_CLONE);
> -}
> -size_t pre_patch_port_outp_size = ctx->odp_actions->size;
> +offset = is_last_action
> + ? ctx->odp_actions->size
> + : nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_CLONE);
>  odp_put_tnl_push_action(ctx->odp_actions, &tnl_push_data);
>  push_action_size = ctx->odp_actions->size;
>  
> ---
>
> What do you think?
>
> If that looks good to you, I can just apply the diff above and
> rename the variable in other places while applying the change.
>
> Best regards, Ilya Maximets.
>

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

[ovs-dev] [PATCH v2] tests: Add OVS-DPDK MTU unit tests.

2022-06-29 Thread Michael Phelan

This adds 8 new unit tests to the 'check-dpdk' subsystem that will
test Maximum Transmission Unit (MTU) functionality.

Signed-off-by: Michael Phelan 

---
v2:
  - Removed DB checks when confirming MTU value in all tests.
  - Added checks to catch errors in MTU setup or if device does not support MTU 
configuration.
  - Fixed typo in comments.
---
---
 tests/system-dpdk.at | 328 +++
 1 file changed, 328 insertions(+)

diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
index b036580de..d9a5fc1c3 100644
--- a/tests/system-dpdk.at
+++ b/tests/system-dpdk.at
@@ -384,6 +384,334 @@ dnl 
--
 
 
 
+dnl --
+dnl MTU increase phy port
+AT_SETUP([OVS-DPDK - MTU increase phy port])
+AT_KEYWORDS([dpdk])
+
+OVS_DPDK_PRE_PHY_SKIP()
+OVS_DPDK_START()
+
+dnl First set MTU to its default value and confirm that value, then increase 
the MTU value and confirm the new value
+
+dnl Add userspace bridge and attach it to OVS with default MTU value
+AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10 datapath_type=netdev])
+AT_CHECK([ovs-vsctl add-port br10 phy0 -- set Interface phy0 type=dpdk 
options:dpdk-devargs=$(cat PCI_ADDR)], [], [stdout], [stderr])
+AT_CHECK([ovs-vsctl show], [], [stdout])
+sleep 2
+
+dnl check default MTU value in the datapath
+AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
+AT_CHECK([egrep 'mtu=1500,' stdout], [], [stdout])
+
+dnl increase MTU value and check in the datapath
+AT_CHECK(ovs-vsctl set Interface phy0 mtu_request=9000)
+
+dnl Fail if MTU is not supported
+AT_FAIL_IF([grep "Interface phy0 does not support MTU configuration" 
ovs-vswitchd.log], [], [stdout])
+
+dnl Fail if error is encountered during MTU setup
+AT_FAIL_IF([grep "Interface phy0 MTU (9000) setup error" ovs-vswitchd.log], 
[], [stdout])
+
+AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
+AT_CHECK([egrep 'mtu=9000,' stdout], [], [stdout])
+
+
+dnl Clean up
+AT_CHECK([ovs-vsctl del-port br10 phy0], [], [stdout], [stderr])
+OVS_VSWITCHD_STOP("[SYSTEM_DPDK_ALLOWED_LOGS]")
+AT_CLEANUP
+dnl --
+
+
+
+dnl --
+dnl MTU decrease phy port
+AT_SETUP([OVS-DPDK - MTU decrease phy port])
+AT_KEYWORDS([dpdk])
+
+OVS_DPDK_PRE_PHY_SKIP()
+OVS_DPDK_START()
+
+dnl First set an increased MTU value and confirm that value, then decrease the 
MTU value and confirm the new value
+
+dnl Add userspace bridge and attach it to OVS and modify MTU value
+AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10 datapath_type=netdev])
+AT_CHECK([ovs-vsctl add-port br10 phy0 -- set Interface phy0 type=dpdk 
options:dpdk-devargs=$(cat PCI_ADDR)], [], [stdout], [stderr])
+AT_CHECK(ovs-vsctl set Interface phy0 mtu_request=9000)
+AT_CHECK([ovs-vsctl show], [], [stdout])
+sleep 2
+
+dnl Fail if MTU is not supported
+AT_FAIL_IF([grep "Interface phy0 does not support MTU configuration" 
ovs-vswitchd.log], [], [stdout])
+
+dnl Fail if error is encountered during MTU setup
+AT_FAIL_IF([grep "Interface phy0 MTU (9000) setup error" ovs-vswitchd.log], 
[], [stdout])
+
+dnl check MTU value in the datapath
+AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
+AT_CHECK([egrep 'mtu=9000,' stdout], [], [stdout])
+
+dnl decrease MTU value and check in the datapath
+AT_CHECK(ovs-vsctl set Interface phy0 mtu_request=2000)
+
+AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
+AT_CHECK([egrep 'mtu=2000,' stdout], [], [stdout])
+
+
+dnl Clean up
+AT_CHECK([ovs-vsctl del-port br10 phy0], [], [stdout], [stderr])
+OVS_VSWITCHD_STOP("[SYSTEM_DPDK_ALLOWED_LOGS]")
+AT_CLEANUP
+dnl --
+
+
+
+dnl --
+dnl MTU increase vport port
+AT_SETUP([OVS-DPDK - MTU increase vport port])
+AT_KEYWORDS([dpdk])
+
+OVS_DPDK_PRE_CHECK()
+OVS_DPDK_START()
+
+dnl Add userspace bridge and attach it to OVS with default MTU value
+AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10 datapath_type=netdev])
+AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuserclient0 -- set Interface 
dpdkvhostuserclient0 type=dpdkvhostuserclient 
options:vhost-server-path=$OVS_RUNDIR/dpdkvhostclient0], [], [stdout], [stderr])
+AT_CHECK([ovs-vsctl show], [], [stdout])
+sleep 2
+
+dnl check default MTU value in the datapath
+AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
+AT_CHECK([egrep 'mtu=1500,' stdout], [], [stdout])
+
+dnl increase MTU value and check in the datapath
+AT_CHECK(ovs-vsctl set Interface dpdkvhostuserclient0 mtu_request=9000)
+
+AT_CHECK([ovs-appctl dpctl/show], [], [stdout])
+AT_CHECK([egrep 'mtu=9000,' stdout], [], [stdout])
+
+
+dnl Parse log file
+AT_CHECK([grep "VHOST_CONFIG: vhost-user client: socket created" 
ovs-vswitchd.log], [], [stdout])
+AT_CHECK([grep "vH

Re: [ovs-dev] [PATCH ovn v2] northd.c: Add flow to skip put_nd action if ip6.src or nd.sll is 0

2022-06-29 Thread Numan Siddique

On Wed, Jun 29, 2022 at 8:27 AM Dumitru Ceara  wrote:
>
> On 6/29/22 07:47, Ales Musil wrote:
> > The ip6.src or nd.sll does not have to be always set.
> > According to rfc4861:
> >
> > Source Address
> >  Either an address assigned to the interface from
> >  which this message is sent or (if Duplicate Address
> >  Detection is in progress [ADDRCONF]) the
> >  unspecified address.
> >
> > Source link-layer address
> >  The link-layer address for the sender.  MUST NOT be
> >  included when the source IP address is the
> >  unspecified address.  Otherwise, on link layers
> >  that have addresses this option MUST be included in
> >  multicast solicitations and SHOULD be included in
> >  unicast solicitations.
> >
> > Add rule that avoids adding MAC binding is either of those
> > is 0. This is continuation after discussion during review on
> > 80187a803 (ovn-northd: Add flow to use eth.src if nd.tll is 0
> > in put_nd() action.)
> >
> > Signed-off-by: Ales Musil 
> > ---
>
> Looks good to me, thanks!
>
> Acked-by: Dumitru Ceara 

Thanks.  I applied this patch to the main and backported to
branch-22.06 and 22.03.

Numan

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

Re: [ovs-dev] [PATCH ovn v3 1/1] Allow arbitrary args to be passed to called binary

2022-06-29 Thread Terry Wilson

On Wed, Jun 29, 2022 at 2:40 AM Dumitru Ceara  wrote:
>
> On 6/28/22 17:22, Terry Wilson wrote:
> > On Tue, Jun 28, 2022 at 4:39 AM Dumitru Ceara  wrote:
> >>
> >> On 6/27/22 18:00, Terry Wilson wrote:
> >>> It is common to pass ovn-ctl options via an /etc/sysconfig file.
> >>> It would be useful to be able to pass custom --remote options or
> >>> additional DBs to listen to via this file. This would give end
> >>> users the ability to have more fine-grained control without
> >>> having to modify ovn-ctl.
> >>>
> >>> Signed-off-by: Terry Wilson 
> >>> ---
> >>
> >> Hi Terry,
> >>
> >> For the change itself:
> >>
> >> Acked-by: Dumitru Ceara 
> >
> > Thanks!
> >
> >> While we can make it work for now with this new arbitrary args feature,
> >> for the actual goal, of passing a custom --remote option and additional
> >> DBs to listen to I think we need a follow up.  Currently, when starting
> >> OVN databases, if the database doesn't exist ovn-ctl takes care of
> >> creating it:
> >>
> >> https://github.com/ovn-org/ovn/blob/86684ad759b4f517b8f34fbf7ba8bb8ed77bb575/utilities/ovn-ctl#L246
> >> (raft)
> >>
> >> https://github.com/ovn-org/ovn/blob/86684ad759b4f517b8f34fbf7ba8bb8ed77bb575/utilities/ovn-ctl#L249
> >> (standalone)
> >>
> >> In the future, after [0] is accepted, can we add a patch that changes
> >> ovn-ctl to check if the local_config.ovsschema file is installed?  If it
> >> is and if DB_*_USE_REMOTE_IN_DB=yes it could then create the
> >> corresponding local_config database if not already found in $ovn_dbdir
> >> and automatically add the --remote and new db file args.
> >>
> >> What do you think?
> >
> > Something like that sounds good to me. With this I was looking for
> > something that would not specifically require the ovs change, but
> > would still let me solve the problem if I needed to pass the
> > schema/create the db locally. And in general it's useful to be able to
> > pass through some options.
> >
>
> Agreed.
>
> > I hadn't originally planned to use Local_Config just because it
> > existed--I can imagine situations where you have a manually added
> > Connection or custom set things like inactivity_probe set up in the
> > main DB, and if ovn-ctl now notices you have a Local_Config schema, it
> > stops using the original table and you lose whatever was in the
> > original db. So I was thinking of an additional --use-local-config
> > kind of option to opt into using it.
> >
>
> Also fine, in my opinion.  Will you be working on adding this support to
> ovn-ctl?

Yeah, I can do that.

> Thanks!
>
> >> Thanks,
> >> Dumitru
> >>
> >>>  utilities/ovn-ctl   | 22 ++
> >>>  utilities/ovn-ctl.8.xml | 14 +-
> >>>  2 files changed, 35 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl
> >>> index 23d4d7f8c..93be9b84b 100755
> >>> --- a/utilities/ovn-ctl
> >>> +++ b/utilities/ovn-ctl
> >>> @@ -311,6 +311,10 @@ $cluster_remote_port
> >>>  set "$@" --sync-from=`cat $active_conf_file`
> >>>  fi
> >>>
> >>> +if test X"$extra_args" != X; then
> >>> +set "$@" $extra_args
> >>> +fi
> >>> +
> >>>  local run_ovsdb_in_bg="no"
> >>>  local process_id=
> >>>  if test X$detach = Xno && test $mode = cluster && test -z 
> >>> "$cluster_remote_addr" ; then
> >>> @@ -541,6 +545,10 @@ start_ic () {
> >>>
> >>>  set "$@" $OVN_IC_LOG $ovn_ic_params
> >>>
> >>> +if test X"$extra_args" != X; then
> >>> +set "$@" $extra_args
> >>> +fi
> >>> +
> >>>  OVS_RUNDIR=${OVS_RUNDIR} start_ovn_daemon "$OVN_IC_PRIORITY" 
> >>> "$OVN_IC_WRAPPER" "$@"
> >>>  fi
> >>>  }
> >>> @@ -563,6 +571,10 @@ start_controller () {
> >>>
> >>>  [ "$OVN_USER" != "" ] && set "$@" --user "$OVN_USER"
> >>>
> >>> +if test X"$extra_args" != X; then
> >>> +set "$@" $extra_args
> >>> +fi
> >>> +
> >>>  OVS_RUNDIR=${OVS_RUNDIR} start_ovn_daemon "$OVN_CONTROLLER_PRIORITY" 
> >>> "$OVN_CONTROLLER_WRAPPER" "$@"
> >>>  }
> >>>
> >>> @@ -590,6 +602,10 @@ start_controller_vtep () {
> >>>
> >>>  [ "$OVN_USER" != "" ] && set "$@" --user "$OVN_USER"
> >>>
> >>> +if test X"$extra_args" != X; then
> >>> +set "$@" $extra_args
> >>> +fi
> >>> +
> >>>  OVS_RUNDIR=${OVS_RUNDIR} start_ovn_daemon "$OVN_CONTROLLER_PRIORITY" 
> >>> "$OVN_CONTROLLER_WRAPPER" "$@"
> >>>  }
> >>>
> >>> @@ -1106,8 +1122,10 @@ EOF
> >>>
> >>>  set_defaults
> >>>  command=
> >>> +extra_args=
> >>>  for arg
> >>>  do
> >>> +shift
> >>>  case $arg in
> >>>  -h | --help)
> >>>  usage
> >>> @@ -1130,6 +1148,10 @@ do
> >>>  type=bool
> >>>  set_option
> >>>  ;;
> >>> +--)
> >>> +extra_args=$@
> >>> +break
> >>> +;;
> >>>  -*)
> >>>  echo >&2 "$0: unknown option \"$arg\" (use --help for help)"
> >>>  exit 1
> >>> diff --git a/utilities/ovn-ctl.8.xml b/utiliti

[ovs-dev] [PATCH v2] python: use setuptools instead of distutils

2022-06-29 Thread Timothy Redaelli

On Python 3.12, distutils will be removed and it's currently (3.10+)
deprecated (see PEP 632).

Since the suggested and simplest replacement is setuptools, this commit
replaces distutils to use setuptools instead.

setuptools < 59.0 doesn't have setuptools.errors and so, in this case,
distutils.errors is still used.

Signed-off-by: Timothy Redaelli 
---

v1 -> v2:
 - As reported by Mike Pattrick, use distutils.errors if setuptools.errors is
   not available (for setuptools < 59.0).

---
 python/setup.py | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/python/setup.py b/python/setup.py
index cfe01763f..8ff5bb0e9 100644
--- a/python/setup.py
+++ b/python/setup.py
@@ -12,9 +12,13 @@
 
 import sys
 
-from distutils.command.build_ext import build_ext
-from distutils.errors import CCompilerError, DistutilsExecError, \
-DistutilsPlatformError
+from setuptools.command.build_ext import build_ext
+try:
+from setuptools.errors import CCompilerError, ExecError, PlatformError
+except ImportError:  # Needed for setuptools < 59.0
+from distutils.errors import CCompilerError
+from distutils.errors import DistutilsExecError as ExecError
+from distutils.errors import DistutilsPlatformError as PlatformError
 
 import setuptools
 
@@ -37,7 +41,7 @@ except IOError:
   file=sys.stderr)
 sys.exit(-1)
 
-ext_errors = (CCompilerError, DistutilsExecError, DistutilsPlatformError)
+ext_errors = (CCompilerError, ExecError, PlatformError)
 if sys.platform == 'win32':
 ext_errors += (IOError, ValueError)
 
@@ -53,7 +57,7 @@ class try_build_ext(build_ext):
 def run(self):
 try:
 build_ext.run(self)
-except DistutilsPlatformError:
+except PlatformError:
 raise BuildFailed()
 
 def build_extension(self, ext):
-- 
2.36.1

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

Re: [ovs-dev] [ovs-build] |fail| pw1650080 [ovs-dev, branch-2.14] dpif-netdev: Refactor AVX512 runtime checks.

2022-06-29 Thread Phelan, Michael


> -Original Message-
> From: Ilya Maximets 
> Sent: Wednesday 29 June 2022 16:02
> To: ovs-bu...@openvswitch.org; Phelan, Michael 
> Cc: i.maxim...@ovn.org; Stokes, Ian ; Aaron Conole
> ; David Marchand ; ovs-dev
> 
> Subject: Re: [ovs-build] |fail| pw1650080 [ovs-dev, branch-2.14] dpif-netdev:
> Refactor AVX512 runtime checks.
> 
> > libtool: compile:  gcc -DHAVE_CONFIG_H -I. -I ./include -I ./include
> > -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare
> > -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum
> > -Wunused-parameter -Wbad-function-cast -Wcast-align
> > -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes
> > -Wmissing-field-initializers -fno-strict-aliasing -Wswitch-bool
> > -Wlogical-not-parentheses -Wsizeof-array-argument -Wbool-compare
> > -Wshift-negative-value -Wduplicated-cond -Wshadow
> > -Wmultistatement-macros -Wcast-align=strict -mssse3 -include
> > rte_config.h -I/usr/local/include -D_FILE_OFFSET_BITS=64 -g -O0
> > -march=native -MT lib/command-line.lo -MD -MP -MF
> > lib/.deps/command-line.Tpo -c lib/command-line.c -o lib/command-line.o
> > lib/netdev-dpdk.c:5058:24: error: ‘RTE_VHOST_USER_DEQUEUE_ZERO_COPY’
> undeclared (first use in this function)
> >  5058 |  & RTE_VHOST_USER_DEQUEUE_ZERO_COPY;
> >   |^~~~
> 
> 
> Hey, Michael.  There seems to be still some problems with a way DPDK is built
> for older branches.
> Could you, please, check?

Hey Ilya,
Sorry about that, let me take a look and get back to you.

Kind regards,
Michael.

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

Re: [ovs-dev] [ovs-build] |fail| pw1650080 [ovs-dev, branch-2.14] dpif-netdev: Refactor AVX512 runtime checks.

2022-06-29 Thread Ilya Maximets

> libtool: compile:  gcc -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib 
> -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith 
> -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter 
> -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition 
> -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing 
> -Wswitch-bool -Wlogical-not-parentheses -Wsizeof-array-argument 
> -Wbool-compare -Wshift-negative-value -Wduplicated-cond -Wshadow 
> -Wmultistatement-macros -Wcast-align=strict -mssse3 -include rte_config.h 
> -I/usr/local/include -D_FILE_OFFSET_BITS=64 -g -O0 -march=native -MT 
> lib/command-line.lo -MD -MP -MF lib/.deps/command-line.Tpo -c 
> lib/command-line.c -o lib/command-line.o
> lib/netdev-dpdk.c:5058:24: error: ‘RTE_VHOST_USER_DEQUEUE_ZERO_COPY’ 
> undeclared (first use in this function)
>  5058 |  & RTE_VHOST_USER_DEQUEUE_ZERO_COPY;
>   |^~~~


Hey, Michael.  There seems to be still some problems with a
way DPDK is built for older branches.
Could you, please, check?


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

Re: [ovs-dev] [PATCH v2] netdev-linux: do not touch LAG slaves if master is not attached to ovs.

2022-06-29 Thread Tao Liu

On Mon, Jun 27, 2022 at 03:47:34PM +0300, Roi Dayan wrote:
> 
> could it be your bond0 configuration?
> beside doing +bond0 into /sys/class/net/bonding_masters how do you
> config bond0 and its slave?
> 
> I don't have any static config file and use ip commands
> as follows:
> 
> ip link add name bond0 type bond mode $mode miimon 100
> ip link set dev $nic1 down
> ip link set dev $nic2 down
> ip link set dev $nic1 master bond0
> ip link set dev $nic2 master bond0
> ip link set dev bond0 up
> ip link set dev $nic1 up
> ip link set dev $nic2 up
> 
> 
> then adding bond0 to ovs bridge, I see bond0 got ingress block 310
> but the slaves didn't get ingress.
> 
> #  tc qdisc show dev bond0 ingress
> #  ovs-vsctl  show
> eb7bb34b-bdf6-4b9b-88d8-22ffeaf47630
> ovs_version: "2.17.90"
> #  ovs-vsctl  add-br ov1
> #  ovs-vsctl  add-port ov1 bond0
> #  tc qdisc show dev bond0 ingress
> qdisc ingress : parent :fff1 ingress_block 310 
> #  tc qdisc show dev enp8s0f0 ingress
> #  tc qdisc show dev enp8s0f1 ingress
> #  ovs-vsctl  show
> eb7bb34b-bdf6-4b9b-88d8-22ffeaf47630
> Bridge ov1
> Port ov1
> Interface ov1
> type: internal
> Port bond0
> Interface bond0
> ovs_version: "2.17.90"
> #  tc qdisc show dev bond0 ingress
> qdisc ingress : parent :fff1 ingress_block 310 
> #  tc qdisc show dev enp8s0f0 ingress
> #  tc qdisc show dev enp8s0f1 ingress
> #
> 
Hi, Roi. Thanks for having tested this patch.

I have reproduced your test case. It is because bond0 first opened with
type == NULL, then auto_classified == true. When bond0 adds to ovs, bond0
opens with type == "system", and auto_classified will not be cleared.

Will send v3.
> 
> printing in the log showing master_netdev->auto_classified is 1.
> you showed in your prev test its 1 as well . so how it got to be
> 0 now for you to pass the if-statement?
> 
Sorry, I copied the wrong log line.
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v3 1/1] Allow arbitrary args to be passed to called binary

2022-06-29 Thread Mark Michelson


On 6/29/22 03:40, Dumitru Ceara wrote:

On 6/28/22 17:22, Terry Wilson wrote:

On Tue, Jun 28, 2022 at 4:39 AM Dumitru Ceara  wrote:


On 6/27/22 18:00, Terry Wilson wrote:

It is common to pass ovn-ctl options via an /etc/sysconfig file.
It would be useful to be able to pass custom --remote options or
additional DBs to listen to via this file. This would give end
users the ability to have more fine-grained control without
having to modify ovn-ctl.

Signed-off-by: Terry Wilson 
---


Hi Terry,

For the change itself:

Acked-by: Dumitru Ceara 


Thanks!


I've merged the change to main, branch-22.06 and branch-22.03.




While we can make it work for now with this new arbitrary args feature,
for the actual goal, of passing a custom --remote option and additional
DBs to listen to I think we need a follow up.  Currently, when starting
OVN databases, if the database doesn't exist ovn-ctl takes care of
creating it:

https://github.com/ovn-org/ovn/blob/86684ad759b4f517b8f34fbf7ba8bb8ed77bb575/utilities/ovn-ctl#L246
(raft)

https://github.com/ovn-org/ovn/blob/86684ad759b4f517b8f34fbf7ba8bb8ed77bb575/utilities/ovn-ctl#L249
(standalone)

In the future, after [0] is accepted, can we add a patch that changes
ovn-ctl to check if the local_config.ovsschema file is installed?  If it
is and if DB_*_USE_REMOTE_IN_DB=yes it could then create the
corresponding local_config database if not already found in $ovn_dbdir
and automatically add the --remote and new db file args.

What do you think?


Something like that sounds good to me. With this I was looking for
something that would not specifically require the ovs change, but
would still let me solve the problem if I needed to pass the
schema/create the db locally. And in general it's useful to be able to
pass through some options.



Agreed.


I hadn't originally planned to use Local_Config just because it
existed--I can imagine situations where you have a manually added
Connection or custom set things like inactivity_probe set up in the
main DB, and if ovn-ctl now notices you have a Local_Config schema, it
stops using the original table and you lose whatever was in the
original db. So I was thinking of an additional --use-local-config
kind of option to opt into using it.



Also fine, in my opinion.  Will you be working on adding this support to
ovn-ctl?

Thanks!


Thanks,
Dumitru


  utilities/ovn-ctl   | 22 ++
  utilities/ovn-ctl.8.xml | 14 +-
  2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl
index 23d4d7f8c..93be9b84b 100755
--- a/utilities/ovn-ctl
+++ b/utilities/ovn-ctl
@@ -311,6 +311,10 @@ $cluster_remote_port
  set "$@" --sync-from=`cat $active_conf_file`
  fi

+if test X"$extra_args" != X; then
+set "$@" $extra_args
+fi
+
  local run_ovsdb_in_bg="no"
  local process_id=
  if test X$detach = Xno && test $mode = cluster && test -z 
"$cluster_remote_addr" ; then
@@ -541,6 +545,10 @@ start_ic () {

  set "$@" $OVN_IC_LOG $ovn_ic_params

+if test X"$extra_args" != X; then
+set "$@" $extra_args
+fi
+
  OVS_RUNDIR=${OVS_RUNDIR} start_ovn_daemon "$OVN_IC_PRIORITY" "$OVN_IC_WRAPPER" 
"$@"
  fi
  }
@@ -563,6 +571,10 @@ start_controller () {

  [ "$OVN_USER" != "" ] && set "$@" --user "$OVN_USER"

+if test X"$extra_args" != X; then
+set "$@" $extra_args
+fi
+
  OVS_RUNDIR=${OVS_RUNDIR} start_ovn_daemon "$OVN_CONTROLLER_PRIORITY" 
"$OVN_CONTROLLER_WRAPPER" "$@"
  }

@@ -590,6 +602,10 @@ start_controller_vtep () {

  [ "$OVN_USER" != "" ] && set "$@" --user "$OVN_USER"

+if test X"$extra_args" != X; then
+set "$@" $extra_args
+fi
+
  OVS_RUNDIR=${OVS_RUNDIR} start_ovn_daemon "$OVN_CONTROLLER_PRIORITY" 
"$OVN_CONTROLLER_WRAPPER" "$@"
  }

@@ -1106,8 +1122,10 @@ EOF

  set_defaults
  command=
+extra_args=
  for arg
  do
+shift
  case $arg in
  -h | --help)
  usage
@@ -1130,6 +1148,10 @@ do
  type=bool
  set_option
  ;;
+--)
+extra_args=$@
+break
+;;
  -*)
  echo >&2 "$0: unknown option \"$arg\" (use --help for help)"
  exit 1
diff --git a/utilities/ovn-ctl.8.xml b/utilities/ovn-ctl.8.xml
index a1d39b22b..42d16fabc 100644
--- a/utilities/ovn-ctl.8.xml
+++ b/utilities/ovn-ctl.8.xml
@@ -4,7 +4,10 @@
  ovn-ctl -- Open Virtual Network northbound daemon lifecycle utility

  Synopsis
-ovn-ctl [options] command
+
+  ovn-ctl [options] command
+  [--- extra_args]
+

  Description
  This program is intended to be invoked internally by Open Virtual 
Network
@@ -156,6 +159,15 @@
  --db-nb-probe-interval-to-active=Time in 
milliseconds
  --db-sb-probe-interval-to-active=Time in 
milliseconds

+ Extra Options 
+
+  Any options after '--' will be passed on to the binary run by
+

Re: [ovs-dev] [PATCH v2 3/6] debian: Update packaging source from Debian/Ubuntu.

2022-06-29 Thread 0-day Robot

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

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


checkpatch:
ERROR: Co-author Luca Boccassi  needs to sign off.
Lines checked: 3736, Warnings: 0, Errors: 1


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

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

Re: [ovs-dev] [PATCH v4] tests: Add ovs-dpdk rate limiting unit tests.

2022-06-29 Thread Stokes, Ian

> From: Seamus Ryan 
> 
> This adds 4 new unit tests to the 'check-dpdk' subsystem that will
> test rate limiting functionality.
> 
> Signed-off-by: Seamus Ryan 
> Signed-off-by: Michael Phelan 
> Co-authored-by: Michael Phelan 
> 

Thanks for the patch Michael.

>From what I can se all comments have been addressed so I'm going to push to 
>master.

Thanks all for the review and inputs.

Thanks
Ian

> ---
> v4:
>   - Changed egress to ingress in error check.
>   - Added ingress to various comments to improve clarity.
>   - Removed unnecessary error catch in phy test.
> 
> v3:
>   - Removed NEWS entry.
>   - Added check to catch error if policer fails to be created.
> 
> v2:
>   - Fixed dn1 typo and spacing issues.
>   - Added check for removing burst and rate values.
>   - Renamed tests to specify ingress policing.
> ---
> ---
>  tests/system-dpdk.at | 162
> +++
>  1 file changed, 162 insertions(+)
> 
> diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
> index 7d2715c4a..b036580de 100644
> --- a/tests/system-dpdk.at
> +++ b/tests/system-dpdk.at
> @@ -222,6 +222,168 @@ OVS_VSWITCHD_STOP("m4_join([],
> [SYSTEM_DPDK_ALLOWED_LOGS], [
>  AT_CLEANUP
>  dnl 
> --
> 
> +
> +
> +dnl 
> --
> +dnl Ingress policing create delete phy port
> +AT_SETUP([OVS-DPDK - Ingress policing create delete phy port])
> +AT_KEYWORDS([dpdk])
> +
> +OVS_DPDK_PRE_PHY_SKIP()
> +OVS_DPDK_START()
> +
> +dnl Add userspace bridge and attach it to OVS and add policer
> +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10
> datapath_type=netdev])
> +AT_CHECK([ovs-vsctl add-port br10 phy0 -- set Interface phy0 type=dpdk
> options:dpdk-devargs=$(cat PCI_ADDR)], [], [stdout], [stderr])
> +AT_CHECK([ovs-vsctl set interface phy0 ingress_policing_rate=1
> ingress_policing_burst=1000])
> +AT_CHECK([ovs-vsctl show], [], [stdout])
> +sleep 2
> +
> +dnl Fail if policer could not be created
> +AT_FAIL_IF([grep "Could not create rte meter for ingress policer" ovs-
> vswitchd.log], [], [stdout])
> +
> +dnl remove policer
> +AT_CHECK([ovs-vsctl set interface phy0 ingress_policing_rate=0
> ingress_policing_burst=0])
> +
> +dnl check policer was removed correctly
> +AT_CHECK([ovs-vsctl list interface phy0], [], [stdout])
> +AT_CHECK([egrep 'ingress_policing_burst: 0' stdout], [], [stdout])
> +
> +AT_CHECK([ovs-vsctl list interface phy0], [], [stdout])
> +AT_CHECK([egrep 'ingress_policing_rate: 0' stdout], [], [stdout])
> +
> +dnl Clean up
> +AT_CHECK([ovs-vsctl del-port br10 phy0], [], [stdout], [stderr])
> +OVS_VSWITCHD_STOP("[SYSTEM_DPDK_ALLOWED_LOGS]")
> +AT_CLEANUP
> +dnl 
> --
> +
> +
> +
> +dnl 
> --
> +dnl Ingress policing create delete vport port
> +AT_SETUP([OVS-DPDK - Ingress policing create delete vport port])
> +AT_KEYWORDS([dpdk])
> +
> +OVS_DPDK_PRE_CHECK()
> +OVS_DPDK_START()
> +
> +dnl Add userspace bridge and attach it to OVS and add ingress policer
> +AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10
> datapath_type=netdev])
> +AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuserclient0 -- set Interface
> dpdkvhostuserclient0 type=dpdkvhostuserclient options:vhost-server-
> path=$OVS_RUNDIR/dpdkvhostclient0], [], [stdout], [stderr])
> +AT_CHECK([ovs-vsctl set interface dpdkvhostuserclient0
> ingress_policing_rate=1 ingress_policing_burst=1000])
> +AT_CHECK([ovs-vsctl show], [], [stdout])
> +sleep 2
> +
> +dnl Fail if ingress policer could not be created
> +AT_FAIL_IF([grep "Could not create rte meter for ingress policer" ovs-
> vswitchd.log], [], [stdout])
> +
> +dnl remove ingress policer
> +AT_CHECK([ovs-vsctl set interface dpdkvhostuserclient0
> ingress_policing_rate=0 ingress_policing_burst=0])
> +
> +dnl check ingress policer was removed correctly
> +AT_CHECK([ovs-vsctl list interface dpdkvhostuserclient0], [], [stdout])
> +AT_CHECK([egrep 'ingress_policing_burst: 0' stdout], [], [stdout])
> +
> +AT_CHECK([ovs-vsctl list interface dpdkvhostuserclient0], [], [stdout])
> +AT_CHECK([egrep 'ingress_policing_rate: 0' stdout], [], [stdout])
> +
> +dnl Parse log file
> +AT_CHECK([grep "VHOST_CONFIG: vhost-user client: socket created" ovs-
> vswitchd.log], [], [stdout])
> +AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in
> 'client' mode, using client socket" ovs-vswitchd.log], [], [stdout])
> +AT_CHECK([grep "VHOST_CONFIG: $OVS_RUNDIR/dpdkvhostclient0:
> reconnecting..." ovs-vswitchd.log], [], [stdout])
> +
> +dnl Clean up
> +AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout],
> [stderr])
> +OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
> +\@VHOST_CONFIG: failed to connect to $OVS_RUNDIR/dpdkvhostclient0:
> No such file or directory@d
> +])")
> +AT_CLEANUP
> +d

[ovs-dev] [PATCH v2 1/6] debian: Archive debian packaging source.

2022-06-29 Thread Frode Nordahl

The packaging source in the OVS repository has drifted away from
what is currently in Debian and Ubuntu.  This state is problematic
because from time to time someone tries to build packages from the
upstream OVS debian package source and then expect that package to
work with up-/down-grades from-/to/ distro versions.

To support the on-going work to remove the out of tree OVS kernel
driver from the repository [0], an update to the debian packaging
is also required.  On the back of the discussion in [0] we agreed
that replacing the current version with what Debian and Ubuntu
is currently converging on would be preferable.

This commit is a first in a series to update the upstream OVS
debian packaging source to be up to date with what is currently
in Debian and Ubuntu.

0: https://mail.openvswitch.org/pipermail/ovs-dev/2022-June/394634.html

Signed-off-by: Frode Nordahl 
---
 .github/workflows/build-and-test.yml  |   3 -
 debian/.gitignore |  27 -
 debian/automake.mk|  82 +-
 debian/compat |   1 -
 debian/control| 266 ---
 debian/control.modules.in |  20 -
 debian/copyright.in   | 711 --
 debian/dirs   |   2 -
 debian/dkms.conf.in   |  11 -
 debian/ifupdown.sh| 107 ---
 debian/libopenvswitch-dev.install |  19 -
 debian/libopenvswitch.install |   5 -
 debian/openvswitch-common.dirs|   1 -
 debian/openvswitch-common.docs|   0
 debian/openvswitch-common.install |  11 -
 debian/openvswitch-common.manpages|   7 -
 debian/openvswitch-datapath-dkms.postinst |  21 -
 debian/openvswitch-datapath-dkms.prerm|  15 -
 ...atapath-module-_KVERS_.postinst.modules.in |  27 -
 .../openvswitch-datapath-source.README.Debian |  31 -
 debian/openvswitch-datapath-source.copyright  |  15 -
 debian/openvswitch-datapath-source.dirs   |   1 -
 debian/openvswitch-datapath-source.install|   5 -
 debian/openvswitch-ipsec.dirs |   1 -
 debian/openvswitch-ipsec.init | 181 -
 debian/openvswitch-ipsec.install  |   1 -
 debian/openvswitch-pki.dirs   |   1 -
 debian/openvswitch-pki.postinst   |  41 -
 debian/openvswitch-pki.postrm |  43 --
 debian/openvswitch-switch.README.Debian   | 316 
 debian/openvswitch-switch.dirs|   2 -
 debian/openvswitch-switch.init| 147 
 debian/openvswitch-switch.install |  17 -
 debian/openvswitch-switch.links   |   2 -
 debian/openvswitch-switch.logrotate   |  16 -
 debian/openvswitch-switch.manpages|  12 -
 debian/openvswitch-switch.postinst|  60 --
 debian/openvswitch-switch.postrm  |  48 --
 debian/openvswitch-switch.template|   8 -
 debian/openvswitch-test.dirs  |   1 -
 debian/openvswitch-test.install   |   3 -
 debian/openvswitch-test.manpages  |   1 -
 .../openvswitch-testcontroller.README.Debian  |  12 -
 debian/openvswitch-testcontroller.default |  29 -
 debian/openvswitch-testcontroller.dirs|   1 -
 debian/openvswitch-testcontroller.init| 278 ---
 debian/openvswitch-testcontroller.install |   1 -
 debian/openvswitch-testcontroller.manpages|   1 -
 debian/openvswitch-testcontroller.postinst|  52 --
 debian/openvswitch-testcontroller.postrm  |  44 --
 debian/openvswitch-vtep.default   |   4 -
 debian/openvswitch-vtep.dirs  |   1 -
 debian/openvswitch-vtep.init  |  78 --
 debian/openvswitch-vtep.install   |   3 -
 debian/openvswitch-vtep.manpages  |   1 -
 debian/python3-openvswitch.dirs   |   2 -
 debian/python3-openvswitch.install|   1 -
 debian/rules  |  94 ---
 debian/rules.modules  |  39 -
 debian/source/format  |   1 -
 60 files changed, 1 insertion(+), 2930 deletions(-)
 delete mode 100644 debian/.gitignore
 delete mode 100644 debian/compat
 delete mode 100644 debian/control
 delete mode 100644 debian/control.modules.in
 delete mode 100644 debian/copyright.in
 delete mode 100644 debian/dirs
 delete mode 100644 debian/dkms.conf.in
 delete mode 100755 debian/ifupdown.sh
 delete mode 100644 debian/libopenvswitch-dev.install
 delete mode 100644 debian/libopenvswitch.install
 delete mode 100644 debian/openvswitch-common.dirs
 delete mode 100644 debian/openvswitch-common.docs
 delete mode 100644 debian/openvswitch-common.install
 delete mode 100644 debian/openvswitch-common.manpages
 delete mode 100644 debian/openvswitch-datapath-dkms.postinst
 delete mode 100644 debian/

[ovs-dev] [PATCH v2 5/6] python: Allow building json C extension with static OVS library.

2022-06-29 Thread Frode Nordahl

Allow caller of setup.py to pass in libopenvswitch.a as an object
for linking through the use of LDFLAGS environment variable when
not building a shared openvswitch library.

To accomplish this set the `enable_shared` environment variable to
'no'.

Example:
LDFLAGS=lib/libopenvswitch.a enable_shared=no setup.py install

Signed-off-by: Frode Nordahl 
---
 python/setup.py | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/python/setup.py b/python/setup.py
index cfe01763f..b6e88e98e 100644
--- a/python/setup.py
+++ b/python/setup.py
@@ -10,6 +10,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import os
 import sys
 
 from distutils.command.build_ext import build_ext
@@ -63,6 +64,15 @@ class try_build_ext(build_ext):
 raise BuildFailed()
 
 
+# allow caller of setup.py to pass in libopenvswitch.a as an object for linking
+# through the use of LDFLAGS environment variable when not building a shared
+# openvswitch library.
+if 'enable_shared' in os.environ and os.environ['enable_shared'] == 'no':
+json_libraries = []
+else:
+json_libraries = ['openvswitch']
+
+
 setup_args = dict(
 name='ovs',
 description='Open vSwitch library',
@@ -85,7 +95,7 @@ setup_args = dict(
 'Programming Language :: Python :: 3.5',
 ],
 ext_modules=[setuptools.Extension("ovs._json", sources=["ovs/_json.c"],
-  libraries=['openvswitch'])],
+  libraries=json_libraries)],
 cmdclass={'build_ext': try_build_ext},
 install_requires=['sortedcontainers'],
 extras_require={':sys_platform == "win32"': ['pywin32 >= 1.0']},
-- 
2.36.1

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

[ovs-dev] [PATCH v2 6/6] debian: Fix build of python json C extension.

2022-06-29 Thread Frode Nordahl

Signed-off-by: Frode Nordahl 
---
 debian/control |  2 +-
 debian/rules   | 28 +++-
 2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/debian/control b/debian/control
index 228c13a4b..c3071a941 100644
--- a/debian/control
+++ b/debian/control
@@ -279,7 +279,7 @@ Description: Open vSwitch VTEP utilities
  VTEP-configured database and a VTEP emulator.
 
 Package: python3-openvswitch
-Architecture: all
+Architecture: linux-any
 Section: python
 Depends:
  python3-six,
diff --git a/debian/rules b/debian/rules
index b9454fa79..8d2695278 100755
--- a/debian/rules
+++ b/debian/rules
@@ -95,16 +95,26 @@ override_dh_install:
mv $(CURDIR)/debian/openvswitch-switch/usr/sbin/ovs-vswitchd \

$(CURDIR)/debian/openvswitch-switch/usr/lib/openvswitch-switch/ovs-vswitchd
set -e && for pyvers in $(PYTHONS); do \
-   cd python && python$$pyvers setup.py install 
--install-layout=deb \
-   --root $(CURDIR)/debian/python-openvswitch; cd ..; \
+   cd python && \
+   enable_shared=no \
+   CFLAGS=-I$(CURDIR)/debian/tmp/usr/include \
+   LDFLAGS=$(CURDIR)/debian/tmp/usr/lib/libopenvswitch.a \
+   python$$pyvers setup.py install --install-layout=deb \
+   --root $(CURDIR)/debian/python-openvswitch; \
+   cd ..; \
done
set -e && for pyvers in $(PYTHON3S); do \
-cd python && python$$pyvers setup.py install --install-layout=deb \
---root $(CURDIR)/debian/python3-openvswitch; cd ..; \
-mkdir -p 
$(CURDIR)/debian/openvswitch-test/usr/lib/python$$pyvers/dist-packages/ovstest; 
\
-install -v -D python/ovstest/*.py \
-
$(CURDIR)/debian/openvswitch-test/usr/lib/python$$pyvers/dist-packages/ovstest; 
\
-done
+   cd python && \
+   enable_shared=no \
+   CFLAGS=-I$(CURDIR)/debian/tmp/usr/include \
+   LDFLAGS=$(CURDIR)/debian/tmp/usr/lib/libopenvswitch.a \
+   python$$pyvers setup.py install --install-layout=deb \
+   --root $(CURDIR)/debian/python3-openvswitch; \
+   cd ..; \
+   mkdir -p 
$(CURDIR)/debian/openvswitch-test/usr/lib/python$$pyvers/dist-packages/ovstest; 
\
+   install -v -D python/ovstest/*.py \
+   
$(CURDIR)/debian/openvswitch-test/usr/lib/python$$pyvers/dist-packages/ovstest; 
\
+   done
 ifneq (,$(filter i386 amd64 ppc64el arm64, $(DEB_HOST_ARCH)))
install -v -D _dpdk/vswitchd/ovs-vswitchd \
 
$(CURDIR)/debian/openvswitch-switch-dpdk/usr/lib/openvswitch-switch-dpdk/ovs-vswitchd-dpdk
@@ -126,7 +136,7 @@ override_dh_strip:
dh_strip --dbg-package=openvswitch-dbg
 
 override_dh_python3:
-   dh_python3 --shebang=/usr/bin/python3
+   DEB_HOST_ARCH=$(DEB_HOST_ARCH) dh_python3 --shebang=/usr/bin/python3
 
 # Helper target for creating snapshots from upstream git
 DATE=$(shell date +%Y%m%d)
-- 
2.36.1

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

[ovs-dev] [PATCH v2 4/6] ci: Separate job for debs, ensure built pkg is tested.

2022-06-29 Thread Frode Nordahl

Use a separate GitHub Actions job for deb test so that we can
control base image for package test.

The CI deb package test code currently attempts to use `apt` to
install local packages.  That may not produce the expected result.

Explicitly install the local packages with `dpkg` after installing
dependencies from `apt` instead.

Also enable test installation of ipsec deb package.

Signed-off-by: Frode Nordahl 
---
 .ci/linux-build.sh   | 21 --
 .github/workflows/build-and-test.yml | 62 +---
 2 files changed, 65 insertions(+), 18 deletions(-)

diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index 9963fb810..a8c437aaf 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -207,10 +207,23 @@ if [ "$DEB_PACKAGE" ]; then
 mk-build-deps --install --root-cmd sudo --remove debian/control
 dpkg-checkbuilddeps
 DEB_BUILD_OPTIONS='parallel=4 nocheck' fakeroot debian/rules binary
-# Not trying to install ipsec package as there are issues with system-wide
-# installed python3-openvswitch package and the pyenv used by Travis.
-packages=$(ls $(pwd)/../*.deb | grep -v ipsec)
-sudo apt install ${packages}
+packages=$(ls $(pwd)/../*.deb)
+deps=""
+for pkg in $packages; do
+_ifs=$IFS
+IFS=","
+for dep in $(dpkg-deb -f $pkg Depends); do
+dep_name=$(echo "$dep"|awk '{print$1}')
+# Don't install internal package inter-dependencies from apt
+echo $dep_name | grep -q openvswitch && continue
+deps+=" $dep_name"
+done
+IFS=$_ifs
+done
+# install package dependencies from apt
+echo $deps | xargs sudo apt -y install
+# install the locally built openvswitch packages
+sudo dpkg -i $packages
 exit 0
 fi
 
diff --git a/.github/workflows/build-and-test.yml 
b/.github/workflows/build-and-test.yml
index ec34a9b0d..b36282ce5 100644
--- a/.github/workflows/build-and-test.yml
+++ b/.github/workflows/build-and-test.yml
@@ -9,13 +9,10 @@ jobs:
 automake libtool gcc bc libjemalloc1 libjemalloc-dev\
 libssl-dev llvm-dev libelf-dev libnuma-dev libpcap-dev  \
 ninja-build selinux-policy-dev
-  deb_dependencies: |
-linux-headers-$(uname -r) build-essential fakeroot devscripts equivs
   AFXDP:   ${{ matrix.afxdp }}
   ASAN:${{ matrix.asan }}
   UBSAN:   ${{ matrix.ubsan }}
   CC:  ${{ matrix.compiler }}
-  DEB_PACKAGE: ${{ matrix.deb_package }}
   DPDK:${{ matrix.dpdk }}
   DPDK_SHARED: ${{ matrix.dpdk_shared }}
   KERNEL:  ${{ matrix.kernel }}
@@ -148,11 +145,7 @@ jobs:
 - name: update APT cache
   run:  sudo apt update || true
 - name: install common dependencies
-  if:   matrix.deb_package == ''
   run:  sudo apt install -y ${{ env.dependencies }}
-- name: install dependencies for debian packages
-  if:   matrix.deb_package != ''
-  run:  sudo apt install -y ${{ env.deb_dependencies }}
 - name: install libunbound libunwind
   if:   matrix.m32 == ''
   run:  sudo apt install -y libunbound-dev libunwind-dev
@@ -163,13 +156,6 @@ jobs:
 - name: build
   run:  ./.ci/linux-build.sh
 
-- name: upload deb packages
-  if:   matrix.deb_package != ''
-  uses: actions/upload-artifact@v2
-  with:
-name: deb-packages
-path: '/home/runner/work/ovs/*.deb'
-
 - name: copy logs on failure
   if: failure() || cancelled()
   run: |
@@ -224,3 +210,51 @@ jobs:
   with:
 name: logs-osx-clang---disable-ssl
 path: config.log
+
+  build-linux-deb:
+env:
+  deb_dependencies: |
+linux-headers-$(uname -r) build-essential fakeroot devscripts equivs
+  DEB_PACKAGE: ${{ matrix.deb_package }}
+
+name: linux ${{ join(matrix.*, ' ') }}
+runs-on: ubuntu-22.04
+timeout-minutes: 30
+
+strategy:
+  fail-fast: false
+  matrix:
+include:
+  - compiler: gcc
+deb_package:  deb
+
+steps:
+- name: checkout
+  uses: actions/checkout@v2
+
+- name: update PATH
+  run:  |
+echo "$HOME/bin">> $GITHUB_PATH
+echo "$HOME/.local/bin" >> $GITHUB_PATH
+
+- name: set up python
+  uses: actions/setup-python@v2
+  with:
+python-version: '3.9'
+
+- name: update APT cache
+  run:  sudo apt update || true
+- name: install dependencies for debian packages
+  run:  sudo apt install -y ${{ env.deb_dependencies }}
+
+- name: prepare
+  run:  ./.ci/linux-prepare.sh
+
+- name: build
+  run:  ./.ci/linux-build.sh
+
+- name: upload deb packages
+  uses: actions/upload-artifact@v2
+  with:
+name: deb-packages
+path: '/home/runner/work/ovs/*.deb'
-- 
2.36.1

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

[ovs-dev] [PATCH v2 2/6] checkpatch: Ignore line length and leading whitespace for debian/*.

2022-06-29 Thread Frode Nordahl

Signed-off-by: Frode Nordahl 
---
 utilities/checkpatch.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 8c02ac3ce..de2420e1f 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -194,12 +194,12 @@ skip_signoff_check = False
 #
 # Python isn't checked as flake8 performs these checks during build.
 line_length_ignore_list = re.compile(
-r'\.(am|at|etc|in|m4|mk|patch|py)$|debian/rules')
+r'\.(am|at|etc|in|m4|mk|patch|py)$|^debian/.*$')
 
 # Don't enforce a requirement that leading whitespace be all spaces on
 # files that include these characters in their name, since these kinds
 # of files need lines with leading tabs.
-leading_whitespace_ignore_list = re.compile(r'\.(mk|am|at)$|debian/rules')
+leading_whitespace_ignore_list = re.compile(r'\.(mk|am|at)$|^debian/.*$')
 
 
 def is_subtracted_line(line):
-- 
2.36.1

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

[ovs-dev] [PATCH v2 4/6] ci: Separate job for debs, ensure built pkg is tested.

2022-06-29 Thread Frode Nordahl

Use a separate GitHub Actions job for deb test so that we can
control base image for package test.

The CI deb package test code currently attempts to use `apt` to
install local packages.  That may not produce the expected result.

Explicitly install the local packages with `dpkg` after installing
dependencies from `apt` instead.

Also enable test installation of ipsec deb package.

Signed-off-by: Frode Nordahl 
---
 .ci/linux-build.sh   | 21 --
 .github/workflows/build-and-test.yml | 62 +---
 2 files changed, 65 insertions(+), 18 deletions(-)

diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index 9963fb810..a8c437aaf 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -207,10 +207,23 @@ if [ "$DEB_PACKAGE" ]; then
 mk-build-deps --install --root-cmd sudo --remove debian/control
 dpkg-checkbuilddeps
 DEB_BUILD_OPTIONS='parallel=4 nocheck' fakeroot debian/rules binary
-# Not trying to install ipsec package as there are issues with system-wide
-# installed python3-openvswitch package and the pyenv used by Travis.
-packages=$(ls $(pwd)/../*.deb | grep -v ipsec)
-sudo apt install ${packages}
+packages=$(ls $(pwd)/../*.deb)
+deps=""
+for pkg in $packages; do
+_ifs=$IFS
+IFS=","
+for dep in $(dpkg-deb -f $pkg Depends); do
+dep_name=$(echo "$dep"|awk '{print$1}')
+# Don't install internal package inter-dependencies from apt
+echo $dep_name | grep -q openvswitch && continue
+deps+=" $dep_name"
+done
+IFS=$_ifs
+done
+# install package dependencies from apt
+echo $deps | xargs sudo apt -y install
+# install the locally built openvswitch packages
+sudo dpkg -i $packages
 exit 0
 fi
 
diff --git a/.github/workflows/build-and-test.yml 
b/.github/workflows/build-and-test.yml
index ec34a9b0d..b36282ce5 100644
--- a/.github/workflows/build-and-test.yml
+++ b/.github/workflows/build-and-test.yml
@@ -9,13 +9,10 @@ jobs:
 automake libtool gcc bc libjemalloc1 libjemalloc-dev\
 libssl-dev llvm-dev libelf-dev libnuma-dev libpcap-dev  \
 ninja-build selinux-policy-dev
-  deb_dependencies: |
-linux-headers-$(uname -r) build-essential fakeroot devscripts equivs
   AFXDP:   ${{ matrix.afxdp }}
   ASAN:${{ matrix.asan }}
   UBSAN:   ${{ matrix.ubsan }}
   CC:  ${{ matrix.compiler }}
-  DEB_PACKAGE: ${{ matrix.deb_package }}
   DPDK:${{ matrix.dpdk }}
   DPDK_SHARED: ${{ matrix.dpdk_shared }}
   KERNEL:  ${{ matrix.kernel }}
@@ -148,11 +145,7 @@ jobs:
 - name: update APT cache
   run:  sudo apt update || true
 - name: install common dependencies
-  if:   matrix.deb_package == ''
   run:  sudo apt install -y ${{ env.dependencies }}
-- name: install dependencies for debian packages
-  if:   matrix.deb_package != ''
-  run:  sudo apt install -y ${{ env.deb_dependencies }}
 - name: install libunbound libunwind
   if:   matrix.m32 == ''
   run:  sudo apt install -y libunbound-dev libunwind-dev
@@ -163,13 +156,6 @@ jobs:
 - name: build
   run:  ./.ci/linux-build.sh
 
-- name: upload deb packages
-  if:   matrix.deb_package != ''
-  uses: actions/upload-artifact@v2
-  with:
-name: deb-packages
-path: '/home/runner/work/ovs/*.deb'
-
 - name: copy logs on failure
   if: failure() || cancelled()
   run: |
@@ -224,3 +210,51 @@ jobs:
   with:
 name: logs-osx-clang---disable-ssl
 path: config.log
+
+  build-linux-deb:
+env:
+  deb_dependencies: |
+linux-headers-$(uname -r) build-essential fakeroot devscripts equivs
+  DEB_PACKAGE: ${{ matrix.deb_package }}
+
+name: linux ${{ join(matrix.*, ' ') }}
+runs-on: ubuntu-22.04
+timeout-minutes: 30
+
+strategy:
+  fail-fast: false
+  matrix:
+include:
+  - compiler: gcc
+deb_package:  deb
+
+steps:
+- name: checkout
+  uses: actions/checkout@v2
+
+- name: update PATH
+  run:  |
+echo "$HOME/bin">> $GITHUB_PATH
+echo "$HOME/.local/bin" >> $GITHUB_PATH
+
+- name: set up python
+  uses: actions/setup-python@v2
+  with:
+python-version: '3.9'
+
+- name: update APT cache
+  run:  sudo apt update || true
+- name: install dependencies for debian packages
+  run:  sudo apt install -y ${{ env.deb_dependencies }}
+
+- name: prepare
+  run:  ./.ci/linux-prepare.sh
+
+- name: build
+  run:  ./.ci/linux-build.sh
+
+- name: upload deb packages
+  uses: actions/upload-artifact@v2
+  with:
+name: deb-packages
+path: '/home/runner/work/ovs/*.deb'
-- 
2.36.1

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

[ovs-dev] [branch-2.16] dpif-netdev: Refactor AVX512 runtime checks.

2022-06-29 Thread David Marchand

[ original commit fe171e4f109f001f07b867756a261d898f0d2cfc ]

As described in the bugzilla below, cpu_has_isa code may be compiled
with some AVX512 instructions in it, because cpu.c is built as part of
the libopenvswitchavx512.
This is a problem when this function (supposed to probe for AVX512
instructions availability) is invoked from generic OVS code, on older
CPUs that don't support them.

For the same reason, dpcls_subtable_avx512_gather_probe,
dp_netdev_input_outer_avx512_probe, mfex_avx512_probe and
mfex_avx512_vbmi_probe are potential runtime bombs and can't either be
built as part of libopenvswitchavx512.

Move cpu.c to be part of the "normal" libopenvswitch.
And move other helpers in generic OVS code.

Note:
- dpcls_subtable_avx512_gather_probe is split in two, because it also
  needs to do its own magic,
- while moving those helpers, prefer direct calls to cpu_has_isa and
  avoid cast to intermediate integer variables when a simple boolean
  is enough,

Fixes: 352b6c7116cd ("dpif-lookup: add avx512 gather implementation.")
Fixes: abb807e27dd4 ("dpif-netdev: Add command to switch dpif implementation.")
Fixes: 250ceddcc2d0 ("dpif-netdev/mfex: Add AVX512 based optimized miniflow 
extract")
Fixes: b366fa2f4947 ("dpif-netdev: Call cpuid for x86 isa availability.")
Reported-at: https://bugzilla.redhat.com/2100393
Reported-by: Ales Musil 
Co-authored-by: Ales Musil 
Signed-off-by: Ales Musil 
Signed-off-by: David Marchand 
---
 lib/dpif-netdev-avx512.c   | 13 
 lib/dpif-netdev-extract-avx512.c   | 43 --
 lib/dpif-netdev-lookup-avx512-gather.c | 11 ++-
 lib/dpif-netdev-lookup.c   | 14 +
 lib/dpif-netdev-lookup.h   |  3 +-
 lib/dpif-netdev-private-dpif.c | 13 
 lib/dpif-netdev-private-dpif.h |  5 +--
 lib/dpif-netdev-private-extract.c  | 37 ++
 lib/dpif-netdev-private-extract.h  |  4 +--
 9 files changed, 70 insertions(+), 73 deletions(-)

diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
index 544d36903e..01011f679a 100644
--- a/lib/dpif-netdev-avx512.c
+++ b/lib/dpif-netdev-avx512.c
@@ -58,19 +58,6 @@ struct dpif_userdata {
 struct pkt_flow_meta pkt_meta[NETDEV_MAX_BURST];
 };
 
-int32_t
-dp_netdev_input_outer_avx512_probe(void)
-{
-bool avx512f_available = dpdk_get_cpu_has_isa("x86_64", "avx512f");
-bool bmi2_available = dpdk_get_cpu_has_isa("x86_64", "bmi2");
-
-if (!avx512f_available || !bmi2_available) {
-return -ENOTSUP;
-}
-
-return 0;
-}
-
 int32_t
 dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread *pmd,
  struct dp_packet_batch *packets,
diff --git a/lib/dpif-netdev-extract-avx512.c b/lib/dpif-netdev-extract-avx512.c
index 28b54ef2f1..993d07e401 100644
--- a/lib/dpif-netdev-extract-avx512.c
+++ b/lib/dpif-netdev-extract-avx512.c
@@ -43,7 +43,6 @@
 #include 
 
 #include "flow.h"
-#include "dpdk.h"
 
 #include "dpif-netdev-private-dpcls.h"
 #include "dpif-netdev-private-extract.h"
@@ -663,47 +662,5 @@ DECLARE_MFEX_FUNC(ip_udp, PROFILE_ETH_IPV4_UDP)
 DECLARE_MFEX_FUNC(ip_tcp, PROFILE_ETH_IPV4_TCP)
 DECLARE_MFEX_FUNC(dot1q_ip_udp, PROFILE_ETH_VLAN_IPV4_UDP)
 DECLARE_MFEX_FUNC(dot1q_ip_tcp, PROFILE_ETH_VLAN_IPV4_TCP)
-
-
-static int32_t
-avx512_isa_probe(uint32_t needs_vbmi)
-{
-static const char *isa_required[] = {
-"avx512f",
-"avx512bw",
-"bmi2",
-};
-
-int32_t ret = 0;
-for (uint32_t i = 0; i < ARRAY_SIZE(isa_required); i++) {
-if (!dpdk_get_cpu_has_isa("x86_64", isa_required[i])) {
-ret = -ENOTSUP;
-}
-}
-
-if (needs_vbmi) {
-if (!dpdk_get_cpu_has_isa("x86_64", "avx512vbmi")) {
-ret = -ENOTSUP;
-}
-}
-
-return ret;
-}
-
-/* Probe functions to check ISA requirements. */
-int32_t
-mfex_avx512_probe(void)
-{
-const uint32_t needs_vbmi = 0;
-return avx512_isa_probe(needs_vbmi);
-}
-
-int32_t
-mfex_avx512_vbmi_probe(void)
-{
-const uint32_t needs_vbmi = 1;
-return avx512_isa_probe(needs_vbmi);
-}
-
 #endif /* __CHECKER__ */
 #endif /* __x86_64__ */
diff --git a/lib/dpif-netdev-lookup-avx512-gather.c 
b/lib/dpif-netdev-lookup-avx512-gather.c
index 072831e96a..154f9318d4 100644
--- a/lib/dpif-netdev-lookup-avx512-gather.c
+++ b/lib/dpif-netdev-lookup-avx512-gather.c
@@ -394,18 +394,11 @@ dpcls_avx512_gather_mf_any(struct dpcls_subtable 
*subtable, uint32_t keys_map,
 }
 
 dpcls_subtable_lookup_func
-dpcls_subtable_avx512_gather_probe(uint32_t u0_bits, uint32_t u1_bits)
+dpcls_subtable_avx512_gather_probe__(uint32_t u0_bits, uint32_t u1_bits,
+ bool use_vpop)
 {
 dpcls_subtable_lookup_func f = NULL;
 
-int avx512f_available = dpdk_get_cpu_has_isa("x86_64", "avx512f");
-int bmi2_available = dpdk_get_cpu_has_isa("x86_64", "bmi2");
-if (!avx512f_available || !bmi2_available) {
-return NULL;
-}
-
-

[ovs-dev] [branch-2.15] dpif-netdev: Refactor AVX512 runtime checks.

2022-06-29 Thread David Marchand

[ original commit fe171e4f109f001f07b867756a261d898f0d2cfc ]

As described in the bugzilla below, cpu_has_isa code may be compiled
with some AVX512 instructions in it, because cpu.c is built as part of
the libopenvswitchavx512.
This is a problem when this function (supposed to probe for AVX512
instructions availability) is invoked from generic OVS code, on older
CPUs that don't support them.

For the same reason, dpcls_subtable_avx512_gather_probe,
dp_netdev_input_outer_avx512_probe, mfex_avx512_probe and
mfex_avx512_vbmi_probe are potential runtime bombs and can't either be
built as part of libopenvswitchavx512.

Move cpu.c to be part of the "normal" libopenvswitch.
And move other helpers in generic OVS code.

Note:
- dpcls_subtable_avx512_gather_probe is split in two, because it also
  needs to do its own magic,
- while moving those helpers, prefer direct calls to cpu_has_isa and
  avoid cast to intermediate integer variables when a simple boolean
  is enough,

Fixes: 352b6c7116cd ("dpif-lookup: add avx512 gather implementation.")
Fixes: abb807e27dd4 ("dpif-netdev: Add command to switch dpif implementation.")
Fixes: 250ceddcc2d0 ("dpif-netdev/mfex: Add AVX512 based optimized miniflow 
extract")
Fixes: b366fa2f4947 ("dpif-netdev: Call cpuid for x86 isa availability.")
Reported-at: https://bugzilla.redhat.com/2100393
Reported-by: Ales Musil 
Co-authored-by: Ales Musil 
Signed-off-by: Ales Musil 
Signed-off-by: David Marchand 
---
 lib/dpif-netdev-lookup-avx512-gather.c |  8 +---
 lib/dpif-netdev-lookup.c   | 13 +
 lib/dpif-netdev-lookup.h   |  2 +-
 3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/lib/dpif-netdev-lookup-avx512-gather.c 
b/lib/dpif-netdev-lookup-avx512-gather.c
index 5e3634249d..55fcc327b0 100644
--- a/lib/dpif-netdev-lookup-avx512-gather.c
+++ b/lib/dpif-netdev-lookup-avx512-gather.c
@@ -237,16 +237,10 @@ dpcls_avx512_gather_mf_any(struct dpcls_subtable 
*subtable, uint32_t keys_map,
 }
 
 dpcls_subtable_lookup_func
-dpcls_subtable_avx512_gather_probe(uint32_t u0_bits, uint32_t u1_bits)
+dpcls_subtable_avx512_gather_probe__(uint32_t u0_bits, uint32_t u1_bits)
 {
 dpcls_subtable_lookup_func f = NULL;
 
-int avx512f_available = dpdk_get_cpu_has_isa("x86_64", "avx512f");
-int bmi2_available = dpdk_get_cpu_has_isa("x86_64", "bmi2");
-if (!avx512f_available || !bmi2_available) {
-return NULL;
-}
-
 CHECK_LOOKUP_FUNCTION(5, 1);
 CHECK_LOOKUP_FUNCTION(4, 1);
 CHECK_LOOKUP_FUNCTION(4, 0);
diff --git a/lib/dpif-netdev-lookup.c b/lib/dpif-netdev-lookup.c
index bd0a99abe7..c19ac8dd37 100644
--- a/lib/dpif-netdev-lookup.c
+++ b/lib/dpif-netdev-lookup.c
@@ -22,6 +22,19 @@
 
 VLOG_DEFINE_THIS_MODULE(dpif_netdev_lookup);
 
+#if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__)
+static dpcls_subtable_lookup_func
+dpcls_subtable_avx512_gather_probe(uint32_t u0_bits, uint32_t u1_bits)
+{
+if (!dpdk_get_cpu_has_isa("x86_64", "avx512f")
+|| !dpdk_get_cpu_has_isa("x86_64", "bmi2")) {
+return NULL;
+}
+
+return dpcls_subtable_avx512_gather_probe__(u0_bits, u1_bits);
+}
+#endif
+
 /* Actual list of implementations goes here */
 static struct dpcls_subtable_lookup_info_t subtable_lookups[] = {
 /* The autovalidator implementation will not be used by default, it must
diff --git a/lib/dpif-netdev-lookup.h b/lib/dpif-netdev-lookup.h
index bd72aa29b8..2c6c6532da 100644
--- a/lib/dpif-netdev-lookup.h
+++ b/lib/dpif-netdev-lookup.h
@@ -44,7 +44,7 @@ dpcls_subtable_generic_probe(uint32_t u0_bit_count, uint32_t 
u1_bit_count);
 
 /* Probe function for AVX-512 gather implementation */
 dpcls_subtable_lookup_func
-dpcls_subtable_avx512_gather_probe(uint32_t u0_bit_cnt, uint32_t u1_bit_cnt);
+dpcls_subtable_avx512_gather_probe__(uint32_t u0_bit_cnt, uint32_t u1_bit_cnt);
 
 
 /* Subtable registration and iteration helpers */
-- 
2.36.1

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

[ovs-dev] [branch-2.14] dpif-netdev: Refactor AVX512 runtime checks.

2022-06-29 Thread David Marchand

[ original commit fe171e4f109f001f07b867756a261d898f0d2cfc ]

As described in the bugzilla below, cpu_has_isa code may be compiled
with some AVX512 instructions in it, because cpu.c is built as part of
the libopenvswitchavx512.
This is a problem when this function (supposed to probe for AVX512
instructions availability) is invoked from generic OVS code, on older
CPUs that don't support them.

For the same reason, dpcls_subtable_avx512_gather_probe,
dp_netdev_input_outer_avx512_probe, mfex_avx512_probe and
mfex_avx512_vbmi_probe are potential runtime bombs and can't either be
built as part of libopenvswitchavx512.

Move cpu.c to be part of the "normal" libopenvswitch.
And move other helpers in generic OVS code.

Note:
- dpcls_subtable_avx512_gather_probe is split in two, because it also
  needs to do its own magic,
- while moving those helpers, prefer direct calls to cpu_has_isa and
  avoid cast to intermediate integer variables when a simple boolean
  is enough,

Fixes: 352b6c7116cd ("dpif-lookup: add avx512 gather implementation.")
Fixes: abb807e27dd4 ("dpif-netdev: Add command to switch dpif implementation.")
Fixes: 250ceddcc2d0 ("dpif-netdev/mfex: Add AVX512 based optimized miniflow 
extract")
Fixes: b366fa2f4947 ("dpif-netdev: Call cpuid for x86 isa availability.")
Reported-at: https://bugzilla.redhat.com/2100393
Reported-by: Ales Musil 
Co-authored-by: Ales Musil 
Signed-off-by: Ales Musil 
Signed-off-by: David Marchand 
---
 lib/dpif-netdev-lookup-avx512-gather.c |  8 +---
 lib/dpif-netdev-lookup.c   | 13 +
 lib/dpif-netdev-lookup.h   |  2 +-
 3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/lib/dpif-netdev-lookup-avx512-gather.c 
b/lib/dpif-netdev-lookup-avx512-gather.c
index 5e3634249d..55fcc327b0 100644
--- a/lib/dpif-netdev-lookup-avx512-gather.c
+++ b/lib/dpif-netdev-lookup-avx512-gather.c
@@ -237,16 +237,10 @@ dpcls_avx512_gather_mf_any(struct dpcls_subtable 
*subtable, uint32_t keys_map,
 }
 
 dpcls_subtable_lookup_func
-dpcls_subtable_avx512_gather_probe(uint32_t u0_bits, uint32_t u1_bits)
+dpcls_subtable_avx512_gather_probe__(uint32_t u0_bits, uint32_t u1_bits)
 {
 dpcls_subtable_lookup_func f = NULL;
 
-int avx512f_available = dpdk_get_cpu_has_isa("x86_64", "avx512f");
-int bmi2_available = dpdk_get_cpu_has_isa("x86_64", "bmi2");
-if (!avx512f_available || !bmi2_available) {
-return NULL;
-}
-
 CHECK_LOOKUP_FUNCTION(5, 1);
 CHECK_LOOKUP_FUNCTION(4, 1);
 CHECK_LOOKUP_FUNCTION(4, 0);
diff --git a/lib/dpif-netdev-lookup.c b/lib/dpif-netdev-lookup.c
index bd0a99abe7..c19ac8dd37 100644
--- a/lib/dpif-netdev-lookup.c
+++ b/lib/dpif-netdev-lookup.c
@@ -22,6 +22,19 @@
 
 VLOG_DEFINE_THIS_MODULE(dpif_netdev_lookup);
 
+#if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__)
+static dpcls_subtable_lookup_func
+dpcls_subtable_avx512_gather_probe(uint32_t u0_bits, uint32_t u1_bits)
+{
+if (!dpdk_get_cpu_has_isa("x86_64", "avx512f")
+|| !dpdk_get_cpu_has_isa("x86_64", "bmi2")) {
+return NULL;
+}
+
+return dpcls_subtable_avx512_gather_probe__(u0_bits, u1_bits);
+}
+#endif
+
 /* Actual list of implementations goes here */
 static struct dpcls_subtable_lookup_info_t subtable_lookups[] = {
 /* The autovalidator implementation will not be used by default, it must
diff --git a/lib/dpif-netdev-lookup.h b/lib/dpif-netdev-lookup.h
index bd72aa29b8..2c6c6532da 100644
--- a/lib/dpif-netdev-lookup.h
+++ b/lib/dpif-netdev-lookup.h
@@ -44,7 +44,7 @@ dpcls_subtable_generic_probe(uint32_t u0_bit_count, uint32_t 
u1_bit_count);
 
 /* Probe function for AVX-512 gather implementation */
 dpcls_subtable_lookup_func
-dpcls_subtable_avx512_gather_probe(uint32_t u0_bit_cnt, uint32_t u1_bit_cnt);
+dpcls_subtable_avx512_gather_probe__(uint32_t u0_bit_cnt, uint32_t u1_bit_cnt);
 
 
 /* Subtable registration and iteration helpers */
-- 
2.36.1

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

Re: [ovs-dev] [PATCH] dpif-netdev: Fix leak of AVX512 DPIF scratch pad.

2022-06-29 Thread Amber, Kumar

Hi Ilya,


> -Original Message-
> From: Ilya Maximets 
> Sent: Wednesday, June 29, 2022 5:55 PM
> To: Eelco Chaudron 
> Cc: i.maxim...@ovn.org; ovs-dev@openvswitch.org; Flavio Leitner
> ; Amber, Kumar 
> Subject: Re: [ovs-dev] [PATCH] dpif-netdev: Fix leak of AVX512 DPIF scratch
> pad.
> 
> On 6/29/22 13:45, Eelco Chaudron wrote:
> >
> >
> > On 29 Jun 2022, at 12:22, Ilya Maximets wrote:
> >
> >> dp_netdev_input_outer_avx512 allocates a 16KB scratch pad per PMD
> >> thread, but it's never freed.  This may cause significant memory
> >> drain in dynamic environments.
> >>
> >>   ==4068109==ERROR: LeakSanitizer: detected memory leaks
> >>
> >>   Direct leak of 38656 byte(s) in 2 object(s) allocated from:
> >>0 0xf069fd in posix_memalign (vswitchd/ovs-vswitchd+0xf069fd)
> >>1 0x1d7ed14 in xmalloc_size_align lib/util.c:254:13
> >>2 0x1d7ed14 in xmalloc_pagealign lib/util.c:352:12
> >>3 0x2098254 in dp_netdev_input_outer_avx512 lib/dpif-netdev-
> avx512.c:69:17
> >>4 0x191591a in dp_netdev_process_rxq_port lib/dpif-netdev.c:5332:19
> >>5 0x190a961 in pmd_thread_main lib/dpif-netdev.c:6963:17
> >>6 0x1c4b69a in ovsthread_wrapper lib/ovs-thread.c:422:12
> >>7 0x7fd5ea6f1179 in start_thread pthread_create.c
> >>
> >>  SUMMARY: AddressSanitizer: 38656 byte(s) leaked in 2 allocation(s).
> >>
> >> Fixes: 9ac84a1a3698 ("dpif-avx512: Add ISA implementation of dpif.")
> >> Signed-off-by: Ilya Maximets 
> >> ---
> >
> > Good catch... Do you have some script to run all kinds of sanitizers?
> 
> No.  For this one I just built with
>   CFLAGS="-msse4.2 -O1 -fno-omit-frame-pointer -fno-common -g -
> fsanitize=address -fsanitize=undefined" CC=clang
> 
> And then tried to run with asan environment variables:
> 
>   ASAN_OPTIONS='detect_leaks=1:abort_on_error=true:log_path=asan'
>   UBSAN_OPTIONS='print_stacktrace=1:halt_on_error=true:log_path=ubsan'
> 
> LeakSanitizer is part of the AddressSanitizer, you just need to ask to detect
> leaks (we're disabling that by default in tests/atlocal.in).
> 
> I didn't check, but I guess, you should be able to reproduce the issue by just
> building with Asan and running:
> 
>   ASAN_OPTIONS='detect_leaks=1:abort_on_error=true:log_path=asan' make
> check-dpdk
> 
> >
> >
> > Acked-by: Eelco Chaudron 
> >
Acked-by: Kumar Amber 

The changes look good to me, and Thanks again for fixing it.

Regards
Amber


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

Re: [ovs-dev] [PATCH ovn] ovn-northd.at: Add OVN_FOR_EACH_NORTHD around LR NB Static_MAC_Binding table

2022-06-29 Thread Dumitru Ceara

On 6/29/22 07:47, Ales Musil wrote:
> This is continuation after discussion during review on
> 80187a803 (ovn-northd: Add flow to use eth.src if nd.tll is 0
> in put_nd() action.)
> 
> Signed-off-by: Ales Musil 
> ---

Looks good to me, thanks!

Acked-by: Dumitru Ceara 

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

Re: [ovs-dev] [PATCH ovn v2] northd.c: Add flow to skip put_nd action if ip6.src or nd.sll is 0

2022-06-29 Thread Dumitru Ceara

On 6/29/22 07:47, Ales Musil wrote:
> The ip6.src or nd.sll does not have to be always set.
> According to rfc4861:
> 
> Source Address
>  Either an address assigned to the interface from
>  which this message is sent or (if Duplicate Address
>  Detection is in progress [ADDRCONF]) the
>  unspecified address.
> 
> Source link-layer address
>  The link-layer address for the sender.  MUST NOT be
>  included when the source IP address is the
>  unspecified address.  Otherwise, on link layers
>  that have addresses this option MUST be included in
>  multicast solicitations and SHOULD be included in
>  unicast solicitations.
> 
> Add rule that avoids adding MAC binding is either of those
> is 0. This is continuation after discussion during review on
> 80187a803 (ovn-northd: Add flow to use eth.src if nd.tll is 0
> in put_nd() action.)
> 
> Signed-off-by: Ales Musil 
> ---

Looks good to me, thanks!

Acked-by: Dumitru Ceara 

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

Re: [ovs-dev] [PATCH] dpif-netdev: Fix leak of AVX512 DPIF scratch pad.

2022-06-29 Thread Ilya Maximets

On 6/29/22 13:45, Eelco Chaudron wrote:
> 
> 
> On 29 Jun 2022, at 12:22, Ilya Maximets wrote:
> 
>> dp_netdev_input_outer_avx512 allocates a 16KB scratch pad per PMD
>> thread, but it's never freed.  This may cause significant memory
>> drain in dynamic environments.
>>
>>   ==4068109==ERROR: LeakSanitizer: detected memory leaks
>>
>>   Direct leak of 38656 byte(s) in 2 object(s) allocated from:
>>0 0xf069fd in posix_memalign (vswitchd/ovs-vswitchd+0xf069fd)
>>1 0x1d7ed14 in xmalloc_size_align lib/util.c:254:13
>>2 0x1d7ed14 in xmalloc_pagealign lib/util.c:352:12
>>3 0x2098254 in dp_netdev_input_outer_avx512 lib/dpif-netdev-avx512.c:69:17
>>4 0x191591a in dp_netdev_process_rxq_port lib/dpif-netdev.c:5332:19
>>5 0x190a961 in pmd_thread_main lib/dpif-netdev.c:6963:17
>>6 0x1c4b69a in ovsthread_wrapper lib/ovs-thread.c:422:12
>>7 0x7fd5ea6f1179 in start_thread pthread_create.c
>>
>>  SUMMARY: AddressSanitizer: 38656 byte(s) leaked in 2 allocation(s).
>>
>> Fixes: 9ac84a1a3698 ("dpif-avx512: Add ISA implementation of dpif.")
>> Signed-off-by: Ilya Maximets 
>> ---
> 
> Good catch... Do you have some script to run all kinds of sanitizers?

No.  For this one I just built with
  CFLAGS="-msse4.2 -O1 -fno-omit-frame-pointer -fno-common -g 
-fsanitize=address -fsanitize=undefined" CC=clang

And then tried to run with asan environment variables:

  ASAN_OPTIONS='detect_leaks=1:abort_on_error=true:log_path=asan'
  UBSAN_OPTIONS='print_stacktrace=1:halt_on_error=true:log_path=ubsan'

LeakSanitizer is part of the AddressSanitizer, you just need to ask to
detect leaks (we're disabling that by default in tests/atlocal.in).

I didn't check, but I guess, you should be able to reproduce the
issue by just building with Asan and running:

  ASAN_OPTIONS='detect_leaks=1:abort_on_error=true:log_path=asan' make 
check-dpdk

> 
> 
> Acked-by: Eelco Chaudron 
> 

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

Re: [ovs-dev] [PATCH] dpif-netdev: Fix leak of AVX512 DPIF scratch pad.

2022-06-29 Thread Eelco Chaudron




On 29 Jun 2022, at 12:22, Ilya Maximets wrote:

> dp_netdev_input_outer_avx512 allocates a 16KB scratch pad per PMD
> thread, but it's never freed.  This may cause significant memory
> drain in dynamic environments.
>
>   ==4068109==ERROR: LeakSanitizer: detected memory leaks
>
>   Direct leak of 38656 byte(s) in 2 object(s) allocated from:
>0 0xf069fd in posix_memalign (vswitchd/ovs-vswitchd+0xf069fd)
>1 0x1d7ed14 in xmalloc_size_align lib/util.c:254:13
>2 0x1d7ed14 in xmalloc_pagealign lib/util.c:352:12
>3 0x2098254 in dp_netdev_input_outer_avx512 lib/dpif-netdev-avx512.c:69:17
>4 0x191591a in dp_netdev_process_rxq_port lib/dpif-netdev.c:5332:19
>5 0x190a961 in pmd_thread_main lib/dpif-netdev.c:6963:17
>6 0x1c4b69a in ovsthread_wrapper lib/ovs-thread.c:422:12
>7 0x7fd5ea6f1179 in start_thread pthread_create.c
>
>  SUMMARY: AddressSanitizer: 38656 byte(s) leaked in 2 allocation(s).
>
> Fixes: 9ac84a1a3698 ("dpif-avx512: Add ISA implementation of dpif.")
> Signed-off-by: Ilya Maximets 
> ---

Good catch... Do you have some script to run all kinds of sanitizers?


Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH v2] dpif-netdev: Refactor AVX512 runtime checks.

2022-06-29 Thread Ilya Maximets

On 6/29/22 10:16, Ales Musil wrote:
> 
> 
> On Wed, Jun 29, 2022 at 9:32 AM David Marchand  > wrote:
> 
> As described in the bugzilla below, cpu_has_isa code may be compiled
> with some AVX512 instructions in it, because cpu.c is built as part of
> the libopenvswitchavx512.
> This is a problem when this function (supposed to probe for AVX512
> instructions availability) is invoked from generic OVS code, on older
> CPUs that don't support them.
> 
> For the same reason, dpcls_subtable_avx512_gather_probe,
> dp_netdev_input_outer_avx512_probe, mfex_avx512_probe and
> mfex_avx512_vbmi_probe are potential runtime bombs and can't either be
> built as part of libopenvswitchavx512.
> 
> Move cpu.c to be part of the "normal" libopenvswitch.
> And move other helpers in generic OVS code.
> 
> Note:
> - dpcls_subtable_avx512_gather_probe is split in two, because it also
>   needs to do its own magic,
> - while moving those helpers, prefer direct calls to cpu_has_isa and
>   avoid cast to intermediate integer variables when a simple boolean
>   is enough,
> 
> Fixes: 352b6c7116cd ("dpif-lookup: add avx512 gather implementation.")
> Fixes: abb807e27dd4 ("dpif-netdev: Add command to switch dpif 
> implementation.")
> Fixes: 250ceddcc2d0 ("dpif-netdev/mfex: Add AVX512 based optimized 
> miniflow extract")
> Fixes: b366fa2f4947 ("dpif-netdev: Call cpuid for x86 isa availability.")
> Reported-at: https://bugzilla.redhat.com/2100393 
> 
> Reported-by: Ales Musil mailto:amu...@redhat.com>>
> Co-authored-by: Ales Musil mailto:amu...@redhat.com>>
> Signed-off-by: Ales Musil mailto:amu...@redhat.com>>
> Signed-off-by: David Marchand  >
> Acked-by: Sunil Pai G  >
> ---
> Changes since v1:
> - restored Ales as co-author,
> - added more Fixes: lines: backports go back to 2.14 and are not that 
> difficult
>   to do, but I can help if needed,
> - fixed one indent issue,
> 
> 
> Acked-by: Ales Musil mailto:sunil.pa...@intel.com>>

Rich-text editors are dangerous. :)

Applied to master and backported to 2.17.

David, if you can prepare backports for older branches, that would be great.
Thanks!

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

Re: [ovs-dev] [PATCH] dpif-netdev: Fix leak of AVX512 DPIF scratch pad.

2022-06-29 Thread David Marchand

On Wed, Jun 29, 2022 at 12:23 PM Ilya Maximets  wrote:
>
> dp_netdev_input_outer_avx512 allocates a 16KB scratch pad per PMD
> thread, but it's never freed.  This may cause significant memory
> drain in dynamic environments.
>
>   ==4068109==ERROR: LeakSanitizer: detected memory leaks
>
>   Direct leak of 38656 byte(s) in 2 object(s) allocated from:
>0 0xf069fd in posix_memalign (vswitchd/ovs-vswitchd+0xf069fd)
>1 0x1d7ed14 in xmalloc_size_align lib/util.c:254:13
>2 0x1d7ed14 in xmalloc_pagealign lib/util.c:352:12
>3 0x2098254 in dp_netdev_input_outer_avx512 lib/dpif-netdev-avx512.c:69:17
>4 0x191591a in dp_netdev_process_rxq_port lib/dpif-netdev.c:5332:19
>5 0x190a961 in pmd_thread_main lib/dpif-netdev.c:6963:17
>6 0x1c4b69a in ovsthread_wrapper lib/ovs-thread.c:422:12
>7 0x7fd5ea6f1179 in start_thread pthread_create.c
>
>  SUMMARY: AddressSanitizer: 38656 byte(s) leaked in 2 allocation(s).
>
> Fixes: 9ac84a1a3698 ("dpif-avx512: Add ISA implementation of dpif.")
> Signed-off-by: Ilya Maximets 

Reviewed-by: David Marchand 


-- 
David Marchand

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

[ovs-dev] [PATCH v2] tests: Add OVS-DPDK QoS unit tests

2022-06-29 Thread Michael Phelan

This adds 4 new unit tests to the 'check-dpdk' subsystem that will
test Quality of Service (QoS) functionality.

Signed-off-by: Michael Phelan 

---
v2:
  - Added check to confirm policer was removed correctly in phy and vport tests.
  - Removed unnecessary error catch in phy test.
  - Added egress to various comments to improve clarity.
---
---
 tests/system-dpdk.at | 147 +++
 1 file changed, 147 insertions(+)

diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
index 7d2715c4a..41987b7b5 100644
--- a/tests/system-dpdk.at
+++ b/tests/system-dpdk.at
@@ -222,6 +222,153 @@ OVS_VSWITCHD_STOP("m4_join([], 
[SYSTEM_DPDK_ALLOWED_LOGS], [
 AT_CLEANUP
 dnl --
 
+
+
+dnl --
+dnl QoS create delete phy port
+AT_SETUP([OVS-DPDK - QoS create delete phy port])
+AT_KEYWORDS([dpdk])
+
+OVS_DPDK_PRE_PHY_SKIP()
+OVS_DPDK_START()
+
+dnl Add userspace bridge and attach it to OVS and add egress policer
+AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10 datapath_type=netdev])
+AT_CHECK([ovs-vsctl add-port br10 phy0 -- set Interface phy0 type=dpdk 
options:dpdk-devargs=$(cat PCI_ADDR)], [], [stdout], [stderr])
+OVS_WAIT_UNTIL([ovs-vsctl set port phy0 qos=@newqos -- --id=@newqos create qos 
type=egress-policer other-config:cir=125 other-config:cbs=2048])
+AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show phy0], [], [stdout])
+sleep 2
+
+dnl Fail if egress policer could not be created
+AT_FAIL_IF([grep "Could not create rte meter for egress policer" 
ovs-vswitchd.log], [], [stdout])
+
+dnl remove egress policer
+AT_CHECK([ovs-vsctl destroy QoS phy0 -- clear Port phy0 qos])
+
+dnl check egress policer was removed correctly
+AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show phy0], [], [stdout])
+AT_CHECK([egrep 'QoS not configured on phy0' stdout], [], [stdout])
+
+dnl Clean up
+AT_CHECK([ovs-vsctl del-port br10 phy0], [], [stdout], [stderr])
+OVS_VSWITCHD_STOP("[SYSTEM_DPDK_ALLOWED_LOGS]")
+AT_CLEANUP
+dnl --
+
+
+
+dnl --
+dnl QoS create delete vport port
+AT_SETUP([OVS-DPDK - QoS create delete vport port])
+AT_KEYWORDS([dpdk])
+
+OVS_DPDK_PRE_CHECK()
+OVS_DPDK_START()
+
+dnl Add userspace bridge and attach it to OVS and add egress policer
+AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10 datapath_type=netdev])
+AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuserclient0 -- set Interface 
dpdkvhostuserclient0 type=dpdkvhostuserclient 
options:vhost-server-path=$OVS_RUNDIR/dpdkvhostclient0], [], [stdout], [stderr])
+OVS_WAIT_UNTIL([ovs-vsctl set port dpdkvhostuserclient0 qos=@newqos -- 
--id=@newqos create qos type=egress-policer other-config:cir=125 \
+  other-config:cbs=2048])
+AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show dpdkvhostuserclient0], [], 
[stdout])
+sleep 2
+
+dnl Fail if egress policer could not be created
+AT_FAIL_IF([grep "Could not create rte meter for egress policer" 
ovs-vswitchd.log], [], [stdout])
+
+dnl remove egress policer
+AT_CHECK([ovs-vsctl destroy QoS dpdkvhostuserclient0 -- clear Port 
dpdkvhostuserclient0 qos])
+
+dnl check egress policer was removed correctly
+AT_CHECK([ovs-appctl -t ovs-vswitchd qos/show dpdkvhostuserclient0], [], 
[stdout])
+AT_CHECK([egrep 'QoS not configured on dpdkvhostuserclient0' stdout], [], 
[stdout])
+
+dnl Parse log file
+AT_CHECK([grep "VHOST_CONFIG: vhost-user client: socket created" 
ovs-vswitchd.log], [], [stdout])
+AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in 'client' 
mode, using client socket" ovs-vswitchd.log], [], [stdout])
+AT_CHECK([grep "VHOST_CONFIG: $OVS_RUNDIR/dpdkvhostclient0: reconnecting..." 
ovs-vswitchd.log], [], [stdout])
+
+dnl Clean up
+AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout], 
[stderr])
+OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
+\@VHOST_CONFIG: failed to connect to $OVS_RUNDIR/dpdkvhostclient0: No such 
file or directory@d
+])")
+AT_CLEANUP
+dnl --
+
+
+
+dnl --
+dnl QoS no cir
+AT_SETUP([OVS-DPDK - QoS no cir])
+AT_KEYWORDS([dpdk])
+
+OVS_DPDK_PRE_CHECK()
+OVS_DPDK_START()
+
+dnl Add userspace bridge and attach it to OVS and add egress policer
+AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10 datapath_type=netdev])
+AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuserclient0 -- set Interface 
dpdkvhostuserclient0 type=dpdkvhostuserclient 
options:vhost-server-path=$OVS_RUNDIR/dpdkvhostclient0], [], [stdout], [stderr])
+OVS_WAIT_UNTIL([ovs-vsctl set port dpdkvhostuserclient0 qos=@newqos -- 
--id=@newqos create qos type=egress-policer other-config:cbs=2048])
+sleep 2
+
+dnl check egress policer was n

[ovs-dev] [PATCH v4] tests: Add ovs-dpdk rate limiting unit tests.

2022-06-29 Thread Michael Phelan

From: Seamus Ryan 

This adds 4 new unit tests to the 'check-dpdk' subsystem that will
test rate limiting functionality.

Signed-off-by: Seamus Ryan 
Signed-off-by: Michael Phelan 
Co-authored-by: Michael Phelan 

---
v4:
  - Changed egress to ingress in error check.
  - Added ingress to various comments to improve clarity.
  - Removed unnecessary error catch in phy test.

v3:
  - Removed NEWS entry.
  - Added check to catch error if policer fails to be created.

v2:
  - Fixed dn1 typo and spacing issues.
  - Added check for removing burst and rate values.
  - Renamed tests to specify ingress policing.
---
---
 tests/system-dpdk.at | 162 +++
 1 file changed, 162 insertions(+)

diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
index 7d2715c4a..b036580de 100644
--- a/tests/system-dpdk.at
+++ b/tests/system-dpdk.at
@@ -222,6 +222,168 @@ OVS_VSWITCHD_STOP("m4_join([], 
[SYSTEM_DPDK_ALLOWED_LOGS], [
 AT_CLEANUP
 dnl --
 
+
+
+dnl --
+dnl Ingress policing create delete phy port
+AT_SETUP([OVS-DPDK - Ingress policing create delete phy port])
+AT_KEYWORDS([dpdk])
+
+OVS_DPDK_PRE_PHY_SKIP()
+OVS_DPDK_START()
+
+dnl Add userspace bridge and attach it to OVS and add policer
+AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10 datapath_type=netdev])
+AT_CHECK([ovs-vsctl add-port br10 phy0 -- set Interface phy0 type=dpdk 
options:dpdk-devargs=$(cat PCI_ADDR)], [], [stdout], [stderr])
+AT_CHECK([ovs-vsctl set interface phy0 ingress_policing_rate=1 
ingress_policing_burst=1000])
+AT_CHECK([ovs-vsctl show], [], [stdout])
+sleep 2
+
+dnl Fail if policer could not be created
+AT_FAIL_IF([grep "Could not create rte meter for ingress policer" 
ovs-vswitchd.log], [], [stdout])
+
+dnl remove policer
+AT_CHECK([ovs-vsctl set interface phy0 ingress_policing_rate=0 
ingress_policing_burst=0])
+
+dnl check policer was removed correctly
+AT_CHECK([ovs-vsctl list interface phy0], [], [stdout])
+AT_CHECK([egrep 'ingress_policing_burst: 0' stdout], [], [stdout])
+
+AT_CHECK([ovs-vsctl list interface phy0], [], [stdout])
+AT_CHECK([egrep 'ingress_policing_rate: 0' stdout], [], [stdout])
+
+dnl Clean up
+AT_CHECK([ovs-vsctl del-port br10 phy0], [], [stdout], [stderr])
+OVS_VSWITCHD_STOP("[SYSTEM_DPDK_ALLOWED_LOGS]")
+AT_CLEANUP
+dnl --
+
+
+
+dnl --
+dnl Ingress policing create delete vport port
+AT_SETUP([OVS-DPDK - Ingress policing create delete vport port])
+AT_KEYWORDS([dpdk])
+
+OVS_DPDK_PRE_CHECK()
+OVS_DPDK_START()
+
+dnl Add userspace bridge and attach it to OVS and add ingress policer
+AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10 datapath_type=netdev])
+AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuserclient0 -- set Interface 
dpdkvhostuserclient0 type=dpdkvhostuserclient 
options:vhost-server-path=$OVS_RUNDIR/dpdkvhostclient0], [], [stdout], [stderr])
+AT_CHECK([ovs-vsctl set interface dpdkvhostuserclient0 
ingress_policing_rate=1 ingress_policing_burst=1000])
+AT_CHECK([ovs-vsctl show], [], [stdout])
+sleep 2
+
+dnl Fail if ingress policer could not be created
+AT_FAIL_IF([grep "Could not create rte meter for ingress policer" 
ovs-vswitchd.log], [], [stdout])
+
+dnl remove ingress policer
+AT_CHECK([ovs-vsctl set interface dpdkvhostuserclient0 ingress_policing_rate=0 
ingress_policing_burst=0])
+
+dnl check ingress policer was removed correctly
+AT_CHECK([ovs-vsctl list interface dpdkvhostuserclient0], [], [stdout])
+AT_CHECK([egrep 'ingress_policing_burst: 0' stdout], [], [stdout])
+
+AT_CHECK([ovs-vsctl list interface dpdkvhostuserclient0], [], [stdout])
+AT_CHECK([egrep 'ingress_policing_rate: 0' stdout], [], [stdout])
+
+dnl Parse log file
+AT_CHECK([grep "VHOST_CONFIG: vhost-user client: socket created" 
ovs-vswitchd.log], [], [stdout])
+AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in 'client' 
mode, using client socket" ovs-vswitchd.log], [], [stdout])
+AT_CHECK([grep "VHOST_CONFIG: $OVS_RUNDIR/dpdkvhostclient0: reconnecting..." 
ovs-vswitchd.log], [], [stdout])
+
+dnl Clean up
+AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout], 
[stderr])
+OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
+\@VHOST_CONFIG: failed to connect to $OVS_RUNDIR/dpdkvhostclient0: No such 
file or directory@d
+])")
+AT_CLEANUP
+dnl --
+
+
+
+dnl --
+dnl Ingress policing no policing rate
+AT_SETUP([OVS-DPDK - Ingress policing no policing rate])
+AT_KEYWORDS([dpdk])
+
+OVS_DPDK_PRE_CHECK()
+OVS_DPDK_START()
+
+dnl Add userspace bridge and attach it to OVS and add ingress policer
+AT_CHECK([ovs-vsctl add-br br10 -- se

[ovs-dev] [PATCH] dpif-netdev: Fix leak of AVX512 DPIF scratch pad.

2022-06-29 Thread Ilya Maximets

dp_netdev_input_outer_avx512 allocates a 16KB scratch pad per PMD
thread, but it's never freed.  This may cause significant memory
drain in dynamic environments.

  ==4068109==ERROR: LeakSanitizer: detected memory leaks

  Direct leak of 38656 byte(s) in 2 object(s) allocated from:
   0 0xf069fd in posix_memalign (vswitchd/ovs-vswitchd+0xf069fd)
   1 0x1d7ed14 in xmalloc_size_align lib/util.c:254:13
   2 0x1d7ed14 in xmalloc_pagealign lib/util.c:352:12
   3 0x2098254 in dp_netdev_input_outer_avx512 lib/dpif-netdev-avx512.c:69:17
   4 0x191591a in dp_netdev_process_rxq_port lib/dpif-netdev.c:5332:19
   5 0x190a961 in pmd_thread_main lib/dpif-netdev.c:6963:17
   6 0x1c4b69a in ovsthread_wrapper lib/ovs-thread.c:422:12
   7 0x7fd5ea6f1179 in start_thread pthread_create.c

 SUMMARY: AddressSanitizer: 38656 byte(s) leaked in 2 allocation(s).

Fixes: 9ac84a1a3698 ("dpif-avx512: Add ISA implementation of dpif.")
Signed-off-by: Ilya Maximets 
---
 lib/dpif-netdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index f46b9fe18..d09138f2c 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -7523,6 +7523,7 @@ dp_netdev_destroy_pmd(struct dp_netdev_pmd_thread *pmd)
 seq_destroy(pmd->reload_seq);
 ovs_mutex_destroy(&pmd->port_mutex);
 ovs_mutex_destroy(&pmd->bond_mutex);
+free(pmd->netdev_input_func_userdata);
 free(pmd);
 }
 
-- 
2.34.3

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

Re: [ovs-dev] [PATCH ovn] patch.c: Avoid patch interface deletion & recreation during restart.

2022-06-29 Thread Dumitru Ceara

On 6/28/22 19:18, Han Zhou wrote:
> On Tue, Jun 28, 2022 at 8:20 AM Dumitru Ceara  wrote:
>>
>> On 6/28/22 02:49, Han Zhou wrote:
>>> When ovn-controller is restarted, it may need multiple iterations of
>>> main loop before completely download all related data from SB DB,
>>> especially when ovn-monitor-all=false, so after restart, before it sees
>>> the related localnet ports from SB DB, it treats the related patch
>>> ports on the chassis as not needed, and deleted them. Later when it
>>> downloads thoses port-bindings it recreates them.  For a graceful
>>> upgrade, we don't this to happen, because it would break the traffic.
>>>
>>> This is especially problematic at ovn-k8s setups because the external
>>> side of the patch port is used to program some flows for external
>>> network communication. When the patch ports are recreated, the OF port
>>> number changes and those flows are broken. The CMS would detect and fix
>>> the flows but it would result in even longer downtime.
>>>
>>> This patch postpone the deletion of the patch ports, with the assumption
>>> that leaving the unused ports on the chassis for little longer is not
>>> harmful.
>>>
>>> Signed-off-by: Han Zhou 
>>> ---
>>>  controller/patch.c  | 15 -
>>>  tests/ovn-controller.at | 71 +
>>>  2 files changed, 85 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/controller/patch.c b/controller/patch.c
>>> index ed831302c..9faae5879 100644
>>> --- a/controller/patch.c
>>> +++ b/controller/patch.c
>>> @@ -307,11 +307,24 @@ patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
>>>
>>>  /* Now 'existing_ports' only still contains patch ports that exist
> in the
>>>   * database but shouldn't.  Delete them from the database. */
>>> +
>>> +/* Wait for some iterations before really deleting any patch
> ports, because
>>> + * with conditional monitoring it is possible that related SB data
> is not
>>> + * completely downloaded yet after last restart of ovn-controller.
>>> + * Otherwise it may cause unncessary dataplane interruption during
>>> + * restart/upgrade. */
>>> +static int delete_patch_port_delay = 10;
>>
>> Hi Han,
>>
>> It's possible that ovn-controller wakes up 10 times in a row immediately
>> due to local OVSDB changes and doesn't process any new SB updates in
>> that time so 10 might not be enough in some cases.
>>
> 
> Thanks Dumitru for the review!
> In theory I agree with you 10 times is not 100% guaranteeing SB update
> completed if other things are triggering the wakeups. However, in practice,
> the purpose here is for the start/restart scenario. I think it is very
> unlikely that local OVSDB is changing that frequently at the same time when
> ovn-controller is being restarted. We have some similar logic (not ideal
> for sure) at vif-plug.c:vif_plug_run() for similar reasons.
> 

Ah I didn't know we do that already for vif-plug.  Thanks for pointing
it out!

>> Does it make sense to wait a given amount of time instead?  E.g., a few
>> seconds?  Can we make this configurable somehow?  Maybe an
>> ovn-controller command line argument to override the default?
> 
> Waiting for a given amount of time is also not ideal. It is possible that
> when ovn-controller starts the SB IDL is not connected (due to server side
> problems/ control plane network problems, etc.) so we don't know how long
> it should wait at all.
> 
> I am ok with adding command line arguments to adjust, but I'd really want
> to avoid that unless it is proved to be necessary. I'd rather use a bigger
> hardcoded value to avoid another knob which is not easy to understand by
> users - it should be something handled by the code itself.
> 

I understand your point against additional knobs.  Maybe we don't need
a new one.  What if we do a mixed approach?  We already have the
ovn-controller startup timestamp so we could have a single (hardcoded)
delay counter but also ensure that at least X seconds elapsed.  It might
be a bit over-engineered but what do you think of the following
incremental?

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 69615308e..153f742b1 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -863,11 +863,12 @@ static void
 store_nb_cfg(struct ovsdb_idl_txn *sb_txn, struct ovsdb_idl_txn *ovs_txn,
  const struct sbrec_chassis_private *chassis,
  const struct ovsrec_bridge *br_int,
- unsigned int delay_nb_cfg_report, int64_t startup_ts)
+ unsigned int delay_nb_cfg_report)
 {
 struct ofctrl_acked_seqnos *acked_nb_cfg_seqnos =
 ofctrl_acked_seqnos_get(ofctrl_seq_type_nb_cfg);
 uint64_t cur_cfg = acked_nb_cfg_seqnos->last_acked;
+int64_t startup_ts = daemon_startup_ts();
 
 if (ovs_txn && br_int
 && startup_ts != smap_get_ullong(&br_int->external_ids,
@@ -3811,7 +3812,6 @@ main(int argc, char *argv[])
 /* Main loop. */
 exiting = false;
 res

Re: [ovs-dev] [PATCH v2] dpif-netdev: Refactor AVX512 runtime checks.

2022-06-29 Thread Ales Musil

On Wed, Jun 29, 2022 at 9:32 AM David Marchand 
wrote:

> As described in the bugzilla below, cpu_has_isa code may be compiled
> with some AVX512 instructions in it, because cpu.c is built as part of
> the libopenvswitchavx512.
> This is a problem when this function (supposed to probe for AVX512
> instructions availability) is invoked from generic OVS code, on older
> CPUs that don't support them.
>
> For the same reason, dpcls_subtable_avx512_gather_probe,
> dp_netdev_input_outer_avx512_probe, mfex_avx512_probe and
> mfex_avx512_vbmi_probe are potential runtime bombs and can't either be
> built as part of libopenvswitchavx512.
>
> Move cpu.c to be part of the "normal" libopenvswitch.
> And move other helpers in generic OVS code.
>
> Note:
> - dpcls_subtable_avx512_gather_probe is split in two, because it also
>   needs to do its own magic,
> - while moving those helpers, prefer direct calls to cpu_has_isa and
>   avoid cast to intermediate integer variables when a simple boolean
>   is enough,
>
> Fixes: 352b6c7116cd ("dpif-lookup: add avx512 gather implementation.")
> Fixes: abb807e27dd4 ("dpif-netdev: Add command to switch dpif
> implementation.")
> Fixes: 250ceddcc2d0 ("dpif-netdev/mfex: Add AVX512 based optimized
> miniflow extract")
> Fixes: b366fa2f4947 ("dpif-netdev: Call cpuid for x86 isa availability.")
> Reported-at: https://bugzilla.redhat.com/2100393
> Reported-by: Ales Musil 
> Co-authored-by: Ales Musil 
> Signed-off-by: Ales Musil 
> Signed-off-by: David Marchand 
> Acked-by: Sunil Pai G 
> ---
> Changes since v1:
> - restored Ales as co-author,
> - added more Fixes: lines: backports go back to 2.14 and are not that
> difficult
>   to do, but I can help if needed,
> - fixed one indent issue,
>
> ---
>  lib/automake.mk|  4 +--
>  lib/dpif-netdev-avx512.c   | 14 -
>  lib/dpif-netdev-extract-avx512.c   | 43 --
>  lib/dpif-netdev-lookup-avx512-gather.c | 12 ++-
>  lib/dpif-netdev-lookup.c   | 16 ++
>  lib/dpif-netdev-lookup.h   |  3 +-
>  lib/dpif-netdev-private-dpif.c | 14 +
>  lib/dpif-netdev-private-dpif.h |  5 +--
>  lib/dpif-netdev-private-extract.c  | 41 
>  lib/dpif-netdev-private-extract.h  |  4 +--
>  10 files changed, 79 insertions(+), 77 deletions(-)
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index cb50578eb7..3b9e775d4e 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -36,8 +36,6 @@ lib_libopenvswitchavx512_la_CFLAGS = \
> -fPIC \
> $(AM_CFLAGS)
>  lib_libopenvswitchavx512_la_SOURCES = \
> -   lib/cpu.c \
> -   lib/cpu.h \
> lib/dpif-netdev-avx512.c
>  if HAVE_AVX512BW
>  lib_libopenvswitchavx512_la_CFLAGS += \
> @@ -92,6 +90,8 @@ lib_libopenvswitch_la_SOURCES = \
> lib/conntrack.h \
> lib/coverage.c \
> lib/coverage.h \
> +   lib/cpu.c \
> +   lib/cpu.h \
> lib/crc32c.c \
> lib/crc32c.h \
> lib/csum.c \
> diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
> index 11d9a00052..82a4138184 100644
> --- a/lib/dpif-netdev-avx512.c
> +++ b/lib/dpif-netdev-avx512.c
> @@ -20,7 +20,6 @@
>
>  #include 
>
> -#include "cpu.h"
>  #include "dpif-netdev.h"
>  #include "dpif-netdev-perf.h"
>  #include "dpif-netdev-private.h"
> @@ -59,19 +58,6 @@ struct dpif_userdata {
>  struct pkt_flow_meta pkt_meta[NETDEV_MAX_BURST];
>  };
>
> -int32_t
> -dp_netdev_input_outer_avx512_probe(void)
> -{
> -bool avx512f_available = cpu_has_isa(OVS_CPU_ISA_X86_AVX512F);
> -bool bmi2_available = cpu_has_isa(OVS_CPU_ISA_X86_BMI2);
> -
> -if (!avx512f_available || !bmi2_available) {
> -return -ENOTSUP;
> -}
> -
> -return 0;
> -}
> -
>  int32_t
>  dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread *pmd,
>   struct dp_packet_batch *packets,
> diff --git a/lib/dpif-netdev-extract-avx512.c
> b/lib/dpif-netdev-extract-avx512.c
> index f1919befd1..5029730292 100644
> --- a/lib/dpif-netdev-extract-avx512.c
> +++ b/lib/dpif-netdev-extract-avx512.c
> @@ -42,7 +42,6 @@
>  #include 
>  #include 
>
> -#include "cpu.h"
>  #include "flow.h"
>
>  #include "dpif-netdev-private-dpcls.h"
> @@ -690,47 +689,5 @@ DECLARE_MFEX_FUNC(ip_udp, PROFILE_ETH_IPV4_UDP)
>  DECLARE_MFEX_FUNC(ip_tcp, PROFILE_ETH_IPV4_TCP)
>  DECLARE_MFEX_FUNC(dot1q_ip_udp, PROFILE_ETH_VLAN_IPV4_UDP)
>  DECLARE_MFEX_FUNC(dot1q_ip_tcp, PROFILE_ETH_VLAN_IPV4_TCP)
> -
> -
> -static int32_t
> -avx512_isa_probe(uint32_t needs_vbmi)
> -{
> -static enum ovs_cpu_isa isa_required[] = {
> -OVS_CPU_ISA_X86_AVX512F,
> -OVS_CPU_ISA_X86_AVX512BW,
> -OVS_CPU_ISA_X86_BMI2,
> -};
> -
> -int32_t ret = 0;
> -for (uint32_t i = 0; i < ARRAY_SIZE(isa_required); i++) {
> -if (!cpu_has_isa(isa_required[i])) {
> -ret = -ENOTSUP;
> -}
> -}
> -
> -if (needs_vbmi) {
> -

Re: [ovs-dev] [PATCH ovn v3 1/1] Allow arbitrary args to be passed to called binary

2022-06-29 Thread Dumitru Ceara

On 6/28/22 17:22, Terry Wilson wrote:
> On Tue, Jun 28, 2022 at 4:39 AM Dumitru Ceara  wrote:
>>
>> On 6/27/22 18:00, Terry Wilson wrote:
>>> It is common to pass ovn-ctl options via an /etc/sysconfig file.
>>> It would be useful to be able to pass custom --remote options or
>>> additional DBs to listen to via this file. This would give end
>>> users the ability to have more fine-grained control without
>>> having to modify ovn-ctl.
>>>
>>> Signed-off-by: Terry Wilson 
>>> ---
>>
>> Hi Terry,
>>
>> For the change itself:
>>
>> Acked-by: Dumitru Ceara 
> 
> Thanks!
> 
>> While we can make it work for now with this new arbitrary args feature,
>> for the actual goal, of passing a custom --remote option and additional
>> DBs to listen to I think we need a follow up.  Currently, when starting
>> OVN databases, if the database doesn't exist ovn-ctl takes care of
>> creating it:
>>
>> https://github.com/ovn-org/ovn/blob/86684ad759b4f517b8f34fbf7ba8bb8ed77bb575/utilities/ovn-ctl#L246
>> (raft)
>>
>> https://github.com/ovn-org/ovn/blob/86684ad759b4f517b8f34fbf7ba8bb8ed77bb575/utilities/ovn-ctl#L249
>> (standalone)
>>
>> In the future, after [0] is accepted, can we add a patch that changes
>> ovn-ctl to check if the local_config.ovsschema file is installed?  If it
>> is and if DB_*_USE_REMOTE_IN_DB=yes it could then create the
>> corresponding local_config database if not already found in $ovn_dbdir
>> and automatically add the --remote and new db file args.
>>
>> What do you think?
> 
> Something like that sounds good to me. With this I was looking for
> something that would not specifically require the ovs change, but
> would still let me solve the problem if I needed to pass the
> schema/create the db locally. And in general it's useful to be able to
> pass through some options.
> 

Agreed.

> I hadn't originally planned to use Local_Config just because it
> existed--I can imagine situations where you have a manually added
> Connection or custom set things like inactivity_probe set up in the
> main DB, and if ovn-ctl now notices you have a Local_Config schema, it
> stops using the original table and you lose whatever was in the
> original db. So I was thinking of an additional --use-local-config
> kind of option to opt into using it.
> 

Also fine, in my opinion.  Will you be working on adding this support to
ovn-ctl?

Thanks!

>> Thanks,
>> Dumitru
>>
>>>  utilities/ovn-ctl   | 22 ++
>>>  utilities/ovn-ctl.8.xml | 14 +-
>>>  2 files changed, 35 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl
>>> index 23d4d7f8c..93be9b84b 100755
>>> --- a/utilities/ovn-ctl
>>> +++ b/utilities/ovn-ctl
>>> @@ -311,6 +311,10 @@ $cluster_remote_port
>>>  set "$@" --sync-from=`cat $active_conf_file`
>>>  fi
>>>
>>> +if test X"$extra_args" != X; then
>>> +set "$@" $extra_args
>>> +fi
>>> +
>>>  local run_ovsdb_in_bg="no"
>>>  local process_id=
>>>  if test X$detach = Xno && test $mode = cluster && test -z 
>>> "$cluster_remote_addr" ; then
>>> @@ -541,6 +545,10 @@ start_ic () {
>>>
>>>  set "$@" $OVN_IC_LOG $ovn_ic_params
>>>
>>> +if test X"$extra_args" != X; then
>>> +set "$@" $extra_args
>>> +fi
>>> +
>>>  OVS_RUNDIR=${OVS_RUNDIR} start_ovn_daemon "$OVN_IC_PRIORITY" 
>>> "$OVN_IC_WRAPPER" "$@"
>>>  fi
>>>  }
>>> @@ -563,6 +571,10 @@ start_controller () {
>>>
>>>  [ "$OVN_USER" != "" ] && set "$@" --user "$OVN_USER"
>>>
>>> +if test X"$extra_args" != X; then
>>> +set "$@" $extra_args
>>> +fi
>>> +
>>>  OVS_RUNDIR=${OVS_RUNDIR} start_ovn_daemon "$OVN_CONTROLLER_PRIORITY" 
>>> "$OVN_CONTROLLER_WRAPPER" "$@"
>>>  }
>>>
>>> @@ -590,6 +602,10 @@ start_controller_vtep () {
>>>
>>>  [ "$OVN_USER" != "" ] && set "$@" --user "$OVN_USER"
>>>
>>> +if test X"$extra_args" != X; then
>>> +set "$@" $extra_args
>>> +fi
>>> +
>>>  OVS_RUNDIR=${OVS_RUNDIR} start_ovn_daemon "$OVN_CONTROLLER_PRIORITY" 
>>> "$OVN_CONTROLLER_WRAPPER" "$@"
>>>  }
>>>
>>> @@ -1106,8 +1122,10 @@ EOF
>>>
>>>  set_defaults
>>>  command=
>>> +extra_args=
>>>  for arg
>>>  do
>>> +shift
>>>  case $arg in
>>>  -h | --help)
>>>  usage
>>> @@ -1130,6 +1148,10 @@ do
>>>  type=bool
>>>  set_option
>>>  ;;
>>> +--)
>>> +extra_args=$@
>>> +break
>>> +;;
>>>  -*)
>>>  echo >&2 "$0: unknown option \"$arg\" (use --help for help)"
>>>  exit 1
>>> diff --git a/utilities/ovn-ctl.8.xml b/utilities/ovn-ctl.8.xml
>>> index a1d39b22b..42d16fabc 100644
>>> --- a/utilities/ovn-ctl.8.xml
>>> +++ b/utilities/ovn-ctl.8.xml
>>> @@ -4,7 +4,10 @@
>>>  ovn-ctl -- Open Virtual Network northbound daemon lifecycle 
>>> utility
>>>
>>>  Synopsis
>>> -ovn-ctl [options] command
>>> +
>>> +  ovn-ctl [options] command
>>> +  [--- extra_args]
>

[ovs-dev] [PATCH v2] dpif-netdev: Refactor AVX512 runtime checks.

2022-06-29 Thread David Marchand

As described in the bugzilla below, cpu_has_isa code may be compiled
with some AVX512 instructions in it, because cpu.c is built as part of
the libopenvswitchavx512.
This is a problem when this function (supposed to probe for AVX512
instructions availability) is invoked from generic OVS code, on older
CPUs that don't support them.

For the same reason, dpcls_subtable_avx512_gather_probe,
dp_netdev_input_outer_avx512_probe, mfex_avx512_probe and
mfex_avx512_vbmi_probe are potential runtime bombs and can't either be
built as part of libopenvswitchavx512.

Move cpu.c to be part of the "normal" libopenvswitch.
And move other helpers in generic OVS code.

Note:
- dpcls_subtable_avx512_gather_probe is split in two, because it also
  needs to do its own magic,
- while moving those helpers, prefer direct calls to cpu_has_isa and
  avoid cast to intermediate integer variables when a simple boolean
  is enough,

Fixes: 352b6c7116cd ("dpif-lookup: add avx512 gather implementation.")
Fixes: abb807e27dd4 ("dpif-netdev: Add command to switch dpif implementation.")
Fixes: 250ceddcc2d0 ("dpif-netdev/mfex: Add AVX512 based optimized miniflow 
extract")
Fixes: b366fa2f4947 ("dpif-netdev: Call cpuid for x86 isa availability.")
Reported-at: https://bugzilla.redhat.com/2100393
Reported-by: Ales Musil 
Co-authored-by: Ales Musil 
Signed-off-by: Ales Musil 
Signed-off-by: David Marchand 
Acked-by: Sunil Pai G 
---
Changes since v1:
- restored Ales as co-author,
- added more Fixes: lines: backports go back to 2.14 and are not that difficult
  to do, but I can help if needed,
- fixed one indent issue,

---
 lib/automake.mk|  4 +--
 lib/dpif-netdev-avx512.c   | 14 -
 lib/dpif-netdev-extract-avx512.c   | 43 --
 lib/dpif-netdev-lookup-avx512-gather.c | 12 ++-
 lib/dpif-netdev-lookup.c   | 16 ++
 lib/dpif-netdev-lookup.h   |  3 +-
 lib/dpif-netdev-private-dpif.c | 14 +
 lib/dpif-netdev-private-dpif.h |  5 +--
 lib/dpif-netdev-private-extract.c  | 41 
 lib/dpif-netdev-private-extract.h  |  4 +--
 10 files changed, 79 insertions(+), 77 deletions(-)

diff --git a/lib/automake.mk b/lib/automake.mk
index cb50578eb7..3b9e775d4e 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -36,8 +36,6 @@ lib_libopenvswitchavx512_la_CFLAGS = \
-fPIC \
$(AM_CFLAGS)
 lib_libopenvswitchavx512_la_SOURCES = \
-   lib/cpu.c \
-   lib/cpu.h \
lib/dpif-netdev-avx512.c
 if HAVE_AVX512BW
 lib_libopenvswitchavx512_la_CFLAGS += \
@@ -92,6 +90,8 @@ lib_libopenvswitch_la_SOURCES = \
lib/conntrack.h \
lib/coverage.c \
lib/coverage.h \
+   lib/cpu.c \
+   lib/cpu.h \
lib/crc32c.c \
lib/crc32c.h \
lib/csum.c \
diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
index 11d9a00052..82a4138184 100644
--- a/lib/dpif-netdev-avx512.c
+++ b/lib/dpif-netdev-avx512.c
@@ -20,7 +20,6 @@
 
 #include 
 
-#include "cpu.h"
 #include "dpif-netdev.h"
 #include "dpif-netdev-perf.h"
 #include "dpif-netdev-private.h"
@@ -59,19 +58,6 @@ struct dpif_userdata {
 struct pkt_flow_meta pkt_meta[NETDEV_MAX_BURST];
 };
 
-int32_t
-dp_netdev_input_outer_avx512_probe(void)
-{
-bool avx512f_available = cpu_has_isa(OVS_CPU_ISA_X86_AVX512F);
-bool bmi2_available = cpu_has_isa(OVS_CPU_ISA_X86_BMI2);
-
-if (!avx512f_available || !bmi2_available) {
-return -ENOTSUP;
-}
-
-return 0;
-}
-
 int32_t
 dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread *pmd,
  struct dp_packet_batch *packets,
diff --git a/lib/dpif-netdev-extract-avx512.c b/lib/dpif-netdev-extract-avx512.c
index f1919befd1..5029730292 100644
--- a/lib/dpif-netdev-extract-avx512.c
+++ b/lib/dpif-netdev-extract-avx512.c
@@ -42,7 +42,6 @@
 #include 
 #include 
 
-#include "cpu.h"
 #include "flow.h"
 
 #include "dpif-netdev-private-dpcls.h"
@@ -690,47 +689,5 @@ DECLARE_MFEX_FUNC(ip_udp, PROFILE_ETH_IPV4_UDP)
 DECLARE_MFEX_FUNC(ip_tcp, PROFILE_ETH_IPV4_TCP)
 DECLARE_MFEX_FUNC(dot1q_ip_udp, PROFILE_ETH_VLAN_IPV4_UDP)
 DECLARE_MFEX_FUNC(dot1q_ip_tcp, PROFILE_ETH_VLAN_IPV4_TCP)
-
-
-static int32_t
-avx512_isa_probe(uint32_t needs_vbmi)
-{
-static enum ovs_cpu_isa isa_required[] = {
-OVS_CPU_ISA_X86_AVX512F,
-OVS_CPU_ISA_X86_AVX512BW,
-OVS_CPU_ISA_X86_BMI2,
-};
-
-int32_t ret = 0;
-for (uint32_t i = 0; i < ARRAY_SIZE(isa_required); i++) {
-if (!cpu_has_isa(isa_required[i])) {
-ret = -ENOTSUP;
-}
-}
-
-if (needs_vbmi) {
-if (!cpu_has_isa(OVS_CPU_ISA_X86_AVX512VBMI)) {
-ret = -ENOTSUP;
-}
-}
-
-return ret;
-}
-
-/* Probe functions to check ISA requirements. */
-int32_t
-mfex_avx512_probe(void)
-{
-const uint32_t needs_vbmi = 0;
-return avx512_isa_probe(needs_vbmi);
-}
-
-int32_t
-mfex_avx512_vbmi_probe(void)

51 matches

Mail list logo