Re: [PATCH for-next 09/16] IB/hfi1: Clean up pin_vector_pages() function

[Harish Chegondi <harish.chegondi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On 08/22/2017 08:46 AM, Leon Romanovsky wrote:
> On Mon, Aug 21, 2017 at 06:27:03PM -0700, Dennis Dalessandro wrote:
>> From: Harish Chegondi <harish.chegondi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>
>> Clean up pin_vector_pages() function by moving page pinning related code
>> to a separate function since it really stands on its own.
>>
>> Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> Signed-off-by: Harish Chegondi <harish.chegondi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> Signed-off-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/infiniband/hw/hfi1/user_sdma.c |   79 ++++++++++++++++++--------------
>>  1 files changed, 45 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/infiniband/hw/hfi1/user_sdma.c b/drivers/infiniband/hw/hfi1/user_sdma.c
>> index d5a2572..6f26253 100644
>> --- a/drivers/infiniband/hw/hfi1/user_sdma.c
>> +++ b/drivers/infiniband/hw/hfi1/user_sdma.c
>> @@ -1124,11 +1124,53 @@ static u32 sdma_cache_evict(struct hfi1_user_sdma_pkt_q *pq, u32 npages)
>>  	return evict_data.cleared;
>>  }
>>
>> +static int pin_sdma_pages(struct user_sdma_request *req,
>> +			  struct user_sdma_iovec *iovec,
>> +			  struct sdma_mmu_node *node,
>> +			  int npages)
>> +{
>> +	int pinned, cleared;
>> +	struct page **pages;
>> +	struct hfi1_user_sdma_pkt_q *pq = req->pq;
>> +
>> +	pages = kcalloc(npages, sizeof(*pages), GFP_KERNEL);
>> +	if (!pages) {
>> +		SDMA_DBG(req, "Failed page array alloc");
> 
> Please don't add prints after k[c|m|z]alloc failures, despite the fact
> that it was before.
> 
> Thanks

SDMA_DBG MACRO is a trace for development debugging. It's not a print.

> 
>> +		return -ENOMEM;
>> +	}
>> +	memcpy(pages, node->pages, node->npages * sizeof(*pages));
>> +
>> +	npages -= node->npages;
>> +retry:
>> +	if (!hfi1_can_pin_pages(pq->dd, pq->mm,
>> +				atomic_read(&pq->n_locked), npages)) {
>> +		cleared = sdma_cache_evict(pq, npages);
>> +		if (cleared >= npages)
>> +			goto retry;
>> +	}
>> +	pinned = hfi1_acquire_user_pages(pq->mm,
>> +					 ((unsigned long)iovec->iov.iov_base +
>> +					 (node->npages * PAGE_SIZE)), npages, 0,
>> +					 pages + node->npages);
>> +	if (pinned < 0) {
>> +		kfree(pages);
>> +		return pinned;
>> +	}
>> +	if (pinned != npages) {
>> +		unpin_vector_pages(pq->mm, pages, node->npages, pinned);
>> +		return -EFAULT;
>> +	}
>> +	kfree(node->pages);
>> +	node->rb.len = iovec->iov.iov_len;
>> +	node->pages = pages;
>> +	atomic_add(pinned, &pq->n_locked);
>> +	return pinned;
>> +}
>> +
>>  static int pin_vector_pages(struct user_sdma_request *req,
>>  			    struct user_sdma_iovec *iovec)
>>  {
>> -	int ret = 0, pinned, npages, cleared;
>> -	struct page **pages;
>> +	int ret = 0, pinned, npages;
>>  	struct hfi1_user_sdma_pkt_q *pq = req->pq;
>>  	struct sdma_mmu_node *node = NULL;
>>  	struct mmu_rb_node *rb_node;
>> @@ -1162,44 +1204,13 @@ static int pin_vector_pages(struct user_sdma_request *req,
>>
>>  	npages = num_user_pages(&iovec->iov);
>>  	if (node->npages < npages) {
>> -		pages = kcalloc(npages, sizeof(*pages), GFP_KERNEL);
>> -		if (!pages) {
>> -			SDMA_DBG(req, "Failed page array alloc");
>> -			ret = -ENOMEM;
>> -			goto bail;
>> -		}
>> -		memcpy(pages, node->pages, node->npages * sizeof(*pages));
>> -
>> -		npages -= node->npages;
>> -
>> -retry:
>> -		if (!hfi1_can_pin_pages(pq->dd, pq->mm,
>> -					atomic_read(&pq->n_locked), npages)) {
>> -			cleared = sdma_cache_evict(pq, npages);
>> -			if (cleared >= npages)
>> -				goto retry;
>> -		}
>> -		pinned = hfi1_acquire_user_pages(pq->mm,
>> -			((unsigned long)iovec->iov.iov_base +
>> -			 (node->npages * PAGE_SIZE)), npages, 0,
>> -			pages + node->npages);
>> +		pinned = pin_sdma_pages(req, iovec, node, npages);
>>  		if (pinned < 0) {
>> -			kfree(pages);
>>  			ret = pinned;
>>  			goto bail;
>>  		}
>> -		if (pinned != npages) {
>> -			unpin_vector_pages(pq->mm, pages, node->npages,
>> -					   pinned);
>> -			ret = -EFAULT;
>> -			goto bail;
>> -		}
>> -		kfree(node->pages);
>> -		node->rb.len = iovec->iov.iov_len;
>> -		node->pages = pages;
>>  		node->npages += pinned;
>>  		npages = node->npages;
>> -		atomic_add(pinned, &pq->n_locked);
>>  	}
>>  	iovec->pages = node->pages;
>>  	iovec->npages = npages;
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html