[dpdk-dev,v6,3/5] eal: Fix memory leaks and needless increment of pci_map_addr
Commit Message
From: "Tetsuya.Mukawa" <mukawa@igel.co.jp>
This patch fixes following memory leaks.
- When open() is failed, uio_res and fds won't be freed in
pci_uio_map_resource().
- When pci_map_resource() is failed but path is allocated correctly,
path and fds won't be freed in pci_uio_map_recource().
Also, some mapped resources should be freed.
- When pci_uio_unmap() is called, path should be freed.
Also, fixes below.
- When pci_map_resource() is failed, mapaddr will be MAP_FAILED.
In this case, pci_map_addr should not be incremented in
pci_uio_map_resource().
- To shrink code, move close().
- Remove fail variable.
Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
---
lib/librte_eal/bsdapp/eal/eal_pci.c | 14 ++++++--
lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 56 ++++++++++++++++++++-----------
2 files changed, 48 insertions(+), 22 deletions(-)
Comments
> -----Original Message-----
> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
> Sent: Friday, June 26, 2015 9:18 AM
> To: dev@dpdk.org
> Cc: Iremonger, Bernard; david.marchand@6wind.com; Tetsuya.Mukawa
> Subject: [PATCH v6 3/5] eal: Fix memory leaks and needless increment of
> pci_map_addr
>
> From: "Tetsuya.Mukawa" <mukawa@igel.co.jp>
>
> This patch fixes following memory leaks.
> - When open() is failed, uio_res and fds won't be freed in
> pci_uio_map_resource().
> - When pci_map_resource() is failed but path is allocated correctly,
> path and fds won't be freed in pci_uio_map_recource().
> Also, some mapped resources should be freed.
> - When pci_uio_unmap() is called, path should be freed.
>
> Also, fixes below.
> - When pci_map_resource() is failed, mapaddr will be MAP_FAILED.
> In this case, pci_map_addr should not be incremented in
> pci_uio_map_resource().
> - To shrink code, move close().
> - Remove fail variable.
>
> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
Acked-by: Bernard Iremonger <Bernard.iremonger@intel.com>
Currently Linux implementation and BSD implementation have almost same
code about pci uio. This patch series cleans up it.
PATCH v7 changes:
- Add below patches. Also, the order of patches are changed.
- eal: Add pci_uio_alloc_resource()
- eal: Add pci_uio_map_resource_by_index()
- eal: Consolidate pci_map and mapped_pci_resource of linuxapp and bsdapp
- eal: Consolidate pci_map/unmap_resource() of linuxapp and bsdapp
- eal: Consolidate pci uio functions of linuxapp and bsdapp
- eal: Consolidate pci_map/unmap_device() of linuxapp and bsdapp
- eal: Consolidate rte_eal_pci_probe/close_one_driver() of linuxapp and bsdapp
(Thanks to Bruce Richardson)
- While adding above, below patches are not changed at all.
- eal: Fix coding style of eal_pci.c and eal_pci_uio.c
- eal: Close file descriptor of uio configuration
- eal: Fix memory leaks and needless increment of pci_map_addr
- eal/bsdapp: Change names of pci related data structure
- eal: Fix uio mapping differences between linuxapp and bsdapp
- some function names are changed like below.
- pci_uio_alloc_uio_resource() to pci_uio_alloc_resource().
- pci_uio_map_uio_resource_by_index() to pci_uio_map_resource_by_index().
(Thanks to Iremonger, Bernard)
PATCH v6 changes:
- Free mapped resources in pci_uio_map_resource().
- Fix error handling in pci_uio_map_resource().
(Thanks to David, Marchand)
PATCH v5 changes:
- Rebase to latest master branch.
PATCH v4 changes:
- Rebase to latest master branch.
- Fix bug in pci_uio_map_resource() of BSD code. 'maps[i].path' shouldn't be freed.
Fixed in below patch:
[PATCH 3/5] eal: Fix memory leaks and needless increment of pci_map_addr
- 'path' member of 'struct mapped_pci_resource' should not be removed because it will be used in BSD code.
Fixed in below patch:
[PATCH 5/5] eal: Fix uio mapping differences between linuxapp and bsdapp
PATCH v3 changes:
- Squash patches related with pci_map_resource().
- Free maps[].path to easy to understand.
(Thanks to Iremonger, Bernard)
- Close fds opened in this function.
- Remove unused path variable from mapped_pci_resource structure.
PATCH v2 changes:
- Move 'if-condition' to later patch series.
- Fix memory leaks of path.
- Fix typos.
(Thanks to David Marchand)
- Fix commit title and body.
- Fix pci_map_resource() to handle MAP_FAILED.
(Thanks to Iremonger, Bernard)
Changes:
- This patch set is derived from below.
"[PATCH v2] eal: Port Hotplug support for BSD"
- Set cfg_fd as -1, when cfg_fd is closed.
(Thanks to Iremonger, Bernard)
- Remove needless coding style fixings.
- Fix coding style of if-else condition.
(Thanks to Richardson, Bruce)
Tetsuya.Mukawa (12):
eal: Fix coding style of eal_pci.c and eal_pci_uio.c
eal: Close file descriptor of uio configuration
eal: Fix memory leaks and needless increment of pci_map_addr
eal/bsdapp: Change names of pci related data structure
eal: Fix uio mapping differences between linuxapp and bsdapp
eal: Add pci_uio_alloc_resource()
eal: Add pci_uio_map_resource_by_index()
eal: Consolidate pci_map and mapped_pci_resource of linuxapp and
bsdapp
eal: Consolidate pci_map/unmap_resource() of linuxapp and bsdapp
eal: Consolidate pci uio functions of linuxapp and bsdapp
eal: Consolidate pci_map/unmap_device() of linuxapp and bsdapp
eal: Consolidate rte_eal_pci_probe/close_one_driver() of linuxapp and
bsdapp
lib/librte_eal/bsdapp/eal/Makefile | 1 +
lib/librte_eal/bsdapp/eal/eal_pci.c | 283 ++++++--------------
.../bsdapp/eal/include/exec-env/rte_interrupts.h | 1 +
lib/librte_eal/common/eal_common_pci.c | 225 ++++++++++++++++
lib/librte_eal/common/eal_common_pci_uio.c | 270 +++++++++++++++++++
lib/librte_eal/common/eal_private.h | 59 ++++-
lib/librte_eal/common/include/rte_pci.h | 40 +++
lib/librte_eal/linuxapp/eal/Makefile | 1 +
lib/librte_eal/linuxapp/eal/eal_pci.c | 236 +----------------
lib/librte_eal/linuxapp/eal/eal_pci_init.h | 39 +--
lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 290 ++++++---------------
lib/librte_ether/rte_ethdev.c | 1 +
12 files changed, 753 insertions(+), 693 deletions(-)
create mode 100644 lib/librte_eal/common/eal_common_pci_uio.c
On Tue, Jun 30, 2015 at 05:24:16PM +0900, Tetsuya Mukawa wrote:
> Currently Linux implementation and BSD implementation have almost same
> code about pci uio. This patch series cleans up it.
>
Overall, patchset looks a good idea. I've made some comments on some of the
individual patches. Quick test on FreeBSD shows that PCI port scanning and
mapping of the BARs of 82599 NICs works ok.
Tested-by: Bruce Richardson <bruce.richardson@intel.com>
On 2015/07/02 20:32, Bruce Richardson wrote:
> On Tue, Jun 30, 2015 at 05:24:16PM +0900, Tetsuya Mukawa wrote:
>> Currently Linux implementation and BSD implementation have almost same
>> code about pci uio. This patch series cleans up it.
>>
> Overall, patchset looks a good idea. I've made some comments on some of the
> individual patches. Quick test on FreeBSD shows that PCI port scanning and
> mapping of the BARs of 82599 NICs works ok.
>
> Tested-by: Bruce Richardson <bruce.richardson@intel.com>
>
I appreciate your testing.
Just for other reviewers, here is my test environment.
- FreeBSD 10.1
- 82572EI Gigabit Ethernet Controller (Copper) (rev 06)
Regards,
Tetsuya
@@ -235,7 +235,7 @@ pci_uio_map_resource(struct rte_pci_device *dev)
if ((uio_res = rte_zmalloc("UIO_RES", sizeof (*uio_res), 0)) == NULL) {
RTE_LOG(ERR, EAL,
"%s(): cannot store uio mmap details\n", __func__);
- return -1;
+ goto close_fd;
}
snprintf(uio_res->path, sizeof(uio_res->path), "%s", devname);
@@ -262,8 +262,7 @@ pci_uio_map_resource(struct rte_pci_device *dev)
(mapaddr = pci_map_resource(NULL, devname, (off_t)offset,
(size_t)maps[j].size)
) == NULL) {
- rte_free(uio_res);
- return -1;
+ goto free_uio_res;
}
maps[j].addr = mapaddr;
@@ -274,6 +273,15 @@ pci_uio_map_resource(struct rte_pci_device *dev)
TAILQ_INSERT_TAIL(uio_res_list, uio_res, next);
return 0;
+
+free_uio_res:
+ rte_free(uio_res);
+close_fd:
+ close(dev->intr_handle.fd);
+ dev->intr_handle.fd = -1;
+ dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
+
+ return -1;
}
/* Scan one pci sysfs entry, and fill the devices list from it. */
@@ -308,7 +308,7 @@ pci_uio_map_resource(struct rte_pci_device *dev)
if (dev->intr_handle.uio_cfg_fd < 0) {
RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
cfgname, strerror(errno));
- return -1;
+ goto close_fd;
}
if (dev->kdrv == RTE_KDRV_IGB_UIO)
@@ -319,7 +319,7 @@ pci_uio_map_resource(struct rte_pci_device *dev)
/* set bus master that is not done by uio_pci_generic */
if (pci_uio_set_bus_master(dev->intr_handle.uio_cfg_fd)) {
RTE_LOG(ERR, EAL, "Cannot set up bus mastering!\n");
- return -1;
+ goto close_fd;
}
}
@@ -328,7 +328,7 @@ pci_uio_map_resource(struct rte_pci_device *dev)
if (uio_res == NULL) {
RTE_LOG(ERR, EAL,
"%s(): cannot store uio mmap details\n", __func__);
- return -1;
+ goto close_fd;
}
snprintf(uio_res->path, sizeof(uio_res->path), "%s", devname);
@@ -338,7 +338,6 @@ pci_uio_map_resource(struct rte_pci_device *dev)
maps = uio_res->maps;
for (i = 0, map_idx = 0; i != PCI_MAX_RESOURCE; i++) {
int fd;
- int fail = 0;
/* skip empty BAR */
phaddr = dev->mem_resource[i].phys_addr;
@@ -352,6 +351,11 @@ pci_uio_map_resource(struct rte_pci_device *dev)
loc->domain, loc->bus, loc->devid, loc->function,
i);
+ /* allocate memory to keep path */
+ maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
+ if (maps[map_idx].path == NULL)
+ goto free_uio_res;
+
/*
* open resource file, to mmap it
*/
@@ -359,7 +363,8 @@ pci_uio_map_resource(struct rte_pci_device *dev)
if (fd < 0) {
RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
devname, strerror(errno));
- return -1;
+ rte_free(maps[map_idx].path);
+ goto free_uio_res;
}
/* try mapping somewhere close to the end of hugepages */
@@ -368,23 +373,15 @@ pci_uio_map_resource(struct rte_pci_device *dev)
mapaddr = pci_map_resource(pci_map_addr, fd, 0,
(size_t)dev->mem_resource[i].len, 0);
- if (mapaddr == MAP_FAILED)
- fail = 1;
+ close(fd);
+ if (mapaddr == MAP_FAILED) {
+ rte_free(maps[map_idx].path);
+ goto free_uio_res;
+ }
pci_map_addr = RTE_PTR_ADD(mapaddr,
(size_t)dev->mem_resource[i].len);
- maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
- if (maps[map_idx].path == NULL)
- fail = 1;
-
- if (fail) {
- rte_free(uio_res);
- close(fd);
- return -1;
- }
- close(fd);
-
maps[map_idx].phaddr = dev->mem_resource[i].phys_addr;
maps[map_idx].size = dev->mem_resource[i].len;
maps[map_idx].addr = mapaddr;
@@ -399,6 +396,25 @@ pci_uio_map_resource(struct rte_pci_device *dev)
TAILQ_INSERT_TAIL(uio_res_list, uio_res, next);
return 0;
+
+free_uio_res:
+ for (i = 0; i < map_idx; i++) {
+ pci_unmap_resource(uio_res->maps[i].addr,
+ (size_t)uio_res->maps[i].size);
+ rte_free(maps[i].path);
+ }
+ rte_free(uio_res);
+close_fd:
+ if (dev->intr_handle.uio_cfg_fd >= 0) {
+ close(dev->intr_handle.uio_cfg_fd);
+ dev->intr_handle.uio_cfg_fd = -1;
+ }
+
+ close(dev->intr_handle.fd);
+ dev->intr_handle.fd = -1;
+ dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
+
+ return -1;
}
#ifdef RTE_LIBRTE_EAL_HOTPLUG
@@ -410,9 +426,11 @@ pci_uio_unmap(struct mapped_pci_resource *uio_res)
if (uio_res == NULL)
return;
- for (i = 0; i != uio_res->nb_maps; i++)
+ for (i = 0; i != uio_res->nb_maps; i++) {
pci_unmap_resource(uio_res->maps[i].addr,
(size_t)uio_res->maps[i].size);
+ rte_free(uio_res->maps[i].path);
+ }
}
static struct mapped_pci_resource *