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 6199045A82; Wed, 2 Oct 2024 21:09:57 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 537BF4060A; Wed, 2 Oct 2024 21:09:57 +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 D804E4060A for ; Wed, 2 Oct 2024 21:09:55 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1727896195; 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=cpjqxNKW0LMxqCi+L6gWoStPR8/6FlM61OxjPMM7mYFSIio0q0DMcye4lkGKXSdJKSyBZU tPb4IDBZsUFLQeCAc5noH76DzQPyh3i6kZIfwgwtL5iZs5KpYkIn0rEt3MR6bYfcPbkQDf 75ndaKeHXVoIkk5eXBKs34RRysULweI= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-417-hZiaUnR2P4CDd3eUo05ZPQ-1; Wed, 02 Oct 2024 15:09:52 -0400 X-MC-Unique: hZiaUnR2P4CDd3eUo05ZPQ-1 Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-42cb22d396cso627455e9.0 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=AxAs0RoklEn8xJre8AAKRuBxlKdGxobUav8YX9bApXZ28NBa6H/rdcjfB1axzqW2dz JzAVNNXTqEyruP+/zE7kfV4R39sJtdmiNRXTwCE3OjPPoCBNfLJqTvOBYnkcse8Ys6Jy 7SfZnaejMjDwbJeoFNCLteeS8UwYF2c6vamDxAz2PlZyWPYQrCE5WRw8nn7g02WgI84H P4GZaIITHiwT3aP2L5sbrlWD2COO1lfVdxJfffZE12RyPodzUgsAyNcrrRiQ3ugLunpf 1MhWT2VbCgvYh0U7pV742rNXHikgmlqniNYpBzztFvxPdI3vUUf6pZW1wecE2tzKtpJI J7rQ== X-Gm-Message-State: AOJu0YwSrnryCvOJxA0Z8e8bbqaVsfm6ootdn28a+xfp+MbM3OyP8uio C03XtSD0cbn13HuHABZFYlSE79IZm16RNjN610NgSXMxN5JEtnj9Cgx4dkga5c16H/+19krB2Ri 0MY4FF3fY7xVT4PJH+e1UhkOqx3ApWYCL8tsUZF0s X-Received: by 2002:a05:600c:1d13:b0:42c:df67:1d35 with SMTP id 5b1f17b1804b1-42f777b02a0mr34616545e9.1.1727896191025; 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: 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 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?