* [PATCH 2/2] ethdev: fix race on ports for telemetry commands [not found] <20241002155709.2522273-1-david.marchand@redhat.com> @ 2024-10-02 15:57 ` David Marchand 2024-10-02 16:27 ` Bruce Richardson 2024-10-08 2:07 ` lihuisong (C) 2024-10-03 11:24 ` [PATCH dpdk v2 2/2] ethdev: fix potential race in telemetry endpoints Robin Jarry [not found] ` <20241014193237.1992382-1-rjarry@redhat.com> 2 siblings, 2 replies; 17+ messages in thread From: David Marchand @ 2024-10-02 15:57 UTC (permalink / raw) To: dev Cc: bruce.richardson, rjarry, ktraynor, stable, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, Keith Wiles, Ciara Power 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ethdev: fix race on ports for telemetry commands 2024-10-02 15:57 ` [PATCH 2/2] ethdev: fix race on ports for telemetry commands David Marchand @ 2024-10-02 16:27 ` Bruce Richardson 2024-10-02 19:06 ` David Marchand 2024-10-08 2:07 ` lihuisong (C) 1 sibling, 1 reply; 17+ messages in thread From: Bruce Richardson @ 2024-10-02 16:27 UTC (permalink / raw) To: David Marchand Cc: dev, rjarry, ktraynor, stable, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, Keith Wiles, Ciara Power 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ethdev: fix race on ports for telemetry commands 2024-10-02 16:27 ` Bruce Richardson @ 2024-10-02 19:06 ` David Marchand 2024-10-02 19:09 ` Robin Jarry 0 siblings, 1 reply; 17+ messages in thread From: David Marchand @ 2024-10-02 19:06 UTC (permalink / raw) To: Bruce Richardson Cc: dev, rjarry, ktraynor, stable, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, Keith Wiles, Ciara Power 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ethdev: fix race on ports for telemetry commands 2024-10-02 19:06 ` David Marchand @ 2024-10-02 19:09 ` Robin Jarry 2024-10-02 19:18 ` David Marchand 0 siblings, 1 reply; 17+ messages in thread From: Robin Jarry @ 2024-10-02 19:09 UTC (permalink / raw) To: David Marchand, Bruce Richardson Cc: dev, ktraynor, stable, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, Keith Wiles, Ciara Power 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? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ethdev: fix race on ports for telemetry commands 2024-10-02 19:09 ` Robin Jarry @ 2024-10-02 19:18 ` David Marchand 2024-10-02 19:26 ` Robin Jarry 0 siblings, 1 reply; 17+ messages in thread From: David Marchand @ 2024-10-02 19:18 UTC (permalink / raw) To: Robin Jarry Cc: Bruce Richardson, dev, ktraynor, stable, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, Keith Wiles, Ciara Power 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ethdev: fix race on ports for telemetry commands 2024-10-02 19:18 ` David Marchand @ 2024-10-02 19:26 ` Robin Jarry 2024-10-03 9:46 ` Bruce Richardson 0 siblings, 1 reply; 17+ messages in thread From: Robin Jarry @ 2024-10-02 19:26 UTC (permalink / raw) To: David Marchand Cc: Bruce Richardson, dev, ktraynor, stable, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, Keith Wiles, Ciara Power 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ethdev: fix race on ports for telemetry commands 2024-10-02 19:26 ` Robin Jarry @ 2024-10-03 9:46 ` Bruce Richardson 2024-10-03 9:58 ` David Marchand 0 siblings, 1 reply; 17+ messages in thread From: Bruce Richardson @ 2024-10-03 9:46 UTC (permalink / raw) To: Robin Jarry Cc: David Marchand, dev, ktraynor, stable, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, Keith Wiles, Ciara Power 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! ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ethdev: fix race on ports for telemetry commands 2024-10-03 9:46 ` Bruce Richardson @ 2024-10-03 9:58 ` David Marchand 0 siblings, 0 replies; 17+ messages in thread From: David Marchand @ 2024-10-03 9:58 UTC (permalink / raw) To: Bruce Richardson Cc: Robin Jarry, dev, ktraynor, stable, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, Keith Wiles, Ciara Power 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ethdev: fix race on ports for telemetry commands 2024-10-02 15:57 ` [PATCH 2/2] ethdev: fix race on ports for telemetry commands David Marchand 2024-10-02 16:27 ` Bruce Richardson @ 2024-10-08 2:07 ` lihuisong (C) 2024-10-08 6:48 ` David Marchand 1 sibling, 1 reply; 17+ messages in thread From: lihuisong (C) @ 2024-10-08 2:07 UTC (permalink / raw) To: David Marchand, dev Cc: bruce.richardson, rjarry, ktraynor, stable, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, Keith Wiles, Ciara Power 在 2024/10/2 23:57, 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. Agree to lock for telemetry. > > 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) \ In addition, the "command" and "usage" are unuseful in this definition. This is confused to me. Can the first macro definition just use 'func' as input parameter? The name of this macro define is the same as the below one. I prefer to use two different macros to do this. > +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 > } ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] ethdev: fix race on ports for telemetry commands 2024-10-08 2:07 ` lihuisong (C) @ 2024-10-08 6:48 ` David Marchand 0 siblings, 0 replies; 17+ messages in thread From: David Marchand @ 2024-10-08 6:48 UTC (permalink / raw) To: lihuisong (C) Cc: dev, bruce.richardson, rjarry, ktraynor, stable, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, Keith Wiles, Ciara Power On Tue, Oct 8, 2024 at 4:09 AM lihuisong (C) <lihuisong@huawei.com> wrote: > > > 在 2024/10/2 23:57, 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. > Agree to lock for telemetry. > > > > 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) \ > In addition, the "command" and "usage" are unuseful in this definition. > This is confused to me. > Can the first macro definition just use 'func' as input parameter? > The name of this macro define is the same as the below one. > I prefer to use two different macros to do this. This patch has been withdrawn. Robin is working on another approach, see: https://patchwork.dpdk.org/project/dpdk/patch/20241003112438.902397-6-rjarry@redhat.com/ https://patchwork.dpdk.org/project/dpdk/patch/20241003112438.902397-7-rjarry@redhat.com/ -- David Marchand ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH dpdk v2 2/2] ethdev: fix potential race in telemetry endpoints [not found] <20241002155709.2522273-1-david.marchand@redhat.com> 2024-10-02 15:57 ` [PATCH 2/2] ethdev: fix race on ports for telemetry commands David Marchand @ 2024-10-03 11:24 ` Robin Jarry 2024-10-03 11:39 ` Bruce Richardson [not found] ` <20241014193237.1992382-1-rjarry@redhat.com> 2 siblings, 1 reply; 17+ messages in thread From: Robin Jarry @ 2024-10-03 11:24 UTC (permalink / raw) To: dev, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, Ciara Power, Keith Wiles, Bruce Richardson Cc: david.marchand, ktraynor, stable 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH dpdk v2 2/2] ethdev: fix potential race in telemetry endpoints 2024-10-03 11:24 ` [PATCH dpdk v2 2/2] ethdev: fix potential race in telemetry endpoints Robin Jarry @ 2024-10-03 11:39 ` Bruce Richardson 0 siblings, 0 replies; 17+ messages in thread From: Bruce Richardson @ 2024-10-03 11:39 UTC (permalink / raw) To: Robin Jarry Cc: dev, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, Ciara Power, Keith Wiles, david.marchand, ktraynor, stable 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> ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <20241014193237.1992382-1-rjarry@redhat.com>]
* [PATCH dpdk v3 2/2] ethdev: fix potential race in telemetry endpoints [not found] ` <20241014193237.1992382-1-rjarry@redhat.com> @ 2024-10-14 19:32 ` Robin Jarry 2024-10-14 20:01 ` Stephen Hemminger 2024-10-15 8:38 ` David Marchand 0 siblings, 2 replies; 17+ messages in thread From: Robin Jarry @ 2024-10-14 19:32 UTC (permalink / raw) To: dev, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, Ciara Power, Bruce Richardson, Keith Wiles Cc: stable 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> --- 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..7599fa2852b6 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, + void *arg, struct rte_tel_data *d) +{ + 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH dpdk v3 2/2] ethdev: fix potential race in telemetry endpoints 2024-10-14 19:32 ` [PATCH dpdk v3 " Robin Jarry @ 2024-10-14 20:01 ` Stephen Hemminger 2024-10-15 8:02 ` David Marchand 2024-10-15 8:38 ` David Marchand 1 sibling, 1 reply; 17+ messages in thread From: Stephen Hemminger @ 2024-10-14 20:01 UTC (permalink / raw) To: Robin Jarry Cc: dev, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, Ciara Power, Bruce Richardson, Keith Wiles, stable On Mon, 14 Oct 2024 21:32:37 +0200 Robin Jarry <rjarry@redhat.com> 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> > --- > 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..7599fa2852b6 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, > + void *arg, struct rte_tel_data *d) > +{ > + 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; > +} If this happens often, and the function takes a long time (like doing i/o) it might be worth changing this to reader/writer in future. Also, would be best to add a comment here as to what is being protected if you do another version. Acked-by: Stephen Hemminger <stephen@networkplumber.org> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH dpdk v3 2/2] ethdev: fix potential race in telemetry endpoints 2024-10-14 20:01 ` Stephen Hemminger @ 2024-10-15 8:02 ` David Marchand 2024-10-15 8:04 ` Robin Jarry 0 siblings, 1 reply; 17+ messages in thread From: David Marchand @ 2024-10-15 8:02 UTC (permalink / raw) To: Stephen Hemminger, Robin Jarry Cc: dev, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, Ciara Power, Bruce Richardson, Keith Wiles, stable On Mon, Oct 14, 2024 at 10:01 PM Stephen Hemminger <stephen@networkplumber.org> wrote: > > On Mon, 14 Oct 2024 21:32:37 +0200 > Robin Jarry <rjarry@redhat.com> 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> > > --- > > 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..7599fa2852b6 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, > > + void *arg, struct rte_tel_data *d) > > +{ > > + 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; > > +} > > If this happens often, and the function takes a long time (like doing i/o) > it might be worth changing this to reader/writer in future. Yes, this was an option mentionned when we discussed the issue in Montréal. For now, a spinlock seems enough. > > Also, would be best to add a comment here as to what is being protected > if you do another version. I can add something when applying, like: @@ -1400,6 +1400,7 @@ static int eth_dev_telemetry_do(const char *cmd, const char *params, { int ret; telemetry_cb fn = arg; + /* Protect against port removal while invoking callback, calling ethdev API. */ rte_spinlock_lock(rte_mcfg_ethdev_get_lock()); ret = fn(cmd, params, d); rte_spinlock_unlock(rte_mcfg_ethdev_get_lock()); -- David Marchand ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH dpdk v3 2/2] ethdev: fix potential race in telemetry endpoints 2024-10-15 8:02 ` David Marchand @ 2024-10-15 8:04 ` Robin Jarry 0 siblings, 0 replies; 17+ messages in thread From: Robin Jarry @ 2024-10-15 8:04 UTC (permalink / raw) To: David Marchand, Stephen Hemminger Cc: dev, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, Ciara Power, Bruce Richardson, Keith Wiles, stable David Marchand, Oct 15, 2024 at 10:02: > On Mon, Oct 14, 2024 at 10:01 PM Stephen Hemminger >> If this happens often, and the function takes a long time (like doing i/o) >> it might be worth changing this to reader/writer in future. > > Yes, this was an option mentionned when we discussed the issue in Montréal. > For now, a spinlock seems enough. As far as I know, no ethdev telemetry endpoint handlers do any i/o. They only access stuff in memory. >> Also, would be best to add a comment here as to what is being protected >> if you do another version. > > I can add something when applying, like: > > @@ -1400,6 +1400,7 @@ static int eth_dev_telemetry_do(const char *cmd, > const char *params, > { > int ret; > telemetry_cb fn = arg; > + /* Protect against port removal while invoking callback, > calling ethdev API. */ > rte_spinlock_lock(rte_mcfg_ethdev_get_lock()); > ret = fn(cmd, params, d); > rte_spinlock_unlock(rte_mcfg_ethdev_get_lock()); Ack, thank you. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH dpdk v3 2/2] ethdev: fix potential race in telemetry endpoints 2024-10-14 19:32 ` [PATCH dpdk v3 " Robin Jarry 2024-10-14 20:01 ` Stephen Hemminger @ 2024-10-15 8:38 ` David Marchand 1 sibling, 0 replies; 17+ messages in thread From: David Marchand @ 2024-10-15 8:38 UTC (permalink / raw) To: Robin Jarry Cc: dev, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, Bruce Richardson, Keith Wiles, stable, Stephen Hemminger On Mon, Oct 14, 2024 at 9:33 PM Robin Jarry <rjarry@redhat.com> 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> Thanks for the fix Robin, series applied. -- David Marchand ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2024-10-15 8:39 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20241002155709.2522273-1-david.marchand@redhat.com>
2024-10-02 15:57 ` [PATCH 2/2] ethdev: fix race on ports for telemetry commands David Marchand
2024-10-02 16:27 ` Bruce Richardson
2024-10-02 19:06 ` David Marchand
2024-10-02 19:09 ` Robin Jarry
2024-10-02 19:18 ` David Marchand
2024-10-02 19:26 ` Robin Jarry
2024-10-03 9:46 ` Bruce Richardson
2024-10-03 9:58 ` David Marchand
2024-10-08 2:07 ` lihuisong (C)
2024-10-08 6:48 ` David Marchand
2024-10-03 11:24 ` [PATCH dpdk v2 2/2] ethdev: fix potential race in telemetry endpoints Robin Jarry
2024-10-03 11:39 ` Bruce Richardson
[not found] ` <20241014193237.1992382-1-rjarry@redhat.com>
2024-10-14 19:32 ` [PATCH dpdk v3 " Robin Jarry
2024-10-14 20:01 ` Stephen Hemminger
2024-10-15 8:02 ` David Marchand
2024-10-15 8:04 ` Robin Jarry
2024-10-15 8:38 ` 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).