From: Maxime Coquelin <maxime.coquelin@redhat.com>
To: "Liu, Changpeng" <changpeng.liu@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>
Cc: "Xia, Chenbo" <chenbo.xia@intel.com>
Subject: Re: [PATCH] vhost: use try_lock in rte_vhost_vring_call
Date: Wed, 21 Sep 2022 11:41:11 +0200 [thread overview]
Message-ID: <fcb3a9b8-f3d9-4f4c-ce94-a6d589220813@redhat.com> (raw)
In-Reply-To: <PH0PR11MB5093DBFC9691EE099F68A64CEE4C9@PH0PR11MB5093.namprd11.prod.outlook.com>
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.
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.
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
>>>>>
>>>
>
next prev parent reply other threads:[~2022-09-21 9:41 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-06 2:22 Changpeng Liu
2022-09-06 21:15 ` Stephen Hemminger
2022-09-07 0:40 ` Liu, Changpeng
2022-09-20 7:12 ` Maxime Coquelin
2022-09-20 2:24 ` Xia, Chenbo
2022-09-20 2:34 ` Liu, Changpeng
2022-09-20 2:53 ` Xia, Chenbo
2022-09-20 3:00 ` Liu, Changpeng
2022-09-20 7:23 ` Maxime Coquelin
2022-09-20 7:30 ` Maxime Coquelin
2022-09-20 7:19 ` Maxime Coquelin
2022-09-20 7:29 ` Liu, Changpeng
2022-09-20 7:34 ` Maxime Coquelin
2022-09-20 7:45 ` Liu, Changpeng
2022-09-20 8:12 ` Maxime Coquelin
2022-09-20 8:43 ` Liu, Changpeng
2022-09-21 9:41 ` Maxime Coquelin [this message]
2022-09-21 9:52 ` Liu, Changpeng
2022-10-11 11:56 ` Maxime Coquelin
2022-10-12 6:40 ` [PATCH v2] vhost: add new `rte_vhost_vring_call_nonblock` API Changpeng Liu
2022-10-13 7:56 ` Maxime Coquelin
2022-10-17 6:46 ` Xia, Chenbo
2022-10-17 7:17 ` Liu, Changpeng
2022-10-17 7:14 ` [PATCH v3] " Changpeng Liu
2022-10-17 7:39 ` Xia, Chenbo
2022-10-17 7:50 ` Liu, Changpeng
2022-10-17 7:48 ` [PATCH v4] " Changpeng Liu
2022-10-19 5:27 ` Xia, Chenbo
2022-10-26 9:24 ` Xia, Chenbo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fcb3a9b8-f3d9-4f4c-ce94-a6d589220813@redhat.com \
--to=maxime.coquelin@redhat.com \
--cc=changpeng.liu@intel.com \
--cc=chenbo.xia@intel.com \
--cc=dev@dpdk.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).