hash: make gfni stubs inline

  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(-)
  

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.
  

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
  

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;
+};
  

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.
  

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.