From: Jens Axboe <axboe@kernel.dk>
To: Andres Freund <andres@anarazel.de>
Cc: io-uring@vger.kernel.org
Subject: Re: [PATCH 2/6] io_uring: add IORING_OP_PROVIDE_BUFFERS
Date: Mon, 9 Mar 2020 11:17:46 -0600 [thread overview]
Message-ID: <a2283eb6-4b86-b858-a440-af4a8a7c2ba9@kernel.dk> (raw)
In-Reply-To: <20200309170313.perf4zbtdhq4jtvs@alap3.anarazel.de>
On 3/9/20 11:03 AM, Andres Freund wrote:
> Hi,
>
> I like this feature quite a bit...
>
> Sorry for the late response.
>
>
> On 2020-02-28 13:30:49 -0700, Jens Axboe wrote:
>> +static int io_provide_buffers_prep(struct io_kiocb *req,
>> + const struct io_uring_sqe *sqe)
>> +{
>> + struct io_provide_buf *p = &req->pbuf;
>> + u64 tmp;
>> +
>> + if (sqe->ioprio || sqe->rw_flags)
>> + return -EINVAL;
>> +
>> + tmp = READ_ONCE(sqe->fd);
>> + if (!tmp || tmp > USHRT_MAX)
>> + return -EINVAL;
>
> Hm, seems like it'd be less confusing if this didn't use
> io_uring_sqe->fd, but a separate union member?
Sure, not a big deal to me, and we can make this change after the fact
too as it won't change the ABI.
>> + p->nbufs = tmp;
>> + p->addr = READ_ONCE(sqe->addr);
>> + p->len = READ_ONCE(sqe->len);
>> +
>> + if (!access_ok(u64_to_user_ptr(p->addr), p->len))
>> + return -EFAULT;
>> +
>> + p->gid = READ_ONCE(sqe->buf_group);
>> + tmp = READ_ONCE(sqe->off);
>> + if (tmp > USHRT_MAX)
>> + return -EINVAL;
>
> Would it make sense to return a distinct error for the >= USHRT_MAX
> cases? E2BIG or something roughly in that direction? Seems good to be
> able to recognizably refuse "large" buffer group ids.
Don't feel too strongly, but it does help to separate the error.
>> +static int io_add_buffers(struct io_provide_buf *pbuf, struct list_head *list)
>> +{
>> + struct io_buffer *buf;
>> + u64 addr = pbuf->addr;
>> + int i, bid = pbuf->bid;
>> +
>> + for (i = 0; i < pbuf->nbufs; i++) {
>> + buf = kmalloc(sizeof(*buf), GFP_KERNEL);
>> + if (!buf)
>> + break;
>> +
>> + buf->addr = addr;
>> + buf->len = pbuf->len;
>> + buf->bid = bid;
>> + list_add(&buf->list, list);
>> + addr += pbuf->len;
>> + bid++;
>> + }
>> +
>> + return i;
>> +}
>
> Hm, aren't you loosing an error here if you kmalloc fails for i > 0?
> Afaict io_provide_buffes() only checks for ret != 0. I think userland
> should know that a PROVIDE_BUFFERS failed, even if just partially (I'd
> just make it fail wholesale).
The above one does have the issue that we're losing the error for i ==
0, current one does:
return i ? i : -ENOMEM;
But this is what most interfaces end up doing, return the number
processed, if any, or error if none of them were added. Like a short
read, for example, and you'd get EIO if you forwarded and tried again.
So I tend to prefer doing it like that, at least to me it seems more
logical than unwinding. The application won't know what buffer caused
the error if you unwind, whereas it's perfectly clear if you asked to
add 128 and we return 64 that the issue is with the 65th buffer.
>> +static int io_provide_buffers(struct io_kiocb *req, struct io_kiocb **nxt,
>> + bool force_nonblock)
>> +{
>> + struct io_provide_buf *p = &req->pbuf;
>> + struct io_ring_ctx *ctx = req->ctx;
>> + struct list_head *list;
>> + int ret = 0;
>
>> + list = idr_find(&ctx->io_buffer_idr, p->gid);
>
> I'd rename gid to bgid or such to distinguish from other types of group
> ids, but whatever.
Noted!
--
Jens Axboe
next prev parent reply other threads:[~2020-03-09 17:17 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-28 20:30 [PATCHSET v3] io_uring support for automatic buffers Jens Axboe
2020-02-28 20:30 ` [PATCH 1/6] io_uring: buffer registration infrastructure Jens Axboe
2020-02-28 20:30 ` [PATCH 2/6] io_uring: add IORING_OP_PROVIDE_BUFFERS Jens Axboe
2020-02-29 0:43 ` Pavel Begunkov
2020-02-29 4:50 ` Jens Axboe
2020-02-29 11:36 ` Pavel Begunkov
2020-02-29 17:32 ` Jens Axboe
2020-02-29 12:08 ` Pavel Begunkov
2020-02-29 17:34 ` Jens Axboe
2020-02-29 18:11 ` Jens Axboe
2020-03-09 17:03 ` Andres Freund
2020-03-09 17:17 ` Jens Axboe [this message]
2020-03-09 17:28 ` Andres Freund
2020-03-10 13:33 ` Jens Axboe
2020-02-28 20:30 ` [PATCH 3/6] io_uring: support buffer selection Jens Axboe
2020-02-29 12:21 ` Pavel Begunkov
2020-02-29 17:35 ` Jens Axboe
2020-03-09 17:21 ` Andres Freund
2020-03-10 13:37 ` Jens Axboe
2020-02-28 20:30 ` [PATCH 4/6] io_uring: add IOSQE_BUFFER_SELECT support for IORING_OP_READV Jens Axboe
2020-02-28 20:30 ` [PATCH 5/6] net: abstract out normal and compat msghdr import Jens Axboe
2020-02-28 20:30 ` [PATCH 6/6] io_uring: add IOSQE_BUFFER_SELECT support for IORING_OP_RECVMSG Jens Axboe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a2283eb6-4b86-b858-a440-af4a8a7c2ba9@kernel.dk \
--to=axboe@kernel.dk \
--cc=andres@anarazel.de \
--cc=io-uring@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.