On 22 Jun 2022, at 18:22, Finn, Emma wrote:
> Hi Eelco,
>
> I've investigated the issue and resolved the failing unit tests.
> Diff's are provided below or if it's convenient I can send as patchset
> instead.
>
> Fix for 05/11: odp-execute: Add command to switch action implementation.
>
> diff --git a/tests/pmd.at b/tests/pmd.at
> index ac05f5f7d..140ce8a8d 100644
> --- a/tests/pmd.at
> +++ b/tests/pmd.at
> @@ -1205,8 +1205,12 @@ AT_SETUP([PMD - ovs-actions configuration])
> OVS_VSWITCHD_START([], [], [], [--dummy-numa 0,0])
> AT_CHECK([ovs-vsctl add-port br0 p1 -- set Interface p1 type=dummy-pmd])
>
> -dnl Scalar impl is set by default.
> AT_CHECK([ovs-vsctl show], [], [stdout])
> +
> +dnl Set the scalar first, so we always have the scalar impl as Active.
> +AT_CHECK([ovs-appctl dpif-netdev/action-impl-set scalar], [0], [dnl
> +Action implementation set to scalar.
> +])
> AT_CHECK([ovs-appctl dpif-netdev/action-impl-show | grep "scalar"], [], [dnl
> scalar (available: Yes, active: Yes)
> ])
>
>
> Fix for 06/11 dpif-netdev: Add configure option to enable actions
> autovalidator at build time.
>
> diff --git a/acinclude.m4 b/acinclude.m4
> index 98f4599b1..18e61b522 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -28,7 +28,7 @@ AC_DEFUN([OVS_CHECK_ACTIONS_AUTOVALIDATOR], [
> if test "$autovalidator" != yes; then
> AC_MSG_RESULT([no])
> else
> - AC_DEFINE([MFEX_AUTOVALIDATOR_DEFAULT], [1],
> + AC_DEFINE([ACTIONS_AUTOVALIDATOR_DEFAULT], [1],
> [Autovalidator for actions is a default implementation.])
> AC_MSG_RESULT([yes])
> Fi
Hi Emma,
Thanks for looking into this! The two changes above I already identified as
part of my review in progress (I’m currently starting with patch 11).
> Fix for 10/11 odp-execute: Add ISA implementation of set_masked ETH
>
> diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
> index e2d650779..6c3dabbd0 100644
> --- a/lib/odp-execute-private.c
> +++ b/lib/odp-execute-private.c
> @@ -21,6 +21,7 @@
>
> #include "dpdk.h"
> #include "dp-packet.h"
> +#include "odp-execute.h"
> #include "odp-execute-private.h"
> #include "odp-netlink.h"
> #include "odp-util.h"
> @@ -243,6 +244,13 @@ action_set_masked_init(struct dp_packet_batch *batch
> OVS_UNUSED,
> if (autoval_impl.set_masked_funcs[attr_type]) {
> set_masked = true;
> autoval_impl.set_masked_funcs[attr_type](batch, a);
> + } else {
> + struct dp_packet *packet;
> + a = nl_attr_get(a);
> +
> + DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
> + odp_execute_masked_set_action(packet, a);
> + }
> }
> }
>
> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> index db6e1ec03..2aa213399 100644
> --- a/lib/odp-execute.c
> +++ b/lib/odp-execute.c
> @@ -561,7 +561,7 @@ odp_execute_set_action(struct dp_packet *packet, const
> struct nlattr *a)
> }
> }
>
> -static void
> +void
> odp_execute_masked_set_action(struct dp_packet *packet,
> const struct nlattr *a)
>
> diff --git a/lib/odp-execute.h b/lib/odp-execute.h
> index 762b99473..4857cb91f 100644
> --- a/lib/odp-execute.h
> +++ b/lib/odp-execute.h
> @@ -53,4 +53,7 @@ void odp_execute_actions(void *dp, struct dp_packet_batch
> *batch,
>
> #define get_mask(a, type) ((const type *)(const void *)(a + 1) + 1)
>
> +void odp_execute_masked_set_action(struct dp_packet *packet,
> + const struct nlattr *a);
> +
> #endif
>
> diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c
> index aa65afec7..22a96b1c8 100644
> --- a/lib/odp-execute-avx512.c
> +++ b/lib/odp-execute-avx512.c
> @@ -26,6 +26,7 @@
> #include "cpu.h"
> #include "dp-packet.h"
> #include "immintrin.h"
> +#include "odp-execute.h"
> #include "odp-execute-private.h"
> #include "odp-netlink.h"
> #include "openvswitch/vlog.h"
> @@ -411,6 +412,13 @@ action_avx512_set_masked(struct dp_packet_batch *batch
> OVS_UNUSED,
>
> if (avx512_impl.set_masked_funcs[attr_type]) {
> avx512_impl.set_masked_funcs[attr_type](batch, a);
> + } else {
> + struct dp_packet *packet;
> + a = nl_attr_get(a);
> +
> + DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
> + odp_execute_masked_set_action(packet, a);
> + }
> }
>
> diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c
> index aa2ed9022..6431b49dc 100644
> --- a/lib/odp-execute-avx512.c
> +++ b/lib/odp-execute-avx512.c
> @@ -197,8 +197,8 @@ static void
> action_avx512_set_masked(struct dp_packet_batch *batch OVS_UNUSED,
> const struct nlattr *a)
> {
> - a = nl_attr_get(a);
> - enum ovs_key_attr attr_type = nl_attr_type(a);
> + const struct nlattr *type = nl_attr_get(a);
> + enum ovs_key_attr attr_type = nl_attr_type(type);
I re-wrote most of patch 10, and looking at the changes, they might no longer
be needed. However, I still need to test all my suggestions :)
So no patch update is needed for now, and I’ll make sure it’s all covered as
part of my review feedback.
Cheers,
Eelco
> Thanks,
> Emma
>
>> -----Original Message-----
>> From: Eelco Chaudron <[email protected]>
>> Sent: Tuesday 21 June 2022 14:29
>> To: Finn, Emma <[email protected]>
>> Cc: Stokes, Ian <[email protected]>; Van Haaren, Harry
>> <[email protected]>; [email protected]
>> Subject: Re: [PATCH v7 00/11] Actions Infrastructure + Optimizations
>>
>> Hi Emma,
>>
>> I started reviewing your patch and found some issues that need investigation.
>>
>> With the autovalidator, some tests are failing. But before you can run them,
>> you
>> need to fix your patch 7, as the autovalidator enablement is not working:
>>
>> [wsfd-advnetlab44:~/...DK_v21.11/ovs_github]$ git diff diff --git
>> a/acinclude.m4
>> b/acinclude.m4 index 98f4599b1..18e61b522 100644
>> --- a/acinclude.m4
>> +++ b/acinclude.m4
>> @@ -28,7 +28,7 @@ AC_DEFUN([OVS_CHECK_ACTIONS_AUTOVALIDATOR], [
>> if test "$autovalidator" != yes; then
>> AC_MSG_RESULT([no])
>> else
>> - AC_DEFINE([MFEX_AUTOVALIDATOR_DEFAULT], [1],
>> + AC_DEFINE([ACTIONS_AUTOVALIDATOR_DEFAULT], [1],
>> [Autovalidator for actions is a default implementation.])
>> AC_MSG_RESULT([yes])
>> fi
>>
>>
>> Build OVS (without DPDK) and run the following tests:
>>
>> # ./configure --prefix=/usr --localstatedir=/var --sysconfdir=/etc && make
>> -j 32
>> check TESTSUITEFLAGS="1040 1041 1132 1145 1294 2493"
>> ...
>> ...
>> ## ------------------------------- ##
>> ## openvswitch 2.17.90 test suite. ##
>> ## ------------------------------- ##
>>
>> dpif-netdev
>>
>> 1040: dpif-netdev - partial hw offload with packet modifications - dummy ok
>> 1041: dpif-netdev - partial hw offload with packet modifications - dummy-pmd
>> ok
>>
>> ofproto-dpif
>>
>> 1132: ofproto-dpif - controller ok
>> 1145: ofproto-dpif - ARP modification slow-path ok
>>
>> ofproto-dpif - flow translation resource limits
>>
>> 1294: ofproto-dpif - Neighbor Discovery set-field with checksum update ok
>>
>> network service header (NSH)
>>
>> 2493: nsh - triangle PTAP bridge setup with NSH over vxlan-gpe ok
>>
>> ## ------------- ##
>> ## Test results. ##
>> ## ------------- ##
>>
>> All 6 tests were successful.
>>
>>
>> Now with the auto-validator on an AVX512 machine:
>>
>> # ./configure --prefix=/usr --localstatedir=/var --sysconfdir=/etc --enable-
>> actions-default-autovalidator && make -j 32 check TESTSUITEFLAGS="1040
>> 1041 1132 1145 1294 2493"
>> ...
>> ...
>> ## ------------------------------- ##
>> ## openvswitch 2.17.90 test suite. ##
>> ## ------------------------------- ##
>>
>> dpif-netdev
>>
>> 1040: dpif-netdev - partial hw offload with packet modifications - dummy
>> FAILED (dpif-netdev.at:542)
>> 1041: dpif-netdev - partial hw offload with packet modifications - dummy-pmd
>> FAILED (dpif-netdev.at:543)
>>
>> ofproto-dpif
>>
>> 1132: ofproto-dpif - controller FAILED
>> (ofproto-dpif.at:1979)
>> 1145: ofproto-dpif - ARP modification slow-path FAILED (ofproto-
>> dpif.at:3777)
>>
>> ofproto-dpif - flow translation resource limits
>>
>> 1294: ofproto-dpif - Neighbor Discovery set-field with checksum update FAILED
>> (ofproto-dpif.at:9936)
>>
>> network service header (NSH)
>>
>> 2493: nsh - triangle PTAP bridge setup with NSH over vxlan-gpe FAILED
>> (nsh.at:778)
>>
>> ## ------------- ##
>> ## Test results. ##
>> ## ------------- ##
>>
>> ERROR: All 6 tests were run,
>> 6 failed unexpectedly.
>>
>>
>> Please start some investigation, but do NOT send out a v8, until I’ve
>> completed
>> v7. If there is a small change I can apply that fixes the issue, just sent
>> that in this
>> thread.
>>
>>
>> //Eelco
>>
>> On 14 Jun 2022, at 13:57, Emma Finn wrote:
>>
>>> This patchset introduces actions infrastructure changes which allows
>>> the user to choose between different action implementations based on
>>> CPU ISA by using different commands. The infrastructure also provides
>>> a way to check the correctness of the ISA optimized action version
>>> against the scalar version.
>>>
>>> This series also introduces optimized versions of the following
>>> actions:
>>> - push_vlan
>>> - pop_vlan
>>> - set_masked eth
>>> - set_masked ipv4
>>>
>>> Below is a table indicating the relative performance benefits for
>>> these actions.
>>>
>>> +-----------------------------------------------+-------------------+-----------------+
>>> | Actions | Salar with series |AVX
>>> with series |
>>> +-----------------------------------------------+-------------------+-----------------+
>>> | mod_dl_dst | 1.04x |1.15x
>>> |
>>> +-----------------------------------------------+-------------------+-----------------+
>>> | push_vlan | 1.10x |1.23x
>>> |
>>> +-----------------------------------------------+-------------------+-----------------+
>>> | strip_vlan | 1.05x |1.14x
>>> |
>>> +-----------------------------------------------+-------------------+-----------------+
>>> | mod_ipv4 1 x field | 1.04x |1.04x
>>> |
>>> +-----------------------------------------------+-------------------+-----------------+
>>> | mod_ipv4 4 x fields | 1.04x |1.23x
>>> |
>>> +-----------------------------------------------+-------------------+-----------------+
>>> | strip_vlan + mod_dl_dst + mod_ipv4 4 x fields | 1.06x |1.36x
>>> |
>>> +-----------------------------------------------+-------------------+-----------------+
>>>
>>> ---
>>> v7:
>>> - Fix review comments from Eelco.
>>> ---
>>> v6:
>>> - Rebase to master
>>> - Add ISA implementation of set_masked eth and ipv4 actions
>>> - Fix incorrect checksums in input packets for ofproto-dpif unit tests
>>> ---
>>> v5:
>>> - Rebase to master
>>> - Minor change to variable names
>>> - Added Tags from Harry.
>>> ---
>>> v4:
>>> - Rebase to master
>>> - Add ISA implementation of push_vlan action
>>> ---
>>> v3:
>>> - Refactored to fix unit test failures
>>> - Removed some sign-off on commits
>>> ---
>>> v2:
>>> - Fix the CI build issues
>>> ---
>>>
>>> Emma Finn (10):
>>> ofproto-dpif: Fix incorrect checksums in input packets
>>> odp-execute: Add function pointers to odp-execute for different action
>>> implementations.
>>> odp-execute: Add function pointer for pop_vlan action.
>>> odp-execute: Add auto validation function for actions.
>>> odp-execute: Add command to switch action implementation.
>>> odp-execute: Add ISA implementation of actions.
>>> odp-execute: Add ISA implementation of pop_vlan action.
>>> odp-execute: Add ISA implementation of push_vlan action.
>>> odp-execute: Add ISA implementation of set_masked ETH
>>> odp-execute: Add ISA implementation of set_masked IPv4 action
>>>
>>> Kumar Amber (1):
>>> dpif-netdev: Add configure option to enable actions autovalidator at
>>> build time.
>>>
>>> Documentation/ref/ovs-actions.7.rst | 26 ++
>>> Documentation/topics/testing.rst | 24 +-
>>> NEWS | 11 +
>>> acinclude.m4 | 21 ++
>>> configure.ac | 1 +
>>> lib/automake.mk | 8 +-
>>> lib/cpu.c | 1 +
>>> lib/cpu.h | 1 +
>>> lib/dp-packet.c | 23 ++
>>> lib/dp-packet.h | 4 +
>>> lib/dpif-netdev-unixctl.man | 8 +
>>> lib/dpif-netdev.c | 42 +++
>>> lib/odp-execute-avx512.c | 463 ++++++++++++++++++++++++++++
>>> lib/odp-execute-private.c | 266 ++++++++++++++++
>>> lib/odp-execute-private.h | 99 ++++++
>>> lib/odp-execute.c | 183 ++++++++---
>>> lib/odp-execute.h | 14 +
>>> tests/ofproto-dpif.at | 10 +-
>>> tests/pmd.at | 30 ++
>>> 19 files changed, 1183 insertions(+), 52 deletions(-) create mode
>>> 100644 lib/odp-execute-avx512.c create mode 100644
>>> lib/odp-execute-private.c create mode 100644
>>> lib/odp-execute-private.h
>>>
>>> --
>>> 2.32.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
