Re: [PATCH] hash: make gfni stubs inline

[Stephen Hemminger <stephen@networkplumber.org>]

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