[PATCH 0/2] Fix race in ethdev telemetry

[PATCH 0/2] Fix race in ethdev telemetry

[David Marchand]

Following a discussion we had during the summit, here is one series to
fix a race between an application thread and the telemetry thread
handling requests on ethdev ports.
The problem may be generic to other device classes providing telemetry
callbacks, but for now, this series goes with a simple and naive
approach of putting locks in the ethdev layer.

Comments welcome.


-- 
David Marchand

David Marchand (2):
  ethdev: expose telemetry dump command for Windows
  ethdev: fix race on ports for telemetry commands

 lib/ethdev/rte_ethdev_telemetry.c | 105 ++++++++++++++++++------------
 1 file changed, 65 insertions(+), 40 deletions(-)

-- 
2.46.2

[PATCH 1/2] ethdev: expose telemetry dump command for Windows

[David Marchand]

A next commit will protect all telemetry commands.

Prefer exposing all commands regardless of OS, and return an error
when invoked on Windows.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/ethdev/rte_ethdev_telemetry.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/lib/ethdev/rte_ethdev_telemetry.c b/lib/ethdev/rte_ethdev_telemetry.c
index 6b873e7abe..8031a58595 100644
--- a/lib/ethdev/rte_ethdev_telemetry.c
+++ b/lib/ethdev/rte_ethdev_telemetry.c
@@ -227,7 +227,15 @@ eth_dev_handle_port_dump_priv(const char *cmd __rte_unused,
 	free(buf);
 	return 0;
 }
-#endif /* !RTE_EXEC_ENV_WINDOWS */
+#else /* !RTE_EXEC_ENV_WINDOWS */
+static int
+eth_dev_handle_port_dump_priv(const char *cmd __rte_unused,
+			const char *params __rte_unused,
+			struct rte_tel_data *d __rte_unused)
+{
+	return -EINVAL;
+}
+#endif /* RTE_EXEC_ENV_WINDOWS */
 
 static int
 eth_dev_handle_port_link_status(const char *cmd __rte_unused,
@@ -1403,10 +1411,8 @@ RTE_INIT(ethdev_init_telemetry)
 			"Returns the common stats for a port. Parameters: int port_id");
 	rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
 			"Returns the extended stats for a port. Parameters: int port_id,hide_zero=true|false(Optional for indicates hide zero xstats)");
-#ifndef RTE_EXEC_ENV_WINDOWS
 	rte_telemetry_register_cmd("/ethdev/dump_priv", eth_dev_handle_port_dump_priv,
 			"Returns dump private information for a port. Parameters: int port_id");
-#endif
 	rte_telemetry_register_cmd("/ethdev/link_status",
 			eth_dev_handle_port_link_status,
 			"Returns the link status for a port. Parameters: int port_id");
-- 
2.46.2

[PATCH 2/2] ethdev: fix race on ports for telemetry commands

[David Marchand]

While invoking telemetry commands (which may happen at any time,
out of control of the application), an application thread may
concurrently add/remove ports.
The telemetry callbacks may then access partially
initialised/uninitialised ethdev data.

Reuse the ethdev lock that protects port allocation/destruction.

Fixes: c190daedb9b1 ("ethdev: add telemetry callbacks")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/ethdev/rte_ethdev_telemetry.c | 93 +++++++++++++++++++------------
 1 file changed, 56 insertions(+), 37 deletions(-)

diff --git a/lib/ethdev/rte_ethdev_telemetry.c b/lib/ethdev/rte_ethdev_telemetry.c
index 8031a58595..7f9c924209 100644
--- a/lib/ethdev/rte_ethdev_telemetry.c
+++ b/lib/ethdev/rte_ethdev_telemetry.c
@@ -6,6 +6,7 @@
 #include <stdlib.h>
 
 #include <rte_kvargs.h>
+#include <rte_spinlock.h>
 #include <rte_telemetry.h>
 
 #include "rte_ethdev.h"
@@ -1403,43 +1404,61 @@ eth_dev_handle_port_tm_node_caps(const char *cmd __rte_unused,
 	return ret;
 }
 
+#define ETHDEV_TELEMETRY_HANDLERS \
+	ETHDEV_TELEMETRY_HANDLER("/ethdev/list", eth_dev_handle_port_list, \
+		"Returns list of available ethdev ports. Takes no parameters") \
+	ETHDEV_TELEMETRY_HANDLER("/ethdev/stats", eth_dev_handle_port_stats, \
+		"Returns the common stats for a port. Parameters: int port_id") \
+	ETHDEV_TELEMETRY_HANDLER("/ethdev/xstats", eth_dev_handle_port_xstats, \
+		"Returns the extended stats for a port. Parameters: int port_id,hide_zero=true|false(Optional for indicates hide zero xstats)") \
+	ETHDEV_TELEMETRY_HANDLER("/ethdev/dump_priv", eth_dev_handle_port_dump_priv, \
+		"Returns dump private information for a port. Parameters: int port_id") \
+	ETHDEV_TELEMETRY_HANDLER("/ethdev/link_status", eth_dev_handle_port_link_status, \
+		"Returns the link status for a port. Parameters: int port_id") \
+	ETHDEV_TELEMETRY_HANDLER("/ethdev/info", eth_dev_handle_port_info, \
+		"Returns the device info for a port. Parameters: int port_id") \
+	ETHDEV_TELEMETRY_HANDLER("/ethdev/module_eeprom", eth_dev_handle_port_module_eeprom, \
+		"Returns module EEPROM info with SFF specs. Parameters: int port_id") \
+	ETHDEV_TELEMETRY_HANDLER("/ethdev/macs", eth_dev_handle_port_macs, \
+		"Returns the MAC addresses for a port. Parameters: int port_id") \
+	ETHDEV_TELEMETRY_HANDLER("/ethdev/flow_ctrl", eth_dev_handle_port_flow_ctrl, \
+		"Returns flow ctrl info for a port. Parameters: int port_id") \
+	ETHDEV_TELEMETRY_HANDLER("/ethdev/rx_queue", eth_dev_handle_port_rxq, \
+		"Returns Rx queue info for a port. Parameters: int port_id, int queue_id (Optional if only one queue)") \
+	ETHDEV_TELEMETRY_HANDLER("/ethdev/tx_queue", eth_dev_handle_port_txq, \
+		"Returns Tx queue info for a port. Parameters: int port_id, int queue_id (Optional if only one queue)") \
+	ETHDEV_TELEMETRY_HANDLER("/ethdev/dcb", eth_dev_handle_port_dcb, \
+		"Returns DCB info for a port. Parameters: int port_id") \
+	ETHDEV_TELEMETRY_HANDLER("/ethdev/rss_info", eth_dev_handle_port_rss_info, \
+		"Returns RSS info for a port. Parameters: int port_id") \
+	ETHDEV_TELEMETRY_HANDLER("/ethdev/fec", eth_dev_handle_port_fec, \
+		"Returns FEC info for a port. Parameters: int port_id") \
+	ETHDEV_TELEMETRY_HANDLER("/ethdev/vlan", eth_dev_handle_port_vlan, \
+		"Returns VLAN info for a port. Parameters: int port_id") \
+	ETHDEV_TELEMETRY_HANDLER("/ethdev/tm_capability", eth_dev_handle_port_tm_caps, \
+		"Returns TM Capabilities info for a port. Parameters: int port_id") \
+	ETHDEV_TELEMETRY_HANDLER("/ethdev/tm_level_capability", eth_dev_handle_port_tm_level_caps, \
+		"Returns TM Level Capabilities info for a port. Parameters: int port_id, int level_id (see tm_capability for the max)") \
+	ETHDEV_TELEMETRY_HANDLER("/ethdev/tm_node_capability", eth_dev_handle_port_tm_node_caps, \
+		"Returns TM Node Capabilities info for a port. Parameters: int port_id, int node_id (see tm_capability for the max)")
+
+#define ETHDEV_TELEMETRY_HANDLER(command, func, usage) \
+static int func ## _locked(const char *cmd __rte_unused, const char *params, \
+	struct rte_tel_data *d) \
+{ \
+	int ret; \
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock()); \
+	ret = func(cmd, params, d); \
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock()); \
+	return ret; \
+}
+ETHDEV_TELEMETRY_HANDLERS
+#undef ETHDEV_TELEMETRY_HANDLER
+
 RTE_INIT(ethdev_init_telemetry)
 {
-	rte_telemetry_register_cmd("/ethdev/list", eth_dev_handle_port_list,
-			"Returns list of available ethdev ports. Takes no parameters");
-	rte_telemetry_register_cmd("/ethdev/stats", eth_dev_handle_port_stats,
-			"Returns the common stats for a port. Parameters: int port_id");
-	rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
-			"Returns the extended stats for a port. Parameters: int port_id,hide_zero=true|false(Optional for indicates hide zero xstats)");
-	rte_telemetry_register_cmd("/ethdev/dump_priv", eth_dev_handle_port_dump_priv,
-			"Returns dump private information for a port. Parameters: int port_id");
-	rte_telemetry_register_cmd("/ethdev/link_status",
-			eth_dev_handle_port_link_status,
-			"Returns the link status for a port. Parameters: int port_id");
-	rte_telemetry_register_cmd("/ethdev/info", eth_dev_handle_port_info,
-			"Returns the device info for a port. Parameters: int port_id");
-	rte_telemetry_register_cmd("/ethdev/module_eeprom", eth_dev_handle_port_module_eeprom,
-			"Returns module EEPROM info with SFF specs. Parameters: int port_id");
-	rte_telemetry_register_cmd("/ethdev/macs", eth_dev_handle_port_macs,
-			"Returns the MAC addresses for a port. Parameters: int port_id");
-	rte_telemetry_register_cmd("/ethdev/flow_ctrl", eth_dev_handle_port_flow_ctrl,
-			"Returns flow ctrl info for a port. Parameters: int port_id");
-	rte_telemetry_register_cmd("/ethdev/rx_queue", eth_dev_handle_port_rxq,
-			"Returns Rx queue info for a port. Parameters: int port_id, int queue_id (Optional if only one queue)");
-	rte_telemetry_register_cmd("/ethdev/tx_queue", eth_dev_handle_port_txq,
-			"Returns Tx queue info for a port. Parameters: int port_id, int queue_id (Optional if only one queue)");
-	rte_telemetry_register_cmd("/ethdev/dcb", eth_dev_handle_port_dcb,
-			"Returns DCB info for a port. Parameters: int port_id");
-	rte_telemetry_register_cmd("/ethdev/rss_info", eth_dev_handle_port_rss_info,
-			"Returns RSS info for a port. Parameters: int port_id");
-	rte_telemetry_register_cmd("/ethdev/fec", eth_dev_handle_port_fec,
-			"Returns FEC info for a port. Parameters: int port_id");
-	rte_telemetry_register_cmd("/ethdev/vlan", eth_dev_handle_port_vlan,
-			"Returns VLAN info for a port. Parameters: int port_id");
-	rte_telemetry_register_cmd("/ethdev/tm_capability", eth_dev_handle_port_tm_caps,
-			"Returns TM Capabilities info for a port. Parameters: int port_id");
-	rte_telemetry_register_cmd("/ethdev/tm_level_capability", eth_dev_handle_port_tm_level_caps,
-			"Returns TM Level Capabilities info for a port. Parameters: int port_id, int level_id (see tm_capability for the max)");
-	rte_telemetry_register_cmd("/ethdev/tm_node_capability", eth_dev_handle_port_tm_node_caps,
-			"Returns TM Node Capabilities info for a port. Parameters: int port_id, int node_id (see tm_capability for the max)");
+#define ETHDEV_TELEMETRY_HANDLER(command, func, usage) \
+	rte_telemetry_register_cmd(command, func ## _locked, usage);
+	ETHDEV_TELEMETRY_HANDLERS
+#undef ETHDEV_TELEMETRY_HANDLER
 }
-- 
2.46.2

Re: [PATCH 2/2] ethdev: fix race on ports for telemetry commands

[Bruce Richardson]

On Wed, Oct 02, 2024 at 05:57:08PM +0200, David Marchand wrote:
> While invoking telemetry commands (which may happen at any time,
> out of control of the application), an application thread may
> concurrently add/remove ports.
> The telemetry callbacks may then access partially
> initialised/uninitialised ethdev data.
> 
> Reuse the ethdev lock that protects port allocation/destruction.
> 
> Fixes: c190daedb9b1 ("ethdev: add telemetry callbacks")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  lib/ethdev/rte_ethdev_telemetry.c | 93 +++++++++++++++++++------------
>  1 file changed, 56 insertions(+), 37 deletions(-)
> 
> diff --git a/lib/ethdev/rte_ethdev_telemetry.c b/lib/ethdev/rte_ethdev_telemetry.c
> index 8031a58595..7f9c924209 100644
> --- a/lib/ethdev/rte_ethdev_telemetry.c
> +++ b/lib/ethdev/rte_ethdev_telemetry.c
> @@ -6,6 +6,7 @@
>  #include <stdlib.h>
>  
>  #include <rte_kvargs.h>
> +#include <rte_spinlock.h>
>  #include <rte_telemetry.h>
>  
>  #include "rte_ethdev.h"
> @@ -1403,43 +1404,61 @@ eth_dev_handle_port_tm_node_caps(const char *cmd __rte_unused,
>  	return ret;
>  }
>  
> +#define ETHDEV_TELEMETRY_HANDLERS \
> +	ETHDEV_TELEMETRY_HANDLER("/ethdev/list", eth_dev_handle_port_list, \
> +		"Returns list of available ethdev ports. Takes no parameters") \
> +	ETHDEV_TELEMETRY_HANDLER("/ethdev/stats", eth_dev_handle_port_stats, \
> +		"Returns the common stats for a port. Parameters: int port_id") \
> +	ETHDEV_TELEMETRY_HANDLER("/ethdev/xstats", eth_dev_handle_port_xstats, \
> +		"Returns the extended stats for a port. Parameters: int port_id,hide_zero=true|false(Optional for indicates hide zero xstats)") \
> +	ETHDEV_TELEMETRY_HANDLER("/ethdev/dump_priv", eth_dev_handle_port_dump_priv, \
> +		"Returns dump private information for a port. Parameters: int port_id") \
> +	ETHDEV_TELEMETRY_HANDLER("/ethdev/link_status", eth_dev_handle_port_link_status, \
> +		"Returns the link status for a port. Parameters: int port_id") \
> +	ETHDEV_TELEMETRY_HANDLER("/ethdev/info", eth_dev_handle_port_info, \
> +		"Returns the device info for a port. Parameters: int port_id") \
> +	ETHDEV_TELEMETRY_HANDLER("/ethdev/module_eeprom", eth_dev_handle_port_module_eeprom, \
> +		"Returns module EEPROM info with SFF specs. Parameters: int port_id") \
> +	ETHDEV_TELEMETRY_HANDLER("/ethdev/macs", eth_dev_handle_port_macs, \
> +		"Returns the MAC addresses for a port. Parameters: int port_id") \
> +	ETHDEV_TELEMETRY_HANDLER("/ethdev/flow_ctrl", eth_dev_handle_port_flow_ctrl, \
> +		"Returns flow ctrl info for a port. Parameters: int port_id") \
> +	ETHDEV_TELEMETRY_HANDLER("/ethdev/rx_queue", eth_dev_handle_port_rxq, \
> +		"Returns Rx queue info for a port. Parameters: int port_id, int queue_id (Optional if only one queue)") \
> +	ETHDEV_TELEMETRY_HANDLER("/ethdev/tx_queue", eth_dev_handle_port_txq, \
> +		"Returns Tx queue info for a port. Parameters: int port_id, int queue_id (Optional if only one queue)") \
> +	ETHDEV_TELEMETRY_HANDLER("/ethdev/dcb", eth_dev_handle_port_dcb, \
> +		"Returns DCB info for a port. Parameters: int port_id") \
> +	ETHDEV_TELEMETRY_HANDLER("/ethdev/rss_info", eth_dev_handle_port_rss_info, \
> +		"Returns RSS info for a port. Parameters: int port_id") \
> +	ETHDEV_TELEMETRY_HANDLER("/ethdev/fec", eth_dev_handle_port_fec, \
> +		"Returns FEC info for a port. Parameters: int port_id") \
> +	ETHDEV_TELEMETRY_HANDLER("/ethdev/vlan", eth_dev_handle_port_vlan, \
> +		"Returns VLAN info for a port. Parameters: int port_id") \
> +	ETHDEV_TELEMETRY_HANDLER("/ethdev/tm_capability", eth_dev_handle_port_tm_caps, \
> +		"Returns TM Capabilities info for a port. Parameters: int port_id") \
> +	ETHDEV_TELEMETRY_HANDLER("/ethdev/tm_level_capability", eth_dev_handle_port_tm_level_caps, \
> +		"Returns TM Level Capabilities info for a port. Parameters: int port_id, int level_id (see tm_capability for the max)") \
> +	ETHDEV_TELEMETRY_HANDLER("/ethdev/tm_node_capability", eth_dev_handle_port_tm_node_caps, \
> +		"Returns TM Node Capabilities info for a port. Parameters: int port_id, int node_id (see tm_capability for the max)")
> +
> +#define ETHDEV_TELEMETRY_HANDLER(command, func, usage) \
> +static int func ## _locked(const char *cmd __rte_unused, const char *params, \
> +	struct rte_tel_data *d) \
> +{ \
> +	int ret; \
> +	rte_spinlock_lock(rte_mcfg_ethdev_get_lock()); \
> +	ret = func(cmd, params, d); \
> +	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock()); \
> +	return ret; \
> +}
> +ETHDEV_TELEMETRY_HANDLERS
> +#undef ETHDEV_TELEMETRY_HANDLER
> +

Not really a massive fan of such use of macros in the code, since I think
it makes things obscure for the casual reader. However, I see why this
approach has been taken. I think the macro code needs some documentation
explaining why this was done this way.

>  RTE_INIT(ethdev_init_telemetry)
>  {
> -	rte_telemetry_register_cmd("/ethdev/list", eth_dev_handle_port_list,
> -			"Returns list of available ethdev ports. Takes no parameters");
> -	rte_telemetry_register_cmd("/ethdev/stats", eth_dev_handle_port_stats,
> -			"Returns the common stats for a port. Parameters: int port_id");
> -	rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
> -			"Returns the extended stats for a port. Parameters: int port_id,hide_zero=true|false(Optional for indicates hide zero xstats)");
> -	rte_telemetry_register_cmd("/ethdev/dump_priv", eth_dev_handle_port_dump_priv,
> -			"Returns dump private information for a port. Parameters: int port_id");
> -	rte_telemetry_register_cmd("/ethdev/link_status",
> -			eth_dev_handle_port_link_status,
> -			"Returns the link status for a port. Parameters: int port_id");
> -	rte_telemetry_register_cmd("/ethdev/info", eth_dev_handle_port_info,
> -			"Returns the device info for a port. Parameters: int port_id");
> -	rte_telemetry_register_cmd("/ethdev/module_eeprom", eth_dev_handle_port_module_eeprom,
> -			"Returns module EEPROM info with SFF specs. Parameters: int port_id");
> -	rte_telemetry_register_cmd("/ethdev/macs", eth_dev_handle_port_macs,
> -			"Returns the MAC addresses for a port. Parameters: int port_id");
> -	rte_telemetry_register_cmd("/ethdev/flow_ctrl", eth_dev_handle_port_flow_ctrl,
> -			"Returns flow ctrl info for a port. Parameters: int port_id");
> -	rte_telemetry_register_cmd("/ethdev/rx_queue", eth_dev_handle_port_rxq,
> -			"Returns Rx queue info for a port. Parameters: int port_id, int queue_id (Optional if only one queue)");
> -	rte_telemetry_register_cmd("/ethdev/tx_queue", eth_dev_handle_port_txq,
> -			"Returns Tx queue info for a port. Parameters: int port_id, int queue_id (Optional if only one queue)");
> -	rte_telemetry_register_cmd("/ethdev/dcb", eth_dev_handle_port_dcb,
> -			"Returns DCB info for a port. Parameters: int port_id");
> -	rte_telemetry_register_cmd("/ethdev/rss_info", eth_dev_handle_port_rss_info,
> -			"Returns RSS info for a port. Parameters: int port_id");
> -	rte_telemetry_register_cmd("/ethdev/fec", eth_dev_handle_port_fec,
> -			"Returns FEC info for a port. Parameters: int port_id");
> -	rte_telemetry_register_cmd("/ethdev/vlan", eth_dev_handle_port_vlan,
> -			"Returns VLAN info for a port. Parameters: int port_id");
> -	rte_telemetry_register_cmd("/ethdev/tm_capability", eth_dev_handle_port_tm_caps,
> -			"Returns TM Capabilities info for a port. Parameters: int port_id");
> -	rte_telemetry_register_cmd("/ethdev/tm_level_capability", eth_dev_handle_port_tm_level_caps,
> -			"Returns TM Level Capabilities info for a port. Parameters: int port_id, int level_id (see tm_capability for the max)");
> -	rte_telemetry_register_cmd("/ethdev/tm_node_capability", eth_dev_handle_port_tm_node_caps,
> -			"Returns TM Node Capabilities info for a port. Parameters: int port_id, int node_id (see tm_capability for the max)");
> +#define ETHDEV_TELEMETRY_HANDLER(command, func, usage) \
> +	rte_telemetry_register_cmd(command, func ## _locked, usage);
> +	ETHDEV_TELEMETRY_HANDLERS
> +#undef ETHDEV_TELEMETRY_HANDLER
>  }

An alternative to this macro-fu, is to just define a single ethdev
telemetry function, and within that, take the lock and then dispatch to the
appropriate subfunction based upon the actual command coming in. The
dispatch may be slightly slower due to the additional text matching (only
from byte 8 onwards, so very short strings), but I think the code could be
a simpler in C rather than in macros, and the perf impact for telemetry is
likely to be negligible, compared to the overhead of the socket I/O etc.

/Bruce

Re: [PATCH 2/2] ethdev: fix race on ports for telemetry commands

[David Marchand]

On Wed, Oct 2, 2024 at 6:27 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Wed, Oct 02, 2024 at 05:57:08PM +0200, David Marchand wrote:
> > While invoking telemetry commands (which may happen at any time,
> > out of control of the application), an application thread may
> > concurrently add/remove ports.
> > The telemetry callbacks may then access partially
> > initialised/uninitialised ethdev data.
> >
> > Reuse the ethdev lock that protects port allocation/destruction.
> >
> > Fixes: c190daedb9b1 ("ethdev: add telemetry callbacks")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> >  lib/ethdev/rte_ethdev_telemetry.c | 93 +++++++++++++++++++------------
> >  1 file changed, 56 insertions(+), 37 deletions(-)
> >
> > diff --git a/lib/ethdev/rte_ethdev_telemetry.c b/lib/ethdev/rte_ethdev_telemetry.c
> > index 8031a58595..7f9c924209 100644
> > --- a/lib/ethdev/rte_ethdev_telemetry.c
> > +++ b/lib/ethdev/rte_ethdev_telemetry.c
> > @@ -6,6 +6,7 @@
> >  #include <stdlib.h>
> >
> >  #include <rte_kvargs.h>
> > +#include <rte_spinlock.h>
> >  #include <rte_telemetry.h>
> >
> >  #include "rte_ethdev.h"
> > @@ -1403,43 +1404,61 @@ eth_dev_handle_port_tm_node_caps(const char *cmd __rte_unused,
> >       return ret;
> >  }
> >
> > +#define ETHDEV_TELEMETRY_HANDLERS \
> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/list", eth_dev_handle_port_list, \
> > +             "Returns list of available ethdev ports. Takes no parameters") \
> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/stats", eth_dev_handle_port_stats, \
> > +             "Returns the common stats for a port. Parameters: int port_id") \
> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/xstats", eth_dev_handle_port_xstats, \
> > +             "Returns the extended stats for a port. Parameters: int port_id,hide_zero=true|false(Optional for indicates hide zero xstats)") \
> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/dump_priv", eth_dev_handle_port_dump_priv, \
> > +             "Returns dump private information for a port. Parameters: int port_id") \
> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/link_status", eth_dev_handle_port_link_status, \
> > +             "Returns the link status for a port. Parameters: int port_id") \
> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/info", eth_dev_handle_port_info, \
> > +             "Returns the device info for a port. Parameters: int port_id") \
> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/module_eeprom", eth_dev_handle_port_module_eeprom, \
> > +             "Returns module EEPROM info with SFF specs. Parameters: int port_id") \
> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/macs", eth_dev_handle_port_macs, \
> > +             "Returns the MAC addresses for a port. Parameters: int port_id") \
> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/flow_ctrl", eth_dev_handle_port_flow_ctrl, \
> > +             "Returns flow ctrl info for a port. Parameters: int port_id") \
> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/rx_queue", eth_dev_handle_port_rxq, \
> > +             "Returns Rx queue info for a port. Parameters: int port_id, int queue_id (Optional if only one queue)") \
> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/tx_queue", eth_dev_handle_port_txq, \
> > +             "Returns Tx queue info for a port. Parameters: int port_id, int queue_id (Optional if only one queue)") \
> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/dcb", eth_dev_handle_port_dcb, \
> > +             "Returns DCB info for a port. Parameters: int port_id") \
> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/rss_info", eth_dev_handle_port_rss_info, \
> > +             "Returns RSS info for a port. Parameters: int port_id") \
> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/fec", eth_dev_handle_port_fec, \
> > +             "Returns FEC info for a port. Parameters: int port_id") \
> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/vlan", eth_dev_handle_port_vlan, \
> > +             "Returns VLAN info for a port. Parameters: int port_id") \
> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/tm_capability", eth_dev_handle_port_tm_caps, \
> > +             "Returns TM Capabilities info for a port. Parameters: int port_id") \
> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/tm_level_capability", eth_dev_handle_port_tm_level_caps, \
> > +             "Returns TM Level Capabilities info for a port. Parameters: int port_id, int level_id (see tm_capability for the max)") \
> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/tm_node_capability", eth_dev_handle_port_tm_node_caps, \
> > +             "Returns TM Node Capabilities info for a port. Parameters: int port_id, int node_id (see tm_capability for the max)")
> > +
> > +#define ETHDEV_TELEMETRY_HANDLER(command, func, usage) \
> > +static int func ## _locked(const char *cmd __rte_unused, const char *params, \
> > +     struct rte_tel_data *d) \
> > +{ \
> > +     int ret; \
> > +     rte_spinlock_lock(rte_mcfg_ethdev_get_lock()); \
> > +     ret = func(cmd, params, d); \
> > +     rte_spinlock_unlock(rte_mcfg_ethdev_get_lock()); \
> > +     return ret; \
> > +}
> > +ETHDEV_TELEMETRY_HANDLERS
> > +#undef ETHDEV_TELEMETRY_HANDLER
> > +
>
> Not really a massive fan of such use of macros in the code, since I think
> it makes things obscure for the casual reader. However, I see why this
> approach has been taken. I think the macro code needs some documentation
> explaining why this was done this way.

I can add comments explaining how this is for protecting accesses to port.

>
> >  RTE_INIT(ethdev_init_telemetry)
> >  {
> > -     rte_telemetry_register_cmd("/ethdev/list", eth_dev_handle_port_list,
> > -                     "Returns list of available ethdev ports. Takes no parameters");
> > -     rte_telemetry_register_cmd("/ethdev/stats", eth_dev_handle_port_stats,
> > -                     "Returns the common stats for a port. Parameters: int port_id");
> > -     rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
> > -                     "Returns the extended stats for a port. Parameters: int port_id,hide_zero=true|false(Optional for indicates hide zero xstats)");
> > -     rte_telemetry_register_cmd("/ethdev/dump_priv", eth_dev_handle_port_dump_priv,
> > -                     "Returns dump private information for a port. Parameters: int port_id");
> > -     rte_telemetry_register_cmd("/ethdev/link_status",
> > -                     eth_dev_handle_port_link_status,
> > -                     "Returns the link status for a port. Parameters: int port_id");
> > -     rte_telemetry_register_cmd("/ethdev/info", eth_dev_handle_port_info,
> > -                     "Returns the device info for a port. Parameters: int port_id");
> > -     rte_telemetry_register_cmd("/ethdev/module_eeprom", eth_dev_handle_port_module_eeprom,
> > -                     "Returns module EEPROM info with SFF specs. Parameters: int port_id");
> > -     rte_telemetry_register_cmd("/ethdev/macs", eth_dev_handle_port_macs,
> > -                     "Returns the MAC addresses for a port. Parameters: int port_id");
> > -     rte_telemetry_register_cmd("/ethdev/flow_ctrl", eth_dev_handle_port_flow_ctrl,
> > -                     "Returns flow ctrl info for a port. Parameters: int port_id");
> > -     rte_telemetry_register_cmd("/ethdev/rx_queue", eth_dev_handle_port_rxq,
> > -                     "Returns Rx queue info for a port. Parameters: int port_id, int queue_id (Optional if only one queue)");
> > -     rte_telemetry_register_cmd("/ethdev/tx_queue", eth_dev_handle_port_txq,
> > -                     "Returns Tx queue info for a port. Parameters: int port_id, int queue_id (Optional if only one queue)");
> > -     rte_telemetry_register_cmd("/ethdev/dcb", eth_dev_handle_port_dcb,
> > -                     "Returns DCB info for a port. Parameters: int port_id");
> > -     rte_telemetry_register_cmd("/ethdev/rss_info", eth_dev_handle_port_rss_info,
> > -                     "Returns RSS info for a port. Parameters: int port_id");
> > -     rte_telemetry_register_cmd("/ethdev/fec", eth_dev_handle_port_fec,
> > -                     "Returns FEC info for a port. Parameters: int port_id");
> > -     rte_telemetry_register_cmd("/ethdev/vlan", eth_dev_handle_port_vlan,
> > -                     "Returns VLAN info for a port. Parameters: int port_id");
> > -     rte_telemetry_register_cmd("/ethdev/tm_capability", eth_dev_handle_port_tm_caps,
> > -                     "Returns TM Capabilities info for a port. Parameters: int port_id");
> > -     rte_telemetry_register_cmd("/ethdev/tm_level_capability", eth_dev_handle_port_tm_level_caps,
> > -                     "Returns TM Level Capabilities info for a port. Parameters: int port_id, int level_id (see tm_capability for the max)");
> > -     rte_telemetry_register_cmd("/ethdev/tm_node_capability", eth_dev_handle_port_tm_node_caps,
> > -                     "Returns TM Node Capabilities info for a port. Parameters: int port_id, int node_id (see tm_capability for the max)");
> > +#define ETHDEV_TELEMETRY_HANDLER(command, func, usage) \
> > +     rte_telemetry_register_cmd(command, func ## _locked, usage);
> > +     ETHDEV_TELEMETRY_HANDLERS
> > +#undef ETHDEV_TELEMETRY_HANDLER
> >  }
>
> An alternative to this macro-fu, is to just define a single ethdev
> telemetry function, and within that, take the lock and then dispatch to the
> appropriate subfunction based upon the actual command coming in. The
> dispatch may be slightly slower due to the additional text matching (only
> from byte 8 onwards, so very short strings), but I think the code could be
> a simpler in C rather than in macros, and the perf impact for telemetry is
> likely to be negligible, compared to the overhead of the socket I/O etc.

Hopefully, dispatching performance is not important here.

(skipping the byte 8 stuff) is your proposal something like:

static int
one_callback_to_rule_them_all(const char *cmd, const char *params,
struct rte_tel_data *d)
{
       telemetry_cb cb = NULL;
       int ret = -EINVAL;

       if (strcmp(cmd, "/ethdev/list") == 0) { cb = eth_dev_handle_port_list; }
       else if (strcmp(cmd, "/ethdev/stats") == 0) { cb =
eth_dev_handle_port_stats; }
       else if (strcmp(cmd, "/ethdev/xstats") == 0) { cb =
eth_dev_handle_port_xstats; }
...
       if (cb != NULL) {
               rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
               ret = cb(cmd, params, d);
               rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
       }

       return ret;
}

RTE_INIT(ethdev_init_telemetry)
{

       rte_telemetry_register_cmd("/ethdev/list", one_callback_to_rule_them_all,
                       "Returns list of available ethdev ports. Takes
no parameters");
       rte_telemetry_register_cmd("/ethdev/stats",
one_callback_to_rule_them_all,
                       "Returns the common stats for a port.
Parameters: int port_id");
       rte_telemetry_register_cmd("/ethdev/xstats",
one_callback_to_rule_them_all,
                       "Returns the extended stats for a port.
Parameters: int port_id,hide_zero=true|false(Optional for indicates
hide zero xstats)");
...

Which I find inelegant and not that great for maintenance: having the
same info (especially strings that are only evaluated at runtime) in
two locations is a call to inadvertence bugs.

The macros I propose are a way to avoid splitting the callback
function and the command names.
Then addition of a handler is tied with a single declaration.

+       ETHDEV_TELEMETRY_HANDLER("/ethdev/newstuff", eth_dev_new_handler, \
+               "New shiny command. Takes no parameters") \


-- 
David Marchand

Re: [PATCH 2/2] ethdev: fix race on ports for telemetry commands

[Robin Jarry]

David Marchand, Oct 02, 2024 at 21:06:
> On Wed, Oct 2, 2024 at 6:27 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
>>
>> On Wed, Oct 02, 2024 at 05:57:08PM +0200, David Marchand wrote:
>> > While invoking telemetry commands (which may happen at any time,
>> > out of control of the application), an application thread may
>> > concurrently add/remove ports.
>> > The telemetry callbacks may then access partially
>> > initialised/uninitialised ethdev data.
>> >
>> > Reuse the ethdev lock that protects port allocation/destruction.
>> >
>> > Fixes: c190daedb9b1 ("ethdev: add telemetry callbacks")
>> > Cc: stable@dpdk.org
>> >
>> > Signed-off-by: David Marchand <david.marchand@redhat.com>
>> > ---
>> >  lib/ethdev/rte_ethdev_telemetry.c | 93 +++++++++++++++++++------------
>> >  1 file changed, 56 insertions(+), 37 deletions(-)
>> >
>> > diff --git a/lib/ethdev/rte_ethdev_telemetry.c b/lib/ethdev/rte_ethdev_telemetry.c
>> > index 8031a58595..7f9c924209 100644
>> > --- a/lib/ethdev/rte_ethdev_telemetry.c
>> > +++ b/lib/ethdev/rte_ethdev_telemetry.c
>> > @@ -6,6 +6,7 @@
>> >  #include <stdlib.h>
>> >
>> >  #include <rte_kvargs.h>
>> > +#include <rte_spinlock.h>
>> >  #include <rte_telemetry.h>
>> >
>> >  #include "rte_ethdev.h"
>> > @@ -1403,43 +1404,61 @@ eth_dev_handle_port_tm_node_caps(const char *cmd __rte_unused,
>> >       return ret;
>> >  }
>> >
>> > +#define ETHDEV_TELEMETRY_HANDLERS \
>> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/list", eth_dev_handle_port_list, \
>> > +             "Returns list of available ethdev ports. Takes no parameters") \
>> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/stats", eth_dev_handle_port_stats, \
>> > +             "Returns the common stats for a port. Parameters: int port_id") \
>> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/xstats", eth_dev_handle_port_xstats, \
>> > +             "Returns the extended stats for a port. Parameters: int port_id,hide_zero=true|false(Optional for indicates hide zero xstats)") \
>> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/dump_priv", eth_dev_handle_port_dump_priv, \
>> > +             "Returns dump private information for a port. Parameters: int port_id") \
>> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/link_status", eth_dev_handle_port_link_status, \
>> > +             "Returns the link status for a port. Parameters: int port_id") \
>> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/info", eth_dev_handle_port_info, \
>> > +             "Returns the device info for a port. Parameters: int port_id") \
>> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/module_eeprom", eth_dev_handle_port_module_eeprom, \
>> > +             "Returns module EEPROM info with SFF specs. Parameters: int port_id") \
>> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/macs", eth_dev_handle_port_macs, \
>> > +             "Returns the MAC addresses for a port. Parameters: int port_id") \
>> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/flow_ctrl", eth_dev_handle_port_flow_ctrl, \
>> > +             "Returns flow ctrl info for a port. Parameters: int port_id") \
>> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/rx_queue", eth_dev_handle_port_rxq, \
>> > +             "Returns Rx queue info for a port. Parameters: int port_id, int queue_id (Optional if only one queue)") \
>> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/tx_queue", eth_dev_handle_port_txq, \
>> > +             "Returns Tx queue info for a port. Parameters: int port_id, int queue_id (Optional if only one queue)") \
>> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/dcb", eth_dev_handle_port_dcb, \
>> > +             "Returns DCB info for a port. Parameters: int port_id") \
>> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/rss_info", eth_dev_handle_port_rss_info, \
>> > +             "Returns RSS info for a port. Parameters: int port_id") \
>> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/fec", eth_dev_handle_port_fec, \
>> > +             "Returns FEC info for a port. Parameters: int port_id") \
>> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/vlan", eth_dev_handle_port_vlan, \
>> > +             "Returns VLAN info for a port. Parameters: int port_id") \
>> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/tm_capability", eth_dev_handle_port_tm_caps, \
>> > +             "Returns TM Capabilities info for a port. Parameters: int port_id") \
>> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/tm_level_capability", eth_dev_handle_port_tm_level_caps, \
>> > +             "Returns TM Level Capabilities info for a port. Parameters: int port_id, int level_id (see tm_capability for the max)") \
>> > +     ETHDEV_TELEMETRY_HANDLER("/ethdev/tm_node_capability", eth_dev_handle_port_tm_node_caps, \
>> > +             "Returns TM Node Capabilities info for a port. Parameters: int port_id, int node_id (see tm_capability for the max)")
>> > +
>> > +#define ETHDEV_TELEMETRY_HANDLER(command, func, usage) \
>> > +static int func ## _locked(const char *cmd __rte_unused, const char *params, \
>> > +     struct rte_tel_data *d) \
>> > +{ \
>> > +     int ret; \
>> > +     rte_spinlock_lock(rte_mcfg_ethdev_get_lock()); \
>> > +     ret = func(cmd, params, d); \
>> > +     rte_spinlock_unlock(rte_mcfg_ethdev_get_lock()); \
>> > +     return ret; \
>> > +}
>> > +ETHDEV_TELEMETRY_HANDLERS
>> > +#undef ETHDEV_TELEMETRY_HANDLER
>> > +
>>
>> Not really a massive fan of such use of macros in the code, since I think
>> it makes things obscure for the casual reader. However, I see why this
>> approach has been taken. I think the macro code needs some documentation
>> explaining why this was done this way.
>
> I can add comments explaining how this is for protecting accesses to port.
>
>>
>> >  RTE_INIT(ethdev_init_telemetry)
>> >  {
>> > -     rte_telemetry_register_cmd("/ethdev/list", eth_dev_handle_port_list,
>> > -                     "Returns list of available ethdev ports. Takes no parameters");
>> > -     rte_telemetry_register_cmd("/ethdev/stats", eth_dev_handle_port_stats,
>> > -                     "Returns the common stats for a port. Parameters: int port_id");
>> > -     rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
>> > -                     "Returns the extended stats for a port. Parameters: int port_id,hide_zero=true|false(Optional for indicates hide zero xstats)");
>> > -     rte_telemetry_register_cmd("/ethdev/dump_priv", eth_dev_handle_port_dump_priv,
>> > -                     "Returns dump private information for a port. Parameters: int port_id");
>> > -     rte_telemetry_register_cmd("/ethdev/link_status",
>> > -                     eth_dev_handle_port_link_status,
>> > -                     "Returns the link status for a port. Parameters: int port_id");
>> > -     rte_telemetry_register_cmd("/ethdev/info", eth_dev_handle_port_info,
>> > -                     "Returns the device info for a port. Parameters: int port_id");
>> > -     rte_telemetry_register_cmd("/ethdev/module_eeprom", eth_dev_handle_port_module_eeprom,
>> > -                     "Returns module EEPROM info with SFF specs. Parameters: int port_id");
>> > -     rte_telemetry_register_cmd("/ethdev/macs", eth_dev_handle_port_macs,
>> > -                     "Returns the MAC addresses for a port. Parameters: int port_id");
>> > -     rte_telemetry_register_cmd("/ethdev/flow_ctrl", eth_dev_handle_port_flow_ctrl,
>> > -                     "Returns flow ctrl info for a port. Parameters: int port_id");
>> > -     rte_telemetry_register_cmd("/ethdev/rx_queue", eth_dev_handle_port_rxq,
>> > -                     "Returns Rx queue info for a port. Parameters: int port_id, int queue_id (Optional if only one queue)");
>> > -     rte_telemetry_register_cmd("/ethdev/tx_queue", eth_dev_handle_port_txq,
>> > -                     "Returns Tx queue info for a port. Parameters: int port_id, int queue_id (Optional if only one queue)");
>> > -     rte_telemetry_register_cmd("/ethdev/dcb", eth_dev_handle_port_dcb,
>> > -                     "Returns DCB info for a port. Parameters: int port_id");
>> > -     rte_telemetry_register_cmd("/ethdev/rss_info", eth_dev_handle_port_rss_info,
>> > -                     "Returns RSS info for a port. Parameters: int port_id");
>> > -     rte_telemetry_register_cmd("/ethdev/fec", eth_dev_handle_port_fec,
>> > -                     "Returns FEC info for a port. Parameters: int port_id");
>> > -     rte_telemetry_register_cmd("/ethdev/vlan", eth_dev_handle_port_vlan,
>> > -                     "Returns VLAN info for a port. Parameters: int port_id");
>> > -     rte_telemetry_register_cmd("/ethdev/tm_capability", eth_dev_handle_port_tm_caps,
>> > -                     "Returns TM Capabilities info for a port. Parameters: int port_id");
>> > -     rte_telemetry_register_cmd("/ethdev/tm_level_capability", eth_dev_handle_port_tm_level_caps,
>> > -                     "Returns TM Level Capabilities info for a port. Parameters: int port_id, int level_id (see tm_capability for the max)");
>> > -     rte_telemetry_register_cmd("/ethdev/tm_node_capability", eth_dev_handle_port_tm_node_caps,
>> > -                     "Returns TM Node Capabilities info for a port. Parameters: int port_id, int node_id (see tm_capability for the max)");
>> > +#define ETHDEV_TELEMETRY_HANDLER(command, func, usage) \
>> > +     rte_telemetry_register_cmd(command, func ## _locked, usage);
>> > +     ETHDEV_TELEMETRY_HANDLERS
>> > +#undef ETHDEV_TELEMETRY_HANDLER
>> >  }
>>
>> An alternative to this macro-fu, is to just define a single ethdev
>> telemetry function, and within that, take the lock and then dispatch to the
>> appropriate subfunction based upon the actual command coming in. The
>> dispatch may be slightly slower due to the additional text matching (only
>> from byte 8 onwards, so very short strings), but I think the code could be
>> a simpler in C rather than in macros, and the perf impact for telemetry is
>> likely to be negligible, compared to the overhead of the socket I/O etc.
>
> Hopefully, dispatching performance is not important here.

I was going to suggest adding a rte_spinlock_t* parameter to a new 
telemetry register function that would need to be held while the 
callback is invoked. Or if we want to keep doors open to other kinds of 
lock, a wrapper callback.

Thoughts?

Re: [PATCH 2/2] ethdev: fix race on ports for telemetry commands

[David Marchand]

On Wed, Oct 2, 2024 at 9:09 PM Robin Jarry <rjarry@redhat.com> wrote:
> >> An alternative to this macro-fu, is to just define a single ethdev
> >> telemetry function, and within that, take the lock and then dispatch to the
> >> appropriate subfunction based upon the actual command coming in. The
> >> dispatch may be slightly slower due to the additional text matching (only
> >> from byte 8 onwards, so very short strings), but I think the code could be
> >> a simpler in C rather than in macros, and the perf impact for telemetry is
> >> likely to be negligible, compared to the overhead of the socket I/O etc.
> >
> > Hopefully, dispatching performance is not important here.
>
> I was going to suggest adding a rte_spinlock_t* parameter to a new
> telemetry register function that would need to be held while the
> callback is invoked. Or if we want to keep doors open to other kinds of
> lock, a wrapper callback.

Well, as you had experimented this approach, we know this does not
work: the ethdev lock is in dpdk shared memory which is not available
yet at the time RTE_INIT() is called.

A single callback is strange, I guess you mean pre/post callbacks then.


-- 
David Marchand

Re: [PATCH 2/2] ethdev: fix race on ports for telemetry commands

[Robin Jarry]

David Marchand, Oct 02, 2024 at 21:18:
> On Wed, Oct 2, 2024 at 9:09 PM Robin Jarry <rjarry@redhat.com> wrote:
>> I was going to suggest adding a rte_spinlock_t* parameter to a new
>> telemetry register function that would need to be held while the
>> callback is invoked. Or if we want to keep doors open to other kinds of
>> lock, a wrapper callback.
>
> Well, as you had experimented this approach, we know this does not
> work: the ethdev lock is in dpdk shared memory which is not available
> yet at the time RTE_INIT() is called.
>
> A single callback is strange, I guess you mean pre/post callbacks then.

It could be a single function that will wrap the callbacks. E.g.:

static int
eth_dev_telemetry_with_lock(
    telemetry_cb fn, const char *cmd, const char *params, struct rte_tel_data *d)
{
    int ret;
    rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
    ret = fn(cmd, params, d);
    rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
    return ret;
}

RTE_INIT(ethdev_init_telemetry)
{
    ....
    rte_telemetry_register_cmd("/ethdev/stats", eth_dev_handle_port_stats,
            "Returns the common stats for a port. Parameters: int port_id",
            eth_dev_telemetry_with_lock);
    ....
}

I'm not sure which solution is the uglier :D

Re: [PATCH 2/2] ethdev: fix race on ports for telemetry commands

[Bruce Richardson]

On Wed, Oct 02, 2024 at 09:26:10PM +0200, Robin Jarry wrote:
> David Marchand, Oct 02, 2024 at 21:18:
> > On Wed, Oct 2, 2024 at 9:09 PM Robin Jarry <rjarry@redhat.com> wrote:
> > > I was going to suggest adding a rte_spinlock_t* parameter to a new
> > > telemetry register function that would need to be held while the
> > > callback is invoked. Or if we want to keep doors open to other kinds of
> > > lock, a wrapper callback.
> > 
> > Well, as you had experimented this approach, we know this does not
> > work: the ethdev lock is in dpdk shared memory which is not available
> > yet at the time RTE_INIT() is called.
> > 
> > A single callback is strange, I guess you mean pre/post callbacks then.
> 
> It could be a single function that will wrap the callbacks. E.g.:
> 
> static int
> eth_dev_telemetry_with_lock(
>    telemetry_cb fn, const char *cmd, const char *params, struct rte_tel_data *d)
> {
>    int ret;
>    rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
>    ret = fn(cmd, params, d);
>    rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
>    return ret;
> }
> 
> RTE_INIT(ethdev_init_telemetry)
> {
>    ....
>    rte_telemetry_register_cmd("/ethdev/stats", eth_dev_handle_port_stats,
>            "Returns the common stats for a port. Parameters: int port_id",
>            eth_dev_telemetry_with_lock);
>    ....
> }
> 
> I'm not sure which solution is the uglier :D
>

I don't actually mind this latter solution, except that the order of the
parameters should be reversed (and it breaks the ABI, unless we add a
special new function for it) For me, the wrapper function should be the
main callback, and the real (unwrapped) function the extra parameter to be
called. That extra parameter to callbacks should just be a generic pointer,
so it can be data or function that is passed around.

	rte_telemetry_register_param_cmd(const char *cmd, telemetry_cb fn, 
			void *param, const char *help)

Or more specifically:


    rte_telemetry_register_param_cmd("/ethdev/stats",
            eth_dev_telemetry_with_lock, /* callback */
            eth_dev_handle_port_stats,   /* parameter */
            "Returns the common stats for a port. Parameters: int port_id");

/Bruce

PS: Yes, I realise that using void * as function pointers is not always
recommended, but we already use dlsym which does so!

Re: [PATCH 2/2] ethdev: fix race on ports for telemetry commands

[David Marchand]

On Thu, Oct 3, 2024 at 11:46 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Wed, Oct 02, 2024 at 09:26:10PM +0200, Robin Jarry wrote:
> > David Marchand, Oct 02, 2024 at 21:18:
> > > On Wed, Oct 2, 2024 at 9:09 PM Robin Jarry <rjarry@redhat.com> wrote:
> > > > I was going to suggest adding a rte_spinlock_t* parameter to a new
> > > > telemetry register function that would need to be held while the
> > > > callback is invoked. Or if we want to keep doors open to other kinds of
> > > > lock, a wrapper callback.
> > >
> > > Well, as you had experimented this approach, we know this does not
> > > work: the ethdev lock is in dpdk shared memory which is not available
> > > yet at the time RTE_INIT() is called.
> > >
> > > A single callback is strange, I guess you mean pre/post callbacks then.
> >
> > It could be a single function that will wrap the callbacks. E.g.:
> >
> > static int
> > eth_dev_telemetry_with_lock(
> >    telemetry_cb fn, const char *cmd, const char *params, struct rte_tel_data *d)
> > {
> >    int ret;
> >    rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
> >    ret = fn(cmd, params, d);
> >    rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
> >    return ret;
> > }
> >
> > RTE_INIT(ethdev_init_telemetry)
> > {
> >    ....
> >    rte_telemetry_register_cmd("/ethdev/stats", eth_dev_handle_port_stats,
> >            "Returns the common stats for a port. Parameters: int port_id",
> >            eth_dev_telemetry_with_lock);
> >    ....
> > }
> >
> > I'm not sure which solution is the uglier :D
> >
>
> I don't actually mind this latter solution, except that the order of the
> parameters should be reversed (and it breaks the ABI, unless we add a
> special new function for it) For me, the wrapper function should be the
> main callback, and the real (unwrapped) function the extra parameter to be
> called. That extra parameter to callbacks should just be a generic pointer,
> so it can be data or function that is passed around.
>
>         rte_telemetry_register_param_cmd(const char *cmd, telemetry_cb fn,
>                         void *param, const char *help)
>
> Or more specifically:
>
>
>     rte_telemetry_register_param_cmd("/ethdev/stats",
>             eth_dev_telemetry_with_lock, /* callback */
>             eth_dev_handle_port_stats,   /* parameter */
>             "Returns the common stats for a port. Parameters: int port_id");

Ok, this way seems nicer.
I'll let Robin submit a v2.


-- 
David Marchand

[PATCH dpdk v2 0/2] Fix race in ethdev telemetry

[Robin Jarry]

Following a discussion we had during the summit, here is one series to
fix a race between an application thread and the telemetry thread
handling requests on ethdev ports.

The problem may be generic to other device classes providing telemetry
callbacks, but for now, this series goes with a simple and naive
approach of putting locks in the ethdev layer.

v2: added new telemetry api to register callbacks with a private arg.

Robin Jarry (2):
  telemetry: add api to register command with private argument
  ethdev: fix potential race in telemetry endpoints

 lib/ethdev/rte_ethdev_telemetry.c | 66 ++++++++++++++++++++++---------
 lib/telemetry/rte_telemetry.h     | 46 +++++++++++++++++++++
 lib/telemetry/telemetry.c         | 38 ++++++++++++++----
 lib/telemetry/version.map         |  3 ++
 4 files changed, 126 insertions(+), 27 deletions(-)

-- 
2.46.2

[PATCH dpdk v2 1/2] telemetry: add api to register command with private argument

[Robin Jarry]

Add a new rte_telemetry_register_cmd_arg public function to register
a telemetry endpoint with a callback that takes an additional private
argument.

This will be used in the next commit to protect ethdev endpoints with
a lock.

Update perform_command() to take a struct callback object copied from
the list of callbacks and invoke the correct function pointer.

Signed-off-by: Robin Jarry <rjarry@redhat.com>
---
 lib/telemetry/rte_telemetry.h | 46 +++++++++++++++++++++++++++++++++++
 lib/telemetry/telemetry.c     | 38 +++++++++++++++++++++++------
 lib/telemetry/version.map     |  3 +++
 3 files changed, 79 insertions(+), 8 deletions(-)

diff --git a/lib/telemetry/rte_telemetry.h b/lib/telemetry/rte_telemetry.h
index cab9daa6fed6..3fbfda138b16 100644
--- a/lib/telemetry/rte_telemetry.h
+++ b/lib/telemetry/rte_telemetry.h
@@ -336,6 +336,30 @@ rte_tel_data_add_dict_uint_hex(struct rte_tel_data *d, const char *name,
 typedef int (*telemetry_cb)(const char *cmd, const char *params,
 		struct rte_tel_data *info);
 
+/**
+ * This telemetry callback is used when registering a telemetry command with
+ * rte_telemetry_register_cmd_arg().
+ *
+ * It handles getting and formatting information to be returned to telemetry
+ * when requested.
+ *
+ * @param cmd
+ * The cmd that was requested by the client.
+ * @param params
+ * Contains data required by the callback function.
+ * @param info
+ * The information to be returned to the caller.
+ * @param arg
+ * The opaque value that was passed to rte_telemetry_register_cmd_arg().
+ *
+ * @return
+ * Length of buffer used on success.
+ * @return
+ * Negative integer on error.
+ */
+typedef int (*telemetry_arg_cb)(const char *cmd, const char *params,
+		struct rte_tel_data *info, void *arg);
+
 /**
  * Used for handling data received over a telemetry socket.
  *
@@ -367,6 +391,28 @@ typedef void * (*handler)(void *sock_id);
 int
 rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const char *help);
 
+/**
+ * Used when registering a command and callback function with telemetry.
+ *
+ * @param cmd
+ * The command to register with telemetry.
+ * @param fn
+ * Callback function to be called when the command is requested.
+ * @param arg
+ * An opaque value that will be passed to the callback function.
+ * @param help
+ * Help text for the command.
+ *
+ * @return
+ *  0 on success.
+ * @return
+ *  -EINVAL for invalid parameters failure.
+ *  @return
+ *  -ENOMEM for mem allocation failure.
+ */
+__rte_experimental
+int
+rte_telemetry_register_cmd_arg(const char *cmd, telemetry_arg_cb fn, void *arg, const char *help);
 
 /**
  * Get a pointer to a container with memory allocated. The container is to be
diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
index c4c5a61a5cf8..2fe48d47f886 100644
--- a/lib/telemetry/telemetry.c
+++ b/lib/telemetry/telemetry.c
@@ -37,6 +37,8 @@ client_handler(void *socket);
 struct cmd_callback {
 	char cmd[MAX_CMD_LEN];
 	telemetry_cb fn;
+	telemetry_arg_cb fn_arg;
+	void *arg;
 	char help[RTE_TEL_MAX_STRING_LEN];
 };
 
@@ -68,14 +70,15 @@ static rte_spinlock_t callback_sl = RTE_SPINLOCK_INITIALIZER;
 static RTE_ATOMIC(uint16_t) v2_clients;
 #endif /* !RTE_EXEC_ENV_WINDOWS */
 
-int
-rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const char *help)
+static int
+__rte_telemetry_register_cmd(const char *cmd, const char *help,
+			     telemetry_cb fn, telemetry_arg_cb fn_arg, void *arg)
 {
 	struct cmd_callback *new_callbacks;
 	const char *cmdp = cmd;
 	int i = 0;
 
-	if (strlen(cmd) >= MAX_CMD_LEN || fn == NULL || cmd[0] != '/'
+	if (strlen(cmd) >= MAX_CMD_LEN || (fn == NULL && fn_arg == NULL) || cmd[0] != '/'
 			|| strlen(help) >= RTE_TEL_MAX_STRING_LEN)
 		return -EINVAL;
 
@@ -102,6 +105,8 @@ rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const char *help)
 
 	strlcpy(callbacks[i].cmd, cmd, MAX_CMD_LEN);
 	callbacks[i].fn = fn;
+	callbacks[i].fn_arg = fn_arg;
+	callbacks[i].arg = arg;
 	strlcpy(callbacks[i].help, help, RTE_TEL_MAX_STRING_LEN);
 	num_callbacks++;
 	rte_spinlock_unlock(&callback_sl);
@@ -109,6 +114,18 @@ rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const char *help)
 	return 0;
 }
 
+int
+rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const char *help)
+{
+	return __rte_telemetry_register_cmd(cmd, help, fn, NULL, NULL);
+}
+
+int
+rte_telemetry_register_cmd_arg(const char *cmd, telemetry_arg_cb fn, void *arg, const char *help)
+{
+	return __rte_telemetry_register_cmd(cmd, help, NULL, fn, arg);
+}
+
 #ifndef RTE_EXEC_ENV_WINDOWS
 
 static int
@@ -349,11 +366,16 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 }
 
 static void
-perform_command(telemetry_cb fn, const char *cmd, const char *param, int s)
+perform_command(const struct cmd_callback *cb, const char *cmd, const char *param, int s)
 {
 	struct rte_tel_data data = {0};
+	int ret;
+
+	if (cb->fn_arg != NULL)
+		ret = cb->fn_arg(cmd, param, &data, cb->arg);
+	else
+		ret = cb->fn(cmd, param, &data);
 
-	int ret = fn(cmd, param, &data);
 	if (ret < 0) {
 		char out_buf[MAX_CMD_LEN + 10];
 		int used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":null}",
@@ -392,19 +414,19 @@ client_handler(void *sock_id)
 		buffer[bytes] = 0;
 		const char *cmd = strtok(buffer, ",");
 		const char *param = strtok(NULL, "\0");
-		telemetry_cb fn = unknown_command;
+		struct cmd_callback cb = {.fn = unknown_command};
 		int i;
 
 		if (cmd && strlen(cmd) < MAX_CMD_LEN) {
 			rte_spinlock_lock(&callback_sl);
 			for (i = 0; i < num_callbacks; i++)
 				if (strcmp(cmd, callbacks[i].cmd) == 0) {
-					fn = callbacks[i].fn;
+					cb = callbacks[i];
 					break;
 				}
 			rte_spinlock_unlock(&callback_sl);
 		}
-		perform_command(fn, cmd, param, s);
+		perform_command(&cb, cmd, param, s);
 
 		bytes = read(s, buffer, sizeof(buffer) - 1);
 	}
diff --git a/lib/telemetry/version.map b/lib/telemetry/version.map
index 2907d28aa03f..8f032bf53230 100644
--- a/lib/telemetry/version.map
+++ b/lib/telemetry/version.map
@@ -28,6 +28,9 @@ EXPERIMENTAL {
 	rte_tel_data_add_array_uint_hex;
 	rte_tel_data_add_dict_uint_hex;
 
+	# added in 24.11
+	rte_telemetry_register_cmd_arg;
+
 	local: *;
 };
 
-- 
2.46.2

Re: [PATCH dpdk v2 1/2] telemetry: add api to register command with private argument

[Bruce Richardson]

On Thu, Oct 03, 2024 at 01:24:41PM +0200, Robin Jarry wrote:
> Add a new rte_telemetry_register_cmd_arg public function to register
> a telemetry endpoint with a callback that takes an additional private
> argument.
> 
> This will be used in the next commit to protect ethdev endpoints with
> a lock.
> 
> Update perform_command() to take a struct callback object copied from
> the list of callbacks and invoke the correct function pointer.
> 
> Signed-off-by: Robin Jarry <rjarry@redhat.com>
> ---

I like this solution, and seems a good general addition to telemetry also.
Couple of minor comments inline below.

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

>  lib/telemetry/rte_telemetry.h | 46 +++++++++++++++++++++++++++++++++++
>  lib/telemetry/telemetry.c     | 38 +++++++++++++++++++++++------
>  lib/telemetry/version.map     |  3 +++
>  3 files changed, 79 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/telemetry/rte_telemetry.h b/lib/telemetry/rte_telemetry.h
> index cab9daa6fed6..3fbfda138b16 100644
> --- a/lib/telemetry/rte_telemetry.h
> +++ b/lib/telemetry/rte_telemetry.h
> @@ -336,6 +336,30 @@ rte_tel_data_add_dict_uint_hex(struct rte_tel_data *d, const char *name,
>  typedef int (*telemetry_cb)(const char *cmd, const char *params,
>  		struct rte_tel_data *info);
>  
> +/**
> + * This telemetry callback is used when registering a telemetry command with
> + * rte_telemetry_register_cmd_arg().
> + *
> + * It handles getting and formatting information to be returned to telemetry
> + * when requested.
> + *
> + * @param cmd
> + * The cmd that was requested by the client.
> + * @param params
> + * Contains data required by the callback function.
> + * @param info
> + * The information to be returned to the caller.
> + * @param arg
> + * The opaque value that was passed to rte_telemetry_register_cmd_arg().
> + *

I think we tend to slightly indent the text on the line after the @param,
as in:

 * @param arg
 *    The opaque value...

> + * @return
> + * Length of buffer used on success.
> + * @return
> + * Negative integer on error.
> + */
> +typedef int (*telemetry_arg_cb)(const char *cmd, const char *params,
> +		struct rte_tel_data *info, void *arg);
> +

Not sure about this, but I'd tend to have the arg parameter as second
parameter, to keep the "info" as the final parameter. My suggested order
would be: (cmd, arg, params, info)

>  /**
>   * Used for handling data received over a telemetry socket.
>   *
> @@ -367,6 +391,28 @@ typedef void * (*handler)(void *sock_id);
>  int
>  rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const char *help);
>  

<snip>

Re: [PATCH dpdk v2 1/2] telemetry: add api to register command with private argument

[Robin Jarry]

Bruce Richardson, Oct 03, 2024 at 13:39:
> On Thu, Oct 03, 2024 at 01:24:41PM +0200, Robin Jarry wrote:
>> Add a new rte_telemetry_register_cmd_arg public function to register
>> a telemetry endpoint with a callback that takes an additional private
>> argument.
>> 
>> This will be used in the next commit to protect ethdev endpoints with
>> a lock.
>> 
>> Update perform_command() to take a struct callback object copied from
>> the list of callbacks and invoke the correct function pointer.
>> 
>> Signed-off-by: Robin Jarry <rjarry@redhat.com>
>> ---
>
> I like this solution, and seems a good general addition to telemetry also.
> Couple of minor comments inline below.
>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>
>>  lib/telemetry/rte_telemetry.h | 46 +++++++++++++++++++++++++++++++++++
>>  lib/telemetry/telemetry.c     | 38 +++++++++++++++++++++++------
>>  lib/telemetry/version.map     |  3 +++
>>  3 files changed, 79 insertions(+), 8 deletions(-)
>> 
>> diff --git a/lib/telemetry/rte_telemetry.h b/lib/telemetry/rte_telemetry.h
>> index cab9daa6fed6..3fbfda138b16 100644
>> --- a/lib/telemetry/rte_telemetry.h
>> +++ b/lib/telemetry/rte_telemetry.h
>> @@ -336,6 +336,30 @@ rte_tel_data_add_dict_uint_hex(struct rte_tel_data *d, const char *name,
>>  typedef int (*telemetry_cb)(const char *cmd, const char *params,
>>  		struct rte_tel_data *info);
>>  
>> +/**
>> + * This telemetry callback is used when registering a telemetry command with
>> + * rte_telemetry_register_cmd_arg().
>> + *
>> + * It handles getting and formatting information to be returned to telemetry
>> + * when requested.
>> + *
>> + * @param cmd
>> + * The cmd that was requested by the client.
>> + * @param params
>> + * Contains data required by the callback function.
>> + * @param info
>> + * The information to be returned to the caller.
>> + * @param arg
>> + * The opaque value that was passed to rte_telemetry_register_cmd_arg().
>> + *
>
> I think we tend to slightly indent the text on the line after the @param,
> as in:
>
>  * @param arg
>  *    The opaque value...

I found this weird too but followed the style in the rest of the file. 
I can fix that for a v3.

>
>> + * @return
>> + * Length of buffer used on success.
>> + * @return
>> + * Negative integer on error.
>> + */
>> +typedef int (*telemetry_arg_cb)(const char *cmd, const char *params,
>> +		struct rte_tel_data *info, void *arg);
>> +
>
> Not sure about this, but I'd tend to have the arg parameter as second
> parameter, to keep the "info" as the final parameter. My suggested order
> would be: (cmd, arg, params, info)

I don't have any objections. I'll send a v3.

Re: [PATCH dpdk v2 1/2] telemetry: add api to register command with private argument

[Robin Jarry]

Robin Jarry, Oct 03, 2024 at 14:36:
>>> +typedef int (*telemetry_arg_cb)(const char *cmd, const char *params,
>>> +		struct rte_tel_data *info, void *arg);
>>> +
>>
>> Not sure about this, but I'd tend to have the arg parameter as second
>> parameter, to keep the "info" as the final parameter. My suggested order
>> would be: (cmd, arg, params, info)
>
> I don't have any objections. I'll send a v3.

Reflecting back, I think the void* arg would fit better as the first 
callback argument. Thoughts?

[PATCH dpdk v2 2/2] ethdev: fix potential race in telemetry endpoints

[Robin Jarry]

While invoking telemetry commands (which may happen at any time, out of
control of the application), an application thread may concurrently
add/remove ports. The telemetry callbacks may then access partially
initialized/uninitialised ethdev data.

Reuse the ethdev lock that protects port allocation/destruction and the
new telemetry callback register api that takes an additional private
argument. Pass eth_dev_telemetry_do as the main callback and the actual
endpoint callbacks as private argument.

Fixes: c190daedb9b1 ("ethdev: add telemetry callbacks")
Cc: stable@dpdk.org

Signed-off-by: Robin Jarry <rjarry@redhat.com>
---
 lib/ethdev/rte_ethdev_telemetry.c | 66 ++++++++++++++++++++++---------
 1 file changed, 47 insertions(+), 19 deletions(-)

diff --git a/lib/ethdev/rte_ethdev_telemetry.c b/lib/ethdev/rte_ethdev_telemetry.c
index 6b873e7abe68..a9336a81b73b 100644
--- a/lib/ethdev/rte_ethdev_telemetry.c
+++ b/lib/ethdev/rte_ethdev_telemetry.c
@@ -1395,45 +1395,73 @@ eth_dev_handle_port_tm_node_caps(const char *cmd __rte_unused,
 	return ret;
 }
 
+static int eth_dev_telemetry_do(const char *cmd, const char *params, struct rte_tel_data *d,
+				void *arg)
+{
+	int ret;
+	telemetry_cb fn = arg;
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
+	ret = fn(cmd, params, d);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
+	return ret;
+}
+
 RTE_INIT(ethdev_init_telemetry)
 {
-	rte_telemetry_register_cmd("/ethdev/list", eth_dev_handle_port_list,
+	rte_telemetry_register_cmd_arg("/ethdev/list",
+			eth_dev_telemetry_do, eth_dev_handle_port_list,
 			"Returns list of available ethdev ports. Takes no parameters");
-	rte_telemetry_register_cmd("/ethdev/stats", eth_dev_handle_port_stats,
+	rte_telemetry_register_cmd_arg("/ethdev/stats",
+			eth_dev_telemetry_do, eth_dev_handle_port_stats,
 			"Returns the common stats for a port. Parameters: int port_id");
-	rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
+	rte_telemetry_register_cmd_arg("/ethdev/xstats",
+			eth_dev_telemetry_do, eth_dev_handle_port_xstats,
 			"Returns the extended stats for a port. Parameters: int port_id,hide_zero=true|false(Optional for indicates hide zero xstats)");
 #ifndef RTE_EXEC_ENV_WINDOWS
-	rte_telemetry_register_cmd("/ethdev/dump_priv", eth_dev_handle_port_dump_priv,
+	rte_telemetry_register_cmd_arg("/ethdev/dump_priv",
+			eth_dev_telemetry_do, eth_dev_handle_port_dump_priv,
 			"Returns dump private information for a port. Parameters: int port_id");
 #endif
-	rte_telemetry_register_cmd("/ethdev/link_status",
-			eth_dev_handle_port_link_status,
+	rte_telemetry_register_cmd_arg("/ethdev/link_status",
+			eth_dev_telemetry_do, eth_dev_handle_port_link_status,
 			"Returns the link status for a port. Parameters: int port_id");
-	rte_telemetry_register_cmd("/ethdev/info", eth_dev_handle_port_info,
+	rte_telemetry_register_cmd_arg("/ethdev/info",
+			eth_dev_telemetry_do, eth_dev_handle_port_info,
 			"Returns the device info for a port. Parameters: int port_id");
-	rte_telemetry_register_cmd("/ethdev/module_eeprom", eth_dev_handle_port_module_eeprom,
+	rte_telemetry_register_cmd_arg("/ethdev/module_eeprom",
+			eth_dev_telemetry_do, eth_dev_handle_port_module_eeprom,
 			"Returns module EEPROM info with SFF specs. Parameters: int port_id");
-	rte_telemetry_register_cmd("/ethdev/macs", eth_dev_handle_port_macs,
+	rte_telemetry_register_cmd_arg("/ethdev/macs",
+			eth_dev_telemetry_do, eth_dev_handle_port_macs,
 			"Returns the MAC addresses for a port. Parameters: int port_id");
-	rte_telemetry_register_cmd("/ethdev/flow_ctrl", eth_dev_handle_port_flow_ctrl,
+	rte_telemetry_register_cmd_arg("/ethdev/flow_ctrl",
+			eth_dev_telemetry_do, eth_dev_handle_port_flow_ctrl,
 			"Returns flow ctrl info for a port. Parameters: int port_id");
-	rte_telemetry_register_cmd("/ethdev/rx_queue", eth_dev_handle_port_rxq,
+	rte_telemetry_register_cmd_arg("/ethdev/rx_queue",
+			eth_dev_telemetry_do, eth_dev_handle_port_rxq,
 			"Returns Rx queue info for a port. Parameters: int port_id, int queue_id (Optional if only one queue)");
-	rte_telemetry_register_cmd("/ethdev/tx_queue", eth_dev_handle_port_txq,
+	rte_telemetry_register_cmd_arg("/ethdev/tx_queue",
+			eth_dev_telemetry_do, eth_dev_handle_port_txq,
 			"Returns Tx queue info for a port. Parameters: int port_id, int queue_id (Optional if only one queue)");
-	rte_telemetry_register_cmd("/ethdev/dcb", eth_dev_handle_port_dcb,
+	rte_telemetry_register_cmd_arg("/ethdev/dcb",
+			eth_dev_telemetry_do, eth_dev_handle_port_dcb,
 			"Returns DCB info for a port. Parameters: int port_id");
-	rte_telemetry_register_cmd("/ethdev/rss_info", eth_dev_handle_port_rss_info,
+	rte_telemetry_register_cmd_arg("/ethdev/rss_info",
+			eth_dev_telemetry_do, eth_dev_handle_port_rss_info,
 			"Returns RSS info for a port. Parameters: int port_id");
-	rte_telemetry_register_cmd("/ethdev/fec", eth_dev_handle_port_fec,
+	rte_telemetry_register_cmd_arg("/ethdev/fec",
+			eth_dev_telemetry_do, eth_dev_handle_port_fec,
 			"Returns FEC info for a port. Parameters: int port_id");
-	rte_telemetry_register_cmd("/ethdev/vlan", eth_dev_handle_port_vlan,
+	rte_telemetry_register_cmd_arg("/ethdev/vlan",
+			eth_dev_telemetry_do, eth_dev_handle_port_vlan,
 			"Returns VLAN info for a port. Parameters: int port_id");
-	rte_telemetry_register_cmd("/ethdev/tm_capability", eth_dev_handle_port_tm_caps,
+	rte_telemetry_register_cmd_arg("/ethdev/tm_capability",
+			eth_dev_telemetry_do, eth_dev_handle_port_tm_caps,
 			"Returns TM Capabilities info for a port. Parameters: int port_id");
-	rte_telemetry_register_cmd("/ethdev/tm_level_capability", eth_dev_handle_port_tm_level_caps,
+	rte_telemetry_register_cmd_arg("/ethdev/tm_level_capability",
+			eth_dev_telemetry_do, eth_dev_handle_port_tm_level_caps,
 			"Returns TM Level Capabilities info for a port. Parameters: int port_id, int level_id (see tm_capability for the max)");
-	rte_telemetry_register_cmd("/ethdev/tm_node_capability", eth_dev_handle_port_tm_node_caps,
+	rte_telemetry_register_cmd_arg("/ethdev/tm_node_capability",
+			eth_dev_telemetry_do, eth_dev_handle_port_tm_node_caps,
 			"Returns TM Node Capabilities info for a port. Parameters: int port_id, int node_id (see tm_capability for the max)");
 }
-- 
2.46.2

Re: [PATCH dpdk v2 2/2] ethdev: fix potential race in telemetry endpoints

[Bruce Richardson]

On Thu, Oct 03, 2024 at 01:24:42PM +0200, Robin Jarry wrote:
> While invoking telemetry commands (which may happen at any time, out of
> control of the application), an application thread may concurrently
> add/remove ports. The telemetry callbacks may then access partially
> initialized/uninitialised ethdev data.
> 
> Reuse the ethdev lock that protects port allocation/destruction and the
> new telemetry callback register api that takes an additional private
> argument. Pass eth_dev_telemetry_do as the main callback and the actual
> endpoint callbacks as private argument.
> 
> Fixes: c190daedb9b1 ("ethdev: add telemetry callbacks")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Robin Jarry <rjarry@redhat.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>