[PATCH] ethdev: optimize activation of fast-path tracepoints

[PATCH] ethdev: optimize activation of fast-path tracepoints

[Adel Belkhiri]

From: Adel Belkhiri <adel.belkhiri@polymtl.ca>

Split the tracepoints rte_ethdev_trace_rx_burst and
rte_eth_trace_call_rx_callbacks into two separate ones
for empty and non-empty calls to avoid saturating
quickly the trace buffer.

Signed-off-by: Adel Belkhiri <adel.belkhiri@polymtl.ca>
---
 .mailmap                         |  1 +
 lib/ethdev/ethdev_private.c      |  8 ++++++--
 lib/ethdev/ethdev_trace_points.c | 14 ++++++++++----
 lib/ethdev/rte_ethdev.h          |  5 ++++-
 lib/ethdev/rte_ethdev_trace_fp.h | 23 +++++++++++++++++++++--
 lib/ethdev/version.map           |  3 ++-
 6 files changed, 44 insertions(+), 10 deletions(-)

diff --git a/.mailmap b/.mailmap
index 4a508bafad..e86241dced 100644
--- a/.mailmap
+++ b/.mailmap
@@ -16,6 +16,7 @@ Abraham Tovar <abrahamx.tovar@intel.com>
 Adam Bynes <adambynes@outlook.com>
 Adam Dybkowski <adamx.dybkowski@intel.com>
 Adam Ludkiewicz <adam.ludkiewicz@intel.com>
+Adel Belkhiri <adel.belkhiri@polymtl.ca>
 Adham Masarwah <adham@nvidia.com> <adham@mellanox.com>
 Adrian Moreno <amorenoz@redhat.com>
 Adrian Pielech <adrian.pielech@intel.com>
diff --git a/lib/ethdev/ethdev_private.c b/lib/ethdev/ethdev_private.c
index 626524558a..9aca7bb635 100644
--- a/lib/ethdev/ethdev_private.c
+++ b/lib/ethdev/ethdev_private.c
@@ -298,8 +298,12 @@ rte_eth_call_rx_callbacks(uint16_t port_id, uint16_t queue_id,
 		cb = cb->next;
 	}
 
-	rte_eth_trace_call_rx_callbacks(port_id, queue_id, (void **)rx_pkts,
-					nb_rx, nb_pkts);
+	if (unlikely(nb_rx > 0))
+		rte_eth_trace_call_rx_callbacks_nonempty(port_id, queue_id, (void **)rx_pkts,
+						nb_rx, nb_pkts);
+	else
+		rte_eth_trace_call_rx_callbacks_empty(port_id, queue_id, (void **)rx_pkts,
+						nb_pkts);
 
 	return nb_rx;
 }
diff --git a/lib/ethdev/ethdev_trace_points.c b/lib/ethdev/ethdev_trace_points.c
index 99e04f5893..6ecbee289b 100644
--- a/lib/ethdev/ethdev_trace_points.c
+++ b/lib/ethdev/ethdev_trace_points.c
@@ -25,14 +25,20 @@ RTE_TRACE_POINT_REGISTER(rte_ethdev_trace_stop,
 RTE_TRACE_POINT_REGISTER(rte_ethdev_trace_close,
 	lib.ethdev.close)
 
-RTE_TRACE_POINT_REGISTER(rte_ethdev_trace_rx_burst,
-	lib.ethdev.rx.burst)
+RTE_TRACE_POINT_REGISTER(rte_ethdev_trace_rx_burst_empty,
+	lib.ethdev.rx.burst.empty)
+
+RTE_TRACE_POINT_REGISTER(rte_ethdev_trace_rx_burst_nonempty,
+	lib.ethdev.rx.burst.nonempty)
 
 RTE_TRACE_POINT_REGISTER(rte_ethdev_trace_tx_burst,
 	lib.ethdev.tx.burst)
 
-RTE_TRACE_POINT_REGISTER(rte_eth_trace_call_rx_callbacks,
-	lib.ethdev.call_rx_callbacks)
+RTE_TRACE_POINT_REGISTER(rte_eth_trace_call_rx_callbacks_empty,
+	lib.ethdev.call_rx_callbacks.empty)
+
+RTE_TRACE_POINT_REGISTER(rte_eth_trace_call_rx_callbacks_nonempty,
+	lib.ethdev.call_rx_callbacks.nonempty)
 
 RTE_TRACE_POINT_REGISTER(rte_eth_trace_call_tx_callbacks,
 	lib.ethdev.call_tx_callbacks)
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 548fada1c7..e9dbbf677b 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -6132,7 +6132,10 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
 	}
 #endif
 
-	rte_ethdev_trace_rx_burst(port_id, queue_id, (void **)rx_pkts, nb_rx);
+	if (unlikely(nb_rx > 0))
+		rte_ethdev_trace_rx_burst_nonempty(port_id, queue_id, (void **)rx_pkts, nb_rx);
+	else
+		rte_ethdev_trace_rx_burst_empty(port_id, queue_id, (void **)rx_pkts);
 	return nb_rx;
 }
 
diff --git a/lib/ethdev/rte_ethdev_trace_fp.h b/lib/ethdev/rte_ethdev_trace_fp.h
index 40b6e4756b..d23865996a 100644
--- a/lib/ethdev/rte_ethdev_trace_fp.h
+++ b/lib/ethdev/rte_ethdev_trace_fp.h
@@ -18,7 +18,16 @@ extern "C" {
 #include <rte_trace_point.h>
 
 RTE_TRACE_POINT_FP(
-	rte_ethdev_trace_rx_burst,
+	rte_ethdev_trace_rx_burst_empty,
+	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t queue_id,
+		void **pkt_tbl),
+	rte_trace_point_emit_u16(port_id);
+	rte_trace_point_emit_u16(queue_id);
+	rte_trace_point_emit_ptr(pkt_tbl);
+)
+
+RTE_TRACE_POINT_FP(
+	rte_ethdev_trace_rx_burst_nonempty,
 	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t queue_id,
 		void **pkt_tbl, uint16_t nb_rx),
 	rte_trace_point_emit_u16(port_id);
@@ -38,7 +47,17 @@ RTE_TRACE_POINT_FP(
 )
 
 RTE_TRACE_POINT_FP(
-	rte_eth_trace_call_rx_callbacks,
+	rte_eth_trace_call_rx_callbacks_empty,
+	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t queue_id,
+		void **rx_pkts, uint16_t nb_pkts),
+	rte_trace_point_emit_u16(port_id);
+	rte_trace_point_emit_u16(queue_id);
+	rte_trace_point_emit_ptr(rx_pkts);
+	rte_trace_point_emit_u16(nb_pkts);
+)
+
+RTE_TRACE_POINT_FP(
+	rte_eth_trace_call_rx_callbacks_nonempty,
 	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t queue_id,
 		void **rx_pkts, uint16_t nb_rx, uint16_t nb_pkts),
 	rte_trace_point_emit_u16(port_id);
diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
index 1669055ca5..92cd4d7a2d 100644
--- a/lib/ethdev/version.map
+++ b/lib/ethdev/version.map
@@ -207,7 +207,8 @@ EXPERIMENTAL {
 	rte_flow_dev_dump;
 
 	# added in 20.05
-	__rte_ethdev_trace_rx_burst;
+	__rte_ethdev_trace_rx_burst_empty;
+	__rte_ethdev_trace_rx_burst_nonempty;
 	__rte_ethdev_trace_tx_burst;
 	rte_flow_get_aged_flows;
 
-- 
2.34.1

RE: [PATCH] ethdev: optimize activation of fast-path tracepoints

[Morten Brørup]

> +++ b/lib/ethdev/version.map
> @@ -207,7 +207,8 @@ EXPERIMENTAL {
>  	rte_flow_dev_dump;
> 
>  	# added in 20.05
> -	__rte_ethdev_trace_rx_burst;
> +	__rte_ethdev_trace_rx_burst_empty;
> +	__rte_ethdev_trace_rx_burst_nonempty;

These two should be moved down below "# added in 24.11" (which is not yet there, so should be added too).

And I don't know why __rte_eth_trace_call_rx_callbacks is not in the version.map file, but I suppose these should be added too:

+ __rte_eth_trace_call_rx_callbacks_empty
+ __rte_eth_trace_call_rx_callbacks_nonempty


Everything else looks good.

With version.map corrected,

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

Re: [PATCH] ethdev: optimize activation of fast-path tracepoints

[Jerin Jacob]

On Sat, Sep 14, 2024 at 1:59 AM Adel Belkhiri <adel.belkhiri@gmail.com> wrote:
>
> From: Adel Belkhiri <adel.belkhiri@polymtl.ca>
>
> Split the tracepoints rte_ethdev_trace_rx_burst and
> rte_eth_trace_call_rx_callbacks into two separate ones
> for empty and non-empty calls to avoid saturating
> quickly the trace buffer.
>
> Signed-off-by: Adel Belkhiri <adel.belkhiri@polymtl.ca>
> ---

> -                                       nb_rx, nb_pkts);
> +       if (unlikely(nb_rx > 0))

You may consider unlikely(nb_rx)


>         rte_flow_dev_dump;
>
>         # added in 20.05
> -       __rte_ethdev_trace_rx_burst;

Removal of a public symbol breaks the ABI. The good news is that 24.11
can break the ABI.
IMO, It is OK to break this ABI.
Also need to update "Removed items" in doc/guides/rel_notes/release_24_11.rst


> +       __rte_ethdev_trace_rx_burst_empty;
> +       __rte_ethdev_trace_rx_burst_nonempty;
>         __rte_ethdev_trace_tx_burst;
>         rte_flow_get_aged_flows;
>
> --
> 2.34.1
>

Re: [PATCH] ethdev: optimize activation of fast-path tracepoints

[David Marchand]

On Mon, Sep 16, 2024 at 10:26 AM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> >         rte_flow_dev_dump;
> >
> >         # added in 20.05
> > -       __rte_ethdev_trace_rx_burst;
>
> Removal of a public symbol breaks the ABI. The good news is that 24.11
> can break the ABI.
> IMO, It is OK to break this ABI.

This symbol is in the experimental ABI.
And in general, I think we should keep the trace symbols as experimental.


-- 
David Marchand

Re: [PATCH] ethdev: optimize activation of fast-path tracepoints

[Jerin Jacob]

On Mon, Sep 16, 2024 at 2:01 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Mon, Sep 16, 2024 at 10:26 AM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > >         rte_flow_dev_dump;
> > >
> > >         # added in 20.05
> > > -       __rte_ethdev_trace_rx_burst;
> >
> > Removal of a public symbol breaks the ABI. The good news is that 24.11
> > can break the ABI.
> > IMO, It is OK to break this ABI.
>
> This symbol is in the experimental ABI.
> And in general, I think we should keep the trace symbols as experimental.


Ack.

So I think, we need to update only  "Removed items" in
doc/guides/rel_notes/release_24_11.rst

>
>
> --
> David Marchand
>

[PATCH v3] ethdev: optimize the activation of fast-path tracepoints

[Adel Belkhiri]

From: Adel Belkhiri <adel.belkhiri@polymtl.ca>

Split the tracepoints rte_ethdev_trace_rx_burst and
rte_eth_trace_call_rx_callbacks into two separate ones
for empty and non-empty calls to avoid saturating
quickly the trace buffer.

Signed-off-by: Adel Belkhiri <adel.belkhiri@polymtl.ca>
---
 .mailmap                               |  1 +
 doc/guides/rel_notes/release_24_11.rst |  2 ++
 lib/ethdev/ethdev_private.c            |  8 ++++++--
 lib/ethdev/ethdev_trace_points.c       | 14 ++++++++++----
 lib/ethdev/rte_ethdev.h                |  5 ++++-
 lib/ethdev/rte_ethdev_trace_fp.h       | 23 +++++++++++++++++++++--
 lib/ethdev/version.map                 |  7 ++++++-
 7 files changed, 50 insertions(+), 10 deletions(-)

diff --git a/.mailmap b/.mailmap
index 4a508bafad..e86241dced 100644
--- a/.mailmap
+++ b/.mailmap
@@ -16,6 +16,7 @@ Abraham Tovar <abrahamx.tovar@intel.com>
 Adam Bynes <adambynes@outlook.com>
 Adam Dybkowski <adamx.dybkowski@intel.com>
 Adam Ludkiewicz <adam.ludkiewicz@intel.com>
+Adel Belkhiri <adel.belkhiri@polymtl.ca>
 Adham Masarwah <adham@nvidia.com> <adham@mellanox.com>
 Adrian Moreno <amorenoz@redhat.com>
 Adrian Pielech <adrian.pielech@intel.com>
diff --git a/doc/guides/rel_notes/release_24_11.rst b/doc/guides/rel_notes/release_24_11.rst
index 0ff70d9057..b7c3ac4054 100644
--- a/doc/guides/rel_notes/release_24_11.rst
+++ b/doc/guides/rel_notes/release_24_11.rst
@@ -68,6 +68,8 @@ Removed Items
    Also, make sure to start the actual text at the margin.
    =======================================================
 
+* ethdev: Removed the __rte_ethdev_trace_rx_burst symbol, as the corresponding
+  tracepoint was split into two separate ones for empty and non-empty calls.
 
 API Changes
 -----------
diff --git a/lib/ethdev/ethdev_private.c b/lib/ethdev/ethdev_private.c
index 626524558a..eed8c78747 100644
--- a/lib/ethdev/ethdev_private.c
+++ b/lib/ethdev/ethdev_private.c
@@ -298,8 +298,12 @@ rte_eth_call_rx_callbacks(uint16_t port_id, uint16_t queue_id,
 		cb = cb->next;
 	}
 
-	rte_eth_trace_call_rx_callbacks(port_id, queue_id, (void **)rx_pkts,
-					nb_rx, nb_pkts);
+	if (unlikely(nb_rx))
+		rte_eth_trace_call_rx_callbacks_nonempty(port_id, queue_id, (void **)rx_pkts,
+						nb_rx, nb_pkts);
+	else
+		rte_eth_trace_call_rx_callbacks_empty(port_id, queue_id, (void **)rx_pkts,
+						nb_pkts);
 
 	return nb_rx;
 }
diff --git a/lib/ethdev/ethdev_trace_points.c b/lib/ethdev/ethdev_trace_points.c
index 99e04f5893..6ecbee289b 100644
--- a/lib/ethdev/ethdev_trace_points.c
+++ b/lib/ethdev/ethdev_trace_points.c
@@ -25,14 +25,20 @@ RTE_TRACE_POINT_REGISTER(rte_ethdev_trace_stop,
 RTE_TRACE_POINT_REGISTER(rte_ethdev_trace_close,
 	lib.ethdev.close)
 
-RTE_TRACE_POINT_REGISTER(rte_ethdev_trace_rx_burst,
-	lib.ethdev.rx.burst)
+RTE_TRACE_POINT_REGISTER(rte_ethdev_trace_rx_burst_empty,
+	lib.ethdev.rx.burst.empty)
+
+RTE_TRACE_POINT_REGISTER(rte_ethdev_trace_rx_burst_nonempty,
+	lib.ethdev.rx.burst.nonempty)
 
 RTE_TRACE_POINT_REGISTER(rte_ethdev_trace_tx_burst,
 	lib.ethdev.tx.burst)
 
-RTE_TRACE_POINT_REGISTER(rte_eth_trace_call_rx_callbacks,
-	lib.ethdev.call_rx_callbacks)
+RTE_TRACE_POINT_REGISTER(rte_eth_trace_call_rx_callbacks_empty,
+	lib.ethdev.call_rx_callbacks.empty)
+
+RTE_TRACE_POINT_REGISTER(rte_eth_trace_call_rx_callbacks_nonempty,
+	lib.ethdev.call_rx_callbacks.nonempty)
 
 RTE_TRACE_POINT_REGISTER(rte_eth_trace_call_tx_callbacks,
 	lib.ethdev.call_tx_callbacks)
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 548fada1c7..eef254c463 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -6132,7 +6132,10 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
 	}
 #endif
 
-	rte_ethdev_trace_rx_burst(port_id, queue_id, (void **)rx_pkts, nb_rx);
+	if (unlikely(nb_rx))
+		rte_ethdev_trace_rx_burst_nonempty(port_id, queue_id, (void **)rx_pkts, nb_rx);
+	else
+		rte_ethdev_trace_rx_burst_empty(port_id, queue_id, (void **)rx_pkts);
 	return nb_rx;
 }
 
diff --git a/lib/ethdev/rte_ethdev_trace_fp.h b/lib/ethdev/rte_ethdev_trace_fp.h
index 40b6e4756b..d23865996a 100644
--- a/lib/ethdev/rte_ethdev_trace_fp.h
+++ b/lib/ethdev/rte_ethdev_trace_fp.h
@@ -18,7 +18,16 @@ extern "C" {
 #include <rte_trace_point.h>
 
 RTE_TRACE_POINT_FP(
-	rte_ethdev_trace_rx_burst,
+	rte_ethdev_trace_rx_burst_empty,
+	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t queue_id,
+		void **pkt_tbl),
+	rte_trace_point_emit_u16(port_id);
+	rte_trace_point_emit_u16(queue_id);
+	rte_trace_point_emit_ptr(pkt_tbl);
+)
+
+RTE_TRACE_POINT_FP(
+	rte_ethdev_trace_rx_burst_nonempty,
 	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t queue_id,
 		void **pkt_tbl, uint16_t nb_rx),
 	rte_trace_point_emit_u16(port_id);
@@ -38,7 +47,17 @@ RTE_TRACE_POINT_FP(
 )
 
 RTE_TRACE_POINT_FP(
-	rte_eth_trace_call_rx_callbacks,
+	rte_eth_trace_call_rx_callbacks_empty,
+	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t queue_id,
+		void **rx_pkts, uint16_t nb_pkts),
+	rte_trace_point_emit_u16(port_id);
+	rte_trace_point_emit_u16(queue_id);
+	rte_trace_point_emit_ptr(rx_pkts);
+	rte_trace_point_emit_u16(nb_pkts);
+)
+
+RTE_TRACE_POINT_FP(
+	rte_eth_trace_call_rx_callbacks_nonempty,
 	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t queue_id,
 		void **rx_pkts, uint16_t nb_rx, uint16_t nb_pkts),
 	rte_trace_point_emit_u16(port_id);
diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
index 1669055ca5..f6aca95069 100644
--- a/lib/ethdev/version.map
+++ b/lib/ethdev/version.map
@@ -207,7 +207,6 @@ EXPERIMENTAL {
 	rte_flow_dev_dump;
 
 	# added in 20.05
-	__rte_ethdev_trace_rx_burst;
 	__rte_ethdev_trace_tx_burst;
 	rte_flow_get_aged_flows;
 
@@ -325,6 +324,12 @@ EXPERIMENTAL {
 	rte_flow_template_table_resizable;
 	rte_flow_template_table_resize;
 	rte_flow_template_table_resize_complete;
+
+	# added in 24.11
+	__rte_ethdev_trace_rx_burst_empty;
+	__rte_ethdev_trace_rx_burst_nonempty;
+	__rte_eth_trace_call_rx_callbacks_empty;
+	__rte_eth_trace_call_rx_callbacks_nonempty;
 };
 
 INTERNAL {
-- 
2.34.1

Re: [PATCH v3] ethdev: optimize the activation of fast-path tracepoints

[Jerin Jacob]

On Thu, Sep 19, 2024 at 12:16 AM Adel Belkhiri <adel.belkhiri@gmail.com> wrote:
>
> From: Adel Belkhiri <adel.belkhiri@polymtl.ca>
>
> Split the tracepoints rte_ethdev_trace_rx_burst and
> rte_eth_trace_call_rx_callbacks into two separate ones
> for empty and non-empty calls to avoid saturating
> quickly the trace buffer.
>
> Signed-off-by: Adel Belkhiri <adel.belkhiri@polymtl.ca>

Acked-by: Jerin Jacob <jerinj@marvell.com>

Re: [PATCH v3] ethdev: optimize the activation of fast-path tracepoints

[Patrick Robb]

[-- Attachment #1: Type: text/plain, Size: 101 bytes --]

Recheck-request: iol-marvell-Functional

putting in a ci lab retest for the failures on this series.

[-- Attachment #2: Type: text/html, Size: 169 bytes --]

Re: [PATCH v3] ethdev: optimize the activation of fast-path tracepoints

[Ferruh Yigit]

On 9/19/2024 5:37 PM, Jerin Jacob wrote:
> On Thu, Sep 19, 2024 at 12:16 AM Adel Belkhiri <adel.belkhiri@gmail.com> wrote:
>>
>> From: Adel Belkhiri <adel.belkhiri@polymtl.ca>
>>
>> Split the tracepoints rte_ethdev_trace_rx_burst and
>> rte_eth_trace_call_rx_callbacks into two separate ones
>> for empty and non-empty calls to avoid saturating
>> quickly the trace buffer.
>>
>> Signed-off-by: Adel Belkhiri <adel.belkhiri@polymtl.ca>
> 
> Acked-by: Jerin Jacob <jerinj@marvell.com>
>

Applied to dpdk-next-net/main, thanks.

Re: [PATCH v3] ethdev: optimize the activation of fast-path tracepoints

[Adel Belkhiri]

[-- Attachment #1: Type: text/plain, Size: 703 bytes --]

Thanks to all of you. Much appreciated!

On Sat, Sep 21, 2024 at 9:07 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> On 9/19/2024 5:37 PM, Jerin Jacob wrote:
> > On Thu, Sep 19, 2024 at 12:16 AM Adel Belkhiri <adel.belkhiri@gmail.com>
> wrote:
> >>
> >> From: Adel Belkhiri <adel.belkhiri@polymtl.ca>
> >>
> >> Split the tracepoints rte_ethdev_trace_rx_burst and
> >> rte_eth_trace_call_rx_callbacks into two separate ones
> >> for empty and non-empty calls to avoid saturating
> >> quickly the trace buffer.
> >>
> >> Signed-off-by: Adel Belkhiri <adel.belkhiri@polymtl.ca>
> >
> > Acked-by: Jerin Jacob <jerinj@marvell.com>
> >
>
> Applied to dpdk-next-net/main, thanks.
>

[-- Attachment #2: Type: text/html, Size: 1338 bytes --]