Re: memory access op ideas

[Jens Axboe <axboe@kernel.dk>]

On 4/23/22 10:30 AM, Avi Kivity wrote:
> 
> On 22/04/2022 18.03, Jens Axboe wrote:
>> On 4/22/22 8:50 AM, Jens Axboe wrote:
>>> On 4/13/22 4:33 AM, Avi Kivity wrote:
>>>> Unfortunately, only ideas, no patches. But at least the first seems very easy.
>>>>
>>>>
>>>> - IORING_OP_MEMCPY_IMMEDIATE - copy some payload included in the op
>>>> itself (1-8 bytes) to a user memory location specified by the op.
>>>>
>>>>
>>>> Linked to another op, this can generate an in-memory notification
>>>> useful for busy-waiters or the UMWAIT instruction
>>>>
>>>> This would be useful for Seastar, which looks at a timer-managed
>>>> memory location to check when to break computation loops.
>>> This one would indeed be trivial to do. If we limit the max size
>>> supported to eg 8 bytes like suggested, then it could be in the sqe
>>> itself and just copied to the user address specified.
>>>
>>> Eg have sqe->len be the length (1..8 bytes), sqe->addr the destination
>>> address, and sqe->off the data to copy.
>>>
>>> If you'll commit to testing this, I can hack it up pretty quickly...
>> Something like this, totally untested. Maybe the return value should be
>> bytes copied?
> 
> 
> Yes. It could be less than what the user expected, unless we enforce
> alignment (perhaps we should).

Yes, this is just a quick hack. Did make some changes after that,
notably just collapsing it into IORING_OP_MEMCPY and having a flag for
immediate mode or not.

>>   +static int io_memcpy_imm_prep(struct io_kiocb *req,
>> +                  const struct io_uring_sqe *sqe)
>> +{
>> +    struct io_mem *mem = &req->mem;
>> +
>> +    if (unlikely(sqe->ioprio || sqe->rw_flags || sqe->buf_index ||
>> +             sqe->splice_fd_in))
>> +        return -EINVAL;
>> +
>> +    mem->value = READ_ONCE(sqe->off);
>> +    mem->dest = u64_to_user_ptr(READ_ONCE(sqe->addr));
>> +    mem->len = READ_ONCE(sqe->len);
>> +    if (!mem->len || mem->len > sizeof(u64))
>> +        return -EINVAL;
>> +
> 
> 
> I'd also check that the length is a power-of-two to avoid having to
> deal with weird sizes if we later find it inconvenient.

Yes, that's a good idea.

>> +    return 0;
>> +}
>> +
>> +static int io_memcpy_imm(struct io_kiocb *req, unsigned int issue_flags)
>> +{
>> +    struct io_mem *mem = &req->mem;
>> +    int ret = 0;
>> +
>> +    if (copy_to_user(mem->dest, &mem->value, mem->len))
>> +        ret = -EFAULT;
>> +
> 
> 
> Is copy_to_user efficient for tiny sizes? Or is it better to use a
> switch and put_user()?
> 
> 
> I guess copy_to_user saves us from having to consider endianness.

I was considering that too, definitely something that should be
investigated. Making it a 1/2/4/8 switch and using put_user() is
probably a better idea. Easy enough to benchmark.

-- 
Jens Axboe