<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=koi8-r">
<style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style>
</head>
<body dir="ltr">
<div class="elementToProof" style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
Hello, Maxime</div>
<div class="elementToProof" style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<br>
</div>
<div class="elementToProof" style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
Thank you for your review.</div>
<div class="elementToProof" style="text-align: left; text-indent: 0px; margin-top: 1em; margin-bottom: 1em; font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
If I understand correctly, you propose modifying the <code>VHOST_USER_ASSERT_LOCK()</code> macro so that a
<code>VHOST_USER_SET_MEM_TABLE</code> request does not trigger an assertion.</div>
<div class="elementToProof" style="text-align: left; text-indent: 0px; margin-top: 1em; margin-bottom: 1em; font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
However, I believe such modification would not be appropriate, as it would revert the logic introduced in commit
<code>5e8fcc60b59d</code> ("vhost: enhance virtqueue access lock asserts"). With this approach, we would be performing memory hotplug without queue locking, which could lead to unintended consequences.</div>
<div class="elementToProof" style="text-align: left; text-indent: 0px; margin-top: 1em; margin-bottom: 1em; font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
Regarding VDPA device regression. We faced with this issue when we request the number of lcores that the default amount of memory on the socket cannot handle it.</div>
<div class="elementToProof" style="text-align: left; text-indent: 0px; margin-top: 1em; margin-bottom: 1em; font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
So, regression occurred during the startup part, during device configuration when it creates pkt mbuf pool. </div>
<div class="elementToProof" style="text-align: left; text-indent: 0px; margin-top: 1em; margin-bottom: 1em; font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<br>
</div>
<div class="elementToProof" style="text-align: left; text-indent: 0px; margin-top: 1em; margin-bottom: 1em; font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
Let me know your thoughts regarding this.</div>
<div class="elementToProof" style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 11pt; color: rgb(0, 0, 0);">
<br>
</div>
<div id="appendonsend"></div>
<hr style="display:inline-block;width:98%" tabindex="-1">
<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>От:</b> Maxime Coquelin <maxime.coquelin@redhat.com><br>
<b>Отправлено:</b> 3 июня 2025 г. 15:30<br>
<b>Кому:</b> Danylo Vodopianov <dvo-plv@napatech.com>; thomas@monjalon.net <thomas@monjalon.net>; aman.deep.singh@intel.com <aman.deep.singh@intel.com>; yuying.zhang@intel.com <yuying.zhang@intel.com>; orika@nvidia.com <orika@nvidia.com>; mcoqueli@redhat.com
<mcoqueli@redhat.com>; Christian Koue Muf <ckm@napatech.com>; matan@mellanox.com <matan@mellanox.com>; david.marchand@redhat.com <david.marchand@redhat.com>; Mykola Kostenok <mko-plv@napatech.com>; Serhii Iliushyk <sil-plv@napatech.com><br>
<b>Копия:</b> stephen@networkplumber.org <stephen@networkplumber.org>; dev@dpdk.org <dev@dpdk.org>; Chenbo Xia <chenbox@nvidia.com><br>
<b>Тема:</b> Re: [PATCH v4 1/1] vhost: handle virtqueue locking for memory hotplug</font>
<div> </div>
</div>
<div class="BodyFragment"><font size="2"><span style="font-size:11pt;">
<div class="PlainText">Hello Danylo,<br>
<br>
On 6/2/25 10:50 AM, Danylo Vodopianov wrote:<br>
> For vDPA devices, virtqueues are not locked once the device has been<br>
> configured. In the<br>
> commit 5e8fcc60b59d ("vhost: enhance virtqueue access lock asserts"),<br>
> the asserts were enhanced to trigger rte_panic functionality, preventing<br>
> access to virtqueues without locking. However, this change introduced<br>
> an issue where the memory hotplug mechanism, added in the<br>
> commit 127f9c6f7b78 ("vhost: handle memory hotplug with vDPA devices"),<br>
> no longer works. Since it expects for all queues are locked.<br>
> <br>
> During the initialization of a vDPA device, the driver sets the<br>
> VIRTIO_DEV_VDPA_CONFIGURED flag, which prevents the<br>
> vhost_user_lock_all_queue_pairs function from locking the<br>
> virtqueues. This leads to the error: the VIRTIO_DEV_VDPA_CONFIGURED<br>
> flag allows the use of the hotplug mechanism, but it fails<br>
> because the virtqueues are not locked, while it expects to be locked<br>
> for VHOST_USER_SET_MEM_TABLE in the table VHOST_MESSAGE_HANDLERS.<br>
> <br>
> This patch addresses the issue by enhancing the conditional statement<br>
> to include a new condition. Specifically, when the device receives the<br>
> VHOST_USER_SET_MEM_TABLE request, the virtqueues are locked to update<br>
> the basic configurations and hotplug the guest memory.<br>
> <br>
> This fix does not impact access lock when vDPA driver is configured<br>
> for other unnecessary message handlers.<br>
> <br>
> Manual memory configuring with "--socket-mem" option allows to avoid<br>
> hotplug mechanism using.<br>
<br>
s/using/use/<br>
<br>
It needs a fixes tag, and stable@dpdk.org should be cc'ed, so that it<br>
gets backported to LTS branches.<br>
<br>
> <br>
> Signed-off-by: Danylo Vodopianov <dvo-plv@napatech.com><br>
> ---<br>
> lib/vhost/vhost_user.c | 8 +++++++-<br>
> 1 file changed, 7 insertions(+), 1 deletion(-)<br>
> <br>
> diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c<br>
> index ec950acf97..16d03e1033 100644<br>
> --- a/lib/vhost/vhost_user.c<br>
> +++ b/lib/vhost/vhost_user.c<br>
> @@ -3178,7 +3178,13 @@ vhost_user_msg_handler(int vid, int fd)<br>
> * would cause a dead lock.<br>
> */<br>
> if (msg_handler != NULL && msg_handler->lock_all_qps) {<br>
> - if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {<br>
> + /* Lock all queue pairs if the device is not configured for vDPA,<br>
> + * or if it is configured for vDPA but the request is VHOST_USER_SET_MEM_TABLE.<br>
> + * This ensures proper queue locking for memory table updates and guest<br>
> + * memory hotplug.<br>
> + */<br>
> + if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED) ||<br>
> + request == VHOST_USER_SET_MEM_TABLE) {<br>
<br>
It looks like a workaround, and I'm afraid it could cause regression<br>
with some vDPA devices, or that it would not be enough and we would have <br>
to add other requests as exception.<br>
<br>
<br>
Wouldn't it better to modify VHOST_USER_ASSERT_LOCK() so that it takes<br>
into account the VIRTIO_DEV_VDPA_CONFIGURED flag?<br>
<br>
Thanks,<br>
Maxime<br>
<br>
> vhost_user_lock_all_queue_pairs(dev);<br>
> unlock_required = 1;<br>
> }<br>
<br>
</div>
</span></font></div>
</body>
</html>