[dpdk-dev,v4,5/5] eal: Fix uio mapping differences between linuxapp and bsdapp

Message ID 1432014898-3543-6-git-send-email-mukawa@igel.co.jp (mailing list archive)
State Superseded, archived
Headers

Commit Message

Tetsuya Mukawa May 19, 2015, 5:54 a.m. UTC
  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

Iremonger, Bernard June 15, 2015, 2:31 p.m. UTC | #1
> -----Original Message-----
> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
> Sent: Tuesday, May 19, 2015 6:55 AM
> To: dev@dpdk.org
> Cc: Iremonger, Bernard; Tetsuya Mukawa
> Subject: [PATCH v4 5/5] eal: Fix uio mapping differences between linuxapp
> and bsdapp
> 
> 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(-)
> 
> diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c
> b/lib/librte_eal/bsdapp/eal/eal_pci.c
> index 8261e09..06c564f 100644
> --- a/lib/librte_eal/bsdapp/eal/eal_pci.c
> +++ b/lib/librte_eal/bsdapp/eal/eal_pci.c
> @@ -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);
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> index 2dd83d3..98f4847 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));
>  			goto free_uio_res;
> +		}
> 
>  		/*
>  		 * open resource file, to mmap it
> --
> 2.1.4

Hi Tetsuya,

This patch fails to apply to the latest code.

Applying: eal: Fix uio mapping differences between linuxapp and bsdapp
error: patch failed: lib/librte_eal/bsdapp/eal/eal_pci.c:238
error: lib/librte_eal/bsdapp/eal/eal_pci.c: patch does not apply
error: patch failed: lib/librte_eal/linuxapp/eal/eal_pci_uio.c:326
error: lib/librte_eal/linuxapp/eal/eal_pci_uio.c: patch does not apply

It looks like another rebase is needed.

Regards,

Bernard.
  
Iremonger, Bernard June 15, 2015, 3:08 p.m. UTC | #2
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Iremonger, Bernard
> Sent: Monday, June 15, 2015 3:32 PM
> To: Tetsuya Mukawa; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 5/5] eal: Fix uio mapping differences
> between linuxapp and bsdapp
> 
> > -----Original Message-----
> > From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
> > Sent: Tuesday, May 19, 2015 6:55 AM
> > To: dev@dpdk.org
> > Cc: Iremonger, Bernard; Tetsuya Mukawa
> > Subject: [PATCH v4 5/5] eal: Fix uio mapping differences between
> > linuxapp and bsdapp
> >
> > 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(-)
> >
> > diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c
> > b/lib/librte_eal/bsdapp/eal/eal_pci.c
> > index 8261e09..06c564f 100644
> > --- a/lib/librte_eal/bsdapp/eal/eal_pci.c
> > +++ b/lib/librte_eal/bsdapp/eal/eal_pci.c
> > @@ -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);
> > diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> > b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> > index 2dd83d3..98f4847 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));
> >  			goto free_uio_res;
> > +		}
> >
> >  		/*
> >  		 * open resource file, to mmap it
> > --
> > 2.1.4
> 
> Hi Tetsuya,
> 
> This patch fails to apply to the latest code.
> 
> Applying: eal: Fix uio mapping differences between linuxapp and bsdapp
> error: patch failed: lib/librte_eal/bsdapp/eal/eal_pci.c:238
> error: lib/librte_eal/bsdapp/eal/eal_pci.c: patch does not apply
> error: patch failed: lib/librte_eal/linuxapp/eal/eal_pci_uio.c:326
> error: lib/librte_eal/linuxapp/eal/eal_pci_uio.c: patch does not apply
> 
> It looks like another rebase is needed.
> 
> Regards,
> 
> Bernard.

Hi Tetsuya,

Please ignore the previous email.
This patch applies ok to the latest code (I applied the v3 patch by mistake).

Regards,

Bernard.
  
Thomas Monjalon June 15, 2015, 3:17 p.m. UTC | #3
Hi Bernard,

2015-06-15 15:08, Iremonger, Bernard:
[snip]

In order to avoid filling too much the patchwork page of the patch,
it's better to remove useless lines while keeping only the lines needed
to understand the history.

> > Hi Tetsuya,
> > 
> > This patch fails to apply to the latest code.
> > 
> > Applying: eal: Fix uio mapping differences between linuxapp and bsdapp
> > error: patch failed: lib/librte_eal/bsdapp/eal/eal_pci.c:238
> > error: lib/librte_eal/bsdapp/eal/eal_pci.c: patch does not apply
> > error: patch failed: lib/librte_eal/linuxapp/eal/eal_pci_uio.c:326
> > error: lib/librte_eal/linuxapp/eal/eal_pci_uio.c: patch does not apply
> > 
> > It looks like another rebase is needed.

About patch rebasing, in general, you can do it yourself by forking a
temporary branch before the conflicting commit, and rebasing it after.

> > 
> > Regards,
> > 
> > Bernard.
> 
> Hi Tetsuya,
> 
> Please ignore the previous email.
> This patch applies ok to the latest code (I applied the v3 patch by mistake).
> 
> Regards,
> 
> Bernard.
  

Patch

diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c b/lib/librte_eal/bsdapp/eal/eal_pci.c
index 8261e09..06c564f 100644
--- a/lib/librte_eal/bsdapp/eal/eal_pci.c
+++ b/lib/librte_eal/bsdapp/eal/eal_pci.c
@@ -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);
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
index 2dd83d3..98f4847 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));
 			goto free_uio_res;
+		}
 
 		/*
 		 * open resource file, to mmap it