On 9/20/22 09:23, Maxime Coquelin wrote:
On 9/20/22 04:53, Xia, Chenbo wrote:
-----Original Message-----
From: Liu, Changpeng <changpeng....@intel.com>
Sent: Tuesday, September 20, 2022 10:34 AM
To: Xia, Chenbo <chenbo....@intel.com>; dev@dpdk.org
Cc: Maxime Coquelin <maxime.coque...@redhat.com>
Subject: RE: [PATCH] vhost: use try_lock in rte_vhost_vring_call
Hi Bo,
-----Original Message-----
From: Xia, Chenbo <chenbo....@intel.com>
Sent: Tuesday, September 20, 2022 10:25 AM
To: Liu, Changpeng <changpeng....@intel.com>; dev@dpdk.org
Cc: Maxime Coquelin <maxime.coque...@redhat.com>
Subject: RE: [PATCH] vhost: use try_lock in rte_vhost_vring_call
Hi Changpeng,
-----Original Message-----
From: Liu, Changpeng <changpeng....@intel.com>
Sent: Tuesday, September 6, 2022 10:22 AM
To: dev@dpdk.org
Cc: Liu, Changpeng <changpeng....@intel.com>; Maxime Coquelin
<maxime.coque...@redhat.com>; Xia, Chenbo <chenbo....@intel.com>
Subject: [PATCH] vhost: use try_lock in rte_vhost_vring_call
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.
Better to describe the issue this patch wants to fix and how does
it fix.
I remember it's a bz issue, do you want to backport? And it has
some bz ID, we need to add it in commit message.
Actually it's my intention not to add bz ID, as I think for this bz ID,
It's better not to lock all VQ's access lock for KICK/CALLFD messages,
Do you plan to add this change? I think that may be an improvement to
current
locking implementation.
Maxime, what do you think of this idea about only locking specific
queue when
handling vring related message (not global config like mem table)?
I think this is not a good idea.
For example SET_VRING_KICK can currently call
translate_ring_addresses(), which itself can call numa_realloc().
numa_realloc() may reallocate the dev, so you don't want it to be used
by other queues while it happens.
Hmm, actually that may be possible because numa_realloc() reallocs the
dev only if it is not running.
So maybe you can propose something, but you will have to test it
carefully with use-cases involving NUMA reallocation.
What do you think? If this is identified as a fix, I can backport it to
22.05.
You can decide, if this is planned to be the fix, just backport. I am
just
thinking if this is not the fix for the bz, do we still need this?
Thanks,
Chenbo
Signed-off-by: Changpeng Liu <changpeng....@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,
Should use VHOST_LOG_DATA
OK.
Thanks,
Chenbo
+ "failed to kick guest, virtqueue busy.\n");
+ return -1;
+ }
if (vq_is_packed(dev))
vhost_vring_call_packed(dev, vq);
--
2.21.3
