From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 54E7945A82; Wed, 2 Oct 2024 21:06:43 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 29C69402A7; Wed, 2 Oct 2024 21:06:43 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mails.dpdk.org (Postfix) with ESMTP id C3E3840299 for ; Wed, 2 Oct 2024 21:06:41 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1727896001; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=XTt9OrGBPJKE4R4lKz1i+fjHweB7g8svCpAE/xPKljs=; b=eslrYV9AwEgKSmsHOa/Nf4mhf9AQrZhAF62R8aVspTUAOWHus6sIhh4zayNfRMOAZwjIQH l2MYGQKSikWyJraPkMl8W36Y7rT4IMImjxFqeuHh1fiVWhyEf1z5PLZgP3LUNWia/LlBC8 Bg29x2eYRAL4G3raQmuRi+uRBnT6w+Q= Received: from mail-lj1-f200.google.com (mail-lj1-f200.google.com [209.85.208.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-691-gPf7STTCMMGtkR5hAUuCwA-1; Wed, 02 Oct 2024 15:06:40 -0400 X-MC-Unique: gPf7STTCMMGtkR5hAUuCwA-1 Received: by mail-lj1-f200.google.com with SMTP id 38308e7fff4ca-2fad9502741so1264101fa.0 for ; Wed, 02 Oct 2024 12:06:39 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1727895999; x=1728500799; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=XTt9OrGBPJKE4R4lKz1i+fjHweB7g8svCpAE/xPKljs=; b=xGdyx8s929AJyW64PB9zSQ4Le2r4h47WSZS4tN/x7p9lLEtpdMPXGjCuZfjlRL90b8 SJidbDEdaQkHaYmPg0UlzBi/Jdx+J84APxUMsfJYGSSi0jNnF8npVTlzDg+WN5gPGA9O OBKS05B+2KmTNXIa+i2Sk23PIY3Bkwf2ruQQqtY4rxAGT0uXJlJ8xLLVVa17jaPMdLU/ Wiysq4oBHuJr155s6utmVdJU/2coQdnYdag6F4jrdN6duosNr0LeDp7TadRmk2xc+Qqv mYeP3m9f64f7y9x1LzjrqTYLpTuVLaYRmamZ2D5cvzjcXvTjirVLo5oDdWWNYIcmrJkT WDjA== X-Gm-Message-State: AOJu0Yx1HXdRjv/io35Imh4IWqkM048YkL6zUfA5r4lBSbwiytyJQO7U B4VNC/REPSaZXnnj3jURa/YUhAmCiPeKxTgnWIsQev1btd5K/ZLFS7gVQ23xkN17Fzfi8GAqSuf wMxYncVxmlgRc5Hp2gy7fU8OZFwA6jcKEbk6MrJ0EcvCt2Mv6VAc5vuFqC+l44f5HZivjvDMzHe 8FJWrbR+qR1lIujKk= X-Received: by 2002:a2e:b8cd:0:b0:2fa:d4ad:f4db with SMTP id 38308e7fff4ca-2fae1016594mr30017451fa.15.1727895998475; Wed, 02 Oct 2024 12:06:38 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHOgpFoeeMqnVsnxh2JU9EmtCbxk4x5xbFN69awmhMrSgYSpdfPtO/JWp0TVEaZfEIFBYqxiPxO50iJ0biifgE= X-Received: by 2002:a2e:b8cd:0:b0:2fa:d4ad:f4db with SMTP id 38308e7fff4ca-2fae1016594mr30017281fa.15.1727895998020; Wed, 02 Oct 2024 12:06:38 -0700 (PDT) MIME-Version: 1.0 References: <20241002155709.2522273-1-david.marchand@redhat.com> <20241002155709.2522273-3-david.marchand@redhat.com> In-Reply-To: From: David Marchand Date: Wed, 2 Oct 2024 21:06:26 +0200 Message-ID: Subject: Re: [PATCH 2/2] ethdev: fix race on ports for telemetry commands To: Bruce Richardson Cc: dev@dpdk.org, rjarry@redhat.com, ktraynor@redhat.com, stable@dpdk.org, Thomas Monjalon , Ferruh Yigit , Andrew Rybchenko , Keith Wiles , Ciara Power X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Wed, Oct 2, 2024 at 6:27=E2=80=AFPM Bruce Richardson 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 > > --- > > 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 > > > > #include > > +#include > > #include > > > > #include "rte_ethdev.h" > > @@ -1403,43 +1404,61 @@ eth_dev_handle_port_tm_node_caps(const char *cm= d __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 paramet= ers") \ > > + ETHDEV_TELEMETRY_HANDLER("/ethdev/stats", eth_dev_handle_port_sta= ts, \ > > + "Returns the common stats for a port. Parameters: int por= t_id") \ > > + ETHDEV_TELEMETRY_HANDLER("/ethdev/xstats", eth_dev_handle_port_xs= tats, \ > > + "Returns the extended stats for a port. Parameters: int p= ort_id,hide_zero=3Dtrue|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_po= rt_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: i= nt port_id") \ > > + ETHDEV_TELEMETRY_HANDLER("/ethdev/macs", eth_dev_handle_port_macs= , \ > > + "Returns the MAC addresses for a port. Parameters: int po= rt_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_i= d, 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_i= d, 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_h= andle_port_tm_level_caps, \ > > + "Returns TM Level Capabilities info for a port. Parameter= s: int port_id, int level_id (see tm_capability for the max)") \ > > + ETHDEV_TELEMETRY_HANDLER("/ethdev/tm_node_capability", eth_dev_ha= ndle_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 *p= arams, \ > > + struct rte_tel_data *d) \ > > +{ \ > > + int ret; \ > > + rte_spinlock_lock(rte_mcfg_ethdev_get_lock()); \ > > + ret =3D 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_li= st, > > - "Returns list of available ethdev ports. Takes no= parameters"); > > - rte_telemetry_register_cmd("/ethdev/stats", eth_dev_handle_port_s= tats, > > - "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. Parameter= s: int port_id,hide_zero=3Dtrue|false(Optional for indicates hide zero xsta= ts)"); > > - rte_telemetry_register_cmd("/ethdev/dump_priv", eth_dev_handle_po= rt_dump_priv, > > - "Returns dump private information for a port. Par= ameters: 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_in= fo, > > - "Returns the device info for a port. Parameters: = int port_id"); > > - rte_telemetry_register_cmd("/ethdev/module_eeprom", eth_dev_handl= e_port_module_eeprom, > > - "Returns module EEPROM info with SFF specs. Param= eters: int port_id"); > > - rte_telemetry_register_cmd("/ethdev/macs", eth_dev_handle_port_ma= cs, > > - "Returns the MAC addresses for a port. Parameters= : int port_id"); > > - rte_telemetry_register_cmd("/ethdev/flow_ctrl", eth_dev_handle_po= rt_flow_ctrl, > > - "Returns flow ctrl info for a port. Parameters: i= nt port_id"); > > - rte_telemetry_register_cmd("/ethdev/rx_queue", eth_dev_handle_por= t_rxq, > > - "Returns Rx queue info for a port. Parameters: in= t port_id, int queue_id (Optional if only one queue)"); > > - rte_telemetry_register_cmd("/ethdev/tx_queue", eth_dev_handle_por= t_txq, > > - "Returns Tx queue info for a port. Parameters: in= t 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 por= t_id"); > > - rte_telemetry_register_cmd("/ethdev/rss_info", eth_dev_handle_por= t_rss_info, > > - "Returns RSS info for a port. Parameters: int por= t_id"); > > - rte_telemetry_register_cmd("/ethdev/fec", eth_dev_handle_port_fec= , > > - "Returns FEC info for a port. Parameters: int por= t_id"); > > - rte_telemetry_register_cmd("/ethdev/vlan", eth_dev_handle_port_vl= an, > > - "Returns VLAN info for a port. Parameters: int po= rt_id"); > > - rte_telemetry_register_cmd("/ethdev/tm_capability", eth_dev_handl= e_port_tm_caps, > > - "Returns TM Capabilities info for a port. Paramet= ers: 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. P= arameters: 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. Pa= rameters: 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 t= he > 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 b= e > a simpler in C rather than in macros, and the perf impact for telemetry i= s > 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 =3D NULL; int ret =3D -EINVAL; if (strcmp(cmd, "/ethdev/list") =3D=3D 0) { cb =3D eth_dev_handle_po= rt_list; } else if (strcmp(cmd, "/ethdev/stats") =3D=3D 0) { cb =3D eth_dev_handle_port_stats; } else if (strcmp(cmd, "/ethdev/xstats") =3D=3D 0) { cb =3D eth_dev_handle_port_xstats; } ... if (cb !=3D NULL) { rte_spinlock_lock(rte_mcfg_ethdev_get_lock()); ret =3D 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=3Dtrue|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") \ --=20 David Marchand