Re: [PATCH v2 04/17] IB/core: Add umem function to read data from user-space

[Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>]

On 11/12/2014 14:39, Yann Droneaud wrote:
> Le jeudi 11 décembre 2014 à 13:09 +0200, Haggai Eran a écrit :
>> On 10/12/2014 18:22, Yann Droneaud wrote:
>>> Hi,
>>>
>>> Le mardi 11 novembre 2014 à 18:36 +0200, Haggai Eran a écrit :
>>>> +{
>>>> +	size_t end = offset + length;
>>>> +
>>>> +	if (offset > umem->length || end > umem->length || end < offset) {
>>>> +		pr_err("ib_umem_copy_from not in range. offset: %zd umem length: %zd end: %zd\n",
>>>> +		       offset, umem->length, end);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
> 
> I think the test could be rewritten as:
> 
> 	if (offset > umem->length || length > umem->length - offset)
> 
> That's one operation less.
> 

Okay.

> 
>>>> +	return sg_pcopy_to_buffer(umem->sg_head.sgl, umem->nmap, dst, length,
>>>> +			offset + ib_umem_offset(umem));
>>>> +}
>>>> +EXPORT_SYMBOL(ib_umem_copy_from);
>>>
>>> As the function return a "int", no more than INT_MAX bytes (likely 2^31
>>> - 1) can be copied. Perhaps changing the return type to to ssize_t would
>>> be better (and a check to enfore ssize_t maximum value). Or change the
>>> function could return 0 in case of success or a error code, just like
>>> ib_copy_from_udata().
>>>
>>
>> Okay. I'll change it to match ib_copy_from_udata. We're checking the
>> umem size in the call site of this function anyway, and the only reason
>> I see sg_pcopy_to_buffer would return less than *length* bytes is when
>> reaching the end of the scatterlist.
>>
> 
> As the length is compared against umem->length (+ offset), this would 
> mean umem->length is not "synchronized" with the length of the data 
> described by the scatter/gather list ?

Yes, I don't think this can happen, so we can just return 0 in case of
success like you suggested.

> 
> BTW, ib_copy_from_udata() is defined as an inline function. Would it be
> better to have ib_umem_copy_from() being an inline function too ?
> (In such case, I would remove the error message to not duplicate it
>  across all modules using the function)

I don't see a great benefit from inlining here. The time to perform this
function is mostly due to the traversal of the scatterlist, and I don't
think an additional function call would make much of a difference.

Regards,
Haggai
--
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