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 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 ; 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: 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" , "dev@dpdk.org" Cc: "Xia, Chenbo" References: <20220906022225.17215-1-changpeng.liu@intel.com> <2a63f996-84f4-434f-1b19-5dd035870e9d@redhat.com> From: Maxime Coquelin In-Reply-To: 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Hi Changpeng, On 9/21/22 11:52, Liu, Changpeng wrote: > > >> -----Original Message----- >> From: Maxime Coquelin >> Sent: Wednesday, September 21, 2022 5:41 PM >> To: Liu, Changpeng ; dev@dpdk.org >> Cc: Xia, Chenbo >> 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 >>>> Sent: Tuesday, September 20, 2022 4:13 PM >>>> To: Liu, Changpeng ; dev@dpdk.org >>>> Cc: Xia, Chenbo >>>> 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 >>>>>> Sent: Tuesday, September 20, 2022 3:35 PM >>>>>> To: Liu, Changpeng ; dev@dpdk.org >>>>>> Cc: Xia, Chenbo >>>>>> 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 >>>>>>>> Sent: Tuesday, September 20, 2022 3:19 PM >>>>>>>> To: Liu, Changpeng ; dev@dpdk.org >>>>>>>> Cc: Xia, Chenbo >>>>>>>> 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 >>>>>>>>> --- >>>>>>>>> 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 >>>>>>> >>>>> >>> >