[PATCH 0/3] Enhance headers check

DPDK patches and discussions
 help / color / mirror / Atom feed

* [PATCH 0/3] Enhance headers check
@ 2024-10-15 12:10 David Marchand
  2024-10-15 12:10 ` [PATCH 1/3] bitops: fix build for GCC without experimental API David Marchand
                   ` (4 more replies)
  0 siblings, 5 replies; 34+ messages in thread
From: David Marchand @ 2024-10-15 12:10 UTC (permalink / raw)
  To: dev; +Cc: thomas, bruce.richardson, ktraynor

We currently check that exported headers are fine with
-DALLOW_EXPERIMENTAL_API and -DALLOW_INTERNAL_API.

Such a check won't catch issues like the one fixed in patch 1, where OVS
compilation is broken by the additional of experimental API in a header
commonly included in other parts of DPDK.

Ideally, I would like to merge patch 1 in rc1.
The patch 2 is not a real issue (more like the enhanced check would
complain about this header).
In any case, I think it would not hurt merging everything now.

Comments, please.


-- 
David Marchand

David Marchand (3):
  bitops: fix build for GCC without experimental API
  bitset: fix build for GCC without experimental API
  buildtools/chkincs: check headers with stable API only

 buildtools/chkincs/meson.build |  27 +++++++-
 lib/eal/include/rte_bitops.h   |   6 ++
 lib/eal/include/rte_bitset.h   | 123 +++++++++++++++++++++++++++++++++
 3 files changed, 154 insertions(+), 2 deletions(-)

-- 
2.46.2


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH 1/3] bitops: fix build for GCC without experimental API
  2024-10-15 12:10 [PATCH 0/3] Enhance headers check David Marchand
@ 2024-10-15 12:10 ` David Marchand
  2024-10-15 12:47   ` Morten Brørup
  2024-10-15 12:10 ` [PATCH 2/3] bitset: " David Marchand
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 34+ messages in thread
From: David Marchand @ 2024-10-15 12:10 UTC (permalink / raw)
  To: dev
  Cc: thomas, bruce.richardson, ktraynor, Jack Bond-Preston,
	Tyler Retzlaff, Morten Brørup, Mattias Rönnblom

Building OVS against current DPDK fails with following warnings:

In file included from .../ovs/dpdk-dir/include/rte_memory.h:18,
                 from .../ovs/dpdk-dir/include/rte_ring_core.h:29,
                 from .../ovs/dpdk-dir/include/rte_ring.h:37,
                 from .../ovs/dpdk-dir/include/rte_mempool.h:49,
                 from .../ovs/dpdk-dir/include/rte_mbuf.h:38,
                 from lib/dp-packet.h:25,
                 from lib/ofp-packet.c:20:
.../ovs/dpdk-dir/include/rte_bitops.h: In function ‘__rte_bit_assign32’:
.../ovs/dpdk-dir/include/rte_bitops.h:528:1: error:
	‘__rte_bit_set32’ is deprecated: Symbol is not yet part of
	stable ABI [-Werror=deprecated-declarations]
...

This comes from the fact that some (experimental) inline helpers
are calling other experimental API.
Hide those calls.

Fixes: 471de107ae23 ("bitops: add new bit manipulation API")

Reported-by: Kevin Traynor <ktraynor@redhat.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/eal/include/rte_bitops.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h
index e08e41199a..deb1fd43f2 100644
--- a/lib/eal/include/rte_bitops.h
+++ b/lib/eal/include/rte_bitops.h
@@ -525,8 +525,10 @@ __rte_bit_ ## variant ## flip ## size(qualifier uint ## size ## _t *addr, unsign
 	__RTE_GEN_BIT_OPS(,, size) \
 	__RTE_GEN_BIT_OPS(v_, volatile, size)
 
+#ifdef ALLOW_EXPERIMENTAL_API
 __RTE_GEN_BIT_OPS_SIZE(32)
 __RTE_GEN_BIT_OPS_SIZE(64)
+#endif
 
 #define __RTE_GEN_BIT_ATOMIC_TEST(variant, qualifier, size) \
 __rte_experimental \
@@ -651,8 +653,10 @@ __rte_bit_atomic_ ## variant ## test_and_assign ## size( \
 	__RTE_GEN_BIT_ATOMIC_OPS(,, size) \
 	__RTE_GEN_BIT_ATOMIC_OPS(v_, volatile, size)
 
+#ifdef ALLOW_EXPERIMENTAL_API
 __RTE_GEN_BIT_ATOMIC_OPS_SIZE(32)
 __RTE_GEN_BIT_ATOMIC_OPS_SIZE(64)
+#endif
 
 /*------------------------ 32-bit relaxed operations ------------------------*/
 
@@ -1481,6 +1485,7 @@ rte_bit_ ## family ## fun(qualifier uint ## size ## _t *addr, arg1_type arg1_nam
 	__RTE_BIT_OVERLOAD_SZ_4R(family, fun, qualifier, 64, ret_type, arg1_type, arg1_name, \
 		arg2_type, arg2_name, arg3_type, arg3_name)
 
+#ifdef ALLOW_EXPERIMENTAL_API
 __RTE_BIT_OVERLOAD_2R(, test, const, bool, unsigned int, nr)
 __RTE_BIT_OVERLOAD_2(, set,, unsigned int, nr)
 __RTE_BIT_OVERLOAD_2(, clear,, unsigned int, nr)
@@ -1496,6 +1501,7 @@ __RTE_BIT_OVERLOAD_3R(atomic_, test_and_set,, bool, unsigned int, nr, int, memor
 __RTE_BIT_OVERLOAD_3R(atomic_, test_and_clear,, bool, unsigned int, nr, int, memory_order)
 __RTE_BIT_OVERLOAD_4R(atomic_, test_and_assign,, bool, unsigned int, nr, bool, value,
 	int, memory_order)
+#endif
 
 #endif
 
-- 
2.46.2


^ permalink raw reply	[flat|nested] 34+ messages in thread

* RE: [PATCH 1/3] bitops: fix build for GCC without experimental API
  2024-10-15 12:10 ` [PATCH 1/3] bitops: fix build for GCC without experimental API David Marchand
@ 2024-10-15 12:47   ` Morten Brørup
  0 siblings, 0 replies; 34+ messages in thread
From: Morten Brørup @ 2024-10-15 12:47 UTC (permalink / raw)
  To: David Marchand, dev
  Cc: thomas, bruce.richardson, ktraynor, Jack Bond-Preston,
	Tyler Retzlaff, Mattias Rönnblom

> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Tuesday, 15 October 2024 14.11
> 
> Building OVS against current DPDK fails with following warnings:
> 
> In file included from .../ovs/dpdk-dir/include/rte_memory.h:18,
>                  from .../ovs/dpdk-dir/include/rte_ring_core.h:29,
>                  from .../ovs/dpdk-dir/include/rte_ring.h:37,
>                  from .../ovs/dpdk-dir/include/rte_mempool.h:49,
>                  from .../ovs/dpdk-dir/include/rte_mbuf.h:38,
>                  from lib/dp-packet.h:25,
>                  from lib/ofp-packet.c:20:
> .../ovs/dpdk-dir/include/rte_bitops.h: In function
> ‘__rte_bit_assign32’:
> .../ovs/dpdk-dir/include/rte_bitops.h:528:1: error:
> 	‘__rte_bit_set32’ is deprecated: Symbol is not yet part of
> 	stable ABI [-Werror=deprecated-declarations]
> ...
> 
> This comes from the fact that some (experimental) inline helpers
> are calling other experimental API.
> Hide those calls.

Are you saying that the compiler warns if some experimental function calls another experimental function?

I understand the fix, so I am wondering about consistency:
Do we generally hide experimental functions if building without ALLOW_EXPERIMENTAL_API?
Or do we only do it to inline functions?


An alternative solution is copy-pasting the inlined implementations of the called experimental functions into the functions calling them.
I generally don't like copy-paste, so probably even worse.


Anyway, this is a viable fix, so
Acked-by: Morten Brørup <mb@smartsharesystems.com>


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH 2/3] bitset: fix build for GCC without experimental API
  2024-10-15 12:10 [PATCH 0/3] Enhance headers check David Marchand
  2024-10-15 12:10 ` [PATCH 1/3] bitops: fix build for GCC without experimental API David Marchand
@ 2024-10-15 12:10 ` David Marchand
  2024-10-15 12:53   ` Morten Brørup
  2024-10-15 12:10 ` [PATCH 3/3] buildtools/chkincs: check headers with stable API only David Marchand
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 34+ messages in thread
From: David Marchand @ 2024-10-15 12:10 UTC (permalink / raw)
  To: dev
  Cc: thomas, bruce.richardson, ktraynor, Mattias Rönnblom,
	Tyler Retzlaff, Morten Brørup

For a reason similar to the change on bitops header, hide bitset
implementation relying on experimental API.

Fixes: 99a1197647d8 ("eal: add bitset type")

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/eal/include/rte_bitset.h | 123 +++++++++++++++++++++++++++++++++++
 1 file changed, 123 insertions(+)

diff --git a/lib/eal/include/rte_bitset.h b/lib/eal/include/rte_bitset.h
index 74c643a72a..8ae8425fc2 100644
--- a/lib/eal/include/rte_bitset.h
+++ b/lib/eal/include/rte_bitset.h
@@ -255,7 +255,13 @@ __rte_experimental
 static inline bool
 rte_bitset_test(const uint64_t *bitset, size_t bit_num)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return __RTE_BITSET_DELEGATE(rte_bit_test, bitset, bit_num);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+	return false;
+#endif
 }
 
 /**
@@ -277,7 +283,12 @@ __rte_experimental
 static inline void
 rte_bitset_set(uint64_t *bitset, size_t bit_num)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	__RTE_BITSET_DELEGATE(rte_bit_set, bitset, bit_num);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+#endif
 }
 
 /**
@@ -299,7 +310,12 @@ __rte_experimental
 static inline void
 rte_bitset_clear(uint64_t *bitset, size_t bit_num)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	__RTE_BITSET_DELEGATE(rte_bit_clear, bitset, bit_num);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+#endif
 }
 
 /**
@@ -323,7 +339,13 @@ __rte_experimental
 static inline void
 rte_bitset_assign(uint64_t *bitset, size_t bit_num, bool bit_value)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	__RTE_BITSET_DELEGATE_N(rte_bit_assign, bitset, bit_num, bit_value);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+	RTE_SET_USED(bit_value);
+#endif
 }
 
 /**
@@ -345,7 +367,12 @@ __rte_experimental
 static inline void
 rte_bitset_flip(uint64_t *bitset, size_t bit_num)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	__RTE_BITSET_DELEGATE(rte_bit_flip, bitset, bit_num);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+#endif
 }
 
 /**
@@ -370,7 +397,14 @@ __rte_experimental
 static inline bool
 rte_bitset_atomic_test(const uint64_t *bitset, size_t bit_num, int memory_order)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return __RTE_BITSET_DELEGATE_N(rte_bit_atomic_test, bitset, bit_num, memory_order);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+	RTE_SET_USED(memory_order);
+	return false;
+#endif
 }
 
 /**
@@ -399,7 +433,13 @@ __rte_experimental
 static inline void
 rte_bitset_atomic_set(uint64_t *bitset, size_t bit_num, int memory_order)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	__RTE_BITSET_DELEGATE_N(rte_bit_atomic_set, bitset, bit_num, memory_order);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+	RTE_SET_USED(memory_order);
+#endif
 }
 
 /**
@@ -428,7 +468,13 @@ __rte_experimental
 static inline void
 rte_bitset_atomic_clear(uint64_t *bitset, size_t bit_num, int memory_order)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	__RTE_BITSET_DELEGATE_N(rte_bit_atomic_clear, bitset, bit_num, memory_order);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+	RTE_SET_USED(memory_order);
+#endif
 }
 
 /**
@@ -459,7 +505,14 @@ __rte_experimental
 static inline void
 rte_bitset_atomic_assign(uint64_t *bitset, size_t bit_num, bool bit_value, int memory_order)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	__RTE_BITSET_DELEGATE_N(rte_bit_atomic_assign, bitset, bit_num, bit_value, memory_order);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+	RTE_SET_USED(bit_value);
+	RTE_SET_USED(memory_order);
+#endif
 }
 
 /**
@@ -488,7 +541,13 @@ __rte_experimental
 static inline void
 rte_bitset_atomic_flip(uint64_t *bitset, size_t bit_num, int memory_order)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	__RTE_BITSET_DELEGATE_N(rte_bit_atomic_flip, bitset, bit_num, memory_order);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+	RTE_SET_USED(memory_order);
+#endif
 }
 
 /**
@@ -524,7 +583,12 @@ __rte_experimental
 static inline void
 rte_bitset_clear_all(uint64_t *bitset, size_t size)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	rte_bitset_init(bitset, size);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+#endif
 }
 
 /**
@@ -576,7 +640,13 @@ __rte_experimental
 static inline size_t
 rte_bitset_count_clear(const uint64_t *bitset, size_t size)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return size - rte_bitset_count_set(bitset, size);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+	return 0;
+#endif
 }
 
 #define __RTE_BITSET_FIND_FLAG_FIND_CLEAR (1U << 0)
@@ -638,6 +708,7 @@ static inline ssize_t
 __rte_bitset_find(const uint64_t *bitset, size_t size, size_t start_bit, size_t len,
 		unsigned int flags)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	bool find_clear = flags & __RTE_BITSET_FIND_FLAG_FIND_CLEAR;
 	bool may_wrap = flags & __RTE_BITSET_FIND_FLAG_WRAP;
 	bool does_wrap = (start_bit + len) > size;
@@ -658,6 +729,14 @@ __rte_bitset_find(const uint64_t *bitset, size_t size, size_t start_bit, size_t
 		rc = __rte_bitset_find_nowrap(bitset, size, start_bit, len, find_clear);
 
 	return rc;
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+	RTE_SET_USED(start_bit);
+	RTE_SET_USED(len);
+	RTE_SET_USED(flags);
+	return 0;
+#endif
 }
 
 /**
@@ -681,7 +760,13 @@ __rte_experimental
 static inline ssize_t
 rte_bitset_find_first_set(const uint64_t *bitset, size_t size)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return __rte_bitset_find(bitset, size, 0, size, 0);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+	return 0;
+#endif
 }
 
 /**
@@ -711,7 +796,15 @@ __rte_experimental
 static inline ssize_t
 rte_bitset_find_set(const uint64_t *bitset, size_t size, size_t start_bit, size_t len)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return __rte_bitset_find(bitset, size, start_bit, len, 0);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+	RTE_SET_USED(start_bit);
+	RTE_SET_USED(len);
+	return 0;
+#endif
 }
 
 /**
@@ -742,7 +835,15 @@ __rte_experimental
 static inline ssize_t
 rte_bitset_find_set_wrap(const uint64_t *bitset, size_t size, size_t start_bit, size_t len)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return __rte_bitset_find(bitset, size, start_bit, len, __RTE_BITSET_FIND_FLAG_WRAP);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+	RTE_SET_USED(start_bit);
+	RTE_SET_USED(len);
+	return 0;
+#endif
 }
 
 /**
@@ -766,7 +867,13 @@ __rte_experimental
 static inline ssize_t
 rte_bitset_find_first_clear(const uint64_t *bitset, size_t size)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return __rte_bitset_find(bitset, size, 0, size, __RTE_BITSET_FIND_FLAG_FIND_CLEAR);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+	return 0;
+#endif
 }
 
 /**
@@ -796,7 +903,15 @@ __rte_experimental
 static inline ssize_t
 rte_bitset_find_clear(const uint64_t *bitset, size_t size, size_t start_bit, size_t len)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return __rte_bitset_find(bitset, size, start_bit, len, __RTE_BITSET_FIND_FLAG_FIND_CLEAR);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+	RTE_SET_USED(start_bit);
+	RTE_SET_USED(len);
+	return 0;
+#endif
 }
 
 /**
@@ -827,8 +942,16 @@ __rte_experimental
 static inline ssize_t
 rte_bitset_find_clear_wrap(const uint64_t *bitset, size_t size, size_t start_bit, size_t len)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return __rte_bitset_find(bitset, size, start_bit, len,
 		__RTE_BITSET_FIND_FLAG_FIND_CLEAR | __RTE_BITSET_FIND_FLAG_WRAP);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+	RTE_SET_USED(start_bit);
+	RTE_SET_USED(len);
+	return 0;
+#endif
 }
 
 /**
-- 
2.46.2


^ permalink raw reply	[flat|nested] 34+ messages in thread

* RE: [PATCH 2/3] bitset: fix build for GCC without experimental API
  2024-10-15 12:10 ` [PATCH 2/3] bitset: " David Marchand
@ 2024-10-15 12:53   ` Morten Brørup
  2024-10-15 14:13     ` Thomas Monjalon
  0 siblings, 1 reply; 34+ messages in thread
From: Morten Brørup @ 2024-10-15 12:53 UTC (permalink / raw)
  To: David Marchand, dev
  Cc: thomas, bruce.richardson, ktraynor, Mattias Rönnblom,
	Tyler Retzlaff

> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Tuesday, 15 October 2024 14.11
> 
> For a reason similar to the change on bitops header, hide bitset
> implementation relying on experimental API.
> 
> Fixes: 99a1197647d8 ("eal: add bitset type")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  lib/eal/include/rte_bitset.h | 123 +++++++++++++++++++++++++++++++++++
>  1 file changed, 123 insertions(+)
> 
> diff --git a/lib/eal/include/rte_bitset.h
> b/lib/eal/include/rte_bitset.h
> index 74c643a72a..8ae8425fc2 100644
> --- a/lib/eal/include/rte_bitset.h
> +++ b/lib/eal/include/rte_bitset.h
> @@ -255,7 +255,13 @@ __rte_experimental
>  static inline bool
>  rte_bitset_test(const uint64_t *bitset, size_t bit_num)
>  {
> +#ifdef ALLOW_EXPERIMENTAL_API
>  	return __RTE_BITSET_DELEGATE(rte_bit_test, bitset, bit_num);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(bit_num);
> +	return false;
> +#endif
>  }

This looks wrong! The API is exposed, but does nothing.

It is possible to build without ALLOW_EXPERIMENTAL_API; the compiler will emit warnings when using experimental APIs.

If those compiler warnings are not handled as errors, the compiled application will be full of bugs.



^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 2/3] bitset: fix build for GCC without experimental API
  2024-10-15 12:53   ` Morten Brørup
@ 2024-10-15 14:13     ` Thomas Monjalon
  2024-10-15 14:44       ` Morten Brørup
  0 siblings, 1 reply; 34+ messages in thread
From: Thomas Monjalon @ 2024-10-15 14:13 UTC (permalink / raw)
  To: Morten Brørup
  Cc: David Marchand, dev, bruce.richardson, ktraynor,
	Mattias Rönnblom, Tyler Retzlaff

15/10/2024 14:53, Morten Brørup:
> > From: David Marchand [mailto:david.marchand@redhat.com]
> > @@ -255,7 +255,13 @@ __rte_experimental
> >  static inline bool
> >  rte_bitset_test(const uint64_t *bitset, size_t bit_num)
> >  {
> > +#ifdef ALLOW_EXPERIMENTAL_API
> >  	return __RTE_BITSET_DELEGATE(rte_bit_test, bitset, bit_num);
> > +#else
> > +	RTE_SET_USED(bitset);
> > +	RTE_SET_USED(bit_num);
> > +	return false;
> > +#endif
> >  }
> 
> This looks wrong! The API is exposed, but does nothing.

Yes, this is what we did already in the past for other experimental functions
called from inline functions.
There is no choice, the compiler is hitting the warning.

> It is possible to build without ALLOW_EXPERIMENTAL_API; the compiler will emit warnings when using experimental APIs.

Yes it is possible to build,
but it is not said it should work the same.

> If those compiler warnings are not handled as errors, the compiled application will be full of bugs.

Yes, that's why there are warnings.
We may document it better but that's the behavior we have for years.
There is no easy solution, and making experimental functions work
without defining ALLOW_EXPERIMENTAL_API is not a really interesting goal.
I think the word "allow" suggests it is not supposed to work if not allowed.

It would be more interesting to make sure the users understand
why we have this flag and how to enable it.
I propose adding some docs, and mentioning ALLOW_EXPERIMENTAL_API
in the the __rte_experimental message in rte_compat.h.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* RE: [PATCH 2/3] bitset: fix build for GCC without experimental API
  2024-10-15 14:13     ` Thomas Monjalon
@ 2024-10-15 14:44       ` Morten Brørup
  2024-10-15 19:58         ` Thomas Monjalon
  0 siblings, 1 reply; 34+ messages in thread
From: Morten Brørup @ 2024-10-15 14:44 UTC (permalink / raw)
  To: Thomas Monjalon, David Marchand
  Cc: dev, bruce.richardson, ktraynor, Mattias Rönnblom, Tyler Retzlaff

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Tuesday, 15 October 2024 16.13
> 
> 15/10/2024 14:53, Morten Brørup:
> > > From: David Marchand [mailto:david.marchand@redhat.com]
> > > @@ -255,7 +255,13 @@ __rte_experimental
> > >  static inline bool
> > >  rte_bitset_test(const uint64_t *bitset, size_t bit_num)
> > >  {
> > > +#ifdef ALLOW_EXPERIMENTAL_API
> > >  	return __RTE_BITSET_DELEGATE(rte_bit_test, bitset, bit_num);
> > > +#else
> > > +	RTE_SET_USED(bitset);
> > > +	RTE_SET_USED(bit_num);
> > > +	return false;
> > > +#endif
> > >  }
> >
> > This looks wrong! The API is exposed, but does nothing.
> 
> Yes, this is what we did already in the past for other experimental
> functions
> called from inline functions.
> There is no choice, the compiler is hitting the warning.
> 
> > It is possible to build without ALLOW_EXPERIMENTAL_API; the compiler
> will emit warnings when using experimental APIs.
> 
> Yes it is possible to build,
> but it is not said it should work the same.
> 
> > If those compiler warnings are not handled as errors, the compiled
> application will be full of bugs.
> 
> Yes, that's why there are warnings.
> We may document it better but that's the behavior we have for years.
> There is no easy solution, and making experimental functions work
> without defining ALLOW_EXPERIMENTAL_API is not a really interesting
> goal.
> I think the word "allow" suggests it is not supposed to work if not
> allowed.

There's a world of difference between "experimental, might have bugs" - which is what I (and possibly other DPDK consumers) expect - and "experimental, we know for a fact that it doesn't work" - which is quite a surprise to me.
> 
> It would be more interesting to make sure the users understand
> why we have this flag and how to enable it.
> I propose adding some docs, and mentioning ALLOW_EXPERIMENTAL_API
> in the the __rte_experimental message in rte_compat.h.
> 

If we know that some of these warnings cause bugs in DPDK, we should elevate these specific instances to error level.

Regarding this specific patch:
Would it be possible to change it to behave like patch 1/3, i.e. completely omit the experimental APIs if ALLOW_EXPERIMENTAL_API is not defined?


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 2/3] bitset: fix build for GCC without experimental API
  2024-10-15 14:44       ` Morten Brørup
@ 2024-10-15 19:58         ` Thomas Monjalon
  2024-10-15 20:30           ` Morten Brørup
  0 siblings, 1 reply; 34+ messages in thread
From: Thomas Monjalon @ 2024-10-15 19:58 UTC (permalink / raw)
  To: Morten Brørup
  Cc: David Marchand, dev, bruce.richardson, ktraynor,
	Mattias Rönnblom, Tyler Retzlaff

15/10/2024 16:44, Morten Brørup:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Tuesday, 15 October 2024 16.13
> > 
> > 15/10/2024 14:53, Morten Brørup:
> > > > From: David Marchand [mailto:david.marchand@redhat.com]
> > > > @@ -255,7 +255,13 @@ __rte_experimental
> > > >  static inline bool
> > > >  rte_bitset_test(const uint64_t *bitset, size_t bit_num)
> > > >  {
> > > > +#ifdef ALLOW_EXPERIMENTAL_API
> > > >  	return __RTE_BITSET_DELEGATE(rte_bit_test, bitset, bit_num);
> > > > +#else
> > > > +	RTE_SET_USED(bitset);
> > > > +	RTE_SET_USED(bit_num);
> > > > +	return false;
> > > > +#endif
> > > >  }
> > >
> > > This looks wrong! The API is exposed, but does nothing.
> > 
> > Yes, this is what we did already in the past for other experimental
> > functions
> > called from inline functions.
> > There is no choice, the compiler is hitting the warning.
> > 
> > > It is possible to build without ALLOW_EXPERIMENTAL_API; the compiler
> > will emit warnings when using experimental APIs.
> > 
> > Yes it is possible to build,
> > but it is not said it should work the same.
> > 
> > > If those compiler warnings are not handled as errors, the compiled
> > application will be full of bugs.
> > 
> > Yes, that's why there are warnings.
> > We may document it better but that's the behavior we have for years.
> > There is no easy solution, and making experimental functions work
> > without defining ALLOW_EXPERIMENTAL_API is not a really interesting
> > goal.
> > I think the word "allow" suggests it is not supposed to work if not
> > allowed.
> 
> There's a world of difference between "experimental, might have bugs" - which is what I (and possibly other DPDK consumers) expect - and "experimental, we know for a fact that it doesn't work" - which is quite a surprise to me.

It does not work if you don't enable it,
and there is a warning when compiling.


> > It would be more interesting to make sure the users understand
> > why we have this flag and how to enable it.
> > I propose adding some docs, and mentioning ALLOW_EXPERIMENTAL_API
> > in the the __rte_experimental message in rte_compat.h.
> > 
> 
> If we know that some of these warnings cause bugs in DPDK, we should elevate these specific instances to error level.

Technically I don't know whether it is possible.
Look at rte_compat.h


> Regarding this specific patch:
> Would it be possible to change it to behave like patch 1/3, i.e. completely omit the experimental APIs if ALLOW_EXPERIMENTAL_API is not defined?

I disagree, it would be a lot of churn in the code to disable all experimental API calls.



^ permalink raw reply	[flat|nested] 34+ messages in thread

* RE: [PATCH 2/3] bitset: fix build for GCC without experimental API
  2024-10-15 19:58         ` Thomas Monjalon
@ 2024-10-15 20:30           ` Morten Brørup
  0 siblings, 0 replies; 34+ messages in thread
From: Morten Brørup @ 2024-10-15 20:30 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: David Marchand, dev, bruce.richardson, ktraynor,
	Mattias Rönnblom, Tyler Retzlaff

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Tuesday, 15 October 2024 21.58
> 
> 15/10/2024 16:44, Morten Brørup:
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > Sent: Tuesday, 15 October 2024 16.13
> > >
> > > 15/10/2024 14:53, Morten Brørup:
> > > > > From: David Marchand [mailto:david.marchand@redhat.com]
> > > > > @@ -255,7 +255,13 @@ __rte_experimental
> > > > >  static inline bool
> > > > >  rte_bitset_test(const uint64_t *bitset, size_t bit_num)
> > > > >  {
> > > > > +#ifdef ALLOW_EXPERIMENTAL_API
> > > > >  	return __RTE_BITSET_DELEGATE(rte_bit_test, bitset,
> bit_num);
> > > > > +#else
> > > > > +	RTE_SET_USED(bitset);
> > > > > +	RTE_SET_USED(bit_num);
> > > > > +	return false;
> > > > > +#endif
> > > > >  }
> > > >
> > > > This looks wrong! The API is exposed, but does nothing.
> > >
> > > Yes, this is what we did already in the past for other experimental
> > > functions
> > > called from inline functions.
> > > There is no choice, the compiler is hitting the warning.
> > >
> > > > It is possible to build without ALLOW_EXPERIMENTAL_API; the
> compiler
> > > will emit warnings when using experimental APIs.
> > >
> > > Yes it is possible to build,
> > > but it is not said it should work the same.
> > >
> > > > If those compiler warnings are not handled as errors, the
> compiled
> > > application will be full of bugs.
> > >
> > > Yes, that's why there are warnings.
> > > We may document it better but that's the behavior we have for
> years.
> > > There is no easy solution, and making experimental functions work
> > > without defining ALLOW_EXPERIMENTAL_API is not a really interesting
> > > goal.
> > > I think the word "allow" suggests it is not supposed to work if not
> > > allowed.
> >
> > There's a world of difference between "experimental, might have bugs"
> - which is what I (and possibly other DPDK consumers) expect - and
> "experimental, we know for a fact that it doesn't work" - which is
> quite a surprise to me.
> 
> It does not work if you don't enable it,
> and there is a warning when compiling.

My issue with this is:
The warning only says that the API is deprecated. It doesn't say that the underlying implementation might be completely missing.

> 
> 
> > > It would be more interesting to make sure the users understand
> > > why we have this flag and how to enable it.
> > > I propose adding some docs, and mentioning ALLOW_EXPERIMENTAL_API
> > > in the the __rte_experimental message in rte_compat.h.
> > >
> >
> > If we know that some of these warnings cause bugs in DPDK, we should
> elevate these specific instances to error level.
> 
> Technically I don't know whether it is possible.
> Look at rte_compat.h
> 
> 
> > Regarding this specific patch:
> > Would it be possible to change it to behave like patch 1/3, i.e.
> completely omit the experimental APIs if ALLOW_EXPERIMENTAL_API is not
> defined?
> 
> I disagree, it would be a lot of churn in the code to disable all
> experimental API calls.

OK.
Apparently the issue is not specific to this patch, so it would be unreasonable to require this patch to handle it differently than it has been handled until now.

Acked-by: Morten Brørup <mb@smartsharesystems.com>


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH 3/3] buildtools/chkincs: check headers with stable API only
  2024-10-15 12:10 [PATCH 0/3] Enhance headers check David Marchand
  2024-10-15 12:10 ` [PATCH 1/3] bitops: fix build for GCC without experimental API David Marchand
  2024-10-15 12:10 ` [PATCH 2/3] bitset: " David Marchand
@ 2024-10-15 12:10 ` David Marchand
  2024-10-16 11:38 ` [PATCH v2 0/4] Enhance headers check David Marchand
  2024-10-21 16:04 ` [PATCH v3 " David Marchand
  4 siblings, 0 replies; 34+ messages in thread
From: David Marchand @ 2024-10-15 12:10 UTC (permalink / raw)
  To: dev; +Cc: thomas, bruce.richardson, ktraynor

An exported header should be usable w/ and w/o ALLOW_EXPERIMENTAL_API so
that an application that only wants stable API may include it.

Plus, the widely common case is that an application will not use
internal API.

Cover those cases but keep the original test.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 buildtools/chkincs/meson.build | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/buildtools/chkincs/meson.build b/buildtools/chkincs/meson.build
index 8da221fc33..787d70272b 100644
--- a/buildtools/chkincs/meson.build
+++ b/buildtools/chkincs/meson.build
@@ -12,8 +12,6 @@ gen_c_files = generator(gen_c_file_for_header,
         arguments: ['@INPUT@', '@OUTPUT@'])
 
 cflags = machine_args
-cflags += '-DALLOW_EXPERIMENTAL_API'
-cflags += '-DALLOW_INTERNAL_API'
 
 sources = files('main.c')
 sources += gen_c_files.process(dpdk_chkinc_headers)
@@ -32,6 +30,18 @@ executable('chkincs', sources,
         dependencies: deps,
         install: false)
 
+executable('chkincs-exp', sources,
+        c_args: [cflags, '-DALLOW_EXPERIMENTAL_API'],
+        include_directories: includes,
+        dependencies: deps,
+        install: false)
+
+executable('chkincs-all', sources,
+        c_args: [cflags, '-DALLOW_EXPERIMENTAL_API', '-DALLOW_INTERNAL_API'],
+        include_directories: includes,
+        dependencies: deps,
+        install: false)
+
 # run tests for c++ builds also
 if not add_languages('cpp', required: false)
     subdir_done()
@@ -49,3 +59,16 @@ executable('chkincs-cpp', cpp_sources,
         include_directories: includes,
         dependencies: deps,
         install: false)
+
+executable('chkincs-cpp-exp', cpp_sources,
+        cpp_args: ['-include', 'rte_config.h', cflags, '-DALLOW_EXPERIMENTAL_API'],
+        include_directories: includes,
+        dependencies: deps,
+        install: false)
+
+executable('chkincs-cpp-all', cpp_sources,
+        cpp_args: ['-include', 'rte_config.h', cflags, '-DALLOW_EXPERIMENTAL_API',
+                   '-DALLOW_INTERNAL_API'],
+        include_directories: includes,
+        dependencies: deps,
+        install: false)
-- 
2.46.2


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v2 0/4] Enhance headers check
  2024-10-15 12:10 [PATCH 0/3] Enhance headers check David Marchand
                   ` (2 preceding siblings ...)
  2024-10-15 12:10 ` [PATCH 3/3] buildtools/chkincs: check headers with stable API only David Marchand
@ 2024-10-16 11:38 ` David Marchand
  2024-10-16 11:38   ` [PATCH v2 1/4] bitops: fix build for GCC without experimental API David Marchand
                     ` (4 more replies)
  2024-10-21 16:04 ` [PATCH v3 " David Marchand
  4 siblings, 5 replies; 34+ messages in thread
From: David Marchand @ 2024-10-16 11:38 UTC (permalink / raw)
  To: dev; +Cc: thomas, bruce.richardson, ktraynor

We currently check that exported headers are fine with
-DALLOW_EXPERIMENTAL_API and -DALLOW_INTERNAL_API.

Such a check won't catch issues like the one fixed in patch 1, where OVS
compilation is broken by the additional of experimental API in a header
commonly included in other parts of DPDK.

Ideally, I would like to merge patch 1 in rc1.
The patch 2 is not a real issue (more like the enhanced check would
complain about this header).
In any case, I think it would not hurt merging everything now.

Comments, please.


-- 
David Marchand

Changes since v1:
- fixed additional issue in vDPA header, raised by CI,

David Marchand (4):
  bitops: fix build for GCC without experimental API
  bitset: fix build for GCC without experimental API
  vhost: remove internal vDPA API description from public header
  buildtools/chkincs: check headers with stable API only

 buildtools/chkincs/meson.build |  27 +++++++-
 lib/eal/include/rte_bitops.h   |   6 ++
 lib/eal/include/rte_bitset.h   | 123 +++++++++++++++++++++++++++++++++
 lib/vhost/rte_vdpa.h           |  17 -----
 4 files changed, 154 insertions(+), 19 deletions(-)

-- 
2.46.2


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v2 1/4] bitops: fix build for GCC without experimental API
  2024-10-16 11:38 ` [PATCH v2 0/4] Enhance headers check David Marchand
@ 2024-10-16 11:38   ` David Marchand
  2024-10-16 11:38   ` [PATCH v2 2/4] bitset: " David Marchand
                     ` (3 subsequent siblings)
  4 siblings, 0 replies; 34+ messages in thread
From: David Marchand @ 2024-10-16 11:38 UTC (permalink / raw)
  To: dev
  Cc: thomas, bruce.richardson, ktraynor, Morten Brørup,
	Jack Bond-Preston, Tyler Retzlaff, Mattias Rönnblom

Building OVS against current DPDK fails with following warnings:

In file included from .../ovs/dpdk-dir/include/rte_memory.h:18,
                 from .../ovs/dpdk-dir/include/rte_ring_core.h:29,
                 from .../ovs/dpdk-dir/include/rte_ring.h:37,
                 from .../ovs/dpdk-dir/include/rte_mempool.h:49,
                 from .../ovs/dpdk-dir/include/rte_mbuf.h:38,
                 from lib/dp-packet.h:25,
                 from lib/ofp-packet.c:20:
.../ovs/dpdk-dir/include/rte_bitops.h: In function ‘__rte_bit_assign32’:
.../ovs/dpdk-dir/include/rte_bitops.h:528:1: error:
	‘__rte_bit_set32’ is deprecated: Symbol is not yet part of
	stable ABI [-Werror=deprecated-declarations]
...

This comes from the fact that some (experimental) inline helpers
are calling other experimental API.
Hide those calls.

Fixes: 471de107ae23 ("bitops: add new bit manipulation API")

Reported-by: Kevin Traynor <ktraynor@redhat.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/eal/include/rte_bitops.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h
index e08e41199a..deb1fd43f2 100644
--- a/lib/eal/include/rte_bitops.h
+++ b/lib/eal/include/rte_bitops.h
@@ -525,8 +525,10 @@ __rte_bit_ ## variant ## flip ## size(qualifier uint ## size ## _t *addr, unsign
 	__RTE_GEN_BIT_OPS(,, size) \
 	__RTE_GEN_BIT_OPS(v_, volatile, size)
 
+#ifdef ALLOW_EXPERIMENTAL_API
 __RTE_GEN_BIT_OPS_SIZE(32)
 __RTE_GEN_BIT_OPS_SIZE(64)
+#endif
 
 #define __RTE_GEN_BIT_ATOMIC_TEST(variant, qualifier, size) \
 __rte_experimental \
@@ -651,8 +653,10 @@ __rte_bit_atomic_ ## variant ## test_and_assign ## size( \
 	__RTE_GEN_BIT_ATOMIC_OPS(,, size) \
 	__RTE_GEN_BIT_ATOMIC_OPS(v_, volatile, size)
 
+#ifdef ALLOW_EXPERIMENTAL_API
 __RTE_GEN_BIT_ATOMIC_OPS_SIZE(32)
 __RTE_GEN_BIT_ATOMIC_OPS_SIZE(64)
+#endif
 
 /*------------------------ 32-bit relaxed operations ------------------------*/
 
@@ -1481,6 +1485,7 @@ rte_bit_ ## family ## fun(qualifier uint ## size ## _t *addr, arg1_type arg1_nam
 	__RTE_BIT_OVERLOAD_SZ_4R(family, fun, qualifier, 64, ret_type, arg1_type, arg1_name, \
 		arg2_type, arg2_name, arg3_type, arg3_name)
 
+#ifdef ALLOW_EXPERIMENTAL_API
 __RTE_BIT_OVERLOAD_2R(, test, const, bool, unsigned int, nr)
 __RTE_BIT_OVERLOAD_2(, set,, unsigned int, nr)
 __RTE_BIT_OVERLOAD_2(, clear,, unsigned int, nr)
@@ -1496,6 +1501,7 @@ __RTE_BIT_OVERLOAD_3R(atomic_, test_and_set,, bool, unsigned int, nr, int, memor
 __RTE_BIT_OVERLOAD_3R(atomic_, test_and_clear,, bool, unsigned int, nr, int, memory_order)
 __RTE_BIT_OVERLOAD_4R(atomic_, test_and_assign,, bool, unsigned int, nr, bool, value,
 	int, memory_order)
+#endif
 
 #endif
 
-- 
2.46.2


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v2 2/4] bitset: fix build for GCC without experimental API
  2024-10-16 11:38 ` [PATCH v2 0/4] Enhance headers check David Marchand
  2024-10-16 11:38   ` [PATCH v2 1/4] bitops: fix build for GCC without experimental API David Marchand
@ 2024-10-16 11:38   ` David Marchand
  2024-10-16 14:14     ` Mattias Rönnblom
  2024-10-16 11:38   ` [PATCH v2 3/4] vhost: remove internal vDPA API description from public header David Marchand
                     ` (2 subsequent siblings)
  4 siblings, 1 reply; 34+ messages in thread
From: David Marchand @ 2024-10-16 11:38 UTC (permalink / raw)
  To: dev
  Cc: thomas, bruce.richardson, ktraynor, Morten Brørup,
	Mattias Rönnblom, Tyler Retzlaff

For a reason similar to the change on bitops header, hide bitset
implementation relying on experimental API.

Fixes: 99a1197647d8 ("eal: add bitset type")

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/eal/include/rte_bitset.h | 123 +++++++++++++++++++++++++++++++++++
 1 file changed, 123 insertions(+)

diff --git a/lib/eal/include/rte_bitset.h b/lib/eal/include/rte_bitset.h
index 74c643a72a..8ae8425fc2 100644
--- a/lib/eal/include/rte_bitset.h
+++ b/lib/eal/include/rte_bitset.h
@@ -255,7 +255,13 @@ __rte_experimental
 static inline bool
 rte_bitset_test(const uint64_t *bitset, size_t bit_num)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return __RTE_BITSET_DELEGATE(rte_bit_test, bitset, bit_num);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+	return false;
+#endif
 }
 
 /**
@@ -277,7 +283,12 @@ __rte_experimental
 static inline void
 rte_bitset_set(uint64_t *bitset, size_t bit_num)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	__RTE_BITSET_DELEGATE(rte_bit_set, bitset, bit_num);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+#endif
 }
 
 /**
@@ -299,7 +310,12 @@ __rte_experimental
 static inline void
 rte_bitset_clear(uint64_t *bitset, size_t bit_num)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	__RTE_BITSET_DELEGATE(rte_bit_clear, bitset, bit_num);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+#endif
 }
 
 /**
@@ -323,7 +339,13 @@ __rte_experimental
 static inline void
 rte_bitset_assign(uint64_t *bitset, size_t bit_num, bool bit_value)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	__RTE_BITSET_DELEGATE_N(rte_bit_assign, bitset, bit_num, bit_value);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+	RTE_SET_USED(bit_value);
+#endif
 }
 
 /**
@@ -345,7 +367,12 @@ __rte_experimental
 static inline void
 rte_bitset_flip(uint64_t *bitset, size_t bit_num)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	__RTE_BITSET_DELEGATE(rte_bit_flip, bitset, bit_num);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+#endif
 }
 
 /**
@@ -370,7 +397,14 @@ __rte_experimental
 static inline bool
 rte_bitset_atomic_test(const uint64_t *bitset, size_t bit_num, int memory_order)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return __RTE_BITSET_DELEGATE_N(rte_bit_atomic_test, bitset, bit_num, memory_order);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+	RTE_SET_USED(memory_order);
+	return false;
+#endif
 }
 
 /**
@@ -399,7 +433,13 @@ __rte_experimental
 static inline void
 rte_bitset_atomic_set(uint64_t *bitset, size_t bit_num, int memory_order)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	__RTE_BITSET_DELEGATE_N(rte_bit_atomic_set, bitset, bit_num, memory_order);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+	RTE_SET_USED(memory_order);
+#endif
 }
 
 /**
@@ -428,7 +468,13 @@ __rte_experimental
 static inline void
 rte_bitset_atomic_clear(uint64_t *bitset, size_t bit_num, int memory_order)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	__RTE_BITSET_DELEGATE_N(rte_bit_atomic_clear, bitset, bit_num, memory_order);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+	RTE_SET_USED(memory_order);
+#endif
 }
 
 /**
@@ -459,7 +505,14 @@ __rte_experimental
 static inline void
 rte_bitset_atomic_assign(uint64_t *bitset, size_t bit_num, bool bit_value, int memory_order)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	__RTE_BITSET_DELEGATE_N(rte_bit_atomic_assign, bitset, bit_num, bit_value, memory_order);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+	RTE_SET_USED(bit_value);
+	RTE_SET_USED(memory_order);
+#endif
 }
 
 /**
@@ -488,7 +541,13 @@ __rte_experimental
 static inline void
 rte_bitset_atomic_flip(uint64_t *bitset, size_t bit_num, int memory_order)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	__RTE_BITSET_DELEGATE_N(rte_bit_atomic_flip, bitset, bit_num, memory_order);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+	RTE_SET_USED(memory_order);
+#endif
 }
 
 /**
@@ -524,7 +583,12 @@ __rte_experimental
 static inline void
 rte_bitset_clear_all(uint64_t *bitset, size_t size)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	rte_bitset_init(bitset, size);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+#endif
 }
 
 /**
@@ -576,7 +640,13 @@ __rte_experimental
 static inline size_t
 rte_bitset_count_clear(const uint64_t *bitset, size_t size)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return size - rte_bitset_count_set(bitset, size);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+	return 0;
+#endif
 }
 
 #define __RTE_BITSET_FIND_FLAG_FIND_CLEAR (1U << 0)
@@ -638,6 +708,7 @@ static inline ssize_t
 __rte_bitset_find(const uint64_t *bitset, size_t size, size_t start_bit, size_t len,
 		unsigned int flags)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	bool find_clear = flags & __RTE_BITSET_FIND_FLAG_FIND_CLEAR;
 	bool may_wrap = flags & __RTE_BITSET_FIND_FLAG_WRAP;
 	bool does_wrap = (start_bit + len) > size;
@@ -658,6 +729,14 @@ __rte_bitset_find(const uint64_t *bitset, size_t size, size_t start_bit, size_t
 		rc = __rte_bitset_find_nowrap(bitset, size, start_bit, len, find_clear);
 
 	return rc;
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+	RTE_SET_USED(start_bit);
+	RTE_SET_USED(len);
+	RTE_SET_USED(flags);
+	return 0;
+#endif
 }
 
 /**
@@ -681,7 +760,13 @@ __rte_experimental
 static inline ssize_t
 rte_bitset_find_first_set(const uint64_t *bitset, size_t size)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return __rte_bitset_find(bitset, size, 0, size, 0);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+	return 0;
+#endif
 }
 
 /**
@@ -711,7 +796,15 @@ __rte_experimental
 static inline ssize_t
 rte_bitset_find_set(const uint64_t *bitset, size_t size, size_t start_bit, size_t len)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return __rte_bitset_find(bitset, size, start_bit, len, 0);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+	RTE_SET_USED(start_bit);
+	RTE_SET_USED(len);
+	return 0;
+#endif
 }
 
 /**
@@ -742,7 +835,15 @@ __rte_experimental
 static inline ssize_t
 rte_bitset_find_set_wrap(const uint64_t *bitset, size_t size, size_t start_bit, size_t len)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return __rte_bitset_find(bitset, size, start_bit, len, __RTE_BITSET_FIND_FLAG_WRAP);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+	RTE_SET_USED(start_bit);
+	RTE_SET_USED(len);
+	return 0;
+#endif
 }
 
 /**
@@ -766,7 +867,13 @@ __rte_experimental
 static inline ssize_t
 rte_bitset_find_first_clear(const uint64_t *bitset, size_t size)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return __rte_bitset_find(bitset, size, 0, size, __RTE_BITSET_FIND_FLAG_FIND_CLEAR);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+	return 0;
+#endif
 }
 
 /**
@@ -796,7 +903,15 @@ __rte_experimental
 static inline ssize_t
 rte_bitset_find_clear(const uint64_t *bitset, size_t size, size_t start_bit, size_t len)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return __rte_bitset_find(bitset, size, start_bit, len, __RTE_BITSET_FIND_FLAG_FIND_CLEAR);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+	RTE_SET_USED(start_bit);
+	RTE_SET_USED(len);
+	return 0;
+#endif
 }
 
 /**
@@ -827,8 +942,16 @@ __rte_experimental
 static inline ssize_t
 rte_bitset_find_clear_wrap(const uint64_t *bitset, size_t size, size_t start_bit, size_t len)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return __rte_bitset_find(bitset, size, start_bit, len,
 		__RTE_BITSET_FIND_FLAG_FIND_CLEAR | __RTE_BITSET_FIND_FLAG_WRAP);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+	RTE_SET_USED(start_bit);
+	RTE_SET_USED(len);
+	return 0;
+#endif
 }
 
 /**
-- 
2.46.2


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 2/4] bitset: fix build for GCC without experimental API
  2024-10-16 11:38   ` [PATCH v2 2/4] bitset: " David Marchand
@ 2024-10-16 14:14     ` Mattias Rönnblom
  2024-10-16 15:36       ` David Marchand
  0 siblings, 1 reply; 34+ messages in thread
From: Mattias Rönnblom @ 2024-10-16 14:14 UTC (permalink / raw)
  To: David Marchand, dev
  Cc: thomas, bruce.richardson, ktraynor, Morten Brørup,
	Mattias Rönnblom, Tyler Retzlaff

On 2024-10-16 13:38, David Marchand wrote:
> For a reason similar to the change on bitops header, hide bitset
> implementation relying on experimental API.
> 
> Fixes: 99a1197647d8 ("eal: add bitset type")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> ---
>   lib/eal/include/rte_bitset.h | 123 +++++++++++++++++++++++++++++++++++
>   1 file changed, 123 insertions(+)
> 
> diff --git a/lib/eal/include/rte_bitset.h b/lib/eal/include/rte_bitset.h
> index 74c643a72a..8ae8425fc2 100644
> --- a/lib/eal/include/rte_bitset.h
> +++ b/lib/eal/include/rte_bitset.h
> @@ -255,7 +255,13 @@ __rte_experimental
>   static inline bool
>   rte_bitset_test(const uint64_t *bitset, size_t bit_num)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	return __RTE_BITSET_DELEGATE(rte_bit_test, bitset, bit_num);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(bit_num);
> +	return false;

This is no RTE_VERIFY(0) here, because this is just dummy code, that 
will never be run. Is that correct?

> +#endif
>   }
>   
>   /**
> @@ -277,7 +283,12 @@ __rte_experimental
>   static inline void
>   rte_bitset_set(uint64_t *bitset, size_t bit_num)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	__RTE_BITSET_DELEGATE(rte_bit_set, bitset, bit_num);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(bit_num);
> +#endif
>   }
>   
>   /**
> @@ -299,7 +310,12 @@ __rte_experimental
>   static inline void
>   rte_bitset_clear(uint64_t *bitset, size_t bit_num)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	__RTE_BITSET_DELEGATE(rte_bit_clear, bitset, bit_num);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(bit_num);
> +#endif
>   }
>   
>   /**
> @@ -323,7 +339,13 @@ __rte_experimental
>   static inline void
>   rte_bitset_assign(uint64_t *bitset, size_t bit_num, bool bit_value)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	__RTE_BITSET_DELEGATE_N(rte_bit_assign, bitset, bit_num, bit_value);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(bit_num);
> +	RTE_SET_USED(bit_value);
> +#endif
>   }
>   
>   /**
> @@ -345,7 +367,12 @@ __rte_experimental
>   static inline void
>   rte_bitset_flip(uint64_t *bitset, size_t bit_num)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	__RTE_BITSET_DELEGATE(rte_bit_flip, bitset, bit_num);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(bit_num);
> +#endif
>   }
>   
>   /**
> @@ -370,7 +397,14 @@ __rte_experimental
>   static inline bool
>   rte_bitset_atomic_test(const uint64_t *bitset, size_t bit_num, int memory_order)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	return __RTE_BITSET_DELEGATE_N(rte_bit_atomic_test, bitset, bit_num, memory_order);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(bit_num);
> +	RTE_SET_USED(memory_order);
> +	return false;
> +#endif
>   }
>   
>   /**
> @@ -399,7 +433,13 @@ __rte_experimental
>   static inline void
>   rte_bitset_atomic_set(uint64_t *bitset, size_t bit_num, int memory_order)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	__RTE_BITSET_DELEGATE_N(rte_bit_atomic_set, bitset, bit_num, memory_order);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(bit_num);
> +	RTE_SET_USED(memory_order);
> +#endif
>   }
>   
>   /**
> @@ -428,7 +468,13 @@ __rte_experimental
>   static inline void
>   rte_bitset_atomic_clear(uint64_t *bitset, size_t bit_num, int memory_order)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	__RTE_BITSET_DELEGATE_N(rte_bit_atomic_clear, bitset, bit_num, memory_order);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(bit_num);
> +	RTE_SET_USED(memory_order);
> +#endif
>   }
>   
>   /**
> @@ -459,7 +505,14 @@ __rte_experimental
>   static inline void
>   rte_bitset_atomic_assign(uint64_t *bitset, size_t bit_num, bool bit_value, int memory_order)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	__RTE_BITSET_DELEGATE_N(rte_bit_atomic_assign, bitset, bit_num, bit_value, memory_order);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(bit_num);
> +	RTE_SET_USED(bit_value);
> +	RTE_SET_USED(memory_order);
> +#endif
>   }
>   
>   /**
> @@ -488,7 +541,13 @@ __rte_experimental
>   static inline void
>   rte_bitset_atomic_flip(uint64_t *bitset, size_t bit_num, int memory_order)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	__RTE_BITSET_DELEGATE_N(rte_bit_atomic_flip, bitset, bit_num, memory_order);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(bit_num);
> +	RTE_SET_USED(memory_order);
> +#endif
>   }
>   
>   /**
> @@ -524,7 +583,12 @@ __rte_experimental
>   static inline void
>   rte_bitset_clear_all(uint64_t *bitset, size_t size)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	rte_bitset_init(bitset, size);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(size);
> +#endif
>   }
>   
>   /**
> @@ -576,7 +640,13 @@ __rte_experimental
>   static inline size_t
>   rte_bitset_count_clear(const uint64_t *bitset, size_t size)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	return size - rte_bitset_count_set(bitset, size);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(size);
> +	return 0;
> +#endif
>   }
>   
>   #define __RTE_BITSET_FIND_FLAG_FIND_CLEAR (1U << 0)
> @@ -638,6 +708,7 @@ static inline ssize_t
>   __rte_bitset_find(const uint64_t *bitset, size_t size, size_t start_bit, size_t len,
>   		unsigned int flags)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	bool find_clear = flags & __RTE_BITSET_FIND_FLAG_FIND_CLEAR;
>   	bool may_wrap = flags & __RTE_BITSET_FIND_FLAG_WRAP;
>   	bool does_wrap = (start_bit + len) > size;
> @@ -658,6 +729,14 @@ __rte_bitset_find(const uint64_t *bitset, size_t size, size_t start_bit, size_t
>   		rc = __rte_bitset_find_nowrap(bitset, size, start_bit, len, find_clear);
>   
>   	return rc;
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(size);
> +	RTE_SET_USED(start_bit);
> +	RTE_SET_USED(len);
> +	RTE_SET_USED(flags);
> +	return 0;
> +#endif
>   }
>   
>   /**
> @@ -681,7 +760,13 @@ __rte_experimental
>   static inline ssize_t
>   rte_bitset_find_first_set(const uint64_t *bitset, size_t size)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	return __rte_bitset_find(bitset, size, 0, size, 0);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(size);
> +	return 0;
> +#endif
>   }
>   
>   /**
> @@ -711,7 +796,15 @@ __rte_experimental
>   static inline ssize_t
>   rte_bitset_find_set(const uint64_t *bitset, size_t size, size_t start_bit, size_t len)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	return __rte_bitset_find(bitset, size, start_bit, len, 0);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(size);
> +	RTE_SET_USED(start_bit);
> +	RTE_SET_USED(len);
> +	return 0;
> +#endif
>   }
>   
>   /**
> @@ -742,7 +835,15 @@ __rte_experimental
>   static inline ssize_t
>   rte_bitset_find_set_wrap(const uint64_t *bitset, size_t size, size_t start_bit, size_t len)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	return __rte_bitset_find(bitset, size, start_bit, len, __RTE_BITSET_FIND_FLAG_WRAP);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(size);
> +	RTE_SET_USED(start_bit);
> +	RTE_SET_USED(len);
> +	return 0;
> +#endif
>   }
>   
>   /**
> @@ -766,7 +867,13 @@ __rte_experimental
>   static inline ssize_t
>   rte_bitset_find_first_clear(const uint64_t *bitset, size_t size)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	return __rte_bitset_find(bitset, size, 0, size, __RTE_BITSET_FIND_FLAG_FIND_CLEAR);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(size);
> +	return 0;
> +#endif
>   }
>   
>   /**
> @@ -796,7 +903,15 @@ __rte_experimental
>   static inline ssize_t
>   rte_bitset_find_clear(const uint64_t *bitset, size_t size, size_t start_bit, size_t len)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	return __rte_bitset_find(bitset, size, start_bit, len, __RTE_BITSET_FIND_FLAG_FIND_CLEAR);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(size);
> +	RTE_SET_USED(start_bit);
> +	RTE_SET_USED(len);
> +	return 0;
> +#endif
>   }
>   
>   /**
> @@ -827,8 +942,16 @@ __rte_experimental
>   static inline ssize_t
>   rte_bitset_find_clear_wrap(const uint64_t *bitset, size_t size, size_t start_bit, size_t len)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	return __rte_bitset_find(bitset, size, start_bit, len,
>   		__RTE_BITSET_FIND_FLAG_FIND_CLEAR | __RTE_BITSET_FIND_FLAG_WRAP);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(size);
> +	RTE_SET_USED(start_bit);
> +	RTE_SET_USED(len);
> +	return 0;
> +#endif
>   }
>   
>   /**


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 2/4] bitset: fix build for GCC without experimental API
  2024-10-16 14:14     ` Mattias Rönnblom
@ 2024-10-16 15:36       ` David Marchand
  2024-10-16 15:42         ` Morten Brørup
  2024-10-16 15:51         ` Mattias Rönnblom
  0 siblings, 2 replies; 34+ messages in thread
From: David Marchand @ 2024-10-16 15:36 UTC (permalink / raw)
  To: Mattias Rönnblom
  Cc: dev, thomas, bruce.richardson, ktraynor, Morten Brørup,
	Mattias Rönnblom, Tyler Retzlaff

On Wed, Oct 16, 2024 at 4:14 PM Mattias Rönnblom <hofors@lysator.liu.se> wrote:
>
> On 2024-10-16 13:38, David Marchand wrote:
> > For a reason similar to the change on bitops header, hide bitset
> > implementation relying on experimental API.
> >
> > Fixes: 99a1197647d8 ("eal: add bitset type")
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> >   lib/eal/include/rte_bitset.h | 123 +++++++++++++++++++++++++++++++++++
> >   1 file changed, 123 insertions(+)
> >
> > diff --git a/lib/eal/include/rte_bitset.h b/lib/eal/include/rte_bitset.h
> > index 74c643a72a..8ae8425fc2 100644
> > --- a/lib/eal/include/rte_bitset.h
> > +++ b/lib/eal/include/rte_bitset.h
> > @@ -255,7 +255,13 @@ __rte_experimental
> >   static inline bool
> >   rte_bitset_test(const uint64_t *bitset, size_t bit_num)
> >   {
> > +#ifdef ALLOW_EXPERIMENTAL_API
> >       return __RTE_BITSET_DELEGATE(rte_bit_test, bitset, bit_num);
> > +#else
> > +     RTE_SET_USED(bitset);
> > +     RTE_SET_USED(bit_num);
> > +     return false;
>
> This is no RTE_VERIFY(0) here, because this is just dummy code, that
> will never be run. Is that correct?

Adding a RTE_VERIFY(false) is an interesting idea.
It is not supposed to be run, indeed.

Do you prefer I respin with this?


-- 
David Marchand


^ permalink raw reply	[flat|nested] 34+ messages in thread

* RE: [PATCH v2 2/4] bitset: fix build for GCC without experimental API
  2024-10-16 15:36       ` David Marchand
@ 2024-10-16 15:42         ` Morten Brørup
  2024-10-16 16:03           ` Mattias Rönnblom
  2024-10-16 15:51         ` Mattias Rönnblom
  1 sibling, 1 reply; 34+ messages in thread
From: Morten Brørup @ 2024-10-16 15:42 UTC (permalink / raw)
  To: David Marchand, Mattias Rönnblom
  Cc: dev, thomas, bruce.richardson, ktraynor, Mattias Rönnblom,
	Tyler Retzlaff

> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Wednesday, 16 October 2024 17.37
> 
> On Wed, Oct 16, 2024 at 4:14 PM Mattias Rönnblom
> <hofors@lysator.liu.se> wrote:
> >
> > On 2024-10-16 13:38, David Marchand wrote:
> > > For a reason similar to the change on bitops header, hide bitset
> > > implementation relying on experimental API.
> > >
> > > Fixes: 99a1197647d8 ("eal: add bitset type")
> > >
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > > ---
> > >   lib/eal/include/rte_bitset.h | 123
> +++++++++++++++++++++++++++++++++++
> > >   1 file changed, 123 insertions(+)
> > >
> > > diff --git a/lib/eal/include/rte_bitset.h
> b/lib/eal/include/rte_bitset.h
> > > index 74c643a72a..8ae8425fc2 100644
> > > --- a/lib/eal/include/rte_bitset.h
> > > +++ b/lib/eal/include/rte_bitset.h
> > > @@ -255,7 +255,13 @@ __rte_experimental
> > >   static inline bool
> > >   rte_bitset_test(const uint64_t *bitset, size_t bit_num)
> > >   {
> > > +#ifdef ALLOW_EXPERIMENTAL_API
> > >       return __RTE_BITSET_DELEGATE(rte_bit_test, bitset, bit_num);
> > > +#else
> > > +     RTE_SET_USED(bitset);
> > > +     RTE_SET_USED(bit_num);
> > > +     return false;
> >
> > This is no RTE_VERIFY(0) here, because this is just dummy code, that
> > will never be run. Is that correct?
> 
> Adding a RTE_VERIFY(false) is an interesting idea.
> It is not supposed to be run, indeed.

Great idea.

> 
> Do you prefer I respin with this?

No need.
Instead, create a ticket in Bugzilla so RTE_VERIFY(false) goes in everywhere there is dummy code, not just here.

> 
> 
> --
> David Marchand


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 2/4] bitset: fix build for GCC without experimental API
  2024-10-16 15:42         ` Morten Brørup
@ 2024-10-16 16:03           ` Mattias Rönnblom
  2024-10-16 16:17             ` Thomas Monjalon
  0 siblings, 1 reply; 34+ messages in thread
From: Mattias Rönnblom @ 2024-10-16 16:03 UTC (permalink / raw)
  To: Morten Brørup, David Marchand
  Cc: dev, thomas, bruce.richardson, ktraynor, Mattias Rönnblom,
	Tyler Retzlaff

On 2024-10-16 17:42, Morten Brørup wrote:
>> From: David Marchand [mailto:david.marchand@redhat.com]
>> Sent: Wednesday, 16 October 2024 17.37
>>
>> On Wed, Oct 16, 2024 at 4:14 PM Mattias Rönnblom
>> <hofors@lysator.liu.se> wrote:
>>>
>>> On 2024-10-16 13:38, David Marchand wrote:
>>>> For a reason similar to the change on bitops header, hide bitset
>>>> implementation relying on experimental API.
>>>>
>>>> Fixes: 99a1197647d8 ("eal: add bitset type")
>>>>
>>>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>>>> Acked-by: Morten Brørup <mb@smartsharesystems.com>
>>>> ---
>>>>    lib/eal/include/rte_bitset.h | 123
>> +++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 123 insertions(+)
>>>>
>>>> diff --git a/lib/eal/include/rte_bitset.h
>> b/lib/eal/include/rte_bitset.h
>>>> index 74c643a72a..8ae8425fc2 100644
>>>> --- a/lib/eal/include/rte_bitset.h
>>>> +++ b/lib/eal/include/rte_bitset.h
>>>> @@ -255,7 +255,13 @@ __rte_experimental
>>>>    static inline bool
>>>>    rte_bitset_test(const uint64_t *bitset, size_t bit_num)
>>>>    {
>>>> +#ifdef ALLOW_EXPERIMENTAL_API
>>>>        return __RTE_BITSET_DELEGATE(rte_bit_test, bitset, bit_num);
>>>> +#else
>>>> +     RTE_SET_USED(bitset);
>>>> +     RTE_SET_USED(bit_num);
>>>> +     return false;
>>>
>>> This is no RTE_VERIFY(0) here, because this is just dummy code, that
>>> will never be run. Is that correct?
>>
>> Adding a RTE_VERIFY(false) is an interesting idea.
>> It is not supposed to be run, indeed.
> 
> Great idea.
> 
>>
>> Do you prefer I respin with this?
> 
> No need.
> Instead, create a ticket in Bugzilla so RTE_VERIFY(false) goes in everywhere there is dummy code, not just here.
> 

No experimental function is supposed to be invoked, if 
ALLOW_EXPERIMENTAL_API is not set, right? So RTE_VERIFY(), or its 
compile-time equivalent, should be in every such function, not just the 
broken ones.

>>
>>
>> --
>> David Marchand
> 


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 2/4] bitset: fix build for GCC without experimental API
  2024-10-16 16:03           ` Mattias Rönnblom
@ 2024-10-16 16:17             ` Thomas Monjalon
  0 siblings, 0 replies; 34+ messages in thread
From: Thomas Monjalon @ 2024-10-16 16:17 UTC (permalink / raw)
  To: Morten Brørup, David Marchand, Mattias Rönnblom
  Cc: dev, bruce.richardson, ktraynor, Mattias Rönnblom, Tyler Retzlaff

16/10/2024 18:03, Mattias Rönnblom:
> On 2024-10-16 17:42, Morten Brørup wrote:
> >> From: David Marchand [mailto:david.marchand@redhat.com]
> >> Sent: Wednesday, 16 October 2024 17.37
> >>
> >> On Wed, Oct 16, 2024 at 4:14 PM Mattias Rönnblom
> >> <hofors@lysator.liu.se> wrote:
> >>>
> >>> On 2024-10-16 13:38, David Marchand wrote:
> >>>> For a reason similar to the change on bitops header, hide bitset
> >>>> implementation relying on experimental API.
> >>>>
> >>>> Fixes: 99a1197647d8 ("eal: add bitset type")
> >>>>
> >>>> Signed-off-by: David Marchand <david.marchand@redhat.com>
> >>>> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> >>>> ---
> >>>>    lib/eal/include/rte_bitset.h | 123
> >> +++++++++++++++++++++++++++++++++++
> >>>>    1 file changed, 123 insertions(+)
> >>>>
> >>>> diff --git a/lib/eal/include/rte_bitset.h
> >> b/lib/eal/include/rte_bitset.h
> >>>> index 74c643a72a..8ae8425fc2 100644
> >>>> --- a/lib/eal/include/rte_bitset.h
> >>>> +++ b/lib/eal/include/rte_bitset.h
> >>>> @@ -255,7 +255,13 @@ __rte_experimental
> >>>>    static inline bool
> >>>>    rte_bitset_test(const uint64_t *bitset, size_t bit_num)
> >>>>    {
> >>>> +#ifdef ALLOW_EXPERIMENTAL_API
> >>>>        return __RTE_BITSET_DELEGATE(rte_bit_test, bitset, bit_num);
> >>>> +#else
> >>>> +     RTE_SET_USED(bitset);
> >>>> +     RTE_SET_USED(bit_num);
> >>>> +     return false;
> >>>
> >>> This is no RTE_VERIFY(0) here, because this is just dummy code, that
> >>> will never be run. Is that correct?
> >>
> >> Adding a RTE_VERIFY(false) is an interesting idea.
> >> It is not supposed to be run, indeed.
> > 
> > Great idea.
> > 
> >>
> >> Do you prefer I respin with this?
> > 
> > No need.
> > Instead, create a ticket in Bugzilla so RTE_VERIFY(false) goes in everywhere there is dummy code, not just here.
> > 
> 
> No experimental function is supposed to be invoked, if 
> ALLOW_EXPERIMENTAL_API is not set, right? So RTE_VERIFY(), or its 
> compile-time equivalent, should be in every such function, not just the 
> broken ones.

There is no other experimental dummy branch in the current version.
I think respin is a good option.



^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 2/4] bitset: fix build for GCC without experimental API
  2024-10-16 15:36       ` David Marchand
  2024-10-16 15:42         ` Morten Brørup
@ 2024-10-16 15:51         ` Mattias Rönnblom
  1 sibling, 0 replies; 34+ messages in thread
From: Mattias Rönnblom @ 2024-10-16 15:51 UTC (permalink / raw)
  To: David Marchand
  Cc: dev, thomas, bruce.richardson, ktraynor, Morten Brørup,
	Mattias Rönnblom, Tyler Retzlaff

On 2024-10-16 17:36, David Marchand wrote:
> On Wed, Oct 16, 2024 at 4:14 PM Mattias Rönnblom <hofors@lysator.liu.se> wrote:
>>
>> On 2024-10-16 13:38, David Marchand wrote:
>>> For a reason similar to the change on bitops header, hide bitset
>>> implementation relying on experimental API.
>>>
>>> Fixes: 99a1197647d8 ("eal: add bitset type")
>>>
>>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>>> Acked-by: Morten Brørup <mb@smartsharesystems.com>
>>> ---
>>>    lib/eal/include/rte_bitset.h | 123 +++++++++++++++++++++++++++++++++++
>>>    1 file changed, 123 insertions(+)
>>>
>>> diff --git a/lib/eal/include/rte_bitset.h b/lib/eal/include/rte_bitset.h
>>> index 74c643a72a..8ae8425fc2 100644
>>> --- a/lib/eal/include/rte_bitset.h
>>> +++ b/lib/eal/include/rte_bitset.h
>>> @@ -255,7 +255,13 @@ __rte_experimental
>>>    static inline bool
>>>    rte_bitset_test(const uint64_t *bitset, size_t bit_num)
>>>    {
>>> +#ifdef ALLOW_EXPERIMENTAL_API
>>>        return __RTE_BITSET_DELEGATE(rte_bit_test, bitset, bit_num);
>>> +#else
>>> +     RTE_SET_USED(bitset);
>>> +     RTE_SET_USED(bit_num);
>>> +     return false;
>>
>> This is no RTE_VERIFY(0) here, because this is just dummy code, that
>> will never be run. Is that correct?
> 
> Adding a RTE_VERIFY(false) is an interesting idea.
> It is not supposed to be run, indeed.
> 
> Do you prefer I respin with this?
> 
> 

I reminded myself how ALLOW_EXPERIMENTAL_APIs works. This code may 
indeed be run. Experimental just generates a compiler warning.

So RTE_VERIFY(0); is needed. You should be able to remove the "return 
false;" statement.


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v2 3/4] vhost: remove internal vDPA API description from public header
  2024-10-16 11:38 ` [PATCH v2 0/4] Enhance headers check David Marchand
  2024-10-16 11:38   ` [PATCH v2 1/4] bitops: fix build for GCC without experimental API David Marchand
  2024-10-16 11:38   ` [PATCH v2 2/4] bitset: " David Marchand
@ 2024-10-16 11:38   ` David Marchand
  2024-10-16 11:47     ` Maxime Coquelin
  2024-10-16 11:38   ` [PATCH v2 4/4] buildtools/chkincs: check headers with stable API only David Marchand
  2024-10-16 20:40   ` [PATCH v2 0/4] Enhance headers check David Marchand
  4 siblings, 1 reply; 34+ messages in thread
From: David Marchand @ 2024-10-16 11:38 UTC (permalink / raw)
  To: dev
  Cc: thomas, bruce.richardson, ktraynor, Maxime Coquelin, Chenbo Xia,
	Adrian Moreno

rte_vdpa_relay_vring_used() is an internal symbol which was left behind
in the application header when doing the application vs driver API
split.

Fixes: a49f758d1170 ("vhost: split vDPA header file")

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/vhost/rte_vdpa.h | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/lib/vhost/rte_vdpa.h b/lib/vhost/rte_vdpa.h
index 18e273c20f..3a7755d82d 100644
--- a/lib/vhost/rte_vdpa.h
+++ b/lib/vhost/rte_vdpa.h
@@ -108,23 +108,6 @@ rte_vdpa_get_features(struct rte_vdpa_device *dev, uint64_t *features);
 int
 rte_vdpa_get_protocol_features(struct rte_vdpa_device *dev, uint64_t *features);
 
-/**
- * Synchronize the used ring from mediated ring to guest, log dirty
- * page for each writeable buffer, caller should handle the used
- * ring logging before device stop.
- *
- * @param vid
- *  vhost device id
- * @param qid
- *  vhost queue id
- * @param vring_m
- *  mediated virtio ring pointer
- * @return
- *  number of synced used entries on success, -1 on failure
- */
-int
-rte_vdpa_relay_vring_used(int vid, uint16_t qid, void *vring_m);
-
 /**
  * Retrieve names of statistics of a vDPA device.
  *
-- 
2.46.2


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 3/4] vhost: remove internal vDPA API description from public header
  2024-10-16 11:38   ` [PATCH v2 3/4] vhost: remove internal vDPA API description from public header David Marchand
@ 2024-10-16 11:47     ` Maxime Coquelin
  0 siblings, 0 replies; 34+ messages in thread
From: Maxime Coquelin @ 2024-10-16 11:47 UTC (permalink / raw)
  To: David Marchand, dev
  Cc: thomas, bruce.richardson, ktraynor, Chenbo Xia, Adrian Moreno



On 10/16/24 13:38, David Marchand wrote:
> rte_vdpa_relay_vring_used() is an internal symbol which was left behind
> in the application header when doing the application vs driver API
> split.
> 
> Fixes: a49f758d1170 ("vhost: split vDPA header file")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>   lib/vhost/rte_vdpa.h | 17 -----------------
>   1 file changed, 17 deletions(-)
> 

Good catch!

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v2 4/4] buildtools/chkincs: check headers with stable API only
  2024-10-16 11:38 ` [PATCH v2 0/4] Enhance headers check David Marchand
                     ` (2 preceding siblings ...)
  2024-10-16 11:38   ` [PATCH v2 3/4] vhost: remove internal vDPA API description from public header David Marchand
@ 2024-10-16 11:38   ` David Marchand
  2024-10-16 20:40   ` [PATCH v2 0/4] Enhance headers check David Marchand
  4 siblings, 0 replies; 34+ messages in thread
From: David Marchand @ 2024-10-16 11:38 UTC (permalink / raw)
  To: dev; +Cc: thomas, bruce.richardson, ktraynor

An exported header should be usable w/ and w/o ALLOW_EXPERIMENTAL_API so
that an application that only wants stable API may include it.

Plus, the widely common case is that an application will not use
internal API.

Cover those cases but keep the original test.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 buildtools/chkincs/meson.build | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/buildtools/chkincs/meson.build b/buildtools/chkincs/meson.build
index 8da221fc33..787d70272b 100644
--- a/buildtools/chkincs/meson.build
+++ b/buildtools/chkincs/meson.build
@@ -12,8 +12,6 @@ gen_c_files = generator(gen_c_file_for_header,
         arguments: ['@INPUT@', '@OUTPUT@'])
 
 cflags = machine_args
-cflags += '-DALLOW_EXPERIMENTAL_API'
-cflags += '-DALLOW_INTERNAL_API'
 
 sources = files('main.c')
 sources += gen_c_files.process(dpdk_chkinc_headers)
@@ -32,6 +30,18 @@ executable('chkincs', sources,
         dependencies: deps,
         install: false)
 
+executable('chkincs-exp', sources,
+        c_args: [cflags, '-DALLOW_EXPERIMENTAL_API'],
+        include_directories: includes,
+        dependencies: deps,
+        install: false)
+
+executable('chkincs-all', sources,
+        c_args: [cflags, '-DALLOW_EXPERIMENTAL_API', '-DALLOW_INTERNAL_API'],
+        include_directories: includes,
+        dependencies: deps,
+        install: false)
+
 # run tests for c++ builds also
 if not add_languages('cpp', required: false)
     subdir_done()
@@ -49,3 +59,16 @@ executable('chkincs-cpp', cpp_sources,
         include_directories: includes,
         dependencies: deps,
         install: false)
+
+executable('chkincs-cpp-exp', cpp_sources,
+        cpp_args: ['-include', 'rte_config.h', cflags, '-DALLOW_EXPERIMENTAL_API'],
+        include_directories: includes,
+        dependencies: deps,
+        install: false)
+
+executable('chkincs-cpp-all', cpp_sources,
+        cpp_args: ['-include', 'rte_config.h', cflags, '-DALLOW_EXPERIMENTAL_API',
+                   '-DALLOW_INTERNAL_API'],
+        include_directories: includes,
+        dependencies: deps,
+        install: false)
-- 
2.46.2


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 0/4] Enhance headers check
  2024-10-16 11:38 ` [PATCH v2 0/4] Enhance headers check David Marchand
                     ` (3 preceding siblings ...)
  2024-10-16 11:38   ` [PATCH v2 4/4] buildtools/chkincs: check headers with stable API only David Marchand
@ 2024-10-16 20:40   ` David Marchand
  4 siblings, 0 replies; 34+ messages in thread
From: David Marchand @ 2024-10-16 20:40 UTC (permalink / raw)
  To: dev; +Cc: thomas, bruce.richardson, ktraynor

On Wed, Oct 16, 2024 at 1:38 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> We currently check that exported headers are fine with
> -DALLOW_EXPERIMENTAL_API and -DALLOW_INTERNAL_API.
>
> Such a check won't catch issues like the one fixed in patch 1, where OVS
> compilation is broken by the additional of experimental API in a header
> commonly included in other parts of DPDK.
>
> Ideally, I would like to merge patch 1 in rc1.
> The patch 2 is not a real issue (more like the enhanced check would
> complain about this header).
> In any case, I think it would not hurt merging everything now.

Applied patch 1 and 3.
I will respin the series (or come up with a different fix for patch 2) for rc2.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v3 0/4] Enhance headers check
  2024-10-15 12:10 [PATCH 0/3] Enhance headers check David Marchand
                   ` (3 preceding siblings ...)
  2024-10-16 11:38 ` [PATCH v2 0/4] Enhance headers check David Marchand
@ 2024-10-21 16:04 ` David Marchand
  2024-10-21 16:04   ` [PATCH v3 1/4] bitset: fix build for GCC without experimental API David Marchand
                     ` (5 more replies)
  4 siblings, 6 replies; 34+ messages in thread
From: David Marchand @ 2024-10-21 16:04 UTC (permalink / raw)
  To: dev; +Cc: thomas, bruce.richardson, ktraynor

We currently check that exported headers are fine with
-DALLOW_EXPERIMENTAL_API and -DALLOW_INTERNAL_API.

Such a check won't catch issues when build is broken in the absence of
those flags, which is the common case for applications consuming DPDK.

This series adds more coverage.


-- 
David Marchand

Changes since v2:
- rebased on rc1,
- added check when calling rte_bitset without ALLOW_EXPERIMENTAL_API,
- added checks (and fixes) on driver specific headers,

David Marchand (4):
  bitset: fix build for GCC without experimental API
  buildtools/chkincs: check headers with stable API only
  bus/ifpga: use C linkage where appropriate in driver header
  buildtools/chkincs: check driver specific headers

 buildtools/chkincs/meson.build       |  30 +++++-
 drivers/bus/ifpga/bus_ifpga_driver.h |   8 +-
 drivers/meson.build                  |   1 +
 lib/eal/include/rte_bitset.h         | 132 +++++++++++++++++++++++++++
 4 files changed, 165 insertions(+), 6 deletions(-)

-- 
2.46.2


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v3 1/4] bitset: fix build for GCC without experimental API
  2024-10-21 16:04 ` [PATCH v3 " David Marchand
@ 2024-10-21 16:04   ` David Marchand
  2024-10-21 17:57     ` Mattias Rönnblom
  2024-10-21 16:04   ` [PATCH v3 2/4] buildtools/chkincs: check headers with stable API only David Marchand
                     ` (4 subsequent siblings)
  5 siblings, 1 reply; 34+ messages in thread
From: David Marchand @ 2024-10-21 16:04 UTC (permalink / raw)
  To: dev
  Cc: thomas, bruce.richardson, ktraynor, Mattias Rönnblom,
	Tyler Retzlaff, Morten Brørup

For a reason similar to the change on bitops header, hide bitset
implementation relying on experimental API.

Fixes: 99a1197647d8 ("eal: add bitset type")

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v2:
- added a runtime assert so that an application won't call those
  experimental APIs,

---
 lib/eal/include/rte_bitset.h | 132 +++++++++++++++++++++++++++++++++++
 1 file changed, 132 insertions(+)

diff --git a/lib/eal/include/rte_bitset.h b/lib/eal/include/rte_bitset.h
index 27b7a2e34d..116121eec4 100644
--- a/lib/eal/include/rte_bitset.h
+++ b/lib/eal/include/rte_bitset.h
@@ -255,7 +255,13 @@ __rte_experimental
 static inline bool
 rte_bitset_test(const uint64_t *bitset, size_t bit_num)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return __RTE_BITSET_DELEGATE(rte_bit_test, bitset, bit_num);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+	RTE_VERIFY(false);
+#endif
 }
 
 /**
@@ -277,7 +283,13 @@ __rte_experimental
 static inline void
 rte_bitset_set(uint64_t *bitset, size_t bit_num)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	__RTE_BITSET_DELEGATE(rte_bit_set, bitset, bit_num);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+	RTE_VERIFY(false);
+#endif
 }
 
 /**
@@ -299,7 +311,13 @@ __rte_experimental
 static inline void
 rte_bitset_clear(uint64_t *bitset, size_t bit_num)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	__RTE_BITSET_DELEGATE(rte_bit_clear, bitset, bit_num);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+	RTE_VERIFY(false);
+#endif
 }
 
 /**
@@ -323,7 +341,14 @@ __rte_experimental
 static inline void
 rte_bitset_assign(uint64_t *bitset, size_t bit_num, bool bit_value)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	__RTE_BITSET_DELEGATE_N(rte_bit_assign, bitset, bit_num, bit_value);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+	RTE_SET_USED(bit_value);
+	RTE_VERIFY(false);
+#endif
 }
 
 /**
@@ -345,7 +370,13 @@ __rte_experimental
 static inline void
 rte_bitset_flip(uint64_t *bitset, size_t bit_num)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	__RTE_BITSET_DELEGATE(rte_bit_flip, bitset, bit_num);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+	RTE_VERIFY(false);
+#endif
 }
 
 /**
@@ -370,7 +401,14 @@ __rte_experimental
 static inline bool
 rte_bitset_atomic_test(const uint64_t *bitset, size_t bit_num, int memory_order)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return __RTE_BITSET_DELEGATE_N(rte_bit_atomic_test, bitset, bit_num, memory_order);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+	RTE_SET_USED(memory_order);
+	RTE_VERIFY(false);
+#endif
 }
 
 /**
@@ -399,7 +437,14 @@ __rte_experimental
 static inline void
 rte_bitset_atomic_set(uint64_t *bitset, size_t bit_num, int memory_order)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	__RTE_BITSET_DELEGATE_N(rte_bit_atomic_set, bitset, bit_num, memory_order);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+	RTE_SET_USED(memory_order);
+	RTE_VERIFY(false);
+#endif
 }
 
 /**
@@ -428,7 +473,14 @@ __rte_experimental
 static inline void
 rte_bitset_atomic_clear(uint64_t *bitset, size_t bit_num, int memory_order)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	__RTE_BITSET_DELEGATE_N(rte_bit_atomic_clear, bitset, bit_num, memory_order);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+	RTE_SET_USED(memory_order);
+	RTE_VERIFY(false);
+#endif
 }
 
 /**
@@ -459,7 +511,15 @@ __rte_experimental
 static inline void
 rte_bitset_atomic_assign(uint64_t *bitset, size_t bit_num, bool bit_value, int memory_order)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	__RTE_BITSET_DELEGATE_N(rte_bit_atomic_assign, bitset, bit_num, bit_value, memory_order);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+	RTE_SET_USED(bit_value);
+	RTE_SET_USED(memory_order);
+	RTE_VERIFY(false);
+#endif
 }
 
 /**
@@ -488,7 +548,14 @@ __rte_experimental
 static inline void
 rte_bitset_atomic_flip(uint64_t *bitset, size_t bit_num, int memory_order)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	__RTE_BITSET_DELEGATE_N(rte_bit_atomic_flip, bitset, bit_num, memory_order);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(bit_num);
+	RTE_SET_USED(memory_order);
+	RTE_VERIFY(false);
+#endif
 }
 
 /**
@@ -524,7 +591,13 @@ __rte_experimental
 static inline void
 rte_bitset_clear_all(uint64_t *bitset, size_t size)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	rte_bitset_init(bitset, size);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+	RTE_VERIFY(false);
+#endif
 }
 
 /**
@@ -576,7 +649,13 @@ __rte_experimental
 static inline size_t
 rte_bitset_count_clear(const uint64_t *bitset, size_t size)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return size - rte_bitset_count_set(bitset, size);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+	RTE_VERIFY(false);
+#endif
 }
 
 #define __RTE_BITSET_FIND_FLAG_FIND_CLEAR (1U << 0)
@@ -635,6 +714,7 @@ static inline ssize_t
 __rte_bitset_find(const uint64_t *bitset, size_t size, size_t start_bit, size_t len,
 		unsigned int flags)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	bool find_clear = flags & __RTE_BITSET_FIND_FLAG_FIND_CLEAR;
 	bool may_wrap = flags & __RTE_BITSET_FIND_FLAG_WRAP;
 	bool does_wrap = (start_bit + len) > size;
@@ -655,6 +735,14 @@ __rte_bitset_find(const uint64_t *bitset, size_t size, size_t start_bit, size_t
 		rc = __rte_bitset_find_nowrap(bitset, size, start_bit, len, find_clear);
 
 	return rc;
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+	RTE_SET_USED(start_bit);
+	RTE_SET_USED(len);
+	RTE_SET_USED(flags);
+	RTE_VERIFY(false);
+#endif
 }
 
 /**
@@ -678,7 +766,13 @@ __rte_experimental
 static inline ssize_t
 rte_bitset_find_first_set(const uint64_t *bitset, size_t size)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return __rte_bitset_find(bitset, size, 0, size, 0);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+	RTE_VERIFY(false);
+#endif
 }
 
 /**
@@ -708,7 +802,15 @@ __rte_experimental
 static inline ssize_t
 rte_bitset_find_set(const uint64_t *bitset, size_t size, size_t start_bit, size_t len)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return __rte_bitset_find(bitset, size, start_bit, len, 0);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+	RTE_SET_USED(start_bit);
+	RTE_SET_USED(len);
+	RTE_VERIFY(false);
+#endif
 }
 
 /**
@@ -739,7 +841,15 @@ __rte_experimental
 static inline ssize_t
 rte_bitset_find_set_wrap(const uint64_t *bitset, size_t size, size_t start_bit, size_t len)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return __rte_bitset_find(bitset, size, start_bit, len, __RTE_BITSET_FIND_FLAG_WRAP);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+	RTE_SET_USED(start_bit);
+	RTE_SET_USED(len);
+	RTE_VERIFY(false);
+#endif
 }
 
 /**
@@ -763,7 +873,13 @@ __rte_experimental
 static inline ssize_t
 rte_bitset_find_first_clear(const uint64_t *bitset, size_t size)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return __rte_bitset_find(bitset, size, 0, size, __RTE_BITSET_FIND_FLAG_FIND_CLEAR);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+	RTE_VERIFY(false);
+#endif
 }
 
 /**
@@ -793,7 +909,15 @@ __rte_experimental
 static inline ssize_t
 rte_bitset_find_clear(const uint64_t *bitset, size_t size, size_t start_bit, size_t len)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return __rte_bitset_find(bitset, size, start_bit, len, __RTE_BITSET_FIND_FLAG_FIND_CLEAR);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+	RTE_SET_USED(start_bit);
+	RTE_SET_USED(len);
+	RTE_VERIFY(false);
+#endif
 }
 
 /**
@@ -824,8 +948,16 @@ __rte_experimental
 static inline ssize_t
 rte_bitset_find_clear_wrap(const uint64_t *bitset, size_t size, size_t start_bit, size_t len)
 {
+#ifdef ALLOW_EXPERIMENTAL_API
 	return __rte_bitset_find(bitset, size, start_bit, len,
 		__RTE_BITSET_FIND_FLAG_FIND_CLEAR | __RTE_BITSET_FIND_FLAG_WRAP);
+#else
+	RTE_SET_USED(bitset);
+	RTE_SET_USED(size);
+	RTE_SET_USED(start_bit);
+	RTE_SET_USED(len);
+	RTE_VERIFY(false);
+#endif
 }
 
 /**
-- 
2.46.2


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v3 1/4] bitset: fix build for GCC without experimental API
  2024-10-21 16:04   ` [PATCH v3 1/4] bitset: fix build for GCC without experimental API David Marchand
@ 2024-10-21 17:57     ` Mattias Rönnblom
  0 siblings, 0 replies; 34+ messages in thread
From: Mattias Rönnblom @ 2024-10-21 17:57 UTC (permalink / raw)
  To: David Marchand, dev
  Cc: thomas, bruce.richardson, ktraynor, Mattias Rönnblom,
	Tyler Retzlaff, Morten Brørup

On 2024-10-21 18:04, David Marchand wrote:
> For a reason similar to the change on bitops header, hide bitset
> implementation relying on experimental API.
> 
> Fixes: 99a1197647d8 ("eal: add bitset type")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Changes since v2:
> - added a runtime assert so that an application won't call those
>    experimental APIs,
> 
> ---
>   lib/eal/include/rte_bitset.h | 132 +++++++++++++++++++++++++++++++++++
>   1 file changed, 132 insertions(+)
> 
> diff --git a/lib/eal/include/rte_bitset.h b/lib/eal/include/rte_bitset.h
> index 27b7a2e34d..116121eec4 100644
> --- a/lib/eal/include/rte_bitset.h
> +++ b/lib/eal/include/rte_bitset.h
> @@ -255,7 +255,13 @@ __rte_experimental
>   static inline bool
>   rte_bitset_test(const uint64_t *bitset, size_t bit_num)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	return __RTE_BITSET_DELEGATE(rte_bit_test, bitset, bit_num);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(bit_num);
> +	RTE_VERIFY(false);
> +#endif
>   }
>   
>   /**
> @@ -277,7 +283,13 @@ __rte_experimental
>   static inline void
>   rte_bitset_set(uint64_t *bitset, size_t bit_num)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	__RTE_BITSET_DELEGATE(rte_bit_set, bitset, bit_num);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(bit_num);
> +	RTE_VERIFY(false);
> +#endif
>   }
>   
>   /**
> @@ -299,7 +311,13 @@ __rte_experimental
>   static inline void
>   rte_bitset_clear(uint64_t *bitset, size_t bit_num)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	__RTE_BITSET_DELEGATE(rte_bit_clear, bitset, bit_num);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(bit_num);
> +	RTE_VERIFY(false);
> +#endif
>   }
>   
>   /**
> @@ -323,7 +341,14 @@ __rte_experimental
>   static inline void
>   rte_bitset_assign(uint64_t *bitset, size_t bit_num, bool bit_value)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	__RTE_BITSET_DELEGATE_N(rte_bit_assign, bitset, bit_num, bit_value);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(bit_num);
> +	RTE_SET_USED(bit_value);
> +	RTE_VERIFY(false);
> +#endif
>   }
>   
>   /**
> @@ -345,7 +370,13 @@ __rte_experimental
>   static inline void
>   rte_bitset_flip(uint64_t *bitset, size_t bit_num)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	__RTE_BITSET_DELEGATE(rte_bit_flip, bitset, bit_num);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(bit_num);
> +	RTE_VERIFY(false);
> +#endif
>   }
>   
>   /**
> @@ -370,7 +401,14 @@ __rte_experimental
>   static inline bool
>   rte_bitset_atomic_test(const uint64_t *bitset, size_t bit_num, int memory_order)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	return __RTE_BITSET_DELEGATE_N(rte_bit_atomic_test, bitset, bit_num, memory_order);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(bit_num);
> +	RTE_SET_USED(memory_order);
> +	RTE_VERIFY(false);
> +#endif
>   }
>   
>   /**
> @@ -399,7 +437,14 @@ __rte_experimental
>   static inline void
>   rte_bitset_atomic_set(uint64_t *bitset, size_t bit_num, int memory_order)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	__RTE_BITSET_DELEGATE_N(rte_bit_atomic_set, bitset, bit_num, memory_order);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(bit_num);
> +	RTE_SET_USED(memory_order);
> +	RTE_VERIFY(false);
> +#endif
>   }
>   
>   /**
> @@ -428,7 +473,14 @@ __rte_experimental
>   static inline void
>   rte_bitset_atomic_clear(uint64_t *bitset, size_t bit_num, int memory_order)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	__RTE_BITSET_DELEGATE_N(rte_bit_atomic_clear, bitset, bit_num, memory_order);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(bit_num);
> +	RTE_SET_USED(memory_order);
> +	RTE_VERIFY(false);
> +#endif
>   }
>   
>   /**
> @@ -459,7 +511,15 @@ __rte_experimental
>   static inline void
>   rte_bitset_atomic_assign(uint64_t *bitset, size_t bit_num, bool bit_value, int memory_order)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	__RTE_BITSET_DELEGATE_N(rte_bit_atomic_assign, bitset, bit_num, bit_value, memory_order);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(bit_num);
> +	RTE_SET_USED(bit_value);
> +	RTE_SET_USED(memory_order);
> +	RTE_VERIFY(false);
> +#endif
>   }
>   
>   /**
> @@ -488,7 +548,14 @@ __rte_experimental
>   static inline void
>   rte_bitset_atomic_flip(uint64_t *bitset, size_t bit_num, int memory_order)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	__RTE_BITSET_DELEGATE_N(rte_bit_atomic_flip, bitset, bit_num, memory_order);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(bit_num);
> +	RTE_SET_USED(memory_order);
> +	RTE_VERIFY(false);
> +#endif
>   }
>   
>   /**
> @@ -524,7 +591,13 @@ __rte_experimental
>   static inline void
>   rte_bitset_clear_all(uint64_t *bitset, size_t size)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	rte_bitset_init(bitset, size);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(size);
> +	RTE_VERIFY(false);
> +#endif
>   }
>   
>   /**
> @@ -576,7 +649,13 @@ __rte_experimental
>   static inline size_t
>   rte_bitset_count_clear(const uint64_t *bitset, size_t size)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	return size - rte_bitset_count_set(bitset, size);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(size);
> +	RTE_VERIFY(false);
> +#endif
>   }
>   
>   #define __RTE_BITSET_FIND_FLAG_FIND_CLEAR (1U << 0)
> @@ -635,6 +714,7 @@ static inline ssize_t
>   __rte_bitset_find(const uint64_t *bitset, size_t size, size_t start_bit, size_t len,
>   		unsigned int flags)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	bool find_clear = flags & __RTE_BITSET_FIND_FLAG_FIND_CLEAR;
>   	bool may_wrap = flags & __RTE_BITSET_FIND_FLAG_WRAP;
>   	bool does_wrap = (start_bit + len) > size;
> @@ -655,6 +735,14 @@ __rte_bitset_find(const uint64_t *bitset, size_t size, size_t start_bit, size_t
>   		rc = __rte_bitset_find_nowrap(bitset, size, start_bit, len, find_clear);
>   
>   	return rc;
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(size);
> +	RTE_SET_USED(start_bit);
> +	RTE_SET_USED(len);
> +	RTE_SET_USED(flags);
> +	RTE_VERIFY(false);
> +#endif
>   }
>   
>   /**
> @@ -678,7 +766,13 @@ __rte_experimental
>   static inline ssize_t
>   rte_bitset_find_first_set(const uint64_t *bitset, size_t size)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	return __rte_bitset_find(bitset, size, 0, size, 0);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(size);
> +	RTE_VERIFY(false);
> +#endif
>   }
>   
>   /**
> @@ -708,7 +802,15 @@ __rte_experimental
>   static inline ssize_t
>   rte_bitset_find_set(const uint64_t *bitset, size_t size, size_t start_bit, size_t len)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	return __rte_bitset_find(bitset, size, start_bit, len, 0);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(size);
> +	RTE_SET_USED(start_bit);
> +	RTE_SET_USED(len);
> +	RTE_VERIFY(false);
> +#endif
>   }
>   
>   /**
> @@ -739,7 +841,15 @@ __rte_experimental
>   static inline ssize_t
>   rte_bitset_find_set_wrap(const uint64_t *bitset, size_t size, size_t start_bit, size_t len)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	return __rte_bitset_find(bitset, size, start_bit, len, __RTE_BITSET_FIND_FLAG_WRAP);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(size);
> +	RTE_SET_USED(start_bit);
> +	RTE_SET_USED(len);
> +	RTE_VERIFY(false);
> +#endif
>   }
>   
>   /**
> @@ -763,7 +873,13 @@ __rte_experimental
>   static inline ssize_t
>   rte_bitset_find_first_clear(const uint64_t *bitset, size_t size)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	return __rte_bitset_find(bitset, size, 0, size, __RTE_BITSET_FIND_FLAG_FIND_CLEAR);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(size);
> +	RTE_VERIFY(false);
> +#endif
>   }
>   
>   /**
> @@ -793,7 +909,15 @@ __rte_experimental
>   static inline ssize_t
>   rte_bitset_find_clear(const uint64_t *bitset, size_t size, size_t start_bit, size_t len)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	return __rte_bitset_find(bitset, size, start_bit, len, __RTE_BITSET_FIND_FLAG_FIND_CLEAR);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(size);
> +	RTE_SET_USED(start_bit);
> +	RTE_SET_USED(len);
> +	RTE_VERIFY(false);
> +#endif
>   }
>   
>   /**
> @@ -824,8 +948,16 @@ __rte_experimental
>   static inline ssize_t
>   rte_bitset_find_clear_wrap(const uint64_t *bitset, size_t size, size_t start_bit, size_t len)
>   {
> +#ifdef ALLOW_EXPERIMENTAL_API
>   	return __rte_bitset_find(bitset, size, start_bit, len,
>   		__RTE_BITSET_FIND_FLAG_FIND_CLEAR | __RTE_BITSET_FIND_FLAG_WRAP);
> +#else
> +	RTE_SET_USED(bitset);
> +	RTE_SET_USED(size);
> +	RTE_SET_USED(start_bit);
> +	RTE_SET_USED(len);
> +	RTE_VERIFY(false);
> +#endif
>   }
>   
>   /**

Reviewed-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>



^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v3 2/4] buildtools/chkincs: check headers with stable API only
  2024-10-21 16:04 ` [PATCH v3 " David Marchand
  2024-10-21 16:04   ` [PATCH v3 1/4] bitset: fix build for GCC without experimental API David Marchand
@ 2024-10-21 16:04   ` David Marchand
  2024-10-21 16:06     ` Bruce Richardson
  2024-10-21 16:04   ` [PATCH v3 3/4] bus/ifpga: use C linkage where appropriate in driver header David Marchand
                     ` (3 subsequent siblings)
  5 siblings, 1 reply; 34+ messages in thread
From: David Marchand @ 2024-10-21 16:04 UTC (permalink / raw)
  To: dev; +Cc: thomas, bruce.richardson, ktraynor

An exported header should be usable w/ and w/o ALLOW_EXPERIMENTAL_API so
that an application that only wants stable API may include it.

Plus, the widely common case is that an application will not use
internal API.

Cover those cases but keep the original test.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 buildtools/chkincs/meson.build | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/buildtools/chkincs/meson.build b/buildtools/chkincs/meson.build
index 8da221fc33..787d70272b 100644
--- a/buildtools/chkincs/meson.build
+++ b/buildtools/chkincs/meson.build
@@ -12,8 +12,6 @@ gen_c_files = generator(gen_c_file_for_header,
         arguments: ['@INPUT@', '@OUTPUT@'])
 
 cflags = machine_args
-cflags += '-DALLOW_EXPERIMENTAL_API'
-cflags += '-DALLOW_INTERNAL_API'
 
 sources = files('main.c')
 sources += gen_c_files.process(dpdk_chkinc_headers)
@@ -32,6 +30,18 @@ executable('chkincs', sources,
         dependencies: deps,
         install: false)
 
+executable('chkincs-exp', sources,
+        c_args: [cflags, '-DALLOW_EXPERIMENTAL_API'],
+        include_directories: includes,
+        dependencies: deps,
+        install: false)
+
+executable('chkincs-all', sources,
+        c_args: [cflags, '-DALLOW_EXPERIMENTAL_API', '-DALLOW_INTERNAL_API'],
+        include_directories: includes,
+        dependencies: deps,
+        install: false)
+
 # run tests for c++ builds also
 if not add_languages('cpp', required: false)
     subdir_done()
@@ -49,3 +59,16 @@ executable('chkincs-cpp', cpp_sources,
         include_directories: includes,
         dependencies: deps,
         install: false)
+
+executable('chkincs-cpp-exp', cpp_sources,
+        cpp_args: ['-include', 'rte_config.h', cflags, '-DALLOW_EXPERIMENTAL_API'],
+        include_directories: includes,
+        dependencies: deps,
+        install: false)
+
+executable('chkincs-cpp-all', cpp_sources,
+        cpp_args: ['-include', 'rte_config.h', cflags, '-DALLOW_EXPERIMENTAL_API',
+                   '-DALLOW_INTERNAL_API'],
+        include_directories: includes,
+        dependencies: deps,
+        install: false)
-- 
2.46.2


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v3 2/4] buildtools/chkincs: check headers with stable API only
  2024-10-21 16:04   ` [PATCH v3 2/4] buildtools/chkincs: check headers with stable API only David Marchand
@ 2024-10-21 16:06     ` Bruce Richardson
  0 siblings, 0 replies; 34+ messages in thread
From: Bruce Richardson @ 2024-10-21 16:06 UTC (permalink / raw)
  To: David Marchand; +Cc: dev, thomas, ktraynor

On Mon, Oct 21, 2024 at 06:04:45PM +0200, David Marchand wrote:
> An exported header should be usable w/ and w/o ALLOW_EXPERIMENTAL_API so
> that an application that only wants stable API may include it.
> 
> Plus, the widely common case is that an application will not use
> internal API.
> 
> Cover those cases but keep the original test.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v3 3/4] bus/ifpga: use C linkage where appropriate in driver header
  2024-10-21 16:04 ` [PATCH v3 " David Marchand
  2024-10-21 16:04   ` [PATCH v3 1/4] bitset: fix build for GCC without experimental API David Marchand
  2024-10-21 16:04   ` [PATCH v3 2/4] buildtools/chkincs: check headers with stable API only David Marchand
@ 2024-10-21 16:04   ` David Marchand
  2024-10-21 18:01     ` Mattias Rönnblom
  2024-10-21 16:04   ` [PATCH v3 4/4] buildtools/chkincs: check driver specific headers David Marchand
                     ` (2 subsequent siblings)
  5 siblings, 1 reply; 34+ messages in thread
From: David Marchand @ 2024-10-21 16:04 UTC (permalink / raw)
  To: dev
  Cc: thomas, bruce.richardson, ktraynor, Rosen Xu,
	Mattias Rönnblom, Morten Brørup

This driver was missed when cleaning up most of the headers.

Fixes: 719834a6849e ("use C linkage where appropriate in headers")

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/bus/ifpga/bus_ifpga_driver.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/bus/ifpga/bus_ifpga_driver.h b/drivers/bus/ifpga/bus_ifpga_driver.h
index 5bbe36d6e0..a42afc7d75 100644
--- a/drivers/bus/ifpga/bus_ifpga_driver.h
+++ b/drivers/bus/ifpga/bus_ifpga_driver.h
@@ -11,16 +11,16 @@
  * RTE Intel FPGA Bus Interface
  */
 
-#ifdef __cplusplus
-extern "C" {
-#endif /* __cplusplus */
-
 #include <rte_compat.h>
 #include <dev_driver.h>
 #include <rte_pci.h>
 #include <rte_interrupts.h>
 #include <rte_spinlock.h>
 
+#ifdef __cplusplus
+extern "C" {
+#endif /* __cplusplus */
+
 #define IFPGA_BUS_NAME ifpga
 
 /* Forward declarations */
-- 
2.46.2


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v3 3/4] bus/ifpga: use C linkage where appropriate in driver header
  2024-10-21 16:04   ` [PATCH v3 3/4] bus/ifpga: use C linkage where appropriate in driver header David Marchand
@ 2024-10-21 18:01     ` Mattias Rönnblom
  2024-10-22 19:27       ` David Marchand
  0 siblings, 1 reply; 34+ messages in thread
From: Mattias Rönnblom @ 2024-10-21 18:01 UTC (permalink / raw)
  To: David Marchand, dev
  Cc: thomas, bruce.richardson, ktraynor, Rosen Xu,
	Mattias Rönnblom, Morten Brørup

On 2024-10-21 18:04, David Marchand wrote:
> This driver was missed when cleaning up most of the headers.
> 
> Fixes: 719834a6849e ("use C linkage where appropriate in headers")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>   drivers/bus/ifpga/bus_ifpga_driver.h | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bus/ifpga/bus_ifpga_driver.h b/drivers/bus/ifpga/bus_ifpga_driver.h
> index 5bbe36d6e0..a42afc7d75 100644
> --- a/drivers/bus/ifpga/bus_ifpga_driver.h
> +++ b/drivers/bus/ifpga/bus_ifpga_driver.h
> @@ -11,16 +11,16 @@
>    * RTE Intel FPGA Bus Interface
>    */
>   
> -#ifdef __cplusplus
> -extern "C" {
> -#endif /* __cplusplus */
> -
>   #include <rte_compat.h>
>   #include <dev_driver.h>
>   #include <rte_pci.h>
>   #include <rte_interrupts.h>
>   #include <rte_spinlock.h>
>   
> +#ifdef __cplusplus
> +extern "C" {
> +#endif /* __cplusplus */
> +
>   #define IFPGA_BUS_NAME ifpga
>   
>   /* Forward declarations */

My effort was limited to the headers passed to chkincs, which I'm 
guessing is the public API headers.

Internal headers shouldn't have any #ifdef __cpluspus at all, anywhere.


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v3 3/4] bus/ifpga: use C linkage where appropriate in driver header
  2024-10-21 18:01     ` Mattias Rönnblom
@ 2024-10-22 19:27       ` David Marchand
  0 siblings, 0 replies; 34+ messages in thread
From: David Marchand @ 2024-10-22 19:27 UTC (permalink / raw)
  To: Mattias Rönnblom
  Cc: dev, thomas, bruce.richardson, ktraynor, Rosen Xu,
	Mattias Rönnblom, Morten Brørup

On Mon, Oct 21, 2024 at 8:01 PM Mattias Rönnblom <hofors@lysator.liu.se> wrote:
>
> On 2024-10-21 18:04, David Marchand wrote:
> > This driver was missed when cleaning up most of the headers.
> >
> > Fixes: 719834a6849e ("use C linkage where appropriate in headers")
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> >   drivers/bus/ifpga/bus_ifpga_driver.h | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/bus/ifpga/bus_ifpga_driver.h b/drivers/bus/ifpga/bus_ifpga_driver.h
> > index 5bbe36d6e0..a42afc7d75 100644
> > --- a/drivers/bus/ifpga/bus_ifpga_driver.h
> > +++ b/drivers/bus/ifpga/bus_ifpga_driver.h
> > @@ -11,16 +11,16 @@
> >    * RTE Intel FPGA Bus Interface
> >    */
> >
> > -#ifdef __cplusplus
> > -extern "C" {
> > -#endif /* __cplusplus */
> > -
> >   #include <rte_compat.h>
> >   #include <dev_driver.h>
> >   #include <rte_pci.h>
> >   #include <rte_interrupts.h>
> >   #include <rte_spinlock.h>
> >
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif /* __cplusplus */
> > +
> >   #define IFPGA_BUS_NAME ifpga
> >
> >   /* Forward declarations */
>
> My effort was limited to the headers passed to chkincs, which I'm
> guessing is the public API headers.

There were many headers that got changed in commit 719834a6849e ("use
C linkage where appropriate in headers") while not being in the
exported headers list, which is what chkincs checks.

For example,
$ git show --stat 719834a6849e | grep raw/ifpga
drivers/raw/ifpga/afu_pmd_core.h                 |  8 ++++----
 drivers/raw/ifpga/afu_pmd_he_hssi.h              |  6 +++---
 drivers/raw/ifpga/afu_pmd_he_lpbk.h              |  6 +++---
 drivers/raw/ifpga/afu_pmd_he_mem.h               |  6 +++---
 drivers/raw/ifpga/afu_pmd_n3000.h                |  6 +++---
 drivers/raw/ifpga/rte_pmd_afu.h                  |  4 ++--
 drivers/raw/ifpga/rte_pmd_ifpga.h                |  4 ++--

$ grep chkincs.*afu build/build.ninja

>
> Internal headers shouldn't have any #ifdef __cpluspus at all, anywhere.
>

Internal headers, I agree, though commit 719834a6849e ("use C linkage
where appropriate in headers") did change some.

For drivers headers that are exported, it is another story, some
people seem to like C++ for writing out of tree drivers.
And this is what this patch of mine is about.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v3 4/4] buildtools/chkincs: check driver specific headers
  2024-10-21 16:04 ` [PATCH v3 " David Marchand
                     ` (2 preceding siblings ...)
  2024-10-21 16:04   ` [PATCH v3 3/4] bus/ifpga: use C linkage where appropriate in driver header David Marchand
@ 2024-10-21 16:04   ` David Marchand
  2024-10-21 16:23   ` [PATCH v3 0/4] Enhance headers check Morten Brørup
  2024-10-23 14:39   ` David Marchand
  5 siblings, 0 replies; 34+ messages in thread
From: David Marchand @ 2024-10-21 16:04 UTC (permalink / raw)
  To: dev; +Cc: thomas, bruce.richardson, ktraynor, Tyler Retzlaff

Only driver headers exported by libraries were checked.
Add driver specific headers to the checked headers list.

This reveals a small issue with the vmbus driver header as some driver
headers rely on it.

Fixes: b232b2aa212b ("buildtools/chkincs: check SDK headers for C++ compatibility")
Fixes: 84aaf06d817c ("bus/vmbus: make driver-only headers private")

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 buildtools/chkincs/meson.build | 3 +++
 drivers/meson.build            | 1 +
 2 files changed, 4 insertions(+)

diff --git a/buildtools/chkincs/meson.build b/buildtools/chkincs/meson.build
index 787d70272b..ed65105599 100644
--- a/buildtools/chkincs/meson.build
+++ b/buildtools/chkincs/meson.build
@@ -19,6 +19,9 @@ sources += gen_c_files.process(dpdk_chkinc_headers)
 # some driver SDK headers depend on these two buses, which are mandatory in build
 # so we always include them in deps list
 deps = [get_variable('shared_rte_bus_vdev'), get_variable('shared_rte_bus_pci')]
+if dpdk_conf.has('RTE_BUS_VMBUS')
+    deps += get_variable('shared_rte_bus_vmbus')
+endif
 # add the rest of the libs to the dependencies
 foreach l:dpdk_libs_enabled
     deps += get_variable('shared_rte_' + l)
diff --git a/drivers/meson.build b/drivers/meson.build
index 2733306698..5270160c56 100644
--- a/drivers/meson.build
+++ b/drivers/meson.build
@@ -220,6 +220,7 @@ foreach subpath:subdirs
         if get_option('enable_driver_sdk')
             install_headers(driver_sdk_headers)
         endif
+        dpdk_chkinc_headers += driver_sdk_headers
 
         if headers.length() > 0
             dpdk_includes += include_directories(drv_path)
-- 
2.46.2


^ permalink raw reply	[flat|nested] 34+ messages in thread

* RE: [PATCH v3 0/4] Enhance headers check
  2024-10-21 16:04 ` [PATCH v3 " David Marchand
                     ` (3 preceding siblings ...)
  2024-10-21 16:04   ` [PATCH v3 4/4] buildtools/chkincs: check driver specific headers David Marchand
@ 2024-10-21 16:23   ` Morten Brørup
  2024-10-23 14:39   ` David Marchand
  5 siblings, 0 replies; 34+ messages in thread
From: Morten Brørup @ 2024-10-21 16:23 UTC (permalink / raw)
  To: David Marchand, dev; +Cc: thomas, bruce.richardson, ktraynor

For the series,
Acked-by: Morten Brørup <mb@smartsharesystems.com>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v3 0/4] Enhance headers check
  2024-10-21 16:04 ` [PATCH v3 " David Marchand
                     ` (4 preceding siblings ...)
  2024-10-21 16:23   ` [PATCH v3 0/4] Enhance headers check Morten Brørup
@ 2024-10-23 14:39   ` David Marchand
  5 siblings, 0 replies; 34+ messages in thread
From: David Marchand @ 2024-10-23 14:39 UTC (permalink / raw)
  To: David Marchand
  Cc: dev, thomas, bruce.richardson, ktraynor, Morten Brørup,
	Mattias Rönnblom

On Mon, Oct 21, 2024 at 6:05 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> We currently check that exported headers are fine with
> -DALLOW_EXPERIMENTAL_API and -DALLOW_INTERNAL_API.
>
> Such a check won't catch issues when build is broken in the absence of
> those flags, which is the common case for applications consuming DPDK.
>
> This series adds more coverage.

Series applied, thanks.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 34+ messages in thread

end of thread, other threads:[~2024-10-23 14:40 UTC | newest]

Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-10-15 12:10 [PATCH 0/3] Enhance headers check David Marchand
2024-10-15 12:10 ` [PATCH 1/3] bitops: fix build for GCC without experimental API David Marchand
2024-10-15 12:47   ` Morten Brørup
2024-10-15 12:10 ` [PATCH 2/3] bitset: " David Marchand
2024-10-15 12:53   ` Morten Brørup
2024-10-15 14:13     ` Thomas Monjalon
2024-10-15 14:44       ` Morten Brørup
2024-10-15 19:58         ` Thomas Monjalon
2024-10-15 20:30           ` Morten Brørup
2024-10-15 12:10 ` [PATCH 3/3] buildtools/chkincs: check headers with stable API only David Marchand
2024-10-16 11:38 ` [PATCH v2 0/4] Enhance headers check David Marchand
2024-10-16 11:38   ` [PATCH v2 1/4] bitops: fix build for GCC without experimental API David Marchand
2024-10-16 11:38   ` [PATCH v2 2/4] bitset: " David Marchand
2024-10-16 14:14     ` Mattias Rönnblom
2024-10-16 15:36       ` David Marchand
2024-10-16 15:42         ` Morten Brørup
2024-10-16 16:03           ` Mattias Rönnblom
2024-10-16 16:17             ` Thomas Monjalon
2024-10-16 15:51         ` Mattias Rönnblom
2024-10-16 11:38   ` [PATCH v2 3/4] vhost: remove internal vDPA API description from public header David Marchand
2024-10-16 11:47     ` Maxime Coquelin
2024-10-16 11:38   ` [PATCH v2 4/4] buildtools/chkincs: check headers with stable API only David Marchand
2024-10-16 20:40   ` [PATCH v2 0/4] Enhance headers check David Marchand
2024-10-21 16:04 ` [PATCH v3 " David Marchand
2024-10-21 16:04   ` [PATCH v3 1/4] bitset: fix build for GCC without experimental API David Marchand
2024-10-21 17:57     ` Mattias Rönnblom
2024-10-21 16:04   ` [PATCH v3 2/4] buildtools/chkincs: check headers with stable API only David Marchand
2024-10-21 16:06     ` Bruce Richardson
2024-10-21 16:04   ` [PATCH v3 3/4] bus/ifpga: use C linkage where appropriate in driver header David Marchand
2024-10-21 18:01     ` Mattias Rönnblom
2024-10-22 19:27       ` David Marchand
2024-10-21 16:04   ` [PATCH v3 4/4] buildtools/chkincs: check driver specific headers David Marchand
2024-10-21 16:23   ` [PATCH v3 0/4] Enhance headers check Morten Brørup
2024-10-23 14:39   ` David Marchand

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).