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

[David Marchand <david.marchand@redhat.com>]

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