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 1ECF045DA5 for ; Mon, 25 Nov 2024 17:44:22 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0BD1A40B9B; Mon, 25 Nov 2024 17:44:22 +0100 (CET) Received: from mail-pf1-f177.google.com (mail-pf1-f177.google.com [209.85.210.177]) by mails.dpdk.org (Postfix) with ESMTP id 30EF640B8C for ; Mon, 25 Nov 2024 17:44:20 +0100 (CET) Received: by mail-pf1-f177.google.com with SMTP id d2e1a72fcca58-7250c199602so1126204b3a.1 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=cecYv3atlg3YpEl/Z4SgC+PWvnGhWScJfudagguo90cXsVb+jTKqanFieSyvQPLMvw 8IH8j9UUtsCV4YUdhCSAGq0VO5VSk7m/d9jL5r/700Byw7i0UagirBMqHi/YKHnmYYwn 1cNXmZiE+DQECHXG0ROi5c8an4vFr3KKalnCWLScRVbdcZDKRhfeLM3YBy4cpUo0icls L8ZhNMN8ZM3Xbctuq6fZM3oE6u1EVSgW+IFLsVPDw1q+2YsTTGaugkhQhlZ4KN4vUTWu KJe6bH834vwyfpwLOkeJS1yGpLfyrmUJicDddslYh/Wjn317YMJxfHzRbbfhTXDo27wN oV0w== X-Forwarded-Encrypted: i=1; AJvYcCV1bA/wvlQT+6rGVLd8/1F5pyq5kP8xwPo8oM5MDtCr9X0oha/kHCc5VFxSOLN8KZfQSZYZdEY=@dpdk.org X-Gm-Message-State: AOJu0YwUGusJuhMpA1SEuz3hEh2O9D5ehMhlb84LfH2Bx0dfNUhXlmZM 4hFMf3m/Q0+8a6Sgw8/qh13JypZsgtWGDMDb+H9+wcnQCCy1BVngnPk1djgN8qM= X-Gm-Gg: ASbGncuwR5o1Ll/K3B95PDuiW+plL0FvkR2wk1YThQc97e7YUdC6ClWKlqk1WAavyKP aKn1Ss+tv97xz4PovduQNzHKXAfS9Js163lhFFEGspg4TLeBA38pqek7+ftI1SleZJr2S5PAP5f +FPEnbptokWmlRxak+n7CRb2CzwWI6v8DOmjTRfagAf/FRGRSvLgqkyvVMpxFve9hbxFadfl8gZ oT663vSVYg5YJOy/LjtoVNh+5DOw663hHSGk6FFTMEDmFCZHpId+06Hd1fyuohzZlJQoTu0lr6r qCabCKJ+s3SQ3RReLYuw1zWflyw= 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: 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 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.