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 32E3B45A82 for ; Wed, 2 Oct 2024 21:09:55 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2D97C40299; Wed, 2 Oct 2024 21:09:55 +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 E04ED40299 for ; Wed, 2 Oct 2024 21:09:53 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1727896193; 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=XFHuAoM1YT8/rXy689m2EGht7VnDevnkLH78JuznwsE=; b=aT+EhK0MPHEIlf4ntEIRPGDqjLkjuAv1+6i7PjrmVdjXAkPwLUcFKU2Y6l1kZ7MuqhURfl P8q/iemehVrP6SA3RxSRS1UKq4UYtwB610SvrUD7P+aI30CSXqy7EGfPwUQcMiL4C7JCYS KTRUn2l71W8PFNdRh6/BoGyzjzmRX+g= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-629-ldA-PpnQNuam3MRPta32vg-1; Wed, 02 Oct 2024 15:09:52 -0400 X-MC-Unique: ldA-PpnQNuam3MRPta32vg-1 Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-42cdeac2da6so511575e9.2 for ; Wed, 02 Oct 2024 12:09:52 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1727896191; x=1728500991; h=in-reply-to:references:user-agent:subject:to:from:cc:message-id :date:content-transfer-encoding:mime-version:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=XFHuAoM1YT8/rXy689m2EGht7VnDevnkLH78JuznwsE=; b=d6l+NBKUStMtb3X4alsvz/9Y+JB/FXpPvSps6rBDQnuqp+OJbm2SHH2HlJjWKq4NGk YcmGooDI/lWeB0cgIwfq+tbVIr3iG3jQ+8x1GNobRPfAYh4W7af3lXpxcxgiBXjC9XVr CuwxL/cjvAkhNNiQr09zU+o/XgVGDMyPS40YOh26dhGROw8GtktjNqlHJlGeNeJoOC0l vT74Mit6OdMwOC7JiO+0OeRUlneKWzecAHdfTaCG8GWIOZ5D5VhrRZDQrgx4BXaN3XbE ipsBlwV7WnW8wA+3OALFafpPWuEim17wtb4ToxJDDXL7+DGknKCeyIepTPgBzeuvAAKq yD0w== X-Forwarded-Encrypted: i=1; AJvYcCW/soc8VUUv8KV01SUZ00XJe+v/uy8oQnQOh96zHWqXTTFyilEwHi/hWdvMbxhHSc+KWIUo40s=@dpdk.org X-Gm-Message-State: AOJu0YybRBWPB4NSx4/c60zpI5M55QBcxAA8m2XQaYirRu8QDp7I+DxE 27aPS1eEJR3YJPKBt+YLU5djiv2S212uGKaxm8tmi0N1Yzh97o3y0vZalomAOLsbQ4qEzeyEIuQ +fZkVjMvUW0LU5UUqD2MFArpJPt0zYjox86zL3g+nE/Cg X-Received: by 2002:a05:600c:1d13:b0:42c:df67:1d35 with SMTP id 5b1f17b1804b1-42f777b02a0mr34616625e9.1.1727896191029; Wed, 02 Oct 2024 12:09:51 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFPH+TGjdXkVm2X8faQDzDnJea+mLYzMDyOBVS2jWiVVufX18vNOtRAi9c0FnjSx4jMw2GZlQ== X-Received: by 2002:a05:600c:1d13:b0:42c:df67:1d35 with SMTP id 5b1f17b1804b1-42f777b02a0mr34616385e9.1.1727896190496; Wed, 02 Oct 2024 12:09:50 -0700 (PDT) Received: from localhost (2a01cb00025433001b9f807167cdbf20.ipv6.abo.wanadoo.fr. [2a01:cb00:254:3300:1b9f:8071:67cd:bf20]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-42f7a01f4c6sm26566015e9.34.2024.10.02.12.09.49 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 02 Oct 2024 12:09:49 -0700 (PDT) Mime-Version: 1.0 Date: Wed, 02 Oct 2024 21:09:48 +0200 Message-Id: Cc: , , , "Thomas Monjalon" , "Ferruh Yigit" , "Andrew Rybchenko" , "Keith Wiles" , "Ciara Power" From: "Robin Jarry" To: "David Marchand" , "Bruce Richardson" Subject: Re: [PATCH 2/2] ethdev: fix race on ports for telemetry commands User-Agent: aerc/0.18.2-74-g7e4ac1a5f54c References: <20241002155709.2522273-1-david.marchand@redhat.com> <20241002155709.2522273-3-david.marchand@redhat.com> In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8; format=Flowed X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org David Marchand, Oct 02, 2024 at 21:06: > 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 *c= md __rte_unused, >> > return ret; >> > } >> > >> > +#define ETHDEV_TELEMETRY_HANDLERS \ >> > + ETHDEV_TELEMETRY_HANDLER("/ethdev/list", eth_dev_handle_port_lis= t, \ >> > + "Returns list of available ethdev ports. Takes no parame= ters") \ >> > + ETHDEV_TELEMETRY_HANDLER("/ethdev/stats", eth_dev_handle_port_st= ats, \ >> > + "Returns the common stats for a port. Parameters: int po= rt_id") \ >> > + ETHDEV_TELEMETRY_HANDLER("/ethdev/xstats", eth_dev_handle_port_x= stats, \ >> > + "Returns the extended stats for a port. Parameters: int = port_id,hide_zero=3Dtrue|false(Optional for indicates hide zero xstats)") \ >> > + ETHDEV_TELEMETRY_HANDLER("/ethdev/dump_priv", eth_dev_handle_por= t_dump_priv, \ >> > + "Returns dump private information for a port. Parameters= : int port_id") \ >> > + ETHDEV_TELEMETRY_HANDLER("/ethdev/link_status", eth_dev_handle_p= ort_link_status, \ >> > + "Returns the link status for a port. Parameters: int por= t_id") \ >> > + ETHDEV_TELEMETRY_HANDLER("/ethdev/info", eth_dev_handle_port_inf= o, \ >> > + "Returns the device info for a port. Parameters: int por= t_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_mac= s, \ >> > + "Returns the MAC addresses for a port. Parameters: int p= ort_id") \ >> > + ETHDEV_TELEMETRY_HANDLER("/ethdev/flow_ctrl", eth_dev_handle_por= t_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_vla= n, \ >> > + "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: in= t port_id") \ >> > + ETHDEV_TELEMETRY_HANDLER("/ethdev/tm_level_capability", eth_dev_= handle_port_tm_level_caps, \ >> > + "Returns TM Level Capabilities info for a port. Paramete= rs: int port_id, int level_id (see tm_capability for the max)") \ >> > + ETHDEV_TELEMETRY_HANDLER("/ethdev/tm_node_capability", eth_dev_h= andle_port_tm_node_caps, \ >> > + "Returns TM Node Capabilities info for a port. Parameter= s: 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 =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 thin= k >> 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_l= ist, >> > - "Returns list of available ethdev ports. Takes n= o 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. Paramete= rs: int port_id,hide_zero=3Dtrue|false(Optional for indicates hide zero xst= ats)"); >> > - rte_telemetry_register_cmd("/ethdev/dump_priv", eth_dev_handle_p= ort_dump_priv, >> > - "Returns dump private information for a port. Pa= rameters: 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_i= nfo, >> > - "Returns the device info for a port. Parameters:= int port_id"); >> > - rte_telemetry_register_cmd("/ethdev/module_eeprom", eth_dev_hand= le_port_module_eeprom, >> > - "Returns module EEPROM info with SFF specs. Para= meters: int port_id"); >> > - rte_telemetry_register_cmd("/ethdev/macs", eth_dev_handle_port_m= acs, >> > - "Returns the MAC addresses for a port. Parameter= s: int port_id"); >> > - rte_telemetry_register_cmd("/ethdev/flow_ctrl", eth_dev_handle_p= ort_flow_ctrl, >> > - "Returns flow ctrl info for a port. Parameters: = int port_id"); >> > - rte_telemetry_register_cmd("/ethdev/rx_queue", eth_dev_handle_po= rt_rxq, >> > - "Returns Rx queue info for a port. Parameters: i= nt port_id, int queue_id (Optional if only one queue)"); >> > - rte_telemetry_register_cmd("/ethdev/tx_queue", eth_dev_handle_po= rt_txq, >> > - "Returns Tx queue info for a port. Parameters: i= nt port_id, int queue_id (Optional if only one queue)"); >> > - rte_telemetry_register_cmd("/ethdev/dcb", eth_dev_handle_port_dc= b, >> > - "Returns DCB info for a port. Parameters: int po= rt_id"); >> > - rte_telemetry_register_cmd("/ethdev/rss_info", eth_dev_handle_po= rt_rss_info, >> > - "Returns RSS info for a port. Parameters: int po= rt_id"); >> > - rte_telemetry_register_cmd("/ethdev/fec", eth_dev_handle_port_fe= c, >> > - "Returns FEC info for a port. Parameters: int po= rt_id"); >> > - rte_telemetry_register_cmd("/ethdev/vlan", eth_dev_handle_port_v= lan, >> > - "Returns VLAN info for a port. Parameters: int p= ort_id"); >> > - rte_telemetry_register_cmd("/ethdev/tm_capability", eth_dev_hand= le_port_tm_caps, >> > - "Returns TM Capabilities info for a port. Parame= ters: int port_id"); >> > - rte_telemetry_register_cmd("/ethdev/tm_level_capability", eth_de= v_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. P= arameters: 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 (onl= y >> 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=20 telemetry register function that would need to be held while the=20 callback is invoked. Or if we want to keep doors open to other kinds of=20 lock, a wrapper callback. Thoughts?