diff mbox series

[ovs-dev,v9,07/10] odp-execute: Add ISA implementation of pop_vlan action.

Message ID 20220712174456.2682549-8-harry.van.haaren@intel.com
State Changes Requested
Headers show
Series Actions Infrastructure + Optimizations | expand

Checks

Context Check Description
ovsrobot/apply-robot fail apply and check: fail
ovsrobot/github-robot-_Build_and_Test fail github build: failed
ovsrobot/intel-ovs-compilation fail test: fail

Commit Message

Van Haaren, Harry July 12, 2022, 5:44 p.m. UTC
From: Emma Finn <emma.finn@intel.com>

This commit adds the AVX512 implementation of the
pop_vlan action.

Signed-off-by: Emma Finn <emma.finn@intel.com>
---
 lib/automake.mk           |   3 +-
 lib/odp-execute-avx512.c  | 182 ++++++++++++++++++++++++++++++++++++++
 lib/odp-execute-private.c |  33 ++++++-
 lib/odp-execute-private.h |   2 +
 4 files changed, 218 insertions(+), 2 deletions(-)
 create mode 100644 lib/odp-execute-avx512.c

Comments

0-day Robot July 12, 2022, 6:11 p.m. UTC | #1
Bleep bloop.  Greetings Harry van Haaren, 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.


build:
mv -f $depbase.Tpo $depbase.Plo
libtool: compile:  g++ -std=gnu++11 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -g -O2 -MT include/openvswitch/cxxtest.lo -MD -MP -MF include/openvswitch/.deps/cxxtest.Tpo -c include/openvswitch/cxxtest.cc -o include/openvswitch/cxxtest.o
/bin/sh ./libtool  --tag=CXX   --mode=link g++ -std=gnu++11  -g -O2     -o include/openvswitch/libcxxtest.la  include/openvswitch/cxxtest.lo  -lpthread -lrt -lm  -lunbound
libtool: link: rm -fr  include/openvswitch/.libs/libcxxtest.a include/openvswitch/.libs/libcxxtest.la
libtool: link: ar cru include/openvswitch/.libs/libcxxtest.a  include/openvswitch/cxxtest.o
libtool: link: ranlib include/openvswitch/.libs/libcxxtest.a
libtool: link: ( cd "include/openvswitch/.libs" && rm -f "libcxxtest.la" && ln -s "../libcxxtest.la" "libcxxtest.la" )
depbase=`echo utilities/ovs-appctl.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
gcc -std=gnu99 -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 -Wshadow -Werror   -g -O2 -MT utilities/ovs-appctl.o -MD -MP -MF $depbase.Tpo -c -o utilities/ovs-appctl.o utilities/ovs-appctl.c &&\
mv -f $depbase.Tpo $depbase.Po
/bin/sh ./libtool  --tag=CC   --mode=link gcc -std=gnu99 -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 -Wshadow -Werror   -g -O2     -o utilities/ovs-appctl utilities/ovs-appctl.o lib/libopenvswitch.la -lpthread -lrt -lm  -lunbound
libtool: link: gcc -std=gnu99 -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 -Wshadow -Werror -g -O2 -o utilities/ovs-appctl utilities/ovs-appctl.o  lib/.libs/libopenvswitch.a -lssl -lcrypto -lcap-ng -lpthread -lrt -lm -lunbound
depbase=`echo utilities/ovs-testcontroller.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
gcc -std=gnu99 -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 -Wshadow -Werror   -g -O2 -MT utilities/ovs-testcontroller.o -MD -MP -MF $depbase.Tpo -c -o utilities/ovs-testcontroller.o utilities/ovs-testcontroller.c &&\
mv -f $depbase.Tpo $depbase.Po
/bin/sh ./libtool  --tag=CC   --mode=link gcc -std=gnu99 -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 -Wshadow -Werror   -g -O2     -o utilities/ovs-testcontroller utilities/ovs-testcontroller.o lib/libopenvswitch.la -lssl -lcrypto   -lpthread -lrt -lm  -lunbound
libtool: link: gcc -std=gnu99 -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 -Wshadow -Werror -g -O2 -o utilities/ovs-testcontroller utilities/ovs-testcontroller.o  lib/.libs/libopenvswitch.a -lcap-ng -lssl -lcrypto -lpthread -lrt -lm -lunbound
lib/.libs/libopenvswitch.a(odp-execute-private.o): In function `action_avx512_probe':
/var/lib/jenkins/jobs/0day_robot_upstream_build_from_pw/workspace/lib/odp-execute-private.c:57: undefined reference to `action_avx512_init'
collect2: error: ld returned 1 exit status
make[2]: *** [utilities/ovs-testcontroller] Error 1
make[2]: Leaving directory `/var/lib/jenkins/jobs/0day_robot_upstream_build_from_pw/workspace'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/var/lib/jenkins/jobs/0day_robot_upstream_build_from_pw/workspace'
make: *** [all] Error 2


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

Thanks,
0-day Robot
Ilya Maximets July 12, 2022, 11:26 p.m. UTC | #2
On 7/12/22 19:44, Harry van Haaren wrote:
> From: Emma Finn <emma.finn@intel.com>
> 
> This commit adds the AVX512 implementation of the
> pop_vlan action.
> 
> Signed-off-by: Emma Finn <emma.finn@intel.com>
> ---
>  lib/automake.mk           |   3 +-
>  lib/odp-execute-avx512.c  | 182 ++++++++++++++++++++++++++++++++++++++
>  lib/odp-execute-private.c |  33 ++++++-
>  lib/odp-execute-private.h |   2 +
>  4 files changed, 218 insertions(+), 2 deletions(-)
>  create mode 100644 lib/odp-execute-avx512.c

Hi.  This patch is causing a build failure in CI.

I also see that it failed to build in previous versions
of the patch set.

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/lib/automake.mk b/lib/automake.mk
index 5c3b05f6b..4ce5cc1ff 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -44,7 +44,8 @@  lib_libopenvswitchavx512_la_CFLAGS += \
 	-mavx512vl
 lib_libopenvswitchavx512_la_SOURCES += \
 	lib/dpif-netdev-extract-avx512.c \
-	lib/dpif-netdev-lookup-avx512-gather.c
+	lib/dpif-netdev-lookup-avx512-gather.c \
+	lib/odp-execute-avx512.c
 endif # HAVE_AVX512VL
 endif # HAVE_AVX512BW
 lib_libopenvswitchavx512_la_LDFLAGS = \
diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c
new file mode 100644
index 000000000..fd10f7f5c
--- /dev/null
+++ b/lib/odp-execute-avx512.c
@@ -0,0 +1,182 @@ 
+/*
+ * Copyright (c) 2022 Intel.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifdef __x86_64__
+/* Sparse cannot handle the AVX512 instructions. */
+#if !defined(__CHECKER__)
+
+#include <config.h>
+#include <errno.h>
+
+#include "dp-packet.h"
+#include "immintrin.h"
+#include "odp-execute-private.h"
+#include "odp-netlink.h"
+#include "openvswitch/vlog.h"
+
+VLOG_DEFINE_THIS_MODULE(odp_execute_avx512);
+
+/* The below three build asserts make sure that l2_5_ofs, l3_ofs, and l4_ofs
+ * fields remain in the same order and offset to l2_padd_size. This is needed
+ * as the avx512_dp_packet_resize_l2() function will manipulate those fields at
+ * a fixed memory index based on the l2_padd_size offset. */
+BUILD_ASSERT_DECL(offsetof(struct dp_packet, l2_pad_size) +
+                  MEMBER_SIZEOF(struct dp_packet, l2_pad_size) ==
+                  offsetof(struct dp_packet, l2_5_ofs));
+
+BUILD_ASSERT_DECL(offsetof(struct dp_packet, l2_5_ofs) +
+                  MEMBER_SIZEOF(struct dp_packet, l2_5_ofs) ==
+                  offsetof(struct dp_packet, l3_ofs));
+
+BUILD_ASSERT_DECL(offsetof(struct dp_packet, l3_ofs) +
+                           MEMBER_SIZEOF(struct dp_packet, l3_ofs) ==
+                           offsetof(struct dp_packet, l4_ofs));
+
+/* The below build assert makes sure it's safe to read/write 128-bits starting
+ * at the l2_pad_size location. */
+BUILD_ASSERT_DECL(sizeof(struct dp_packet) -
+                  offsetof(struct dp_packet, l2_pad_size) >= sizeof(__m128i));
+
+static inline void ALWAYS_INLINE
+avx512_dp_packet_resize_l2(struct dp_packet *b, int resize_by_bytes)
+{
+    /* Update packet size/data pointers, same as the scalar implementation. */
+    if (resize_by_bytes >= 0) {
+        dp_packet_push_uninit(b, resize_by_bytes);
+    } else {
+        dp_packet_pull(b, -resize_by_bytes);
+    }
+
+    /* The next step is to update the l2_5_ofs, l3_ofs and l4_ofs fields which
+     * the scalar implementation does with the  dp_packet_adjust_layer_offset()
+     * function. */
+
+    /* Set the v_zero register to all zero's. */
+    const __m128i v_zeros = _mm_setzero_si128();
+
+    /* Set the v_u16_max register to all one's. */
+    const __m128i v_u16_max = _mm_cmpeq_epi16(v_zeros, v_zeros);
+
+    /* Each lane represents 16 bits in a 12-bit register. In this case the
+     * first three 16-bit values, which will map to the l2_5_ofs, l3_ofs and
+     * l4_ofs fields. */
+    const uint8_t k_lanes = 0b1110;
+
+    /* Set all 16-bit words in the 128-bits v_offset register to the value we
+     * need to add/substract from the l2_5_ofs, l3_ofs, and l4_ofs fields. */
+    __m128i v_offset = _mm_set1_epi16(abs(resize_by_bytes));
+
+    /* Load 128 bits from the dp_packet structure starting at the l2_pad_size
+     * offset. */
+    void *adjust_ptr = &b->l2_pad_size;
+    __m128i v_adjust_src = _mm_loadu_si128(adjust_ptr);
+
+    /* Here is the tricky part, we only need to update the value of the three
+     * fields if they are not UINT16_MAX. The following function will return
+     * a mask of lanes (read fields) that are not UINT16_MAX. It will do this
+     * by comparing only the lanes we requested, k_lanes, and if they match
+     * v_u16_max, the bit will be set. */
+    __mmask8 k_cmp = _mm_mask_cmpneq_epu16_mask(k_lanes, v_adjust_src,
+                                                v_u16_max);
+
+    /* Based on the bytes adjust (positive, or negative) it will do the actual
+     * add or subtraction. These functions will only operate on the lanes
+     * (fields) requested based on k_cmp, i.e:
+     *   k_cmp = [l2_5_ofs, l3_ofs, l4_ofs]
+     *   for field in kcmp
+     *       v_adjust_src[field] = v_adjust_src[field] + v_offset
+     */
+    __m128i v_adjust_wip;
+
+    if (resize_by_bytes >= 0) {
+        v_adjust_wip = _mm_mask_add_epi16(v_adjust_src, k_cmp,
+                                          v_adjust_src, v_offset);
+    } else {
+        v_adjust_wip = _mm_mask_sub_epi16(v_adjust_src, k_cmp,
+                                          v_adjust_src, v_offset);
+    }
+
+    /* Here we write back the full 128-bits. */
+    _mm_storeu_si128(adjust_ptr, v_adjust_wip);
+}
+
+/* This function performs the same operation on each packet in the batch as
+ * the scalar eth_pop_vlan() function. */
+static void
+action_avx512_pop_vlan(struct dp_packet_batch *batch,
+                       const struct nlattr *a OVS_UNUSED)
+{
+    struct dp_packet *packet;
+
+    /* Set the v_zero register to all zero's. */
+    const __m128i v_zeros = _mm_setzero_si128();
+
+    DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
+        struct vlan_eth_header *veh = dp_packet_eth(packet);
+
+        if (veh && dp_packet_size(packet) >= sizeof *veh &&
+            eth_type_vlan(veh->veth_type)) {
+
+            /* Load the first 128-bits of l2 header into the v_ether register.
+             * This result in the veth_dst/src and veth_type/tci of the
+             * vlan_eth_header structure to be loaded. */
+            __m128i v_ether = _mm_loadu_si128((void *) veh);
+
+            /* This creates a 256-bit value containing the first four fields
+             * of the vlan_eth_header plus 128 zero-bit. The result will be the
+             * lowest 128-bits after the right shift, hence we shift the data
+             * 128(zero)-bits minus the VLAN_HEADER_LEN, so we are left with
+             * only the veth_dst and veth_src fields. */
+            __m128i v_realign = _mm_alignr_epi8(v_ether, v_zeros,
+                                                sizeof(__m128i) -
+                                                VLAN_HEADER_LEN);
+
+            /* Write back the modified ethernet header. */
+            _mm_storeu_si128((void *) veh, v_realign);
+
+            /* As we removed the VLAN_HEADER we now need to adjust all the
+             * offsets. */
+            avx512_dp_packet_resize_l2(packet, -VLAN_HEADER_LEN);
+        }
+    }
+}
+
+int
+action_avx512_init(struct odp_execute_action_impl *self OVS_UNUSED)
+{
+    /* Set function pointers for actions that can be applied directly, these
+     * are identified by OVS_ACTION_ATTR_*. */
+    self->funcs[OVS_ACTION_ATTR_POP_VLAN] = action_avx512_pop_vlan;
+    return 0;
+}
+
+#endif /* Sparse */
+
+#else /* __x86_64__ */
+
+#include <config.h>
+#include "odp-execute-private.h"
+/* Function itself is required to be called, even in e.g. 32-bit builds.
+ * This dummy init function ensures 32-bit builds succeed too.
+ */
+
+int
+action_avx512_init(struct odp_execute_action_impl *self OVS_UNUSED)
+{
+  return 0;
+}
+
+#endif
diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
index f7fb60467..ad736523b 100644
--- a/lib/odp-execute-private.c
+++ b/lib/odp-execute-private.c
@@ -19,6 +19,7 @@ 
 #include <stdio.h>
 #include <string.h>
 
+#include "cpu.h"
 #include "dpdk.h"
 #include "dp-packet.h"
 #include "odp-execute-private.h"
@@ -29,6 +30,36 @@ 
 VLOG_DEFINE_THIS_MODULE(odp_execute_impl);
 static int active_action_impl_index;
 
+#ifdef ACTION_IMPL_AVX512_CHECK
+/* Probe functions to check ISA requirements. */
+static bool
+action_avx512_isa_probe(void)
+{
+    static enum ovs_cpu_isa isa_required[] = {
+        OVS_CPU_ISA_X86_AVX512F,
+        OVS_CPU_ISA_X86_AVX512BW,
+        OVS_CPU_ISA_X86_BMI2,
+        OVS_CPU_ISA_X86_AVX512VL,
+    };
+    for (int i = 0; i < ARRAY_SIZE(isa_required); i++) {
+        if (!cpu_has_isa(isa_required[i])) {
+            return false;
+        }
+    }
+    return true;
+}
+static int
+action_avx512_probe(struct odp_execute_action_impl *self)
+{
+    if (!action_avx512_isa_probe()) {
+        return -ENOTSUP;
+    } else {
+        action_avx512_init(self);
+    }
+    return 0;
+}
+#endif
+
 static struct odp_execute_action_impl action_impls[] = {
     [ACTION_IMPL_AUTOVALIDATOR] = {
         .available = false,
@@ -46,7 +77,7 @@  static struct odp_execute_action_impl action_impls[] = {
     [ACTION_IMPL_AVX512] = {
         .available = false,
         .name = "avx512",
-        .init_func = NULL,
+        .init_func = action_avx512_probe,
     },
 #endif
 };
diff --git a/lib/odp-execute-private.h b/lib/odp-execute-private.h
index 1c636faeb..b3a10cd82 100644
--- a/lib/odp-execute-private.h
+++ b/lib/odp-execute-private.h
@@ -90,6 +90,8 @@  struct odp_execute_action_impl * odp_execute_action_set(const char *name);
 
 int action_autoval_init(struct odp_execute_action_impl *self);
 
+int action_avx512_init(struct odp_execute_action_impl *self);
+
 void odp_execute_action_get_info(struct ds *name);
 
 #endif /* ODP_EXTRACT_PRIVATE */