[PATCH] hash: make gfni stubs inline

[PATCH] hash: make gfni stubs inline

[Stephen Hemminger]

This reverts commit 07d836e5929d18ad6640ebae90dd2f81a2cafb71.

Tyler found build issues with MSVC and the thash gfni stubs.
The problem would be link errors from missing symbols.

The purpose of the original commit was to allow local definition of
RTE_LOGTYPE_HASH. Put the thash gfni back as inlines, but require
that the header only be included inside rte_thash.h so that the
log macros are defined.

Reported-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/hash/meson.build      |  1 -
 lib/hash/rte_thash_gfni.h | 42 +++++++++++++++++++++++----------------
 lib/hash/version.map      |  2 --
 3 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/lib/hash/meson.build b/lib/hash/meson.build
index 277eb9fa9366..541b1d2790fa 100644
--- a/lib/hash/meson.build
+++ b/lib/hash/meson.build
@@ -22,7 +22,6 @@ sources = files(
         'rte_hash_crc.c',
         'rte_fbk_hash.c',
         'rte_thash.c',
-        'rte_thash_gfni.c',
 )
 
 deps += ['net']
diff --git a/lib/hash/rte_thash_gfni.h b/lib/hash/rte_thash_gfni.h
index eed55fc86c86..fba68f7bc250 100644
--- a/lib/hash/rte_thash_gfni.h
+++ b/lib/hash/rte_thash_gfni.h
@@ -2,14 +2,13 @@
  * Copyright(c) 2021 Intel Corporation
  */
 
-#ifndef _RTE_THASH_GFNI_H_
-#define _RTE_THASH_GFNI_H_
 
-#ifdef __cplusplus
-extern "C" {
-#endif
-
-#include <rte_log.h>
+/*
+ * This header file is not supposed to included directly in application
+ */
+#ifndef _RTE_THASH_H
+#error Do not include rte_thash_gfni.h directly
+#else
 
 #ifdef RTE_ARCH_X86
 
@@ -33,8 +32,13 @@ extern "C" {
  * @return
  *  Calculated Toeplitz hash value.
  */
-uint32_t
-rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len);
+static inline uint32_t
+rte_thash_gfni(const uint64_t *mtrx __rte_unused,
+	const uint8_t *key __rte_unused, int len __rte_unused)
+{
+	RTE_LOG(ERR, HASH, "%s is undefined under given arch\n", __func__);
+	return 0;
+}
 
 /**
  * Bulk implementation for Toeplitz hash.
@@ -53,14 +57,18 @@ rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len);
  * @param num
  *  Number of tuples to hash.
  */
-void
-rte_thash_gfni_bulk(const uint64_t *mtrx, int len, uint8_t *tuple[],
-	uint32_t val[], uint32_t num);
+static inline void
+rte_thash_gfni_bulk(const uint64_t *mtrx __rte_unused,
+	int len __rte_unused, uint8_t *tuple[] __rte_unused,
+	uint32_t val[], uint32_t num)
+{
+	unsigned int i;
 
-#endif /* RTE_THASH_GFNI_DEFINED */
-
-#ifdef __cplusplus
+	RTE_LOG(ERR, HASH, "%s is undefined under given arch\n", __func__);
+	for (i = 0; i < num; i++)
+		val[i] = 0;
 }
-#endif
 
-#endif /* _RTE_THASH_GFNI_H_ */
+#endif /* !RTE_THASH_GFNI_DEFINED */
+
+#endif /* !_RTE_HASH__H_ */
diff --git a/lib/hash/version.map b/lib/hash/version.map
index 6b2afebf6b46..6236b24722a2 100644
--- a/lib/hash/version.map
+++ b/lib/hash/version.map
@@ -41,8 +41,6 @@ DPDK_24 {
 	rte_thash_get_gfni_matrices;
 	rte_thash_get_helper;
 	rte_thash_get_key;
-	rte_thash_gfni;
-	rte_thash_gfni_bulk;
 	rte_thash_gfni_supported;
 	rte_thash_init_ctx;
 
-- 
2.43.0

[PATCH v2] hash: make GFNI stubs inline (again)

[Stephen Hemminger]

Tyler found build issues with MSVC and the thash gfni stubs.
The problem would be link errors from missing symbols.

This version puts back the rte_thash_gfni function stubs as
inlines, but instead of logging a message, they panic.
This is intentional because any application should be checking
with function rte_thash_gfni_supported() before calling the
hashing functions here. Better to panic then return zero
and put out log message which will be ignored...

Fixes: 07d836e5929d ("hash: uninline GFNI stubs")
Reported-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v2 - uses panic instead of logging.
     Please ignore the checkpatch nag about this.

 lib/hash/meson.build      |  1 -
 lib/hash/rte_thash_gfni.c | 51 ---------------------------------------
 lib/hash/rte_thash_gfni.h | 34 ++++++++++++++++++--------
 lib/hash/version.map      |  2 --
 4 files changed, 24 insertions(+), 64 deletions(-)
 delete mode 100644 lib/hash/rte_thash_gfni.c

diff --git a/lib/hash/meson.build b/lib/hash/meson.build
index 277eb9fa9366..541b1d2790fa 100644
--- a/lib/hash/meson.build
+++ b/lib/hash/meson.build
@@ -22,7 +22,6 @@ sources = files(
         'rte_hash_crc.c',
         'rte_fbk_hash.c',
         'rte_thash.c',
-        'rte_thash_gfni.c',
 )
 
 deps += ['net']
diff --git a/lib/hash/rte_thash_gfni.c b/lib/hash/rte_thash_gfni.c
deleted file mode 100644
index f1525f9838de..000000000000
--- a/lib/hash/rte_thash_gfni.c
+++ /dev/null
@@ -1,51 +0,0 @@
-/* SPDX-License-Identifier: BSD-3-Clause
- * Copyright(c) 2021 Intel Corporation
- */
-
-#include <stdbool.h>
-
-#include <rte_log.h>
-#include <rte_thash_gfni.h>
-
-#ifndef RTE_THASH_GFNI_DEFINED
-
-RTE_LOG_REGISTER_SUFFIX(hash_gfni_logtype, gfni, INFO);
-#define RTE_LOGTYPE_HASH hash_gfni_logtype
-#define HASH_LOG(level, ...) \
-	RTE_LOG_LINE(level, HASH, "" __VA_ARGS__)
-
-uint32_t
-rte_thash_gfni(const uint64_t *mtrx __rte_unused,
-	const uint8_t *key __rte_unused, int len __rte_unused)
-{
-	static bool warned;
-
-	if (!warned) {
-		warned = true;
-		HASH_LOG(ERR,
-			"%s is undefined under given arch", __func__);
-	}
-
-	return 0;
-}
-
-void
-rte_thash_gfni_bulk(const uint64_t *mtrx __rte_unused,
-	int len __rte_unused, uint8_t *tuple[] __rte_unused,
-	uint32_t val[], uint32_t num)
-{
-	unsigned int i;
-
-	static bool warned;
-
-	if (!warned) {
-		warned = true;
-		HASH_LOG(ERR,
-			"%s is undefined under given arch", __func__);
-	}
-
-	for (i = 0; i < num; i++)
-		val[i] = 0;
-}
-
-#endif
diff --git a/lib/hash/rte_thash_gfni.h b/lib/hash/rte_thash_gfni.h
index eed55fc86c86..d1e7c02398c2 100644
--- a/lib/hash/rte_thash_gfni.h
+++ b/lib/hash/rte_thash_gfni.h
@@ -9,8 +9,6 @@
 extern "C" {
 #endif
 
-#include <rte_log.h>
-
 #ifdef RTE_ARCH_X86
 
 #include <rte_thash_x86_gfni.h>
@@ -19,9 +17,10 @@ extern "C" {
 
 #ifndef RTE_THASH_GFNI_DEFINED
 
+#include <rte_debug.h>
+
 /**
  * Calculate Toeplitz hash.
- * Dummy implementation.
  *
  * @param m
  *  Pointer to the matrices generated from the corresponding
@@ -31,14 +30,21 @@ extern "C" {
  * @param len
  *  Length of the data to be hashed.
  * @return
- *  Calculated Toeplitz hash value.
+ *   Calculated hash value.
  */
-uint32_t
-rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len);
+static inline uint32_t
+rte_thash_gfni(const uint64_t *mtrx __rte_unused,
+	const uint8_t *key __rte_unused, int len __rte_unused)
+{
+	/*
+	 * This stub always panics because the application should be calling
+	 * rte_thash_gfni_supported() to check if the arch supports this.
+	 */
+	rte_panic("%s is undefined under given arch\n", __func__);
+}
 
 /**
  * Bulk implementation for Toeplitz hash.
- * Dummy implementation.
  *
  * @param m
  *  Pointer to the matrices generated from the corresponding
@@ -53,9 +59,17 @@ rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len);
  * @param num
  *  Number of tuples to hash.
  */
-void
-rte_thash_gfni_bulk(const uint64_t *mtrx, int len, uint8_t *tuple[],
-	uint32_t val[], uint32_t num);
+static inline void
+rte_thash_gfni_bulk(const uint64_t *mtrx __rte_unused,
+	int len __rte_unused, uint8_t *tuple[] __rte_unused,
+	uint32_t val[] __rte_unused, uint32_t num __rte_unused)
+{
+	/*
+	 * This stub always panics because the application should be calling
+	 * rte_thash_gfni_supported() to check if the arch supports this.
+	 */
+	rte_panic("%s is undefined under given arch\n", __func__);
+}
 
 #endif /* RTE_THASH_GFNI_DEFINED */
 
diff --git a/lib/hash/version.map b/lib/hash/version.map
index 6b2afebf6b46..6236b24722a2 100644
--- a/lib/hash/version.map
+++ b/lib/hash/version.map
@@ -41,8 +41,6 @@ DPDK_24 {
 	rte_thash_get_gfni_matrices;
 	rte_thash_get_helper;
 	rte_thash_get_key;
-	rte_thash_gfni;
-	rte_thash_gfni_bulk;
 	rte_thash_gfni_supported;
 	rte_thash_init_ctx;
 
-- 
2.43.0

Re: [PATCH v2] hash: make GFNI stubs inline (again)

[Tyler Retzlaff]

On Mon, Mar 04, 2024 at 07:07:24PM -0800, Stephen Hemminger wrote:
> Tyler found build issues with MSVC and the thash gfni stubs.
> The problem would be link errors from missing symbols.
> 
> This version puts back the rte_thash_gfni function stubs as
> inlines, but instead of logging a message, they panic.
> This is intentional because any application should be checking
> with function rte_thash_gfni_supported() before calling the
> hashing functions here. Better to panic then return zero
> and put out log message which will be ignored...
> 
> Fixes: 07d836e5929d ("hash: uninline GFNI stubs")
> Reported-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---

Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

Re: [PATCH v2] hash: make GFNI stubs inline (again)

[Thomas Monjalon]

05/03/2024 04:07, Stephen Hemminger:
> Tyler found build issues with MSVC and the thash gfni stubs.
> The problem would be link errors from missing symbols.
> 
> This version puts back the rte_thash_gfni function stubs as
> inlines, but instead of logging a message, they panic.
> This is intentional because any application should be checking
> with function rte_thash_gfni_supported() before calling the
> hashing functions here. Better to panic then return zero
> and put out log message which will be ignored...

I want to be able to grep rte_panic in the lib directory
to find those we should remove.
Having "legit" panic calls is a no-go for me.

Re: [PATCH] hash: make gfni stubs inline

[David Marchand]

On Mon, Mar 4, 2024 at 7:45 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> This reverts commit 07d836e5929d18ad6640ebae90dd2f81a2cafb71.
>
> Tyler found build issues with MSVC and the thash gfni stubs.
> The problem would be link errors from missing symbols.

Trying to understand this link error.
Does it come from the fact that rte_thash_gfni/rte_thash_gfni_bulk
declarations are hidden under RTE_THASH_GFNI_DEFINED in
rte_thash_gfni.h?

If so, why not always expose those two symbols unconditionnally and
link with the stub only when ! RTE_THASH_GFNI_DEFINED.


-- 
David Marchand

Re: [PATCH] hash: make gfni stubs inline

[Tyler Retzlaff]

On Tue, Mar 05, 2024 at 11:14:45AM +0100, David Marchand wrote:
> On Mon, Mar 4, 2024 at 7:45 PM Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> >
> > This reverts commit 07d836e5929d18ad6640ebae90dd2f81a2cafb71.
> >
> > Tyler found build issues with MSVC and the thash gfni stubs.
> > The problem would be link errors from missing symbols.
> 
> Trying to understand this link error.
> Does it come from the fact that rte_thash_gfni/rte_thash_gfni_bulk
> declarations are hidden under RTE_THASH_GFNI_DEFINED in
> rte_thash_gfni.h?
> 
> If so, why not always expose those two symbols unconditionnally and
> link with the stub only when ! RTE_THASH_GFNI_DEFINED.

So I don't have a lot of background of this lib.

I think we understand that we can't conditionally expose symbols. That's
what windows was picking up because it seems none of our CI's ever end
up with RTE_THASH_GFNI_DEFINED but my local test system did and failed.
(my experiments showed that Linux would complain too if it was defined)

If we always expose the symbols then as you point out we have to
conditionally link with the stub otherwise the inline (non-stub) will be
duplicate and build / link will fail.

I guess the part I don't understand with your suggestion is how we would
conditionally link with just the stub? We have to link with rte_hash to
get the rest of hash and the stub. I've probably missed something here.

Since we never had a release exposing the new symbols introduced by
Stephen in question my suggestion was that we just revert for 24.03 so
we don't end up with an ABI break later if we choose to solve the
problem without exports.

I don't know what else to do, but I think we need to decide for 24.03.

ty

> 
> -- 
> David Marchand

Re: [PATCH] hash: make gfni stubs inline

[Stephen Hemminger]

On Tue, 5 Mar 2024 09:53:00 -0800
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> On Tue, Mar 05, 2024 at 11:14:45AM +0100, David Marchand wrote:
> > On Mon, Mar 4, 2024 at 7:45 PM Stephen Hemminger
> > <stephen@networkplumber.org> wrote:  
> > >
> > > This reverts commit 07d836e5929d18ad6640ebae90dd2f81a2cafb71.
> > >
> > > Tyler found build issues with MSVC and the thash gfni stubs.
> > > The problem would be link errors from missing symbols.  
> > 
> > Trying to understand this link error.
> > Does it come from the fact that rte_thash_gfni/rte_thash_gfni_bulk
> > declarations are hidden under RTE_THASH_GFNI_DEFINED in
> > rte_thash_gfni.h?
> > 
> > If so, why not always expose those two symbols unconditionnally and
> > link with the stub only when ! RTE_THASH_GFNI_DEFINED.  
> 
> So I don't have a lot of background of this lib.
> 
> I think we understand that we can't conditionally expose symbols. That's
> what windows was picking up because it seems none of our CI's ever end
> up with RTE_THASH_GFNI_DEFINED but my local test system did and failed.
> (my experiments showed that Linux would complain too if it was defined)
> 
> If we always expose the symbols then as you point out we have to
> conditionally link with the stub otherwise the inline (non-stub) will be
> duplicate and build / link will fail.
> 
> I guess the part I don't understand with your suggestion is how we would
> conditionally link with just the stub? We have to link with rte_hash to
> get the rest of hash and the stub. I've probably missed something here.
> 
> Since we never had a release exposing the new symbols introduced by
> Stephen in question my suggestion was that we just revert for 24.03 so
> we don't end up with an ABI break later if we choose to solve the
> problem without exports.
> 
> I don't know what else to do, but I think we need to decide for 24.03.
> 
> ty

Another option would be introduce dead code stubs all the time.
Then have inline wrapper that redirect to the dead stub if needed.

Something like:
From 7bb972d342e939200f8f993a9074b20794941f6a Mon Sep 17 00:00:00 2001
From: Stephen Hemminger <stephen@networkplumber.org>
Date: Tue, 5 Mar 2024 10:42:48 -0800
Subject: [PATCH] hash: rename GFNI stubs

Make the GFNI stub functions always built. This solves the conditional
linking problem. If GFNI is available, they will never get used.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/hash/rte_thash_gfni.c | 11 +++++------
 lib/hash/rte_thash_gfni.h | 23 ++++++++++++++++++-----
 lib/hash/version.map      |  9 +++++++--
 3 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/lib/hash/rte_thash_gfni.c b/lib/hash/rte_thash_gfni.c
index f1525f9838de..de67abb8b211 100644
--- a/lib/hash/rte_thash_gfni.c
+++ b/lib/hash/rte_thash_gfni.c
@@ -4,18 +4,18 @@
 
 #include <stdbool.h>
 
+#include <rte_common.h>
 #include <rte_log.h>
 #include <rte_thash_gfni.h>
 
-#ifndef RTE_THASH_GFNI_DEFINED
-
 RTE_LOG_REGISTER_SUFFIX(hash_gfni_logtype, gfni, INFO);
 #define RTE_LOGTYPE_HASH hash_gfni_logtype
 #define HASH_LOG(level, ...) \
 	RTE_LOG_LINE(level, HASH, "" __VA_ARGS__)
 
+__rte_internal
 uint32_t
-rte_thash_gfni(const uint64_t *mtrx __rte_unused,
+___rte_thash_gfni(const uint64_t *mtrx __rte_unused,
 	const uint8_t *key __rte_unused, int len __rte_unused)
 {
 	static bool warned;
@@ -29,8 +29,9 @@ rte_thash_gfni(const uint64_t *mtrx __rte_unused,
 	return 0;
 }
 
+__rte_internal
 void
-rte_thash_gfni_bulk(const uint64_t *mtrx __rte_unused,
+___rte_thash_gfni_bulk(const uint64_t *mtrx __rte_unused,
 	int len __rte_unused, uint8_t *tuple[] __rte_unused,
 	uint32_t val[], uint32_t num)
 {
@@ -47,5 +48,3 @@ rte_thash_gfni_bulk(const uint64_t *mtrx __rte_unused,
 	for (i = 0; i < num; i++)
 		val[i] = 0;
 }
-
-#endif
diff --git a/lib/hash/rte_thash_gfni.h b/lib/hash/rte_thash_gfni.h
index eed55fc86c86..1cb61cf39675 100644
--- a/lib/hash/rte_thash_gfni.h
+++ b/lib/hash/rte_thash_gfni.h
@@ -9,7 +9,16 @@
 extern "C" {
 #endif
 
-#include <rte_log.h>
+#include <rte_common.h>
+/*
+ * @internal
+ * Stubs defined for use when GFNI is not available
+ */
+uint32_t
+___rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len);
+void
+___rte_thash_gfni_bulk(const uint64_t *mtrx, int len, uint8_t *tuple[],
+		       uint32_t val[], uint32_t num);
 
 #ifdef RTE_ARCH_X86
 
@@ -18,10 +27,8 @@ extern "C" {
 #endif
 
 #ifndef RTE_THASH_GFNI_DEFINED
-
 /**
  * Calculate Toeplitz hash.
- * Dummy implementation.
  *
  * @param m
  *  Pointer to the matrices generated from the corresponding
@@ -34,7 +41,10 @@ extern "C" {
  *  Calculated Toeplitz hash value.
  */
 uint32_t
-rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len);
+rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len)
+{
+	return ___rte_thash_gfni(mtrx, key, len);
+}
 
 /**
  * Bulk implementation for Toeplitz hash.
@@ -55,7 +65,10 @@ rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len);
  */
 void
 rte_thash_gfni_bulk(const uint64_t *mtrx, int len, uint8_t *tuple[],
-	uint32_t val[], uint32_t num);
+	uint32_t val[], uint32_t num)
+{
+	return ___rte_thash_gfni_bulk(mtrx, len, tuple, val);
+}
 
 #endif /* RTE_THASH_GFNI_DEFINED */
 
diff --git a/lib/hash/version.map b/lib/hash/version.map
index 6b2afebf6b46..942e2998578f 100644
--- a/lib/hash/version.map
+++ b/lib/hash/version.map
@@ -41,10 +41,15 @@ DPDK_24 {
 	rte_thash_get_gfni_matrices;
 	rte_thash_get_helper;
 	rte_thash_get_key;
-	rte_thash_gfni;
-	rte_thash_gfni_bulk;
 	rte_thash_gfni_supported;
 	rte_thash_init_ctx;
 
 	local: *;
 };
+
+INTERNAL {
+	global:
+
+	___rte_thash_gfni;
+	___rte_thash_gfni_bulk;
+};
-- 
2.43.0

Re: [PATCH] hash: make gfni stubs inline

[David Marchand]

On Tue, Mar 5, 2024 at 6:53 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> On Tue, Mar 05, 2024 at 11:14:45AM +0100, David Marchand wrote:
> > On Mon, Mar 4, 2024 at 7:45 PM Stephen Hemminger
> > <stephen@networkplumber.org> wrote:
> > >
> > > This reverts commit 07d836e5929d18ad6640ebae90dd2f81a2cafb71.
> > >
> > > Tyler found build issues with MSVC and the thash gfni stubs.
> > > The problem would be link errors from missing symbols.
> >
> > Trying to understand this link error.
> > Does it come from the fact that rte_thash_gfni/rte_thash_gfni_bulk
> > declarations are hidden under RTE_THASH_GFNI_DEFINED in
> > rte_thash_gfni.h?
> >
> > If so, why not always expose those two symbols unconditionnally and
> > link with the stub only when ! RTE_THASH_GFNI_DEFINED.
>
> So I don't have a lot of background of this lib.
>
> I think we understand that we can't conditionally expose symbols. That's
> what windows was picking up because it seems none of our CI's ever end
> up with RTE_THASH_GFNI_DEFINED but my local test system did and failed.
> (my experiments showed that Linux would complain too if it was defined)

I can't reproduce a problem when I build (gcc/clang) for a target that
has GFNI/AVX512F.
binutils ld seems to just ignore unknown symbols in the map.

With current main:
[dmarchan@dmarchan main]$ nm build/lib/librte_hash.so.24.1 | grep rte_thash_gfni
00000000000088b0 T rte_thash_gfni_supported
[dmarchan@dmarchan main]$ nm build-nogfni/lib/librte_hash.so.24.1 |
grep rte_thash_gfni
00000000000102c0 T rte_thash_gfni
00000000000102d0 T rte_thash_gfni_bulk
000000000000294e t rte_thash_gfni_bulk.cold
0000000000002918 t rte_thash_gfni.cold
000000000000d3c0 T rte_thash_gfni_supported


>
> If we always expose the symbols then as you point out we have to
> conditionally link with the stub otherwise the inline (non-stub) will be
> duplicate and build / link will fail.
>
> I guess the part I don't understand with your suggestion is how we would
> conditionally link with just the stub? We have to link with rte_hash to
> get the rest of hash and the stub. I've probably missed something here.

No we can't, Stephen suggestion is a full solution.

>
> Since we never had a release exposing the new symbols introduced by
> Stephen in question my suggestion was that we just revert for 24.03 so
> we don't end up with an ABI break later if we choose to solve the
> problem without exports.
>
> I don't know what else to do, but I think we need to decide for 24.03.

I am fully aware that we must fix this for 24.03.

I would like to be sure Stephen fix (see v3) works for you, so have a
look because I am not able to reproduce an issue and validate the fix
myself.


-- 
David Marchand

Re: [PATCH] hash: make gfni stubs inline

[Stephen Hemminger]

On Thu, 7 Mar 2024 11:32:36 +0100
David Marchand <david.marchand@redhat.com> wrote:

> On Tue, Mar 5, 2024 at 6:53 PM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> >
> > On Tue, Mar 05, 2024 at 11:14:45AM +0100, David Marchand wrote:  
> > > On Mon, Mar 4, 2024 at 7:45 PM Stephen Hemminger
> > > <stephen@networkplumber.org> wrote:  
> > > >
> > > > This reverts commit 07d836e5929d18ad6640ebae90dd2f81a2cafb71.
> > > >
> > > > Tyler found build issues with MSVC and the thash gfni stubs.
> > > > The problem would be link errors from missing symbols.  
> > >
> > > Trying to understand this link error.
> > > Does it come from the fact that rte_thash_gfni/rte_thash_gfni_bulk
> > > declarations are hidden under RTE_THASH_GFNI_DEFINED in
> > > rte_thash_gfni.h?
> > >
> > > If so, why not always expose those two symbols unconditionnally and
> > > link with the stub only when ! RTE_THASH_GFNI_DEFINED.  
> >
> > So I don't have a lot of background of this lib.
> >
> > I think we understand that we can't conditionally expose symbols. That's
> > what windows was picking up because it seems none of our CI's ever end
> > up with RTE_THASH_GFNI_DEFINED but my local test system did and failed.
> > (my experiments showed that Linux would complain too if it was defined)  
> 
> I can't reproduce a problem when I build (gcc/clang) for a target that
> has GFNI/AVX512F.
> binutils ld seems to just ignore unknown symbols in the map.


Turns out MSVC linker does not. That is why Tyler complained.

[PATCH v3] hash: put GFNI stubs back

[Stephen Hemminger]

Make the GFNI stub functions always built. This solves the conditional
linking problem. If GFNI is available, they will never get used.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/hash/rte_thash_gfni.c |  9 +++------
 lib/hash/rte_thash_gfni.h | 30 +++++++++++++++++++++++-------
 lib/hash/version.map      |  9 +++++++--
 3 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/lib/hash/rte_thash_gfni.c b/lib/hash/rte_thash_gfni.c
index f1525f9838de..5ead51dd3426 100644
--- a/lib/hash/rte_thash_gfni.c
+++ b/lib/hash/rte_thash_gfni.c
@@ -4,18 +4,17 @@
 
 #include <stdbool.h>
 
+#include <rte_compat.h>
 #include <rte_log.h>
 #include <rte_thash_gfni.h>
 
-#ifndef RTE_THASH_GFNI_DEFINED
-
 RTE_LOG_REGISTER_SUFFIX(hash_gfni_logtype, gfni, INFO);
 #define RTE_LOGTYPE_HASH hash_gfni_logtype
 #define HASH_LOG(level, ...) \
 	RTE_LOG_LINE(level, HASH, "" __VA_ARGS__)
 
 uint32_t
-rte_thash_gfni(const uint64_t *mtrx __rte_unused,
+___rte_thash_gfni(const uint64_t *mtrx __rte_unused,
 	const uint8_t *key __rte_unused, int len __rte_unused)
 {
 	static bool warned;
@@ -30,7 +29,7 @@ rte_thash_gfni(const uint64_t *mtrx __rte_unused,
 }
 
 void
-rte_thash_gfni_bulk(const uint64_t *mtrx __rte_unused,
+___rte_thash_gfni_bulk(const uint64_t *mtrx __rte_unused,
 	int len __rte_unused, uint8_t *tuple[] __rte_unused,
 	uint32_t val[], uint32_t num)
 {
@@ -47,5 +46,3 @@ rte_thash_gfni_bulk(const uint64_t *mtrx __rte_unused,
 	for (i = 0; i < num; i++)
 		val[i] = 0;
 }
-
-#endif
diff --git a/lib/hash/rte_thash_gfni.h b/lib/hash/rte_thash_gfni.h
index eed55fc86c86..ebc531817b87 100644
--- a/lib/hash/rte_thash_gfni.h
+++ b/lib/hash/rte_thash_gfni.h
@@ -17,11 +17,22 @@ extern "C" {
 
 #endif
 
-#ifndef RTE_THASH_GFNI_DEFINED
+/*
+ * @internal
+ * Stubs defined for only used when GFNI is not available
+ */
+__rte_internal
+uint32_t
+___rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len);
+
+__rte_internal
+void
+___rte_thash_gfni_bulk(const uint64_t *mtrx, int len, uint8_t *tuple[],
+		       uint32_t val[], uint32_t num);
 
+#ifndef RTE_THASH_GFNI_DEFINED
 /**
  * Calculate Toeplitz hash.
- * Dummy implementation.
  *
  * @param m
  *  Pointer to the matrices generated from the corresponding
@@ -33,12 +44,14 @@ extern "C" {
  * @return
  *  Calculated Toeplitz hash value.
  */
-uint32_t
-rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len);
+static inline uint32_t
+rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len)
+{
+	return ___rte_thash_gfni(mtrx, key, len);
+}
 
 /**
  * Bulk implementation for Toeplitz hash.
- * Dummy implementation.
  *
  * @param m
  *  Pointer to the matrices generated from the corresponding
@@ -53,9 +66,12 @@ rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len);
  * @param num
  *  Number of tuples to hash.
  */
-void
+static inline void
 rte_thash_gfni_bulk(const uint64_t *mtrx, int len, uint8_t *tuple[],
-	uint32_t val[], uint32_t num);
+	uint32_t val[], uint32_t num)
+{
+	return ___rte_thash_gfni_bulk(mtrx, len, tuple, val);
+}
 
 #endif /* RTE_THASH_GFNI_DEFINED */
 
diff --git a/lib/hash/version.map b/lib/hash/version.map
index 6b2afebf6b46..942e2998578f 100644
--- a/lib/hash/version.map
+++ b/lib/hash/version.map
@@ -41,10 +41,15 @@ DPDK_24 {
 	rte_thash_get_gfni_matrices;
 	rte_thash_get_helper;
 	rte_thash_get_key;
-	rte_thash_gfni;
-	rte_thash_gfni_bulk;
 	rte_thash_gfni_supported;
 	rte_thash_init_ctx;
 
 	local: *;
 };
+
+INTERNAL {
+	global:
+
+	___rte_thash_gfni;
+	___rte_thash_gfni_bulk;
+};
-- 
2.43.0

[PATCH v3] hash: put GFNI stubs back

[Stephen Hemminger]

Make the GFNI stub functions always built. This solves the conditional
linking problem. If GFNI is available, they will never get used.

Fixes: 07d836e5929d ("hash: uninline GFNI stubs")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v3 - fix typo in call to ___rte_thash_gfni_bulk

 lib/hash/rte_thash_gfni.c |  9 +++------
 lib/hash/rte_thash_gfni.h | 30 +++++++++++++++++++++++-------
 lib/hash/version.map      |  9 +++++++--
 3 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/lib/hash/rte_thash_gfni.c b/lib/hash/rte_thash_gfni.c
index f1525f9838de..5ead51dd3426 100644
--- a/lib/hash/rte_thash_gfni.c
+++ b/lib/hash/rte_thash_gfni.c
@@ -4,18 +4,17 @@
 
 #include <stdbool.h>
 
+#include <rte_compat.h>
 #include <rte_log.h>
 #include <rte_thash_gfni.h>
 
-#ifndef RTE_THASH_GFNI_DEFINED
-
 RTE_LOG_REGISTER_SUFFIX(hash_gfni_logtype, gfni, INFO);
 #define RTE_LOGTYPE_HASH hash_gfni_logtype
 #define HASH_LOG(level, ...) \
 	RTE_LOG_LINE(level, HASH, "" __VA_ARGS__)
 
 uint32_t
-rte_thash_gfni(const uint64_t *mtrx __rte_unused,
+___rte_thash_gfni(const uint64_t *mtrx __rte_unused,
 	const uint8_t *key __rte_unused, int len __rte_unused)
 {
 	static bool warned;
@@ -30,7 +29,7 @@ rte_thash_gfni(const uint64_t *mtrx __rte_unused,
 }
 
 void
-rte_thash_gfni_bulk(const uint64_t *mtrx __rte_unused,
+___rte_thash_gfni_bulk(const uint64_t *mtrx __rte_unused,
 	int len __rte_unused, uint8_t *tuple[] __rte_unused,
 	uint32_t val[], uint32_t num)
 {
@@ -47,5 +46,3 @@ rte_thash_gfni_bulk(const uint64_t *mtrx __rte_unused,
 	for (i = 0; i < num; i++)
 		val[i] = 0;
 }
-
-#endif
diff --git a/lib/hash/rte_thash_gfni.h b/lib/hash/rte_thash_gfni.h
index eed55fc86c86..432d5b63a12e 100644
--- a/lib/hash/rte_thash_gfni.h
+++ b/lib/hash/rte_thash_gfni.h
@@ -17,11 +17,22 @@ extern "C" {
 
 #endif
 
-#ifndef RTE_THASH_GFNI_DEFINED
+/*
+ * @internal
+ * Stubs defined for only used when GFNI is not available
+ */
+__rte_internal
+uint32_t
+___rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len);
+
+__rte_internal
+void
+___rte_thash_gfni_bulk(const uint64_t *mtrx, int len, uint8_t *tuple[],
+		       uint32_t val[], uint32_t num);
 
+#ifndef RTE_THASH_GFNI_DEFINED
 /**
  * Calculate Toeplitz hash.
- * Dummy implementation.
  *
  * @param m
  *  Pointer to the matrices generated from the corresponding
@@ -33,12 +44,14 @@ extern "C" {
  * @return
  *  Calculated Toeplitz hash value.
  */
-uint32_t
-rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len);
+static inline uint32_t
+rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len)
+{
+	return ___rte_thash_gfni(mtrx, key, len);
+}
 
 /**
  * Bulk implementation for Toeplitz hash.
- * Dummy implementation.
  *
  * @param m
  *  Pointer to the matrices generated from the corresponding
@@ -53,9 +66,12 @@ rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len);
  * @param num
  *  Number of tuples to hash.
  */
-void
+static inline void
 rte_thash_gfni_bulk(const uint64_t *mtrx, int len, uint8_t *tuple[],
-	uint32_t val[], uint32_t num);
+	uint32_t val[], uint32_t num)
+{
+	return ___rte_thash_gfni_bulk(mtrx, len, tuple, val, num);
+}
 
 #endif /* RTE_THASH_GFNI_DEFINED */
 
diff --git a/lib/hash/version.map b/lib/hash/version.map
index 6b2afebf6b46..942e2998578f 100644
--- a/lib/hash/version.map
+++ b/lib/hash/version.map
@@ -41,10 +41,15 @@ DPDK_24 {
 	rte_thash_get_gfni_matrices;
 	rte_thash_get_helper;
 	rte_thash_get_key;
-	rte_thash_gfni;
-	rte_thash_gfni_bulk;
 	rte_thash_gfni_supported;
 	rte_thash_init_ctx;
 
 	local: *;
 };
+
+INTERNAL {
+	global:
+
+	___rte_thash_gfni;
+	___rte_thash_gfni_bulk;
+};
-- 
2.43.0

Re: [PATCH v3] hash: put GFNI stubs back

[David Marchand]

Hello,

On Thu, Mar 7, 2024 at 2:37 AM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> Make the GFNI stub functions always built. This solves the conditional
> linking problem. If GFNI is available, they will never get used.
>
> Fixes: 07d836e5929d ("hash: uninline GFNI stubs")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> v3 - fix typo in call to ___rte_thash_gfni_bulk
>
>  lib/hash/rte_thash_gfni.c |  9 +++------
>  lib/hash/rte_thash_gfni.h | 30 +++++++++++++++++++++++-------
>  lib/hash/version.map      |  9 +++++++--
>  3 files changed, 33 insertions(+), 15 deletions(-)
>
> diff --git a/lib/hash/rte_thash_gfni.c b/lib/hash/rte_thash_gfni.c
> index f1525f9838de..5ead51dd3426 100644
> --- a/lib/hash/rte_thash_gfni.c
> +++ b/lib/hash/rte_thash_gfni.c
> @@ -4,18 +4,17 @@
>
>  #include <stdbool.h>
>
> +#include <rte_compat.h>

This should be in the header since this is where the __rte_internal tag is used.
Moving this inclusion will fix the failing headers check for arches != x86:
http://mails.dpdk.org/archives/test-report/2024-March/603562.html


>  #include <rte_log.h>
>  #include <rte_thash_gfni.h>
>
> -#ifndef RTE_THASH_GFNI_DEFINED
> -
>  RTE_LOG_REGISTER_SUFFIX(hash_gfni_logtype, gfni, INFO);
>  #define RTE_LOGTYPE_HASH hash_gfni_logtype
>  #define HASH_LOG(level, ...) \
>         RTE_LOG_LINE(level, HASH, "" __VA_ARGS__)
>
>  uint32_t
> -rte_thash_gfni(const uint64_t *mtrx __rte_unused,
> +___rte_thash_gfni(const uint64_t *mtrx __rte_unused,

Let's go with a s/___rte_thash_gfni/rte_thash_gfni_stub/g.


>         const uint8_t *key __rte_unused, int len __rte_unused)
>  {
>         static bool warned;
> @@ -30,7 +29,7 @@ rte_thash_gfni(const uint64_t *mtrx __rte_unused,
>  }
>
>  void
> -rte_thash_gfni_bulk(const uint64_t *mtrx __rte_unused,
> +___rte_thash_gfni_bulk(const uint64_t *mtrx __rte_unused,
>         int len __rte_unused, uint8_t *tuple[] __rte_unused,
>         uint32_t val[], uint32_t num)
>  {
> @@ -47,5 +46,3 @@ rte_thash_gfni_bulk(const uint64_t *mtrx __rte_unused,
>         for (i = 0; i < num; i++)
>                 val[i] = 0;
>  }
> -
> -#endif
> diff --git a/lib/hash/rte_thash_gfni.h b/lib/hash/rte_thash_gfni.h
> index eed55fc86c86..432d5b63a12e 100644
> --- a/lib/hash/rte_thash_gfni.h
> +++ b/lib/hash/rte_thash_gfni.h
> @@ -17,11 +17,22 @@ extern "C" {
>
>  #endif
>
> -#ifndef RTE_THASH_GFNI_DEFINED
> +/*
> + * @internal
> + * Stubs defined for only used when GFNI is not available

This reads strange, I'll reword when applying as:
* Stubs only used when GFNI is not available


> + */
> +__rte_internal
> +uint32_t
> +___rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len);
> +
> +__rte_internal
> +void
> +___rte_thash_gfni_bulk(const uint64_t *mtrx, int len, uint8_t *tuple[],
> +                      uint32_t val[], uint32_t num);
>
> +#ifndef RTE_THASH_GFNI_DEFINED
>  /**
>   * Calculate Toeplitz hash.
> - * Dummy implementation.
>   *
>   * @param m
>   *  Pointer to the matrices generated from the corresponding
> @@ -33,12 +44,14 @@ extern "C" {
>   * @return
>   *  Calculated Toeplitz hash value.
>   */
> -uint32_t
> -rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len);
> +static inline uint32_t
> +rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len)
> +{
> +       return ___rte_thash_gfni(mtrx, key, len);
> +}
>
>  /**
>   * Bulk implementation for Toeplitz hash.
> - * Dummy implementation.
>   *
>   * @param m
>   *  Pointer to the matrices generated from the corresponding
> @@ -53,9 +66,12 @@ rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len);
>   * @param num
>   *  Number of tuples to hash.
>   */
> -void
> +static inline void
>  rte_thash_gfni_bulk(const uint64_t *mtrx, int len, uint8_t *tuple[],
> -       uint32_t val[], uint32_t num);
> +       uint32_t val[], uint32_t num)
> +{
> +       return ___rte_thash_gfni_bulk(mtrx, len, tuple, val, num);
> +}
>
>  #endif /* RTE_THASH_GFNI_DEFINED */
>
> diff --git a/lib/hash/version.map b/lib/hash/version.map
> index 6b2afebf6b46..942e2998578f 100644
> --- a/lib/hash/version.map
> +++ b/lib/hash/version.map
> @@ -41,10 +41,15 @@ DPDK_24 {
>         rte_thash_get_gfni_matrices;
>         rte_thash_get_helper;
>         rte_thash_get_key;
> -       rte_thash_gfni;
> -       rte_thash_gfni_bulk;
>         rte_thash_gfni_supported;
>         rte_thash_init_ctx;
>
>         local: *;
>  };
> +
> +INTERNAL {
> +       global:
> +
> +       ___rte_thash_gfni;
> +       ___rte_thash_gfni_bulk;
> +};
> --
> 2.43.0
>

The rest lgtm but I can't reproduce a link error in the first place.
I'll wait for Tyler to confirm this patch fixes the issue he hit.


-- 
David Marchand

Re: [PATCH v3] hash: put GFNI stubs back

[Tyler Retzlaff]

On Wed, Mar 06, 2024 at 05:36:42PM -0800, Stephen Hemminger wrote:
> Make the GFNI stub functions always built. This solves the conditional
> linking problem. If GFNI is available, they will never get used.
> 
> Fixes: 07d836e5929d ("hash: uninline GFNI stubs")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---

I can confirm that v3 of the series allows me to complete build & link
with LLVM 14 / clang.

Tested-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

> v3 - fix typo in call to ___rte_thash_gfni_bulk
> 
>  lib/hash/rte_thash_gfni.c |  9 +++------
>  lib/hash/rte_thash_gfni.h | 30 +++++++++++++++++++++++-------
>  lib/hash/version.map      |  9 +++++++--
>  3 files changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/lib/hash/rte_thash_gfni.c b/lib/hash/rte_thash_gfni.c
> index f1525f9838de..5ead51dd3426 100644
> --- a/lib/hash/rte_thash_gfni.c
> +++ b/lib/hash/rte_thash_gfni.c
> @@ -4,18 +4,17 @@
>  
>  #include <stdbool.h>
>  
> +#include <rte_compat.h>
>  #include <rte_log.h>
>  #include <rte_thash_gfni.h>
>  
> -#ifndef RTE_THASH_GFNI_DEFINED
> -
>  RTE_LOG_REGISTER_SUFFIX(hash_gfni_logtype, gfni, INFO);
>  #define RTE_LOGTYPE_HASH hash_gfni_logtype
>  #define HASH_LOG(level, ...) \
>  	RTE_LOG_LINE(level, HASH, "" __VA_ARGS__)
>  
>  uint32_t
> -rte_thash_gfni(const uint64_t *mtrx __rte_unused,
> +___rte_thash_gfni(const uint64_t *mtrx __rte_unused,
>  	const uint8_t *key __rte_unused, int len __rte_unused)
>  {
>  	static bool warned;
> @@ -30,7 +29,7 @@ rte_thash_gfni(const uint64_t *mtrx __rte_unused,
>  }
>  
>  void
> -rte_thash_gfni_bulk(const uint64_t *mtrx __rte_unused,
> +___rte_thash_gfni_bulk(const uint64_t *mtrx __rte_unused,
>  	int len __rte_unused, uint8_t *tuple[] __rte_unused,
>  	uint32_t val[], uint32_t num)
>  {
> @@ -47,5 +46,3 @@ rte_thash_gfni_bulk(const uint64_t *mtrx __rte_unused,
>  	for (i = 0; i < num; i++)
>  		val[i] = 0;
>  }
> -
> -#endif
> diff --git a/lib/hash/rte_thash_gfni.h b/lib/hash/rte_thash_gfni.h
> index eed55fc86c86..432d5b63a12e 100644
> --- a/lib/hash/rte_thash_gfni.h
> +++ b/lib/hash/rte_thash_gfni.h
> @@ -17,11 +17,22 @@ extern "C" {
>  
>  #endif
>  
> -#ifndef RTE_THASH_GFNI_DEFINED
> +/*
> + * @internal
> + * Stubs defined for only used when GFNI is not available
> + */
> +__rte_internal
> +uint32_t
> +___rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len);
> +
> +__rte_internal
> +void
> +___rte_thash_gfni_bulk(const uint64_t *mtrx, int len, uint8_t *tuple[],
> +		       uint32_t val[], uint32_t num);
>  
> +#ifndef RTE_THASH_GFNI_DEFINED
>  /**
>   * Calculate Toeplitz hash.
> - * Dummy implementation.
>   *
>   * @param m
>   *  Pointer to the matrices generated from the corresponding
> @@ -33,12 +44,14 @@ extern "C" {
>   * @return
>   *  Calculated Toeplitz hash value.
>   */
> -uint32_t
> -rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len);
> +static inline uint32_t
> +rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len)
> +{
> +	return ___rte_thash_gfni(mtrx, key, len);
> +}
>  
>  /**
>   * Bulk implementation for Toeplitz hash.
> - * Dummy implementation.
>   *
>   * @param m
>   *  Pointer to the matrices generated from the corresponding
> @@ -53,9 +66,12 @@ rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len);
>   * @param num
>   *  Number of tuples to hash.
>   */
> -void
> +static inline void
>  rte_thash_gfni_bulk(const uint64_t *mtrx, int len, uint8_t *tuple[],
> -	uint32_t val[], uint32_t num);
> +	uint32_t val[], uint32_t num)
> +{
> +	return ___rte_thash_gfni_bulk(mtrx, len, tuple, val, num);
> +}
>  
>  #endif /* RTE_THASH_GFNI_DEFINED */
>  
> diff --git a/lib/hash/version.map b/lib/hash/version.map
> index 6b2afebf6b46..942e2998578f 100644
> --- a/lib/hash/version.map
> +++ b/lib/hash/version.map
> @@ -41,10 +41,15 @@ DPDK_24 {
>  	rte_thash_get_gfni_matrices;
>  	rte_thash_get_helper;
>  	rte_thash_get_key;
> -	rte_thash_gfni;
> -	rte_thash_gfni_bulk;
>  	rte_thash_gfni_supported;
>  	rte_thash_init_ctx;
>  
>  	local: *;
>  };
> +
> +INTERNAL {
> +	global:
> +
> +	___rte_thash_gfni;
> +	___rte_thash_gfni_bulk;
> +};
> -- 
> 2.43.0

Re: [PATCH v3] hash: put GFNI stubs back

[David Marchand]

On Thu, Mar 7, 2024 at 6:37 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
> On Wed, Mar 06, 2024 at 05:36:42PM -0800, Stephen Hemminger wrote:
> > Make the GFNI stub functions always built. This solves the conditional
> > linking problem. If GFNI is available, they will never get used.
> >
> > Fixes: 07d836e5929d ("hash: uninline GFNI stubs")
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Tested-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

Applied with the edits I mentionned previously.


-- 
David Marchand

Re: [PATCH v3] hash: put GFNI stubs back

[Stephen Hemminger]

On Thu, 7 Mar 2024 18:59:11 +0100
David Marchand <david.marchand@redhat.com> wrote:

> On Thu, Mar 7, 2024 at 6:37 PM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> > On Wed, Mar 06, 2024 at 05:36:42PM -0800, Stephen Hemminger wrote:  
> > > Make the GFNI stub functions always built. This solves the conditional
> > > linking problem. If GFNI is available, they will never get used.
> > >
> > > Fixes: 07d836e5929d ("hash: uninline GFNI stubs")
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>  
> > Tested-by: Tyler Retzlaff <roretzla@linux.microsoft.com>  
> 
> Applied with the edits I mentionned previously.
> 
> 

I noticed that there is an still an unnecessary inclusion of rte_log.h
in rte_thash_gfni.h but that doesn't matter.

[PATCH v4] hash: put GFNI stubs back

[Stephen Hemminger]

Make the GFNI stub functions always built. This solves the conditional
linking problem. If GFNI is available, they will never get used.

Fixes: 07d836e5929d ("hash: uninline GFNI stubs")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v4 - fix build on aarch64 and __rte_internal

 lib/hash/rte_thash_gfni.c |  8 ++------
 lib/hash/rte_thash_gfni.h | 32 ++++++++++++++++++++++++--------
 lib/hash/version.map      |  9 +++++++--
 3 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/lib/hash/rte_thash_gfni.c b/lib/hash/rte_thash_gfni.c
index f1525f9838de..feb2b464de10 100644
--- a/lib/hash/rte_thash_gfni.c
+++ b/lib/hash/rte_thash_gfni.c
@@ -7,15 +7,13 @@
 #include <rte_log.h>
 #include <rte_thash_gfni.h>
 
-#ifndef RTE_THASH_GFNI_DEFINED
-
 RTE_LOG_REGISTER_SUFFIX(hash_gfni_logtype, gfni, INFO);
 #define RTE_LOGTYPE_HASH hash_gfni_logtype
 #define HASH_LOG(level, ...) \
 	RTE_LOG_LINE(level, HASH, "" __VA_ARGS__)
 
 uint32_t
-rte_thash_gfni(const uint64_t *mtrx __rte_unused,
+___rte_thash_gfni(const uint64_t *mtrx __rte_unused,
 	const uint8_t *key __rte_unused, int len __rte_unused)
 {
 	static bool warned;
@@ -30,7 +28,7 @@ rte_thash_gfni(const uint64_t *mtrx __rte_unused,
 }
 
 void
-rte_thash_gfni_bulk(const uint64_t *mtrx __rte_unused,
+___rte_thash_gfni_bulk(const uint64_t *mtrx __rte_unused,
 	int len __rte_unused, uint8_t *tuple[] __rte_unused,
 	uint32_t val[], uint32_t num)
 {
@@ -47,5 +45,3 @@ rte_thash_gfni_bulk(const uint64_t *mtrx __rte_unused,
 	for (i = 0; i < num; i++)
 		val[i] = 0;
 }
-
-#endif
diff --git a/lib/hash/rte_thash_gfni.h b/lib/hash/rte_thash_gfni.h
index eed55fc86c86..2e169caef23f 100644
--- a/lib/hash/rte_thash_gfni.h
+++ b/lib/hash/rte_thash_gfni.h
@@ -9,7 +9,7 @@
 extern "C" {
 #endif
 
-#include <rte_log.h>
+#include <rte_compat.h>
 
 #ifdef RTE_ARCH_X86
 
@@ -17,11 +17,22 @@ extern "C" {
 
 #endif
 
-#ifndef RTE_THASH_GFNI_DEFINED
+/*
+ * @internal
+ * Stubs defined for only used when GFNI is not available
+ */
+__rte_internal
+uint32_t
+___rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len);
+
+__rte_internal
+void
+___rte_thash_gfni_bulk(const uint64_t *mtrx, int len, uint8_t *tuple[],
+		       uint32_t val[], uint32_t num);
 
+#ifndef RTE_THASH_GFNI_DEFINED
 /**
  * Calculate Toeplitz hash.
- * Dummy implementation.
  *
  * @param m
  *  Pointer to the matrices generated from the corresponding
@@ -33,12 +44,14 @@ extern "C" {
  * @return
  *  Calculated Toeplitz hash value.
  */
-uint32_t
-rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len);
+static inline uint32_t
+rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len)
+{
+	return ___rte_thash_gfni(mtrx, key, len);
+}
 
 /**
  * Bulk implementation for Toeplitz hash.
- * Dummy implementation.
  *
  * @param m
  *  Pointer to the matrices generated from the corresponding
@@ -53,9 +66,12 @@ rte_thash_gfni(const uint64_t *mtrx, const uint8_t *key, int len);
  * @param num
  *  Number of tuples to hash.
  */
-void
+static inline void
 rte_thash_gfni_bulk(const uint64_t *mtrx, int len, uint8_t *tuple[],
-	uint32_t val[], uint32_t num);
+	uint32_t val[], uint32_t num)
+{
+	return ___rte_thash_gfni_bulk(mtrx, len, tuple, val, num);
+}
 
 #endif /* RTE_THASH_GFNI_DEFINED */
 
diff --git a/lib/hash/version.map b/lib/hash/version.map
index 6b2afebf6b46..942e2998578f 100644
--- a/lib/hash/version.map
+++ b/lib/hash/version.map
@@ -41,10 +41,15 @@ DPDK_24 {
 	rte_thash_get_gfni_matrices;
 	rte_thash_get_helper;
 	rte_thash_get_key;
-	rte_thash_gfni;
-	rte_thash_gfni_bulk;
 	rte_thash_gfni_supported;
 	rte_thash_init_ctx;
 
 	local: *;
 };
+
+INTERNAL {
+	global:
+
+	___rte_thash_gfni;
+	___rte_thash_gfni_bulk;
+};
-- 
2.43.0