DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH 0/3] eal: mark API's as stable
@ 2024-09-04 18:08 Stephen Hemminger
  2024-09-04 18:08 ` [PATCH 1/3] eal: make rte_cpu_get_intrinsics_support stable Stephen Hemminger
                   ` (5 more replies)
  0 siblings, 6 replies; 25+ messages in thread
From: Stephen Hemminger @ 2024-09-04 18:08 UTC (permalink / raw)
  To: dev; +Cc: Stephen Hemminger

The API's in ethtool from before 23.11 should be marked stable.
Should probably include the trace api's but that is more complex change.

Stephen Hemminger (3):
  eal: make rte_cpu_get_intrinsics_support stable
  eal: mark rte_lcore_register_usage_cb stable
  eal: mark rte_memzone_max_get/set stable

 lib/eal/include/generic/rte_cpuflags.h |  4 ----
 lib/eal/include/rte_lcore.h            |  4 ----
 lib/eal/include/rte_memzone.h          |  8 --------
 lib/eal/version.map                    | 10 ++++------
 4 files changed, 4 insertions(+), 22 deletions(-)

-- 
2.45.2


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

* [PATCH 1/3] eal: make rte_cpu_get_intrinsics_support stable
  2024-09-04 18:08 [PATCH 0/3] eal: mark API's as stable Stephen Hemminger
@ 2024-09-04 18:08 ` Stephen Hemminger
  2024-09-04 18:08 ` [PATCH 2/3] eal: mark rte_lcore_register_usage_cb stable Stephen Hemminger
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 25+ messages in thread
From: Stephen Hemminger @ 2024-09-04 18:08 UTC (permalink / raw)
  To: dev; +Cc: Stephen Hemminger, Tyler Retzlaff

This API was added in 20.11, after four years it should be stable.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/eal/include/generic/rte_cpuflags.h | 4 ----
 lib/eal/version.map                    | 2 +-
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/lib/eal/include/generic/rte_cpuflags.h b/lib/eal/include/generic/rte_cpuflags.h
index d35551e931..fe48d62518 100644
--- a/lib/eal/include/generic/rte_cpuflags.h
+++ b/lib/eal/include/generic/rte_cpuflags.h
@@ -29,15 +29,11 @@ struct rte_cpu_intrinsics {
 };
 
 /**
- * @warning
- * @b EXPERIMENTAL: this API may change without prior notice
- *
  * Check CPU support for various intrinsics at runtime.
  *
  * @param intrinsics
  *     Pointer to a structure to be filled.
  */
-__rte_experimental
 void
 rte_cpu_get_intrinsics_support(struct rte_cpu_intrinsics *intrinsics);
 
diff --git a/lib/eal/version.map b/lib/eal/version.map
index e3ff412683..cabe881bfe 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -23,6 +23,7 @@ DPDK_25 {
 	rte_class_unregister;
 	rte_cpu_get_flag_enabled;
 	rte_cpu_get_flag_name;
+	rte_cpu_get_intrinsics_support; # WINDOWS_NO_EXPORT
 	rte_cpu_is_supported; # WINDOWS_NO_EXPORT
 	rte_cycles_vmware_tsc_map; # WINDOWS_NO_EXPORT
 	rte_delay_us;
@@ -384,7 +385,6 @@ EXPERIMENTAL {
 
 	# added in 20.11
 	__rte_eal_trace_generic_size_t; # WINDOWS_NO_EXPORT
-	rte_cpu_get_intrinsics_support; # WINDOWS_NO_EXPORT
 
 	# added in 23.03
 	rte_lcore_register_usage_cb;
-- 
2.45.2


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

* [PATCH 2/3] eal: mark rte_lcore_register_usage_cb stable
  2024-09-04 18:08 [PATCH 0/3] eal: mark API's as stable Stephen Hemminger
  2024-09-04 18:08 ` [PATCH 1/3] eal: make rte_cpu_get_intrinsics_support stable Stephen Hemminger
@ 2024-09-04 18:08 ` Stephen Hemminger
  2024-10-04 12:56   ` Robin Jarry
  2024-09-04 18:08 ` [PATCH 3/3] eal: mark rte_memzone_max_get/set stable Stephen Hemminger
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 25+ messages in thread
From: Stephen Hemminger @ 2024-09-04 18:08 UTC (permalink / raw)
  To: dev; +Cc: Stephen Hemminger, Tyler Retzlaff

This API was added back in 23.03, can be marked stable now.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/eal/include/rte_lcore.h | 4 ----
 lib/eal/version.map         | 2 +-
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/lib/eal/include/rte_lcore.h b/lib/eal/include/rte_lcore.h
index 7deae47af3..549b9e68c5 100644
--- a/lib/eal/include/rte_lcore.h
+++ b/lib/eal/include/rte_lcore.h
@@ -359,9 +359,6 @@ struct rte_lcore_usage {
 typedef int (*rte_lcore_usage_cb)(unsigned int lcore_id, struct rte_lcore_usage *usage);
 
 /**
- * @warning
- * @b EXPERIMENTAL: this API may change without prior notice.
- *
  * Register a callback from an application to be called in rte_lcore_dump() and
  * the /eal/lcore/info telemetry endpoint handler. Applications are expected to
  * report lcore usage statistics via this callback.
@@ -373,7 +370,6 @@ typedef int (*rte_lcore_usage_cb)(unsigned int lcore_id, struct rte_lcore_usage
  * @param cb
  *   The callback function.
  */
-__rte_experimental
 void rte_lcore_register_usage_cb(rte_lcore_usage_cb cb);
 
 /**
diff --git a/lib/eal/version.map b/lib/eal/version.map
index cabe881bfe..e0fa68bbfc 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -165,6 +165,7 @@ DPDK_25 {
 	rte_lcore_iterate;
 	rte_lcore_to_cpu_id;
 	rte_lcore_to_socket_id;
+	rte_lcore_register_usage_cb;
 	rte_malloc;
 	rte_malloc_dump_heaps;
 	rte_malloc_dump_stats;
@@ -387,7 +388,6 @@ EXPERIMENTAL {
 	__rte_eal_trace_generic_size_t; # WINDOWS_NO_EXPORT
 
 	# added in 23.03
-	rte_lcore_register_usage_cb;
 	__rte_eal_trace_generic_blob;
 
 	# added in 23.07
-- 
2.45.2


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

* [PATCH 3/3] eal: mark rte_memzone_max_get/set stable
  2024-09-04 18:08 [PATCH 0/3] eal: mark API's as stable Stephen Hemminger
  2024-09-04 18:08 ` [PATCH 1/3] eal: make rte_cpu_get_intrinsics_support stable Stephen Hemminger
  2024-09-04 18:08 ` [PATCH 2/3] eal: mark rte_lcore_register_usage_cb stable Stephen Hemminger
@ 2024-09-04 18:08 ` Stephen Hemminger
  2024-09-05  6:07 ` [PATCH 0/3] eal: mark API's as stable Morten Brørup
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 25+ messages in thread
From: Stephen Hemminger @ 2024-09-04 18:08 UTC (permalink / raw)
  To: dev; +Cc: Stephen Hemminger, Anatoly Burakov, Tyler Retzlaff

These were added in 23.03

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/eal/include/rte_memzone.h | 8 --------
 lib/eal/version.map           | 6 ++----
 2 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/lib/eal/include/rte_memzone.h b/lib/eal/include/rte_memzone.h
index 931497f37c..e1563994d5 100644
--- a/lib/eal/include/rte_memzone.h
+++ b/lib/eal/include/rte_memzone.h
@@ -65,9 +65,6 @@ struct rte_memzone {
 } __rte_packed;
 
 /**
- * @warning
- * @b EXPERIMENTAL: this API may change without prior notice.
- *
  * Set the maximum number of memzones.
  *
  * This function can only be called prior to rte_eal_init().
@@ -77,13 +74,9 @@ struct rte_memzone {
  * @return
  *  0 on success, -1 otherwise.
  */
-__rte_experimental
 int rte_memzone_max_set(size_t max);
 
 /**
- * @warning
- * @b EXPERIMENTAL: this API may change without prior notice.
- *
  * Get the maximum number of memzones.
  *
  * @note: The maximum value will not change after calling rte_eal_init().
@@ -91,7 +84,6 @@ int rte_memzone_max_set(size_t max);
  * @return
  *   Maximum number of memzones.
  */
-__rte_experimental
 size_t rte_memzone_max_get(void);
 
 /**
diff --git a/lib/eal/version.map b/lib/eal/version.map
index e0fa68bbfc..c45fc55486 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -225,6 +225,8 @@ DPDK_25 {
 	rte_memzone_dump;
 	rte_memzone_free;
 	rte_memzone_lookup;
+	rte_memzone_max_get;
+	rte_memzone_max_set;
 	rte_memzone_reserve;
 	rte_memzone_reserve_aligned;
 	rte_memzone_reserve_bounded;
@@ -390,10 +392,6 @@ EXPERIMENTAL {
 	# added in 23.03
 	__rte_eal_trace_generic_blob;
 
-	# added in 23.07
-	rte_memzone_max_get;
-	rte_memzone_max_set;
-
 	# added in 24.03
 	rte_vfio_get_device_info; # WINDOWS_NO_EXPORT
 };
-- 
2.45.2


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

* RE: [PATCH 0/3] eal: mark API's as stable
  2024-09-04 18:08 [PATCH 0/3] eal: mark API's as stable Stephen Hemminger
                   ` (2 preceding siblings ...)
  2024-09-04 18:08 ` [PATCH 3/3] eal: mark rte_memzone_max_get/set stable Stephen Hemminger
@ 2024-09-05  6:07 ` Morten Brørup
  2024-09-05  7:58 ` David Marchand
  2024-10-30  9:10 ` David Marchand
  5 siblings, 0 replies; 25+ messages in thread
From: Morten Brørup @ 2024-09-05  6:07 UTC (permalink / raw)
  To: Stephen Hemminger, dev

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Wednesday, 4 September 2024 20.09
> 
> The API's in ethtool from before 23.11 should be marked stable.
> Should probably include the trace api's but that is more complex change.

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


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

* Re: [PATCH 0/3] eal: mark API's as stable
  2024-09-04 18:08 [PATCH 0/3] eal: mark API's as stable Stephen Hemminger
                   ` (3 preceding siblings ...)
  2024-09-05  6:07 ` [PATCH 0/3] eal: mark API's as stable Morten Brørup
@ 2024-09-05  7:58 ` David Marchand
  2024-09-05  8:55   ` Morten Brørup
  2024-09-06  9:34   ` Ferruh Yigit
  2024-10-30  9:10 ` David Marchand
  5 siblings, 2 replies; 25+ messages in thread
From: David Marchand @ 2024-09-05  7:58 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: dev, Jerin Jacob Kollanukkaran, Thomas Monjalon, Morten Brørup

On Wed, Sep 4, 2024 at 8:10 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> The API's in ethtool from before 23.11 should be marked stable.

EAL* ?

> Should probably include the trace api's but that is more complex change.

On the trace API itself it should be ok.
The problem is with the tracepoint variables themselves, and I don't
think we should mark them stable.


-- 
David Marchand


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

* RE: [PATCH 0/3] eal: mark API's as stable
  2024-09-05  7:58 ` David Marchand
@ 2024-09-05  8:55   ` Morten Brørup
  2024-09-05  9:03     ` David Marchand
  2024-09-06  9:34   ` Ferruh Yigit
  1 sibling, 1 reply; 25+ messages in thread
From: Morten Brørup @ 2024-09-05  8:55 UTC (permalink / raw)
  To: David Marchand, Stephen Hemminger, bruce.richardson,
	Jerin Jacob Kollanukkaran, Sunil Kumar Kori
  Cc: dev, Thomas Monjalon

> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Thursday, 5 September 2024 09.59
> 
> On Wed, Sep 4, 2024 at 8:10 PM Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> >
> > The API's in ethtool from before 23.11 should be marked stable.
> 
> EAL* ?
> 
> > Should probably include the trace api's but that is more complex change.
> 
> On the trace API itself it should be ok.

No!

Trace must remain experimental until controlled by a meson option, e.g. "enable_trace", whereby trace can be completely disabled and omitted from the compiled application/libraries/drivers at build time.

<rant>
Furthermore, I would prefer having trace as a separate library, not part of the EAL bloat.
The EAL should - as its name says - provide hardware and O/S abstractions, not a collection of features.
</rant>

> The problem is with the tracepoint variables themselves, and I don't
> think we should mark them stable.

Agree. They are only there - as a middle step - to assist generating the trace output, so I don't see any benefit in marking them stable.
We could introduce a means to mark their trace output format as stable; but only if some trace output processors actually benefit from this. And it introduces extra work for maintaining trace points and adding new trace points.


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

* Re: [PATCH 0/3] eal: mark API's as stable
  2024-09-05  8:55   ` Morten Brørup
@ 2024-09-05  9:03     ` David Marchand
  2024-09-05  9:44       ` Morten Brørup
  0 siblings, 1 reply; 25+ messages in thread
From: David Marchand @ 2024-09-05  9:03 UTC (permalink / raw)
  To: Morten Brørup
  Cc: Stephen Hemminger, bruce.richardson, Jerin Jacob Kollanukkaran,
	Sunil Kumar Kori, dev, Thomas Monjalon

On Thu, Sep 5, 2024 at 10:55 AM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: David Marchand [mailto:david.marchand@redhat.com]
> > Sent: Thursday, 5 September 2024 09.59
> >
> > On Wed, Sep 4, 2024 at 8:10 PM Stephen Hemminger
> > <stephen@networkplumber.org> wrote:
> > >
> > > The API's in ethtool from before 23.11 should be marked stable.
> >
> > EAL* ?
> >
> > > Should probably include the trace api's but that is more complex change.
> >
> > On the trace API itself it should be ok.
>
> No!

*sigh*

>
> Trace must remain experimental until controlled by a meson option, e.g. "enable_trace", whereby trace can be completely disabled and omitted from the compiled application/libraries/drivers at build time.

This seems unrelated to marking the API stable as regardless of the
API state at the moment, this code is always present.
Patches welcome if you want it stripped.


-- 
David Marchand


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

* RE: [PATCH 0/3] eal: mark API's as stable
  2024-09-05  9:03     ` David Marchand
@ 2024-09-05  9:44       ` Morten Brørup
  2024-09-05 14:01         ` Jerin Jacob
  0 siblings, 1 reply; 25+ messages in thread
From: Morten Brørup @ 2024-09-05  9:44 UTC (permalink / raw)
  To: David Marchand, Jerin Jacob Kollanukkaran
  Cc: Stephen Hemminger, bruce.richardson, Sunil Kumar Kori, dev,
	Thomas Monjalon

> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Thursday, 5 September 2024 11.03
> 
> On Thu, Sep 5, 2024 at 10:55 AM Morten Brørup <mb@smartsharesystems.com>
> wrote:
> >
> > > From: David Marchand [mailto:david.marchand@redhat.com]
> > > Sent: Thursday, 5 September 2024 09.59
> > >
> > > On Wed, Sep 4, 2024 at 8:10 PM Stephen Hemminger
> > > <stephen@networkplumber.org> wrote:
> > > >
> > > > The API's in ethtool from before 23.11 should be marked stable.
> > >
> > > EAL* ?
> > >
> > > > Should probably include the trace api's but that is more complex change.
> > >
> > > On the trace API itself it should be ok.
> >
> > No!
> 
> *sigh*
> 
> >
> > Trace must remain experimental until controlled by a meson option, e.g.
> "enable_trace", whereby trace can be completely disabled and omitted from the
> compiled application/libraries/drivers at build time.
> 
> This seems unrelated to marking the API stable as regardless of the
> API state at the moment, this code is always present.

I cannot foresee if disabling trace at build time will require changes to the trace API. So I'm being cautious here.

However, if Jerin (as author of the trace subsystem) foresees that it will be possible to disable trace at build time without affecting the trace API, I don't object to marking the trace API (or some of it) stable.

Before doing that, rte_trace_mode_get/set() and the accompanying enum rte_trace_mode should be changed to rte_trace_config_get/set() using a new struct rte_trace_config (containing the enum rte_trace_mode, and expandable with new fields as the need arises). This will prepare for e.g. tracing to other destinations than system memory, such as a remote trace collector on the network, like SYSLOG.

> Patches welcome if you want it stripped.

Don't have time myself, so I suggested it as a code challenge instead. :-)


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

* Re: [PATCH 0/3] eal: mark API's as stable
  2024-09-05  9:44       ` Morten Brørup
@ 2024-09-05 14:01         ` Jerin Jacob
  2024-09-05 14:18           ` Morten Brørup
  2024-09-06  8:54           ` Ferruh Yigit
  0 siblings, 2 replies; 25+ messages in thread
From: Jerin Jacob @ 2024-09-05 14:01 UTC (permalink / raw)
  To: Morten Brørup
  Cc: David Marchand, Jerin Jacob Kollanukkaran, Stephen Hemminger,
	bruce.richardson, Sunil Kumar Kori, dev, Thomas Monjalon

On Thu, Sep 5, 2024 at 3:14 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: David Marchand [mailto:david.marchand@redhat.com]
> > Sent: Thursday, 5 September 2024 11.03
> >
> > On Thu, Sep 5, 2024 at 10:55 AM Morten Brørup <mb@smartsharesystems.com>
> > wrote:
> > >
> > > > From: David Marchand [mailto:david.marchand@redhat.com]
> > > > Sent: Thursday, 5 September 2024 09.59
> > > >
> > > > On Wed, Sep 4, 2024 at 8:10 PM Stephen Hemminger
> > > > <stephen@networkplumber.org> wrote:
> > > > >
> > > > > The API's in ethtool from before 23.11 should be marked stable.
> > > >
> > > > EAL* ?
> > > >
> > > > > Should probably include the trace api's but that is more complex change.
> > > >
> > > > On the trace API itself it should be ok.
> > >
> > > No!
> >
> > *sigh*
> >
> > >
> > > Trace must remain experimental until controlled by a meson option, e.g.
> > "enable_trace", whereby trace can be completely disabled and omitted from the
> > compiled application/libraries/drivers at build time.
> >
> > This seems unrelated to marking the API stable as regardless of the
> > API state at the moment, this code is always present.
>
> I cannot foresee if disabling trace at build time will require changes to the trace API. So I'm being cautious here.
>
> However, if Jerin (as author of the trace subsystem) foresees that it will be possible to disable trace at build time without affecting the trace API, I don't object to marking the trace API (or some of it) stable.

I don't for foresee any ABI changes when adding disabling trace
compile time support. However, I don't understand why we need to do
that. In the sense, fast path functions are already having an option
to compile out.
Slow path functions can be disabled at runtime at the cost of 1 cycle
as instrumentation cost. Having said that, I don't have any concern
about disabling trace as an option.


>
> Before doing that, rte_trace_mode_get/set() and the accompanying enum rte_trace_mode should be changed to rte_trace_config_get/set() using a new struct rte_trace_config (containing the enum rte_trace_mode, and expandable with new fields as the need arises). This will prepare for e.g. tracing to other destinations than system memory, such as a remote trace collector on the network, like SYSLOG.
>
> > Patches welcome if you want it stripped.
>
> Don't have time myself, so I suggested it as a code challenge instead. :-)
>

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

* RE: [PATCH 0/3] eal: mark API's as stable
  2024-09-05 14:01         ` Jerin Jacob
@ 2024-09-05 14:18           ` Morten Brørup
  2024-09-08 23:58             ` Stephen Hemminger
  2024-09-06  8:54           ` Ferruh Yigit
  1 sibling, 1 reply; 25+ messages in thread
From: Morten Brørup @ 2024-09-05 14:18 UTC (permalink / raw)
  To: Jerin Jacob, David Marchand, Stephen Hemminger
  Cc: Jerin Jacob Kollanukkaran, bruce.richardson, Sunil Kumar Kori,
	dev, Thomas Monjalon

> From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> Sent: Thursday, 5 September 2024 16.02
> 
> On Thu, Sep 5, 2024 at 3:14 PM Morten Brørup <mb@smartsharesystems.com> wrote:
> >
> > > From: David Marchand [mailto:david.marchand@redhat.com]
> > > Sent: Thursday, 5 September 2024 11.03
> > >
> > > On Thu, Sep 5, 2024 at 10:55 AM Morten Brørup <mb@smartsharesystems.com>
> > > wrote:
> > > >
> > > > > From: David Marchand [mailto:david.marchand@redhat.com]
> > > > > Sent: Thursday, 5 September 2024 09.59
> > > > >
> > > > > On Wed, Sep 4, 2024 at 8:10 PM Stephen Hemminger
> > > > > <stephen@networkplumber.org> wrote:
> > > > > >
> > > > > > The API's in ethtool from before 23.11 should be marked stable.
> > > > >
> > > > > EAL* ?
> > > > >
> > > > > > Should probably include the trace api's but that is more complex
> change.
> > > > >
> > > > > On the trace API itself it should be ok.
> > > >
> > > > No!
> > >
> > > *sigh*
> > >
> > > >
> > > > Trace must remain experimental until controlled by a meson option, e.g.
> > > "enable_trace", whereby trace can be completely disabled and omitted from
> the
> > > compiled application/libraries/drivers at build time.
> > >
> > > This seems unrelated to marking the API stable as regardless of the
> > > API state at the moment, this code is always present.
> >
> > I cannot foresee if disabling trace at build time will require changes to
> the trace API. So I'm being cautious here.
> >
> > However, if Jerin (as author of the trace subsystem) foresees that it will
> be possible to disable trace at build time without affecting the trace API, I
> don't object to marking the trace API (or some of it) stable.
> 
> I don't for foresee any ABI changes when adding disabling trace
> compile time support.

Based on Jerin's feedback, I'm retracting my objection.

> However, I don't understand why we need to do
> that.

To reduce code size.
Relevant for embedded/memory-constrained systems.

> In the sense, fast path functions are already having an option
> to compile out.
> Slow path functions can be disabled at runtime at the cost of 1 cycle
> as instrumentation cost. Having said that, I don't have any concern
> about disabling trace as an option.

Great.

> 
> 
> >
> > Before doing that, rte_trace_mode_get/set() and the accompanying enum
> rte_trace_mode should be changed to rte_trace_config_get/set() using a new
> struct rte_trace_config (containing the enum rte_trace_mode, and expandable
> with new fields as the need arises). This will prepare for e.g. tracing to
> other destinations than system memory, such as a remote trace collector on the
> network, like SYSLOG.

I'm also retracting this precondition...

If the need for further trace configuration should ever arise, rte_trace_config_get/set() can be added later.
And rte_trace_mode_get/set(), if not marked as experimental anymore, will be kept for backwards compatibility.

> >
> > > Patches welcome if you want it stripped.
> >
> > Don't have time myself, so I suggested it as a code challenge instead. :-)
> >

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

* Re: [PATCH 0/3] eal: mark API's as stable
  2024-09-05 14:01         ` Jerin Jacob
  2024-09-05 14:18           ` Morten Brørup
@ 2024-09-06  8:54           ` Ferruh Yigit
  2024-09-06 10:04             ` Morten Brørup
  1 sibling, 1 reply; 25+ messages in thread
From: Ferruh Yigit @ 2024-09-06  8:54 UTC (permalink / raw)
  To: Jerin Jacob, Morten Brørup
  Cc: David Marchand, Jerin Jacob Kollanukkaran, Stephen Hemminger,
	bruce.richardson, Sunil Kumar Kori, dev, Thomas Monjalon

On 9/5/2024 3:01 PM, Jerin Jacob wrote:
> On Thu, Sep 5, 2024 at 3:14 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>>
>>> From: David Marchand [mailto:david.marchand@redhat.com]
>>> Sent: Thursday, 5 September 2024 11.03
>>>
>>> On Thu, Sep 5, 2024 at 10:55 AM Morten Brørup <mb@smartsharesystems.com>
>>> wrote:
>>>>
>>>>> From: David Marchand [mailto:david.marchand@redhat.com]
>>>>> Sent: Thursday, 5 September 2024 09.59
>>>>>
>>>>> On Wed, Sep 4, 2024 at 8:10 PM Stephen Hemminger
>>>>> <stephen@networkplumber.org> wrote:
>>>>>>
>>>>>> The API's in ethtool from before 23.11 should be marked stable.
>>>>>
>>>>> EAL* ?
>>>>>
>>>>>> Should probably include the trace api's but that is more complex change.
>>>>>
>>>>> On the trace API itself it should be ok.
>>>>
>>>> No!
>>>
>>> *sigh*
>>>
>>>>
>>>> Trace must remain experimental until controlled by a meson option, e.g.
>>> "enable_trace", whereby trace can be completely disabled and omitted from the
>>> compiled application/libraries/drivers at build time.
>>>
>>> This seems unrelated to marking the API stable as regardless of the
>>> API state at the moment, this code is always present.
>>
>> I cannot foresee if disabling trace at build time will require changes to the trace API. So I'm being cautious here.
>>
>> However, if Jerin (as author of the trace subsystem) foresees that it will be possible to disable trace at build time without affecting the trace API, I don't object to marking the trace API (or some of it) stable.
> 
> I don't for foresee any ABI changes when adding disabling trace
> compile time support. However, I don't understand why we need to do
> that. In the sense, fast path functions are already having an option
> to compile out.
> Slow path functions can be disabled at runtime at the cost of 1 cycle
> as instrumentation cost. Having said that, I don't have any concern
> about disabling trace as an option.
> 

I agree with Jerin, I don't see motivation to disable slow path traces
when they can be disabled in runtime.
And fast path traces already have compile flag to disable them.

Build time configurations in long term has problems too, so I am for not
using them unless we don't have to.

> 
>>
>> Before doing that, rte_trace_mode_get/set() and the accompanying enum rte_trace_mode should be changed to rte_trace_config_get/set() using a new struct rte_trace_config (containing the enum rte_trace_mode, and expandable with new fields as the need arises). This will prepare for e.g. tracing to other destinations than system memory, such as a remote trace collector on the network, like SYSLOG.
>>
>>> Patches welcome if you want it stripped.
>>
>> Don't have time myself, so I suggested it as a code challenge instead. :-)
>>


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

* Re: [PATCH 0/3] eal: mark API's as stable
  2024-09-05  7:58 ` David Marchand
  2024-09-05  8:55   ` Morten Brørup
@ 2024-09-06  9:34   ` Ferruh Yigit
  2024-09-06  9:48     ` David Marchand
  2024-09-06 13:11     ` Jerin Jacob
  1 sibling, 2 replies; 25+ messages in thread
From: Ferruh Yigit @ 2024-09-06  9:34 UTC (permalink / raw)
  To: David Marchand, Stephen Hemminger
  Cc: dev, Jerin Jacob Kollanukkaran, Thomas Monjalon, Morten Brørup

On 9/5/2024 8:58 AM, David Marchand wrote:
> On Wed, Sep 4, 2024 at 8:10 PM Stephen Hemminger
> <stephen@networkplumber.org> wrote:
>>
>> The API's in ethtool from before 23.11 should be marked stable.
> 
> EAL* ?
> 
>> Should probably include the trace api's but that is more complex change.
> 
> On the trace API itself it should be ok.
> The problem is with the tracepoint variables themselves, and I don't
> think we should mark them stable.
> 

We cleaned tracepoint variables from ethdev map file, why they exist for
'eal'?

I can see .map file has bunch of "__rte_eal_trace_generic_*", I think
they exists to support 'rte_eal_trace_generic_*()' APIs which can be
called from other libraries.

Do we really need them?
Why not whoever calls them directly call 'rte_trace_point_emit_*' instead?
As these rte_eal_trace_generic_*()' not used at all, I assume this is
what done already.

@Jerin,
what do think to remove 'rte_eal_trace_generic_*()' APIs, so trace
always keeps local to library, and don't bloat the eal .map file?



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

* Re: [PATCH 0/3] eal: mark API's as stable
  2024-09-06  9:34   ` Ferruh Yigit
@ 2024-09-06  9:48     ` David Marchand
  2024-09-06 11:00       ` Ferruh Yigit
  2024-09-06 13:11     ` Jerin Jacob
  1 sibling, 1 reply; 25+ messages in thread
From: David Marchand @ 2024-09-06  9:48 UTC (permalink / raw)
  To: Ferruh Yigit
  Cc: Stephen Hemminger, dev, Jerin Jacob Kollanukkaran,
	Thomas Monjalon, Morten Brørup

On Fri, Sep 6, 2024 at 11:34 AM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> > On the trace API itself it should be ok.
> > The problem is with the tracepoint variables themselves, and I don't
> > think we should mark them stable.
> >
>
> We cleaned tracepoint variables from ethdev map file, why they exist for
> 'eal'?
>
> I can see .map file has bunch of "__rte_eal_trace_generic_*", I think
> they exists to support 'rte_eal_trace_generic_*()' APIs which can be
> called from other libraries.
>
> Do we really need them?
> Why not whoever calls them directly call 'rte_trace_point_emit_*' instead?
> As these rte_eal_trace_generic_*()' not used at all, I assume this is
> what done already.
>
> @Jerin,
> what do think to remove 'rte_eal_trace_generic_*()' APIs, so trace
> always keeps local to library, and don't bloat the eal .map file?

IIRC, we still need to export them for inline helpers.


-- 
David Marchand


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

* RE: [PATCH 0/3] eal: mark API's as stable
  2024-09-06  8:54           ` Ferruh Yigit
@ 2024-09-06 10:04             ` Morten Brørup
  2024-09-06 14:12               ` Ferruh Yigit
  0 siblings, 1 reply; 25+ messages in thread
From: Morten Brørup @ 2024-09-06 10:04 UTC (permalink / raw)
  To: Ferruh Yigit, Jerin Jacob
  Cc: David Marchand, Jerin Jacob Kollanukkaran, Stephen Hemminger,
	bruce.richardson, Sunil Kumar Kori, dev, Thomas Monjalon

> From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
> Sent: Friday, 6 September 2024 10.54
> 
> On 9/5/2024 3:01 PM, Jerin Jacob wrote:
> > On Thu, Sep 5, 2024 at 3:14 PM Morten Brørup <mb@smartsharesystems.com>
> wrote:
> >>
> >>> From: David Marchand [mailto:david.marchand@redhat.com]
> >>> Sent: Thursday, 5 September 2024 11.03
> >>>
> >>> On Thu, Sep 5, 2024 at 10:55 AM Morten Brørup <mb@smartsharesystems.com>
> >>> wrote:
> >>>>
> >>>>> From: David Marchand [mailto:david.marchand@redhat.com]
> >>>>> Sent: Thursday, 5 September 2024 09.59
> >>>>>
> >>>>> On Wed, Sep 4, 2024 at 8:10 PM Stephen Hemminger
> >>>>> <stephen@networkplumber.org> wrote:
> >>>>>>
> >>>>>> The API's in ethtool from before 23.11 should be marked stable.
> >>>>>
> >>>>> EAL* ?
> >>>>>
> >>>>>> Should probably include the trace api's but that is more complex
> change.
> >>>>>
> >>>>> On the trace API itself it should be ok.
> >>>>
> >>>> No!
> >>>
> >>> *sigh*
> >>>
> >>>>
> >>>> Trace must remain experimental until controlled by a meson option, e.g.
> >>> "enable_trace", whereby trace can be completely disabled and omitted from
> the
> >>> compiled application/libraries/drivers at build time.
> >>>
> >>> This seems unrelated to marking the API stable as regardless of the
> >>> API state at the moment, this code is always present.
> >>
> >> I cannot foresee if disabling trace at build time will require changes to
> the trace API. So I'm being cautious here.
> >>
> >> However, if Jerin (as author of the trace subsystem) foresees that it will
> be possible to disable trace at build time without affecting the trace API, I
> don't object to marking the trace API (or some of it) stable.
> >
> > I don't for foresee any ABI changes when adding disabling trace
> > compile time support. However, I don't understand why we need to do
> > that. In the sense, fast path functions are already having an option
> > to compile out.
> > Slow path functions can be disabled at runtime at the cost of 1 cycle
> > as instrumentation cost. Having said that, I don't have any concern
> > about disabling trace as an option.
> >
> 
> I agree with Jerin, I don't see motivation to disable slow path traces
> when they can be disabled in runtime.
> And fast path traces already have compile flag to disable them.
> 
> Build time configurations in long term has problems too, so I am for not
> using them unless we don't have to.

For some use cases, trace is dead code, and should be omitted.
You don't want dead code in production systems.

Please remember that DPDK is also being used in highly optimized embedded systems, hardware appliances and other systems where memory is not abundant.

DPDK is not only for cloud and distros. ;-)

The CI only tests DPDK with a build time configuration expected to be usable for distros.
I'm not asking to change that.
I'm only asking for more build time configurability to support other use cases.


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

* Re: [PATCH 0/3] eal: mark API's as stable
  2024-09-06  9:48     ` David Marchand
@ 2024-09-06 11:00       ` Ferruh Yigit
  0 siblings, 0 replies; 25+ messages in thread
From: Ferruh Yigit @ 2024-09-06 11:00 UTC (permalink / raw)
  To: David Marchand
  Cc: Stephen Hemminger, dev, Jerin Jacob Kollanukkaran,
	Thomas Monjalon, Morten Brørup

On 9/6/2024 10:48 AM, David Marchand wrote:
> On Fri, Sep 6, 2024 at 11:34 AM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>>> On the trace API itself it should be ok.
>>> The problem is with the tracepoint variables themselves, and I don't
>>> think we should mark them stable.
>>>
>>
>> We cleaned tracepoint variables from ethdev map file, why they exist for
>> 'eal'?
>>
>> I can see .map file has bunch of "__rte_eal_trace_generic_*", I think
>> they exists to support 'rte_eal_trace_generic_*()' APIs which can be
>> called from other libraries.
>>
>> Do we really need them?
>> Why not whoever calls them directly call 'rte_trace_point_emit_*' instead?
>> As these rte_eal_trace_generic_*()' not used at all, I assume this is
>> what done already.
>>
>> @Jerin,
>> what do think to remove 'rte_eal_trace_generic_*()' APIs, so trace
>> always keeps local to library, and don't bloat the eal .map file?
> 
> IIRC, we still need to export them for inline helpers.
> 

As far as I can see they are only used for 'rte_eal_trace_generic_*()'
trace helper APIs, but does eal really expose these helper APIs?

Is there any other inline helpers I am missing?

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

* Re: [PATCH 0/3] eal: mark API's as stable
  2024-09-06  9:34   ` Ferruh Yigit
  2024-09-06  9:48     ` David Marchand
@ 2024-09-06 13:11     ` Jerin Jacob
  2024-09-06 14:03       ` Ferruh Yigit
  1 sibling, 1 reply; 25+ messages in thread
From: Jerin Jacob @ 2024-09-06 13:11 UTC (permalink / raw)
  To: Ferruh Yigit
  Cc: David Marchand, Stephen Hemminger, dev,
	Jerin Jacob Kollanukkaran, Thomas Monjalon, Morten Brørup

On Fri, Sep 6, 2024 at 3:04 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> On 9/5/2024 8:58 AM, David Marchand wrote:
> > On Wed, Sep 4, 2024 at 8:10 PM Stephen Hemminger
> > <stephen@networkplumber.org> wrote:
> >>
> >> The API's in ethtool from before 23.11 should be marked stable.
> >
> > EAL* ?
> >
> >> Should probably include the trace api's but that is more complex change.
> >
> > On the trace API itself it should be ok.
> > The problem is with the tracepoint variables themselves, and I don't
> > think we should mark them stable.
> >
>
> We cleaned tracepoint variables from ethdev map file, why they exist for
> 'eal'?
>
> I can see .map file has bunch of "__rte_eal_trace_generic_*", I think
> they exists to support 'rte_eal_trace_generic_*()' APIs which can be
> called from other libraries.
>
> Do we really need them?
> Why not whoever calls them directly call 'rte_trace_point_emit_*' instead?
> As these rte_eal_trace_generic_*()' not used at all, I assume this is
> what done already.
>
> @Jerin,
> what do think to remove 'rte_eal_trace_generic_*()' APIs, so trace
> always keeps local to library, and don't bloat the eal .map file?

The purpose of exposing rte_eal_trace_generic_* is that, applications
can add generic trace points
in the application.


>
>

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

* Re: [PATCH 0/3] eal: mark API's as stable
  2024-09-06 13:11     ` Jerin Jacob
@ 2024-09-06 14:03       ` Ferruh Yigit
  2024-09-09  4:46         ` Jerin Jacob
  0 siblings, 1 reply; 25+ messages in thread
From: Ferruh Yigit @ 2024-09-06 14:03 UTC (permalink / raw)
  To: Jerin Jacob
  Cc: David Marchand, Stephen Hemminger, dev,
	Jerin Jacob Kollanukkaran, Thomas Monjalon, Morten Brørup

On 9/6/2024 2:11 PM, Jerin Jacob wrote:
> On Fri, Sep 6, 2024 at 3:04 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>>
>> On 9/5/2024 8:58 AM, David Marchand wrote:
>>> On Wed, Sep 4, 2024 at 8:10 PM Stephen Hemminger
>>> <stephen@networkplumber.org> wrote:
>>>>
>>>> The API's in ethtool from before 23.11 should be marked stable.
>>>
>>> EAL* ?
>>>
>>>> Should probably include the trace api's but that is more complex change.
>>>
>>> On the trace API itself it should be ok.
>>> The problem is with the tracepoint variables themselves, and I don't
>>> think we should mark them stable.
>>>
>>
>> We cleaned tracepoint variables from ethdev map file, why they exist for
>> 'eal'?
>>
>> I can see .map file has bunch of "__rte_eal_trace_generic_*", I think
>> they exists to support 'rte_eal_trace_generic_*()' APIs which can be
>> called from other libraries.
>>
>> Do we really need them?
>> Why not whoever calls them directly call 'rte_trace_point_emit_*' instead?
>> As these rte_eal_trace_generic_*()' not used at all, I assume this is
>> what done already.
>>
>> @Jerin,
>> what do think to remove 'rte_eal_trace_generic_*()' APIs, so trace
>> always keeps local to library, and don't bloat the eal .map file?
> 
> The purpose of exposing rte_eal_trace_generic_* is that, applications
> can add generic trace points
> in the application.
> 

Can't applications use 'rte_trace_point_emit_*()' directly, as libraries
does?


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

* Re: [PATCH 0/3] eal: mark API's as stable
  2024-09-06 10:04             ` Morten Brørup
@ 2024-09-06 14:12               ` Ferruh Yigit
  2024-09-06 14:42                 ` Morten Brørup
  0 siblings, 1 reply; 25+ messages in thread
From: Ferruh Yigit @ 2024-09-06 14:12 UTC (permalink / raw)
  To: Morten Brørup, Jerin Jacob
  Cc: David Marchand, Jerin Jacob Kollanukkaran, Stephen Hemminger,
	bruce.richardson, Sunil Kumar Kori, dev, Thomas Monjalon

On 9/6/2024 11:04 AM, Morten Brørup wrote:
>> From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
>> Sent: Friday, 6 September 2024 10.54
>>
>> On 9/5/2024 3:01 PM, Jerin Jacob wrote:
>>> On Thu, Sep 5, 2024 at 3:14 PM Morten Brørup <mb@smartsharesystems.com>
>> wrote:
>>>>
>>>>> From: David Marchand [mailto:david.marchand@redhat.com]
>>>>> Sent: Thursday, 5 September 2024 11.03
>>>>>
>>>>> On Thu, Sep 5, 2024 at 10:55 AM Morten Brørup <mb@smartsharesystems.com>
>>>>> wrote:
>>>>>>
>>>>>>> From: David Marchand [mailto:david.marchand@redhat.com]
>>>>>>> Sent: Thursday, 5 September 2024 09.59
>>>>>>>
>>>>>>> On Wed, Sep 4, 2024 at 8:10 PM Stephen Hemminger
>>>>>>> <stephen@networkplumber.org> wrote:
>>>>>>>>
>>>>>>>> The API's in ethtool from before 23.11 should be marked stable.
>>>>>>>
>>>>>>> EAL* ?
>>>>>>>
>>>>>>>> Should probably include the trace api's but that is more complex
>> change.
>>>>>>>
>>>>>>> On the trace API itself it should be ok.
>>>>>>
>>>>>> No!
>>>>>
>>>>> *sigh*
>>>>>
>>>>>>
>>>>>> Trace must remain experimental until controlled by a meson option, e.g.
>>>>> "enable_trace", whereby trace can be completely disabled and omitted from
>> the
>>>>> compiled application/libraries/drivers at build time.
>>>>>
>>>>> This seems unrelated to marking the API stable as regardless of the
>>>>> API state at the moment, this code is always present.
>>>>
>>>> I cannot foresee if disabling trace at build time will require changes to
>> the trace API. So I'm being cautious here.
>>>>
>>>> However, if Jerin (as author of the trace subsystem) foresees that it will
>> be possible to disable trace at build time without affecting the trace API, I
>> don't object to marking the trace API (or some of it) stable.
>>>
>>> I don't for foresee any ABI changes when adding disabling trace
>>> compile time support. However, I don't understand why we need to do
>>> that. In the sense, fast path functions are already having an option
>>> to compile out.
>>> Slow path functions can be disabled at runtime at the cost of 1 cycle
>>> as instrumentation cost. Having said that, I don't have any concern
>>> about disabling trace as an option.
>>>
>>
>> I agree with Jerin, I don't see motivation to disable slow path traces
>> when they can be disabled in runtime.
>> And fast path traces already have compile flag to disable them.
>>
>> Build time configurations in long term has problems too, so I am for not
>> using them unless we don't have to.
> 
> For some use cases, trace is dead code, and should be omitted.
> You don't want dead code in production systems.
> 
> Please remember that DPDK is also being used in highly optimized embedded systems, hardware appliances and other systems where memory is not abundant.
> 
> DPDK is not only for cloud and distros. ;-)
> 
> The CI only tests DPDK with a build time configuration expected to be usable for distros.
> I'm not asking to change that.
> I'm only asking for more build time configurability to support other use cases.
> 

I see, but that build time configuration argument exists in multiple
aspects. And with meson switch we lean to dynamic configuration approach.

When a build time config introduced, again and again we are having cases
that specific code enabled with compile time macro broken and nobody
noticed.
Having code enabled always and configured in runtime produces more
robust deliverable.

We are aware that DPDK is used in embedded device, but they are not
mostly very resource restricted devices, is it really matter to have a
few megabytes (I didn't check but I expect this the max binary size can
increase with tracing code) larger DPDK binary, does it really makes any
difference?


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

* RE: [PATCH 0/3] eal: mark API's as stable
  2024-09-06 14:12               ` Ferruh Yigit
@ 2024-09-06 14:42                 ` Morten Brørup
  2024-09-09  4:48                   ` Jerin Jacob
  0 siblings, 1 reply; 25+ messages in thread
From: Morten Brørup @ 2024-09-06 14:42 UTC (permalink / raw)
  To: Ferruh Yigit, Jerin Jacob, David Marchand
  Cc: Jerin Jacob Kollanukkaran, Stephen Hemminger, bruce.richardson,
	Sunil Kumar Kori, dev, Thomas Monjalon

> From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
> Sent: Friday, 6 September 2024 16.12
> 
> On 9/6/2024 11:04 AM, Morten Brørup wrote:
> >> From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
> >> Sent: Friday, 6 September 2024 10.54
> >>
> >> On 9/5/2024 3:01 PM, Jerin Jacob wrote:
> >>> On Thu, Sep 5, 2024 at 3:14 PM Morten Brørup <mb@smartsharesystems.com>
> >> wrote:
> >>>>
> >>>>> From: David Marchand [mailto:david.marchand@redhat.com]
> >>>>> Sent: Thursday, 5 September 2024 11.03
> >>>>>
> >>>>> On Thu, Sep 5, 2024 at 10:55 AM Morten Brørup <mb@smartsharesystems.com>
> >>>>> wrote:
> >>>>>>
> >>>>>>> From: David Marchand [mailto:david.marchand@redhat.com]
> >>>>>>> Sent: Thursday, 5 September 2024 09.59
> >>>>>>>
> >>>>>>> On Wed, Sep 4, 2024 at 8:10 PM Stephen Hemminger
> >>>>>>> <stephen@networkplumber.org> wrote:
> >>>>>>>>
> >>>>>>>> The API's in ethtool from before 23.11 should be marked stable.
> >>>>>>>
> >>>>>>> EAL* ?
> >>>>>>>
> >>>>>>>> Should probably include the trace api's but that is more complex
> >> change.
> >>>>>>>
> >>>>>>> On the trace API itself it should be ok.
> >>>>>>
> >>>>>> No!
> >>>>>
> >>>>> *sigh*
> >>>>>
> >>>>>>
> >>>>>> Trace must remain experimental until controlled by a meson option, e.g.
> >>>>> "enable_trace", whereby trace can be completely disabled and omitted
> from
> >> the
> >>>>> compiled application/libraries/drivers at build time.
> >>>>>
> >>>>> This seems unrelated to marking the API stable as regardless of the
> >>>>> API state at the moment, this code is always present.
> >>>>
> >>>> I cannot foresee if disabling trace at build time will require changes to
> >> the trace API. So I'm being cautious here.
> >>>>
> >>>> However, if Jerin (as author of the trace subsystem) foresees that it
> will
> >> be possible to disable trace at build time without affecting the trace API,
> I
> >> don't object to marking the trace API (or some of it) stable.
> >>>
> >>> I don't for foresee any ABI changes when adding disabling trace
> >>> compile time support. However, I don't understand why we need to do
> >>> that. In the sense, fast path functions are already having an option
> >>> to compile out.
> >>> Slow path functions can be disabled at runtime at the cost of 1 cycle
> >>> as instrumentation cost. Having said that, I don't have any concern
> >>> about disabling trace as an option.
> >>>
> >>
> >> I agree with Jerin, I don't see motivation to disable slow path traces
> >> when they can be disabled in runtime.
> >> And fast path traces already have compile flag to disable them.
> >>
> >> Build time configurations in long term has problems too, so I am for not
> >> using them unless we don't have to.
> >
> > For some use cases, trace is dead code, and should be omitted.
> > You don't want dead code in production systems.
> >
> > Please remember that DPDK is also being used in highly optimized embedded
> systems, hardware appliances and other systems where memory is not abundant.
> >
> > DPDK is not only for cloud and distros. ;-)
> >
> > The CI only tests DPDK with a build time configuration expected to be usable
> for distros.
> > I'm not asking to change that.
> > I'm only asking for more build time configurability to support other use
> cases.
> >
> 
> I see, but that build time configuration argument exists in multiple
> aspects. And with meson switch we lean to dynamic configuration approach.

It can be rte_config.h instead of Meson option. Either is perfectly fine with me.

> 
> When a build time config introduced, again and again we are having cases
> that specific code enabled with compile time macro broken and nobody
> noticed.

I acknowledge this risk.

Trace doesn't interact with anything, so I consider the risk extremely low in this case.

> Having code enabled always and configured in runtime produces more
> robust deliverable.

The same can be said about the Linux kernel, but yet it is configurable.

> 
> We are aware that DPDK is used in embedded device, but they are not
> mostly very resource restricted devices, is it really matter to have a
> few megabytes (I didn't check but I expect this the max binary size can
> increase with tracing code) larger DPDK binary, does it really makes any
> difference?

Code size also affects system boot time, because those superfluous megabytes take time to decompress when booting from FLASH memory.
For reference, the size of our system image (including Linux kernel, OpenSSL, the DPDK application, webserver, GUI, CLI, SNMP and everything) is 12 MB compressed.

From a security aspect, trace also increases the attack surface and can potentially assist a hacker trying to break into a system.


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

* Re: [PATCH 0/3] eal: mark API's as stable
  2024-09-05 14:18           ` Morten Brørup
@ 2024-09-08 23:58             ` Stephen Hemminger
  0 siblings, 0 replies; 25+ messages in thread
From: Stephen Hemminger @ 2024-09-08 23:58 UTC (permalink / raw)
  To: Morten Brørup
  Cc: Jerin Jacob, David Marchand, Jerin Jacob Kollanukkaran,
	bruce.richardson, Sunil Kumar Kori, dev, Thomas Monjalon

On Thu, 5 Sep 2024 16:18:13 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:

> > From: Jerin Jacob [mailto:jerinjacobk@gmail.com]
> > Sent: Thursday, 5 September 2024 16.02
> > 
> > On Thu, Sep 5, 2024 at 3:14 PM Morten Brørup <mb@smartsharesystems.com> wrote:  
> > >  
> > > > From: David Marchand [mailto:david.marchand@redhat.com]
> > > > Sent: Thursday, 5 September 2024 11.03
> > > >
> > > > On Thu, Sep 5, 2024 at 10:55 AM Morten Brørup <mb@smartsharesystems.com>
> > > > wrote:  
> > > > >  
> > > > > > From: David Marchand [mailto:david.marchand@redhat.com]
> > > > > > Sent: Thursday, 5 September 2024 09.59
> > > > > >
> > > > > > On Wed, Sep 4, 2024 at 8:10 PM Stephen Hemminger
> > > > > > <stephen@networkplumber.org> wrote:  
> > > > > > >
> > > > > > > The API's in ethtool from before 23.11 should be marked stable.  
> > > > > >
> > > > > > EAL* ?
> > > > > >  
> > > > > > > Should probably include the trace api's but that is more complex  
> > change.  
> > > > > >
> > > > > > On the trace API itself it should be ok.  
> > > > >
> > > > > No!  
> > > >
> > > > *sigh*
> > > >  
> > > > >
> > > > > Trace must remain experimental until controlled by a meson option, e.g.  
> > > > "enable_trace", whereby trace can be completely disabled and omitted from  
> > the  
> > > > compiled application/libraries/drivers at build time.
> > > >
> > > > This seems unrelated to marking the API stable as regardless of the
> > > > API state at the moment, this code is always present.  
> > >
> > > I cannot foresee if disabling trace at build time will require changes to  
> > the trace API. So I'm being cautious here.  
> > >
> > > However, if Jerin (as author of the trace subsystem) foresees that it will  
> > be possible to disable trace at build time without affecting the trace API, I
> > don't object to marking the trace API (or some of it) stable.
> > 
> > I don't for foresee any ABI changes when adding disabling trace
> > compile time support.  
> 
> Based on Jerin's feedback, I'm retracting my objection.
> 
> > However, I don't understand why we need to do
> > that.  
> 
> To reduce code size.
> Relevant for embedded/memory-constrained systems.
> 
> > In the sense, fast path functions are already having an option
> > to compile out.
> > Slow path functions can be disabled at runtime at the cost of 1 cycle
> > as instrumentation cost. Having said that, I don't have any concern
> > about disabling trace as an option.  
> 
> Great.
> 
> > 
> >   
> > >
> > > Before doing that, rte_trace_mode_get/set() and the accompanying enum  
> > rte_trace_mode should be changed to rte_trace_config_get/set() using a new
> > struct rte_trace_config (containing the enum rte_trace_mode, and expandable
> > with new fields as the need arises). This will prepare for e.g. tracing to
> > other destinations than system memory, such as a remote trace collector on the
> > network, like SYSLOG.  
> 
> I'm also retracting this precondition...
> 
> If the need for further trace configuration should ever arise, rte_trace_config_get/set() can be added later.
> And rte_trace_mode_get/set(), if not marked as experimental anymore, will be kept for backwards compatibility.
> 
> > >  
> > > > Patches welcome if you want it stripped.  
> > >
> > > Don't have time myself, so I suggested it as a code challenge instead. :-)
> > >  

My feeling is that the the experimental flag is not intended as permanent "get out of ABI compatiablity"

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

* Re: [PATCH 0/3] eal: mark API's as stable
  2024-09-06 14:03       ` Ferruh Yigit
@ 2024-09-09  4:46         ` Jerin Jacob
  0 siblings, 0 replies; 25+ messages in thread
From: Jerin Jacob @ 2024-09-09  4:46 UTC (permalink / raw)
  To: Ferruh Yigit
  Cc: David Marchand, Stephen Hemminger, dev,
	Jerin Jacob Kollanukkaran, Thomas Monjalon, Morten Brørup

On Fri, Sep 6, 2024 at 7:33 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> On 9/6/2024 2:11 PM, Jerin Jacob wrote:
> > On Fri, Sep 6, 2024 at 3:04 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> >>
> >> On 9/5/2024 8:58 AM, David Marchand wrote:
> >>> On Wed, Sep 4, 2024 at 8:10 PM Stephen Hemminger
> >>> <stephen@networkplumber.org> wrote:
> >>>>
> >>>> The API's in ethtool from before 23.11 should be marked stable.
> >>>
> >>> EAL* ?
> >>>
> >>>> Should probably include the trace api's but that is more complex change.
> >>>
> >>> On the trace API itself it should be ok.
> >>> The problem is with the tracepoint variables themselves, and I don't
> >>> think we should mark them stable.
> >>>
> >>
> >> We cleaned tracepoint variables from ethdev map file, why they exist for
> >> 'eal'?
> >>
> >> I can see .map file has bunch of "__rte_eal_trace_generic_*", I think
> >> they exists to support 'rte_eal_trace_generic_*()' APIs which can be
> >> called from other libraries.
> >>
> >> Do we really need them?
> >> Why not whoever calls them directly call 'rte_trace_point_emit_*' instead?
> >> As these rte_eal_trace_generic_*()' not used at all, I assume this is
> >> what done already.
> >>
> >> @Jerin,
> >> what do think to remove 'rte_eal_trace_generic_*()' APIs, so trace
> >> always keeps local to library, and don't bloat the eal .map file?
> >
> > The purpose of exposing rte_eal_trace_generic_* is that, applications
> > can add generic trace points
> > in the application.
> >
>
> Can't applications use 'rte_trace_point_emit_*()' directly, as libraries
> does?

It is two different usages.
'rte_eal_trace_generic_ case is more like, application wants to simply
emit int via generic trace API but not add any _new_ trace point.

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

* Re: [PATCH 0/3] eal: mark API's as stable
  2024-09-06 14:42                 ` Morten Brørup
@ 2024-09-09  4:48                   ` Jerin Jacob
  0 siblings, 0 replies; 25+ messages in thread
From: Jerin Jacob @ 2024-09-09  4:48 UTC (permalink / raw)
  To: Morten Brørup
  Cc: Ferruh Yigit, David Marchand, Jerin Jacob Kollanukkaran,
	Stephen Hemminger, bruce.richardson, Sunil Kumar Kori, dev,
	Thomas Monjalon

On Fri, Sep 6, 2024 at 8:12 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
> > Sent: Friday, 6 September 2024 16.12
> >
> > On 9/6/2024 11:04 AM, Morten Brørup wrote:
> > >> From: Ferruh Yigit [mailto:ferruh.yigit@amd.com]
> > >> Sent: Friday, 6 September 2024 10.54
> > >>
> > >> On 9/5/2024 3:01 PM, Jerin Jacob wrote:
> > >>> On Thu, Sep 5, 2024 at 3:14 PM Morten Brørup <mb@smartsharesystems.com>
> > >> wrote:
> > >>>>
> > >>>>> From: David Marchand [mailto:david.marchand@redhat.com]
> > >>>>> Sent: Thursday, 5 September 2024 11.03
> > >>>>>
> > >>>>> On Thu, Sep 5, 2024 at 10:55 AM Morten Brørup <mb@smartsharesystems.com>
> > >>>>> wrote:
> > >>>>>>
> > >>>>>>> From: David Marchand [mailto:david.marchand@redhat.com]
> > >>>>>>> Sent: Thursday, 5 September 2024 09.59
> > >>>>>>>
> > >>>>>>> On Wed, Sep 4, 2024 at 8:10 PM Stephen Hemminger
> > >>>>>>> <stephen@networkplumber.org> wrote:
> > >>>>>>>>
> > >>>>>>>> The API's in ethtool from before 23.11 should be marked stable.
> > >>>>>>>
> > >>>>>>> EAL* ?
> > >>>>>>>
> > >>>>>>>> Should probably include the trace api's but that is more complex
> > >> change.
> > >>>>>>>
> > >>>>>>> On the trace API itself it should be ok.
> > >>>>>>
> > >>>>>> No!
> > >>>>>
> > >>>>> *sigh*
> > >>>>>
> > >>>>>>
> > >>>>>> Trace must remain experimental until controlled by a meson option, e.g.
> > >>>>> "enable_trace", whereby trace can be completely disabled and omitted
> > from
> > >> the
> > >>>>> compiled application/libraries/drivers at build time.
> > >>>>>
> > >>>>> This seems unrelated to marking the API stable as regardless of the
> > >>>>> API state at the moment, this code is always present.
> > >>>>
> > >>>> I cannot foresee if disabling trace at build time will require changes to
> > >> the trace API. So I'm being cautious here.
> > >>>>
> > >>>> However, if Jerin (as author of the trace subsystem) foresees that it
> > will
> > >> be possible to disable trace at build time without affecting the trace API,
> > I
> > >> don't object to marking the trace API (or some of it) stable.
> > >>>
> > >>> I don't for foresee any ABI changes when adding disabling trace
> > >>> compile time support. However, I don't understand why we need to do
> > >>> that. In the sense, fast path functions are already having an option
> > >>> to compile out.
> > >>> Slow path functions can be disabled at runtime at the cost of 1 cycle
> > >>> as instrumentation cost. Having said that, I don't have any concern
> > >>> about disabling trace as an option.
> > >>>
> > >>
> > >> I agree with Jerin, I don't see motivation to disable slow path traces
> > >> when they can be disabled in runtime.
> > >> And fast path traces already have compile flag to disable them.
> > >>
> > >> Build time configurations in long term has problems too, so I am for not
> > >> using them unless we don't have to.
> > >
> > > For some use cases, trace is dead code, and should be omitted.
> > > You don't want dead code in production systems.
> > >
> > > Please remember that DPDK is also being used in highly optimized embedded
> > systems, hardware appliances and other systems where memory is not abundant.
> > >
> > > DPDK is not only for cloud and distros. ;-)
> > >
> > > The CI only tests DPDK with a build time configuration expected to be usable
> > for distros.
> > > I'm not asking to change that.
> > > I'm only asking for more build time configurability to support other use
> > cases.
> > >
> >
> > I see, but that build time configuration argument exists in multiple
> > aspects. And with meson switch we lean to dynamic configuration approach.
>
> It can be rte_config.h instead of Meson option. Either is perfectly fine with me.
>
> >
> > When a build time config introduced, again and again we are having cases
> > that specific code enabled with compile time macro broken and nobody
> > noticed.
>
> I acknowledge this risk.


if we use

if (0)
{

}

scheme instead of #ifdef scheme.
The compiler will check this leg even it is not active.


>
> Trace doesn't interact with anything, so I consider the risk extremely low in this case.
>
> > Having code enabled always and configured in runtime produces more
> > robust deliverable.
>
> The same can be said about the Linux kernel, but yet it is configurable.
>
> >
> > We are aware that DPDK is used in embedded device, but they are not
> > mostly very resource restricted devices, is it really matter to have a
> > few megabytes (I didn't check but I expect this the max binary size can
> > increase with tracing code) larger DPDK binary, does it really makes any
> > difference?
>
> Code size also affects system boot time, because those superfluous megabytes take time to decompress when booting from FLASH memory.
> For reference, the size of our system image (including Linux kernel, OpenSSL, the DPDK application, webserver, GUI, CLI, SNMP and everything) is 12 MB compressed.
>
> From a security aspect, trace also increases the attack surface and can potentially assist a hacker trying to break into a system.
>

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

* Re: [PATCH 2/3] eal: mark rte_lcore_register_usage_cb stable
  2024-09-04 18:08 ` [PATCH 2/3] eal: mark rte_lcore_register_usage_cb stable Stephen Hemminger
@ 2024-10-04 12:56   ` Robin Jarry
  0 siblings, 0 replies; 25+ messages in thread
From: Robin Jarry @ 2024-10-04 12:56 UTC (permalink / raw)
  To: Stephen Hemminger, dev; +Cc: Tyler Retzlaff

Stephen Hemminger, Sep 04, 2024 at 20:08:
> This API was added back in 23.03, can be marked stable now.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Thanks Stephen, this is used in grout and works properly.

https://github.com/DPDK/grout/blob/v0.2/modules/infra/control/worker.c#L303-L321

Acked-by: Robin Jarry <rjarry@redhat.com>


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

* Re: [PATCH 0/3] eal: mark API's as stable
  2024-09-04 18:08 [PATCH 0/3] eal: mark API's as stable Stephen Hemminger
                   ` (4 preceding siblings ...)
  2024-09-05  7:58 ` David Marchand
@ 2024-10-30  9:10 ` David Marchand
  5 siblings, 0 replies; 25+ messages in thread
From: David Marchand @ 2024-10-30  9:10 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev, Morten Brørup, Robin Jarry

On Wed, Sep 4, 2024 at 8:10 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> The API's in ethtool from before 23.11 should be marked stable.
> Should probably include the trace api's but that is more complex change.
>
> Stephen Hemminger (3):
>   eal: make rte_cpu_get_intrinsics_support stable
>   eal: mark rte_lcore_register_usage_cb stable
>   eal: mark rte_memzone_max_get/set stable
>
>  lib/eal/include/generic/rte_cpuflags.h |  4 ----
>  lib/eal/include/rte_lcore.h            |  4 ----
>  lib/eal/include/rte_memzone.h          |  8 --------
>  lib/eal/version.map                    | 10 ++++------
>  4 files changed, 4 insertions(+), 22 deletions(-)

Series applied, thanks Stephen.


-- 
David Marchand


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

end of thread, other threads:[~2024-10-30  9:10 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-09-04 18:08 [PATCH 0/3] eal: mark API's as stable Stephen Hemminger
2024-09-04 18:08 ` [PATCH 1/3] eal: make rte_cpu_get_intrinsics_support stable Stephen Hemminger
2024-09-04 18:08 ` [PATCH 2/3] eal: mark rte_lcore_register_usage_cb stable Stephen Hemminger
2024-10-04 12:56   ` Robin Jarry
2024-09-04 18:08 ` [PATCH 3/3] eal: mark rte_memzone_max_get/set stable Stephen Hemminger
2024-09-05  6:07 ` [PATCH 0/3] eal: mark API's as stable Morten Brørup
2024-09-05  7:58 ` David Marchand
2024-09-05  8:55   ` Morten Brørup
2024-09-05  9:03     ` David Marchand
2024-09-05  9:44       ` Morten Brørup
2024-09-05 14:01         ` Jerin Jacob
2024-09-05 14:18           ` Morten Brørup
2024-09-08 23:58             ` Stephen Hemminger
2024-09-06  8:54           ` Ferruh Yigit
2024-09-06 10:04             ` Morten Brørup
2024-09-06 14:12               ` Ferruh Yigit
2024-09-06 14:42                 ` Morten Brørup
2024-09-09  4:48                   ` Jerin Jacob
2024-09-06  9:34   ` Ferruh Yigit
2024-09-06  9:48     ` David Marchand
2024-09-06 11:00       ` Ferruh Yigit
2024-09-06 13:11     ` Jerin Jacob
2024-09-06 14:03       ` Ferruh Yigit
2024-09-09  4:46         ` Jerin Jacob
2024-10-30  9:10 ` 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).