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

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

Hi,

On 6/19/25 3:01 PM, Danylo Vodopianov wrote:
> 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 <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
> 
> 
>   2.
>     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 ?

And if instead of relying on the VIRTIO_DEV_VDPA_CONFIGURED flag as I 
suggested, we rely on the vdpa_dev being set?

> 
> 
> ------------------------------------------------------------------------
> *От:* Maxime Coquelin <maxime.coquelin@redhat.com>
> *Отправлено:* 12 июня 2025 г. 14:38
> *Кому:* 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
> 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;
>>>                }
>> 
>