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 C77C642BF0; Wed, 31 May 2023 13:13:50 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 710D840F18; Wed, 31 May 2023 13:13:50 +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 313EF40ED7 for ; Wed, 31 May 2023 13:13:48 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1685531627; 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=DWw09JVg/AYH9NWiv7TzkyQCuQe4KfJSA98DSXPo+zE=; b=MFvZTQvxbBU7R+D7CU6AJ7Al0Z7KDZQYU467ETmlICi9Y2eQUCmKd1+O79jl1o8Kg4YeV0 tOw1yuLbR1baCE0Wi4Jfbny3X4aGdsD5/GYhoplZnf+wKIQJqzVznN0fvuQgjv77SY03ld jfG7zh7aecuOR88YmyT7IaWlcsl1Q04= Received: from mail-ej1-f70.google.com (mail-ej1-f70.google.com [209.85.218.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-523-eE-u8VATMmO6RF1tTnDC6Q-1; Wed, 31 May 2023 07:13:46 -0400 X-MC-Unique: eE-u8VATMmO6RF1tTnDC6Q-1 Received: by mail-ej1-f70.google.com with SMTP id a640c23a62f3a-9715654aba1so110545266b.0 for ; Wed, 31 May 2023 04:13:46 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685531625; x=1688123625; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=S6JobI8LDazg+h01vy3nRyQCODKijErBYwrgheY7vqY=; b=ccrvNcgi/1rDC19vNyPm4CntJuCzBw7xS5VBpbNZeNkIXeLFAxcDr3W9aDPq8eVKI+ hsxkMQLPBJxNtk6a9aA1lAb4WtRRabpVwCDu1YhYluViVZyQUfb6A20DT7YpWcQfl/JS iM//FxEZs6vscoaG79FhSsQxLFBN9mTLhlt7Qr0LKnYmNkhupGCb18vUJz5pgwMgMzfz MO5ahDejfxADfhKTN7BmimyIJ1qlINLxffDJLAtz7/6DQ9i7YoQP4NZElFxcMwiV0naq 0KmQFjQtJ+SKV4lqn+qD/3Y0zY8LtnrBaW2gDGXVXK3vx6Byq//eFsMAoFuUCuXX5ltY RUqQ== X-Gm-Message-State: AC+VfDz2yhQhRAifibdCDVke7gVxcccToJU8IpvGmixyL/G7QB+e3lmU c8kwAX+1iiz4kC9hUk0h2Gd9/3ynRm5uISgaGcPMnPmCJjFhSeSoLehxsQKUBLV3U5oGSaNIQD0 bhlk= X-Received: by 2002:a17:907:6e15:b0:966:a691:55f9 with SMTP id sd21-20020a1709076e1500b00966a69155f9mr5475505ejc.30.1685531625300; Wed, 31 May 2023 04:13:45 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ6WpbRNMDqqI/Hrhv79TfbZPnB5PTOYyMeDu9fvgBEYqLlI6eF6TPw5VYXXohhwUrj8dpmzYA== X-Received: by 2002:a17:907:6e15:b0:966:a691:55f9 with SMTP id sd21-20020a1709076e1500b00966a69155f9mr5475487ejc.30.1685531624923; Wed, 31 May 2023 04:13:44 -0700 (PDT) Received: from [10.39.192.204] (5920ab7b.static.cust.trined.nl. [89.32.171.123]) by smtp.gmail.com with ESMTPSA id la11-20020a170906ad8b00b00965f5d778e3sm8832595ejb.120.2023.05.31.04.13.43 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Wed, 31 May 2023 04:13:43 -0700 (PDT) From: Eelco Chaudron To: Maxime Coquelin Cc: "Xia, Chenbo" , david.marchand@redhat.com, dev@dpdk.org Subject: Re: [PATCH v3 1/4] vhost: change vhost_virtqueue access lock to a read/write one Date: Wed, 31 May 2023 13:13:42 +0200 X-Mailer: MailMate (1.14r5964) Message-ID: <04BFE798-9FA7-4D8F-9360-266946DAD5BC@redhat.com> In-Reply-To: References: <168431450017.558450.16680518469610688737.stgit@ebuild.local> <168431452543.558450.14131829672896784074.stgit@ebuild.local> MIME-Version: 1.0 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 31 May 2023, at 11:27, Maxime Coquelin wrote: > On 5/31/23 08:37, Xia, Chenbo wrote: >> Hi Eelco, >> >>> -----Original Message----- >>> From: Eelco Chaudron >>> Sent: Wednesday, May 17, 2023 5:09 PM >>> To: maxime.coquelin@redhat.com; Xia, Chenbo ; >>> david.marchand@redhat.com >>> Cc: dev@dpdk.org >>> Subject: [PATCH v3 1/4] vhost: change vhost_virtqueue access lock to a >>> read/write one >>> >>> This change will allow the vhost interrupt datapath handling to be spli= t >>> between two processed without one of them holding an explicit lock. >>> >>> Signed-off-by: Eelco Chaudron >>> --- >>> lib/eal/include/generic/rte_rwlock.h | 17 ++++++ >>> lib/vhost/vhost.c | 46 +++++++++-------- >>> lib/vhost/vhost.h | 4 +- >>> lib/vhost/vhost_user.c | 14 +++-- >>> lib/vhost/virtio_net.c | 90 +++++++++++++++++--------= ---- >>> ----- >>> 5 files changed, 94 insertions(+), 77 deletions(-) >>> >>> diff --git a/lib/eal/include/generic/rte_rwlock.h >>> b/lib/eal/include/generic/rte_rwlock.h >>> index 71e2d8d5f4..9e083bbc61 100644 >>> --- a/lib/eal/include/generic/rte_rwlock.h >>> +++ b/lib/eal/include/generic/rte_rwlock.h >>> @@ -236,6 +236,23 @@ rte_rwlock_write_unlock(rte_rwlock_t *rwl) >>> =09__atomic_fetch_sub(&rwl->cnt, RTE_RWLOCK_WRITE, __ATOMIC_RELEASE); >>> } >>> >>> +/** >>> + * Test if the write lock is taken. >>> + * >>> + * @param rwl >>> + * A pointer to a rwlock structure. >>> + * @return >>> + * 1 if the write lock is currently taken; 0 otherwise. >>> + */ >>> +static inline int >>> +rte_rwlock_write_is_locked(rte_rwlock_t *rwl) >>> +{ >>> +=09if (__atomic_load_n(&rwl->cnt, __ATOMIC_RELAXED) & RTE_RWLOCK_WRITE= ) >>> +=09=09return 1; >>> + >>> +=09return 0; >>> +} >>> + >> >> Again we need to update release note as it's a new EAL API. >> >>> /** >>> * Try to execute critical section in a hardware memory transaction, = if >>> it >>> * fails or not available take a read lock >>> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c >>> index ef37943817..74bdbfd810 100644 >>> --- a/lib/vhost/vhost.c >>> +++ b/lib/vhost/vhost.c >>> @@ -393,9 +393,9 @@ free_vq(struct virtio_net *dev, struct vhost_virtqu= eue >>> *vq) >>> =09else >>> =09=09rte_free(vq->shadow_used_split); >>> >>> -=09rte_spinlock_lock(&vq->access_lock); >>> +=09rte_rwlock_write_lock(&vq->access_lock); >>> =09vhost_free_async_mem(vq); >>> -=09rte_spinlock_unlock(&vq->access_lock); >>> +=09rte_rwlock_write_unlock(&vq->access_lock); >>> =09rte_free(vq->batch_copy_elems); >>> =09vhost_user_iotlb_destroy(vq); >>> =09rte_free(vq->log_cache); >>> @@ -630,7 +630,7 @@ alloc_vring_queue(struct virtio_net *dev, uint32_t >>> vring_idx) >>> >>> =09=09dev->virtqueue[i] =3D vq; >>> =09=09init_vring_queue(dev, vq, i); >>> -=09=09rte_spinlock_init(&vq->access_lock); >>> +=09=09rte_rwlock_init(&vq->access_lock); >>> =09=09vq->avail_wrap_counter =3D 1; >>> =09=09vq->used_wrap_counter =3D 1; >>> =09=09vq->signalled_used_valid =3D false; >>> @@ -1305,14 +1305,14 @@ rte_vhost_vring_call(int vid, uint16_t vring_id= x) >>> =09if (!vq) >>> =09=09return -1; >>> >>> -=09rte_spinlock_lock(&vq->access_lock); >>> +=09rte_rwlock_read_lock(&vq->access_lock); >>> >>> =09if (vq_is_packed(dev)) >>> =09=09vhost_vring_call_packed(dev, vq); >>> =09else >>> =09=09vhost_vring_call_split(dev, vq); >>> >>> -=09rte_spinlock_unlock(&vq->access_lock); >>> +=09rte_rwlock_read_unlock(&vq->access_lock); >> >> Not sure about this. vhost_ring_call_packed/split is changing some field= in >> Vq. Should we use write lock here? > > I don't think so, the purpose of the access_lock is not to make the > datapath threads-safe, but to protect the datapath from metadata changes > by the control path. Thanks Chinbo for the review, and see Maxime=E2=80=99s comment above. Does = this clarify your concern/question? >> >> Thanks, >> Chenbo >>