From: Maxime Coquelin <maxime.coquelin@redhat.com>
To: "Xia, Chenbo" <chenbo.xia@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>,
"david.marchand@redhat.com" <david.marchand@redhat.com>
Cc: "stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-stable] [PATCH v4 4/7] vhost: fix NUMA reallocation with multiqueue
Date: Fri, 18 Jun 2021 10:48:18 +0200 [thread overview]
Message-ID: <e6a38d96-1cff-2e52-aa60-e4855503fac5@redhat.com> (raw)
In-Reply-To: <MN2PR11MB4063639E6A90C1C85A7E814F9C0D9@MN2PR11MB4063.namprd11.prod.outlook.com>
On 6/18/21 10:21 AM, Xia, Chenbo wrote:
> Hi Maxime,
>
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Friday, June 18, 2021 4:01 PM
>> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org;
>> david.marchand@redhat.com
>> Cc: stable@dpdk.org
>> Subject: Re: [PATCH v4 4/7] vhost: fix NUMA reallocation with multiqueue
>>
>>
>>
>> On 6/18/21 6:34 AM, Xia, Chenbo wrote:
>>> Hi Maxime,
>>>
>>>> -----Original Message-----
>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> Sent: Thursday, June 17, 2021 11:38 PM
>>>> To: dev@dpdk.org; david.marchand@redhat.com; Xia, Chenbo
>> <chenbo.xia@intel.com>
>>>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org
>>>> Subject: [PATCH v4 4/7] vhost: fix NUMA reallocation with multiqueue
>>>>
>>>> Since the Vhost-user device initialization has been reworked,
>>>> enabling the application to start using the device as soon as
>>>> the first queue pair is ready, NUMA reallocation no more
>>>> happened on queue pairs other than the first one since
>>>> numa_realloc() was returning early if the device was running.
>>>>
>>>> This patch fixes this issue by only preventing the device
>>>> metadata to be allocated if the device is running. For the
>>>> virtqueues, a vring state change notification is sent to
>>>> notify the application of its disablement. Since the callback
>>>> is supposed to be blocking, it is safe to reallocate it
>>>> afterwards.
>>>
>>> Is there a corner case? Numa_realloc may happen during vhost-user msg
>>> set_vring_addr/kick, set_mem_table and iotlb msg. And iotlb msg will
>>> not take vq access lock. It could happen when numa_realloc happens on
>>> iotlb msg and app accesses vq in the meantime?
>>
>> I think we are safe wrt to numa_realloc(), because the app's
>> .vring_state_changed() callback is only returning when it is no more
>> processing the rings.
>
> Yes, I think it should be. But in this iotlb msg case (take vhost pmd for example),
> can't vhost pmd still access vq since vq access lock is not took? Do I miss something?
Vhost PMD sends RTE_ETH_EVENT_QUEUE_STATE, and my assumption was that
the application would stop processing the rings when handling this
event and only return from the callback when it's one, but this seems
that's not done at least in testpmd. So we may not rely on that after
all :/.
We cannot rely on the VQ's access lock since the goal of numa_realloc is
to reallocate the vhost_virtqueue itself which contains the acces_lock.
Relying on it would cause a use after free.
Maybe the safest thing to do is to just skip the reallocation if
vq->ready == true.
Having vq->ready == true means we already received all the vrings info
from QEMU, which means the driver has already initialized the device.
It should not change runtime behavior compared to this patch since it
would not reallocate anyway.
What do you think?
> Thanks,
> Chenbo
>
>>
>>
>>> Thanks,
>>> Chenbo
>>>
>>>>
>>>> Fixes: d0fcc38f5fa4 ("vhost: improve device readiness notifications")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> ---
>>>> lib/vhost/vhost_user.c | 11 ++++++++---
>>>> 1 file changed, 8 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
>>>> index 0e9e26ebe0..6e7b327ef8 100644
>>>> --- a/lib/vhost/vhost_user.c
>>>> +++ b/lib/vhost/vhost_user.c
>>>> @@ -488,9 +488,6 @@ numa_realloc(struct virtio_net *dev, int index)
>>>> struct batch_copy_elem *new_batch_copy_elems;
>>>> int ret;
>>>>
>>>> - if (dev->flags & VIRTIO_DEV_RUNNING)
>>>> - return dev;
>>>> -
>>>> old_dev = dev;
>>>> vq = old_vq = dev->virtqueue[index];
>>>>
>>>> @@ -506,6 +503,11 @@ numa_realloc(struct virtio_net *dev, int index)
>>>> return dev;
>>>> }
>>>> if (oldnode != newnode) {
>>>> + if (vq->ready) {
>>>> + vq->ready = false;
>>>> + vhost_user_notify_queue_state(dev, index, 0);
>>>> + }
>>>> +
>>>> VHOST_LOG_CONFIG(INFO,
>>>> "reallocate vq from %d to %d node\n", oldnode, newnode);
>>>> vq = rte_malloc_socket(NULL, sizeof(*vq), 0, newnode);
>>>> @@ -558,6 +560,9 @@ numa_realloc(struct virtio_net *dev, int index)
>>>> rte_free(old_vq);
>>>> }
>>>>
>>>> + if (dev->flags & VIRTIO_DEV_RUNNING)
>>>> + goto out;
>>>> +
>>>> /* check if we need to reallocate dev */
>>>> ret = get_mempolicy(&oldnode, NULL, 0, old_dev,
>>>> MPOL_F_NODE | MPOL_F_ADDR);
>>>> --
>>>> 2.31.1
>>>
>
next prev parent reply other threads:[~2021-06-18 8:48 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20210617153739.178011-1-maxime.coquelin@redhat.com>
2021-06-17 15:37 ` [dpdk-stable] [PATCH v4 1/7] vhost: fix missing memory table NUMA realloc Maxime Coquelin
2021-06-18 4:34 ` Xia, Chenbo
2021-06-18 7:40 ` Maxime Coquelin
2021-06-17 15:37 ` [dpdk-stable] [PATCH v4 2/7] vhost: fix missing guest pages " Maxime Coquelin
2021-06-17 15:37 ` [dpdk-stable] [PATCH v4 4/7] vhost: fix NUMA reallocation with multiqueue Maxime Coquelin
2021-06-18 4:34 ` Xia, Chenbo
2021-06-18 8:01 ` Maxime Coquelin
2021-06-18 8:21 ` Xia, Chenbo
2021-06-18 8:48 ` Maxime Coquelin [this message]
2021-06-24 10:49 ` Xia, Chenbo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e6a38d96-1cff-2e52-aa60-e4855503fac5@redhat.com \
--to=maxime.coquelin@redhat.com \
--cc=chenbo.xia@intel.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=stable@dpdk.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).