Re: [PATCH 2/3] tcm_vhost: Optimize gup in vhost_scsi_map_to_sgl

[Asias He <asias@redhat.com>]

On 01/22/2013 02:57 AM, Marcelo Tosatti wrote:
> On Mon, Jan 21, 2013 at 04:05:27PM +0800, Asias He wrote:
>> We can get all the pages in one time instead of calling
>> gup N times.
>>
>> Signed-off-by: Asias He <asias@redhat.com>
>> ---
>>  drivers/vhost/tcm_vhost.c | 33 ++++++++++++++++++++-------------
>>  1 file changed, 20 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
>> index ca35c16..59be442 100644
>> --- a/drivers/vhost/tcm_vhost.c
>> +++ b/drivers/vhost/tcm_vhost.c
>> @@ -430,37 +430,45 @@ static struct tcm_vhost_cmd *vhost_scsi_allocate_cmd(
>>   * Returns the number of scatterlist entries used or -errno on error.
>>   */
>>  static int vhost_scsi_map_to_sgl(struct scatterlist *sgl,
>> -	unsigned int sgl_count, void __user *ptr, size_t len, int write)
>> +	unsigned int sgl_count, struct iovec *iov, int write)
>>  {
>>  	struct scatterlist *sg = sgl;
>>  	unsigned int npages = 0;
>> +	void __user *ptr = iov->iov_base;
>> +	size_t len = iov->iov_len;
>>  	int ret;
>> +	unsigned int pages_nr, offset, nbytes;
>> +	struct page **pages;
>> +
>> +	pages_nr = iov_num_pages(iov);
>> +	pages = kmalloc(pages_nr * sizeof(struct page *), GFP_ATOMIC);
>> +	if (!pages)
>> +		return -ENOMEM;
>> +
>> +	ret = get_user_pages_fast((unsigned long)ptr, pages_nr, write, pages);
>> +	if (ret != pages_nr)
>> +		goto err;
> 
> 1. Why GFP_ATOMIC? get_user_pages_fast can sleep, so this path must not
> be atomic (if it is, should use __get_user_pages_fast).
> 
> GFP_ATOMIC should be avoided.
> 
> 2. Should drop reference to pages whose refcount has been increased,
> if ret > 0 && ret != pages_nr (see last phrase of get_user_pages_fast
> commentary).

Thanks Marcelo. V2 is on the way.

-- 
Asias