On Sun, Dec 13, 2015 at 5:47 PM, Haggai Eran <haggaie@xxxxxxxxxxxx> wrote:
> On 10/12/2015 19:29, Matan Barak wrote:
>> On Thu, Dec 10, 2015 at 5:20 PM, Haggai Eran <haggaie@xxxxxxxxxxxx> wrote:
>>> On 10/12/2015 16:59, Matan Barak wrote:
>>>> On Mon, Dec 7, 2015 at 3:18 PM, Haggai Eran <haggaie@xxxxxxxxxxxx> 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.
>
I'm not sure regarding the string.c location, as it deals with user
buffers, but in order not to
be dependent on this, I'll change this code to the following.
static inline bool ib_is_udata_cleared(struct ib_udata *udata,
u8 cleared_char,
size_t offset,
size_t len)
{
const void __user *p = udata->inbuf + offset;
bool ret = false;
u8 *buf;
if (len > USHRT_MAX)
return false;
buf = kmalloc(len, GFP_KERNEL);
if (!buf)
return false;
if (copy_from_user(buf, p, len))
goto free;
ret = !memchr_inv(buf, cleared_char, len);
free:
kfree(buf);
return ret;
}
> Haggai
Regards,
Matan
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
