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 D892F45B42; Tue, 15 Oct 2024 10:02:20 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B651E402C3; Tue, 15 Oct 2024 10:02:20 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id B8012402C3 for ; Tue, 15 Oct 2024 10:02:18 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1728979338; 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=qwlei1n52bb13P8Oom4Y9it1TzdKNXfnggc8mORVUaY=; b=dbKRd5SVcqdEziLHRadFIjwUDGXBWy4mqHCkQVGx9CDXcHozNKSug09EBghdZcUeWG1oS+ w5CHJX3wSMID1QliOA1yU/J2dlAaIxKe/11Kly8ca0DHUYYRKWR4ZFneFsT0W8ACKw/z6Y KYQrwJq/heY/IRw8IYVcMw+FjBoeJa4= Received: from mail-lf1-f72.google.com (mail-lf1-f72.google.com [209.85.167.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-44-1fBjZAs3Og-i3M8jqIy8Ew-1; Tue, 15 Oct 2024 04:02:15 -0400 X-MC-Unique: 1fBjZAs3Og-i3M8jqIy8Ew-1 Received: by mail-lf1-f72.google.com with SMTP id 2adb3069b0e04-539ea0fcd4bso2059749e87.2 for ; Tue, 15 Oct 2024 01:02:14 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1728979333; x=1729584133; 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=qwlei1n52bb13P8Oom4Y9it1TzdKNXfnggc8mORVUaY=; b=PhKH2s0/IVB/7i+QhmfefcMjOh+1bcfxl7QlmVYfqqF0silKnWqlvU/JwVsxHkMU0j 6Ceiwoc3PeSQBpZgKVJHJIA5IDo4rIm9qvCJPA40uxxhLrlHVIvCn35wrgoUKPqBPsTR 0I13W7UC84Jy9UXv7Kzy/OoIFMHibC1UZxlI7goIDAHfkRgzIEdvMqRF7cDgmavJiIci Hz+BU8/9YT5xYy9AfQ+RXzGYyk5uA5R4K26pmf22dTL0Jqe7RfNr+ySfZe6HcziSlR5F KT3zqO56Ba9iH9MKvsEggM1RpnWI2TEa09eV9Co7Zq2A3bvFgyQdzukdz2fDFoPvxMyo 8fKw== X-Gm-Message-State: AOJu0Ywr/MbS2HhbIfUabIh3Wd1aE2LD6jpeWjoYwOesH9bwHTa0f2ki i91hzcs4hGvmRu/epKVDRatwU7AUbLTfutoUxpc4J2+WrQygKt6DTJDOmqS+v4gex4N0Y2aX1pl fsbJtxLaGyEReICpSTcji2xvzRWntefMPq84xcmw7EnCk5uTDWHGoaKPFr+iBjJ7e02X9C7yW9V Ejn3Y4UlfMu481yRc= X-Received: by 2002:a05:6512:33cb:b0:539:f65b:3f9 with SMTP id 2adb3069b0e04-539f65b15bdmr3161997e87.10.1728979333439; Tue, 15 Oct 2024 01:02:13 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGrcRt7QsC8vo05cLs4KaaBuRHKgr8GtxlfDEUTejvmEdM28MFOtv1pTtZJR3l9qWtxyiwEfwEHgrtVVRmTFOk= X-Received: by 2002:a05:6512:33cb:b0:539:f65b:3f9 with SMTP id 2adb3069b0e04-539f65b15bdmr3161969e87.10.1728979332966; Tue, 15 Oct 2024 01:02:12 -0700 (PDT) MIME-Version: 1.0 References: <20241002155709.2522273-1-david.marchand@redhat.com> <20241014193237.1992382-1-rjarry@redhat.com> <20241014193237.1992382-3-rjarry@redhat.com> <20241014130124.32c7b4bf@hermes.local> In-Reply-To: <20241014130124.32c7b4bf@hermes.local> From: David Marchand Date: Tue, 15 Oct 2024 10:02:01 +0200 Message-ID: Subject: Re: [PATCH dpdk v3 2/2] ethdev: fix potential race in telemetry endpoints To: Stephen Hemminger , Robin Jarry Cc: dev@dpdk.org, Thomas Monjalon , Ferruh Yigit , Andrew Rybchenko , Ciara Power , Bruce Richardson , Keith Wiles , stable@dpdk.org 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 Mon, Oct 14, 2024 at 10:01=E2=80=AFPM Stephen Hemminger wrote: > > On Mon, 14 Oct 2024 21:32:37 +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 > > Acked-by: Bruce Richardson > > --- > > 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 *cm= d __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 =3D arg; > > + rte_spinlock_lock(rte_mcfg_ethdev_get_lock()); > > + ret =3D 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=C3= =A9al. 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 =3D arg; + /* Protect against port removal while invoking callback, calling ethdev API. */ rte_spinlock_lock(rte_mcfg_ethdev_get_lock()); ret =3D fn(cmd, params, d); rte_spinlock_unlock(rte_mcfg_ethdev_get_lock()); --=20 David Marchand