[v3] eal: fix create user mem map repeatedly when it exists
diff mbox series

Message ID 1602840525-8848-1-git-send-email-wangyunjian@huawei.com
State New
Delegated to: David Marchand
Headers show
Series
  • [v3] eal: fix create user mem map repeatedly when it exists
Related show

Checks

Context Check Description
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing fail Testing issues
ci/iol-intel-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed
ci/Intel-compilation success Compilation OK
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/checkpatch success coding style OK

Commit Message

wangyunjian Oct. 16, 2020, 9:28 a.m. UTC
From: Yunjian Wang <wangyunjian@huawei.com>

Currently, a issue that a container has many devices and the
application will map the same memory many times. The kernel
driver returns EEXIST as long as there are overlapping memory
areas. As a result, we repeatedly create new user mem map entry
for the same memory segment and this will lead to no more space
for other user mem maps.

To resolve the issue, add support to remove the same entry in
the function compact_user_maps().

Fixes: 0cbce3a167f1 ("vfio: skip DMA map failure if already mapped")
Cc: stable@dpdk.org

Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
v3:
   Only update commit log and title
---
 lib/librte_eal/linux/eal_vfio.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Thomas Monjalon Oct. 20, 2020, 2:09 p.m. UTC | #1
16/10/2020 11:28, wangyunjian:
> From: Yunjian Wang <wangyunjian@huawei.com>
> 
> Currently, a issue that a container has many devices and the
> application will map the same memory many times. The kernel
> driver returns EEXIST as long as there are overlapping memory
> areas. As a result, we repeatedly create new user mem map entry
> for the same memory segment and this will lead to no more space
> for other user mem maps.
> 
> To resolve the issue, add support to remove the same entry in
> the function compact_user_maps().

Sorry I don't understand the explanations above.
Anatoly, please could you help in rewording?
Thomas Monjalon Nov. 15, 2020, 2:23 p.m. UTC | #2
20/10/2020 16:09, Thomas Monjalon:
> 16/10/2020 11:28, wangyunjian:
> > From: Yunjian Wang <wangyunjian@huawei.com>
> > 
> > Currently, a issue that a container has many devices and the
> > application will map the same memory many times. The kernel
> > driver returns EEXIST as long as there are overlapping memory
> > areas. As a result, we repeatedly create new user mem map entry
> > for the same memory segment and this will lead to no more space
> > for other user mem maps.
> > 
> > To resolve the issue, add support to remove the same entry in
> > the function compact_user_maps().
> 
> Sorry I don't understand the explanations above.
> Anatoly, please could you help in rewording?

Ping for rewording please.
Thomas Monjalon Nov. 22, 2020, 6:20 p.m. UTC | #3
15/11/2020 15:23, Thomas Monjalon:
> 20/10/2020 16:09, Thomas Monjalon:
> > 16/10/2020 11:28, wangyunjian:
> > > From: Yunjian Wang <wangyunjian@huawei.com>
> > > 
> > > Currently, a issue that a container has many devices and the
> > > application will map the same memory many times. The kernel
> > > driver returns EEXIST as long as there are overlapping memory
> > > areas. As a result, we repeatedly create new user mem map entry
> > > for the same memory segment and this will lead to no more space
> > > for other user mem maps.
> > > 
> > > To resolve the issue, add support to remove the same entry in
> > > the function compact_user_maps().
> > 
> > Sorry I don't understand the explanations above.
> > Anatoly, please could you help in rewording?
> 
> Ping for rewording please.

What is the conclusion? This fix is not worth the effort?
wangyunjian Nov. 23, 2020, 7:40 a.m. UTC | #4
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Monday, November 23, 2020 2:20 AM
> To: anatoly.burakov@intel.com; wangyunjian <wangyunjian@huawei.com>
> Cc: stable@dpdk.org; dev@dpdk.org; david.marchand@redhat.com; Lilijun
> (Jerry) <jerry.lilijun@huawei.com>; xudingke <xudingke@huawei.com>;
> bruce.richardson@intel.com; john.mcnamara@intel.com; asafp@nvidia.com
> Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v3] eal: fix create user mem map
> repeatedly when it exists
> 
> 15/11/2020 15:23, Thomas Monjalon:
> > 20/10/2020 16:09, Thomas Monjalon:
> > > 16/10/2020 11:28, wangyunjian:
> > > > From: Yunjian Wang <wangyunjian@huawei.com>
> > > >
> > > > Currently, a issue that a container has many devices and the
> > > > application will map the same memory many times. The kernel driver
> > > > returns EEXIST as long as there are overlapping memory areas. As a
> > > > result, we repeatedly create new user mem map entry for the same
> > > > memory segment and this will lead to no more space for other user
> > > > mem maps.
> > > >
> > > > To resolve the issue, add support to remove the same entry in the
> > > > function compact_user_maps().
> > >
> > > Sorry I don't understand the explanations above.
> > > Anatoly, please could you help in rewording?
> >
> > Ping for rewording please.
> 
> What is the conclusion? This fix is not worth the effort?
> 

In my opinion, this issue needs to be fixed. Currently, the 'user_mem_maps->maps[]'
may store many same user mem maps but the maps array is limited, which will lead
to other mem maps cannot be created because of no more space left.

Thanks,
Yunjian
Burakov, Anatoly Nov. 27, 2020, 12:54 p.m. UTC | #5
On 20-Oct-20 3:09 PM, Thomas Monjalon wrote:
> 16/10/2020 11:28, wangyunjian:
>> From: Yunjian Wang <wangyunjian@huawei.com>
>>
>> Currently, a issue that a container has many devices and the
>> application will map the same memory many times. The kernel
>> driver returns EEXIST as long as there are overlapping memory
>> areas. As a result, we repeatedly create new user mem map entry
>> for the same memory segment and this will lead to no more space
>> for other user mem maps.
>>
>> To resolve the issue, add support to remove the same entry in
>> the function compact_user_maps().
> 
> Sorry I don't understand the explanations above.
> Anatoly, please could you help in rewording?
> 
> 
> 

Apologies for delay, fell through the cracks.

Suggested rewording:

Currently, user mem maps will check if the newly mapped area is adjacent 
to any existing mapping, but will not check if the mapping is identical 
because it assumes that the API will never get called with the same 
mapping twice. This will result in duplicate entries in the user mem 
maps list.

Fix it by also checking for duplicate mappings, and skipping them if 
they are found.

Patch
diff mbox series

diff --git a/lib/librte_eal/linux/eal_vfio.c b/lib/librte_eal/linux/eal_vfio.c
index 380f2f44a..7cff51e16 100644
--- a/lib/librte_eal/linux/eal_vfio.c
+++ b/lib/librte_eal/linux/eal_vfio.c
@@ -167,6 +167,10 @@  adjust_map(struct user_mem_map *src, struct user_mem_map *end,
 static int
 merge_map(struct user_mem_map *left, struct user_mem_map *right)
 {
+	/* merge the same maps into one */
+	if (memcmp(left, right, sizeof(struct user_mem_map)) == 0)
+		goto out;
+
 	if (left->addr + left->len != right->addr)
 		return 0;
 	if (left->iova + left->len != right->iova)
@@ -174,6 +178,7 @@  merge_map(struct user_mem_map *left, struct user_mem_map *right)
 
 	left->len += right->len;
 
+out:
 	memset(right, 0, sizeof(*right));
 
 	return 1;