Hi, Maxime I understand your point. However we coould have a situation like this, could resize twice: VHOST_CONFIG: (/usr/local/var/run/stdvio4) read message VHOST_USER_SET_MEM_TABLE VHOST_CONFIG: (/usr/local/var/run/stdvio4) guest memory region size: 0x40000000 VHOST_CONFIG: (/usr/local/var/run/stdvio4) guest physical addr: 0x140000000 VHOST_CONFIG: (/usr/local/var/run/stdvio4) guest virtual addr: 0x140000000 VHOST_CONFIG: (/usr/local/var/run/stdvio4) host virtual addr: 0x7fde00000000 VHOST_CONFIG: (/usr/local/var/run/stdvio4) mmap addr : 0x7fde00000000 VHOST_CONFIG: (/usr/local/var/run/stdvio4) mmap size : 0x40000000 VHOST_CONFIG: (/usr/local/var/run/stdvio4) mmap align: 0x40000000 VHOST_CONFIG: (/usr/local/var/run/stdvio4) mmap off : 0x0 VHOST_CONFIG: (/usr/local/var/run/stdvio4) guest memory region size: 0x40000000 VHOST_CONFIG: (/usr/local/var/run/stdvio4) guest physical addr: 0x11c0000000 VHOST_CONFIG: (/usr/local/var/run/stdvio4) guest virtual addr: 0x11c0000000 VHOST_CONFIG: (/usr/local/var/run/stdvio4) host virtual addr: 0x7fddc0000000 VHOST_CONFIG: (/usr/local/var/run/stdvio4) mmap addr : 0x7fddc0000000 VHOST_CONFIG: (/usr/local/var/run/stdvio4) mmap size : 0x40000000 VHOST_CONFIG: (/usr/local/var/run/stdvio4) mmap align: 0x40000000 VHOST_CONFIG: (/usr/local/var/run/stdvio4) mmap off : 0x0 When we set memory region twice. After first iteration VIRTIO_DEV_VDPA_CONFIGURED flag will be unset here: https://github.com/DPDK/dpdk/blob/main/lib/vhost/vhost_user.c#L1425 On the second iteration, this leads to an rte_panic, as queues are accessed without a lock. So we should check vhost message id ( VHOST_USER_SET_MEM_TABLE ). However, extend VHOST_USER_ASSERT_LOCK macros with additional check if we work with this message VHOST_USER_SET_MEM_TABLE not handle this case, therefore translate_ring_addresses function calls q_assert_lock directly, without macros wrapper. In this function is check access_lock vhost_virtqueue and this case should be handle. To address the described issue, we need to make the following changes: 1. Extend VHOST_USER_ASSERT_LOCK macro: * Add a check for the VHOST_USER_SET_MEM_TABLE message ID. * Skip rte_panic if the message ID matches VHOST_USER_SET_MEM_TABLE. 2. Modify translate_ring_addresses function: * Extend its signature to include the id parameter (message ID). * Add logic to skip vq_assert_lock if the message ID matches VHOST_USER_SET_MEM_TABLE. 1. Extend VHOST_USER_ASSERT_LOCK Macro: #define VHOST_USER_ASSERT_LOCK(dev, vq, id) \ do { \ if ((id) == VHOST_USER_SET_MEM_TABLE) \ break; \ if (!(vq)->access_ok) \ rte_panic("Virtqueue access lock not held\n"); \ } while (0 1. Modify translate_ring_addresses Function: static int translate_ring_addresses(struct virtio_net *dev, struct vhost_virtqueue *vq, uint32_t id) { if (id != VHOST_USER_SET_MEM_TABLE) vq_assert_lock(dev, vq); // Existing logic for translating ring addresses... // ...existing code... return 0; } This approach requires extending more functions with conditional checks to handle cases where queue locking should be ignored when the memory table is impacted. Do you have any thoughts about this? Or I should rework my patchset according to this described solution above ? ________________________________ От: Maxime Coquelin Отправлено: 12 июня 2025 г. 14:38 Кому: Danylo Vodopianov ; thomas@monjalon.net ; aman.deep.singh@intel.com ; yuying.zhang@intel.com ; orika@nvidia.com ; mcoqueli@redhat.com ; Christian Koue Muf ; matan@mellanox.com ; david.marchand@redhat.com ; Mykola Kostenok ; Serhii Iliushyk Копия: stephen@networkplumber.org ; dev@dpdk.org ; Chenbo Xia Тема: Re: [PATCH v4 1/1] vhost: handle virtqueue locking for memory hotplug 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 > *Отправлено:* 3 июня 2025 г. 15:30 > *Кому:* Danylo Vodopianov ; thomas@monjalon.net > ; aman.deep.singh@intel.com > ; yuying.zhang@intel.com > ; orika@nvidia.com ; > mcoqueli@redhat.com ; Christian Koue Muf > ; matan@mellanox.com ; > david.marchand@redhat.com ; Mykola Kostenok > ; Serhii Iliushyk > *Копия:* stephen@networkplumber.org ; > dev@dpdk.org ; Chenbo Xia > *Тема:* 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 >> --- >> 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; >> } >