Re: [dpdk-stable] [PATCH v2 2/2] contigmem: don't zero the pages during each mmap

[Shahaf Shuler <shahafs@mellanox.com>]

Nélio, related to the issue you saw with secondary process support?

--Shahaf
Monday, May 22, 2017 12:04 PM, Tiwei Bie:
> Don't zero the pages during each mmap. Instead, only zero the pages when
> they are not already mmapped. Otherwise, the multi-process support will be
> broken, as the pages will be zeroed when secondary processes map the
> memory. Besides, track the open and mmap operations on the cdev, and
> prevent the module from being unloaded when it is still in use.
> 
> Fixes: 82f931805506 ("contigmem: zero all pages during mmap")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  lib/librte_eal/bsdapp/contigmem/contigmem.c | 183
> ++++++++++++++++++++++++----
>  1 file changed, 157 insertions(+), 26 deletions(-)
> 
> diff --git a/lib/librte_eal/bsdapp/contigmem/contigmem.c
> b/lib/librte_eal/bsdapp/contigmem/contigmem.c
> index 03e3e8def..b201d7bf4 100644
> --- a/lib/librte_eal/bsdapp/contigmem/contigmem.c
> +++ b/lib/librte_eal/bsdapp/contigmem/contigmem.c
> @@ -50,24 +50,37 @@ __FBSDID("$FreeBSD$");
> 
>  #include <vm/vm.h>
>  #include <vm/pmap.h>
> +#include <vm/vm_param.h>
>  #include <vm/vm_object.h>
>  #include <vm/vm_page.h>
>  #include <vm/vm_pager.h>
> +#include <vm/vm_phys.h>
> +
> +struct contigmem_buffer {
> +	void           *addr;
> +	int             refcnt;
> +	struct mtx      mtx;
> +};
> +
> +struct contigmem_vm_handle {
> +	int             buffer_index;
> +};
> 
>  static int              contigmem_load(void);
>  static int              contigmem_unload(void);
>  static int              contigmem_physaddr(SYSCTL_HANDLER_ARGS);
> 
> -static d_mmap_t         contigmem_mmap;
>  static d_mmap_single_t  contigmem_mmap_single;
>  static d_open_t         contigmem_open;
> +static d_close_t        contigmem_close;
> 
>  static int              contigmem_num_buffers =
> RTE_CONTIGMEM_DEFAULT_NUM_BUFS;
>  static int64_t          contigmem_buffer_size =
> RTE_CONTIGMEM_DEFAULT_BUF_SIZE;
> 
>  static eventhandler_tag contigmem_eh_tag;
> -static void
> *contigmem_buffers[RTE_CONTIGMEM_MAX_NUM_BUFS];
> +static struct contigmem_buffer
> +contigmem_buffers[RTE_CONTIGMEM_MAX_NUM_BUFS];
>  static struct cdev     *contigmem_cdev = NULL;
> +static int              contigmem_refcnt;
> 
>  TUNABLE_INT("hw.contigmem.num_buffers", &contigmem_num_buffers);
> TUNABLE_QUAD("hw.contigmem.buffer_size", &contigmem_buffer_size);
> @@ -78,6 +91,8 @@ SYSCTL_INT(_hw_contigmem, OID_AUTO,
> num_buffers, CTLFLAG_RD,
>  	&contigmem_num_buffers, 0, "Number of contigmem buffers
> allocated");  SYSCTL_QUAD(_hw_contigmem, OID_AUTO, buffer_size,
> CTLFLAG_RD,
>  	&contigmem_buffer_size, 0, "Size of each contiguous buffer");
> +SYSCTL_INT(_hw_contigmem, OID_AUTO, num_references, CTLFLAG_RD,
> +	&contigmem_refcnt, 0, "Number of references to contigmem");
> 
>  static SYSCTL_NODE(_hw_contigmem, OID_AUTO, physaddr, CTLFLAG_RD,
> 0,
>  	"physaddr");
> @@ -114,9 +129,10 @@ MODULE_VERSION(contigmem, 1);  static struct
> cdevsw contigmem_ops = {
>  	.d_name         = "contigmem",
>  	.d_version      = D_VERSION,
> -	.d_mmap         = contigmem_mmap,
> +	.d_flags        = D_TRACKCLOSE,
>  	.d_mmap_single  = contigmem_mmap_single,
>  	.d_open         = contigmem_open,
> +	.d_close        = contigmem_close,
>  };
> 
>  static int
> @@ -124,6 +140,7 @@ contigmem_load()
>  {
>  	char index_string[8], description[32];
>  	int  i, error = 0;
> +	void *addr;
> 
>  	if (contigmem_num_buffers >
> RTE_CONTIGMEM_MAX_NUM_BUFS) {
>  		printf("%d buffers requested is greater than %d allowed\n",
> @@ -141,18 +158,20 @@ contigmem_load()
>  	}
> 
>  	for (i = 0; i < contigmem_num_buffers; i++) {
> -		contigmem_buffers[i] =
> -				contigmalloc(contigmem_buffer_size,
> M_CONTIGMEM, M_ZERO, 0,
> -			BUS_SPACE_MAXADDR, contigmem_buffer_size, 0);
> -
> -		if (contigmem_buffers[i] == NULL) {
> +		addr = contigmalloc(contigmem_buffer_size,
> M_CONTIGMEM, M_ZERO,
> +			0, BUS_SPACE_MAXADDR, contigmem_buffer_size,
> 0);
> +		if (addr == NULL) {
>  			printf("contigmalloc failed for buffer %d\n", i);
>  			error = ENOMEM;
>  			goto error;
>  		}
> 
> -		printf("%2u: virt=%p phys=%p\n", i, contigmem_buffers[i],
> -				(void
> *)pmap_kextract((vm_offset_t)contigmem_buffers[i]));
> +		printf("%2u: virt=%p phys=%p\n", i, addr,
> +			(void *)pmap_kextract((vm_offset_t)addr));
> +
> +		mtx_init(&contigmem_buffers[i].mtx, "contigmem", NULL,
> MTX_DEF);
> +		contigmem_buffers[i].addr = addr;
> +		contigmem_buffers[i].refcnt = 0;
> 
>  		snprintf(index_string, sizeof(index_string), "%d", i);
>  		snprintf(description, sizeof(description), @@ -170,10 +189,13
> @@ contigmem_load()
>  	return 0;
> 
>  error:
> -	for (i = 0; i < contigmem_num_buffers; i++)
> -		if (contigmem_buffers[i] != NULL)
> -			contigfree(contigmem_buffers[i],
> contigmem_buffer_size,
> -					M_CONTIGMEM);
> +	for (i = 0; i < contigmem_num_buffers; i++) {
> +		if (contigmem_buffers[i].addr != NULL)
> +			contigfree(contigmem_buffers[i].addr,
> +				contigmem_buffer_size, M_CONTIGMEM);
> +		if (mtx_initialized(&contigmem_buffers[i].mtx))
> +			mtx_destroy(&contigmem_buffers[i].mtx);
> +	}
> 
>  	return error;
>  }
> @@ -183,16 +205,22 @@ contigmem_unload()  {
>  	int i;
> 
> +	if (contigmem_refcnt > 0)
> +		return EBUSY;
> +
>  	if (contigmem_cdev != NULL)
>  		destroy_dev(contigmem_cdev);
> 
>  	if (contigmem_eh_tag != NULL)
>  		EVENTHANDLER_DEREGISTER(process_exit,
> contigmem_eh_tag);
> 
> -	for (i = 0; i < RTE_CONTIGMEM_MAX_NUM_BUFS; i++)
> -		if (contigmem_buffers[i] != NULL)
> -			contigfree(contigmem_buffers[i],
> contigmem_buffer_size,
> -					M_CONTIGMEM);
> +	for (i = 0; i < RTE_CONTIGMEM_MAX_NUM_BUFS; i++) {
> +		if (contigmem_buffers[i].addr != NULL)
> +			contigfree(contigmem_buffers[i].addr,
> +				contigmem_buffer_size, M_CONTIGMEM);
> +		if (mtx_initialized(&contigmem_buffers[i].mtx))
> +			mtx_destroy(&contigmem_buffers[i].mtx);
> +	}
> 
>  	return 0;
>  }
> @@ -203,7 +231,7 @@ contigmem_physaddr(SYSCTL_HANDLER_ARGS)
>  	uint64_t	physaddr;
>  	int		index = (int)(uintptr_t)arg1;
> 
> -	physaddr = (uint64_t)vtophys(contigmem_buffers[index]);
> +	physaddr = (uint64_t)vtophys(contigmem_buffers[index].addr);
>  	return sysctl_handle_64(oidp, &physaddr, 0, req);  }
> 
> @@ -211,22 +239,118 @@ static int
>  contigmem_open(struct cdev *cdev, int fflags, int devtype,
>  		struct thread *td)
>  {
> +
> +	atomic_add_int(&contigmem_refcnt, 1);
> +
> +	return 0;
> +}
> +
> +static int
> +contigmem_close(struct cdev *cdev, int fflags, int devtype,
> +		struct thread *td)
> +{
> +
> +	atomic_subtract_int(&contigmem_refcnt, 1);
> +
>  	return 0;
>  }
> 
>  static int
> -contigmem_mmap(struct cdev *cdev, vm_ooffset_t offset, vm_paddr_t
> *paddr,
> -		int prot, vm_memattr_t *memattr)
> +contigmem_cdev_pager_ctor(void *handle, vm_ooffset_t size, vm_prot_t
> prot,
> +		vm_ooffset_t foff, struct ucred *cred, u_short *color)
>  {
> +	struct contigmem_vm_handle *vmh = handle;
> +	struct contigmem_buffer *buf;
> +
> +	buf = &contigmem_buffers[vmh->buffer_index];
> +
> +	atomic_add_int(&contigmem_refcnt, 1);
> +
> +	mtx_lock(&buf->mtx);
> +	if (buf->refcnt == 0)
> +		memset(buf->addr, 0, contigmem_buffer_size);
> +	buf->refcnt++;
> +	mtx_unlock(&buf->mtx);
> 
> -	*paddr = offset;
>  	return 0;
>  }
> 
> +static void
> +contigmem_cdev_pager_dtor(void *handle) {
> +	struct contigmem_vm_handle *vmh = handle;
> +	struct contigmem_buffer *buf;
> +
> +	buf = &contigmem_buffers[vmh->buffer_index];
> +
> +	mtx_lock(&buf->mtx);
> +	buf->refcnt--;
> +	mtx_unlock(&buf->mtx);
> +
> +	free(vmh, M_CONTIGMEM);
> +
> +	atomic_subtract_int(&contigmem_refcnt, 1); }
> +
> +static int
> +contigmem_cdev_pager_fault(vm_object_t object, vm_ooffset_t offset,
> int prot,
> +		vm_page_t *mres)
> +{
> +	vm_paddr_t paddr;
> +	vm_page_t m_paddr, page;
> +	vm_memattr_t memattr, memattr1;
> +
> +	memattr = object->memattr;
> +
> +	VM_OBJECT_WUNLOCK(object);
> +
> +	paddr = offset;
> +
> +	m_paddr = vm_phys_paddr_to_vm_page(paddr);
> +	if (m_paddr != NULL) {
> +		memattr1 = pmap_page_get_memattr(m_paddr);
> +		if (memattr1 != memattr)
> +			memattr = memattr1;
> +	}
> +
> +	if (((*mres)->flags & PG_FICTITIOUS) != 0) {
> +		/*
> +		 * If the passed in result page is a fake page, update it with
> +		 * the new physical address.
> +		 */
> +		page = *mres;
> +		VM_OBJECT_WLOCK(object);
> +		vm_page_updatefake(page, paddr, memattr);
> +	} else {
> +		/*
> +		 * Replace the passed in reqpage page with our own fake
> page and
> +		 * free up the all of the original pages.
> +		 */
> +		page = vm_page_getfake(paddr, memattr);
> +		VM_OBJECT_WLOCK(object);
> +		vm_page_replace_checked(page, object, (*mres)->pindex,
> *mres);
> +		vm_page_lock(*mres);
> +		vm_page_free(*mres);
> +		vm_page_unlock(*mres);
> +		*mres = page;
> +	}
> +
> +	page->valid = VM_PAGE_BITS_ALL;
> +
> +	return VM_PAGER_OK;
> +}
> +
> +static struct cdev_pager_ops contigmem_cdev_pager_ops = {
> +	.cdev_pg_ctor = contigmem_cdev_pager_ctor,
> +	.cdev_pg_dtor = contigmem_cdev_pager_dtor,
> +	.cdev_pg_fault = contigmem_cdev_pager_fault, };
> +
>  static int
>  contigmem_mmap_single(struct cdev *cdev, vm_ooffset_t *offset,
> vm_size_t size,
>  		struct vm_object **obj, int nprot)
>  {
> +	struct contigmem_vm_handle *vmh;
>  	uint64_t buffer_index;
> 
>  	/*
> @@ -238,10 +362,17 @@ contigmem_mmap_single(struct cdev *cdev,
> vm_ooffset_t *offset, vm_size_t size,
>  	if (buffer_index >= contigmem_num_buffers)
>  		return EINVAL;
> 
> -	memset(contigmem_buffers[buffer_index], 0,
> contigmem_buffer_size);
> -	*offset =
> (vm_ooffset_t)vtophys(contigmem_buffers[buffer_index]);
> -	*obj = vm_pager_allocate(OBJT_DEVICE, cdev, size, nprot, *offset,
> -			curthread->td_ucred);
> +	if (size > contigmem_buffer_size)
> +		return EINVAL;
> +
> +	vmh = malloc(sizeof(*vmh), M_CONTIGMEM, M_NOWAIT |
> M_ZERO);
> +	if (vmh == NULL)
> +		return ENOMEM;
> +	vmh->buffer_index = buffer_index;
> +
> +	*offset =
> (vm_ooffset_t)vtophys(contigmem_buffers[buffer_index].addr);
> +	*obj = cdev_pager_allocate(vmh, OBJT_DEVICE,
> &contigmem_cdev_pager_ops,
> +			size, nprot, *offset, curthread->td_ucred);
> 
>  	return 0;
>  }
> --
> 2.12.1