From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <stable-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
	by inbox.dpdk.org (Postfix) with ESMTP id 851C145B42
	for <public@inbox.dpdk.org>; Tue, 15 Oct 2024 10:02:18 +0200 (CEST)
Received: from mails.dpdk.org (localhost [127.0.0.1])
	by mails.dpdk.org (Postfix) with ESMTP id 79D1B400EF;
	Tue, 15 Oct 2024 10:02:18 +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 B07E2400EF
 for <stable@dpdk.org>; Tue, 15 Oct 2024 10:02:16 +0200 (CEST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;
 s=mimecast20190719; t=1728979336;
 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=OqhX/YjvjJ8s8TuPLoSVQakSOStiuDiSLdY3I6EpVr5t7Hb1KGpnVQ+ADxatP5VI98u8nb
 ZaQQSXcGJYkhQVCyvbE44SYpaqlErdK3kilTjCImcQfvQkAzsjWNTmcvMoNVdQoT1cYSPB
 lveaEe2f+tsw+PW021I9AFzgL8AUATE=
Received: from mail-lf1-f70.google.com (mail-lf1-f70.google.com
 [209.85.167.70]) by relay.mimecast.com with ESMTP with STARTTLS
 (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id
 us-mta-42-0lytbJk5OmaSJpKYQYXkTw-1; Tue, 15 Oct 2024 04:02:15 -0400
X-MC-Unique: 0lytbJk5OmaSJpKYQYXkTw-1
Received: by mail-lf1-f70.google.com with SMTP id
 2adb3069b0e04-539ea0fcd4bso2059746e87.2
 for <stable@dpdk.org>; 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=C0PRmdHALX8onVlBNlSOm08Vc9Mu2rIGYLc+MN9hijB5o6CqXexqj2rgcw2c4Z7rdh
 U7PGWXf40t20zDMcZxe/5c78QAycjGxm3XJBV6qx+ZiTJIu2T541kHWCO7gluUGKPL4m
 KayRKXH7bXsXhDa+K/X2lDhzzt1gzIofKVDvU3XFZ1uFZeyoAmBumqwjfyLtkjHpJUxf
 5aStt6nozL3QnPR6l2CivLU7Osuc7o+lohxC0aHc/KhQ4hIYFnh9L8g4YywcZs0fgALT
 Uc/ZM9M0w4UcmkGszQCV7u7l7rrVX59PTOu8GqyGpauQbDGGa1++dtDd0+dvejAeq1ey
 ZCcw==
X-Forwarded-Encrypted: i=1;
 AJvYcCVKQE93A9GvlGwnrEndsLqkMUlMVFlCGwR8NMbaivRF/7+BjXvyMnJLN4GKQR0ZpOgOBIdG2o4=@dpdk.org
X-Gm-Message-State: AOJu0Yz6reVu2lVh3wDq+v6ye83zrg1xlLnJA0VdV/fWPxBRCotrzOl9
 kso5ujSUZcdMPux8t9UPfgwPsDqKpg62SRfLo0COC/D1rmtgXVfalc+mLlWCxI6VMcDjckei6Lv
 8nYeK7oyJxiLC+ZA/4L5E6wRvUxZ+pPRIBt18QgaleIvk2PXJj3mJ3FxRDy5u7cn1J1rnppanRr
 mF7i/A1zKsSlMo2zAhajk=
X-Received: by 2002:a05:6512:33cb:b0:539:f65b:3f9 with SMTP id
 2adb3069b0e04-539f65b15bdmr3161995e87.10.1728979333436; 
 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 <david.marchand@redhat.com>
Date: Tue, 15 Oct 2024 10:02:01 +0200
Message-ID: <CAJFAV8ytVV5DEuBB=ipMEDy2WvFZgSnqZAxWaf2JkCWhCLgupg@mail.gmail.com>
Subject: Re: [PATCH dpdk v3 2/2] ethdev: fix potential race in telemetry
 endpoints
To: Stephen Hemminger <stephen@networkplumber.org>,
 Robin Jarry <rjarry@redhat.com>
Cc: dev@dpdk.org, Thomas Monjalon <thomas@monjalon.net>,
 Ferruh Yigit <ferruh.yigit@amd.com>, 
 Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>,
 Ciara Power <ciara.power@intel.com>, 
 Bruce Richardson <bruce.richardson@intel.com>,
 Keith Wiles <keith.wiles@intel.com>, 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: stable@dpdk.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: patches for DPDK stable branches <stable.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/stable>,
 <mailto:stable-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/stable/>
List-Post: <mailto:stable@dpdk.org>
List-Help: <mailto:stable-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/stable>,
 <mailto:stable-request@dpdk.org?subject=subscribe>
Errors-To: stable-bounces@dpdk.org

On Mon, Oct 14, 2024 at 10:01=E2=80=AFPM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Mon, 14 Oct 2024 21:32:37 +0200
> Robin Jarry <rjarry@redhat.com> 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 <rjarry@redhat.com>
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >  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