On Tue, 5 Mar 2024 09:53:00 -0800
Tyler Retzlaff <roret...@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
> > <step...@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 <step...@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 <step...@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
