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 5DFCD45DA4; Mon, 25 Nov 2024 17:44:21 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E231040B90; Mon, 25 Nov 2024 17:44:20 +0100 (CET) Received: from mail-pf1-f181.google.com (mail-pf1-f181.google.com [209.85.210.181]) by mails.dpdk.org (Postfix) with ESMTP id 35E8240B90 for ; Mon, 25 Nov 2024 17:44:20 +0100 (CET) Received: by mail-pf1-f181.google.com with SMTP id d2e1a72fcca58-724fc1aaa91so1721584b3a.3 for ; Mon, 25 Nov 2024 08:44:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20230601.gappssmtp.com; s=20230601; t=1732553059; x=1733157859; darn=dpdk.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=gv4UjzcZwn1S2/ZEROadx95cRt+9ws81WC79WlkEqcs=; b=vKDW9Qz1HZni0BDOiTfZJNvttKLDsit3fpO4U1YkQCNbrfdhjhpO2kW+T7eEUw+ct1 4HpgcLXK9JOB/z7fgbOJpym0s97H+nqRWMI+SILBtuIitWlrJUQyFP8NhiuS2LKW15Kc iwTqiumuUBwaBj7J/Mq3UEZ40PkkITn4l+vZSLEb+JRjTWQvoZVcT9L6wkhLIHFe6pdR w92VigeOcNpLNjHge+smETIamjYzIjtvprhGlcX7NWLqaP4RRywWz0L0teSEIYadCtfc xkXgi4S+AIG72xChUExoqrl/KeKGywsWEB8ECHnDpzA5GxSZjpRcgyHtj+XGONJj0lRn WgpA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1732553059; x=1733157859; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=gv4UjzcZwn1S2/ZEROadx95cRt+9ws81WC79WlkEqcs=; b=MOOcDrR4nGDmfAO2abC+bjZ9ow51PDJ+MqfHdpMryg45zGKTZ8VxBcpU4+W5k8EVGh BoUZrJGWMrrZ+MV/GacOwIp1kHOXWDlQlaKn5ri9dbqBhxKp2uq5D/bmR5XRTRdH1zT7 7FzYwxX4rIAX3NNzxjx2q5zoHn3Sei+IDeap3ROFva5HslqVqcXySLpYrk3Y4Op1esQh 0gTbJnBmi3qi1axW4RsMxOKqp1EgFabAtQGvXFpTM8l0IiQ4xKGuCJAO0gWAy6UuMFrp qUevInx4vXwWJftyAPDsXAe4xIlUTG2UFHMrs4C1IBDRMqqtoWXizADg5wBdGnjN8Fmf QtVQ== X-Forwarded-Encrypted: i=1; AJvYcCVgCVnANR5vc0TPrlI2FLGwpw6+n1NAEihrfPaSO1Mvjl9jSMexZpDtcmBb63klJWr0x/E=@dpdk.org X-Gm-Message-State: AOJu0YxCC2EMMSta5TDWsUUIfuVLUAiv0sxkitsHP2ukjru7fgZIZnIX wlMVDJMeS3eOUYaAb2sUwh0AbRTsRY39Kjn3+b5mdB5jg8n3sW3wZYKO5WkRF9o= X-Gm-Gg: ASbGncua2tgowcAeCdy0E5FK7xdJnt39XomT0D1jktD4Ly+1XrUlpPCiN5rljKT/2sP wHTNwgJyJGSViAeYZlDTarsEIHaqQbtkW14sx58rqsNsW1WqpW+TrtZE0niMucFKNw8gzIC6iTj j+evEM0PKNgQniif4SzPq69DULfcNMLF/voJ28YDMkACItH92bLkQDXKaDO2bu9pbeUzrYrnKH2 E3Lnw2tIyoW/EE0WWIqvilB3z9fmwIxHZlctGNMzyzk2i/XLDX4M+2X0SO5KGh4zcyfUZtyEspX Es8BchoKJ9YXvOD9wdWjQ6bszwo= X-Google-Smtp-Source: AGHT+IEPPsAX06W/49YS5MceBjcJX573+Z94eyw/3lZt/Yn08oT9kymcstjGtkJgwK/fNsiuoduIag== X-Received: by 2002:a05:6a00:3e1f:b0:71e:4cff:2654 with SMTP id d2e1a72fcca58-724df4db30dmr15323616b3a.6.1732553059051; Mon, 25 Nov 2024 08:44:19 -0800 (PST) Received: from hermes.local (204-195-96-226.wavecable.com. [204.195.96.226]) by smtp.gmail.com with ESMTPSA id 41be03b00d2f7-7fbcc3dde07sm6926559a12.59.2024.11.25.08.44.18 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 25 Nov 2024 08:44:18 -0800 (PST) Date: Mon, 25 Nov 2024 08:44:17 -0800 From: Stephen Hemminger To: Maxime Coquelin Cc: David Marchand , dev@dpdk.org, stable@dpdk.org, Chenbo Xia , Eelco Chaudron Subject: Re: [PATCH] vhost: fix read vs write lock mismatch Message-ID: <20241125084417.66f9562f@hermes.local> In-Reply-To: <326651da-d313-4fa7-8736-1dc139aafb68@redhat.com> References: <20241118162424.9038-1-stephen@networkplumber.org> <9ea5b1d6-bafa-44d5-bdb7-1eaeaa62a587@redhat.com> <326651da-d313-4fa7-8736-1dc139aafb68@redhat.com> MIME-Version: 1.0 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, 25 Nov 2024 17:31:26 +0100 Maxime Coquelin wrote: > On 11/25/24 17:28, David Marchand wrote: > > On Mon, Nov 25, 2024 at 5:20=E2=80=AFPM Maxime Coquelin > > wrote: =20 > >> On 11/25/24 12:14, David Marchand wrote: =20 > >>> On Mon, Nov 18, 2024 at 5:24=E2=80=AFPM Stephen Hemminger > >>> wrote: =20 > >>>> > >>>> If lock is acquired for write, it must be released for write > >>>> or a deadlock is likely. > >>>> > >>>> Bugzilla ID: 1582 > >>>> Fixes: 9fc93a1e2320 ("vhost: fix virtqueue access check in datapath") > >>>> Cc: david.marchand@redhat.com > >>>> Cc: stable@dpdk.org > >>>> > >>>> Signed-off-by: Stephen Hemminger > >>>> --- > >>>> lib/vhost/virtio_net.c | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c > >>>> index 298a5dae74..d764d4bc6a 100644 > >>>> --- a/lib/vhost/virtio_net.c > >>>> +++ b/lib/vhost/virtio_net.c > >>>> @@ -2538,7 +2538,7 @@ virtio_dev_rx_async_submit(struct virtio_net *= dev, struct vhost_virtqueue *vq, > >>>> > >>>> if (unlikely(!vq->access_ok)) { > >>>> vhost_user_iotlb_rd_unlock(vq); > >>>> - rte_rwlock_read_unlock(&vq->access_lock); > >>>> + rte_rwlock_write_unlock(&vq->access_lock); =20 > >>> > >>> A write lock is taken earlier, because virtio_dev_rx_async_submit_* > >>> need it for access to vq->async (as opposed to the sync code that only > >>> takes read lock). > >>> > >>> Here, no need to release/take again all locks. > >>> A simpler fix is to directly call vring_translate(dev, vq). > >>> > >>> =20 > >> > >> Ok, so both solutions are correct. > >> > >> David's one is more optimized, but this is a corner case in the async > >> datapath so not really critical. > >> > >> On the other hand, Stephen's solution keeps the same pattern as the > >> other datapaths. =20 > >=20 > > Ok, it avoids having a special case, and I prefer limiting special case= s too. > >=20 > > I'll send a RFC for new lock annotations (for catching such bug) for > > the next release. =20 >=20 > Great, thanks for that! >=20 > > =20 > >> > >> I'd go with Stephen's solution, but would change the commit title to: > >> > >> vhost: fix deadlock in Rx async path > >> > >> With this change: > >> > >> Reviewed-by: Maxime Coquelin =20 > >=20 > > Yes, the title needed some work. > >=20 > > =20 >=20 > Ack, will change while applying. Yes better title would be great.