[dpdk-dev,v7,05/12] eal: Fix uio mapping differences between linuxapp and bsdapp
Commit Message
From: "Tetsuya.Mukawa" <mukawa@igel.co.jp>
This patch fixes below.
- bsdapp
- Use map_id in pci_uio_map_resource().
- Fix interface of pci_map_resource().
- Move path variable of mapped_pci_resource structure to pci_map.
- linuxapp
- Remove redundant error message of linuxapp.
'pci_uio_map_resource()' is implemented in both linuxapp and bsdapp,
but interface is different. The patch fixes the function of bsdapp
to do same as linuxapp. After applying it, file descriptor should be
opened and closed out of pci_map_resource().
Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
---
lib/librte_eal/bsdapp/eal/eal_pci.c | 118 ++++++++++++++++++------------
lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 21 +++---
2 files changed, 80 insertions(+), 59 deletions(-)
Comments
> -----Original Message-----
> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
> Sent: Tuesday, June 30, 2015 9:24 AM
> To: dev@dpdk.org
> Cc: Iremonger, Bernard; david.marchand@6wind.com; Richardson, Bruce;
> Tetsuya.Mukawa
> Subject: [PATCH v7 05/12] eal: Fix uio mapping differences between linuxapp
> and bsdapp
>
> From: "Tetsuya.Mukawa" <mukawa@igel.co.jp>
>
> This patch fixes below.
> - bsdapp
> - Use map_id in pci_uio_map_resource().
> - Fix interface of pci_map_resource().
> - Move path variable of mapped_pci_resource structure to pci_map.
> - linuxapp
> - Remove redundant error message of linuxapp.
>
> 'pci_uio_map_resource()' is implemented in both linuxapp and bsdapp, but
> interface is different. The patch fixes the function of bsdapp to do same as
> linuxapp. After applying it, file descriptor should be opened and closed out of
> pci_map_resource().
>
> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>
On Tue, Jun 30, 2015 at 05:24:21PM +0900, Tetsuya Mukawa wrote:
> From: "Tetsuya.Mukawa" <mukawa@igel.co.jp>
>
> This patch fixes below.
> - bsdapp
> - Use map_id in pci_uio_map_resource().
> - Fix interface of pci_map_resource().
> - Move path variable of mapped_pci_resource structure to pci_map.
> - linuxapp
> - Remove redundant error message of linuxapp.
>
> 'pci_uio_map_resource()' is implemented in both linuxapp and bsdapp,
> but interface is different. The patch fixes the function of bsdapp
> to do same as linuxapp. After applying it, file descriptor should be
> opened and closed out of pci_map_resource().
>
> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> ---
> lib/librte_eal/bsdapp/eal/eal_pci.c | 118 ++++++++++++++++++------------
> lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 21 +++---
> 2 files changed, 80 insertions(+), 59 deletions(-)
>
<snip>
> free_uio_res:
> + for (i = 0; i < map_idx; i++)
> + rte_free(maps[i].path);
> rte_free(uio_res);
> close_fd:
> close(dev->intr_handle.fd);
Comments on previous patch about merging error labels would also apply here.
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> index c3b259b..19620fe 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> @@ -116,17 +116,11 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
> fd, (off_t)uio_res->maps[i].offset,
> (size_t)uio_res->maps[i].size, 0);
> if (mapaddr != uio_res->maps[i].addr) {
> - if (mapaddr == MAP_FAILED)
> - RTE_LOG(ERR, EAL,
> - "Cannot mmap device resource file %s: %s\n",
> - uio_res->maps[i].path,
> - strerror(errno));
> - else
> - RTE_LOG(ERR, EAL,
> - "Cannot mmap device resource file %s to address: %p\n",
> - uio_res->maps[i].path,
> - uio_res->maps[i].addr);
> -
> + RTE_LOG(ERR, EAL,
> + "Cannot mmap device resource "
> + "file %s to address: %p\n",
> + uio_res->maps[i].path,
> + uio_res->maps[i].addr);
> close(fd);
> return -1;
> }
> @@ -353,8 +347,11 @@ pci_uio_map_resource(struct rte_pci_device *dev)
>
> /* allocate memory to keep path */
> maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
> - if (maps[map_idx].path == NULL)
> + if (maps[map_idx].path == NULL) {
> + RTE_LOG(ERR, EAL, "Cannot allocate memory for "
> + "path: %s\n", strerror(errno));
It's recommended not to split literal strings across multiple lines. This is
so that it's easy to find error messages in the source code. In this case, because
of the split, someone using git grep to search for the error message
"Cannot allocate memory for path"
would not be able to find it in the code. Longer lines are allowed in code for
literal strings.
/Bruce
On 2015/07/02 19:20, Bruce Richardson wrote:
> On Tue, Jun 30, 2015 at 05:24:21PM +0900, Tetsuya Mukawa wrote:
>> From: "Tetsuya.Mukawa" <mukawa@igel.co.jp>
>>
>> This patch fixes below.
>> - bsdapp
>> - Use map_id in pci_uio_map_resource().
>> - Fix interface of pci_map_resource().
>> - Move path variable of mapped_pci_resource structure to pci_map.
>> - linuxapp
>> - Remove redundant error message of linuxapp.
>>
>> 'pci_uio_map_resource()' is implemented in both linuxapp and bsdapp,
>> but interface is different. The patch fixes the function of bsdapp
>> to do same as linuxapp. After applying it, file descriptor should be
>> opened and closed out of pci_map_resource().
>>
>> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
>> ---
>> lib/librte_eal/bsdapp/eal/eal_pci.c | 118 ++++++++++++++++++------------
>> lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 21 +++---
>> 2 files changed, 80 insertions(+), 59 deletions(-)
>>
> <snip>
>> free_uio_res:
>> + for (i = 0; i < map_idx; i++)
>> + rte_free(maps[i].path);
>> rte_free(uio_res);
>> close_fd:
>> close(dev->intr_handle.fd);
> Comments on previous patch about merging error labels would also apply here.
Right, I will fix it also.
>> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
>> index c3b259b..19620fe 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
>> @@ -116,17 +116,11 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
>> fd, (off_t)uio_res->maps[i].offset,
>> (size_t)uio_res->maps[i].size, 0);
>> if (mapaddr != uio_res->maps[i].addr) {
>> - if (mapaddr == MAP_FAILED)
>> - RTE_LOG(ERR, EAL,
>> - "Cannot mmap device resource file %s: %s\n",
>> - uio_res->maps[i].path,
>> - strerror(errno));
>> - else
>> - RTE_LOG(ERR, EAL,
>> - "Cannot mmap device resource file %s to address: %p\n",
>> - uio_res->maps[i].path,
>> - uio_res->maps[i].addr);
>> -
>> + RTE_LOG(ERR, EAL,
>> + "Cannot mmap device resource "
>> + "file %s to address: %p\n",
>> + uio_res->maps[i].path,
>> + uio_res->maps[i].addr);
>> close(fd);
>> return -1;
>> }
>> @@ -353,8 +347,11 @@ pci_uio_map_resource(struct rte_pci_device *dev)
>>
>> /* allocate memory to keep path */
>> maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
>> - if (maps[map_idx].path == NULL)
>> + if (maps[map_idx].path == NULL) {
>> + RTE_LOG(ERR, EAL, "Cannot allocate memory for "
>> + "path: %s\n", strerror(errno));
> It's recommended not to split literal strings across multiple lines. This is
> so that it's easy to find error messages in the source code. In this case, because
> of the split, someone using git grep to search for the error message
> "Cannot allocate memory for path"
> would not be able to find it in the code. Longer lines are allowed in code for
> literal strings.
>
> /Bruce
>
Sure, I will fix it.
Tetsuya
@@ -85,6 +85,7 @@
struct pci_map {
void *addr;
+ char *path;
uint64_t offset;
uint64_t size;
uint64_t phaddr;
@@ -99,7 +100,7 @@ struct mapped_pci_resource {
struct rte_pci_addr pci_addr;
char path[PATH_MAX];
- size_t nb_maps;
+ int nb_maps;
struct pci_map maps[PCI_MAX_RESOURCE];
};
@@ -121,47 +122,30 @@ pci_unbind_kernel_driver(struct rte_pci_device *dev __rte_unused)
/* map a particular resource from a file */
static void *
-pci_map_resource(void *requested_addr, const char *devname, off_t offset,
- size_t size)
+pci_map_resource(void *requested_addr, int fd, off_t offset, size_t size,
+ int additional_flags)
{
- int fd;
void *mapaddr;
- /*
- * open devname, to mmap it
- */
- fd = open(devname, O_RDWR);
- if (fd < 0) {
- RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
- devname, strerror(errno));
- goto fail;
- }
-
/* Map the PCI memory resource of device */
mapaddr = mmap(requested_addr, size, PROT_READ | PROT_WRITE,
- MAP_SHARED, fd, offset);
- close(fd);
- if (mapaddr == MAP_FAILED ||
- (requested_addr != NULL && mapaddr != requested_addr)) {
- RTE_LOG(ERR, EAL, "%s(): cannot mmap(%s(%d), %p, 0x%lx, 0x%lx):"
- " %s (%p)\n", __func__, devname, fd, requested_addr,
+ MAP_SHARED | additional_flags, fd, offset);
+ if (mapaddr == MAP_FAILED) {
+ RTE_LOG(ERR, EAL,
+ "%s(): cannot mmap(%d, %p, 0x%lx, 0x%lx): %s (%p)\n",
+ __func__, fd, requested_addr,
(unsigned long)size, (unsigned long)offset,
strerror(errno), mapaddr);
- goto fail;
- }
-
- RTE_LOG(DEBUG, EAL, " PCI memory mapped at %p\n", mapaddr);
+ } else
+ RTE_LOG(DEBUG, EAL, " PCI memory mapped at %p\n", mapaddr);
return mapaddr;
-
-fail:
- return NULL;
}
static int
pci_uio_map_secondary(struct rte_pci_device *dev)
{
- size_t i;
+ int i, fd;
struct mapped_pci_resource *uio_res;
struct mapped_pci_res_list *uio_res_list =
RTE_TAILQ_CAST(rte_uio_tailq.head, mapped_pci_res_list);
@@ -169,19 +153,34 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
TAILQ_FOREACH(uio_res, uio_res_list, next) {
/* skip this element if it doesn't match our PCI address */
- if (memcmp(&uio_res->pci_addr, &dev->addr, sizeof(dev->addr)))
+ if (rte_eal_compare_pci_addr(&uio_res->pci_addr, &dev->addr))
continue;
for (i = 0; i != uio_res->nb_maps; i++) {
- if (pci_map_resource(uio_res->maps[i].addr,
- uio_res->path,
- (off_t)uio_res->maps[i].offset,
- (size_t)uio_res->maps[i].size)
- != uio_res->maps[i].addr) {
+ /*
+ * open devname, to mmap it
+ */
+ fd = open(uio_res->maps[i].path, O_RDWR);
+ if (fd < 0) {
+ RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
+ uio_res->maps[i].path, strerror(errno));
+ return -1;
+ }
+
+ void *mapaddr = pci_map_resource(uio_res->maps[i].addr,
+ fd, (off_t)uio_res->maps[i].offset,
+ (size_t)uio_res->maps[i].size, 0);
+ if (mapaddr != uio_res->maps[i].addr) {
RTE_LOG(ERR, EAL,
- "Cannot mmap device resource\n");
+ "Cannot mmap device resource "
+ "file %s to address: %p\n",
+ uio_res->maps[i].path,
+ uio_res->maps[i].addr);
+ close(fd);
return -1;
}
+ /* fd is not needed in slave process, close it */
+ close(fd);
}
return 0;
}
@@ -194,7 +193,7 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
static int
pci_uio_map_resource(struct rte_pci_device *dev)
{
- int i, j;
+ int i, map_idx;
char devname[PATH_MAX]; /* contains the /dev/uioX */
void *mapaddr;
uint64_t phaddr;
@@ -246,35 +245,60 @@ pci_uio_map_resource(struct rte_pci_device *dev)
pagesz = sysconf(_SC_PAGESIZE);
maps = uio_res->maps;
- for (i = uio_res->nb_maps = 0; i != PCI_MAX_RESOURCE; i++) {
+ for (i = 0, map_idx = 0; i != PCI_MAX_RESOURCE; i++) {
+ int fd;
- j = uio_res->nb_maps;
/* skip empty BAR */
if ((phaddr = dev->mem_resource[i].phys_addr) == 0)
continue;
+ /* allocate memory to keep path */
+ maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
+ if (maps[map_idx].path == NULL) {
+ RTE_LOG(ERR, EAL, "Cannot allocate memory for "
+ "path: %s\n", strerror(errno));
+ goto free_uio_res;
+ }
+
+ /*
+ * open resource file, to mmap it
+ */
+ fd = open(devname, O_RDWR);
+ if (fd < 0) {
+ RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
+ devname, strerror(errno));
+ rte_free(maps[map_idx].path);
+ goto free_uio_res;
+ }
+
/* if matching map is found, then use it */
offset = i * pagesz;
- maps[j].offset = offset;
- maps[j].phaddr = dev->mem_resource[i].phys_addr;
- maps[j].size = dev->mem_resource[i].len;
- if (maps[j].addr != NULL ||
- (mapaddr = pci_map_resource(NULL, devname, (off_t)offset,
- (size_t)maps[j].size)
- ) == NULL) {
+ mapaddr = pci_map_resource(NULL, fd, (off_t)offset,
+ (size_t)dev->mem_resource[i].len, 0);
+ close(fd);
+ if (mapaddr == MAP_FAILED) {
+ rte_free(maps[map_idx].path);
goto free_uio_res;
}
- maps[j].addr = mapaddr;
- uio_res->nb_maps++;
+ maps[map_idx].phaddr = dev->mem_resource[i].phys_addr;
+ maps[map_idx].size = dev->mem_resource[i].len;
+ maps[map_idx].addr = mapaddr;
+ maps[map_idx].offset = offset;
+ strcpy(maps[map_idx].path, devname);
+ map_idx++;
dev->mem_resource[i].addr = mapaddr;
}
+ uio_res->nb_maps = map_idx;
+
TAILQ_INSERT_TAIL(uio_res_list, uio_res, next);
return 0;
free_uio_res:
+ for (i = 0; i < map_idx; i++)
+ rte_free(maps[i].path);
rte_free(uio_res);
close_fd:
close(dev->intr_handle.fd);
@@ -116,17 +116,11 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
fd, (off_t)uio_res->maps[i].offset,
(size_t)uio_res->maps[i].size, 0);
if (mapaddr != uio_res->maps[i].addr) {
- if (mapaddr == MAP_FAILED)
- RTE_LOG(ERR, EAL,
- "Cannot mmap device resource file %s: %s\n",
- uio_res->maps[i].path,
- strerror(errno));
- else
- RTE_LOG(ERR, EAL,
- "Cannot mmap device resource file %s to address: %p\n",
- uio_res->maps[i].path,
- uio_res->maps[i].addr);
-
+ RTE_LOG(ERR, EAL,
+ "Cannot mmap device resource "
+ "file %s to address: %p\n",
+ uio_res->maps[i].path,
+ uio_res->maps[i].addr);
close(fd);
return -1;
}
@@ -353,8 +347,11 @@ pci_uio_map_resource(struct rte_pci_device *dev)
/* allocate memory to keep path */
maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
- if (maps[map_idx].path == NULL)
+ if (maps[map_idx].path == NULL) {
+ RTE_LOG(ERR, EAL, "Cannot allocate memory for "
+ "path: %s\n", strerror(errno));
goto free_uio_res;
+ }
/*
* open resource file, to mmap it