| Message ID | 8996ffd533db8bd12c84cdc2ccef1fddbbb3da27.1743783348.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | buffer table registration cleanup | expand |
On 4/4/25 10:22 AM, Pavel Begunkov wrote: > @@ -316,17 +318,26 @@ static int __io_sqe_buffers_update(struct io_ring_ctx *ctx, > err = PTR_ERR(node); > break; > } > - i = array_index_nospec(up->offset + done, ctx->buf_table.nr); > - io_reset_rsrc_node(ctx, &ctx->buf_table, i); > - ctx->buf_table.nodes[i] = node; > + i = array_index_nospec(up->offset + done, buf_table->nr); > + io_reset_rsrc_node(ctx, buf_table, i); > + buf_table->nodes[i] = node; > if (ctx->compat) > user_data += sizeof(struct compat_iovec); > else > user_data += sizeof(struct iovec); > } > + if (last_error) > + *last_error = err; > return done ? done : err; > } > > +static int __io_sqe_buffers_update(struct io_ring_ctx *ctx, > + struct io_uring_rsrc_update2 *up, > + unsigned int nr_args) > +{ > + return io_buffer_table_update(ctx, &ctx->buf_table, up, nr_args, NULL); > +} Minor style preference, but just do: unsigned last_err; return io_buffer_table_update(ctx, &ctx->buf_table, up, nr_args, &last_err); and skip that last_error could be conditionally set? Outside of that, no comments on the series, nice cleanups.
On 4/4/25 17:38, Jens Axboe wrote: > On 4/4/25 10:22 AM, Pavel Begunkov wrote: >> @@ -316,17 +318,26 @@ static int __io_sqe_buffers_update(struct io_ring_ctx *ctx, >> err = PTR_ERR(node); >> break; >> } >> - i = array_index_nospec(up->offset + done, ctx->buf_table.nr); >> - io_reset_rsrc_node(ctx, &ctx->buf_table, i); >> - ctx->buf_table.nodes[i] = node; >> + i = array_index_nospec(up->offset + done, buf_table->nr); >> + io_reset_rsrc_node(ctx, buf_table, i); >> + buf_table->nodes[i] = node; >> if (ctx->compat) >> user_data += sizeof(struct compat_iovec); >> else >> user_data += sizeof(struct iovec); >> } >> + if (last_error) >> + *last_error = err; >> return done ? done : err; >> } >> >> +static int __io_sqe_buffers_update(struct io_ring_ctx *ctx, >> + struct io_uring_rsrc_update2 *up, >> + unsigned int nr_args) >> +{ >> + return io_buffer_table_update(ctx, &ctx->buf_table, up, nr_args, NULL); >> +} > > Minor style preference, but just do: > > unsigned last_err; > return io_buffer_table_update(ctx, &ctx->buf_table, up, nr_args, &last_err); > > and skip that last_error could be conditionally set? I can't say I see how that's better though
On 4/4/25 3:38 PM, Pavel Begunkov wrote: > On 4/4/25 17:38, Jens Axboe wrote: >> On 4/4/25 10:22 AM, Pavel Begunkov wrote: >>> @@ -316,17 +318,26 @@ static int __io_sqe_buffers_update(struct io_ring_ctx *ctx, >>> err = PTR_ERR(node); >>> break; >>> } >>> - i = array_index_nospec(up->offset + done, ctx->buf_table.nr); >>> - io_reset_rsrc_node(ctx, &ctx->buf_table, i); >>> - ctx->buf_table.nodes[i] = node; >>> + i = array_index_nospec(up->offset + done, buf_table->nr); >>> + io_reset_rsrc_node(ctx, buf_table, i); >>> + buf_table->nodes[i] = node; >>> if (ctx->compat) >>> user_data += sizeof(struct compat_iovec); >>> else >>> user_data += sizeof(struct iovec); >>> } >>> + if (last_error) >>> + *last_error = err; >>> return done ? done : err; >>> } >>> +static int __io_sqe_buffers_update(struct io_ring_ctx *ctx, >>> + struct io_uring_rsrc_update2 *up, >>> + unsigned int nr_args) >>> +{ >>> + return io_buffer_table_update(ctx, &ctx->buf_table, up, nr_args, NULL); >>> +} >> >> Minor style preference, but just do: >> >> unsigned last_err; >> return io_buffer_table_update(ctx, &ctx->buf_table, up, nr_args, &last_err); >> >> and skip that last_error could be conditionally set? > > I can't say I see how that's better though I think so though, it makes the store unconditional and there's no risk of anything forgetting that last_error is sometimes valid, sometimes not. The last part is the most annoying to me, it's just more fragile imho.
diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c index 958eee7b4a47..6b5ec1504dcd 100644 --- a/io_uring/rsrc.c +++ b/io_uring/rsrc.c @@ -277,9 +277,11 @@ static int __io_sqe_files_update(struct io_ring_ctx *ctx, return done ? done : err; } -static int __io_sqe_buffers_update(struct io_ring_ctx *ctx, - struct io_uring_rsrc_update2 *up, - unsigned int nr_args) +static int io_buffer_table_update(struct io_ring_ctx *ctx, + struct io_rsrc_data *buf_table, + struct io_uring_rsrc_update2 *up, + unsigned int nr_args, + unsigned *last_error) { u64 __user *tags = u64_to_user_ptr(up->tags); struct iovec fast_iov, *iov; @@ -289,9 +291,9 @@ static int __io_sqe_buffers_update(struct io_ring_ctx *ctx, __u32 done; int i, err; - if (!ctx->buf_table.nr) + if (!buf_table->nr) return -ENXIO; - if (up->offset + nr_args > ctx->buf_table.nr) + if (up->offset + nr_args > buf_table->nr) return -EINVAL; for (done = 0; done < nr_args; done++) { @@ -316,17 +318,26 @@ static int __io_sqe_buffers_update(struct io_ring_ctx *ctx, err = PTR_ERR(node); break; } - i = array_index_nospec(up->offset + done, ctx->buf_table.nr); - io_reset_rsrc_node(ctx, &ctx->buf_table, i); - ctx->buf_table.nodes[i] = node; + i = array_index_nospec(up->offset + done, buf_table->nr); + io_reset_rsrc_node(ctx, buf_table, i); + buf_table->nodes[i] = node; if (ctx->compat) user_data += sizeof(struct compat_iovec); else user_data += sizeof(struct iovec); } + if (last_error) + *last_error = err; return done ? done : err; } +static int __io_sqe_buffers_update(struct io_ring_ctx *ctx, + struct io_uring_rsrc_update2 *up, + unsigned int nr_args) +{ + return io_buffer_table_update(ctx, &ctx->buf_table, up, nr_args, NULL); +} + static int __io_register_rsrc_update(struct io_ring_ctx *ctx, unsigned type, struct io_uring_rsrc_update2 *up, unsigned nr_args) @@ -851,11 +862,8 @@ static struct io_rsrc_node *io_sqe_buffer_register(struct io_ring_ctx *ctx, int io_sqe_buffers_register(struct io_ring_ctx *ctx, void __user *arg, unsigned int nr_args, u64 __user *tags) { - struct page *last_hpage = NULL; struct io_rsrc_data data; - struct iovec fast_iov, *iov = &fast_iov; - const struct iovec __user *uvec; - int i, ret; + int ret, err; BUILD_BUG_ON(IORING_MAX_REG_BUFFERS >= (1u << 16)); @@ -867,51 +875,22 @@ int io_sqe_buffers_register(struct io_ring_ctx *ctx, void __user *arg, if (ret) return ret; - if (!arg) { - ctx->buf_table = data; - return 0; - } - - for (i = 0; i < nr_args; i++) { - struct io_rsrc_node *node; - u64 tag = 0; - - uvec = (struct iovec __user *) arg; - iov = iovec_from_user(uvec, 1, 1, &fast_iov, ctx->compat); - if (IS_ERR(iov)) { - ret = PTR_ERR(iov); - break; - } - ret = io_buffer_validate(iov); - if (ret) - break; - if (ctx->compat) - arg += sizeof(struct compat_iovec); - else - arg += sizeof(struct iovec); - - if (tags) { - if (copy_from_user(&tag, &tags[i], sizeof(tag))) { - ret = -EFAULT; - break; - } - } + if (arg) { + struct io_uring_rsrc_update2 update_arg = { + .tags = (u64)(unsigned long)tags, + .data = (u64)(unsigned long)arg, + .offset = 0, + }; - node = io_sqe_buffer_register(ctx, iov, &last_hpage, tag); - if (IS_ERR(node)) { - ret = PTR_ERR(node); - break; + ret = io_buffer_table_update(ctx, &data, &update_arg, nr_args, &err); + if (ret != nr_args) { + io_clear_table_tags(&data); + io_rsrc_data_free(ctx, &data); + return err; } - data.nodes[i] = node; - } - - if (ret) { - io_clear_table_tags(&data); - io_rsrc_data_free(ctx, &data); - return ret; } ctx->buf_table = data; - return ret; + return 0; } int io_buffer_register_bvec(struct io_uring_cmd *cmd, struct request *rq,
After registering an empty buffer table, we can reuse the code for buffer updates to actually register buffers. Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- io_uring/rsrc.c | 85 +++++++++++++++++++------------------------------ 1 file changed, 32 insertions(+), 53 deletions(-)