[ofa-general] Re: [PATCH][RFC] IB/uverbs: Export ib_umem_get()/ib_umem_release() to modules

[Michael S. Tsirkin]

These are not new, but re-reading this I got a couple of minor questions:

> +	diff = PAGE_ALIGN(umem->length + umem->offset) >> PAGE_SHIFT;
> +	kfree(umem);
> +
>  	/*
>  	 * We may be called with the mm's mmap_sem already held.  This
>  	 * can happen when a userspace munmap() is the call that drops
>  	 * the last reference to our file and calls our release
>  	 * method.  If there are memory regions to destroy, we'll end
> -	 * up here and not be able to take the mmap_sem.  Therefore we
> -	 * defer the vm_locked accounting to the system workqueue.
> +	 * up here and not be able to take the mmap_sem.  In that case
> +	 * we defer the vm_locked accounting to the system workqueue.
>  	 */
> +	if (context->closing && !down_write_trylock(&mm->mmap_sem)) {
> +		work = kmalloc(sizeof *work, GFP_KERNEL);
> +		if (!work) {
> +			mmput(mm);

Error handling looks a bit bogus here - we'll never give the task
it's rlimit back. Wouldn't it be a bit cleaner to allocate
the work object together with umem?

> +			return;
> +		}
>  
> -	work = kmalloc(sizeof *work, GFP_KERNEL);
> -	if (!work) {
> -		mmput(mm);
> +		INIT_WORK(&work->work, ib_umem_account);
> +		work->mm   = mm;
> +		work->diff = diff;
> +
> +		schedule_work(&work->work);

We never flush the work queue.
I wonder whether there's a potential issue with module unloading.


>  		return;
> -	}
> +	} else
> +		down_write(&mm->mmap_sem);
> +
> +	current->mm->locked_vm -= diff;
> +	up_write(&mm->mmap_sem);
> +	mmput(mm);
> +}


-- 
MST