Re: [PATCH v4 1/1] vhost: handle virtqueue locking for memory hotplug

[Maxime Coquelin <maxime.coquelin@redhat.com>]

Hi Danylo,

On 6/4/25 10:32 AM, Danylo Vodopianov wrote:
> Hello, Maxime
> 
> Thank you for your review.
> If I understand correctly, you propose modifying the | 
> VHOST_USER_ASSERT_LOCK()| macro so that a |VHOST_USER_SET_MEM_TABLE| 
>   request does not trigger an assertion.
> However, I believe such modification would not be appropriate, as it 
> would revert the logic introduced in commit |5e8fcc60b59d| ("vhost: 
> enhance virtqueue access lock asserts"). With this approach, we would be 
> performing memory hotplug without queue locking, which could lead to 
> unintended consequences.
> 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.
> So, regression occurred during the startup part, during device 
> configuration when it creates pkt mbuf pool.
> 
> Let me know your thoughts regarding this.

No, my point was to modify VHOST_USER_ASSERT_LOCK() to no trigger an
assertion in case vDPA is configured, as we don't want to lock in this
case.

Regards,
Maxime

> ------------------------------------------------------------------------
> *От:* Maxime Coquelin <maxime.coquelin@redhat.com>
> *Отправлено:* 3 июня 2025 г. 15:30
> *Кому:* 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>
> *Копия:* stephen@networkplumber.org <stephen@networkplumber.org>; 
> dev@dpdk.org <dev@dpdk.org>; Chenbo Xia <chenbox@nvidia.com>
> *Тема:* Re: [PATCH v4 1/1] vhost: handle virtqueue locking for memory 
> hotplug
> Hello Danylo,
> 
> On 6/2/25 10:50 AM, Danylo Vodopianov wrote:
>> For vDPA devices, virtqueues are not locked once the device has been
>> configured. In the
>> commit 5e8fcc60b59d ("vhost: enhance virtqueue access lock asserts"),
>> the asserts were enhanced to trigger rte_panic functionality, preventing
>> access to virtqueues without locking. However, this change introduced
>> an issue where the memory hotplug mechanism, added in the
>> commit 127f9c6f7b78 ("vhost: handle memory hotplug with vDPA devices"),
>> no longer works. Since it expects for all queues are locked.
>> 
>> During the initialization of a vDPA device, the driver sets the
>> VIRTIO_DEV_VDPA_CONFIGURED flag, which prevents the
>> vhost_user_lock_all_queue_pairs function from locking the
>> virtqueues. This leads to the error: the VIRTIO_DEV_VDPA_CONFIGURED
>> flag allows the use of the hotplug mechanism, but it fails
>> because the virtqueues are not locked, while it expects to be locked
>> for VHOST_USER_SET_MEM_TABLE in the table VHOST_MESSAGE_HANDLERS.
>> 
>> This patch addresses the issue by enhancing the conditional statement
>> to include a new condition. Specifically, when the device receives the
>> VHOST_USER_SET_MEM_TABLE request, the virtqueues are locked to update
>> the basic configurations and hotplug the guest memory.
>> 
>> This fix does not impact access lock when vDPA driver is configured
>> for other unnecessary message handlers.
>> 
>> Manual memory configuring with "--socket-mem" option allows to avoid
>> hotplug mechanism using.
> 
> s/using/use/
> 
> It needs a fixes tag, and stable@dpdk.org should be cc'ed, so that it
> gets backported to LTS branches.
> 
>> 
>> Signed-off-by: Danylo Vodopianov <dvo-plv@napatech.com>
>> ---
>>   lib/vhost/vhost_user.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
>> index ec950acf97..16d03e1033 100644
>> --- a/lib/vhost/vhost_user.c
>> +++ b/lib/vhost/vhost_user.c
>> @@ -3178,7 +3178,13 @@ vhost_user_msg_handler(int vid, int fd)
>>         * would cause a dead lock.
>>         */
>>        if (msg_handler != NULL && msg_handler->lock_all_qps) {
>> -             if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
>> +             /* Lock all queue pairs if the device is not configured for vDPA,
>> +              * or if it is configured for vDPA but the request is VHOST_USER_SET_MEM_TABLE.
>> +              * This ensures proper queue locking for memory table updates and guest
>> +              * memory hotplug.
>> +              */
>> +             if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED) ||
>> +                     request == VHOST_USER_SET_MEM_TABLE) {
> 
> It looks like a workaround, and I'm afraid it could cause regression
> with some vDPA devices, or that it would not be enough and we would have
> to add other requests as exception.
> 
> 
> Wouldn't it better to modify VHOST_USER_ASSERT_LOCK() so that it takes
> into account the VIRTIO_DEV_VDPA_CONFIGURED flag?
> 
> Thanks,
> Maxime
> 
>>                        vhost_user_lock_all_queue_pairs(dev);
>>                        unlock_required = 1;
>>                }
>