Re: [PATCH for-next V1 2/5] IB/core: Add ib_is_udata_cleared

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

On 10/12/2015 19:29, Matan Barak wrote:
> On Thu, Dec 10, 2015 at 5:20 PM, Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
>> On 10/12/2015 16:59, Matan Barak wrote:
>>> On Mon, Dec 7, 2015 at 3:18 PM, Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
>>>> On 12/03/2015 05:44 PM, Matan Barak wrote:
>>>>> Extending core and vendor verb commands require us to check that the
>>>>> unknown part of the user's given command is all zeros.
>>>>> Adding ib_is_udata_cleared in order to do so.
>>>>>
>>>>
>>>> Why not copy the data into kernel space and run memchr_inv() on it?
>>>>
>>>
>>> Probably less efficient, isn't it?
>> Why do you think it is less efficient?
>>
>> I'm not sure calling copy_from_user multiple times is very efficient.
>> For once, you are calling access_ok multiple times. I guess it depends
>> on the amount of data you are copying.
>>
> 
> Isn't access_ok pretty cheap?
> It calls __chk_range_not_ok which on x86 seems like a very cheap
> function and __chk_user_ptr which is a compiler check.
> I guess most kernel-user implementation will be pretty much in sync,
> so we'll possibly call it for a few/dozens of bytes. In that case, I
> think this implementation is a bit faster.
> 
>>> I know it isn't data path, but we'll execute this code in all extended
>>> functions (sometimes even more than once).
>> Do you think it is important enough to maintain our own copy of
>> memchr_inv()?
>>
> 
> True, I'm not sure it's important enough, but do you think it's that
> complicated?

It is complicated in my opinion. It is 67 lines of code, it's
architecture dependent and relies on preprocessor macros and conditional
code. I think this kind of stuff belongs in lib/string.c and not in the
RDMA stack.

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