From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
by inbox.dpdk.org (Postfix) with ESMTP id 4D782A0545;
Tue, 11 Oct 2022 13:56:44 +0200 (CEST)
Received: from [217.70.189.124] (localhost [127.0.0.1])
by mails.dpdk.org (Postfix) with ESMTP id 3DDA542D81;
Tue, 11 Oct 2022 13:56:44 +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 1E73C42D42
for <dev@dpdk.org>; Tue, 11 Oct 2022 13:56:43 +0200 (CEST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;
s=mimecast20190719; t=1665489402;
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=ZYeM4tv+4wDb8NGzOXqX2ykmMH/I/MNDpuqB6VgtjNM=;
b=RNWIBAMh1BfaaLMQsZFGUi40f+ZjoH2SKegEd1fOb9Lk7Dz5mX7tAAyXKtcHaiWLraSBq2
nubLmHPlKTdfZpVaQQsC7H+S/ytZa25saWekwPpJsA9WeF7JWRuAWn78WnAbtCo1bU7lwS
AJgKjzaW1eQ2qlQAGjk0pAyWHvg3VMQ=
Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com
[66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS
(version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id
us-mta-214-EDFkwL0pN1Olik1s3LEixw-1; Tue, 11 Oct 2022 07:56:39 -0400
X-MC-Unique: EDFkwL0pN1Olik1s3LEixw-1
Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com
[10.11.54.7])
(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))
(No client certificate requested)
by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 5CC66811E67;
Tue, 11 Oct 2022 11:56:39 +0000 (UTC)
Received: from [10.39.208.19] (unknown [10.39.208.19])
by smtp.corp.redhat.com (Postfix) with ESMTPS id 789A11402BDC;
Tue, 11 Oct 2022 11:56:38 +0000 (UTC)
Message-ID: <f1614eca-5fbd-e4f0-dde5-c7f7285a3cda@redhat.com>
Date: Tue, 11 Oct 2022 13:56:36 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
Thunderbird/102.3.1
Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
To: "Liu, Changpeng" <changpeng.liu@intel.com>, "dev@dpdk.org" <dev@dpdk.org>
Cc: "Xia, Chenbo" <chenbo.xia@intel.com>
References: <20220906022225.17215-1-changpeng.liu@intel.com>
<2a63f996-84f4-434f-1b19-5dd035870e9d@redhat.com>
<PH0PR11MB5093F694F8E386CD6B5808ECEE4C9@PH0PR11MB5093.namprd11.prod.outlook.com>
<b281daaa-c07c-72cb-cb86-5c2fa3583001@redhat.com>
<PH0PR11MB509307ACDC534051ABAC8399EE4C9@PH0PR11MB5093.namprd11.prod.outlook.com>
<ce550b77-44b7-a5c4-2cd9-0a580abde3bd@redhat.com>
<PH0PR11MB5093DBFC9691EE099F68A64CEE4C9@PH0PR11MB5093.namprd11.prod.outlook.com>
<fcb3a9b8-f3d9-4f4c-ce94-a6d589220813@redhat.com>
<PH0PR11MB509346AFE740B394C6CB81B8EE4F9@PH0PR11MB5093.namprd11.prod.outlook.com>
From: Maxime Coquelin <maxime.coquelin@redhat.com>
In-Reply-To: <PH0PR11MB509346AFE740B394C6CB81B8EE4F9@PH0PR11MB5093.namprd11.prod.outlook.com>
X-Scanned-By: MIMEDefang 3.1 on 10.11.54.7
X-Mimecast-Spam-Score: 0
X-Mimecast-Originator: redhat.com
Content-Language: en-US
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Hi Changpeng,
On 9/21/22 11:52, Liu, Changpeng wrote:
>
>
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Wednesday, September 21, 2022 5:41 PM
>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>> Cc: Xia, Chenbo <chenbo.xia@intel.com>
>> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
>>
>>
>>
>> On 9/20/22 10:43, Liu, Changpeng wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> Sent: Tuesday, September 20, 2022 4:13 PM
>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>>> Cc: Xia, Chenbo <chenbo.xia@intel.com>
>>>> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
>>>>
>>>>
>>>>
>>>> On 9/20/22 09:45, Liu, Changpeng wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>>> Sent: Tuesday, September 20, 2022 3:35 PM
>>>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>>>>> Cc: Xia, Chenbo <chenbo.xia@intel.com>
>>>>>> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 9/20/22 09:29, Liu, Changpeng wrote:
>>>>>>> Hi Maxime,
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>>>>> Sent: Tuesday, September 20, 2022 3:19 PM
>>>>>>>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>>>>>>>> Cc: Xia, Chenbo <chenbo.xia@intel.com>
>>>>>>>> Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 9/6/22 04:22, Changpeng Liu wrote:
>>>>>>>>> Note that this function is in data path, so the thread context
>>>>>>>>> may not same as socket messages processing context, by using
>>>>>>>>> try_lock here, users can have another try in case of VQ's access
>>>>>>>>> lock is held by `vhost-events` thread.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
>>>>>>>>> ---
>>>>>>>>> lib/vhost/vhost.c | 6 +++++-
>>>>>>>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
>>>>>>>>> index 60cb05a0ff..072d2acb7b 100644
>>>>>>>>> --- a/lib/vhost/vhost.c
>>>>>>>>> +++ b/lib/vhost/vhost.c
>>>>>>>>> @@ -1329,7 +1329,11 @@ rte_vhost_vring_call(int vid, uint16_t
>> vring_idx)
>>>>>>>>> if (!vq)
>>>>>>>>> return -1;
>>>>>>>>>
>>>>>>>>> - rte_spinlock_lock(&vq->access_lock);
>>>>>>>>> + if (!rte_spinlock_trylock(&vq->access_lock)) {
>>>>>>>>> + VHOST_LOG_CONFIG(dev->ifname, DEBUG,
>>>>>>>>> + "failed to kick guest, virtqueue busy.\n");
>>>>>>>>> + return -1;
>>>>>>>>> + }
>>>>>>>>>
>>>>>>>>> if (vq_is_packed(dev))
>>>>>>>>> vhost_vring_call_packed(dev, vq);
>>>>>>>>
>>>>>>>> I think that's problematic, because it will break other applications
>>>>>>>> that currently rely on the API to block until the call is done.
>>>>>>>>
>>>>>>>> Just some internal DPDK usage of this API:
>>>>>>>> ./drivers/vdpa/ifc/ifcvf_vdpa.c:871: rte_vhost_vring_call(internal->vid,
>>>>>>>> qid);
>>>>>>>> ./examples/vhost/virtio_net.c:236: rte_vhost_vring_call(dev->vid,
>>>> queue_id);
>>>>>>>> ./examples/vhost/virtio_net.c:446: rte_vhost_vring_call(dev->vid,
>>>> queue_id);
>>>>>>>> ./examples/vhost_blk/vhost_blk.c:99:
>>>>>>>> rte_vhost_vring_call(task->ctrlr->vid, vq->id);
>>>>>>>> ./examples/vhost_blk/vhost_blk.c:134:
>>>>>>>> rte_vhost_vring_call(task->ctrlr->vid, vq->id);
>>>>>>>>
>>>>>>>> This change will break all the above uses.
>>>>>>>>
>>>>>>>> And that's not counting external projects.
>>>>>>>>
>>>>>>>> ou should better introduce a new API that does not block.
>>>>>>> Could you add a new API to do this?
>>>>>> >
>>>>>>> I think we can use the new API in SPDK as a workaround, note that SPDK
>>>> project
>>>>>> is blocked for
>>>>>>> a while which can't be used with DPDK 22.05 or newer.
>>>>>>
>>>>>> DPDK v22.05?
>>>>>> What is the commit introducing the regression?
>>>>> Here is the commit introducing this issue
>>>>> c5736998305d ("vhost: fix missing virtqueue lock protection")
>>>>> Bugzilla ID: 1015
>>>>
>>>> Ok, it cannot be reverted, as it prevents some undefined
>>>> behaviors/crashes.
>>>>
>>>>>>
>>>>>> Note that if we introduce a new API, it won't be backported to stable
>>>>>> branches.
>>>>> I understand, but do we have better idea in short time? we're planning
>>>>> to release SPDK 22.09 recently.
>>>>
>>>> You can have another thread that sends the call?
>>> We already use two threads to do this. Here is the example for existing code in
>> SPDK:
>>>
>>> DPDK vhost-events thread SPDK thread
>>>
>>> SET_VRING_KICK VQ1 ----> Start polling VQ1
>>> Reply to DPDK <---- Done
>>> SET_VRING_KICK VQ2 ----> thread is blocked on VQ's access lock,
>> SPDK thread can't provide reply message
>>>
>>> For example, we can just return for SET_VRING_KICK VQ2 message without
>> checking SPDK thread, but this leave
>>> uncertain replies to VM.
>>
>> I'm sorry but you will have to find a workaround while v22.11 is out and
>> you can consume it. We can neither backport new API nor we can break all
>> the other applications not handling locking failure.
> By processing vhost-user message in asynchronous way in SPDK can be a
> workaround now, we can backport the workaround to SPDK earlier version
> so that it can work with distro DPDK releases.
>>
>> Regarding the new API for v22.11, I should be named something like
>> rte_vhost_vring_call_nonblock(), and ideally should return some like
>> -EAGAIN instead of -1 o that the applications can distinguish between a
>> real failure and a need for retry.
> Agreed, then we can switch to the new API finally.
Just a reminder that -rc2 is in ~ two weeks, have you prepared the patch
adding the new API?
Regards,
Maxime
>> Regards,
>> Maxime
>>
>>>>
>>>>>>
>>>>>>
>>>>>>> Vhost-blk and scsi devices are not same with vhost-net, we need to cover
>>>>>> SeaBIOS and VM
>>>>>>> cases, so we need to start processing vrings after 1 vring is ready.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Maxime
>>>>>>>
>>>>>
>>>
>